[PATCH] powerpc/85xx: Workaround MPC8536 GPIO 1 errata.

[PATCH] powerpc/85xx: Workaround MPC8536 GPIO 1 errata.

[Felix Radensky]

On MPC8536 Rev 1.0 the status of GPIO pins configured
as output cannot be determined by reading GPDAT register.
Workaround by reading the status of input pins from GPDAT
and the status of output pins from a shadow register.

Signed-off-by: Felix Radensky <felix@embedded-sol.com>
---
 arch/powerpc/sysdev/mpc8xxx_gpio.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/sysdev/mpc8xxx_gpio.c b/arch/powerpc/sysdev/mpc8xxx_gpio.c
index 103eace..0b996f3 100644
--- a/arch/powerpc/sysdev/mpc8xxx_gpio.c
+++ b/arch/powerpc/sysdev/mpc8xxx_gpio.c
@@ -56,9 +56,13 @@ static void mpc8xxx_gpio_save_regs(struct of_mm_gpio_chip *mm)
 
 static int mpc8xxx_gpio_get(struct gpio_chip *gc, unsigned int gpio)
 {
+	u32 val;
 	struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc);
+	struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm);
+	
+	val = in_be32(mm->regs + GPIO_DAT) && ~in_be32(mm->regs + GPIO_DIR);
 
-	return in_be32(mm->regs + GPIO_DAT) & mpc8xxx_gpio2mask(gpio);
+	return (val | mpc8xxx_gc->data) & mpc8xxx_gpio2mask(gpio);
 }
 
 static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
-- 
1.5.4.3

Re: [PATCH] powerpc/85xx: Workaround MPC8536 GPIO 1 errata.

[Kumar Gala]

On Aug 11, 2009, at 4:04 AM, Felix Radensky wrote:

> On MPC8536 Rev 1.0 the status of GPIO pins configured
> as output cannot be determined by reading GPDAT register.
> Workaround by reading the status of input pins from GPDAT
> and the status of output pins from a shadow register.
>
> Signed-off-by: Felix Radensky <felix@embedded-sol.com>
> ---
> arch/powerpc/sysdev/mpc8xxx_gpio.c |    6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/mpc8xxx_gpio.c b/arch/powerpc/ 
> sysdev/mpc8xxx_gpio.c
> index 103eace..0b996f3 100644
> --- a/arch/powerpc/sysdev/mpc8xxx_gpio.c
> +++ b/arch/powerpc/sysdev/mpc8xxx_gpio.c
> @@ -56,9 +56,13 @@ static void mpc8xxx_gpio_save_regs(struct  
> of_mm_gpio_chip *mm)
>
> static int mpc8xxx_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> {
> +	u32 val;
> 	struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc);
> +	struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm);
> +	
> +	val = in_be32(mm->regs + GPIO_DAT) && ~in_be32(mm->regs + GPIO_DIR);

it would be good to add a comment in the code about working around the  
errata.

>
> -	return in_be32(mm->regs + GPIO_DAT) & mpc8xxx_gpio2mask(gpio);
> +	return (val | mpc8xxx_gc->data) & mpc8xxx_gpio2mask(gpio);
> }
>
> static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int  
> gpio, int val)
> -- 
> 1.5.4.3

- k

Re: [PATCH] powerpc/85xx: Workaround MPC8536 GPIO 1 errata.

[Anton Vorontsov]

On Tue, Aug 11, 2009 at 12:04:18PM +0300, Felix Radensky wrote:
> On MPC8536 Rev 1.0 the status of GPIO pins configured
> as output cannot be determined by reading GPDAT register.
> Workaround by reading the status of input pins from GPDAT
> and the status of output pins from a shadow register.
> 
> Signed-off-by: Felix Radensky <felix@embedded-sol.com>
> ---
>  arch/powerpc/sysdev/mpc8xxx_gpio.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/mpc8xxx_gpio.c b/arch/powerpc/sysdev/mpc8xxx_gpio.c
> index 103eace..0b996f3 100644
> --- a/arch/powerpc/sysdev/mpc8xxx_gpio.c
> +++ b/arch/powerpc/sysdev/mpc8xxx_gpio.c
> @@ -56,9 +56,13 @@ static void mpc8xxx_gpio_save_regs(struct of_mm_gpio_chip *mm)
>  
>  static int mpc8xxx_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>  {
> +	u32 val;
>  	struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc);
> +	struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm);
> +	
> +	val = in_be32(mm->regs + GPIO_DAT) && ~in_be32(mm->regs + GPIO_DIR);

Are you sure about &&?

Plus, this are two reads instead of just one. I think it'll be better
to implement mpc8536_gpio_get(), and then do

if (of_device_is_compatible(np, ... 8536-gpio-bank ...))
	gc->get = mpc8536_gpio_get;
else
	gc->get = mpc8xxx_gpio_get;

> -	return in_be32(mm->regs + GPIO_DAT) & mpc8xxx_gpio2mask(gpio);
> +	return (val | mpc8xxx_gc->data) & mpc8xxx_gpio2mask(gpio);
>  }
>  
>  static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] powerpc/85xx: Workaround MPC8536 GPIO 1 errata.

[Kumar Gala]

