On 17/11/2023 12:17, Théo Lebrun wrote: > 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. Even if driver doesn't have runtime_suspend/resume hooks, wouldn't the USB Power domain turn off if we enable runtime PM and allow runtime autosuspend and all children have runtime suspended? > >>> 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 > -- cheers, -roger
