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.
+ */
+ 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";
+};
--
2.44.2Hi 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/
>
© 2016 - 2024 Red Hat, Inc.