Skip to contentSkip to conversation

gpio-mpc8xxx: add mpc8xxx_gpio_set_multiple function

PATCH Superseded Rojhalat Ibrahim 3 participants 3 messages May 27, 2014 – Jun 1, 2014 GPIO

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Rojhalat Ibrahim

author
#0 lore raw

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

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 <[email protected]>

---
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(+)
drivers/gpio/gpio-mpc8xxx.c +28 -0
index d7d6d72..d226f5c 100644
@@ -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

#1 lore raw
Quoted 4 lines · 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.

OK that's substantial. We cannot deny this. Performance is awesome.

Quoted 25 lines
Signed-off-by Rojhalat Ibrahim <[email protected]>

---
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(+)
drivers/gpio/gpio-mpc8xxx.c +5 -0
index d7d6d72..d226f5c 100644
@@ -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);

drivers/gpio/gpio-mpc8xxx.c static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) · line 122

Quoted 10 lines
drivers/gpio/gpio-mpc8xxx.c +10 -0
+       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))

....

drivers/gpio/gpio-mpc8xxx.c static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) · line 125

Quoted 3 lines
drivers/gpio/gpio-mpc8xxx.c +3 -0
+                               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.

Quoted 1 line

+ *mask &= ~(1 << i);

*mask &= ~BIT(i);

drivers/gpio/gpio-mpc8xxx.c static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) · line 130

Quoted 3 lines
drivers/gpio/gpio-mpc8xxx.c +3 -0
+               }
+       }
+       out_be32(mm->regs + GPIO_DAT, mpc8xxx_gc->data);

Yours,
Linus Walleij

#2 lore raw
Quoted 133 lines · Linus Walleij
Quoted 4 lines · 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.

OK that's substantial. We cannot deny this. Performance is awesome.

Quoted 25 lines
Signed-off-by Rojhalat Ibrahim <[email protected]>

---
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(+)
drivers/gpio/gpio-mpc8xxx.c +5 -0
index d7d6d72..d226f5c 100644
@@ -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);

drivers/gpio/gpio-mpc8xxx.c static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) · line 122

Quoted 10 lines
drivers/gpio/gpio-mpc8xxx.c +10 -0
+       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))

....

drivers/gpio/gpio-mpc8xxx.c static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) · line 125

Quoted 3 lines
drivers/gpio/gpio-mpc8xxx.c +3 -0
+                               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;
}
Quoted 7 lines

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.

Quoted 1 line

+ *mask &= ~(1 << i);

*mask &= ~BIT(i);

Linus, just for my own education do you have anything against __clear_bit(*mask, BIT(i))?

Quoted 7 lines

drivers/gpio/gpio-mpc8xxx.c static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) · line 130

Quoted 4 lines
drivers/gpio/gpio-mpc8xxx.c +3 -0
+               }
+       }
+       out_be32(mm->regs + GPIO_DAT, mpc8xxx_gc->data);

Yours,
Linus Walleij