[v2,04/35] irq: Add a method to convert an interrupt to ACPI

Message ID 20200510143320.v2.4.I29adaf758a9ea81f51ffbecf89b53983968a0d2a@changeid
State Accepted
Commit f4955137f5f15e615376cf38559414a9b53e3d55
Headers show
Series
  • dm: Add programmatic generation of ACPI tables (part B)
Related show

Commit Message

Simon Glass May 10, 2020, 8:33 p.m.
When generating ACPI tables we need to convert IRQs in U-Boot to the ACPI
structures required by ACPI. This is a SoC-specific conversion and cannot
be handled by generic code, so add a new IRQ method to do the conversion.

Signed-off-by: Simon Glass <sjg at chromium.org>
---

Changes in v2: None
Changes in v1:
- Fix 'the an' typo
- Move header definitions into this patch

 drivers/misc/irq-uclass.c  | 18 ++++++++++--
 drivers/misc/irq_sandbox.c | 16 +++++++++++
 include/acpi/acpi_device.h | 59 ++++++++++++++++++++++++++++++++++++++
 include/irq.h              | 43 +++++++++++++++++++++++++++
 test/dm/irq.c              | 22 ++++++++++++++
 5 files changed, 156 insertions(+), 2 deletions(-)

Comments

Wolfgang Wallner May 13, 2020, 1:01 p.m. | #1
Hi Simon,

-----"Simon Glass" <sjg at chromium.org> schrieb: -----
>Betreff: [PATCH v2 04/35] irq: Add a method to convert an interrupt
>to ACPI
>
>When generating ACPI tables we need to convert IRQs in U-Boot to the
>ACPI
>structures required by ACPI. This is a SoC-specific conversion and
>cannot
>be handled by generic code, so add a new IRQ method to do the
>conversion.
>
>Signed-off-by: Simon Glass <sjg at chromium.org>
>---
>
>Changes in v2: None
>Changes in v1:
>- Fix 'the an' typo
>- Move header definitions into this patch
>
> drivers/misc/irq-uclass.c  | 18 ++++++++++--
> drivers/misc/irq_sandbox.c | 16 +++++++++++
> include/acpi/acpi_device.h | 59
>++++++++++++++++++++++++++++++++++++++
> include/irq.h              | 43 +++++++++++++++++++++++++++
> test/dm/irq.c              | 22 ++++++++++++++
> 5 files changed, 156 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
>index 16dc0be75c..98bc79eaba 100644
>--- a/drivers/misc/irq-uclass.c
>+++ b/drivers/misc/irq-uclass.c
>@@ -154,8 +154,6 @@ int irq_request(struct udevice *dev, struct irq
>*irq)
> 	const struct irq_ops *ops;
> 
> 	log_debug("(dev=%p, irq=%p)\n", dev, irq);
>-	if (!irq)
>-		return 0;

Why are these lines dropped?

As far as I understand the code they can be dropped, I just fail
to see how that is related to the ACPI changes.

