| Message ID | 20190314182035.7101-1-papadakospan@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: rockchip: add usb3 controller node for RK3328 SoCs | expand |
Hi, Am Donnerstag, 14. März 2019, 19:20:35 CET schrieb Leonidas P. Papadakos: > From: William Wu <william.wu@rock-chips.com> > > RK3328 has one USB 3.0 OTG controller which uses DWC_USB3 > core's general architecture. It can act as static xHCI host > controller, static device controller, USB 3.0/2.0 OTG basing > on ID of USB3.0 PHY. > > Signed-off-by: William Wu <william.wu@rock-chips.com> > Signed-off-by: Leonidas P. Papadakos <papadakospan@gmail.com> the rk3328 usb3-phy has an issue with detecting any plugin events after a previous device got removed - see the inno-usb3-phy driver in the vendor kernel. So once you unplug an usb device from the port it won't detect a newly connected device. This is also the reason why the original usb3 patches are still sitting around in my tree, as I don't want to codify a binding that may not last once somebody actually manages to go around that issue with kernel code - especially as the node below claims to be compatible with the rk3399 dwc3. Heiko > --- > arch/arm64/boot/dts/rockchip/rk3328.dtsi | 27 ++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi > b/arch/arm64/boot/dts/rockchip/rk3328.dtsi index 84f14b132..f5c1ab596 > 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi > @@ -917,6 +917,33 @@ > status = "disabled"; > }; > > + usbdrd3: usb@ff600000 { > + compatible = "rockchip,rk3328-dwc3", "rockchip,rk3399-dwc3"; > + clocks = <&cru SCLK_USB3OTG_REF>, <&cru SCLK_USB3OTG_SUSPEND>, > + <&cru ACLK_USB3OTG>; > + clock-names = "ref_clk", "suspend_clk", > + "bus_clk"; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + status = "disabled"; > + > + usbdrd_dwc3: dwc3@ff600000 { > + compatible = "snps,dwc3"; > + reg = <0x0 0xff600000 0x0 0x100000>; > + interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; > + dr_mode = "otg"; > + phy_type = "utmi_wide"; > + snps,dis_enblslpm_quirk; > + snps,dis-u2-freeclk-exists-quirk; > + snps,dis_u2_susphy_quirk; > + snps,dis_u3_susphy_quirk; > + snps,dis-del-phy-power-chg-quirk; > + snps,dis-tx-ipgap-linecheck-quirk; > + status = "disabled"; > + }; > + }; > + > gic: interrupt-controller@ff811000 { > compatible = "arm,gic-400"; > #interrupt-cells = <3>;
Στις Πεμ, 14 Μαρ, 2019 at 9:02 ΜΜ, ο/η Heiko =?iso-8859-1?q?St=FCbner?= <heiko@sntech.de> έγραψε: > Hi, > > Am Donnerstag, 14. März 2019, 19:20:35 CET schrieb Leonidas P. > Papadakos: >> From: William Wu <william.wu@rock-chips.com> >> >> RK3328 has one USB 3.0 OTG controller which uses DWC_USB3 >> core's general architecture. It can act as static xHCI host >> controller, static device controller, USB 3.0/2.0 OTG basing >> on ID of USB3.0 PHY. >> >> Signed-off-by: William Wu <william.wu@rock-chips.com> >> Signed-off-by: Leonidas P. Papadakos <papadakospan@gmail.com> > > the rk3328 usb3-phy has an issue with detecting any plugin events > after a previous device got removed - see the inno-usb3-phy driver > in the vendor kernel. > > So once you unplug an usb device from the port it won't detect > a newly connected device. This is also the reason why the original > usb3 patches are still sitting around in my tree, as I don't want to > codify a binding that may not last once somebody actually manages > to go around that issue with kernel code - especially as the node > below claims to be compatible with the rk3399 dwc3. > > > Heiko > >> --- >> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 27 >> ++++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi index >> 84f14b132..f5c1ab596 >> 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >> @@ -917,6 +917,33 @@ >> status = "disabled"; >> }; >> >> + usbdrd3: usb@ff600000 { >> + compatible = "rockchip,rk3328-dwc3", "rockchip,rk3399-dwc3"; >> + clocks = <&cru SCLK_USB3OTG_REF>, <&cru SCLK_USB3OTG_SUSPEND>, >> + <&cru ACLK_USB3OTG>; >> + clock-names = "ref_clk", "suspend_clk", >> + "bus_clk"; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; >> + status = "disabled"; >> + >> + usbdrd_dwc3: dwc3@ff600000 { >> + compatible = "snps,dwc3"; >> + reg = <0x0 0xff600000 0x0 0x100000>; >> + interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; >> + dr_mode = "otg"; >> + phy_type = "utmi_wide"; >> + snps,dis_enblslpm_quirk; >> + snps,dis-u2-freeclk-exists-quirk; >> + snps,dis_u2_susphy_quirk; >> + snps,dis_u3_susphy_quirk; >> + snps,dis-del-phy-power-chg-quirk; >> + snps,dis-tx-ipgap-linecheck-quirk; >> + status = "disabled"; >> + }; >> + }; >> + >> gic: interrupt-controller@ff811000 { >> compatible = "arm,gic-400"; >> #interrupt-cells = <3>; > > > > Now it makes sense. I thought it was board specific. It's a general rk3328 issue, then. Thanks for clearing it up. (yet again, oops)
I must say with my testing, I never really have any disconnects. My drive generally works Surely it's better that it works somewhat than not at all. Maybe we could include the patches and revert them later if a solution arrises? Or is that against the Linux way of doing things?
Hi again :-) Am Donnerstag, 14. März 2019, 20:08:30 CET schrieb Leonidas P. Papadakos: > I must say with my testing, I never really have any disconnects. My > drive generally works It's not any disconnects, it is really when you unplug the device from the root port that the > Surely it's better that it works somewhat than not at all. > Maybe we could include the patches and revert them later if a solution > arrises? > > Or is that against the Linux way of doing things? The main issue is compatible = "rockchip,rk3328-dwc3", "rockchip,rk3399-dwc3"; because: (1) On the devicetree-side you declare that they are compatible which may or may not conflict with needed later changes (2) On the kernel-driver-side with the current status the rk3328-dwc3 will get ignored and rk3399-dwc3 used to bind to the generic dt-dwc3 driver. From looking at the Rockchip code in the vendor kernel we may very well need a separate driver to handle the big issue which creates the issue which driver will bind to the dt-node. It will be either the generic one binding to the rk3399-dwc3 or the special one binding to rk3328-dwc3. And sadly it will probably depend on which module gets loaded first. And devicetrees are supposed to be backwards compatible so a newer kernel should keep working with an old devicetree. A possible way forward would be to introduce a separate compatible for rk3328 (both in the binding doc and the generic dwc3 driver), so that we get only a compatible = "rockchip,rk3328-dwc3"; Which then even can move easily into a new driver if necessary without causing issues for existing devicetrees. Heiko
Has anyone tested this since Jonas Karlman's clk patch landed? I noticed a lot of my USB hotplug issues disappeared when I pulled that patch in. Especially since one of the fixes was aclk_usb3otg, since it was incorrectly defined to usb2's aclk. On Thu, Mar 14, 2019 at 3:21 PM Heiko Stübner <heiko@sntech.de> wrote: > > Hi again :-) > > Am Donnerstag, 14. März 2019, 20:08:30 CET schrieb Leonidas P. Papadakos: > > I must say with my testing, I never really have any disconnects. My > > drive generally works > > It's not any disconnects, it is really when you unplug the device from > the root port that the > > > Surely it's better that it works somewhat than not at all. > > Maybe we could include the patches and revert them later if a solution > > arrises? > > > > Or is that against the Linux way of doing things? > > The main issue is > compatible = "rockchip,rk3328-dwc3", "rockchip,rk3399-dwc3"; > > because: > (1) On the devicetree-side you declare that they are compatible which may > or may not conflict with needed later changes > (2) On the kernel-driver-side with the current status the rk3328-dwc3 > will get ignored and rk3399-dwc3 used to bind to the generic dt-dwc3 > driver. > From looking at the Rockchip code in the vendor kernel we may very well > need a separate driver to handle the big issue which creates the issue > which driver will bind to the dt-node. It will be either the generic one > binding to the rk3399-dwc3 or the special one binding to rk3328-dwc3. > And sadly it will probably depend on which module gets loaded first. > > And devicetrees are supposed to be backwards compatible so a newer > kernel should keep working with an old devicetree. > > A possible way forward would be to introduce a separate compatible for > rk3328 (both in the binding doc and the generic dwc3 driver), so that > we get only a > compatible = "rockchip,rk3328-dwc3"; > > Which then even can move easily into a new driver if necessary without > causing issues for existing devicetrees. > > Heiko > > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
I have been using that clk patch since I started initial testing of rk3328 on mainline 4.16 [1] and I cannot remember seeing any real issue with usb2/usb3 (beside usb regulator related issue). The clk patch may have helped with some usb issues that I never experienced :-) [1] https://github.com/Kwiboo/linux-rockchip/commits/rockchip-4.16/drivers/clk/rockchip Regards, Jonas On 2019-03-14 20:24, Peter Geis wrote: > Has anyone tested this since Jonas Karlman's clk patch landed? > I noticed a lot of my USB hotplug issues disappeared when I pulled > that patch in. > Especially since one of the fixes was aclk_usb3otg, since it was > incorrectly defined to usb2's aclk. > > On Thu, Mar 14, 2019 at 3:21 PM Heiko Stübner <heiko@sntech.de> wrote: >> Hi again :-) >> >> Am Donnerstag, 14. März 2019, 20:08:30 CET schrieb Leonidas P. Papadakos: >>> I must say with my testing, I never really have any disconnects. My >>> drive generally works >> It's not any disconnects, it is really when you unplug the device from >> the root port that the >> >>> Surely it's better that it works somewhat than not at all. >>> Maybe we could include the patches and revert them later if a solution >>> arrises? >>> >>> Or is that against the Linux way of doing things? >> The main issue is >> compatible = "rockchip,rk3328-dwc3", "rockchip,rk3399-dwc3"; >> >> because: >> (1) On the devicetree-side you declare that they are compatible which may >> or may not conflict with needed later changes >> (2) On the kernel-driver-side with the current status the rk3328-dwc3 >> will get ignored and rk3399-dwc3 used to bind to the generic dt-dwc3 >> driver. >> From looking at the Rockchip code in the vendor kernel we may very well >> need a separate driver to handle the big issue which creates the issue >> which driver will bind to the dt-node. It will be either the generic one >> binding to the rk3399-dwc3 or the special one binding to rk3328-dwc3. >> And sadly it will probably depend on which module gets loaded first. >> >> And devicetrees are supposed to be backwards compatible so a newer >> kernel should keep working with an old devicetree. >> >> A possible way forward would be to introduce a separate compatible for >> rk3328 (both in the binding doc and the generic dwc3 driver), so that >> we get only a >> compatible = "rockchip,rk3328-dwc3"; >> >> Which then even can move easily into a new driver if necessary without >> causing issues for existing devicetrees. >> >> Heiko >> >> >> >> _______________________________________________ >> Linux-rockchip mailing list >> Linux-rockchip@lists.infradead.org >> https://nam01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-rockchip&data=02%7C01%7C%7C9a4c35bda0bf40b48ce508d6a8b2bb01%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636881882922347560&sdata=GJpD0AQPTcCT0BLjb%2BKKdjSMlPKvniqOxigx5qnOEmA%3D&reserved=0 > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > https://nam01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-rockchip&data=02%7C01%7C%7C9a4c35bda0bf40b48ce508d6a8b2bb01%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636881882922347560&sdata=GJpD0AQPTcCT0BLjb%2BKKdjSMlPKvniqOxigx5qnOEmA%3D&reserved=0
The usb3 issue Heiko describe is still there, my usb3 device is only discovered once on my Rock 64. I must have only tested inserting my usb3 devices once when I testing before. Regards, Jonas On 2019-03-14 20:52, Jonas Karlman wrote: > I have been using that clk patch since I started initial testing of rk3328 on mainline 4.16 [1] > and I cannot remember seeing any real issue with usb2/usb3 (beside usb regulator related issue). > The clk patch may have helped with some usb issues that I never experienced :-) > > [1] https://github.com/Kwiboo/linux-rockchip/commits/rockchip-4.16/drivers/clk/rockchip > > Regards, > Jonas > > On 2019-03-14 20:24, Peter Geis wrote: >> Has anyone tested this since Jonas Karlman's clk patch landed? >> I noticed a lot of my USB hotplug issues disappeared when I pulled >> that patch in. >> Especially since one of the fixes was aclk_usb3otg, since it was >> incorrectly defined to usb2's aclk. >> >> On Thu, Mar 14, 2019 at 3:21 PM Heiko Stübner <heiko@sntech.de> wrote: >>> Hi again :-) >>> >>> Am Donnerstag, 14. März 2019, 20:08:30 CET schrieb Leonidas P. Papadakos: >>>> I must say with my testing, I never really have any disconnects. My >>>> drive generally works >>> It's not any disconnects, it is really when you unplug the device from >>> the root port that the >>> >>>> Surely it's better that it works somewhat than not at all. >>>> Maybe we could include the patches and revert them later if a solution >>>> arrises? >>>> >>>> Or is that against the Linux way of doing things? >>> The main issue is >>> compatible = "rockchip,rk3328-dwc3", "rockchip,rk3399-dwc3"; >>> >>> because: >>> (1) On the devicetree-side you declare that they are compatible which may >>> or may not conflict with needed later changes >>> (2) On the kernel-driver-side with the current status the rk3328-dwc3 >>> will get ignored and rk3399-dwc3 used to bind to the generic dt-dwc3 >>> driver. >>> From looking at the Rockchip code in the vendor kernel we may very well >>> need a separate driver to handle the big issue which creates the issue >>> which driver will bind to the dt-node. It will be either the generic one >>> binding to the rk3399-dwc3 or the special one binding to rk3328-dwc3. >>> And sadly it will probably depend on which module gets loaded first. >>> >>> And devicetrees are supposed to be backwards compatible so a newer >>> kernel should keep working with an old devicetree. >>> >>> A possible way forward would be to introduce a separate compatible for >>> rk3328 (both in the binding doc and the generic dwc3 driver), so that >>> we get only a >>> compatible = "rockchip,rk3328-dwc3"; >>> >>> Which then even can move easily into a new driver if necessary without >>> causing issues for existing devicetrees. >>> >>> Heiko >>> >>> >>> >>> _______________________________________________ >>> Linux-rockchip mailing list >>> Linux-rockchip@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip >> _______________________________________________ >> Linux-rockchip mailing list >> Linux-rockchip@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi index 84f14b132..f5c1ab596 100644 --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi @@ -917,6 +917,33 @@ status = "disabled"; }; + usbdrd3: usb@ff600000 { + compatible = "rockchip,rk3328-dwc3", "rockchip,rk3399-dwc3"; + clocks = <&cru SCLK_USB3OTG_REF>, <&cru SCLK_USB3OTG_SUSPEND>, + <&cru ACLK_USB3OTG>; + clock-names = "ref_clk", "suspend_clk", + "bus_clk"; + #address-cells = <2>; + #size-cells = <2>; + ranges; + status = "disabled"; + + usbdrd_dwc3: dwc3@ff600000 { + compatible = "snps,dwc3"; + reg = <0x0 0xff600000 0x0 0x100000>; + interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>; + dr_mode = "otg"; + phy_type = "utmi_wide"; + snps,dis_enblslpm_quirk; + snps,dis-u2-freeclk-exists-quirk; + snps,dis_u2_susphy_quirk; + snps,dis_u3_susphy_quirk; + snps,dis-del-phy-power-chg-quirk; + snps,dis-tx-ipgap-linecheck-quirk; + status = "disabled"; + }; + }; + gic: interrupt-controller@ff811000 { compatible = "arm,gic-400"; #interrupt-cells = <3>;