| Message ID | 1249981458-16102-1-git-send-email-felix@embedded-sol.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kumar Gala |
| Headers | show |
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
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,
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
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, >
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!
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)
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(-)