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.
Thanks,
Rafael
--
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
