| Message ID | 1527518174-27860-2-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Roger Quadros <rogerq@ti.com> writes: > Don't call pm_runtime_set_active() as it will prevent the device > from being activated in the next pm_runtime_get_sync() call. > > Also call pm_runtime_get_sync() before of_platform_populate(). > > Signed-off-by: Roger Quadros <rogerq@ti.com> This patch is wrong. > --- > drivers/usb/dwc3/dwc3-of-simple.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c > index e98d221..2cbb5c0 100644 > --- a/drivers/usb/dwc3/dwc3-of-simple.c > +++ b/drivers/usb/dwc3/dwc3-of-simple.c > @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) > if (ret) > goto err_resetc_assert; > > + pm_runtime_enable(dev); > + pm_runtime_get_sync(dev); No, this is the wrong way to do things. My device should be enabled already from probe, specially since I have already enabled clocks. > ret = of_platform_populate(np, NULL, NULL, dev); > if (ret) { > for (i = 0; i < simple->num_clocks; i++) { > @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) > goto err_resetc_assert; > } > > - pm_runtime_set_active(dev); > - pm_runtime_enable(dev); > - pm_runtime_get_sync(dev); this hunk is not wrong at all.
On 30/05/18 15:31, Felipe Balbi wrote: > Roger Quadros <rogerq@ti.com> writes: > >> Don't call pm_runtime_set_active() as it will prevent the device >> from being activated in the next pm_runtime_get_sync() call. >> >> Also call pm_runtime_get_sync() before of_platform_populate(). >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> > > This patch is wrong. > >> --- >> drivers/usb/dwc3/dwc3-of-simple.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c >> index e98d221..2cbb5c0 100644 >> --- a/drivers/usb/dwc3/dwc3-of-simple.c >> +++ b/drivers/usb/dwc3/dwc3-of-simple.c >> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) >> if (ret) >> goto err_resetc_assert; >> >> + pm_runtime_enable(dev); >> + pm_runtime_get_sync(dev); > > No, this is the wrong way to do things. My device should be enabled > already from probe, specially since I have already enabled clocks. As far as I understood just enabling clocks (which might not include bus clock) doesn't ensure device is enabled. Did you mean that I don't need to do a pm_runtime_get_sync() to enable my device in probe? Who is enabling by device for me then? Is device core supposed to do it? > >> ret = of_platform_populate(np, NULL, NULL, dev); >> if (ret) { >> for (i = 0; i < simple->num_clocks; i++) { >> @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) >> goto err_resetc_assert; >> } >> >> - pm_runtime_set_active(dev); >> - pm_runtime_enable(dev); >> - pm_runtime_get_sync(dev); > > this hunk is not wrong at all. > The issue I was facing is that without this patch my device wasn't being enabled as pm_runtime_set_active() is being done _before_ pm_runtime_get_sync(). It could be an issue with the platform's PM domain code as well. Tero/Dave what do you think?
Hi, Roger Quadros <rogerq@ti.com> writes: > On 30/05/18 15:31, Felipe Balbi wrote: >> Roger Quadros <rogerq@ti.com> writes: >> >>> Don't call pm_runtime_set_active() as it will prevent the device >>> from being activated in the next pm_runtime_get_sync() call. >>> >>> Also call pm_runtime_get_sync() before of_platform_populate(). >>> >>> Signed-off-by: Roger Quadros <rogerq@ti.com> >> >> This patch is wrong. >> >>> --- >>> drivers/usb/dwc3/dwc3-of-simple.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c >>> index e98d221..2cbb5c0 100644 >>> --- a/drivers/usb/dwc3/dwc3-of-simple.c >>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c >>> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) >>> if (ret) >>> goto err_resetc_assert; >>> >>> + pm_runtime_enable(dev); >>> + pm_runtime_get_sync(dev); >> >> No, this is the wrong way to do things. My device should be enabled >> already from probe, specially since I have already enabled clocks. > > As far as I understood just enabling clocks (which might not include bus clock) > doesn't ensure device is enabled. > > Did you mean that I don't need to do a pm_runtime_get_sync() to enable my device in probe? > Who is enabling by device for me then? Is device core supposed to do it? Not device core, but the bus code. Look at how PCI handles it. IIRC, only TI's omap_device behaves peculiarly WRT probe & pm runtime. > The issue I was facing is that without this patch my device wasn't being enabled > as pm_runtime_set_active() is being done _before_ pm_runtime_get_sync(). > It could be an issue with the platform's PM domain code as well. Could be
On 31/05/18 10:59, Felipe Balbi wrote: > > Hi, > > Roger Quadros <rogerq@ti.com> writes: > >> On 30/05/18 15:31, Felipe Balbi wrote: >>> Roger Quadros <rogerq@ti.com> writes: >>> >>>> Don't call pm_runtime_set_active() as it will prevent the device >>>> from being activated in the next pm_runtime_get_sync() call. >>>> >>>> Also call pm_runtime_get_sync() before of_platform_populate(). >>>> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>> >>> This patch is wrong. >>> >>>> --- >>>> drivers/usb/dwc3/dwc3-of-simple.c | 7 +++---- >>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c >>>> index e98d221..2cbb5c0 100644 >>>> --- a/drivers/usb/dwc3/dwc3-of-simple.c >>>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c >>>> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) >>>> if (ret) >>>> goto err_resetc_assert; >>>> >>>> + pm_runtime_enable(dev); >>>> + pm_runtime_get_sync(dev); >>> >>> No, this is the wrong way to do things. My device should be enabled >>> already from probe, specially since I have already enabled clocks. >> >> As far as I understood just enabling clocks (which might not include bus clock) >> doesn't ensure device is enabled. >> >> Did you mean that I don't need to do a pm_runtime_get_sync() to enable my device in probe? >> Who is enabling by device for me then? Is device core supposed to do it? > > Not device core, but the bus code. Look at how PCI handles it. IIRC, > only TI's omap_device behaves peculiarly WRT probe & pm runtime. PCI does it doesn't mean it is a rule that bus code has to enable the device before probe. At least platform bus doesn't seem to do it as part of of_platform_populate(). see __device_attach() and driver_probe_device() in drivers/base/dd.c It does a pm_runtime_get_sync() on dev->parent but not on dev. Also, from section 5 of Documentation/power/runtime_pm.txt "In addition to that, the initial runtime PM status of all devices is 'suspended', but it need not reflect the actual physical state of the device. Thus, if the device is initially active (i.e. it is able to process I/O), its runtime PM status must be changed to 'active', with the help of pm_runtime_set_active(), before pm_runtime_enable() is called for the device." "Note, if the device may execute pm_runtime calls during the probe (such as if it is registers with a subsystem that may call back in) then the pm_runtime_get_sync() call paired with a pm_runtime_put() call will be appropriate to ensure that the device is not put back to sleep during the probe. This can happen with systems such as the network device layer." So looks like we can't assume that the device is "active" when probe() is called. Which means, we need to do pm_runtime_get_sync(); enable optional clocks; pm_runtime_forbid(); if we don't yet want runtime suspend/resume. OR pm_runtime_put(); if we want runtime suspend/resume support. I don't think we should be calling pm_runtime_set_active() as we can't be sure of the initial state of the device for different buses. > >> The issue I was facing is that without this patch my device wasn't being enabled >> as pm_runtime_set_active() is being done _before_ pm_runtime_get_sync(). >> It could be an issue with the platform's PM domain code as well. > > Could be >
On Wed, Jun 13, 2018 at 02:15:11PM +0300, Roger Quadros wrote: > On 31/05/18 10:59, Felipe Balbi wrote: > > > > Hi, > > > > Roger Quadros <rogerq@ti.com> writes: > > > >> On 30/05/18 15:31, Felipe Balbi wrote: > >>> Roger Quadros <rogerq@ti.com> writes: > >>> > >>>> Don't call pm_runtime_set_active() as it will prevent the device > >>>> from being activated in the next pm_runtime_get_sync() call. > >>>> > >>>> Also call pm_runtime_get_sync() before of_platform_populate(). > >>>> > >>>> Signed-off-by: Roger Quadros <rogerq@ti.com> > >>> > >>> This patch is wrong. > >>> > >>>> --- > >>>> drivers/usb/dwc3/dwc3-of-simple.c | 7 +++---- > >>>> 1 file changed, 3 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c > >>>> index e98d221..2cbb5c0 100644 > >>>> --- a/drivers/usb/dwc3/dwc3-of-simple.c > >>>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c > >>>> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) > >>>> if (ret) > >>>> goto err_resetc_assert; > >>>> > >>>> + pm_runtime_enable(dev); > >>>> + pm_runtime_get_sync(dev); > >>> > >>> No, this is the wrong way to do things. My device should be enabled > >>> already from probe, specially since I have already enabled clocks. > >> > >> As far as I understood just enabling clocks (which might not > >> include bus clock) doesn't ensure device is enabled. In general, that cannot be assumed no. Specifically, this cannot currently be assumed for OMAP due to how its power domain has been implemented. > >> Did you mean that I don't need to do a pm_runtime_get_sync() to > >> enable my device in probe? > >> Who is enabling by device for me then? Is device core supposed to > >> do it? Driver core calls the power domain activate() callback before probe, but OMAP does not implement that callback. So you're right that you currently need a pm_runtime_get_sync() in probe instead of setting status active explicitly as the OMAP pm domain will not enable the bus clock otherwise. You still need to deal with the current driver explicit clock enable if you want to change this, though. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c index e98d221..2cbb5c0 100644 --- a/drivers/usb/dwc3/dwc3-of-simple.c +++ b/drivers/usb/dwc3/dwc3-of-simple.c @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) if (ret) goto err_resetc_assert; + pm_runtime_enable(dev); + pm_runtime_get_sync(dev); + ret = of_platform_populate(np, NULL, NULL, dev); if (ret) { for (i = 0; i < simple->num_clocks; i++) { @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev) goto err_resetc_assert; } - pm_runtime_set_active(dev); - pm_runtime_enable(dev); - pm_runtime_get_sync(dev); - return 0; err_resetc_assert:
Don't call pm_runtime_set_active() as it will prevent the device from being activated in the next pm_runtime_get_sync() call. Also call pm_runtime_get_sync() before of_platform_populate(). Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/usb/dwc3/dwc3-of-simple.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)