> 	ops = irq_get_ops(dev);
> 
> 	irq->dev = dev;
>@@ -177,6 +175,22 @@ int irq_first_device_type(enum irq_dev_t type,
>struct udevice **devp)
> 	return 0;
> }
> 
>+#if CONFIG_IS_ENABLED(ACPIGEN)
>+int irq_get_acpi(const struct irq *irq, struct acpi_irq *acpi_irq)
>+{
>+	struct irq_ops *ops;
>+
>+	if (!irq_is_valid(irq))
>+		return -EINVAL;
>+
>+	ops = irq_get_ops(irq->dev);
>+	if (!ops->get_acpi)
>+		return -ENOSYS;
>+
>+	return ops->get_acpi(irq, acpi_irq);
>+}
>+#endif
>+
> UCLASS_DRIVER(irq) = {
> 	.id		= UCLASS_IRQ,
> 	.name		= "irq",
>diff --git a/drivers/misc/irq_sandbox.c b/drivers/misc/irq_sandbox.c
>index 54bc47c8d8..a2511b32fc 100644
>--- a/drivers/misc/irq_sandbox.c
>+++ b/drivers/misc/irq_sandbox.c
>@@ -8,6 +8,7 @@
> #include <common.h>
> #include <dm.h>
> #include <irq.h>
>+#include <acpi/acpi_device.h>
> #include <asm/test.h>
> 
> /**
>@@ -73,6 +74,18 @@ static int sandbox_irq_of_xlate(struct irq *irq,
> 	return 0;
> }
> 
>+static __maybe_unused int sandbox_get_acpi(const struct irq *irq,
>+					   struct acpi_irq *acpi_irq)
>+{
>+	acpi_irq->pin = irq->id;
>+	acpi_irq->mode = ACPI_IRQ_LEVEL_TRIGGERED;
>+	acpi_irq->polarity = ACPI_IRQ_ACTIVE_HIGH;
>+	acpi_irq->shared = ACPI_IRQ_SHARED;
>+	acpi_irq->wake = ACPI_IRQ_WAKE;
>+
>+	return 0;
>+}
>+
> static const struct irq_ops sandbox_irq_ops = {
> 	.route_pmc_gpio_gpe	= sandbox_route_pmc_gpio_gpe,
> 	.set_polarity		= sandbox_set_polarity,
>@@ -80,6 +93,9 @@ static const struct irq_ops sandbox_irq_ops = {
> 	.restore_polarities	= sandbox_restore_polarities,
> 	.read_and_clear		= sandbox_irq_read_and_clear,
> 	.of_xlate		= sandbox_irq_of_xlate,
>+#if CONFIG_IS_ENABLED(ACPIGEN)
>+	.get_acpi		= sandbox_get_acpi,
>+#endif
> };
> 
> static const struct udevice_id sandbox_irq_ids[] = {
>diff --git a/include/acpi/acpi_device.h b/include/acpi/acpi_device.h
>index 09c227489a..24895de0da 100644
>--- a/include/acpi/acpi_device.h
>+++ b/include/acpi/acpi_device.h
>@@ -13,6 +13,12 @@
> 
> struct udevice;
> 
>+/* ACPI descriptor values for common descriptors: SERIAL_BUS means
>I2C */

I don't understand the comment above. Why does SERIAL_BUS mean I2C?
It could also mean SPI, or am I missing something?

>+#define ACPI_DESCRIPTOR_LARGE		BIT(7)
>+#define ACPI_DESCRIPTOR_INTERRUPT	(ACPI_DESCRIPTOR_LARGE | 9)
>+#define ACPI_DESCRIPTOR_GPIO		(ACPI_DESCRIPTOR_LARGE | 12)
>+#define ACPI_DESCRIPTOR_SERIAL_BUS	(ACPI_DESCRIPTOR_LARGE | 14)
>+
> /* Length of a full path to an ACPI device */
> #define ACPI_PATH_MAX		30
> 
>@@ -31,6 +37,59 @@ enum acpi_dev_status {
> 		ACPI_DSTATUS_SHOW_IN_UI,
> };
> 
>+/** enum acpi_irq_mode - edge/level trigger mode */
>+enum acpi_irq_mode {
>+	ACPI_IRQ_EDGE_TRIGGERED,
>+	ACPI_IRQ_LEVEL_TRIGGERED,
>+};
>+
>+/**
>+ * enum acpi_irq_polarity - polarity of interrupt
>+ *
>+ * @ACPI_IRQ_ACTIVE_LOW - for ACPI_IRQ_EDGE_TRIGGERED this means
>falling edge
>+ * @ACPI_IRQ_ACTIVE_HIGH - for ACPI_IRQ_EDGE_TRIGGERED this means
>rising edge
>+ * @ACPI_IRQ_ACTIVE_BOTH - not meaningful for
>ACPI_IRQ_EDGE_TRIGGERED
>+ */
>+enum acpi_irq_polarity {
>+	ACPI_IRQ_ACTIVE_LOW,
>+	ACPI_IRQ_ACTIVE_HIGH,
>+	ACPI_IRQ_ACTIVE_BOTH,
>+};
>+
>+/**
>+ * enum acpi_irq_shared - whether interrupt is shared or not
>+ *
>+ * @ACPI_IRQ_EXCLUSIVE: only this device uses the interrupt
>+ * @ACPI_IRQ_SHARED: other devices may use this interrupt
>+ */
>+enum acpi_irq_shared {
>+	ACPI_IRQ_EXCLUSIVE,
>+	ACPI_IRQ_SHARED,
>+};
>+
>+/** enum acpi_irq_wake - indicates whether this interrupt can wake
>the device */
>+enum acpi_irq_wake {
>+	ACPI_IRQ_NO_WAKE,
>+	ACPI_IRQ_WAKE,
>+};
>+
>+/**
>+ * struct acpi_irq - representation of an ACPI interrupt
>+ *
>+ * @pin: ACPI pin that is monitored for the interrupt
>+ * @mode: Edge/level triggering
>+ * @polarity: Interrupt polarity
>+ * @shared: Whether interrupt is shared or not
>+ * @wake: Whether interrupt can wake the device from sleep
>+ */
>+struct acpi_irq {
>+	unsigned int pin;
>+	enum acpi_irq_mode mode;
>+	enum acpi_irq_polarity polarity;
>+	enum acpi_irq_shared shared;
>+	enum acpi_irq_wake wake;
>+};
>+
> /**
>  * acpi_device_path() - Get the full path to an ACPI device
>  *
>diff --git a/include/irq.h b/include/irq.h
>index b71afe9bee..8527e4dd79 100644
>--- a/include/irq.h
>+++ b/include/irq.h
>@@ -8,6 +8,9 @@
> #ifndef __irq_H
> #define __irq_H
> 
>+struct acpi_irq;
>+struct ofnode_phandle_args;
>+
> /*
>  * Interrupt controller types available. You can find a particular
>one with
>  * irq_first_device_type()
>@@ -24,10 +27,12 @@ enum irq_dev_t {
>  *
>  * @dev: IRQ device that handles this irq
>  * @id: ID to identify this irq with the device
>+ * @flags: Flags associated with this interrupt (IRQ_TYPE_...)
>  */
> struct irq {
> 	struct udevice *dev;
> 	ulong id;
>+	ulong flags;
> };
> 
> /**
>@@ -119,10 +124,36 @@ struct irq_ops {
> 	 * @return 0 if OK, or a negative error code.
> 	 */
> 	int (*free)(struct irq *irq);
>+
>+#if CONFIG_IS_ENABLED(ACPIGEN)
>+	/**
>+	 * get_acpi() - Get the ACPI info for an irq
>+	 *
>+	 * This converts a irq to an ACPI structure for adding to the ACPI
>+	 * tables.
>+	 *
>+	 * @irq:	irq to convert
>+	 * @acpi_irq:	Output ACPI interrupt information
>+	 * @return ACPI pin number or -ve on error
>+	 */
>+	int (*get_acpi)(const struct irq *irq, struct acpi_irq *acpi_irq);
>+#endif
> };
> 
> #define irq_get_ops(dev)	((struct irq_ops *)(dev)->driver->ops)
> 
>+/**
>+ * irq_is_valid() - Check if an IRQ is valid
>+ *
>+ * @irq:	IRQ description containing device and ID, e.g. previously
>+ *		returned by irq_get_by_index()
>+ * @return true if valid, false if not
>+ */
>+static inline bool irq_is_valid(const struct irq *irq)
>+{
>+	return irq->dev != NULL;
>+}
>+
> /**
>  * irq_route_pmc_gpio_gpe() - Get the GPIO for an event
>  *
>@@ -223,4 +254,16 @@ int irq_free(struct irq *irq);
>  */
> int irq_first_device_type(enum irq_dev_t type, struct udevice
>**devp);
> 
>+/**
>+ * irq_get_acpi() - Get the ACPI info for an irq
>+ *
>+ * This converts a irq to an ACPI structure for adding to the ACPI
>+ * tables.
>+ *
>+ * @irq:	irq to convert
>+ * @acpi_irq:	Output ACPI interrupt information
>+ * @return ACPI pin number or -ve on error
>+ */
>+int irq_get_acpi(const struct irq *irq, struct acpi_irq *acpi_irq);
>+
> #endif
>diff --git a/test/dm/irq.c b/test/dm/irq.c
>index 192d80d7e1..50e505e657 100644
>--- a/test/dm/irq.c
>+++ b/test/dm/irq.c
>@@ -8,6 +8,7 @@
> #include <common.h>
> #include <dm.h>
> #include <irq.h>
>+#include <acpi/acpi_device.h>
> #include <asm/test.h>
> #include <dm/test.h>
> #include <test/ut.h>
>@@ -75,3 +76,24 @@ static int dm_test_request(struct unit_test_state
>*uts)
> 	return 0;
> }
> DM_TEST(dm_test_request, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
>+
>+/* Test of irq_get_acpi() */
>+static int dm_test_irq_get_acpi(struct unit_test_state *uts)
>+{
>+	struct acpi_irq airq;
>+	struct udevice *dev;
>+	struct irq irq;
>+
>+	ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev));
>+	ut_assertok(irq_get_by_index(dev, 0, &irq));
>+
>+	ut_assertok(irq_get_acpi(&irq, &airq));
>+	ut_asserteq(3, airq.pin);
>+	ut_asserteq(ACPI_IRQ_LEVEL_TRIGGERED, airq.mode);
>+	ut_asserteq(ACPI_IRQ_ACTIVE_HIGH, airq.polarity);
>+	ut_asserteq(ACPI_IRQ_SHARED, airq.shared);
>+	ut_asserteq(ACPI_IRQ_WAKE, airq.wake);
>+
>+	return 0;
>+}
>+DM_TEST(dm_test_irq_get_acpi, DM_TESTF_SCAN_PDATA |
>DM_TESTF_SCAN_FDT);
>-- 
>2.26.2.645.ge9eca65c58-goog
>

