powerpc/gpio: Fix the wrong GPIO input data on MPC8572/MPC8536

For MPC8572/MPC8536, the status of GPIOs defined as output
cannot be determined by reading GPDAT register, so the code
use shadow data register instead. But if the input pins are
asserted high, they will always read high due to the shadow
data, even if the pins are set to low.

So the input pins should be read directly from GPDAT, not
the shadow data.

Signed-off-by: Liu Gang <Gang.Liu@freescale.com>
---
 drivers/gpio/gpio-mpc8xxx.c | 1 +
 1 file changed, 1 insertion(+)

On Fri, Nov 15, 2013 at 8:16 AM, Liu Gang <Gang.Liu@freescale.com> wrote:

> For MPC8572/MPC8536, the status of GPIOs defined as output
> cannot be determined by reading GPDAT register, so the code
> use shadow data register instead. But if the input pins are
> asserted high, they will always read high due to the shadow
> data, even if the pins are set to low.
>
> So the input pins should be read directly from GPDAT, not
> the shadow data.
>
> Signed-off-by: Liu Gang <Gang.Liu@freescale.com>
> ---
>  drivers/gpio/gpio-mpc8xxx.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> index 9ae29cc..1d4ac75 100644
> --- a/drivers/gpio/gpio-mpc8xxx.c
> +++ b/drivers/gpio/gpio-mpc8xxx.c
> @@ -71,6 +71,7 @@ static int mpc8572_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>         struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm);
>
>         val = in_be32(mm->regs + GPIO_DAT) & ~in_be32(mm->regs + GPIO_DIR);
> +       mpc8xxx_gc->data &= in_be32(mm->regs + GPIO_DIR);
>
>         return (val | mpc8xxx_gc->data) & mpc8xxx_gpio2mask(gpio);
>  }

Anatolij, Ben: can either of you take a look at this patch and ACK it
if OK?

Yours,
Linus Walleij

On Fri, 15 Nov 2013 15:16:29 +0800
Liu Gang <Gang.Liu@freescale.com> wrote:

> For MPC8572/MPC8536, the status of GPIOs defined as output
> cannot be determined by reading GPDAT register, so the code
> use shadow data register instead. But if the input pins are
> asserted high, they will always read high due to the shadow
> data, even if the pins are set to low.

Could you please add a better description of the problem?
I'm having some difficulties to understand the last sentence
above. Does the issue appear if some pins were configured as
inputs and were asserted high before booting the kernel, and
therefore the shadow data has been initialized with these pin
values?

Or does the issue appear if some pin has been configured as output
first and has been set to the high value, then reconfigured as
input? Now reading the pin state will always return high even
if the actual pin state is low?

It seems the issue will appear in both cases. If so, please add
this information to the commit message.

> So the input pins should be read directly from GPDAT, not
> the shadow data.
> 
> Signed-off-by: Liu Gang <Gang.Liu@freescale.com>
> ---
>  drivers/gpio/gpio-mpc8xxx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> index 9ae29cc..1d4ac75 100644
> --- a/drivers/gpio/gpio-mpc8xxx.c
> +++ b/drivers/gpio/gpio-mpc8xxx.c
> @@ -71,6 +71,7 @@ static int mpc8572_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>  	struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm);
>  
>  	val = in_be32(mm->regs + GPIO_DAT) & ~in_be32(mm->regs + GPIO_DIR);
> +	mpc8xxx_gc->data &= in_be32(mm->regs + GPIO_DIR);

we can reduce one in_be32() call here, i.e.

	u32 out_mask;
	...
	out_mask = in_be32(mm->regs + GPIO_DIR);
	val = in_be32(mm->regs + GPIO_DAT) & ~out_mask;
	mpc8xxx_gc->data &= out_mask;

>  	return (val | mpc8xxx_gc->data) & mpc8xxx_gpio2mask(gpio);
>  }

Thanks,

