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 <[email protected]>
---
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
@@ -16,6 +16,11 @@
#include <linux/slab.h>
#include <linux/pm_runtime.h>
#include <linux/clk.h>
+#include <linux/of_irq.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
/* Register Offset Definitions */
#define XGPIO_DATA_OFFSET (0x0) /* Data register */
@@ -23,8 +28,13 @@
#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)
+#if defined(CONFIG_ARCH_ZYNQ) || defined(CONFIG_X86) || defined(CONFIG_ARM64)
# define xgpio_readreg(offset) readl(offset)
# define xgpio_writereg(offset, val) writel(val, offset)
#else
@@ -41,7 +51,11 @@
* @gpio_dir: GPIO direction shadow register
* @gpio_lock: Lock used for synchronization
* @clk: clock resource for this driver
+ * @irq_base: GPIO channel irq base address
+ * @irq_enable: GPIO irq enable/disable bitfield
+ * @irq_domain: irq_domain of the controller
*/
+
struct xgpio_instance {
struct gpio_chip gc;
void __iomem *regs;
@@ -50,6 +64,9 @@ struct xgpio_instance {
u32 gpio_dir[2];
spinlock_t gpio_lock[2]; /* For serializing operations */
struct clk *clk;
+ int irq_base;
+ u32 irq_enable;
+ struct irq_domain *irq_domain;
};
static inline int xgpio_index(struct xgpio_instance *chip, int gpio)
@@ -324,6 +341,211 @@ static const struct dev_pm_ops xgpio_dev_pm_ops = {
};
/**
+ * 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;
+ xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
+
+ /* Disable global interrupt if channel interrupts are unused */
+ temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
+ 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) {
+ /* Enable per channel interrupt */
+ temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
+ val = offset - (chip->gpio_width[0] - 1);
+ 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_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 otherwise -EINVAL
+ */
+static int xgpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
+{
+ /* Only rising edge case is supported now */
+ if (type & IRQ_TYPE_EDGE_RISING)
+ return 0;
+
+ return -EINVAL;
+}
+
+/* irq chip descriptor */
+static struct irq_chip xgpio_irqchip = {
+ .name = "xgpio",
+ .irq_mask = xgpio_irq_mask,
+ .irq_unmask = xgpio_irq_unmask,
+ .irq_set_type = xgpio_set_irq_type,
+};
+
+/**
+ * xgpio_to_irq - Find out gpio to Linux irq mapping
+ * @gc: Pointer to gpio_chip device structure.
+ * @offset: Gpio pin offset
+ *
+ * Return:
+ * irq number otherwise -EINVAL
+ */
+static int xgpio_to_irq(struct gpio_chip *gc, unsigned int offset)
+{
+ struct xgpio_instance *chip = gpiochip_get_data(gc);
+
+ return irq_find_mapping(chip->irq_domain, offset);
+}
+
+/**
+ * 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);
+ }
+
+ status = xgpio_readreg(chip->regs + XGPIO_IPISR_OFFSET);
+ xgpio_writereg(chip->regs + XGPIO_IPISR_OFFSET, channel);
+
+ 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);
+ 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);
+ /*
+ * 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);
+
+ return 0;
+}
+
+/**
* xgpio_of_probe - Probe method for the GPIO device.
* @pdev: pointer to the platform device
*
@@ -434,6 +656,15 @@ static int xgpio_probe(struct platform_device *pdev)
goto err_pm_put;
}
+ status = xgpio_irq_setup(np, chip);
+ if (status) {
+ pr_err("%s: GPIO IRQ initialization failed %d\n",
+ np->full_name, status);
+ goto err_pm_put;
+ }
+ pr_info("XGpio: %s: registered, base is %d\n", np->full_name,
+ chip->gc.base);
+
pm_runtime_put(&pdev->dev);
return 0;
err_pm_put:
--
2.7.4
Hi Srinivas,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.6-rc2]
[also build test ERROR on next-20200219]
[cannot apply to xlnx/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Srinivas-Neeli/gpio-xilinx-Add-clock-adaptation-support/20200219-110158
base: 11a48a5a18c63fd7621bb050228cebf13566e4d8
config: x86_64-randconfig-b001-20200219 (attached as .config)
compiler: gcc-7 (Debian 7.5.0-5) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
>> ERROR: "of_irq_to_resource" [drivers/gpio/gpio-xilinx.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Srinivas,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.6-rc2]
[also build test ERROR on next-20200219]
[cannot apply to xlnx/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Srinivas-Neeli/gpio-xilinx-Add-clock-adaptation-support/20200219-110158
base: 11a48a5a18c63fd7621bb050228cebf13566e4d8
config: i386-randconfig-b003-20200219 (attached as .config)
compiler: gcc-7 (Debian 7.5.0-5) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
ld: drivers/gpio/gpio-xilinx.o: in function `xgpio_irq_setup':
>> drivers/gpio/gpio-xilinx.c:512: undefined reference to `of_irq_to_resource'
vim +512 drivers/gpio/gpio-xilinx.c
499
500 /**
501 * xgpio_irq_setup - Allocate irq for gpio and setup appropriate functions
502 * @np: Device node of the GPIO chip
503 * @chip: Pointer to private gpio channel structure
504 *
505 * Return:
506 * 0 if success, otherwise -1
507 */
508 static int xgpio_irq_setup(struct device_node *np, struct xgpio_instance *chip)
509 {
510 u32 pin_num;
511 struct resource res;
> 512 int ret = of_irq_to_resource(np, 0, &res);
513
514 if (ret <= 0) {
515 pr_info("GPIO IRQ not connected\n");
516 return 0;
517 }
518
519 chip->gc.to_irq = xgpio_to_irq;
520 chip->irq_base = irq_alloc_descs(-1, 0, chip->gc.ngpio, 0);
521 if (chip->irq_base < 0) {
522 pr_err("Couldn't allocate IRQ numbers\n");
523 return -1;
524 }
525 chip->irq_domain = irq_domain_add_legacy(np, chip->gc.ngpio,
526 chip->irq_base, 0,
527 &irq_domain_simple_ops, NULL);
528 /*
529 * set the irq chip, handler and irq chip data for callbacks for
530 * each pin
531 */
532 for (pin_num = 0; pin_num < chip->gc.ngpio; pin_num++) {
533 u32 gpio_irq = irq_find_mapping(chip->irq_domain, pin_num);
534
535 irq_set_lockdep_class(gpio_irq, &gpio_lock_class,
536 &gpio_request_class);
537 pr_debug("IRQ Base: %d, Pin %d = IRQ %d\n",
538 chip->irq_base, pin_num, gpio_irq);
539 irq_set_chip_and_handler(gpio_irq, &xgpio_irqchip,
540 handle_simple_irq);
541 irq_set_chip_data(gpio_irq, (void *)chip);
542 }
543 irq_set_handler_data(res.start, (void *)chip);
544 irq_set_chained_handler(res.start, xgpio_irqhandler);
545
546 return 0;
547 }
548
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
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 <[email protected]>
> ---
> 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