[RFC PATCH 0/3] synquacer: implement ACPI gpio/interrupt support

[RFC PATCH 0/3] synquacer: implement ACPI gpio/interrupt support

[Ard Biesheuvel]

Wire up the existing GPIO and interrupt controller drivers to the ACPI
subsystem so they can be used on ACPI systems for ACPI event (power
button, hardware error notification etc)

Cc: Masahisa Kojima <masahisa.kojima@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Graeme Gregory <graeme.gregory@linaro.org>

Ard Biesheuvel (3):
  irqchip/exiu: preparatory refactor for ACPI support
  irqchip/exiu: implement ACPI gpiolib/irqchip support
  gpio: mb86s70: enable ACPI and irqchip support

 drivers/gpio/Kconfig           |   4 +
 drivers/gpio/gpio-mb86s7x.c    |  58 ++++++--
 drivers/irqchip/irq-sni-exiu.c | 157 +++++++++++++++-----
 3 files changed, 173 insertions(+), 46 deletions(-)

-- 
2.20.1

[RFC PATCH 1/3] irqchip/exiu: preparatory refactor for ACPI support

[Ard Biesheuvel]

In preparation of exposing the EXIU controller as the irqchip part of
the GPIO controller, split the DT init function in a DT specific and
a generic part, where the latter will be reused for ACPI support later.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/irqchip/irq-sni-exiu.c | 77 ++++++++++++--------
 1 file changed, 48 insertions(+), 29 deletions(-)

diff --git a/drivers/irqchip/irq-sni-exiu.c b/drivers/irqchip/irq-sni-exiu.c
index 1927b2f36ff6..52ce662334d4 100644
--- a/drivers/irqchip/irq-sni-exiu.c
+++ b/drivers/irqchip/irq-sni-exiu.c
@@ -1,7 +1,7 @@
 /*
  * Driver for Socionext External Interrupt Unit (EXIU)
  *
- * Copyright (c) 2017 Linaro, Ltd. <ard.biesheuvel@linaro.org>
+ * Copyright (c) 2017-2019 Linaro, Ltd. <ard.biesheuvel@linaro.org>
  *
  * Based on irq-tegra.c:
  *   Copyright (C) 2011 Google, Inc.
@@ -167,35 +167,25 @@ static const struct irq_domain_ops exiu_domain_ops = {
 	.free		= irq_domain_free_irqs_common,
 };
 
-static int __init exiu_init(struct device_node *node,
-			    struct device_node *parent)
+static struct irq_domain *exiu_init(struct irq_domain *parent_domain,
+				    struct fwnode_handle *fwnode,
+				    struct resource *res)
 {
-	struct irq_domain *parent_domain, *domain;
+	struct irq_domain *domain;
 	struct exiu_irq_data *data;
 	int err;
 
-	if (!parent) {
-		pr_err("%pOF: no parent, giving up\n", node);
-		return -ENODEV;
-	}
-
-	parent_domain = irq_find_host(parent);
-	if (!parent_domain) {
-		pr_err("%pOF: unable to obtain parent domain\n", node);
-		return -ENXIO;
-	}
-
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
-	if (of_property_read_u32(node, "socionext,spi-base", &data->spi_base)) {
-		pr_err("%pOF: failed to parse 'spi-base' property\n", node);
+	if (fwnode_property_read_u32_array(fwnode, "socionext,spi-base",
+					   &data->spi_base, 1)) {
 		err = -ENODEV;
 		goto out_free;
 	}
 
-	data->base = of_iomap(node, 0);
+	data->base = ioremap(res->start, resource_size(res));
 	if (!data->base) {
 		err = -ENODEV;
 		goto out_free;
@@ -205,23 +195,52 @@ static int __init exiu_init(struct device_node *node,
 	writel_relaxed(0xFFFFFFFF, data->base + EIREQCLR);
 	writel_relaxed(0xFFFFFFFF, data->base + EIMASK);
 
-	domain = irq_domain_add_hierarchy(parent_domain, 0, NUM_IRQS, node,
-					  &exiu_domain_ops, data);
+	domain = irq_domain_create_hierarchy(parent_domain, 0, NUM_IRQS,
+					     fwnode, &exiu_domain_ops, data);
 	if (!domain) {
-		pr_err("%pOF: failed to allocate domain\n", node);
 		err = -ENOMEM;
 		goto out_unmap;
 	}
+	return domain;
+out_unmap:
+	iounmap(data->base);
+out_free:
+	kfree(data);
+	return ERR_PTR(err);
+}
+
+static int __init exiu_dt_init(struct device_node *node,
+			       struct device_node *parent)
+{
+	struct irq_domain *parent_domain, *domain;
+	struct resource res;
+
+	if (!parent) {
+		pr_err("%pOF: no parent, giving up\n", node);
+		return -ENODEV;
+	}
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		pr_err("%pOF: unable to obtain parent domain\n", node);
+		return -ENXIO;
+	}
+
+	if (of_address_to_resource(node, 0, &res)) {
+		pr_err("%pOF: failed to parse memory resource\n", node);
+		return -ENXIO;
+	}
+
+	domain = exiu_init(parent_domain, of_node_to_fwnode(node), &res);
+	if (IS_ERR(domain)) {
+		pr_err("%pOF: failed to create IRQ domain (%ld)\n", node,
+		       PTR_ERR(domain));
+		return PTR_ERR(domain);
+	}
 
 	pr_info("%pOF: %d interrupts forwarded to %pOF\n", node, NUM_IRQS,
 		parent);
 
 	return 0;
-
-out_unmap:
-	iounmap(data->base);
-out_free:
-	kfree(data);
-	return err;
 }
-IRQCHIP_DECLARE(exiu, "socionext,synquacer-exiu", exiu_init);
+IRQCHIP_DECLARE(exiu, "socionext,synquacer-exiu", exiu_dt_init);
-- 
2.20.1

[RFC PATCH 2/3] irqchip/exiu: implement ACPI gpiolib/irqchip support

[Ard Biesheuvel]

Expose the existing EXIU hierarchical irqchip domain code to permit
the interrupt controller to be used as the irqchip component of a
GPIO controller on ACPI systems.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/irqchip/irq-sni-exiu.c | 82 +++++++++++++++++---
 1 file changed, 73 insertions(+), 9 deletions(-)

diff --git a/drivers/irqchip/irq-sni-exiu.c b/drivers/irqchip/irq-sni-exiu.c
index 52ce662334d4..99351cf997d9 100644
--- a/drivers/irqchip/irq-sni-exiu.c
+++ b/drivers/irqchip/irq-sni-exiu.c
@@ -12,6 +12,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/irq.h>
@@ -20,6 +21,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/platform_device.h>
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 
@@ -134,9 +136,13 @@ static int exiu_domain_translate(struct irq_domain *domain,
 
 		*hwirq = fwspec->param[1] - info->spi_base;
 		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
-		return 0;
+	} else {
+		if (fwspec->param_count != 2)
+			return -EINVAL;
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
 	}
-	return -EINVAL;
+	return 0;
 }
 
 static int exiu_domain_alloc(struct irq_domain *dom, unsigned int virq,
@@ -147,16 +153,23 @@ static int exiu_domain_alloc(struct irq_domain *dom, unsigned int virq,
 	struct exiu_irq_data *info = dom->host_data;
 	irq_hw_number_t hwirq;
 
-	if (fwspec->param_count != 3)
-		return -EINVAL;	/* Not GIC compliant */
-	if (fwspec->param[0] != GIC_SPI)
-		return -EINVAL;	/* No PPI should point to this domain */
-
+	parent_fwspec = *fwspec;
+	if (is_of_node(dom->parent->fwnode)) {
+		if (fwspec->param_count != 3)
+			return -EINVAL;	/* Not GIC compliant */
+		if (fwspec->param[0] != GIC_SPI)
+			return -EINVAL;	/* No PPI should point to this domain */
+
+		hwirq = fwspec->param[1] - info->spi_base;
+	} else if (is_fwnode_irqchip(dom->parent->fwnode)) {
+		hwirq = fwspec->param[0];
+		parent_fwspec.param[0] = hwirq + info->spi_base + 32;
+	} else {
+		return -EINVAL;
+	}
 	WARN_ON(nr_irqs != 1);
