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

[Linus Walleij <linus.walleij@linaro.org>]

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