Re: [Qemu-devel] [PATCH v4 14/16] register: Add GPIO API

["Alex Bennée" <alex.bennee@linaro.org>]

Alistair Francis <alistair.francis@xilinx.com> writes:

> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Add GPIO functionality to the register API. This allows association
> and automatic connection of GPIOs to bits in registers. GPIO inputs
> will attach to handlers that automatically set read-only bits in
> registers. GPIO outputs will be updated to reflect their field value
> when their respective registers are written (or reset). Supports
> active low GPIOs.
>
> This is particularly effective for implementing system level
> controllers, where heterogenous collections of control signals are
> placed is a SoC specific peripheral then propagated all over the
> system.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> [ EI Changes:
>   * register: Add a polarity field to GPIO connections
>               Makes it possible to directly connect active low signals
>               to generic interrupt pins.
> ]
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  hw/core/register.c    | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/register.h | 37 ++++++++++++++++++++
>  2 files changed, 133 insertions(+)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index ac866f6..1dc7ccc 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -106,6 +106,7 @@ void register_write(RegisterInfo *reg, uint64_t val, uint64_t we)
>      }
>
>      register_write_val(reg, new_val);
> +    register_refresh_gpios(reg, old_val);
>
>      if (ac->post_write) {
>          ac->post_write(reg, new_val);
> @@ -147,19 +148,113 @@ void register_reset(RegisterInfo *reg)
>      g_assert(reg);
>      g_assert(reg->data);
>      g_assert(reg->access);
> +    uint64_t old_val;
> +
> +    old_val = register_read_val(reg);
>
>      register_write_val(reg, reg->access->reset);
> +    register_refresh_gpios(reg, old_val);
> +}
> +
> +void register_refresh_gpios(RegisterInfo *reg, uint64_t old_value)
> +{
> +    const RegisterAccessInfo *ac;
> +    const RegisterGPIOMapping *gpio;
> +
> +    ac = reg->access;
> +    for (gpio = ac->gpios; gpio && gpio->name; gpio++) {
> +        int i;
> +
> +        if (gpio->input) {
> +            continue;
> +        }
> +
> +        for (i = 0; i < gpio->num; ++i) {
> +            uint64_t gpio_value, gpio_value_old;
> +
> +            qemu_irq gpo = qdev_get_gpio_out_named(DEVICE(reg), gpio->name, i);
> +            gpio_value_old = extract64(old_value,
> +                                       gpio->bit_pos + i * gpio->width,
> +                                       gpio->width) ^ gpio->polarity;
> +            gpio_value = extract64(register_read_val(reg),
> +                                   gpio->bit_pos + i * gpio->width,
> +                                   gpio->width) ^ gpio->polarity;
> +            if (!(gpio_value_old ^ gpio_value)) {
> +                continue;
> +            }
> +            if (reg->debug && gpo) {
> +                qemu_log("refreshing gpio out %s to %" PRIx64 "\n",
> +                         gpio->name, gpio_value);
> +            }
> +            qemu_set_irq(gpo, gpio_value);
> +        }
> +    }
> +}
> +
> +typedef struct DeviceNamedGPIOHandlerOpaque {
> +    DeviceState *dev;
> +    const char *name;
> +} DeviceNamedGPIOHandlerOpaque;
> +
> +static void register_gpio_handler(void *opaque, int n, int level)
> +{
> +    DeviceNamedGPIOHandlerOpaque *gho = opaque;
> +    RegisterInfo *reg = REGISTER(gho->dev);
> +
> +    const RegisterAccessInfo *ac;
> +    const RegisterGPIOMapping *gpio;
> +
> +    ac = reg->access;
> +    for (gpio = ac->gpios; gpio && gpio->name; gpio++) {
> +        if (gpio->input && !strcmp(gho->name, gpio->name)) {
> +            register_write_val(reg, deposit64(register_read_val(reg),
> +                                              gpio->bit_pos + n * gpio->width,
> +                                              gpio->width,
> +                                              level ^ gpio->polarity));
> +            return;
> +        }
> +    }
> +
> +    abort();
>  }
>
>  void register_init(RegisterInfo *reg)
>  {
>      assert(reg);
> +    const RegisterAccessInfo *ac;
> +    const RegisterGPIOMapping *gpio;
>
>      if (!reg->data || !reg->access) {
>          return;
>      }
>
>      object_initialize((void *)reg, sizeof(*reg), TYPE_REGISTER);
> +
> +    ac = reg->access;
> +    for (gpio = ac->gpios; gpio && gpio->name; gpio++) {
> +        if (!gpio->num) {
> +            ((RegisterGPIOMapping *)gpio)->num = 1;
> +        }
> +        if (!gpio->width) {
> +            ((RegisterGPIOMapping *)gpio)->width = 1;
> +        }
> +        if (gpio->input) {
> +            DeviceNamedGPIOHandlerOpaque gho = {
> +                .name = gpio->name,
> +                .dev = DEVICE(reg),
> +            };
> +            qemu_irq irq;
> +
> +            qdev_init_gpio_in_named(DEVICE(reg), register_gpio_handler,
> +                                    gpio->name, gpio->num);
> +            irq = qdev_get_gpio_in_named(DEVICE(reg), gpio->name, gpio->num);
> +            qemu_irq_set_opaque(irq, g_memdup(&gho, sizeof(gho)));
> +        } else {
> +            qemu_irq *gpos = g_new0(qemu_irq, gpio->num);
> +
> +            qdev_init_gpio_out_named(DEVICE(reg), gpos, gpio->name, gpio->num);
> +        }
> +    }
>  }
>
>  static inline void register_write_memory(void *opaque, hwaddr addr,
> @@ -231,6 +326,7 @@ void register_init_block32(DeviceState *owner, const RegisterAccessInfo *rae,
>              .opaque = owner,
>          };
>          register_init(r);
> +        qdev_pass_all_gpios(DEVICE(r), owner);
>
>          memory_region_init_io(&r->mem, OBJECT(owner), ops, r, r->access->name,
>                                sizeof(uint32_t));
> diff --git a/include/hw/register.h b/include/hw/register.h
> index 6ac005c..4d284a9 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -13,11 +13,35 @@
>
>  #include "hw/qdev-core.h"
>  #include "exec/memory.h"
> +#include "hw/irq.h"
>
>  typedef struct RegisterInfo RegisterInfo;
>  typedef struct RegisterAccessInfo RegisterAccessInfo;
>
>  /**
> + * A register access error message
> + * @mask: Bits in the register the error applies to
> + * @reason: Reason why this access is an error
> + */
> +
> +typedef struct RegisterAccessError {
> +    uint64_t mask;
> +    const char *reason;
> +} RegisterAccessError;

This seems out of place here. It's not used by any of the code.

> +
> +#define REG_GPIO_POL_HIGH 0
> +#define REG_GPIO_POL_LOW  1
> +
> +typedef struct RegisterGPIOMapping {
> +    const char *name;
> +    uint8_t bit_pos;
> +    bool input;
> +    bool polarity;
> +    uint8_t num;
> +    uint8_t width;
> +} RegisterGPIOMapping;
> +
> +/**
>   * Access description for a register that is part of guest accessible device
>   * state.
>   *
> @@ -61,6 +85,8 @@ struct RegisterAccessInfo {
>
>      uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
>
> +    const RegisterGPIOMapping *gpios;
> +
>      struct {
>          hwaddr addr;
>      } decode;
> @@ -137,6 +163,17 @@ void register_reset(RegisterInfo *reg);
>  void register_init(RegisterInfo *reg);
>
>  /**
> + * Refresh GPIO outputs based on diff between old value register current value.
> + * GPIOs are refreshed for fields where the old value differs to the current
> + * value.
> + *
> + * @reg: Register to refresh GPIO outs
> + * @old_value: previous value of register
> + */
> +
> +void register_refresh_gpios(RegisterInfo *reg, uint64_t old_value);
> +
> +/**
>   * Memory API MMIO write handler that will write to a Register API register.
>   *  _be for big endian variant and _le for little endian.
>   * @opaque: RegisterInfo to write to


--
Alex Bennée