-	hwirq = fwspec->param[1] - info->spi_base;
 	irq_domain_set_hwirq_and_chip(dom, virq, hwirq, &exiu_irq_chip, info);
 
-	parent_fwspec = *fwspec;
 	parent_fwspec.fwnode = dom->parent->fwnode;
 	return irq_domain_alloc_irqs_parent(dom, virq, nr_irqs, &parent_fwspec);
 }
@@ -244,3 +257,54 @@ static int __init exiu_dt_init(struct device_node *node,
 	return 0;
 }
 IRQCHIP_DECLARE(exiu, "socionext,synquacer-exiu", exiu_dt_init);
+
+#ifdef CONFIG_ACPI
+static int exiu_acpi_gpio_to_irq(struct gpio_chip *gc, u32 gpio)
+{
+	struct irq_fwspec fwspec;
+
+	fwspec.fwnode		= gc->parent->fwnode;
+	fwspec.param_count	= 2;
+	fwspec.param[0]		= gpio;
+	fwspec.param[1]		= IRQ_TYPE_LEVEL_HIGH;
+
+	return irq_create_fwspec_mapping(&fwspec);
+}
+
+int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc)
+{
+	struct irq_domain *parent_domain = NULL, *domain;
+	struct resource *res;
+	int irq;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0)
+		parent_domain = irq_get_irq_data(irq)->domain;
+
+	if (!parent_domain) {
+		dev_err(&pdev->dev, "unable to obtain parent domain\n");
+		return -ENODEV;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to parse memory resource\n");
+		return -ENXIO;
+	}
+
+	domain = exiu_init(parent_domain, dev_fwnode(&pdev->dev), res);
+	if (IS_ERR(domain)) {
+		dev_err(&pdev->dev, "failed to create IRQ domain (%ld)\n",
+			PTR_ERR(domain));
+		return PTR_ERR(domain);
+	}
+
+	gc->irq.domain = domain;
+	gc->to_irq = exiu_acpi_gpio_to_irq;
+
+	dev_info(&pdev->dev, "%d interrupts forwarded\n", NUM_IRQS);
+
+	return 0;
+}
+EXPORT_SYMBOL(exiu_acpi_init);
+#endif
-- 
2.20.1

Re: [RFC PATCH 2/3] irqchip/exiu: implement ACPI gpiolib/irqchip support

[Linus Walleij]

Hi Ard!

Thanks for your patch!

As it involves ACPI I suggest to include Mika Westerberg on review since
he's doing the core gpiolib ACPI stuff.

On Thu, Apr 25, 2019 at 12:20 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:

> Expose the existing EXIU hierarchical irqchip domain code to permit
> the interrupt controller to be used as the irqchip component of a
> GPIO controller on ACPI systems.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

(...)

> +#include <linux/gpio/driver.h>

So now this is starting to go from being a pure irqchip driver
to a combined irq+gpio driver.

> +       gc->irq.domain = domain;
> +       gc->to_irq = exiu_acpi_gpio_to_irq;

And there is some gpiochip involved.

> +EXPORT_SYMBOL(exiu_acpi_init);

Including exporting functions.

What about we just move this driver over to drivers/gpio and
start working out a combined GPIO+irqchip driver there? I am
working on adding generic hierarchical irqchip helpers in the
gpiolib core and then it's gonna be nice to have all combined
drivers under the drivers/gpio folder.

Yours,
Linus Walleij

Re: [RFC PATCH 2/3] irqchip/exiu: implement ACPI gpiolib/irqchip support

[Marc Zyngier]

Hi Ard,

On 25/04/2019 11:20, Ard Biesheuvel wrote:
> Expose the existing EXIU hierarchical irqchip domain code to permit
> the interrupt controller to be used as the irqchip component of a
> GPIO controller on ACPI systems.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/irqchip/irq-sni-exiu.c | 82 +++++++++++++++++---
>  1 file changed, 73 insertions(+), 9 deletions(-)
> 

[...]

> +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc)
> +{
> +	struct irq_domain *parent_domain = NULL, *domain;
> +	struct resource *res;
> +	int irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq > 0)
> +		parent_domain = irq_get_irq_data(irq)->domain;
> +
> +	if (!parent_domain) {
> +		dev_err(&pdev->dev, "unable to obtain parent domain\n");
> +		return -ENODEV;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		dev_err(&pdev->dev, "failed to parse memory resource\n");
> +		return -ENXIO;
> +	}
> +
> +	domain = exiu_init(parent_domain, dev_fwnode(&pdev->dev), res);
> +	if (IS_ERR(domain)) {
> +		dev_err(&pdev->dev, "failed to create IRQ domain (%ld)\n",
> +			PTR_ERR(domain));
> +		return PTR_ERR(domain);
> +	}
> +
> +	gc->irq.domain = domain;
> +	gc->to_irq = exiu_acpi_gpio_to_irq;
> +
> +	dev_info(&pdev->dev, "%d interrupts forwarded\n", NUM_IRQS);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(exiu_acpi_init);
> +#endif
> 

I must say I'm not overly keen on this function. Why can't this be
probed as an ACPI device, creating the corresponding domain, and leaving
to the GPIO driver the task of doing the GPIO stuff?

I appreciate there is a dependency between the two, but that's something
we should be able to solve (probe deferral?).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH 2/3] irqchip/exiu: implement ACPI gpiolib/irqchip support

[Ard Biesheuvel]

