Hi Simon, -----"Simon Glass" <s...@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.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.
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.wall...@br-automation.com>