| Message ID | 20231113-j7200-usb-suspend-v1-3-ad1ee714835c@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible | expand |
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
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 >
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
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(-)