[PATCH v2] pinctrl: at91-pio4: implement .get_multiple and .set_multiple

[PATCH v2] pinctrl: at91-pio4: implement .get_multiple and .set_multiple

[Alexandre Belloni]

Implement .get_multiple and .set_multiple to allow reading or setting
multiple pins simultaneously. Pins in the same bank will all be switched at
the same time, improving synchronization and performances.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---

Changes since v1
https://lore.kernel.org/lkml/20190905141304.22005-1-alexandre.belloni@bootlin.com/ :
 - Removed debug line



 drivers/pinctrl/pinctrl-at91-pio4.c | 58 +++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
index d6de4d360cd4..d281ec40e098 100644
--- a/drivers/pinctrl/pinctrl-at91-pio4.c
+++ b/drivers/pinctrl/pinctrl-at91-pio4.c
@@ -328,6 +328,33 @@ static int atmel_gpio_get(struct gpio_chip *chip, unsigned offset)
 	return !!(reg & BIT(pin->line));
 }
 
+static int atmel_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
+				   unsigned long *bits)
+{
+	struct atmel_pioctrl *atmel_pioctrl = gpiochip_get_data(chip);
+	unsigned int bank;
+
+	bitmap_zero(bits, atmel_pioctrl->npins);
+
+	for (bank = 0; bank < atmel_pioctrl->nbanks; bank++) {
+		unsigned int word = bank;
+		unsigned int offset = 0;
+		unsigned int reg;
+
+#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
+		word = BIT_WORD(bank * ATMEL_PIO_NPINS_PER_BANK);
+		offset = bank * ATMEL_PIO_NPINS_PER_BANK % BITS_PER_LONG;
+#endif
+		if (!mask[word])
+			continue;
+
+		reg = atmel_gpio_read(atmel_pioctrl, bank, ATMEL_PIO_PDSR);
+		bits[word] |= mask[word] & (reg << offset);
+	}
+
+	return 0;
+}
+
 static int atmel_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
 				       int value)
 {
@@ -358,11 +385,42 @@ static void atmel_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
 			 BIT(pin->line));
 }
 
+static void atmel_gpio_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+				    unsigned long *bits)
+{
+	struct atmel_pioctrl *atmel_pioctrl = gpiochip_get_data(chip);
+	unsigned int bank;
+
+	for (bank = 0; bank < atmel_pioctrl->nbanks; bank++) {
+		unsigned int bitmask;
+		unsigned int word = bank;
+
+#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
+		word = BIT_WORD(bank * ATMEL_PIO_NPINS_PER_BANK);
+#endif
+		if (!mask[word])
+			continue;
+
+		bitmask = mask[word] & bits[word];
+		atmel_gpio_write(atmel_pioctrl, bank, ATMEL_PIO_SODR, bitmask);
+
+		bitmask = mask[word] & ~bits[word];
+		atmel_gpio_write(atmel_pioctrl, bank, ATMEL_PIO_CODR, bitmask);
+
+#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
+		mask[word] >>= ATMEL_PIO_NPINS_PER_BANK;
+		bits[word] >>= ATMEL_PIO_NPINS_PER_BANK;
+#endif
+	}
+}
+
 static struct gpio_chip atmel_gpio_chip = {
 	.direction_input        = atmel_gpio_direction_input,
 	.get                    = atmel_gpio_get,
+	.get_multiple           = atmel_gpio_get_multiple,
 	.direction_output       = atmel_gpio_direction_output,
 	.set                    = atmel_gpio_set,
+	.set_multiple           = atmel_gpio_set_multiple,
 	.to_irq                 = atmel_gpio_to_irq,
 	.base                   = 0,
 };
-- 
2.21.0

RE: [PATCH v2] pinctrl: at91-pio4: implement .get_multiple and .set_multiple

[David Laight]

From: Alexandre Belloni
> Implement .get_multiple and .set_multiple to allow reading or setting
> multiple pins simultaneously. Pins in the same bank will all be switched at
> the same time, improving synchronization and performances.

Actually it won't 'improve synchronisation', instead it will lead to
random synchronisation errors and potential metastability if one
pin is used as a clock and another as data, or if the code is reading
a free-flowing counter.

It will improve performance though.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2] pinctrl: at91-pio4: implement .get_multiple and .set_multiple

[Alexandre Belloni]

On 06/09/2019 09:05:36+0000, David Laight wrote:
> From: Alexandre Belloni
> > Implement .get_multiple and .set_multiple to allow reading or setting
> > multiple pins simultaneously. Pins in the same bank will all be switched at
> > the same time, improving synchronization and performances.
> 
> Actually it won't 'improve synchronisation', instead it will lead to
> random synchronisation errors and potential metastability if one
> pin is used as a clock and another as data, or if the code is reading
> a free-flowing counter.
> 

It does improve gpio switching synchronisation when they are in the same
bank as it will remove the 250ns delay. Of course, if you need this
delay between clk and data, then the consumer driver should ensure the
delay is present.

> It will improve performance though.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH v2] pinctrl: at91-pio4: implement .get_multiple and .set_multiple

[David Laight]

