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

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

[Andy Shevchenko]

I'm now struggling to test this on Intel Minnowboard (v1), but it seems not
working and it's only one possible pin to test, so I have to take soldering
iron in my hands and do some hardware hacks to test. Meanwhile I sent this in
order to Jan to test on his side that everything is working on their platform.

Changes in v4:
- 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

Jan Kiszka (2):
  gpio: sch: Add edge event support
  gpio: sch: Hook into ACPI SCI handler to catch GPIO edge events

 drivers/gpio/gpio-sch.c | 146 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 140 insertions(+), 6 deletions(-)

-- 
2.30.2

[PATCH v4 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 | 91 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 85 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 3a1b1adb08c6..bbf8ee0b54de 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -18,9 +18,14 @@
 #define GEN	0x00
 #define GIO	0x04
 #define GLV	0x08
+#define GTPE	0x0c
+#define GTNE	0x10
+#define GGPE	0x14
+#define GTS	0x1c
 
 struct sch_gpio {
 	struct gpio_chip chip;
+	struct irq_chip irqchip;
 	spinlock_t lock;
 	unsigned short iobase;
 	unsigned short resume_base;
@@ -79,10 +84,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 +102,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 +152,65 @@ 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_set_enable(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_set_enable(d, 0);
+}
+
+static void sch_irq_unmask(struct irq_data *d)
+{
+	sch_irq_set_enable(d, 1);
+}
+
 static int sch_gpio_probe(struct platform_device *pdev)
 {
 	struct sch_gpio *sch;
@@ -207,6 +274,18 @@ static int sch_gpio_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sch);
 
+	sch->irqchip.name = "sch_gpio";
+	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 v4 2/2] gpio: sch: Hook into ACPI SCI handler to catch GPIO edge events

[Andy Shevchenko]

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

Neither the ACPI description on the Quark platform provides the required
information is to do establish generic handling nor hardware capable of
doing it. According to the datasheet the hardware can generate SCI events.
Therefore, we need to hook from the driver directly into SCI handler of
the ACPI subsystem in order to catch and report GPIO-related events.

Validated on the Quark-based IOT2000 platform.

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 | 55 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index bbf8ee0b54de..022677d36a7c 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -26,6 +26,7 @@
 struct sch_gpio {
 	struct gpio_chip chip;
 	struct irq_chip irqchip;
+	acpi_sci_handler sci_handler;
 	spinlock_t lock;
 	unsigned short iobase;
 	unsigned short resume_base;
@@ -152,6 +153,28 @@ static const struct gpio_chip sch_gpio_chip = {
 	.get_direction		= sch_gpio_get_direction,
 };
 
+static u32 sch_gpio_sci_handler(void *context)
+{
+	struct sch_gpio *sch = context;
+	struct gpio_chip *gc = &sch->chip;
+	unsigned long core_status, resume_status;
+	unsigned long pending;
+	int offset;
+
+	core_status = inl(sch->iobase + GTS + 0x00);
+	resume_status = inl(sch->iobase + GTS + 0x20);
+
+	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));
+
+	outl(core_status, sch->iobase + GTS + 0x00);
+	outl(resume_status, sch->iobase + GTS + 0x20);
+
+	return pending ? ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;
+}
+
 static int sch_irq_type(struct irq_data *d, unsigned int type)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
@@ -211,10 +234,36 @@ static void sch_irq_unmask(struct irq_data *d)
 	sch_irq_set_enable(d, 1);
 }
 
+static void sch_gpio_remove_sci_handler(void *data)
+{
+	struct sch_gpio *sch = data;
+	struct device *dev = sch->chip.parent;
+	acpi_status status;
+
+	status = acpi_remove_sci_handler(sch->sci_handler);
+	if (ACPI_FAILURE(status))
+		dev_err(dev, "Can't remove SCI handler\n");
+}
+
+static int sch_gpio_install_sci_handler(struct sch_gpio *sch)
+{
+	struct device *dev = sch->chip.parent;
+	acpi_status status;
+
+	status = acpi_install_sci_handler(sch->sci_handler, sch);
+	if (ACPI_SUCCESS(status))
+		return devm_add_action_or_reset(dev, sch_gpio_remove_sci_handler, sch);
+
+	/* SCI handler is optional */
+	dev_warn(dev, "Can't install SCI handler, no IRQ support\n");
+	return 0;
+}
+
 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)
@@ -286,6 +335,12 @@ static int sch_gpio_probe(struct platform_device *pdev)
 	sch->chip.irq.default_type = IRQ_TYPE_NONE;
 	sch->chip.irq.handler = handle_bad_irq;
 
+	sch->sci_handler = sch_gpio_sci_handler;
+
+	ret = sch_gpio_install_sci_handler(sch);
+	if (ret)
+		return ret;
+
 	return devm_gpiochip_add_data(&pdev->dev, &sch->chip, sch);
 }
 
-- 
2.30.2

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

[Andy Shevchenko]

On Tue, Mar 16, 2021 at 06:26:13PM +0200, Andy Shevchenko wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Neither the ACPI description on the Quark platform provides the required
> information is to do establish generic handling nor hardware capable of
> doing it. According to the datasheet the hardware can generate SCI events.
> Therefore, we need to hook from the driver directly into SCI handler of
> the ACPI subsystem in order to catch and report GPIO-related events.

> Validated on the Quark-based IOT2000 platform.

This depends on the test by Jan or somebody with the same hardware available.

> +static u32 sch_gpio_sci_handler(void *context)
> +{
> +	struct sch_gpio *sch = context;
> +	struct gpio_chip *gc = &sch->chip;
> +	unsigned long core_status, resume_status;
> +	unsigned long pending;
> +	int offset;

> +	core_status = inl(sch->iobase + GTS + 0x00);
> +	resume_status = inl(sch->iobase + GTS + 0x20);
> +
> +	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));
> +
> +	outl(core_status, sch->iobase + GTS + 0x00);
> +	outl(resume_status, sch->iobase + GTS + 0x20);

