pxa: Remove unused MFP LPM definition

PATCH New David Hunter 3 participants 5 messages Apr 27, 2010 – May 5, 2010

Subsystems (3)

Arm (32-bit) Arm sub-architectures PXA2xx/PXA3xx support

lore

Share & tools

lore

David Hunter

author

2010-04-27 02:46 UTC #0 lore raw

PATCH 1/1 pxa: Remove unused MFP LPM definition

MFP_LPM_INPUT snuck in some time ago as part of "[PATCH] pxa: better MFP low power state support for pxa25x/pxa27x", but went unused in the final revision of said patch. Using it on PXA3xx would cause an out-of-bounds access of mfpr_table[]. Let's remove it before someone gets hurt.

Signed-off-by David Hunter <[email protected]>

---
  arch/arm/plat-pxa/include/plat/mfp.h |    1 -
  1 files changed, 0 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

b/arch/arm/plat-pxa/include/plat/mfp.h
index 857a683..c5d24e9 100644

arch/arm/plat-pxa/include/plat/mfp.h +0 -1

@@ -378,7 +378,6 @@ typedef unsigned long mfp_cfg_t;
  #define MFP_LPM_PULL_LOW       (0x3 << 16)
  #define MFP_LPM_PULL_HIGH      (0x4 << 16)
  #define MFP_LPM_FLOAT          (0x5 << 16)
-#define MFP_LPM_INPUT          (0x6 << 16)
  #define MFP_LPM_STATE_MASK     (0x7 << 16)
  #define MFP_LPM_STATE(x)       (((x) >> 16) & 0x7)

--
1.6.0.3

Eric Miao

2010-04-27 03:16 UTC #1 lore raw

PATCH 1/1 pxa: Remove unused MFP LPM definition

Quoted 6 lines · David Hunter

MFP_LPM_INPUT snuck in some time ago as part of "[PATCH] pxa: better MFP low power state support for pxa25x/pxa27x", but went unused in the final revision of said patch. Using it on PXA3xx would cause an out-of-bounds access of mfpr_table[]. Let's remove it before someone gets hurt.

Instead of removing this potentially useful low power mode pin status, what about the following patch:

commit f5d406d50f924c82ddf469f120fa420f0499d901

Author: Eric Miao <[email protected]>
Date:   Tue Apr 27 11:14:24 2010 +0800

[ARM] pxa: allow MFP_LPM_INPUT to be explicitly specified

Signed-off-by Eric Miao <[email protected]>

arch/arm/mach-pxa/mfp-pxa2xx.c +1 -0

index e5b7921..1d1419b 100644
@@ -81,6 +81,7 @@ static int __mfp_config_gpio(unsigned gpio, unsigned long c)
 		PGSR(bank) &= ~mask;
 		is_out = 1;
 		break;
+	case MFP_LPM_INPUT:
 	case MFP_LPM_DEFAULT:
 		break;
 	default:

arch/arm/plat-pxa/mfp.c +1 -0

index be58f9f..b77e018 100644
@@ -110,6 +110,7 @@ static const unsigned long mfpr_lpm[] = {
 	MFPR_LPM_PULL_LOW,
 	MFPR_LPM_PULL_HIGH,
 	MFPR_LPM_FLOAT,
+	MFPR_LPM_INPUT,
 };
 
 /* mapping of MFP_PULL_* definitions to MFPR_PULL_* register bits */

David Hunter

author

2010-05-04 22:18 UTC #2 lore raw

PATCH 1/1 pxa: Remove unused MFP LPM definition

Quoted 2 lines · Eric Miao

Instead of removing this potentially useful low power mode pin status, what about the following patch:

I think the name itself is confusing. All of the other MFP_LPM_ definitions state what the pin is going to do: drive high/low, pull high/low, or float. My (strictly document-derived) understanding of PXA27x is that you can only choose to drive or float a pin, while with PXA3xx you can do all of the above. What would MFP_LPM_INPUT do that MFP_LPM_FLOAT wouldn't?

Quoted 13 lines