Anatolij

--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0  Fax: +49-8142-66989-80 Email: office@denx.de

On Fri, 2013-11-15 at 15:16 +0800, Liu Gang wrote:
> For MPC8572/MPC8536, the status of GPIOs defined as output
> cannot be determined by reading GPDAT register, so the code
> use shadow data register instead. But if the input pins are
> asserted high, they will always read high due to the shadow
> data, even if the pins are set to low.
> 
> So the input pins should be read directly from GPDAT, not
> the shadow data.
> 
> Signed-off-by: Liu Gang <Gang.Liu@freescale.com>
> ---
>  drivers/gpio/gpio-mpc8xxx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
> index 9ae29cc..1d4ac75 100644
> --- a/drivers/gpio/gpio-mpc8xxx.c
> +++ b/drivers/gpio/gpio-mpc8xxx.c
> @@ -71,6 +71,7 @@ static int mpc8572_gpio_get(struct gpio_chip *gc, unsigned int gpio)
>  	struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm);
>  
>  	val = in_be32(mm->regs + GPIO_DAT) & ~in_be32(mm->regs + GPIO_DIR);
> +	mpc8xxx_gc->data &= in_be32(mm->regs + GPIO_DIR);
>  
>  	return (val | mpc8xxx_gc->data) & mpc8xxx_gpio2mask(gpio);
>  }

It seems odd to update ->data in a function that's supposed to be
reading things...  Perhaps it would be better to keep ->data in a good
state from the beginning.

-Scott

On Tue, 2013-11-19 at 16:32 +0100, Anatolij Gustschin wrote:
> On Fri, 15 Nov 2013 15:16:29 +0800
> Liu Gang <Gang.Liu@freescale.com> wrote:
> 
> > For MPC8572/MPC8536, the status of GPIOs defined as output
> > cannot be determined by reading GPDAT register, so the code
> > use shadow data register instead. But if the input pins are
> > asserted high, they will always read high due to the shadow
> > data, even if the pins are set to low.
> 
> Could you please add a better description of the problem?
> I'm having some difficulties to understand the last sentence
> above. Does the issue appear if some pins were configured as
> inputs and were asserted high before booting the kernel, and
> therefore the shadow data has been initialized with these pin
> values?
> 
> Or does the issue appear if some pin has been configured as output
> first and has been set to the high value, then reconfigured as
> input? Now reading the pin state will always return high even
> if the actual pin state is low?
> 
> It seems the issue will appear in both cases. If so, please add
> this information to the commit message.
> 
Yes, you are right.
I'll updated the description more clear.

> >  	val = in_be32(mm->regs + GPIO_DAT) & ~in_be32(mm->regs + GPIO_DIR);
> > +	mpc8xxx_gc->data &= in_be32(mm->regs + GPIO_DIR);
> 
> we can reduce one in_be32() call here, i.e.
> 
> 	u32 out_mask;
> 	...
> 	out_mask = in_be32(mm->regs + GPIO_DIR);
> 	val = in_be32(mm->regs + GPIO_DAT) & ~out_mask;
> 	mpc8xxx_gc->data &= out_mask;
> 
> >  	return (val | mpc8xxx_gc->data) & mpc8xxx_gpio2mask(gpio);
> >  }
> 
> Thanks,
> 
> Anatolij
> 
Granted, it will be better to reduce one in_be32() call.
I'll improve the method based on your and Scott's comments.

Thanks
Liu Gang

On Tue, 2013-11-19 at 16:51 -0600, Scott Wood wrote:
> > @@ -71,6 +71,7 @@ static int mpc8572_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> >  	struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm);
> >  
> >  	val = in_be32(mm->regs + GPIO_DAT) & ~in_be32(mm->regs + GPIO_DIR);
> > +	mpc8xxx_gc->data &= in_be32(mm->regs + GPIO_DIR);
> >  
> >  	return (val | mpc8xxx_gc->data) & mpc8xxx_gpio2mask(gpio);
> >  }
> 
> It seems odd to update ->data in a function that's supposed to be
> reading things...  Perhaps it would be better to keep ->data in a good
> state from the beginning.
> 
> -Scott

