| Message ID | 1364824448-14732-3-git-send-email-gautam.vivek@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 01, 2013 at 07:24:01PM +0530, Vivek Gautam wrote: > The current code in the dwc3 probe effectively disables runtime pm > from ever working because it calls a get() that was never put() until > device removal. Change the runtime pm code to match the standard > formula and allow runtime pm to function. > > Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> > CC: Doug Anderson <dianders@chromium.org> > --- > drivers/usb/dwc3/core.c | 8 +++++++- > 1 files changed, 7 insertions(+), 1 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index e2325ad..3a6993c 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -491,6 +491,11 @@ static int dwc3_probe(struct platform_device *pdev) > > dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize"); > > + /* Setting device state as 'suspended' initially, wrong comment style. > + * to make sure we know device state prior to > + * pm_runtime_enable > + */ > + pm_runtime_set_suspended(dev); didn't Alan mention this should be done at the Bus level ? In that case, shouldn't you have call pm_runtime_set_active/suspended() based on DT's status=okay or status=disabled ? Or something similar ?
Hi Felipe, On Tue, Apr 2, 2013 at 1:59 PM, Felipe Balbi <balbi@ti.com> wrote: > On Mon, Apr 01, 2013 at 07:24:01PM +0530, Vivek Gautam wrote: >> The current code in the dwc3 probe effectively disables runtime pm >> from ever working because it calls a get() that was never put() until >> device removal. Change the runtime pm code to match the standard >> formula and allow runtime pm to function. >> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> >> CC: Doug Anderson <dianders@chromium.org> >> --- >> drivers/usb/dwc3/core.c | 8 +++++++- >> 1 files changed, 7 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index e2325ad..3a6993c 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -491,6 +491,11 @@ static int dwc3_probe(struct platform_device *pdev) >> >> dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize"); >> >> + /* Setting device state as 'suspended' initially, > > wrong comment style. Yea :-( will fix this. > >> + * to make sure we know device state prior to >> + * pm_runtime_enable >> + */ >> + pm_runtime_set_suspended(dev); > > didn't Alan mention this should be done at the Bus level ? In that case, > shouldn't you have call pm_runtime_set_active/suspended() based on > DT's status=okay or status=disabled ? Or something similar ? True, we should be doing this at bus level. But he did also mention to let pm core know of the state of the device before enabling the runtime pm. Right ? Moreover immediately after pm_runtime_enable(), we do pm_runtime_get_sync() so that device comes to active state. I am possibly missing things out here, not able to grab this whole picture completely :-( Wouldn't DT's status=disabled actually be disabling the device as a whole ? So, how much will runtime power management on the device be affecting ?
Hi, On Wed, Apr 03, 2013 at 11:35:43AM +0530, Vivek Gautam wrote: > >> The current code in the dwc3 probe effectively disables runtime pm > >> from ever working because it calls a get() that was never put() until > >> device removal. Change the runtime pm code to match the standard > >> formula and allow runtime pm to function. > >> > >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> > >> CC: Doug Anderson <dianders@chromium.org> > >> --- > >> drivers/usb/dwc3/core.c | 8 +++++++- > >> 1 files changed, 7 insertions(+), 1 deletions(-) > >> > >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >> index e2325ad..3a6993c 100644 > >> --- a/drivers/usb/dwc3/core.c > >> +++ b/drivers/usb/dwc3/core.c > >> @@ -491,6 +491,11 @@ static int dwc3_probe(struct platform_device *pdev) > >> > >> dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize"); > >> > >> + /* Setting device state as 'suspended' initially, > > > > wrong comment style. > Yea :-( will fix this. > > > > >> + * to make sure we know device state prior to > >> + * pm_runtime_enable > >> + */ > >> + pm_runtime_set_suspended(dev); > > > > didn't Alan mention this should be done at the Bus level ? In that case, > > shouldn't you have call pm_runtime_set_active/suspended() based on > > DT's status=okay or status=disabled ? Or something similar ? > > True, we should be doing this at bus level. But he did also mention to > let pm core > know of the state of the device before enabling the runtime pm. Right ? > Moreover immediately after pm_runtime_enable(), we do pm_runtime_get_sync() > so that device comes to active state. > I am possibly missing things out here, not able to grab this whole > picture completely :-( > > Wouldn't DT's status=disabled actually be disabling the device as a whole ? > So, how much will runtime power management on the device be affecting ? indeed, maybe we can keep it like this, but it would be nice to have OF core handle this for us based on whatever data.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index e2325ad..3a6993c 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -491,6 +491,11 @@ static int dwc3_probe(struct platform_device *pdev) dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize"); + /* Setting device state as 'suspended' initially, + * to make sure we know device state prior to + * pm_runtime_enable + */ + pm_runtime_set_suspended(dev); pm_runtime_enable(dev); pm_runtime_get_sync(dev); pm_runtime_forbid(dev); @@ -566,6 +571,7 @@ static int dwc3_probe(struct platform_device *pdev) goto err3; } + pm_runtime_put_sync(dev); pm_runtime_allow(dev); return 0; @@ -595,6 +601,7 @@ err1: err0: dwc3_free_event_buffers(dwc); + pm_runtime_disable(&pdev->dev); return ret; } @@ -606,7 +613,6 @@ static int dwc3_remove(struct platform_device *pdev) usb_phy_set_suspend(dwc->usb2_phy, 1); usb_phy_set_suspend(dwc->usb3_phy, 1); - pm_runtime_put(&pdev->dev); pm_runtime_disable(&pdev->dev); dwc3_debugfs_exit(dwc);
The current code in the dwc3 probe effectively disables runtime pm from ever working because it calls a get() that was never put() until device removal. Change the runtime pm code to match the standard formula and allow runtime pm to function. Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> CC: Doug Anderson <dianders@chromium.org> --- drivers/usb/dwc3/core.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)