diff mbox series

[resend,2/2] gpio: mpc8xxx: don't do RMW on gpdat register when setting value

Message ID 20200128120423.16667-3-rasmus.villemoes@prevas.dk
State Accepted
Commit dd4cf53f9864362d9e92472298ad60dcbb8b7e72
Delegated to: Tom Rini
Headers show
Series gpio: mpc8xxx: honour shadow register when writing gpdat | expand

Commit Message

Rasmus Villemoes Jan. 28, 2020, 12:04 p.m. UTC 
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(-)

Comments

Tom Rini March 31, 2020, 7:09 p.m. UTC | #1 
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!
diff mbox series

Patch

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;
 }