commit f5d406d50f924c82ddf469f120fa420f0499d901
Author: Eric Miao<[email protected]>
Date:   Tue Apr 27 11:14:24 2010 +0800
[ARM] pxa: allow MFP_LPM_INPUT to be explicitly specified
Signed-off-by Eric Miao<[email protected]>
arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0
index e5b7921..1d1419b 100644

arch/arm/mach-pxa/mfp-pxa2xx.c static int __mfp_config_gpio(unsigned gpio, unsigned long c) · line 86

Quoted 6 lines

arch/arm/mach-pxa/mfp-pxa2xx.c +1 -0

@@ -81,6 +81,7 @@ static int __mfp_config_gpio(unsigned gpio, unsigned long c)
  		PGSR(bank)&= ~mask;
  		is_out = 1;
  		break;
+	case MFP_LPM_INPUT:
  	case MFP_LPM_DEFAULT:
  		break;

If I read this correctly, _DEFAULT will, on entry to LPM, change the pin direction to the one implied by bit 23 (MFP_DIR_IN/MFP_DIR_OUT). For _INPUT to live up to its name, maybe it should always float the pin, like this:

+ case MFP_LPM_INPUT:
+ is_out = 0;

+		break;
	case MFP_LPM_DEFAULT:
		break;

Also, I'm not sure what effect GAFR will have (if any) in LPM. Does the AF have to be set to GPIO to enable the PGSR/GPDR settings? Your original patch did just that:

[http://www.spinics.net/lists/arm-kernel/msg55842.html]

arch/arm/plat-pxa/mfp.c static const unsigned long mfpr_lpm[] = { · line 116

Quoted 12 lines

arch/arm/plat-pxa/mfp.c +1 -0

index be58f9f..b77e018 100644
@@ -110,6 +110,7 @@ static const unsigned long mfpr_lpm[] = {
  	MFPR_LPM_PULL_LOW,
  	MFPR_LPM_PULL_HIGH,
  	MFPR_LPM_FLOAT,
+	MFPR_LPM_INPUT,
  };
 
  /* mapping of MFP_PULL_* definitions to MFPR_PULL_* register bits */

This adds a bug, which I've been trying to find the right way to fix. It's the same case as MFPR_LPM_DEFAULT. Since MFPR_LPM_INPUT = 0, MFPR[SLEEP_OE_N] will be cleared at LPM, and the pin will be driven low. (Not a good idea for a "default".) Either MFPR_LPM_FLOAT, _PULL_LOW, or _PULL_HIGH should be used instead. And only the board code knows enough to pick the right one for each pin.

I've been working to cobble together a patch to address this in my spare time, but don't have U-Boot running on Littleton yet. If you know of a way to get this going -- or something other way of loading the kernel -- I'd be grateful for the assist. The difference between blob's idea of bad block management vs. Linux's has me avoiding blob at all costs.

-Dave

Eric Miao

2010-05-05 00:23 UTC #3 lore raw

PATCH 1/1 pxa: Remove unused MFP LPM definition

Quoted 11 lines · David Hunter

Quoted 4 lines · Eric Miao
Instead of removing this potentially useful low power mode pin status, what
about the following patch:
I think the name itself is confusing. All of the other MFP_LPM_ definitions state what the pin is going to do: drive high/low, pull high/low, or float. My (strictly document-derived) understanding of PXA27x is that you can only choose to drive or float a pin, while with PXA3xx you can do all of the above. What would MFP_LPM_INPUT do that MFP_LPM_FLOAT wouldn't?

I'm currently not sure of how input is implemented in pxa27x during low power mode, but strictly, input != float. While float is usually a Hi-Z without pull-up or pull-down, input could be different. That's why I still intend to keep input there.

Quoted 18 lines

Quoted 18 lines
commit f5d406d50f924c82ddf469f120fa420f0499d901
Author: Eric Miao<[email protected]>
Date: ? Tue Apr 27 11:14:24 2010 +0800
? ? [ARM] pxa: allow MFP_LPM_INPUT to be explicitly specified
? ? Signed-off-by: Eric Miao<[email protected]>
arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0
index e5b7921..1d1419b 100644
@@ -81,6 +81,7 @@ static int __mfp_config_gpio(unsigned gpio, unsigned
 long c)
 ? ? ? ? ? ? ? ?PGSR(bank)&= ~mask;
 ? ? ? ? ? ? ? ?is_out = 1;

Quoted 9 lines

Quoted 4 lines
arch/arm/mach-pxa/mfp-pxa2xx.c +1 -0
 ? ? ? ? ? ? ? ?break;
+ ? ? ? case MFP_LPM_INPUT:
? ? ? ?case MFP_LPM_DEFAULT:
? ? ? ? ? ? ? ?break;
If I read this correctly, _DEFAULT will, on entry to LPM, change the pin direction to the one implied by bit 23 (MFP_DIR_IN/MFP_DIR_OUT). For _INPUT to live up to its name, maybe it should always float the pin, like this:

Good catch.

Quoted 10 lines

+ ? ? ? case MFP_LPM_INPUT:
+ ? ? ? ? ? ? ? is_out = 0;
+ ? ? ? ? ? ? ? break;
? ? ? ?case MFP_LPM_DEFAULT:
? ? ? ? ? ? ? ?break;
Also, I'm not sure what effect GAFR will have (if any) in LPM. Does the AF have to be set to GPIO to enable the PGSR/GPDR settings? Your original patch did just that:

My understanding is so. While PGSR will be loaded into GPSR or GPCR when entering low power mode, and GPSR/GPCR is one of the mux input, which I'd say making them to be GPIO will be safer to ensure the correct low power value.

Quoted 13 lines

[http://www.spinics.net/lists/arm-kernel/msg55842.html]

Quoted 11 lines

arch/arm/plat-pxa/mfp.c +1 -0

index be58f9f..b77e018 100644
@@ -110,6 +110,7 @@ static const unsigned long mfpr_lpm[] = {
 ? ? ? ?MFPR_LPM_PULL_LOW,
 ? ? ? ?MFPR_LPM_PULL_HIGH,
 ? ? ? ?MFPR_LPM_FLOAT,
+ ? ? ? MFPR_LPM_INPUT,
 ?};

Quoted 9 lines

Quoted 1 line
?/* mapping of MFP_PULL_* definitions to MFPR_PULL_* register bits */
This adds a bug, which I've been trying to find the right way to fix. It's the same case as MFPR_LPM_DEFAULT. Since MFPR_LPM_INPUT = 0, MFPR[SLEEP_OE_N] will be cleared at LPM, and the pin will be driven low. (Not a good idea for a "default".) Either MFPR_LPM_FLOAT, _PULL_LOW, or _PULL_HIGH should be used instead. And only the board code knows enough to pick the right one for each pin.

This looks odd to me as well, yet the setting is derived from some piece of code long time ago, which I have no way to track. But yes, feel free to submit patch for this. I'm yet not sure enough about which low power state is most power conserving, before that's figured out, I guess _FLOAT is a choice as it imposes minimum external effect at least.

Quoted 4 lines

I've been working to cobble together a patch to address this in my spare time, but don't have U-Boot running on Littleton yet. If you know of a way to get this going -- or something other way of loading the kernel -- I'd be grateful for the assist.

blob I'd say - but that's definitely not sexy. u-boot, on the other hand, you can ask Marek for detail, he's managed to get it running on littleton now. (I've got him CC'ed)

Quoted 5 lines

The difference between blob's idea
of bad block management vs. Linux's has me avoiding blob at all costs.
-Dave

Marek Vasut

2010-05-05 00:41 UTC #4 lore raw

PATCH 1/1 pxa: Remove unused MFP LPM definition

Dne St 5. kv?tna 2010 02:23:27 Eric Miao napsal(a):

Quoted 92 lines · David Hunter

Quoted 11 lines · Eric Miao
Quoted 4 lines
Instead of removing this potentially useful low power mode pin status, what
about the following patch:
I think the name itself is confusing. All of the other MFP_LPM_ definitions state what the pin is going to do: drive high/low, pull high/low, or float. My (strictly document-derived) understanding of PXA27x is that you can only choose to drive or float a pin, while with PXA3xx you can do all of the above. What would MFP_LPM_INPUT do that MFP_LPM_FLOAT wouldn't?
I'm currently not sure of how input is implemented in pxa27x during low power mode, but strictly, input != float. While float is usually a Hi-Z without pull-up or pull-down, input could be different. That's why I still intend to keep input there.
Quoted 26 lines
Quoted 14 lines
commit f5d406d50f924c82ddf469f120fa420f0499d901
Author: Eric Miao<[email protected]>
Date:   Tue Apr 27 11:14:24 2010 +0800
[ARM] pxa: allow MFP_LPM_INPUT to be explicitly specified
Signed-off-by Eric Miao<[email protected]>
arch/arm/mach-pxa/mfp-pxa2xx.c +0 -0
index e5b7921..1d1419b 100644
arch/arm/mach-pxa/mfp-pxa2xx.c static int __mfp_config_gpio(unsigned gpio, unsigned · line 87
Quoted 7 lines
arch/arm/mach-pxa/mfp-pxa2xx.c +1 -0
@@ -81,6 +81,7 @@ static int __mfp_config_gpio(unsigned gpio, unsigned
 long c)
               PGSR(bank)&= ~mask;
               is_out = 1;
               break;
+       case MFP_LPM_INPUT:
       case MFP_LPM_DEFAULT:
               break;
If I read this correctly, _DEFAULT will, on entry to LPM, change the pin direction to the one implied by bit 23 (MFP_DIR_IN/MFP_DIR_OUT). For _INPUT
Quoted 1 line
to live up to its name, maybe it should always float the pin, like this:
Good catch.
Quoted 11 lines
arch/arm/mach-pxa/mfp-pxa2xx.c +3 -0
+       case MFP_LPM_INPUT:
+               is_out = 0;
+               break;
       case MFP_LPM_DEFAULT:
               break;
Also, I'm not sure what effect GAFR will have (if any) in LPM. Does the AF have to be set to GPIO to enable the PGSR/GPDR settings? Your original patch
did just that:
My understanding is so. While PGSR will be loaded into GPSR or GPCR when entering low power mode, and GPSR/GPCR is one of the mux input, which I'd say making them to be GPIO will be safer to ensure the correct low power value.
Quoted 13 lines
[http://www.spinics.net/lists/arm-kernel/msg55842.html]
Quoted 11 lines
arch/arm/plat-pxa/mfp.c +1 -0
index be58f9f..b77e018 100644
@@ -110,6 +110,7 @@ static const unsigned long mfpr_lpm[] = {
       MFPR_LPM_PULL_LOW,
       MFPR_LPM_PULL_HIGH,
       MFPR_LPM_FLOAT,
+       MFPR_LPM_INPUT,
 };
 
Quoted 8 lines
arch/arm/plat-pxa/mfp.c static const unsigned long mfpr_lpm[] = { · line 116
Quoted 1 line
arch/arm/plat-pxa/mfp.c +0 -0
 /* mapping of MFP_PULL_* definitions to MFPR_PULL_* register bits */
This adds a bug, which I've been trying to find the right way to fix. It's the same case as MFPR_LPM_DEFAULT. Since MFPR_LPM_INPUT = 0, MFPR[SLEEP_OE_N] will be cleared at LPM, and the pin will be driven low. (Not a good idea for a "default".) Either MFPR_LPM_FLOAT, _PULL_LOW, or _PULL_HIGH should be used instead. And only the board code knows enough to pick the right one for each pin.
This looks odd to me as well, yet the setting is derived from some piece of code long time ago, which I have no way to track. But yes, feel free to submit patch for this. I'm yet not sure enough about which low power state is most power conserving, before that's figured out, I guess _FLOAT is a choice as it imposes minimum external effect at least.

Quoted 8 lines

Quoted 4 lines
I've been working to cobble together a patch to address this in my spare time, but don't have U-Boot running on Littleton yet. If you know of a way to get this going -- or something other way of loading the kernel -- I'd be grateful for the assist.
blob I'd say - but that's definitely not sexy. u-boot, on the other hand, you can ask Marek for detail, he's managed to get it running on littleton now. (I've got him CC'ed)

Hi, does the OBM2 give you problems still ? Or is it uboot that's buggy ?

Quoted 5 lines

Quoted 5 lines
The difference between blob's idea
of bad block management vs. Linux's has me avoiding blob at all costs.
-Dave