On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> Hi Ard,
>
> On 25/04/2019 11:20, Ard Biesheuvel wrote:
> > Expose the existing EXIU hierarchical irqchip domain code to permit
> > the interrupt controller to be used as the irqchip component of a
> > GPIO controller on ACPI systems.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  drivers/irqchip/irq-sni-exiu.c | 82 +++++++++++++++++---
> >  1 file changed, 73 insertions(+), 9 deletions(-)
> >
>
> [...]
>
> > +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc)
> > +{
> > +     struct irq_domain *parent_domain = NULL, *domain;
> > +     struct resource *res;
> > +     int irq;
> > +
> > +     irq = platform_get_irq(pdev, 0);
> > +     if (irq > 0)
> > +             parent_domain = irq_get_irq_data(irq)->domain;
> > +
> > +     if (!parent_domain) {
> > +             dev_err(&pdev->dev, "unable to obtain parent domain\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +     if (!res) {
> > +             dev_err(&pdev->dev, "failed to parse memory resource\n");
> > +             return -ENXIO;
> > +     }
> > +
> > +     domain = exiu_init(parent_domain, dev_fwnode(&pdev->dev), res);
> > +     if (IS_ERR(domain)) {
> > +             dev_err(&pdev->dev, "failed to create IRQ domain (%ld)\n",
> > +                     PTR_ERR(domain));
> > +             return PTR_ERR(domain);
> > +     }
> > +
> > +     gc->irq.domain = domain;
> > +     gc->to_irq = exiu_acpi_gpio_to_irq;
> > +
> > +     dev_info(&pdev->dev, "%d interrupts forwarded\n", NUM_IRQS);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(exiu_acpi_init);
> > +#endif
> >
>
> I must say I'm not overly keen on this function. Why can't this be
> probed as an ACPI device, creating the corresponding domain, and leaving
> to the GPIO driver the task of doing the GPIO stuff?
>

Because ACPI only permits 'system' interrupts or GPIO interrupts, it
does not allow arbitrary device objects in the ACPI namespace to be
targeted as interrupt controllers.

> I appreciate there is a dependency between the two, but that's something
> we should be able to solve (probe deferral?).
>

Perhaps it would be better to just clone the irqchip part into the
GPIO driver? Everything else needs to be modified anyway, including
the domain alloc/translate methods.

That still leaves the question how to deal with the first four signal
lines, which are not shared between the EXIU and the GPIO controller.
but personally, I'm happy to just categorize that as "don't do that"
territory, and just ignore it for the time being.

Re: [RFC PATCH 2/3] irqchip/exiu: implement ACPI gpiolib/irqchip support

[Marc Zyngier]

On 26/04/2019 09:24, Ard Biesheuvel wrote:
> On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>
>> Hi Ard,
>>
>> On 25/04/2019 11:20, Ard Biesheuvel wrote:
>>> Expose the existing EXIU hierarchical irqchip domain code to permit
>>> the interrupt controller to be used as the irqchip component of a
>>> GPIO controller on ACPI systems.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  drivers/irqchip/irq-sni-exiu.c | 82 +++++++++++++++++---
>>>  1 file changed, 73 insertions(+), 9 deletions(-)
>>>
>>
>> [...]
>>
>>> +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc)
>>> +{
>>> +     struct irq_domain *parent_domain = NULL, *domain;
>>> +     struct resource *res;
>>> +     int irq;
>>> +
>>> +     irq = platform_get_irq(pdev, 0);
>>> +     if (irq > 0)
>>> +             parent_domain = irq_get_irq_data(irq)->domain;
>>> +
>>> +     if (!parent_domain) {
>>> +             dev_err(&pdev->dev, "unable to obtain parent domain\n");
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>> +     if (!res) {
>>> +             dev_err(&pdev->dev, "failed to parse memory resource\n");
>>> +             return -ENXIO;
>>> +     }
>>> +
>>> +     domain = exiu_init(parent_domain, dev_fwnode(&pdev->dev), res);
>>> +     if (IS_ERR(domain)) {
>>> +             dev_err(&pdev->dev, "failed to create IRQ domain (%ld)\n",
>>> +                     PTR_ERR(domain));
>>> +             return PTR_ERR(domain);
>>> +     }
>>> +
>>> +     gc->irq.domain = domain;
>>> +     gc->to_irq = exiu_acpi_gpio_to_irq;
>>> +
>>> +     dev_info(&pdev->dev, "%d interrupts forwarded\n", NUM_IRQS);
>>> +
>>> +     return 0;
>>> +}
>>> +EXPORT_SYMBOL(exiu_acpi_init);
>>> +#endif
>>>
>>
>> I must say I'm not overly keen on this function. Why can't this be
>> probed as an ACPI device, creating the corresponding domain, and leaving
>> to the GPIO driver the task of doing the GPIO stuff?
>>
> 
> Because ACPI only permits 'system' interrupts or GPIO interrupts, it
> does not allow arbitrary device objects in the ACPI namespace to be
> targeted as interrupt controllers.

Hmmm. We already have at least one driver (irq-mbigen.c) that does
exactly that. It uses interrupts from the GIC (it is notionally behind
the ITS), and offers a bunch of interrupt lines itself. Why is it different?

> 
>> I appreciate there is a dependency between the two, but that's something
>> we should be able to solve (probe deferral?).
>>
> 
> Perhaps it would be better to just clone the irqchip part into the
> GPIO driver? Everything else needs to be modified anyway, including
> the domain alloc/translate methods.
> 
> That still leaves the question how to deal with the first four signal
> lines, which are not shared between the EXIU and the GPIO controller.
> but personally, I'm happy to just categorize that as "don't do that"
> territory, and just ignore it for the time being.
> 

Maybe. But I'd really like to understand why the above doesn't work in
your case (as you can tell, my ACPI-foo is pretty basic...).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH 2/3] irqchip/exiu: implement ACPI gpiolib/irqchip support

[Ard Biesheuvel]

On Fri, 26 Apr 2019 at 10:44, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> On 26/04/2019 09:24, Ard Biesheuvel wrote:
> > On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >>
> >> Hi Ard,
> >>
> >> On 25/04/2019 11:20, Ard Biesheuvel wrote:
> >>> Expose the existing EXIU hierarchical irqchip domain code to permit
> >>> the interrupt controller to be used as the irqchip component of a
> >>> GPIO controller on ACPI systems.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> ---
> >>>  drivers/irqchip/irq-sni-exiu.c | 82 +++++++++++++++++---
> >>>  1 file changed, 73 insertions(+), 9 deletions(-)
> >>>
> >>
> >> [...]
> >>
> >>> +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc)
> >>> +{
> >>> +     struct irq_domain *parent_domain = NULL, *domain;
> >>> +     struct resource *res;
> >>> +     int irq;
> >>> +
> >>> +     irq = platform_get_irq(pdev, 0);
> >>> +     if (irq > 0)
> >>> +             parent_domain = irq_get_irq_data(irq)->domain;
> >>> +
> >>> +     if (!parent_domain) {
> >>> +             dev_err(&pdev->dev, "unable to obtain parent domain\n");
> >>> +             return -ENODEV;
> >>> +     }
> >>> +
> >>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >>> +     if (!res) {
> >>> +             dev_err(&pdev->dev, "failed to parse memory resource\n");
> >>> +             return -ENXIO;
> >>> +     }
> >>> +
> >>> +     domain = exiu_init(parent_domain, dev_fwnode(&pdev->dev), res);
> >>> +     if (IS_ERR(domain)) {
> >>> +             dev_err(&pdev->dev, "failed to create IRQ domain (%ld)\n",
> >>> +                     PTR_ERR(domain));
> >>> +             return PTR_ERR(domain);
> >>> +     }
> >>> +
> >>> +     gc->irq.domain = domain;
> >>> +     gc->to_irq = exiu_acpi_gpio_to_irq;
> >>> +
> >>> +     dev_info(&pdev->dev, "%d interrupts forwarded\n", NUM_IRQS);
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +EXPORT_SYMBOL(exiu_acpi_init);
> >>> +#endif
> >>>
> >>
> >> I must say I'm not overly keen on this function. Why can't this be
> >> probed as an ACPI device, creating the corresponding domain, and leaving
> >> to the GPIO driver the task of doing the GPIO stuff?
> >>
> >
> > Because ACPI only permits 'system' interrupts or GPIO interrupts, it
> > does not allow arbitrary device objects in the ACPI namespace to be
> > targeted as interrupt controllers.
>
> Hmmm. We already have at least one driver (irq-mbigen.c) that does
> exactly that. It uses interrupts from the GIC (it is notionally behind
> the ITS), and offers a bunch of interrupt lines itself. Why is it different?
>

You are right, it isn't. It turns out that there is another way to
signal ACPI events based on interrupts, and it involves the ACPI0013
GED device. I will try to model it that way instead.

> >
> >> I appreciate there is a dependency between the two, but that's something
> >> we should be able to solve (probe deferral?).
> >>
> >
> > Perhaps it would be better to just clone the irqchip part into the
> > GPIO driver? Everything else needs to be modified anyway, including
> > the domain alloc/translate methods.
> >
> > That still leaves the question how to deal with the first four signal
> > lines, which are not shared between the EXIU and the GPIO controller.
> > but personally, I'm happy to just categorize that as "don't do that"
> > territory, and just ignore it for the time being.
> >
>
> Maybe. But I'd really like to understand why the above doesn't work in
> your case (as you can tell, my ACPI-foo is pretty basic...).
>

As is mine. I am just trying to make sense of all of this so we can
report non-fatal RAS errors via ACPI events.

Re: [RFC PATCH 2/3] irqchip/exiu: implement ACPI gpiolib/irqchip support

[Ard Biesheuvel]

On Fri, 26 Apr 2019 at 13:45, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 26 Apr 2019 at 10:44, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >
> > On 26/04/2019 09:24, Ard Biesheuvel wrote:
> > > On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > >>
> > >> Hi Ard,
> > >>
> > >> On 25/04/2019 11:20, Ard Biesheuvel wrote:
> > >>> Expose the existing EXIU hierarchical irqchip domain code to permit
> > >>> the interrupt controller to be used as the irqchip component of a
> > >>> GPIO controller on ACPI systems.
> > >>>
> > >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >>> ---
> > >>>  drivers/irqchip/irq-sni-exiu.c | 82 +++++++++++++++++---
> > >>>  1 file changed, 73 insertions(+), 9 deletions(-)
> > >>>
> > >>
> > >> [...]
> > >>
> > >>> +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc)
> > >>> +{
> > >>> +     struct irq_domain *parent_domain = NULL, *domain;
> > >>> +     struct resource *res;
> > >>> +     int irq;
> > >>> +
> > >>> +     irq = platform_get_irq(pdev, 0);
> > >>> +     if (irq > 0)
> > >>> +             parent_domain = irq_get_irq_data(irq)->domain;
> > >>> +
> > >>> +     if (!parent_domain) {
> > >>> +             dev_err(&pdev->dev, "unable to obtain parent domain\n");
> > >>> +             return -ENODEV;
> > >>> +     }
> > >>> +
> > >>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > >>> +     if (!res) {
> > >>> +             dev_err(&pdev->dev, "failed to parse memory resource\n");
> > >>> +             return -ENXIO;
> > >>> +     }
> > >>> +
> > >>> +     domain = exiu_init(parent_domain, dev_fwnode(&pdev->dev), res);
> > >>> +     if (IS_ERR(domain)) {
> > >>> +             dev_err(&pdev->dev, "failed to create IRQ domain (%ld)\n",
> > >>> +                     PTR_ERR(domain));
> > >>> +             return PTR_ERR(domain);
> > >>> +     }
> > >>> +
> > >>> +     gc->irq.domain = domain;
> > >>> +     gc->to_irq = exiu_acpi_gpio_to_irq;
> > >>> +
> > >>> +     dev_info(&pdev->dev, "%d interrupts forwarded\n", NUM_IRQS);
> > >>> +
> > >>> +     return 0;
> > >>> +}
> > >>> +EXPORT_SYMBOL(exiu_acpi_init);
> > >>> +#endif
> > >>>
> > >>
> > >> I must say I'm not overly keen on this function. Why can't this be
> > >> probed as an ACPI device, creating the corresponding domain, and leaving
> > >> to the GPIO driver the task of doing the GPIO stuff?
> > >>
> > >
> > > Because ACPI only permits 'system' interrupts or GPIO interrupts, it
> > > does not allow arbitrary device objects in the ACPI namespace to be
> > > targeted as interrupt controllers.
> >
> > Hmmm. We already have at least one driver (irq-mbigen.c) that does
> > exactly that. It uses interrupts from the GIC (it is notionally behind
> > the ITS), and offers a bunch of interrupt lines itself. Why is it different?
> >
>
> You are right, it isn't. It turns out that there is another way to
> signal ACPI events based on interrupts, and it involves the ACPI0013
> GED device. I will try to model it that way instead.
>