regards, Wolfgang
Simon Glass May 14, 2020, 4:02 p.m. | #2
Hi Wolfgang,

On Wed, 13 May 2020 at 07:01, Wolfgang Wallner
<wolfgang.wallner at br-automation.com> wrote:
>
> Hi Simon,
>
> -----"Simon Glass" <sjg at chromium.org> schrieb: -----
> >Betreff: [PATCH v2 04/35] irq: Add a method to convert an interrupt
> >to ACPI
> >
> >When generating ACPI tables we need to convert IRQs in U-Boot to the
> >ACPI
> >structures required by ACPI. This is a SoC-specific conversion and
> >cannot
> >be handled by generic code, so add a new IRQ method to do the
> >conversion.
> >
> >Signed-off-by: Simon Glass <sjg at chromium.org>
> >---
> >
> >Changes in v2: None
> >Changes in v1:
> >- Fix 'the an' typo
> >- Move header definitions into this patch
> >
> > drivers/misc/irq-uclass.c  | 18 ++++++++++--
> > drivers/misc/irq_sandbox.c | 16 +++++++++++
> > include/acpi/acpi_device.h | 59
> >++++++++++++++++++++++++++++++++++++++
> > include/irq.h              | 43 +++++++++++++++++++++++++++
> > test/dm/irq.c              | 22 ++++++++++++++
> > 5 files changed, 156 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
> >index 16dc0be75c..98bc79eaba 100644
> >--- a/drivers/misc/irq-uclass.c
> >+++ b/drivers/misc/irq-uclass.c
> >@@ -154,8 +154,6 @@ int irq_request(struct udevice *dev, struct irq
> >*irq)
> >       const struct irq_ops *ops;
> >
> >       log_debug("(dev=%p, irq=%p)\n", dev, irq);
> >-      if (!irq)
> >-              return 0;
>
> Why are these lines dropped?

