| Message ID | 1415263010-7992-2-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 6 November 2014 09:36, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > Some drivers (e.g. bus drivers) may want to check if power.irq_safe was > called by child driver, regardless of CONFIG_PM_RUNTIME. > > An example scenario is amba/bus.c and dma/pl330.c drivers. The runtime > suspend/resume callbacks in amba bus driver act differently if irq_safe > was set by child driver (in irq_safe mode bus clock is only disabled). > > The pl330 driver sets irq_safe and assumes that amba bus driver will > only disable the clock in runtime PM. So in system sleep suspend > callback the pl330 driver unprepares the clock after calling > pm_runtime_force_suspend(). > > However inconsistency would appear if CONFIG_PM_RUNTIME is not set and > child drivers do not want the irq_safe runtime PM. In such case amba bus > driver still has to know whether child driver wanted irq_safe - by > looking at dev->power.irq_safe field. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> FWIW: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > include/linux/pm.h | 2 +- > include/linux/pm_runtime.h | 5 ++++- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 383fd68aaee1..b05fa954f50d 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -566,6 +566,7 @@ struct dev_pm_info { > bool ignore_children:1; > bool early_init:1; /* Owned by the PM core */ > bool direct_complete:1; /* Owned by the PM core */ > + unsigned int irq_safe:1; /* PM runtime */ > spinlock_t lock; > #ifdef CONFIG_PM_SLEEP > struct list_head entry; > @@ -590,7 +591,6 @@ struct dev_pm_info { > unsigned int run_wake:1; > unsigned int runtime_auto:1; > unsigned int no_callbacks:1; > - unsigned int irq_safe:1; > unsigned int use_autosuspend:1; > unsigned int timer_autosuspends:1; > unsigned int memalloc_noio:1; > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > index 367f49b9a1c9..d94a65662a60 100644 > --- a/include/linux/pm_runtime.h > +++ b/include/linux/pm_runtime.h > @@ -166,7 +166,10 @@ static inline bool pm_runtime_suspended_if_enabled(struct device *dev) { return > static inline bool pm_runtime_enabled(struct device *dev) { return false; } > > static inline void pm_runtime_no_callbacks(struct device *dev) {} > -static inline void pm_runtime_irq_safe(struct device *dev) {} > +static inline void pm_runtime_irq_safe(struct device *dev) > +{ > + dev->power.irq_safe = 1; > +} > > static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; } > static inline void pm_runtime_mark_last_busy(struct device *dev) {} > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, November 06, 2014 09:36:46 AM Krzysztof Kozlowski wrote: > Some drivers (e.g. bus drivers) may want to check if power.irq_safe was > called by child driver, regardless of CONFIG_PM_RUNTIME. > > An example scenario is amba/bus.c and dma/pl330.c drivers. The runtime > suspend/resume callbacks in amba bus driver act differently if irq_safe > was set by child driver (in irq_safe mode bus clock is only disabled). > > The pl330 driver sets irq_safe and assumes that amba bus driver will > only disable the clock in runtime PM. So in system sleep suspend > callback the pl330 driver unprepares the clock after calling > pm_runtime_force_suspend(). > > However inconsistency would appear if CONFIG_PM_RUNTIME is not set and > child drivers do not want the irq_safe runtime PM. In such case amba bus > driver still has to know whether child driver wanted irq_safe - by > looking at dev->power.irq_safe field. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > include/linux/pm.h | 2 +- > include/linux/pm_runtime.h | 5 ++++- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 383fd68aaee1..b05fa954f50d 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -566,6 +566,7 @@ struct dev_pm_info { > bool ignore_children:1; > bool early_init:1; /* Owned by the PM core */ > bool direct_complete:1; /* Owned by the PM core */ > + unsigned int irq_safe:1; /* PM runtime */ > spinlock_t lock; > #ifdef CONFIG_PM_SLEEP > struct list_head entry; > @@ -590,7 +591,6 @@ struct dev_pm_info { > unsigned int run_wake:1; > unsigned int runtime_auto:1; > unsigned int no_callbacks:1; > - unsigned int irq_safe:1; > unsigned int use_autosuspend:1; > unsigned int timer_autosuspends:1; > unsigned int memalloc_noio:1; Well, that is a good reason to introduce a wrapper around power.irq_safe in my view. And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME unset. This way not only you wouldn't need to move the flag from under the #ifdef, but also you would make the compiler skip the relevant pieces of code entiretly for CONFIG_PM_RUNTIME unset. > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h > index 367f49b9a1c9..d94a65662a60 100644 > --- a/include/linux/pm_runtime.h > +++ b/include/linux/pm_runtime.h > @@ -166,7 +166,10 @@ static inline bool pm_runtime_suspended_if_enabled(struct device *dev) { return > static inline bool pm_runtime_enabled(struct device *dev) { return false; } > > static inline void pm_runtime_no_callbacks(struct device *dev) {} > -static inline void pm_runtime_irq_safe(struct device *dev) {} > +static inline void pm_runtime_irq_safe(struct device *dev) > +{ > + dev->power.irq_safe = 1; > +} > > static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; } > static inline void pm_runtime_mark_last_busy(struct device *dev) {} >
On czw, 2014-11-06 at 23:51 +0100, Rafael J. Wysocki wrote: > On Thursday, November 06, 2014 09:36:46 AM Krzysztof Kozlowski wrote: > > Some drivers (e.g. bus drivers) may want to check if power.irq_safe was > > called by child driver, regardless of CONFIG_PM_RUNTIME. > > > > An example scenario is amba/bus.c and dma/pl330.c drivers. The runtime > > suspend/resume callbacks in amba bus driver act differently if irq_safe > > was set by child driver (in irq_safe mode bus clock is only disabled). > > > > The pl330 driver sets irq_safe and assumes that amba bus driver will > > only disable the clock in runtime PM. So in system sleep suspend > > callback the pl330 driver unprepares the clock after calling > > pm_runtime_force_suspend(). > > > > However inconsistency would appear if CONFIG_PM_RUNTIME is not set and > > child drivers do not want the irq_safe runtime PM. In such case amba bus > > driver still has to know whether child driver wanted irq_safe - by > > looking at dev->power.irq_safe field. > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > --- > > include/linux/pm.h | 2 +- > > include/linux/pm_runtime.h | 5 ++++- > > 2 files changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/pm.h b/include/linux/pm.h > > index 383fd68aaee1..b05fa954f50d 100644 > > --- a/include/linux/pm.h > > +++ b/include/linux/pm.h > > @@ -566,6 +566,7 @@ struct dev_pm_info { > > bool ignore_children:1; > > bool early_init:1; /* Owned by the PM core */ > > bool direct_complete:1; /* Owned by the PM core */ > > + unsigned int irq_safe:1; /* PM runtime */ > > spinlock_t lock; > > #ifdef CONFIG_PM_SLEEP > > struct list_head entry; > > @@ -590,7 +591,6 @@ struct dev_pm_info { > > unsigned int run_wake:1; > > unsigned int runtime_auto:1; > > unsigned int no_callbacks:1; > > - unsigned int irq_safe:1; > > unsigned int use_autosuspend:1; > > unsigned int timer_autosuspends:1; > > unsigned int memalloc_noio:1; > > Well, that is a good reason to introduce a wrapper around power.irq_safe in my > view. > > And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME > unset. > > This way not only you wouldn't need to move the flag from under the #ifdef, > but also you would make the compiler skip the relevant pieces of code > entiretly for CONFIG_PM_RUNTIME unset. Few days ago I would be happy with your opinion :), but know I think this is better solution than wrapper. Consider case: 1. PM_RUNTIME unset. 2. System suspends. 3. The pl330 in its suspend callback calls force_runtime_suspend which leads us to amba/bus. 4. The amba/bus.c in runtime suspend checks for irq_safe (it is FALSE), so it disables and unprepares the clock. 5. The pl330 in probe requested irq_safe so it assumes amba/bus will only disable the clock. So the pl330 unprepares the clock. Again. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 7 Nov 2014, Krzysztof Kozlowski wrote: > > Well, that is a good reason to introduce a wrapper around power.irq_safe in my > > view. > > > > And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME > > unset. > > > > This way not only you wouldn't need to move the flag from under the #ifdef, > > but also you would make the compiler skip the relevant pieces of code > > entiretly for CONFIG_PM_RUNTIME unset. > > Few days ago I would be happy with your opinion :), but know I think > this is better solution than wrapper. Consider case: > 1. PM_RUNTIME unset. > 2. System suspends. > 3. The pl330 in its suspend callback calls force_runtime_suspend which > leads us to amba/bus. > 4. The amba/bus.c in runtime suspend checks for irq_safe (it is FALSE), > so it disables and unprepares the clock. > 5. The pl330 in probe requested irq_safe so it assumes amba/bus will > only disable the clock. So the pl330 unprepares the clock. Again. To me, this sounds like a good reason to avoid using force_runtime_suspend(). In fact, it sounds like a good reason to avoid relying on the runtime PM mechanism to handle non-runtime-PM things (like a system suspend callback). If CONFIG_PM_RUNTIME isn't enabled then the runtime PM stack simply should not be used. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, November 07, 2014 09:50:58 AM Alan Stern wrote: > On Fri, 7 Nov 2014, Krzysztof Kozlowski wrote: > > > > Well, that is a good reason to introduce a wrapper around power.irq_safe in my > > > view. > > > > > > And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME > > > unset. > > > > > > This way not only you wouldn't need to move the flag from under the #ifdef, > > > but also you would make the compiler skip the relevant pieces of code > > > entiretly for CONFIG_PM_RUNTIME unset. > > > > Few days ago I would be happy with your opinion :), but know I think > > this is better solution than wrapper. Consider case: > > 1. PM_RUNTIME unset. > > 2. System suspends. > > 3. The pl330 in its suspend callback calls force_runtime_suspend which > > leads us to amba/bus. > > 4. The amba/bus.c in runtime suspend checks for irq_safe (it is FALSE), > > so it disables and unprepares the clock. > > 5. The pl330 in probe requested irq_safe so it assumes amba/bus will > > only disable the clock. So the pl330 unprepares the clock. Again. > > To me, this sounds like a good reason to avoid using > force_runtime_suspend(). In fact, it sounds like a good reason to > avoid relying on the runtime PM mechanism to handle non-runtime-PM > things (like a system suspend callback). If CONFIG_PM_RUNTIME isn't > enabled then the runtime PM stack simply should not be used. Amen. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7 November 2014 09:06, Krzysztof Kozlowski <k.kozlowski@samsung.com> wrote: > On czw, 2014-11-06 at 23:51 +0100, Rafael J. Wysocki wrote: >> On Thursday, November 06, 2014 09:36:46 AM Krzysztof Kozlowski wrote: >> > Some drivers (e.g. bus drivers) may want to check if power.irq_safe was >> > called by child driver, regardless of CONFIG_PM_RUNTIME. >> > >> > An example scenario is amba/bus.c and dma/pl330.c drivers. The runtime >> > suspend/resume callbacks in amba bus driver act differently if irq_safe >> > was set by child driver (in irq_safe mode bus clock is only disabled). >> > >> > The pl330 driver sets irq_safe and assumes that amba bus driver will >> > only disable the clock in runtime PM. So in system sleep suspend >> > callback the pl330 driver unprepares the clock after calling >> > pm_runtime_force_suspend(). >> > >> > However inconsistency would appear if CONFIG_PM_RUNTIME is not set and >> > child drivers do not want the irq_safe runtime PM. In such case amba bus >> > driver still has to know whether child driver wanted irq_safe - by >> > looking at dev->power.irq_safe field. >> > >> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> > --- >> > include/linux/pm.h | 2 +- >> > include/linux/pm_runtime.h | 5 ++++- >> > 2 files changed, 5 insertions(+), 2 deletions(-) >> > >> > diff --git a/include/linux/pm.h b/include/linux/pm.h >> > index 383fd68aaee1..b05fa954f50d 100644 >> > --- a/include/linux/pm.h >> > +++ b/include/linux/pm.h >> > @@ -566,6 +566,7 @@ struct dev_pm_info { >> > bool ignore_children:1; >> > bool early_init:1; /* Owned by the PM core */ >> > bool direct_complete:1; /* Owned by the PM core */ >> > + unsigned int irq_safe:1; /* PM runtime */ >> > spinlock_t lock; >> > #ifdef CONFIG_PM_SLEEP >> > struct list_head entry; >> > @@ -590,7 +591,6 @@ struct dev_pm_info { >> > unsigned int run_wake:1; >> > unsigned int runtime_auto:1; >> > unsigned int no_callbacks:1; >> > - unsigned int irq_safe:1; >> > unsigned int use_autosuspend:1; >> > unsigned int timer_autosuspends:1; >> > unsigned int memalloc_noio:1; >> >> Well, that is a good reason to introduce a wrapper around power.irq_safe in my >> view. >> >> And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME >> unset. >> >> This way not only you wouldn't need to move the flag from under the #ifdef, >> but also you would make the compiler skip the relevant pieces of code >> entiretly for CONFIG_PM_RUNTIME unset. > > Few days ago I would be happy with your opinion :), but know I think > this is better solution than wrapper. Consider case: > 1. PM_RUNTIME unset. > 2. System suspends. > 3. The pl330 in its suspend callback calls force_runtime_suspend which > leads us to amba/bus. > 4. The amba/bus.c in runtime suspend checks for irq_safe (it is FALSE), > so it disables and unprepares the clock. > 5. The pl330 in probe requested irq_safe so it assumes amba/bus will > only disable the clock. So the pl330 unprepares the clock. Again. This is easy to solve, still by using Rafael's approach. In the pl330 system PM callbacks, you need to check "pm_runtime_is_irqsafe()" or whatever the wrapper would be called. When it returns true, that's when you should do clk_prepare|unprepare(). I think that would be quite nice. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
unsubscribe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7 November 2014 15:50, Alan Stern <stern@rowland.harvard.edu> wrote: > On Fri, 7 Nov 2014, Krzysztof Kozlowski wrote: > >> > Well, that is a good reason to introduce a wrapper around power.irq_safe in my >> > view. >> > >> > And define the wrapper so that it always returns false for CONFIG_PM_RUNTIME >> > unset. >> > >> > This way not only you wouldn't need to move the flag from under the #ifdef, >> > but also you would make the compiler skip the relevant pieces of code >> > entiretly for CONFIG_PM_RUNTIME unset. >> >> Few days ago I would be happy with your opinion :), but know I think >> this is better solution than wrapper. Consider case: >> 1. PM_RUNTIME unset. >> 2. System suspends. >> 3. The pl330 in its suspend callback calls force_runtime_suspend which >> leads us to amba/bus. >> 4. The amba/bus.c in runtime suspend checks for irq_safe (it is FALSE), >> so it disables and unprepares the clock. >> 5. The pl330 in probe requested irq_safe so it assumes amba/bus will >> only disable the clock. So the pl330 unprepares the clock. Again. > > To me, this sounds like a good reason to avoid using > force_runtime_suspend(). In fact, it sounds like a good reason to > avoid relying on the runtime PM mechanism to handle non-runtime-PM > things (like a system suspend callback). If CONFIG_PM_RUNTIME isn't > enabled then the runtime PM stack simply should not be used. There are an important advantage of using the pm_runtime_force_suspend() here. For the driver to handle clock gating at system PM suspend, it first needs to bring the device into full power, through pm_runtime_get_sync(). Otherwise it's not safe to gate the clock, since it may already be gated. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 10 Nov 2014, Ulf Hansson wrote: > > To me, this sounds like a good reason to avoid using > > force_runtime_suspend(). In fact, it sounds like a good reason to > > avoid relying on the runtime PM mechanism to handle non-runtime-PM > > things (like a system suspend callback). If CONFIG_PM_RUNTIME isn't > > enabled then the runtime PM stack simply should not be used. > > There are an important advantage of using the pm_runtime_force_suspend() here. > > For the driver to handle clock gating at system PM suspend, it first > needs to bring the device into full power, through > pm_runtime_get_sync(). Otherwise it's not safe to gate the clock, > since it may already be gated. That's fine, but it has nothing to do with pm_runtime_force_suspend(). Besides, if the real question is whether or not to gate the clock (or in other words, has the clock already been gated), why not just store a "clock_is_gated" flag somewhere? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 November 2014 17:36, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 10 Nov 2014, Ulf Hansson wrote: > >> > To me, this sounds like a good reason to avoid using >> > force_runtime_suspend(). In fact, it sounds like a good reason to >> > avoid relying on the runtime PM mechanism to handle non-runtime-PM >> > things (like a system suspend callback). If CONFIG_PM_RUNTIME isn't >> > enabled then the runtime PM stack simply should not be used. >> >> There are an important advantage of using the pm_runtime_force_suspend() here. >> >> For the driver to handle clock gating at system PM suspend, it first >> needs to bring the device into full power, through >> pm_runtime_get_sync(). Otherwise it's not safe to gate the clock, >> since it may already be gated. > > That's fine, but it has nothing to do with pm_runtime_force_suspend(). > > Besides, if the real question is whether or not to gate the clock (or > in other words, has the clock already been gated), why not just store a > "clock_is_gated" flag somewhere? You could do that, but it's easier to not. You will need to update the runtime PM status and disable runtime PM anyway, done by the API. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[CC: list severely trimmed] On Mon, 10 Nov 2014, Ulf Hansson wrote: > >> There are an important advantage of using the pm_runtime_force_suspend() here. > >> > >> For the driver to handle clock gating at system PM suspend, it first > >> needs to bring the device into full power, through > >> pm_runtime_get_sync(). Otherwise it's not safe to gate the clock, > >> since it may already be gated. > > > > That's fine, but it has nothing to do with pm_runtime_force_suspend(). > > > > Besides, if the real question is whether or not to gate the clock (or > > in other words, has the clock already been gated), why not just store a > > "clock_is_gated" flag somewhere? > > You could do that, but it's easier to not. I'm not convinced. And you haven't said how this is related to pm_runtime_force_suspend(). > You will need to update the runtime PM status and disable runtime PM > anyway, done by the API. So what? And what API are you referring to: the runtime PM API or something else? I get the feeling that I'm missing a lot of important points here. What have you left out? As I understand the problem, you've got a bus type where some of the drivers have IRQ-safe runtime PM (and therefore carry out their suspend/resume operations in interrupt context) and others don't. The subsystem core wants to maximize the power saving by deconfiguring some clocks whenever a device is suspended, but this requires a process context and so it can't be done directly when IRQ-safe runtime PM is used. Is that a good summary? If it is, there are ways of dealing with this that don't involve messing around with the runtime PM internals, or using pm_runtime_force_suspend(). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On pon, 2014-11-10 at 13:59 -0500, Alan Stern wrote: > [CC: list severely trimmed] > > On Mon, 10 Nov 2014, Ulf Hansson wrote: > > > >> There are an important advantage of using the pm_runtime_force_suspend() here. > > >> > > >> For the driver to handle clock gating at system PM suspend, it first > > >> needs to bring the device into full power, through > > >> pm_runtime_get_sync(). Otherwise it's not safe to gate the clock, > > >> since it may already be gated. > > > > > > That's fine, but it has nothing to do with pm_runtime_force_suspend(). > > > > > > Besides, if the real question is whether or not to gate the clock (or > > > in other words, has the clock already been gated), why not just store a > > > "clock_is_gated" flag somewhere? > > > > You could do that, but it's easier to not. > > I'm not convinced. And you haven't said how this is related to > pm_runtime_force_suspend(). > > > You will need to update the runtime PM status and disable runtime PM > > anyway, done by the API. > > So what? And what API are you referring to: the runtime PM API or > something else? > > I get the feeling that I'm missing a lot of important points here. > What have you left out? > > As I understand the problem, you've got a bus type where some of the > drivers have IRQ-safe runtime PM (and therefore carry out their > suspend/resume operations in interrupt context) and others don't. The > subsystem core wants to maximize the power saving by deconfiguring some > clocks whenever a device is suspended, but this requires a process > context and so it can't be done directly when IRQ-safe runtime PM is > used. > > Is that a good summary? If it is, there are ways of dealing with this > that don't involve messing around with the runtime PM internals, or > using pm_runtime_force_suspend(). Thank you all for this precious feedback. I'll try the way with skipping runtime PM stack completely during system suspend. Probably that will add some complexity to the amba/bus and pl330 but there shouldn't be any changes to PM core. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 November 2014 19:59, Alan Stern <stern@rowland.harvard.edu> wrote: > [CC: list severely trimmed] > > On Mon, 10 Nov 2014, Ulf Hansson wrote: > >> >> There are an important advantage of using the pm_runtime_force_suspend() here. >> >> >> >> For the driver to handle clock gating at system PM suspend, it first >> >> needs to bring the device into full power, through >> >> pm_runtime_get_sync(). Otherwise it's not safe to gate the clock, >> >> since it may already be gated. >> > >> > That's fine, but it has nothing to do with pm_runtime_force_suspend(). >> > >> > Besides, if the real question is whether or not to gate the clock (or >> > in other words, has the clock already been gated), why not just store a >> > "clock_is_gated" flag somewhere? >> >> You could do that, but it's easier to not. > > I'm not convinced. And you haven't said how this is related to > pm_runtime_force_suspend(). > >> You will need to update the runtime PM status and disable runtime PM >> anyway, done by the API. > > So what? And what API are you referring to: the runtime PM API or > something else? Sorry if I was unclear, I was referring to pm_runtime_force_suspend(). > > I get the feeling that I'm missing a lot of important points here. > What have you left out? First, this patch which we are discussing is about how the amba bus/drivers should cope with irq_safe devices. I think we have already solved that through Rafael's latest suggestion. In principle what you want me to elaborate upon, is why the pm_runtime_force_suspend|resume() helpers is a good choice for AMBA bus/drivers to use, right? Indeed the pm_runtime_force_suspend|resume() helpers was discussed thoroughly before Rafael, decided to merge them. I do now realize that I forgot to add some information about them into the runtime PM documentation, my bad. Will fix it soon. Anyway, this is copied from the function header, which gives you an overall idea why they are good for AMBA. /** * pm_runtime_force_suspend - Force a device into suspend state if needed. * @dev: Device to suspend. * * Disable runtime PM so we safely can check the device's runtime PM status and * if it is active, invoke it's .runtime_suspend callback to bring it into * suspend state. Keep runtime PM disabled to preserve the state unless we * encounter errors. * * Typically this function may be invoked from a system suspend callback to make * sure the device is put into low power state. */ Since AMBA bus/drivers are handling runtime PM resources like clocks/pinctrl/etc from their runtime PM callbacks, using pm_runtime_force_suspen|resume() from their system PM callbacks makes perfect sense to me. > > As I understand the problem, you've got a bus type where some of the > drivers have IRQ-safe runtime PM (and therefore carry out their > suspend/resume operations in interrupt context) and others don't. The > subsystem core wants to maximize the power saving by deconfiguring some > clocks whenever a device is suspended, but this requires a process > context and so it can't be done directly when IRQ-safe runtime PM is > used. > > Is that a good summary? If it is, there are ways of dealing with this Yes. > that don't involve messing around with the runtime PM internals, or > using pm_runtime_force_suspend(). The logic to handle that would in principle mean to keep track of the device's runtime PM status. That doesn't make sense, it's the job of the runtime PM core to do that!? Additionally, you would also need to protect the runtime PM resume callback from doing clock enable during system PM, unless you also disable runtime PM. To me, pm_runtime_force_suspend|resume() will do exactly what we need for the AMBA case. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On ?ro, 2014-11-12 at 11:39 +0100, Ulf Hansson wrote: > On 10 November 2014 19:59, Alan Stern <stern@rowland.harvard.edu> wrote: > > [CC: list severely trimmed] > > > > On Mon, 10 Nov 2014, Ulf Hansson wrote: > > > >> >> There are an important advantage of using the pm_runtime_force_suspend() here. > >> >> > >> >> For the driver to handle clock gating at system PM suspend, it first > >> >> needs to bring the device into full power, through > >> >> pm_runtime_get_sync(). Otherwise it's not safe to gate the clock, > >> >> since it may already be gated. > >> > > >> > That's fine, but it has nothing to do with pm_runtime_force_suspend(). > >> > > >> > Besides, if the real question is whether or not to gate the clock (or > >> > in other words, has the clock already been gated), why not just store a > >> > "clock_is_gated" flag somewhere? > >> > >> You could do that, but it's easier to not. > > > > I'm not convinced. And you haven't said how this is related to > > pm_runtime_force_suspend(). > > > >> You will need to update the runtime PM status and disable runtime PM > >> anyway, done by the API. > > > > So what? And what API are you referring to: the runtime PM API or > > something else? > > Sorry if I was unclear, I was referring to pm_runtime_force_suspend(). > > > > > I get the feeling that I'm missing a lot of important points here. > > What have you left out? > > First, this patch which we are discussing is about how the amba > bus/drivers should cope with irq_safe devices. I think we have already > solved that through Rafael's latest suggestion. > > In principle what you want me to elaborate upon, is why the > pm_runtime_force_suspend|resume() helpers is a good choice for AMBA > bus/drivers to use, right? > > Indeed the pm_runtime_force_suspend|resume() helpers was discussed > thoroughly before Rafael, decided to merge them. I do now realize that > I forgot to add some information about them into the runtime PM > documentation, my bad. Will fix it soon. > > Anyway, this is copied from the function header, which gives you an > overall idea why they are good for AMBA. > > /** > * pm_runtime_force_suspend - Force a device into suspend state if needed. > * @dev: Device to suspend. > * > * Disable runtime PM so we safely can check the device's runtime PM status and > * if it is active, invoke it's .runtime_suspend callback to bring it into > * suspend state. Keep runtime PM disabled to preserve the state unless we > * encounter errors. > * > * Typically this function may be invoked from a system suspend callback to make > * sure the device is put into low power state. > */ > > Since AMBA bus/drivers are handling runtime PM resources like > clocks/pinctrl/etc from their runtime PM callbacks, using > pm_runtime_force_suspen|resume() from their system PM callbacks makes > perfect sense to me. > > > > > As I understand the problem, you've got a bus type where some of the > > drivers have IRQ-safe runtime PM (and therefore carry out their > > suspend/resume operations in interrupt context) and others don't. The > > subsystem core wants to maximize the power saving by deconfiguring some > > clocks whenever a device is suspended, but this requires a process > > context and so it can't be done directly when IRQ-safe runtime PM is > > used. > > > > Is that a good summary? If it is, there are ways of dealing with this > > Yes. > > > that don't involve messing around with the runtime PM internals, or > > using pm_runtime_force_suspend(). > > The logic to handle that would in principle mean to keep track of the > device's runtime PM status. That doesn't make sense, it's the job of > the runtime PM core to do that!? > > Additionally, you would also need to protect the runtime PM resume > callback from doing clock enable during system PM, unless you also > disable runtime PM. > > To me, pm_runtime_force_suspend|resume() will do exactly what we need > for the AMBA case. Hmm... you're saying that runtime resume/suspend could happen during execution of system resume/suspend callback? Or maybe the runtime resume callback could be called after suspending the device but before system sleep finishes? In pl330 I wanted to do something like: static int __maybe_unused pl330_suspend(struct device *dev) { struct amba_device *pcdev = to_amba_device(dev); if (!pm_runtime_status_suspended(dev)) { /* amba did not disable the clock */ amba_pclk_disable(pcdev); } amba_pclk_unprepare(pcdev); return 0; } However I still wonder how this is safe against concurrent runtime PM... Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 12 Nov 2014, Ulf Hansson wrote: > Since AMBA bus/drivers are handling runtime PM resources like > clocks/pinctrl/etc from their runtime PM callbacks, using > pm_runtime_force_suspen|resume() from their system PM callbacks makes > perfect sense to me. This sounds like a case of misdesign. If clocks/pinctrl/etc are all purely _runtime_ PM resources then the _system_ PM callbacks should have nothing to do with them. On the other hand, if they are general PM resources then they should be handled from a general-purpose routine that can be invoked by both the runtime-PM and system-PM callbacks. It makes very little sense to have a special-purpose routine manage a general-purpose resource. > > As I understand the problem, you've got a bus type where some of the > > drivers have IRQ-safe runtime PM (and therefore carry out their > > suspend/resume operations in interrupt context) and others don't. The > > subsystem core wants to maximize the power saving by deconfiguring some > > clocks whenever a device is suspended, but this requires a process > > context and so it can't be done directly when IRQ-safe runtime PM is > > used. > > > > Is that a good summary? If it is, there are ways of dealing with this > > Yes. > > > that don't involve messing around with the runtime PM internals, or > > using pm_runtime_force_suspend(). > > The logic to handle that would in principle mean to keep track of the > device's runtime PM status. That doesn't make sense, it's the job of > the runtime PM core to do that!? Why do think we have pm_runtime_suspended() and related routines? They are there so that your code can make decisions based on the runtime PM status without having to keep track of it. > Additionally, you would also need to protect the runtime PM resume > callback from doing clock enable during system PM, unless you also > disable runtime PM. > > To me, pm_runtime_force_suspend|resume() will do exactly what we need > for the AMBA case. Your description leaves out large parts of the design. Please list the relevant resources, together with an explanation of exactly how each one is handled by the system- and runtime-PM callback routines, in your scheme. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 November 2014 17:34, Alan Stern <stern@rowland.harvard.edu> wrote: > On Wed, 12 Nov 2014, Ulf Hansson wrote: > >> Since AMBA bus/drivers are handling runtime PM resources like >> clocks/pinctrl/etc from their runtime PM callbacks, using >> pm_runtime_force_suspen|resume() from their system PM callbacks makes >> perfect sense to me. > > This sounds like a case of misdesign. If clocks/pinctrl/etc are all > purely _runtime_ PM resources then the _system_ PM callbacks should > have nothing to do with them. On the other hand, if they are general > PM resources then they should be handled from a general-purpose routine > that can be invoked by both the runtime-PM and system-PM callbacks. > > It makes very little sense to have a special-purpose routine manage a > general-purpose resource. I don't agree and thought you also were of the opposite opinion. :-) If I remember correctly, it was you who came up with the really brilliant idea to reuse the runtime PM callbacks for this matter!? From that, we ended up in adding the SET_PM_RUNTIME_PM_OPS macro. > >> > As I understand the problem, you've got a bus type where some of the >> > drivers have IRQ-safe runtime PM (and therefore carry out their >> > suspend/resume operations in interrupt context) and others don't. The >> > subsystem core wants to maximize the power saving by deconfiguring some >> > clocks whenever a device is suspended, but this requires a process >> > context and so it can't be done directly when IRQ-safe runtime PM is >> > used. >> > >> > Is that a good summary? If it is, there are ways of dealing with this >> >> Yes. >> >> > that don't involve messing around with the runtime PM internals, or >> > using pm_runtime_force_suspend(). >> >> The logic to handle that would in principle mean to keep track of the >> device's runtime PM status. That doesn't make sense, it's the job of >> the runtime PM core to do that!? > > Why do think we have pm_runtime_suspended() and related routines? They > are there so that your code can make decisions based on the runtime PM > status without having to keep track of it. Those runtime PM routines your refer to, are exactly what the pm_runtime_force_suspend|resume() helpers make use of. It seems like a bad idea to duplicate similar code into each an every bus/driver to handle the same issue. If we shouldn't use pm_runtime_force_suspend|resume() helpers for these cases, when do you think it would be appropriate to use them? > >> Additionally, you would also need to protect the runtime PM resume >> callback from doing clock enable during system PM, unless you also >> disable runtime PM. >> >> To me, pm_runtime_force_suspend|resume() will do exactly what we need >> for the AMBA case. > > Your description leaves out large parts of the design. Please list the > relevant resources, together with an explanation of exactly how each > one is handled by the system- and runtime-PM callback routines, in your > scheme. The runtime PM resources depends on what AMBA device/driver is being used, typically clocks and pinctrls. Simplified described, these bus/drivers PM support are designed in a runtime PM centric manner. It means at system PM suspend, the bus/driver wants to make sure the runtime PM resources are in a runtime PM suspend state. Does that answer your question? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 13 Nov 2014, Ulf Hansson wrote: > The runtime PM resources depends on what AMBA device/driver is being > used, typically clocks and pinctrls. > > Simplified described, these bus/drivers PM support are designed in a > runtime PM centric manner. It means at system PM suspend, the > bus/driver wants to make sure the runtime PM resources are in a > runtime PM suspend state. > > Does that answer your question? Yes, pretty much. It sounds like pm_runtime_force_suspend does do want you want, provided you don't have to worry about wakeup settings. And I guess if you're going to take this runtime-PM-centric approach, and if you're also going to rely on some drivers being IRQ-safe and others not, then the irq_safe flag needs to exist even when runtime PM is not configured. It still feels like a backward approach, not the way I would have done it. But never mind... Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" 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/include/linux/pm.h b/include/linux/pm.h index 383fd68aaee1..b05fa954f50d 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -566,6 +566,7 @@ struct dev_pm_info { bool ignore_children:1; bool early_init:1; /* Owned by the PM core */ bool direct_complete:1; /* Owned by the PM core */ + unsigned int irq_safe:1; /* PM runtime */ spinlock_t lock; #ifdef CONFIG_PM_SLEEP struct list_head entry; @@ -590,7 +591,6 @@ struct dev_pm_info { unsigned int run_wake:1; unsigned int runtime_auto:1; unsigned int no_callbacks:1; - unsigned int irq_safe:1; unsigned int use_autosuspend:1; unsigned int timer_autosuspends:1; unsigned int memalloc_noio:1; diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index 367f49b9a1c9..d94a65662a60 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -166,7 +166,10 @@ static inline bool pm_runtime_suspended_if_enabled(struct device *dev) { return static inline bool pm_runtime_enabled(struct device *dev) { return false; } static inline void pm_runtime_no_callbacks(struct device *dev) {} -static inline void pm_runtime_irq_safe(struct device *dev) {} +static inline void pm_runtime_irq_safe(struct device *dev) +{ + dev->power.irq_safe = 1; +} static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; } static inline void pm_runtime_mark_last_busy(struct device *dev) {}
Some drivers (e.g. bus drivers) may want to check if power.irq_safe was called by child driver, regardless of CONFIG_PM_RUNTIME. An example scenario is amba/bus.c and dma/pl330.c drivers. The runtime suspend/resume callbacks in amba bus driver act differently if irq_safe was set by child driver (in irq_safe mode bus clock is only disabled). The pl330 driver sets irq_safe and assumes that amba bus driver will only disable the clock in runtime PM. So in system sleep suspend callback the pl330 driver unprepares the clock after calling pm_runtime_force_suspend(). However inconsistency would appear if CONFIG_PM_RUNTIME is not set and child drivers do not want the irq_safe runtime PM. In such case amba bus driver still has to know whether child driver wanted irq_safe - by looking at dev->power.irq_safe field. Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- include/linux/pm.h | 2 +- include/linux/pm_runtime.h | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-)