[PATCH v3] PCI: hosts: mark pcie/pci (msi) irq cascade handler as IRQF_NO_THREAD
On -RT and if kernel is booting with "threadirqs" cmd line parameter
pcie/pci (msi) irq cascade handlers (like dra7xx_pcie_msi_irq_handler())
will be forced threaded and, as result, will generate warnings like:
WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150
handle_irq_event_percpu+0x14c/0x174()
irq 460 handler irq_default_primary_handler+0x0/0x14 enabled interrupts
Backtrace:
(warn_slowpath_common) from [] (warn_slowpath_fmt+0x38/0x40)
(warn_slowpath_fmt) from [] (handle_irq_event_percpu+0x14c/0x174)
(handle_irq_event_percpu) from [] (handle_irq_event+0x84/0xb8)
(handle_irq_event) from [] (handle_simple_irq+0x90/0x118)
(handle_simple_irq) from [] (generic_handle_irq+0x30/0x44)
(generic_handle_irq) from [] (dra7xx_pcie_msi_irq_handler+0x7c/0x8c)
(dra7xx_pcie_msi_irq_handler) from []
(irq_forced_thread_fn+0x28/0x5c)
(irq_forced_thread_fn) from [] (irq_thread+0x128/0x204)
This happens because all of them invoke generic_handle_irq() from the
requsted handler. generic_handle_irq grabs raw_locks and this needs to
run in raw-irq context.
This issue was originally reproduced on TI dra7-evem, but, as was
identified during dicussion [1], other PCI(e) hosts can also suffer
from this issue. So let's fix all them at once and mark pcie/pci (msi)
irq cascade handlers IRQF_NO_THREAD explicitly.
[1] https://lkml.org/lkml/2015/11/20/356
Cc: Kishon Vijay Abraham I <kis...@ti.com>
Cc: Bjorn Helgaas <bhelg...@google.com>
Cc: Jingoo Han <jingooh...@gmail.com>
Cc: Kukjin Kim <kg...@kernel.org>
Cc: Krzysztof Kozlowski <k.kozlow...@samsung.com>
Cc: Richard Zhu <richard@freescale.com>
Cc: Lucas Stach <l.st...@pengutronix.de>
Cc: Thierry Reding <thierry.red...@gmail.com>
Cc: Stephen Warren <swar...@wwwdotorg.org>
Cc: Alexandre Courbot <gnu...@gmail.com>
Cc: Simon Horman <ho...@verge.net.au>
Cc: Pratyush Anand <pratyush.an...@gmail.com>
Cc: Michal Simek <michal.si...@xilinx.com>
Cc: "Sören Brinkmann" <soren.brinkm...@xilinx.com>
Cc: Sebastian Andrzej Siewior <bige...@linutronix.de>
Signed-off-by: Grygorii Strashko <grygorii.stras...@ti.com>
---
Changes in v3:
- change applied to all affected pci(e) host drivers in drivers/pci/hosts.
After some invsetigation I've decided to not touch arch code - it is not easy
to identify all places which need to be fixed.
if it's still required - i can send separate patches for
arch/mips/pci/msi-octeon.c and arch/sparc/kernel/pci_msi.c.
Links
v2: https://lkml.org/lkml/2015/11/20/356
v1: https://lkml.org/lkml/2015/11/5/593
ref: https://lkml.org/lkml/2015/11/3/660
drivers/pci/host/pci-dra7xx.c | 13 -
drivers/pci/host/pci-exynos.c | 3 ++-
drivers/pci/host/pci-imx6.c | 3 ++-
drivers/pci/host/pci-tegra.c | 2 +-
drivers/pci/host/pcie-rcar.c | 6 --
drivers/pci/host/pcie-spear13xx.c | 3 ++-
drivers/pci/host/pcie-xilinx.c| 3 ++-
7 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
index 8c36880..0415192 100644
--- a/drivers/pci/host/pci-dra7xx.c
+++ b/drivers/pci/host/pci-dra7xx.c
@@ -301,8 +301,19 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie
*dra7xx,
return -EINVAL;
}
+ /*
+* Mark dra7xx_pcie_msi IRQ as IRQF_NO_THREAD
+* On -RT and if kernel is booting with "threadirqs" cmd line parameter
+* the dra7xx_pcie_msi_irq_handler() will be forced threaded but,
+* in the same time, it's IRQ dispatcher and calls generic_handle_irq(),
+* which, in turn, will be resolved to handle_simple_irq() call.
+* The handle_simple_irq() expected to be called with IRQ disabled, as
+* result kernle will display warning:
+* "irq XXX handler YYY+0x0/0x14 enabled interrupts".
+*/
ret = devm_request_irq(>dev, pp->irq,
- dra7xx_pcie_msi_irq_handler, IRQF_SHARED,
+ dra7xx_pcie_msi_irq_handler,
+ IRQF_SHARED | IRQF_NO_THREAD,
"dra7-pcie-msi", pp);
if (ret) {
dev_err(>dev, "failed to request irq\n");
diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
index 01095e1..d997d22 100644
--- a/drivers/pci/host/pci-exynos.c
+++ b/drivers/pci/host/pci-exynos.c
@@ -522,7 +522,8 @@ static int __init exynos_add_pcie_port(struct pcie_port *pp,
ret = devm_request_irq(>dev, pp->msi_irq,
exynos_pcie_msi_irq_handler,
- IRQF_SHARED, "exynos-pcie", pp);
+ IRQF_SHARED | IRQF_NO_THREAD,
+ "exynos-pcie", pp);
if (ret) {
Re: [PATCH] PM / Domains: Power on the PM domain right after attach completes
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=14132090703r=1w=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
Re: [PATCH] PM / Domains: Power on the PM domain right after attach completes
On 11/20/2014 03:01 PM, Ulf Hansson wrote:
On 20 November 2014 13:17, Grygorii Strashko grygorii.stras...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
On 11/13/2014 04:07 AM, Rafael J. Wysocki wrote:
On Friday, November 07, 2014 07:25:08 PM Grygorii Strashko wrote:
[cut]
4) I've copied here comment from Rafael:
Of course, if -probe() is to call pm_runtime_resume() for this
purpose,
it must take the fact that the driver's own -runtime_resume() may be
called
as a result of this into account.
Agree, that's a little bit annoying, but we are living with that for more
then
5 years already (I'm 3 years) - so, I am, as driver developer, expecting
above behavior
(just walk through the drivers and you will see how many drivers expecting
the same).
So, any volunteers to check and fix ~500 drivers.
Where did you get that number from?
Sry, my bad. Rechecked - found 289 occurrences of pm_runtime_enable().
Number of Runtime PM enabled drivers = at about 289 +-10%.
- headers, suspend/resume,..
+ buses like amba and spmi
Also please note that some bus types don't have this problem by design (e.g.
PCI,
as pointed to by Alan).
Right. I worry about Platform bus first of all, because HW PM implementation
(at least for ARM SoCs) can be very different there.
[cut]
- Runtime PM (if compiled in) needs to be enabled for all devices in power
domains by default. Otherwise devices may lose power as a result for
power management of the other devices in the same domain.
It should prevent enabling/disabling of RPM from sys_fs too.
It looks like you're confusing disable/enable with auto/on. These are
different
things.
- The core should try to power up domains before calling really_probe()
both
for CONFIG_PM_RUNTIME set and unset, so -probe() can always make the
device is accessible assumption.
Here I'm still think that pm_runtime_get_sync() (or similar) API should work.
Another way, PM domain should decide what to do when the first device
attached to it
- power up and stay powered on for example
(It will work if devices are attached before -probe();
it will not work if devices will be attached once they've been created,
but this is different
question)
The PM domain itself can't do that. The only place that has enough knowledge
is the code that enumerates devices (DT, ACPI or board-specific).
And how exactly will you then power up the PM domain when
CONFIG_PM_RUNTIME is unset?
- Bus types may need to do more on top of that in their -probe(), so the
driver's -probe() can make that assumption too in all cases.
Does that make sense to you?
I would like to take the liberty to add a couple of points from me:
- it seems reasonable to have ability to disable Runtime PM globally from
sys_fs:
once disabled - get should power on device, put should do nothing
all the
time except when -remove() is called.
This is how the power/control file works and you can easily have this by
writing
on to that file for every device.
- It might be reasonable to add API like pm_runtime_probe_getXXX() which
will do
everything what standard pm_runtime_get_sync() is doing with one
exception:
it will not call driver's .runtime_resume() callback.
Use case?
This is just a different view (RFC) on your idea to call pm_runtime_get_sync()
before dev-driver is initialized
((b) bypassing the driver callbacks somehow.
http://marc.info/?l=linux-pmm=141505768026211w=2)
- add some flag like dev-is_probing
- in pm_runtime_probe_get_sync() do
dev-is_probing = true;
pm_runtime_get_sync();
dev-is_probing = false;
- update 3 places in code pm_generic_runtime_resume,
pm_genpd_default_save_state and
__rpm_get_callback like below (only for .runtime_resume):
+++ b/drivers/base/power/generic_ops.c
@@ -42,6 +42,9 @@ int pm_generic_runtime_resume(struct device *dev)
{
const struct dev_pm_ops *pm = dev-driver ? dev-driver-pm : NULL;
int ret;
+
+ if (dev-is_probing)
+ pm = NULL;
Then drivers (at least platform drivers) will be able to call
pm_runtime_enable(dev);
pm_runtime_probe_get_sync(dev);
at any time they think is right (no problem with parent devices,
Power domain will be enabled, class/type/bus callback will be called,
no need to use pm_runtime_set_active()).
regards,
-grygorii
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
Hi Ulf, On 11/11/2014 01:05 PM, Ulf Hansson wrote: [...] 2) There are no requirements for arch/platforms/drivers to work in both cases CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both cases. For cross SoC drivers this statement is not correct! Driver _must_ support the various combinations of CONFIG_PM*. Therefore, I think it's better to strive towards a common solution and to get the building blocks in place. There are no common solution ;), because CONFIG_PM_GENERIC_DOMAINS can be n too :) and number of combinations of config options to handle will grow. Right? What if voltage or clocks domains will be added? So, everything is about the decision made for arch/platform/soc/driver and such problems should be solved individually. Look, once you'll add in core code smth. like in this series - the driver's code will partially lose ability to control PM. This, in turn, may lead to the fact that PM management for some drivers or even SoC will be impossible to add (assume you know that HW engineers can do absolutely crazy things to reach power savings targets). For example, for Keystone 2 only CONFIG_PM_RUNTIME=y is going to be supported and if some one decided to disable it - it can be perfectly done from sys_fs/ user space. I can't see why the sysfs option to disable runtime PM should affect this discussion. That's done at runtime and after the device has been probed. 3) pm_runtime_get_sync()(or similar) is good not only because i's waking up device, but also because: - it can wake up chain of devices (dev-parent-parent-...) That's right. But that's not what this patchset aims to do. I realize that the header of the cover letter isn't describing the problem I am trying to solve very well. I guess the below header would have been better: PM / Domains: Power up PM domains prior drivers starts to probe their devices Yep. ;) You've mixed a lot of things together in your comments (. And if i understand right - first of all you are fixing the issue for the case CONFIG_PM_RUNTIME=y and, in my opinion, the root cause of such issues is exactly the usage of pm_runtime_set_active(), because it breaks symmetry of Runtime PM. As for me It much more simple to deal with well known limitation of Runtime PM: calling of driver's own -runtime_resume() when pm_runtime_resume() called from probe and still benefit from all features provided by pm_runtime_get/put() than playing with pm_runtime_set_active() and: - fixing unbalanced get/put calls (good example is http://marc.info/?l=linux-pmm=141580289601886w=2 where unexpected (as for me) call of pm_runtime_get_noresume() is added in .remove() callback); - fixing PD - fixing enabling of parent devices (it will happen, believe me;). - it can wake up power domain Yes, but it requires CONFIG_PM_RUNTIME to be set. Thus, to solve the problem during driver -probe() we need another solution which don't require CONFIG_PM_RUNTIME to be set. As this patchset proposes. - it connects device to domain/class/type/bus and so allow to add additional PM layer on top of Platform bus (for example arch/arm/mach-omap2/omap_device.c). So, it will do all needed things, and if it doesn't that problem is in platfrom/bus/driver code and not in Runtime PM. if pm_runtime_get_sync() will be dropped - than all above will need to be implemented around the -probe(). I am not sure what you mean about dropping pm_runtime_get_sync()? All I am saying is that we can't use it to power on PM domains during the probe sequence. What I'm saying is that it should work with pm_runtime_get_sync for the case CONFIG_PM_RUNTIME=y ;) and It might be better to power on PD once the first device is attached to it if CONFIG_PM_RUNTIME=n and do this inside GPD code. (^ it could even make your patch smaller) Of course, you may still use pm_runtime_get_sync() from anywhere it's needed, to for example handle device's parent/child relationships. 4) I've copied here comment from Rafael: Of course, if -probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own -runtime_resume() may be called as a result of this into account. Agree, that's a little bit annoying, but we are living with that for more then 5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior (just walk through the drivers and you will see how many drivers expecting the same). So, any volunteers to check and fix ~500 drivers. We don't have to change these drivers. An certainly they are not 500. :-) +- 10% - I've calculated number of occurrences of pm_runtime_enable() :) They will still work as is! Though we need this fix to comply with them, which is supposed to go in for 3.18 rc[n]. PM / Domains: Fix initial default state of the need_restore flag Ok. Looks like we both still hold one's ground :) I give up (found at about
Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot
Hi All, Uh... just finished reading this thread and finally decided to add my 5 cents here as I've already had very bad experience with some changes introduced to DD/PM core :( (just check the history of the change 1e2ef05bb PM: Limit race conditions between runtime PM and system sleep (v2) On 11/04/2014 06:42 PM, Ulf Hansson wrote: On 4 November 2014 14:51, Rafael J. Wysocki r...@rjwysocki.net wrote: On Tuesday, November 04, 2014 09:54:19 AM Ulf Hansson wrote: [...] Generally, there are two or even three levels of runtime PM handling, driver, (possibly) bus type and (possibly) PM domain (and multiple levels of these are possible in principle). All of them have to be initialized at different times. Quite arguably, the PM domain and/or bus type runtime PM handling should be initialized even before registerind the device or during device registration. Doing that later may be too late. When the device has been registered, runtime PM should work to an extent allowing the driver to access the device and configure it further after calling pm_runtime_resume(). Of course, if -probe() is to call pm_runtime_resume() for this purpose, it must take the fact that the driver's own -runtime_resume() may be called as a result of this into account. That's why I'm asking whether or not the core should call pm_runtime_resume() before calling really_probe() in a followup branch of this thread. I am reading the other thread, let's see. The driver's own runtime PM handling must be initialized in the driver and the only place suitable for that is -probe(). However, it needs to be done *before* the driver's own -runtime_resume() or -runtime_suspend() callback is executed. If that is done properly, it should be possible to cover both the CONFIG_PM_RUNTIME set/unset cases in that code. And I wouldn't recommend anyone to do the runtime PM initialization in -runtime_resume() (when it is called for the first time), as that would be error prone and fragile. Great! That's means we are at least aligned on this topic. :-) The AMBA bus and some of its drivers a good example of how this has been implemented: driver/amba/bus.c drivers/mmc/host/mmci.c drivers/spi/spi-pl022.c This conclusion I have made from this is: - Using pm_runtime_get_sync() during the -probe() path to explicitly power up a PM domain, is not suitable as the _common_ solution to solve the race condition. It certainly may work for some scenarios, but not for those I am looking at. Amba bus is not good example as for me, because it's cheating ;) Why? Bus/Host can't know when its child device is powered on or not unless it's been notified directly about that and, therefor it's not correct to call pm_runtime_set_active() from amba_probe() - instead it should be called from driver. For example: drivers/mmc/host/mmci.c will be active only when its functional clock is enabled. Apparently someone decided to save few lines of code. [Pls, don't comment here unless you have read all my reply :)] I think, however, that it might work if the core calls pm_runtime_get_sync() from driver_probe_device(). Currently this won't work. That's because the buses' -probe() are invoked in this path and they are doing the attachment of the device to its PM domain. In other words, we can't power up the PM domain using pm_runtime_get_sync(), until the device has been attached to its PM domain. Right? Yes, but my point was that those bus types might need to be changed. We can't make everyone happy at the same time if their ideas about what to do are different. Urgh. I fail to understand this comment. Why do we prefer the pm_runtime_get_sync() solution in favour of this pathset's approach? What are the benefit do we get with pm_runtime_get_sync()? So, few points below: 1) It will be good to treat Generic Power domain as new feature (in 3.18 there are only 5 users of it). From another side, there are at about 500 users of Runtime PM. So, Problems discussed here could be treated as not related to Runtime PM but rather related to GPD ;) For example (lets use AMBA bus): - after the commit 207f1a2d amba: Add support for attach/detach of PM domains and if any device of amba bus will be added to some GPD then amba_pm_runtime_suspend/resume callbacks will never be called for this device and apb_pclk will not be enabled/disabled. Simply because GPD will set dev-pm_domain == genpd-domain, which PM ops will have higher priority for Runtime PM than Bus PM ops. 2) There are no requirements for arch/platforms/drivers to work in both cases CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both cases. For example, for Keystone 2 only CONFIG_PM_RUNTIME=y is going to be supported and if some one decided to disable it - it can be perfectly done from sys_fs/ user space. 3) pm_runtime_get_sync()(or similar) is good not only because i's waking up device, but also because: - it can wake
Re: [RFC 1/2] PM / Domains: Power on domain early during system resume
On 11/03/2014 06:13 PM, Kevin Hilman wrote:
Andrzej Hajda a.ha...@samsung.com writes:
On 10/30/2014 08:36 AM, Krzysztof Kozlowski wrote:
On śro, 2014-10-29 at 10:46 -0700, Kevin Hilman wrote:
Krzysztof Kozlowski k.kozlow...@samsung.com writes:
When resuming the system the power domain has to be powered on early so
any runtime PM aware devices could resume.
This fixes following scenario reproduced on Exynos DRM:
1. Power domain is off before suspending the system.
2. System is suspended to RAM.
3. Resuming starts. The Exynos DRM driver resume callback is called.
4. The Exynos DRM driver calls drm_helper_resume_force_mode which turns
the screen on by calling exynos_dsi_dpms with DRM_MODE_DPMS_ON.
Dumb Q: if the device (and power domain) were off before (and during)
suspend, why are they being resumed?
Shouldn't the resume path restore things to the same state they were
before suspend?
One could expect that... but the Exynos DRM driver behaves differently
(and some other drivers also). In resume method it calls
drm_helper_resume_force_mode() which forces restoring mode setting
configuration. Apparently setting a mode needs DPMS on:
static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
{
...
exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
...
The previous DPMS status (status during suspend) is completely ignored
here.
Suspend callback switches off all connectors (thus all other devs in
their pipeline) by calling dpms_off,
in restore callback all devs are restored to their previous state by
calling appropriate dpms.
So I guess drm_helper_resume_force_mode() call at the end of resume is
incorrect.
Though I'm not terribly familiar with DRM, it seems incorrect because I
expect resume to restore the state of things when suspend happened, not
forcibly resume everything.
On the other side it is present in many other drivers, so I am also
little bit confused.
Many other DRM drivers? or other drivers too?
If I understand GPD code right :)
There is small problem :( Now if PM domain is OFF before suspend
- it's prohibited to turn it on during suspending/resuming.
commit 596ba34bcd2978ee9823cc1d84df230576f8ffb9
PM / Domains: System-wide transitions support for generic domains (v5)
But, it is possible to have devices which supports few power states, like:
on, sleep/low power, off (for example OMAP devices and also I saw this
for some I2C devices - can recollect only that it was some sensor and
touchscreen).
In normal operational mode Runtime PM switches device between on and sleep/low
power states,
but during suspend device need to be woken up and reconfigured to off state.
Also I found, that It looks like due to continuous refactoring the call of
pm_generic_suspend_noirq(dev) was finally dropped from pm_genpd_suspend_noirq(),
so .suspend_noirq() callback will never be called for devices from GPD.
Seems, that is the problem which this patch tries to fix and looks like there
are
will be more such kind of report as GPD is become used widely.
regards,
-grygorii
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/2] PM / Domains: Power on domain early during system resume
Hi Krzysztof,
On 10/23/2014 04:48 PM, Krzysztof Kozlowski wrote:
When resuming the system the power domain has to be powered on early so
any runtime PM aware devices could resume.
This fixes following scenario reproduced on Exynos DRM:
1. Power domain is off before suspending the system.
2. System is suspended to RAM.
3. Resuming starts. The Exynos DRM driver resume callback is called.
4. The Exynos DRM driver calls drm_helper_resume_force_mode which turns
the screen on by calling exynos_dsi_dpms with DRM_MODE_DPMS_ON.
5. The Exynos DSI driver calls pm_runtime_get. The driver runtime
resumes and this should turn LCD power domain on.
6. Unfortunately the domain cannot be turned on because system resume is
in progress and genpd-prepared_count is positive.
Just interesting, what value will be returned by pm_runtime_enabled()
from any of your .resume() callback (for any device which belongs to
some Generic PM domain)?
I'm asking, because as I can see Runtime PM can be disabled from
pm_genpd_prepare().
Thank you.
Oh. I've just found that you might get this issue if you will try to do
suspend when PM domain is ON ;)
Any way, In my opinion, It might be better to fix pm_genpd_prepare() so
it will not increment prepared_count when initial state of the GPD is
GPD_STATE_POWER_OFF. Seems it's needed only in opposite case -
when state of GPD has to be restored from pm_genpd_resume_noirq().
Steps to reproduce:
1. Add runtime PM to Exynos DSI driver.
2. Build Exynos DRM/FB without FRAMEBUFFER_CONSOLE.
3. Enable the connector and screen (e.g. with modeset-vsync application).
4. echo 3 /sys/devices/platform/exynos-drm/graphics/fb0/blank
5. echo mem /sys/power/state
6. Resume.
[ 77.712469] PM: early resume of devices complete after 3.854 msecs
[ 77.712739] exynos-dsi 11c8.dsi: pm_genpd_resume()
[ 77.712758] exynos4-fimc 1180.fimc: pm_genpd_resume()
[ 77.712774] exynos4-fimc 1181.fimc: pm_genpd_resume()
[ 77.712787] exynos-drm-fimc 1182.fimc: pm_genpd_resume()
[ 77.712802] exynos-drm-fimc 1183.fimc: pm_genpd_resume()
[ 77.712815] s5p-mipi-csis 1188.csis: pm_genpd_resume()
[ 77.712829] s5p-mipi-csis 1189.csis: pm_genpd_resume()
[ 77.712843] exynos-fimc-lite 1239.fimc-lite: pm_genpd_resume()
[ 77.712856] exynos-fimc-lite 123a.fimc-lite: pm_genpd_resume()
[ 77.713788] exynos4-fb 11c0.fimd: pm_genpd_resume()
[ 77.713912] wake disabled for irq 184
[ 77.713923] wake disabled for irq 185
[ 77.714082] wake disabled for irq 173
[ 77.715676] wake disabled for irq 176
[ 77.718540] exynos4-fb 11c0.fimd: pm_genpd_runtime_resume()
[ 77.718567] exynos4-fb 11c0.fimd: state restore latency exceeded, new
value 1708 ns
[ 77.718636] exynos-dsi 11c8.dsi: pm_genpd_runtime_resume()
[ 77.892366] exynos-dsi 11c8.dsi: PLL failed to stabilize
[ 77.892377] exynos-dsi 11c8.dsi: failed to configure DSI PLL
[ 78.192168] exynos-dsi 11c8.dsi: timeout waiting for reset
[ 78.211578] exynos-dsi 11c8.dsi: waiting for bus lanes timed out
[ 78.307173] exynos-dsi 11c8.dsi: xfer timed out: d1 00 (null)
[ 78.307190] panel_s6e8aa0 11c8.dsi.0: error -110 reading dcs seq(0xd1)
[ 78.307199] panel_s6e8aa0 11c8.dsi.0: read id failed
Signed-off-by: Krzysztof Kozlowski k.kozlow...@samsung.com
---
drivers/base/power/domain.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 40bc2f4072cc..4fdfe404a04c 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -179,8 +179,7 @@ static int __pm_genpd_poweron(struct generic_pm_domain
*genpd)
}
finish_wait(genpd-status_wait_queue, wait);
- if (genpd-status == GPD_STATE_ACTIVE
- || (genpd-prepared_count 0 genpd-suspend_power_off))
+ if (genpd-status == GPD_STATE_ACTIVE)
return 0;
if (genpd-status != GPD_STATE_POWER_OFF) {
regards,
-grygorii
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
