| Message ID | 20250215-vf610-mmio-v2-2-4a91f8c8e8d5@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio-mmio: Extend to handle pinctrl back-ends | expand |
> -----Original Message----- > From: Linus Walleij <linus.walleij@linaro.org> > Sent: 2025年2月15日 7:13 > To: Johan Korsnes <johan.korsnes@remarkable.no>; Bough Chen > <haibo.chen@nxp.com>; Bartosz Golaszewski <brgl@bgdev.pl>; > imx@lists.linux.dev > Cc: linux-gpio@vger.kernel.org; Linus Walleij <linus.walleij@linaro.org> > Subject: [PATCH v2 2/2] gpio: vf610: Switch to gpio-mmio > > After adding a pinctrl flag to gpio-mmio we can use it for driving gpio-vf610. > > The existing code has the same semantics and the generic gpio-mmio, including > reading from the data out register when the direction is set to input, and it can > also handle the absence of the direction register better than the current driver: > we get the direction from the shadow direction registers in gpio-mmio instead. > > Since gpio-mmio has an internal spinlock we can drop the direction-protecting > spinlock from the driver. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - Use the dual input/output set/clear registers for output. > - Provide the BGPIOF_READ_OUTPUT_REG_SET flag so the driver > behaves as described in the commit message... > - Drop the now unused spinlock (gpio-mmio has its own). > --- > drivers/gpio/Kconfig | 1 + > drivers/gpio/gpio-vf610.c | 109 ++++++++-------------------------------------- > 2 files changed, 18 insertions(+), 92 deletions(-) > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index > add5ad29a673c09082a913cb2404073b2034af48..ab104ce85ee6cef1549d3174 > 4625bcc625d75179 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -756,6 +756,7 @@ config GPIO_VF610 > default y if SOC_VF610 > depends on ARCH_MXC || COMPILE_TEST > select GPIOLIB_IRQCHIP > + select GPIO_GENERIC > help > Say yes here to support i.MX or Vybrid vf610 GPIOs. > > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c index > 3527487d42c8ac3ef39c3be468dd5177c85f6b44..4aa80dae9badbe8a9cc7b2ad > acf321ed237c3b57 100644 > --- a/drivers/gpio/gpio-vf610.c > +++ b/drivers/gpio/gpio-vf610.c > @@ -36,7 +36,6 @@ struct vf610_gpio_port { > struct clk *clk_port; > struct clk *clk_gpio; > int irq; > - spinlock_t lock; /* protect gpio direction registers */ > }; > > #define GPIO_PDOR 0x00 > @@ -94,82 +93,6 @@ static inline u32 vf610_gpio_readl(void __iomem *reg) > return readl_relaxed(reg); > } > > -static int vf610_gpio_get(struct gpio_chip *gc, unsigned int gpio) -{ > - struct vf610_gpio_port *port = gpiochip_get_data(gc); > - u32 mask = BIT(gpio); > - unsigned long offset = GPIO_PDIR; > - > - if (port->sdata->have_paddr) { > - mask &= vf610_gpio_readl(port->gpio_base + GPIO_PDDR); > - if (mask) > - offset = GPIO_PDOR; > - } > - > - return !!(vf610_gpio_readl(port->gpio_base + offset) & BIT(gpio)); > -} > - > -static void vf610_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) -{ > - struct vf610_gpio_port *port = gpiochip_get_data(gc); > - u32 mask = BIT(gpio); > - unsigned long offset = val ? GPIO_PSOR : GPIO_PCOR; > - > - vf610_gpio_writel(mask, port->gpio_base + offset); > -} > - > -static int vf610_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio) > -{ > - struct vf610_gpio_port *port = gpiochip_get_data(chip); > - u32 mask = BIT(gpio); > - unsigned long flags; > - u32 val; > - > - if (port->sdata->have_paddr) { > - spin_lock_irqsave(&port->lock, flags); > - val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR); > - val &= ~mask; > - vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR); > - spin_unlock_irqrestore(&port->lock, flags); > - } > - > - return pinctrl_gpio_direction_input(chip, gpio); > -} > - > -static int vf610_gpio_direction_output(struct gpio_chip *chip, unsigned int > gpio, > - int value) > -{ > - struct vf610_gpio_port *port = gpiochip_get_data(chip); > - u32 mask = BIT(gpio); > - unsigned long flags; > - u32 val; > - > - vf610_gpio_set(chip, gpio, value); > - > - if (port->sdata->have_paddr) { > - spin_lock_irqsave(&port->lock, flags); > - val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR); > - val |= mask; > - vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR); > - spin_unlock_irqrestore(&port->lock, flags); > - } > - > - return pinctrl_gpio_direction_output(chip, gpio); > -} > - > -static int vf610_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio) -{ > - struct vf610_gpio_port *port = gpiochip_get_data(gc); > - u32 mask = BIT(gpio); > - > - mask &= vf610_gpio_readl(port->gpio_base + GPIO_PDDR); > - > - if (mask) > - return GPIO_LINE_DIRECTION_OUT; > - > - return GPIO_LINE_DIRECTION_IN; > -} > - > static void vf610_gpio_irq_handler(struct irq_desc *desc) { > struct vf610_gpio_port *port = > @@ -295,6 +218,7 @@ static int vf610_gpio_probe(struct platform_device > *pdev) > struct vf610_gpio_port *port; > struct gpio_chip *gc; > struct gpio_irq_chip *girq; > + unsigned long flags; > int i; > int ret; > bool dual_base; > @@ -304,7 +228,6 @@ static int vf610_gpio_probe(struct platform_device > *pdev) > return -ENOMEM; > > port->sdata = device_get_match_data(dev); > - spin_lock_init(&port->lock); > > dual_base = port->sdata->have_dual_base; > > @@ -371,23 +294,25 @@ static int vf610_gpio_probe(struct platform_device > *pdev) > } > > gc = &port->gc; > - gc->parent = dev; > - gc->label = dev_name(dev); > - gc->ngpio = VF610_GPIO_PER_PORT; > - gc->base = -1; > - > - gc->request = gpiochip_generic_request; > - gc->free = gpiochip_generic_free; > - gc->direction_input = vf610_gpio_direction_input; > - gc->get = vf610_gpio_get; > - gc->direction_output = vf610_gpio_direction_output; > - gc->set = vf610_gpio_set; > + flags = BGPIOF_PINCTRL_BACKEND; > /* > - * only IP has Port Data Direction Register(PDDR) can > - * support get direction > + * We only read the output register for current value on output > + * lines if the direction register is available so we can switch > + * direction. > */ > if (port->sdata->have_paddr) > - gc->get_direction = vf610_gpio_get_direction; > + flags |= BGPIOF_READ_OUTPUT_REG_SET; > + ret = bgpio_init(gc, dev, 4, > + port->base + GPIO_PDOR, > + port->base + GPIO_PSOR, > + port->base + GPIO_PCOR, Hi Linus, Two issue here: 1, need to use port->gpio_base instead port->base. 2, vf610 has register GPIO_PDIR, if find the direction is input, need to read this register to get the input value. But for mmio driver, it will read the gc->reg_set, which is GPIO_PSOR with flags BGPIOF_READ_OUTPUT_REG_SET, this is not correct. Seems current bgpio_init() need to add another parameter to input this GPIO_PDIR, but this will has big impact. I try to adjust the bgpio_init like this: ret = bgpio_init(gc, dev, 4, port->gpio_base + GPIO_PDIR, port->gpio_base + GPIO_PSOR, port->gpio_base + GPIO_PCOR, port->sdata->have_paddr ? port->base + GPIO_PDDR : NULL, NULL, flags); But still can't work fine, because when gpio is config in output mode, it will read GPIO_PSOR as output value, but for vf610, read GPIO_PSOR/GPIO_PCOR already return 0 root@imx943evk:/unit_tests# ./memtool 0x43840048=0x00100000 (write PCOR) Writing 32-bit value 0x100000 to address 0x43840048 root@imx943evk:/unit_tests# ./memtool 0x43840040 7 (check PCOR after write, it is 0, but find bit 20 of PDOR change to 0, means the PCOR logic works) E Reading 0x7 count starting at address 0x43840040 0x43840040: 00000000 00000000 00000000 00000000 0x43840050: 0000000C 08100000 00000000 root@imx943evk:/unit_tests# ./memtool 0x43840044=0x00100000 (write PSOR) Writing 32-bit value 0x100000 to address 0x43840044 root@imx943evk:/unit_tests# ./memtool 0x43840040 7 (check PSOR after write, it is 0, but find bit 20 of PDOR change to 1, means the PSOR logic works) E Reading 0x7 count starting at address 0x43840040 0x43840040: 00100000 00000000 00000000 00000000 0x43840050: 0000000C 08100000 00000000 Regards Haibo Chen > + port->sdata->have_paddr ? port->base + GPIO_PDDR : NULL, > + NULL, > + flags); > + if (ret) > + return dev_err_probe(dev, ret, "unable to init generic GPIO\n"); > + gc->label = dev_name(dev); > + gc->base = -1; > > /* Mask all GPIO interrupts */ > for (i = 0; i < gc->ngpio; i++) > > -- > 2.48.1
Hi Bough, thanks for testing all my patches! On Mon, Feb 17, 2025 at 12:24 PM Bough Chen <haibo.chen@nxp.com> wrote: > 1, need to use port->gpio_base instead port->base. Oh, what a mistake. Fixed. > 2, vf610 has register GPIO_PDIR, if find the direction is input, need to read this register to get the input value. > But for mmio driver, it will read the gc->reg_set, which is GPIO_PSOR with flags > BGPIOF_READ_OUTPUT_REG_SET, this is not correct. Seems current bgpio_init() > need to add another parameter to input this GPIO_PDIR, but this will has big impact. > > I try to adjust the bgpio_init like this: > ret = bgpio_init(gc, dev, 4, port->gpio_base + GPIO_PDIR, port->gpio_base + GPIO_PSOR, > port->gpio_base + GPIO_PCOR, port->sdata->have_paddr ? port->base + GPIO_PDDR : NULL, NULL, flags); > > But still can't work fine, because when gpio is config in output mode, it will read GPIO_PSOR > as output value, but for vf610, read GPIO_PSOR/GPIO_PCOR already return 0 But what happens if we *only* specify PSOR as setting register to bgpio? Usually when there are set/clear registers, the block also allows direct read/modify/write on the output data register (I have seen this once before...) I sent a v3 like that. Does this work? Else I think we should perhaps simply bite the bullet and add a new argument to bgpio_init() for passing set/clear/data triplets. It can't be avoided: if this sort of hardware does exist with set and clear and a non-writeable data register, then gpio-mmio should support it. Yours, Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index add5ad29a673c09082a913cb2404073b2034af48..ab104ce85ee6cef1549d31744625bcc625d75179 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -756,6 +756,7 @@ config GPIO_VF610 default y if SOC_VF610 depends on ARCH_MXC || COMPILE_TEST select GPIOLIB_IRQCHIP + select GPIO_GENERIC help Say yes here to support i.MX or Vybrid vf610 GPIOs. diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c index 3527487d42c8ac3ef39c3be468dd5177c85f6b44..4aa80dae9badbe8a9cc7b2adacf321ed237c3b57 100644 --- a/drivers/gpio/gpio-vf610.c +++ b/drivers/gpio/gpio-vf610.c @@ -36,7 +36,6 @@ struct vf610_gpio_port { struct clk *clk_port; struct clk *clk_gpio; int irq; - spinlock_t lock; /* protect gpio direction registers */ }; #define GPIO_PDOR 0x00 @@ -94,82 +93,6 @@ static inline u32 vf610_gpio_readl(void __iomem *reg) return readl_relaxed(reg); } -static int vf610_gpio_get(struct gpio_chip *gc, unsigned int gpio) -{ - struct vf610_gpio_port *port = gpiochip_get_data(gc); - u32 mask = BIT(gpio); - unsigned long offset = GPIO_PDIR; - - if (port->sdata->have_paddr) { - mask &= vf610_gpio_readl(port->gpio_base + GPIO_PDDR); - if (mask) - offset = GPIO_PDOR; - } - - return !!(vf610_gpio_readl(port->gpio_base + offset) & BIT(gpio)); -} - -static void vf610_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) -{ - struct vf610_gpio_port *port = gpiochip_get_data(gc); - u32 mask = BIT(gpio); - unsigned long offset = val ? GPIO_PSOR : GPIO_PCOR; - - vf610_gpio_writel(mask, port->gpio_base + offset); -} - -static int vf610_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio) -{ - struct vf610_gpio_port *port = gpiochip_get_data(chip); - u32 mask = BIT(gpio); - unsigned long flags; - u32 val; - - if (port->sdata->have_paddr) { - spin_lock_irqsave(&port->lock, flags); - val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR); - val &= ~mask; - vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR); - spin_unlock_irqrestore(&port->lock, flags); - } - - return pinctrl_gpio_direction_input(chip, gpio); -} - -static int vf610_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio, - int value) -{ - struct vf610_gpio_port *port = gpiochip_get_data(chip); - u32 mask = BIT(gpio); - unsigned long flags; - u32 val; - - vf610_gpio_set(chip, gpio, value); - - if (port->sdata->have_paddr) { - spin_lock_irqsave(&port->lock, flags); - val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR); - val |= mask; - vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR); - spin_unlock_irqrestore(&port->lock, flags); - } - - return pinctrl_gpio_direction_output(chip, gpio); -} - -static int vf610_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio) -{ - struct vf610_gpio_port *port = gpiochip_get_data(gc); - u32 mask = BIT(gpio); - - mask &= vf610_gpio_readl(port->gpio_base + GPIO_PDDR); - - if (mask) - return GPIO_LINE_DIRECTION_OUT; - - return GPIO_LINE_DIRECTION_IN; -} - static void vf610_gpio_irq_handler(struct irq_desc *desc) { struct vf610_gpio_port *port = @@ -295,6 +218,7 @@ static int vf610_gpio_probe(struct platform_device *pdev) struct vf610_gpio_port *port; struct gpio_chip *gc; struct gpio_irq_chip *girq; + unsigned long flags; int i; int ret; bool dual_base; @@ -304,7 +228,6 @@ static int vf610_gpio_probe(struct platform_device *pdev) return -ENOMEM; port->sdata = device_get_match_data(dev); - spin_lock_init(&port->lock); dual_base = port->sdata->have_dual_base; @@ -371,23 +294,25 @@ static int vf610_gpio_probe(struct platform_device *pdev) } gc = &port->gc; - gc->parent = dev; - gc->label = dev_name(dev); - gc->ngpio = VF610_GPIO_PER_PORT; - gc->base = -1; - - gc->request = gpiochip_generic_request; - gc->free = gpiochip_generic_free; - gc->direction_input = vf610_gpio_direction_input; - gc->get = vf610_gpio_get; - gc->direction_output = vf610_gpio_direction_output; - gc->set = vf610_gpio_set; + flags = BGPIOF_PINCTRL_BACKEND; /* - * only IP has Port Data Direction Register(PDDR) can - * support get direction + * We only read the output register for current value on output + * lines if the direction register is available so we can switch + * direction. */ if (port->sdata->have_paddr) - gc->get_direction = vf610_gpio_get_direction; + flags |= BGPIOF_READ_OUTPUT_REG_SET; + ret = bgpio_init(gc, dev, 4, + port->base + GPIO_PDOR, + port->base + GPIO_PSOR, + port->base + GPIO_PCOR, + port->sdata->have_paddr ? port->base + GPIO_PDDR : NULL, + NULL, + flags); + if (ret) + return dev_err_probe(dev, ret, "unable to init generic GPIO\n"); + gc->label = dev_name(dev); + gc->base = -1; /* Mask all GPIO interrupts */ for (i = 0; i < gc->ngpio; i++)
After adding a pinctrl flag to gpio-mmio we can use it for driving gpio-vf610. The existing code has the same semantics and the generic gpio-mmio, including reading from the data out register when the direction is set to input, and it can also handle the absence of the direction register better than the current driver: we get the direction from the shadow direction registers in gpio-mmio instead. Since gpio-mmio has an internal spinlock we can drop the direction-protecting spinlock from the driver. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v1->v2: - Use the dual input/output set/clear registers for output. - Provide the BGPIOF_READ_OUTPUT_REG_SET flag so the driver behaves as described in the commit message... - Drop the now unused spinlock (gpio-mmio has its own). --- drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-vf610.c | 109 ++++++++-------------------------------------- 2 files changed, 18 insertions(+), 92 deletions(-)