I guess this still needs to be protected by spin_lock.

> +	return pending ? ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;
> +}

...

Also I am in doubt that we need to instantiate an IRQ chip if the ACPI SCI
handler registration fails. But I don't know what is better approach here, to
NULL the pointer, or try to register handler before we will have an IRQ chip in
place. Any recommendations?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Mar 16, 2021 at 06:26:13PM +0200, Andy Shevchenko wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Neither the ACPI description on the Quark platform provides the required
> information is to do establish generic handling nor hardware capable of
> doing it. According to the datasheet the hardware can generate SCI events.
> Therefore, we need to hook from the driver directly into SCI handler of
> the ACPI subsystem in order to catch and report GPIO-related events.
> 
> Validated on the Quark-based IOT2000 platform.

This patch must be dropped completely. SCI handler is not correct way to do
this. The proper way (and we have already few examples in the kernel) is to
register GPE event.

It took me a while to gather all bits of this puzzle.

At least now I get an event, but kernel oopses, I'll continue debugging
tomorrow.

-- 
With Best Regards,
Andy Shevchenko

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

[Jan Kiszka]

On 16.03.21 21:49, Andy Shevchenko wrote:
> On Tue, Mar 16, 2021 at 06:26:13PM +0200, Andy Shevchenko wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Neither the ACPI description on the Quark platform provides the required
>> information is to do establish generic handling nor hardware capable of
>> doing it. According to the datasheet the hardware can generate SCI events.
>> Therefore, we need to hook from the driver directly into SCI handler of
>> the ACPI subsystem in order to catch and report GPIO-related events.
>>
>> Validated on the Quark-based IOT2000 platform.
> 
> This patch must be dropped completely. SCI handler is not correct way to do
> this. The proper way (and we have already few examples in the kernel) is to
> register GPE event.

As explained above, this is not supported by the preexisting firmware,
and there won't be any updates to it anymore.

This platform is history, the SoC was discontinued by Intel long ago,
and our devices reaching their support end as well. The race to upstream
was lost in this case - backlog too long, we being too slow.

Jan

> 
> It took me a while to gather all bits of this puzzle.
> 
> At least now I get an event, but kernel oopses, I'll continue debugging
> tomorrow.
> 

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

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

[Andy Shevchenko]

On Wed, Mar 17, 2021 at 07:57:44AM +0100, Jan Kiszka wrote:
> On 16.03.21 21:49, Andy Shevchenko wrote:
> > On Tue, Mar 16, 2021 at 06:26:13PM +0200, Andy Shevchenko wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> Neither the ACPI description on the Quark platform provides the required
> >> information is to do establish generic handling nor hardware capable of
> >> doing it. According to the datasheet the hardware can generate SCI events.
> >> Therefore, we need to hook from the driver directly into SCI handler of
> >> the ACPI subsystem in order to catch and report GPIO-related events.
> >>
> >> Validated on the Quark-based IOT2000 platform.
> > 
> > This patch must be dropped completely. SCI handler is not correct way to do
> > this. The proper way (and we have already few examples in the kernel) is to
> > register GPE event.
> 
> As explained above, this is not supported by the preexisting firmware,
> and there won't be any updates to it anymore.
> 
> This platform is history, the SoC was discontinued by Intel long ago,
> and our devices reaching their support end as well. The race to upstream
> was lost in this case - backlog too long, we being too slow.

So you have no device to test and there is actually no device which has this
capability in the wild.

Am I reading this correct?

In any case, we have platforms in the wild that actually support GPEs and this
makes sense for them.

> > It took me a while to gather all bits of this puzzle.
> > 
> > At least now I get an event, but kernel oopses, I'll continue debugging
> > tomorrow.

-- 
With Best Regards,
Andy Shevchenko

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

[Jan Kiszka]

On 17.03.21 10:52, Andy Shevchenko wrote:
> On Wed, Mar 17, 2021 at 07:57:44AM +0100, Jan Kiszka wrote:
>> On 16.03.21 21:49, Andy Shevchenko wrote:
>>> On Tue, Mar 16, 2021 at 06:26:13PM +0200, Andy Shevchenko wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Neither the ACPI description on the Quark platform provides the required
>>>> information is to do establish generic handling nor hardware capable of
>>>> doing it. According to the datasheet the hardware can generate SCI events.
>>>> Therefore, we need to hook from the driver directly into SCI handler of
>>>> the ACPI subsystem in order to catch and report GPIO-related events.
>>>>
>>>> Validated on the Quark-based IOT2000 platform.
>>>
>>> This patch must be dropped completely. SCI handler is not correct way to do
>>> this. The proper way (and we have already few examples in the kernel) is to
>>> register GPE event.
>>
>> As explained above, this is not supported by the preexisting firmware,
>> and there won't be any updates to it anymore.
>>
>> This platform is history, the SoC was discontinued by Intel long ago,
>> and our devices reaching their support end as well. The race to upstream
>> was lost in this case - backlog too long, we being too slow.
> 
> So you have no device to test and there is actually no device which has this
> capability in the wild.
> 
> Am I reading this correct?

No. We do have devices but we don't have the time to invest further into
bringing missing features upstream - not to speak of changing the
firmware in order to support cleaner upstream integration.

For the remaining lifetime of the devices, we are stuck on 4.4.y-cip
with a few additional patches, including this one.

> 
> In any case, we have platforms in the wild that actually support GPEs and this
> makes sense for them.

Sure, I don't want to judge for them. Just our original target of this
patch is no longer relevant for upstream.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux