[3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

  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.

Reuse the newly extracted cdns_ti_init_hw() function that contains the
register write sequence.

We guard this behavior based on compatible to avoid modifying the
current behavior on other platforms. If the controller does not reset
we do not want to touch PM runtime & do not want to redo reg writes.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/cdns3-ti.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)
  

Hello Théo,

> 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.
>
> Reuse the newly extracted cdns_ti_init_hw() function that contains the
> register write sequence.
>
> We guard this behavior based on compatible to avoid modifying the
> current behavior on other platforms. If the controller does not reset
> we do not want to touch PM runtime & do not want to redo reg writes.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> 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;

Just a small remark:

What about adding a boolean in the cdns_ti struct for taking care of
it ? Then you will go through the device tree only during probe. Moreover
if this behaviour is needed for more compatible we can just add them in
the probe too.

Besides this you still can add my

Reviewed-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory

> +
> +	pm_runtime_put_sync(data->dev);
> +
> +	return 0;
> +}
> +
> +static int cdns_ti_resume(struct device *dev)
> +{
> +	struct cdns_ti *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!of_device_is_compatible(dev_of_node(dev), "ti,j7200-usb"))
> +		return 0;
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
> +		goto err;
> +	}
> +
> +	cdns_ti_init_hw(data);
> +
> +	return 0;
> +
> +err:
> +	pm_runtime_put_sync(data->dev);
> +	pm_runtime_disable(data->dev);
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops cdns_ti_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(cdns_ti_suspend, cdns_ti_resume)
> +};
> +
> +#endif /* CONFIG_PM */
> +
>  static int cdns_ti_remove_core(struct device *dev, void *c)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -218,6 +262,7 @@ static void cdns_ti_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id cdns_ti_of_match[] = {
> +	{ .compatible = "ti,j7200-usb", },
>  	{ .compatible = "ti,j721e-usb", },
>  	{ .compatible = "ti,am64-usb", },
>  	{},
> @@ -228,8 +273,9 @@ static struct platform_driver cdns_ti_driver = {
>  	.probe		= cdns_ti_probe,
>  	.remove_new	= cdns_ti_remove,
>  	.driver		= {
> -		.name	= "cdns3-ti",
> +		.name		= "cdns3-ti",
>  		.of_match_table	= cdns_ti_of_match,
> +		.pm		= pm_ptr(&cdns_ti_pm_ops),
>  	},
>  };
>  
>
> -- 
> 2.41.0
>
>
  

Hello,

On Mon Nov 13, 2023 at 4:39 PM CET, Gregory CLEMENT wrote:
> > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> > 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;
>
> Just a small remark:
>
> What about adding a boolean in the cdns_ti struct for taking care of
> it ? Then you will go through the device tree only during probe. Moreover
> if this behaviour is needed for more compatible we can just add them in
> the probe too.

Will do. The hardest part will be to pick a good name.

> Besides this you still can add my
>
> Reviewed-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks for the review.

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
  

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.

> 
> Reuse the newly extracted cdns_ti_init_hw() function that contains the
> register write sequence.
> 
> We guard this behavior based on compatible to avoid modifying the
> current behavior on other platforms. If the controller does not reset
> we do not want to touch PM runtime & do not want to redo reg writes.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 48 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> 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.

> +}
> +
> +static int cdns_ti_resume(struct device *dev)
> +{
> +	struct cdns_ti *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!of_device_is_compatible(dev_of_node(dev), "ti,j7200-usb"))
> +		return 0;
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
> +		goto err;
> +	}
> +
> +	cdns_ti_init_hw(data);
> +
> +	return 0;
> +
> +err:
> +	pm_runtime_put_sync(data->dev);
> +	pm_runtime_disable(data->dev);
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops cdns_ti_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(cdns_ti_suspend, cdns_ti_resume)
> +};
> +
> +#endif /* CONFIG_PM */
> +
>  static int cdns_ti_remove_core(struct device *dev, void *c)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -218,6 +262,7 @@ static void cdns_ti_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id cdns_ti_of_match[] = {
> +	{ .compatible = "ti,j7200-usb", },
>  	{ .compatible = "ti,j721e-usb", },
>  	{ .compatible = "ti,am64-usb", },
>  	{},
> @@ -228,8 +273,9 @@ static struct platform_driver cdns_ti_driver = {
>  	.probe		= cdns_ti_probe,
>  	.remove_new	= cdns_ti_remove,
>  	.driver		= {
> -		.name	= "cdns3-ti",
> +		.name		= "cdns3-ti",
>  		.of_match_table	= cdns_ti_of_match,
> +		.pm		= pm_ptr(&cdns_ti_pm_ops),
>  	},
>  };
>  
>
  

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

------------------------------------------------------------------------
  

On 15/11/2023 17:02, Théo Lebrun wrote:
> 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?

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.
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? */
        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.

One comment below.

> +	return ret;
> +}


