| Message ID | 20220225145432.422130-1-pgwipeout@gmail.com |
|---|---|
| Headers | show |
| Series | enable usb support on rk356x | expand |
Hi Peter, (It should be noted that there was a slight mishap in communications between the two of us resulting in two series with the same goal. Now let's clean up the mess :-) Thanks for your series. Seeing that it contains more patches than mine it probably makes sense to use your series as basis. Please Cc: me in future iterations of this patch series and consider my comments below. On 2/25/22 15:54, Peter Geis wrote: > Add the dwc3 device nodes to the rk356x device trees. > The rk3566 has one usb2 capable dwc3 otg controller and one usb3 capable > dwc3 host controller. > The rk3568 has one usb3 capable dwc3 otg controller and one usb3 capable > dwc3 host controller. > > Signed-off-by: Peter Geis <pgwipeout@gmail.com> > --- > arch/arm64/boot/dts/rockchip/rk3566.dtsi | 12 +++++++ > arch/arm64/boot/dts/rockchip/rk3568.dtsi | 9 +++++ > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 45 +++++++++++++++++++++++- > 3 files changed, 65 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3566.dtsi b/arch/arm64/boot/dts/rockchip/rk3566.dtsi > index 3839eef5e4f7..8e8b52f58f44 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3566.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3566.dtsi > @@ -6,6 +6,10 @@ / { > compatible = "rockchip,rk3566"; > }; > > +&pipegrf { > + compatible = "rockchip,rk3566-pipe-grf", "syscon"; > +}; > + > &power { > power-domain@RK3568_PD_PIPE { > reg = <RK3568_PD_PIPE>; > @@ -18,3 +22,11 @@ power-domain@RK3568_PD_PIPE { > #power-domain-cells = <0>; > }; > }; > + > +&usbdrd30 { I would really love to have some alignment with the other USB controllers usb_host{0,1}_{e,o}hci here. I am aware that older SoCs and the SDK are using these names and it might be painful to have different versions to maintain at the moment, but can we please agree on usb_host0_xhci usb_host1_xhci or something like that? > + phys = <&usb2phy0_otg>; > + phy-names = "usb2-phy"; > + extcon = <&usb2phy0>; > + maximum-speed = "high-speed"; > + snps,dis_u2_susphy_quirk; > +}; > diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi > index 5b0f528d6818..77c044cbaaad 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi > @@ -99,6 +99,10 @@ opp-1992000000 { > }; > }; > > +&pipegrf { > + compatible = "rockchip,rk3568-pipe-grf", "syscon"; > +}; > + > &power { > power-domain@RK3568_PD_PIPE { > reg = <RK3568_PD_PIPE>; > @@ -114,3 +118,8 @@ power-domain@RK3568_PD_PIPE { > #power-domain-cells = <0>; > }; > }; > + > +&usbdrd30 { > + phys = <&usb2phy0_otg>, <&combphy0 PHY_TYPE_USB3>; > + phy-names = "usb2-phy", "usb3-phy"; > +}; > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > index 84d5d607e693..4fae5b3b326e 100644 > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > @@ -230,6 +230,50 @@ scmi_shmem: sram@0 { > }; > }; > > + usbdrd30: usbdrd@fcc00000 { > + compatible = "rockchip,rk3568-dwc3", "snps,dwc3"; > + reg = <0x0 0xfcc00000 0x0 0x400000>; > + interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cru CLK_USB3OTG0_REF>, <&cru CLK_USB3OTG0_SUSPEND>, > + <&cru ACLK_USB3OTG0>, <&cru PCLK_PIPE>; > + clock-names = "ref_clk", "suspend_clk", > + "bus_clk", "grf_clk"; Please consider Johan's comments on my first series. In my tests removing the PCLK_PIPE clock did not make any difference. > + dr_mode = "host"; Based on the description in the commit message above it should be "otg", right? Boards are free to overrule this, of course. > + phy_type = "utmi_wide"; > + power-domains = <&power RK3568_PD_PIPE>; > + resets = <&cru SRST_USB3OTG0>; > + reset-names = "usb3-otg"; > + snps,dis_enblslpm_quirk; > + snps,dis-u2-freeclk-exists-quirk; > + snps,dis-del-phy-power-chg-quirk; > + snps,dis-tx-ipgap-linecheck-quirk; > + snps,xhci-trb-ent-quirk; In my first version I had all those quirks as well, but are they actually necessary? I decided to remove them all to have a fresh start (also activating them did not seem to affect my test setup). > + status = "disabled"; > + }; > + > + usbhost30: usbhost@fd000000 { Please reconsider the this name as well. > + compatible = "rockchip,rk3568-dwc3", "snps,dwc3"; > + reg = <0x0 0xfd000000 0x0 0x400000>; > + interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>, > + <&cru ACLK_USB3OTG1>, <&cru PCLK_PIPE>; > + clock-names = "ref_clk", "suspend_clk", > + "bus_clk", "grf_clk"; > + dr_mode = "host"; Here "host" clearly makes sense, as this controller is not capable of otg. > + phys = <&usb2phy0_host>, <&combphy1 PHY_TYPE_USB3>; > + phy-names = "usb2-phy", "usb3-phy"; > + phy_type = "utmi_wide"; > + power-domains = <&power RK3568_PD_PIPE>; > + resets = <&cru SRST_USB3OTG1>; > + reset-names = "usb3-host"; > + snps,dis_enblslpm_quirk; > + snps,dis-u2-freeclk-exists-quirk; > + snps,dis_u2_susphy_quirk; > + snps,dis-del-phy-power-chg-quirk; > + snps,dis-tx-ipgap-linecheck-quirk; What was said about quirks above holds here as well (although one quirk not documented in the bindings is missing here). Best regards, Michael > + status = "disabled"; > + }; > + > gic: interrupt-controller@fd400000 { > compatible = "arm,gic-v3"; > reg = <0x0 0xfd400000 0 0x10000>, /* GICD */ > @@ -297,7 +341,6 @@ pmu_io_domains: io-domains { > }; > > pipegrf: syscon@fdc50000 { > - compatible = "rockchip,rk3568-pipe-grf", "syscon"; > reg = <0x0 0xfdc50000 0x0 0x1000>; > }; >
On Fri, Feb 25, 2022 at 12:01 PM Michael Riesch <michael.riesch@wolfvision.net> wrote: > > Hi Peter, > > (It should be noted that there was a slight mishap in communications > between the two of us resulting in two series with the same goal. Now > let's clean up the mess :-) > > Thanks for your series. Seeing that it contains more patches than mine > it probably makes sense to use your series as basis. Please Cc: me in > future iterations of this patch series and consider my comments below. Will do. If you'd like I can also pull your enablement patch. > > On 2/25/22 15:54, Peter Geis wrote: > > Add the dwc3 device nodes to the rk356x device trees. > > The rk3566 has one usb2 capable dwc3 otg controller and one usb3 capable > > dwc3 host controller. > > The rk3568 has one usb3 capable dwc3 otg controller and one usb3 capable > > dwc3 host controller. > > > > Signed-off-by: Peter Geis <pgwipeout@gmail.com> > > --- > > arch/arm64/boot/dts/rockchip/rk3566.dtsi | 12 +++++++ > > arch/arm64/boot/dts/rockchip/rk3568.dtsi | 9 +++++ > > arch/arm64/boot/dts/rockchip/rk356x.dtsi | 45 +++++++++++++++++++++++- > > 3 files changed, 65 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3566.dtsi b/arch/arm64/boot/dts/rockchip/rk3566.dtsi > > index 3839eef5e4f7..8e8b52f58f44 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3566.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk3566.dtsi > > @@ -6,6 +6,10 @@ / { > > compatible = "rockchip,rk3566"; > > }; > > > > +&pipegrf { > > + compatible = "rockchip,rk3566-pipe-grf", "syscon"; > > +}; > > + > > &power { > > power-domain@RK3568_PD_PIPE { > > reg = <RK3568_PD_PIPE>; > > @@ -18,3 +22,11 @@ power-domain@RK3568_PD_PIPE { > > #power-domain-cells = <0>; > > }; > > }; > > + > > +&usbdrd30 { > > I would really love to have some alignment with the other USB controllers > > usb_host{0,1}_{e,o}hci > > here. I am aware that older SoCs and the SDK are using these names and > it might be painful to have different versions to maintain at the > moment, but can we please agree on > > usb_host0_xhci > usb_host1_xhci > > or something like that? I agree, I like your naming better. > > > + phys = <&usb2phy0_otg>; > > + phy-names = "usb2-phy"; > > + extcon = <&usb2phy0>; > > + maximum-speed = "high-speed"; > > + snps,dis_u2_susphy_quirk; > > +}; > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi > > index 5b0f528d6818..77c044cbaaad 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi > > @@ -99,6 +99,10 @@ opp-1992000000 { > > }; > > }; > > > > +&pipegrf { > > + compatible = "rockchip,rk3568-pipe-grf", "syscon"; > > +}; > > + > > &power { > > power-domain@RK3568_PD_PIPE { > > reg = <RK3568_PD_PIPE>; > > @@ -114,3 +118,8 @@ power-domain@RK3568_PD_PIPE { > > #power-domain-cells = <0>; > > }; > > }; > > + > > +&usbdrd30 { > > + phys = <&usb2phy0_otg>, <&combphy0 PHY_TYPE_USB3>; > > + phy-names = "usb2-phy", "usb3-phy"; > > +}; > > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > index 84d5d607e693..4fae5b3b326e 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi > > @@ -230,6 +230,50 @@ scmi_shmem: sram@0 { > > }; > > }; > > > > + usbdrd30: usbdrd@fcc00000 { > > + compatible = "rockchip,rk3568-dwc3", "snps,dwc3"; > > + reg = <0x0 0xfcc00000 0x0 0x400000>; > > + interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&cru CLK_USB3OTG0_REF>, <&cru CLK_USB3OTG0_SUSPEND>, > > + <&cru ACLK_USB3OTG0>, <&cru PCLK_PIPE>; > > + clock-names = "ref_clk", "suspend_clk", > > + "bus_clk", "grf_clk"; > > Please consider Johan's comments on my first series. In my tests > removing the PCLK_PIPE clock did not make any difference. I'd like to test that this isn't being used, or isn't just working because it's enabled elsewhere. If both of those are false, then I'll be happy to drop this. > > > + dr_mode = "host"; > > Based on the description in the commit message above it should be "otg", > right? Boards are free to overrule this, of course. Currently the usb2phy does not support OTG mode correctly. There are patches in the works for this, but at the moment it's safer to default to host. > > > + phy_type = "utmi_wide"; > > + power-domains = <&power RK3568_PD_PIPE>; > > + resets = <&cru SRST_USB3OTG0>; > > + reset-names = "usb3-otg"; > > + snps,dis_enblslpm_quirk; > > + snps,dis-u2-freeclk-exists-quirk; > > + snps,dis-del-phy-power-chg-quirk; > > + snps,dis-tx-ipgap-linecheck-quirk; > > + snps,xhci-trb-ent-quirk; > > In my first version I had all those quirks as well, but are they > actually necessary? I decided to remove them all to have a fresh start > (also activating them did not seem to affect my test setup). I'm now curious about this, can someone weigh in on valid ways of testing each one of these in a way that is definite? > > > + status = "disabled"; > > + }; > > + > > + usbhost30: usbhost@fd000000 { > > Please reconsider the this name as well. > > > + compatible = "rockchip,rk3568-dwc3", "snps,dwc3"; > > + reg = <0x0 0xfd000000 0x0 0x400000>; > > + interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>, > > + <&cru ACLK_USB3OTG1>, <&cru PCLK_PIPE>; > > + clock-names = "ref_clk", "suspend_clk", > > + "bus_clk", "grf_clk"; > > + dr_mode = "host"; > > Here "host" clearly makes sense, as this controller is not capable of otg. > > > + phys = <&usb2phy0_host>, <&combphy1 PHY_TYPE_USB3>; > > + phy-names = "usb2-phy", "usb3-phy"; > > + phy_type = "utmi_wide"; > > + power-domains = <&power RK3568_PD_PIPE>; > > + resets = <&cru SRST_USB3OTG1>; > > + reset-names = "usb3-host"; > > + snps,dis_enblslpm_quirk; > > + snps,dis-u2-freeclk-exists-quirk; > > + snps,dis_u2_susphy_quirk; > > + snps,dis-del-phy-power-chg-quirk; > > + snps,dis-tx-ipgap-linecheck-quirk; > > What was said about quirks above holds here as well (although one quirk > not documented in the bindings is missing here). Same thing here, I'd like absolute testing to determine that these are not necessary, since downstream (the oem) felt they were. > > Best regards, > Michael > > > + status = "disabled"; > > + }; > > + > > gic: interrupt-controller@fd400000 { > > compatible = "arm,gic-v3"; > > reg = <0x0 0xfd400000 0 0x10000>, /* GICD */ > > @@ -297,7 +341,6 @@ pmu_io_domains: io-domains { > > }; > > > > pipegrf: syscon@fdc50000 { > > - compatible = "rockchip,rk3568-pipe-grf", "syscon"; > > reg = <0x0 0xfdc50000 0x0 0x1000>; > > }; > >
Hi Peter, On 2/25/22 18:59, Peter Geis wrote: > On Fri, Feb 25, 2022 at 12:01 PM Michael Riesch > <michael.riesch@wolfvision.net> wrote: >> >> Hi Peter, >> >> (It should be noted that there was a slight mishap in communications >> between the two of us resulting in two series with the same goal. Now >> let's clean up the mess :-) >> >> Thanks for your series. Seeing that it contains more patches than mine >> it probably makes sense to use your series as basis. Please Cc: me in >> future iterations of this patch series and consider my comments below. > > Will do. > If you'd like I can also pull your enablement patch. That'd be great, thanks! I'll help testing of course :-) >> On 2/25/22 15:54, Peter Geis wrote: > [...] >>> + dr_mode = "host"; >> >> Based on the description in the commit message above it should be "otg", >> right? Boards are free to overrule this, of course. > > Currently the usb2phy does not support OTG mode correctly. > There are patches in the works for this, but at the moment it's safer > to default to host. Ah OK, makes sense! Best regards, Michael > >> >>> + phy_type = "utmi_wide"; >>> + power-domains = <&power RK3568_PD_PIPE>; >>> + resets = <&cru SRST_USB3OTG0>; >>> + reset-names = "usb3-otg"; >>> + snps,dis_enblslpm_quirk; >>> + snps,dis-u2-freeclk-exists-quirk; >>> + snps,dis-del-phy-power-chg-quirk; >>> + snps,dis-tx-ipgap-linecheck-quirk; >>> + snps,xhci-trb-ent-quirk; >> >> In my first version I had all those quirks as well, but are they >> actually necessary? I decided to remove them all to have a fresh start >> (also activating them did not seem to affect my test setup). > > I'm now curious about this, can someone weigh in on valid ways of > testing each one of these in a way that is definite? > >> >>> + status = "disabled"; >>> + }; >>> + >>> + usbhost30: usbhost@fd000000 { >> >> Please reconsider the this name as well. >> >>> + compatible = "rockchip,rk3568-dwc3", "snps,dwc3"; >>> + reg = <0x0 0xfd000000 0x0 0x400000>; >>> + interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>; >>> + clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>, >>> + <&cru ACLK_USB3OTG1>, <&cru PCLK_PIPE>; >>> + clock-names = "ref_clk", "suspend_clk", >>> + "bus_clk", "grf_clk"; >>> + dr_mode = "host"; >> >> Here "host" clearly makes sense, as this controller is not capable of otg. >> >>> + phys = <&usb2phy0_host>, <&combphy1 PHY_TYPE_USB3>; >>> + phy-names = "usb2-phy", "usb3-phy"; >>> + phy_type = "utmi_wide"; >>> + power-domains = <&power RK3568_PD_PIPE>; >>> + resets = <&cru SRST_USB3OTG1>; >>> + reset-names = "usb3-host"; >>> + snps,dis_enblslpm_quirk; >>> + snps,dis-u2-freeclk-exists-quirk; >>> + snps,dis_u2_susphy_quirk; >>> + snps,dis-del-phy-power-chg-quirk; >>> + snps,dis-tx-ipgap-linecheck-quirk; >> >> What was said about quirks above holds here as well (although one quirk >> not documented in the bindings is missing here). > > Same thing here, I'd like absolute testing to determine that these are > not necessary, since downstream (the oem) felt they were. > >> >> Best regards, >> Michael >> >>> + status = "disabled"; >>> + }; >>> + >>> gic: interrupt-controller@fd400000 { >>> compatible = "arm,gic-v3"; >>> reg = <0x0 0xfd400000 0 0x10000>, /* GICD */ >>> @@ -297,7 +341,6 @@ pmu_io_domains: io-domains { >>> }; >>> >>> pipegrf: syscon@fdc50000 { >>> - compatible = "rockchip,rk3568-pipe-grf", "syscon"; >>> reg = <0x0 0xfdc50000 0x0 0x1000>; >>> }; >>>