Because we are not allowed to pass a NULL irq to this function. That
check should not have been there.

>
> As far as I understand the code they can be dropped, I just fail
> to see how that is related to the ACPI changes.
>
> >       ops = irq_get_ops(dev);
> >
> >       irq->dev = dev;
> >@@ -177,6 +175,22 @@ int irq_first_device_type(enum irq_dev_t type,
> >struct udevice **devp)
> >       return 0;
> > }


> >
> >+#if CONFIG_IS_ENABLED(ACPIGEN)
> >+int irq_get_acpi(const struct irq *irq, struct acpi_irq *acpi_irq)
> >+{
> >+      struct irq_ops *ops;
> >+
> >+      if (!irq_is_valid(irq))
> >+              return -EINVAL;
> >+
> >+      ops = irq_get_ops(irq->dev);
> >+      if (!ops->get_acpi)
> >+              return -ENOSYS;
> >+
> >+      return ops->get_acpi(irq, acpi_irq);
> >+}
> >+#endif
> >+
> > UCLASS_DRIVER(irq) = {
> >       .id             = UCLASS_IRQ,
> >       .name           = "irq",
> >diff --git a/drivers/misc/irq_sandbox.c b/drivers/misc/irq_sandbox.c
> >index 54bc47c8d8..a2511b32fc 100644
> >--- a/drivers/misc/irq_sandbox.c
> >+++ b/drivers/misc/irq_sandbox.c
> >@@ -8,6 +8,7 @@
> > #include <common.h>
> > #include <dm.h>
> > #include <irq.h>
> >+#include <acpi/acpi_device.h>
> > #include <asm/test.h>
> >
> > /**
> >@@ -73,6 +74,18 @@ static int sandbox_irq_of_xlate(struct irq *irq,
> >       return 0;
> > }
> >
> >+static __maybe_unused int sandbox_get_acpi(const struct irq *irq,
> >+                                         struct acpi_irq *acpi_irq)
> >+{
> >+      acpi_irq->pin = irq->id;
> >+      acpi_irq->mode = ACPI_IRQ_LEVEL_TRIGGERED;
> >+      acpi_irq->polarity = ACPI_IRQ_ACTIVE_HIGH;
> >+      acpi_irq->shared = ACPI_IRQ_SHARED;
> >+      acpi_irq->wake = ACPI_IRQ_WAKE;
> >+
> >+      return 0;
> >+}
> >+
> > static const struct irq_ops sandbox_irq_ops = {
> >       .route_pmc_gpio_gpe     = sandbox_route_pmc_gpio_gpe,
> >       .set_polarity           = sandbox_set_polarity,
> >@@ -80,6 +93,9 @@ static const struct irq_ops sandbox_irq_ops = {
> >       .restore_polarities     = sandbox_restore_polarities,
> >       .read_and_clear         = sandbox_irq_read_and_clear,
> >       .of_xlate               = sandbox_irq_of_xlate,
> >+#if CONFIG_IS_ENABLED(ACPIGEN)
> >+      .get_acpi               = sandbox_get_acpi,
> >+#endif
> > };
> >
> > static const struct udevice_id sandbox_irq_ids[] = {
> >diff --git a/include/acpi/acpi_device.h b/include/acpi/acpi_device.h
> >index 09c227489a..24895de0da 100644
> >--- a/include/acpi/acpi_device.h
> >+++ b/include/acpi/acpi_device.h
> >@@ -13,6 +13,12 @@
> >
> > struct udevice;
> >
> >+/* ACPI descriptor values for common descriptors: SERIAL_BUS means
> >I2C */
>
> I don't understand the comment above. Why does SERIAL_BUS mean I2C?
> It could also mean SPI, or am I missing something?

Just that descriptor 14 is used for serial bus (UART, SPI, I2C) and in
this code it is used for I2C. I didn't want to call it I2C since that
would be inaccurate, hence the comment.

>
> >+#define ACPI_DESCRIPTOR_LARGE         BIT(7)
> >+#define ACPI_DESCRIPTOR_INTERRUPT     (ACPI_DESCRIPTOR_LARGE | 9)
> >+#define ACPI_DESCRIPTOR_GPIO          (ACPI_DESCRIPTOR_LARGE | 12)
> >+#define ACPI_DESCRIPTOR_SERIAL_BUS    (ACPI_DESCRIPTOR_LARGE | 14)
> >+

Regards,
Simon
Wolfgang Wallner May 18, 2020, 7:47 a.m. | #3
Hi Simon,

-----"Simon Glass" <sjg at chromium.org> schrieb: -----
> Betreff: Re: [PATCH v2 04/35] irq: Add a method to convert an interrupt to ACPI
> 
> Hi Wolfgang,
> 
> On Wed, 13 May 2020 at 07:01, Wolfgang Wallner
> <wolfgang.wallner at br-automation.com> wrote:
> >
> > Hi Simon,
> >
> > -----"Simon Glass" <sjg at chromium.org> schrieb: -----
> > >Betreff: [PATCH v2 04/35] irq: Add a method to convert an interrupt
> > >to ACPI
> > >
> > >When generating ACPI tables we need to convert IRQs in U-Boot to the
> > >ACPI
> > >structures required by ACPI. This is a SoC-specific conversion and
> > >cannot
> > >be handled by generic code, so add a new IRQ method to do the
> > >conversion.
> > >
> > >Signed-off-by: Simon Glass <sjg at chromium.org>
> > >---
> > >
> > >Changes in v2: None
> > >Changes in v1:
> > >- Fix 'the an' typo
> > >- Move header definitions into this patch
> > >
> > > drivers/misc/irq-uclass.c  | 18 ++++++++++--
> > > drivers/misc/irq_sandbox.c | 16 +++++++++++
> > > include/acpi/acpi_device.h | 59
> > >++++++++++++++++++++++++++++++++++++++
> > > include/irq.h              | 43 +++++++++++++++++++++++++++
> > > test/dm/irq.c              | 22 ++++++++++++++
> > > 5 files changed, 156 insertions(+), 2 deletions(-)
> > >
> > >diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
> > >index 16dc0be75c..98bc79eaba 100644
> > >--- a/drivers/misc/irq-uclass.c
> > >+++ b/drivers/misc/irq-uclass.c
> > >@@ -154,8 +154,6 @@ int irq_request(struct udevice *dev, struct irq
> > >*irq)
> > >       const struct irq_ops *ops;
> > >
> > >       log_debug("(dev=%p, irq=%p)\n", dev, irq);
> > >-      if (!irq)
> > >-              return 0;
> >
> > Why are these lines dropped?
> 
> Because we are not allowed to pass a NULL irq to this function. That
> check should not have been there.
> 
> >
> > As far as I understand the code they can be dropped, I just fail
> > to see how that is related to the ACPI changes.
> >
> > >       ops = irq_get_ops(dev);
> > >
> > >       irq->dev = dev;
> > >@@ -177,6 +175,22 @@ int irq_first_device_type(enum irq_dev_t type,
> > >struct udevice **devp)
> > >       return 0;
> > > }
> 
> 
> > >
> > >+#if CONFIG_IS_ENABLED(ACPIGEN)
> > >+int irq_get_acpi(const struct irq *irq, struct acpi_irq *acpi_irq)
> > >+{
> > >+      struct irq_ops *ops;
> > >+
> > >+      if (!irq_is_valid(irq))
> > >+              return -EINVAL;
> > >+
> > >+      ops = irq_get_ops(irq->dev);
> > >+      if (!ops->get_acpi)
> > >+              return -ENOSYS;
> > >+
> > >+      return ops->get_acpi(irq, acpi_irq);
> > >+}
> > >+#endif
> > >+
> > > UCLASS_DRIVER(irq) = {
> > >       .id             = UCLASS_IRQ,
> > >       .name           = "irq",
> > >diff --git a/drivers/misc/irq_sandbox.c b/drivers/misc/irq_sandbox.c
> > >index 54bc47c8d8..a2511b32fc 100644
> > >--- a/drivers/misc/irq_sandbox.c
> > >+++ b/drivers/misc/irq_sandbox.c
> > >@@ -8,6 +8,7 @@
> > > #include <common.h>
> > > #include <dm.h>
> > > #include <irq.h>
> > >+#include <acpi/acpi_device.h>
> > > #include <asm/test.h>
> > >
> > > /**
> > >@@ -73,6 +74,18 @@ static int sandbox_irq_of_xlate(struct irq *irq,
> > >       return 0;
> > > }
> > >
> > >+static __maybe_unused int sandbox_get_acpi(const struct irq *irq,
> > >+                                         struct acpi_irq *acpi_irq)
> > >+{
> > >+      acpi_irq->pin = irq->id;
> > >+      acpi_irq->mode = ACPI_IRQ_LEVEL_TRIGGERED;
> > >+      acpi_irq->polarity = ACPI_IRQ_ACTIVE_HIGH;
> > >+      acpi_irq->shared = ACPI_IRQ_SHARED;
> > >+      acpi_irq->wake = ACPI_IRQ_WAKE;
> > >+
> > >+      return 0;
> > >+}
> > >+
> > > static const struct irq_ops sandbox_irq_ops = {
> > >       .route_pmc_gpio_gpe     = sandbox_route_pmc_gpio_gpe,
> > >       .set_polarity           = sandbox_set_polarity,
> > >@@ -80,6 +93,9 @@ static const struct irq_ops sandbox_irq_ops = {
> > >       .restore_polarities     = sandbox_restore_polarities,
> > >       .read_and_clear         = sandbox_irq_read_and_clear,
> > >       .of_xlate               = sandbox_irq_of_xlate,
> > >+#if CONFIG_IS_ENABLED(ACPIGEN)
> > >+      .get_acpi               = sandbox_get_acpi,
> > >+#endif
> > > };
> > >
> > > static const struct udevice_id sandbox_irq_ids[] = {
> > >diff --git a/include/acpi/acpi_device.h b/include/acpi/acpi_device.h
> > >index 09c227489a..24895de0da 100644
> > >--- a/include/acpi/acpi_device.h
> > >+++ b/include/acpi/acpi_device.h
> > >@@ -13,6 +13,12 @@
> > >
> > > struct udevice;
> > >
> > >+/* ACPI descriptor values for common descriptors: SERIAL_BUS means
> > >I2C */
> >
> > I don't understand the comment above. Why does SERIAL_BUS mean I2C?
> > It could also mean SPI, or am I missing something?
> 
> Just that descriptor 14 is used for serial bus (UART, SPI, I2C) and in
> this code it is used for I2C. I didn't want to call it I2C since that
> would be inaccurate, hence the comment.

Nit: I would suggest to either drop or expand "SERIAL_BUS means I2C".
In its current form that comment confused me more than it helped me.

> 
> >
> > >+#define ACPI_DESCRIPTOR_LARGE         BIT(7)
> > >+#define ACPI_DESCRIPTOR_INTERRUPT     (ACPI_DESCRIPTOR_LARGE | 9)
> > >+#define ACPI_DESCRIPTOR_GPIO          (ACPI_DESCRIPTOR_LARGE | 12)
> > >+#define ACPI_DESCRIPTOR_SERIAL_BUS    (ACPI_DESCRIPTOR_LARGE | 14)
> > >+
> 
> Regards,
> Simon

Reviewed-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com>

Patch

diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
index 16dc0be75c..98bc79eaba 100644
--- a/drivers/misc/irq-uclass.c
+++ b/drivers/misc/irq-uclass.c
@@ -154,8 +154,6 @@  int irq_request(struct udevice *dev, struct irq *irq)
 	const struct irq_ops *ops;
 
 	log_debug("(dev=%p, irq=%p)\n", dev, irq);
-	if (!irq)
-		return 0;
 	ops = irq_get_ops(dev);
 
 	irq->dev = dev;
@@ -177,6 +175,22 @@  int irq_first_device_type(enum irq_dev_t type, struct udevice **devp)
 	return 0;
 }
 
