[PATCH 2/2][v3] gpio-mpc8xxx: add mpc8xxx_gpio_set_multiple function

[PATCH 2/2][v3] gpio-mpc8xxx: add mpc8xxx_gpio_set_multiple function

[Rojhalat Ibrahim]

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

Re: [PATCH 2/2][v3] gpio-mpc8xxx: add mpc8xxx_gpio_set_multiple function

[Linus Walleij]

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

Re: [PATCH 2/2][v3] gpio-mpc8xxx: add mpc8xxx_gpio_set_multiple function

[Alexandre Courbot]

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