From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Szyprowski Subject: Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains Date: Mon, 14 May 2012 11:59:26 +0200 Message-ID: <4FB0D77E.2030805@samsung.com> References: <1333699379-16700-1-git-send-email-m.szyprowski@samsung.com> <023801cd2eaa$e5ddf8a0$b199e9e0$%szyprowski@samsung.com> <201205102152.56294.rjw@sisk.pl> <201205112251.31064.rjw@sisk.pl> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7BIT Return-path: Received: from mailout3.w1.samsung.com ([210.118.77.13]:63816 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755493Ab2ENJ7y (ORCPT ); Mon, 14 May 2012 05:59:54 -0400 In-reply-to: <201205112251.31064.rjw@sisk.pl> Sender: linux-samsung-soc-owner@vger.kernel.org List-Id: linux-samsung-soc@vger.kernel.org To: "Rafael J. Wysocki" Cc: linux-samsung-soc@vger.kernel.org, linux-pm@vger.kernel.org, 'Kyungmin Park' 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 >>>>>>> Signed-off-by: Kyungmin Park >>>>>>> --- >>>>>>> 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