Message ID | 1384499789-3631-1-git-send-email-Gang.Liu@freescale.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
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
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); }
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(+)