+#if CONFIG_IS_ENABLED(ACPIGEN)
+int irq_get_acpi(const struct irq *irq, struct acpi_irq *acpi_irq)
+{
+	struct irq_ops *ops;
+
+	if (!irq_is_valid(irq))
+		return -EINVAL;
+
+	ops = irq_get_ops(irq->dev);
+	if (!ops->get_acpi)
+		return -ENOSYS;
+
+	return ops->get_acpi(irq, acpi_irq);
+}
+#endif
+
 UCLASS_DRIVER(irq) = {
 	.id		= UCLASS_IRQ,
 	.name		= "irq",
diff --git a/drivers/misc/irq_sandbox.c b/drivers/misc/irq_sandbox.c
index 54bc47c8d8..a2511b32fc 100644
--- a/drivers/misc/irq_sandbox.c
+++ b/drivers/misc/irq_sandbox.c
@@ -8,6 +8,7 @@ 
 #include <common.h>
 #include <dm.h>
 #include <irq.h>
+#include <acpi/acpi_device.h>
 #include <asm/test.h>
 
 /**
@@ -73,6 +74,18 @@  static int sandbox_irq_of_xlate(struct irq *irq,
 	return 0;
 }
 
+static __maybe_unused int sandbox_get_acpi(const struct irq *irq,
+					   struct acpi_irq *acpi_irq)
+{
+	acpi_irq->pin = irq->id;
+	acpi_irq->mode = ACPI_IRQ_LEVEL_TRIGGERED;
+	acpi_irq->polarity = ACPI_IRQ_ACTIVE_HIGH;
+	acpi_irq->shared = ACPI_IRQ_SHARED;
+	acpi_irq->wake = ACPI_IRQ_WAKE;
+
+	return 0;
+}
+
 static const struct irq_ops sandbox_irq_ops = {
 	.route_pmc_gpio_gpe	= sandbox_route_pmc_gpio_gpe,
 	.set_polarity		= sandbox_set_polarity,
@@ -80,6 +93,9 @@  static const struct irq_ops sandbox_irq_ops = {
 	.restore_polarities	= sandbox_restore_polarities,
 	.read_and_clear		= sandbox_irq_read_and_clear,
 	.of_xlate		= sandbox_irq_of_xlate,
+#if CONFIG_IS_ENABLED(ACPIGEN)
+	.get_acpi		= sandbox_get_acpi,
+#endif
 };
 
 static const struct udevice_id sandbox_irq_ids[] = {
diff --git a/include/acpi/acpi_device.h b/include/acpi/acpi_device.h
index 09c227489a..24895de0da 100644
--- a/include/acpi/acpi_device.h
+++ b/include/acpi/acpi_device.h
@@ -13,6 +13,12 @@ 
 
 struct udevice;
 
+/* ACPI descriptor values for common descriptors: SERIAL_BUS means I2C */
+#define ACPI_DESCRIPTOR_LARGE		BIT(7)
+#define ACPI_DESCRIPTOR_INTERRUPT	(ACPI_DESCRIPTOR_LARGE | 9)
+#define ACPI_DESCRIPTOR_GPIO		(ACPI_DESCRIPTOR_LARGE | 12)
+#define ACPI_DESCRIPTOR_SERIAL_BUS	(ACPI_DESCRIPTOR_LARGE | 14)
+
 /* Length of a full path to an ACPI device */
 #define ACPI_PATH_MAX		30
 
@@ -31,6 +37,59 @@  enum acpi_dev_status {
 		ACPI_DSTATUS_SHOW_IN_UI,
 };
 
+/** enum acpi_irq_mode - edge/level trigger mode */
+enum acpi_irq_mode {
+	ACPI_IRQ_EDGE_TRIGGERED,
+	ACPI_IRQ_LEVEL_TRIGGERED,
+};
+
+/**
+ * enum acpi_irq_polarity - polarity of interrupt
+ *
+ * @ACPI_IRQ_ACTIVE_LOW - for ACPI_IRQ_EDGE_TRIGGERED this means falling edge
+ * @ACPI_IRQ_ACTIVE_HIGH - for ACPI_IRQ_EDGE_TRIGGERED this means rising edge
+ * @ACPI_IRQ_ACTIVE_BOTH - not meaningful for ACPI_IRQ_EDGE_TRIGGERED
+ */
+enum acpi_irq_polarity {
+	ACPI_IRQ_ACTIVE_LOW,
+	ACPI_IRQ_ACTIVE_HIGH,
+	ACPI_IRQ_ACTIVE_BOTH,
+};
+
+/**
+ * enum acpi_irq_shared - whether interrupt is shared or not
+ *
+ * @ACPI_IRQ_EXCLUSIVE: only this device uses the interrupt
+ * @ACPI_IRQ_SHARED: other devices may use this interrupt
+ */
+enum acpi_irq_shared {
+	ACPI_IRQ_EXCLUSIVE,
+	ACPI_IRQ_SHARED,
+};
+
+/** enum acpi_irq_wake - indicates whether this interrupt can wake the device */
+enum acpi_irq_wake {
+	ACPI_IRQ_NO_WAKE,
+	ACPI_IRQ_WAKE,
+};
+
+/**
+ * struct acpi_irq - representation of an ACPI interrupt
+ *
+ * @pin: ACPI pin that is monitored for the interrupt
+ * @mode: Edge/level triggering
+ * @polarity: Interrupt polarity
+ * @shared: Whether interrupt is shared or not
+ * @wake: Whether interrupt can wake the device from sleep
+ */
+struct acpi_irq {
+	unsigned int pin;
+	enum acpi_irq_mode mode;
+	enum acpi_irq_polarity polarity;
+	enum acpi_irq_shared shared;
+	enum acpi_irq_wake wake;
+};
+
 /**
  * acpi_device_path() - Get the full path to an ACPI device
  *
diff --git a/include/irq.h b/include/irq.h
index b71afe9bee..8527e4dd79 100644
--- a/include/irq.h
+++ b/include/irq.h
@@ -8,6 +8,9 @@ 
 #ifndef __irq_H
 #define __irq_H
 
+struct acpi_irq;
+struct ofnode_phandle_args;
+
 /*
  * Interrupt controller types available. You can find a particular one with
  * irq_first_device_type()
@@ -24,10 +27,12 @@  enum irq_dev_t {
  *
  * @dev: IRQ device that handles this irq
  * @id: ID to identify this irq with the device
+ * @flags: Flags associated with this interrupt (IRQ_TYPE_...)
  */
 struct irq {
 	struct udevice *dev;
 	ulong id;
+	ulong flags;
 };
 
 /**
@@ -119,10 +124,36 @@  struct irq_ops {
 	 * @return 0 if OK, or a negative error code.
 	 */
 	int (*free)(struct irq *irq);
+
+#if CONFIG_IS_ENABLED(ACPIGEN)
+	/**
+	 * get_acpi() - Get the ACPI info for an irq
+	 *
+	 * This converts a irq to an ACPI structure for adding to the ACPI
+	 * tables.
+	 *
+	 * @irq:	irq to convert
+	 * @acpi_irq:	Output ACPI interrupt information
+	 * @return ACPI pin number or -ve on error
+	 */
+	int (*get_acpi)(const struct irq *irq, struct acpi_irq *acpi_irq);
+#endif
 };
 
 #define irq_get_ops(dev)	((struct irq_ops *)(dev)->driver->ops)
 
