* Re: [PATCH V5 3/5] gpio: gpio-xilinx: Add interrupt support
[not found] ` <1611930410-25747-4-git-send-email-srinivas.neeli@xilinx.com>
@ 2021-01-30 0:34 ` Robert Hancock
2021-02-02 19:36 ` Linus Walleij
1 sibling, 0 replies; 5+ messages in thread
From: Robert Hancock @ 2021-01-30 0:34 UTC (permalink / raw)
To: vilhelm.gray, srinivas.neeli, linus.walleij, michal.simek, sgoud,
shubhrajyoti.datta, syednwaris, bgolaszewski
Cc: linux-arm-kernel, linux-kernel, linux-gpio, git, hancock
Noticed one issue, see below:
On Fri, 2021-01-29 at 19:56 +0530, Srinivas Neeli wrote:
> Adds interrupt support to the Xilinx GPIO driver so that rising and
> falling edge line events can be supported. Since interrupt support is
> an optional feature in the Xilinx IP, the driver continues to support
> devices which have no interrupt provided.
> Depends on OF_GPIO framework for of_xlate function to translate
> gpiospec to the GPIO number and flags.
>
> Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com>
> ---
> Changes in V5:
> -Removed IRQ_DOMAIN_HIERARCHY from Kconfig and
> #include <linux/of_gpio.h> from includes.
> -Fixed "detected irqchip that is shared with multiple
> gpiochips: please fix the driver"error message.
> -Added check for #gpio-cells and error message in failure case.
> Changes in V4:
> -Added more commit description.
> Changes in V3:
> -Created separate patch for Clock changes and runtime resume
> and suspend.
> -Updated minor review comments.
>
> Changes in V2:
> -Added check for return value of platform_get_irq() API.
> -Updated code to support rising edge and falling edge.
> -Added xgpio_xlate() API to support switch.
> -Added MAINTAINERS fragment.
> ---
> drivers/gpio/Kconfig | 2 +
> drivers/gpio/gpio-xilinx.c | 246
> ++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 244 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index c70f46e80a3b..2ee57797d908 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -690,6 +690,8 @@ config GPIO_XGENE_SB
>
> config GPIO_XILINX
> tristate "Xilinx GPIO support"
> + select GPIOLIB_IRQCHIP
> + depends on OF_GPIO
> help
> Say yes here to support the Xilinx FPGA GPIO device
>
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index f88db56543c2..62deb755f910 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -10,7 +10,9 @@
> #include <linux/errno.h>
> #include <linux/gpio/driver.h>
> #include <linux/init.h>
> +#include <linux/interrupt.h>
> #include <linux/io.h>
> +#include <linux/irq.h>
> #include <linux/module.h>
> #include <linux/of_device.h>
> #include <linux/of_platform.h>
> @@ -22,6 +24,11 @@
>
> #define XGPIO_CHANNEL_OFFSET 0x8
>
> +#define XGPIO_GIER_OFFSET 0x11c /* Global Interrupt Enable */
> +#define XGPIO_GIER_IE BIT(31)
> +#define XGPIO_IPISR_OFFSET 0x120 /* IP Interrupt Status */
> +#define XGPIO_IPIER_OFFSET 0x128 /* IP Interrupt Enable */
> +
> /* Read/Write access to the GPIO registers */
> #if defined(CONFIG_ARCH_ZYNQ) || defined(CONFIG_X86)
> # define xgpio_readreg(offset) readl(offset)
> @@ -36,9 +43,15 @@
> * @gc: GPIO chip
> * @regs: register block
> * @gpio_width: GPIO width for every channel
> - * @gpio_state: GPIO state shadow register
> + * @gpio_state: GPIO write state shadow register
> + * @gpio_last_irq_read: GPIO read state register from last interrupt
> * @gpio_dir: GPIO direction shadow register
> * @gpio_lock: Lock used for synchronization
> + * @irq: IRQ used by GPIO device
> + * @irqchip: IRQ chip
> + * @irq_enable: GPIO IRQ enable/disable bitfield
> + * @irq_rising_edge: GPIO IRQ rising edge enable/disable bitfield
> + * @irq_falling_edge: GPIO IRQ falling edge enable/disable bitfield
> * @clk: clock resource for this driver
> */
> struct xgpio_instance {
> @@ -46,8 +59,14 @@ struct xgpio_instance {
> void __iomem *regs;
> unsigned int gpio_width[2];
> u32 gpio_state[2];
> + u32 gpio_last_irq_read[2];
> u32 gpio_dir[2];
> spinlock_t gpio_lock; /* For serializing operations */
> + int irq;
> + struct irq_chip irqchip;
> + u32 irq_enable[2];
> + u32 irq_rising_edge[2];
> + u32 irq_falling_edge[2];
> struct clk *clk;
> };
>
> @@ -277,6 +296,175 @@ static int xgpio_remove(struct platform_device *pdev)
> }
>
> /**
> + * xgpio_irq_ack - Acknowledge a child GPIO interrupt.
> + * @irq_data: per IRQ and chip data passed down to chip functions
> + * This currently does nothing, but irq_ack is unconditionally called by
> + * handle_edge_irq and therefore must be defined.
> + */
> +static void xgpio_irq_ack(struct irq_data *irq_data)
> +{
> +}
> +
> +/**
> + * xgpio_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);
> + int irq_offset = irqd_to_hwirq(irq_data);
> + int index = xgpio_index(chip, irq_offset);
> + int offset = xgpio_offset(chip, irq_offset);
> +
> + spin_lock_irqsave(&chip->gpio_lock, flags);
> +
> + chip->irq_enable[index] &= ~BIT(offset);
> +
> + if (!chip->irq_enable[index]) {
> + /* Disable per channel interrupt */
> + u32 temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
> +
> + temp &= ~BIT(index);
> + xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
> + }
> + spin_unlock_irqrestore(&chip->gpio_lock, 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);
> + int irq_offset = irqd_to_hwirq(irq_data);
> + int index = xgpio_index(chip, irq_offset);
> + int offset = xgpio_offset(chip, irq_offset);
> + u32 old_enable = chip->irq_enable[index];
> +
> + spin_lock_irqsave(&chip->gpio_lock, flags);
> +
> + chip->irq_enable[index] |= BIT(offset);
> +
> + if (!old_enable) {
> + /* Clear any existing per-channel interrupts */
> + u32 val = xgpio_readreg(chip->regs + XGPIO_IPISR_OFFSET) &
> + BIT(index);
> +
> + if (val)
> + xgpio_writereg(chip->regs + XGPIO_IPISR_OFFSET, val);
> +
> + /* Update GPIO IRQ read data before enabling interrupt*/
> + val = xgpio_readreg(chip->regs + XGPIO_DATA_OFFSET +
> + index * XGPIO_CHANNEL_OFFSET);
> + chip->gpio_last_irq_read[index] = val;
> +
> + /* Enable per channel interrupt */
> + val = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
> + val |= BIT(index);
> + xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, val);
> + }
> +
> + spin_unlock_irqrestore(&chip->gpio_lock, flags);
> +}
> +
> +/**
> + * xgpio_set_irq_type - Write the specified signal of the GPIO device.
> + * @irq_data: Per IRQ and chip data passed down to chip functions
> + * @type: Interrupt type that is to be set for the gpio pin
> + *
> + * Return:
> + * 0 if interrupt type is supported otherwise -EINVAL
> + */
> +static int xgpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
> +{
> + struct xgpio_instance *chip = irq_data_get_irq_chip_data(irq_data);
> + int irq_offset = irqd_to_hwirq(irq_data);
> + int index = xgpio_index(chip, irq_offset);
> + int offset = xgpio_offset(chip, irq_offset);
> +
> + /*
> + * The Xilinx GPIO hardware provides a single interrupt status
> + * indication for any state change in a given GPIO channel (bank).
> + * Therefore, only rising edge or falling edge triggers are
> + * supported.
> + */
> + switch (type & IRQ_TYPE_SENSE_MASK) {
> + case IRQ_TYPE_EDGE_BOTH:
> + chip->irq_rising_edge[index] |= BIT(offset);
> + chip->irq_falling_edge[index] |= BIT(offset);
> + break;
> + case IRQ_TYPE_EDGE_RISING:
> + chip->irq_rising_edge[index] |= BIT(offset);
> + chip->irq_falling_edge[index] &= ~BIT(offset);
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + chip->irq_rising_edge[index] &= ~BIT(offset);
> + chip->irq_falling_edge[index] |= BIT(offset);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + irq_set_handler_locked(irq_data, handle_edge_irq);
> + return 0;
> +}
> +
> +/**
> + * xgpio_irqhandler - Gpio interrupt service routine
> + * @desc: Pointer to interrupt description
> + */
> +static void xgpio_irqhandler(struct irq_desc *desc)
> +{
> + struct xgpio_instance *chip = irq_desc_get_handler_data(desc);
> + struct irq_chip *irqchip = irq_desc_get_chip(desc);
> + u32 num_channels = chip->gpio_width[1] ? 2 : 1;
> + u32 offset = 0, index;
> + u32 status = xgpio_readreg(chip->regs + XGPIO_IPISR_OFFSET);
> +
> + xgpio_writereg(chip->regs + XGPIO_IPISR_OFFSET, status);
> +
> + chained_irq_enter(irqchip, desc);
> + for (index = 0; index < num_channels; index++) {
> + if ((status & BIT(index))) {
> + unsigned long rising_events, falling_events,
> all_events;
> + unsigned long flags;
> + u32 data, bit;
> + unsigned int irq;
> +
> + spin_lock_irqsave(&chip->gpio_lock, flags);
> + data = xgpio_readreg(chip->regs + XGPIO_DATA_OFFSET +
> + index * XGPIO_CHANNEL_OFFSET);
> + rising_events = data &
> + ~chip->gpio_last_irq_read[index] &
> + chip->irq_enable[index] &
> + chip->irq_rising_edge[index];
> + falling_events = ~data &
> + chip->gpio_last_irq_read[index] &
> + chip->irq_enable[index] &
> + chip->irq_falling_edge[index];
> + dev_dbg(chip->gc.parent,
> + "IRQ chan %u rising 0x%lx falling 0x%lx\n",
> + index, rising_events, falling_events);
> + all_events = rising_events | falling_events;
> + chip->gpio_last_irq_read[index] = data;
> + spin_unlock_irqrestore(&chip->gpio_lock, flags);
> +
> + for_each_set_bit(bit, &all_events, 32) {
> + irq = irq_find_mapping(chip->gc.irq.domain,
> + offset + bit);
> + generic_handle_irq(irq);
> + }
> + }
> + offset += chip->gpio_width[index];
> + }
> +
> + chained_irq_exit(irqchip, desc);
> +}
> +
> +/**
> * xgpio_of_probe - Probe method for the GPIO device.
> * @pdev: pointer to the platform device
> *
> @@ -289,7 +477,10 @@ static int xgpio_probe(struct platform_device *pdev)
> struct xgpio_instance *chip;
> int status = 0;
> struct device_node *np = pdev->dev.of_node;
> - u32 is_dual;
> + u32 is_dual = 0;
> + u32 cells = 2;
> + struct gpio_irq_chip *girq;
> + u32 temp;
>
> chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> if (!chip)
> @@ -305,6 +496,15 @@ static int xgpio_probe(struct platform_device *pdev)
> if (of_property_read_u32(np, "xlnx,tri-default", &chip->gpio_dir[0]))
> chip->gpio_dir[0] = 0xFFFFFFFF;
>
> + /* Update cells with gpio-cells value */
> + if (of_property_read_u32(np, "#gpio-cells", &cells))
> + dev_dbg(&pdev->dev, "Missing gpio-cells property\n");
> +
> + if (cells != 2) {
> + dev_err(&pdev->dev, "#gpio-cells mismatch\n");
> + return -EINVAL;
> + }
> +
> /*
> * Check device node and parent device node for device width
> * and assume default width of 32
> @@ -343,6 +543,7 @@ static int xgpio_probe(struct platform_device *pdev)
> chip->gc.parent = &pdev->dev;
> chip->gc.direction_input = xgpio_dir_in;
> chip->gc.direction_output = xgpio_dir_out;
> + chip->gc.of_gpio_n_cells = cells;
> chip->gc.get = xgpio_get;
> chip->gc.set = xgpio_set;
> chip->gc.set_multiple = xgpio_set_multiple;
> @@ -367,14 +568,51 @@ static int xgpio_probe(struct platform_device *pdev)
>
> xgpio_save_regs(chip);
>
> + chip->irq = platform_get_irq_optional(pdev, 0);
> + if (chip->irq <= 0)
> + goto skip_irq;
> +
> + chip->irqchip.name = "gpio-xilinx";
> + chip->irqchip.irq_ack = xgpio_irq_ack;
> + chip->irqchip.irq_mask = xgpio_irq_mask;
> + chip->irqchip.irq_unmask = xgpio_irq_unmask;
> + chip->irqchip.irq_set_type = xgpio_set_irq_type;
> +
> + /* Disable per-channel interrupts */
> + xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, 0);
> + /* Clear any existing per-channel interrupts */
> + temp = xgpio_readreg(chip->regs + XGPIO_IPISR_OFFSET);
> + xgpio_writereg(chip->regs + XGPIO_IPISR_OFFSET, temp);
> + /* Enable global interrupts */
> + xgpio_writereg(chip->regs + XGPIO_GIER_OFFSET, XGPIO_GIER_IE);
> +
> + girq = &chip->gc.irq;
> + girq->chip = &chip->irqchip;
> + girq->parent_handler = xgpio_irqhandler;
I think you want to also set "girq->parent_handler_data = chip;" here. The
original version of this patch did that, but this may have gotten lost when it
was changed to a per-instance irqchip.
It appears that if parent_handler_data is not set, gpiochip_add_irqchip uses
the gpio_chip pointer as the data when calling the parent IRQ handler.
xgpio_irqhandler then converts this pointer to xgpio_instance* when it is
called. Since the gpio_chip is currently the first member of the xgpio_instance
structure, the pointers end up the same and this would currently work. However,
it may be unwise to depend on this, as changing the order of members in
xgpio_instance would unexpectedly break the code.
> + girq->num_parents = 1;
> + girq->parents = devm_kcalloc(&pdev->dev, 1,
> + sizeof(*girq->parents),
> + GFP_KERNEL);
> + if (!girq->parents) {
> + status = -ENOMEM;
> + goto err_unprepare_clk;
> + }
> + girq->parents[0] = chip->irq;
> + girq->default_type = IRQ_TYPE_NONE;
> + girq->handler = handle_bad_irq;
> +
> +skip_irq:
> status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip);
> if (status) {
> dev_err(&pdev->dev, "failed to add GPIO chip\n");
> - clk_disable_unprepare(chip->clk);
> - return status;
> + goto err_unprepare_clk;
> }
>
> return 0;
> +
> +err_unprepare_clk:
> + clk_disable_unprepare(chip->clk);
> + return status;
> }
>
> static const struct of_device_id xgpio_of_match[] = {
--
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com
^ permalink raw reply [flat|nested] 5+ messages in thread