| Message ID | 1413795888-18559-4-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote: > The AMBA bus driver defines runtime Power Management functions which > disable and unprepare AMBA bus clock. This is problematic for runtime PM > because unpreparing a clock might sleep so it is not interrupt safe. > > However some drivers may want to implement runtime PM functions in > interrupt-safe way (see pm_runtime_irq_safe()). In such case the AMBA > bus driver should only disable/enable the clock in runtime suspend and > resume callbacks. > > Detect the device driver behavior after calling its probe function and > store it. During runtime suspend/resume deal with clocks according to > stored value. > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/amba/bus.c | 29 +++++++++++++++++++++++++---- > include/linux/amba/bus.h | 1 + > 2 files changed, 26 insertions(+), 4 deletions(-) > > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c > index 47bbdc1b5be3..474434e1b1b9 100644 > --- a/drivers/amba/bus.c > +++ b/drivers/amba/bus.c > @@ -95,8 +95,18 @@ static int amba_pm_runtime_suspend(struct device *dev) > struct amba_device *pcdev = to_amba_device(dev); > int ret = pm_generic_runtime_suspend(dev); > > - if (ret == 0 && dev->driver) > - clk_disable_unprepare(pcdev->pclk); > + if (ret == 0 && dev->driver) { > + /* > + * Drivers should not change pm_runtime_irq_safe() > + * after probe. > + */ > + WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev)); > + > + if (pcdev->irq_safe) > + clk_disable(pcdev->pclk); > + else > + clk_disable_unprepare(pcdev->pclk); > + } > > return ret; > } > @@ -107,7 +117,16 @@ static int amba_pm_runtime_resume(struct device *dev) > int ret; > > if (dev->driver) { > - ret = clk_prepare_enable(pcdev->pclk); > + /* > + * Drivers should not change pm_runtime_irq_safe() > + * after probe. > + */ > + WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev)); > + > + if (pcdev->irq_safe) > + ret = clk_enable(pcdev->pclk); > + else > + ret = clk_prepare_enable(pcdev->pclk); > /* Failure is probably fatal to the system, but... */ > if (ret) > return ret; > @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev) > pm_runtime_enable(dev); > > ret = pcdrv->probe(pcdev, id); > - if (ret == 0) > + if (ret == 0) { > + pcdev->irq_safe = pm_runtime_is_irq_safe(dev); This looks racy. Is it guaranteed that runtime PM callbacks won't be run for the device after pcdrv->probe() has returned and before setting pcdev->irq_safe? If not, inconsistent behavior may ensue. > break; > + } > > pm_runtime_disable(dev); > pm_runtime_set_suspended(dev); > diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h > index ac02f9bd63dc..c4bae79851fb 100644 > --- a/include/linux/amba/bus.h > +++ b/include/linux/amba/bus.h > @@ -32,6 +32,7 @@ struct amba_device { > struct clk *pclk; > unsigned int periphid; > unsigned int irq[AMBA_NR_IRQS]; > + unsigned int irq_safe:1; > }; > > struct amba_driver { >
On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote: > On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote: > > @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev) > > pm_runtime_enable(dev); > > > > ret = pcdrv->probe(pcdev, id); > > - if (ret == 0) > > + if (ret == 0) { > > + pcdev->irq_safe = pm_runtime_is_irq_safe(dev); > > This looks racy. > > Is it guaranteed that runtime PM callbacks won't be run for the device > after pcdrv->probe() has returned and before setting pcdev->irq_safe? > If not, inconsistent behavior may ensue. You are absolutely correct. So that knocks that idea on its head.
On Sat, Nov 01, 2014 at 12:55:14AM +0000, Russell King - ARM Linux wrote: > On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote: > > On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote: > > > @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev) > > > pm_runtime_enable(dev); > > > > > > ret = pcdrv->probe(pcdev, id); > > > - if (ret == 0) > > > + if (ret == 0) { > > > + pcdev->irq_safe = pm_runtime_is_irq_safe(dev); > > > > This looks racy. > > > > Is it guaranteed that runtime PM callbacks won't be run for the device > > after pcdrv->probe() has returned and before setting pcdev->irq_safe? > > If not, inconsistent behavior may ensue. > > You are absolutely correct. So that knocks that idea on its head. Actually, I think we shouldn't give up hope here. Currently, we do this: pm_runtime_get_noresume(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); ret = pcdrv->probe(pcdev, id); What we could do is: pm_runtime_get_noresume(dev); pm_runtime_get_noresume(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); ret = pcdrv->probe(pcdev, id); if (ret == 0) { pcdev->irq_safe = pm_runtime_is_irq_safe(dev); pm_runtime_put(dev); break; } pm_runtime_disable(dev); pm_runtime_set_suspended(dev); pm_runtime_put_noidle(dev); pm_runtime_put_noidle(dev); which would ensure that we hold a usecount until after the probe function has returned. Would that work? I'll give you that it's pretty horrid. Would another possible solution be to remember the irq-safeness in the suspend handler, and use that in the resume handler? Resume should /always/ undo what the suspend handler previously did wrt clk API stuff.
On sob, 2014-11-01 at 01:01 +0000, Russell King - ARM Linux wrote: > On Sat, Nov 01, 2014 at 12:55:14AM +0000, Russell King - ARM Linux wrote: > > On Sat, Nov 01, 2014 at 01:45:47AM +0100, Rafael J. Wysocki wrote: > > > On Monday, October 20, 2014 11:04:46 AM Krzysztof Kozlowski wrote: > > > > @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev) > > > > pm_runtime_enable(dev); > > > > > > > > ret = pcdrv->probe(pcdev, id); > > > > - if (ret == 0) > > > > + if (ret == 0) { > > > > + pcdev->irq_safe = pm_runtime_is_irq_safe(dev); > > > > > > This looks racy. > > > > > > Is it guaranteed that runtime PM callbacks won't be run for the device > > > after pcdrv->probe() has returned and before setting pcdev->irq_safe? > > > If not, inconsistent behavior may ensue. > > > > You are absolutely correct. So that knocks that idea on its head. > > Actually, I think we shouldn't give up hope here. Currently, we do this: > > pm_runtime_get_noresume(dev); > pm_runtime_set_active(dev); > pm_runtime_enable(dev); > > ret = pcdrv->probe(pcdev, id); > > What we could do is: > > pm_runtime_get_noresume(dev); > pm_runtime_get_noresume(dev); > pm_runtime_set_active(dev); > pm_runtime_enable(dev); > > ret = pcdrv->probe(pcdev, id); > if (ret == 0) { > pcdev->irq_safe = pm_runtime_is_irq_safe(dev); > pm_runtime_put(dev); > break; > } > > pm_runtime_disable(dev); > pm_runtime_set_suspended(dev); > pm_runtime_put_noidle(dev); > pm_runtime_put_noidle(dev); > > which would ensure that we hold a usecount until after the probe function > has returned. Would that work? > > I'll give you that it's pretty horrid. > Would another possible solution be to remember the irq-safeness in the > suspend handler, and use that in the resume handler? Resume should > /always/ undo what the suspend handler previously did wrt clk API stuff. I think the second solution could be more readable. The WARN_ON wouldn't be needed. However this won't solve the two dual nature of runtime callbacks. I wondered also about removing runtime PM callbacks from amba/bus.c completely and moving this to child drivers. This way runtime PM would be obvious in each driver case. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 03, 2014 at 09:36:45AM +0100, Krzysztof Kozlowski wrote: > On sob, 2014-11-01 at 01:01 +0000, Russell King - ARM Linux wrote: > > Would another possible solution be to remember the irq-safeness in the > > suspend handler, and use that in the resume handler? Resume should > > /always/ undo what the suspend handler previously did wrt clk API stuff. > > I think the second solution could be more readable. The WARN_ON wouldn't > be needed. However this won't solve the two dual nature of runtime > callbacks. > > I wondered also about removing runtime PM callbacks from amba/bus.c > completely and moving this to child drivers. This way runtime PM would > be obvious in each driver case. That makes it pretty horrid from the point of view of having bus management code, because we now have the management of the bus clock split between the bus layer and the device driver. This is /really/ a problem for runtime PM. Runtime PM permits there to be a bus layer involved - and runtime PM can also be coupled up to PM domains as well. For all this stuff, the context which the callbacks are called in depends on whether the driver itself has marked the device as having IRQ-safe callbacks. That's fine, but the bus and PM domain level code then /really/ needs to know what context they're being called in, so they know whether they can sleep or not, or they must to be written to always use non-sleeping functions so they work in both contexts. If we assume the former, then that implies that the irq-safe flag must never change state between a suspend and a resume.
On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: > That makes it pretty horrid from the point of view of having bus > management code, because we now have the management of the bus clock > split between the bus layer and the device driver. > > This is /really/ a problem for runtime PM. Runtime PM permits there > to be a bus layer involved - and runtime PM can also be coupled up > to PM domains as well. For all this stuff, the context which the > callbacks are called in depends on whether the driver itself has > marked the device as having IRQ-safe callbacks. > > That's fine, but the bus and PM domain level code then /really/ needs > to know what context they're being called in, so they know whether > they can sleep or not, or they must to be written to always use > non-sleeping functions so they work in both contexts. If we assume > the former, then that implies that the irq-safe flag must never change > state between a suspend and a resume. If a bus subsystem or PM domain is going to allow its drivers to choose between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the subsystem to come up with a way for drivers to indicate their choice. I tend to agree with Rafael that testing dev->power.irq_safe should be good enough, with no real need for a wrapper. But the subsystem can use a different mechanism if it wants. Bear in mind, however, that once the irq_safe flag has been set, the runtime PM core offers no way to turn it off again. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 03, 2014 at 10:41:02AM -0500, Alan Stern wrote: > Bear in mind, however, that once the irq_safe flag has been set, the > runtime PM core offers no way to turn it off again. Ah, I thought it did permit it to change both ways. In that case, we don't need to validate that it doesn't change state on each call, and we can just get away with checking its value.
Russell King - ARM Linux <linux@arm.linux.org.uk> writes: [...] > Would another possible solution be to remember the irq-safeness in the > suspend handler, and use that in the resume handler? Resume should > /always/ undo what the suspend handler previously did wrt clk API stuff. This seems more reasonable to me. Currently, the setting of irq_safe-ness is permanent (there's no exposed API to unset it after you set it.) so assuming it to be the same across suspend resume is a safe assumption. Kevin -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: > On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: > > > That makes it pretty horrid from the point of view of having bus > > management code, because we now have the management of the bus clock > > split between the bus layer and the device driver. > > > > This is /really/ a problem for runtime PM. Runtime PM permits there > > to be a bus layer involved - and runtime PM can also be coupled up > > to PM domains as well. For all this stuff, the context which the > > callbacks are called in depends on whether the driver itself has > > marked the device as having IRQ-safe callbacks. > > > > That's fine, but the bus and PM domain level code then /really/ needs > > to know what context they're being called in, so they know whether > > they can sleep or not, or they must to be written to always use > > non-sleeping functions so they work in both contexts. If we assume > > the former, then that implies that the irq-safe flag must never change > > state between a suspend and a resume. > > If a bus subsystem or PM domain is going to allow its drivers to choose > between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the > subsystem to come up with a way for drivers to indicate their choice. > > I tend to agree with Rafael that testing dev->power.irq_safe should be > good enough, with no real need for a wrapper. But the subsystem can > use a different mechanism if it wants. > > Bear in mind, however, that once the irq_safe flag has been set, the > runtime PM core offers no way to turn it off again. There is a problem with it, though. Say, a driver handles a device that may or may not be in a power domain. Or in other words, the power domain the device is in may or may not be always on. If the domain is always on, the runtime PM callbacks are IRQ-safe (they depend on the driver only). If it isn't, they may not be IRQ-safe. How's the driver going to decide whether or not to set power.irq_safe? Rafael -- To unsubscribe from this list: send the line "unsubscribe dmaengine" 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-03 at 15:44 +0000, Russell King - ARM Linux wrote: > On Mon, Nov 03, 2014 at 10:41:02AM -0500, Alan Stern wrote: > > Bear in mind, however, that once the irq_safe flag has been set, the > > runtime PM core offers no way to turn it off again. > > Ah, I thought it did permit it to change both ways. In that case, we > don't need to validate that it doesn't change state on each call, and > we can just get away with checking its value. It cannot be unset but still it could be *set* during runtime (not only in probe). However that shouldn't happen between suspend-resume calls, so the solution of undoing suspend's work seems fine. I'll send a new patch doing this way. Without the wrapper. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On wto, 2014-11-04 at 02:57 +0100, Rafael J. Wysocki wrote: > On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: > > On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: > > > > > That makes it pretty horrid from the point of view of having bus > > > management code, because we now have the management of the bus clock > > > split between the bus layer and the device driver. > > > > > > This is /really/ a problem for runtime PM. Runtime PM permits there > > > to be a bus layer involved - and runtime PM can also be coupled up > > > to PM domains as well. For all this stuff, the context which the > > > callbacks are called in depends on whether the driver itself has > > > marked the device as having IRQ-safe callbacks. > > > > > > That's fine, but the bus and PM domain level code then /really/ needs > > > to know what context they're being called in, so they know whether > > > they can sleep or not, or they must to be written to always use > > > non-sleeping functions so they work in both contexts. If we assume > > > the former, then that implies that the irq-safe flag must never change > > > state between a suspend and a resume. > > > > If a bus subsystem or PM domain is going to allow its drivers to choose > > between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the > > subsystem to come up with a way for drivers to indicate their choice. > > > > I tend to agree with Rafael that testing dev->power.irq_safe should be > > good enough, with no real need for a wrapper. But the subsystem can > > use a different mechanism if it wants. > > > > Bear in mind, however, that once the irq_safe flag has been set, the > > runtime PM core offers no way to turn it off again. > > There is a problem with it, though. Say, a driver handles a device that > may or may not be in a power domain. Or in other words, the power domain > the device is in may or may not be always on. If the domain is always on, > the runtime PM callbacks are IRQ-safe (they depend on the driver only). > If it isn't, they may not be IRQ-safe. How's the driver going to decide > whether or not to set power.irq_safe? The pl330 driver (being a part of amba bus) always wants IRQ safe callbacks. However other amba drivers may not. So actually the driver always sets one or another. The dualism is present only in the amba/bus.c driver which encapsulates the child drivers. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4 November 2014 02:57, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: >> On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: >> >> > That makes it pretty horrid from the point of view of having bus >> > management code, because we now have the management of the bus clock >> > split between the bus layer and the device driver. >> > >> > This is /really/ a problem for runtime PM. Runtime PM permits there >> > to be a bus layer involved - and runtime PM can also be coupled up >> > to PM domains as well. For all this stuff, the context which the >> > callbacks are called in depends on whether the driver itself has >> > marked the device as having IRQ-safe callbacks. >> > >> > That's fine, but the bus and PM domain level code then /really/ needs >> > to know what context they're being called in, so they know whether >> > they can sleep or not, or they must to be written to always use >> > non-sleeping functions so they work in both contexts. If we assume >> > the former, then that implies that the irq-safe flag must never change >> > state between a suspend and a resume. >> >> If a bus subsystem or PM domain is going to allow its drivers to choose >> between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the >> subsystem to come up with a way for drivers to indicate their choice. >> >> I tend to agree with Rafael that testing dev->power.irq_safe should be >> good enough, with no real need for a wrapper. But the subsystem can >> use a different mechanism if it wants. >> >> Bear in mind, however, that once the irq_safe flag has been set, the >> runtime PM core offers no way to turn it off again. > > There is a problem with it, though. Say, a driver handles a device that > may or may not be in a power domain. Or in other words, the power domain > the device is in may or may not be always on. If the domain is always on, > the runtime PM callbacks are IRQ-safe (they depend on the driver only). > If it isn't, they may not be IRQ-safe. How's the driver going to decide > whether or not to set power.irq_safe? From my point of view; the decision whether the driver will set the IRQ safe flag is in principle a software design choice. Currently genpd isn't able to power off, if one of its devices are IRQ safe configured. That's a limitation in genpd which we need to fix and it's on my TODO list. My point is thus, I don't think the driver should care about PM domains at all regarding using the IRQ safe option. Does that make sense? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, November 04, 2014 10:11:35 AM Ulf Hansson wrote: > On 4 November 2014 02:57, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: > >> On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: > >> > >> > That makes it pretty horrid from the point of view of having bus > >> > management code, because we now have the management of the bus clock > >> > split between the bus layer and the device driver. > >> > > >> > This is /really/ a problem for runtime PM. Runtime PM permits there > >> > to be a bus layer involved - and runtime PM can also be coupled up > >> > to PM domains as well. For all this stuff, the context which the > >> > callbacks are called in depends on whether the driver itself has > >> > marked the device as having IRQ-safe callbacks. > >> > > >> > That's fine, but the bus and PM domain level code then /really/ needs > >> > to know what context they're being called in, so they know whether > >> > they can sleep or not, or they must to be written to always use > >> > non-sleeping functions so they work in both contexts. If we assume > >> > the former, then that implies that the irq-safe flag must never change > >> > state between a suspend and a resume. > >> > >> If a bus subsystem or PM domain is going to allow its drivers to choose > >> between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the > >> subsystem to come up with a way for drivers to indicate their choice. > >> > >> I tend to agree with Rafael that testing dev->power.irq_safe should be > >> good enough, with no real need for a wrapper. But the subsystem can > >> use a different mechanism if it wants. > >> > >> Bear in mind, however, that once the irq_safe flag has been set, the > >> runtime PM core offers no way to turn it off again. > > > > There is a problem with it, though. Say, a driver handles a device that > > may or may not be in a power domain. Or in other words, the power domain > > the device is in may or may not be always on. If the domain is always on, > > the runtime PM callbacks are IRQ-safe (they depend on the driver only). > > If it isn't, they may not be IRQ-safe. How's the driver going to decide > > whether or not to set power.irq_safe? > > From my point of view; the decision whether the driver will set the > IRQ safe flag is in principle a software design choice. > > Currently genpd isn't able to power off, if one of its devices are IRQ > safe configured. That's a limitation in genpd which we need to fix and > it's on my TODO list. > > My point is thus, I don't think the driver should care about PM > domains at all regarding using the IRQ safe option. Does that make > sense? Yes, it does, and that's the heart of the problem above. The driver should not care about wherther or not the device is in a power domain, but it needs to know that when deciding whether or not to set power.irq_safe. Catch 22.
On 4 November 2014 14:59, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Tuesday, November 04, 2014 10:11:35 AM Ulf Hansson wrote: >> On 4 November 2014 02:57, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> > On Monday, November 03, 2014 10:41:02 AM Alan Stern wrote: >> >> On Mon, 3 Nov 2014, Russell King - ARM Linux wrote: >> >> >> >> > That makes it pretty horrid from the point of view of having bus >> >> > management code, because we now have the management of the bus clock >> >> > split between the bus layer and the device driver. >> >> > >> >> > This is /really/ a problem for runtime PM. Runtime PM permits there >> >> > to be a bus layer involved - and runtime PM can also be coupled up >> >> > to PM domains as well. For all this stuff, the context which the >> >> > callbacks are called in depends on whether the driver itself has >> >> > marked the device as having IRQ-safe callbacks. >> >> > >> >> > That's fine, but the bus and PM domain level code then /really/ needs >> >> > to know what context they're being called in, so they know whether >> >> > they can sleep or not, or they must to be written to always use >> >> > non-sleeping functions so they work in both contexts. If we assume >> >> > the former, then that implies that the irq-safe flag must never change >> >> > state between a suspend and a resume. >> >> >> >> If a bus subsystem or PM domain is going to allow its drivers to choose >> >> between IRQ-safe and non-IRQ-safe runtime PM, then it is up to the >> >> subsystem to come up with a way for drivers to indicate their choice. >> >> >> >> I tend to agree with Rafael that testing dev->power.irq_safe should be >> >> good enough, with no real need for a wrapper. But the subsystem can >> >> use a different mechanism if it wants. >> >> >> >> Bear in mind, however, that once the irq_safe flag has been set, the >> >> runtime PM core offers no way to turn it off again. >> > >> > There is a problem with it, though. Say, a driver handles a device that >> > may or may not be in a power domain. Or in other words, the power domain >> > the device is in may or may not be always on. If the domain is always on, >> > the runtime PM callbacks are IRQ-safe (they depend on the driver only). >> > If it isn't, they may not be IRQ-safe. How's the driver going to decide >> > whether or not to set power.irq_safe? >> >> From my point of view; the decision whether the driver will set the >> IRQ safe flag is in principle a software design choice. >> >> Currently genpd isn't able to power off, if one of its devices are IRQ >> safe configured. That's a limitation in genpd which we need to fix and >> it's on my TODO list. >> >> My point is thus, I don't think the driver should care about PM >> domains at all regarding using the IRQ safe option. Does that make >> sense? > > Yes, it does, and that's the heart of the problem above. The driver should > not care about wherther or not the device is in a power domain, but it needs > to know that when deciding whether or not to set power.irq_safe. Catch 22. Why is it catch22? The problem is supposed to be fixed in the generic PM domain. How we do that is a different question. Until genpd get fixed, the driver can still keep using irq_safe if they want to. It will only lead to limitations if the device is attached to a genpd. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe dmaengine" 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/amba/bus.c b/drivers/amba/bus.c index 47bbdc1b5be3..474434e1b1b9 100644 --- a/drivers/amba/bus.c +++ b/drivers/amba/bus.c @@ -95,8 +95,18 @@ static int amba_pm_runtime_suspend(struct device *dev) struct amba_device *pcdev = to_amba_device(dev); int ret = pm_generic_runtime_suspend(dev); - if (ret == 0 && dev->driver) - clk_disable_unprepare(pcdev->pclk); + if (ret == 0 && dev->driver) { + /* + * Drivers should not change pm_runtime_irq_safe() + * after probe. + */ + WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev)); + + if (pcdev->irq_safe) + clk_disable(pcdev->pclk); + else + clk_disable_unprepare(pcdev->pclk); + } return ret; } @@ -107,7 +117,16 @@ static int amba_pm_runtime_resume(struct device *dev) int ret; if (dev->driver) { - ret = clk_prepare_enable(pcdev->pclk); + /* + * Drivers should not change pm_runtime_irq_safe() + * after probe. + */ + WARN_ON(pcdev->irq_safe != pm_runtime_is_irq_safe(dev)); + + if (pcdev->irq_safe) + ret = clk_enable(pcdev->pclk); + else + ret = clk_prepare_enable(pcdev->pclk); /* Failure is probably fatal to the system, but... */ if (ret) return ret; @@ -198,8 +217,10 @@ static int amba_probe(struct device *dev) pm_runtime_enable(dev); ret = pcdrv->probe(pcdev, id); - if (ret == 0) + if (ret == 0) { + pcdev->irq_safe = pm_runtime_is_irq_safe(dev); break; + } pm_runtime_disable(dev); pm_runtime_set_suspended(dev); diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h index ac02f9bd63dc..c4bae79851fb 100644 --- a/include/linux/amba/bus.h +++ b/include/linux/amba/bus.h @@ -32,6 +32,7 @@ struct amba_device { struct clk *pclk; unsigned int periphid; unsigned int irq[AMBA_NR_IRQS]; + unsigned int irq_safe:1; }; struct amba_driver {