Hello, On Thu Nov 16, 2023 at 10:44 PM CET, Roger Quadros wrote: > On 16/11/2023 20:56, Théo Lebrun wrote: > > 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: > >>>> 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. What's the point of runtime PM if nothing is done based on it? This is the current behavior of the driver. > > 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. I'm not seeing any behavior change in cdns3-ti since its addition in Oct 2019. > > 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. Why care about being enabled or disabled if we don't do anything based on that? Children does do runtime PM stuff but I don't understand how that could influence us. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
