| Message ID | 1416237550-31092-1-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 17, 2014 at 04:19:10PM +0100, Ulf Hansson wrote: > The amba bus, amba drivers and a vast amount of platform drivers which > enables runtime PM, don't invoke a pm_runtime_get_sync() while probing > their devices. > > Instead, once they have turned on their PM resourses during ->probe() > and are ready to handle I/O, these invokes pm_runtime_set_active() to > synchronize its state towards the runtime PM core. The above is misleading for amba. The code sequence is: pm_runtime_get_noresume(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); ret = pcdrv->probe(pcdev, id); AMBA drivers should never call pm_runtime_set_active(), as the runtime PM state has already been initialised by the bus code. Platform drivers are different; the platform code provides zero help for runtime PM. The sequence used by AMBA bus code is the sequence which was used by PCI (as per commit f3ec4f87d607) at the time the runtime PM support was written for AMBA. PCI assumes that unbound devices are already powered up, and as far as I'm aware, that's also true of AMBA devices as well. I have yet to have access to a platform where this isn't true, neither has anyone reported that such a platform exists.
Hi Russell, On 11/17/2014 05:32 PM, Russell King - ARM Linux wrote: > On Mon, Nov 17, 2014 at 04:19:10PM +0100, Ulf Hansson wrote: >> The amba bus, amba drivers and a vast amount of platform drivers which >> enables runtime PM, don't invoke a pm_runtime_get_sync() while probing >> their devices. >> >> Instead, once they have turned on their PM resourses during ->probe() >> and are ready to handle I/O, these invokes pm_runtime_set_active() to >> synchronize its state towards the runtime PM core. > > The above is misleading for amba. The code sequence is: > > pm_runtime_get_noresume(dev); > pm_runtime_set_active(dev); > pm_runtime_enable(dev); > > ret = pcdrv->probe(pcdev, id); > > AMBA drivers should never call pm_runtime_set_active(), as the runtime PM > state has already been initialised by the bus code. Platform drivers are > different; the platform code provides zero help for runtime PM. > > The sequence used by AMBA bus code is the sequence which was used by PCI > (as per commit f3ec4f87d607) at the time the runtime PM support was > written for AMBA. PCI assumes that unbound devices are already powered > up, and as far as I'm aware, that's also true of AMBA devices as well. > I have yet to have access to a platform where this isn't true, neither > has anyone reported that such a platform exists. > I'd be very appreciated if you would be able to clarify one point to me as I'm not familiar with amba hw? I've found at least 2 AMBA drivers where secondary clock is used to enable/disable device in addition to "apb_pclk": - drivers/mmc/host/mmci.c - drivers/spi/spi-pl022.c So, form the code point of view, the assumption that "unbound AMBA devices are already powered up" is not always true. And "apb_pclk" is just an interface clock in such cases. From another side above statement is true for mailbox/pl320-ipc.c which seems like is simple device. Am I wrong? Thanks in advance. regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 17, 2014 at 06:54:44PM +0200, Grygorii Strashko wrote: > I'd be very appreciated if you would be able to clarify one point to me as > I'm not familiar with amba hw? > > I've found at least 2 AMBA drivers where secondary clock is used > to enable/disable device in addition to "apb_pclk": > - drivers/mmc/host/mmci.c > - drivers/spi/spi-pl022.c > So, form the code point of view, the assumption that "unbound AMBA > devices are already powered up" is not always true. And "apb_pclk" > is just an interface clock in such cases. Right, what we're talking about here is that the device is accessible to the CPU - that it is powered up and the bus clock to it is running, so that accesses to the device will not cause a bus fault. It does not mean that the external interface to the outside world is functional - for example, there's no suggestion that the clock to the SD/MMC card attached to MMCI would be running, or that the SD/MMC card itself would be powered up, and the same goes for the clocks supplied as functional clocks. The driver is expected to manage these clocks (see below.) How this all works in the AMBA context is: - we ensure that the apb_pclk is running by getting it, preparing and enabling it. - the bus code calls pm_runtime_get_noresume() which increments the usage counter without calling the (as yet not bound) driver. - if a PM domain is attached to the device, it should already be active and powered up. - the device is marked active (as it should now be powered and bus-clocked.) - runtime PM is enabled. At this point, the device must be accessible to the CPU. - the driver probe function is called, and it can go around getting any additional clocks it needs, and enabling or disabling them as it requires. (For example, in the case of MMCI, a card may not be inserted in the slot, so the driver may decide to keep the functional MMCICLK disabled.) - if the driver wishes to take part in runtime PM, it calls pm_runtime_put() or an equivalent function. This balances the pm_runtime_get_noresume() above, and allows the runtime PM infrastructure to consider the device idle, and a candidate for runtime suspend. Should the device runtime suspend, it's up to the driver to deal with the functional clocks it is managing, because only it knows whether it kept those clocks running, and disabling an already disabled clock is not permitted.
Ulf Hansson <ulf.hansson@linaro.org> writes: > The amba bus, amba drivers and a vast amount of platform drivers which > enables runtime PM, don't invoke a pm_runtime_get_sync() while probing > their devices. > > Instead, once they have turned on their PM resourses during ->probe() > and are ready to handle I/O, these invokes pm_runtime_set_active() to > synchronize its state towards the runtime PM core. > > From a runtime PM point of view this behavior is perfectly acceptable, In the context of PM domains that can be dynamically powered on/off, I'm not so sure it's perfectly acceptable anymore. Why doesn't the bus do a _get_sync() instead of a _get_noresume() followed by a _set_active(). By using the _get_noresume() you're bypassing the paths that would bring up your PM domain. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 17 Nov 2014, Kevin Hilman wrote: > Ulf Hansson <ulf.hansson@linaro.org> writes: > > > The amba bus, amba drivers and a vast amount of platform drivers which > > enables runtime PM, don't invoke a pm_runtime_get_sync() while probing > > their devices. > > > > Instead, once they have turned on their PM resourses during ->probe() > > and are ready to handle I/O, these invokes pm_runtime_set_active() to > > synchronize its state towards the runtime PM core. > > > > From a runtime PM point of view this behavior is perfectly acceptable, > > In the context of PM domains that can be dynamically powered on/off, I'm > not so sure it's perfectly acceptable anymore. > > Why doesn't the bus do a _get_sync() instead of a _get_noresume() > followed by a _set_active(). > > By using the _get_noresume() you're bypassing the paths that would bring > up your PM domain. This probably comes about because devices that are part of a power domain need special treatment. Before the driver's probe routine gets called, the bus ought to resume the entire power domain. For that, some sort of _get_sync() is needed. For devices that aren't part of a power domain, things are simpler. The bus does _get_noresume() to make sure the device won't be runtime suspended while the probe routine is running. It doesn't do _get_sync(), because that would end up calling the driver's runtime_resume routine before the driver was bound to the device. (The bus could prevent that from happening by taking special precautions, like PCI does, but in general it's a nuisance.) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 17, 2014 at 11:06 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 17 Nov 2014, Kevin Hilman wrote: > >> Ulf Hansson <ulf.hansson@linaro.org> writes: >> >> > The amba bus, amba drivers and a vast amount of platform drivers which >> > enables runtime PM, don't invoke a pm_runtime_get_sync() while probing >> > their devices. >> > >> > Instead, once they have turned on their PM resourses during ->probe() >> > and are ready to handle I/O, these invokes pm_runtime_set_active() to >> > synchronize its state towards the runtime PM core. >> > >> > From a runtime PM point of view this behavior is perfectly acceptable, >> >> In the context of PM domains that can be dynamically powered on/off, I'm >> not so sure it's perfectly acceptable anymore. >> >> Why doesn't the bus do a _get_sync() instead of a _get_noresume() >> followed by a _set_active(). >> >> By using the _get_noresume() you're bypassing the paths that would bring >> up your PM domain. > > This probably comes about because devices that are part of a power > domain need special treatment. Before the driver's probe routine gets > called, the bus ought to resume the entire power domain. For that, > some sort of _get_sync() is needed. > > For devices that aren't part of a power domain, things are simpler. > The bus does _get_noresume() to make sure the device won't be runtime > suspended while the probe routine is running. It doesn't do > _get_sync(), because that would end up calling the driver's > runtime_resume routine before the driver was bound to the device. (The > bus could prevent that from happening by taking special precautions, > like PCI does, but in general it's a nuisance.) That's why I think we need some new call that would mean "make sure the device is powered" which would properly handle power domain and bus, but ignore all driver stuff since it may not be initialized yet. And similar call for asking to put device and maybe domain in powered down state in case probing failed. Thanks.
On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > For devices that aren't part of a power domain, things are simpler. > > The bus does _get_noresume() to make sure the device won't be runtime > > suspended while the probe routine is running. It doesn't do > > _get_sync(), because that would end up calling the driver's > > runtime_resume routine before the driver was bound to the device. (The > > bus could prevent that from happening by taking special precautions, > > like PCI does, but in general it's a nuisance.) > > That's why I think we need some new call that would mean "make sure the > device is powered" which would properly handle power domain and bus, but > ignore all driver stuff since it may not be initialized yet. And similar > call for asking to put device and maybe domain in powered down state in > case probing failed. I can't imagine how such a call would work. The PM core invokes the subsystem's runtime_suspend/resume callback, and then the subsystem's routine is responsible for invoking the driver's callback (or _not_ invoking it, in this case). Thus, the PM core has no way to tell the subsystem's callback not to invoke the driver's routine, and adding a new runtime PM call wouldn't change that. You'd have to add a new pair of callbacks instead, which IMO would be a tremendous waste. Furthermore, the subsystem already _knows_ when the driver gets probed, because probing works in the same sort of way: The subsystem's probe routine gets invoked, and it is responsible for invoking the driver's probe routine. Therefore the PM core doesn't _need_ to provide this extra information to the subsystem. Rather, the subsystem just needs to keep track of the information it already has available. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 17, 2014 at 02:54:53PM -0500, Alan Stern wrote: > On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > > For devices that aren't part of a power domain, things are simpler. > > > The bus does _get_noresume() to make sure the device won't be runtime > > > suspended while the probe routine is running. It doesn't do > > > _get_sync(), because that would end up calling the driver's > > > runtime_resume routine before the driver was bound to the device. (The > > > bus could prevent that from happening by taking special precautions, > > > like PCI does, but in general it's a nuisance.) > > > > That's why I think we need some new call that would mean "make sure the > > device is powered" which would properly handle power domain and bus, but > > ignore all driver stuff since it may not be initialized yet. And similar > > call for asking to put device and maybe domain in powered down state in > > case probing failed. > > I can't imagine how such a call would work. > > The PM core invokes the subsystem's runtime_suspend/resume callback, > and then the subsystem's routine is responsible for invoking the > driver's callback (or _not_ invoking it, in this case). > > Thus, the PM core has no way to tell the subsystem's callback not to > invoke the driver's routine, and adding a new runtime PM call wouldn't > change that. You'd have to add a new pair of callbacks instead, which > IMO would be a tremendous waste. > > Furthermore, the subsystem already _knows_ when the driver gets probed, > because probing works in the same sort of way: The subsystem's probe > routine gets invoked, and it is responsible for invoking the driver's > probe routine. Therefore the PM core doesn't _need_ to provide this > extra information to the subsystem. Rather, the subsystem just needs > to keep track of the information it already has available. You are missing concept of power domains in this picture. True, subsystem knows when it probes but power domain does not. Subsystem has no knowledge of power domain (devices in the same subsystem can come from different domains). We need to have either subsystem or device core to indicate to power management core that we do not need "full" runtime resume, but rather a "partial" one since driver is not ready yet. We would not need new callbacks here I think, we just need to be able to select appropriate set of callbacks, depending on the binding state. Thanks.
On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > On Mon, Nov 17, 2014 at 02:54:53PM -0500, Alan Stern wrote: > > On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > > > > For devices that aren't part of a power domain, things are simpler. > > > > The bus does _get_noresume() to make sure the device won't be runtime > > > > suspended while the probe routine is running. It doesn't do > > > > _get_sync(), because that would end up calling the driver's > > > > runtime_resume routine before the driver was bound to the device. (The > > > > bus could prevent that from happening by taking special precautions, > > > > like PCI does, but in general it's a nuisance.) > > > > > > That's why I think we need some new call that would mean "make sure the > > > device is powered" which would properly handle power domain and bus, but > > > ignore all driver stuff since it may not be initialized yet. And similar > > > call for asking to put device and maybe domain in powered down state in > > > case probing failed. > > > > I can't imagine how such a call would work. > > > > The PM core invokes the subsystem's runtime_suspend/resume callback, > > and then the subsystem's routine is responsible for invoking the > > driver's callback (or _not_ invoking it, in this case). > > > > Thus, the PM core has no way to tell the subsystem's callback not to > > invoke the driver's routine, and adding a new runtime PM call wouldn't > > change that. You'd have to add a new pair of callbacks instead, which > > IMO would be a tremendous waste. > > > > Furthermore, the subsystem already _knows_ when the driver gets probed, > > because probing works in the same sort of way: The subsystem's probe > > routine gets invoked, and it is responsible for invoking the driver's > > probe routine. Therefore the PM core doesn't _need_ to provide this > > extra information to the subsystem. Rather, the subsystem just needs > > to keep track of the information it already has available. > > You are missing concept of power domains in this picture. True, > subsystem knows when it probes but power domain does not. Subsystem has > no knowledge of power domain (devices in the same subsystem can come > from different domains). Okay, I take your point. > We need to have either subsystem or device core to indicate to power > management core that we do not need "full" runtime resume, but rather a > "partial" one since driver is not ready yet. > > We would not need new callbacks here I think, we just need to be able to > select appropriate set of callbacks, depending on the binding state. When the runtime PM core invokes a power domain's callback routine, what does the domain's routine usually do? Does it go ahead and invoke the driver's callback? Or does it try to invoke the subsystem's callback? Obviously this depends on how the power domain code is written. But suppose every power domain would always use the same strategy as the PM core: Invoke the subsystem's callback if there is one; otherwise invoke the driver's callback. Then there wouldn't be a problem. Even when a runtime-resume went via the power domain, the subsystem would still be able to protect the not-yet-bound driver from being called. (... Unless the subsystem itself was incapable of doing this the right way. But subsystems can be fixed.) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 17, 2014 at 03:49:14PM -0500, Alan Stern wrote: > On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > On Mon, Nov 17, 2014 at 02:54:53PM -0500, Alan Stern wrote: > > > On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > > > > > > For devices that aren't part of a power domain, things are simpler. > > > > > The bus does _get_noresume() to make sure the device won't be runtime > > > > > suspended while the probe routine is running. It doesn't do > > > > > _get_sync(), because that would end up calling the driver's > > > > > runtime_resume routine before the driver was bound to the device. (The > > > > > bus could prevent that from happening by taking special precautions, > > > > > like PCI does, but in general it's a nuisance.) > > > > > > > > That's why I think we need some new call that would mean "make sure the > > > > device is powered" which would properly handle power domain and bus, but > > > > ignore all driver stuff since it may not be initialized yet. And similar > > > > call for asking to put device and maybe domain in powered down state in > > > > case probing failed. > > > > > > I can't imagine how such a call would work. > > > > > > The PM core invokes the subsystem's runtime_suspend/resume callback, > > > and then the subsystem's routine is responsible for invoking the > > > driver's callback (or _not_ invoking it, in this case). > > > > > > Thus, the PM core has no way to tell the subsystem's callback not to > > > invoke the driver's routine, and adding a new runtime PM call wouldn't > > > change that. You'd have to add a new pair of callbacks instead, which > > > IMO would be a tremendous waste. > > > > > > Furthermore, the subsystem already _knows_ when the driver gets probed, > > > because probing works in the same sort of way: The subsystem's probe > > > routine gets invoked, and it is responsible for invoking the driver's > > > probe routine. Therefore the PM core doesn't _need_ to provide this > > > extra information to the subsystem. Rather, the subsystem just needs > > > to keep track of the information it already has available. > > > > You are missing concept of power domains in this picture. True, > > subsystem knows when it probes but power domain does not. Subsystem has > > no knowledge of power domain (devices in the same subsystem can come > > from different domains). > > Okay, I take your point. > > > We need to have either subsystem or device core to indicate to power > > management core that we do not need "full" runtime resume, but rather a > > "partial" one since driver is not ready yet. > > > > We would not need new callbacks here I think, we just need to be able to > > select appropriate set of callbacks, depending on the binding state. > > When the runtime PM core invokes a power domain's callback routine, > what does the domain's routine usually do? Does it go ahead and invoke > the driver's callback? Or does it try to invoke the subsystem's > callback? > > Obviously this depends on how the power domain code is written. But > suppose every power domain would always use the same strategy as the PM > core: Invoke the subsystem's callback if there is one; otherwise invoke > the driver's callback. > > Then there wouldn't be a problem. Even when a runtime-resume went via > the power domain, the subsystem would still be able to protect the > not-yet-bound driver from being called. > > (... Unless the subsystem itself was incapable of doing this the right > way. But subsystems can be fixed.) The genpd code currently starts by powering on the domain (if it is not on already) and then does "device restore" which is: /** * pm_genpd_default_restore_state - Default PM domians "restore device state". * @dev: Device to handle. */ static int pm_genpd_default_restore_state(struct device *dev) { int (*cb)(struct device *__dev); cb = dev_gpd_data(dev)->ops.restore_state; if (cb) return cb(dev); if (dev->type && dev->type->pm) cb = dev->type->pm->runtime_resume; else if (dev->class && dev->class->pm) cb = dev->class->pm->runtime_resume; else if (dev->bus && dev->bus->pm) cb = dev->bus->pm->runtime_resume; else cb = NULL; if (!cb && dev->driver && dev->driver->pm) cb = dev->driver->pm->runtime_resume; return cb ? cb(dev) : 0; } So I guess bus (or class or type) can take care of it. Except buses usually call pm_generic_runtime_resume() which ends up fetching driver's callbacks. Maybe pm_generic_runtime_*() need be a bit smarter? Thanks.
On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > When the runtime PM core invokes a power domain's callback routine, > > what does the domain's routine usually do? Does it go ahead and invoke > > the driver's callback? Or does it try to invoke the subsystem's > > callback? > > > > Obviously this depends on how the power domain code is written. But > > suppose every power domain would always use the same strategy as the PM > > core: Invoke the subsystem's callback if there is one; otherwise invoke > > the driver's callback. > > > > Then there wouldn't be a problem. Even when a runtime-resume went via > > the power domain, the subsystem would still be able to protect the > > not-yet-bound driver from being called. > > > > (... Unless the subsystem itself was incapable of doing this the right > > way. But subsystems can be fixed.) > > The genpd code currently starts by powering on the domain (if it is not > on already) and then does "device restore" which is: > > /** > * pm_genpd_default_restore_state - Default PM domians "restore device state". > * @dev: Device to handle. > */ > static int pm_genpd_default_restore_state(struct device *dev) > { > int (*cb)(struct device *__dev); > > cb = dev_gpd_data(dev)->ops.restore_state; > if (cb) > return cb(dev); > > if (dev->type && dev->type->pm) > cb = dev->type->pm->runtime_resume; > else if (dev->class && dev->class->pm) > cb = dev->class->pm->runtime_resume; > else if (dev->bus && dev->bus->pm) > cb = dev->bus->pm->runtime_resume; > else > cb = NULL; > > if (!cb && dev->driver && dev->driver->pm) > cb = dev->driver->pm->runtime_resume; > > return cb ? cb(dev) : 0; > } > > So I guess bus (or class or type) can take care of it. The bus could. I don't think the class or type knows when a driver is being probed. > Except buses > usually call pm_generic_runtime_resume() which ends up fetching driver's > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter? No, the bus subsystem needs to be smarter. It shouldn't call pm_generic_runtime_resume() if the driver hasn't been probed yet, or if the driver has already been unbound from the device. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 17, 2014 at 04:44:56PM -0500, Alan Stern wrote: > On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > > When the runtime PM core invokes a power domain's callback routine, > > > what does the domain's routine usually do? Does it go ahead and invoke > > > the driver's callback? Or does it try to invoke the subsystem's > > > callback? > > > > > > Obviously this depends on how the power domain code is written. But > > > suppose every power domain would always use the same strategy as the PM > > > core: Invoke the subsystem's callback if there is one; otherwise invoke > > > the driver's callback. > > > > > > Then there wouldn't be a problem. Even when a runtime-resume went via > > > the power domain, the subsystem would still be able to protect the > > > not-yet-bound driver from being called. > > > > > > (... Unless the subsystem itself was incapable of doing this the right > > > way. But subsystems can be fixed.) > > > > The genpd code currently starts by powering on the domain (if it is not > > on already) and then does "device restore" which is: > > > > /** > > * pm_genpd_default_restore_state - Default PM domians "restore device state". > > * @dev: Device to handle. > > */ > > static int pm_genpd_default_restore_state(struct device *dev) > > { > > int (*cb)(struct device *__dev); > > > > cb = dev_gpd_data(dev)->ops.restore_state; > > if (cb) > > return cb(dev); > > > > if (dev->type && dev->type->pm) > > cb = dev->type->pm->runtime_resume; > > else if (dev->class && dev->class->pm) > > cb = dev->class->pm->runtime_resume; > > else if (dev->bus && dev->bus->pm) > > cb = dev->bus->pm->runtime_resume; > > else > > cb = NULL; > > > > if (!cb && dev->driver && dev->driver->pm) > > cb = dev->driver->pm->runtime_resume; > > > > return cb ? cb(dev) : 0; > > } > > > > So I guess bus (or class or type) can take care of it. > > The bus could. I don't think the class or type knows when a driver is > being probed. > > > Except buses > > usually call pm_generic_runtime_resume() which ends up fetching driver's > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter? > > No, the bus subsystem needs to be smarter. It shouldn't call > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if > the driver has already been unbound from the device. But that code wold be exactly the same for all buses, right? So why can't pm_generic_runtime_resume() be smarter? However, is it allowed to call pm_runtime_get_sync() on devices that didn't issue pm_runtime_enable()? Thanks.
On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > Except buses > > > usually call pm_generic_runtime_resume() which ends up fetching driver's > > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter? > > > > No, the bus subsystem needs to be smarter. It shouldn't call > > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if > > the driver has already been unbound from the device. > > But that code wold be exactly the same for all buses, right? So why > can't pm_generic_runtime_resume() be smarter? It would not be the same for all buses. Each bus will have its own way of recognizing whether or not a driver has been probed (i.e., by checking some field in the bus-specific part of the device structure). > However, is it allowed to call pm_runtime_get_sync() on devices that > didn't issue pm_runtime_enable()? Yes. But the bus has to issue pm_runtime_enable() before probing the driver, because the driver will expect runtime PM to work properly while its probe routine runs. For example, the probe routine might want to leave the device in a runtime-suspended state. It can't do that if the device isn't enabled for runtime PM. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 17, 2014 at 05:12:35PM -0500, Alan Stern wrote: > On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > > > Except buses > > > > usually call pm_generic_runtime_resume() which ends up fetching driver's > > > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter? > > > > > > No, the bus subsystem needs to be smarter. It shouldn't call > > > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if > > > the driver has already been unbound from the device. > > > > But that code wold be exactly the same for all buses, right? So why > > can't pm_generic_runtime_resume() be smarter? > > It would not be the same for all buses. Each bus will have its own way > of recognizing whether or not a driver has been probed (i.e., by > checking some field in the bus-specific part of the device structure). > > > However, is it allowed to call pm_runtime_get_sync() on devices that > > didn't issue pm_runtime_enable()? > > Yes. But the bus has to issue pm_runtime_enable() before probing the > driver, because the driver will expect runtime PM to work properly > while its probe routine runs. For example, the probe routine might > want to leave the device in a runtime-suspended state. It can't do > that if the device isn't enabled for runtime PM. That means that runtime PM will be enabled for all devices on given bus while up till now drivers were deciding if their devices should be runtime-pm-managed or not. I do not think we are quite ready for this. Thanks.
On Tue, Nov 18, 2014 at 12:28:26AM +0100, Rafael J. Wysocki wrote: > On Monday, November 17, 2014 02:17:00 PM Dmitry Torokhov wrote: > > On Mon, Nov 17, 2014 at 05:12:35PM -0500, Alan Stern wrote: > > > On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > > > > > > > Except buses > > > > > > usually call pm_generic_runtime_resume() which ends up fetching driver's > > > > > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter? > > > > > > > > > > No, the bus subsystem needs to be smarter. It shouldn't call > > > > > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if > > > > > the driver has already been unbound from the device. > > > > > > > > But that code wold be exactly the same for all buses, right? So why > > > > can't pm_generic_runtime_resume() be smarter? > > > > > > It would not be the same for all buses. Each bus will have its own way > > > of recognizing whether or not a driver has been probed (i.e., by > > > checking some field in the bus-specific part of the device structure). > > > > > > > However, is it allowed to call pm_runtime_get_sync() on devices that > > > > didn't issue pm_runtime_enable()? > > > > > > Yes. But the bus has to issue pm_runtime_enable() before probing the > > > driver, because the driver will expect runtime PM to work properly > > > while its probe routine runs. For example, the probe routine might > > > want to leave the device in a runtime-suspended state. It can't do > > > that if the device isn't enabled for runtime PM. > > > > That means that runtime PM will be enabled for all devices on given bus > > while up till now drivers were deciding if their devices should be > > runtime-pm-managed or not. > > That's not the case for PCI drivers. > > > I do not think we are quite ready for this. > > We have to do that if power domains are in use, however, because if at least > one device in a power domain in enabled for runtime PM, that will affect the > other devices in that domain. > > We could make a rule to keep a domail always up if at least one device in it > has runtime PM disabled, but that is equivalent to enabling runtime PM for > that device, powering the domain up and bumping up the device's usage counter. What will driver will see if it tries to check pm_runtime_active()? Would not it get unexpected result if the driver did not call pm_runtime_enable() on it's device?
On Monday, November 17, 2014 02:17:00 PM Dmitry Torokhov wrote: > On Mon, Nov 17, 2014 at 05:12:35PM -0500, Alan Stern wrote: > > On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > > > > > Except buses > > > > > usually call pm_generic_runtime_resume() which ends up fetching driver's > > > > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter? > > > > > > > > No, the bus subsystem needs to be smarter. It shouldn't call > > > > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if > > > > the driver has already been unbound from the device. > > > > > > But that code wold be exactly the same for all buses, right? So why > > > can't pm_generic_runtime_resume() be smarter? > > > > It would not be the same for all buses. Each bus will have its own way > > of recognizing whether or not a driver has been probed (i.e., by > > checking some field in the bus-specific part of the device structure). > > > > > However, is it allowed to call pm_runtime_get_sync() on devices that > > > didn't issue pm_runtime_enable()? > > > > Yes. But the bus has to issue pm_runtime_enable() before probing the > > driver, because the driver will expect runtime PM to work properly > > while its probe routine runs. For example, the probe routine might > > want to leave the device in a runtime-suspended state. It can't do > > that if the device isn't enabled for runtime PM. > > That means that runtime PM will be enabled for all devices on given bus > while up till now drivers were deciding if their devices should be > runtime-pm-managed or not. That's not the case for PCI drivers. > I do not think we are quite ready for this. We have to do that if power domains are in use, however, because if at least one device in a power domain in enabled for runtime PM, that will affect the other devices in that domain. We could make a rule to keep a domail always up if at least one device in it has runtime PM disabled, but that is equivalent to enabling runtime PM for that device, powering the domain up and bumping up the device's usage counter. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 17, 2014 03:26:04 PM Dmitry Torokhov wrote: > On Tue, Nov 18, 2014 at 12:28:26AM +0100, Rafael J. Wysocki wrote: > > On Monday, November 17, 2014 02:17:00 PM Dmitry Torokhov wrote: > > > On Mon, Nov 17, 2014 at 05:12:35PM -0500, Alan Stern wrote: > > > > On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > > > > > > > > > Except buses > > > > > > > usually call pm_generic_runtime_resume() which ends up fetching driver's > > > > > > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter? > > > > > > > > > > > > No, the bus subsystem needs to be smarter. It shouldn't call > > > > > > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if > > > > > > the driver has already been unbound from the device. > > > > > > > > > > But that code wold be exactly the same for all buses, right? So why > > > > > can't pm_generic_runtime_resume() be smarter? > > > > > > > > It would not be the same for all buses. Each bus will have its own way > > > > of recognizing whether or not a driver has been probed (i.e., by > > > > checking some field in the bus-specific part of the device structure). > > > > > > > > > However, is it allowed to call pm_runtime_get_sync() on devices that > > > > > didn't issue pm_runtime_enable()? > > > > > > > > Yes. But the bus has to issue pm_runtime_enable() before probing the > > > > driver, because the driver will expect runtime PM to work properly > > > > while its probe routine runs. For example, the probe routine might > > > > want to leave the device in a runtime-suspended state. It can't do > > > > that if the device isn't enabled for runtime PM. > > > > > > That means that runtime PM will be enabled for all devices on given bus > > > while up till now drivers were deciding if their devices should be > > > runtime-pm-managed or not. > > > > That's not the case for PCI drivers. > > > > > I do not think we are quite ready for this. > > > > We have to do that if power domains are in use, however, because if at least > > one device in a power domain in enabled for runtime PM, that will affect the > > other devices in that domain. > > > > We could make a rule to keep a domail always up if at least one device in it > > has runtime PM disabled, but that is equivalent to enabling runtime PM for > > that device, powering the domain up and bumping up the device's usage counter. > > What will driver will see if it tries to check pm_runtime_active()? > Would not it get unexpected result if the driver did not call > pm_runtime_enable() on it's device? Well, that part was supposed to depend on the bus type. For example, it won't be unexpected for PCI drivers, because runtime PM is always enabled for PCI devices (although it may be blocked as I described). A problem arises if a power domain is used along with a bus type that doesn't enable runtime PM for devices by default and drivers are expected to do that. That could be addressed by powering up a PM domain (and bumping up its usage counter) when adding a device to it and keeping it up until all of the devices in it are runtime suspended, but then it also should be turned off eventually if there are no drivers for any of those devices. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 18, 2014 01:26:38 AM Rafael J. Wysocki wrote: > On Monday, November 17, 2014 03:26:04 PM Dmitry Torokhov wrote: > > On Tue, Nov 18, 2014 at 12:28:26AM +0100, Rafael J. Wysocki wrote: > > > On Monday, November 17, 2014 02:17:00 PM Dmitry Torokhov wrote: > > > > On Mon, Nov 17, 2014 at 05:12:35PM -0500, Alan Stern wrote: > > > > > On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > > > > > > > > > > > Except buses > > > > > > > > usually call pm_generic_runtime_resume() which ends up fetching driver's > > > > > > > > callbacks. Maybe pm_generic_runtime_*() need be a bit smarter? > > > > > > > > > > > > > > No, the bus subsystem needs to be smarter. It shouldn't call > > > > > > > pm_generic_runtime_resume() if the driver hasn't been probed yet, or if > > > > > > > the driver has already been unbound from the device. > > > > > > > > > > > > But that code wold be exactly the same for all buses, right? So why > > > > > > can't pm_generic_runtime_resume() be smarter? > > > > > > > > > > It would not be the same for all buses. Each bus will have its own way > > > > > of recognizing whether or not a driver has been probed (i.e., by > > > > > checking some field in the bus-specific part of the device structure). > > > > > > > > > > > However, is it allowed to call pm_runtime_get_sync() on devices that > > > > > > didn't issue pm_runtime_enable()? > > > > > > > > > > Yes. But the bus has to issue pm_runtime_enable() before probing the > > > > > driver, because the driver will expect runtime PM to work properly > > > > > while its probe routine runs. For example, the probe routine might > > > > > want to leave the device in a runtime-suspended state. It can't do > > > > > that if the device isn't enabled for runtime PM. > > > > > > > > That means that runtime PM will be enabled for all devices on given bus > > > > while up till now drivers were deciding if their devices should be > > > > runtime-pm-managed or not. > > > > > > That's not the case for PCI drivers. > > > > > > > I do not think we are quite ready for this. > > > > > > We have to do that if power domains are in use, however, because if at least > > > one device in a power domain in enabled for runtime PM, that will affect the > > > other devices in that domain. > > > > > > We could make a rule to keep a domail always up if at least one device in it > > > has runtime PM disabled, but that is equivalent to enabling runtime PM for > > > that device, powering the domain up and bumping up the device's usage counter. > > > > What will driver will see if it tries to check pm_runtime_active()? > > Would not it get unexpected result if the driver did not call > > pm_runtime_enable() on it's device? > > Well, that part was supposed to depend on the bus type. For example, it > won't be unexpected for PCI drivers, because runtime PM is always enabled > for PCI devices (although it may be blocked as I described). > > A problem arises if a power domain is used along with a bus type that doesn't > enable runtime PM for devices by default and drivers are expected to do that. > That could be addressed by powering up a PM domain (and bumping up its usage > counter) when adding a device to it and keeping it up until all of the devices > in it are runtime suspended, but then it also should be turned off eventually > if there are no drivers for any of those devices. In other words, I agree with the Ulf's approach in the $subject patch, although the changelog needs to be updated. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >> > > > > >> > > > > It would not be the same for all buses. Each bus will have its own way >> > > > > of recognizing whether or not a driver has been probed (i.e., by >> > > > > checking some field in the bus-specific part of the device structure). >> > > > > >> > > > > > However, is it allowed to call pm_runtime_get_sync() on devices that >> > > > > > didn't issue pm_runtime_enable()? >> > > > > >> > > > > Yes. But the bus has to issue pm_runtime_enable() before probing the >> > > > > driver, because the driver will expect runtime PM to work properly >> > > > > while its probe routine runs. For example, the probe routine might >> > > > > want to leave the device in a runtime-suspended state. It can't do >> > > > > that if the device isn't enabled for runtime PM. >> > > > >> > > > That means that runtime PM will be enabled for all devices on given bus >> > > > while up till now drivers were deciding if their devices should be >> > > > runtime-pm-managed or not. >> > > >> > > That's not the case for PCI drivers. >> > > >> > > > I do not think we are quite ready for this. >> > > >> > > We have to do that if power domains are in use, however, because if at least >> > > one device in a power domain in enabled for runtime PM, that will affect the >> > > other devices in that domain. >> > > >> > > We could make a rule to keep a domail always up if at least one device in it >> > > has runtime PM disabled, but that is equivalent to enabling runtime PM for >> > > that device, powering the domain up and bumping up the device's usage counter. >> > >> > What will driver will see if it tries to check pm_runtime_active()? >> > Would not it get unexpected result if the driver did not call >> > pm_runtime_enable() on it's device? >> >> Well, that part was supposed to depend on the bus type. For example, it >> won't be unexpected for PCI drivers, because runtime PM is always enabled >> for PCI devices (although it may be blocked as I described). >> >> A problem arises if a power domain is used along with a bus type that doesn't >> enable runtime PM for devices by default and drivers are expected to do that. >> That could be addressed by powering up a PM domain (and bumping up its usage >> counter) when adding a device to it and keeping it up until all of the devices >> in it are runtime suspended, but then it also should be turned off eventually >> if there are no drivers for any of those devices. > > In other words, I agree with the Ulf's approach in the $subject patch, although > the changelog needs to be updated. > I am having second thoughts here! The reason to why, are because of the scenario 5) below, which I forgot about when writing/testing $subject patch. As you know, I have been thinking over and over about how to address the issues $subject patch is trying to solve. From the discussions we have had around this topic, I would like to summarize the situation describing the current existing scenarios that I know about. It would be nice to reach a consensus on how to cope with them, especially since we keep forgetting to consider at least one of them during our discussions. Note that some minor variations may exist for each scenario, but let's focus on the concepts of how runtime PM is managed during ->probe(). The scenarios applies to those drivers/buses for which devices may have an attached generic PM domain. Scenario 1), a platform driver without runtime PM callbacks. ->probe() - do some initialization - pm_runtime_enable() - pm_runtime_get_sync() - probe the device - pm_runtime_put() Scenario 2), platform driver with runtime PM callbacks. ->probe() - do some initialization - fetch handles to runtime PM resources - pm_runtime_enable() - pm_runtime_get_sync() - enable its runtime PM resources, in some cases it's done by also requiring its runtime PM resume callback to be invoked - probe the device - pm_runtime_put() Scenario 3), platform driver with runtime PM callbacks. ->probe() - do some initialization - fetch handles to runtime PM resources - enable its runtime PM resources - pm_runtime_get_noresume() - pm_runtime_set_active() - pm_runtime_enable() - probe the device - pm_runtime_put() Scenario 4), amba bus and amba driver, both have runtime PM callbacks. ->amba bus probe() - fetch handles to runtime PM resources - enable its runtime PM resources - pm_runtime_get_noresume() - pm_runtime_set_active() - pm_runtime_enable() ->amba driver probe() - do some initialization - fetch handles to runtime PM resources - enable its runtime PM resources - probe the device - pm_runtime_put() Scenario 5), a platform driver with/without runtime PM callbacks. ->probe() - do some initialization - may fetch handles to runtime PM resources - pm_runtime_enable() Note 1) Scenario 1) and 2), both relies on the approach to power on the PM domain by using pm_runtime_get_sync(). That approach didn't work when CONFIG_PM_RUNTIME was unset, but we recently decided to fixed that by the below patch, so that's good! "[PATCH] PM / domains: Kconfig: always enable PM_RUNTIME when genpd enabled" Note 2) Scenario 3) and 4) use the same principles for managing runtime PM. These scenarios needs a way to power on the generic PM domain prior probing the device. The call to pm_runtime_set_active(), prevents an already powered PM domain from power off until after probe, but that's not enough. Note 3) The $subject patch, tried to address the issues for scenario 3) and 4). It does so, but will affect scenario 5) which was working nicely before. In scenario 5), the $subject patch will cause the generic PM domain to potentially stay powered after ->probe() even if the device is runtime PM suspended. I see three options going forward. Option 1) Convert scenario 3) and 4) into using the pm_runtime_get_sync() approach. There are no theoretical obstacles to do this, but pure practical. There are a lot of drivers that needs to be converted and we also need to teach driver authors future wise to not use pm_runtime_set_active() in this manner. Option 2) Add some kind of get/put API for PM domains. The buses invokes it to control the power to the PM domain. From what I understand, that's also what Dmitry think is needed!? Anyway, that somehow means to proceed with the approach I took in the below patchset. [PATCH v3 0/9] PM / Domains: Fix race conditions during boot http://marc.info/?t=141320907000003&r=1&w=2 Option 3) Live we the limitation this $subject patch introduces for scenario 5). Is there maybe any additional options, that I didn't think of? Apologize for the long email, I hope I didn't bored you too much. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > However, is it allowed to call pm_runtime_get_sync() on devices that > > > didn't issue pm_runtime_enable()? > > > > Yes. But the bus has to issue pm_runtime_enable() before probing the > > driver, because the driver will expect runtime PM to work properly > > while its probe routine runs. For example, the probe routine might > > want to leave the device in a runtime-suspended state. It can't do > > that if the device isn't enabled for runtime PM. > > That means that runtime PM will be enabled for all devices on given bus > while up till now drivers were deciding if their devices should be > runtime-pm-managed or not. I do not think we are quite ready for this. It's up to both the bus _and_ the driver to make this decision. If a driver is completely runtime-PM-unaware then it will never decrement the device's usage counter (which was incremented when the bus called _get_noresume()), and therefore the device will never be runtime-suspended. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 18, 2014 at 11:13:28AM -0500, Alan Stern wrote: > On Mon, 17 Nov 2014, Dmitry Torokhov wrote: > > > > > However, is it allowed to call pm_runtime_get_sync() on devices that > > > > didn't issue pm_runtime_enable()? > > > > > > Yes. But the bus has to issue pm_runtime_enable() before probing the > > > driver, because the driver will expect runtime PM to work properly > > > while its probe routine runs. For example, the probe routine might > > > want to leave the device in a runtime-suspended state. It can't do > > > that if the device isn't enabled for runtime PM. > > > > That means that runtime PM will be enabled for all devices on given bus > > while up till now drivers were deciding if their devices should be > > runtime-pm-managed or not. I do not think we are quite ready for this. > > It's up to both the bus _and_ the driver to make this decision. > > If a driver is completely runtime-PM-unaware then it will never > decrement the device's usage counter (which was incremented when the > bus called _get_noresume()), and therefore the device will never be > runtime-suspended. OK. Another question then: pm_runtime_get_noresume() does literally this: atomic_inc(&dev->power.usage_count); So who is responsible for actually waking up parent device and/or power domain? Is it simply missing because we did not really have PM domains before? Thanks.
On Tue, 18 Nov 2014, Dmitry Torokhov wrote: > OK. Another question then: pm_runtime_get_noresume() does literally this: > > atomic_inc(&dev->power.usage_count); > > So who is responsible for actually waking up parent device and/or power > domain? Is it simply missing because we did not really have PM domains > before? Ths bus is responsible for making sure that all the standard resources are available -- that is, all the resources that would be needed by a normal device on that bus. Anything beyond that (such as special-purpose clocks) has to be set up by the driver. Thus the bus would insure that the device was powered, its parent was resumed, and the usual clock inputs were enabled. And of course, one mechanism for doing this is to runtime-resume the power domain. Often the bus doesn't have to do anything special to resume the device's parent. This is because the device gets probed when it is registered, which happens when it is discovered, and the discovery is done by the parent's driver as part of its normal activity. Since the parent has to be powered up to carry out this normal activity, nothing more is needed. (Of course, devices can get probed at other times too, not just when they are discovered. For example, a device might get probed when a driver module is loaded, and that can occur at any time. So in general it might indeed be necessary for the bus to wake up the parent before calling the driver's probe routine.) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote: > On Tue, 18 Nov 2014, Dmitry Torokhov wrote: > > > OK. Another question then: pm_runtime_get_noresume() does literally this: > > > > atomic_inc(&dev->power.usage_count); > > > > So who is responsible for actually waking up parent device and/or power > > domain? Is it simply missing because we did not really have PM domains > > before? > > Ths bus is responsible for making sure that all the standard resources > are available -- that is, all the resources that would be needed by a > normal device on that bus. Anything beyond that (such as > special-purpose clocks) has to be set up by the driver. > > Thus the bus would insure that the device was powered, its parent was > resumed, and the usual clock inputs were enabled. And of course, one > mechanism for doing this is to runtime-resume the power domain. This does not sound like anything bus-specific. Can we move powering on the domain before probing into the driver core, similarly to the default pin selection by pinctrl? I do not see why we want to have every individual bus to implement this. I guess right now we can't because we assign the domain to device in individual bus's probe function, but I do not think this is proper place for that either: bus and pm domain are orthogonal concepts IMO. > > Often the bus doesn't have to do anything special to resume the > device's parent. This is because the device gets probed when it is > registered, which happens when it is discovered, and the discovery is > done by the parent's driver as part of its normal activity. Since the > parent has to be powered up to carry out this normal activity, nothing > more is needed. I think this only true for buses that support discovery. > > (Of course, devices can get probed at other times too, not just when > they are discovered. For example, a device might get probed when a > driver module is loaded, and that can occur at any time. So in general > it might indeed be necessary for the bus to wake up the parent before > calling the driver's probe routine.) > Thanks.
On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote: > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote: > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote: > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote: > > > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this: > > > > > > > > atomic_inc(&dev->power.usage_count); > > > > > > > > So who is responsible for actually waking up parent device and/or power > > > > domain? Is it simply missing because we did not really have PM domains > > > > before? > > > > > > Ths bus is responsible for making sure that all the standard resources > > > are available -- that is, all the resources that would be needed by a > > > normal device on that bus. Anything beyond that (such as > > > special-purpose clocks) has to be set up by the driver. > > > > > > Thus the bus would insure that the device was powered, its parent was > > > resumed, and the usual clock inputs were enabled. And of course, one > > > mechanism for doing this is to runtime-resume the power domain. > > > > This does not sound like anything bus-specific. Can we move powering on > > the domain before probing into the driver core, similarly to the default > > pin selection by pinctrl? > > We could do that for genpd if devices were added to domains before registering > (those devices). Otherwise, there's no guarantee that all has been set up yet. > > Note that this would only be the case for genpd, not for the ACPI PM domain > in particular, for example. The reason why is that the ACPI PM domain cannot > be used along with bus types that provide non-trivial PM callbacks, so pretty > much the bus type's ->probe needs to decide whether or not to use it. In genpd code there is a notion of providers that match devices and domains. Can we do the same for ACPI and stuff all that knowledge into it's "provider"? IOW why ACPI is that special?
On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote: > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote: > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote: > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this: > > > > > > atomic_inc(&dev->power.usage_count); > > > > > > So who is responsible for actually waking up parent device and/or power > > > domain? Is it simply missing because we did not really have PM domains > > > before? > > > > Ths bus is responsible for making sure that all the standard resources > > are available -- that is, all the resources that would be needed by a > > normal device on that bus. Anything beyond that (such as > > special-purpose clocks) has to be set up by the driver. > > > > Thus the bus would insure that the device was powered, its parent was > > resumed, and the usual clock inputs were enabled. And of course, one > > mechanism for doing this is to runtime-resume the power domain. > > This does not sound like anything bus-specific. Can we move powering on > the domain before probing into the driver core, similarly to the default > pin selection by pinctrl? We could do that for genpd if devices were added to domains before registering (those devices). Otherwise, there's no guarantee that all has been set up yet. Note that this would only be the case for genpd, not for the ACPI PM domain in particular, for example. The reason why is that the ACPI PM domain cannot be used along with bus types that provide non-trivial PM callbacks, so pretty much the bus type's ->probe needs to decide whether or not to use it. > I do not see why we want to have every > individual bus to implement this. I guess right now we can't because we > assign the domain to device in individual bus's probe function, but I do > not think this is proper place for that either: bus and pm domain are > orthogonal concepts IMO. For genpd, yes. Not necessarily for other types of PM domains. [A "PM domain" is just an override for bus type PM callbacks in general, genpd is only one use case here.] I think that the the general DT bindings for genpd are the source of the whole complication here. Before, devices were added to domains in the board-specific code before registration, but now that can be done automatically if the appropriate bindings are present in the DT. > > Often the bus doesn't have to do anything special to resume the > > device's parent. This is because the device gets probed when it is > > registered, which happens when it is discovered, and the discovery is > > done by the parent's driver as part of its normal activity. Since the > > parent has to be powered up to carry out this normal activity, nothing > > more is needed. > > I think this only true for buses that support discovery. It may not be the case for the platform bus type or other "root kind" bus types where devices may not have a functional parent. Otherwise it usually is the case. Anyway, that really depends on the bus type. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 18, 2014 03:05:08 PM Ulf Hansson wrote: > [...] > > >> > > > > > >> > > > > It would not be the same for all buses. Each bus will have its own way > >> > > > > of recognizing whether or not a driver has been probed (i.e., by > >> > > > > checking some field in the bus-specific part of the device structure). > >> > > > > > >> > > > > > However, is it allowed to call pm_runtime_get_sync() on devices that > >> > > > > > didn't issue pm_runtime_enable()? > >> > > > > > >> > > > > Yes. But the bus has to issue pm_runtime_enable() before probing the > >> > > > > driver, because the driver will expect runtime PM to work properly > >> > > > > while its probe routine runs. For example, the probe routine might > >> > > > > want to leave the device in a runtime-suspended state. It can't do > >> > > > > that if the device isn't enabled for runtime PM. > >> > > > > >> > > > That means that runtime PM will be enabled for all devices on given bus > >> > > > while up till now drivers were deciding if their devices should be > >> > > > runtime-pm-managed or not. > >> > > > >> > > That's not the case for PCI drivers. > >> > > > >> > > > I do not think we are quite ready for this. > >> > > > >> > > We have to do that if power domains are in use, however, because if at least > >> > > one device in a power domain in enabled for runtime PM, that will affect the > >> > > other devices in that domain. > >> > > > >> > > We could make a rule to keep a domail always up if at least one device in it > >> > > has runtime PM disabled, but that is equivalent to enabling runtime PM for > >> > > that device, powering the domain up and bumping up the device's usage counter. > >> > > >> > What will driver will see if it tries to check pm_runtime_active()? > >> > Would not it get unexpected result if the driver did not call > >> > pm_runtime_enable() on it's device? > >> > >> Well, that part was supposed to depend on the bus type. For example, it > >> won't be unexpected for PCI drivers, because runtime PM is always enabled > >> for PCI devices (although it may be blocked as I described). > >> > >> A problem arises if a power domain is used along with a bus type that doesn't > >> enable runtime PM for devices by default and drivers are expected to do that. > >> That could be addressed by powering up a PM domain (and bumping up its usage > >> counter) when adding a device to it and keeping it up until all of the devices > >> in it are runtime suspended, but then it also should be turned off eventually > >> if there are no drivers for any of those devices. > > > > In other words, I agree with the Ulf's approach in the $subject patch, although > > the changelog needs to be updated. > > > > I am having second thoughts here! The reason to why, are because of > the scenario 5) below, which I forgot about when writing/testing > $subject patch. > > As you know, I have been thinking over and over about how to address > the issues $subject patch is trying to solve. > > From the discussions we have had around this topic, I would like to > summarize the situation describing the current existing scenarios that > I know about. It would be nice to reach a consensus on how to cope > with them, especially since we keep forgetting to consider at least > one of them during our discussions. > > Note that some minor variations may exist for each scenario, but let's > focus on the concepts of how runtime PM is managed during ->probe(). > The scenarios applies to those drivers/buses for which devices may > have an attached generic PM domain. > > Scenario 1), a platform driver without runtime PM callbacks. > ->probe() > - do some initialization > - pm_runtime_enable() > - pm_runtime_get_sync() > - probe the device > - pm_runtime_put() > > Scenario 2), platform driver with runtime PM callbacks. > ->probe() > - do some initialization > - fetch handles to runtime PM resources > - pm_runtime_enable() > - pm_runtime_get_sync() > - enable its runtime PM resources, in some cases it's done by also > requiring its runtime PM resume callback to be invoked > - probe the device > - pm_runtime_put() > > Scenario 3), platform driver with runtime PM callbacks. > ->probe() > - do some initialization > - fetch handles to runtime PM resources > - enable its runtime PM resources > - pm_runtime_get_noresume() > - pm_runtime_set_active() > - pm_runtime_enable() > - probe the device > - pm_runtime_put() > > Scenario 4), amba bus and amba driver, both have runtime PM callbacks. > ->amba bus probe() > - fetch handles to runtime PM resources > - enable its runtime PM resources > - pm_runtime_get_noresume() > - pm_runtime_set_active() > - pm_runtime_enable() > ->amba driver probe() > - do some initialization > - fetch handles to runtime PM resources > - enable its runtime PM resources > - probe the device > - pm_runtime_put() > > Scenario 5), a platform driver with/without runtime PM callbacks. > ->probe() > - do some initialization > - may fetch handles to runtime PM resources > - pm_runtime_enable() Well, and now how the driver knows if the device is "on" before accessing it? > Note 1) > Scenario 1) and 2), both relies on the approach to power on the PM > domain by using pm_runtime_get_sync(). That approach didn't work when > CONFIG_PM_RUNTIME was unset, but we recently decided to fixed that by > the below patch, so that's good! > "[PATCH] PM / domains: Kconfig: always enable PM_RUNTIME when genpd enabled" > > Note 2) > Scenario 3) and 4) use the same principles for managing runtime PM. > These scenarios needs a way to power on the generic PM domain prior > probing the device. The call to pm_runtime_set_active(), prevents an > already powered PM domain from power off until after probe, but that's > not enough. > > Note 3) > The $subject patch, tried to address the issues for scenario 3) and > 4). It does so, but will affect scenario 5) which was working nicely > before. In scenario 5), the $subject patch will cause the generic PM > domain to potentially stay powered after ->probe() even if the device > is runtime PM suspended. Why would it? If the device is runtime-suspended, the domain will know that, because its callbacks will be used for that. At least, that's what I'd expect to happen, so is there a problem here? > I see three options going forward. > > Option 1) > Convert scenario 3) and 4) into using the pm_runtime_get_sync() > approach. There are no theoretical obstacles to do this, but pure > practical. There are a lot of drivers that needs to be converted and > we also need to teach driver authors future wise to not use > pm_runtime_set_active() in this manner. I'd say we need to do something like this anyway. That is, standardize on *one* approach. I'm actually not sure what approach is the most useful, but the pm_runtime_get_sync() one seems to be the most popular to me. > Option 2) > Add some kind of get/put API for PM domains. The buses invokes it to > control the power to the PM domain. From what I understand, that's > also what Dmitry think is needed!? > Anyway, that somehow means to proceed with the approach I took in the > below patchset. > [PATCH v3 0/9] PM / Domains: Fix race conditions during boot > http://marc.info/?t=141320907000003&r=1&w=2 I don't like that. The API is already quite complicated in my view and adding even more complexity to it is not going to help in the long run. > Option 3) > Live we the limitation this $subject patch introduces for scenario 5). I'd say, 3) for now and 1) going forward. > Is there maybe any additional options, that I didn't think of? > > Apologize for the long email, I hope I didn't bored you too much. That's fine, thanks for taking the time to lay out all of the cases, that's always helpful. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 18, 2014 at 10:17:46PM +0100, Rafael J. Wysocki wrote: > On Tuesday, November 18, 2014 10:03:18 PM Rafael J. Wysocki wrote: > > On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote: > > > On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote: > > > > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote: > > > > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote: > > > > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote: > > > > > > > > > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this: > > > > > > > > > > > > > > atomic_inc(&dev->power.usage_count); > > > > > > > > > > > > > > So who is responsible for actually waking up parent device and/or power > > > > > > > domain? Is it simply missing because we did not really have PM domains > > > > > > > before? > > > > > > > > > > > > Ths bus is responsible for making sure that all the standard resources > > > > > > are available -- that is, all the resources that would be needed by a > > > > > > normal device on that bus. Anything beyond that (such as > > > > > > special-purpose clocks) has to be set up by the driver. > > > > > > > > > > > > Thus the bus would insure that the device was powered, its parent was > > > > > > resumed, and the usual clock inputs were enabled. And of course, one > > > > > > mechanism for doing this is to runtime-resume the power domain. > > > > > > > > > > This does not sound like anything bus-specific. Can we move powering on > > > > > the domain before probing into the driver core, similarly to the default > > > > > pin selection by pinctrl? > > > > > > > > We could do that for genpd if devices were added to domains before registering > > > > (those devices). Otherwise, there's no guarantee that all has been set up yet. > > > > > > > > Note that this would only be the case for genpd, not for the ACPI PM domain > > > > in particular, for example. The reason why is that the ACPI PM domain cannot > > > > be used along with bus types that provide non-trivial PM callbacks, so pretty > > > > much the bus type's ->probe needs to decide whether or not to use it. > > > > > > In genpd code there is a notion of providers that match devices and > > > domains. Can we do the same for ACPI and stuff all that knowledge into > > > it's "provider"? > > > > It is in ACPI like that too, but not in the form of the ACPI PM domain. > > > > > IOW why ACPI is that special? > > > > The ACPI PM domain is there specifically for bus types that don't provide > > non-trivial PM callbacks to avoid duplication of code (if it didn't exist, > > all of the bus types in question would need to provide callbacks with > > optional ACPI handling in them). That's all about it. > > > > And there are bus types that provide non-trivial PM callbacks *and* use > > ACPI in them, like PCI, and that is more interleaved with the native PM > > in there. For those bus types we can't add devices to the ACPI PM domain > > just because they have ACPI companion objects. > > > > I'm not really sure why it is important here, though. We're talking about > > genpd, aren't we? > > > > I just wanted to indicate that the PM domains concept is not only about > > handling power domains and not all of its use cases can be shoehorned into > > the same scheme. > > And by the way, things worked just fine for the ACPI PM domain before commit > 46420dd73b80 (PM / Domains: Add APIs to attach/detach a PM domain for a device) > which put the ACPI PM domain and genpd into one bag, which was a mistake, > because they are different things. > Can we maybe settle on the naming then so that we do not mix them up in the future? For me PM domain is group of devices that share certain power constraints so that they have to be powered up and down together. Is this definition is not correct (for genpd at least)? And what is the proper definition for ACPI PM domain? Thanks.
On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote: > On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote: > > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote: > > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote: > > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote: > > > > > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this: > > > > > > > > > > atomic_inc(&dev->power.usage_count); > > > > > > > > > > So who is responsible for actually waking up parent device and/or power > > > > > domain? Is it simply missing because we did not really have PM domains > > > > > before? > > > > > > > > Ths bus is responsible for making sure that all the standard resources > > > > are available -- that is, all the resources that would be needed by a > > > > normal device on that bus. Anything beyond that (such as > > > > special-purpose clocks) has to be set up by the driver. > > > > > > > > Thus the bus would insure that the device was powered, its parent was > > > > resumed, and the usual clock inputs were enabled. And of course, one > > > > mechanism for doing this is to runtime-resume the power domain. > > > > > > This does not sound like anything bus-specific. Can we move powering on > > > the domain before probing into the driver core, similarly to the default > > > pin selection by pinctrl? > > > > We could do that for genpd if devices were added to domains before registering > > (those devices). Otherwise, there's no guarantee that all has been set up yet. > > > > Note that this would only be the case for genpd, not for the ACPI PM domain > > in particular, for example. The reason why is that the ACPI PM domain cannot > > be used along with bus types that provide non-trivial PM callbacks, so pretty > > much the bus type's ->probe needs to decide whether or not to use it. > > In genpd code there is a notion of providers that match devices and > domains. Can we do the same for ACPI and stuff all that knowledge into > it's "provider"? It is in ACPI like that too, but not in the form of the ACPI PM domain. > IOW why ACPI is that special? The ACPI PM domain is there specifically for bus types that don't provide non-trivial PM callbacks to avoid duplication of code (if it didn't exist, all of the bus types in question would need to provide callbacks with optional ACPI handling in them). That's all about it. And there are bus types that provide non-trivial PM callbacks *and* use ACPI in them, like PCI, and that is more interleaved with the native PM in there. For those bus types we can't add devices to the ACPI PM domain just because they have ACPI companion objects. I'm not really sure why it is important here, though. We're talking about genpd, aren't we? I just wanted to indicate that the PM domains concept is not only about handling power domains and not all of its use cases can be shoehorned into the same scheme. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 18, 2014 10:03:18 PM Rafael J. Wysocki wrote: > On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote: > > On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote: > > > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote: > > > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote: > > > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote: > > > > > > > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this: > > > > > > > > > > > > atomic_inc(&dev->power.usage_count); > > > > > > > > > > > > So who is responsible for actually waking up parent device and/or power > > > > > > domain? Is it simply missing because we did not really have PM domains > > > > > > before? > > > > > > > > > > Ths bus is responsible for making sure that all the standard resources > > > > > are available -- that is, all the resources that would be needed by a > > > > > normal device on that bus. Anything beyond that (such as > > > > > special-purpose clocks) has to be set up by the driver. > > > > > > > > > > Thus the bus would insure that the device was powered, its parent was > > > > > resumed, and the usual clock inputs were enabled. And of course, one > > > > > mechanism for doing this is to runtime-resume the power domain. > > > > > > > > This does not sound like anything bus-specific. Can we move powering on > > > > the domain before probing into the driver core, similarly to the default > > > > pin selection by pinctrl? > > > > > > We could do that for genpd if devices were added to domains before registering > > > (those devices). Otherwise, there's no guarantee that all has been set up yet. > > > > > > Note that this would only be the case for genpd, not for the ACPI PM domain > > > in particular, for example. The reason why is that the ACPI PM domain cannot > > > be used along with bus types that provide non-trivial PM callbacks, so pretty > > > much the bus type's ->probe needs to decide whether or not to use it. > > > > In genpd code there is a notion of providers that match devices and > > domains. Can we do the same for ACPI and stuff all that knowledge into > > it's "provider"? > > It is in ACPI like that too, but not in the form of the ACPI PM domain. > > > IOW why ACPI is that special? > > The ACPI PM domain is there specifically for bus types that don't provide > non-trivial PM callbacks to avoid duplication of code (if it didn't exist, > all of the bus types in question would need to provide callbacks with > optional ACPI handling in them). That's all about it. > > And there are bus types that provide non-trivial PM callbacks *and* use > ACPI in them, like PCI, and that is more interleaved with the native PM > in there. For those bus types we can't add devices to the ACPI PM domain > just because they have ACPI companion objects. > > I'm not really sure why it is important here, though. We're talking about > genpd, aren't we? > > I just wanted to indicate that the PM domains concept is not only about > handling power domains and not all of its use cases can be shoehorned into > the same scheme. And by the way, things worked just fine for the ACPI PM domain before commit 46420dd73b80 (PM / Domains: Add APIs to attach/detach a PM domain for a device) which put the ACPI PM domain and genpd into one bag, which was a mistake, because they are different things. Moreover, commit 207f1a2d294e (amba: Add support for attach/detach of PM domains) is outright buggy, because the AMBA bus type provides its own non-trivial PM callbacks, so using the ACPI PM domain with it is not correct. Grumble. It looks like we need to untangle these things again and start over from square one. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 18, 2014 at 10:58:17PM +0100, Rafael J. Wysocki wrote: > On Tuesday, November 18, 2014 01:02:29 PM Dmitry Torokhov wrote: > > On Tue, Nov 18, 2014 at 10:17:46PM +0100, Rafael J. Wysocki wrote: > > > On Tuesday, November 18, 2014 10:03:18 PM Rafael J. Wysocki wrote: > > > > On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote: > > > > > On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote: > > > > > > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote: > > > > > > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote: > > > > > > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote: > > > > > > > > > > > > > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this: > > > > > > > > > > > > > > > > > > atomic_inc(&dev->power.usage_count); > > > > > > > > > > > > > > > > > > So who is responsible for actually waking up parent device and/or power > > > > > > > > > domain? Is it simply missing because we did not really have PM domains > > > > > > > > > before? > > > > > > > > > > > > > > > > Ths bus is responsible for making sure that all the standard resources > > > > > > > > are available -- that is, all the resources that would be needed by a > > > > > > > > normal device on that bus. Anything beyond that (such as > > > > > > > > special-purpose clocks) has to be set up by the driver. > > > > > > > > > > > > > > > > Thus the bus would insure that the device was powered, its parent was > > > > > > > > resumed, and the usual clock inputs were enabled. And of course, one > > > > > > > > mechanism for doing this is to runtime-resume the power domain. > > > > > > > > > > > > > > This does not sound like anything bus-specific. Can we move powering on > > > > > > > the domain before probing into the driver core, similarly to the default > > > > > > > pin selection by pinctrl? > > > > > > > > > > > > We could do that for genpd if devices were added to domains before registering > > > > > > (those devices). Otherwise, there's no guarantee that all has been set up yet. > > > > > > > > > > > > Note that this would only be the case for genpd, not for the ACPI PM domain > > > > > > in particular, for example. The reason why is that the ACPI PM domain cannot > > > > > > be used along with bus types that provide non-trivial PM callbacks, so pretty > > > > > > much the bus type's ->probe needs to decide whether or not to use it. > > > > > > > > > > In genpd code there is a notion of providers that match devices and > > > > > domains. Can we do the same for ACPI and stuff all that knowledge into > > > > > it's "provider"? > > > > > > > > It is in ACPI like that too, but not in the form of the ACPI PM domain. > > > > > > > > > IOW why ACPI is that special? > > > > > > > > The ACPI PM domain is there specifically for bus types that don't provide > > > > non-trivial PM callbacks to avoid duplication of code (if it didn't exist, > > > > all of the bus types in question would need to provide callbacks with > > > > optional ACPI handling in them). That's all about it. > > > > > > > > And there are bus types that provide non-trivial PM callbacks *and* use > > > > ACPI in them, like PCI, and that is more interleaved with the native PM > > > > in there. For those bus types we can't add devices to the ACPI PM domain > > > > just because they have ACPI companion objects. > > > > > > > > I'm not really sure why it is important here, though. We're talking about > > > > genpd, aren't we? > > > > > > > > I just wanted to indicate that the PM domains concept is not only about > > > > handling power domains and not all of its use cases can be shoehorned into > > > > the same scheme. > > > > > > And by the way, things worked just fine for the ACPI PM domain before commit > > > 46420dd73b80 (PM / Domains: Add APIs to attach/detach a PM domain for a device) > > > which put the ACPI PM domain and genpd into one bag, which was a mistake, > > > because they are different things. > > > > > > > Can we maybe settle on the naming then so that we do not mix them up in > > the future? For me PM domain is group of devices that share certain > > power constraints so that they have to be powered up and down together. > > Is this definition is not correct (for genpd at least)? > > It is correct for genpd, it isn't correct for the ACPI PM domain. > > > And what is the proper definition for ACPI PM domain? > > I agree that the terminology is (somewhat?) confusing. > > From the code perspective, using a PM domain object is a way to provide PM > callbacks that will be executed for a subset of devices instead of or in > addition to the bus type (or class or device type) callbacks. Of course, > that applies to proper power domains in particular, but it can also apply > to broader sets of devices. In the ACPI PM domain case this covers devices > with ACPI power management support (or more precisely, devices with ACPI > companion objects that can provide PM support). In this context the word > "domain" means as much as "area of control" (which is a proper dictionary > definition of it AFAICS). > > genpd is all about proper power domains, like you said. OK, thank you for explaining this. Up until now I was blissfully oblivious to the majority of PM intricacies :)
On Tuesday, November 18, 2014 01:02:29 PM Dmitry Torokhov wrote: > On Tue, Nov 18, 2014 at 10:17:46PM +0100, Rafael J. Wysocki wrote: > > On Tuesday, November 18, 2014 10:03:18 PM Rafael J. Wysocki wrote: > > > On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote: > > > > On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote: > > > > > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote: > > > > > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote: > > > > > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote: > > > > > > > > > > > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this: > > > > > > > > > > > > > > > > atomic_inc(&dev->power.usage_count); > > > > > > > > > > > > > > > > So who is responsible for actually waking up parent device and/or power > > > > > > > > domain? Is it simply missing because we did not really have PM domains > > > > > > > > before? > > > > > > > > > > > > > > Ths bus is responsible for making sure that all the standard resources > > > > > > > are available -- that is, all the resources that would be needed by a > > > > > > > normal device on that bus. Anything beyond that (such as > > > > > > > special-purpose clocks) has to be set up by the driver. > > > > > > > > > > > > > > Thus the bus would insure that the device was powered, its parent was > > > > > > > resumed, and the usual clock inputs were enabled. And of course, one > > > > > > > mechanism for doing this is to runtime-resume the power domain. > > > > > > > > > > > > This does not sound like anything bus-specific. Can we move powering on > > > > > > the domain before probing into the driver core, similarly to the default > > > > > > pin selection by pinctrl? > > > > > > > > > > We could do that for genpd if devices were added to domains before registering > > > > > (those devices). Otherwise, there's no guarantee that all has been set up yet. > > > > > > > > > > Note that this would only be the case for genpd, not for the ACPI PM domain > > > > > in particular, for example. The reason why is that the ACPI PM domain cannot > > > > > be used along with bus types that provide non-trivial PM callbacks, so pretty > > > > > much the bus type's ->probe needs to decide whether or not to use it. > > > > > > > > In genpd code there is a notion of providers that match devices and > > > > domains. Can we do the same for ACPI and stuff all that knowledge into > > > > it's "provider"? > > > > > > It is in ACPI like that too, but not in the form of the ACPI PM domain. > > > > > > > IOW why ACPI is that special? > > > > > > The ACPI PM domain is there specifically for bus types that don't provide > > > non-trivial PM callbacks to avoid duplication of code (if it didn't exist, > > > all of the bus types in question would need to provide callbacks with > > > optional ACPI handling in them). That's all about it. > > > > > > And there are bus types that provide non-trivial PM callbacks *and* use > > > ACPI in them, like PCI, and that is more interleaved with the native PM > > > in there. For those bus types we can't add devices to the ACPI PM domain > > > just because they have ACPI companion objects. > > > > > > I'm not really sure why it is important here, though. We're talking about > > > genpd, aren't we? > > > > > > I just wanted to indicate that the PM domains concept is not only about > > > handling power domains and not all of its use cases can be shoehorned into > > > the same scheme. > > > > And by the way, things worked just fine for the ACPI PM domain before commit > > 46420dd73b80 (PM / Domains: Add APIs to attach/detach a PM domain for a device) > > which put the ACPI PM domain and genpd into one bag, which was a mistake, > > because they are different things. > > > > Can we maybe settle on the naming then so that we do not mix them up in > the future? For me PM domain is group of devices that share certain > power constraints so that they have to be powered up and down together. > Is this definition is not correct (for genpd at least)? It is correct for genpd, it isn't correct for the ACPI PM domain. > And what is the proper definition for ACPI PM domain? I agree that the terminology is (somewhat?) confusing. From the code perspective, using a PM domain object is a way to provide PM callbacks that will be executed for a subset of devices instead of or in addition to the bus type (or class or device type) callbacks. Of course, that applies to proper power domains in particular, but it can also apply to broader sets of devices. In the ACPI PM domain case this covers devices with ACPI power management support (or more precisely, devices with ACPI companion objects that can provide PM support). In this context the word "domain" means as much as "area of control" (which is a proper dictionary definition of it AFAICS). genpd is all about proper power domains, like you said. That's why I'm saying that they are different and clamping them both together was a mistake. I overlooked that, my bad. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 18, 2014 10:58:17 PM Rafael J. Wysocki wrote: > On Tuesday, November 18, 2014 01:02:29 PM Dmitry Torokhov wrote: > > On Tue, Nov 18, 2014 at 10:17:46PM +0100, Rafael J. Wysocki wrote: > > > On Tuesday, November 18, 2014 10:03:18 PM Rafael J. Wysocki wrote: > > > > On Tuesday, November 18, 2014 12:04:38 PM Dmitry Torokhov wrote: > > > > > On Tue, Nov 18, 2014 at 09:14:56PM +0100, Rafael J. Wysocki wrote: > > > > > > On Tuesday, November 18, 2014 09:55:15 AM Dmitry Torokhov wrote: > > > > > > > On Tue, Nov 18, 2014 at 12:44:22PM -0500, Alan Stern wrote: > > > > > > > > On Tue, 18 Nov 2014, Dmitry Torokhov wrote: > > > > > > > > > > > > > > > > > OK. Another question then: pm_runtime_get_noresume() does literally this: > > > > > > > > > > > > > > > > > > atomic_inc(&dev->power.usage_count); > > > > > > > > > > > > > > > > > > So who is responsible for actually waking up parent device and/or power > > > > > > > > > domain? Is it simply missing because we did not really have PM domains > > > > > > > > > before? > > > > > > > > > > > > > > > > Ths bus is responsible for making sure that all the standard resources > > > > > > > > are available -- that is, all the resources that would be needed by a > > > > > > > > normal device on that bus. Anything beyond that (such as > > > > > > > > special-purpose clocks) has to be set up by the driver. > > > > > > > > > > > > > > > > Thus the bus would insure that the device was powered, its parent was > > > > > > > > resumed, and the usual clock inputs were enabled. And of course, one > > > > > > > > mechanism for doing this is to runtime-resume the power domain. > > > > > > > > > > > > > > This does not sound like anything bus-specific. Can we move powering on > > > > > > > the domain before probing into the driver core, similarly to the default > > > > > > > pin selection by pinctrl? > > > > > > > > > > > > We could do that for genpd if devices were added to domains before registering > > > > > > (those devices). Otherwise, there's no guarantee that all has been set up yet. > > > > > > > > > > > > Note that this would only be the case for genpd, not for the ACPI PM domain > > > > > > in particular, for example. The reason why is that the ACPI PM domain cannot > > > > > > be used along with bus types that provide non-trivial PM callbacks, so pretty > > > > > > much the bus type's ->probe needs to decide whether or not to use it. > > > > > > > > > > In genpd code there is a notion of providers that match devices and > > > > > domains. Can we do the same for ACPI and stuff all that knowledge into > > > > > it's "provider"? > > > > > > > > It is in ACPI like that too, but not in the form of the ACPI PM domain. > > > > > > > > > IOW why ACPI is that special? > > > > > > > > The ACPI PM domain is there specifically for bus types that don't provide > > > > non-trivial PM callbacks to avoid duplication of code (if it didn't exist, > > > > all of the bus types in question would need to provide callbacks with > > > > optional ACPI handling in them). That's all about it. > > > > > > > > And there are bus types that provide non-trivial PM callbacks *and* use > > > > ACPI in them, like PCI, and that is more interleaved with the native PM > > > > in there. For those bus types we can't add devices to the ACPI PM domain > > > > just because they have ACPI companion objects. > > > > > > > > I'm not really sure why it is important here, though. We're talking about > > > > genpd, aren't we? > > > > > > > > I just wanted to indicate that the PM domains concept is not only about > > > > handling power domains and not all of its use cases can be shoehorned into > > > > the same scheme. > > > > > > And by the way, things worked just fine for the ACPI PM domain before commit > > > 46420dd73b80 (PM / Domains: Add APIs to attach/detach a PM domain for a device) > > > which put the ACPI PM domain and genpd into one bag, which was a mistake, > > > because they are different things. > > > > > > > Can we maybe settle on the naming then so that we do not mix them up in > > the future? For me PM domain is group of devices that share certain > > power constraints so that they have to be powered up and down together. > > Is this definition is not correct (for genpd at least)? > > It is correct for genpd, it isn't correct for the ACPI PM domain. > > > And what is the proper definition for ACPI PM domain? > > I agree that the terminology is (somewhat?) confusing. > > From the code perspective, using a PM domain object is a way to provide PM > callbacks that will be executed for a subset of devices instead of or in > addition to the bus type (or class or device type) callbacks. Of course, > that applies to proper power domains in particular, but it can also apply > to broader sets of devices. In the ACPI PM domain case this covers devices > with ACPI power management support (or more precisely, devices with ACPI > companion objects that can provide PM support). In this context the word > "domain" means as much as "area of control" (which is a proper dictionary > definition of it AFAICS). > > genpd is all about proper power domains, like you said. > > That's why I'm saying that they are different and clamping them both > together was a mistake. I overlooked that, my bad. For completeness, please let me quote the changelog of commit 564b905ab10d (PM / Domains: Rename struct dev_power_domain to struct dev_pm_domain) dealing with this particular thing: The naming convention used by commit 7538e3db6e015e890825fbd9f86599b (PM: Add support for device power domains), which introduced the struct dev_power_domain type for representing device power domains, evidently confuses some developers who tend to think that objects of this type must correspond to "power domains" as defined by hardware, which is not the case. Namely, at the kernel level, a struct dev_power_domain object can represent arbitrary set of devices that are mutually dependent power management-wise and need not belong to one hardware power domain. To avoid that confusion, rename struct dev_power_domain to struct dev_pm_domain and rename the related pointers in struct device and struct pm_clk_notifier_block from pwr_domain to pm_domain. And that should be explicitly explained in the PM documentation which I'm going to do. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >> >> Scenario 5), a platform driver with/without runtime PM callbacks. >> ->probe() >> - do some initialization >> - may fetch handles to runtime PM resources >> - pm_runtime_enable() > > Well, and now how the driver knows if the device is "on" before accessing it? In this case the driver don't need to access the device during ->probe(). That's postponed until sometime later. > >> Note 1) >> Scenario 1) and 2), both relies on the approach to power on the PM >> domain by using pm_runtime_get_sync(). That approach didn't work when >> CONFIG_PM_RUNTIME was unset, but we recently decided to fixed that by >> the below patch, so that's good! >> "[PATCH] PM / domains: Kconfig: always enable PM_RUNTIME when genpd enabled" >> >> Note 2) >> Scenario 3) and 4) use the same principles for managing runtime PM. >> These scenarios needs a way to power on the generic PM domain prior >> probing the device. The call to pm_runtime_set_active(), prevents an >> already powered PM domain from power off until after probe, but that's >> not enough. >> >> Note 3) >> The $subject patch, tried to address the issues for scenario 3) and >> 4). It does so, but will affect scenario 5) which was working nicely >> before. In scenario 5), the $subject patch will cause the generic PM >> domain to potentially stay powered after ->probe() even if the device >> is runtime PM suspended. > > Why would it? If the device is runtime-suspended, the domain will know > that, because its callbacks will be used for that. At least, that's > what I'd expect to happen, so is there a problem here? Genpd do knows about the device but it doesn’t get a "notification" to power off. There are no issues whatsoever for driver. This is a somewhat special case. Let's go through an example. 1. The PM domain is initially in powered off state. 2. The bus ->probe() invokes dev_pm_domain_attach() and then the PM domain gets attached to the device. 3. $subject patch causes the PM domain to power on. 4. A driver ->probe() sequence start, following the Scenario 5). 5. The device is initially in runtime PM suspended state and it will remain so during ->probe(). 6. The pm_request_idle() invoked after really_probe() in driver core, won't trigger a runtime PM suspend callback to be invoked. In other words, genpd don't get a "notification" that it may power off. In this state, genpd will either power off from the late_initcall, genpd_poweroff_unused() (depending on when the driver was probed) or wait until some device's runtime PM suspend callback is invoked at any later point. > >> I see three options going forward. >> >> Option 1) >> Convert scenario 3) and 4) into using the pm_runtime_get_sync() >> approach. There are no theoretical obstacles to do this, but pure >> practical. There are a lot of drivers that needs to be converted and >> we also need to teach driver authors future wise to not use >> pm_runtime_set_active() in this manner. > > I'd say we need to do something like this anyway. That is, standardize on > *one* approach. I'm actually not sure what approach is the most useful, > but the pm_runtime_get_sync() one seems to be the most popular to me. > >> Option 2) >> Add some kind of get/put API for PM domains. The buses invokes it to >> control the power to the PM domain. From what I understand, that's >> also what Dmitry think is needed!? >> Anyway, that somehow means to proceed with the approach I took in the >> below patchset. >> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot >> http://marc.info/?t=141320907000003&r=1&w=2 > > I don't like that. The API is already quite complicated in my view and > adding even more complexity to it is not going to help in the long run. I absolutely agree that we shouldn't add unnecessary APIs and keep APIs as simple as possible. In that context, I think the effect from proceeding with Option 2) also means there are no need for the below APIs any more. pm_genpd_poweron() pm_genpd_name_poweron() (requires some additional work though) pm_genpd_poweroff_unused() pm_genpd_dev_need_restore() I guess you figured out that I am in favour of Option 2). :-) Especially since it cover all scenarios and we don't have to go a fix a vast amount of drivers. > >> Option 3) >> Live we the limitation this $subject patch introduces for scenario 5). > > I'd say, 3) for now and 1) going forward. This could work! The hardest part is to know when we should revert $subject patch, to fix the regression introduced for scenario 5). > >> Is there maybe any additional options, that I didn't think of? >> >> Apologize for the long email, I hope I didn't bored you too much. > > That's fine, thanks for taking the time to lay out all of the cases, that's > always helpful. > > Rafael > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, November 19, 2014 09:54:00 AM Ulf Hansson wrote: > [...] > > >> > >> Scenario 5), a platform driver with/without runtime PM callbacks. > >> ->probe() > >> - do some initialization > >> - may fetch handles to runtime PM resources > >> - pm_runtime_enable() > > > > Well, and now how the driver knows if the device is "on" before accessing it? > > In this case the driver don't need to access the device during > ->probe(). That's postponed until sometime later. If this is a platform driver, it rather does need to access the device, precisely because it doesn't know what power state the device is in otherwise. See below. > >> Note 1) > >> Scenario 1) and 2), both relies on the approach to power on the PM > >> domain by using pm_runtime_get_sync(). That approach didn't work when > >> CONFIG_PM_RUNTIME was unset, but we recently decided to fixed that by > >> the below patch, so that's good! > >> "[PATCH] PM / domains: Kconfig: always enable PM_RUNTIME when genpd enabled" > >> > >> Note 2) > >> Scenario 3) and 4) use the same principles for managing runtime PM. > >> These scenarios needs a way to power on the generic PM domain prior > >> probing the device. The call to pm_runtime_set_active(), prevents an > >> already powered PM domain from power off until after probe, but that's > >> not enough. > >> > >> Note 3) > >> The $subject patch, tried to address the issues for scenario 3) and > >> 4). It does so, but will affect scenario 5) which was working nicely > >> before. In scenario 5), the $subject patch will cause the generic PM > >> domain to potentially stay powered after ->probe() even if the device > >> is runtime PM suspended. > > > > Why would it? If the device is runtime-suspended, the domain will know > > that, because its callbacks will be used for that. At least, that's > > what I'd expect to happen, so is there a problem here? > > Genpd do knows about the device but it doesn’t get a "notification" to > power off. There are no issues whatsoever for driver. Except that the driver is arguably buggy. > This is a somewhat special case. Let's go through an example. > > 1. The PM domain is initially in powered off state. > 2. The bus ->probe() invokes dev_pm_domain_attach() and then the PM > domain gets attached to the device. > 3. $subject patch causes the PM domain to power on. > 4. A driver ->probe() sequence start, following the Scenario 5). > 5. The device is initially in runtime PM suspended state and it will > remain so during ->probe(). But is it physically suspended? The runtime PM status of the device after ->probe is required to reflect its real state if runtime PM is enabled. If that's not the case, it is a bug. Now, for platform drivers, the driver can't really assume anything in particular about the current power state of the device at ->probe time, because different platforms including devices handled by that driver may behave differently. A good example would be two platforms A and B where the same device X is in a power domain such that A boots with the domain (physically) "on", while B boots with the domain "off". If the driver for X assumes anything about the initial power state of the device, it may not work on either A or B. > 6. The pm_request_idle() invoked after really_probe() in driver core, > won't trigger a runtime PM suspend callback to be invoked. In other > words, genpd don't get a "notification" that it may power off. > > In this state, genpd will either power off from the late_initcall, > genpd_poweroff_unused() (depending on when the driver was probed) or > wait until some device's runtime PM suspend callback is invoked at any > later point. Which sounds OK to me, so why is it a problem? > >> I see three options going forward. > >> > >> Option 1) > >> Convert scenario 3) and 4) into using the pm_runtime_get_sync() > >> approach. There are no theoretical obstacles to do this, but pure > >> practical. There are a lot of drivers that needs to be converted and > >> we also need to teach driver authors future wise to not use > >> pm_runtime_set_active() in this manner. > > > > I'd say we need to do something like this anyway. That is, standardize on > > *one* approach. I'm actually not sure what approach is the most useful, > > but the pm_runtime_get_sync() one seems to be the most popular to me. > > > >> Option 2) > >> Add some kind of get/put API for PM domains. The buses invokes it to > >> control the power to the PM domain. From what I understand, that's > >> also what Dmitry think is needed!? > >> Anyway, that somehow means to proceed with the approach I took in the > >> below patchset. > >> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot > >> http://marc.info/?t=141320907000003&r=1&w=2 > > > > I don't like that. The API is already quite complicated in my view and > > adding even more complexity to it is not going to help in the long run. > > I absolutely agree that we shouldn't add unnecessary APIs and keep > APIs as simple as possible. > > In that context, I think the effect from proceeding with Option 2) > also means there are no need for the below APIs any more. > pm_genpd_poweron() > pm_genpd_name_poweron() (requires some additional work though) > pm_genpd_poweroff_unused() > pm_genpd_dev_need_restore() > > I guess you figured out that I am in favour of Option 2). :-) > Especially since it cover all scenarios and we don't have to go a fix > a vast amount of drivers. Adding more callbacks to struct dev_pm_domain just in order to handle some genpd-specific use case is out of the question. If you find a way within genpd to address this the way you like, I'm open for ideas. Otherwise, let's just do what I said. > > > >> Option 3) > >> Live we the limitation this $subject patch introduces for scenario 5). > > > > I'd say, 3) for now and 1) going forward. > > This could work! > > The hardest part is to know when we should revert $subject patch, to > fix the regression introduced for scenario 5). Well, I don't think so. First of all, there is a (very) limited number of platforms using genpd today and it should be perfectly possible to go through the platform drivers used by them and see which of those drivers are affected. Fixing them should not be too hard either. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20 November 2014 01:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Wednesday, November 19, 2014 09:54:00 AM Ulf Hansson wrote: >> [...] >> >> >> >> >> Scenario 5), a platform driver with/without runtime PM callbacks. >> >> ->probe() >> >> - do some initialization >> >> - may fetch handles to runtime PM resources >> >> - pm_runtime_enable() >> > >> > Well, and now how the driver knows if the device is "on" before accessing it? >> >> In this case the driver don't need to access the device during >> ->probe(). That's postponed until sometime later. > > If this is a platform driver, it rather does need to access the device, > precisely because it doesn't know what power state the device is in otherwise. > See below. > >> >> Note 1) >> >> Scenario 1) and 2), both relies on the approach to power on the PM >> >> domain by using pm_runtime_get_sync(). That approach didn't work when >> >> CONFIG_PM_RUNTIME was unset, but we recently decided to fixed that by >> >> the below patch, so that's good! >> >> "[PATCH] PM / domains: Kconfig: always enable PM_RUNTIME when genpd enabled" >> >> >> >> Note 2) >> >> Scenario 3) and 4) use the same principles for managing runtime PM. >> >> These scenarios needs a way to power on the generic PM domain prior >> >> probing the device. The call to pm_runtime_set_active(), prevents an >> >> already powered PM domain from power off until after probe, but that's >> >> not enough. >> >> >> >> Note 3) >> >> The $subject patch, tried to address the issues for scenario 3) and >> >> 4). It does so, but will affect scenario 5) which was working nicely >> >> before. In scenario 5), the $subject patch will cause the generic PM >> >> domain to potentially stay powered after ->probe() even if the device >> >> is runtime PM suspended. >> > >> > Why would it? If the device is runtime-suspended, the domain will know >> > that, because its callbacks will be used for that. At least, that's >> > what I'd expect to happen, so is there a problem here? >> >> Genpd do knows about the device but it doesn’t get a "notification" to >> power off. There are no issues whatsoever for driver. > > Except that the driver is arguably buggy. > >> This is a somewhat special case. Let's go through an example. >> >> 1. The PM domain is initially in powered off state. >> 2. The bus ->probe() invokes dev_pm_domain_attach() and then the PM >> domain gets attached to the device. >> 3. $subject patch causes the PM domain to power on. >> 4. A driver ->probe() sequence start, following the Scenario 5). >> 5. The device is initially in runtime PM suspended state and it will >> remain so during ->probe(). > > But is it physically suspended? > > The runtime PM status of the device after ->probe is required to reflect its > real state if runtime PM is enabled. If that's not the case, it is a bug. Agree. While I was searching for drivers that behave as in scenario 5), they tend to register some subsystem specific callbacks and don't access the device until some of those callbacks are invoked. At least that was my interpretation of their ->probe() methods, but it's not always easy to tell how those callbacks are being used for each subsystem. > > Now, for platform drivers, the driver can't really assume anything in > particular about the current power state of the device at ->probe time, > because different platforms including devices handled by that driver may > behave differently. > > A good example would be two platforms A and B where the same device X is in a > power domain such that A boots with the domain (physically) "on", while B boots > with the domain "off". If the driver for X assumes anything about the initial > power state of the device, it may not work on either A or B. I get your point and agree! > >> 6. The pm_request_idle() invoked after really_probe() in driver core, >> won't trigger a runtime PM suspend callback to be invoked. In other >> words, genpd don't get a "notification" that it may power off. >> >> In this state, genpd will either power off from the late_initcall, >> genpd_poweroff_unused() (depending on when the driver was probed) or >> wait until some device's runtime PM suspend callback is invoked at any >> later point. > > Which sounds OK to me, so why is it a problem? The late_initcall doesn't work for modules. Also, suppose the PM domain only holds this "inactive device" that was probed as in scenario 5). Then, you could end up having the PM domain powered, even if it isn't needed. Anyway, I can live with this. It's likely the driver that behave as in scenario 5) that should be fixed as you stated. > >> >> I see three options going forward. >> >> >> >> Option 1) >> >> Convert scenario 3) and 4) into using the pm_runtime_get_sync() >> >> approach. There are no theoretical obstacles to do this, but pure >> >> practical. There are a lot of drivers that needs to be converted and >> >> we also need to teach driver authors future wise to not use >> >> pm_runtime_set_active() in this manner. >> > >> > I'd say we need to do something like this anyway. That is, standardize on >> > *one* approach. I'm actually not sure what approach is the most useful, >> > but the pm_runtime_get_sync() one seems to be the most popular to me. >> > >> >> Option 2) >> >> Add some kind of get/put API for PM domains. The buses invokes it to >> >> control the power to the PM domain. From what I understand, that's >> >> also what Dmitry think is needed!? >> >> Anyway, that somehow means to proceed with the approach I took in the >> >> below patchset. >> >> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot >> >> http://marc.info/?t=141320907000003&r=1&w=2 >> > >> > I don't like that. The API is already quite complicated in my view and >> > adding even more complexity to it is not going to help in the long run. >> >> I absolutely agree that we shouldn't add unnecessary APIs and keep >> APIs as simple as possible. >> >> In that context, I think the effect from proceeding with Option 2) >> also means there are no need for the below APIs any more. >> pm_genpd_poweron() >> pm_genpd_name_poweron() (requires some additional work though) >> pm_genpd_poweroff_unused() >> pm_genpd_dev_need_restore() >> >> I guess you figured out that I am in favour of Option 2). :-) >> Especially since it cover all scenarios and we don't have to go a fix >> a vast amount of drivers. > > Adding more callbacks to struct dev_pm_domain just in order to handle some > genpd-specific use case is out of the question. If you find a way within > genpd to address this the way you like, I'm open for ideas. Otherwise, > let's just do what I said. > >> > >> >> Option 3) >> >> Live we the limitation this $subject patch introduces for scenario 5). >> > >> > I'd say, 3) for now and 1) going forward. >> >> This could work! >> >> The hardest part is to know when we should revert $subject patch, to >> fix the regression introduced for scenario 5). > > Well, I don't think so. > > First of all, there is a (very) limited number of platforms using genpd today > and it should be perfectly possible to go through the platform drivers used > by them and see which of those drivers are affected. Fixing them should not > be too hard either. Considering that scenario 5) probably exists because of buggy drivers and that the related issues is very limited, I am perfectly fine with your proposal! Will you queue this patch then? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/19/2014 10:54 AM, Ulf Hansson wrote: > [...] > >>> >>> Scenario 5), a platform driver with/without runtime PM callbacks. >>> ->probe() >>> - do some initialization >>> - may fetch handles to runtime PM resources >>> - pm_runtime_enable() >> >> Well, and now how the driver knows if the device is "on" before accessing it? > > In this case the driver don't need to access the device during > ->probe(). That's postponed until sometime later. > >> >>> Note 1) >>> Scenario 1) and 2), both relies on the approach to power on the PM >>> domain by using pm_runtime_get_sync(). That approach didn't work when >>> CONFIG_PM_RUNTIME was unset, but we recently decided to fixed that by >>> the below patch, so that's good! >>> "[PATCH] PM / domains: Kconfig: always enable PM_RUNTIME when genpd enabled" >>> >>> Note 2) >>> Scenario 3) and 4) use the same principles for managing runtime PM. >>> These scenarios needs a way to power on the generic PM domain prior >>> probing the device. The call to pm_runtime_set_active(), prevents an >>> already powered PM domain from power off until after probe, but that's >>> not enough. >>> >>> Note 3) >>> The $subject patch, tried to address the issues for scenario 3) and >>> 4). It does so, but will affect scenario 5) which was working nicely >>> before. In scenario 5), the $subject patch will cause the generic PM >>> domain to potentially stay powered after ->probe() even if the device >>> is runtime PM suspended. >> >> Why would it? If the device is runtime-suspended, the domain will know >> that, because its callbacks will be used for that. At least, that's >> what I'd expect to happen, so is there a problem here? > > Genpd do knows about the device but it doesn’t get a "notification" to > power off. There are no issues whatsoever for driver. > > This is a somewhat special case. Let's go through an example. > > 1. The PM domain is initially in powered off state. > 2. The bus ->probe() invokes dev_pm_domain_attach() and then the PM > domain gets attached to the device. > 3. $subject patch causes the PM domain to power on. > 4. A driver ->probe() sequence start, following the Scenario 5). > 5. The device is initially in runtime PM suspended state and it will > remain so during ->probe(). > 6. The pm_request_idle() invoked after really_probe() in driver core, > won't trigger a runtime PM suspend callback to be invoked. In other > words, genpd don't get a "notification" that it may power off. > > In this state, genpd will either power off from the late_initcall, > genpd_poweroff_unused() (depending on when the driver was probed) or > wait until some device's runtime PM suspend callback is invoked at any > later point. if I understand things right (thanks to Russell), the Power domain may not be powered off not only in above case, but also in some cases when driver is unloaded. AMBA bus for example: static int amba_remove(struct device *dev) { pm_runtime_get_sync(dev); <---------- GPD=on, dev is active, usage_count >= 1 ret = drv->remove(pcdev); <----------- GPD=on, should do balancing put() to compensate all get() made by driver, usage_count == 1 <----------- GPD=on, should do balancing get() to compensate put() in probe, usage_count == 2 pm_runtime_put_noidle(dev); <---------- GPD=on, dev is active, usage_count == 1 /* Undo the runtime PM settings in amba_probe() */ pm_runtime_disable(dev); <---------- GPD=on, dev is active, usage_count == 1 pm_runtime_set_suspended(dev); <---------- GPD=on, dev is suspended, usage_count == 1 pm_runtime_put_noidle(dev); <---------- GPD=on, dev is suspended, usage_count == 0 amba_put_disable_pclk(pcdev); dev_pm_domain_detach(dev, true); <---------- GPD=on, dev is suspended, usage_count == 0 also: i2c-qup.c i2c-hix5hd2.c exynos_drm_gsc.c exynos_drm_fimc.c ab8500-gpadc.c ... Is it? > >> >>> I see three options going forward. >>> >>> Option 1) >>> Convert scenario 3) and 4) into using the pm_runtime_get_sync() >>> approach. There are no theoretical obstacles to do this, but pure >>> practical. There are a lot of drivers that needs to be converted and >>> we also need to teach driver authors future wise to not use >>> pm_runtime_set_active() in this manner. >> >> I'd say we need to do something like this anyway. That is, standardize on >> *one* approach. I'm actually not sure what approach is the most useful, >> but the pm_runtime_get_sync() one seems to be the most popular to me. >> >>> Option 2) >>> Add some kind of get/put API for PM domains. The buses invokes it to >>> control the power to the PM domain. From what I understand, that's >>> also what Dmitry think is needed!? >>> Anyway, that somehow means to proceed with the approach I took in the >>> below patchset. >>> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot >>> http://marc.info/?t=141320907000003&r=1&w=2 >> >> I don't like that. The API is already quite complicated in my view and >> adding even more complexity to it is not going to help in the long run. > > I absolutely agree that we shouldn't add unnecessary APIs and keep > APIs as simple as possible. > > In that context, I think the effect from proceeding with Option 2) > also means there are no need for the below APIs any more. > pm_genpd_poweron() > pm_genpd_name_poweron() (requires some additional work though) > pm_genpd_poweroff_unused() > pm_genpd_dev_need_restore() > > I guess you figured out that I am in favour of Option 2). :-) > Especially since it cover all scenarios and we don't have to go a fix > a vast amount of drivers. > >> >>> Option 3) >>> Live we the limitation this $subject patch introduces for scenario 5). >> >> I'd say, 3) for now and 1) going forward. > [...] regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20 November 2014 13:17, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > On 11/19/2014 10:54 AM, Ulf Hansson wrote: >> [...] >> >>>> >>>> Scenario 5), a platform driver with/without runtime PM callbacks. >>>> ->probe() >>>> - do some initialization >>>> - may fetch handles to runtime PM resources >>>> - pm_runtime_enable() >>> >>> Well, and now how the driver knows if the device is "on" before accessing it? >> >> In this case the driver don't need to access the device during >> ->probe(). That's postponed until sometime later. >> >>> >>>> Note 1) >>>> Scenario 1) and 2), both relies on the approach to power on the PM >>>> domain by using pm_runtime_get_sync(). That approach didn't work when >>>> CONFIG_PM_RUNTIME was unset, but we recently decided to fixed that by >>>> the below patch, so that's good! >>>> "[PATCH] PM / domains: Kconfig: always enable PM_RUNTIME when genpd enabled" >>>> >>>> Note 2) >>>> Scenario 3) and 4) use the same principles for managing runtime PM. >>>> These scenarios needs a way to power on the generic PM domain prior >>>> probing the device. The call to pm_runtime_set_active(), prevents an >>>> already powered PM domain from power off until after probe, but that's >>>> not enough. >>>> >>>> Note 3) >>>> The $subject patch, tried to address the issues for scenario 3) and >>>> 4). It does so, but will affect scenario 5) which was working nicely >>>> before. In scenario 5), the $subject patch will cause the generic PM >>>> domain to potentially stay powered after ->probe() even if the device >>>> is runtime PM suspended. >>> >>> Why would it? If the device is runtime-suspended, the domain will know >>> that, because its callbacks will be used for that. At least, that's >>> what I'd expect to happen, so is there a problem here? >> >> Genpd do knows about the device but it doesn’t get a "notification" to >> power off. There are no issues whatsoever for driver. >> >> This is a somewhat special case. Let's go through an example. >> >> 1. The PM domain is initially in powered off state. >> 2. The bus ->probe() invokes dev_pm_domain_attach() and then the PM >> domain gets attached to the device. >> 3. $subject patch causes the PM domain to power on. >> 4. A driver ->probe() sequence start, following the Scenario 5). >> 5. The device is initially in runtime PM suspended state and it will >> remain so during ->probe(). >> 6. The pm_request_idle() invoked after really_probe() in driver core, >> won't trigger a runtime PM suspend callback to be invoked. In other >> words, genpd don't get a "notification" that it may power off. >> >> In this state, genpd will either power off from the late_initcall, >> genpd_poweroff_unused() (depending on when the driver was probed) or >> wait until some device's runtime PM suspend callback is invoked at any >> later point. > > if I understand things right (thanks to Russell), the Power domain may not > be powered off not only in above case, but also in some cases when > driver is unloaded. > > AMBA bus for example: > static int amba_remove(struct device *dev) > { > pm_runtime_get_sync(dev); <---------- GPD=on, dev is active, usage_count >= 1 > ret = drv->remove(pcdev); <----------- GPD=on, should do balancing put() to compensate all get() made by driver, usage_count == 1 > <----------- GPD=on, should do balancing get() to compensate put() in probe, usage_count == 2 > pm_runtime_put_noidle(dev); <---------- GPD=on, dev is active, usage_count == 1 > > /* Undo the runtime PM settings in amba_probe() */ > pm_runtime_disable(dev); <---------- GPD=on, dev is active, usage_count == 1 > pm_runtime_set_suspended(dev); <---------- GPD=on, dev is suspended, usage_count == 1 > pm_runtime_put_noidle(dev); <---------- GPD=on, dev is suspended, usage_count == 0 > > amba_put_disable_pclk(pcdev); > dev_pm_domain_detach(dev, true); <---------- GPD=on, dev is suspended, usage_count == 0 For the generic OF-based PM domain look-up case: ->dev_pm_domain_detach() ->genpd_dev_pm_detach() - to remove the device from the PM domain. ->genpd_queue_power_off_work() - to power off unused PM domains. That does the trick, right!? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/20/2014 03:01 PM, Ulf Hansson wrote: > On 20 November 2014 13:17, Grygorii Strashko <grygorii.strashko@ti.com> wrote: >> On 11/19/2014 10:54 AM, Ulf Hansson wrote: >>> [...] >>> >>>>> >>>>> Scenario 5), a platform driver with/without runtime PM callbacks. >>>>> ->probe() >>>>> - do some initialization >>>>> - may fetch handles to runtime PM resources >>>>> - pm_runtime_enable() >>>> >>>> Well, and now how the driver knows if the device is "on" before accessing it? >>> >>> In this case the driver don't need to access the device during >>> ->probe(). That's postponed until sometime later. >>> >>>> >>>>> Note 1) >>>>> Scenario 1) and 2), both relies on the approach to power on the PM >>>>> domain by using pm_runtime_get_sync(). That approach didn't work when >>>>> CONFIG_PM_RUNTIME was unset, but we recently decided to fixed that by >>>>> the below patch, so that's good! >>>>> "[PATCH] PM / domains: Kconfig: always enable PM_RUNTIME when genpd enabled" >>>>> >>>>> Note 2) >>>>> Scenario 3) and 4) use the same principles for managing runtime PM. >>>>> These scenarios needs a way to power on the generic PM domain prior >>>>> probing the device. The call to pm_runtime_set_active(), prevents an >>>>> already powered PM domain from power off until after probe, but that's >>>>> not enough. >>>>> >>>>> Note 3) >>>>> The $subject patch, tried to address the issues for scenario 3) and >>>>> 4). It does so, but will affect scenario 5) which was working nicely >>>>> before. In scenario 5), the $subject patch will cause the generic PM >>>>> domain to potentially stay powered after ->probe() even if the device >>>>> is runtime PM suspended. >>>> >>>> Why would it? If the device is runtime-suspended, the domain will know >>>> that, because its callbacks will be used for that. At least, that's >>>> what I'd expect to happen, so is there a problem here? >>> >>> Genpd do knows about the device but it doesn’t get a "notification" to >>> power off. There are no issues whatsoever for driver. >>> >>> This is a somewhat special case. Let's go through an example. >>> >>> 1. The PM domain is initially in powered off state. >>> 2. The bus ->probe() invokes dev_pm_domain_attach() and then the PM >>> domain gets attached to the device. >>> 3. $subject patch causes the PM domain to power on. >>> 4. A driver ->probe() sequence start, following the Scenario 5). >>> 5. The device is initially in runtime PM suspended state and it will >>> remain so during ->probe(). >>> 6. The pm_request_idle() invoked after really_probe() in driver core, >>> won't trigger a runtime PM suspend callback to be invoked. In other >>> words, genpd don't get a "notification" that it may power off. >>> >>> In this state, genpd will either power off from the late_initcall, >>> genpd_poweroff_unused() (depending on when the driver was probed) or >>> wait until some device's runtime PM suspend callback is invoked at any >>> later point. >> >> if I understand things right (thanks to Russell), the Power domain may not >> be powered off not only in above case, but also in some cases when >> driver is unloaded. >> >> AMBA bus for example: >> static int amba_remove(struct device *dev) >> { >> pm_runtime_get_sync(dev); <---------- GPD=on, dev is active, usage_count >= 1 >> ret = drv->remove(pcdev); <----------- GPD=on, should do balancing put() to compensate all get() made by driver, usage_count == 1 >> <----------- GPD=on, should do balancing get() to compensate put() in probe, usage_count == 2 >> pm_runtime_put_noidle(dev); <---------- GPD=on, dev is active, usage_count == 1 >> >> /* Undo the runtime PM settings in amba_probe() */ >> pm_runtime_disable(dev); <---------- GPD=on, dev is active, usage_count == 1 >> pm_runtime_set_suspended(dev); <---------- GPD=on, dev is suspended, usage_count == 1 >> pm_runtime_put_noidle(dev); <---------- GPD=on, dev is suspended, usage_count == 0 >> >> amba_put_disable_pclk(pcdev); >> dev_pm_domain_detach(dev, true); <---------- GPD=on, dev is suspended, usage_count == 0 > > For the generic OF-based PM domain look-up case: > ->dev_pm_domain_detach() > ->genpd_dev_pm_detach() - to remove the device from the PM domain. > ->genpd_queue_power_off_work() - to power off unused PM domains. > > That does the trick, right!? True. Thanks a lot for pointing me on right place. regards, -grygorii -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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 20, 2014 11:13:01 AM Ulf Hansson wrote: > On 20 November 2014 01:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Wednesday, November 19, 2014 09:54:00 AM Ulf Hansson wrote: > >> [...] > >> > >> >> > >> >> Scenario 5), a platform driver with/without runtime PM callbacks. > >> >> ->probe() > >> >> - do some initialization > >> >> - may fetch handles to runtime PM resources > >> >> - pm_runtime_enable() > >> > > >> > Well, and now how the driver knows if the device is "on" before accessing it? > >> > >> In this case the driver don't need to access the device during > >> ->probe(). That's postponed until sometime later. > > > > If this is a platform driver, it rather does need to access the device, > > precisely because it doesn't know what power state the device is in otherwise. > > See below. > > > >> >> Note 1) > >> >> Scenario 1) and 2), both relies on the approach to power on the PM > >> >> domain by using pm_runtime_get_sync(). That approach didn't work when > >> >> CONFIG_PM_RUNTIME was unset, but we recently decided to fixed that by > >> >> the below patch, so that's good! > >> >> "[PATCH] PM / domains: Kconfig: always enable PM_RUNTIME when genpd enabled" > >> >> > >> >> Note 2) > >> >> Scenario 3) and 4) use the same principles for managing runtime PM. > >> >> These scenarios needs a way to power on the generic PM domain prior > >> >> probing the device. The call to pm_runtime_set_active(), prevents an > >> >> already powered PM domain from power off until after probe, but that's > >> >> not enough. > >> >> > >> >> Note 3) > >> >> The $subject patch, tried to address the issues for scenario 3) and > >> >> 4). It does so, but will affect scenario 5) which was working nicely > >> >> before. In scenario 5), the $subject patch will cause the generic PM > >> >> domain to potentially stay powered after ->probe() even if the device > >> >> is runtime PM suspended. > >> > > >> > Why would it? If the device is runtime-suspended, the domain will know > >> > that, because its callbacks will be used for that. At least, that's > >> > what I'd expect to happen, so is there a problem here? > >> > >> Genpd do knows about the device but it doesn’t get a "notification" to > >> power off. There are no issues whatsoever for driver. > > > > Except that the driver is arguably buggy. > > > >> This is a somewhat special case. Let's go through an example. > >> > >> 1. The PM domain is initially in powered off state. > >> 2. The bus ->probe() invokes dev_pm_domain_attach() and then the PM > >> domain gets attached to the device. > >> 3. $subject patch causes the PM domain to power on. > >> 4. A driver ->probe() sequence start, following the Scenario 5). > >> 5. The device is initially in runtime PM suspended state and it will > >> remain so during ->probe(). > > > > But is it physically suspended? > > > > The runtime PM status of the device after ->probe is required to reflect its > > real state if runtime PM is enabled. If that's not the case, it is a bug. > > Agree. > > While I was searching for drivers that behave as in scenario 5), they > tend to register some subsystem specific callbacks and don't access > the device until some of those callbacks are invoked. > > At least that was my interpretation of their ->probe() methods, but > it's not always easy to tell how those callbacks are being used for > each subsystem. > > > > > Now, for platform drivers, the driver can't really assume anything in > > particular about the current power state of the device at ->probe time, > > because different platforms including devices handled by that driver may > > behave differently. > > > > A good example would be two platforms A and B where the same device X is in a > > power domain such that A boots with the domain (physically) "on", while B boots > > with the domain "off". If the driver for X assumes anything about the initial > > power state of the device, it may not work on either A or B. > > I get your point and agree! > > > > >> 6. The pm_request_idle() invoked after really_probe() in driver core, > >> won't trigger a runtime PM suspend callback to be invoked. In other > >> words, genpd don't get a "notification" that it may power off. > >> > >> In this state, genpd will either power off from the late_initcall, > >> genpd_poweroff_unused() (depending on when the driver was probed) or > >> wait until some device's runtime PM suspend callback is invoked at any > >> later point. > > > > Which sounds OK to me, so why is it a problem? > > The late_initcall doesn't work for modules. > > Also, suppose the PM domain only holds this "inactive device" that was > probed as in scenario 5). Then, you could end up having the PM domain > powered, even if it isn't needed. > > Anyway, I can live with this. It's likely the driver that behave as in > scenario 5) that should be fixed as you stated. > > > > >> >> I see three options going forward. > >> >> > >> >> Option 1) > >> >> Convert scenario 3) and 4) into using the pm_runtime_get_sync() > >> >> approach. There are no theoretical obstacles to do this, but pure > >> >> practical. There are a lot of drivers that needs to be converted and > >> >> we also need to teach driver authors future wise to not use > >> >> pm_runtime_set_active() in this manner. > >> > > >> > I'd say we need to do something like this anyway. That is, standardize on > >> > *one* approach. I'm actually not sure what approach is the most useful, > >> > but the pm_runtime_get_sync() one seems to be the most popular to me. > >> > > >> >> Option 2) > >> >> Add some kind of get/put API for PM domains. The buses invokes it to > >> >> control the power to the PM domain. From what I understand, that's > >> >> also what Dmitry think is needed!? > >> >> Anyway, that somehow means to proceed with the approach I took in the > >> >> below patchset. > >> >> [PATCH v3 0/9] PM / Domains: Fix race conditions during boot > >> >> http://marc.info/?t=141320907000003&r=1&w=2 > >> > > >> > I don't like that. The API is already quite complicated in my view and > >> > adding even more complexity to it is not going to help in the long run. > >> > >> I absolutely agree that we shouldn't add unnecessary APIs and keep > >> APIs as simple as possible. > >> > >> In that context, I think the effect from proceeding with Option 2) > >> also means there are no need for the below APIs any more. > >> pm_genpd_poweron() > >> pm_genpd_name_poweron() (requires some additional work though) > >> pm_genpd_poweroff_unused() > >> pm_genpd_dev_need_restore() > >> > >> I guess you figured out that I am in favour of Option 2). :-) > >> Especially since it cover all scenarios and we don't have to go a fix > >> a vast amount of drivers. > > > > Adding more callbacks to struct dev_pm_domain just in order to handle some > > genpd-specific use case is out of the question. If you find a way within > > genpd to address this the way you like, I'm open for ideas. Otherwise, > > let's just do what I said. > > > >> > > >> >> Option 3) > >> >> Live we the limitation this $subject patch introduces for scenario 5). > >> > > >> > I'd say, 3) for now and 1) going forward. > >> > >> This could work! > >> > >> The hardest part is to know when we should revert $subject patch, to > >> fix the regression introduced for scenario 5). > > > > Well, I don't think so. > > > > First of all, there is a (very) limited number of platforms using genpd today > > and it should be perfectly possible to go through the platform drivers used > > by them and see which of those drivers are affected. Fixing them should not > > be too hard either. > > Considering that scenario 5) probably exists because of buggy drivers > and that the related issues is very limited, I am perfectly fine with > your proposal! > > Will you queue this patch then? I will, but I'll change the first line of the changelog to read: "Vast amount of platform drivers enables runtime PM, [...]" (ie. leave out AMBA from there). Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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/base/power/domain.c b/drivers/base/power/domain.c index 3989eb6..1bfb54c 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -2235,6 +2235,7 @@ int genpd_dev_pm_attach(struct device *dev) } dev->pm_domain->detach = genpd_dev_pm_detach; + pm_genpd_poweron(pd); return 0; }
The amba bus, amba drivers and a vast amount of platform drivers which enables runtime PM, don't invoke a pm_runtime_get_sync() while probing their devices. Instead, once they have turned on their PM resourses during ->probe() and are ready to handle I/O, these invokes pm_runtime_set_active() to synchronize its state towards the runtime PM core. From a runtime PM point of view this behavior is perfectly acceptable, but we encounter probe failures if their corresponding devices resides in the generic PM domain. The issues are observed for those devices, which requires its PM domain to stay powered during ->probe() since that's not being controlled. While using the generic OF-based PM domain look-up, a device's PM domain will be attached during the probe sequence. For this path, let's fix the probe failures, by simply power on the PM domain right after when it's been attached to the device. The generic PM domain stays powered until all of its devices becomes runtime PM enabled and runtime PM suspended. The old SOCs which makes use of the generic PM domain but don't use the generic OF-based PM domain look-up, will not be affected from this change. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/base/power/domain.c | 1 + 1 file changed, 1 insertion(+)