Antwort: [PATCH v2 32/39] irq: Add a method to convert an interrupt to ACPI

[Wolfgang Wallner <wolfgang.wallner@br-automation.com>]

Hi Simon,

"Simon Glass" <sjg@chromium.org> schrieb am 09.03.2020 04:44:56:

> Von: "Simon Glass" <sjg@chromium.org>
> An: "U-Boot Mailing List" <u-boot@lists.denx.de>, 
> Kopie: "Bin Meng" <bmeng.cn@gmail.com>, "Wolfgang Wallner" 
> <wolfgang.wallner@br-automation.com>, "Andy Shevchenko" 
> <andriy.shevchenko@linux.intel.com>, "Simon Glass" <sjg@chromium.org>
> Datum: 09.03.2020 04:46
> Betreff: [PATCH v2 32/39] 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@chromium.org>
> ---
> 
> Changes in v2: None
> 
>  drivers/misc/irq-uclass.c | 18 +++++++-
>  include/acpi_device.h     | 27 +++++++++++
>  include/irq.h             | 41 +++++++++++++++++
>  lib/acpi/acpi_device.c    | 94 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 178 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/irq-uclass.c b/drivers/misc/irq-uclass.c
> index 61aa10e465..b4a8b7b429 100644
> --- a/drivers/misc/irq-uclass.c
> +++ b/drivers/misc/irq-uclass.c
> @@ -153,8 +153,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 is this code dropped?

