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

[Sam Edwards]

On Thu, Sep 12, 2024 at 3:35 PM Jonas Karlman <jonas@xxxxxxxxx> wrote:
>
> On 2024-09-12 23:06, Sam Edwards wrote:
> > 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>;
>
> 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@xxxxxxxxx/
>