* Re: [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype
2011-04-11 19:49 [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype Magnus Damm
@ 2011-04-11 22:45 ` Rafael J. Wysocki
2011-04-15 19:35 ` Guennadi Liakhovetski
` (9 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2011-04-11 22:45 UTC (permalink / raw)
To: linux-sh
Hi,
To start with, I think we'll need to do that a bit differently, because
apparently some clock management code may be shared with OMAP.
Most imprtantly, the PM core is going to be changed so that power domain
callbacks will be executed instead of and not in addition to the bus type
callbacks, so platform_pm_runtime_suspend() and platform_pm_runtime_resume()
will have to be replaced by the power domain callbacks entirely.
I'm working on those changes right now, but I'd like to make the core
changes first and get everybody involved to agree with them.
Some more comments below.
On Monday, April 11, 2011, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
>
> This experimental sh7372-specific Runtime PM prototype
> adds support for the A4LC and A4MP power domains.
>
> Tested on the Mackerel board together with the LCDC:
>
> # blank HDMI fbdev
> echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.1/graphics/fb1/blank
>
> # blank LCD panel fbdev
> echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> # unblank LCD panel fbdev
> echo 0 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
>
> Not-yet-signed-off-by: Magnus Damm <damm@opensource.se>
> ---
>
> arch/arm/mach-shmobile/pm_runtime.c | 227 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 222 insertions(+), 5 deletions(-)
>
> --- 0001/arch/arm/mach-shmobile/pm_runtime.c
> +++ work/arch/arm/mach-shmobile/pm_runtime.c 2011-04-12 04:37:17.000000000 +0900
> @@ -3,7 +3,7 @@
> *
> * Runtime PM support code for SuperH Mobile ARM
> *
> - * Copyright (C) 2009-2010 Magnus Damm
> + * Copyright (C) 2009-2011 Magnus Damm
> *
> * This file is subject to the terms and conditions of the GNU General Public
> * License. See the file "COPYING" in the main directory of this archive
> @@ -20,13 +20,71 @@
> #include <linux/bitmap.h>
>
> #ifdef CONFIG_PM_RUNTIME
> +
> +struct pm_runtime_power_domain {
> + spinlock_t lock;
I'm not sure if that has to be a spinlock.
> + struct list_head idle_list;
> + unsigned int use_cnt;
> + unsigned int powered_off;
> + unsigned int power_domain_bit;
> +};
My idea is that at least in some cases the power domain structure will look
the same, or very similarly, on other platforms, so perhaps it makes sense
to define it in the core rather than platform-specific.
> +
> +/* sh7372 specific stuff in common file, FIXME this should be broken out */
> +
> +static struct pm_runtime_power_domain sh7372_a4lc = {
> + .power_domain_bit = 1,
> +};
> +
> +static struct pm_runtime_power_domain sh7372_a4mp = {
> + .power_domain_bit = 2,
> +};
> +
> +#define SPDCR 0xe6180008
> +#define SWUCR 0xe6180014
> +#define PSTR 0xe6180080
> +
> +static void power_down(struct pm_runtime_power_domain *domain_data)
> +{
> + unsigned int mask = 1 << domain_data->power_domain_bit;
> +
> + if (__raw_readl(PSTR) & mask) {
> + __raw_writel(mask, SPDCR);
> +
> + while (__raw_readl(SPDCR) & mask)
> + ;
> +
> + pr_info("sh7372 power domain down 0x%08x -> PSTR = 0x%08x\n",
> + mask, __raw_readl(PSTR));
> + }
> +}
> +
> +static void power_up(struct pm_runtime_power_domain *domain_data)
> +{
> + unsigned int mask = 1 << domain_data->power_domain_bit;
> +
> + if (!(__raw_readl(PSTR) & mask)) {
> + __raw_writel(mask, SWUCR);
> +
> + while (__raw_readl(SWUCR) & mask)
> + ;
> +
> + pr_info("sh7372 power domain up 0x%08x -> PSTR = 0x%08x\n",
> + mask, __raw_readl(PSTR));
> + }
> +}
> +
> #define BIT_ONCE 0
> #define BIT_ACTIVE 1
> #define BIT_CLK_ENABLED 2
> +#define BIT_SUSPENDED 3
>
> struct pm_runtime_data {
> unsigned long flags;
> struct clk *clk;
> +
> + struct device *dev;
> + struct list_head entry; /* for pm_runtime_power_domain idle_list */
> + struct pm_runtime_power_domain *domain_data;
> };
>
> static void __devres_release(struct device *dev, void *res)
> @@ -47,6 +105,159 @@ static struct pm_runtime_data *__to_prd(
> return devres_find(dev, __devres_release, NULL, NULL);
> }
>
> +static int __power_domain_runtime_suspend(struct device *d)
> +{
> + struct pm_runtime_data *prd = __to_prd(d);
> + int ret = 0;
> +
> + dev_dbg(d, "__power_domain_runtime_suspend()\n");
> +
> + BUG_ON(!prd);
> +
> + if (!test_bit(BIT_SUSPENDED, &prd->flags)) {
Those things need not be handled with the help of test_bit() and friends I think.
The PM core guarantees that runtime PM callbacks will never be executed
concurrently with one another for the same device, so all things that are
only accessed by those callbacks may be simple bit fields.
> + if (d->driver && d->driver->pm &&
> + d->driver->pm->runtime_suspend) {
> + if (!test_bit(BIT_CLK_ENABLED, &prd->flags))
> + clk_enable(prd->clk);
> + ret = d->driver->pm->runtime_suspend(d);
> + clk_disable(prd->clk);
> + clear_bit(BIT_CLK_ENABLED, &prd->flags);
> +
> + if (ret = 0)
> + set_bit(BIT_SUSPENDED, &prd->flags);
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int power_domain_runtime_suspend(struct device *dev)
> +{
> + struct pm_runtime_data *prd = __to_prd(dev);
> + struct pm_runtime_power_domain *domain_data = prd->domain_data;
> + unsigned long flags;
> + int ret = 0;
> +
> + dev_dbg(dev, "power_domain_runtime_suspend() %d\n",
> + domain_data->use_cnt);
> +
> + spin_lock_irqsave(&domain_data->lock, flags);
> +
> + list_add_tail(&prd->entry, &domain_data->idle_list);
> + domain_data->use_cnt--;
> +
> + if (domain_data->use_cnt = 0) {
> + struct pm_runtime_data *p;
> +
> + list_for_each_entry(p, &domain_data->idle_list, entry) {
> + ret = __power_domain_runtime_suspend(p->dev);
> + if (ret)
> + break;
> + }
> +
> + if (ret = 0)
> + power_down(domain_data);
> + }
> +
> + spin_unlock_irqrestore(&domain_data->lock, flags);
> +
> + return ret;
> +}
> +
> +static int __power_domain_runtime_resume(struct device *d)
> +{
> + struct pm_runtime_data *prd = __to_prd(d);
> + int ret = 0;
> +
> + dev_dbg(d, "__power_domain_runtime_resume()\n");
> +
> + BUG_ON(!prd);
> +
> + if (test_bit(BIT_SUSPENDED, &prd->flags)) {
> + if (!test_bit(BIT_CLK_ENABLED, &prd->flags)) {
> + clk_enable(prd->clk);
> + set_bit(BIT_CLK_ENABLED, &prd->flags);
> + }
> +
> + if (d->driver && d->driver->pm &&
> + d->driver->pm->runtime_resume)
> + ret = d->driver->pm->runtime_resume(d);
> +
> + if (ret = 0)
> + clear_bit(BIT_SUSPENDED, &prd->flags);
> + else {
> + clk_disable(prd->clk);
> + clear_bit(BIT_CLK_ENABLED, &prd->flags);
> + }
> + }
> +
> + return ret;
> +}
> +
> +static int power_domain_runtime_resume(struct device *dev)
> +{
> + struct pm_runtime_data *prd = __to_prd(dev);
> + struct pm_runtime_power_domain *domain_data = prd->domain_data;
> + unsigned long flags;
> + int ret = 0;
> +
> + dev_dbg(dev, "power_domain_runtime_resume() %d\n",
> + domain_data->use_cnt);
> +
> + spin_lock_irqsave(&domain_data->lock, flags);
> +
> + if (domain_data->use_cnt = 0)
> + power_up(domain_data);
> +
> + ret = __power_domain_runtime_resume(dev);
> +
> + if (ret = 0) {
> + domain_data->use_cnt++;
> + list_del(&prd->entry);
> + } else {
> + if (domain_data->use_cnt = 0)
> + power_down(domain_data);
> + }
> +
> + spin_unlock_irqrestore(&domain_data->lock, flags);
> +
> + return ret;
> +}
The majority of the code above doesn't look like shmobile-specific or even
ARM-specific, so I think it will make sense to put it into drivers/base/power.
Of course, power_up() and power_down() are mackerel-specific, but they may
be provided as function pointers in the power domain structure.
> +
> +static struct dev_power_domain power_domain = {
> + .ops.runtime_suspend = power_domain_runtime_suspend,
> + .ops.runtime_resume = power_domain_runtime_resume,
> +};
> +
> +static void power_domain_init(struct device *dev, struct pm_runtime_data *prd)
> +{
> + struct pm_runtime_power_domain *domain_data = NULL;
> +
> + if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.0") = 0)
> + domain_data = &sh7372_a4lc;
> +
> + if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.1") = 0)
> + domain_data = &sh7372_a4lc;
> +
> + if (strcmp(dev_name(dev), "sh_fsi2") = 0)
> + domain_data = &sh7372_a4mp;
The above things don't look quite right. I think the connections between
device objects and power domain objects should be set up in the board file.
> +
> + if (domain_data) {
> + dev->pwr_domain = &power_domain;
> + prd->dev = dev;
> + INIT_LIST_HEAD(&prd->entry);
> + prd->domain_data = domain_data;
> + domain_data->use_cnt++;
> + if (domain_data->use_cnt = 1) {
This doesn't look nice to me.
> + INIT_LIST_HEAD(&domain_data->idle_list);
> + spin_lock_init(&domain_data->lock);
> + }
> +
> + dev_dbg(dev, "domain data available use cnt = %d\n",
> + domain_data->use_cnt);
> + }
> +}
> +
> static void platform_pm_runtime_init(struct device *dev,
> struct pm_runtime_data *prd)
> {
> @@ -55,6 +266,7 @@ static void platform_pm_runtime_init(str
> if (!IS_ERR(prd->clk)) {
> set_bit(BIT_ACTIVE, &prd->flags);
> dev_info(dev, "clocks managed by runtime pm\n");
> + power_domain_init(dev, prd);
> }
> }
> }
> @@ -75,8 +287,11 @@ int platform_pm_runtime_suspend(struct d
> platform_pm_runtime_bug(dev, prd);
>
> if (prd && test_bit(BIT_ACTIVE, &prd->flags)) {
> - clk_disable(prd->clk);
> - clear_bit(BIT_CLK_ENABLED, &prd->flags);
> + if (test_bit(BIT_CLK_ENABLED, &prd->flags)) {
> + clk_disable(prd->clk);
> + clear_bit(BIT_CLK_ENABLED, &prd->flags);
> + }
> +
> }
>
> return 0;
> @@ -91,8 +306,10 @@ int platform_pm_runtime_resume(struct de
> platform_pm_runtime_init(dev, prd);
>
> if (prd && test_bit(BIT_ACTIVE, &prd->flags)) {
> - clk_enable(prd->clk);
> - set_bit(BIT_CLK_ENABLED, &prd->flags);
> + if (!test_bit(BIT_CLK_ENABLED, &prd->flags)) {
> + clk_enable(prd->clk);
> + set_bit(BIT_CLK_ENABLED, &prd->flags);
> + }
> }
>
> return 0;
Thanks,
Rafael
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype
2011-04-11 19:49 [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype Magnus Damm
2011-04-11 22:45 ` Rafael J. Wysocki
@ 2011-04-15 19:35 ` Guennadi Liakhovetski
2011-04-18 9:41 ` Magnus Damm
` (8 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Guennadi Liakhovetski @ 2011-04-15 19:35 UTC (permalink / raw)
To: linux-sh
Hi Magnus
On Tue, 12 Apr 2011, Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
>
> This experimental sh7372-specific Runtime PM prototype
> adds support for the A4LC and A4MP power domains.
>
> Tested on the Mackerel board together with the LCDC:
>
> # blank HDMI fbdev
> echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.1/graphics/fb1/blank
>
> # blank LCD panel fbdev
> echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> # unblank LCD panel fbdev
> echo 0 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
>
> Not-yet-signed-off-by: Magnus Damm <damm@opensource.se>
> ---
>
> arch/arm/mach-shmobile/pm_runtime.c | 227 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 222 insertions(+), 5 deletions(-)
Your patch worked mostly, below is a small addition (to be applied on top
of your patch), that adds several new domains and devices and fixes a
couple of issues with your original patch. Notice an "#if 0" around i2c
devices: activating them causes problems - output on the LCDC disappears
and the boot process doesn't complete, but without a serial console I
couldn't tell, what exactly was the problem. Should be investigated
further.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
diff --git a/arch/arm/mach-shmobile/pm_runtime.c b/arch/arm/mach-shmobile/pm_runtime.c
index 48d6e4d..b9aeccf 100644
--- a/arch/arm/mach-shmobile/pm_runtime.c
+++ b/arch/arm/mach-shmobile/pm_runtime.c
@@ -31,6 +31,12 @@ struct pm_runtime_power_domain {
/* sh7372 specific stuff in common file, FIXME this should be broken out */
+/*
+ * Further domains:
+ * A4R: CEU, VEU, BEU, VOU, VPU, CSI2, MSIOF0, IIC0/2
+ * A3SP: MMCIF, MSIOF1/2, SDHI0/1/2, SCIFA, SCIFB, IIC1/3/4, DMAC1/2/3
+ */
+
static struct pm_runtime_power_domain sh7372_a4lc = {
.power_domain_bit = 1,
};
@@ -39,6 +45,14 @@ static struct pm_runtime_power_domain sh7372_a4mp = {
.power_domain_bit = 2,
};
+static struct pm_runtime_power_domain sh7372_a4r = {
+ .power_domain_bit = 5,
+};
+
+static struct pm_runtime_power_domain sh7372_a3sp = {
+ .power_domain_bit = 11,
+};
+
#define SPDCR 0xe6180008
#define SWUCR 0xe6180014
#define PSTR 0xe6180080
@@ -235,23 +249,61 @@ static void power_domain_init(struct device *dev, struct pm_runtime_data *prd)
if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.0") = 0)
domain_data = &sh7372_a4lc;
-
- if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.1") = 0)
+ else if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.1") = 0)
domain_data = &sh7372_a4lc;
-
- if (strcmp(dev_name(dev), "sh_fsi2") = 0)
+ else if (strcmp(dev_name(dev), "sh-mipi-dsi.0") = 0)
+ domain_data = &sh7372_a4lc;
+ else if (strcmp(dev_name(dev), "sh_fsi2") = 0)
domain_data = &sh7372_a4mp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.3") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.4") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.5") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.6") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_ceu.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.3") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.4") = 0)
+ domain_data = &sh7372_a3sp;
+#if 0
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.0") = 0)
+ domain_data = &sh7372_a4r;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.2") = 0)
+ domain_data = &sh7372_a4r;
+#endif
+ else if (strcmp(dev_name(dev), "sh_mmcif.0") = 0)
+ domain_data = &sh7372_a3sp;
if (domain_data) {
dev->pwr_domain = &power_domain;
prd->dev = dev;
INIT_LIST_HEAD(&prd->entry);
prd->domain_data = domain_data;
- domain_data->use_cnt++;
- if (domain_data->use_cnt = 1) {
- INIT_LIST_HEAD(&domain_data->idle_list);
- spin_lock_init(&domain_data->lock);
- }
+ domain_data->use_cnt = 1;
dev_dbg(dev, "domain data available use cnt = %d\n",
domain_data->use_cnt);
@@ -291,7 +343,6 @@ int platform_pm_runtime_suspend(struct device *dev)
clk_disable(prd->clk);
clear_bit(BIT_CLK_ENABLED, &prd->flags);
}
-
}
return 0;
@@ -329,12 +380,17 @@ static int platform_bus_notify(struct notifier_block *nb,
dev_dbg(dev, "platform_bus_notify() %ld !\n", action);
- if (action = BUS_NOTIFY_BIND_DRIVER) {
+ switch (action) {
+ case BUS_NOTIFY_BIND_DRIVER:
prd = devres_alloc(__devres_release, sizeof(*prd), GFP_KERNEL);
if (prd)
devres_add(dev, prd);
else
dev_err(dev, "unable to alloc memory for runtime pm\n");
+ break;
+ case BUS_NOTIFY_UNBIND_DRIVER:
+ dev->pwr_domain = NULL;
+ break;
}
return 0;
@@ -380,6 +436,15 @@ static struct notifier_block platform_bus_notifier = {
static int __init sh_pm_runtime_init(void)
{
+ INIT_LIST_HEAD(&sh7372_a4lc.idle_list);
+ spin_lock_init(&sh7372_a4lc.lock);
+ INIT_LIST_HEAD(&sh7372_a4mp.idle_list);
+ spin_lock_init(&sh7372_a4mp.lock);
+ INIT_LIST_HEAD(&sh7372_a4r.idle_list);
+ spin_lock_init(&sh7372_a4r.lock);
+ INIT_LIST_HEAD(&sh7372_a3sp.idle_list);
+ spin_lock_init(&sh7372_a3sp.lock);
+
bus_register_notifier(&platform_bus_type, &platform_bus_notifier);
return 0;
}
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype
2011-04-11 19:49 [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype Magnus Damm
2011-04-11 22:45 ` Rafael J. Wysocki
2011-04-15 19:35 ` Guennadi Liakhovetski
@ 2011-04-18 9:41 ` Magnus Damm
2011-04-18 20:08 ` Rafael J. Wysocki
` (7 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Magnus Damm @ 2011-04-18 9:41 UTC (permalink / raw)
To: linux-sh
Hi Rafael,
2011/4/12 Rafael J. Wysocki <rjw@sisk.pl>:
> Hi,
>
> To start with, I think we'll need to do that a bit differently, because
> apparently some clock management code may be shared with OMAP.
>
> Most imprtantly, the PM core is going to be changed so that power domain
> callbacks will be executed instead of and not in addition to the bus type
> callbacks, so platform_pm_runtime_suspend() and platform_pm_runtime_resume()
> will have to be replaced by the power domain callbacks entirely.
>
> I'm working on those changes right now, but I'd like to make the core
> changes first and get everybody involved to agree with them.
Thanks for your help!
> Some more comments below.
[snip]
> The above things don't look quite right. I think the connections between
> device objects and power domain objects should be set up in the board file.
Yes, no doubt about that!!!
> This doesn't look nice to me.
I have to agree with you on that one. =)
This prototype code is rather filthy and can easily be seen as one big
layering violation.
However, just so you understand the reason behind this:
I simply posted the patch to publish _some_ code that exercises the
Runtime PM callbacks in combination with a bit of power domain code.
This so anyone with sh7372 can do some simple testing of drivers using
Runtime PM.
I'd like to see the drivers hardened in parallel with Runtime PM power
domain support. When we have something nicer we will of course switch
to using that. Feel free to post a better version that is properly
abstracted. =)
Thanks,
/ magnus
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype
2011-04-11 19:49 [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype Magnus Damm
` (2 preceding siblings ...)
2011-04-18 9:41 ` Magnus Damm
@ 2011-04-18 20:08 ` Rafael J. Wysocki
2011-04-19 10:55 ` Guennadi Liakhovetski
` (6 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2011-04-18 20:08 UTC (permalink / raw)
To: linux-sh
On Monday, April 18, 2011, Magnus Damm wrote:
> Hi Rafael,
>
> 2011/4/12 Rafael J. Wysocki <rjw@sisk.pl>:
> > Hi,
> >
> > To start with, I think we'll need to do that a bit differently, because
> > apparently some clock management code may be shared with OMAP.
> >
> > Most imprtantly, the PM core is going to be changed so that power domain
> > callbacks will be executed instead of and not in addition to the bus type
> > callbacks, so platform_pm_runtime_suspend() and platform_pm_runtime_resume()
> > will have to be replaced by the power domain callbacks entirely.
> >
> > I'm working on those changes right now, but I'd like to make the core
> > changes first and get everybody involved to agree with them.
>
> Thanks for your help!
>
> > Some more comments below.
>
> [snip]
>
> > The above things don't look quite right. I think the connections between
> > device objects and power domain objects should be set up in the board file.
>
> Yes, no doubt about that!!!
>
> > This doesn't look nice to me.
>
> I have to agree with you on that one. =)
>
> This prototype code is rather filthy and can easily be seen as one big
> layering violation.
>
> However, just so you understand the reason behind this:
>
> I simply posted the patch to publish _some_ code that exercises the
> Runtime PM callbacks in combination with a bit of power domain code.
> This so anyone with sh7372 can do some simple testing of drivers using
> Runtime PM.
>
> I'd like to see the drivers hardened in parallel with Runtime PM power
> domain support. When we have something nicer we will of course switch
> to using that. Feel free to post a better version that is properly
> abstracted. =)
Will do, on top of my patches clarifying the existing runtime PM code:
https://lkml.org/lkml/2011/4/16/129
Thanks,
Rafael
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype
2011-04-11 19:49 [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype Magnus Damm
` (3 preceding siblings ...)
2011-04-18 20:08 ` Rafael J. Wysocki
@ 2011-04-19 10:55 ` Guennadi Liakhovetski
2011-04-21 7:37 ` Guennadi Liakhovetski
` (5 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Guennadi Liakhovetski @ 2011-04-19 10:55 UTC (permalink / raw)
To: linux-sh
On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote:
> Hi Magnus
>
> On Tue, 12 Apr 2011, Magnus Damm wrote:
>
> > From: Magnus Damm <damm@opensource.se>
> >
> > This experimental sh7372-specific Runtime PM prototype
> > adds support for the A4LC and A4MP power domains.
> >
> > Tested on the Mackerel board together with the LCDC:
> >
> > # blank HDMI fbdev
> > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.1/graphics/fb1/blank
> >
> > # blank LCD panel fbdev
> > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > # unblank LCD panel fbdev
> > echo 0 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> >
> > Not-yet-signed-off-by: Magnus Damm <damm@opensource.se>
> > ---
> >
> > arch/arm/mach-shmobile/pm_runtime.c | 227 ++++++++++++++++++++++++++++++++++-
> > 1 file changed, 222 insertions(+), 5 deletions(-)
>
> Your patch worked mostly, below is a small addition (to be applied on top
> of your patch), that adds several new domains and devices and fixes a
> couple of issues with your original patch. Notice an "#if 0" around i2c
> devices: activating them causes problems - output on the LCDC disappears
> and the boot process doesn't complete, but without a serial console I
> couldn't tell, what exactly was the problem. Should be investigated
> further.
Below is a fixed version of the earlier proposed fix;)
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
diff --git a/arch/arm/mach-shmobile/pm_runtime.c b/arch/arm/mach-shmobile/pm_runtime.c
index 48d6e4d..ae8b7a1 100644
--- a/arch/arm/mach-shmobile/pm_runtime.c
+++ b/arch/arm/mach-shmobile/pm_runtime.c
@@ -31,6 +31,12 @@ struct pm_runtime_power_domain {
/* sh7372 specific stuff in common file, FIXME this should be broken out */
+/*
+ * Further domains:
+ * A4R: CEU, VEU, BEU, VOU, VPU, CSI2, MSIOF0, IIC0/2
+ * A3SP: MMCIF, MSIOF1/2, SDHI0/1/2, SCIFA, SCIFB, IIC1/3/4, DMAC1/2/3
+ */
+
static struct pm_runtime_power_domain sh7372_a4lc = {
.power_domain_bit = 1,
};
@@ -39,6 +45,14 @@ static struct pm_runtime_power_domain sh7372_a4mp = {
.power_domain_bit = 2,
};
+static struct pm_runtime_power_domain sh7372_a4r = {
+ .power_domain_bit = 5,
+};
+
+static struct pm_runtime_power_domain sh7372_a3sp = {
+ .power_domain_bit = 11,
+};
+
#define SPDCR 0xe6180008
#define SWUCR 0xe6180014
#define PSTR 0xe6180080
@@ -134,16 +148,27 @@ static int __power_domain_runtime_suspend(struct device *d)
static int power_domain_runtime_suspend(struct device *dev)
{
struct pm_runtime_data *prd = __to_prd(dev);
- struct pm_runtime_power_domain *domain_data = prd->domain_data;
+ struct pm_runtime_power_domain *domain_data;
unsigned long flags;
int ret = 0;
+ if (!prd)
+ return 0;
+
+ domain_data = prd->domain_data;
+
dev_dbg(dev, "power_domain_runtime_suspend() %d\n",
domain_data->use_cnt);
spin_lock_irqsave(&domain_data->lock, flags);
- list_add_tail(&prd->entry, &domain_data->idle_list);
+ /*
+ * suspend can be called repeatedly - see rpm_suspend() -
+ * rpm_check_suspend_allowed() returns 1, if already suspended, and the
+ * process continues.
+ */
+ if (list_empty(&prd->entry))
+ list_add_tail(&prd->entry, &domain_data->idle_list);
domain_data->use_cnt--;
if (domain_data->use_cnt = 0) {
@@ -213,7 +238,7 @@ static int power_domain_runtime_resume(struct device *dev)
if (ret = 0) {
domain_data->use_cnt++;
- list_del(&prd->entry);
+ list_del_init(&prd->entry);
} else {
if (domain_data->use_cnt = 0)
power_down(domain_data);
@@ -235,23 +260,61 @@ static void power_domain_init(struct device *dev, struct pm_runtime_data *prd)
if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.0") = 0)
domain_data = &sh7372_a4lc;
-
- if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.1") = 0)
+ else if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.1") = 0)
domain_data = &sh7372_a4lc;
-
- if (strcmp(dev_name(dev), "sh_fsi2") = 0)
+ else if (strcmp(dev_name(dev), "sh-mipi-dsi.0") = 0)
+ domain_data = &sh7372_a4lc;
+ else if (strcmp(dev_name(dev), "sh_fsi2") = 0)
domain_data = &sh7372_a4mp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.3") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.4") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.5") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.6") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_ceu.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.3") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.4") = 0)
+ domain_data = &sh7372_a3sp;
+#if 0
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.0") = 0)
+ domain_data = &sh7372_a4r;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.2") = 0)
+ domain_data = &sh7372_a4r;
+#endif
+ else if (strcmp(dev_name(dev), "sh_mmcif.0") = 0)
+ domain_data = &sh7372_a3sp;
if (domain_data) {
dev->pwr_domain = &power_domain;
prd->dev = dev;
INIT_LIST_HEAD(&prd->entry);
prd->domain_data = domain_data;
- domain_data->use_cnt++;
- if (domain_data->use_cnt = 1) {
- INIT_LIST_HEAD(&domain_data->idle_list);
- spin_lock_init(&domain_data->lock);
- }
+ domain_data->use_cnt = 1;
dev_dbg(dev, "domain data available use cnt = %d\n",
domain_data->use_cnt);
@@ -291,7 +354,6 @@ int platform_pm_runtime_suspend(struct device *dev)
clk_disable(prd->clk);
clear_bit(BIT_CLK_ENABLED, &prd->flags);
}
-
}
return 0;
@@ -329,12 +391,20 @@ static int platform_bus_notify(struct notifier_block *nb,
dev_dbg(dev, "platform_bus_notify() %ld !\n", action);
- if (action = BUS_NOTIFY_BIND_DRIVER) {
+ switch (action) {
+ case BUS_NOTIFY_BIND_DRIVER:
prd = devres_alloc(__devres_release, sizeof(*prd), GFP_KERNEL);
if (prd)
devres_add(dev, prd);
else
dev_err(dev, "unable to alloc memory for runtime pm\n");
+ break;
+ case BUS_NOTIFY_UNBIND_DRIVER:
+ prd = __to_prd(dev);
+ if (prd)
+ list_del(&prd->entry);
+ dev->pwr_domain = NULL;
+ break;
}
return 0;
@@ -380,6 +450,15 @@ static struct notifier_block platform_bus_notifier = {
static int __init sh_pm_runtime_init(void)
{
+ INIT_LIST_HEAD(&sh7372_a4lc.idle_list);
+ spin_lock_init(&sh7372_a4lc.lock);
+ INIT_LIST_HEAD(&sh7372_a4mp.idle_list);
+ spin_lock_init(&sh7372_a4mp.lock);
+ INIT_LIST_HEAD(&sh7372_a4r.idle_list);
+ spin_lock_init(&sh7372_a4r.lock);
+ INIT_LIST_HEAD(&sh7372_a3sp.idle_list);
+ spin_lock_init(&sh7372_a3sp.lock);
+
bus_register_notifier(&platform_bus_type, &platform_bus_notifier);
return 0;
}
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype
2011-04-11 19:49 [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype Magnus Damm
` (4 preceding siblings ...)
2011-04-19 10:55 ` Guennadi Liakhovetski
@ 2011-04-21 7:37 ` Guennadi Liakhovetski
2011-04-29 17:19 ` Guennadi Liakhovetski
` (4 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Guennadi Liakhovetski @ 2011-04-21 7:37 UTC (permalink / raw)
To: linux-sh
On Tue, 19 Apr 2011, Guennadi Liakhovetski wrote:
> On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote:
>
> > Hi Magnus
> >
> > On Tue, 12 Apr 2011, Magnus Damm wrote:
> >
> > > From: Magnus Damm <damm@opensource.se>
> > >
> > > This experimental sh7372-specific Runtime PM prototype
> > > adds support for the A4LC and A4MP power domains.
> > >
> > > Tested on the Mackerel board together with the LCDC:
> > >
> > > # blank HDMI fbdev
> > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.1/graphics/fb1/blank
> > >
> > > # blank LCD panel fbdev
> > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > > # unblank LCD panel fbdev
> > > echo 0 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > >
> > > Not-yet-signed-off-by: Magnus Damm <damm@opensource.se>
> > > ---
> > >
> > > arch/arm/mach-shmobile/pm_runtime.c | 227 ++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 222 insertions(+), 5 deletions(-)
> >
> > Your patch worked mostly, below is a small addition (to be applied on top
> > of your patch), that adds several new domains and devices and fixes a
> > couple of issues with your original patch. Notice an "#if 0" around i2c
> > devices: activating them causes problems - output on the LCDC disappears
> > and the boot process doesn't complete, but without a serial console I
> > couldn't tell, what exactly was the problem. Should be investigated
> > further.
>
> Below is a fixed version of the earlier proposed fix;)
v3: I think, I've got the use-counting and power-domain initialisation
correct this time too. At least this version works without any need to
modify the runtime PM core.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
diff --git a/arch/arm/mach-shmobile/pm_runtime.c b/arch/arm/mach-shmobile/pm_runtime.c
index 48d6e4d..cd0266f 100644
--- a/arch/arm/mach-shmobile/pm_runtime.c
+++ b/arch/arm/mach-shmobile/pm_runtime.c
@@ -31,6 +33,12 @@ struct pm_runtime_power_domain {
/* sh7372 specific stuff in common file, FIXME this should be broken out */
+/*
+ * Further domains:
+ * A4R: CEU, VEU, BEU, VOU, VPU, CSI2, MSIOF0, IIC0/2
+ * A3SP: MMCIF, MSIOF1/2, SDHI0/1/2, SCIFA, SCIFB, IIC1/3/4, DMAC1/2/3
+ */
+
static struct pm_runtime_power_domain sh7372_a4lc = {
.power_domain_bit = 1,
};
@@ -39,6 +47,14 @@ static struct pm_runtime_power_domain sh7372_a4mp = {
.power_domain_bit = 2,
};
+static struct pm_runtime_power_domain sh7372_a4r = {
+ .power_domain_bit = 5,
+};
+
+static struct pm_runtime_power_domain sh7372_a3sp = {
+ .power_domain_bit = 11,
+};
+
#define SPDCR 0xe6180008
#define SWUCR 0xe6180014
#define PSTR 0xe6180080
@@ -143,7 +159,14 @@ static int power_domain_runtime_suspend(struct device *dev)
spin_lock_irqsave(&domain_data->lock, flags);
- list_add_tail(&prd->entry, &domain_data->idle_list);
+ /*
+ * suspend can be called repeatedly - see rpm_suspend() -
+ * rpm_check_suspend_allowed() returns 1, if already suspended, and the
+ * process continues.
+ */
+ if (list_empty(&prd->entry))
+ list_add_tail(&prd->entry, &domain_data->idle_list);
+
domain_data->use_cnt--;
if (domain_data->use_cnt = 0) {
@@ -213,7 +236,7 @@ static int power_domain_runtime_resume(struct device *dev)
if (ret = 0) {
domain_data->use_cnt++;
- list_del(&prd->entry);
+ list_del_init(&prd->entry);
} else {
if (domain_data->use_cnt = 0)
power_down(domain_data);
@@ -235,23 +258,60 @@ static void power_domain_init(struct device *dev, struct pm_runtime_data *prd)
if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.0") = 0)
domain_data = &sh7372_a4lc;
-
- if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.1") = 0)
+ else if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.1") = 0)
domain_data = &sh7372_a4lc;
-
- if (strcmp(dev_name(dev), "sh_fsi2") = 0)
+ else if (strcmp(dev_name(dev), "sh-mipi-dsi.0") = 0)
+ domain_data = &sh7372_a4lc;
+ else if (strcmp(dev_name(dev), "sh_fsi2") = 0)
domain_data = &sh7372_a4mp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.3") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.4") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.5") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.6") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_ceu.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.3") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.4") = 0)
+ domain_data = &sh7372_a3sp;
+#if 0
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.0") = 0)
+ domain_data = &sh7372_a4r;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.2") = 0)
+ domain_data = &sh7372_a4r;
+#endif
+ else if (strcmp(dev_name(dev), "sh_mmcif.0") = 0)
+ domain_data = &sh7372_a3sp;
if (domain_data) {
dev->pwr_domain = &power_domain;
prd->dev = dev;
INIT_LIST_HEAD(&prd->entry);
prd->domain_data = domain_data;
- domain_data->use_cnt++;
- if (domain_data->use_cnt = 1) {
- INIT_LIST_HEAD(&domain_data->idle_list);
- spin_lock_init(&domain_data->lock);
- }
dev_dbg(dev, "domain data available use cnt = %d\n",
domain_data->use_cnt);
@@ -266,7 +326,6 @@ static void platform_pm_runtime_init(struct device *dev,
if (!IS_ERR(prd->clk)) {
set_bit(BIT_ACTIVE, &prd->flags);
dev_info(dev, "clocks managed by runtime pm\n");
- power_domain_init(dev, prd);
}
}
}
@@ -291,7 +350,6 @@ int platform_pm_runtime_suspend(struct device *dev)
clk_disable(prd->clk);
clear_bit(BIT_CLK_ENABLED, &prd->flags);
}
-
}
return 0;
@@ -329,12 +387,24 @@ static int platform_bus_notify(struct notifier_block *nb,
dev_dbg(dev, "platform_bus_notify() %ld !\n", action);
- if (action = BUS_NOTIFY_BIND_DRIVER) {
+ switch (action) {
+ case BUS_NOTIFY_BIND_DRIVER:
prd = devres_alloc(__devres_release, sizeof(*prd), GFP_KERNEL);
- if (prd)
+ if (prd) {
devres_add(dev, prd);
- else
+ power_domain_init(dev, prd);
+ } else {
dev_err(dev, "unable to alloc memory for runtime pm\n");
+ }
+ break;
+ case BUS_NOTIFY_UNBIND_DRIVER:
+ prd = __to_prd(dev);
+ if (prd)
+ list_del(&prd->entry);
+ break;
+ case BUS_NOTIFY_UNBOUND_DRIVER:
+ dev->pwr_domain = NULL;
+ break;
}
return 0;
@@ -380,6 +450,15 @@ static struct notifier_block platform_bus_notifier = {
static int __init sh_pm_runtime_init(void)
{
+ INIT_LIST_HEAD(&sh7372_a4lc.idle_list);
+ spin_lock_init(&sh7372_a4lc.lock);
+ INIT_LIST_HEAD(&sh7372_a4mp.idle_list);
+ spin_lock_init(&sh7372_a4mp.lock);
+ INIT_LIST_HEAD(&sh7372_a4r.idle_list);
+ spin_lock_init(&sh7372_a4r.lock);
+ INIT_LIST_HEAD(&sh7372_a3sp.idle_list);
+ spin_lock_init(&sh7372_a3sp.lock);
+
bus_register_notifier(&platform_bus_type, &platform_bus_notifier);
return 0;
}
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype
2011-04-11 19:49 [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype Magnus Damm
` (5 preceding siblings ...)
2011-04-21 7:37 ` Guennadi Liakhovetski
@ 2011-04-29 17:19 ` Guennadi Liakhovetski
2011-04-29 20:16 ` Rafael J. Wysocki
` (3 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Guennadi Liakhovetski @ 2011-04-29 17:19 UTC (permalink / raw)
To: linux-sh
On Thu, 21 Apr 2011, Guennadi Liakhovetski wrote:
> On Tue, 19 Apr 2011, Guennadi Liakhovetski wrote:
>
> > On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote:
> >
> > > Hi Magnus
> > >
> > > On Tue, 12 Apr 2011, Magnus Damm wrote:
> > >
> > > > From: Magnus Damm <damm@opensource.se>
> > > >
> > > > This experimental sh7372-specific Runtime PM prototype
> > > > adds support for the A4LC and A4MP power domains.
> > > >
> > > > Tested on the Mackerel board together with the LCDC:
> > > >
> > > > # blank HDMI fbdev
> > > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.1/graphics/fb1/blank
> > > >
> > > > # blank LCD panel fbdev
> > > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > > > # unblank LCD panel fbdev
> > > > echo 0 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > > >
> > > > Not-yet-signed-off-by: Magnus Damm <damm@opensource.se>
> > > > ---
> > > >
> > > > arch/arm/mach-shmobile/pm_runtime.c | 227 ++++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 222 insertions(+), 5 deletions(-)
> > >
> > > Your patch worked mostly, below is a small addition (to be applied on top
> > > of your patch), that adds several new domains and devices and fixes a
> > > couple of issues with your original patch. Notice an "#if 0" around i2c
> > > devices: activating them causes problems - output on the LCDC disappears
> > > and the boot process doesn't complete, but without a serial console I
> > > couldn't tell, what exactly was the problem. Should be investigated
> > > further.
> >
> > Below is a fixed version of the earlier proposed fix;)
>
> v3: I think, I've got the use-counting and power-domain initialisation
> correct this time too. At least this version works without any need to
> modify the runtime PM core.
v4: (1) add checks for BIT_ACTIVE for devices without any clocks
associated with them, this fixes Oopses. (2) I think, this time I've
finally found _the_ correct time and place to remove the "prd" from the
list. As before, this is a work in progress, subject to change, depending
on surrounding developments. Posting here just for reference.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
diff --git a/arch/arm/mach-shmobile/pm_runtime.c b/arch/arm/mach-shmobile/pm_runtime.c
index 48d6e4d..320d24d 100644
--- a/arch/arm/mach-shmobile/pm_runtime.c
+++ b/arch/arm/mach-shmobile/pm_runtime.c
@@ -31,6 +33,12 @@ struct pm_runtime_power_domain {
/* sh7372 specific stuff in common file, FIXME this should be broken out */
+/*
+ * Further domains:
+ * A4R: CEU, VEU, BEU, VOU, VPU, CSI2, MSIOF0, IIC0/2
+ * A3SP: MMCIF, MSIOF1/2, SDHI0/1/2, SCIFA, SCIFB, IIC1/3/4, DMAC1/2/3
+ */
+
static struct pm_runtime_power_domain sh7372_a4lc = {
.power_domain_bit = 1,
};
@@ -39,6 +47,14 @@ static struct pm_runtime_power_domain sh7372_a4mp = {
.power_domain_bit = 2,
};
+static struct pm_runtime_power_domain sh7372_a4r = {
+ .power_domain_bit = 5,
+};
+
+static struct pm_runtime_power_domain sh7372_a3sp = {
+ .power_domain_bit = 11,
+};
+
#define SPDCR 0xe6180008
#define SWUCR 0xe6180014
#define PSTR 0xe6180080
@@ -93,6 +109,8 @@ static void __devres_release(struct device *dev, void *res)
dev_dbg(dev, "__devres_release()\n");
+ list_del(&prd->entry);
+
if (test_bit(BIT_CLK_ENABLED, &prd->flags))
clk_disable(prd->clk);
@@ -117,10 +135,12 @@ static int __power_domain_runtime_suspend(struct device *d)
if (!test_bit(BIT_SUSPENDED, &prd->flags)) {
if (d->driver && d->driver->pm &&
d->driver->pm->runtime_suspend) {
- if (!test_bit(BIT_CLK_ENABLED, &prd->flags))
+ if (test_bit(BIT_ACTIVE, &prd->flags) &&
+ !test_bit(BIT_CLK_ENABLED, &prd->flags))
clk_enable(prd->clk);
ret = d->driver->pm->runtime_suspend(d);
- clk_disable(prd->clk);
+ if (test_bit(BIT_ACTIVE, &prd->flags))
+ clk_disable(prd->clk);
clear_bit(BIT_CLK_ENABLED, &prd->flags);
if (ret = 0)
@@ -143,7 +163,14 @@ static int power_domain_runtime_suspend(struct device *dev)
spin_lock_irqsave(&domain_data->lock, flags);
- list_add_tail(&prd->entry, &domain_data->idle_list);
+ /*
+ * suspend can be called repeatedly - see rpm_suspend() -
+ * rpm_check_suspend_allowed() returns 1, if already suspended, and the
+ * process continues.
+ */
+ if (list_empty(&prd->entry))
+ list_add_tail(&prd->entry, &domain_data->idle_list);
+
domain_data->use_cnt--;
if (domain_data->use_cnt = 0) {
@@ -169,12 +196,13 @@ static int __power_domain_runtime_resume(struct device *d)
struct pm_runtime_data *prd = __to_prd(d);
int ret = 0;
- dev_dbg(d, "__power_domain_runtime_resume()\n");
+ dev_dbg(d, "__power_domain_runtime_resume(): %lu\n", prd->flags);
BUG_ON(!prd);
if (test_bit(BIT_SUSPENDED, &prd->flags)) {
- if (!test_bit(BIT_CLK_ENABLED, &prd->flags)) {
+ if (test_bit(BIT_ACTIVE, &prd->flags) &&
+ !test_bit(BIT_CLK_ENABLED, &prd->flags)) {
clk_enable(prd->clk);
set_bit(BIT_CLK_ENABLED, &prd->flags);
}
@@ -186,7 +214,8 @@ static int __power_domain_runtime_resume(struct device *d)
if (ret = 0)
clear_bit(BIT_SUSPENDED, &prd->flags);
else {
- clk_disable(prd->clk);
+ if (test_bit(BIT_ACTIVE, &prd->flags))
+ clk_disable(prd->clk);
clear_bit(BIT_CLK_ENABLED, &prd->flags);
}
}
@@ -213,7 +242,7 @@ static int power_domain_runtime_resume(struct device *dev)
if (ret = 0) {
domain_data->use_cnt++;
- list_del(&prd->entry);
+ list_del_init(&prd->entry);
} else {
if (domain_data->use_cnt = 0)
power_down(domain_data);
@@ -235,23 +264,60 @@ static void power_domain_init(struct device *dev, struct pm_runtime_data *prd)
if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.0") = 0)
domain_data = &sh7372_a4lc;
-
- if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.1") = 0)
+ else if (strcmp(dev_name(dev), "sh_mobile_lcdc_fb.1") = 0)
domain_data = &sh7372_a4lc;
-
- if (strcmp(dev_name(dev), "sh_fsi2") = 0)
+ else if (strcmp(dev_name(dev), "sh-mipi-dsi.0") = 0)
+ domain_data = &sh7372_a4lc;
+ else if (strcmp(dev_name(dev), "sh_fsi2") = 0)
domain_data = &sh7372_a4mp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_sdhi.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-dma-engine.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.2") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.3") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.4") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.5") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh-sci.6") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "sh_mobile_ceu.0") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.1") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.3") = 0)
+ domain_data = &sh7372_a3sp;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.4") = 0)
+ domain_data = &sh7372_a3sp;
+#if 0
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.0") = 0)
+ domain_data = &sh7372_a4r;
+ else if (strcmp(dev_name(dev), "i2c-sh_mobile.2") = 0)
+ domain_data = &sh7372_a4r;
+#endif
+ else if (strcmp(dev_name(dev), "sh_mmcif.0") = 0)
+ domain_data = &sh7372_a3sp;
if (domain_data) {
dev->pwr_domain = &power_domain;
prd->dev = dev;
INIT_LIST_HEAD(&prd->entry);
prd->domain_data = domain_data;
- domain_data->use_cnt++;
- if (domain_data->use_cnt = 1) {
- INIT_LIST_HEAD(&domain_data->idle_list);
- spin_lock_init(&domain_data->lock);
- }
dev_dbg(dev, "domain data available use cnt = %d\n",
domain_data->use_cnt);
@@ -266,7 +332,8 @@ static void platform_pm_runtime_init(struct device *dev,
if (!IS_ERR(prd->clk)) {
set_bit(BIT_ACTIVE, &prd->flags);
dev_info(dev, "clocks managed by runtime pm\n");
- power_domain_init(dev, prd);
+ } else {
+ dev_err(dev, "clk-get error %ld\n", PTR_ERR(prd->clk));
}
}
}
@@ -291,7 +358,6 @@ int platform_pm_runtime_suspend(struct device *dev)
clk_disable(prd->clk);
clear_bit(BIT_CLK_ENABLED, &prd->flags);
}
-
}
return 0;
@@ -329,12 +395,19 @@ static int platform_bus_notify(struct notifier_block *nb,
dev_dbg(dev, "platform_bus_notify() %ld !\n", action);
- if (action = BUS_NOTIFY_BIND_DRIVER) {
+ switch (action) {
+ case BUS_NOTIFY_BIND_DRIVER:
prd = devres_alloc(__devres_release, sizeof(*prd), GFP_KERNEL);
- if (prd)
+ if (prd) {
devres_add(dev, prd);
- else
+ power_domain_init(dev, prd);
+ } else {
dev_err(dev, "unable to alloc memory for runtime pm\n");
+ }
+ break;
+ case BUS_NOTIFY_UNBOUND_DRIVER:
+ dev->pwr_domain = NULL;
+ break;
}
return 0;
@@ -380,6 +453,15 @@ static struct notifier_block platform_bus_notifier = {
static int __init sh_pm_runtime_init(void)
{
+ INIT_LIST_HEAD(&sh7372_a4lc.idle_list);
+ spin_lock_init(&sh7372_a4lc.lock);
+ INIT_LIST_HEAD(&sh7372_a4mp.idle_list);
+ spin_lock_init(&sh7372_a4mp.lock);
+ INIT_LIST_HEAD(&sh7372_a4r.idle_list);
+ spin_lock_init(&sh7372_a4r.lock);
+ INIT_LIST_HEAD(&sh7372_a3sp.idle_list);
+ spin_lock_init(&sh7372_a3sp.lock);
+
bus_register_notifier(&platform_bus_type, &platform_bus_notifier);
return 0;
}
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype
2011-04-11 19:49 [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype Magnus Damm
` (6 preceding siblings ...)
2011-04-29 17:19 ` Guennadi Liakhovetski
@ 2011-04-29 20:16 ` Rafael J. Wysocki
2011-04-30 1:02 ` Rafael J. Wysocki
` (2 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2011-04-29 20:16 UTC (permalink / raw)
To: linux-sh
On Friday, April 29, 2011, Guennadi Liakhovetski wrote:
> On Thu, 21 Apr 2011, Guennadi Liakhovetski wrote:
>
> > On Tue, 19 Apr 2011, Guennadi Liakhovetski wrote:
> >
> > > On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote:
> > >
> > > > Hi Magnus
> > > >
> > > > On Tue, 12 Apr 2011, Magnus Damm wrote:
> > > >
> > > > > From: Magnus Damm <damm@opensource.se>
> > > > >
> > > > > This experimental sh7372-specific Runtime PM prototype
> > > > > adds support for the A4LC and A4MP power domains.
> > > > >
> > > > > Tested on the Mackerel board together with the LCDC:
> > > > >
> > > > > # blank HDMI fbdev
> > > > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.1/graphics/fb1/blank
> > > > >
> > > > > # blank LCD panel fbdev
> > > > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > > > > # unblank LCD panel fbdev
> > > > > echo 0 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > > > >
> > > > > Not-yet-signed-off-by: Magnus Damm <damm@opensource.se>
> > > > > ---
> > > > >
> > > > > arch/arm/mach-shmobile/pm_runtime.c | 227 ++++++++++++++++++++++++++++++++++-
> > > > > 1 file changed, 222 insertions(+), 5 deletions(-)
> > > >
> > > > Your patch worked mostly, below is a small addition (to be applied on top
> > > > of your patch), that adds several new domains and devices and fixes a
> > > > couple of issues with your original patch. Notice an "#if 0" around i2c
> > > > devices: activating them causes problems - output on the LCDC disappears
> > > > and the boot process doesn't complete, but without a serial console I
> > > > couldn't tell, what exactly was the problem. Should be investigated
> > > > further.
> > >
> > > Below is a fixed version of the earlier proposed fix;)
> >
> > v3: I think, I've got the use-counting and power-domain initialisation
> > correct this time too. At least this version works without any need to
> > modify the runtime PM core.
>
> v4: (1) add checks for BIT_ACTIVE for devices without any clocks
> associated with them, this fixes Oopses. (2) I think, this time I've
> finally found _the_ correct time and place to remove the "prd" from the
> list. As before, this is a work in progress, subject to change, depending
> on surrounding developments. Posting here just for reference.
Please have a look at these patches:
https://patchwork.kernel.org/patch/740162/
https://patchwork.kernel.org/patch/740152/
which are based on the patchset from:
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/suspend-2.6.git power-domains
Do you think it would be possible to rebase your work on top of them?
If not, please tell me what the problems are.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype
2011-04-11 19:49 [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype Magnus Damm
` (7 preceding siblings ...)
2011-04-29 20:16 ` Rafael J. Wysocki
@ 2011-04-30 1:02 ` Rafael J. Wysocki
2011-05-03 8:22 ` Guennadi Liakhovetski
2011-05-03 22:37 ` Rafael J. Wysocki
10 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2011-04-30 1:02 UTC (permalink / raw)
To: linux-sh
On Friday, April 29, 2011, Rafael J. Wysocki wrote:
> On Friday, April 29, 2011, Guennadi Liakhovetski wrote:
> > On Thu, 21 Apr 2011, Guennadi Liakhovetski wrote:
> >
> > > On Tue, 19 Apr 2011, Guennadi Liakhovetski wrote:
> > >
> > > > On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote:
> > > >
> > > > > Hi Magnus
> > > > >
> > > > > On Tue, 12 Apr 2011, Magnus Damm wrote:
> > > > >
> > > > > > From: Magnus Damm <damm@opensource.se>
> > > > > >
> > > > > > This experimental sh7372-specific Runtime PM prototype
> > > > > > adds support for the A4LC and A4MP power domains.
> > > > > >
> > > > > > Tested on the Mackerel board together with the LCDC:
> > > > > >
> > > > > > # blank HDMI fbdev
> > > > > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.1/graphics/fb1/blank
> > > > > >
> > > > > > # blank LCD panel fbdev
> > > > > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > > > > > # unblank LCD panel fbdev
> > > > > > echo 0 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > > > > >
> > > > > > Not-yet-signed-off-by: Magnus Damm <damm@opensource.se>
> > > > > > ---
> > > > > >
> > > > > > arch/arm/mach-shmobile/pm_runtime.c | 227 ++++++++++++++++++++++++++++++++++-
> > > > > > 1 file changed, 222 insertions(+), 5 deletions(-)
> > > > >
> > > > > Your patch worked mostly, below is a small addition (to be applied on top
> > > > > of your patch), that adds several new domains and devices and fixes a
> > > > > couple of issues with your original patch. Notice an "#if 0" around i2c
> > > > > devices: activating them causes problems - output on the LCDC disappears
> > > > > and the boot process doesn't complete, but without a serial console I
> > > > > couldn't tell, what exactly was the problem. Should be investigated
> > > > > further.
> > > >
> > > > Below is a fixed version of the earlier proposed fix;)
> > >
> > > v3: I think, I've got the use-counting and power-domain initialisation
> > > correct this time too. At least this version works without any need to
> > > modify the runtime PM core.
> >
> > v4: (1) add checks for BIT_ACTIVE for devices without any clocks
> > associated with them, this fixes Oopses. (2) I think, this time I've
> > finally found _the_ correct time and place to remove the "prd" from the
> > list. As before, this is a work in progress, subject to change, depending
> > on surrounding developments. Posting here just for reference.
>
> Please have a look at these patches:
> https://patchwork.kernel.org/patch/740162/
> https://patchwork.kernel.org/patch/740152/
> which are based on the patchset from:
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/suspend-2.6.git power-domains
I had to rebase this branch due to recent modifications requested by reviewers.
Also the two patches above have been updated, but the latest versions have
been tested with the A4LC power domain and LCDC and confirmed to work.
> Do you think it would be possible to rebase your work on top of them?
> If not, please tell me what the problems are.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype
2011-04-11 19:49 [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype Magnus Damm
` (8 preceding siblings ...)
2011-04-30 1:02 ` Rafael J. Wysocki
@ 2011-05-03 8:22 ` Guennadi Liakhovetski
2011-05-03 22:37 ` Rafael J. Wysocki
10 siblings, 0 replies; 12+ messages in thread
From: Guennadi Liakhovetski @ 2011-05-03 8:22 UTC (permalink / raw)
To: linux-sh
On Sat, 30 Apr 2011, Rafael J. Wysocki wrote:
> On Friday, April 29, 2011, Rafael J. Wysocki wrote:
> > On Friday, April 29, 2011, Guennadi Liakhovetski wrote:
> > > On Thu, 21 Apr 2011, Guennadi Liakhovetski wrote:
> > >
> > > > On Tue, 19 Apr 2011, Guennadi Liakhovetski wrote:
> > > >
> > > > > On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote:
> > > > >
> > > > > > Hi Magnus
> > > > > >
> > > > > > On Tue, 12 Apr 2011, Magnus Damm wrote:
> > > > > >
> > > > > > > From: Magnus Damm <damm@opensource.se>
> > > > > > >
> > > > > > > This experimental sh7372-specific Runtime PM prototype
> > > > > > > adds support for the A4LC and A4MP power domains.
> > > > > > >
> > > > > > > Tested on the Mackerel board together with the LCDC:
> > > > > > >
> > > > > > > # blank HDMI fbdev
> > > > > > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.1/graphics/fb1/blank
> > > > > > >
> > > > > > > # blank LCD panel fbdev
> > > > > > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > > > > > > # unblank LCD panel fbdev
> > > > > > > echo 0 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > > > > > >
> > > > > > > Not-yet-signed-off-by: Magnus Damm <damm@opensource.se>
> > > > > > > ---
> > > > > > >
> > > > > > > arch/arm/mach-shmobile/pm_runtime.c | 227 ++++++++++++++++++++++++++++++++++-
> > > > > > > 1 file changed, 222 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > Your patch worked mostly, below is a small addition (to be applied on top
> > > > > > of your patch), that adds several new domains and devices and fixes a
> > > > > > couple of issues with your original patch. Notice an "#if 0" around i2c
> > > > > > devices: activating them causes problems - output on the LCDC disappears
> > > > > > and the boot process doesn't complete, but without a serial console I
> > > > > > couldn't tell, what exactly was the problem. Should be investigated
> > > > > > further.
> > > > >
> > > > > Below is a fixed version of the earlier proposed fix;)
> > > >
> > > > v3: I think, I've got the use-counting and power-domain initialisation
> > > > correct this time too. At least this version works without any need to
> > > > modify the runtime PM core.
> > >
> > > v4: (1) add checks for BIT_ACTIVE for devices without any clocks
> > > associated with them, this fixes Oopses. (2) I think, this time I've
> > > finally found _the_ correct time and place to remove the "prd" from the
> > > list. As before, this is a work in progress, subject to change, depending
> > > on surrounding developments. Posting here just for reference.
> >
> > Please have a look at these patches:
> > https://patchwork.kernel.org/patch/740162/
> > https://patchwork.kernel.org/patch/740152/
> > which are based on the patchset from:
> > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/suspend-2.6.git power-domains
>
> I had to rebase this branch due to recent modifications requested by reviewers.
>
> Also the two patches above have been updated, but the latest versions have
> been tested with the A4LC power domain and LCDC and confirmed to work.
Ok, I rebased my patches on top of your work. In principle, it works
nicely practically out of the box. Just had to add power domain
definitions, and add my devices to those domains - nice work!
But now I have the following question: currently for rtpm to power down a
domain, all devices' rtpm has to be enabled. Which is not the case by
default without drivers loaded. This is already my first doubt - if there
is no driver, shall the device be really holding the domain? Does it have
to be powered on? Maybe we could define a flag in device PM properties for
this? Then, if a driver is bound to a device, it usually enables rtpm, so,
as long as it is bound, it can put the device, which can lead to domain
powering down - if all other devices agree. But when the driver unbinds
from the device again we have the same problem as above. Also, in some
cases unbinding the driver is (almost) the only possibility to identify
the moment, from which the device is not needed anymore. Shall we keep its
rtpm enabled then?
For example, consider SDHI / TMIO SD-host driver on sh-mobile. As long as
TMIO is bound to the device, it has to watch for card insert / eject
events, and for that it has to be powered on. There are also many other
peripherals in the same power domain, so, to power the domain down you
either have to disable card-detection, or unbind the driver, but keep the
rtpm enabled. What do you think is the preferred way to handle this?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype
2011-04-11 19:49 [PATCH/RFC] ARM: mach-shmobile: sh7372 power domain prototype Magnus Damm
` (9 preceding siblings ...)
2011-05-03 8:22 ` Guennadi Liakhovetski
@ 2011-05-03 22:37 ` Rafael J. Wysocki
10 siblings, 0 replies; 12+ messages in thread
From: Rafael J. Wysocki @ 2011-05-03 22:37 UTC (permalink / raw)
To: linux-sh
On Tuesday, May 03, 2011, Guennadi Liakhovetski wrote:
> On Sat, 30 Apr 2011, Rafael J. Wysocki wrote:
>
> > On Friday, April 29, 2011, Rafael J. Wysocki wrote:
> > > On Friday, April 29, 2011, Guennadi Liakhovetski wrote:
> > > > On Thu, 21 Apr 2011, Guennadi Liakhovetski wrote:
> > > >
> > > > > On Tue, 19 Apr 2011, Guennadi Liakhovetski wrote:
> > > > >
> > > > > > On Fri, 15 Apr 2011, Guennadi Liakhovetski wrote:
> > > > > >
> > > > > > > Hi Magnus
> > > > > > >
> > > > > > > On Tue, 12 Apr 2011, Magnus Damm wrote:
> > > > > > >
> > > > > > > > From: Magnus Damm <damm@opensource.se>
> > > > > > > >
> > > > > > > > This experimental sh7372-specific Runtime PM prototype
> > > > > > > > adds support for the A4LC and A4MP power domains.
> > > > > > > >
> > > > > > > > Tested on the Mackerel board together with the LCDC:
> > > > > > > >
> > > > > > > > # blank HDMI fbdev
> > > > > > > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.1/graphics/fb1/blank
> > > > > > > >
> > > > > > > > # blank LCD panel fbdev
> > > > > > > > echo 3 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > > > > > > > # unblank LCD panel fbdev
> > > > > > > > echo 0 > /sys/devices/platform/sh_mobile_lcdc_fb.0/graphics/fb0/blank
> > > > > > > >
> > > > > > > > Not-yet-signed-off-by: Magnus Damm <damm@opensource.se>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > arch/arm/mach-shmobile/pm_runtime.c | 227 ++++++++++++++++++++++++++++++++++-
> > > > > > > > 1 file changed, 222 insertions(+), 5 deletions(-)
> > > > > > >
> > > > > > > Your patch worked mostly, below is a small addition (to be applied on top
> > > > > > > of your patch), that adds several new domains and devices and fixes a
> > > > > > > couple of issues with your original patch. Notice an "#if 0" around i2c
> > > > > > > devices: activating them causes problems - output on the LCDC disappears
> > > > > > > and the boot process doesn't complete, but without a serial console I
> > > > > > > couldn't tell, what exactly was the problem. Should be investigated
> > > > > > > further.
> > > > > >
> > > > > > Below is a fixed version of the earlier proposed fix;)
> > > > >
> > > > > v3: I think, I've got the use-counting and power-domain initialisation
> > > > > correct this time too. At least this version works without any need to
> > > > > modify the runtime PM core.
> > > >
> > > > v4: (1) add checks for BIT_ACTIVE for devices without any clocks
> > > > associated with them, this fixes Oopses. (2) I think, this time I've
> > > > finally found _the_ correct time and place to remove the "prd" from the
> > > > list. As before, this is a work in progress, subject to change, depending
> > > > on surrounding developments. Posting here just for reference.
> > >
> > > Please have a look at these patches:
> > > https://patchwork.kernel.org/patch/740162/
> > > https://patchwork.kernel.org/patch/740152/
> > > which are based on the patchset from:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/suspend-2.6.git power-domains
> >
> > I had to rebase this branch due to recent modifications requested by reviewers.
> >
> > Also the two patches above have been updated, but the latest versions have
> > been tested with the A4LC power domain and LCDC and confirmed to work.
>
> Ok, I rebased my patches on top of your work. In principle, it works
> nicely practically out of the box. Just had to add power domain
> definitions, and add my devices to those domains - nice work!
Thanks!
> But now I have the following question: currently for rtpm to power down a
> domain, all devices' rtpm has to be enabled. Which is not the case by
> default without drivers loaded. This is already my first doubt - if there
> is no driver, shall the device be really holding the domain?
Well, that depends. In general, when runtime PM is not enabled, the
core doesn't know if it is safe to power down the device (it may be
someone's parent or something like this). This extends to power domains IMO.
However, in specific situations it may be safe to remove power from a
power domain regardless, but that would require an additional power domain
field or flag and a little more complicated code to handle that. I decided
to do the simpler thing first, but I'm not against adding the more complicated
handling on top of it in principle.
> Does it have to be powered on? Maybe we could define a flag in device PM
> properties for this?
I'm not sure if device PM properties are a good place to add the flag to.
In that case the meaning of the flag would overlap with the meaning of
disable_depth somewhat. Perhaps it's better if that's a power domain flag,
but as I said I'm not sure.
> Then, if a driver is bound to a device, it usually enables rtpm, so,
> as long as it is bound, it can put the device, which can lead to domain
> powering down - if all other devices agree. But when the driver unbinds
> from the device again we have the same problem as above. Also, in some
> cases unbinding the driver is (almost) the only possibility to identify
> the moment, from which the device is not needed anymore. Shall we keep its
> rtpm enabled then?
Well, I'm slightly against that.
It follows from my experience that trying to do runtime PM of devices without
drivers is going to fail at least for some of them (it did for PCI and USB)
and there's no good way to distinguish working from non-working ones.
For this reason, I think it's better to require that a driver be bound to the
device and let the driver enable/disable the runtime PM.
> For example, consider SDHI / TMIO SD-host driver on sh-mobile. As long as
> TMIO is bound to the device, it has to watch for card insert / eject
> events, and for that it has to be powered on. There are also many other
> peripherals in the same power domain, so, to power the domain down you
> either have to disable card-detection, or unbind the driver, but keep the
> rtpm enabled. What do you think is the preferred way to handle this?
I would go for disabling card detection.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 12+ messages in thread