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.szyprow...@samsung.com>
> > > > > > Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com>
> > > > > > ---
> > > > > >  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to