[PATCH 1/1] ARM: EXYNOS: Add enable property to power domains

[PATCH 1/1] ARM: EXYNOS: Add enable property to power domains

[Sachin Kamat]

From: Prathyush K <prathyush.k@samsung.com>

Different power domains of Exynos SOCs have different enable values.
E.g. Exynos5250:
ROTATOR_MEM_CONFIGURATION -> 0x3
GSCL_CONFIGURATION -> 0x7
Currently, there is no way to differentiate between these power domains
and we write default value of 0x7 to turn on all the power domains.

This patch adds a new 'enable' property to the power domain structure.
This enable value can be set from the device tree by adding a property
'enable' in the device node. If no such property is found, the default
value of 0x7 is used as enable value.

Signed-off-by: Prathyush K <prathyush.k@samsung.com>
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 .../bindings/arm/exynos/power_domain.txt           |    5 +++++
 arch/arm/mach-exynos/pm_domains.c                  |   10 +++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/exynos/power_domain.txt b/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
index 5216b419016a..6b24b234617c 100644
--- a/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
+++ b/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
@@ -9,6 +9,10 @@ Required Properties:
 - reg: physical base address of the controller and length of memory mapped
     region.
 
+Optional Properties:
+- enable: enable value of the register which is used to turn on the power
+    domain. If no enable is specificed, default value of 0x7 is used.
+
 Node of a device using power domains must have a samsung,power-domain property
 defined with a phandle to respective power domain.
 
@@ -17,6 +21,7 @@ Example:
 	lcd0: power-domain-lcd0 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x10023C00 0x10>;
+		enable = <0x1>;
 	};
 
 Example of the node using power domain:
diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
index 1703593e366c..84e0483a0500 100644
--- a/arch/arm/mach-exynos/pm_domains.c
+++ b/arch/arm/mach-exynos/pm_domains.c
@@ -33,6 +33,7 @@ struct exynos_pm_domain {
 	char const *name;
 	bool is_off;
 	struct generic_pm_domain pd;
+	u32 enable;
 };
 
 static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
@@ -45,13 +46,13 @@ static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
 	pd = container_of(domain, struct exynos_pm_domain, pd);
 	base = pd->base;
 
-	pwr = power_on ? S5P_INT_LOCAL_PWR_EN : 0;
+	pwr = power_on ? pd->enable : 0;
 	__raw_writel(pwr, base);
 
 	/* Wait max 1ms */
 	timeout = 10;
 
