Hi Wolfgang, On Wed, 13 May 2020 at 07:01, Wolfgang Wallner <wolfgang.wall...@br-automation.com> wrote: > > Hi Simon, > > -----"Simon Glass" <s...@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 <s...@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