Hi Roger, On Wed Nov 15, 2023 at 12:37 PM CET, Roger Quadros wrote: > On 13/11/2023 16:26, Théo Lebrun wrote: > > Hardware initialisation is only done at probe. The J7200 USB controller > > is reset at resume because of power-domains toggling off & on. We > > therefore (1) toggle PM runtime at suspend/resume & (2) reconfigure the > > hardware at resume. > > at probe we are doing a pm_runtime_get() and never doing a put thus > preventing any runtime PM. Indeed. The get() from probe/resume are in symmetry with the put() from suspend. Is this wrong in some manner? > > index c331bcd2faeb..50b38c4b9c87 100644 > > --- a/drivers/usb/cdns3/cdns3-ti.c > > +++ b/drivers/usb/cdns3/cdns3-ti.c > > @@ -197,6 +197,50 @@ static int cdns_ti_probe(struct platform_device *pdev) > > return error; > > } > > > > +#ifdef CONFIG_PM > > + > > +static int cdns_ti_suspend(struct device *dev) > > +{ > > + struct cdns_ti *data = dev_get_drvdata(dev); > > + > > + if (!of_device_is_compatible(dev_of_node(dev), "ti,j7200-usb")) > > + return 0; > > + > > + pm_runtime_put_sync(data->dev); > > + > > + return 0; > > You might want to check suspend/resume ops in cdns3-plat and > do something similar here. I'm unsure what you are referring to specifically in cdns3-plat? - Using pm_runtime_status_suspended() to get the current PM runtime state & act on it? I don't see why because we know the pm_runtime state is a single put() at probe. - Having a `in_lpm` flag to track low-power mode state? I wouldn't see why we'd want that as we don't register runtime_suspend & runtime_resume callbacks and system syspend/resume can be assumed to be called in the right order. - Checking the `device_may_wakeup()`? That doesn't apply to this driver which cannot be a wakeup source. Thanks for your review! Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ------------------------------------------------------------------------