+/**
+ * irq_is_valid() - Check if an IRQ is valid
+ *
+ * @irq:	IRQ description containing device and ID, e.g. previously
+ *		returned by irq_get_by_index()
+ * @return true if valid, false if not
+ */
+static inline bool irq_is_valid(const struct irq *irq)
+{
+	return irq->dev != NULL;
+}
+
 /**
  * irq_route_pmc_gpio_gpe() - Get the GPIO for an event
  *
@@ -223,4 +254,16 @@  int irq_free(struct irq *irq);
  */
 int irq_first_device_type(enum irq_dev_t type, struct udevice **devp);
 
+/**
+ * irq_get_acpi() - Get the ACPI info for an irq
+ *
+ * This converts a irq to an ACPI structure for adding to the ACPI
+ * tables.
+ *
+ * @irq:	irq to convert
+ * @acpi_irq:	Output ACPI interrupt information
+ * @return ACPI pin number or -ve on error
+ */
+int irq_get_acpi(const struct irq *irq, struct acpi_irq *acpi_irq);
+
 #endif
diff --git a/test/dm/irq.c b/test/dm/irq.c
index 192d80d7e1..50e505e657 100644
--- a/test/dm/irq.c
+++ b/test/dm/irq.c
@@ -8,6 +8,7 @@ 
 #include <common.h>
 #include <dm.h>
 #include <irq.h>
+#include <acpi/acpi_device.h>
 #include <asm/test.h>
 #include <dm/test.h>
 #include <test/ut.h>
@@ -75,3 +76,24 @@  static int dm_test_request(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_request, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+/* Test of irq_get_acpi() */
+static int dm_test_irq_get_acpi(struct unit_test_state *uts)
+{
+	struct acpi_irq airq;
+	struct udevice *dev;
+	struct irq irq;
+
+	ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev));
+	ut_assertok(irq_get_by_index(dev, 0, &irq));
+
+	ut_assertok(irq_get_acpi(&irq, &airq));
+	ut_asserteq(3, airq.pin);
+	ut_asserteq(ACPI_IRQ_LEVEL_TRIGGERED, airq.mode);
+	ut_asserteq(ACPI_IRQ_ACTIVE_HIGH, airq.polarity);
+	ut_asserteq(ACPI_IRQ_SHARED, airq.shared);
+	ut_asserteq(ACPI_IRQ_WAKE, airq.wake);
+
+	return 0;
+}
+DM_TEST(dm_test_irq_get_acpi, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);