>     ops = irq_get_ops(dev);
> 
>     irq->dev = dev;
> @@ -176,6 +174,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/include/acpi_device.h b/include/acpi_device.h
> index acd26c0f54..50ba9b66aa 100644
> --- a/include/acpi_device.h
> +++ b/include/acpi_device.h
> @@ -545,6 +545,33 @@ int acpi_dp_write(struct acpi_ctx *ctx, struct 
> acpi_dp *table);
>  int acpi_device_write_gpio_desc(struct acpi_ctx *ctx,
>              const struct gpio_desc *desc);
> 
> +/**
> + * acpi_device_write_interrupt_irq() - Write an interrupt to ACPI
> + *
> + * This creates an interrupt descriptor for an interrupt, including
> information
> + * ACPI needs to use it.
> + *
> + * @req_irq: Interrupt to write
> + * @return 0 if OK, -ve on error
> + */
> +int acpi_device_write_interrupt_irq(struct acpi_ctx *ctx,
> +                const struct irq *req_irq);
> +
> +/**
> + * acpi_device_write_interrupt_or_gpio() - Write interrupt or GPIO to 
ACPI
> + *
> + * This reads the an interrupt from the device tree, if available. If 
not it

typo: "the an"

The description of what this function should do is rather vague.
At least I'm not sure how it is meant to work.

> + * reads the first GPIO with the name @prop.
> + *
> + * If an interrupt is found, that is written to ACPI. If not, but an 
GPIO is
> + * found, that is written.
> + *
> + * @return 0 if OK, -ve if neither an interrupt nor a GPIO could 
befound, or
> + * some other error occurred
> + */
> +int acpi_device_write_interrupt_or_gpio(struct acpi_ctx *ctx,
> +               struct udevice *dev, const char *prop);
> +
>  /**
>   * acpi_device_write_i2c_dev() - Write an I2C device to ACPI, including
>   * information ACPI needs to use it.
> diff --git a/include/irq.h b/include/irq.h
> index d4948e6dc4..8527e4dd79 100644
> --- a/include/irq.h
> +++ b/include/irq.h
> @@ -8,6 +8,7 @@
>  #ifndef __irq_H
>  #define __irq_H
> 
> +struct acpi_irq;
>  struct ofnode_phandle_args;
> 
>  /*
> @@ -26,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;
>  };
> 
>  /**
> @@ -121,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
>   *
> @@ -225,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/lib/acpi/acpi_device.c b/lib/acpi/acpi_device.c
> index adc32f1216..aa5edfe807 100644
> --- a/lib/acpi/acpi_device.c
> +++ b/lib/acpi/acpi_device.c
> @@ -154,6 +154,58 @@ int acpi_device_status(const struct udevice *dev)
>     return ACPI_STATUS_DEVICE_ALL_ON;
>  }
> 
> +/* ACPI 6.1 section 6.4.3.6: Extended Interrupt Descriptor */
> +static int acpi_device_write_interrupt(struct acpi_ctx *ctx,
> +                   const struct acpi_irq *irq)
> +{
> +   void *desc_length;
> +   u8 flags;
> +
> +   if (!irq || !irq->pin)
> +      return -ENOENT;
> +
> +   /* This is supported by GpioInt() but not Interrupt() */
> +   if (irq->polarity == ACPI_IRQ_ACTIVE_BOTH)
> +      return -EINVAL;
> +
> +   /* Byte 0: Descriptor Type */
> +   acpigen_emit_byte(ctx, ACPI_DESCRIPTOR_INTERRUPT);
> +
> +   /* Byte 1-2: Length (filled in later) */
> +   desc_length = acpi_device_write_zero_len(ctx);
> +
> +   /*
> +    * Byte 3: Flags
> +    *  [7:5]: Reserved
> +    *    [4]: Wake     (0=NO_WAKE   1=WAKE)
> +    *    [3]: Sharing  (0=EXCLUSIVE 1=SHARED)
> +    *    [2]: Polarity (0=HIGH      1=LOW)
> +    *    [1]: Mode     (0=LEVEL     1=EDGE)
> +    *    [0]: Resource (0=PRODUCER  1=CONSUMER)
> +    */
> +   flags = 1 << 0; /* ResourceConsumer */
> +   if (irq->mode == ACPI_IRQ_EDGE_TRIGGERED)
> +      flags |= 1 << 1;
> +   if (irq->polarity == ACPI_IRQ_ACTIVE_LOW)
> +      flags |= 1 << 2;
> +   if (irq->shared == ACPI_IRQ_SHARED)
> +      flags |= 1 << 3;
> +   if (irq->wake == ACPI_IRQ_WAKE)
> +      flags |= 1 << 4;
> +   acpigen_emit_byte(ctx, flags);
> +
> +   /* Byte 4: Interrupt Table Entry Count */
> +   acpigen_emit_byte(ctx, 1);
> +
> +   /* Byte 5-8: Interrupt Number */
> +   acpigen_emit_dword(ctx, irq->pin);
> +
> +   /* Fill in Descriptor Length (account for len word) */
> +   acpi_device_fill_len(ctx, desc_length);
> +
> +   return 0;
> +}
> +
>  /* ACPI 6.1 section 6.4.3.8.1 - GPIO Interrupt or I/O */
>  int acpi_device_write_gpio(struct acpi_ctx *ctx, const struct 
> acpi_gpio *gpio)
>  {
> @@ -304,6 +356,48 @@ int acpi_device_write_gpio_desc(struct acpi_ctx 
*ctx,
>     return 0;
>  }
> 
> +int acpi_device_write_interrupt_irq(struct acpi_ctx *ctx,
> +                const struct irq *req_irq)
> +{
> +   struct acpi_irq irq;
> +   int ret;
> +
> +   ret = irq_get_acpi(req_irq, &irq);
> +   if (ret)
> +      return log_msg_ret("get", ret);
> +   ret = acpi_device_write_interrupt(ctx, &irq);
> +   if (ret)
> +      return log_msg_ret("write", ret);
> +
> +   return 0;
> +}
> +
> +int acpi_device_write_interrupt_or_gpio(struct acpi_ctx *ctx,
> +               struct udevice *dev, const char *prop)
> +{
> +   struct irq req_irq;
> +   int ret;
> +
> +   ret = irq_get_by_index(dev, 0, &req_irq);
> +   if (!ret) {
> +      ret = acpi_device_write_interrupt_irq(ctx, &req_irq);
> +      if (ret)
> +         return log_msg_ret("irq", ret);
> +   } else {
> +      struct gpio_desc req_gpio;
> +
> +      ret = gpio_request_by_name(dev, prop, 0, &req_gpio,
> +                  GPIOD_IS_IN);
> +      if (ret)
> +         return log_msg_ret("no gpio", ret);
> +      ret = acpi_device_write_gpio_desc(ctx, &req_gpio);
> +      if (ret)
> +         return log_msg_ret("gpio", ret);
> +   }

Both code paths set the index value hardcoded to 0.
Why is that? Would other indices not make sense?

> +
> +   return 0;
> +}
> +
>  /* ACPI 6.1 section 6.4.3.8.2.1 - I2cSerialBus() */
>  static void acpi_device_write_i2c(struct acpi_ctx *ctx,
>                const struct acpi_i2c *i2c)
> -- 
> 2.25.1.481.gfbce0eb801-goog
> 

regards, Wolfgang