-	while ((__raw_readl(base + 0x4) & S5P_INT_LOCAL_PWR_EN)	!= pwr) {
+	while ((__raw_readl(base + 0x4) & pd->enable) != pwr) {
 		if (!timeout) {
 			op = (power_on) ? "enable" : "disable";
 			pr_err("Power domain %s %s failed\n", domain->name, op);
@@ -164,6 +165,9 @@ static __init int exynos4_pm_init_power_domain(void)
 			return -ENOMEM;
 		}
 
+		if (of_property_read_u32(np, "enable", &pd->enable))
+			pd->enable = S5P_INT_LOCAL_PWR_EN;
+
 		pd->pd.name = kstrdup(np->name, GFP_KERNEL);
 		pd->name = pd->pd.name;
 		pd->base = of_iomap(np, 0);
@@ -173,7 +177,7 @@ static __init int exynos4_pm_init_power_domain(void)
 
 		platform_set_drvdata(pdev, pd);
 
-		on = __raw_readl(pd->base + 0x4) & S5P_INT_LOCAL_PWR_EN;
+		on = __raw_readl(pd->base + 0x4) & pd->enable;
 
 		pm_genpd_init(&pd->pd, NULL, !on);
 	}
-- 
1.7.9.5

Re: [PATCH 1/1] ARM: EXYNOS: Add enable property to power domains

[Tomasz Figa]

Hi Sachin,

On Thursday 07 of November 2013 12:12:55 Sachin Kamat wrote:
> From: Prathyush K <prathyush.k@samsung.com>
> 
> Different power domains of Exynos SOCs have different enable values.
> E.g. Exynos5250:
> ROTATOR_MEM_CONFIGURATION -> 0x3
> GSCL_CONFIGURATION -> 0x7
> Currently, there is no way to differentiate between these power domains
> and we write default value of 0x7 to turn on all the power domains.
> 
> This patch adds a new 'enable' property to the power domain structure.
> This enable value can be set from the device tree by adding a property
> 'enable' in the device node. If no such property is found, the default
> value of 0x7 is used as enable value.

Is this patch really needed? Is there any problem with simply using 0x7?
Patch description should always include rationale behind the change.

> 
> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  .../bindings/arm/exynos/power_domain.txt           |    5 +++++
>  arch/arm/mach-exynos/pm_domains.c                  |   10 +++++++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/exynos/power_domain.txt b/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> index 5216b419016a..6b24b234617c 100644
> --- a/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> +++ b/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> @@ -9,6 +9,10 @@ Required Properties:
>  - reg: physical base address of the controller and length of memory mapped
>      region.
>  
> +Optional Properties:
> +- enable: enable value of the register which is used to turn on the power
> +    domain. If no enable is specificed, default value of 0x7 is used.

Vendor-specific properties should have vendor prefix added, so this one
should be called samsung,enable instead. Also enable is not a very
specific name.

So in the end, if it turns out that we really need such patch, I'd prefer
something like:

- samsung,enable-bit-mask: Mask of power control register bits that need
    to be set to enable the power domain. If omitted, it defaults to 0x7.

Best regards,
Tomasz

Re: [PATCH 1/1] ARM: EXYNOS: Add enable property to power domains

[Tomasz Figa]

Hi Prathyush,

Thanks for your reply. However, please do not use HTML messages to reply
to mailing lists, as many of them filter out such.

On Monday 11 of November 2013 05:53:18 Prathyush Kalashwaram wrote:
> Hi Tomasz,
>  
> The enable values for some configuration registers are not 0x7.
> Though there is no issue observed (so far) in using 0x7, it is better to use the correct enable values.
> Thats the reason for this patch.

Well, this means that the patch does not fix or improve anything, just
introduces changes that are not needed for anything. This is especially
bad, because it introduces a change in DT binding that is not needed yet
and might turn out to be insufficient for future SoCs.

This patch will be okay, if a need for explicit mask specification shows
up in future, though.

Best regards,
Tomasz

> And you're right. Using "samsung,enable-bit-mask" makes more sense. I will change and update with v2.
>  
> Regards,
> Prathyush
>  
> ------- Original Message -------
> Sender : Tomasz Figa<tomasz.figa@gmail.com>
> Date : Nov 11, 2013 00:36 (GMT+05:30)
> Title : Re: [PATCH 1/1] ARM: EXYNOS: Add enable property to power domains
>  
> Hi Sachin,
> 
> On Thursday 07 of November 2013 12:12:55 Sachin Kamat wrote:
> > From: Prathyush K
> >
> > Different power domains of Exynos SOCs have different enable values.
> > E.g. Exynos5250:
> > ROTATOR_MEM_CONFIGURATION -> 0x3
> > GSCL_CONFIGURATION -> 0x7
> > Currently, there is no way to differentiate between these power domains
> > and we write default value of 0x7 to turn on all the power domains.
> >
> > This patch adds a new 'enable' property to the power domain structure.
> > This enable value can be set from the device tree by adding a property
> > 'enable' in the device node. If no such property is found, the default
> > value of 0x7 is used as enable value.
> 
> Is this patch really needed? Is there any problem with simply using 0x7?
> Patch description should always include rationale behind the change.
> 
> >
> > Signed-off-by: Prathyush K
> > Signed-off-by: Sachin Kamat
> > ---
> >  .../bindings/arm/exynos/power_domain.txt           |    5 +++++
> >  arch/arm/mach-exynos/pm_domains.c                  |   10 +++++++---
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/exynos/power_domain.txt b/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> > index 5216b419016a..6b24b234617c 100644
> > --- a/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> > +++ b/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> > @@ -9,6 +9,10 @@ Required Properties:
> >  - reg: physical base address of the controller and length of memory mapped
> >      region.
> >  
> > +Optional Properties:
> > +- enable: enable value of the register which is used to turn on the power
> > +    domain. If no enable is specificed, default value of 0x7 is used.
> 
> Vendor-specific properties should have vendor prefix added, so this one
> should be called samsung,enable instead. Also enable is not a very
> specific name.
> 
> So in the end, if it turns out that we really need such patch, I'd prefer
> something like:
> 
> - samsung,enable-bit-mask: Mask of power control register bits that need
>     to be set to enable the power domain. If omitted, it defaults to 0x7.
> 
> Best regards,
> Tomasz
> 
>  
>  
>  
> 
> 

Re: [PATCH 1/1] ARM: EXYNOS: Add enable property to power domains

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday, November 11, 2013 03:28:44 PM Tomasz Figa wrote:
> Hi Prathyush,
> 
> Thanks for your reply. However, please do not use HTML messages to reply
> to mailing lists, as many of them filter out such.
> 
> On Monday 11 of November 2013 05:53:18 Prathyush Kalashwaram wrote:
> > Hi Tomasz,
> >  
> > The enable values for some configuration registers are not 0x7.
> > Though there is no issue observed (so far) in using 0x7, it is better to use the correct enable values.
> > Thats the reason for this patch.
> 
> Well, this means that the patch does not fix or improve anything, just
> introduces changes that are not needed for anything. This is especially

Writing reserved bits of hardware registers is not a good practice and
such behavior should be fixed.  However in this particular case of power
domains I don't see a need for fixing anything.  After looking at the
documentation and the code I see no improper use of currently supported
power domains.

Prathyush, could you please confirm that there are no currently supported
power domains (defined in exynos*.dtsi device tree files) that need fixing?

> bad, because it introduces a change in DT binding that is not needed yet
> and might turn out to be insufficient for future SoCs.
> 
> This patch will be okay, if a need for explicit mask specification shows
> up in future, though.

Agreed.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> Best regards,
> Tomasz
> 
> > And you're right. Using "samsung,enable-bit-mask" makes more sense. I will change and update with v2.
> >  
> > Regards,
> > Prathyush
> >  
> > ------- Original Message -------
> > Sender : Tomasz Figa<tomasz.figa@gmail.com>
> > Date : Nov 11, 2013 00:36 (GMT+05:30)
> > Title : Re: [PATCH 1/1] ARM: EXYNOS: Add enable property to power domains
> >  
> > Hi Sachin,
> > 
> > On Thursday 07 of November 2013 12:12:55 Sachin Kamat wrote:
> > > From: Prathyush K
> > >
> > > Different power domains of Exynos SOCs have different enable values.
> > > E.g. Exynos5250:
> > > ROTATOR_MEM_CONFIGURATION -> 0x3
> > > GSCL_CONFIGURATION -> 0x7
> > > Currently, there is no way to differentiate between these power domains
> > > and we write default value of 0x7 to turn on all the power domains.
> > >
> > > This patch adds a new 'enable' property to the power domain structure.
> > > This enable value can be set from the device tree by adding a property
> > > 'enable' in the device node. If no such property is found, the default
> > > value of 0x7 is used as enable value.
> > 
> > Is this patch really needed? Is there any problem with simply using 0x7?
> > Patch description should always include rationale behind the change.
> > 
> > >
> > > Signed-off-by: Prathyush K
> > > Signed-off-by: Sachin Kamat
> > > ---
> > >  .../bindings/arm/exynos/power_domain.txt           |    5 +++++
> > >  arch/arm/mach-exynos/pm_domains.c                  |   10 +++++++---
> > >  2 files changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/arm/exynos/power_domain.txt b/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> > > index 5216b419016a..6b24b234617c 100644
> > > --- a/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> > > +++ b/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
> > > @@ -9,6 +9,10 @@ Required Properties:
> > >  - reg: physical base address of the controller and length of memory mapped
> > >      region.
> > >  
> > > +Optional Properties:
> > > +- enable: enable value of the register which is used to turn on the power
> > > +    domain. If no enable is specificed, default value of 0x7 is used.
> > 
> > Vendor-specific properties should have vendor prefix added, so this one
> > should be called samsung,enable instead. Also enable is not a very
> > specific name.
> > 
> > So in the end, if it turns out that we really need such patch, I'd prefer
> > something like:
> > 
> > - samsung,enable-bit-mask: Mask of power control register bits that need
> >     to be set to enable the power domain. If omitted, it defaults to 0x7.
> > 
> > Best regards,
> > Tomasz