Re: [PATCH 2/2] gpio: aspeed: Use a cache of output data registers

[Joel Stanley <joel@xxxxxxxxx>]

On 17 May 2018 at 17:42, Benjamin Herrenschmidt <benh@xxxxxxxxxxx> wrote:
> The current driver does a read/modify/write of the output
> registers when changing a bit in __aspeed_gpio_set().
>
> This is sub-optimal for a couple of reasons:
>
>   - If any of the neighbouring GPIOs (sharing the shared
> register) isn't (yet) configured as an output, it will
> read the current input value, and then apply it to the
> output latch, which may not be what the user expects. There
> should be no bug in practice as aspeed_gpio_dir_out() will
> establish a new value but it's not great either.
>
>   - The GPIO block in the aspeed chip is clocked rather
> slowly (typically 25Mhz). That extra MMIO read halves the maximum
> speed at which we can toggle the GPIO.
>
> This provides a significant performance improvement to the GPIO
> based FSI master.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Christopher Bostic <cbostic@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Andrew Jeffery <andrew@xxxxxxxx>

Tested-by: Joel Stanley <joel@xxxxxxxxx>

Cheers,

Joel


> ---
>  drivers/gpio/gpio-aspeed.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index bccf0399550e..5e89f1c74a33 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -54,6 +54,8 @@ struct aspeed_gpio {
>         u8 *offset_timer;
>         unsigned int timer_users[4];
>         struct clk *clk;
> +
> +       u32 *dcache;
>  };
>
>  struct aspeed_gpio_bank {
> @@ -231,12 +233,13 @@ static void __aspeed_gpio_set(struct gpio_chip *gc, unsigned int offset,
>         u32 reg;
>
>         addr = bank_val_reg(gpio, bank, GPIO_DATA);
> -       reg = ioread32(addr);
> +       reg = gpio->dcache[GPIO_BANK(offset)];
>
>         if (val)
>                 reg |= GPIO_BIT(offset);
>         else
>                 reg &= ~GPIO_BIT(offset);
> +       gpio->dcache[GPIO_BANK(offset)] = reg;
>
>         iowrite32(reg, addr);
>  }
> @@ -851,7 +854,7 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
>         const struct of_device_id *gpio_id;
>         struct aspeed_gpio *gpio;
>         struct resource *res;
> -       int rc;
> +       int rc, i, banks;
>
>         gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
>         if (!gpio)
> @@ -892,6 +895,20 @@ static int __init aspeed_gpio_probe(struct platform_device *pdev)
>         gpio->chip.base = -1;
>         gpio->chip.irq.need_valid_mask = true;
>
> +       /* Allocate a cache of the output registers */
> +       banks = gpio->config->nr_gpios >> 5;
> +       gpio->dcache = devm_kzalloc(&pdev->dev,
> +                                   sizeof(u32) * banks, GFP_KERNEL);
> +       if (!gpio->dcache)
> +               return -ENOMEM;
> +
> +       /* Populate it with initial values read from the HW */
> +       for (i = 0; i < banks; i++) {
> +               const struct aspeed_gpio_bank *bank = &aspeed_gpio_banks[i];
> +               gpio->dcache[i] = ioread32(gpio->base + bank->val_regs +
> +                                          GPIO_DATA);
> +       }
> +
>         rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
>         if (rc < 0)
>                 return rc;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html