Date:   Wed, 29 Apr 2020 15:56:45 +0200
From:   Daniel Mack <daniel@...que.org>
To:     Srinivas Neeli <srinivas.neeli@...inx.com>,
        bgolaszewski@...libre.com, michal.simek@...inx.com,
        shubhrajyoti.datta@...inx.com, sgoud@...inx.com
Cc:     linus.walleij@...aro.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
        git@...inx.com
Subject: Re: [PATCH 2/2] gpio: xilinx: Add irq support to the driver

Hi Srinivas,

Thanks for these patches. We're using them on a custom board, and I have
some remarks as they didn't work as intended. See below.


On 2/17/20 11:57 AM, Srinivas Neeli wrote:
> Allocate single chip for both channels.
> Add irq support to the driver.
> Supporting rising edge interrupts and in cascade mode supporting
> first channel for interrupts on 32bit machines.
> 
> Signed-off-by: Srinivas Neeli <srinivas.neeli@...inx.com>
> ---
>  drivers/gpio/gpio-xilinx.c | 233 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 232 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index 26753ae58295..f6dd316b2c62 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c

[...]

>  /**
> + * xgpiops_irq_mask - Write the specified signal of the GPIO device.
> + * @irq_data: per irq and chip data passed down to chip functions
> + */
> +static void xgpio_irq_mask(struct irq_data *irq_data)
> +{
> +	unsigned long flags;
> +	struct xgpio_instance *chip = irq_data_get_irq_chip_data(irq_data);
> +	u32 offset = irq_data->irq - chip->irq_base;
> +	u32 temp;
> +	s32 val;
> +	int index = xgpio_index(chip, 0);
> +
> +	pr_debug("%s: Disable %d irq, irq_enable_mask 0x%x\n",
> +		 __func__, offset, chip->irq_enable);
> +
> +	spin_lock_irqsave(&chip->gpio_lock[index], flags);
> +
> +	chip->irq_enable &= ~BIT(offset);
> +
> +	if (!chip->irq_enable) {
> +		/* Enable per channel interrupt */
> +		temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
> +		val = offset - chip->gpio_width[0] + 1;
> +		if (val > 0)
> +			temp &= 1;
> +		else
> +			temp &= 2;


This is a bit confusing. Why not write

  if (offset <= chip->gpio_width[0])
	temp &= 1;
  else
        temp &= 2;

?

> +		xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
> +
> +		/* Disable global interrupt if channel interrupts are unused */
> +		temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);

You know that interrupts are unused when you get here, right? Why this
extra check?

> +		if (!temp)
> +			xgpio_writereg(chip->regs + XGPIO_GIER_OFFSET,
> +				       ~XGPIO_GIER_IE);
> +	}
> +	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
> +}
> +
> +/**
> + * xgpio_irq_unmask - Write the specified signal of the GPIO device.
> + * @irq_data: per irq and chip data passed down to chip functions
> + */
> +static void xgpio_irq_unmask(struct irq_data *irq_data)
> +{
> +	unsigned long flags;
> +	struct xgpio_instance *chip = irq_data_get_irq_chip_data(irq_data);
> +	u32 offset = irq_data->irq - chip->irq_base;
> +	u32 temp;
> +	s32 val;
> +	int index = xgpio_index(chip, 0);
> +
> +	pr_debug("%s: Enable %d irq, irq_enable_mask 0x%x\n",
> +		 __func__, offset, chip->irq_enable);
> +
> +	/* Setup pin as input */
> +	xgpio_dir_in(&chip->gc, offset);
> +
> +	spin_lock_irqsave(&chip->gpio_lock[index], flags);
> +
> +	chip->irq_enable |= BIT(offset);
> +
> +	if (chip->irq_enable) {

As you set a bit in the instruction above, this condition will always be
true. So I guess the check can be omitted.

> +		/* Enable per channel interrupt */
> +		temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
> +		val = offset - (chip->gpio_width[0] - 1);

This is different from the the statement in the mask function, but it
can be simplified as noted above.

> +		if (val > 0)
> +			temp |= 2;
> +		else
> +			temp |= 1;
> +		xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
> +
> +		/* Enable global interrupts */
> +		xgpio_writereg(chip->regs + XGPIO_GIER_OFFSET, XGPIO_GIER_IE);
> +	}
> +
> +	spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
> +}

[...]

