RE: [PATCH v2 2/2] gpio: vf610: Switch to gpio-mmio

[Bough Chen <haibo.chen@xxxxxxx>]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Sent: 2025年2月15日 7:13
> To: Johan Korsnes <johan.korsnes@xxxxxxxxxxxxx>; Bough Chen
> <haibo.chen@xxxxxxx>; Bartosz Golaszewski <brgl@xxxxxxxx>;
> imx@xxxxxxxxxxxxxxx
> Cc: linux-gpio@xxxxxxxxxxxxxxx; Linus Walleij <linus.walleij@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> 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