Re: [PATCH v6 2/5] usb: cdns3-ti: run HW init at resume() if HW was reset

["Théo Lebrun" <theo.lebrun@bootlin.com>]

On Sat Dec 14, 2024 at 9:49 AM CET, Peter Chen wrote:
> On 24-12-10 18:13:36, Théo Lebrun wrote:
> > At runtime_resume(), read the W1 (Wrapper Register 1) register to detect
> > if an hardware reset occurred. If it did, run the hardware init sequence.
> > 
> > This callback will be called at system-wide resume. Previously, if a
> > reset occurred during suspend, we would crash. The wrapper config had
> > not been written, leading to invalid register accesses inside cdns3.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > ---
> >  drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> > index d704eb39820ad08a8774be7f00aa473c3ff267c0..d35be7db7616ef5e5bed7dbd53b78a094809f7cc 100644
> > --- a/drivers/usb/cdns3/cdns3-ti.c
> > +++ b/drivers/usb/cdns3/cdns3-ti.c
> > @@ -188,6 +188,12 @@ static int cdns_ti_probe(struct platform_device *pdev)
> >  	data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
> >  	data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
> >  
> > +	/*
> > +	 * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it
> > +	 * detects it as uninitialised. We want to enforce a reset at probe,
> > +	 * and so do it manually here. This means the first runtime_resume()
> > +	 * will be a no-op.
> > +	 */
> >  	cdns_ti_reset_and_init_hw(data);
> >  
> >  	pm_runtime_enable(dev);
> > @@ -232,6 +238,24 @@ static void cdns_ti_remove(struct platform_device *pdev)
> >  	platform_set_drvdata(pdev, NULL);
> >  }
> >  
> > +static int cdns_ti_runtime_resume(struct device *dev)
> > +{
> > +	const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL;
> > +	struct cdns_ti *data = dev_get_drvdata(dev);
> > +	u32 w1;
> > +
> > +	w1 = cdns_ti_readl(data, USBSS_W1);
> > +	if ((w1 & mask) != mask)
> > +		cdns_ti_reset_and_init_hw(data);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops cdns_ti_pm_ops = {
> > +	RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
>
> Why only runtime resume, but without runtime suspend?

I don't see any situation where we would want "runtime suspend" be
equivalent to "reset the cdns3 wrapper". It implies losing child
states, triggering rediscovery of all USB devices at resume. Is that a
desired property of runtime suspend on any discoverable bus?

Sidenote: also, in our implementation, we do the standard thing of using
pm_runtime_force_suspend|resume() as suspend callback implementations.
With that, if we did reset the wrapper at suspend, we would:
 - always have to rediscover USB devices at resume and,
 - break wakeup sources.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com