> 
>  - 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,
> 
> --> +static int cdns_ti_resume(struct device *dev)
> +{
> +	struct cdns_ti *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!of_device_is_compatible(dev_of_node(dev), "ti,j7200-usb"))
> +		return 0;
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
> +		goto err;
> +	}
> +
> +	cdns_ti_init_hw(data);
> +
> +	return 0;
> +
> +err:
> +	pm_runtime_put_sync(data->dev);
> +	pm_runtime_disable(data->dev);

Why do you do a pm_runtime_disable() here?

> +	return ret;
> +}


> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
> ------------------------------------------------------------------------
> 
>
  

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?

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.

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?

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.

>         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
  

+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
  

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
  

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
>
  

Hi Roger,

On Fri Nov 17, 2023 at 12:51 PM CET, Roger Quadros wrote:
> On 17/11/2023 12:17, Théo Lebrun wrote:
> > 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?

That cannot be the currently desired behavior as it would require a
runtime_resume implementation that restores this wrapper to its desired
state.

It could however be something that could be implemented. It would be a
matter of enabling PM runtime and that is it in the probe. No need to
even init the hardware in the probe. Then the runtime_resume
implementation would call the new cdns_ti_init_hw.

This is what the cdns3-imx wrapper is doing in a way, though what they
need is clocks rather than some registers to be written.

That all feels like outside the scope of the current patch series
though.

My suggestion for V2 would still therefore be to remove all PM runtime
as it has no impact. It could be added later down the road if cutting
the power-domain is a goal of yours.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
  

On 17/11/2023 16:20, Théo Lebrun wrote:
> Hi Roger,
> 
> On Fri Nov 17, 2023 at 12:51 PM CET, Roger Quadros wrote:
>> On 17/11/2023 12:17, Théo Lebrun wrote:
>>> 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?
> 
> That cannot be the currently desired behavior as it would require a
> runtime_resume implementation that restores this wrapper to its desired
> state.
> 
> It could however be something that could be implemented. It would be a
> matter of enabling PM runtime and that is it in the probe. No need to
> even init the hardware in the probe. Then the runtime_resume
> implementation would call the new cdns_ti_init_hw.
> 
> This is what the cdns3-imx wrapper is doing in a way, though what they
> need is clocks rather than some registers to be written.
> 
> That all feels like outside the scope of the current patch series
> though.
> 
> My suggestion for V2 would still therefore be to remove all PM runtime
> as it has no impact. It could be added later down the road if cutting
> the power-domain is a goal of yours.

OK let's do this.
  

Théo Lebrun <theo.lebrun@bootlin.com> writes:

> Hi Roger,
>
> On Fri Nov 17, 2023 at 12:51 PM CET, Roger Quadros wrote:
>> On 17/11/2023 12:17, Théo Lebrun wrote:
>> > 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.

The point is to signal to the power domain the device is in that it can
power on/off.  These IP blocks are (re)used on many different SoCs, so
the driver should not make any assumptions about what power domain it is
in (if any.)

>> 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?
>
> That cannot be the currently desired behavior as it would require a
> runtime_resume implementation that restores this wrapper to its desired
> state.

Or, for this USB IP block to be in a power domain that has memory
retention, in which case the power domain can still go off to save
power, but not lose context.

