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

[Alistair Francis <alistair.francis@xilinx.com>]

On Mon, Feb 29, 2016 at 4:22 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> 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.

Woops, it is leftover from some other code I removed. Good catch

Thanks,

Alistair

>
>> +
>> +#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
>