[PATCH v5 0/2] gpio: sch: Interrupt support

[PATCH v5 0/2] gpio: sch: Interrupt support

[Andy Shevchenko]

The series adds event support to the Intel GPIO SCH driver. The hardware
routes all events through GPE0 GPIO event.

I validated this on Intel Minnowboard (v1).

If somebody has different hardware with the same GPIO controller, I would
appreciate additional testing.

Changes in v5:
- added missed IRQ acknowledge callback (hence kernel Oops)
- rewrite patch 2 completely from SCI to GPE hook

Changes in v4 (https://lore.kernel.org/linux-gpio/20210316162613.87710-1-andriy.shevchenko@linux.intel.com/T/#u):
- turned to GPIO core infrastructure of IRQ chip instantiation (Linus)
- converted IRQ callbacks to use better APIs
- use handle_bad_irq() as default handler and now I know why, see
  eb441337c714 ("gpio: pca953x: Set IRQ type when handle Intel Galileo Gen 2")
    for the real example what happens if it's preset to something meaningful
- fixed remove stage (we have to remove SCI handler, which wasn't done in v3)

Changes in v3 (https://lore.kernel.org/linux-gpio/cover.1574277614.git.jan.kiszka@siemens.com/T/#u):
- split-up of the irq enabling patch as requested by Andy

Andy Shevchenko (1):
  gpio: sch: Hook into ACPI GPE handler to catch GPIO edge events

Jan Kiszka (1):
  gpio: sch: Add edge event support

 drivers/gpio/gpio-sch.c | 196 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 188 insertions(+), 8 deletions(-)

-- 
2.30.2

[PATCH v5 1/2] gpio: sch: Add edge event support

[Andy Shevchenko]

From: Jan Kiszka <jan.kiszka@siemens.com>

Add the required infrastructure to enable and report edge events
of the pins to the GPIO core. The actual hook-up of the event interrupt
will happen separately.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-sch.c | 114 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 106 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 3a1b1adb08c6..5e08e26d0b86 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -10,17 +10,28 @@
 #include <linux/errno.h>
 #include <linux/gpio/driver.h>
 #include <linux/io.h>
+#include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci_ids.h>
 #include <linux/platform_device.h>
+#include <linux/types.h>
 
 #define GEN	0x00
 #define GIO	0x04
 #define GLV	0x08
+#define GTPE	0x0c
+#define GTNE	0x10
+#define GGPE	0x14
+#define GSMI	0x18
+#define GTS	0x1c
+
+#define CORE_BANK_OFFSET	0x00
+#define RESUME_BANK_OFFSET	0x20
 
 struct sch_gpio {
 	struct gpio_chip chip;
+	struct irq_chip irqchip;
 	spinlock_t lock;
 	unsigned short iobase;
 	unsigned short resume_base;
@@ -29,11 +40,11 @@ struct sch_gpio {
 static unsigned int sch_gpio_offset(struct sch_gpio *sch, unsigned int gpio,
 				unsigned int reg)
 {
-	unsigned int base = 0;
+	unsigned int base = CORE_BANK_OFFSET;
 
 	if (gpio >= sch->resume_base) {
 		gpio -= sch->resume_base;
-		base += 0x20;
+		base = RESUME_BANK_OFFSET;
 	}
 
 	return base + reg + gpio / 8;
@@ -79,10 +90,11 @@ static void sch_gpio_reg_set(struct sch_gpio *sch, unsigned int gpio, unsigned i
 static int sch_gpio_direction_in(struct gpio_chip *gc, unsigned int gpio_num)
 {
 	struct sch_gpio *sch = gpiochip_get_data(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GIO, 1);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 	return 0;
 }
 
@@ -96,20 +108,22 @@ static int sch_gpio_get(struct gpio_chip *gc, unsigned int gpio_num)
 static void sch_gpio_set(struct gpio_chip *gc, unsigned int gpio_num, int val)
 {
 	struct sch_gpio *sch = gpiochip_get_data(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GLV, val);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 }
 
 static int sch_gpio_direction_out(struct gpio_chip *gc, unsigned int gpio_num,
 				  int val)
 {
 	struct sch_gpio *sch = gpiochip_get_data(gc);
+	unsigned long flags;
 
-	spin_lock(&sch->lock);
+	spin_lock_irqsave(&sch->lock, flags);
 	sch_gpio_reg_set(sch, gpio_num, GIO, 0);
-	spin_unlock(&sch->lock);
+	spin_unlock_irqrestore(&sch->lock, flags);
 
 	/*
 	 * according to the datasheet, writing to the level register has no
@@ -144,6 +158,77 @@ static const struct gpio_chip sch_gpio_chip = {
 	.get_direction		= sch_gpio_get_direction,
 };
 
+static int sch_irq_type(struct irq_data *d, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct sch_gpio *sch = gpiochip_get_data(gc);
+	irq_hw_number_t gpio_num = irqd_to_hwirq(d);
+	unsigned long flags;
+	int rising, falling;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		rising = 1;
+		falling = 0;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		rising = 0;
+		falling = 1;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		rising = 1;
+		falling = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	spin_lock_irqsave(&sch->lock, flags);
+
+	sch_gpio_reg_set(sch, gpio_num, GTPE, rising);
+	sch_gpio_reg_set(sch, gpio_num, GTNE, falling);
+
+	irq_set_handler_locked(d, handle_edge_irq);
+
+	spin_unlock_irqrestore(&sch->lock, flags);
+
+	return 0;
+}
+
+static void sch_irq_ack(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct sch_gpio *sch = gpiochip_get_data(gc);
+	irq_hw_number_t gpio_num = irqd_to_hwirq(d);
+	unsigned long flags;
+
+	spin_lock_irqsave(&sch->lock, flags);
+	sch_gpio_reg_set(sch, gpio_num, GTS, 1);
+	spin_unlock_irqrestore(&sch->lock, flags);
+}
+
+static void sch_irq_mask_unmask(struct irq_data *d, int val)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct sch_gpio *sch = gpiochip_get_data(gc);
+	irq_hw_number_t gpio_num = irqd_to_hwirq(d);
+	unsigned long flags;
+
+	spin_lock_irqsave(&sch->lock, flags);
+	sch_gpio_reg_set(sch, gpio_num, GGPE, val);
+	spin_unlock_irqrestore(&sch->lock, flags);
+}
+
+static void sch_irq_mask(struct irq_data *d)
+{
+	sch_irq_mask_unmask(d, 0);
+}
+
+static void sch_irq_unmask(struct irq_data *d)
+{
+	sch_irq_mask_unmask(d, 1);
+}
+
 static int sch_gpio_probe(struct platform_device *pdev)
 {
 	struct sch_gpio *sch;
@@ -207,6 +292,19 @@ static int sch_gpio_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sch);
 
+	sch->irqchip.name = "sch_gpio";
+	sch->irqchip.irq_ack = sch_irq_ack;
+	sch->irqchip.irq_mask = sch_irq_mask;
+	sch->irqchip.irq_unmask = sch_irq_unmask;
+	sch->irqchip.irq_set_type = sch_irq_type;
+
+	sch->chip.irq.chip = &sch->irqchip;
+	sch->chip.irq.num_parents = 0;
+	sch->chip.irq.parents = NULL;
+	sch->chip.irq.parent_handler = NULL;
+	sch->chip.irq.default_type = IRQ_TYPE_NONE;
+	sch->chip.irq.handler = handle_bad_irq;
+
 	return devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
 }
 
-- 
2.30.2

[PATCH v5 2/2] gpio: sch: Hook into ACPI GPE handler to catch GPIO edge events

[Andy Shevchenko]

Neither the ACPI description on Intel Minnowboard (v1) platform provides
the required information to establish a generic handling nor the hardware
capable of doing it. According to the data sheet the hardware can generate
SCI events. Therefore, we need to hook from the driver into GPE handler of
the ACPI subsystem in order to catch and report GPIO-related events.

Validated on the Inlel Minnowboard (v1) platform.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-sch.c | 82 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 5e08e26d0b86..f043ae9982bd 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/bitops.h>
 #include <linux/errno.h>
 #include <linux/gpio/driver.h>
 #include <linux/io.h>
@@ -29,12 +30,22 @@
 #define CORE_BANK_OFFSET	0x00
 #define RESUME_BANK_OFFSET	0x20
 
+/*
+ * iLB datasheet describes GPE0BLK registers, in particular GPE0E.GPIO bit.
+ * Document Number: 328195-001
+ */
+#define GPE0E_GPIO	14
+
 struct sch_gpio {
 	struct gpio_chip chip;
 	struct irq_chip irqchip;
 	spinlock_t lock;
 	unsigned short iobase;
 	unsigned short resume_base;
+
+	/* GPE handling */
+	u32 gpe;
+	acpi_gpe_handler gpe_handler;
 };
 
 static unsigned int sch_gpio_offset(struct sch_gpio *sch, unsigned int gpio,
@@ -229,10 +240,73 @@ static void sch_irq_unmask(struct irq_data *d)
 	sch_irq_mask_unmask(d, 1);
 }
 
+static u32 sch_gpio_gpe_handler(acpi_handle gpe_device, u32 gpe, void *context)
+{
+	struct sch_gpio *sch = context;
+	struct gpio_chip *gc = &sch->chip;
+	unsigned long core_status, resume_status;
+	unsigned long pending;
+	unsigned long flags;
+	int offset;
+	u32 ret;
+
+	spin_lock_irqsave(&sch->lock, flags);
+
+	core_status = inl(sch->iobase + CORE_BANK_OFFSET + GTS);
+	resume_status = inl(sch->iobase + RESUME_BANK_OFFSET + GTS);
+
+	spin_unlock_irqrestore(&sch->lock, flags);
+
+	pending = (resume_status << sch->resume_base) | core_status;
+	for_each_set_bit(offset, &pending, sch->chip.ngpio)
+		generic_handle_irq(irq_find_mapping(gc->irq.domain, offset));
+
+	/* Set returning value depending on whether we handled an interrupt */
+	ret = pending ? ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;
+
+	/* Acknowledge GPE to ACPICA */
+	ret |= ACPI_REENABLE_GPE;
+
+	return ret;
+}
+
+static void sch_gpio_remove_gpe_handler(void *data)
+{
+	struct sch_gpio *sch = data;
+
+	acpi_disable_gpe(NULL, sch->gpe);
+	acpi_remove_gpe_handler(NULL, sch->gpe, sch->gpe_handler);
+}
+
+static int sch_gpio_install_gpe_handler(struct sch_gpio *sch)
+{
+	struct device *dev = sch->chip.parent;
+	acpi_status status;
+
+	status = acpi_install_gpe_handler(NULL, sch->gpe, ACPI_GPE_EDGE_TRIGGERED,
+					  sch->gpe_handler, sch);
+	if (ACPI_FAILURE(status)) {
+		dev_err(dev, "Failed to install GPE handler for %u: %s\n",
+			sch->gpe, acpi_format_exception(status));
+		return -ENODEV;
+	}
+
+	status = acpi_enable_gpe(NULL, sch->gpe);
+	if (ACPI_FAILURE(status)) {
+		dev_err(dev, "Failed to enable GPE handler for %u: %s\n",
+			sch->gpe, acpi_format_exception(status));
+		acpi_remove_gpe_handler(NULL, sch->gpe, sch->gpe_handler);
+		return -ENODEV;
+	}
+
+	return devm_add_action_or_reset(dev, sch_gpio_remove_gpe_handler, sch);
+}
+
 static int sch_gpio_probe(struct platform_device *pdev)
 {
 	struct sch_gpio *sch;
 	struct resource *res;
+	int ret;
 
 	sch = devm_kzalloc(&pdev->dev, sizeof(*sch), GFP_KERNEL);
 	if (!sch)
@@ -305,6 +379,14 @@ static int sch_gpio_probe(struct platform_device *pdev)
 	sch->chip.irq.default_type = IRQ_TYPE_NONE;
 	sch->chip.irq.handler = handle_bad_irq;
 
+	/* GPE setup is optional */
+	sch->gpe = GPE0E_GPIO;
+	sch->gpe_handler = sch_gpio_gpe_handler;
+
+	ret = sch_gpio_install_gpe_handler(sch);
+	if (ret)
+		dev_warn(&pdev->dev, "Can't setup GPE, no IRQ support\n");
+
 	return devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
 }
 
-- 
2.30.2

Re: [PATCH v5 0/2] gpio: sch: Interrupt support

[Andy Shevchenko]

On Wed, Mar 17, 2021 at 05:19:26PM +0200, Andy Shevchenko wrote:
> The series adds event support to the Intel GPIO SCH driver. The hardware
> routes all events through GPE0 GPIO event.
> 
> I validated this on Intel Minnowboard (v1).
> 
> If somebody has different hardware with the same GPIO controller, I would
> appreciate additional testing.

I've applied this to my review and testing queue, thanks!

> Changes in v5:
> - added missed IRQ acknowledge callback (hence kernel Oops)
> - rewrite patch 2 completely from SCI to GPE hook
> 
> Changes in v4 (https://lore.kernel.org/linux-gpio/20210316162613.87710-1-andriy.shevchenko@linux.intel.com/T/#u):
> - turned to GPIO core infrastructure of IRQ chip instantiation (Linus)
> - converted IRQ callbacks to use better APIs
> - use handle_bad_irq() as default handler and now I know why, see
>   eb441337c714 ("gpio: pca953x: Set IRQ type when handle Intel Galileo Gen 2")
>     for the real example what happens if it's preset to something meaningful
> - fixed remove stage (we have to remove SCI handler, which wasn't done in v3)
> 
> Changes in v3 (https://lore.kernel.org/linux-gpio/cover.1574277614.git.jan.kiszka@siemens.com/T/#u):
> - split-up of the irq enabling patch as requested by Andy
> 
> Andy Shevchenko (1):
>   gpio: sch: Hook into ACPI GPE handler to catch GPIO edge events
> 
> Jan Kiszka (1):
>   gpio: sch: Add edge event support
> 
>  drivers/gpio/gpio-sch.c | 196 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 188 insertions(+), 8 deletions(-)
> 
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] gpio: sch: Add edge event support

[Linus Walleij]

On Wed, Mar 17, 2021 at 4:19 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Add the required infrastructure to enable and report edge events
> of the pins to the GPIO core. The actual hook-up of the event interrupt
> will happen separately.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I can't believe it that nobody added irq support to this driver for 10
years given how widely deployed it is! (Good work.)

Don't you need to add

select GPIOLIB_IRQCHIP

to Kconfig? So the gpio_chip contains the .irq member you're using.

> +       sch->irqchip.name = "sch_gpio";
> +       sch->irqchip.irq_ack = sch_irq_ack;
> +       sch->irqchip.irq_mask = sch_irq_mask;
> +       sch->irqchip.irq_unmask = sch_irq_unmask;
> +       sch->irqchip.irq_set_type = sch_irq_type;
> +
> +       sch->chip.irq.chip = &sch->irqchip;
> +       sch->chip.irq.num_parents = 0;
> +       sch->chip.irq.parents = NULL;
> +       sch->chip.irq.parent_handler = NULL;
> +       sch->chip.irq.default_type = IRQ_TYPE_NONE;
> +       sch->chip.irq.handler = handle_bad_irq;

I always add a local variable like:

struct gpio_irq_chip *girq;

And assign with the arrow, so as to make it easier to read:

girq->parent_handler = NULL

etc.

+/- the above:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v5 2/2] gpio: sch: Hook into ACPI GPE handler to catch GPIO edge events

[Linus Walleij]

On Wed, Mar 17, 2021 at 4:19 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Neither the ACPI description on Intel Minnowboard (v1) platform provides
> the required information to establish a generic handling nor the hardware
> capable of doing it. According to the data sheet the hardware can generate
> SCI events. Therefore, we need to hook from the driver into GPE handler of
> the ACPI subsystem in order to catch and report GPIO-related events.
>
> Validated on the Inlel Minnowboard (v1) platform.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Looks good to me:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v5 1/2] gpio: sch: Add edge event support

[Andy Shevchenko]

On Thu, Mar 25, 2021 at 09:13:51AM +0100, Linus Walleij wrote:
> On Wed, Mar 17, 2021 at 4:19 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > From: Jan Kiszka <jan.kiszka@siemens.com>
> >
> > Add the required infrastructure to enable and report edge events
> > of the pins to the GPIO core. The actual hook-up of the event interrupt
> > will happen separately.
> >
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > Co-developed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I can't believe it that nobody added irq support to this driver for 10
> years given how widely deployed it is! (Good work.)
> 
> Don't you need to add
> 
> select GPIOLIB_IRQCHIP
> 
> to Kconfig? So the gpio_chip contains the .irq member you're using.

Seems legit, thanks!

> > +       sch->irqchip.name = "sch_gpio";
> > +       sch->irqchip.irq_ack = sch_irq_ack;
> > +       sch->irqchip.irq_mask = sch_irq_mask;
> > +       sch->irqchip.irq_unmask = sch_irq_unmask;
> > +       sch->irqchip.irq_set_type = sch_irq_type;
> > +
> > +       sch->chip.irq.chip = &sch->irqchip;
> > +       sch->chip.irq.num_parents = 0;
> > +       sch->chip.irq.parents = NULL;
> > +       sch->chip.irq.parent_handler = NULL;
> > +       sch->chip.irq.default_type = IRQ_TYPE_NONE;
> > +       sch->chip.irq.handler = handle_bad_irq;
> 
> I always add a local variable like:
> 
> struct gpio_irq_chip *girq;
> 
> And assign with the arrow, so as to make it easier to read:
> 
> girq->parent_handler = NULL
> 
> etc.

OK!

> +/- the above:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!

-- 
With Best Regards,
Andy Shevchenko