> It could however be something that could be implemented. It would be a
> matter of enabling PM runtime and that is it in the probe. No need to
> even init the hardware in the probe. Then the runtime_resume
> implementation would call the new cdns_ti_init_hw.

This is the way.

> This is what the cdns3-imx wrapper is doing in a way, though what they
> need is clocks rather than some registers to be written.
>
> That all feels like outside the scope of the current patch series
> though.
>
> My suggestion for V2 would still therefore be to remove all PM runtime
> as it has no impact. It could be added later down the road if cutting
> the power-domain is a goal of yours.

It may have no impact on the platform you are on, but it's very likely
to have an impact on other platforms with this same IP. 

Kevin
  

Hello,

On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> > On Fri Nov 17, 2023 at 12:51 PM CET, Roger Quadros wrote:
> >> On 17/11/2023 12:17, Théo Lebrun wrote:
> >> > 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.
>
> The point is to signal to the power domain the device is in that it can
> power on/off.  These IP blocks are (re)used on many different SoCs, so
> the driver should not make any assumptions about what power domain it is
> in (if any.)

On my platform, when the device is attached to the PD it gets turned on.
That feels logical to me: if a driver is not RPM aware it "just works".
Are there platforms where RPM must get enabled for the attached
power-domains to be turned on?

The call chain that attaches & enables PD is platform_probe ->
dev_pm_domain_attach. That function takes a bool power_on which is
always true. In the genpd case, genpd_dev_pm_attach
calls __genpd_dev_pm_attach which does a genpd_power_on.

Things I've not accounted for:

 - ACPI looks like it does the same but I've not checked. It gets passed
   the power_on bool argument.

 - genpd_dev_pm_attach early returns with no error if there are multiple
   PM domains attached to the device. There are many platforms in the
   case according to some devicetree grepping. I can imagine a behavior
   difference where dev_pm_domain callpaths handle that differently in
   the RPM case. Is that what we are discussing?

> >> 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?
> >
> > That cannot be the currently desired behavior as it would require a
> > runtime_resume implementation that restores this wrapper to its desired
> > state.
>
> Or, for this USB IP block to be in a power domain that has memory
> retention, in which case the power domain can still go off to save
> power, but not lose context.
>
> > It could however be something that could be implemented. It would be a
> > matter of enabling PM runtime and that is it in the probe. No need to
> > even init the hardware in the probe. Then the runtime_resume
> > implementation would call the new cdns_ti_init_hw.
>
> This is the way.

I agree & I have a prototype, but that requires some work on the child
devices as from what I remember they are not eager to go to runtime
suspend (ie a driver might be holding a reference).

I feel this leans outside the scope of this patch series though.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
  

Théo Lebrun <theo.lebrun@bootlin.com> writes:

> Hello,
>
> On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
>> Théo Lebrun <theo.lebrun@bootlin.com> writes:
>> > On Fri Nov 17, 2023 at 12:51 PM CET, Roger Quadros wrote:
>> >> On 17/11/2023 12:17, Théo Lebrun wrote:
>> >> > 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.
>>
>> The point is to signal to the power domain the device is in that it can
>> power on/off.  These IP blocks are (re)used on many different SoCs, so
>> the driver should not make any assumptions about what power domain it is
>> in (if any.)
>
> On my platform, when the device is attached to the PD it gets turned on.
> That feels logical to me: if a driver is not RPM aware it "just works".

It "just works"... until the domain gets turned off.

> Are there platforms where RPM must get enabled for the attached
> power-domains to be turned on?

Yes, but but more importantly, there are platforms where RPM must get
enabled for the power domain to *stay* on.  For example, the power
domain might get turned on due to devices probing etc, but as soon as
all the RPM-enabled drivers drop their refcount, the domain will turn
off.  If there is a device in that domain with a non-RPM enabled driver,
that device will be powered off anc cause a crash.

