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?
Rafael
--
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
