[PATCH resend 0/2] gpio: mpc8xxx: honour shadow register when writing gpdat
[resending with Mario's correct address, sorry for the double post]
The driver correctly uses the shadow register when asked for the current value of an output gpio. Unfortunately, it does RMW on the gpdat register both when setting a gpio as input and output. These two patches fix that.
Aside: Apparently, the mpc8309 also partially suffers from the errata preventing outputs from being read back - the bits corresponding to gpios 0-7 are always read as 0, while at least the value of gpio10 is correctly reflected when reading gpdat. Which is how I noticed these bugs - I couldn't understand why turning one LED on would turn off another.
Rasmus Villemoes (2): gpio: mpc8xxx: don't modify gpdat when setting gpio as input gpio: mpc8xxx: don't do RMW on gpdat register when setting value
drivers/gpio/mpc8xxx_gpio.c | 41 ++++++++++++++----------------------- 1 file changed, 15 insertions(+), 26 deletions(-)
Since some chips don't support reading back the value of output gpios from the gpdat register, we should not do a RMW cycle (i.e., the clrbits_be32) on the gpdat register when setting a gpio as input, as that might accidentally change the value of some other (still configured as output) gpio.
The extra indirection through mpc8xxx_gpio_set_in() does not help readability, so just fold the gpdir update into mpc8xxx_gpio_direction_input().
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- drivers/gpio/mpc8xxx_gpio.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpio/mpc8xxx_gpio.c b/drivers/gpio/mpc8xxx_gpio.c index 9964d69035..5438196fe0 100644 --- a/drivers/gpio/mpc8xxx_gpio.c +++ b/drivers/gpio/mpc8xxx_gpio.c @@ -57,13 +57,6 @@ static inline u32 mpc8xxx_gpio_get_dir(struct ccsr_gpio *base, u32 mask) return in_be32(&base->gpdir) & mask; }
-static inline void mpc8xxx_gpio_set_in(struct ccsr_gpio *base, u32 gpios) -{ - clrbits_be32(&base->gpdat, gpios); - /* GPDIR register 0 -> input */ - clrbits_be32(&base->gpdir, gpios); -} - static inline void mpc8xxx_gpio_set_low(struct ccsr_gpio *base, u32 gpios) { clrbits_be32(&base->gpdat, gpios); @@ -100,8 +93,11 @@ static inline void mpc8xxx_gpio_open_drain_off(struct ccsr_gpio *base, static int mpc8xxx_gpio_direction_input(struct udevice *dev, uint gpio) { struct mpc8xxx_gpio_data *data = dev_get_priv(dev); + u32 mask = gpio_mask(gpio); + + /* GPDIR register 0 -> input */ + clrbits_be32(&data->base->gpdir, mask);
- mpc8xxx_gpio_set_in(data->base, gpio_mask(gpio)); return 0; }
On Tue, Jan 28, 2020 at 12:04:33PM +0000, Rasmus Villemoes wrote:
Since some chips don't support reading back the value of output gpios from the gpdat register, we should not do a RMW cycle (i.e., the clrbits_be32) on the gpdat register when setting a gpio as input, as that might accidentally change the value of some other (still configured as output) gpio.
The extra indirection through mpc8xxx_gpio_set_in() does not help readability, so just fold the gpdir update into mpc8xxx_gpio_direction_input().
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
Applied to u-boot/master, thanks!
The driver correctly handles reading back the value of an output gpio by reading from the shadow register for output, and from gpdat for inputs.
Unfortunately, when setting the value of some gpio, we do a RMW cycle on the gpdat register without taking the shadow register into account, thus accidentally setting other output gpios (at least those whose value cannot be read back) to 0 at the same time.
When changing a gpio from input to output, we still need to make sure it initially has the requested value. So, the procedure is
- update the shadow register - compute the new gpdir register - write the bitwise and of the shadow and new gpdir register to gpdat - write the new gpdir register
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk --- drivers/gpio/mpc8xxx_gpio.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/gpio/mpc8xxx_gpio.c b/drivers/gpio/mpc8xxx_gpio.c index 5438196fe0..c5be8e673b 100644 --- a/drivers/gpio/mpc8xxx_gpio.c +++ b/drivers/gpio/mpc8xxx_gpio.c @@ -57,20 +57,6 @@ static inline u32 mpc8xxx_gpio_get_dir(struct ccsr_gpio *base, u32 mask) return in_be32(&base->gpdir) & mask; }
-static inline void mpc8xxx_gpio_set_low(struct ccsr_gpio *base, u32 gpios) -{ - clrbits_be32(&base->gpdat, gpios); - /* GPDIR register 1 -> output */ - setbits_be32(&base->gpdir, gpios); -} - -static inline void mpc8xxx_gpio_set_high(struct ccsr_gpio *base, u32 gpios) -{ - setbits_be32(&base->gpdat, gpios); - /* GPDIR register 1 -> output */ - setbits_be32(&base->gpdir, gpios); -} - static inline int mpc8xxx_gpio_open_drain_val(struct ccsr_gpio *base, u32 mask) { return in_be32(&base->gpodr) & mask; @@ -104,14 +90,21 @@ static int mpc8xxx_gpio_direction_input(struct udevice *dev, uint gpio) static int mpc8xxx_gpio_set_value(struct udevice *dev, uint gpio, int value) { struct mpc8xxx_gpio_data *data = dev_get_priv(dev); + struct ccsr_gpio *base = data->base; + u32 mask = gpio_mask(gpio); + u32 gpdir;
if (value) { - data->dat_shadow |= gpio_mask(gpio); - mpc8xxx_gpio_set_high(data->base, gpio_mask(gpio)); + data->dat_shadow |= mask; } else { - data->dat_shadow &= ~gpio_mask(gpio); - mpc8xxx_gpio_set_low(data->base, gpio_mask(gpio)); + data->dat_shadow &= ~mask; } + + gpdir = in_be32(&base->gpdir); + gpdir |= gpio_mask(gpio); + out_be32(&base->gpdat, gpdir & data->dat_shadow); + out_be32(&base->gpdir, gpdir); + return 0; }
On Tue, Jan 28, 2020 at 12:04:34PM +0000, Rasmus Villemoes wrote:
The driver correctly handles reading back the value of an output gpio by reading from the shadow register for output, and from gpdat for inputs.
Unfortunately, when setting the value of some gpio, we do a RMW cycle on the gpdat register without taking the shadow register into account, thus accidentally setting other output gpios (at least those whose value cannot be read back) to 0 at the same time.
When changing a gpio from input to output, we still need to make sure it initially has the requested value. So, the procedure is
- update the shadow register
- compute the new gpdir register
- write the bitwise and of the shadow and new gpdir register to gpdat
- write the new gpdir register
Signed-off-by: Rasmus Villemoes rasmus.villemoes@prevas.dk
Applied to u-boot/master, thanks!
On 28/01/2020 13.04, Rasmus Villemoes wrote:
Rasmus Villemoes (2): gpio: mpc8xxx: don't modify gpdat when setting gpio as input gpio: mpc8xxx: don't do RMW on gpdat register when setting value
drivers/gpio/mpc8xxx_gpio.c | 41 ++++++++++++++----------------------- 1 file changed, 15 insertions(+), 26 deletions(-)
ping
On 28/01/2020 13.04, Rasmus Villemoes wrote:
[resending with Mario's correct address, sorry for the double post]
The driver correctly uses the shadow register when asked for the current value of an output gpio. Unfortunately, it does RMW on the gpdat register both when setting a gpio as input and output. These two patches fix that.
Ping. Any chance these fixes can make it to 2020.04 ?
Rasmus
On 03/03/2020 13.19, Rasmus Villemoes wrote:
On 28/01/2020 13.04, Rasmus Villemoes wrote:
[resending with Mario's correct address, sorry for the double post]
The driver correctly uses the shadow register when asked for the current value of an output gpio. Unfortunately, it does RMW on the gpdat register both when setting a gpio as input and output. These two patches fix that.
Ping. Any chance these fixes can make it to 2020.04 ?
Ping^3.
Tom, will you consider applying these directly? It's been two months and I haven't got any response. Or advise on what I could do.
Thanks, Rasmus
On Mon, Mar 23, 2020 at 01:13:01AM +0100, Rasmus Villemoes wrote:
On 03/03/2020 13.19, Rasmus Villemoes wrote:
On 28/01/2020 13.04, Rasmus Villemoes wrote:
[resending with Mario's correct address, sorry for the double post]
The driver correctly uses the shadow register when asked for the current value of an output gpio. Unfortunately, it does RMW on the gpdat register both when setting a gpio as input and output. These two patches fix that.
Ping. Any chance these fixes can make it to 2020.04 ?
Ping^3.
Tom, will you consider applying these directly? It's been two months and I haven't got any response. Or advise on what I could do.
I suppose that if Mario doesn't speak up soon I'll take a look at picking them up myself, thanks!
On 23/03/2020 21.45, Tom Rini wrote:
On Mon, Mar 23, 2020 at 01:13:01AM +0100, Rasmus Villemoes wrote:
On 03/03/2020 13.19, Rasmus Villemoes wrote:
On 28/01/2020 13.04, Rasmus Villemoes wrote:
[resending with Mario's correct address, sorry for the double post]
The driver correctly uses the shadow register when asked for the current value of an output gpio. Unfortunately, it does RMW on the gpdat register both when setting a gpio as input and output. These two patches fix that.
Ping. Any chance these fixes can make it to 2020.04 ?
Ping^3.
Tom, will you consider applying these directly? It's been two months and I haven't got any response. Or advise on what I could do.
I suppose that if Mario doesn't speak up soon I'll take a look at picking them up myself, thanks!
Thanks. And in the unlikely event you have plenty of time, can I also have you look at the
https://patchwork.ozlabs.org/project/uboot/list/?series=157899
series? The mpc8xxx_spi driver is mostly unusable without the 4/5 patch, at least for accessing flash devices - perhaps some other spi peripherals never encounter the "transfer size is 3 mod 4".
Thanks, Rasmus
participants (2)
-
Rasmus Villemoes -
Tom Rini