Message-ID: <h57ok2hw6os7bcafqkrqknfvm7hnu25m2oe54qmrsuzdwqlos3@m4och2fcdm7s>
Date: Wed, 26 Feb 2025 18:03:42 +0100
From: Ondřej Jirman <megi@....cz>
To: Heiko Stübner <heiko@...ech.de>
Cc: vkoul@...nel.org, kishon@...nel.org, linux-phy@...ts.infradead.org, 
	linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	quentin.schulz@...rry.de, sebastian.reichel@...labora.com, 
	Heiko Stuebner <heiko.stuebner@...rry.de>
Subject: Re: [PATCH 2/2] phy: rockchip: usbdp: re-init the phy on
 orientation-change

Hi Heiko,

On Wed, Feb 26, 2025 at 04:53:45PM +0100, Heiko Stübner wrote:
> Hey Ondřej,
> 
> Am Mittwoch, 26. Februar 2025, 15:46:11 MEZ schrieb Ondřej Jirman:
> > On Tue, Feb 25, 2025 at 07:45:19PM +0100, Heiko Stuebner wrote:
> > > From: Heiko Stuebner <heiko.stuebner@...rry.de>
> > > 
> > > Until now the usbdp in the orientation-handler set the new lane setup in
> > > its internal state variables and adapted the sbu gpios as needed.
> > > It never actually updated the phy itself though, but relied on the
> > > controlling usb-controller to disable and re-enable the phy.
> > > 
> > > And while on the vendor-kernel, I could see that on every unplug the dwc3
> > > did go to its suspend and woke up on the next device plug-in event,
> > > thus toggling the phy as needed, this does not happen in all cases and we
> > > should not rely on that behaviour.
> > 
> > On RK3399 there's a similar issue with the equivalent type-c PHY driver.
> > The TRM (part 2) states that:
> > 
> > 4.6.1 Some Special Settings before Initialization
> > 
> > - Set USB3.0 OTG controller AXI master setting.
> > - Clear USB2.0 only mode setting (bit 3 of register GRF_USB3PHY0/1_CON0 in Chapter GRF)
> > - USB3.0 OTG controller should be hold in reset during the initialization of the corresponding
> >   TypeC PHY until TypeC PHY is ready for USB operation.
> > - Set PHYIF to 1 to use 16-bit UTMI+ interface (see register GUSB2PHYCFG0)
> > - Clear ENBLSLPM to 0 to disable sleep and l1 suspend (see register GUSB2PHYCFG0)
> >   ...
> > 
> > The PHY for Superspeed signals is expected to be set up while the USB
> > controller is held in reset, which makes sense HW wise, and it's what downstream
> > kernel efectivelly does, via its RPM based hack.
> > 
> > RK3588 TRM doesn't have very detailed notes on this, but I expect it will be
> > similar.
> > 
> > So reconfiguring the phy here, while it's actively linked to the USB controller
> > without the controller driver driving the process so it reliably happens while
> > it's in reset, or at least so that USB controller reset happens afterwards, may
> > not be correct way to approach this.
> 
> the function here, is using infrastructure from the type-c framework.
> 
> The function in question tcpm_mux_set(), which then ends up in the
> usbdp_phy only gets called from tcpm_reset_port() .
>
> Which I think will do the right already - judging by its name ;-) .

No. This function has nothing to do with USB controller reset. And while  dwc3
driver consumes usb_role_switch interface, calling set_role on that interface
just translates to calling dwc3_set_mode(), which will do nothing if the mode
did not change from what it was previously. (and it stays the same if you just
re-plug the same device in same mode but just in different cable orientation)

Only time DWC3 reset seems to happen currently is sometimes in dwc3_set_mode()
depending on driver state, and then in dwc3_init()/exit() which is called only
at driver probe/remove time and in suspend/resume.

So if PHY driver reconfigures/"power-cycles" the PHY on its own, DWC3 driver
will not issue any reset to the controller HW, at least not based on the
set_role signal from usb_role_switch alone most of the time.

Also tcpm_orientation interface you use for PHY reset is called from many places
in TCPM driver, based on many events, mainly from tcpm_set_roles(), so you'll
be resetting the PHY quite a bit in uncoordinated manner with the dwc3 MAC
interface it's connected to in HW.

> [and also by the fact that the referenced qcom phy behaves similarly
>  when talking to the type-c framework]
> 
> 
> For the rk3399 I would think converting the old typec-phy-driver over to
> use the actual kernel type-c framework, might just magically solve the
> issue you have on it.

It will not. The problem is not getting the information about roles between the
drivers. extcon works fine for that. Problem is that your proposed solution
doesn't coordinate the PHY reset with USB controller to ensure proper ordering,
or even ensuring that any USB controller reset happens, or the controller knows
about the PHY reset.

Resulting issues may happen only occasionally, dunno. I guess it depends on
whether SS MAC will happen to be accessing USBDP PHY at the time when you're
turning off the PHY, and if this messes up the internal state of the MAC pipe
interface, so after PHY comes back up from reset state, whether that locks up
the MAC interface logic or not will depend on luck. :)

You can ask Rockchip what they think about uncoordinated PHY reset, while DWC3
is up and ready. Maybe it's ok on RK3588, but they're not doing it in their
vendor driver this way, and they forbid it on RK3399, so perhaps it's not? From
HW point of view it looks sketchy.

Kind regards,
	o.

> Rockchip actually has converted the rk3399 typec-phy to use the type-c
> framework in their vendor kernel.
> 
> 
> Heiko
> 
> 

Powered by blists - more mailing lists