* [PATCH 2/2][v3] gpio-mpc8xxx: add mpc8xxx_gpio_set_multiple function @ 2014-05-27 11:14 Rojhalat Ibrahim 2014-05-29 13:27 ` Linus Walleij 0 siblings, 1 reply; 3+ messages in thread From: Rojhalat Ibrahim @ 2014-05-27 11:14 UTC (permalink / raw) To: linux-gpio; +Cc: Alexandre Courbot, Gerhard Sittig, Linus Walleij Add a set_multiple function to the MPC8xxx GPIO chip driver and thereby allow for actual performance improvements when setting multiple outputs simultaneously. In my case the time needed to configure an FPGA goes down from 48 s to 19 s. Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de> --- This patch depends on my previous patch "gpiolib: allow simultaneous setting of multiple GPIO outputs". Change log: v3: - change commit message v2: - add this patch (v1 included only changes to gpiolib) drivers/gpio/gpio-mpc8xxx.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c index d7d6d72..d226f5c 100644 --- a/drivers/gpio/gpio-mpc8xxx.c +++ b/drivers/gpio/gpio-mpc8xxx.c @@ -105,6 +105,33 @@ static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); } +static void mpc8xxx_gpio_set_multiple(struct gpio_chip *gc, + u32 *mask, u32 *bits) +{ + struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); + struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm); + unsigned long flags; + int i; + + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); + + for (i = 0; i < gc->ngpio; i++) { + if (*mask == 0) + break; + if (*mask & (1 << i)) { + if ((*bits >> i) & 1) + mpc8xxx_gc->data |= mpc8xxx_gpio2mask(i); + else + mpc8xxx_gc->data &= ~mpc8xxx_gpio2mask(i); + *mask &= ~(1 << i); + } + } + + out_be32(mm->regs + GPIO_DAT, mpc8xxx_gc->data); + + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); +} + static int mpc8xxx_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio) { struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); @@ -344,6 +371,7 @@ static void __init mpc8xxx_add_controller(struct device_node *np) gc->get = of_device_is_compatible(np, "fsl,mpc8572-gpio") ? mpc8572_gpio_get : mpc8xxx_gpio_get; gc->set = mpc8xxx_gpio_set; + gc->set_multiple = mpc8xxx_gpio_set_multiple; gc->to_irq = mpc8xxx_gpio_to_irq; ret = of_mm_gpiochip_add(np, mm_gc); -- 1.8.5.5 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2][v3] gpio-mpc8xxx: add mpc8xxx_gpio_set_multiple function 2014-05-27 11:14 [PATCH 2/2][v3] gpio-mpc8xxx: add mpc8xxx_gpio_set_multiple function Rojhalat Ibrahim @ 2014-05-29 13:27 ` Linus Walleij 2014-06-01 9:39 ` Alexandre Courbot 0 siblings, 1 reply; 3+ messages in thread From: Linus Walleij @ 2014-05-29 13:27 UTC (permalink / raw) To: Rojhalat Ibrahim, Grant Likely Cc: linux-gpio@vger.kernel.org, Alexandre Courbot, Gerhard Sittig, Mark Brown On Tue, May 27, 2014 at 1:14 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote: > Add a set_multiple function to the MPC8xxx GPIO chip driver and thereby allow > for actual performance improvements when setting multiple outputs > simultaneously. In my case the time needed to configure an FPGA goes down from > 48 s to 19 s. OK that's substantial. We cannot deny this. Performance is awesome. > Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de> > --- > This patch depends on my previous patch "gpiolib: allow simultaneous setting > of multiple GPIO outputs". > > Change log: > v3: - change commit message > v2: - add this patch (v1 included only changes to gpiolib) > > drivers/gpio/gpio-mpc8xxx.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c > index d7d6d72..d226f5c 100644 > --- a/drivers/gpio/gpio-mpc8xxx.c > +++ b/drivers/gpio/gpio-mpc8xxx.c > @@ -105,6 +105,33 @@ static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) > spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); > } > > +static void mpc8xxx_gpio_set_multiple(struct gpio_chip *gc, > + u32 *mask, u32 *bits) > +{ > + struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); > + struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm); > + unsigned long flags; > + int i; > + > + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); > + > + for (i = 0; i < gc->ngpio; i++) { > + if (*mask == 0) > + break; > + if (*mask & (1 << i)) { > + if ((*bits >> i) & 1) Use #include <linux/bitops.h> if (*mask & BIT(i)) { if ((*bits & BIT(i)) .... > + mpc8xxx_gc->data |= mpc8xxx_gpio2mask(i); > + else > + mpc8xxx_gc->data &= ~mpc8xxx_gpio2mask(i); But hey: #define MPC8XXX_GPIO_PINS 32 static inline u32 mpc8xxx_gpio2mask(unsigned int gpio) { return 1u << (MPC8XXX_GPIO_PINS - 1 - gpio); } So what is happening? Bits are swizzled: bit 0 becomes bit 31, bit 1 becomes bit 30 ... bit 31 becomes bit 0. I'm actually for having a function u32 swizzle(u32 foo) {} In the kernel doing exactly this, because I read somewhere that there are ultimax ways for doing this. Namely so (drivers/crypto/ux500/cryp/cryp_core.c): /** * swap_bits_in_byte - mirror the bits in a byte * @b: the byte to be mirrored * * The bits are swapped the following way: * Byte b include bits 0-7, nibble 1 (n1) include bits 0-3 and * nibble 2 (n2) bits 4-7. * * Nibble 1 (n1): * (The "old" (moved) bit is replaced with a zero) * 1. Move bit 6 and 7, 4 positions to the left. * 2. Move bit 3 and 5, 2 positions to the left. * 3. Move bit 1-4, 1 position to the left. * * Nibble 2 (n2): * 1. Move bit 0 and 1, 4 positions to the right. * 2. Move bit 2 and 4, 2 positions to the right. * 3. Move bit 3-6, 1 position to the right. * * Combine the two nibbles to a complete and swapped byte. */ static inline u8 swap_bits_in_byte(u8 b) { #define R_SHIFT_4_MASK (0xc0) /* Bits 6 and 7, right shift 4 */ #define R_SHIFT_2_MASK (0x28) /* (After right shift 4) Bits 3 and 5, right shift 2 */ #define R_SHIFT_1_MASK (0x1e) /* (After right shift 2) Bits 1-4, right shift 1 */ #define L_SHIFT_4_MASK (0x03) /* Bits 0 and 1, left shift 4 */ #define L_SHIFT_2_MASK (0x14) /* (After left shift 4) Bits 2 and 4, left shift 2 */ #define L_SHIFT_1_MASK (0x78) /* (After left shift 1) Bits 3-6, left shift 1 */ u8 n1; u8 n2; /* Swap most significant nibble */ /* Right shift 4, bits 6 and 7 */ n1 = ((b & R_SHIFT_4_MASK) >> 4) | (b & ~(R_SHIFT_4_MASK >> 4)); /* Right shift 2, bits 3 and 5 */ n1 = ((n1 & R_SHIFT_2_MASK) >> 2) | (n1 & ~(R_SHIFT_2_MASK >> 2)); /* Right shift 1, bits 1-4 */ n1 = (n1 & R_SHIFT_1_MASK) >> 1; /* Swap least significant nibble */ /* Left shift 4, bits 0 and 1 */ n2 = ((b & L_SHIFT_4_MASK) << 4) | (b & ~(L_SHIFT_4_MASK << 4)); /* Left shift 2, bits 2 and 4 */ n2 = ((n2 & L_SHIFT_2_MASK) << 2) | (n2 & ~(L_SHIFT_2_MASK << 2)); /* Left shift 1, bits 3-6 */ n2 = (n2 & L_SHIFT_1_MASK) << 1; return n1 | n2; } This can be extended to 16 and 32 bits so if this happens on many places in the kernel then it should be moved to <linux/bitops.h> or similar. > + *mask &= ~(1 << i); *mask &= ~BIT(i); > + } > + } > + out_be32(mm->regs + GPIO_DAT, mpc8xxx_gc->data); Yours, Linus Walleij ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2][v3] gpio-mpc8xxx: add mpc8xxx_gpio_set_multiple function 2014-05-29 13:27 ` Linus Walleij @ 2014-06-01 9:39 ` Alexandre Courbot 0 siblings, 0 replies; 3+ messages in thread From: Alexandre Courbot @ 2014-06-01 9:39 UTC (permalink / raw) To: Linus Walleij Cc: Rojhalat Ibrahim, Grant Likely, linux-gpio@vger.kernel.org, Gerhard Sittig, Mark Brown On Thu, May 29, 2014 at 10:27 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, May 27, 2014 at 1:14 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote: > >> Add a set_multiple function to the MPC8xxx GPIO chip driver and thereby allow >> for actual performance improvements when setting multiple outputs >> simultaneously. In my case the time needed to configure an FPGA goes down from >> 48 s to 19 s. > > OK that's substantial. We cannot deny this. Performance is awesome. > >> Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de> >> --- >> This patch depends on my previous patch "gpiolib: allow simultaneous setting >> of multiple GPIO outputs". >> >> Change log: >> v3: - change commit message >> v2: - add this patch (v1 included only changes to gpiolib) >> >> drivers/gpio/gpio-mpc8xxx.c | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c >> index d7d6d72..d226f5c 100644 >> --- a/drivers/gpio/gpio-mpc8xxx.c >> +++ b/drivers/gpio/gpio-mpc8xxx.c >> @@ -105,6 +105,33 @@ static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) >> spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); >> } >> >> +static void mpc8xxx_gpio_set_multiple(struct gpio_chip *gc, >> + u32 *mask, u32 *bits) >> +{ >> + struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); >> + struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm); >> + unsigned long flags; >> + int i; >> + >> + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); >> + >> + for (i = 0; i < gc->ngpio; i++) { >> + if (*mask == 0) >> + break; >> + if (*mask & (1 << i)) { >> + if ((*bits >> i) & 1) > > Use > #include <linux/bitops.h> > > if (*mask & BIT(i)) { > if ((*bits & BIT(i)) > > .... > >> + mpc8xxx_gc->data |= mpc8xxx_gpio2mask(i); >> + else >> + mpc8xxx_gc->data &= ~mpc8xxx_gpio2mask(i); > > But hey: > > #define MPC8XXX_GPIO_PINS 32 > > static inline u32 mpc8xxx_gpio2mask(unsigned int gpio) > { > return 1u << (MPC8XXX_GPIO_PINS - 1 - gpio); > } > > So what is happening? Bits are swizzled: bit 0 becomes > bit 31, bit 1 becomes bit 30 ... bit 31 becomes bit 0. > I'm actually for having a function > > u32 swizzle(u32 foo) {} > > In the kernel doing exactly this, because I read somewhere > that there are ultimax ways for doing this. Namely so > (drivers/crypto/ux500/cryp/cryp_core.c): > > /** > * swap_bits_in_byte - mirror the bits in a byte > * @b: the byte to be mirrored > * > * The bits are swapped the following way: > * Byte b include bits 0-7, nibble 1 (n1) include bits 0-3 and > * nibble 2 (n2) bits 4-7. > * > * Nibble 1 (n1): > * (The "old" (moved) bit is replaced with a zero) > * 1. Move bit 6 and 7, 4 positions to the left. > * 2. Move bit 3 and 5, 2 positions to the left. > * 3. Move bit 1-4, 1 position to the left. > * > * Nibble 2 (n2): > * 1. Move bit 0 and 1, 4 positions to the right. > * 2. Move bit 2 and 4, 2 positions to the right. > * 3. Move bit 3-6, 1 position to the right. > * > * Combine the two nibbles to a complete and swapped byte. > */ > > static inline u8 swap_bits_in_byte(u8 b) > { > #define R_SHIFT_4_MASK (0xc0) /* Bits 6 and 7, right shift 4 */ > #define R_SHIFT_2_MASK (0x28) /* (After right shift 4) Bits 3 and 5, > right shift 2 */ > #define R_SHIFT_1_MASK (0x1e) /* (After right shift 2) Bits 1-4, > right shift 1 */ > #define L_SHIFT_4_MASK (0x03) /* Bits 0 and 1, left shift 4 */ > #define L_SHIFT_2_MASK (0x14) /* (After left shift 4) Bits 2 and 4, > left shift 2 */ > #define L_SHIFT_1_MASK (0x78) /* (After left shift 1) Bits 3-6, > left shift 1 */ > > u8 n1; > u8 n2; > > /* Swap most significant nibble */ > /* Right shift 4, bits 6 and 7 */ > n1 = ((b & R_SHIFT_4_MASK) >> 4) | (b & ~(R_SHIFT_4_MASK >> 4)); > /* Right shift 2, bits 3 and 5 */ > n1 = ((n1 & R_SHIFT_2_MASK) >> 2) | (n1 & ~(R_SHIFT_2_MASK >> 2)); > /* Right shift 1, bits 1-4 */ > n1 = (n1 & R_SHIFT_1_MASK) >> 1; > > /* Swap least significant nibble */ > /* Left shift 4, bits 0 and 1 */ > n2 = ((b & L_SHIFT_4_MASK) << 4) | (b & ~(L_SHIFT_4_MASK << 4)); > /* Left shift 2, bits 2 and 4 */ > n2 = ((n2 & L_SHIFT_2_MASK) << 2) | (n2 & ~(L_SHIFT_2_MASK << 2)); > /* Left shift 1, bits 3-6 */ > n2 = (n2 & L_SHIFT_1_MASK) << 1; > > return n1 | n2; > } > > This can be extended to 16 and 32 bits so if this happens on > many places in the kernel then it should be moved to > <linux/bitops.h> or similar. > >> + *mask &= ~(1 << i); > > *mask &= ~BIT(i); Linus, just for my own education do you have anything against __clear_bit(*mask, BIT(i))? > >> + } >> + } >> + out_be32(mm->regs + GPIO_DAT, mpc8xxx_gc->data); > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-06-01 9:39 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-27 11:14 [PATCH 2/2][v3] gpio-mpc8xxx: add mpc8xxx_gpio_set_multiple function Rojhalat Ibrahim 2014-05-29 13:27 ` Linus Walleij 2014-06-01 9:39 ` Alexandre Courbot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).