Hi
On 2012-05-11 22:51, Rafael J. Wysocki wrote:
On Thursday, May 10, 2012, Rafael J. Wysocki wrote:
On Thursday, May 10, 2012, Marek Szyprowski wrote:
Hi Rafael,
On Monday, May 07, 2012 8:45 PM Rafael J. Wysocki wrote:
On Monday, May 07, 2012, Marek Szyprowski wrote:
Hi Rafael,
I'm sorry for a late reply, I was on holidays last week and just got back to
the office.
On Sunday, April 29, 2012 10:55 PM Rafael J. Wysocki wrote:
On Friday, April 06, 2012, Marek Szyprowski wrote:
Some bootloaders disable power domains on boot and the platform startup
code registers them in the 'disabled' state. Current gen_pd code assumed
that the devices can be registered only to the power domain which is in
'enabled' state and these devices are active at the time of the
registration. This is not correct in our case. This patch lets drivers
to be registered into 'disabled' power domains and finally solves
mysterious freezes and lack of resume/suspend calls on Samsung Exynos4
NURI and UniversalC210 platforms.
Signed-off-by: Marek Szyprowski<m.szyprowski@xxxxxxxxxxx>
Signed-off-by: Kyungmin Park<kyungmin.park@xxxxxxxxxxx>
---
drivers/base/power/domain.c | 7 +------
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 73ce9fb..05f5799f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1211,11 +1211,6 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct
device *dev,
genpd_acquire_lock(genpd);
- if (genpd->status == GPD_STATE_POWER_OFF) {
- ret = -EINVAL;
- goto out;
- }
-
if (genpd->prepared_count> 0) {
ret = -EAGAIN;
goto out;
@@ -1239,7 +1234,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct
device *dev,
dev_pm_get_subsys_data(dev);
dev->power.subsys_data->domain_data =&gpd_data->base;
gpd_data->base.dev = dev;
- gpd_data->need_restore = false;
+ gpd_data->need_restore = true;
I think that should be:
+ gpd_data->need_restore = genpd->status == GPD_STATE_POWER_OFF;
Otherwise, on the next domain power off the device's state won't be saved.
list_add_tail(&gpd_data->base.list_node,&genpd->dev_list);
if (td)
gpd_data->td = *td;
I've tested the above change and there is problem. Let me explain in detail the
sw/hw configuration I have.
There is a power domain and the device driver. The device itself also has it's own
power management code (which enables and disables clocks). Some power domains are
disabled by bootloader and some devices in the active power domains have their
clocks disabled too. In the current runtime pm code the devices were probed in
'disabled' state and had to enable itself by calling get_runtime_sync(). My initial
patch restored runtime pm handling to the old state (the same which was with non
gen_pd based driver or no power domain driver at all, where runtime pm was handled
by platform bus). If I apply your patch the runtime_restore
I guess you mean .runtime_resume().
callback is not called on first driver probe for devices inside the domain which
has been left enabled by the bootloader.
I don't see why .probe() should depend on the runtime PM framework to call
.runtime_resume() for it. It looks like .probe() could just call
.runtime_resume() directly if needed.
Besides, your change breaks existing code as I said.
Before using gen_pd power domains we had the following flow of calls/controls:
1. fimc_probe(fimd_pdev)
...
2. pm_runtime_enable(fimd_pdev->dev)
3. pm_runtime_get_sync(fimd_pdev->dev)
3a. parent device's runtime_resume()
3b. fimc_runtime_resume(fimd_pdev->dev)
...
4. pm_runtime_put(fimd_pdev->dev)
...
5. (runtime put timer kicks off)
5a. fimc_runtime_put(fimd_pdev->dev)
5b. parent device's runtime_suspend()
(this flow assumed that fimc device was the only child of its parent platform device).
Now with power gen_pd driver with my patch I get the following call sequence:
1. fimc_probe(fimd_pdev)
...
2. pm_runtime_enable(fimd_pdev->dev)
3. pm_runtime_get_sync(fimd_pdev->dev)
3a. gen_pd pd_power_on(...)
3b. fimc_runtime_resume(fimd_pdev->dev)
4. pm_runtime_put(fimd_pdev->dev)
...
5. (runtime put timer kicks off)
5a. fimc_runtime_put(fimd_pdev->dev)
5b. gen_pd pd_power_off (...)
so it works like before.
Now with your suggested change I get following call sequence:
1. fimc_probe(fimc_pdev)
...
2. pm_runtime_enable(fimd_pdev->dev)
3. pm_runtime_get_sync(fimd_pdev->dev)
(gen_pd finds that the power domain is already activated)
...
4. pm_runtime_put(fimd_pdev->dev)
...
5. (runtime put timer kicks off)
5a. fimc_runtime_put(fimd_pdev->dev)
5b. gen_pd pd_power_off (...)
As you can notice in point 3, gen_pd driver checks its internal state,
finds that the power domain is already enabled and skips calling
fimc_runtime_resume(). This breaks the driver which worked fine before.
It doesn't break anything, you're just using a wrong tool for a wrong
purpose. Generic PM domains are not supposed to be a drop-in replacement
for platform bus type!
Please notice that fimc_runtime_resume() does something completely
different than the power domain driver and those operations are essential
for getting the driver to work correctly.
I don't quite understand what you mean here. What's the power domain driver
in particular?
Now, you can kind of make things work with my proposed modification of the
patch if you make your platform code that adds the fmc device to the PM
domain set its need_restore flag directly afterwards.
So, you do
pm_genpd_add_device(domain, fmc);
fmc->power.subsys_data->domain_data->need_restore = true;
Or you can actually modify __pm_genpd_add_device() so that it takes
need_restore as an additional argument. That would be fine by me too so long
as pm_genpd_add_device() worked in the same way as before.
However, there is code already in the kernel that will break if you change
__pm_genpd_add_device() to set need_restore unconditionally. Is that clear
enough?
I think we can use the
+ gpd_data->need_restore = genpd->status == GPD_STATE_POWER_OFF;
variant of the $subject patch and add e helper for setting the need_restore
flag to it and use that helper along with pm_genpd_add_device() wherever
necessary.
So, the entire patch might look like the one below.
What do you think?
Rafael
---
arch/arm/mach-exynos/pm_domains.c | 2 ++
drivers/base/power/domain.c | 27 +++++++++++++++++++++------
include/linux/pm_domain.h | 2 ++
3 files changed, 25 insertions(+), 6 deletions(-)
Index: linux/drivers/base/power/domain.c
===================================================================
--- linux.orig/drivers/base/power/domain.c
+++ linux/drivers/base/power/domain.c
@@ -1302,11 +1302,6 @@ int __pm_genpd_add_device(struct generic
genpd_acquire_lock(genpd);
- if (genpd->status == GPD_STATE_POWER_OFF) {
- ret = -EINVAL;
- goto out;
- }
-
if (genpd->prepared_count> 0) {
ret = -EAGAIN;
goto out;
@@ -1329,7 +1324,7 @@ int __pm_genpd_add_device(struct generic
dev->power.subsys_data->domain_data =&gpd_data->base;
gpd_data->base.dev = dev;
list_add_tail(&gpd_data->base.list_node,&genpd->dev_list);
- gpd_data->need_restore = false;
+ gpd_data->need_restore = genpd->status == GPD_STATE_POWER_OFF;
if (td)
gpd_data->td = *td;
@@ -1457,6 +1452,26 @@ void pm_genpd_dev_always_on(struct devic
EXPORT_SYMBOL_GPL(pm_genpd_dev_always_on);
/**
+ * pm_genpd_dev_need_restore - Set/unset the device's "need restore" flag.
+ * @dev: Device to set/unset the flag for.
+ * @val: The new value of the device's "need restore" flag.
+ */
+void pm_genpd_dev_need_restore(struct device *dev, bool val)
+{
+ struct pm_subsys_data *psd;
+ unsigned long flags;
+
+ spin_lock_irqsave(&dev->power.lock, flags);
+
+ psd = dev_to_psd(dev);
+ if (psd&& psd->domain_data)
+ to_gpd_data(psd->domain_data)->need_restore = val;
+
+ spin_unlock_irqrestore(&dev->power.lock, flags);
+}
+EXPORT_SYMBOL_GPL(pm_genpd_dev_need_restore);
+
+/**
* pm_genpd_add_subdomain - Add a subdomain to an I/O PM domain.
* @genpd: Master PM domain to add the subdomain to.
* @subdomain: Subdomain to be added.
Index: linux/include/linux/pm_domain.h
===================================================================
--- linux.orig/include/linux/pm_domain.h
+++ linux/include/linux/pm_domain.h
@@ -156,6 +156,7 @@ static inline int pm_genpd_of_add_device
extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
struct device *dev);
extern void pm_genpd_dev_always_on(struct device *dev, bool val);
+extern void pm_genpd_dev_need_restore(struct device *dev, bool val);
extern int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
struct generic_pm_domain *new_subdomain);
extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
@@ -201,6 +202,7 @@ static inline int pm_genpd_remove_device
return -ENOSYS;
}
static inline void pm_genpd_dev_always_on(struct device *dev, bool val) {}
+static inline void pm_genpd_dev_need_restore(struct device *dev, bool val) {}
static inline int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
struct generic_pm_domain *new_sd)
{
Index: linux/arch/arm/mach-exynos/pm_domains.c
===================================================================
--- linux.orig/arch/arm/mach-exynos/pm_domains.c
+++ linux/arch/arm/mach-exynos/pm_domains.c
@@ -123,6 +123,8 @@ static __init void exynos_pm_add_dev_to_
pr_info("%s: error in adding %s device to %s power"
"domain\n", __func__, dev_name(&pdev->dev),
pd->name);
+ else
+ pm_genpd_dev_need_restore(&pdev->dev, true);
}
}
I'm fine with this solution. Thanks for your help!
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
