[PATCH] pxa: Remove unused MFP LPM definition

[PATCH] pxa: Remove unused MFP LPM definition

[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.

Signed-off-by: David Hunter <hunterd42@gmail.com>
---
  arch/arm/plat-pxa/include/plat/mfp.h |    1 -
  1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-pxa/include/plat/mfp.h 
b/arch/arm/plat-pxa/include/plat/mfp.h
index 857a683..c5d24e9 100644
--- a/arch/arm/plat-pxa/include/plat/mfp.h
+++ b/arch/arm/plat-pxa/include/plat/mfp.h
@@ -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

[PATCH] pxa: Remove unused MFP LPM definition

[Eric Miao]

On Tue, Apr 27, 2010 at 10:46 AM, David Hunter <hunterd42@gmail.com> wrote:
> 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 <eric.y.miao@gmail.com>
Date:   Tue Apr 27 11:14:24 2010 +0800

    [ARM] pxa: allow MFP_LPM_INPUT to be explicitly specified

    Signed-off-by: Eric Miao <eric.y.miao@gmail.com>

diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c b/arch/arm/mach-pxa/mfp-pxa2xx.c
index e5b7921..1d1419b 100644
--- a/arch/arm/mach-pxa/mfp-pxa2xx.c
+++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
@@ -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:
diff --git a/arch/arm/plat-pxa/mfp.c b/arch/arm/plat-pxa/mfp.c
index be58f9f..b77e018 100644
--- a/arch/arm/plat-pxa/mfp.c
+++ b/arch/arm/plat-pxa/mfp.c
@@ -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 */

[PATCH] pxa: Remove unused MFP LPM definition

[David Hunter]

On 4/26/2010 8:16 PM, Eric Miao wrote:
> 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?

> commit f5d406d50f924c82ddf469f120fa420f0499d901
> Author: Eric Miao<eric.y.miao@gmail.com>
> Date:   Tue Apr 27 11:14:24 2010 +0800
>
>      [ARM] pxa: allow MFP_LPM_INPUT to be explicitly specified
>
>      Signed-off-by: Eric Miao<eric.y.miao@gmail.com>
>
> diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c b/arch/arm/mach-pxa/mfp-pxa2xx.c
> index e5b7921..1d1419b 100644
> --- a/arch/arm/mach-pxa/mfp-pxa2xx.c
> +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
> @@ -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]

> diff --git a/arch/arm/plat-pxa/mfp.c b/arch/arm/plat-pxa/mfp.c
> index be58f9f..b77e018 100644
> --- a/arch/arm/plat-pxa/mfp.c
> +++ b/arch/arm/plat-pxa/mfp.c
> @@ -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

[PATCH] pxa: Remove unused MFP LPM definition

[Eric Miao]

On Wed, May 5, 2010 at 6:18 AM, David Hunter <hunterd42@gmail.com> wrote:
> On 4/26/2010 8:16 PM, Eric Miao wrote:
>>
>> 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.

>
>> commit f5d406d50f924c82ddf469f120fa420f0499d901
>> Author: Eric Miao<eric.y.miao@gmail.com>
>> Date: ? Tue Apr 27 11:14:24 2010 +0800
>>
>> ? ? [ARM] pxa: allow MFP_LPM_INPUT to be explicitly specified
>>
>> ? ? Signed-off-by: Eric Miao<eric.y.miao@gmail.com>
>>
>> diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c
>> b/arch/arm/mach-pxa/mfp-pxa2xx.c
>> index e5b7921..1d1419b 100644
>> --- a/arch/arm/mach-pxa/mfp-pxa2xx.c
>> +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
>> @@ -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:
>

Good catch.

> + ? ? ? 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.

> [http://www.spinics.net/lists/arm-kernel/msg55842.html]
>
>> diff --git a/arch/arm/plat-pxa/mfp.c b/arch/arm/plat-pxa/mfp.c
>> index be58f9f..b77e018 100644
>> --- a/arch/arm/plat-pxa/mfp.c
>> +++ b/arch/arm/plat-pxa/mfp.c
>> @@ -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.
>

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.

> 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)

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

[PATCH] pxa: Remove unused MFP LPM definition

[Marek Vasut]

Dne St 5. kv?tna 2010 02:23:27 Eric Miao napsal(a):
> On Wed, May 5, 2010 at 6:18 AM, David Hunter <hunterd42@gmail.com> wrote:
> > On 4/26/2010 8:16 PM, Eric Miao wrote:
> >> 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.
> 
> >> commit f5d406d50f924c82ddf469f120fa420f0499d901
> >> Author: Eric Miao<eric.y.miao@gmail.com>
> >> Date:   Tue Apr 27 11:14:24 2010 +0800
> >> 
> >>     [ARM] pxa: allow MFP_LPM_INPUT to be explicitly specified
> >> 
> >>     Signed-off-by: Eric Miao<eric.y.miao@gmail.com>
> >> 
> >> diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c
> >> b/arch/arm/mach-pxa/mfp-pxa2xx.c
> >> index e5b7921..1d1419b 100644
> >> --- a/arch/arm/mach-pxa/mfp-pxa2xx.c
> >> +++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
> >> @@ -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:
> Good catch.
> 
> > +       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.
> 
> > [http://www.spinics.net/lists/arm-kernel/msg55842.html]
> > 
> >> diff --git a/arch/arm/plat-pxa/mfp.c b/arch/arm/plat-pxa/mfp.c
> >> index be58f9f..b77e018 100644
> >> --- a/arch/arm/plat-pxa/mfp.c
> >> +++ b/arch/arm/plat-pxa/mfp.c
> >> @@ -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.
> 
> 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.
> 
> > 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 ?
> 
> > The difference between blob's idea
> > of bad block management vs. Linux's has me avoiding blob at all costs.
> > 
> > -Dave

[PATCH] pxa: Remove unused MFP LPM definition

[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.

Signed-off-by: David Hunter <hunterd42@gmail.com>
---
  arch/arm/plat-pxa/include/plat/mfp.h |    1 -
  1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-pxa/include/plat/mfp.h 
b/arch/arm/plat-pxa/include/plat/mfp.h
index 857a683..c5d24e9 100644
--- a/arch/arm/plat-pxa/include/plat/mfp.h
+++ b/arch/arm/plat-pxa/include/plat/mfp.h
@@ -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