Unfortunately, this doesn't work either. The GED device can respond to
GSIVs only, and so going via the EXIU doesn't work.

I have attempted to hack it up via AML, but the GED driver uses a
threaded interrupt, and so the interrupt is re-enabled at the GIC
before the AML handler is executed (which clears it in the EXIU)

For the RAS case, I guess we could let the firmware pick an arbitrary
unused SPI and signal that directly into the GIC, but for the power
button (which is physically wired to the EXIU), we have to model the
EXIU either has a separate pseudo-GPIO controller, or as part of the
real GPIO block.

Re: [RFC PATCH 2/3] irqchip/exiu: implement ACPI gpiolib/irqchip support

[Ard Biesheuvel]

On Sat, 27 Apr 2019 at 00:27, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Fri, 26 Apr 2019 at 13:45, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Fri, 26 Apr 2019 at 10:44, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > >
> > > On 26/04/2019 09:24, Ard Biesheuvel wrote:
> > > > On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > >>
> > > >> Hi Ard,
> > > >>
> > > >> On 25/04/2019 11:20, Ard Biesheuvel wrote:
> > > >>> Expose the existing EXIU hierarchical irqchip domain code to permit
> > > >>> the interrupt controller to be used as the irqchip component of a
> > > >>> GPIO controller on ACPI systems.
> > > >>>
> > > >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > >>> ---
> > > >>>  drivers/irqchip/irq-sni-exiu.c | 82 +++++++++++++++++---
> > > >>>  1 file changed, 73 insertions(+), 9 deletions(-)
> > > >>>
> > > >>
> > > >> [...]
> > > >>
> > > >>> +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc)
> > > >>> +{
> > > >>> +     struct irq_domain *parent_domain = NULL, *domain;
> > > >>> +     struct resource *res;
> > > >>> +     int irq;
> > > >>> +
> > > >>> +     irq = platform_get_irq(pdev, 0);
> > > >>> +     if (irq > 0)
> > > >>> +             parent_domain = irq_get_irq_data(irq)->domain;
> > > >>> +
> > > >>> +     if (!parent_domain) {
> > > >>> +             dev_err(&pdev->dev, "unable to obtain parent domain\n");
> > > >>> +             return -ENODEV;
> > > >>> +     }
> > > >>> +
> > > >>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > >>> +     if (!res) {
> > > >>> +             dev_err(&pdev->dev, "failed to parse memory resource\n");
> > > >>> +             return -ENXIO;
> > > >>> +     }
> > > >>> +
> > > >>> +     domain = exiu_init(parent_domain, dev_fwnode(&pdev->dev), res);
> > > >>> +     if (IS_ERR(domain)) {
> > > >>> +             dev_err(&pdev->dev, "failed to create IRQ domain (%ld)\n",
> > > >>> +                     PTR_ERR(domain));
> > > >>> +             return PTR_ERR(domain);
> > > >>> +     }
> > > >>> +
> > > >>> +     gc->irq.domain = domain;
> > > >>> +     gc->to_irq = exiu_acpi_gpio_to_irq;
> > > >>> +
> > > >>> +     dev_info(&pdev->dev, "%d interrupts forwarded\n", NUM_IRQS);
> > > >>> +
> > > >>> +     return 0;
> > > >>> +}
> > > >>> +EXPORT_SYMBOL(exiu_acpi_init);
> > > >>> +#endif
> > > >>>
> > > >>
> > > >> I must say I'm not overly keen on this function. Why can't this be
> > > >> probed as an ACPI device, creating the corresponding domain, and leaving
> > > >> to the GPIO driver the task of doing the GPIO stuff?
> > > >>
> > > >
> > > > Because ACPI only permits 'system' interrupts or GPIO interrupts, it
> > > > does not allow arbitrary device objects in the ACPI namespace to be
> > > > targeted as interrupt controllers.
> > >
> > > Hmmm. We already have at least one driver (irq-mbigen.c) that does
> > > exactly that. It uses interrupts from the GIC (it is notionally behind
> > > the ITS), and offers a bunch of interrupt lines itself. Why is it different?
> > >
> >
> > You are right, it isn't. It turns out that there is another way to
> > signal ACPI events based on interrupts, and it involves the ACPI0013
> > GED device. I will try to model it that way instead.
> >
>
> Unfortunately, this doesn't work either. The GED device can respond to
> GSIVs only, and so going via the EXIU doesn't work.
>
> I have attempted to hack it up via AML, but the GED driver uses a
> threaded interrupt, and so the interrupt is re-enabled at the GIC
> before the AML handler is executed (which clears it in the EXIU)
>
> For the RAS case, I guess we could let the firmware pick an arbitrary
> unused SPI and signal that directly into the GIC, but for the power
> button (which is physically wired to the EXIU), we have to model the
> EXIU either has a separate pseudo-GPIO controller, or as part of the
> real GPIO block.

