diff mbox

[v6,11/13] register: Add GPIO API

Message ID d12fdc99542adfd0b2e1a02707572235ada52a97.1463093051.git.alistair.francis@xilinx.com
State New
Headers show

Commit Message

Alistair Francis May 12, 2016, 10:46 p.m. UTC 
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>
---
V6:
 - Don't use qdev_pass_all_gpios() as it doesn't seem to work
V5
 - Remove RegisterAccessError struct

 hw/core/register.c    | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/register.h | 27 +++++++++++++++
 2 files changed, 123 insertions(+)

Comments

Peter Maydell June 10, 2016, 11:52 a.m. UTC | #1 
On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
> 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>

I'm definitely not convinced of the utility of this. I think
almost all devices don't have registers with bits which map
1:1 to GPIO lines like this, and the few devices which do can
easily enough just implement them by hand. GPIOs are (in my
view) a device level concept, not a register level concept,
and I think they're better implemented at the device level.

thanks
-- PMM
Alistair Francis June 21, 2016, 10:34 p.m. UTC | #2 
On Fri, Jun 10, 2016 at 4:52 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 May 2016 at 23:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> 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>
>
> I'm definitely not convinced of the utility of this. I think
> almost all devices don't have registers with bits which map
> 1:1 to GPIO lines like this, and the few devices which do can
> easily enough just implement them by hand. GPIOs are (in my
> view) a device level concept, not a register level concept,
> and I think they're better implemented at the device level.

I disagree, I think there are a lot of times where they are useful,
especially as SoCs become more complex and interlinked.

This device is a good example:
https://github.com/Xilinx/qemu/commit/880f24ab6ee2f869d199c0d2aecc92c1fbd8b8cb

They are also an optional feature, if someone wants to do it the
current way that is still supported (and sometimes still required).

Thanks,

Alistair

>
> thanks
> -- PMM
>
diff mbox

Patch

diff --git a/hw/core/register.c b/hw/core/register.c
index c68d510..306d196 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -101,6 +101,7 @@  void register_write(RegisterInfo *reg, uint64_t val, uint64_t we,
     }
 
     register_write_val(reg, new_val);
+    register_refresh_gpios(reg, old_val, debug);
 
     if (ac->post_write) {
         ac->post_write(reg, new_val);
@@ -140,23 +141,118 @@  uint64_t register_read(RegisterInfo *reg, const char* prefix, bool debug)
 void register_reset(RegisterInfo *reg)
 {
     g_assert(reg);
+    uint64_t old_val;
 
     if (!reg->data || !reg->access) {
         return;
     }
 
+    old_val = register_read_val(reg);
+
     register_write_val(reg, reg->access->reset);
+    register_refresh_gpios(reg, old_val, false);
+}
+
+void register_refresh_gpios(RegisterInfo *reg, uint64_t old_value, bool debug)
+{
+    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 (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);
+            qdev_pass_gpios(DEVICE(reg), reg->opaque, gpio->name);
+        }
+    }
 }
 
 static inline void register_write_memory(void *opaque, hwaddr addr,
diff --git a/include/hw/register.h b/include/hw/register.h
index c40cf03..245d80e 100644
--- a/include/hw/register.h
+++ b/include/hw/register.h
@@ -13,11 +13,24 @@ 
 
 #include "hw/qdev-core.h"
 #include "exec/memory.h"
+#include "hw/irq.h"
 
 typedef struct RegisterInfo RegisterInfo;
 typedef struct RegisterAccessInfo RegisterAccessInfo;
 typedef struct RegisterInfoArray RegisterInfoArray;
 
+#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.
@@ -54,6 +67,8 @@  struct RegisterAccessInfo {
 
     uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
 
+    const RegisterGPIOMapping *gpios;
+
     struct {
         hwaddr addr;
     } decode;
@@ -149,6 +164,18 @@  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
+ * @debug: Should the read operation debug information be printed?
+ */
+
+void register_refresh_gpios(RegisterInfo *reg, uint64_t old_value, bool debug);
+
+/**
  * 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