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

[Wolfgang Wallner]

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>