+Vibhore, On 16/11/2023 20:56, Théo Lebrun wrote: > Hello Roger, > > On Thu Nov 16, 2023 at 1:40 PM CET, Roger Quadros wrote: >> On 15/11/2023 17:02, Théo Lebrun wrote: >>> 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? >> >> What I meant is, calling pm_runtime_get/put() from system suspend/resume >> hooks doesn't seem right. >> >> How about using something like pm_runtime_forbid(dev) on devices which >> loose USB context on runtime suspend e.g. J7200. >> So at probe we can get rid of the pm_runtime_get_sync() call. > > What is the goal of enabling PM runtime to then block (ie forbid) it in > its enabled state until system suspend? If USB controller retains context on runtime_suspend on some platforms then we don't want to forbid PM runtime. > > Thinking some more about it and having read parts of the genpd source, > it's unclear to me why there even is some PM runtime calls in this > driver. No runtime_suspend/runtime_resume callbacks are registered. > Also, power-domains work as expected without any PM runtime calls. Probably it was required when the driver was introduced. > > The power domain is turned on when attached to a device > (see genpd_dev_pm_attach). It gets turned off automatically at > suspend_noirq (taking into account the many things that make genpd > complex: multiple devices per PD, subdomains, flags to customise the > behavior, etc.). Removing calls to PM runtime at probe keeps the driver > working. > > So my new proposal would be: remove all all PM runtime calls from this > driver. Anything wrong with this approach? Nothing wrong if we don't expect runtime_pm to work with this driver. > > Only possible reason I see for having PM runtime in this wrapper driver > would be cut the full power-domain when USB isn't used, with some PM > runtime interaction with the children node. But that cannot work > currently as we don't register a runtime_resume to init the hardware, > so this cannot be the current expected behavior. > >> e.g. >> >> pm_runtime_set_active(dev); >> pm_runtime_enable(dev); >> if (cnds_ti->can_loose_context) >> pm_runtime_forbid(dev); >> >> pm_runtime_set_autosuspend_delay(dev, CNDS_TI_AUTOSUSPEND_DELAY); /* could be 20ms? */ > > Why mention autosuspend in this driver? This will turn the device off in > CNDS_TI_AUTOSUSPEND_DELAY then nothing enables it back using > pm_runtime_get. We have nothing to reconfigure the device, ie no > runtime_resume, so we must not go into runtime suspend. It would be enabled/disabled based on when the child "cdns3,usb" does runtime_resume/suspend. > >> pm_runtime_mark_last_busy(dev); >> pm_runtime_use_autosuspend(dev); >> >> You will need to modify the suspend/resume handlers accordingly. >> https://docs.kernel.org/power/runtime_pm.html#runtime-pm-and-system-sleep >> >> What I'm not sure of is if there are any TI platforms that retain USB context >> on power domain off. Let me get back on this. Till then we can assume that >> all platforms loose USB context on power domain off. > > Good question indeed! Thanks for looking into it. From what I see all 5 > DT nodes which use this driver in upstream devicetrees have a > power-domain configured. So if the behavior is the same on all three TI > platforms (which would be the logical thing to assume) it would make > sense that all controllers lose power at suspend. > > Thanks, > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com -- cheers, -roger