Re: [PATCH 4/5] arm64: dts: rockchip: Enable all 3 USBs on Turing RK1

[Sam Edwards]

On Thu, Sep 12, 2024 at 12:53 PM Jonas Karlman <jonas@xxxxxxxxx> 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@xxxxxxxxx/