2020-11-12 17:15:04

by Srinivas Neeli

[permalink] [raw]
Subject: [LINUX PATCH V3 5/9] gpio: gpio-xilinx: Add interrupt support

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.

Signed-off-by: Robert Hancock <[email protected]>
Signed-off-by: Shubhrajyoti Datta <[email protected]>
Signed-off-by: Srinivas Neeli <[email protected]>
---
Chnages 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 | 242 ++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 240 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 5d4de5cd6759..cf4959891eab 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -677,6 +677,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 69bdf1910215..855550a06ded 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -10,9 +10,12 @@
#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_gpio.h>
#include <linux/of_platform.h>
#include <linux/slab.h>

@@ -22,6 +25,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 +44,14 @@
* @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
+ * @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,13 @@ 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;
+ u32 irq_enable[2];
+ u32 irq_rising_edge[2];
+ u32 irq_falling_edge[2];
struct clk *clk;
};

@@ -258,6 +276,183 @@ static void xgpio_save_regs(struct xgpio_instance *chip)
}

/**
+ * 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;
+}
+
+static struct irq_chip xgpio_irqchip = {
+ .name = "gpio-xilinx",
+ .irq_ack = xgpio_irq_ack,
+ .irq_mask = xgpio_irq_mask,
+ .irq_unmask = xgpio_irq_unmask,
+ .irq_set_type = xgpio_set_irq_type,
+};
+
+/**
+ * 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
*
@@ -270,7 +465,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)
@@ -285,6 +483,10 @@ 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");
+
/*
* Check device node and parent device node for device width
* and assume default width of 32
@@ -321,6 +523,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;
@@ -348,14 +551,45 @@ 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;
+
+ /* 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 = &xgpio_irqchip;
+ girq->parent_handler = xgpio_irqhandler;
+ 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[] = {
--
2.7.4


2020-11-16 16:50:10

by Robert Hancock

[permalink] [raw]
Subject: Re: [LINUX PATCH V3 5/9] gpio: gpio-xilinx: Add interrupt support

On Thu, 2020-11-12 at 22:42 +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.
>
> Signed-off-by: Robert Hancock <[email protected]>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> Signed-off-by: Srinivas Neeli <[email protected]>
> ---
> Chnages 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 | 242
> ++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 240 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 5d4de5cd6759..cf4959891eab 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -677,6 +677,8 @@ config GPIO_XGENE_SB
>
> config GPIO_XILINX
> tristate "Xilinx GPIO support"
> + select GPIOLIB_IRQCHIP
> + depends on OF_GPIO

This OF_GPIO dependency was previously removed - is this required? It
appears the code is now setting of_gpio_n_cells but I am not sure if
this is necessary or helpful since the other of_gpio functions are not
used.

> 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 69bdf1910215..855550a06ded 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -10,9 +10,12 @@
> #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_gpio.h>
> #include <linux/of_platform.h>
> #include <linux/slab.h>
>
> @@ -22,6 +25,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 +44,14 @@
> * @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
> + * @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,13 @@ 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;
> + u32 irq_enable[2];
> + u32 irq_rising_edge[2];
> + u32 irq_falling_edge[2];
> struct clk *clk;
> };
>
> @@ -258,6 +276,183 @@ static void xgpio_save_regs(struct
> xgpio_instance *chip)
> }
>
> /**
> + * 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;
> +}
> +
> +static struct irq_chip xgpio_irqchip = {
> + .name = "gpio-xilinx",
> + .irq_ack = xgpio_irq_ack,
> + .irq_mask = xgpio_irq_mask,
> + .irq_unmask = xgpio_irq_unmask,
> + .irq_set_type = xgpio_set_irq_type,
> +};
> +
> +/**
> + * 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
> *
> @@ -270,7 +465,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)
> @@ -285,6 +483,10 @@ 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");
> +
> /*
> * Check device node and parent device node for device width
> * and assume default width of 32
> @@ -321,6 +523,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;
> @@ -348,14 +551,45 @@ 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;
> +
> + /* 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 = &xgpio_irqchip;
> + girq->parent_handler = xgpio_irqhandler;
> + 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
http://www.calian.com

2020-11-18 00:18:40

by Linus Walleij

[permalink] [raw]
Subject: Re: [LINUX PATCH V3 5/9] gpio: gpio-xilinx: Add interrupt support

Hi Srinivas!

On Thu, Nov 12, 2020 at 6:12 PM Srinivas Neeli
<[email protected]> 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.
>
> Signed-off-by: Robert Hancock <[email protected]>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> Signed-off-by: Srinivas Neeli <[email protected]>

(...)
> 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

Please add:
select IRQ_DOMAIN_HIERARCHY

Because your driver requires this.

> + /* Update cells with gpio-cells value */
> + if (of_property_read_u32(np, "#gpio-cells", &cells))
> + dev_dbg(&pdev->dev, "Missing gpio-cells property\n");
(...)
> + chip->gc.of_gpio_n_cells = cells;

Why is this necessary?
Mention in the commit.

Other than that this looks very good and good use
of the hierarchical IRQ feature in gpiolib!

Yours,
Linus Walleij