From: Alexandre Belloni
> Sent: 06 September 2019 10:12
> On 06/09/2019 09:05:36+0000, David Laight wrote:
> > From: Alexandre Belloni
> > > Implement .get_multiple and .set_multiple to allow reading or setting
> > > multiple pins simultaneously. Pins in the same bank will all be switched at
> > > the same time, improving synchronization and performances.
> >
> > Actually it won't 'improve synchronisation', instead it will lead to
> > random synchronisation errors and potential metastability if one
> > pin is used as a clock and another as data, or if the code is reading
> > a free-flowing counter.
> >
> 
> It does improve gpio switching synchronisation when they are in the same
> bank as it will remove the 250ns delay. Of course, if you need this
> delay between clk and data, then the consumer driver should ensure the
> delay is present.

With multiple requests the output pin changes will always be in the
same order and will be separated by (say) 250ns.
This is a guaranteed synchronisation.

If you change multiple pins with the same 'iowrite()' then the pins
will change at approximately the same time.
But the actual order will depend on internal device delays (which
may depend on the actual silicon and temperature).
You then have to take account of varying track lengths and the
target devices input stage properties before knowing which change
arrives first.
The delays might be sub-nanosecond, but they matter if you are
talking about synchronisation.

IIRC both SMBus and I2C now quote 0ns setup time.
Changing both clock and data with the same IOW isn't enough to
guarantee this.
(In practise the I2C setup time required by a device is probably
slightly negative (In order to support 0ns inputs) so a very small
-ve setup will (mostly) work.)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2] pinctrl: at91-pio4: implement .get_multiple and .set_multiple

[Alexandre Belloni]

On 06/09/2019 09:46:02+0000, David Laight wrote:
> From: Alexandre Belloni
> > Sent: 06 September 2019 10:12
> > On 06/09/2019 09:05:36+0000, David Laight wrote:
> > > From: Alexandre Belloni
> > > > Implement .get_multiple and .set_multiple to allow reading or setting
> > > > multiple pins simultaneously. Pins in the same bank will all be switched at
> > > > the same time, improving synchronization and performances.
> > >
> > > Actually it won't 'improve synchronisation', instead it will lead to
> > > random synchronisation errors and potential metastability if one
> > > pin is used as a clock and another as data, or if the code is reading
> > > a free-flowing counter.
> > >
> > 
> > It does improve gpio switching synchronisation when they are in the same
> > bank as it will remove the 250ns delay. Of course, if you need this
> > delay between clk and data, then the consumer driver should ensure the
> > delay is present.
> 
> With multiple requests the output pin changes will always be in the
> same order and will be separated by (say) 250ns.
> This is a guaranteed synchronisation.
> 
> If you change multiple pins with the same 'iowrite()' then the pins
> will change at approximately the same time.
> But the actual order will depend on internal device delays (which
> may depend on the actual silicon and temperature).
> You then have to take account of varying track lengths and the
> target devices input stage properties before knowing which change
> arrives first.
> The delays might be sub-nanosecond, but they matter if you are
> talking about synchronisation.
> 

And my point is that this means that your gpio consumer driver is buggy
if it doesn't do multiple requests if it requires a delay between two
pin changes.

> IIRC both SMBus and I2C now quote 0ns setup time.
> Changing both clock and data with the same IOW isn't enough to
> guarantee this.
> (In practise the I2C setup time required by a device is probably
> slightly negative (In order to support 0ns inputs) so a very small
> -ve setup will (mostly) work.)

I'm not sure what is your point exactly as this patch doesn't break any
existing use cases.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2] pinctrl: at91-pio4: implement .get_multiple and .set_multiple

[Linus Walleij]

On Fri, Sep 6, 2019 at 11:46 AM David Laight <David.Laight@aculab.com> wrote:
> > On 06/09/2019 09:05:36+0000, David Laight wrote:

> > It does improve gpio switching synchronisation when they are in the same
> > bank as it will remove the 250ns delay. Of course, if you need this
> > delay between clk and data, then the consumer driver should ensure the
> > delay is present.
>
> With multiple requests the output pin changes will always be in the
> same order and will be separated by (say) 250ns.
> This is a guaranteed synchronisation.

Do you mean that hardware will guarantee this synchronization?
Linux device driver code cannot rely on that. We will simply
grab two individual GPIO lines (not get_multiple()) then issue
set() on the clock, ndelay(250) and then set() data.

It doesn't matter much because bitbanging is always extremely
slow anyways so one will get lots of delay, which is why
e.g. spi-gpio doesn't insert any delay IIRC.

The point is that the lines need be grabbed individually so
the delay between can be controlled.

> IIRC both SMBus and I2C now quote 0ns setup time.
> Changing both clock and data with the same IOW isn't enough to
> guarantee this.
> (In practise the I2C setup time required by a device is probably
> slightly negative (In order to support 0ns inputs) so a very small
> -ve setup will (mostly) work.)

If you are referring to drivers/i2c/busses/i2c-gpio.c it does seem
to do proper delays using bit_data->udelay from the bitbang
library.

Yours,
Linus Walleij