(+ Mika)

I made some progress describing the EXIU and GPIO controllers as follow.

    Device (EXIU) {
      Name (_HID, "SCX0008")
      Name (_UID, Zero)
      Name (_CRS, ResourceTemplate () {
        Memory32Fixed (ReadWrite, SYNQUACER_EXIU_BASE, SYNQUACER_EXIU_SIZE)
        Interrupt (ResourceConsumer, Level, ActiveHigh, ExclusiveAndWake) {
          144, 145, 146, 147, 148, 149, 150, 151,
          152, 153, 154, 155, 156, 157, 158, 159,
          160, 161, 162, 163, 164, 165, 166, 167,
          168, 169, 170, 171, 172, 173, 174, 175,
        }
      })
      Name (_DSD, Package () {
        ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
          Package () { "socionext,spi-base", 112 },
        }
      })
    }

and

    Device (GPIO) {
      Name (_HID, "SCX0007")
      Name (_UID, Zero)
      Name (_CRS, ResourceTemplate () {
        Memory32Fixed (ReadWrite, SYNQUACER_GPIO_BASE, SYNQUACER_GPIO_SIZE)
        Interrupt (ResourceConsumer, Edge, ActiveLow,
ExclusiveAndWake, 0, "\\_SB.EXIU") {
          7,
        }
      })
      Name (_AEI, ResourceTemplate () {
        GpioInt (Edge, ActiveHigh, ExclusiveAndWake, PullDefault, 0,
"\\_SB.GPIO")
        {
          7
        }
      })
      Method (_E07) {
        Notify (\_SB.PWRB, 0x80)
      }
    }

This is actually a fairly accurate depiction of reality: the EXIU is a
separate unit, and only some of the GPIOs are routed through it.

In the GPIO controller's to_irq() callback, I return the Linux IRQ
number based on the platform IRQ resources claimed by the GPIO device,
and I end up with something like

 46:          0 ... 0      EXIU   7 Edge      ACPI:Event

which looks correct to me. debugfs tells me it is mapped as

handler:  handle_fasteoi_irq
device:   (null)
status:   0x00000001
istate:   0x00000020
            IRQS_ONESHOT
ddepth:   0
wdepth:   1
dstate:   0x03404201
            IRQ_TYPE_EDGE_RISING
            IRQD_ACTIVATED
            IRQD_IRQ_STARTED
            IRQD_SINGLE_TARGET
            IRQD_WAKEUP_STATE
node:     0
affinity: 0-23
effectiv: 0
domain:  \_SB_.EXIU
 hwirq:   0x7
 chip:    EXIU
  flags:   0x55
             IRQCHIP_SET_TYPE_MASKED
             IRQCHIP_MASK_ON_SUSPEND
             IRQCHIP_SKIP_SET_WAKE
             IRQCHIP_EOI_THREADED
 parent:
    domain:  irqchip@(____ptrval____)-1
     hwirq:   0x97
     chip:    GICv3
      flags:   0x15
                 IRQCHIP_SET_TYPE_MASKED
                 IRQCHIP_MASK_ON_SUSPEND
                 IRQCHIP_SKIP_SET_WAKE

The EXIU domain is described as

name:   \_SB_.EXIU
 size:   32
 mapped: 1
 flags:  0x00000041
 parent: irqchip@(____ptrval____)-1
    name:   irqchip@(____ptrval____)-1
     size:   0
     mapped: 55
     flags:  0x00000041

Unfortunately, the system locks up entirely as soon as the interrupt
is triggered (I use a DIP switch in this case). So while the
description is accurate and the correct interrupt does get mapped,
something is still not working entirely as expected.

Any thoughts?

Thanks,
Ard.

Re: [RFC PATCH 2/3] irqchip/exiu: implement ACPI gpiolib/irqchip support

[Marc Zyngier]

