On 17/11/2023 12:17, Théo Lebrun wrote:

Hello,

On Thu Nov 16, 2023 at 10:44 PM CET, Roger Quadros wrote:

quoted

On 16/11/2023 20:56, Théo Lebrun wrote:

quoted

On Thu Nov 16, 2023 at 1:40 PM CET, Roger Quadros wrote:

quoted

On 15/11/2023 17:02, Théo Lebrun wrote:

quoted

On Wed Nov 15, 2023 at 12:37 PM CET, Roger Quadros wrote:

quoted

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?

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel