| Message ID | 20231113-j7200-usb-suspend-v1-3-ad1ee714835c@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: cdns: fix suspend on J7200 by assuming reset on resume | expand |
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
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; +} + +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), }, };
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(-)