On 29/04/2019 10:09, Ard Biesheuvel wrote:
> On Sat, 27 Apr 2019 at 00:27, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> On Fri, 26 Apr 2019 at 13:45, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>
>>> On Fri, 26 Apr 2019 at 10:44, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>
>>>> On 26/04/2019 09:24, Ard Biesheuvel wrote:
>>>>> On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>>>
>>>>>> Hi Ard,
>>>>>>
>>>>>> On 25/04/2019 11:20, Ard Biesheuvel wrote:
>>>>>>> Expose the existing EXIU hierarchical irqchip domain code to permit
>>>>>>> the interrupt controller to be used as the irqchip component of a
>>>>>>> GPIO controller on ACPI systems.
>>>>>>>
>>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>>>> ---
>>>>>>>  drivers/irqchip/irq-sni-exiu.c | 82 +++++++++++++++++---
>>>>>>>  1 file changed, 73 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc)
>>>>>>> +{
>>>>>>> +     struct irq_domain *parent_domain = NULL, *domain;
>>>>>>> +     struct resource *res;
>>>>>>> +     int irq;
>>>>>>> +
>>>>>>> +     irq = platform_get_irq(pdev, 0);
>>>>>>> +     if (irq > 0)
>>>>>>> +             parent_domain = irq_get_irq_data(irq)->domain;
>>>>>>> +
>>>>>>> +     if (!parent_domain) {
>>>>>>> +             dev_err(&pdev->dev, "unable to obtain parent domain\n");
>>>>>>> +             return -ENODEV;
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>>>>> +     if (!res) {
>>>>>>> +             dev_err(&pdev->dev, "failed to parse memory resource\n");
>>>>>>> +             return -ENXIO;
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     domain = exiu_init(parent_domain, dev_fwnode(&pdev->dev), res);
>>>>>>> +     if (IS_ERR(domain)) {
>>>>>>> +             dev_err(&pdev->dev, "failed to create IRQ domain (%ld)\n",
>>>>>>> +                     PTR_ERR(domain));
>>>>>>> +             return PTR_ERR(domain);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     gc->irq.domain = domain;
>>>>>>> +     gc->to_irq = exiu_acpi_gpio_to_irq;
>>>>>>> +
>>>>>>> +     dev_info(&pdev->dev, "%d interrupts forwarded\n", NUM_IRQS);
>>>>>>> +
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(exiu_acpi_init);
>>>>>>> +#endif
>>>>>>>
>>>>>>
>>>>>> I must say I'm not overly keen on this function. Why can't this be
>>>>>> probed as an ACPI device, creating the corresponding domain, and leaving
>>>>>> to the GPIO driver the task of doing the GPIO stuff?
>>>>>>
>>>>>
>>>>> Because ACPI only permits 'system' interrupts or GPIO interrupts, it
>>>>> does not allow arbitrary device objects in the ACPI namespace to be
>>>>> targeted as interrupt controllers.
>>>>
>>>> Hmmm. We already have at least one driver (irq-mbigen.c) that does
>>>> exactly that. It uses interrupts from the GIC (it is notionally behind
>>>> the ITS), and offers a bunch of interrupt lines itself. Why is it different?
>>>>
>>>
>>> You are right, it isn't. It turns out that there is another way to
>>> signal ACPI events based on interrupts, and it involves the ACPI0013
>>> GED device. I will try to model it that way instead.
>>>
>>
>> Unfortunately, this doesn't work either. The GED device can respond to
>> GSIVs only, and so going via the EXIU doesn't work.
>>
>> I have attempted to hack it up via AML, but the GED driver uses a
>> threaded interrupt, and so the interrupt is re-enabled at the GIC
>> before the AML handler is executed (which clears it in the EXIU)
>>
>> For the RAS case, I guess we could let the firmware pick an arbitrary
>> unused SPI and signal that directly into the GIC, but for the power
>> button (which is physically wired to the EXIU), we have to model the
>> EXIU either has a separate pseudo-GPIO controller, or as part of the
>> real GPIO block.
> 
> (+ Mika)
> 
> I made some progress describing the EXIU and GPIO controllers as follow.
> 
>     Device (EXIU) {
>       Name (_HID, "SCX0008")
>       Name (_UID, Zero)
>       Name (_CRS, ResourceTemplate () {
>         Memory32Fixed (ReadWrite, SYNQUACER_EXIU_BASE, SYNQUACER_EXIU_SIZE)
>         Interrupt (ResourceConsumer, Level, ActiveHigh, ExclusiveAndWake) {
>           144, 145, 146, 147, 148, 149, 150, 151,
>           152, 153, 154, 155, 156, 157, 158, 159,
>           160, 161, 162, 163, 164, 165, 166, 167,
>           168, 169, 170, 171, 172, 173, 174, 175,
>         }
>       })
>       Name (_DSD, Package () {
>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>           Package () { "socionext,spi-base", 112 },
>         }
>       })
>     }
> 
> and
> 
>     Device (GPIO) {
>       Name (_HID, "SCX0007")
>       Name (_UID, Zero)
>       Name (_CRS, ResourceTemplate () {
>         Memory32Fixed (ReadWrite, SYNQUACER_GPIO_BASE, SYNQUACER_GPIO_SIZE)
>         Interrupt (ResourceConsumer, Edge, ActiveLow,
> ExclusiveAndWake, 0, "\\_SB.EXIU") {
>           7,
>         }
>       })
>       Name (_AEI, ResourceTemplate () {
>         GpioInt (Edge, ActiveHigh, ExclusiveAndWake, PullDefault, 0,
> "\\_SB.GPIO")
>         {
>           7
>         }
>       })
>       Method (_E07) {
>         Notify (\_SB.PWRB, 0x80)
>       }
>     }
> 
> This is actually a fairly accurate depiction of reality: the EXIU is a
> separate unit, and only some of the GPIOs are routed through it.
> 
> In the GPIO controller's to_irq() callback, I return the Linux IRQ
> number based on the platform IRQ resources claimed by the GPIO device,
> and I end up with something like
> 
>  46:          0 ... 0      EXIU   7 Edge      ACPI:Event
> 
> which looks correct to me. debugfs tells me it is mapped as
> 
> handler:  handle_fasteoi_irq
> device:   (null)
> status:   0x00000001
> istate:   0x00000020
>             IRQS_ONESHOT
> ddepth:   0
> wdepth:   1
> dstate:   0x03404201
>             IRQ_TYPE_EDGE_RISING
>             IRQD_ACTIVATED
>             IRQD_IRQ_STARTED
>             IRQD_SINGLE_TARGET
>             IRQD_WAKEUP_STATE
> node:     0
> affinity: 0-23
> effectiv: 0
> domain:  \_SB_.EXIU
>  hwirq:   0x7
>  chip:    EXIU
>   flags:   0x55
>              IRQCHIP_SET_TYPE_MASKED
>              IRQCHIP_MASK_ON_SUSPEND
>              IRQCHIP_SKIP_SET_WAKE
>              IRQCHIP_EOI_THREADED
>  parent:
>     domain:  irqchip@(____ptrval____)-1
>      hwirq:   0x97
>      chip:    GICv3
>       flags:   0x15
>                  IRQCHIP_SET_TYPE_MASKED
>                  IRQCHIP_MASK_ON_SUSPEND
>                  IRQCHIP_SKIP_SET_WAKE
> 
> The EXIU domain is described as
> 
> name:   \_SB_.EXIU
>  size:   32
>  mapped: 1
>  flags:  0x00000041
>  parent: irqchip@(____ptrval____)-1
>     name:   irqchip@(____ptrval____)-1
>      size:   0
>      mapped: 55
>      flags:  0x00000041
> 
> Unfortunately, the system locks up entirely as soon as the interrupt
> is triggered (I use a DIP switch in this case). So while the
> description is accurate and the correct interrupt does get mapped,
> something is still not working entirely as expected.
> 
> Any thoughts?

It feels like an interrupt storm with an edge vs level misconfiguration.
Can you check that the IRQ_TYPE_EDGE_RISING is correctly propagated
across the hierarchy?

The EXIU block has:

Interrupt (ResourceConsumer, Level, ActiveHigh, ExclusiveAndWake) {

while the GPIO block has:

Interrupt (ResourceConsumer, Edge, ActiveLow, ExclusiveAndWake, 0,
"\\_SB.EXIU")

and the interrupt is configured for yet another trigger
(IRQ_TYPE_EDGE_RISING).

Hope this helps,

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC PATCH 3/3] gpio: mb86s70: enable ACPI and irqchip support

[Ard Biesheuvel]

In order to support this GPIO block in combination with an EXIU
interrupt controller on ACPI systems such as Socionext SynQuacer,
add the enumeration boilerplate, and add the EXIU irqchip handling
to the probe path. Also, make the clock handling conditonal on
non-ACPI enumeration, since ACPI handles this internally.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/gpio/Kconfig        |  4 ++
 drivers/gpio/gpio-mb86s7x.c | 58 +++++++++++++++++---
 2 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3f50526a771f..2c2773ea9627 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -315,6 +315,10 @@ config GPIO_MB86S7X
 	help
 	  Say yes here to support the GPIO controller in Fujitsu MB86S70 SoCs.
 
+config GPIO_MB86S7X_ACPI
+	def_bool y
+	depends on ACPI && ARCH_SYNQUACER
+
 config GPIO_MENZ127
 	tristate "MEN 16Z127 GPIO support"
 	depends on MCB
diff --git a/drivers/gpio/gpio-mb86s7x.c b/drivers/gpio/gpio-mb86s7x.c
index 3134c0d2bfe4..d254783f7e71 100644
--- a/drivers/gpio/gpio-mb86s7x.c
+++ b/drivers/gpio/gpio-mb86s7x.c
@@ -14,6 +14,7 @@
  *  GNU General Public License for more details.
  */
 
+#include <linux/acpi.h>
 #include <linux/io.h>
 #include <linux/init.h>
 #include <linux/clk.h>
@@ -27,6 +28,8 @@
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 
+#include "gpiolib.h"
+
 /*
  * Only first 8bits of a register correspond to each pin,
  * so there are 4 registers for 32 pins.
@@ -44,6 +47,8 @@ struct mb86s70_gpio_chip {
 	spinlock_t lock;
 };
 
+int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc);
+
 static int mb86s70_gpio_request(struct gpio_chip *gc, unsigned gpio)
 {
 	struct mb86s70_gpio_chip *gchip = gpiochip_get_data(gc);
@@ -143,6 +148,12 @@ static void mb86s70_gpio_set(struct gpio_chip *gc, unsigned gpio, int value)
 	spin_unlock_irqrestore(&gchip->lock, flags);
 }
 
+static bool mb86s70_gpio_have_acpi(struct platform_device *pdev)
+{
+	return IS_ENABLED(CONFIG_GPIO_MB86S7X_ACPI) &&
+	       ACPI_COMPANION(&pdev->dev);
+}
+
 static int mb86s70_gpio_probe(struct platform_device *pdev)
 {
 	struct mb86s70_gpio_chip *gchip;
@@ -160,13 +171,15 @@ static int mb86s70_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(gchip->base))
 		return PTR_ERR(gchip->base);
 
-	gchip->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(gchip->clk))
-		return PTR_ERR(gchip->clk);
+	if (!mb86s70_gpio_have_acpi(pdev)) {
+		gchip->clk = devm_clk_get(&pdev->dev, NULL);
+		if (IS_ERR(gchip->clk))
+			return PTR_ERR(gchip->clk);
 
-	ret = clk_prepare_enable(gchip->clk);
-	if (ret)
-		return ret;
+		ret = clk_prepare_enable(gchip->clk);
+		if (ret)
+			return ret;
+	}
 
 	spin_lock_init(&gchip->lock);
 
@@ -182,21 +195,39 @@ static int mb86s70_gpio_probe(struct platform_device *pdev)
 	gchip->gc.parent = &pdev->dev;
 	gchip->gc.base = -1;
 
+	if (mb86s70_gpio_have_acpi(pdev)) {
+		ret = exiu_acpi_init(pdev, &gchip->gc);
+		if (ret) {
+			dev_err(&pdev->dev, "couldn't register gpio irqchip\n");
+			return ret;
+		}
+	}
+
 	ret = gpiochip_add_data(&gchip->gc, gchip);
 	if (ret) {
 		dev_err(&pdev->dev, "couldn't register gpio driver\n");
-		clk_disable_unprepare(gchip->clk);
+		if (gchip->clk)
+			clk_disable_unprepare(gchip->clk);
+		return ret;
 	}
 
-	return ret;
+	if (mb86s70_gpio_have_acpi(pdev))
+		acpi_gpiochip_request_interrupts(&gchip->gc);
+
+	return 0;
 }
 
 static int mb86s70_gpio_remove(struct platform_device *pdev)
 {
 	struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);
 
+	if (gchip->gc.irq.domain) {
+		acpi_gpiochip_free_interrupts(&gchip->gc);
+		irq_domain_remove(gchip->gc.irq.domain);
+	}
 	gpiochip_remove(&gchip->gc);
-	clk_disable_unprepare(gchip->clk);
+	if (gchip->clk)
+		clk_disable_unprepare(gchip->clk);
 
 	return 0;
 }
@@ -207,10 +238,19 @@ static const struct of_device_id mb86s70_gpio_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, mb86s70_gpio_dt_ids);
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id mb86s70_gpio_acpi_ids[] = {
+	{ "SCX0007" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(acpi, mb86s70_gpio_acpi_ids);
+#endif
+
 static struct platform_driver mb86s70_gpio_driver = {
 	.driver = {
 		.name = "mb86s70-gpio",
 		.of_match_table = mb86s70_gpio_dt_ids,
+		.acpi_match_table = ACPI_PTR(mb86s70_gpio_acpi_ids),
 	},
 	.probe = mb86s70_gpio_probe,
 	.remove = mb86s70_gpio_remove,
-- 
2.20.1

Re: [RFC PATCH 3/3] gpio: mb86s70: enable ACPI and irqchip support

[Linus Walleij]

Hi Ard, thanks for your patch!

Including Mika here as well, he knows how ACPI is supposed to
be wired up with GPIOs.

On Thu, Apr 25, 2019 at 12:20 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:

> In order to support this GPIO block in combination with an EXIU
> interrupt controller on ACPI systems such as Socionext SynQuacer,
> add the enumeration boilerplate, and add the EXIU irqchip handling
> to the probe path. Also, make the clock handling conditonal on
> non-ACPI enumeration, since ACPI handles this internally.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
(...)

> +config GPIO_MB86S7X_ACPI
> +       def_bool y
> +       depends on ACPI && ARCH_SYNQUACER

I would say

depends on ACPI
depends on (ARCH_SYNQUACER || COMPILE_TEST)

so we get some compiler coverage.

> +#include <linux/acpi.h>
>  #include <linux/io.h>
>  #include <linux/init.h>
>  #include <linux/clk.h>
> @@ -27,6 +28,8 @@
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
>
> +#include "gpiolib.h"

Normally not a good sign to dereference gpiolib internals,
but there are some few exceptions. Why do you do this?

> +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc);

Just merge it all into one file instead of having these crisscrossy calls.
I prefer some minor #ifdef or straight C if (IS_ENABLED()) around
specific code.

> -       return ret;
> +       if (mb86s70_gpio_have_acpi(pdev))
> +               acpi_gpiochip_request_interrupts(&gchip->gc);
> +
> +       return 0;

I guess this is right...

>         struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);
>
> +       if (gchip->gc.irq.domain) {
> +               acpi_gpiochip_free_interrupts(&gchip->gc);
> +               irq_domain_remove(gchip->gc.irq.domain);
> +       }

Mika can possibly comment on the right way to do ACPI bring-up
and take-down.

Overall it looks pretty straightforward to me, but I'd prefer to just
merge the irqchip driver over into the gpio driver, I can't see why
not.

Or is the irqchip used without the GPIO driver sometimes?

Yours,
Linus Walleij

Re: [RFC PATCH 3/3] gpio: mb86s70: enable ACPI and irqchip support

[Ard Biesheuvel]

On Thu, 25 Apr 2019 at 15:24, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Ard, thanks for your patch!
>
> Including Mika here as well, he knows how ACPI is supposed to
> be wired up with GPIOs.
>
> On Thu, Apr 25, 2019 at 12:20 PM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>
> > In order to support this GPIO block in combination with an EXIU
> > interrupt controller on ACPI systems such as Socionext SynQuacer,
> > add the enumeration boilerplate, and add the EXIU irqchip handling
> > to the probe path. Also, make the clock handling conditonal on
> > non-ACPI enumeration, since ACPI handles this internally.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> (...)
>
> > +config GPIO_MB86S7X_ACPI
> > +       def_bool y
> > +       depends on ACPI && ARCH_SYNQUACER
>
> I would say
>
> depends on ACPI
> depends on (ARCH_SYNQUACER || COMPILE_TEST)
>
> so we get some compiler coverage.
>
> > +#include <linux/acpi.h>
> >  #include <linux/io.h>
> >  #include <linux/init.h>
> >  #include <linux/clk.h>
> > @@ -27,6 +28,8 @@
> >  #include <linux/spinlock.h>
> >  #include <linux/slab.h>
> >
> > +#include "gpiolib.h"
>
> Normally not a good sign to dereference gpiolib internals,
> but there are some few exceptions. Why do you do this?
>

I needed this for acpi_gpiochip_request_interrupts() et al

> > +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc);
>
> Just merge it all into one file instead of having these crisscrossy calls.
> I prefer some minor #ifdef or straight C if (IS_ENABLED()) around
> specific code.
>

Fair enough, but please see below.

> > -       return ret;
> > +       if (mb86s70_gpio_have_acpi(pdev))
> > +               acpi_gpiochip_request_interrupts(&gchip->gc);
> > +
> > +       return 0;
>
> I guess this is right...
>
> >         struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);
> >
> > +       if (gchip->gc.irq.domain) {
> > +               acpi_gpiochip_free_interrupts(&gchip->gc);
> > +               irq_domain_remove(gchip->gc.irq.domain);
> > +       }
>
> Mika can possibly comment on the right way to do ACPI bring-up
> and take-down.
>
> Overall it looks pretty straightforward to me, but I'd prefer to just
> merge the irqchip driver over into the gpio driver, I can't see why
> not.
>
> Or is the irqchip used without the GPIO driver sometimes?
>

This is where it becomes a bit nasty: the GPIO controller and the EXIU
interrupt controller are two separate IP blocks, but they are
obviously complimentary. *However*, on SynQuacer, the first 4 physical
lines are not shared between the two, and so you could have 4
interrupt lines handled through the EXIU and 4 different GPIO lines
through the GPIO unit.

On DT, we don't care since we model them as two different units. On
ACPI, however, we cannot model interrupt controllers in the same way,
and so I decided to merge them.

Perhaps the solution is to model the EXIU as a pseudo-GPIO block that
only supports interrupts and not ordinary GPIO?

Re: [RFC PATCH 3/3] gpio: mb86s70: enable ACPI and irqchip support

[Mika Westerberg]

On Thu, Apr 25, 2019 at 03:23:53PM +0200, Linus Walleij wrote:
> > -       return ret;
> > +       if (mb86s70_gpio_have_acpi(pdev))
> > +               acpi_gpiochip_request_interrupts(&gchip->gc);
> > +
> > +       return 0;
> 
> I guess this is right...
> 
> >         struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);
> >
> > +       if (gchip->gc.irq.domain) {
> > +               acpi_gpiochip_free_interrupts(&gchip->gc);
> > +               irq_domain_remove(gchip->gc.irq.domain);
> > +       }
> 
> Mika can possibly comment on the right way to do ACPI bring-up
> and take-down.

Only comment I have is that normally you don't need to call
acpi_gpiochip_request_interrupts() and acpi_gpiochip_free_interrupts()
in GPIO chip drivers itself. The core should take care of this if the
device ACPI description has an _AEI method.

Re: [RFC PATCH 3/3] gpio: mb86s70: enable ACPI and irqchip support

[Ard Biesheuvel]

On Thu, 25 Apr 2019 at 16:27, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Thu, Apr 25, 2019 at 03:23:53PM +0200, Linus Walleij wrote:
> > > -       return ret;
> > > +       if (mb86s70_gpio_have_acpi(pdev))
> > > +               acpi_gpiochip_request_interrupts(&gchip->gc);
> > > +
> > > +       return 0;
> >
> > I guess this is right...
> >
> > >         struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);
> > >
> > > +       if (gchip->gc.irq.domain) {
> > > +               acpi_gpiochip_free_interrupts(&gchip->gc);
> > > +               irq_domain_remove(gchip->gc.irq.domain);
> > > +       }
> >
> > Mika can possibly comment on the right way to do ACPI bring-up
> > and take-down.
>
> Only comment I have is that normally you don't need to call
> acpi_gpiochip_request_interrupts() and acpi_gpiochip_free_interrupts()
> in GPIO chip drivers itself. The core should take care of this if the
> device ACPI description has an _AEI method.

Thanks Mika.

The core only takes care of this for nested/cascaded domains. In this
case, the calls are needed, or the ACPI event interrupt don't get
registered.

Re: [RFC PATCH 3/3] gpio: mb86s70: enable ACPI and irqchip support

[Mika Westerberg]

On Fri, Apr 26, 2019 at 10:19:42AM +0200, Ard Biesheuvel wrote:
> On Thu, 25 Apr 2019 at 16:27, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Thu, Apr 25, 2019 at 03:23:53PM +0200, Linus Walleij wrote:
> > > > -       return ret;
> > > > +       if (mb86s70_gpio_have_acpi(pdev))
> > > > +               acpi_gpiochip_request_interrupts(&gchip->gc);
> > > > +
> > > > +       return 0;
> > >
> > > I guess this is right...
> > >
> > > >         struct mb86s70_gpio_chip *gchip = platform_get_drvdata(pdev);
> > > >
> > > > +       if (gchip->gc.irq.domain) {
> > > > +               acpi_gpiochip_free_interrupts(&gchip->gc);
> > > > +               irq_domain_remove(gchip->gc.irq.domain);
> > > > +       }
> > >
> > > Mika can possibly comment on the right way to do ACPI bring-up
> > > and take-down.
> >
> > Only comment I have is that normally you don't need to call
> > acpi_gpiochip_request_interrupts() and acpi_gpiochip_free_interrupts()
> > in GPIO chip drivers itself. The core should take care of this if the
> > device ACPI description has an _AEI method.
> 
> Thanks Mika.
> 
> The core only takes care of this for nested/cascaded domains. In this
> case, the calls are needed, or the ACPI event interrupt don't get
> registered.

I see. Then I agree calling those directly in the driver is the right
thing to do.