| Message ID | 20240912025034.180233-5-CFSworks@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Turing RK1 SoM DT updates | expand |
Hi Sam, On 2024-09-12 04:50, Sam Edwards wrote: > The Turing RK1 contains 3 different USBs: > - USB0: USB 2.0, OTG > - USB1: USB 3.0, host > - USB2: USB 2.0, host > > This patch activates the necessary DT nodes to enable all 3 buses. > > Future work will be needed on USB0: it is not USB 3.0, but Linux creates > an unused USB 3.0 root hub and the controller also requires that USBDP0 > be powered up. However, it is possible to remove this dependency. By > either patching the xHCI driver to ignore the enumerated USB 3.0 port or > setting usb3otg0_host_num_u3_port=0 in the GRF to stop the controller > from enumerating a USB 3.0 port in the first place, neither Linux nor > the controller will expect USB 3.0 capability, and USBDP0 can then > safely be removed from the `phys` property. > > Signed-off-by: Sam Edwards <CFSworks@gmail.com> > --- > .../boot/dts/rockchip/rk3588-turing-rk1.dtsi | 64 +++++++++++++++++++ > 1 file changed, 64 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi > index f6a12fe12d45..6036c4fe6727 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi > @@ -666,3 +666,67 @@ &uart9 { > pinctrl-0 = <&uart9m0_xfer>; > status = "okay"; > }; > + > +/* USB 0: USB 2.0 only, OTG-capable */ > +&u2phy0 { > + status = "okay"; > +}; > + > +&u2phy0_otg { > + status = "okay"; > +}; > + > +&usbdp_phy0 { > + /* > + * TODO: On the RK1, USBDP0 drives the DisplayPort pins, and is not > + * involved in this USB2-only bus. However, if the USB3 port is > + * enabled in the xHCI below, the controller will expect this PHY to be > + * powered up and holding RX_STATUS idle, or else it will generate an > + * endless stream of CSC events whenever a device is plugged in. Until > + * there is a way to communicate to usb_host0_xhci that it doesn't have > + * a USB3 port, we have no choice but to power up USBDP0. > + */ Sounds like this may be missing rockchip,dp-lane-mux = <0 1 2 3>; or rockchip,dp-lane-mux = <3 2 1 0>; if all lanes are used for DP and none are used for USB. It should help describe the hw and also help the driver set mode to UDPHY_MODE_DP and that should disable the u3 port, or there may be an issue to fix in the phy driver. > + status = "okay"; > +}; > + > +&usb_host0_xhci { > + extcon = <&u2phy0>; > + maximum-speed = "high-speed"; If this only use the USB2 phy, this should probably also override the default phys and phy-names props with: phys = <&u2phy0_otg>; phy-names = "usb2-phy"; Regards, Jonas > + status = "okay"; > +}; > + > +/* USB 1: USB 3.0, host only */ > +&u2phy1 { > + status = "okay"; > +}; > + > +&u2phy1_otg { > + status = "okay"; > +}; > + > +&usbdp_phy1 { > + status = "okay"; > +}; > + > +&usb_host1_xhci { > + extcon = <&u2phy1>; > + dr_mode = "host"; > + status = "okay"; > +}; > + > +/* USB 2: USB 2.0, host only */ > +&u2phy2 { > + status = "okay"; > +}; > + > +&u2phy2_host { > + status = "okay"; > +}; > + > +&usb_host0_ehci { > + status = "okay"; > +}; > + > +&usb_host0_ohci { > + status = "okay"; > +};
On Thu, Sep 12, 2024 at 12:53 PM Jonas Karlman <jonas@kwiboo.se> wrote: > > Hi Sam, > > Sounds like this may be missing > > rockchip,dp-lane-mux = <0 1 2 3>; > > or > > rockchip,dp-lane-mux = <3 2 1 0>; > > if all lanes are used for DP and none are used for USB. > > It should help describe the hw and also help the driver set mode to > UDPHY_MODE_DP and that should disable the u3 port, or there may be an > issue to fix in the phy driver. Thanks for your insights Jonas! I haven't yet gotten to DP enablement so I don't know the correct DP layout. Ultimately I do want the USBDP0 node to have the necessary properties for DP, but alas that's a patch for another day. Nonetheless, I briefly tried it and I don't think UDPHY_MODE_DP affects the PHY's "backend" (ctrl<->phy iface) at all, only the availability of frontend lanes to the USB-specific backend: So port u3 is still enabled, there's just no way to reach it electrically. With that in mind, would you recommend that I add a placeholder dp-lane-mux of 0 1 2 3 for now, just to keep the PHY from attempting to speak USB to a DP device? I don't foresee any harm in leaving it as-is but you may know something that I don't. > > > + status = "okay"; > > +}; > > + > > +&usb_host0_xhci { > > + extcon = <&u2phy0>; > > + maximum-speed = "high-speed"; > > If this only use the USB2 phy, this should probably also override the > default phys and phy-names props with: > > phys = <&u2phy0_otg>; > phy-names = "usb2-phy"; I agree completely: if the controller doesn't need the USB3 PHY, then it should not (implicitly) be specified in the DT. Being able to add these overrides is a big goal of mine as well. :) Sadly, `phys` is what initializes USBDP's USB-side backend, so without it the RX_STATUS line goes floating, and because the controller still expects a port there, it misbehaves: [ 30.981076] usb usb2-port1: connect-debounce failed I can tell the controller that there is no u3 port by doing this in U-Boot: => mw.l 0xfd5ac01c 0xf0000000 # usb3otg0_host_num_u3_port=0 => boot ...and that makes single-PHY operation work perfectly! But unless Linux itself effects that change, this patch can't rely on that GRF being set correctly. Do you have a recommendation on how I might go about disabling this port? I sent a patchset last year [1] that had the DWC3/xHCI driver ignore enumerated u3 ports when the maximum-speed was set to high-speed, but the consensus seems to be that this shouldn't be addressed at the xHCI driver level, so somehow zeroing the necessary GRF bits sounds like the way to go here. What do you think? Kind regards, Sam [1]: https://lore.kernel.org/all/20231208210458.912776-1-CFSworks@gmail.com/
On 2024-09-12 23:06, Sam Edwards wrote: > On Thu, Sep 12, 2024 at 12:53 PM Jonas Karlman <jonas@kwiboo.se> wrote: >> >> Hi Sam, >> >> Sounds like this may be missing >> >> rockchip,dp-lane-mux = <0 1 2 3>; >> >> or >> >> rockchip,dp-lane-mux = <3 2 1 0>; Small correction, the second 4-lane mode would be described as: rockchip,dp-lane-mux = <2 3 0 1>; >> >> if all lanes are used for DP and none are used for USB. >> >> It should help describe the hw and also help the driver set mode to >> UDPHY_MODE_DP and that should disable the u3 port, or there may be an >> issue to fix in the phy driver. > > Thanks for your insights Jonas! > > I haven't yet gotten to DP enablement so I don't know the correct DP > layout. Ultimately I do want the USBDP0 node to have the necessary > properties for DP, but alas that's a patch for another day. > > Nonetheless, I briefly tried it and I don't think UDPHY_MODE_DP > affects the PHY's "backend" (ctrl<->phy iface) at all, only the > availability of frontend lanes to the USB-specific backend: So port u3 > is still enabled, there's just no way to reach it electrically. > > With that in mind, would you recommend that I add a placeholder > dp-lane-mux of 0 1 2 3 for now, just to keep the PHY from attempting > to speak USB to a DP device? I don't foresee any harm in leaving it > as-is but you may know something that I don't. The rk_udphy_u3_port_disable() call in usbdp phy driver should help set the usb3otg0_host_num_u3_port=0 you mentioned: .usb3otg0_cfg = RK_UDPHY_GEN_GRF_REG(0x001c, 15, 0, 0x1100, 0x0188), .usb3otg1_cfg = RK_UDPHY_GEN_GRF_REG(0x0034, 15, 0, 0x1100, 0x0188), Here the disable/enable values is little bit inverted in macro, i.e. enable=0x0188 is the value set when u3_port_disable(disable=true) is called. Guessing that because the phy is not referenced its init() ops never gets called and u3 never gets disabled unless a usb3-phy is referenced. > >> >>> + status = "okay"; >>> +}; >>> + >>> +&usb_host0_xhci { >>> + extcon = <&u2phy0>; >>> + maximum-speed = "high-speed"; >> >> If this only use the USB2 phy, this should probably also override the >> default phys and phy-names props with: >> >> phys = <&u2phy0_otg>; >> phy-names = "usb2-phy"; > > I agree completely: if the controller doesn't need the USB3 PHY, then > it should not (implicitly) be specified in the DT. Being able to add > these overrides is a big goal of mine as well. :) > > Sadly, `phys` is what initializes USBDP's USB-side backend, so without > it the RX_STATUS line goes floating, and because the controller still > expects a port there, it misbehaves: > [ 30.981076] usb usb2-port1: connect-debounce failed > > I can tell the controller that there is no u3 port by doing this in U-Boot: > => mw.l 0xfd5ac01c 0xf0000000 # usb3otg0_host_num_u3_port=0 > => boot > ...and that makes single-PHY operation work perfectly! But unless > Linux itself effects that change, this patch can't rely on that GRF > being set correctly. > > Do you have a recommendation on how I might go about disabling this > port? I sent a patchset last year [1] that had the DWC3/xHCI driver > ignore enumerated u3 ports when the maximum-speed was set to > high-speed, but the consensus seems to be that this shouldn't be > addressed at the xHCI driver level, so somehow zeroing the necessary > GRF bits sounds like the way to go here. What do you think? Not sure if it would help but could be that part of init() ops should be moved to probe(). Would still require the phy driver to probe before usb controller for that to help/work. Adding a rockchip,dp-lane-mux prop and keeping the phys prop as-is is probably easiest way for now. One option for future could possible be to have grf driver disable u3 and modify usbdp phy driver to enable u3 instead of disable u3, not sure this will fully work, init of the usbdp phy seem very sensitive and possible a one-time op. Trying to "usb start" in U-Boot will only work one time, using "usb reset" or a "usb stop/start" cycle will cause usbdp phy init to fail and a full board reset is needed to get port working again. Regards, Jonas > > Kind regards, > Sam > > [1]: https://lore.kernel.org/all/20231208210458.912776-1-CFSworks@gmail.com/
On Thu, Sep 12, 2024 at 3:35 PM Jonas Karlman <jonas@kwiboo.se> wrote: > > On 2024-09-12 23:06, Sam Edwards wrote: > > On Thu, Sep 12, 2024 at 12:53 PM Jonas Karlman <jonas@kwiboo.se> wrote: > >> > >> Hi Sam, > >> > >> Sounds like this may be missing > >> > >> rockchip,dp-lane-mux = <0 1 2 3>; > >> > >> or > >> > >> rockchip,dp-lane-mux = <3 2 1 0>; > > Small correction, the second 4-lane mode would be described as: > > rockchip,dp-lane-mux = <2 3 0 1>; > > >> > >> if all lanes are used for DP and none are used for USB. > >> > >> It should help describe the hw and also help the driver set mode to > >> UDPHY_MODE_DP and that should disable the u3 port, or there may be an > >> issue to fix in the phy driver. > > > > Thanks for your insights Jonas! > > > > I haven't yet gotten to DP enablement so I don't know the correct DP > > layout. Ultimately I do want the USBDP0 node to have the necessary > > properties for DP, but alas that's a patch for another day. > > > > Nonetheless, I briefly tried it and I don't think UDPHY_MODE_DP > > affects the PHY's "backend" (ctrl<->phy iface) at all, only the > > availability of frontend lanes to the USB-specific backend: So port u3 > > is still enabled, there's just no way to reach it electrically. > > > > With that in mind, would you recommend that I add a placeholder > > dp-lane-mux of 0 1 2 3 for now, just to keep the PHY from attempting > > to speak USB to a DP device? I don't foresee any harm in leaving it > > as-is but you may know something that I don't. > > The rk_udphy_u3_port_disable() call in usbdp phy driver should help set > the usb3otg0_host_num_u3_port=0 you mentioned: > > .usb3otg0_cfg = RK_UDPHY_GEN_GRF_REG(0x001c, 15, 0, 0x1100, 0x0188), > .usb3otg1_cfg = RK_UDPHY_GEN_GRF_REG(0x0034, 15, 0, 0x1100, 0x0188), > > Here the disable/enable values is little bit inverted in macro, i.e. > enable=0x0188 is the value set when u3_port_disable(disable=true) is > called. Aha, I didn't notice that the PHY driver had this, thanks for pointing it out! Yes, it's good that it's also switching the clock source and disabling PHY status signals so I should definitely be relying on this code for now. > > Guessing that because the phy is not referenced its init() ops never > gets called and u3 never gets disabled unless a usb3-phy is referenced. > > > > >> > >>> + status = "okay"; > >>> +}; > >>> + > >>> +&usb_host0_xhci { > >>> + extcon = <&u2phy0>; > >>> + maximum-speed = "high-speed"; > >> > >> If this only use the USB2 phy, this should probably also override the > >> default phys and phy-names props with: > >> > >> phys = <&u2phy0_otg>; > >> phy-names = "usb2-phy"; > > > > I agree completely: if the controller doesn't need the USB3 PHY, then > > it should not (implicitly) be specified in the DT. Being able to add > > these overrides is a big goal of mine as well. :) > > > > Sadly, `phys` is what initializes USBDP's USB-side backend, so without > > it the RX_STATUS line goes floating, and because the controller still > > expects a port there, it misbehaves: > > [ 30.981076] usb usb2-port1: connect-debounce failed > > > > I can tell the controller that there is no u3 port by doing this in U-Boot: > > => mw.l 0xfd5ac01c 0xf0000000 # usb3otg0_host_num_u3_port=0 > > => boot > > ...and that makes single-PHY operation work perfectly! But unless > > Linux itself effects that change, this patch can't rely on that GRF > > being set correctly. > > > > Do you have a recommendation on how I might go about disabling this > > port? I sent a patchset last year [1] that had the DWC3/xHCI driver > > ignore enumerated u3 ports when the maximum-speed was set to > > high-speed, but the consensus seems to be that this shouldn't be > > addressed at the xHCI driver level, so somehow zeroing the necessary > > GRF bits sounds like the way to go here. What do you think? > > Not sure if it would help but could be that part of init() ops should be > moved to probe(). Would still require the phy driver to probe before usb > controller for that to help/work. > > Adding a rockchip,dp-lane-mux prop and keeping the phys prop as-is is > probably easiest way for now. Continuing my comments above: I agree fully. My v2 will add a placeholder dp-lane-mux. Maintainers: I will be sending a v2 of this patch separately in the future; please consider this patch withdrawn from the series. > One option for future could possible be to have grf driver disable u3 > and modify usbdp phy driver to enable u3 instead of disable u3, not > sure this will fully work, init of the usbdp phy seem very sensitive > and possible a one-time op. Trying to "usb start" in U-Boot will only > work one time, using "usb reset" or a "usb stop/start" cycle will cause > usbdp phy init to fail and a full board reset is needed to get port > working again. Arguably, it doesn't make sense for the USBDP driver to be affecting these GRFs at all, because *technically* they're configuration signals fed into the DWC3: therefore whatever driver binds to "rockchip,rk3588-dwc3" ought to be setting them according to the PHYs it discovers in the DT. I suspect that this code was only put in the PHY driver because that's a more convenient place to put Rockchip-specific management code for the time being. Of course, this is all a discussion to be had in a different thread. For now, suffice to say, I agree with your thoughts about the USB3OTGn GRF management situation needing improvement and am interested in lending a hand wherever I can. :) Thank you for your insights, Sam > > Regards, > Jonas > > > > > Kind regards, > > Sam > > > > [1]: https://lore.kernel.org/all/20231208210458.912776-1-CFSworks@gmail.com/ >
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi index f6a12fe12d45..6036c4fe6727 100644 --- a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi @@ -666,3 +666,67 @@ &uart9 { pinctrl-0 = <&uart9m0_xfer>; status = "okay"; }; + +/* USB 0: USB 2.0 only, OTG-capable */ +&u2phy0 { + status = "okay"; +}; + +&u2phy0_otg { + status = "okay"; +}; + +&usbdp_phy0 { + /* + * TODO: On the RK1, USBDP0 drives the DisplayPort pins, and is not + * involved in this USB2-only bus. However, if the USB3 port is + * enabled in the xHCI below, the controller will expect this PHY to be + * powered up and holding RX_STATUS idle, or else it will generate an + * endless stream of CSC events whenever a device is plugged in. Until + * there is a way to communicate to usb_host0_xhci that it doesn't have + * a USB3 port, we have no choice but to power up USBDP0. + */ + status = "okay"; +}; + +&usb_host0_xhci { + extcon = <&u2phy0>; + maximum-speed = "high-speed"; + status = "okay"; +}; + +/* USB 1: USB 3.0, host only */ +&u2phy1 { + status = "okay"; +}; + +&u2phy1_otg { + status = "okay"; +}; + +&usbdp_phy1 { + status = "okay"; +}; + +&usb_host1_xhci { + extcon = <&u2phy1>; + dr_mode = "host"; + status = "okay"; +}; + +/* USB 2: USB 2.0, host only */ +&u2phy2 { + status = "okay"; +}; + +&u2phy2_host { + status = "okay"; +}; + +&usb_host0_ehci { + status = "okay"; +}; + +&usb_host0_ohci { + status = "okay"; +};
The Turing RK1 contains 3 different USBs: - USB0: USB 2.0, OTG - USB1: USB 3.0, host - USB2: USB 2.0, host This patch activates the necessary DT nodes to enable all 3 buses. Future work will be needed on USB0: it is not USB 3.0, but Linux creates an unused USB 3.0 root hub and the controller also requires that USBDP0 be powered up. However, it is possible to remove this dependency. By either patching the xHCI driver to ignore the enumerated USB 3.0 port or setting usb3otg0_host_num_u3_port=0 in the GRF to stop the controller from enumerating a USB 3.0 port in the first place, neither Linux nor the controller will expect USB 3.0 capability, and USBDP0 can then safely be removed from the `phys` property. Signed-off-by: Sam Edwards <CFSworks@gmail.com> --- .../boot/dts/rockchip/rk3588-turing-rk1.dtsi | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+)