On Aug 11, 2009, at 8:44 AM, Anton Vorontsov wrote:

> On Tue, Aug 11, 2009 at 12:04:18PM +0300, Felix Radensky wrote:
>> On MPC8536 Rev 1.0 the status of GPIO pins configured
>> as output cannot be determined by reading GPDAT register.
>> Workaround by reading the status of input pins from GPDAT
>> and the status of output pins from a shadow register.
>>
>> Signed-off-by: Felix Radensky <felix@embedded-sol.com>
>> ---
>> arch/powerpc/sysdev/mpc8xxx_gpio.c |    6 +++++-
>> 1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/powerpc/sysdev/mpc8xxx_gpio.c b/arch/powerpc/ 
>> sysdev/mpc8xxx_gpio.c
>> index 103eace..0b996f3 100644
>> --- a/arch/powerpc/sysdev/mpc8xxx_gpio.c
>> +++ b/arch/powerpc/sysdev/mpc8xxx_gpio.c
>> @@ -56,9 +56,13 @@ static void mpc8xxx_gpio_save_regs(struct  
>> of_mm_gpio_chip *mm)
>>
>> static int mpc8xxx_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>> {
>> +	u32 val;
>> 	struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc);
>> +	struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm);
>> +	
>> +	val = in_be32(mm->regs + GPIO_DAT) && ~in_be32(mm->regs +  
>> GPIO_DIR);
>
> Are you sure about &&?
>
> Plus, this are two reads instead of just one. I think it'll be better
> to implement mpc8536_gpio_get(), and then do
>
> if (of_device_is_compatible(np, ... 8536-gpio-bank ...))
> 	gc->get = mpc8536_gpio_get;
> else
> 	gc->get = mpc8xxx_gpio_get;

I think 8572 has the same errata.

>
>> -	return in_be32(mm->regs + GPIO_DAT) & mpc8xxx_gpio2mask(gpio);
>> +	return (val | mpc8xxx_gc->data) & mpc8xxx_gpio2mask(gpio);
>> }
>>
>> static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int  
>> gpio, int val)
>
> Thanks,
>
> -- 
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc/85xx: Workaround MPC8536 GPIO 1 errata.

[Felix Radensky]

Anton Vorontsov wrote:
> On Tue, Aug 11, 2009 at 12:04:18PM +0300, Felix Radensky wrote:
>> On MPC8536 Rev 1.0 the status of GPIO pins configured
>> as output cannot be determined by reading GPDAT register.
>> Workaround by reading the status of input pins from GPDAT
>> and the status of output pins from a shadow register.
>>
>> Signed-off-by: Felix Radensky <felix@embedded-sol.com>
>> ---
>>  arch/powerpc/sysdev/mpc8xxx_gpio.c |    6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/powerpc/sysdev/mpc8xxx_gpio.c b/arch/powerpc/sysdev/mpc8xxx_gpio.c
>> index 103eace..0b996f3 100644
>> --- a/arch/powerpc/sysdev/mpc8xxx_gpio.c
>> +++ b/arch/powerpc/sysdev/mpc8xxx_gpio.c
>> @@ -56,9 +56,13 @@ static void mpc8xxx_gpio_save_regs(struct of_mm_gpio_chip *mm)
>>  
>>  static int mpc8xxx_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>>  {
>> +	u32 val;
>>  	struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc);
>> +	struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm);
>> +	
>> +	val = in_be32(mm->regs + GPIO_DAT) && ~in_be32(mm->regs + GPIO_DIR);
> 
> Are you sure about &&?


No, that's obviously a typo. Bitwise & intended.

> 
> Plus, this are two reads instead of just one. I think it'll be better
> to implement mpc8536_gpio_get(), and then do
> 
> if (of_device_is_compatible(np, ... 8536-gpio-bank ...))
> 	gc->get = mpc8536_gpio_get;
> else
> 	gc->get = mpc8xxx_gpio_get;

The reads are from 2 different registers, how do you propose to replace
them by a single read ?

I'll implement mpc8572_gpio_get(), suitable for both 8572 and 8536.


> 
>> -	return in_be32(mm->regs + GPIO_DAT) & mpc8xxx_gpio2mask(gpio);
>> +	return (val | mpc8xxx_gc->data) & mpc8xxx_gpio2mask(gpio);
>>  }
>>  
>>  static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> 
> Thanks,
> 

Re: [PATCH] powerpc/85xx: Workaround MPC8536 GPIO 1 errata.

[Anton Vorontsov]

On Tue, Aug 11, 2009 at 06:12:12PM +0300, Felix Radensky wrote:
[...]
> >Plus, this are two reads instead of just one. I think it'll be better
> >to implement mpc8536_gpio_get(), and then do
> >
> >if (of_device_is_compatible(np, ... 8536-gpio-bank ...))
> >	gc->get = mpc8536_gpio_get;
> >else
> >	gc->get = mpc8xxx_gpio_get;
> 
> The reads are from 2 different registers, how do you propose to replace
> them by a single read ?

I didn't propose that, I proposed to not do 2 reads for CPUs that
don't have the errata. ;-)

> I'll implement mpc8572_gpio_get(), suitable for both 8572 and 8536.

Great, thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2