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);
}
}
--
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
