Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
On Monday, May 14, 2012, Marek Szyprowski wrote:
> Hello,
>
> On 5/14/2012 9:04 PM, Rafael J. Wysocki wrote:
> > On Monday, May 14, 2012, Marek Szyprowski wrote:
[...]
> >> I'm fine with this solution. Thanks for your help!
> >
> > OK, no problem.
> >
> > Do you want me to apply the patch literally in the above form or should I
> > skip
> > the arch/arm/mach-exynos/pm_domains.c change for now?
>
> Yes, please skip arch/arm/mach-exynos/pm_domains.c part. We will handle
> it with a separate patch on Samsung tree.
OK, so below it goes again without that hunk and with a changelog.
Thanks,
Rafael
---
From: Rafael J. Wysocki
Subject: PM / Domains: Make it possible to add devices to inactive domains
The generic PM domains core code currently requires domains to be in
the "power on" state for adding devices to them, but this limitation
turns out to be inconvenient in some situations, so remove it.
For this purpose, make __pm_genpd_add_device() set the device's
need_restore flag if the domain is in the "power off" state, so that
the device's "restore state" (usually .runtime_resume()) callback
is executed when it is resumed after the domain has been turned on.
If the domain is in the "power on" state, the device's need_restore
flag will be cleared by __pm_genpd_add_device(), so that its "save
state" (usually .runtime_suspend()) callback is executed when the
domain is about to be turned off. However, since that default
behavior need not be always desirable, add a helper function
pm_genpd_dev_need_restore() allowing a device's need_restore flag
to be set/unset at any time.
Signed-off-by: Rafael J. Wysocki
---
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
@@ -1269,11 +1269,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;
@@ -1296,7 +1291,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;
@@ -1424,6 +1419,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
@@ -146,6 +146,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,
@@ -189,6 +190,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)
{
--
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
Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
Hello,
On 5/14/2012 9:04 PM, Rafael J. Wysocki wrote:
On Monday, May 14, 2012, Marek Szyprowski wrote:
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 ch
Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
On Monday, May 14, 2012, Marek Szyprowski wrote:
> 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).
> >>>
>
Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
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
Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
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
Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
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 kic
RE: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
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 t
Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
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.
Thanks,
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
RE: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
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 callback is not
called
on first driver probe for devices inside the domain which has been left enabled
by
the bootloader.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
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
Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
On 5/2/12, Rafael J. Wysocki wrote:
> On Sunday, April 29, 2012, 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.
>
> Ping?
Hi Rafael,
Now Marek is on vacation util 6 May. He'll answer at that time.
Sorry.
Thank you,
Kyungmin Park
>
> --
> 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
>
--
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
Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
On Sunday, April 29, 2012, 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.
Ping?
--
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
Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
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;
>
Thanks,
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
Re: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
On Friday, April 27, 2012, Marek Szyprowski wrote:
> Hi Rafeal,
>
> On Friday, April 06, 2012 10:03 AM 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
>
> Could you comment on this patch? It is really annoying issue and I would like
> to have it
> finally solved.
It looks reasonable to me, I'm going to push it for v3.5.
Thanks,
Rafael
> > ---
> > 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;
> > list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
> > if (td)
> > gpd_data->td = *td;
> > --
> > 1.7.1.569.g6f426
>
> Best regards
>
--
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
RE: [PATCH] drivers: genpd: let platform code to register devices into disabled domains
Hi Rafeal,
On Friday, April 06, 2012 10:03 AM 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
Could you comment on this patch? It is really annoying issue and I would like
to have it
finally solved.
> ---
> 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;
> list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
> if (td)
> gpd_data->td = *td;
> --
> 1.7.1.569.g6f426
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
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
