| Message ID | 20190905144849.24882-1-alexandre.belloni@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pinctrl: at91-pio4: implement .get_multiple and .set_multiple | expand |
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)
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) >
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)
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.
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
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, };
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(+)