Message-ID: <8c0b8145-3e90-42e6-8e1a-5be6424d1055@kwiboo.se>
Date: Fri, 13 Sep 2024 00:35:23 +0200
From: Jonas Karlman <jonas@...boo.se>
To: Sam Edwards <cfsworks@...il.com>
Cc: Heiko Stuebner <heiko@...ech.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Ondrej Jirman <megi@....cz>,
 Chris Morgan <macromorgan@...mail.com>, Alex Zhao <zzc@...k-chips.com>,
 Dragan Simic <dsimic@...jaro.org>, FUKAUMI Naoki <naoki@...xa.com>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Jing Luo <jing@...g.rocks>, Kever Yang <kever.yang@...k-chips.com>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linux-rockchip@...ts.infradead.org" <linux-rockchip@...ts.infradead.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 Daniel Kukieła <daniel@...iela.pl>,
 Joshua Riek <jjriek@...izon.net>
Subject: Re: [PATCH 4/5] arm64: dts: rockchip: Enable all 3 USBs on Turing RK1

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

Powered by blists - more mailing lists