> The call chain that attaches & enables PD is platform_probe ->
> dev_pm_domain_attach. That function takes a bool power_on which is
> always true. In the genpd case, genpd_dev_pm_attach
> calls __genpd_dev_pm_attach which does a genpd_power_on.
>
> Things I've not accounted for:
>
>  - ACPI looks like it does the same but I've not checked. It gets passed
>    the power_on bool argument.
>
>  - genpd_dev_pm_attach early returns with no error if there are multiple
>    PM domains attached to the device. There are many platforms in the
>    case according to some devicetree grepping. I can imagine a behavior
>    difference where dev_pm_domain callpaths handle that differently in
>    the RPM case. Is that what we are discussing?

You're only looking at the attach, power-on phase.  You need to think
about when the domain powers off and powers back on.

Kevin
  

Hello,

On Sun Nov 26, 2023 at 11:36 PM CET, Kevin Hilman wrote:
> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> > On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> >> The point is to signal to the power domain the device is in that it can
> >> power on/off.  These IP blocks are (re)used on many different SoCs, so
> >> the driver should not make any assumptions about what power domain it is
> >> in (if any.)
> >
> > On my platform, when the device is attached to the PD it gets turned on.
> > That feels logical to me: if a driver is not RPM aware it "just works".
>
> It "just works"... until the domain gets turned off.
>
> > Are there platforms where RPM must get enabled for the attached
> > power-domains to be turned on?
>
> Yes, but but more importantly, there are platforms where RPM must get
> enabled for the power domain to *stay* on.  For example, the power
> domain might get turned on due to devices probing etc, but as soon as
> all the RPM-enabled drivers drop their refcount, the domain will turn
> off.  If there is a device in that domain with a non-RPM enabled driver,
> that device will be powered off anc cause a crash.

OK, that makes sense, thanks for taking the time to explain. This topic
makes me see two things that I feel are close to being bugs. I'd be
curious to get your view on both.

 - If a device does not use RPM but its children do, it might get its
   associated power-domain turned off. That forces every single driver
   that want to stay alive to enable & increment RPM.

   What I naively expect: a genpd with a device attached to it that is
   not using RPM should mean that it should not be powered off at
   runtime_suspend. Benefit: no RPM calls in drivers that do not use
   it, and the behavior is that the genpd associated stays alive "as
   expected".

 - If a device uses RPM & has a refcount strictly positive, its
   associated power-domain gets turned off either way at suspend_noirq.
   That feels non-intuitive as well.

   What I naively expect: check for RPM refcounts of attached devices
   when doing suspend_noirq of power-domains. Benefit: control of what
   power-domains do from attached devices is done through the RPM API.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
  

Théo Lebrun <theo.lebrun@bootlin.com> writes:

> Hello,
>
> On Sun Nov 26, 2023 at 11:36 PM CET, Kevin Hilman wrote:
>> Théo Lebrun <theo.lebrun@bootlin.com> writes:
>> > On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
>> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
>> >> The point is to signal to the power domain the device is in that it can
>> >> power on/off.  These IP blocks are (re)used on many different SoCs, so
>> >> the driver should not make any assumptions about what power domain it is
>> >> in (if any.)
>> >
>> > On my platform, when the device is attached to the PD it gets turned on.
>> > That feels logical to me: if a driver is not RPM aware it "just works".
>>
>> It "just works"... until the domain gets turned off.
>>
>> > Are there platforms where RPM must get enabled for the attached
>> > power-domains to be turned on?
>>
>> Yes, but but more importantly, there are platforms where RPM must get
>> enabled for the power domain to *stay* on.  For example, the power
>> domain might get turned on due to devices probing etc, but as soon as
>> all the RPM-enabled drivers drop their refcount, the domain will turn
>> off.  If there is a device in that domain with a non-RPM enabled driver,
>> that device will be powered off anc cause a crash.
>
> OK, that makes sense, thanks for taking the time to explain. This topic
> makes me see two things that I feel are close to being bugs. I'd be
> curious to get your view on both.

TL;DR; they are features, not bugs.  ;)

