[PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position

[PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position

[Thara Gopinath]

MPU power domain bank 0 bits are displayed in position of bank 1
in PWRSTS and PREPWRSTS registers. So read them from correct
position

Signed-off-by: Thara Gopinath <thara@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/powerdomain.c             |    6 ++++++
 arch/arm/mach-omap2/powerdomains34xx.h        |    1 +
 arch/arm/plat-omap/include/plat/powerdomain.h |    5 ++++-
 3 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index b6990e3..1af447e 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -985,6 +985,9 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 	if (pwrdm->banks < (bank + 1))
 		return -EEXIST;
 
+	if (pwrdm->flags & PWRDM_HAS_MPU_QUIRK)
+		bank = 1;
+
 	/*
 	 * The register bit names below may not correspond to the
 	 * actual names of the bits in each powerdomain's register,
@@ -1032,6 +1035,9 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 	if (pwrdm->banks < (bank + 1))
 		return -EEXIST;
 
+	if (pwrdm->flags & PWRDM_HAS_MPU_QUIRK)
+		bank = 1;
+
 	/*
 	 * The register bit names below may not correspond to the
 	 * actual names of the bits in each powerdomain's register,
diff --git a/arch/arm/mach-omap2/powerdomains34xx.h b/arch/arm/mach-omap2/powerdomains34xx.h
index fd09b08..588f7e0 100644
--- a/arch/arm/mach-omap2/powerdomains34xx.h
+++ b/arch/arm/mach-omap2/powerdomains34xx.h
@@ -190,6 +190,7 @@ static struct powerdomain mpu_34xx_pwrdm = {
 	.wkdep_srcs	  = mpu_34xx_wkdeps,
 	.pwrsts		  = PWRSTS_OFF_RET_ON,
 	.pwrsts_logic_ret = PWRSTS_OFF_RET,
+	.flags		  = PWRDM_HAS_MPU_QUIRK,
 	.banks		  = 1,
 	.pwrsts_mem_ret	  = {
 		[0] = PWRSTS_OFF_RET,
diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h b/arch/arm/plat-omap/include/plat/powerdomain.h
index 3d45ee1..ad070a6 100644
--- a/arch/arm/plat-omap/include/plat/powerdomain.h
+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
@@ -40,7 +40,10 @@
 
 /* Powerdomain flags */
 #define PWRDM_HAS_HDWR_SAR	(1 << 0) /* hardware save-and-restore support */
-
+#define PWRDM_HAS_MPU_QUIRK	(1 << 1) /* MPU pwr domain has MEM bank 0 bits
+					  * in MEM bank 1 position. This is
+					  * true for OMAP3430
+					  */
 
 /*
  * Number of memory banks that are power-controllable.	On OMAP3430, the
-- 
1.5.4.7

Re: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position

[Paul Walmsley]

On Thu, 26 Nov 2009, Thara Gopinath wrote:

> MPU power domain bank 0 bits are displayed in position of bank 1
> in PWRSTS and PREPWRSTS registers. So read them from correct
> position
> 
> Signed-off-by: Thara Gopinath <thara@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>

Thanks Thara, will queue this up.

- Paul

Re: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> On Thu, 26 Nov 2009, Thara Gopinath wrote:
>
>> MPU power domain bank 0 bits are displayed in position of bank 1
>> in PWRSTS and PREPWRSTS registers. So read them from correct
>> position
>> 
>> Signed-off-by: Thara Gopinath <thara@ti.com>
>> Cc: Kevin Hilman <khilman@deeprootsystems.com>

Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>

> Thanks Thara, will queue this up.

Kevin

RE: [PATCH] OMAP3: PM: Fix for MPU power domain MEM BANK position

[Aguirre Rodriguez, Sergio Alberto]

Thara,

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Thara Gopinath
> Sent: Thursday, August 27, 2009 11:48 AM
> To: linux-omap@vger.kernel.org
> Cc: Gopinath, Thara
> Subject: [PATCH] OMAP3: PM: Fix for MPU power domain MEM BANK position
> 
> MPU power domain bank 0 bits are displayed in position of bank 1
> in PWRSTS and PREPWRSTS registers. So read them from correct
> position
> 
> Signed-off-by: Thara Gopinath <thara@ti.com>
> ---
>  arch/arm/mach-omap2/powerdomain.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-
> omap2/powerdomain.c
> index 2594cbf..6c5fee9 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -971,6 +971,16 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm,
> u8 bank)
>  		return -EEXIST;
> 
>  	/*
> +	 * In 3430, for MPU domain bank 0 status bits
> +	 * are displayed in the position of bank1 status bits
> +	 * in PWST  . So the hack. Think of a cleaner
> +	 * way of doing this
> +	 */
> +	if (cpu_is_omap34xx)

AFAIK, above should be:

	if (cpu_is_omap34xx())

Instead, which is how it's being used everywhere else in the kernel.

> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
> +			bank = 1;
> +
> +	/*
>  	 * The register bit names below may not correspond to the
>  	 * actual names of the bits in each powerdomain's register,
>  	 * but the type of value returned is the same for each
> @@ -1018,6 +1028,15 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain
> *pwrdm, u8 bank)
>  		return -EEXIST;
> 
>  	/*
> +	 * In 3430, for MPU domain bank 0 status bits
> +	 * are displayed in the position of bank1 status bits
> +	 * in PREPWST  . So the hack. Think of a cleaner
> +	 * way of doing this
> +	 */
> +	if (cpu_is_omap34xx)

Same here.

Regards,
Sergio
> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
> +			bank = 1;
> +	/*
>  	 * The register bit names below may not correspond to the
>  	 * actual names of the bits in each powerdomain's register,
>  	 * but the type of value returned is the same for each
> --
> 1.5.4.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position

[Thara Gopinath]

MPU power domain bank 0 bits are displayed in position of bank 1
in PWRSTS and PREPWRSTS registers. So read them from correct
position

Signed-off-by: Thara Gopinath <thara@ti.com>
---
Patch refresh issue.

 arch/arm/mach-omap2/powerdomain.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 2594cbf..6c5fee9 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -971,6 +971,16 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 		return -EEXIST;
 
 	/*
+	 * In 3430, for MPU domain bank 0 status bits
+	 * are displayed in the position of bank1 status bits
+	 * in PWST  . So the hack. Think of a cleaner
+	 * way of doing this
+	 */
+	if (cpu_is_omap34xx())
+		if (!strcmp("mpu_pwrdm", pwrdm->name))
+			bank = 1;
+
+	/*
 	 * The register bit names below may not correspond to the
 	 * actual names of the bits in each powerdomain's register,
 	 * but the type of value returned is the same for each
@@ -1018,6 +1028,15 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 		return -EEXIST;
 
 	/*
+	 * In 3430, for MPU domain bank 0 status bits
+	 * are displayed in the position of bank1 status bits
+	 * in PREPWST  . So the hack. Think of a cleaner
+	 * way of doing this
+	 */
+	if (cpu_is_omap34xx())
+		if (!strcmp("mpu_pwrdm", pwrdm->name))
+			bank = 1;
+	/*
 	 * The register bit names below may not correspond to the
 	 * actual names of the bits in each powerdomain's register,
 	 * but the type of value returned is the same for each
-- 
1.5.4.7

Re: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position

[Kevin Hilman]

Thara Gopinath <thara@ti.com> writes:

> MPU power domain bank 0 bits are displayed in position of bank 1
> in PWRSTS and PREPWRSTS registers. So read them from correct
> position
>
> Signed-off-by: Thara Gopinath <thara@ti.com>
> ---
> Patch refresh issue.
>
>  arch/arm/mach-omap2/powerdomain.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index 2594cbf..6c5fee9 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -971,6 +971,16 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>  		return -EEXIST;
>  
>  	/*
> +	 * In 3430, for MPU domain bank 0 status bits
> +	 * are displayed in the position of bank1 status bits
> +	 * in PWST  . So the hack. Think of a cleaner
> +	 * way of doing this
> +	 */
> +	if (cpu_is_omap34xx())
> +		if (!strcmp("mpu_pwrdm", pwrdm->name))

Rather than a string compare, you chould check

               if (pwrdm->prcm_offs == MPU_MOD)

                     
> +			bank = 1;
> +
> +	/*
>  	 * The register bit names below may not correspond to the
>  	 * actual names of the bits in each powerdomain's register,
>  	 * but the type of value returned is the same for each

This comment should also be changed because based on this patch, it
doesn't seem to be true.

Paul, are you OK with Thara's proposed change?

Otherwise, seems like the right fix is to have the shift value for
each bank encoded into the struct powerdomain.  Looks like the would
all be identical except MPU.

The patch below is a proposal/hack for how this could look, but it was
only done for the MPU powerdomain.  The others would need to be
completed.

Kevin


commit 7b4c705dc524b7c002284b2bf644ffc664d30b04
Author: Kevin Hilman <khilman@deeprootsystems.com>
Date:   Tue Sep 15 09:16:14 2009 -0700

    OMAP3: powerdomain: encode per-bank shift values for memory state
    
    The current and previous state of a powerdomains memory state are
    encoded in the PM_PWSTST and PM_PREPWSTST registers.  For most
    powerdomains, the shift values are the same for each bank, but for
    some they may be different.
    
    This patch adds the per-bank shift values for each powerdomain to the
    struct powerdomain so the code to read the values can be the same
    across all powerdomains.

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 2594cbf..5181adc 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -970,29 +970,8 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 	if (pwrdm->banks < (bank + 1))
 		return -EEXIST;
 
-	/*
-	 * The register bit names below may not correspond to the
-	 * actual names of the bits in each powerdomain's register,
-	 * but the type of value returned is the same for each
-	 * powerdomain.
-	 */
-	switch (bank) {
-	case 0:
-		m = OMAP3430_SHAREDL1CACHEFLATSTATEST_MASK;
-		break;
-	case 1:
-		m = OMAP3430_L1FLATMEMSTATEST_MASK;
-		break;
-	case 2:
-		m = OMAP3430_SHAREDL2CACHEFLATSTATEST_MASK;
-		break;
-	case 3:
-		m = OMAP3430_L2FLATMEMSTATEST_MASK;
-		break;
-	default:
-		WARN_ON(1); /* should never happen */
-		return -EEXIST;
-	}
+	m = pwrdm->pwrsts_mem_shift[bank];
+	WARN_ON(!m);
 
 	return prm_read_mod_bits_shift(pwrdm->prcm_offs, PM_PWSTST, m);
 }
@@ -1017,29 +996,8 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 	if (pwrdm->banks < (bank + 1))
 		return -EEXIST;
 
-	/*
-	 * The register bit names below may not correspond to the
-	 * actual names of the bits in each powerdomain's register,
-	 * but the type of value returned is the same for each
-	 * powerdomain.
-	 */
-	switch (bank) {
-	case 0:
-		m = OMAP3430_LASTMEM1STATEENTERED_MASK;
-		break;
-	case 1:
-		m = OMAP3430_LASTMEM2STATEENTERED_MASK;
-		break;
-	case 2:
-		m = OMAP3430_LASTSHAREDL2CACHEFLATSTATEENTERED_MASK;
-		break;
-	case 3:
-		m = OMAP3430_LASTL2FLATMEMSTATEENTERED_MASK;
-		break;
-	default:
-		WARN_ON(1); /* should never happen */
-		return -EEXIST;
-	}
+	m = pwrdm->pwrsts_mem_shift[bank];
+	WARN_ON(!m);
 
 	return prm_read_mod_bits_shift(pwrdm->prcm_offs,
 					OMAP3430_PM_PREPWSTST, m);
diff --git a/arch/arm/mach-omap2/powerdomains34xx.h b/arch/arm/mach-omap2/powerdomains34xx.h
index aa557b2..960cc84 100644
--- a/arch/arm/mach-omap2/powerdomains34xx.h
+++ b/arch/arm/mach-omap2/powerdomains34xx.h
@@ -191,6 +191,9 @@ static struct powerdomain mpu_34xx_pwrdm = {
 	.pwrsts		  = PWRSTS_OFF_RET_ON,
 	.pwrsts_logic_ret = PWRSTS_OFF_RET,
 	.banks		  = 1,
+	.pwrsts_mem_shift = {
+		[0] = OMAP3430_L2CACHESTATEST_SHIFT,
+	},
 	.pwrsts_mem_ret	  = {
 		[0] = PWRSTS_OFF_RET,
 	},
diff --git a/arch/arm/plat-omap/include/mach/powerdomain.h b/arch/arm/plat-omap/include/mach/powerdomain.h
index 6271d85..8a0c41d 100644
--- a/arch/arm/plat-omap/include/mach/powerdomain.h
+++ b/arch/arm/plat-omap/include/mach/powerdomain.h
@@ -106,6 +106,9 @@ struct powerdomain {
 	/* Number of software-controllable memory banks in this powerdomain */
 	const u8 banks;
 
+	/* Memory state shift value for each bank */
+	const u8 pwrsts_mem_shift[PWRDM_MAX_MEM_BANKS];
+
 	/* Possible memory bank pwrstates when pwrdm in RETENTION */
 	const u8 pwrsts_mem_ret[PWRDM_MAX_MEM_BANKS];
 

Re: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position

[Paul Walmsley]

Hi Thara,

I regret the delay.  A comment:

On Fri, 28 Aug 2009, Thara Gopinath wrote:

> MPU power domain bank 0 bits are displayed in position of bank 1
> in PWRSTS and PREPWRSTS registers. So read them from correct
> position

Indeed.  What do you think about a slightly different approach: changing 
powerdomains34xx.h to be correct?  In other words, instead of

	.pwrsts_mem_ret	  = {
		[0] = PWRSTS_OFF_RET,
	},
	.pwrsts_mem_on	  = {
		[0] = PWRSTS_OFF_ON,
	},

we would use:

	.pwrsts_mem_ret	  = {
		[1] = PWRSTS_OFF_RET,
	},
	.pwrsts_mem_on	  = {
		[1] = PWRSTS_OFF_ON,
	},

We have to deal with the bank count field in struct powerdomain - we could 
just convert it into a bitmap representing available banks.  So instead 
of:

        .banks = 1,

use maybe:

	.banks = PWRDM_BANK_1,  /* | PWRDM_BANK_0, etc */


Can you foresee any problems with the above approach?

- Paul

> Patch refresh issue.
> 
>  arch/arm/mach-omap2/powerdomain.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index 2594cbf..6c5fee9 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -971,6 +971,16 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>  		return -EEXIST;
>  
>  	/*
> +	 * In 3430, for MPU domain bank 0 status bits
> +	 * are displayed in the position of bank1 status bits
> +	 * in PWST  . So the hack. Think of a cleaner
> +	 * way of doing this
> +	 */
> +	if (cpu_is_omap34xx())
> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
> +			bank = 1;
> +
> +	/*
>  	 * The register bit names below may not correspond to the
>  	 * actual names of the bits in each powerdomain's register,
>  	 * but the type of value returned is the same for each
> @@ -1018,6 +1028,15 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>  		return -EEXIST;
>  
>  	/*
> +	 * In 3430, for MPU domain bank 0 status bits
> +	 * are displayed in the position of bank1 status bits
> +	 * in PREPWST  . So the hack. Think of a cleaner
> +	 * way of doing this
> +	 */
> +	if (cpu_is_omap34xx())
> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
> +			bank = 1;
> +	/*
>  	 * The register bit names below may not correspond to the
>  	 * actual names of the bits in each powerdomain's register,
>  	 * but the type of value returned is the same for each
> -- 
> 1.5.4.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


- Paul

RE: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position

[Gopinath, Thara]

Thanks Paul. The bit positions are goofed up only in PWRSTS and PREPWRSTS not in PWSTCTRL register. As per the approach you have suggested below, if we change the mem bank number from 0 to 1 , we will have to change the logic for read/write into PWSTCTRL for mpu pwr domain.

Regards
Thara

>>-----Original Message-----
>>From: Paul Walmsley [mailto:paul@pwsan.com]
>>Sent: Thursday, October 15, 2009 4:51 AM
>>To: Gopinath, Thara
>>Cc: linux-omap@vger.kernel.org; khilman@deeprootsystems.com
>>Subject: Re: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position
>>
>>Hi Thara,
>>
>>I regret the delay.  A comment:
>>
>>On Fri, 28 Aug 2009, Thara Gopinath wrote:
>>
>>> MPU power domain bank 0 bits are displayed in position of bank 1
>>> in PWRSTS and PREPWRSTS registers. So read them from correct
>>> position
>>
>>Indeed.  What do you think about a slightly different approach: changing
>>powerdomains34xx.h to be correct?  In other words, instead of
>>
>>	.pwrsts_mem_ret	  = {
>>		[0] = PWRSTS_OFF_RET,
>>	},
>>	.pwrsts_mem_on	  = {
>>		[0] = PWRSTS_OFF_ON,
>>	},
>>
>>we would use:
>>
>>	.pwrsts_mem_ret	  = {
>>		[1] = PWRSTS_OFF_RET,
>>	},
>>	.pwrsts_mem_on	  = {
>>		[1] = PWRSTS_OFF_ON,
>>	},
>>
>>We have to deal with the bank count field in struct powerdomain - we could
>>just convert it into a bitmap representing available banks.  So instead
>>of:
>>
>>        .banks = 1,
>>
>>use maybe:
>>
>>	.banks = PWRDM_BANK_1,  /* | PWRDM_BANK_0, etc */
>>
>>
>>Can you foresee any problems with the above approach?
>>
>>- Paul
>>
>>> Patch refresh issue.
>>>
>>>  arch/arm/mach-omap2/powerdomain.c |   19 +++++++++++++++++++
>>>  1 files changed, 19 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
>>> index 2594cbf..6c5fee9 100644
>>> --- a/arch/arm/mach-omap2/powerdomain.c
>>> +++ b/arch/arm/mach-omap2/powerdomain.c
>>> @@ -971,6 +971,16 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>>>  		return -EEXIST;
>>>
>>>  	/*
>>> +	 * In 3430, for MPU domain bank 0 status bits
>>> +	 * are displayed in the position of bank1 status bits
>>> +	 * in PWST  . So the hack. Think of a cleaner
>>> +	 * way of doing this
>>> +	 */
>>> +	if (cpu_is_omap34xx())
>>> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
>>> +			bank = 1;
>>> +
>>> +	/*
>>>  	 * The register bit names below may not correspond to the
>>>  	 * actual names of the bits in each powerdomain's register,
>>>  	 * but the type of value returned is the same for each
>>> @@ -1018,6 +1028,15 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>>>  		return -EEXIST;
>>>
>>>  	/*
>>> +	 * In 3430, for MPU domain bank 0 status bits
>>> +	 * are displayed in the position of bank1 status bits
>>> +	 * in PREPWST  . So the hack. Think of a cleaner
>>> +	 * way of doing this
>>> +	 */
>>> +	if (cpu_is_omap34xx())
>>> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
>>> +			bank = 1;
>>> +	/*
>>>  	 * The register bit names below may not correspond to the
>>>  	 * actual names of the bits in each powerdomain's register,
>>>  	 * but the type of value returned is the same for each
>>> --
>>> 1.5.4.7
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>>- Paul

RE: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position

[Paul Walmsley]

Hi Thara,

On Thu, 15 Oct 2009, Gopinath, Thara wrote:

> Thanks Paul. The bit positions are goofed up only in PWRSTS and 
> PREPWRSTS not in PWSTCTRL register. As per the approach you have 
> suggested below, if we change the mem bank number from 0 to 1 , we will 
> have to change the logic for read/write into PWSTCTRL for mpu pwr 
> domain.

Okay.  Here's what I'd propose: rather than testing for the 
powerdomain name in the function, or testing for the MPU PRCM module 
offset, let's add a new powerdomain flag into powerdomain.h,

#define PWRDM_OMAP3_MPU_QUIRK	 (1 << 1) /* MPU bit pos quirk */

or something similar.  Then let's set that for the mpu_pwrdm in 
powerdomains34xx.h and test for that flag in powerdomain.c.

I prefer this since RMK has previously NAK'ed strcmp()s for this sort of 
thing, for good reason, and testing the PRCM internal module offset has 
many of the same problems.

Sound reasonable?


- Paul
 

> 
> Regards
> Thara
> 
> >>-----Original Message-----
> >>From: Paul Walmsley [mailto:paul@pwsan.com]
> >>Sent: Thursday, October 15, 2009 4:51 AM
> >>To: Gopinath, Thara
> >>Cc: linux-omap@vger.kernel.org; khilman@deeprootsystems.com
> >>Subject: Re: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position
> >>
> >>Hi Thara,
> >>
> >>I regret the delay.  A comment:
> >>
> >>On Fri, 28 Aug 2009, Thara Gopinath wrote:
> >>
> >>> MPU power domain bank 0 bits are displayed in position of bank 1
> >>> in PWRSTS and PREPWRSTS registers. So read them from correct
> >>> position
> >>
> >>Indeed.  What do you think about a slightly different approach: changing
> >>powerdomains34xx.h to be correct?  In other words, instead of
> >>
> >>	.pwrsts_mem_ret	  = {
> >>		[0] = PWRSTS_OFF_RET,
> >>	},
> >>	.pwrsts_mem_on	  = {
> >>		[0] = PWRSTS_OFF_ON,
> >>	},
> >>
> >>we would use:
> >>
> >>	.pwrsts_mem_ret	  = {
> >>		[1] = PWRSTS_OFF_RET,
> >>	},
> >>	.pwrsts_mem_on	  = {
> >>		[1] = PWRSTS_OFF_ON,
> >>	},
> >>
> >>We have to deal with the bank count field in struct powerdomain - we could
> >>just convert it into a bitmap representing available banks.  So instead
> >>of:
> >>
> >>        .banks = 1,
> >>
> >>use maybe:
> >>
> >>	.banks = PWRDM_BANK_1,  /* | PWRDM_BANK_0, etc */
> >>
> >>
> >>Can you foresee any problems with the above approach?
> >>
> >>- Paul
> >>
> >>> Patch refresh issue.
> >>>
> >>>  arch/arm/mach-omap2/powerdomain.c |   19 +++++++++++++++++++
> >>>  1 files changed, 19 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> >>> index 2594cbf..6c5fee9 100644
> >>> --- a/arch/arm/mach-omap2/powerdomain.c
> >>> +++ b/arch/arm/mach-omap2/powerdomain.c
> >>> @@ -971,6 +971,16 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
> >>>  		return -EEXIST;
> >>>
> >>>  	/*
> >>> +	 * In 3430, for MPU domain bank 0 status bits
> >>> +	 * are displayed in the position of bank1 status bits
> >>> +	 * in PWST  . So the hack. Think of a cleaner
> >>> +	 * way of doing this
> >>> +	 */
> >>> +	if (cpu_is_omap34xx())
> >>> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
> >>> +			bank = 1;
> >>> +
> >>> +	/*
> >>>  	 * The register bit names below may not correspond to the
> >>>  	 * actual names of the bits in each powerdomain's register,
> >>>  	 * but the type of value returned is the same for each
> >>> @@ -1018,6 +1028,15 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
> >>>  		return -EEXIST;
> >>>
> >>>  	/*
> >>> +	 * In 3430, for MPU domain bank 0 status bits
> >>> +	 * are displayed in the position of bank1 status bits
> >>> +	 * in PREPWST  . So the hack. Think of a cleaner
> >>> +	 * way of doing this
> >>> +	 */
> >>> +	if (cpu_is_omap34xx())
> >>> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
> >>> +			bank = 1;
> >>> +	/*
> >>>  	 * The register bit names below may not correspond to the
> >>>  	 * actual names of the bits in each powerdomain's register,
> >>>  	 * but the type of value returned is the same for each
> >>> --
> >>> 1.5.4.7
> >>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>>
> >>
> >>
> >>- Paul
> 


- Paul

RE: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position

[Gopinath, Thara]

Yes sounds definitely reasonable :-). I agree,we should try to do away with the strcmp if possible.
I will implement the same and repost this fix.

Regards
Thara

>>-----Original Message-----
>>From: Paul Walmsley [mailto:paul@pwsan.com]
>>Sent: Friday, October 16, 2009 1:51 PM
>>To: Gopinath, Thara
>>Cc: linux-omap@vger.kernel.org; khilman@deeprootsystems.com
>>Subject: RE: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position
>>
>>Hi Thara,
>>
>>On Thu, 15 Oct 2009, Gopinath, Thara wrote:
>>
>>> Thanks Paul. The bit positions are goofed up only in PWRSTS and
>>> PREPWRSTS not in PWSTCTRL register. As per the approach you have
>>> suggested below, if we change the mem bank number from 0 to 1 , we will
>>> have to change the logic for read/write into PWSTCTRL for mpu pwr
>>> domain.
>>
>>Okay.  Here's what I'd propose: rather than testing for the
>>powerdomain name in the function, or testing for the MPU PRCM module
>>offset, let's add a new powerdomain flag into powerdomain.h,
>>
>>#define PWRDM_OMAP3_MPU_QUIRK	 (1 << 1) /* MPU bit pos quirk */
>>
>>or something similar.  Then let's set that for the mpu_pwrdm in
>>powerdomains34xx.h and test for that flag in powerdomain.c.
>>
>>I prefer this since RMK has previously NAK'ed strcmp()s for this sort of
>>thing, for good reason, and testing the PRCM internal module offset has
>>many of the same problems.
>>
>>Sound reasonable?
>>
>>
>>- Paul
>>
>>
>>>
>>> Regards
>>> Thara
>>>
>>> >>-----Original Message-----
>>> >>From: Paul Walmsley [mailto:paul@pwsan.com]
>>> >>Sent: Thursday, October 15, 2009 4:51 AM
>>> >>To: Gopinath, Thara
>>> >>Cc: linux-omap@vger.kernel.org; khilman@deeprootsystems.com
>>> >>Subject: Re: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position
>>> >>
>>> >>Hi Thara,
>>> >>
>>> >>I regret the delay.  A comment:
>>> >>
>>> >>On Fri, 28 Aug 2009, Thara Gopinath wrote:
>>> >>
>>> >>> MPU power domain bank 0 bits are displayed in position of bank 1
>>> >>> in PWRSTS and PREPWRSTS registers. So read them from correct
>>> >>> position
>>> >>
>>> >>Indeed.  What do you think about a slightly different approach: changing
>>> >>powerdomains34xx.h to be correct?  In other words, instead of
>>> >>
>>> >>	.pwrsts_mem_ret	  = {
>>> >>		[0] = PWRSTS_OFF_RET,
>>> >>	},
>>> >>	.pwrsts_mem_on	  = {
>>> >>		[0] = PWRSTS_OFF_ON,
>>> >>	},
>>> >>
>>> >>we would use:
>>> >>
>>> >>	.pwrsts_mem_ret	  = {
>>> >>		[1] = PWRSTS_OFF_RET,
>>> >>	},
>>> >>	.pwrsts_mem_on	  = {
>>> >>		[1] = PWRSTS_OFF_ON,
>>> >>	},
>>> >>
>>> >>We have to deal with the bank count field in struct powerdomain - we could
>>> >>just convert it into a bitmap representing available banks.  So instead
>>> >>of:
>>> >>
>>> >>        .banks = 1,
>>> >>
>>> >>use maybe:
>>> >>
>>> >>	.banks = PWRDM_BANK_1,  /* | PWRDM_BANK_0, etc */
>>> >>
>>> >>
>>> >>Can you foresee any problems with the above approach?
>>> >>
>>> >>- Paul
>>> >>
>>> >>> Patch refresh issue.
>>> >>>
>>> >>>  arch/arm/mach-omap2/powerdomain.c |   19 +++++++++++++++++++
>>> >>>  1 files changed, 19 insertions(+), 0 deletions(-)
>>> >>>
>>> >>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
>>> >>> index 2594cbf..6c5fee9 100644
>>> >>> --- a/arch/arm/mach-omap2/powerdomain.c
>>> >>> +++ b/arch/arm/mach-omap2/powerdomain.c
>>> >>> @@ -971,6 +971,16 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>>> >>>  		return -EEXIST;
>>> >>>
>>> >>>  	/*
>>> >>> +	 * In 3430, for MPU domain bank 0 status bits
>>> >>> +	 * are displayed in the position of bank1 status bits
>>> >>> +	 * in PWST  . So the hack. Think of a cleaner
>>> >>> +	 * way of doing this
>>> >>> +	 */
>>> >>> +	if (cpu_is_omap34xx())
>>> >>> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
>>> >>> +			bank = 1;
>>> >>> +
>>> >>> +	/*
>>> >>>  	 * The register bit names below may not correspond to the
>>> >>>  	 * actual names of the bits in each powerdomain's register,
>>> >>>  	 * but the type of value returned is the same for each
>>> >>> @@ -1018,6 +1028,15 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>>> >>>  		return -EEXIST;
>>> >>>
>>> >>>  	/*
>>> >>> +	 * In 3430, for MPU domain bank 0 status bits
>>> >>> +	 * are displayed in the position of bank1 status bits
>>> >>> +	 * in PREPWST  . So the hack. Think of a cleaner
>>> >>> +	 * way of doing this
>>> >>> +	 */
>>> >>> +	if (cpu_is_omap34xx())
>>> >>> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
>>> >>> +			bank = 1;
>>> >>> +	/*
>>> >>>  	 * The register bit names below may not correspond to the
>>> >>>  	 * actual names of the bits in each powerdomain's register,
>>> >>>  	 * but the type of value returned is the same for each
>>> >>> --
>>> >>> 1.5.4.7
>>> >>>
>>> >>> --
>>> >>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>>> >>> the body of a message to majordomo@vger.kernel.org
>>> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> >>>
>>> >>
>>> >>
>>> >>- Paul
>>>
>>
>>
>>- Paul

RE: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position

[Paul Walmsley]

Hello Thara,

On Fri, 16 Oct 2009, Gopinath, Thara wrote:

> Yes sounds definitely reasonable :-). I agree,we should try to do away with the strcmp if possible.
> I will implement the same and repost this fix.

Had a chance to work on this yet?  It would be good to get this fix in at 
some point.  
.

- Paul

> 
> Regards
> Thara
> 
> >>-----Original Message-----
> >>From: Paul Walmsley [mailto:paul@pwsan.com]
> >>Sent: Friday, October 16, 2009 1:51 PM
> >>To: Gopinath, Thara
> >>Cc: linux-omap@vger.kernel.org; khilman@deeprootsystems.com
> >>Subject: RE: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position
> >>
> >>Hi Thara,
> >>
> >>On Thu, 15 Oct 2009, Gopinath, Thara wrote:
> >>
> >>> Thanks Paul. The bit positions are goofed up only in PWRSTS and
> >>> PREPWRSTS not in PWSTCTRL register. As per the approach you have
> >>> suggested below, if we change the mem bank number from 0 to 1 , we will
> >>> have to change the logic for read/write into PWSTCTRL for mpu pwr
> >>> domain.
> >>
> >>Okay.  Here's what I'd propose: rather than testing for the
> >>powerdomain name in the function, or testing for the MPU PRCM module
> >>offset, let's add a new powerdomain flag into powerdomain.h,
> >>
> >>#define PWRDM_OMAP3_MPU_QUIRK	 (1 << 1) /* MPU bit pos quirk */
> >>
> >>or something similar.  Then let's set that for the mpu_pwrdm in
> >>powerdomains34xx.h and test for that flag in powerdomain.c.
> >>
> >>I prefer this since RMK has previously NAK'ed strcmp()s for this sort of
> >>thing, for good reason, and testing the PRCM internal module offset has
> >>many of the same problems.
> >>
> >>Sound reasonable?
> >>
> >>
> >>- Paul
> >>
> >>
> >>>
> >>> Regards
> >>> Thara
> >>>
> >>> >>-----Original Message-----
> >>> >>From: Paul Walmsley [mailto:paul@pwsan.com]
> >>> >>Sent: Thursday, October 15, 2009 4:51 AM
> >>> >>To: Gopinath, Thara
> >>> >>Cc: linux-omap@vger.kernel.org; khilman@deeprootsystems.com
> >>> >>Subject: Re: [PATCH V2] OMAP3: PM: Fix for MPU power domain MEM BANK position
> >>> >>
> >>> >>Hi Thara,
> >>> >>
> >>> >>I regret the delay.  A comment:
> >>> >>
> >>> >>On Fri, 28 Aug 2009, Thara Gopinath wrote:
> >>> >>
> >>> >>> MPU power domain bank 0 bits are displayed in position of bank 1
> >>> >>> in PWRSTS and PREPWRSTS registers. So read them from correct
> >>> >>> position
> >>> >>
> >>> >>Indeed.  What do you think about a slightly different approach: changing
> >>> >>powerdomains34xx.h to be correct?  In other words, instead of
> >>> >>
> >>> >>	.pwrsts_mem_ret	  = {
> >>> >>		[0] = PWRSTS_OFF_RET,
> >>> >>	},
> >>> >>	.pwrsts_mem_on	  = {
> >>> >>		[0] = PWRSTS_OFF_ON,
> >>> >>	},
> >>> >>
> >>> >>we would use:
> >>> >>
> >>> >>	.pwrsts_mem_ret	  = {
> >>> >>		[1] = PWRSTS_OFF_RET,
> >>> >>	},
> >>> >>	.pwrsts_mem_on	  = {
> >>> >>		[1] = PWRSTS_OFF_ON,
> >>> >>	},
> >>> >>
> >>> >>We have to deal with the bank count field in struct powerdomain - we could
> >>> >>just convert it into a bitmap representing available banks.  So instead
> >>> >>of:
> >>> >>
> >>> >>        .banks = 1,
> >>> >>
> >>> >>use maybe:
> >>> >>
> >>> >>	.banks = PWRDM_BANK_1,  /* | PWRDM_BANK_0, etc */
> >>> >>
> >>> >>
> >>> >>Can you foresee any problems with the above approach?
> >>> >>
> >>> >>- Paul
> >>> >>
> >>> >>> Patch refresh issue.
> >>> >>>
> >>> >>>  arch/arm/mach-omap2/powerdomain.c |   19 +++++++++++++++++++
> >>> >>>  1 files changed, 19 insertions(+), 0 deletions(-)
> >>> >>>
> >>> >>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> >>> >>> index 2594cbf..6c5fee9 100644
> >>> >>> --- a/arch/arm/mach-omap2/powerdomain.c
> >>> >>> +++ b/arch/arm/mach-omap2/powerdomain.c
> >>> >>> @@ -971,6 +971,16 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
> >>> >>>  		return -EEXIST;
> >>> >>>
> >>> >>>  	/*
> >>> >>> +	 * In 3430, for MPU domain bank 0 status bits
> >>> >>> +	 * are displayed in the position of bank1 status bits
> >>> >>> +	 * in PWST  . So the hack. Think of a cleaner
> >>> >>> +	 * way of doing this
> >>> >>> +	 */
> >>> >>> +	if (cpu_is_omap34xx())
> >>> >>> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
> >>> >>> +			bank = 1;
> >>> >>> +
> >>> >>> +	/*
> >>> >>>  	 * The register bit names below may not correspond to the
> >>> >>>  	 * actual names of the bits in each powerdomain's register,
> >>> >>>  	 * but the type of value returned is the same for each
> >>> >>> @@ -1018,6 +1028,15 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
> >>> >>>  		return -EEXIST;
> >>> >>>
> >>> >>>  	/*
> >>> >>> +	 * In 3430, for MPU domain bank 0 status bits
> >>> >>> +	 * are displayed in the position of bank1 status bits
> >>> >>> +	 * in PREPWST  . So the hack. Think of a cleaner
> >>> >>> +	 * way of doing this
> >>> >>> +	 */
> >>> >>> +	if (cpu_is_omap34xx())
> >>> >>> +		if (!strcmp("mpu_pwrdm", pwrdm->name))
> >>> >>> +			bank = 1;
> >>> >>> +	/*
> >>> >>>  	 * The register bit names below may not correspond to the
> >>> >>>  	 * actual names of the bits in each powerdomain's register,
> >>> >>>  	 * but the type of value returned is the same for each
> >>> >>> --
> >>> >>> 1.5.4.7
> >>> >>>
> >>> >>> --
> >>> >>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> >>> >>> the body of a message to majordomo@vger.kernel.org
> >>> >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>> >>>
> >>> >>
> >>> >>
> >>> >>- Paul
> >>>
> >>
> >>
> >>- Paul
> 


- Paul