| Message ID | 1373537788-30413-1-git-send-email-balbi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/11/2013 01:16 PM, Felipe Balbi wrote: > in order to make HWMOD and pm_runtime agree on the > initial state of the device, we will unidle the device > and call pm_runtime_set_active() to tell pm_runtime > that the device is really active. > > By the time driver's probe() is reached, a call to > pm_runtime_get_sync() will not cause driver's > ->runtime_resume() method to be called at first, only > after a successful ->runtime_suspend(). > > Note that we must prevent pm_runtime transitions while > driver is probing otherwise drivers would be suspended > as soon as they call pm_runtime_use_autosuspend(). By > calling pm_runtime_forbid() before probe() and > pm_runtime_allow() after probe() we 'fix' that detail. > > Note that this patch was inspired by PCI's pci_pm_init(). NAK. This is a hack. In addition to what I've mentioned in http://www.spinics.net/lists/arm-kernel/msg258061.html there are following issues: 1) this patch disables call to PM runtime callbacks for all OMAP drivers which is wrong - I've found, for example, that omap-usb-host.c driver enables TLL in some configurations in its .runtime_resume(): usbhs_runtime_resume() |-omap_tll_enable() 2) even with this fix the restore context issue will not be fixed for *non* console UARTs. Just try: #echo 0xDEAD > dev/ttyO3 // checked on OMAP4 SDP 3) I've checked most of OMAP drivers and all of them solve such kind of problem internally (SPI, MMC, I2C, etc.) 4) See inline > > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > > boot tested on top of today's Linus master > 6d128e1e72bf082542e85f72e6b7ddd704193588 with OMAP4 > panda. Reached console prompt and, after setting a > proper autosuspend delay, consoles autosuspend just > fine. > > It needs to be tested on other platforms. > > ps: note that we also call pm_runtime_set_suspended(dev) > from our late_initcall() to disable devices so that pm_runtime > and HWMOD continue to aggree on device's state. > > arch/arm/mach-omap2/omap_device.c | 44 +++++++++++++++++++++++++++++++++++---- > 1 file changed, 40 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c > index 5cc9287..cb1fc1d 100644 > --- a/arch/arm/mach-omap2/omap_device.c > +++ b/arch/arm/mach-omap2/omap_device.c > @@ -178,6 +178,26 @@ odbfd_exit: > return ret; > } > > +static void omap_device_pm_init(struct platform_device *pdev) > +{ > + omap_device_enable(pdev); > + pm_runtime_forbid(&pdev->dev); It's wrong to use pm_runtime_forbid() - pm_runtime_get_noresume() should be used instead. pm_runtime_forbid() |-rpm_resume() > + pm_runtime_set_active(&pdev->dev); > + device_enable_async_suspend(&pdev->dev); why did you use device_enable_async_suspend() - it would enable async suspend for devices - this is not related to the current issue and topic for discussion (currently is used by usb/scsi/pci/ata drivers only). > +} > + > +static void omap_device_pm_allow(struct platform_device *pdev) > +{ > + pm_runtime_allow(&pdev->dev); > +} > + > +static void omap_device_pm_exit(struct platform_device *pdev) > +{ > + device_disable_async_suspend(&pdev->dev); > + pm_runtime_set_suspended(&pdev->dev); > + omap_device_idle(pdev); > +} > + > static int _omap_device_notifier_call(struct notifier_block *nb, > unsigned long event, void *dev) > { > @@ -189,16 +209,31 @@ static int _omap_device_notifier_call(struct notifier_block *nb, > if (pdev->archdata.od) > omap_device_delete(pdev->archdata.od); > break; > + case BUS_NOTIFY_BIND_DRIVER: > + if (pdev->archdata.od) > + omap_device_pm_init(pdev); > + break; > + case BUS_NOTIFY_BOUND_DRIVER: > + if (pdev->archdata.od) > + omap_device_pm_allow(pdev); > + break; > + case BUS_NOTIFY_UNBOUND_DRIVER: > + if (pdev->archdata.od) > + omap_device_pm_exit(pdev); > + break; > case BUS_NOTIFY_ADD_DEVICE: > if (pdev->dev.of_node) > omap_device_build_from_dt(pdev); > - /* fall through */ > + break; > default: > - od = to_omap_device(pdev); > - if (od) > - od->_driver_status = event; > + /* nothing */ > + break; > } > > + od = to_omap_device(pdev); > + if (od) > + od->_driver_status = event; > + > return NOTIFY_DONE; > } > > @@ -855,6 +890,7 @@ static int __init omap_device_late_idle(struct device *dev, void *data) > dev_warn(dev, "%s: enabled but no driver. Idling\n", > __func__); > omap_device_idle(pdev); > + pm_runtime_set_suspended(dev); > } > } > >
On Fri, Jul 12, 2013 at 02:58:17PM +0300, Grygorii Strashko wrote: > On 07/11/2013 01:16 PM, Felipe Balbi wrote: > >in order to make HWMOD and pm_runtime agree on the > >initial state of the device, we will unidle the device > >and call pm_runtime_set_active() to tell pm_runtime > >that the device is really active. > > > >By the time driver's probe() is reached, a call to > >pm_runtime_get_sync() will not cause driver's > >->runtime_resume() method to be called at first, only > >after a successful ->runtime_suspend(). > > > >Note that we must prevent pm_runtime transitions while > >driver is probing otherwise drivers would be suspended > >as soon as they call pm_runtime_use_autosuspend(). By > >calling pm_runtime_forbid() before probe() and > >pm_runtime_allow() after probe() we 'fix' that detail. > > > >Note that this patch was inspired by PCI's pci_pm_init(). > > NAK. This is a hack. hack is your flag to check if the driver is "initialized". pff > In addition to what I've mentioned in > http://www.spinics.net/lists/arm-kernel/msg258061.html there are > following issues: > 1) this patch disables call to PM runtime callbacks for all no, it does not. It forbids pm runtime transitions during probe. > OMAP drivers which is wrong - I've found, for example, that > omap-usb-host.c driver enables TLL in some configurations in its > .runtime_resume(): > > usbhs_runtime_resume() > |-omap_tll_enable() which is wrong. PM runtime callbacks are supposed to be use for, surprise, PM! > 2) even with this fix the restore context issue will not be fixed for > *non* console UARTs. Just try: > #echo 0xDEAD > dev/ttyO3 // checked on OMAP4 SDP that I have not checked, but then again, with that you're not calling set_termios() anyway. > 3) I've checked most of OMAP drivers and all of them solve such kind > of problem internally (SPI, MMC, I2C, etc.) and you see no problem with that ? Repeating the same thing over and over again ? > 4) See inline > > > >Signed-off-by: Felipe Balbi <balbi@ti.com> > >--- > > > >boot tested on top of today's Linus master > >6d128e1e72bf082542e85f72e6b7ddd704193588 with OMAP4 > >panda. Reached console prompt and, after setting a > >proper autosuspend delay, consoles autosuspend just > >fine. > > > >It needs to be tested on other platforms. > > > >ps: note that we also call pm_runtime_set_suspended(dev) > >from our late_initcall() to disable devices so that pm_runtime > >and HWMOD continue to aggree on device's state. > > > > arch/arm/mach-omap2/omap_device.c | 44 +++++++++++++++++++++++++++++++++++---- > > 1 file changed, 40 insertions(+), 4 deletions(-) > > > >diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c > >index 5cc9287..cb1fc1d 100644 > >--- a/arch/arm/mach-omap2/omap_device.c > >+++ b/arch/arm/mach-omap2/omap_device.c > >@@ -178,6 +178,26 @@ odbfd_exit: > > return ret; > > } > > > >+static void omap_device_pm_init(struct platform_device *pdev) > >+{ > >+ omap_device_enable(pdev); > >+ pm_runtime_forbid(&pdev->dev); > It's wrong to use pm_runtime_forbid() - pm_runtime_get_noresume() > should be used instead. how come ? What makes you think pm_runtime_get_noresume() is the right thing here ? > pm_runtime_forbid() > |-rpm_resume() so what ? flags is zero.
On Friday 12 July 2013 05:40 PM, Felipe Balbi wrote: >> 3) I've checked most of OMAP drivers and all of them solve such kind >> > of problem internally (SPI, MMC, I2C, etc.) > and you see no problem with that ? Repeating the same thing over and > over again ? > I agree with Felipe that this is best handled at the bus/omap-device level to avoid all drivers having to repeat the same thing over. Having said that I dont know whats the best way to do this for OMAP and the patch we have currently does seem to have atleast issues handling processor IPs as Suman mentioned. regards, Rajendra
Felipe Balbi <balbi@ti.com> writes: > in order to make HWMOD and pm_runtime agree on the > initial state of the device, we will unidle the device > and call pm_runtime_set_active() to tell pm_runtime > that the device is really active. > > By the time driver's probe() is reached, a call to > pm_runtime_get_sync() will not cause driver's > ->runtime_resume() method to be called at first, only > after a successful ->runtime_suspend(). > > Note that we must prevent pm_runtime transitions while > driver is probing otherwise drivers would be suspended > as soon as they call pm_runtime_use_autosuspend(). By > calling pm_runtime_forbid() before probe() and > pm_runtime_allow() after probe() we 'fix' that detail. This part sounds a bit strange to me, and sounds more like a driver bug. Looking at omap-serial, this probably happens because the driver calls _use_autosuspend() after it has called _enable() but before has done its _get_sync(). > Note that this patch was inspired by PCI's pci_pm_init(). > > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > > boot tested on top of today's Linus master > 6d128e1e72bf082542e85f72e6b7ddd704193588 with OMAP4 > panda. Reached console prompt and, after setting a > proper autosuspend delay, consoles autosuspend just > fine. > > It needs to be tested on other platforms. > > ps: note that we also call pm_runtime_set_suspended(dev) > from our late_initcall() to disable devices so that pm_runtime > and HWMOD continue to aggree on device's state. Excellent. I like this approach, but agree it needs more testing like you mentioned. > arch/arm/mach-omap2/omap_device.c | 44 +++++++++++++++++++++++++++++++++++---- > 1 file changed, 40 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c > index 5cc9287..cb1fc1d 100644 > --- a/arch/arm/mach-omap2/omap_device.c > +++ b/arch/arm/mach-omap2/omap_device.c > @@ -178,6 +178,26 @@ odbfd_exit: > return ret; > } > > +static void omap_device_pm_init(struct platform_device *pdev) > +{ > + omap_device_enable(pdev); > + pm_runtime_forbid(&pdev->dev); > + pm_runtime_set_active(&pdev->dev); > + device_enable_async_suspend(&pdev->dev); You don't comment in the changelog about what this is for. I think we probably want this, but I suspect it belongs in a follow-up patch with its own changelog. Also Like PCI, I think we also want the device_set_wakeup_capable() here also, but that should have its own patch+changelog also. Kevin
diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c index 5cc9287..cb1fc1d 100644 --- a/arch/arm/mach-omap2/omap_device.c +++ b/arch/arm/mach-omap2/omap_device.c @@ -178,6 +178,26 @@ odbfd_exit: return ret; } +static void omap_device_pm_init(struct platform_device *pdev) +{ + omap_device_enable(pdev); + pm_runtime_forbid(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + device_enable_async_suspend(&pdev->dev); +} + +static void omap_device_pm_allow(struct platform_device *pdev) +{ + pm_runtime_allow(&pdev->dev); +} + +static void omap_device_pm_exit(struct platform_device *pdev) +{ + device_disable_async_suspend(&pdev->dev); + pm_runtime_set_suspended(&pdev->dev); + omap_device_idle(pdev); +} + static int _omap_device_notifier_call(struct notifier_block *nb, unsigned long event, void *dev) { @@ -189,16 +209,31 @@ static int _omap_device_notifier_call(struct notifier_block *nb, if (pdev->archdata.od) omap_device_delete(pdev->archdata.od); break; + case BUS_NOTIFY_BIND_DRIVER: + if (pdev->archdata.od) + omap_device_pm_init(pdev); + break; + case BUS_NOTIFY_BOUND_DRIVER: + if (pdev->archdata.od) + omap_device_pm_allow(pdev); + break; + case BUS_NOTIFY_UNBOUND_DRIVER: + if (pdev->archdata.od) + omap_device_pm_exit(pdev); + break; case BUS_NOTIFY_ADD_DEVICE: if (pdev->dev.of_node) omap_device_build_from_dt(pdev); - /* fall through */ + break; default: - od = to_omap_device(pdev); - if (od) - od->_driver_status = event; + /* nothing */ + break; } + od = to_omap_device(pdev); + if (od) + od->_driver_status = event; + return NOTIFY_DONE; } @@ -855,6 +890,7 @@ static int __init omap_device_late_idle(struct device *dev, void *data) dev_warn(dev, "%s: enabled but no driver. Idling\n", __func__); omap_device_idle(pdev); + pm_runtime_set_suspended(dev); } }
in order to make HWMOD and pm_runtime agree on the initial state of the device, we will unidle the device and call pm_runtime_set_active() to tell pm_runtime that the device is really active. By the time driver's probe() is reached, a call to pm_runtime_get_sync() will not cause driver's ->runtime_resume() method to be called at first, only after a successful ->runtime_suspend(). Note that we must prevent pm_runtime transitions while driver is probing otherwise drivers would be suspended as soon as they call pm_runtime_use_autosuspend(). By calling pm_runtime_forbid() before probe() and pm_runtime_allow() after probe() we 'fix' that detail. Note that this patch was inspired by PCI's pci_pm_init(). Signed-off-by: Felipe Balbi <balbi@ti.com> --- boot tested on top of today's Linus master 6d128e1e72bf082542e85f72e6b7ddd704193588 with OMAP4 panda. Reached console prompt and, after setting a proper autosuspend delay, consoles autosuspend just fine. It needs to be tested on other platforms. ps: note that we also call pm_runtime_set_suspended(dev) from our late_initcall() to disable devices so that pm_runtime and HWMOD continue to aggree on device's state. arch/arm/mach-omap2/omap_device.c | 44 +++++++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 4 deletions(-)