> +/**
> + * xgpio_irqhandler - Gpio interrupt service routine
> + * @desc: Pointer to interrupt description
> + */
> +static void xgpio_irqhandler(struct irq_desc *desc)
> +{
> +	unsigned int irq = irq_desc_get_irq(desc);
> +	struct xgpio_instance *chip = (struct xgpio_instance *)
> +		irq_get_handler_data(irq);
> +	struct irq_chip *irqchip = irq_desc_get_chip(desc);
> +	u32 offset, status, channel = 1;
> +	unsigned long val;
> +
> +	chained_irq_enter(irqchip, desc);
> +
> +	val = xgpio_readreg(chip->regs);
> +	if (!val) {
> +		channel = 2;
> +		val = xgpio_readreg(chip->regs + XGPIO_CHANNEL_OFFSET);
> +		val = val << chip->gpio_width[0];
> +	}
> +
> +	/* Only rising edge is supported */
> +	val &= chip->irq_enable;
> +	for_each_set_bit(offset, &val, chip->gc.ngpio) {
> +		generic_handle_irq(chip->irq_base + offset);

This needs to include irq_find_mapping(chip->irq_domain, gpio).

> +	}
> +
> +	status = xgpio_readreg(chip->regs + XGPIO_IPISR_OFFSET);

The value assigned here is not used. Typo?

> +	xgpio_writereg(chip->regs + XGPIO_IPISR_OFFSET, channel);

This function causes issues of general IRQ handling that makes the
entire system deadlock for reasons I don't fully grok. I changed the
logic to the following to make it work:

1. Read IPISR
2. Write the read value back to IPISR
3. Depending on the value of IPISR, read the state of either channel 1
   or 2
4. chained_irq_enter()
5. Iterate over bits and call generic_handle_irq()
6. chained_irq_exit()

> +
> +	chained_irq_exit(irqchip, desc);
> +}
> +
> +static struct lock_class_key gpio_lock_class;
> +static struct lock_class_key gpio_request_class;
> +
> +/**
> + * xgpio_irq_setup - Allocate irq for gpio and setup appropriate functions
> + * @np: Device node of the GPIO chip
> + * @chip: Pointer to private gpio channel structure
> + *
> + * Return:
> + * 0 if success, otherwise -1
> + */
> +static int xgpio_irq_setup(struct device_node *np, struct xgpio_instance *chip)
> +{
> +	u32 pin_num;
> +	struct resource res;
> +	int ret = of_irq_to_resource(np, 0, &res);
> +
> +	if (ret <= 0) {
> +		pr_info("GPIO IRQ not connected\n");
> +		return 0;
> +	}
> +
> +	chip->gc.to_irq = xgpio_to_irq;
> +	chip->irq_base = irq_alloc_descs(-1, 0, chip->gc.ngpio, 0);

This should use the devm_ variant to automatically free the resources.

> +	if (chip->irq_base < 0) {
> +		pr_err("Couldn't allocate IRQ numbers\n");
> +		return -1;
> +	}
> +	chip->irq_domain = irq_domain_add_legacy(np, chip->gc.ngpio,
> +						 chip->irq_base, 0,
> +						 &irq_domain_simple_ops, NULL);

This can fail, so the return value should be checked for NULL.

> +	/*
> +	 * set the irq chip, handler and irq chip data for callbacks for
> +	 * each pin
> +	 */
> +	for (pin_num = 0; pin_num < chip->gc.ngpio; pin_num++) {
> +		u32 gpio_irq = irq_find_mapping(chip->irq_domain, pin_num);
> +
> +		irq_set_lockdep_class(gpio_irq, &gpio_lock_class,
> +				      &gpio_request_class);
> +		pr_debug("IRQ Base: %d, Pin %d = IRQ %d\n",
> +			 chip->irq_base, pin_num, gpio_irq);
> +		irq_set_chip_and_handler(gpio_irq, &xgpio_irqchip,
> +					 handle_simple_irq);
> +		irq_set_chip_data(gpio_irq, (void *)chip);
> +	}
> +	irq_set_handler_data(res.start, (void *)chip);
> +	irq_set_chained_handler(res.start, xgpio_irqhandler);

I guess all this can be achieved by setting chip->gc.irq* and let the
GPIO core handle the IRQ chip allocation and setup. There are some
examples in Documentation/driver-api/gpio/driver.rst.

I'm happy to test the next iteration of these patches.


Thanks,
Daniel

Powered by blists - more mailing lists