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

[Peter Chen <peter.chen@kernel.org>]

On 24-12-16 15:02:43, Théo Lebrun wrote:
> 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?

Usually, if wrapper needs to put controller to lower power state, 
it will do some jobs like close clock, etc. And if there is a option for
wakeup enable at wrapper, you may also need to set it.

Peter