>  - If a device does not use RPM but its children do, it might get its
>    associated power-domain turned off. That forces every single driver
>    that want to stay alive to enable & increment RPM.
>
>    What I naively expect: a genpd with a device attached to it that is
>    not using RPM should mean that it should not be powered off at
>    runtime_suspend. Benefit: no RPM calls in drivers that do not use
>    it, and the behavior is that the genpd associated stays alive "as
>    expected".

Your expectation makes sense, but unfortunately, that's not how RPM was
designed.

Also remember that we don't really want specific device drivers to know
which PM domain they are in, or whether they are in a PM domain at
all. The same IP block can be integrated in different ways across
different SoCs, even within the same SoC family, and we want the device
driver to just work.  

For that to work well, any driver that might be in any PM domain should
add RPM calls.

>  - If a device uses RPM & has a refcount strictly positive, its
>    associated power-domain gets turned off either way at suspend_noirq.
>    That feels non-intuitive as well.
>
>    What I naively expect: check for RPM refcounts of attached devices
>    when doing suspend_noirq of power-domains. Benefit: control of what
>    power-domains do from attached devices is done through the RPM API.

I agree that this is non-intuitive from an RPM PoV, but remember that
RPM was added on top of existing system-wide suspend support.  And from
a system-wide suspend PoV, it might be non-intuitive that a driver
thinks it should be active (non-zero refcount) when user just requested
a system-wide suspend.  Traditionally, when a user requests a
system-wide suspend, they expect the whole system to shut down.

On real SoCs in real products, power management is not so black and
white, and I fully understand that, and personally, I'm definitely open
to not forcing RPM-active devices off in suspend, but that would require
changes to core code, and remove some assumptions of core code that
would need to be validated/tested.

Kevin
  

On Tue, Dec 12, 2023 at 10:26:05AM -0800, Kevin Hilman wrote:
> Théo Lebrun <theo.lebrun@bootlin.com> writes:

> 
> > Hello,
> >
> > On Sun Nov 26, 2023 at 11:36 PM CET, Kevin Hilman wrote:
> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> >> > On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
> >> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> >> >> The point is to signal to the power domain the device is in that it can
> >> >> power on/off.  These IP blocks are (re)used on many different SoCs, so
> >> >> the driver should not make any assumptions about what power domain it is
> >> >> in (if any.)
> >> >
> >> > On my platform, when the device is attached to the PD it gets turned on.
> >> > That feels logical to me: if a driver is not RPM aware it "just works".
> >>
> >> It "just works"... until the domain gets turned off.
> >>
> >> > Are there platforms where RPM must get enabled for the attached
> >> > power-domains to be turned on?
> >>
> >> Yes, but but more importantly, there are platforms where RPM must get
> >> enabled for the power domain to *stay* on.  For example, the power
> >> domain might get turned on due to devices probing etc, but as soon as
> >> all the RPM-enabled drivers drop their refcount, the domain will turn
> >> off.  If there is a device in that domain with a non-RPM enabled driver,
> >> that device will be powered off anc cause a crash.
> >
> > OK, that makes sense, thanks for taking the time to explain. This topic
> > makes me see two things that I feel are close to being bugs. I'd be
> > curious to get your view on both.
> 
> TL;DR; they are features, not bugs.  ;)
> 
> >  - If a device does not use RPM but its children do, it might get its
> >    associated power-domain turned off. That forces every single driver
> >    that want to stay alive to enable & increment RPM.
> >
> >    What I naively expect: a genpd with a device attached to it that is
> >    not using RPM should mean that it should not be powered off at
> >    runtime_suspend. Benefit: no RPM calls in drivers that do not use
> >    it, and the behavior is that the genpd associated stays alive "as
> >    expected".
> 
> Your expectation makes sense, but unfortunately, that's not how RPM was
> designed.

I'm not sure how runtime PM is meant to work with power domains.  

However, from the very beginning of runtime PM the intention was that 
device drivers and subsystems could safely ignore it.  Their devices 
would have a permanently nonzero disable_depth (permanent because the 
driver/subsystem would not know to change it) and therefore would always 
remain in the active state (see rpm_check_suspend_allowed()).

It would be very surprising if runtime PM for power domains was written 
in a way that would subvert this intention.

Alan Stern