Yes, keeping the ->data in a good state from the beginning will be
better. But this will need more code in different functions to cover
all the scenarios.
First, we should check the direct of the pin in the function
"mpc8xxx_gpio_set", and clean the input bit in ->data after setting
operation.
In addition, we may change a output pin to input and then read the
input status. So we also should update the ->data in
"mpc8xxx_gpio_dir_in" function.
So maybe it's better to eliminate the effects of the ->data to the
input pins when reading the status, regardless of the possible changes
of the pins and the data.
Do you think so?

Best Regards,
Liu Gang

On Wed, 2013-11-20 at 10:54 +0800, Liu Gang wrote:
> On Tue, 2013-11-19 at 16:51 -0600, Scott Wood wrote:
> > > @@ -71,6 +71,7 @@ static int mpc8572_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> > >  	struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm);
> > >  
> > >  	val = in_be32(mm->regs + GPIO_DAT) & ~in_be32(mm->regs + GPIO_DIR);
> > > +	mpc8xxx_gc->data &= in_be32(mm->regs + GPIO_DIR);
> > >  
> > >  	return (val | mpc8xxx_gc->data) & mpc8xxx_gpio2mask(gpio);
> > >  }
> > 
> > It seems odd to update ->data in a function that's supposed to be
> > reading things...  Perhaps it would be better to keep ->data in a good
> > state from the beginning.
> > 
> > -Scott
> 
> Yes, keeping the ->data in a good state from the beginning will be
> better. But this will need more code in different functions to cover
> all the scenarios.
> First, we should check the direct of the pin in the function
> "mpc8xxx_gpio_set", and clean the input bit in ->data after setting
> operation.

For userspace value setting, it looks like gpiolib blocks the write if
the pin if FLAG_IS_OUT is set.  This suggests that this is an error
condition for other uses as well.  Though, I notice that
mpc8xxx_gpio_dir_out() calls gpio_set() before actually changing the
direction.  So it may be useful to avoid races where the wrong value is
output briefly after the direction is changed (especially in open drain
situations, where the signal could have a meaningful default even before
we begin outputting).  But that raises the question of how you'd do that
from userspace, and it also renders the to-be-output value as write-only
data (until the direction is actually changed), since a readback would
get the input value instead.

> So maybe it's better to eliminate the effects of the ->data to the
> input pins when reading the status, regardless of the possible changes
> of the pins and the data.
> Do you think so?

Perhaps, but that doesn't require you to modify ->data in the get()
function.

-Scott

On Wed, 2013-11-20 at 18:32 -0600, Scott Wood wrote:
> For userspace value setting, it looks like gpiolib blocks the write if
> the pin if FLAG_IS_OUT is set.  This suggests that this is an error
> condition for other uses as well.  Though, I notice that
> mpc8xxx_gpio_dir_out() calls gpio_set() before actually changing the
> direction.  So it may be useful to avoid races where the wrong value is
> output briefly after the direction is changed (especially in open drain
> situations, where the signal could have a meaningful default even before
> we begin outputting).  But that raises the question of how you'd do that
> from userspace, and it also renders the to-be-output value as write-only
> data (until the direction is actually changed), since a readback would
> get the input value instead.
> 
> > So maybe it's better to eliminate the effects of the ->data to the
> > input pins when reading the status, regardless of the possible changes
> > of the pins and the data.
> > Do you think so?
> 
> Perhaps, but that doesn't require you to modify ->data in the get()
> function.
> 
> -Scott
> 
I think you considered about this more comprehensive.
I'll update the code without the modification of ->data in the get()
function, and also with the comments from Anatolij.

Best Regards,
Liu Gang