qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/2] Add Aspeed GPIO controller model
@ 2019-06-18  8:51 Rashmica Gupta
  2019-06-18  8:51 ` [Qemu-devel] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model Rashmica Gupta
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Rashmica Gupta @ 2019-06-18  8:51 UTC (permalink / raw)
  To: qemu-arm; +Cc: andrew, clg, qemu-devel, Rashmica Gupta, joel

Hi,

These two patches add a bunch of the functionality of the ast2400 and
ast2500 gpio controllers. 

This is based off upstream/master with two of Cédric's patches:
- aspeed: add a per SoC mapping for the interrupt space
- aspeed: add a per SoC mapping for the memory space


Any feedback would be great!

Rashmica Gupta (2):
  hw/gpio: Add basic Aspeed GPIO model
  aspeed: add a GPIO controller to the SoC

 hw/arm/aspeed_soc.c           |  17 +
 hw/gpio/Makefile.objs         |   1 +
 hw/gpio/aspeed_gpio.c         | 869 ++++++++++++++++++++++++++++++++++
 include/hw/arm/aspeed_soc.h   |   3 +
 include/hw/gpio/aspeed_gpio.h |  76 +++
 5 files changed, 966 insertions(+)
 create mode 100644 hw/gpio/aspeed_gpio.c
 create mode 100644 include/hw/gpio/aspeed_gpio.h

-- 
2.17.2



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model
  2019-06-18  8:51 [Qemu-devel] [PATCH 0/2] Add Aspeed GPIO controller model Rashmica Gupta
@ 2019-06-18  8:51 ` Rashmica Gupta
  2019-06-18  9:04   ` Cédric Le Goater
  2019-07-03  4:16   ` Andrew Jeffery
  2019-06-18  8:51 ` [Qemu-devel] [PATCH 2/2] aspeed: add a GPIO controller to the SoC Rashmica Gupta
  2019-06-18  9:24 ` [Qemu-devel] [PATCH 0/2] Add Aspeed GPIO controller model Cédric Le Goater
  2 siblings, 2 replies; 11+ messages in thread
From: Rashmica Gupta @ 2019-06-18  8:51 UTC (permalink / raw)
  To: qemu-arm; +Cc: andrew, clg, qemu-devel, Rashmica Gupta, joel

Add in details for GPIO controller for AST 2400 and 2500

Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 hw/gpio/Makefile.objs         |   1 +
 hw/gpio/aspeed_gpio.c         | 869 ++++++++++++++++++++++++++++++++++
 include/hw/gpio/aspeed_gpio.h |  76 +++
 3 files changed, 946 insertions(+)
 create mode 100644 hw/gpio/aspeed_gpio.c
 create mode 100644 include/hw/gpio/aspeed_gpio.h

diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
index e5da0cb54f..d305b3b24b 100644
--- a/hw/gpio/Makefile.objs
+++ b/hw/gpio/Makefile.objs
@@ -9,3 +9,4 @@ obj-$(CONFIG_OMAP) += omap_gpio.o
 obj-$(CONFIG_IMX) += imx_gpio.o
 obj-$(CONFIG_RASPI) += bcm2835_gpio.o
 obj-$(CONFIG_NRF51_SOC) += nrf51_gpio.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_gpio.o
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
new file mode 100644
index 0000000000..d84dd69255
--- /dev/null
+++ b/hw/gpio/aspeed_gpio.c
@@ -0,0 +1,869 @@
+/*
+ *  ASPEED GPIO Controller
+ *
+ *  Copyright (C) 2017-2019 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ *
+ *
+ * This models the ast2400 and ast2500 GPIO controllers.
+ *
+ * GPIO pins are arranged in groups of 8 pins labeled A,B,..,Y,Z,AA,AB,AC.
+ * (Note that the ast2400 controller only goes up to group AB).
+ * A set has four groups (except set AC which only has one) and is
+ * referred to by the groups it is composed of (eg ABCD,EFGH,...,YZAAAB).
+ * Each set is accessed and controlled by a bank of 14 registers.
+ *
+ * These registers operate on a per pin level where each bit in the register
+ * corresponds to a pin, except for the command source registers. The command
+ * source registers operate on a per group level where bits 24, 16, 8 and 0
+ * correspond to each group in the set.
+ *
+ *  eg. registers for set ABCD:
+ *  |D7...D0|C7...C0|B7...B0|A7...A0| <- GPIOs
+ *  |31...24|23...16|15....8|7.....0| <- bit position
+ *
+ * Note that there are a couple of groups that only have 4 pins.
+
+ * There are three ways that this model deviates from the behaviour of the
+ * actual controller:
+ * (1) There are three debounce registers which aren't modeled and so the per
+ * set debounce setting registers don't affect anything.
+ *
+ * (2) The only control source driving the GPIO pins in the model is the ARM
+ * model (as there currently aren't models for the LPC or Coprocessor [TODO]).
+ *
+ * (3) None of the registers in the model are reset tolerant. [TODO]
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/gpio/aspeed_gpio.h"
+#include "include/hw/misc/aspeed_scu.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+
+#define ASPEED_GPIOS_PER_REG 32
+
+/* GPIO Source Types */
+#define ASPEED_CMD_SRC_MASK         0x01010101
+#define ASPEED_SOURCE_ARM           0
+#define ASPEED_SOURCE_LPC           1
+#define ASPEED_SOURCE_COPROCESSOR   2
+#define ASPEED_SOURCE_RESERVED      3
+
+/* GPIO Interrupt Triggers */
+#define ASPEED_FALLING_EDGE 0
+#define ASPEED_RISING_EDGE  1
+#define ASPEED_LEVEL_LOW    2
+#define ASPEED_LEVEL_HIGH   3
+#define ASPEED_DUAL_EDGE    4
+
+/* GPIO Register Address Offsets */
+#define GPIO_ABCD_DATA_VALUE     0x000
+#define GPIO_ABCD_DIRECTION      0x004
+#define GPIO_ABCD_INT_ENABLE     0x008
+#define GPIO_ABCD_INT_SENS_0     0x00C
+#define GPIO_ABCD_INT_SENS_1     0x010
+#define GPIO_ABCD_INT_SENS_2     0x014
+#define GPIO_ABCD_INT_STATUS     0x018
+#define GPIO_ABCD_RESET_TOLERANT 0x01C
+#define GPIO_EFGH_DATA_VALUE     0x020
+#define GPIO_EFGH_DIRECTION      0x024
+#define GPIO_EFGH_INT_ENABLE     0x028
+#define GPIO_EFGH_INT_SENS_0     0x02C
+#define GPIO_EFGH_INT_SENS_1     0x030
+#define GPIO_EFGH_INT_SENS_2     0x034
+#define GPIO_EFGH_INT_STATUS     0x038
+#define GPIO_EFGH_RESET_TOL      0x03C
+#define GPIO_ABCD_DEBOUNCE_1     0x040
+#define GPIO_ABCD_DEBOUNCE_2     0x044
+#define GPIO_EFGH_DEBOUNCE_1     0x048
+#define GPIO_EFGH_DEBOUNCE_2     0x04C
+#define GPIO_DEBOUNCE_TIME_1     0x050
+#define GPIO_DEBOUNCE_TIME_2     0x054
+#define GPIO_DEBOUNCE_TIME_3     0x058
+#define GPIO_ABCD_COMMAND_SRC_0  0x060
+#define GPIO_ABCD_COMMAND_SRC_1  0x064
+#define GPIO_EFGH_COMMAND_SRC_0  0x068
+#define GPIO_EFGH_COMMAND_SRC_1  0x06C
+#define GPIO_IJKL_DATA_VALUE     0x070
+#define GPIO_IJKL_DIRECTION      0x074
+#define GPIO_MNOP_DATA_VALUE     0x078
+#define GPIO_MNOP_DIRECTION      0x07C
+#define GPIO_QRST_DATA_VALUE     0x080
+#define GPIO_QRST_DIRECTION      0x084
+#define GPIO_UVWX_DATA_VALUE     0x088
+#define GPIO_UWVX_DIRECTION      0x08C
+#define GPIO_IJKL_COMMAND_SRC_0  0x090
+#define GPIO_IJKL_COMMAND_SRC_1  0x094
+#define GPIO_IJKL_INT_ENABLE     0x098
+#define GPIO_IJKL_INT_SENS_0     0x09C
+#define GPIO_IJKL_INT_SENS_1     0x0A0
+#define GPIO_IJKL_INT_SENS_2     0x0A4
+#define GPIO_IJKL_INT_STATUS     0x0A8
+#define GPIO_IJKL_RESET_TOLERANT 0x0AC
+#define GPIO_IJKL_DEBOUNCE_1     0x0B0
+#define GPIO_IJKL_DEBOUNCE_2     0x0B4
+#define GPIO_IJKL_INPUT_MASK     0x0B8
+#define GPIO_ABCD_DATA_READ      0x0C0
+#define GPIO_EFGH_DATA_READ      0x0C4
+#define GPIO_IJKL_DATA_READ      0x0C8
+#define GPIO_MNOP_DATA_READ      0x0CC
+#define GPIO_QRST_DATA_READ      0x0D0
+#define GPIO_UVWX_DATA_READ      0x0D4
+#define GPIO_YZAAAB_DATA_READ    0x0D8
+#define GPIO_AC_DATA_READ        0x0DC
+#define GPIO_MNOP_COMMAND_SRC_0  0x0E0
+#define GPIO_MNOP_COMMAND_SRC_1  0x0E4
+#define GPIO_MNOP_INT_ENABLE     0x0E8
+#define GPIO_MNOP_INT_SENS_0     0x0EC
+#define GPIO_MNOP_INT_SENS_1     0x0F0
+#define GPIO_MNOP_INT_SENS_2     0x0F4
+#define GPIO_MNOP_INT_STATUS     0x0F8
+#define GPIO_MNOP_RESET_TOLERANT 0x0FC
+#define GPIO_MNOP_DEBOUNCE_1     0x100
+#define GPIO_MNOP_DEBOUNCE_2     0x104
+#define GPIO_MNOP_INPUT_MASK     0x108
+#define GPIO_QRST_COMMAND_SRC_0  0x110
+#define GPIO_QRST_COMMAND_SRC_1  0x114
+#define GPIO_QRST_INT_ENABLE     0x118
+#define GPIO_QRST_INT_SENS_0     0x11C
+#define GPIO_QRST_INT_SENS_1     0x120
+#define GPIO_QRST_INT_SENS_2     0x124
+#define GPIO_QRST_INT_STATUS     0x128
+#define GPIO_QRST_RESET_TOLERANT 0x12C
+#define GPIO_QRST_DEBOUNCE_1     0x130
+#define GPIO_QRST_DEBOUNCE_2     0x134
+#define GPIO_QRST_INPUT_MASK     0x138
+#define GPIO_UVWX_COMMAND_SRC_0  0x140
+#define GPIO_UVWX_COMMAND_SRC_1  0x144
+#define GPIO_UVWX_INT_ENABLE     0x148
+#define GPIO_UVWX_INT_SENS_0     0x14C
+#define GPIO_UVWX_INT_SENS_1     0x150
+#define GPIO_UVWX_INT_SENS_2     0x154
+#define GPIO_UVWX_INT_STATUS     0x158
+#define GPIO_UVWX_RESET_TOLERANT 0x15C
+#define GPIO_UVWX_DEBOUNCE_1     0x160
+#define GPIO_UVWX_DEBOUNCE_2     0x164
+#define GPIO_UVWX_INPUT_MASK     0x168
+#define GPIO_YZAAAB_COMMAND_SRC_0 0x170
+#define GPIO_YZAAAB_COMMAND_SRC_1 0x174
+#define GPIO_YZAAAB_INT_ENABLE   0x178
+#define GPIO_YZAAAB_INT_SENS_0   0x17C
+#define GPIO_YZAAAB_INT_SENS_1   0x180
+#define GPIO_YZAAAB_INT_SENS_2   0x184
+#define GPIO_YZAAAB_INT_STATUS   0x188
+#define GPIO_YZAAAB_RESET_TOLERANT 0x18C
+#define GPIO_YZAAAB_DEBOUNCE_1   0x190
+#define GPIO_YZAAAB_DEBOUNCE_2   0x194
+#define GPIO_YZAAAB_INPUT_MASK   0x198
+#define GPIO_AC_COMMAND_SRC_0    0x1A0
+#define GPIO_AC_COMMAND_SRC_1    0x1A4
+#define GPIO_AC_INT_ENABLE       0x1A8
+#define GPIO_AC_INT_SENS_0       0x1AC
+#define GPIO_AC_INT_SENS_1       0x1B0
+#define GPIO_AC_INT_SENS_2       0x1B4
+#define GPIO_AC_INT_STATUS       0x1B8
+#define GPIO_AC_RESET_TOLERANT   0x1BC
+#define GPIO_AC_DEBOUNCE_1       0x1C0
+#define GPIO_AC_DEBOUNCE_2       0x1C4
+#define GPIO_AC_INPUT_MASK       0x1C8
+#define GPIO_ABCD_INPUT_MASK     0x1D0
+#define GPIO_EFGH_INPUT_MASK     0x1D4
+#define GPIO_YZAAAB_DATA_VALUE   0x1E0
+#define GPIO_YZAAAB_DIRECTION    0x1E4
+#define GPIO_AC_DATA_VALUE       0x1E8
+#define GPIO_AC_DIRECTION        0x1EC
+
+struct AspeedGPIO {
+    uint16_t set_idx;
+    uint32_t (*get)(GPIORegs *regs);
+    void (*set)(AspeedGPIOState *s, GPIORegs *regs,
+                GPIOSetProperties *props, uint32_t val);
+};
+
+static int aspeed_evaluate_irq(GPIORegs *regs, int prev_value_high, int bit)
+{
+    uint32_t falling_edge = 0, rising_edge = 0;
+    uint32_t int_trigger = extract32(regs->int_sens_0, bit, 1)
+                           | extract32(regs->int_sens_1, bit, 1) << 1
+                           | extract32(regs->int_sens_2, bit, 1) << 2 ;
+    uint32_t curr_pin_high = extract32(regs->data_value, bit, 1);
+
+    /* Detect edges */
+    if (curr_pin_high && !prev_value_high) {
+            rising_edge = 1;
+    } else if (!curr_pin_high && prev_value_high) {
+            falling_edge = 1;
+    }
+
+    if (((int_trigger == ASPEED_FALLING_EDGE)  && falling_edge)  ||
+        ((int_trigger == ASPEED_RISING_EDGE)  && rising_edge)  ||
+        ((int_trigger == ASPEED_LEVEL_LOW)  && !curr_pin_high)  ||
+        ((int_trigger == ASPEED_LEVEL_HIGH)  && curr_pin_high)  ||
+        ((int_trigger >= ASPEED_DUAL_EDGE)  && (rising_edge || falling_edge)))
+    {
+        regs->int_status = deposit32(regs->int_status, bit, 1, 1);
+        return 1;
+    }
+    return 0;
+}
+
+static void aspeed_gpio_update(AspeedGPIOState *s, GPIORegs *regs)
+{
+    uint32_t input_mask = regs->input_mask;
+    uint32_t direction = regs->direction;
+    uint32_t old = regs->data_value;
+    uint32_t new = regs->data_read;
+    uint32_t diff;
+    int bit;
+
+    diff = old ^ new;
+    if (!diff) {
+        return;
+    }
+
+    if (!direction) {
+        return;
+    }
+
+    for (bit = 0; bit < ASPEED_GPIOS_PER_REG; bit++) {
+        uint32_t mask = 1 << bit;
+        /* If the bit needs to be updated... */
+        if (!(diff & mask)) {
+            continue;
+        }
+        /* ...and corresponds to an output pin...*/
+        if (!(direction & mask)) {
+            continue;
+        }
+        /* ...and isn't masked...  */
+        if (input_mask & mask) {
+            continue;
+        }
+        /* ... then update it*/
+        if (mask & new) {
+            regs->data_value |= mask;
+        } else {
+            regs->data_value &= ~mask;
+        }
+
+        if (aspeed_evaluate_irq(regs, old & mask, bit)) {
+            qemu_set_irq(s->output[bit], 1);
+        }
+    }
+}
+
+static uint32_t aspeed_adjust_pin(AspeedGPIOState *s, uint32_t pin)
+{
+    if (pin >= s->ctrl->gap) {
+        pin += 4;
+    }
+
+    return pin;
+}
+
+static uint32_t aspeed_get_set_idx_from_pin(AspeedGPIOState *s, uint32_t pin)
+{
+    /*
+     * For most pins, dividing by 32 gets the set index.
+     *
+     * However the 2500 has a 4 pin gap in group AB and the 2400 has a 4 pin
+     * gap in group Y (and only four pins in AB but this is the last group so
+     * it doesn't matter).
+     */
+    return aspeed_adjust_pin(s, pin) >> 5;
+}
+
+static bool aspeed_gpio_get_pin_level(AspeedGPIOState *s, uint32_t pin)
+{
+    uint32_t reg_val;
+    uint32_t mask = 1 << (pin);
+    uint32_t set_idx = aspeed_get_set_idx_from_pin(s, pin);
+
+    reg_val = s->sets[set_idx].data_value;
+
+    return !!(reg_val & mask);
+}
+
+static void aspeed_gpio_set_pin_level(AspeedGPIOState *s, uint32_t pin,
+                                      bool level)
+{
+    uint32_t mask  = 1 << (pin);
+    uint32_t set_idx = aspeed_get_set_idx_from_pin(s, pin);
+
+    if (level) {
+        s->sets[set_idx].data_read |= mask;
+    } else {
+        s->sets[set_idx].data_read &= !mask;
+    }
+
+    aspeed_gpio_update(s, &s->sets[set_idx]);
+}
+
+/*
+ *  | src_1 | src_2 |  source     |
+ *  |-----------------------------|
+ *  |   0   |   0   |  ARM        |
+ *  |   0   |   1   |  LPC        |
+ *  |   1   |   0   |  Coprocessor|
+ *  |   1   |   1   |  Reserved   |
+ *
+ *  Once the source of a set is programmed, corresponding bits in the
+ *  data_value, direction, interrupt [enable, sens[0-2]], reset_tol and
+ *  debounce registers can only be written by the source.
+ *
+ *  Source is ARM by default
+ *  only bits 24, 16, 8, and 0 can be set
+ *
+ *  we don't currently have a model for the LPC or Coprocessor
+ */
+static uint32_t update_value_control_source(GPIORegs *regs, uint32_t old_value,
+                                            uint32_t value)
+{
+    int i;
+    int cmd_source;
+
+    /* assume the source is always ARM for now */
+    int source = ASPEED_SOURCE_ARM;
+
+    uint32_t new_value = 0;
+
+    /* for each group in set */
+    for (i = 0; i < ASPEED_GPIOS_PER_REG; i += 8) {
+        cmd_source = extract32(regs->cmd_source_0, i, 1)
+                | (extract32(regs->cmd_source_1, i, 1) << 1);
+
+        if (source == cmd_source) {
+            new_value |= (0xff << i) & value;
+        } else {
+            new_value |= (0xff << i) & old_value;
+        }
+    }
+    return new_value;
+}
+
+/* Reader helper functions */
+static uint32_t read_direction(GPIORegs *regs)
+{
+    return regs->direction;
+}
+
+static uint32_t read_data_value(GPIORegs *regs)
+{
+    return regs->data_value;
+}
+
+static uint32_t read_int_enable(GPIORegs *regs)
+{
+    return regs->int_enable;
+}
+
+static uint32_t read_int_sens_0(GPIORegs *regs)
+{
+    return regs->int_sens_0;
+}
+
+static uint32_t read_int_sens_1(GPIORegs *regs)
+{
+    return regs->int_sens_1;
+}
+
+static uint32_t read_int_sens_2(GPIORegs *regs)
+{
+    return regs->int_sens_2;
+}
+
+static uint32_t read_int_status(GPIORegs *regs)
+{
+    return regs->int_status;
+}
+
+static uint32_t read_reset_tol(GPIORegs *regs)
+{
+    return regs->reset_tol;
+}
+
+static uint32_t read_debounce_1(GPIORegs *regs)
+{
+    return regs->debounce_1;
+}
+
+static uint32_t read_debounce_2(GPIORegs *regs)
+{
+    return regs->debounce_2;
+}
+
+static uint32_t read_cmd_source_0(GPIORegs *regs)
+{
+    return regs->cmd_source_0;
+}
+
+static uint32_t read_cmd_source_1(GPIORegs *regs)
+{
+    return regs->cmd_source_1;
+}
+
+static uint32_t read_data(GPIORegs *regs)
+{
+    return regs->data_read;
+}
+
+static uint32_t read_input_mask(GPIORegs *regs)
+{
+    return regs->input_mask;
+}
+
+/* Write helper functions */
+
+static void _write_data_value(AspeedGPIOState *s, GPIORegs *regs,
+                              GPIOSetProperties *props, uint32_t val)
+{
+    val &= props->output | ~props->input;
+    regs->data_read = update_value_control_source(regs, regs->data_read, val);
+    aspeed_gpio_update(s, regs);
+}
+
+static void _write_direction(AspeedGPIOState *s, GPIORegs *regs,
+                             GPIOSetProperties *props, uint32_t val)
+{
+    val &= props->output | ~props->input;
+    regs->direction = update_value_control_source(regs, regs->direction, val);
+    aspeed_gpio_update(s, regs);
+}
+
+static void _write_int_enable(AspeedGPIOState *s, GPIORegs *regs,
+                              GPIOSetProperties *props, uint32_t val)
+{
+    regs->int_enable = update_value_control_source(regs, regs->int_enable, val);
+    aspeed_gpio_update(s, regs);
+}
+
+static void _write_int_sens_0(AspeedGPIOState *s, GPIORegs *regs,
+                              GPIOSetProperties *props, uint32_t val)
+{
+    regs->int_sens_0 = update_value_control_source(regs, regs->int_sens_0, val);
+    aspeed_gpio_update(s, regs);
+}
+
+static void _write_int_sens_1(AspeedGPIOState *s, GPIORegs *regs,
+                              GPIOSetProperties *props, uint32_t val)
+{
+    regs->int_sens_1 = update_value_control_source(regs, regs->int_sens_1, val);
+    aspeed_gpio_update(s, regs);
+}
+
+static void _write_int_sens_2(AspeedGPIOState *s, GPIORegs *regs,
+                              GPIOSetProperties *props, uint32_t val)
+{
+    regs->int_sens_2 = update_value_control_source(regs, regs->int_sens_2, val);
+    aspeed_gpio_update(s, regs);
+}
+
+static void _write_int_status(AspeedGPIOState *s, GPIORegs *regs,
+                              GPIOSetProperties *props, uint32_t val)
+{
+    regs->int_status = val;
+    aspeed_gpio_update(s, regs);
+}
+
+static void _write_reset_tol(AspeedGPIOState *s, GPIORegs *regs,
+                             GPIOSetProperties *props, uint32_t val)
+{
+    regs->reset_tol = update_value_control_source(regs, regs->reset_tol, val);
+}
+
+static void _write_debounce_1(AspeedGPIOState *s, GPIORegs *regs,
+                              GPIOSetProperties *props, uint32_t val)
+{
+    regs->debounce_1 = update_value_control_source(regs, regs->debounce_1, val);
+}
+
+static void _write_debounce_2(AspeedGPIOState *s, GPIORegs *regs,
+                              GPIOSetProperties *props, uint32_t val)
+{
+    regs->debounce_2 = update_value_control_source(regs, regs->debounce_2, val);
+}
+
+static void _write_cmd_source_0(AspeedGPIOState *s, GPIORegs *regs,
+                                GPIOSetProperties *props, uint32_t val)
+{
+    regs->cmd_source_0 = val & ASPEED_CMD_SRC_MASK;
+}
+
+static void _write_cmd_source_1(AspeedGPIOState *s, GPIORegs *regs,
+                                GPIOSetProperties *props, uint32_t val)
+{
+    regs->cmd_source_1 = val & ASPEED_CMD_SRC_MASK;
+}
+
+/*
+ * feeds into interrupt generation
+ * 0: read from data value reg will be updated
+ * 1: read from data value reg will not be updated
+ */
+static void _write_input_mask(AspeedGPIOState *s, GPIORegs *regs,
+                              GPIOSetProperties *props, uint32_t val)
+{
+    regs->input_mask = val & props->input;
+    aspeed_gpio_update(s, regs);
+}
+
+static const struct AspeedGPIO gpios[0x1f0] = {
+    /* Set ABCD */
+    [GPIO_ABCD_DATA_VALUE] = {0, read_data_value, _write_data_value},
+    [GPIO_ABCD_DIRECTION] = {0, read_direction, _write_direction},
+    [GPIO_ABCD_INT_ENABLE] = {0, read_int_enable, _write_int_enable},
+    [GPIO_ABCD_INT_SENS_0] = {0, read_int_sens_0, _write_int_sens_0},
+    [GPIO_ABCD_INT_SENS_1] = {0, read_int_sens_1, _write_int_sens_1},
+    [GPIO_ABCD_INT_SENS_2] = {0, read_int_sens_2, _write_int_sens_2},
+    [GPIO_ABCD_INT_STATUS] = {0, read_int_status, _write_int_status},
+    [GPIO_ABCD_RESET_TOLERANT] = {0, read_reset_tol, _write_reset_tol},
+    [GPIO_ABCD_DEBOUNCE_1] = {0, read_debounce_1, _write_debounce_1},
+    [GPIO_ABCD_DEBOUNCE_2] = {0, read_debounce_2, _write_debounce_2},
+    [GPIO_ABCD_COMMAND_SRC_0] = {0, read_cmd_source_0, _write_cmd_source_0},
+    [GPIO_ABCD_COMMAND_SRC_1] = {0, read_cmd_source_1, _write_cmd_source_1},
+    [GPIO_ABCD_DATA_READ] = {0, read_data, NULL},
+    [GPIO_ABCD_INPUT_MASK] = {0, read_input_mask, _write_input_mask},
+    /* Set EFGH */
+    [GPIO_EFGH_DATA_VALUE] = {1, read_data_value, _write_data_value},
+    [GPIO_EFGH_DIRECTION] = {1, read_direction, _write_direction },
+    [GPIO_EFGH_INT_ENABLE] = {1, read_int_enable, _write_int_enable},
+    [GPIO_EFGH_INT_SENS_0] = {1, read_int_sens_0, _write_int_sens_0},
+    [GPIO_EFGH_INT_SENS_1] = {1, read_int_sens_1, _write_int_sens_1},
+    [GPIO_EFGH_INT_SENS_2] = {1, read_int_sens_2, _write_int_sens_2},
+    [GPIO_EFGH_INT_STATUS] = {1, read_int_status, _write_int_status},
+    [GPIO_EFGH_RESET_TOL] = {1, read_reset_tol,   _write_reset_tol},
+    [GPIO_EFGH_DEBOUNCE_1] = {1, read_debounce_1,  _write_debounce_1},
+    [GPIO_EFGH_DEBOUNCE_2] = {1, read_debounce_2,  _write_debounce_2},
+    [GPIO_EFGH_COMMAND_SRC_0] = {1, read_cmd_source_0,  _write_cmd_source_0},
+    [GPIO_EFGH_COMMAND_SRC_1] = {1, read_cmd_source_1,  _write_cmd_source_1},
+    [GPIO_EFGH_DATA_READ] = {1, read_data, NULL},
+    [GPIO_EFGH_INPUT_MASK] = {1, read_input_mask,  _write_input_mask},
+    /* Set IJKL */
+    [GPIO_IJKL_DATA_VALUE] = {2, read_data_value, _write_data_value},
+    [GPIO_IJKL_DIRECTION] = {2, read_direction, _write_direction},
+    [GPIO_IJKL_INT_ENABLE] = {2, read_int_enable, _write_int_enable},
+    [GPIO_IJKL_INT_SENS_0] = {2, read_int_sens_0, _write_int_sens_0},
+    [GPIO_IJKL_INT_SENS_1] = {2, read_int_sens_1, _write_int_sens_1},
+    [GPIO_IJKL_INT_SENS_2] = {2, read_int_sens_2, _write_int_sens_2},
+    [GPIO_IJKL_INT_STATUS] = {2, read_int_status, _write_int_status},
+    [GPIO_IJKL_RESET_TOLERANT] = {2, read_reset_tol, _write_reset_tol},
+    [GPIO_IJKL_DEBOUNCE_1] = {2, read_debounce_1, _write_debounce_1},
+    [GPIO_IJKL_DEBOUNCE_2] = {2, read_debounce_2, _write_debounce_2},
+    [GPIO_IJKL_COMMAND_SRC_0] = {2, read_cmd_source_0, _write_cmd_source_0},
+    [GPIO_IJKL_COMMAND_SRC_1] = {2, read_cmd_source_1, _write_cmd_source_1},
+    [GPIO_IJKL_DATA_READ] = {2, read_data, NULL},
+    [GPIO_IJKL_INPUT_MASK] = {2, read_input_mask, _write_input_mask},
+    /* Set MNOP */
+    [GPIO_MNOP_DATA_VALUE] = {3, read_data_value, _write_data_value},
+    [GPIO_MNOP_DIRECTION] = {3, read_direction, _write_direction},
+    [GPIO_MNOP_INT_ENABLE] = {3, read_int_enable, _write_int_enable},
+    [GPIO_MNOP_INT_SENS_0] = {3, read_int_sens_0, _write_int_sens_0},
+    [GPIO_MNOP_INT_SENS_1] = {3, read_int_sens_1, _write_int_sens_1},
+    [GPIO_MNOP_INT_SENS_2] = {3, read_int_sens_2, _write_int_sens_2},
+    [GPIO_MNOP_INT_STATUS] = {3, read_int_status, _write_int_status},
+    [GPIO_MNOP_RESET_TOLERANT] = {3, read_reset_tol,  _write_reset_tol},
+    [GPIO_MNOP_DEBOUNCE_1] = {3, read_debounce_1, _write_debounce_1},
+    [GPIO_MNOP_DEBOUNCE_2] = {3, read_debounce_2, _write_debounce_2},
+    [GPIO_MNOP_COMMAND_SRC_0] = {3, read_cmd_source_0, _write_cmd_source_0},
+    [GPIO_MNOP_COMMAND_SRC_1] = {3, read_cmd_source_1, _write_cmd_source_1},
+    [GPIO_MNOP_DATA_READ] = {3, read_data, NULL},
+    [GPIO_MNOP_INPUT_MASK] = {3, read_input_mask, _write_input_mask},
+    /* Set QRST */
+    [GPIO_QRST_DATA_VALUE] = {4, read_data_value, _write_data_value},
+    [GPIO_QRST_DIRECTION] = {4, read_direction, _write_direction},
+    [GPIO_QRST_INT_ENABLE] = {4, read_int_enable, _write_int_enable},
+    [GPIO_QRST_INT_SENS_0] = {4, read_int_sens_0, _write_int_sens_0},
+    [GPIO_QRST_INT_SENS_1] = {4, read_int_sens_1, _write_int_sens_1},
+    [GPIO_QRST_INT_SENS_2] = {4, read_int_sens_2, _write_int_sens_2},
+    [GPIO_QRST_INT_STATUS] = {4, read_int_status, _write_int_status},
+    [GPIO_QRST_RESET_TOLERANT] = {4, read_reset_tol, _write_reset_tol},
+    [GPIO_QRST_DEBOUNCE_1] = {4, read_debounce_1, _write_debounce_1},
+    [GPIO_QRST_DEBOUNCE_2] = {4, read_debounce_2, _write_debounce_2},
+    [GPIO_QRST_COMMAND_SRC_0] = {4, read_cmd_source_0, _write_cmd_source_0},
+    [GPIO_QRST_COMMAND_SRC_1] = {4, read_cmd_source_1, _write_cmd_source_1},
+    [GPIO_QRST_DATA_READ] = {4, read_data, NULL},
+    [GPIO_QRST_INPUT_MASK] = {4, read_input_mask,  _write_input_mask},
+    /* Set UVWX */
+    [GPIO_UVWX_DATA_VALUE] = {5, read_data_value, _write_data_value},
+    [GPIO_UWVX_DIRECTION] = {5, read_direction, _write_direction},
+    [GPIO_UVWX_INT_ENABLE] = {5, read_int_enable, _write_int_enable},
+    [GPIO_UVWX_INT_SENS_0] = {5, read_int_sens_0, _write_int_sens_0},
+    [GPIO_UVWX_INT_SENS_1] = {5, read_int_sens_1, _write_int_sens_1},
+    [GPIO_UVWX_INT_SENS_2] = {5, read_int_sens_2, _write_int_sens_2},
+    [GPIO_UVWX_INT_STATUS] = {5, read_int_status, _write_int_status},
+    [GPIO_UVWX_RESET_TOLERANT] = {5, read_reset_tol, _write_reset_tol},
+    [GPIO_UVWX_DEBOUNCE_1] = {5, read_debounce_1, _write_debounce_1},
+    [GPIO_UVWX_DEBOUNCE_2] = {5, read_debounce_2, _write_debounce_2},
+    [GPIO_UVWX_COMMAND_SRC_0] = {5, read_cmd_source_0, _write_cmd_source_0},
+    [GPIO_UVWX_COMMAND_SRC_1] = {5, read_cmd_source_1, _write_cmd_source_1},
+    [GPIO_UVWX_DATA_READ] = {5, read_data, NULL},
+    [GPIO_UVWX_INPUT_MASK] = {5, read_input_mask, _write_input_mask},
+    /* Set YZAAAB */
+    [GPIO_YZAAAB_DATA_VALUE] = {6, read_data_value, _write_data_value},
+    [GPIO_YZAAAB_DIRECTION] = {6, read_direction, _write_direction},
+    [GPIO_YZAAAB_INT_ENABLE] = {6, read_int_enable, _write_int_enable},
+    [GPIO_YZAAAB_INT_SENS_0] = {6, read_int_sens_0, _write_int_sens_0},
+    [GPIO_YZAAAB_INT_SENS_1] = {6, read_int_sens_1, _write_int_sens_1},
+    [GPIO_YZAAAB_INT_SENS_2] = {6, read_int_sens_2, _write_int_sens_2},
+    [GPIO_YZAAAB_INT_STATUS] = {6, read_int_status, _write_int_status},
+    [GPIO_YZAAAB_RESET_TOLERANT] = {6, read_reset_tol, _write_reset_tol},
+    [GPIO_YZAAAB_DEBOUNCE_1] = {6, read_debounce_1, _write_debounce_1},
+    [GPIO_YZAAAB_DEBOUNCE_2] = {6, read_debounce_2, _write_debounce_2},
+    [GPIO_YZAAAB_COMMAND_SRC_0] = {6, read_cmd_source_0, _write_cmd_source_0},
+    [GPIO_YZAAAB_COMMAND_SRC_1] = {6, read_cmd_source_1, _write_cmd_source_1},
+    [GPIO_YZAAAB_DATA_READ] = {6, read_data, NULL},
+    [GPIO_YZAAAB_INPUT_MASK] = {6, read_input_mask, _write_input_mask},
+    /* Set AC */
+    [GPIO_AC_DATA_VALUE] = {7, read_data_value, _write_data_value},
+    [GPIO_AC_DIRECTION] = {7, read_direction, _write_direction},
+    [GPIO_AC_INT_ENABLE] = {7, read_int_enable, _write_int_enable},
+    [GPIO_AC_INT_SENS_0] = {7, read_int_sens_0, _write_int_sens_0},
+    [GPIO_AC_INT_SENS_1] = {7, read_int_sens_1, _write_int_sens_1},
+    [GPIO_AC_INT_SENS_2] = {7, read_int_sens_2, _write_int_sens_2},
+    [GPIO_AC_INT_STATUS] = {7, read_int_status, _write_int_status},
+    [GPIO_AC_RESET_TOLERANT] = {7, read_reset_tol, _write_reset_tol},
+    [GPIO_AC_DEBOUNCE_1] = {7, read_debounce_1, _write_debounce_1},
+    [GPIO_AC_DEBOUNCE_2] = {7, read_debounce_2, _write_debounce_2},
+    [GPIO_AC_COMMAND_SRC_0] = {7, read_cmd_source_0, _write_cmd_source_0},
+    [GPIO_AC_COMMAND_SRC_1] = {7, read_cmd_source_1, _write_cmd_source_1},
+    [GPIO_AC_DATA_READ] = {7, read_data, NULL},
+    [GPIO_AC_INPUT_MASK] = {7, read_input_mask,     _write_input_mask},
+    /* Debounce registers */
+    [GPIO_DEBOUNCE_TIME_1] = {-1, NULL, NULL},
+};
+
+static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size)
+{
+    AspeedGPIOState *s = ASPEED_GPIO(opaque);
+    uint32_t val = 0;
+
+    if (size != 4) {
+        return 0;
+    }
+
+    if (gpios[offset].get == NULL) {
+        qemu_log_mask(LOG_GUEST_ERROR, "no getter for offset %lx", offset);
+        return 0;
+    }
+
+    val = gpios[offset].get(&s->sets[gpios[offset].set_idx]);
+    return (uint64_t) val;
+}
+
+static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
+                              uint32_t size)
+{
+    AspeedGPIOState *s = ASPEED_GPIO(opaque);
+    GPIOSetProperties *props = &s->ctrl->props[gpios[offset].set_idx];
+
+    if (gpios[offset].set == NULL) {
+        qemu_log_mask(LOG_GUEST_ERROR, "no setter for offset %lx", offset);
+        return;
+    }
+
+    uint32_t mask = props->input | props->output;
+
+    gpios[offset].set(s, &s->sets[gpios[offset].set_idx],
+                      props, data & mask);
+}
+
+static void aspeed_gpio_get_pin(Object *obj, Visitor *v, const char *name,
+                                void *opaque, Error **errp)
+{
+    int pin = 0xfff;
+    bool level = true;
+    char group[3];
+    AspeedGPIOState *s = ASPEED_GPIO(obj);
+    if (sscanf(name, "gpio%2[A-Z]%1d", group, &pin) != 2) {
+        qemu_log_mask(LOG_GUEST_ERROR, "error reading %s", name);
+        return;
+    }
+    level = aspeed_gpio_get_pin_level(s, pin);
+    visit_type_bool(v, name, &level, errp);
+}
+
+static void aspeed_gpio_set_pin(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    Error *local_err = NULL;
+    bool level;
+    int pin = 0xfff;
+    char group[3];
+    AspeedGPIOState *s = ASPEED_GPIO(obj);
+    visit_type_bool(v, name, &level, &local_err);
+    if (sscanf(name, "gpio%2[A-Z]%1d", group, &pin) != 2) {
+        qemu_log_mask(LOG_GUEST_ERROR, "error reading %s", name);
+        return;
+    }
+    aspeed_gpio_set_pin_level(s, pin, level);
+}
+
+
+/* Setup functions */
+static const MemoryRegionOps aspeed_gpio_ops = {
+    .read = aspeed_gpio_read,
+    .write = aspeed_gpio_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
+};
+
+static void aspeed_gpio_reset(DeviceState *dev)
+{
+    struct AspeedGPIOState *s = ASPEED_GPIO(dev);
+
+    /* TODO: respect the reset tolerance registers */
+    memset(s->sets, 0, sizeof(s->sets));
+}
+
+static void aspeed_gpio_realize(DeviceState *dev, Error **errp)
+{
+    AspeedGPIOState *s = ASPEED_GPIO(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    sysbus_init_irq(sbd, &s->irq);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s,
+            TYPE_ASPEED_GPIO, 0x1000);
+
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static const VMStateDescription vmstate_gpio_regs = {
+    .name = TYPE_ASPEED_GPIO"/regs",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(data_value,   GPIORegs),
+        VMSTATE_UINT32(data_read,    GPIORegs),
+        VMSTATE_UINT32(direction,    GPIORegs),
+        VMSTATE_UINT32(int_enable,   GPIORegs),
+        VMSTATE_UINT32(int_sens_0,   GPIORegs),
+        VMSTATE_UINT32(int_sens_1,   GPIORegs),
+        VMSTATE_UINT32(int_sens_2,   GPIORegs),
+        VMSTATE_UINT32(int_status,   GPIORegs),
+        VMSTATE_UINT32(reset_tol,    GPIORegs),
+        VMSTATE_UINT32(cmd_source_0, GPIORegs),
+        VMSTATE_UINT32(cmd_source_1, GPIORegs),
+        VMSTATE_UINT32(debounce_1,   GPIORegs),
+        VMSTATE_UINT32(debounce_2,   GPIORegs),
+        VMSTATE_UINT32(input_mask,   GPIORegs),
+        VMSTATE_END_OF_LIST(),
+    }
+};
+static const VMStateDescription vmstate_aspeed_gpio = {
+    .name = TYPE_ASPEED_GPIO,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT_ARRAY(sets, AspeedGPIOState, ASPEED_GPIO_MAX_NR_SETS,
+                             1, vmstate_gpio_regs, GPIORegs),
+        VMSTATE_END_OF_LIST(),
+   }
+};
+
+static void aspeed_gpio_init(Object *obj)
+{
+    AspeedGPIOState *s = ASPEED_GPIO(obj);
+    AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
+    int pin;
+
+    s->ctrl = agc->ctrl;
+
+    for (pin = 0; pin < agc->ctrl->nr_gpio_pins; pin++) {
+        char *name;
+        int set_idx = aspeed_get_set_idx_from_pin(s, pin);
+        int _pin = aspeed_adjust_pin(s, pin);
+        int pin_idx = _pin - (set_idx * 32);
+        int group_idx = pin_idx >> 3;
+
+        name = g_strdup_printf("gpio%s%d",
+                               agc->ctrl->props[set_idx].set[group_idx],
+                               pin_idx % 8);
+        object_property_add(obj, name, "bool",
+                            aspeed_gpio_get_pin,
+                            aspeed_gpio_set_pin,
+                            NULL, NULL, NULL);
+    }
+}
+
+static void aspeed_gpio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    AspeedGPIOClass *agc = ASPEED_GPIO_CLASS(klass);
+
+    dc->realize = aspeed_gpio_realize;
+    dc->reset = aspeed_gpio_reset;
+    dc->desc = "Aspeed GPIO Controller";
+    dc->vmsd = &vmstate_aspeed_gpio;
+    agc->ctrl = data;
+}
+
+static GPIOSetProperties ast2400_set_props[] = {
+    [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
+    [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
+    [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
+    [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
+    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
+    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
+    [6] = {0x0000000f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
+};
+
+static GPIOSetProperties ast2500_set_props[] = {
+    [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
+    [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
+    [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
+    [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
+    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
+    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
+    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
+    [7] = {0x000000ff,  0x000000ff,  {"AC"} },
+};
+
+static AspeedGPIOController controllers[] = {
+    {
+        .name           = TYPE_ASPEED_GPIO "-ast2500",
+        .props          = ast2500_set_props,
+        .nr_gpio_pins   = 228,
+        .nr_gpio_sets   = 8,
+        .gap            = 220,
+    }, {
+        .name           = TYPE_ASPEED_GPIO "-ast2400",
+        .props          = ast2400_set_props,
+        .nr_gpio_pins   = 216,
+        .nr_gpio_sets   = 7,
+        .gap            = 196,
+    }
+};
+
+static const TypeInfo aspeed_gpio_info = {
+    .name = TYPE_ASPEED_GPIO,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedGPIOState),
+    .class_size     = sizeof(AspeedGPIOClass),
+    .instance_init = aspeed_gpio_init,
+    .abstract       = true,
+};
+
+static void aspeed_gpio_register_types(void)
+{
+    int i;
+
+    type_register_static(&aspeed_gpio_info);
+    for (i = 0; i < ARRAY_SIZE(controllers); i++) {
+        TypeInfo t = {
+            .name = controllers[i].name,
+            .parent = TYPE_ASPEED_GPIO,
+            .class_init = aspeed_gpio_class_init,
+            .class_data = (void *)&controllers[i],
+        };
+        type_register(&t);
+    };
+}
+
+type_init(aspeed_gpio_register_types);
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
new file mode 100644
index 0000000000..97a84a5da1
--- /dev/null
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -0,0 +1,76 @@
+/*
+ *  ASPEED GPIO Controller
+ *
+ *  Copyright (C) 2017-2018 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef ASPEED_GPIO_H
+#define ASPEED_GPIO_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_GPIO "aspeed.gpio"
+#define ASPEED_GPIO(obj) OBJECT_CHECK(AspeedGPIOState, (obj), TYPE_ASPEED_GPIO)
+#define ASPEED_GPIO_CLASS(klass) \
+     OBJECT_CLASS_CHECK(AspeedGPIOClass, (klass), TYPE_ASPEED_GPIO)
+#define ASPEED_GPIO_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(AspeedGPIOClass, (obj), TYPE_ASPEED_GPIO)
+
+#define ASPEED_GPIO_MAX_NR_SETS 8
+#define ASPEED_REGS_PER_BANK 14
+#define ASPEED_GPIO_MAX_NR_REGS (ASPEED_REGS_PER_BANK * ASPEED_GPIO_MAX_NR_SETS)
+#define ASPEED_GPIO_NR_PINS 228
+
+typedef struct GPIORegs GPIORegs;
+typedef const struct GPIOSetProperties {
+    uint32_t input;
+    uint32_t output;
+    char set[4][3];
+} GPIOSetProperties;
+
+typedef const struct AspeedGPIOController {
+    const char *name;
+    GPIOSetProperties *props;
+    uint32_t nr_gpio_pins;
+    uint32_t nr_gpio_sets;
+    uint32_t gap;
+} AspeedGPIOController;
+
+typedef struct  AspeedGPIOClass {
+    SysBusDevice parent_obj;
+    AspeedGPIOController *ctrl;
+}  AspeedGPIOClass;
+
+typedef struct AspeedGPIOState {
+    /* <private> */
+    SysBusDevice parent;
+
+    /*< public >*/
+    MemoryRegion iomem;
+    qemu_irq output[ASPEED_GPIO_NR_PINS];
+    qemu_irq irq;
+    AspeedGPIOController *ctrl;
+
+/* Parallel GPIO Registers */
+    struct GPIORegs {
+        uint32_t data_value; /* Reflects pin values */
+        uint32_t data_read; /* Contains last value written to data value */
+        uint32_t direction;
+        uint32_t int_enable;
+        uint32_t int_sens_0;
+        uint32_t int_sens_1;
+        uint32_t int_sens_2;
+        uint32_t int_status;
+        uint32_t reset_tol;
+        uint32_t cmd_source_0;
+        uint32_t cmd_source_1;
+        uint32_t debounce_1;
+        uint32_t debounce_2;
+        uint32_t input_mask;
+    } sets[ASPEED_GPIO_MAX_NR_SETS];
+} AspeedGPIOState;
+
+#endif /* _ASPEED_GPIO_H_ */
-- 
2.17.2



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 2/2] aspeed: add a GPIO controller to the SoC
  2019-06-18  8:51 [Qemu-devel] [PATCH 0/2] Add Aspeed GPIO controller model Rashmica Gupta
  2019-06-18  8:51 ` [Qemu-devel] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model Rashmica Gupta
@ 2019-06-18  8:51 ` Rashmica Gupta
  2019-06-18  9:21   ` Cédric Le Goater
  2019-06-18  9:24 ` [Qemu-devel] [PATCH 0/2] Add Aspeed GPIO controller model Cédric Le Goater
  2 siblings, 1 reply; 11+ messages in thread
From: Rashmica Gupta @ 2019-06-18  8:51 UTC (permalink / raw)
  To: qemu-arm; +Cc: andrew, clg, qemu-devel, Rashmica Gupta, joel

Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 hw/arm/aspeed_soc.c         | 17 +++++++++++++++++
 include/hw/arm/aspeed_soc.h |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 1cc98b9f40..8583869acf 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -23,6 +23,7 @@
 #include "net/net.h"
 
 #define ASPEED_SOC_IOMEM_SIZE       0x00200000
+#define ASPEED_SOC_GPIO_BASE        0x1E780000
 
 static const hwaddr aspeed_soc_ast2400_memmap[] = {
     [ASPEED_IOMEM]  = 0x1E600000,
@@ -120,6 +121,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .spis_num     = 1,
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
+        .gpio_typename = "aspeed.gpio-ast2400",
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
         .memmap       = aspeed_soc_ast2400_memmap,
@@ -131,6 +133,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .spis_num     = 1,
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
+        .gpio_typename = "aspeed.gpio-ast2400",
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
         .memmap       = aspeed_soc_ast2400_memmap,
@@ -142,6 +145,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .spis_num     = 1,
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
+        .gpio_typename = "aspeed.gpio-ast2400",
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
         .memmap       = aspeed_soc_ast2400_memmap,
@@ -153,6 +157,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .spis_num     = 2,
         .fmc_typename = "aspeed.smc.ast2500-fmc",
         .spi_typename = aspeed_soc_ast2500_typenames,
+        .gpio_typename = "aspeed.gpio-ast2500",
         .wdts_num     = 3,
         .irqmap       = aspeed_soc_ast2500_irqmap,
         .memmap       = aspeed_soc_ast2500_memmap,
@@ -225,6 +230,8 @@ static void aspeed_soc_init(Object *obj)
 
     sysbus_init_child_obj(obj, "ftgmac100", OBJECT(&s->ftgmac100),
                           sizeof(s->ftgmac100), TYPE_FTGMAC100);
+    sysbus_init_child_obj(obj, "gpio", OBJECT(&s->gpio), sizeof(s->gpio),
+                          sc->info->gpio_typename);
 }
 
 static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -366,6 +373,16 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
                     sc->info->memmap[ASPEED_ETH1]);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100), 0,
                        aspeed_soc_get_irq(s, ASPEED_ETH1));
+
+    /* GPIO */
+    object_property_set_bool(OBJECT(&s->gpio), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio), 0, ASPEED_SOC_GPIO_BASE);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
+            qdev_get_gpio_in(DEVICE(&s->vic), 20));
 }
 
 static void aspeed_soc_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 88b901d5df..28ff2bedb4 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -20,6 +20,7 @@
 #include "hw/ssi/aspeed_smc.h"
 #include "hw/watchdog/wdt_aspeed.h"
 #include "hw/net/ftgmac100.h"
+#include "hw/gpio/aspeed_gpio.h"
 
 #define ASPEED_SPIS_NUM  2
 #define ASPEED_WDTS_NUM  3
@@ -40,6 +41,7 @@ typedef struct AspeedSoCState {
     AspeedSDMCState sdmc;
     AspeedWDTState wdt[ASPEED_WDTS_NUM];
     FTGMAC100State ftgmac100;
+    AspeedGPIOState gpio;
 } AspeedSoCState;
 
 #define TYPE_ASPEED_SOC "aspeed-soc"
@@ -53,6 +55,7 @@ typedef struct AspeedSoCInfo {
     int spis_num;
     const char *fmc_typename;
     const char **spi_typename;
+    const char *gpio_typename;
     int wdts_num;
     const int *irqmap;
     const hwaddr *memmap;
-- 
2.17.2



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model
  2019-06-18  8:51 ` [Qemu-devel] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model Rashmica Gupta
@ 2019-06-18  9:04   ` Cédric Le Goater
  2019-07-03  4:16   ` Andrew Jeffery
  1 sibling, 0 replies; 11+ messages in thread
From: Cédric Le Goater @ 2019-06-18  9:04 UTC (permalink / raw)
  To: Rashmica Gupta, qemu-arm; +Cc: andrew, qemu-devel, joel

On 18/06/2019 10:51, Rashmica Gupta wrote:
> Add in details for GPIO controller for AST 2400 and 2500

I agree.

C. 


> 
> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
> ---
>  hw/gpio/Makefile.objs         |   1 +
>  hw/gpio/aspeed_gpio.c         | 869 ++++++++++++++++++++++++++++++++++
>  include/hw/gpio/aspeed_gpio.h |  76 +++
>  3 files changed, 946 insertions(+)
>  create mode 100644 hw/gpio/aspeed_gpio.c
>  create mode 100644 include/hw/gpio/aspeed_gpio.h
> 
> diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
> index e5da0cb54f..d305b3b24b 100644
> --- a/hw/gpio/Makefile.objs
> +++ b/hw/gpio/Makefile.objs
> @@ -9,3 +9,4 @@ obj-$(CONFIG_OMAP) += omap_gpio.o
>  obj-$(CONFIG_IMX) += imx_gpio.o
>  obj-$(CONFIG_RASPI) += bcm2835_gpio.o
>  obj-$(CONFIG_NRF51_SOC) += nrf51_gpio.o
> +obj-$(CONFIG_ASPEED_SOC) += aspeed_gpio.o
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> new file mode 100644
> index 0000000000..d84dd69255
> --- /dev/null
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -0,0 +1,869 @@
> +/*
> + *  ASPEED GPIO Controller
> + *
> + *  Copyright (C) 2017-2019 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + *
> + *
> + * This models the ast2400 and ast2500 GPIO controllers.
> + *
> + * GPIO pins are arranged in groups of 8 pins labeled A,B,..,Y,Z,AA,AB,AC.
> + * (Note that the ast2400 controller only goes up to group AB).
> + * A set has four groups (except set AC which only has one) and is
> + * referred to by the groups it is composed of (eg ABCD,EFGH,...,YZAAAB).
> + * Each set is accessed and controlled by a bank of 14 registers.
> + *
> + * These registers operate on a per pin level where each bit in the register
> + * corresponds to a pin, except for the command source registers. The command
> + * source registers operate on a per group level where bits 24, 16, 8 and 0
> + * correspond to each group in the set.
> + *
> + *  eg. registers for set ABCD:
> + *  |D7...D0|C7...C0|B7...B0|A7...A0| <- GPIOs
> + *  |31...24|23...16|15....8|7.....0| <- bit position
> + *
> + * Note that there are a couple of groups that only have 4 pins.
> +
> + * There are three ways that this model deviates from the behaviour of the
> + * actual controller:
> + * (1) There are three debounce registers which aren't modeled and so the per
> + * set debounce setting registers don't affect anything.
> + *
> + * (2) The only control source driving the GPIO pins in the model is the ARM
> + * model (as there currently aren't models for the LPC or Coprocessor [TODO]).
> + *
> + * (3) None of the registers in the model are reset tolerant. [TODO]
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/gpio/aspeed_gpio.h"
> +#include "include/hw/misc/aspeed_scu.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +
> +#define ASPEED_GPIOS_PER_REG 32
> +
> +/* GPIO Source Types */
> +#define ASPEED_CMD_SRC_MASK         0x01010101
> +#define ASPEED_SOURCE_ARM           0
> +#define ASPEED_SOURCE_LPC           1
> +#define ASPEED_SOURCE_COPROCESSOR   2
> +#define ASPEED_SOURCE_RESERVED      3
> +
> +/* GPIO Interrupt Triggers */
> +#define ASPEED_FALLING_EDGE 0
> +#define ASPEED_RISING_EDGE  1
> +#define ASPEED_LEVEL_LOW    2
> +#define ASPEED_LEVEL_HIGH   3
> +#define ASPEED_DUAL_EDGE    4
> +
> +/* GPIO Register Address Offsets */
> +#define GPIO_ABCD_DATA_VALUE     0x000
> +#define GPIO_ABCD_DIRECTION      0x004
> +#define GPIO_ABCD_INT_ENABLE     0x008
> +#define GPIO_ABCD_INT_SENS_0     0x00C
> +#define GPIO_ABCD_INT_SENS_1     0x010
> +#define GPIO_ABCD_INT_SENS_2     0x014
> +#define GPIO_ABCD_INT_STATUS     0x018
> +#define GPIO_ABCD_RESET_TOLERANT 0x01C
> +#define GPIO_EFGH_DATA_VALUE     0x020
> +#define GPIO_EFGH_DIRECTION      0x024
> +#define GPIO_EFGH_INT_ENABLE     0x028
> +#define GPIO_EFGH_INT_SENS_0     0x02C
> +#define GPIO_EFGH_INT_SENS_1     0x030
> +#define GPIO_EFGH_INT_SENS_2     0x034
> +#define GPIO_EFGH_INT_STATUS     0x038
> +#define GPIO_EFGH_RESET_TOL      0x03C
> +#define GPIO_ABCD_DEBOUNCE_1     0x040
> +#define GPIO_ABCD_DEBOUNCE_2     0x044
> +#define GPIO_EFGH_DEBOUNCE_1     0x048
> +#define GPIO_EFGH_DEBOUNCE_2     0x04C
> +#define GPIO_DEBOUNCE_TIME_1     0x050
> +#define GPIO_DEBOUNCE_TIME_2     0x054
> +#define GPIO_DEBOUNCE_TIME_3     0x058
> +#define GPIO_ABCD_COMMAND_SRC_0  0x060
> +#define GPIO_ABCD_COMMAND_SRC_1  0x064
> +#define GPIO_EFGH_COMMAND_SRC_0  0x068
> +#define GPIO_EFGH_COMMAND_SRC_1  0x06C
> +#define GPIO_IJKL_DATA_VALUE     0x070
> +#define GPIO_IJKL_DIRECTION      0x074
> +#define GPIO_MNOP_DATA_VALUE     0x078
> +#define GPIO_MNOP_DIRECTION      0x07C
> +#define GPIO_QRST_DATA_VALUE     0x080
> +#define GPIO_QRST_DIRECTION      0x084
> +#define GPIO_UVWX_DATA_VALUE     0x088
> +#define GPIO_UWVX_DIRECTION      0x08C
> +#define GPIO_IJKL_COMMAND_SRC_0  0x090
> +#define GPIO_IJKL_COMMAND_SRC_1  0x094
> +#define GPIO_IJKL_INT_ENABLE     0x098
> +#define GPIO_IJKL_INT_SENS_0     0x09C
> +#define GPIO_IJKL_INT_SENS_1     0x0A0
> +#define GPIO_IJKL_INT_SENS_2     0x0A4
> +#define GPIO_IJKL_INT_STATUS     0x0A8
> +#define GPIO_IJKL_RESET_TOLERANT 0x0AC
> +#define GPIO_IJKL_DEBOUNCE_1     0x0B0
> +#define GPIO_IJKL_DEBOUNCE_2     0x0B4
> +#define GPIO_IJKL_INPUT_MASK     0x0B8
> +#define GPIO_ABCD_DATA_READ      0x0C0
> +#define GPIO_EFGH_DATA_READ      0x0C4
> +#define GPIO_IJKL_DATA_READ      0x0C8
> +#define GPIO_MNOP_DATA_READ      0x0CC
> +#define GPIO_QRST_DATA_READ      0x0D0
> +#define GPIO_UVWX_DATA_READ      0x0D4
> +#define GPIO_YZAAAB_DATA_READ    0x0D8
> +#define GPIO_AC_DATA_READ        0x0DC
> +#define GPIO_MNOP_COMMAND_SRC_0  0x0E0
> +#define GPIO_MNOP_COMMAND_SRC_1  0x0E4
> +#define GPIO_MNOP_INT_ENABLE     0x0E8
> +#define GPIO_MNOP_INT_SENS_0     0x0EC
> +#define GPIO_MNOP_INT_SENS_1     0x0F0
> +#define GPIO_MNOP_INT_SENS_2     0x0F4
> +#define GPIO_MNOP_INT_STATUS     0x0F8
> +#define GPIO_MNOP_RESET_TOLERANT 0x0FC
> +#define GPIO_MNOP_DEBOUNCE_1     0x100
> +#define GPIO_MNOP_DEBOUNCE_2     0x104
> +#define GPIO_MNOP_INPUT_MASK     0x108
> +#define GPIO_QRST_COMMAND_SRC_0  0x110
> +#define GPIO_QRST_COMMAND_SRC_1  0x114
> +#define GPIO_QRST_INT_ENABLE     0x118
> +#define GPIO_QRST_INT_SENS_0     0x11C
> +#define GPIO_QRST_INT_SENS_1     0x120
> +#define GPIO_QRST_INT_SENS_2     0x124
> +#define GPIO_QRST_INT_STATUS     0x128
> +#define GPIO_QRST_RESET_TOLERANT 0x12C
> +#define GPIO_QRST_DEBOUNCE_1     0x130
> +#define GPIO_QRST_DEBOUNCE_2     0x134
> +#define GPIO_QRST_INPUT_MASK     0x138
> +#define GPIO_UVWX_COMMAND_SRC_0  0x140
> +#define GPIO_UVWX_COMMAND_SRC_1  0x144
> +#define GPIO_UVWX_INT_ENABLE     0x148
> +#define GPIO_UVWX_INT_SENS_0     0x14C
> +#define GPIO_UVWX_INT_SENS_1     0x150
> +#define GPIO_UVWX_INT_SENS_2     0x154
> +#define GPIO_UVWX_INT_STATUS     0x158
> +#define GPIO_UVWX_RESET_TOLERANT 0x15C
> +#define GPIO_UVWX_DEBOUNCE_1     0x160
> +#define GPIO_UVWX_DEBOUNCE_2     0x164
> +#define GPIO_UVWX_INPUT_MASK     0x168
> +#define GPIO_YZAAAB_COMMAND_SRC_0 0x170
> +#define GPIO_YZAAAB_COMMAND_SRC_1 0x174
> +#define GPIO_YZAAAB_INT_ENABLE   0x178
> +#define GPIO_YZAAAB_INT_SENS_0   0x17C
> +#define GPIO_YZAAAB_INT_SENS_1   0x180
> +#define GPIO_YZAAAB_INT_SENS_2   0x184
> +#define GPIO_YZAAAB_INT_STATUS   0x188
> +#define GPIO_YZAAAB_RESET_TOLERANT 0x18C
> +#define GPIO_YZAAAB_DEBOUNCE_1   0x190
> +#define GPIO_YZAAAB_DEBOUNCE_2   0x194
> +#define GPIO_YZAAAB_INPUT_MASK   0x198
> +#define GPIO_AC_COMMAND_SRC_0    0x1A0
> +#define GPIO_AC_COMMAND_SRC_1    0x1A4
> +#define GPIO_AC_INT_ENABLE       0x1A8
> +#define GPIO_AC_INT_SENS_0       0x1AC
> +#define GPIO_AC_INT_SENS_1       0x1B0
> +#define GPIO_AC_INT_SENS_2       0x1B4
> +#define GPIO_AC_INT_STATUS       0x1B8
> +#define GPIO_AC_RESET_TOLERANT   0x1BC
> +#define GPIO_AC_DEBOUNCE_1       0x1C0
> +#define GPIO_AC_DEBOUNCE_2       0x1C4
> +#define GPIO_AC_INPUT_MASK       0x1C8
> +#define GPIO_ABCD_INPUT_MASK     0x1D0
> +#define GPIO_EFGH_INPUT_MASK     0x1D4
> +#define GPIO_YZAAAB_DATA_VALUE   0x1E0
> +#define GPIO_YZAAAB_DIRECTION    0x1E4
> +#define GPIO_AC_DATA_VALUE       0x1E8
> +#define GPIO_AC_DIRECTION        0x1EC
> +
> +struct AspeedGPIO {
> +    uint16_t set_idx;
> +    uint32_t (*get)(GPIORegs *regs);
> +    void (*set)(AspeedGPIOState *s, GPIORegs *regs,
> +                GPIOSetProperties *props, uint32_t val);
> +};
> +
> +static int aspeed_evaluate_irq(GPIORegs *regs, int prev_value_high, int bit)
> +{
> +    uint32_t falling_edge = 0, rising_edge = 0;
> +    uint32_t int_trigger = extract32(regs->int_sens_0, bit, 1)
> +                           | extract32(regs->int_sens_1, bit, 1) << 1
> +                           | extract32(regs->int_sens_2, bit, 1) << 2 ;
> +    uint32_t curr_pin_high = extract32(regs->data_value, bit, 1);
> +
> +    /* Detect edges */
> +    if (curr_pin_high && !prev_value_high) {
> +            rising_edge = 1;
> +    } else if (!curr_pin_high && prev_value_high) {
> +            falling_edge = 1;
> +    }
> +
> +    if (((int_trigger == ASPEED_FALLING_EDGE)  && falling_edge)  ||
> +        ((int_trigger == ASPEED_RISING_EDGE)  && rising_edge)  ||
> +        ((int_trigger == ASPEED_LEVEL_LOW)  && !curr_pin_high)  ||
> +        ((int_trigger == ASPEED_LEVEL_HIGH)  && curr_pin_high)  ||
> +        ((int_trigger >= ASPEED_DUAL_EDGE)  && (rising_edge || falling_edge)))
> +    {
> +        regs->int_status = deposit32(regs->int_status, bit, 1, 1);
> +        return 1;
> +    }
> +    return 0;
> +}
> +
> +static void aspeed_gpio_update(AspeedGPIOState *s, GPIORegs *regs)
> +{
> +    uint32_t input_mask = regs->input_mask;
> +    uint32_t direction = regs->direction;
> +    uint32_t old = regs->data_value;
> +    uint32_t new = regs->data_read;
> +    uint32_t diff;
> +    int bit;
> +
> +    diff = old ^ new;
> +    if (!diff) {
> +        return;
> +    }
> +
> +    if (!direction) {
> +        return;
> +    }
> +
> +    for (bit = 0; bit < ASPEED_GPIOS_PER_REG; bit++) {
> +        uint32_t mask = 1 << bit;
> +        /* If the bit needs to be updated... */
> +        if (!(diff & mask)) {
> +            continue;
> +        }
> +        /* ...and corresponds to an output pin...*/
> +        if (!(direction & mask)) {
> +            continue;
> +        }
> +        /* ...and isn't masked...  */
> +        if (input_mask & mask) {
> +            continue;
> +        }
> +        /* ... then update it*/
> +        if (mask & new) {
> +            regs->data_value |= mask;
> +        } else {
> +            regs->data_value &= ~mask;
> +        }
> +
> +        if (aspeed_evaluate_irq(regs, old & mask, bit)) {
> +            qemu_set_irq(s->output[bit], 1);
> +        }
> +    }
> +}
> +
> +static uint32_t aspeed_adjust_pin(AspeedGPIOState *s, uint32_t pin)
> +{
> +    if (pin >= s->ctrl->gap) {
> +        pin += 4;
> +    }
> +
> +    return pin;
> +}
> +
> +static uint32_t aspeed_get_set_idx_from_pin(AspeedGPIOState *s, uint32_t pin)
> +{
> +    /*
> +     * For most pins, dividing by 32 gets the set index.
> +     *
> +     * However the 2500 has a 4 pin gap in group AB and the 2400 has a 4 pin
> +     * gap in group Y (and only four pins in AB but this is the last group so
> +     * it doesn't matter).
> +     */
> +    return aspeed_adjust_pin(s, pin) >> 5;
> +}
> +
> +static bool aspeed_gpio_get_pin_level(AspeedGPIOState *s, uint32_t pin)
> +{
> +    uint32_t reg_val;
> +    uint32_t mask = 1 << (pin);
> +    uint32_t set_idx = aspeed_get_set_idx_from_pin(s, pin);
> +
> +    reg_val = s->sets[set_idx].data_value;
> +
> +    return !!(reg_val & mask);
> +}
> +
> +static void aspeed_gpio_set_pin_level(AspeedGPIOState *s, uint32_t pin,
> +                                      bool level)
> +{
> +    uint32_t mask  = 1 << (pin);
> +    uint32_t set_idx = aspeed_get_set_idx_from_pin(s, pin);
> +
> +    if (level) {
> +        s->sets[set_idx].data_read |= mask;
> +    } else {
> +        s->sets[set_idx].data_read &= !mask;
> +    }
> +
> +    aspeed_gpio_update(s, &s->sets[set_idx]);
> +}
> +
> +/*
> + *  | src_1 | src_2 |  source     |
> + *  |-----------------------------|
> + *  |   0   |   0   |  ARM        |
> + *  |   0   |   1   |  LPC        |
> + *  |   1   |   0   |  Coprocessor|
> + *  |   1   |   1   |  Reserved   |
> + *
> + *  Once the source of a set is programmed, corresponding bits in the
> + *  data_value, direction, interrupt [enable, sens[0-2]], reset_tol and
> + *  debounce registers can only be written by the source.
> + *
> + *  Source is ARM by default
> + *  only bits 24, 16, 8, and 0 can be set
> + *
> + *  we don't currently have a model for the LPC or Coprocessor
> + */
> +static uint32_t update_value_control_source(GPIORegs *regs, uint32_t old_value,
> +                                            uint32_t value)
> +{
> +    int i;
> +    int cmd_source;
> +
> +    /* assume the source is always ARM for now */
> +    int source = ASPEED_SOURCE_ARM;
> +
> +    uint32_t new_value = 0;
> +
> +    /* for each group in set */
> +    for (i = 0; i < ASPEED_GPIOS_PER_REG; i += 8) {
> +        cmd_source = extract32(regs->cmd_source_0, i, 1)
> +                | (extract32(regs->cmd_source_1, i, 1) << 1);
> +
> +        if (source == cmd_source) {
> +            new_value |= (0xff << i) & value;
> +        } else {
> +            new_value |= (0xff << i) & old_value;
> +        }
> +    }
> +    return new_value;
> +}
> +
> +/* Reader helper functions */
> +static uint32_t read_direction(GPIORegs *regs)
> +{
> +    return regs->direction;
> +}
> +
> +static uint32_t read_data_value(GPIORegs *regs)
> +{
> +    return regs->data_value;
> +}
> +
> +static uint32_t read_int_enable(GPIORegs *regs)
> +{
> +    return regs->int_enable;
> +}
> +
> +static uint32_t read_int_sens_0(GPIORegs *regs)
> +{
> +    return regs->int_sens_0;
> +}
> +
> +static uint32_t read_int_sens_1(GPIORegs *regs)
> +{
> +    return regs->int_sens_1;
> +}
> +
> +static uint32_t read_int_sens_2(GPIORegs *regs)
> +{
> +    return regs->int_sens_2;
> +}
> +
> +static uint32_t read_int_status(GPIORegs *regs)
> +{
> +    return regs->int_status;
> +}
> +
> +static uint32_t read_reset_tol(GPIORegs *regs)
> +{
> +    return regs->reset_tol;
> +}
> +
> +static uint32_t read_debounce_1(GPIORegs *regs)
> +{
> +    return regs->debounce_1;
> +}
> +
> +static uint32_t read_debounce_2(GPIORegs *regs)
> +{
> +    return regs->debounce_2;
> +}
> +
> +static uint32_t read_cmd_source_0(GPIORegs *regs)
> +{
> +    return regs->cmd_source_0;
> +}
> +
> +static uint32_t read_cmd_source_1(GPIORegs *regs)
> +{
> +    return regs->cmd_source_1;
> +}
> +
> +static uint32_t read_data(GPIORegs *regs)
> +{
> +    return regs->data_read;
> +}
> +
> +static uint32_t read_input_mask(GPIORegs *regs)
> +{
> +    return regs->input_mask;
> +}
> +
> +/* Write helper functions */
> +
> +static void _write_data_value(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    val &= props->output | ~props->input;
> +    regs->data_read = update_value_control_source(regs, regs->data_read, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_direction(AspeedGPIOState *s, GPIORegs *regs,
> +                             GPIOSetProperties *props, uint32_t val)
> +{
> +    val &= props->output | ~props->input;
> +    regs->direction = update_value_control_source(regs, regs->direction, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_int_enable(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->int_enable = update_value_control_source(regs, regs->int_enable, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_int_sens_0(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->int_sens_0 = update_value_control_source(regs, regs->int_sens_0, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_int_sens_1(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->int_sens_1 = update_value_control_source(regs, regs->int_sens_1, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_int_sens_2(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->int_sens_2 = update_value_control_source(regs, regs->int_sens_2, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_int_status(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->int_status = val;
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_reset_tol(AspeedGPIOState *s, GPIORegs *regs,
> +                             GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->reset_tol = update_value_control_source(regs, regs->reset_tol, val);
> +}
> +
> +static void _write_debounce_1(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->debounce_1 = update_value_control_source(regs, regs->debounce_1, val);
> +}
> +
> +static void _write_debounce_2(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->debounce_2 = update_value_control_source(regs, regs->debounce_2, val);
> +}
> +
> +static void _write_cmd_source_0(AspeedGPIOState *s, GPIORegs *regs,
> +                                GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->cmd_source_0 = val & ASPEED_CMD_SRC_MASK;
> +}
> +
> +static void _write_cmd_source_1(AspeedGPIOState *s, GPIORegs *regs,
> +                                GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->cmd_source_1 = val & ASPEED_CMD_SRC_MASK;
> +}
> +
> +/*
> + * feeds into interrupt generation
> + * 0: read from data value reg will be updated
> + * 1: read from data value reg will not be updated
> + */
> +static void _write_input_mask(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->input_mask = val & props->input;
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static const struct AspeedGPIO gpios[0x1f0] = {
> +    /* Set ABCD */
> +    [GPIO_ABCD_DATA_VALUE] = {0, read_data_value, _write_data_value},
> +    [GPIO_ABCD_DIRECTION] = {0, read_direction, _write_direction},
> +    [GPIO_ABCD_INT_ENABLE] = {0, read_int_enable, _write_int_enable},
> +    [GPIO_ABCD_INT_SENS_0] = {0, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_ABCD_INT_SENS_1] = {0, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_ABCD_INT_SENS_2] = {0, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_ABCD_INT_STATUS] = {0, read_int_status, _write_int_status},
> +    [GPIO_ABCD_RESET_TOLERANT] = {0, read_reset_tol, _write_reset_tol},
> +    [GPIO_ABCD_DEBOUNCE_1] = {0, read_debounce_1, _write_debounce_1},
> +    [GPIO_ABCD_DEBOUNCE_2] = {0, read_debounce_2, _write_debounce_2},
> +    [GPIO_ABCD_COMMAND_SRC_0] = {0, read_cmd_source_0, _write_cmd_source_0},
> +    [GPIO_ABCD_COMMAND_SRC_1] = {0, read_cmd_source_1, _write_cmd_source_1},
> +    [GPIO_ABCD_DATA_READ] = {0, read_data, NULL},
> +    [GPIO_ABCD_INPUT_MASK] = {0, read_input_mask, _write_input_mask},
> +    /* Set EFGH */
> +    [GPIO_EFGH_DATA_VALUE] = {1, read_data_value, _write_data_value},
> +    [GPIO_EFGH_DIRECTION] = {1, read_direction, _write_direction },
> +    [GPIO_EFGH_INT_ENABLE] = {1, read_int_enable, _write_int_enable},
> +    [GPIO_EFGH_INT_SENS_0] = {1, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_EFGH_INT_SENS_1] = {1, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_EFGH_INT_SENS_2] = {1, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_EFGH_INT_STATUS] = {1, read_int_status, _write_int_status},
> +    [GPIO_EFGH_RESET_TOL] = {1, read_reset_tol,   _write_reset_tol},
> +    [GPIO_EFGH_DEBOUNCE_1] = {1, read_debounce_1,  _write_debounce_1},
> +    [GPIO_EFGH_DEBOUNCE_2] = {1, read_debounce_2,  _write_debounce_2},
> +    [GPIO_EFGH_COMMAND_SRC_0] = {1, read_cmd_source_0,  _write_cmd_source_0},
> +    [GPIO_EFGH_COMMAND_SRC_1] = {1, read_cmd_source_1,  _write_cmd_source_1},
> +    [GPIO_EFGH_DATA_READ] = {1, read_data, NULL},
> +    [GPIO_EFGH_INPUT_MASK] = {1, read_input_mask,  _write_input_mask},
> +    /* Set IJKL */
> +    [GPIO_IJKL_DATA_VALUE] = {2, read_data_value, _write_data_value},
> +    [GPIO_IJKL_DIRECTION] = {2, read_direction, _write_direction},
> +    [GPIO_IJKL_INT_ENABLE] = {2, read_int_enable, _write_int_enable},
> +    [GPIO_IJKL_INT_SENS_0] = {2, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_IJKL_INT_SENS_1] = {2, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_IJKL_INT_SENS_2] = {2, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_IJKL_INT_STATUS] = {2, read_int_status, _write_int_status},
> +    [GPIO_IJKL_RESET_TOLERANT] = {2, read_reset_tol, _write_reset_tol},
> +    [GPIO_IJKL_DEBOUNCE_1] = {2, read_debounce_1, _write_debounce_1},
> +    [GPIO_IJKL_DEBOUNCE_2] = {2, read_debounce_2, _write_debounce_2},
> +    [GPIO_IJKL_COMMAND_SRC_0] = {2, read_cmd_source_0, _write_cmd_source_0},
> +    [GPIO_IJKL_COMMAND_SRC_1] = {2, read_cmd_source_1, _write_cmd_source_1},
> +    [GPIO_IJKL_DATA_READ] = {2, read_data, NULL},
> +    [GPIO_IJKL_INPUT_MASK] = {2, read_input_mask, _write_input_mask},
> +    /* Set MNOP */
> +    [GPIO_MNOP_DATA_VALUE] = {3, read_data_value, _write_data_value},
> +    [GPIO_MNOP_DIRECTION] = {3, read_direction, _write_direction},
> +    [GPIO_MNOP_INT_ENABLE] = {3, read_int_enable, _write_int_enable},
> +    [GPIO_MNOP_INT_SENS_0] = {3, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_MNOP_INT_SENS_1] = {3, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_MNOP_INT_SENS_2] = {3, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_MNOP_INT_STATUS] = {3, read_int_status, _write_int_status},
> +    [GPIO_MNOP_RESET_TOLERANT] = {3, read_reset_tol,  _write_reset_tol},
> +    [GPIO_MNOP_DEBOUNCE_1] = {3, read_debounce_1, _write_debounce_1},
> +    [GPIO_MNOP_DEBOUNCE_2] = {3, read_debounce_2, _write_debounce_2},
> +    [GPIO_MNOP_COMMAND_SRC_0] = {3, read_cmd_source_0, _write_cmd_source_0},
> +    [GPIO_MNOP_COMMAND_SRC_1] = {3, read_cmd_source_1, _write_cmd_source_1},
> +    [GPIO_MNOP_DATA_READ] = {3, read_data, NULL},
> +    [GPIO_MNOP_INPUT_MASK] = {3, read_input_mask, _write_input_mask},
> +    /* Set QRST */
> +    [GPIO_QRST_DATA_VALUE] = {4, read_data_value, _write_data_value},
> +    [GPIO_QRST_DIRECTION] = {4, read_direction, _write_direction},
> +    [GPIO_QRST_INT_ENABLE] = {4, read_int_enable, _write_int_enable},
> +    [GPIO_QRST_INT_SENS_0] = {4, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_QRST_INT_SENS_1] = {4, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_QRST_INT_SENS_2] = {4, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_QRST_INT_STATUS] = {4, read_int_status, _write_int_status},
> +    [GPIO_QRST_RESET_TOLERANT] = {4, read_reset_tol, _write_reset_tol},
> +    [GPIO_QRST_DEBOUNCE_1] = {4, read_debounce_1, _write_debounce_1},
> +    [GPIO_QRST_DEBOUNCE_2] = {4, read_debounce_2, _write_debounce_2},
> +    [GPIO_QRST_COMMAND_SRC_0] = {4, read_cmd_source_0, _write_cmd_source_0},
> +    [GPIO_QRST_COMMAND_SRC_1] = {4, read_cmd_source_1, _write_cmd_source_1},
> +    [GPIO_QRST_DATA_READ] = {4, read_data, NULL},
> +    [GPIO_QRST_INPUT_MASK] = {4, read_input_mask,  _write_input_mask},
> +    /* Set UVWX */
> +    [GPIO_UVWX_DATA_VALUE] = {5, read_data_value, _write_data_value},
> +    [GPIO_UWVX_DIRECTION] = {5, read_direction, _write_direction},
> +    [GPIO_UVWX_INT_ENABLE] = {5, read_int_enable, _write_int_enable},
> +    [GPIO_UVWX_INT_SENS_0] = {5, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_UVWX_INT_SENS_1] = {5, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_UVWX_INT_SENS_2] = {5, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_UVWX_INT_STATUS] = {5, read_int_status, _write_int_status},
> +    [GPIO_UVWX_RESET_TOLERANT] = {5, read_reset_tol, _write_reset_tol},
> +    [GPIO_UVWX_DEBOUNCE_1] = {5, read_debounce_1, _write_debounce_1},
> +    [GPIO_UVWX_DEBOUNCE_2] = {5, read_debounce_2, _write_debounce_2},
> +    [GPIO_UVWX_COMMAND_SRC_0] = {5, read_cmd_source_0, _write_cmd_source_0},
> +    [GPIO_UVWX_COMMAND_SRC_1] = {5, read_cmd_source_1, _write_cmd_source_1},
> +    [GPIO_UVWX_DATA_READ] = {5, read_data, NULL},
> +    [GPIO_UVWX_INPUT_MASK] = {5, read_input_mask, _write_input_mask},
> +    /* Set YZAAAB */
> +    [GPIO_YZAAAB_DATA_VALUE] = {6, read_data_value, _write_data_value},
> +    [GPIO_YZAAAB_DIRECTION] = {6, read_direction, _write_direction},
> +    [GPIO_YZAAAB_INT_ENABLE] = {6, read_int_enable, _write_int_enable},
> +    [GPIO_YZAAAB_INT_SENS_0] = {6, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_YZAAAB_INT_SENS_1] = {6, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_YZAAAB_INT_SENS_2] = {6, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_YZAAAB_INT_STATUS] = {6, read_int_status, _write_int_status},
> +    [GPIO_YZAAAB_RESET_TOLERANT] = {6, read_reset_tol, _write_reset_tol},
> +    [GPIO_YZAAAB_DEBOUNCE_1] = {6, read_debounce_1, _write_debounce_1},
> +    [GPIO_YZAAAB_DEBOUNCE_2] = {6, read_debounce_2, _write_debounce_2},
> +    [GPIO_YZAAAB_COMMAND_SRC_0] = {6, read_cmd_source_0, _write_cmd_source_0},
> +    [GPIO_YZAAAB_COMMAND_SRC_1] = {6, read_cmd_source_1, _write_cmd_source_1},
> +    [GPIO_YZAAAB_DATA_READ] = {6, read_data, NULL},
> +    [GPIO_YZAAAB_INPUT_MASK] = {6, read_input_mask, _write_input_mask},
> +    /* Set AC */
> +    [GPIO_AC_DATA_VALUE] = {7, read_data_value, _write_data_value},
> +    [GPIO_AC_DIRECTION] = {7, read_direction, _write_direction},
> +    [GPIO_AC_INT_ENABLE] = {7, read_int_enable, _write_int_enable},
> +    [GPIO_AC_INT_SENS_0] = {7, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_AC_INT_SENS_1] = {7, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_AC_INT_SENS_2] = {7, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_AC_INT_STATUS] = {7, read_int_status, _write_int_status},
> +    [GPIO_AC_RESET_TOLERANT] = {7, read_reset_tol, _write_reset_tol},
> +    [GPIO_AC_DEBOUNCE_1] = {7, read_debounce_1, _write_debounce_1},
> +    [GPIO_AC_DEBOUNCE_2] = {7, read_debounce_2, _write_debounce_2},
> +    [GPIO_AC_COMMAND_SRC_0] = {7, read_cmd_source_0, _write_cmd_source_0},
> +    [GPIO_AC_COMMAND_SRC_1] = {7, read_cmd_source_1, _write_cmd_source_1},
> +    [GPIO_AC_DATA_READ] = {7, read_data, NULL},
> +    [GPIO_AC_INPUT_MASK] = {7, read_input_mask,     _write_input_mask},
> +    /* Debounce registers */
> +    [GPIO_DEBOUNCE_TIME_1] = {-1, NULL, NULL},
> +};
> +
> +static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size)
> +{
> +    AspeedGPIOState *s = ASPEED_GPIO(opaque);
> +    uint32_t val = 0;
> +
> +    if (size != 4) {
> +        return 0;
> +    }
> +
> +    if (gpios[offset].get == NULL) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "no getter for offset %lx", offset);
> +        return 0;
> +    }
> +
> +    val = gpios[offset].get(&s->sets[gpios[offset].set_idx]);
> +    return (uint64_t) val;
> +}
> +
> +static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
> +                              uint32_t size)
> +{
> +    AspeedGPIOState *s = ASPEED_GPIO(opaque);
> +    GPIOSetProperties *props = &s->ctrl->props[gpios[offset].set_idx];
> +
> +    if (gpios[offset].set == NULL) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "no setter for offset %lx", offset);
> +        return;
> +    }
> +
> +    uint32_t mask = props->input | props->output;
> +
> +    gpios[offset].set(s, &s->sets[gpios[offset].set_idx],
> +                      props, data & mask);
> +}
> +
> +static void aspeed_gpio_get_pin(Object *obj, Visitor *v, const char *name,
> +                                void *opaque, Error **errp)
> +{
> +    int pin = 0xfff;
> +    bool level = true;
> +    char group[3];
> +    AspeedGPIOState *s = ASPEED_GPIO(obj);
> +    if (sscanf(name, "gpio%2[A-Z]%1d", group, &pin) != 2) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "error reading %s", name);
> +        return;
> +    }
> +    level = aspeed_gpio_get_pin_level(s, pin);
> +    visit_type_bool(v, name, &level, errp);
> +}
> +
> +static void aspeed_gpio_set_pin(Object *obj, Visitor *v, const char *name,
> +                               void *opaque, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    bool level;
> +    int pin = 0xfff;
> +    char group[3];
> +    AspeedGPIOState *s = ASPEED_GPIO(obj);
> +    visit_type_bool(v, name, &level, &local_err);
> +    if (sscanf(name, "gpio%2[A-Z]%1d", group, &pin) != 2) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "error reading %s", name);
> +        return;
> +    }
> +    aspeed_gpio_set_pin_level(s, pin, level);
> +}
> +
> +
> +/* Setup functions */
> +static const MemoryRegionOps aspeed_gpio_ops = {
> +    .read = aspeed_gpio_read,
> +    .write = aspeed_gpio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +};
> +
> +static void aspeed_gpio_reset(DeviceState *dev)
> +{
> +    struct AspeedGPIOState *s = ASPEED_GPIO(dev);
> +
> +    /* TODO: respect the reset tolerance registers */
> +    memset(s->sets, 0, sizeof(s->sets));
> +}
> +
> +static void aspeed_gpio_realize(DeviceState *dev, Error **errp)
> +{
> +    AspeedGPIOState *s = ASPEED_GPIO(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +    sysbus_init_irq(sbd, &s->irq);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s,
> +            TYPE_ASPEED_GPIO, 0x1000);
> +
> +    sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static const VMStateDescription vmstate_gpio_regs = {
> +    .name = TYPE_ASPEED_GPIO"/regs",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(data_value,   GPIORegs),
> +        VMSTATE_UINT32(data_read,    GPIORegs),
> +        VMSTATE_UINT32(direction,    GPIORegs),
> +        VMSTATE_UINT32(int_enable,   GPIORegs),
> +        VMSTATE_UINT32(int_sens_0,   GPIORegs),
> +        VMSTATE_UINT32(int_sens_1,   GPIORegs),
> +        VMSTATE_UINT32(int_sens_2,   GPIORegs),
> +        VMSTATE_UINT32(int_status,   GPIORegs),
> +        VMSTATE_UINT32(reset_tol,    GPIORegs),
> +        VMSTATE_UINT32(cmd_source_0, GPIORegs),
> +        VMSTATE_UINT32(cmd_source_1, GPIORegs),
> +        VMSTATE_UINT32(debounce_1,   GPIORegs),
> +        VMSTATE_UINT32(debounce_2,   GPIORegs),
> +        VMSTATE_UINT32(input_mask,   GPIORegs),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};
> +static const VMStateDescription vmstate_aspeed_gpio = {
> +    .name = TYPE_ASPEED_GPIO,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT_ARRAY(sets, AspeedGPIOState, ASPEED_GPIO_MAX_NR_SETS,
> +                             1, vmstate_gpio_regs, GPIORegs),
> +        VMSTATE_END_OF_LIST(),
> +   }
> +};
> +
> +static void aspeed_gpio_init(Object *obj)
> +{
> +    AspeedGPIOState *s = ASPEED_GPIO(obj);
> +    AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> +    int pin;
> +
> +    s->ctrl = agc->ctrl;
> +
> +    for (pin = 0; pin < agc->ctrl->nr_gpio_pins; pin++) {
> +        char *name;
> +        int set_idx = aspeed_get_set_idx_from_pin(s, pin);
> +        int _pin = aspeed_adjust_pin(s, pin);
> +        int pin_idx = _pin - (set_idx * 32);
> +        int group_idx = pin_idx >> 3;
> +
> +        name = g_strdup_printf("gpio%s%d",
> +                               agc->ctrl->props[set_idx].set[group_idx],
> +                               pin_idx % 8);
> +        object_property_add(obj, name, "bool",
> +                            aspeed_gpio_get_pin,
> +                            aspeed_gpio_set_pin,
> +                            NULL, NULL, NULL);
> +    }
> +}
> +
> +static void aspeed_gpio_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    AspeedGPIOClass *agc = ASPEED_GPIO_CLASS(klass);
> +
> +    dc->realize = aspeed_gpio_realize;
> +    dc->reset = aspeed_gpio_reset;
> +    dc->desc = "Aspeed GPIO Controller";
> +    dc->vmsd = &vmstate_aspeed_gpio;
> +    agc->ctrl = data;
> +}
> +
> +static GPIOSetProperties ast2400_set_props[] = {
> +    [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
> +    [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
> +    [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
> +    [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
> +    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
> +    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> +    [6] = {0x0000000f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
> +};
> +
> +static GPIOSetProperties ast2500_set_props[] = {
> +    [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
> +    [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
> +    [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
> +    [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
> +    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
> +    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> +    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
> +    [7] = {0x000000ff,  0x000000ff,  {"AC"} },
> +};
> +
> +static AspeedGPIOController controllers[] = {
> +    {
> +        .name           = TYPE_ASPEED_GPIO "-ast2500",
> +        .props          = ast2500_set_props,
> +        .nr_gpio_pins   = 228,
> +        .nr_gpio_sets   = 8,
> +        .gap            = 220,
> +    }, {
> +        .name           = TYPE_ASPEED_GPIO "-ast2400",
> +        .props          = ast2400_set_props,
> +        .nr_gpio_pins   = 216,
> +        .nr_gpio_sets   = 7,
> +        .gap            = 196,
> +    }
> +};
> +
> +static const TypeInfo aspeed_gpio_info = {
> +    .name = TYPE_ASPEED_GPIO,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AspeedGPIOState),
> +    .class_size     = sizeof(AspeedGPIOClass),
> +    .instance_init = aspeed_gpio_init,
> +    .abstract       = true,
> +};
> +
> +static void aspeed_gpio_register_types(void)
> +{
> +    int i;
> +
> +    type_register_static(&aspeed_gpio_info);
> +    for (i = 0; i < ARRAY_SIZE(controllers); i++) {
> +        TypeInfo t = {
> +            .name = controllers[i].name,
> +            .parent = TYPE_ASPEED_GPIO,
> +            .class_init = aspeed_gpio_class_init,
> +            .class_data = (void *)&controllers[i],
> +        };
> +        type_register(&t);
> +    };
> +}
> +
> +type_init(aspeed_gpio_register_types);
> diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
> new file mode 100644
> index 0000000000..97a84a5da1
> --- /dev/null
> +++ b/include/hw/gpio/aspeed_gpio.h
> @@ -0,0 +1,76 @@
> +/*
> + *  ASPEED GPIO Controller
> + *
> + *  Copyright (C) 2017-2018 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef ASPEED_GPIO_H
> +#define ASPEED_GPIO_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_ASPEED_GPIO "aspeed.gpio"
> +#define ASPEED_GPIO(obj) OBJECT_CHECK(AspeedGPIOState, (obj), TYPE_ASPEED_GPIO)
> +#define ASPEED_GPIO_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(AspeedGPIOClass, (klass), TYPE_ASPEED_GPIO)
> +#define ASPEED_GPIO_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(AspeedGPIOClass, (obj), TYPE_ASPEED_GPIO)
> +
> +#define ASPEED_GPIO_MAX_NR_SETS 8
> +#define ASPEED_REGS_PER_BANK 14
> +#define ASPEED_GPIO_MAX_NR_REGS (ASPEED_REGS_PER_BANK * ASPEED_GPIO_MAX_NR_SETS)
> +#define ASPEED_GPIO_NR_PINS 228
> +
> +typedef struct GPIORegs GPIORegs;
> +typedef const struct GPIOSetProperties {
> +    uint32_t input;
> +    uint32_t output;
> +    char set[4][3];
> +} GPIOSetProperties;
> +
> +typedef const struct AspeedGPIOController {
> +    const char *name;
> +    GPIOSetProperties *props;
> +    uint32_t nr_gpio_pins;
> +    uint32_t nr_gpio_sets;
> +    uint32_t gap;
> +} AspeedGPIOController;
> +
> +typedef struct  AspeedGPIOClass {
> +    SysBusDevice parent_obj;
> +    AspeedGPIOController *ctrl;
> +}  AspeedGPIOClass;
> +
> +typedef struct AspeedGPIOState {
> +    /* <private> */
> +    SysBusDevice parent;
> +
> +    /*< public >*/
> +    MemoryRegion iomem;
> +    qemu_irq output[ASPEED_GPIO_NR_PINS];
> +    qemu_irq irq;
> +    AspeedGPIOController *ctrl;
> +
> +/* Parallel GPIO Registers */
> +    struct GPIORegs {
> +        uint32_t data_value; /* Reflects pin values */
> +        uint32_t data_read; /* Contains last value written to data value */
> +        uint32_t direction;
> +        uint32_t int_enable;
> +        uint32_t int_sens_0;
> +        uint32_t int_sens_1;
> +        uint32_t int_sens_2;
> +        uint32_t int_status;
> +        uint32_t reset_tol;
> +        uint32_t cmd_source_0;
> +        uint32_t cmd_source_1;
> +        uint32_t debounce_1;
> +        uint32_t debounce_2;
> +        uint32_t input_mask;
> +    } sets[ASPEED_GPIO_MAX_NR_SETS];
> +} AspeedGPIOState;
> +
> +#endif /* _ASPEED_GPIO_H_ */
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] aspeed: add a GPIO controller to the SoC
  2019-06-18  8:51 ` [Qemu-devel] [PATCH 2/2] aspeed: add a GPIO controller to the SoC Rashmica Gupta
@ 2019-06-18  9:21   ` Cédric Le Goater
  2019-06-19  0:19     ` Rashmica Gupta
  0 siblings, 1 reply; 11+ messages in thread
From: Cédric Le Goater @ 2019-06-18  9:21 UTC (permalink / raw)
  To: Rashmica Gupta, qemu-arm; +Cc: andrew, qemu-devel, joel

On 18/06/2019 10:51, Rashmica Gupta wrote:
> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
> ---
>  hw/arm/aspeed_soc.c         | 17 +++++++++++++++++
>  include/hw/arm/aspeed_soc.h |  3 +++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 1cc98b9f40..8583869acf 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -23,6 +23,7 @@
>  #include "net/net.h"
>  
>  #define ASPEED_SOC_IOMEM_SIZE       0x00200000
> +#define ASPEED_SOC_GPIO_BASE        0x1E780000
>

You should put this value in the memmap array.

C. 


>  static const hwaddr aspeed_soc_ast2400_memmap[] = {
>      [ASPEED_IOMEM]  = 0x1E600000,
> @@ -120,6 +121,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
>          .spis_num     = 1,
>          .fmc_typename = "aspeed.smc.fmc",
>          .spi_typename = aspeed_soc_ast2400_typenames,
> +        .gpio_typename = "aspeed.gpio-ast2400",
>          .wdts_num     = 2,
>          .irqmap       = aspeed_soc_ast2400_irqmap,
>          .memmap       = aspeed_soc_ast2400_memmap,
> @@ -131,6 +133,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
>          .spis_num     = 1,
>          .fmc_typename = "aspeed.smc.fmc",
>          .spi_typename = aspeed_soc_ast2400_typenames,
> +        .gpio_typename = "aspeed.gpio-ast2400",
>          .wdts_num     = 2,
>          .irqmap       = aspeed_soc_ast2400_irqmap,
>          .memmap       = aspeed_soc_ast2400_memmap,
> @@ -142,6 +145,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
>          .spis_num     = 1,
>          .fmc_typename = "aspeed.smc.fmc",
>          .spi_typename = aspeed_soc_ast2400_typenames,
> +        .gpio_typename = "aspeed.gpio-ast2400",
>          .wdts_num     = 2,
>          .irqmap       = aspeed_soc_ast2400_irqmap,
>          .memmap       = aspeed_soc_ast2400_memmap,
> @@ -153,6 +157,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
>          .spis_num     = 2,
>          .fmc_typename = "aspeed.smc.ast2500-fmc",
>          .spi_typename = aspeed_soc_ast2500_typenames,
> +        .gpio_typename = "aspeed.gpio-ast2500",
>          .wdts_num     = 3,
>          .irqmap       = aspeed_soc_ast2500_irqmap,
>          .memmap       = aspeed_soc_ast2500_memmap,
> @@ -225,6 +230,8 @@ static void aspeed_soc_init(Object *obj)
>  
>      sysbus_init_child_obj(obj, "ftgmac100", OBJECT(&s->ftgmac100),
>                            sizeof(s->ftgmac100), TYPE_FTGMAC100);
> +    sysbus_init_child_obj(obj, "gpio", OBJECT(&s->gpio), sizeof(s->gpio),
> +                          sc->info->gpio_typename);
>  }
>  
>  static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> @@ -366,6 +373,16 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>                      sc->info->memmap[ASPEED_ETH1]);
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100), 0,
>                         aspeed_soc_get_irq(s, ASPEED_ETH1));
> +
> +    /* GPIO */
> +    object_property_set_bool(OBJECT(&s->gpio), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio), 0, ASPEED_SOC_GPIO_BASE);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
> +            qdev_get_gpio_in(DEVICE(&s->vic), 20));
>  }
>  
>  static void aspeed_soc_class_init(ObjectClass *oc, void *data)
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 88b901d5df..28ff2bedb4 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -20,6 +20,7 @@
>  #include "hw/ssi/aspeed_smc.h"
>  #include "hw/watchdog/wdt_aspeed.h"
>  #include "hw/net/ftgmac100.h"
> +#include "hw/gpio/aspeed_gpio.h"
>  
>  #define ASPEED_SPIS_NUM  2
>  #define ASPEED_WDTS_NUM  3
> @@ -40,6 +41,7 @@ typedef struct AspeedSoCState {
>      AspeedSDMCState sdmc;
>      AspeedWDTState wdt[ASPEED_WDTS_NUM];
>      FTGMAC100State ftgmac100;
> +    AspeedGPIOState gpio;
>  } AspeedSoCState;
>  
>  #define TYPE_ASPEED_SOC "aspeed-soc"
> @@ -53,6 +55,7 @@ typedef struct AspeedSoCInfo {
>      int spis_num;
>      const char *fmc_typename;
>      const char **spi_typename;
> +    const char *gpio_typename;
>      int wdts_num;
>      const int *irqmap;
>      const hwaddr *memmap;
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] Add Aspeed GPIO controller model
  2019-06-18  8:51 [Qemu-devel] [PATCH 0/2] Add Aspeed GPIO controller model Rashmica Gupta
  2019-06-18  8:51 ` [Qemu-devel] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model Rashmica Gupta
  2019-06-18  8:51 ` [Qemu-devel] [PATCH 2/2] aspeed: add a GPIO controller to the SoC Rashmica Gupta
@ 2019-06-18  9:24 ` Cédric Le Goater
  2 siblings, 0 replies; 11+ messages in thread
From: Cédric Le Goater @ 2019-06-18  9:24 UTC (permalink / raw)
  To: Rashmica Gupta, qemu-arm; +Cc: andrew, qemu-devel, joel

On 18/06/2019 10:51, Rashmica Gupta wrote:
> Hi,
> 
> These two patches add a bunch of the functionality of the ast2400 and
> ast2500 gpio controllers. 
> 
> This is based off upstream/master with two of Cédric's patches:
> - aspeed: add a per SoC mapping for the interrupt space
> - aspeed: add a per SoC mapping for the memory space
> 
> 
> Any feedback would be great!


When you resend, could you please use the comment in aspeed_gpio.c 
as a commit log ? and add Peter in cc:

Thanks,

C. 


> Rashmica Gupta (2):
>   hw/gpio: Add basic Aspeed GPIO model
>   aspeed: add a GPIO controller to the SoC
> 
>  hw/arm/aspeed_soc.c           |  17 +
>  hw/gpio/Makefile.objs         |   1 +
>  hw/gpio/aspeed_gpio.c         | 869 ++++++++++++++++++++++++++++++++++
>  include/hw/arm/aspeed_soc.h   |   3 +
>  include/hw/gpio/aspeed_gpio.h |  76 +++
>  5 files changed, 966 insertions(+)
>  create mode 100644 hw/gpio/aspeed_gpio.c
>  create mode 100644 include/hw/gpio/aspeed_gpio.h
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] aspeed: add a GPIO controller to the SoC
  2019-06-18  9:21   ` Cédric Le Goater
@ 2019-06-19  0:19     ` Rashmica Gupta
  0 siblings, 0 replies; 11+ messages in thread
From: Rashmica Gupta @ 2019-06-19  0:19 UTC (permalink / raw)
  To: Cédric Le Goater, qemu-arm; +Cc: andrew, qemu-devel, joel

On Tue, 2019-06-18 at 11:21 +0200, Cédric Le Goater wrote:
> On 18/06/2019 10:51, Rashmica Gupta wrote:
> > Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
> > ---
> >  hw/arm/aspeed_soc.c         | 17 +++++++++++++++++
> >  include/hw/arm/aspeed_soc.h |  3 +++
> >  2 files changed, 20 insertions(+)
> > 
> > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> > index 1cc98b9f40..8583869acf 100644
> > --- a/hw/arm/aspeed_soc.c
> > +++ b/hw/arm/aspeed_soc.c
> > @@ -23,6 +23,7 @@
> >  #include "net/net.h"
> >  
> >  #define ASPEED_SOC_IOMEM_SIZE       0x00200000
> > +#define ASPEED_SOC_GPIO_BASE        0x1E780000
> > 
> 
> You should put this value in the memmap array.
>

Oops, good spot!

> C. 
> 
> 
> >  static const hwaddr aspeed_soc_ast2400_memmap[] = {
> >      [ASPEED_IOMEM]  = 0x1E600000,
> > @@ -120,6 +121,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
> >          .spis_num     = 1,
> >          .fmc_typename = "aspeed.smc.fmc",
> >          .spi_typename = aspeed_soc_ast2400_typenames,
> > +        .gpio_typename = "aspeed.gpio-ast2400",
> >          .wdts_num     = 2,
> >          .irqmap       = aspeed_soc_ast2400_irqmap,
> >          .memmap       = aspeed_soc_ast2400_memmap,
> > @@ -131,6 +133,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
> >          .spis_num     = 1,
> >          .fmc_typename = "aspeed.smc.fmc",
> >          .spi_typename = aspeed_soc_ast2400_typenames,
> > +        .gpio_typename = "aspeed.gpio-ast2400",
> >          .wdts_num     = 2,
> >          .irqmap       = aspeed_soc_ast2400_irqmap,
> >          .memmap       = aspeed_soc_ast2400_memmap,
> > @@ -142,6 +145,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
> >          .spis_num     = 1,
> >          .fmc_typename = "aspeed.smc.fmc",
> >          .spi_typename = aspeed_soc_ast2400_typenames,
> > +        .gpio_typename = "aspeed.gpio-ast2400",
> >          .wdts_num     = 2,
> >          .irqmap       = aspeed_soc_ast2400_irqmap,
> >          .memmap       = aspeed_soc_ast2400_memmap,
> > @@ -153,6 +157,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
> >          .spis_num     = 2,
> >          .fmc_typename = "aspeed.smc.ast2500-fmc",
> >          .spi_typename = aspeed_soc_ast2500_typenames,
> > +        .gpio_typename = "aspeed.gpio-ast2500",
> >          .wdts_num     = 3,
> >          .irqmap       = aspeed_soc_ast2500_irqmap,
> >          .memmap       = aspeed_soc_ast2500_memmap,
> > @@ -225,6 +230,8 @@ static void aspeed_soc_init(Object *obj)
> >  
> >      sysbus_init_child_obj(obj, "ftgmac100", OBJECT(&s->ftgmac100),
> >                            sizeof(s->ftgmac100), TYPE_FTGMAC100);
> > +    sysbus_init_child_obj(obj, "gpio", OBJECT(&s->gpio), sizeof(s-
> > >gpio),
> > +                          sc->info->gpio_typename);
> >  }
> >  
> >  static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> > @@ -366,6 +373,16 @@ static void aspeed_soc_realize(DeviceState
> > *dev, Error **errp)
> >                      sc->info->memmap[ASPEED_ETH1]);
> >      sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100), 0,
> >                         aspeed_soc_get_irq(s, ASPEED_ETH1));
> > +
> > +    /* GPIO */
> > +    object_property_set_bool(OBJECT(&s->gpio), true, "realized",
> > &err);
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +        return;
> > +    }
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio), 0,
> > ASPEED_SOC_GPIO_BASE);
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
> > +            qdev_get_gpio_in(DEVICE(&s->vic), 20));
> >  }
> >  
> >  static void aspeed_soc_class_init(ObjectClass *oc, void *data)
> > diff --git a/include/hw/arm/aspeed_soc.h
> > b/include/hw/arm/aspeed_soc.h
> > index 88b901d5df..28ff2bedb4 100644
> > --- a/include/hw/arm/aspeed_soc.h
> > +++ b/include/hw/arm/aspeed_soc.h
> > @@ -20,6 +20,7 @@
> >  #include "hw/ssi/aspeed_smc.h"
> >  #include "hw/watchdog/wdt_aspeed.h"
> >  #include "hw/net/ftgmac100.h"
> > +#include "hw/gpio/aspeed_gpio.h"
> >  
> >  #define ASPEED_SPIS_NUM  2
> >  #define ASPEED_WDTS_NUM  3
> > @@ -40,6 +41,7 @@ typedef struct AspeedSoCState {
> >      AspeedSDMCState sdmc;
> >      AspeedWDTState wdt[ASPEED_WDTS_NUM];
> >      FTGMAC100State ftgmac100;
> > +    AspeedGPIOState gpio;
> >  } AspeedSoCState;
> >  
> >  #define TYPE_ASPEED_SOC "aspeed-soc"
> > @@ -53,6 +55,7 @@ typedef struct AspeedSoCInfo {
> >      int spis_num;
> >      const char *fmc_typename;
> >      const char **spi_typename;
> > +    const char *gpio_typename;
> >      int wdts_num;
> >      const int *irqmap;
> >      const hwaddr *memmap;
> > 
> 
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model
  2019-06-18  8:51 ` [Qemu-devel] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model Rashmica Gupta
  2019-06-18  9:04   ` Cédric Le Goater
@ 2019-07-03  4:16   ` Andrew Jeffery
  2019-07-15  2:36     ` Rashmica Gupta
  1 sibling, 1 reply; 11+ messages in thread
From: Andrew Jeffery @ 2019-07-03  4:16 UTC (permalink / raw)
  To: Rashmica Gupta, qemu-arm; +Cc: Cédric Le Goater, qemu-devel, Joel Stanley



On Tue, 18 Jun 2019, at 18:52, Rashmica Gupta wrote:
> Add in details for GPIO controller for AST 2400 and 2500
> 
> Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
> ---
>  hw/gpio/Makefile.objs         |   1 +
>  hw/gpio/aspeed_gpio.c         | 869 ++++++++++++++++++++++++++++++++++
>  include/hw/gpio/aspeed_gpio.h |  76 +++
>  3 files changed, 946 insertions(+)
>  create mode 100644 hw/gpio/aspeed_gpio.c
>  create mode 100644 include/hw/gpio/aspeed_gpio.h
> 
> diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
> index e5da0cb54f..d305b3b24b 100644
> --- a/hw/gpio/Makefile.objs
> +++ b/hw/gpio/Makefile.objs
> @@ -9,3 +9,4 @@ obj-$(CONFIG_OMAP) += omap_gpio.o
>  obj-$(CONFIG_IMX) += imx_gpio.o
>  obj-$(CONFIG_RASPI) += bcm2835_gpio.o
>  obj-$(CONFIG_NRF51_SOC) += nrf51_gpio.o
> +obj-$(CONFIG_ASPEED_SOC) += aspeed_gpio.o
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> new file mode 100644
> index 0000000000..d84dd69255
> --- /dev/null
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -0,0 +1,869 @@
> +/*
> + *  ASPEED GPIO Controller
> + *
> + *  Copyright (C) 2017-2019 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + *

I'd use a SPDX license marker here instead.

> + *
> + * This models the ast2400 and ast2500 GPIO controllers.
> + *
> + * GPIO pins are arranged in groups of 8 pins labeled 
> A,B,..,Y,Z,AA,AB,AC.
> + * (Note that the ast2400 controller only goes up to group AB).
> + * A set has four groups (except set AC which only has one) and is
> + * referred to by the groups it is composed of (eg 
> ABCD,EFGH,...,YZAAAB).
> + * Each set is accessed and controlled by a bank of 14 registers.
> + *
> + * These registers operate on a per pin level where each bit in the 
> register
> + * corresponds to a pin, except for the command source registers. The 
> command
> + * source registers operate on a per group level where bits 24, 16, 8 
> and 0
> + * correspond to each group in the set.
> + *
> + *  eg. registers for set ABCD:
> + *  |D7...D0|C7...C0|B7...B0|A7...A0| <- GPIOs
> + *  |31...24|23...16|15....8|7.....0| <- bit position
> + *
> + * Note that there are a couple of groups that only have 4 pins.
> +
> + * There are three ways that this model deviates from the behaviour of 
> the
> + * actual controller:
> + * (1) There are three debounce registers which aren't modeled and so 
> the per
> + * set debounce setting registers don't affect anything.
> + *
> + * (2) The only control source driving the GPIO pins in the model is 
> the ARM
> + * model (as there currently aren't models for the LPC or Coprocessor 
> [TODO]).
> + *
> + * (3) None of the registers in the model are reset tolerant. [TODO]
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/gpio/aspeed_gpio.h"
> +#include "include/hw/misc/aspeed_scu.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +
> +#define ASPEED_GPIOS_PER_REG 32
> +
> +/* GPIO Source Types */
> +#define ASPEED_CMD_SRC_MASK         0x01010101
> +#define ASPEED_SOURCE_ARM           0
> +#define ASPEED_SOURCE_LPC           1
> +#define ASPEED_SOURCE_COPROCESSOR   2
> +#define ASPEED_SOURCE_RESERVED      3
> +
> +/* GPIO Interrupt Triggers */
> +#define ASPEED_FALLING_EDGE 0
> +#define ASPEED_RISING_EDGE  1
> +#define ASPEED_LEVEL_LOW    2
> +#define ASPEED_LEVEL_HIGH   3
> +#define ASPEED_DUAL_EDGE    4
> +
> +/* GPIO Register Address Offsets */
> +#define GPIO_ABCD_DATA_VALUE     0x000
> +#define GPIO_ABCD_DIRECTION      0x004
> +#define GPIO_ABCD_INT_ENABLE     0x008
> +#define GPIO_ABCD_INT_SENS_0     0x00C
> +#define GPIO_ABCD_INT_SENS_1     0x010
> +#define GPIO_ABCD_INT_SENS_2     0x014
> +#define GPIO_ABCD_INT_STATUS     0x018
> +#define GPIO_ABCD_RESET_TOLERANT 0x01C
> +#define GPIO_EFGH_DATA_VALUE     0x020
> +#define GPIO_EFGH_DIRECTION      0x024
> +#define GPIO_EFGH_INT_ENABLE     0x028
> +#define GPIO_EFGH_INT_SENS_0     0x02C
> +#define GPIO_EFGH_INT_SENS_1     0x030
> +#define GPIO_EFGH_INT_SENS_2     0x034
> +#define GPIO_EFGH_INT_STATUS     0x038
> +#define GPIO_EFGH_RESET_TOL      0x03C
> +#define GPIO_ABCD_DEBOUNCE_1     0x040
> +#define GPIO_ABCD_DEBOUNCE_2     0x044
> +#define GPIO_EFGH_DEBOUNCE_1     0x048
> +#define GPIO_EFGH_DEBOUNCE_2     0x04C
> +#define GPIO_DEBOUNCE_TIME_1     0x050
> +#define GPIO_DEBOUNCE_TIME_2     0x054
> +#define GPIO_DEBOUNCE_TIME_3     0x058
> +#define GPIO_ABCD_COMMAND_SRC_0  0x060
> +#define GPIO_ABCD_COMMAND_SRC_1  0x064
> +#define GPIO_EFGH_COMMAND_SRC_0  0x068
> +#define GPIO_EFGH_COMMAND_SRC_1  0x06C
> +#define GPIO_IJKL_DATA_VALUE     0x070
> +#define GPIO_IJKL_DIRECTION      0x074
> +#define GPIO_MNOP_DATA_VALUE     0x078
> +#define GPIO_MNOP_DIRECTION      0x07C
> +#define GPIO_QRST_DATA_VALUE     0x080
> +#define GPIO_QRST_DIRECTION      0x084
> +#define GPIO_UVWX_DATA_VALUE     0x088
> +#define GPIO_UWVX_DIRECTION      0x08C
> +#define GPIO_IJKL_COMMAND_SRC_0  0x090
> +#define GPIO_IJKL_COMMAND_SRC_1  0x094
> +#define GPIO_IJKL_INT_ENABLE     0x098
> +#define GPIO_IJKL_INT_SENS_0     0x09C
> +#define GPIO_IJKL_INT_SENS_1     0x0A0
> +#define GPIO_IJKL_INT_SENS_2     0x0A4
> +#define GPIO_IJKL_INT_STATUS     0x0A8
> +#define GPIO_IJKL_RESET_TOLERANT 0x0AC
> +#define GPIO_IJKL_DEBOUNCE_1     0x0B0
> +#define GPIO_IJKL_DEBOUNCE_2     0x0B4
> +#define GPIO_IJKL_INPUT_MASK     0x0B8
> +#define GPIO_ABCD_DATA_READ      0x0C0
> +#define GPIO_EFGH_DATA_READ      0x0C4
> +#define GPIO_IJKL_DATA_READ      0x0C8
> +#define GPIO_MNOP_DATA_READ      0x0CC
> +#define GPIO_QRST_DATA_READ      0x0D0
> +#define GPIO_UVWX_DATA_READ      0x0D4
> +#define GPIO_YZAAAB_DATA_READ    0x0D8
> +#define GPIO_AC_DATA_READ        0x0DC
> +#define GPIO_MNOP_COMMAND_SRC_0  0x0E0
> +#define GPIO_MNOP_COMMAND_SRC_1  0x0E4
> +#define GPIO_MNOP_INT_ENABLE     0x0E8
> +#define GPIO_MNOP_INT_SENS_0     0x0EC
> +#define GPIO_MNOP_INT_SENS_1     0x0F0
> +#define GPIO_MNOP_INT_SENS_2     0x0F4
> +#define GPIO_MNOP_INT_STATUS     0x0F8
> +#define GPIO_MNOP_RESET_TOLERANT 0x0FC
> +#define GPIO_MNOP_DEBOUNCE_1     0x100
> +#define GPIO_MNOP_DEBOUNCE_2     0x104
> +#define GPIO_MNOP_INPUT_MASK     0x108
> +#define GPIO_QRST_COMMAND_SRC_0  0x110
> +#define GPIO_QRST_COMMAND_SRC_1  0x114
> +#define GPIO_QRST_INT_ENABLE     0x118
> +#define GPIO_QRST_INT_SENS_0     0x11C
> +#define GPIO_QRST_INT_SENS_1     0x120
> +#define GPIO_QRST_INT_SENS_2     0x124
> +#define GPIO_QRST_INT_STATUS     0x128
> +#define GPIO_QRST_RESET_TOLERANT 0x12C
> +#define GPIO_QRST_DEBOUNCE_1     0x130
> +#define GPIO_QRST_DEBOUNCE_2     0x134
> +#define GPIO_QRST_INPUT_MASK     0x138
> +#define GPIO_UVWX_COMMAND_SRC_0  0x140
> +#define GPIO_UVWX_COMMAND_SRC_1  0x144
> +#define GPIO_UVWX_INT_ENABLE     0x148
> +#define GPIO_UVWX_INT_SENS_0     0x14C
> +#define GPIO_UVWX_INT_SENS_1     0x150
> +#define GPIO_UVWX_INT_SENS_2     0x154
> +#define GPIO_UVWX_INT_STATUS     0x158
> +#define GPIO_UVWX_RESET_TOLERANT 0x15C
> +#define GPIO_UVWX_DEBOUNCE_1     0x160
> +#define GPIO_UVWX_DEBOUNCE_2     0x164
> +#define GPIO_UVWX_INPUT_MASK     0x168
> +#define GPIO_YZAAAB_COMMAND_SRC_0 0x170
> +#define GPIO_YZAAAB_COMMAND_SRC_1 0x174
> +#define GPIO_YZAAAB_INT_ENABLE   0x178
> +#define GPIO_YZAAAB_INT_SENS_0   0x17C
> +#define GPIO_YZAAAB_INT_SENS_1   0x180
> +#define GPIO_YZAAAB_INT_SENS_2   0x184
> +#define GPIO_YZAAAB_INT_STATUS   0x188
> +#define GPIO_YZAAAB_RESET_TOLERANT 0x18C
> +#define GPIO_YZAAAB_DEBOUNCE_1   0x190
> +#define GPIO_YZAAAB_DEBOUNCE_2   0x194
> +#define GPIO_YZAAAB_INPUT_MASK   0x198
> +#define GPIO_AC_COMMAND_SRC_0    0x1A0
> +#define GPIO_AC_COMMAND_SRC_1    0x1A4
> +#define GPIO_AC_INT_ENABLE       0x1A8
> +#define GPIO_AC_INT_SENS_0       0x1AC
> +#define GPIO_AC_INT_SENS_1       0x1B0
> +#define GPIO_AC_INT_SENS_2       0x1B4
> +#define GPIO_AC_INT_STATUS       0x1B8
> +#define GPIO_AC_RESET_TOLERANT   0x1BC
> +#define GPIO_AC_DEBOUNCE_1       0x1C0
> +#define GPIO_AC_DEBOUNCE_2       0x1C4
> +#define GPIO_AC_INPUT_MASK       0x1C8
> +#define GPIO_ABCD_INPUT_MASK     0x1D0
> +#define GPIO_EFGH_INPUT_MASK     0x1D4
> +#define GPIO_YZAAAB_DATA_VALUE   0x1E0
> +#define GPIO_YZAAAB_DIRECTION    0x1E4
> +#define GPIO_AC_DATA_VALUE       0x1E8
> +#define GPIO_AC_DIRECTION        0x1EC
> +
> +struct AspeedGPIO {
> +    uint16_t set_idx;
> +    uint32_t (*get)(GPIORegs *regs);
> +    void (*set)(AspeedGPIOState *s, GPIORegs *regs,
> +                GPIOSetProperties *props, uint32_t val);
> +};
> +
> +static int aspeed_evaluate_irq(GPIORegs *regs, int prev_value_high, 
> int bit)
> +{
> +    uint32_t falling_edge = 0, rising_edge = 0;
> +    uint32_t int_trigger = extract32(regs->int_sens_0, bit, 1)
> +                           | extract32(regs->int_sens_1, bit, 1) << 1
> +                           | extract32(regs->int_sens_2, bit, 1) << 2 ;
> +    uint32_t curr_pin_high = extract32(regs->data_value, bit, 1);
> +
> +    /* Detect edges */
> +    if (curr_pin_high && !prev_value_high) {
> +            rising_edge = 1;
> +    } else if (!curr_pin_high && prev_value_high) {
> +            falling_edge = 1;
> +    }
> +
> +    if (((int_trigger == ASPEED_FALLING_EDGE)  && falling_edge)  ||
> +        ((int_trigger == ASPEED_RISING_EDGE)  && rising_edge)  ||
> +        ((int_trigger == ASPEED_LEVEL_LOW)  && !curr_pin_high)  ||
> +        ((int_trigger == ASPEED_LEVEL_HIGH)  && curr_pin_high)  ||
> +        ((int_trigger >= ASPEED_DUAL_EDGE)  && (rising_edge || 
> falling_edge)))
> +    {
> +        regs->int_status = deposit32(regs->int_status, bit, 1, 1);
> +        return 1;
> +    }
> +    return 0;
> +}
> +
> +static void aspeed_gpio_update(AspeedGPIOState *s, GPIORegs *regs)
> +{
> +    uint32_t input_mask = regs->input_mask;
> +    uint32_t direction = regs->direction;
> +    uint32_t old = regs->data_value;
> +    uint32_t new = regs->data_read;
> +    uint32_t diff;
> +    int bit;
> +
> +    diff = old ^ new;
> +    if (!diff) {
> +        return;
> +    }
> +
> +    if (!direction) {
> +        return;
> +    }
> +
> +    for (bit = 0; bit < ASPEED_GPIOS_PER_REG; bit++) {
> +        uint32_t mask = 1 << bit;
> +        /* If the bit needs to be updated... */
> +        if (!(diff & mask)) {
> +            continue;
> +        }
> +        /* ...and corresponds to an output pin...*/
> +        if (!(direction & mask)) {
> +            continue;
> +        }
> +        /* ...and isn't masked...  */
> +        if (input_mask & mask) {
> +            continue;
> +        }
> +        /* ... then update it*/
> +        if (mask & new) {
> +            regs->data_value |= mask;
> +        } else {
> +            regs->data_value &= ~mask;
> +        }
> +
> +        if (aspeed_evaluate_irq(regs, old & mask, bit)) {
> +            qemu_set_irq(s->output[bit], 1);
> +        }
> +    }
> +}
> +
> +static uint32_t aspeed_adjust_pin(AspeedGPIOState *s, uint32_t pin)
> +{
> +    if (pin >= s->ctrl->gap) {
> +        pin += 4;
> +    }
> +
> +    return pin;
> +}
> +
> +static uint32_t aspeed_get_set_idx_from_pin(AspeedGPIOState *s, 
> uint32_t pin)
> +{
> +    /*
> +     * For most pins, dividing by 32 gets the set index.
> +     *
> +     * However the 2500 has a 4 pin gap in group AB and the 2400 has a 
> 4 pin
> +     * gap in group Y (and only four pins in AB but this is the last 
> group so
> +     * it doesn't matter).
> +     */
> +    return aspeed_adjust_pin(s, pin) >> 5;
> +}
> +
> +static bool aspeed_gpio_get_pin_level(AspeedGPIOState *s, uint32_t pin)
> +{
> +    uint32_t reg_val;
> +    uint32_t mask = 1 << (pin);
> +    uint32_t set_idx = aspeed_get_set_idx_from_pin(s, pin);
> +
> +    reg_val = s->sets[set_idx].data_value;
> +
> +    return !!(reg_val & mask);
> +}
> +
> +static void aspeed_gpio_set_pin_level(AspeedGPIOState *s, uint32_t pin,
> +                                      bool level)
> +{
> +    uint32_t mask  = 1 << (pin);
> +    uint32_t set_idx = aspeed_get_set_idx_from_pin(s, pin);
> +
> +    if (level) {
> +        s->sets[set_idx].data_read |= mask;
> +    } else {
> +        s->sets[set_idx].data_read &= !mask;
> +    }
> +
> +    aspeed_gpio_update(s, &s->sets[set_idx]);
> +}
> +
> +/*
> + *  | src_1 | src_2 |  source     |
> + *  |-----------------------------|
> + *  |   0   |   0   |  ARM        |
> + *  |   0   |   1   |  LPC        |
> + *  |   1   |   0   |  Coprocessor|
> + *  |   1   |   1   |  Reserved   |
> + *
> + *  Once the source of a set is programmed, corresponding bits in the
> + *  data_value, direction, interrupt [enable, sens[0-2]], reset_tol and
> + *  debounce registers can only be written by the source.
> + *
> + *  Source is ARM by default
> + *  only bits 24, 16, 8, and 0 can be set
> + *
> + *  we don't currently have a model for the LPC or Coprocessor
> + */
> +static uint32_t update_value_control_source(GPIORegs *regs, uint32_t 
> old_value,
> +                                            uint32_t value)
> +{
> +    int i;
> +    int cmd_source;
> +
> +    /* assume the source is always ARM for now */
> +    int source = ASPEED_SOURCE_ARM;
> +
> +    uint32_t new_value = 0;
> +
> +    /* for each group in set */
> +    for (i = 0; i < ASPEED_GPIOS_PER_REG; i += 8) {
> +        cmd_source = extract32(regs->cmd_source_0, i, 1)
> +                | (extract32(regs->cmd_source_1, i, 1) << 1);
> +
> +        if (source == cmd_source) {
> +            new_value |= (0xff << i) & value;
> +        } else {
> +            new_value |= (0xff << i) & old_value;
> +        }
> +    }
> +    return new_value;
> +}
> +
> +/* Reader helper functions */
> +static uint32_t read_direction(GPIORegs *regs)
> +{
> +    return regs->direction;
> +}
> +
> +static uint32_t read_data_value(GPIORegs *regs)
> +{
> +    return regs->data_value;
> +}
> +
> +static uint32_t read_int_enable(GPIORegs *regs)
> +{
> +    return regs->int_enable;
> +}
> +
> +static uint32_t read_int_sens_0(GPIORegs *regs)
> +{
> +    return regs->int_sens_0;
> +}
> +
> +static uint32_t read_int_sens_1(GPIORegs *regs)
> +{
> +    return regs->int_sens_1;
> +}
> +
> +static uint32_t read_int_sens_2(GPIORegs *regs)
> +{
> +    return regs->int_sens_2;
> +}
> +
> +static uint32_t read_int_status(GPIORegs *regs)
> +{
> +    return regs->int_status;
> +}
> +
> +static uint32_t read_reset_tol(GPIORegs *regs)
> +{
> +    return regs->reset_tol;
> +}
> +
> +static uint32_t read_debounce_1(GPIORegs *regs)
> +{
> +    return regs->debounce_1;
> +}
> +
> +static uint32_t read_debounce_2(GPIORegs *regs)
> +{
> +    return regs->debounce_2;
> +}
> +
> +static uint32_t read_cmd_source_0(GPIORegs *regs)
> +{
> +    return regs->cmd_source_0;
> +}
> +
> +static uint32_t read_cmd_source_1(GPIORegs *regs)
> +{
> +    return regs->cmd_source_1;
> +}
> +
> +static uint32_t read_data(GPIORegs *regs)
> +{
> +    return regs->data_read;
> +}
> +
> +static uint32_t read_input_mask(GPIORegs *regs)
> +{
> +    return regs->input_mask;
> +}
> +
> +/* Write helper functions */
> +
> +static void _write_data_value(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)

Lets remove the underscore prefix from the write helpers. It's not
present on the read helpers.

> +{
> +    val &= props->output | ~props->input;

Can you add some comments about why the value is being masked
in this way? It's not clear to me that it's what we want.

> +    regs->data_read = update_value_control_source(regs, 
> regs->data_read, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_direction(AspeedGPIOState *s, GPIORegs *regs,
> +                             GPIOSetProperties *props, uint32_t val)
> +{
> +    val &= props->output | ~props->input;
> +    regs->direction = update_value_control_source(regs, 
> regs->direction, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_int_enable(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->int_enable = update_value_control_source(regs, 
> regs->int_enable, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_int_sens_0(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->int_sens_0 = update_value_control_source(regs, 
> regs->int_sens_0, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_int_sens_1(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->int_sens_1 = update_value_control_source(regs, 
> regs->int_sens_1, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_int_sens_2(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->int_sens_2 = update_value_control_source(regs, 
> regs->int_sens_2, val);
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_int_status(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->int_status = val;
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static void _write_reset_tol(AspeedGPIOState *s, GPIORegs *regs,
> +                             GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->reset_tol = update_value_control_source(regs, 
> regs->reset_tol, val);
> +}
> +
> +static void _write_debounce_1(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->debounce_1 = update_value_control_source(regs, 
> regs->debounce_1, val);
> +}
> +
> +static void _write_debounce_2(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->debounce_2 = update_value_control_source(regs, 
> regs->debounce_2, val);
> +}
> +
> +static void _write_cmd_source_0(AspeedGPIOState *s, GPIORegs *regs,
> +                                GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->cmd_source_0 = val & ASPEED_CMD_SRC_MASK;
> +}
> +
> +static void _write_cmd_source_1(AspeedGPIOState *s, GPIORegs *regs,
> +                                GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->cmd_source_1 = val & ASPEED_CMD_SRC_MASK;
> +}
> +
> +/*
> + * feeds into interrupt generation
> + * 0: read from data value reg will be updated
> + * 1: read from data value reg will not be updated
> + */
> +static void _write_input_mask(AspeedGPIOState *s, GPIORegs *regs,
> +                              GPIOSetProperties *props, uint32_t val)
> +{
> +    regs->input_mask = val & props->input;
> +    aspeed_gpio_update(s, regs);
> +}
> +
> +static const struct AspeedGPIO gpios[0x1f0] = {
> +    /* Set ABCD */
> +    [GPIO_ABCD_DATA_VALUE] = {0, read_data_value, _write_data_value},

Maybe there would be less tedium (and risk of minor bugs) here if we add
a table for looking up the read/write callbacks from a type, and just stash
the type in struct AspeedGPIO (or a pointer to the corresponding type ops),
e.g:

enum gpio_property { gpio_direction = 0, gpio_int_enable, ... };

struct aspeed_gpio_reg_ops {
    int (*read)(...);
    int (*write)(...);
};

struct aspeed_gpio_reg_ops gpio_reg_ops[] = {
    [gpio_direction]  = { .read = read_direction, .write = write_direction };
    ...
};

Then we have:

static const struct AspeedGPIO gpios[...] = {
     [GPIO_ABCD_DIRECTION] = { .set = 0, .type = gpio_direction },
     ...
};

Not wedded to the idea, just thinking out loud. What do you think?

> +    [GPIO_ABCD_DIRECTION] = {0, read_direction, _write_direction},
> +    [GPIO_ABCD_INT_ENABLE] = {0, read_int_enable, _write_int_enable},
> +    [GPIO_ABCD_INT_SENS_0] = {0, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_ABCD_INT_SENS_1] = {0, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_ABCD_INT_SENS_2] = {0, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_ABCD_INT_STATUS] = {0, read_int_status, _write_int_status},
> +    [GPIO_ABCD_RESET_TOLERANT] = {0, read_reset_tol, _write_reset_tol},
> +    [GPIO_ABCD_DEBOUNCE_1] = {0, read_debounce_1, _write_debounce_1},
> +    [GPIO_ABCD_DEBOUNCE_2] = {0, read_debounce_2, _write_debounce_2},
> +    [GPIO_ABCD_COMMAND_SRC_0] = {0, read_cmd_source_0, 
> _write_cmd_source_0},
> +    [GPIO_ABCD_COMMAND_SRC_1] = {0, read_cmd_source_1, 
> _write_cmd_source_1},
> +    [GPIO_ABCD_DATA_READ] = {0, read_data, NULL},
> +    [GPIO_ABCD_INPUT_MASK] = {0, read_input_mask, _write_input_mask},
> +    /* Set EFGH */
> +    [GPIO_EFGH_DATA_VALUE] = {1, read_data_value, _write_data_value},
> +    [GPIO_EFGH_DIRECTION] = {1, read_direction, _write_direction },
> +    [GPIO_EFGH_INT_ENABLE] = {1, read_int_enable, _write_int_enable},
> +    [GPIO_EFGH_INT_SENS_0] = {1, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_EFGH_INT_SENS_1] = {1, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_EFGH_INT_SENS_2] = {1, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_EFGH_INT_STATUS] = {1, read_int_status, _write_int_status},
> +    [GPIO_EFGH_RESET_TOL] = {1, read_reset_tol,   _write_reset_tol},
> +    [GPIO_EFGH_DEBOUNCE_1] = {1, read_debounce_1,  _write_debounce_1},
> +    [GPIO_EFGH_DEBOUNCE_2] = {1, read_debounce_2,  _write_debounce_2},
> +    [GPIO_EFGH_COMMAND_SRC_0] = {1, read_cmd_source_0,  
> _write_cmd_source_0},
> +    [GPIO_EFGH_COMMAND_SRC_1] = {1, read_cmd_source_1,  
> _write_cmd_source_1},
> +    [GPIO_EFGH_DATA_READ] = {1, read_data, NULL},
> +    [GPIO_EFGH_INPUT_MASK] = {1, read_input_mask,  _write_input_mask},
> +    /* Set IJKL */
> +    [GPIO_IJKL_DATA_VALUE] = {2, read_data_value, _write_data_value},
> +    [GPIO_IJKL_DIRECTION] = {2, read_direction, _write_direction},
> +    [GPIO_IJKL_INT_ENABLE] = {2, read_int_enable, _write_int_enable},
> +    [GPIO_IJKL_INT_SENS_0] = {2, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_IJKL_INT_SENS_1] = {2, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_IJKL_INT_SENS_2] = {2, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_IJKL_INT_STATUS] = {2, read_int_status, _write_int_status},
> +    [GPIO_IJKL_RESET_TOLERANT] = {2, read_reset_tol, _write_reset_tol},
> +    [GPIO_IJKL_DEBOUNCE_1] = {2, read_debounce_1, _write_debounce_1},
> +    [GPIO_IJKL_DEBOUNCE_2] = {2, read_debounce_2, _write_debounce_2},
> +    [GPIO_IJKL_COMMAND_SRC_0] = {2, read_cmd_source_0, 
> _write_cmd_source_0},
> +    [GPIO_IJKL_COMMAND_SRC_1] = {2, read_cmd_source_1, 
> _write_cmd_source_1},
> +    [GPIO_IJKL_DATA_READ] = {2, read_data, NULL},
> +    [GPIO_IJKL_INPUT_MASK] = {2, read_input_mask, _write_input_mask},
> +    /* Set MNOP */
> +    [GPIO_MNOP_DATA_VALUE] = {3, read_data_value, _write_data_value},
> +    [GPIO_MNOP_DIRECTION] = {3, read_direction, _write_direction},
> +    [GPIO_MNOP_INT_ENABLE] = {3, read_int_enable, _write_int_enable},
> +    [GPIO_MNOP_INT_SENS_0] = {3, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_MNOP_INT_SENS_1] = {3, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_MNOP_INT_SENS_2] = {3, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_MNOP_INT_STATUS] = {3, read_int_status, _write_int_status},
> +    [GPIO_MNOP_RESET_TOLERANT] = {3, read_reset_tol,  
> _write_reset_tol},
> +    [GPIO_MNOP_DEBOUNCE_1] = {3, read_debounce_1, _write_debounce_1},
> +    [GPIO_MNOP_DEBOUNCE_2] = {3, read_debounce_2, _write_debounce_2},
> +    [GPIO_MNOP_COMMAND_SRC_0] = {3, read_cmd_source_0, 
> _write_cmd_source_0},
> +    [GPIO_MNOP_COMMAND_SRC_1] = {3, read_cmd_source_1, 
> _write_cmd_source_1},
> +    [GPIO_MNOP_DATA_READ] = {3, read_data, NULL},
> +    [GPIO_MNOP_INPUT_MASK] = {3, read_input_mask, _write_input_mask},
> +    /* Set QRST */
> +    [GPIO_QRST_DATA_VALUE] = {4, read_data_value, _write_data_value},
> +    [GPIO_QRST_DIRECTION] = {4, read_direction, _write_direction},
> +    [GPIO_QRST_INT_ENABLE] = {4, read_int_enable, _write_int_enable},
> +    [GPIO_QRST_INT_SENS_0] = {4, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_QRST_INT_SENS_1] = {4, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_QRST_INT_SENS_2] = {4, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_QRST_INT_STATUS] = {4, read_int_status, _write_int_status},
> +    [GPIO_QRST_RESET_TOLERANT] = {4, read_reset_tol, _write_reset_tol},
> +    [GPIO_QRST_DEBOUNCE_1] = {4, read_debounce_1, _write_debounce_1},
> +    [GPIO_QRST_DEBOUNCE_2] = {4, read_debounce_2, _write_debounce_2},
> +    [GPIO_QRST_COMMAND_SRC_0] = {4, read_cmd_source_0, 
> _write_cmd_source_0},
> +    [GPIO_QRST_COMMAND_SRC_1] = {4, read_cmd_source_1, 
> _write_cmd_source_1},
> +    [GPIO_QRST_DATA_READ] = {4, read_data, NULL},
> +    [GPIO_QRST_INPUT_MASK] = {4, read_input_mask,  _write_input_mask},
> +    /* Set UVWX */
> +    [GPIO_UVWX_DATA_VALUE] = {5, read_data_value, _write_data_value},
> +    [GPIO_UWVX_DIRECTION] = {5, read_direction, _write_direction},
> +    [GPIO_UVWX_INT_ENABLE] = {5, read_int_enable, _write_int_enable},
> +    [GPIO_UVWX_INT_SENS_0] = {5, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_UVWX_INT_SENS_1] = {5, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_UVWX_INT_SENS_2] = {5, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_UVWX_INT_STATUS] = {5, read_int_status, _write_int_status},
> +    [GPIO_UVWX_RESET_TOLERANT] = {5, read_reset_tol, _write_reset_tol},
> +    [GPIO_UVWX_DEBOUNCE_1] = {5, read_debounce_1, _write_debounce_1},
> +    [GPIO_UVWX_DEBOUNCE_2] = {5, read_debounce_2, _write_debounce_2},
> +    [GPIO_UVWX_COMMAND_SRC_0] = {5, read_cmd_source_0, 
> _write_cmd_source_0},
> +    [GPIO_UVWX_COMMAND_SRC_1] = {5, read_cmd_source_1, 
> _write_cmd_source_1},
> +    [GPIO_UVWX_DATA_READ] = {5, read_data, NULL},
> +    [GPIO_UVWX_INPUT_MASK] = {5, read_input_mask, _write_input_mask},
> +    /* Set YZAAAB */
> +    [GPIO_YZAAAB_DATA_VALUE] = {6, read_data_value, _write_data_value},
> +    [GPIO_YZAAAB_DIRECTION] = {6, read_direction, _write_direction},
> +    [GPIO_YZAAAB_INT_ENABLE] = {6, read_int_enable, _write_int_enable},
> +    [GPIO_YZAAAB_INT_SENS_0] = {6, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_YZAAAB_INT_SENS_1] = {6, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_YZAAAB_INT_SENS_2] = {6, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_YZAAAB_INT_STATUS] = {6, read_int_status, _write_int_status},
> +    [GPIO_YZAAAB_RESET_TOLERANT] = {6, read_reset_tol, 
> _write_reset_tol},
> +    [GPIO_YZAAAB_DEBOUNCE_1] = {6, read_debounce_1, _write_debounce_1},
> +    [GPIO_YZAAAB_DEBOUNCE_2] = {6, read_debounce_2, _write_debounce_2},
> +    [GPIO_YZAAAB_COMMAND_SRC_0] = {6, read_cmd_source_0, 
> _write_cmd_source_0},
> +    [GPIO_YZAAAB_COMMAND_SRC_1] = {6, read_cmd_source_1, 
> _write_cmd_source_1},
> +    [GPIO_YZAAAB_DATA_READ] = {6, read_data, NULL},
> +    [GPIO_YZAAAB_INPUT_MASK] = {6, read_input_mask, _write_input_mask},
> +    /* Set AC */
> +    [GPIO_AC_DATA_VALUE] = {7, read_data_value, _write_data_value},
> +    [GPIO_AC_DIRECTION] = {7, read_direction, _write_direction},
> +    [GPIO_AC_INT_ENABLE] = {7, read_int_enable, _write_int_enable},
> +    [GPIO_AC_INT_SENS_0] = {7, read_int_sens_0, _write_int_sens_0},
> +    [GPIO_AC_INT_SENS_1] = {7, read_int_sens_1, _write_int_sens_1},
> +    [GPIO_AC_INT_SENS_2] = {7, read_int_sens_2, _write_int_sens_2},
> +    [GPIO_AC_INT_STATUS] = {7, read_int_status, _write_int_status},
> +    [GPIO_AC_RESET_TOLERANT] = {7, read_reset_tol, _write_reset_tol},
> +    [GPIO_AC_DEBOUNCE_1] = {7, read_debounce_1, _write_debounce_1},
> +    [GPIO_AC_DEBOUNCE_2] = {7, read_debounce_2, _write_debounce_2},
> +    [GPIO_AC_COMMAND_SRC_0] = {7, read_cmd_source_0, 
> _write_cmd_source_0},
> +    [GPIO_AC_COMMAND_SRC_1] = {7, read_cmd_source_1, 
> _write_cmd_source_1},
> +    [GPIO_AC_DATA_READ] = {7, read_data, NULL},
> +    [GPIO_AC_INPUT_MASK] = {7, read_input_mask,     _write_input_mask},
> +    /* Debounce registers */
> +    [GPIO_DEBOUNCE_TIME_1] = {-1, NULL, NULL},
> +};
> +
> +static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t 
> size)
> +{
> +    AspeedGPIOState *s = ASPEED_GPIO(opaque);
> +    uint32_t val = 0;
> +
> +    if (size != 4) {
> +        return 0;
> +    }
> +
> +    if (gpios[offset].get == NULL) {

With the AST2600 we might need to make gpios class-specific rather than
a static global to the compilation unit (to deal with the 1.8V GPIO registers
later in the address-space). Not sure though, haven't thought about it for
more than this paragraph.

> +        qemu_log_mask(LOG_GUEST_ERROR, "no getter for offset %lx", 
> offset);

Really this is a model error. We should only encounter it if we haven't
completely described the controller. So maybe it should be an assert
or some kind of non-guest warning?

> +        return 0;
> +    }
> +
> +    val = gpios[offset].get(&s->sets[gpios[offset].set_idx]);

The indexing here is starting to get a bit hard to follow. I think we could
improve readability by introducing some locals, and rewriting in terms
of those:

const struct AspeedGPIO *reg = &gpios[offset];
const struct GPIORegs *set = &s->sets[gpio.set_idx];

if (!reg.get) {
    ...
}

val = reg.get(set);

I think it makes the relationship of the structs a bit clearer too, and I think
it also suggests we could improve the names:

s/struct AspeedGPIO/struct AspeedGPIOReg/
s/struct GPIORegs/struct GPIOProperties/

GPIOProperties clashes a bit with GPIOSetProperties though, so maybe we
need to think of a better name for that too?

> +    return (uint64_t) val;
> +}
> +
> +static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t 
> data,
> +                              uint32_t size)
> +{
> +    AspeedGPIOState *s = ASPEED_GPIO(opaque);
> +    GPIOSetProperties *props = &s->ctrl->props[gpios[offset].set_idx];
> +
> +    if (gpios[offset].set == NULL) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "no setter for offset %lx", 
> offset);

Same issue here with guest vs model error.

> +        return;
> +    }
> +
> +    uint32_t mask = props->input | props->output;
> +
> +    gpios[offset].set(s, &s->sets[gpios[offset].set_idx],
> +                      props, data & mask);
> +}
> +
> +static void aspeed_gpio_get_pin(Object *obj, Visitor *v, const char 
> *name,
> +                                void *opaque, Error **errp)
> +{
> +    int pin = 0xfff;
> +    bool level = true;
> +    char group[3];
> +    AspeedGPIOState *s = ASPEED_GPIO(obj);
> +    if (sscanf(name, "gpio%2[A-Z]%1d", group, &pin) != 2) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "error reading %s", name);
> +        return;
> +    }
> +    level = aspeed_gpio_get_pin_level(s, pin);

We're not using the captured group to determine the pin, and as such the
code as it stands will use the first bank in all cases. The strings look like
"gpioAB3" right? That should be pin=bank_index(AB)*8+3=27*8+3=219,
but the code would pass through pin=3.

> +    visit_type_bool(v, name, &level, errp);
> +}
> +
> +static void aspeed_gpio_set_pin(Object *obj, Visitor *v, const char 
> *name,
> +                               void *opaque, Error **errp)
> +{
> +    Error *local_err = NULL;
> +    bool level;
> +    int pin = 0xfff;
> +    char group[3];
> +    AspeedGPIOState *s = ASPEED_GPIO(obj);
> +    visit_type_bool(v, name, &level, &local_err);
> +    if (sscanf(name, "gpio%2[A-Z]%1d", group, &pin) != 2) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "error reading %s", name);
> +        return;
> +    }
> +    aspeed_gpio_set_pin_level(s, pin, level);

As above.

> +}
> +
> +
> +/* Setup functions */
> +static const MemoryRegionOps aspeed_gpio_ops = {
> +    .read = aspeed_gpio_read,
> +    .write = aspeed_gpio_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +};
> +
> +static void aspeed_gpio_reset(DeviceState *dev)
> +{
> +    struct AspeedGPIOState *s = ASPEED_GPIO(dev);
> +
> +    /* TODO: respect the reset tolerance registers */
> +    memset(s->sets, 0, sizeof(s->sets));
> +}
> +
> +static void aspeed_gpio_realize(DeviceState *dev, Error **errp)
> +{
> +    AspeedGPIOState *s = ASPEED_GPIO(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +    sysbus_init_irq(sbd, &s->irq);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s,
> +            TYPE_ASPEED_GPIO, 0x1000);
> +
> +    sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static const VMStateDescription vmstate_gpio_regs = {
> +    .name = TYPE_ASPEED_GPIO"/regs",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(data_value,   GPIORegs),
> +        VMSTATE_UINT32(data_read,    GPIORegs),
> +        VMSTATE_UINT32(direction,    GPIORegs),
> +        VMSTATE_UINT32(int_enable,   GPIORegs),
> +        VMSTATE_UINT32(int_sens_0,   GPIORegs),
> +        VMSTATE_UINT32(int_sens_1,   GPIORegs),
> +        VMSTATE_UINT32(int_sens_2,   GPIORegs),
> +        VMSTATE_UINT32(int_status,   GPIORegs),
> +        VMSTATE_UINT32(reset_tol,    GPIORegs),
> +        VMSTATE_UINT32(cmd_source_0, GPIORegs),
> +        VMSTATE_UINT32(cmd_source_1, GPIORegs),
> +        VMSTATE_UINT32(debounce_1,   GPIORegs),
> +        VMSTATE_UINT32(debounce_2,   GPIORegs),
> +        VMSTATE_UINT32(input_mask,   GPIORegs),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};
> +static const VMStateDescription vmstate_aspeed_gpio = {
> +    .name = TYPE_ASPEED_GPIO,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT_ARRAY(sets, AspeedGPIOState, 
> ASPEED_GPIO_MAX_NR_SETS,
> +                             1, vmstate_gpio_regs, GPIORegs),
> +        VMSTATE_END_OF_LIST(),
> +   }
> +};
> +
> +static void aspeed_gpio_init(Object *obj)
> +{
> +    AspeedGPIOState *s = ASPEED_GPIO(obj);
> +    AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
> +    int pin;
> +
> +    s->ctrl = agc->ctrl;
> +
> +    for (pin = 0; pin < agc->ctrl->nr_gpio_pins; pin++) {
> +        char *name;
> +        int set_idx = aspeed_get_set_idx_from_pin(s, pin);
> +        int _pin = aspeed_adjust_pin(s, pin);
> +        int pin_idx = _pin - (set_idx * 32);
> +        int group_idx = pin_idx >> 3;
> +
> +        name = g_strdup_printf("gpio%s%d",
> +                               
> agc->ctrl->props[set_idx].set[group_idx],
> +                               pin_idx % 8);
> +        object_property_add(obj, name, "bool",
> +                            aspeed_gpio_get_pin,
> +                            aspeed_gpio_set_pin,
> +                            NULL, NULL, NULL);
> +    }
> +}
> +
> +static void aspeed_gpio_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    AspeedGPIOClass *agc = ASPEED_GPIO_CLASS(klass);
> +
> +    dc->realize = aspeed_gpio_realize;
> +    dc->reset = aspeed_gpio_reset;
> +    dc->desc = "Aspeed GPIO Controller";
> +    dc->vmsd = &vmstate_aspeed_gpio;
> +    agc->ctrl = data;
> +}
> +
> +static GPIOSetProperties ast2400_set_props[] = {
> +    [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
> +    [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
> +    [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
> +    [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
> +    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
> +    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> +    [6] = {0x0000000f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
> +};
> +
> +static GPIOSetProperties ast2500_set_props[] = {
> +    [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
> +    [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
> +    [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
> +    [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
> +    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
> +    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> +    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
> +    [7] = {0x000000ff,  0x000000ff,  {"AC"} },
> +};
> +
> +static AspeedGPIOController controllers[] = {
> +    {
> +        .name           = TYPE_ASPEED_GPIO "-ast2500",
> +        .props          = ast2500_set_props,
> +        .nr_gpio_pins   = 228,
> +        .nr_gpio_sets   = 8,
> +        .gap            = 220,
> +    }, {
> +        .name           = TYPE_ASPEED_GPIO "-ast2400",
> +        .props          = ast2400_set_props,
> +        .nr_gpio_pins   = 216,
> +        .nr_gpio_sets   = 7,
> +        .gap            = 196,
> +    }
> +};
> +
> +static const TypeInfo aspeed_gpio_info = {
> +    .name = TYPE_ASPEED_GPIO,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(AspeedGPIOState),
> +    .class_size     = sizeof(AspeedGPIOClass),
> +    .instance_init = aspeed_gpio_init,
> +    .abstract       = true,
> +};
> +
> +static void aspeed_gpio_register_types(void)
> +{
> +    int i;
> +
> +    type_register_static(&aspeed_gpio_info);
> +    for (i = 0; i < ARRAY_SIZE(controllers); i++) {
> +        TypeInfo t = {
> +            .name = controllers[i].name,
> +            .parent = TYPE_ASPEED_GPIO,
> +            .class_init = aspeed_gpio_class_init,
> +            .class_data = (void *)&controllers[i],
> +        };
> +        type_register(&t);
> +    };

I think we want separate classes for the 2400 and 2500, especially with
the 2600 on the way.

> +}
> +
> +type_init(aspeed_gpio_register_types);
> diff --git a/include/hw/gpio/aspeed_gpio.h 
> b/include/hw/gpio/aspeed_gpio.h
> new file mode 100644
> index 0000000000..97a84a5da1
> --- /dev/null
> +++ b/include/hw/gpio/aspeed_gpio.h
> @@ -0,0 +1,76 @@
> +/*
> + *  ASPEED GPIO Controller
> + *
> + *  Copyright (C) 2017-2018 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#ifndef ASPEED_GPIO_H
> +#define ASPEED_GPIO_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_ASPEED_GPIO "aspeed.gpio"
> +#define ASPEED_GPIO(obj) OBJECT_CHECK(AspeedGPIOState, (obj), 
> TYPE_ASPEED_GPIO)
> +#define ASPEED_GPIO_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(AspeedGPIOClass, (klass), TYPE_ASPEED_GPIO)
> +#define ASPEED_GPIO_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(AspeedGPIOClass, (obj), TYPE_ASPEED_GPIO)
> +
> +#define ASPEED_GPIO_MAX_NR_SETS 8
> +#define ASPEED_REGS_PER_BANK 14
> +#define ASPEED_GPIO_MAX_NR_REGS (ASPEED_REGS_PER_BANK * 
> ASPEED_GPIO_MAX_NR_SETS)
> +#define ASPEED_GPIO_NR_PINS 228
> +
> +typedef struct GPIORegs GPIORegs;
> +typedef const struct GPIOSetProperties {

The const in the typedef seems odd. I feel like it would be better as
explicit in the code?

> +    uint32_t input;
> +    uint32_t output;
> +    char set[4][3];
> +} GPIOSetProperties;
> +
> +typedef const struct AspeedGPIOController {

As above.

Cheers,

Andrew

> +    const char *name;
> +    GPIOSetProperties *props;
> +    uint32_t nr_gpio_pins;
> +    uint32_t nr_gpio_sets;
> +    uint32_t gap;
> +} AspeedGPIOController;
> +
> +typedef struct  AspeedGPIOClass {
> +    SysBusDevice parent_obj;
> +    AspeedGPIOController *ctrl;
> +}  AspeedGPIOClass;
> +
> +typedef struct AspeedGPIOState {
> +    /* <private> */
> +    SysBusDevice parent;
> +
> +    /*< public >*/
> +    MemoryRegion iomem;
> +    qemu_irq output[ASPEED_GPIO_NR_PINS];
> +    qemu_irq irq;
> +    AspeedGPIOController *ctrl;
> +
> +/* Parallel GPIO Registers */
> +    struct GPIORegs {
> +        uint32_t data_value; /* Reflects pin values */
> +        uint32_t data_read; /* Contains last value written to data 
> value */
> +        uint32_t direction;
> +        uint32_t int_enable;
> +        uint32_t int_sens_0;
> +        uint32_t int_sens_1;
> +        uint32_t int_sens_2;
> +        uint32_t int_status;
> +        uint32_t reset_tol;
> +        uint32_t cmd_source_0;
> +        uint32_t cmd_source_1;
> +        uint32_t debounce_1;
> +        uint32_t debounce_2;
> +        uint32_t input_mask;
> +    } sets[ASPEED_GPIO_MAX_NR_SETS];
> +} AspeedGPIOState;
> +
> +#endif /* _ASPEED_GPIO_H_ */
> -- 
> 2.17.2
> 
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model
  2019-07-03  4:16   ` Andrew Jeffery
@ 2019-07-15  2:36     ` Rashmica Gupta
  2019-07-15  2:43       ` Andrew Jeffery
  0 siblings, 1 reply; 11+ messages in thread
From: Rashmica Gupta @ 2019-07-15  2:36 UTC (permalink / raw)
  To: Andrew Jeffery, qemu-arm; +Cc: Cédric Le Goater, qemu-devel, Joel Stanley

Sorry for the late reply! I agree with most of your feedback and will
send out
a v2 shortly with those changes. I have a few replies below

[snip]

> > +static const struct AspeedGPIO gpios[0x1f0] = {
> > +    /* Set ABCD */
> > +    [GPIO_ABCD_DATA_VALUE] = {0, read_data_value,
> > _write_data_value},
> 
> Maybe there would be less tedium (and risk of minor bugs) here if we
> add
> a table for looking up the read/write callbacks from a type, and just
> stash
> the type in struct AspeedGPIO (or a pointer to the corresponding type
> ops),
> e.g:
> 
> enum gpio_property { gpio_direction = 0, gpio_int_enable, ... };
> 
> struct aspeed_gpio_reg_ops {
>     int (*read)(...);
>     int (*write)(...);
> };
> 
> struct aspeed_gpio_reg_ops gpio_reg_ops[] = {
>     [gpio_direction]  = { .read = read_direction, .write =
> write_direction };
>     ...
> };
> 
> Then we have:
> 
> static const struct AspeedGPIO gpios[...] = {
>      [GPIO_ABCD_DIRECTION] = { .set = 0, .type = gpio_direction },
>      ...
> };
> 
> Not wedded to the idea, just thinking out loud. What do you think?
> 

I'm not a huge fan of the extra indirection. It is less error prone but
is this a place that is likely to be updated regularly?


> > +    [GPIO_ABCD_DIRECTION] = {0, read_direction, _write_direction},
> > +    [GPIO_ABCD_INT_ENABLE] = {0, read_int_enable,
> > _write_int_enable},
> > +    [GPIO_ABCD_INT_SENS_0] = {0, read_int_sens_0,
> > _write_int_sens_0},
> > +    [GPIO_ABCD_INT_SENS_1] = {0, read_int_sens_1,
> > _write_int_sens_1},
> > +    [GPIO_ABCD_INT_SENS_2] = {0, read_int_sens_2,
> > _write_int_sens_2},
> > +    [GPIO_ABCD_INT_STATUS] = {0, read_int_status,
> > _write_int_status},
> > +    [GPIO_ABCD_RESET_TOLERANT] = {0, read_reset_tol,
> > _write_reset_tol},
> > +    [GPIO_ABCD_DEBOUNCE_1] = {0, read_debounce_1,
> > _write_debounce_1},
> > +    [GPIO_ABCD_DEBOUNCE_2] = {0, read_debounce_2,
> > _write_debounce_2},
> > +    [GPIO_ABCD_COMMAND_SRC_0] = {0, read_cmd_source_0, 
> > _write_cmd_source_0},
> > +    [GPIO_ABCD_COMMAND_SRC_1] = {0, read_cmd_source_1, 
> > _write_cmd_source_1},
> > +    [GPIO_ABCD_DATA_READ] = {0, read_data, NULL},
> > +    [GPIO_ABCD_INPUT_MASK] = {0, read_input_mask,
> > _write_input_mask},
> > +    /* Set EFGH */
> > +    [GPIO_EFGH_DATA_VALUE] = {1, read_data_value,
> > _write_data_value},
> > +    [GPIO_EFGH_DIRECTION] = {1, read_direction, _write_direction
> > },
> > +    [GPIO_EFGH_INT_ENABLE] = {1, read_int_enable,
> > _write_int_enable},
> > +    [GPIO_EFGH_INT_SENS_0] = {1, read_int_sens_0,
> > _write_int_sens_0},
> > +    [GPIO_EFGH_INT_SENS_1] = {1, read_int_sens_1,
> > _write_int_sens_1},
> > +    [GPIO_EFGH_INT_SENS_2] = {1, read_int_sens_2,
> > _write_int_sens_2},
> > +    [GPIO_EFGH_INT_STATUS] = {1, read_int_status,
> > _write_int_status},
> > +    [GPIO_EFGH_RESET_TOL] = {1,
> > read_reset_tol,   _write_reset_tol},
> > +    [GPIO_EFGH_DEBOUNCE_1] = {1,
> > read_debounce_1,  _write_debounce_1},
> > +    [GPIO_EFGH_DEBOUNCE_2] = {1,
> > read_debounce_2,  _write_debounce_2},
> > +    [GPIO_EFGH_COMMAND_SRC_0] = {1, read_cmd_source_0,  
> > _write_cmd_source_0},
> > +    [GPIO_EFGH_COMMAND_SRC_1] = {1, read_cmd_source_1,  
> > _write_cmd_source_1},
> > +    [GPIO_EFGH_DATA_READ] = {1, read_data, NULL},
> > +    [GPIO_EFGH_INPUT_MASK] = {1,
> > read_input_mask,  _write_input_mask},
> > +    /* Set IJKL */
> > +    [GPIO_IJKL_DATA_VALUE] = {2, read_data_value,
> > _write_data_value},
> > +    [GPIO_IJKL_DIRECTION] = {2, read_direction, _write_direction},
> > +    [GPIO_IJKL_INT_ENABLE] = {2, read_int_enable,
> > _write_int_enable},
> > +    [GPIO_IJKL_INT_SENS_0] = {2, read_int_sens_0,
> > _write_int_sens_0},
> > +    [GPIO_IJKL_INT_SENS_1] = {2, read_int_sens_1,
> > _write_int_sens_1},
> > +    [GPIO_IJKL_INT_SENS_2] = {2, read_int_sens_2,
> > _write_int_sens_2},
> > +    [GPIO_IJKL_INT_STATUS] = {2, read_int_status,
> > _write_int_status},
> > +    [GPIO_IJKL_RESET_TOLERANT] = {2, read_reset_tol,
> > _write_reset_tol},
> > +    [GPIO_IJKL_DEBOUNCE_1] = {2, read_debounce_1,
> > _write_debounce_1},
> > +    [GPIO_IJKL_DEBOUNCE_2] = {2, read_debounce_2,
> > _write_debounce_2},
> > +    [GPIO_IJKL_COMMAND_SRC_0] = {2, read_cmd_source_0, 
> > _write_cmd_source_0},
> > +    [GPIO_IJKL_COMMAND_SRC_1] = {2, read_cmd_source_1, 
> > _write_cmd_source_1},
> > +    [GPIO_IJKL_DATA_READ] = {2, read_data, NULL},
> > +    [GPIO_IJKL_INPUT_MASK] = {2, read_input_mask,
> > _write_input_mask},
> > +    /* Set MNOP */
> > +    [GPIO_MNOP_DATA_VALUE] = {3, read_data_value,
> > _write_data_value},
> > +    [GPIO_MNOP_DIRECTION] = {3, read_direction, _write_direction},
> > +    [GPIO_MNOP_INT_ENABLE] = {3, read_int_enable,
> > _write_int_enable},
> > +    [GPIO_MNOP_INT_SENS_0] = {3, read_int_sens_0,
> > _write_int_sens_0},
> > +    [GPIO_MNOP_INT_SENS_1] = {3, read_int_sens_1,
> > _write_int_sens_1},
> > +    [GPIO_MNOP_INT_SENS_2] = {3, read_int_sens_2,
> > _write_int_sens_2},
> > +    [GPIO_MNOP_INT_STATUS] = {3, read_int_status,
> > _write_int_status},
> > +    [GPIO_MNOP_RESET_TOLERANT] = {3, read_reset_tol,  
> > _write_reset_tol},
> > +    [GPIO_MNOP_DEBOUNCE_1] = {3, read_debounce_1,
> > _write_debounce_1},
> > +    [GPIO_MNOP_DEBOUNCE_2] = {3, read_debounce_2,
> > _write_debounce_2},
> > +    [GPIO_MNOP_COMMAND_SRC_0] = {3, read_cmd_source_0, 
> > _write_cmd_source_0},
> > +    [GPIO_MNOP_COMMAND_SRC_1] = {3, read_cmd_source_1, 
> > _write_cmd_source_1},
> > +    [GPIO_MNOP_DATA_READ] = {3, read_data, NULL},
> > +    [GPIO_MNOP_INPUT_MASK] = {3, read_input_mask,
> > _write_input_mask},
> > +    /* Set QRST */
> > +    [GPIO_QRST_DATA_VALUE] = {4, read_data_value,
> > _write_data_value},
> > +    [GPIO_QRST_DIRECTION] = {4, read_direction, _write_direction},
> > +    [GPIO_QRST_INT_ENABLE] = {4, read_int_enable,
> > _write_int_enable},
> > +    [GPIO_QRST_INT_SENS_0] = {4, read_int_sens_0,
> > _write_int_sens_0},
> > +    [GPIO_QRST_INT_SENS_1] = {4, read_int_sens_1,
> > _write_int_sens_1},
> > +    [GPIO_QRST_INT_SENS_2] = {4, read_int_sens_2,
> > _write_int_sens_2},
> > +    [GPIO_QRST_INT_STATUS] = {4, read_int_status,
> > _write_int_status},
> > +    [GPIO_QRST_RESET_TOLERANT] = {4, read_reset_tol,
> > _write_reset_tol},
> > +    [GPIO_QRST_DEBOUNCE_1] = {4, read_debounce_1,
> > _write_debounce_1},
> > +    [GPIO_QRST_DEBOUNCE_2] = {4, read_debounce_2,
> > _write_debounce_2},
> > +    [GPIO_QRST_COMMAND_SRC_0] = {4, read_cmd_source_0, 
> > _write_cmd_source_0},
> > +    [GPIO_QRST_COMMAND_SRC_1] = {4, read_cmd_source_1, 
> > _write_cmd_source_1},
> > +    [GPIO_QRST_DATA_READ] = {4, read_data, NULL},
> > +    [GPIO_QRST_INPUT_MASK] = {4,
> > read_input_mask,  _write_input_mask},
> > +    /* Set UVWX */
> > +    [GPIO_UVWX_DATA_VALUE] = {5, read_data_value,
> > _write_data_value},
> > +    [GPIO_UWVX_DIRECTION] = {5, read_direction, _write_direction},
> > +    [GPIO_UVWX_INT_ENABLE] = {5, read_int_enable,
> > _write_int_enable},
> > +    [GPIO_UVWX_INT_SENS_0] = {5, read_int_sens_0,
> > _write_int_sens_0},
> > +    [GPIO_UVWX_INT_SENS_1] = {5, read_int_sens_1,
> > _write_int_sens_1},
> > +    [GPIO_UVWX_INT_SENS_2] = {5, read_int_sens_2,
> > _write_int_sens_2},
> > +    [GPIO_UVWX_INT_STATUS] = {5, read_int_status,
> > _write_int_status},
> > +    [GPIO_UVWX_RESET_TOLERANT] = {5, read_reset_tol,
> > _write_reset_tol},
> > +    [GPIO_UVWX_DEBOUNCE_1] = {5, read_debounce_1,
> > _write_debounce_1},
> > +    [GPIO_UVWX_DEBOUNCE_2] = {5, read_debounce_2,
> > _write_debounce_2},
> > +    [GPIO_UVWX_COMMAND_SRC_0] = {5, read_cmd_source_0, 
> > _write_cmd_source_0},
> > +    [GPIO_UVWX_COMMAND_SRC_1] = {5, read_cmd_source_1, 
> > _write_cmd_source_1},
> > +    [GPIO_UVWX_DATA_READ] = {5, read_data, NULL},
> > +    [GPIO_UVWX_INPUT_MASK] = {5, read_input_mask,
> > _write_input_mask},
> > +    /* Set YZAAAB */
> > +    [GPIO_YZAAAB_DATA_VALUE] = {6, read_data_value,
> > _write_data_value},
> > +    [GPIO_YZAAAB_DIRECTION] = {6, read_direction,
> > _write_direction},
> > +    [GPIO_YZAAAB_INT_ENABLE] = {6, read_int_enable,
> > _write_int_enable},
> > +    [GPIO_YZAAAB_INT_SENS_0] = {6, read_int_sens_0,
> > _write_int_sens_0},
> > +    [GPIO_YZAAAB_INT_SENS_1] = {6, read_int_sens_1,
> > _write_int_sens_1},
> > +    [GPIO_YZAAAB_INT_SENS_2] = {6, read_int_sens_2,
> > _write_int_sens_2},
> > +    [GPIO_YZAAAB_INT_STATUS] = {6, read_int_status,
> > _write_int_status},
> > +    [GPIO_YZAAAB_RESET_TOLERANT] = {6, read_reset_tol, 
> > _write_reset_tol},
> > +    [GPIO_YZAAAB_DEBOUNCE_1] = {6, read_debounce_1,
> > _write_debounce_1},
> > +    [GPIO_YZAAAB_DEBOUNCE_2] = {6, read_debounce_2,
> > _write_debounce_2},
> > +    [GPIO_YZAAAB_COMMAND_SRC_0] = {6, read_cmd_source_0, 
> > _write_cmd_source_0},
> > +    [GPIO_YZAAAB_COMMAND_SRC_1] = {6, read_cmd_source_1, 
> > _write_cmd_source_1},
> > +    [GPIO_YZAAAB_DATA_READ] = {6, read_data, NULL},
> > +    [GPIO_YZAAAB_INPUT_MASK] = {6, read_input_mask,
> > _write_input_mask},
> > +    /* Set AC */
> > +    [GPIO_AC_DATA_VALUE] = {7, read_data_value,
> > _write_data_value},
> > +    [GPIO_AC_DIRECTION] = {7, read_direction, _write_direction},
> > +    [GPIO_AC_INT_ENABLE] = {7, read_int_enable,
> > _write_int_enable},
> > +    [GPIO_AC_INT_SENS_0] = {7, read_int_sens_0,
> > _write_int_sens_0},
> > +    [GPIO_AC_INT_SENS_1] = {7, read_int_sens_1,
> > _write_int_sens_1},
> > +    [GPIO_AC_INT_SENS_2] = {7, read_int_sens_2,
> > _write_int_sens_2},
> > +    [GPIO_AC_INT_STATUS] = {7, read_int_status,
> > _write_int_status},
> > +    [GPIO_AC_RESET_TOLERANT] = {7, read_reset_tol,
> > _write_reset_tol},
> > +    [GPIO_AC_DEBOUNCE_1] = {7, read_debounce_1,
> > _write_debounce_1},
> > +    [GPIO_AC_DEBOUNCE_2] = {7, read_debounce_2,
> > _write_debounce_2},
> > +    [GPIO_AC_COMMAND_SRC_0] = {7, read_cmd_source_0, 
> > _write_cmd_source_0},
> > +    [GPIO_AC_COMMAND_SRC_1] = {7, read_cmd_source_1, 
> > _write_cmd_source_1},
> > +    [GPIO_AC_DATA_READ] = {7, read_data, NULL},
> > +    [GPIO_AC_INPUT_MASK] = {7,
> > read_input_mask,     _write_input_mask},
> > +    /* Debounce registers */
> > +    [GPIO_DEBOUNCE_TIME_1] = {-1, NULL, NULL},
> > +};
> > +
> > +static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset,
> > uint32_t 
> > size)
> > +{
> > +    AspeedGPIOState *s = ASPEED_GPIO(opaque);
> > +    uint32_t val = 0;
> > +
> > +    if (size != 4) {
> > +        return 0;
> > +    }
> > +
> > +    if (gpios[offset].get == NULL) {
> 
> With the AST2600 we might need to make gpios class-specific rather
> than
> a static global to the compilation unit (to deal with the 1.8V GPIO
> registers
> later in the address-space). Not sure though, haven't thought about
> it for
> more than this paragraph.


Yup, I agree. Still trying to sort out a nice way to deal with the
AST2600 differences so I figured I'd leave it like this for now? 

> 
> > +        qemu_log_mask(LOG_GUEST_ERROR, "no getter for offset
> > %lx", 
> > offset);
> 
> Really this is a model error. We should only encounter it if we
> haven't
> completely described the controller. So maybe it should be an assert
> or some kind of non-guest warning?
>

I don't think this is quite true. There are values in the gpio memory
space
that do not have a register associated with them, ie registers with
offsets 0x138 and 0x140 exist, but there is no 0x13c. So if the
kernel tries to access this address, this isn't really a gpio model
error? 


> > +        return 0;
> > +    }
> > +
> > +    val = gpios[offset].get(&s->sets[gpios[offset].set_idx]);
> 
> The indexing here is starting to get a bit hard to follow. I think we
> could
> improve readability by introducing some locals, and rewriting in
> terms
> of those:
> 
> const struct AspeedGPIO *reg = &gpios[offset];
> const struct GPIORegs *set = &s->sets[gpio.set_idx];
> 
> if (!reg.get) {
>     ...
> }
> 
> val = reg.get(set);
> 
> I think it makes the relationship of the structs a bit clearer too,
> and I think
> it also suggests we could improve the names:
> 
> s/struct AspeedGPIO/struct AspeedGPIOReg/
> s/struct GPIORegs/struct GPIOProperties/
> 

I agree with the local variables, and renaming AspeedGPIO-
>AspeedGPIOReg
but I think GPIOSet makes more sense than GPIOProperties.



> GPIOProperties clashes a bit with GPIOSetProperties though, so maybe
> we
> need to think of a better name for that too?
> 
> > +    return (uint64_t) val;
> > +}
> > +
> > +static void aspeed_gpio_write(void *opaque, hwaddr offset,
> > uint64_t 
> > data,
> > +static GPIOSetProperties ast2400_set_props[] = {
	
[snip]

> > +    [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
> > +    [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
> > +    [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
> > +    [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
> > +    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
> > +    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> > +    [6] = {0x0000000f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
> > +};
> > +
> > +static GPIOSetProperties ast2500_set_props[] = {
> > +    [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
> > +    [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
> > +    [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
> > +    [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
> > +    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
> > +    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> > +    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
> > +    [7] = {0x000000ff,  0x000000ff,  {"AC"} },
> > +};
> > +
> > +static AspeedGPIOController controllers[] = {
> > +    {
> > +        .name           = TYPE_ASPEED_GPIO "-ast2500",
> > +        .props          = ast2500_set_props,
> > +        .nr_gpio_pins   = 228,
> > +        .nr_gpio_sets   = 8,
> > +        .gap            = 220,
> > +    }, {
> > +        .name           = TYPE_ASPEED_GPIO "-ast2400",
> > +        .props          = ast2400_set_props,
> > +        .nr_gpio_pins   = 216,
> > +        .nr_gpio_sets   = 7,
> > +        .gap            = 196,
> > +    }
> > +};
> > +
> > +static const TypeInfo aspeed_gpio_info = {
> > +    .name = TYPE_ASPEED_GPIO,
> > +    .parent = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(AspeedGPIOState),
> > +    .class_size     = sizeof(AspeedGPIOClass),
> > +    .instance_init = aspeed_gpio_init,
> > +    .abstract       = true,
> > +};
> > +
> > +static void aspeed_gpio_register_types(void)
> > +{
> > +    int i;
> > +
> > +    type_register_static(&aspeed_gpio_info);
> > +    for (i = 0; i < ARRAY_SIZE(controllers); i++) {
> > +        TypeInfo t = {
> > +            .name = controllers[i].name,
> > +            .parent = TYPE_ASPEED_GPIO,
> > +            .class_init = aspeed_gpio_class_init,
> > +            .class_data = (void *)&controllers[i],
> > +        };
> > +        type_register(&t);
> > +    };
> 
> I think we want separate classes for the 2400 and 2500, especially
> with
> the 2600 on the way.

I thought this was defining seperate classes that just happen to have
the same init function? I don't fully grasp classes in qemu yet, so
likely to be wrong :) 




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model
  2019-07-15  2:36     ` Rashmica Gupta
@ 2019-07-15  2:43       ` Andrew Jeffery
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Jeffery @ 2019-07-15  2:43 UTC (permalink / raw)
  To: Rashmica Gupta, qemu-arm; +Cc: Cédric Le Goater, qemu-devel, Joel Stanley



On Mon, 15 Jul 2019, at 12:06, Rashmica Gupta wrote:
> Sorry for the late reply! I agree with most of your feedback and will
> send out
> a v2 shortly with those changes. I have a few replies below
> 
> [snip]
> 
> > > +static const struct AspeedGPIO gpios[0x1f0] = {
> > > +    /* Set ABCD */
> > > +    [GPIO_ABCD_DATA_VALUE] = {0, read_data_value,
> > > _write_data_value},
> > 
> > Maybe there would be less tedium (and risk of minor bugs) here if we
> > add
> > a table for looking up the read/write callbacks from a type, and just
> > stash
> > the type in struct AspeedGPIO (or a pointer to the corresponding type
> > ops),
> > e.g:
> > 
> > enum gpio_property { gpio_direction = 0, gpio_int_enable, ... };
> > 
> > struct aspeed_gpio_reg_ops {
> >     int (*read)(...);
> >     int (*write)(...);
> > };
> > 
> > struct aspeed_gpio_reg_ops gpio_reg_ops[] = {
> >     [gpio_direction]  = { .read = read_direction, .write =
> > write_direction };
> >     ...
> > };
> > 
> > Then we have:
> > 
> > static const struct AspeedGPIO gpios[...] = {
> >      [GPIO_ABCD_DIRECTION] = { .set = 0, .type = gpio_direction },
> >      ...
> > };
> > 
> > Not wedded to the idea, just thinking out loud. What do you think?
> > 
> 
> I'm not a huge fan of the extra indirection. It is less error prone but
> is this a place that is likely to be updated regularly?

Right - if you've written correctly already then there's not much benefit.

> 
> 
> > > +    [GPIO_ABCD_DIRECTION] = {0, read_direction, _write_direction},
> > > +    [GPIO_ABCD_INT_ENABLE] = {0, read_int_enable,
> > > _write_int_enable},
> > > +    [GPIO_ABCD_INT_SENS_0] = {0, read_int_sens_0,
> > > _write_int_sens_0},
> > > +    [GPIO_ABCD_INT_SENS_1] = {0, read_int_sens_1,
> > > _write_int_sens_1},
> > > +    [GPIO_ABCD_INT_SENS_2] = {0, read_int_sens_2,
> > > _write_int_sens_2},
> > > +    [GPIO_ABCD_INT_STATUS] = {0, read_int_status,
> > > _write_int_status},
> > > +    [GPIO_ABCD_RESET_TOLERANT] = {0, read_reset_tol,
> > > _write_reset_tol},
> > > +    [GPIO_ABCD_DEBOUNCE_1] = {0, read_debounce_1,
> > > _write_debounce_1},
> > > +    [GPIO_ABCD_DEBOUNCE_2] = {0, read_debounce_2,
> > > _write_debounce_2},
> > > +    [GPIO_ABCD_COMMAND_SRC_0] = {0, read_cmd_source_0, 
> > > _write_cmd_source_0},
> > > +    [GPIO_ABCD_COMMAND_SRC_1] = {0, read_cmd_source_1, 
> > > _write_cmd_source_1},
> > > +    [GPIO_ABCD_DATA_READ] = {0, read_data, NULL},
> > > +    [GPIO_ABCD_INPUT_MASK] = {0, read_input_mask,
> > > _write_input_mask},
> > > +    /* Set EFGH */
> > > +    [GPIO_EFGH_DATA_VALUE] = {1, read_data_value,
> > > _write_data_value},
> > > +    [GPIO_EFGH_DIRECTION] = {1, read_direction, _write_direction
> > > },
> > > +    [GPIO_EFGH_INT_ENABLE] = {1, read_int_enable,
> > > _write_int_enable},
> > > +    [GPIO_EFGH_INT_SENS_0] = {1, read_int_sens_0,
> > > _write_int_sens_0},
> > > +    [GPIO_EFGH_INT_SENS_1] = {1, read_int_sens_1,
> > > _write_int_sens_1},
> > > +    [GPIO_EFGH_INT_SENS_2] = {1, read_int_sens_2,
> > > _write_int_sens_2},
> > > +    [GPIO_EFGH_INT_STATUS] = {1, read_int_status,
> > > _write_int_status},
> > > +    [GPIO_EFGH_RESET_TOL] = {1,
> > > read_reset_tol,   _write_reset_tol},
> > > +    [GPIO_EFGH_DEBOUNCE_1] = {1,
> > > read_debounce_1,  _write_debounce_1},
> > > +    [GPIO_EFGH_DEBOUNCE_2] = {1,
> > > read_debounce_2,  _write_debounce_2},
> > > +    [GPIO_EFGH_COMMAND_SRC_0] = {1, read_cmd_source_0,  
> > > _write_cmd_source_0},
> > > +    [GPIO_EFGH_COMMAND_SRC_1] = {1, read_cmd_source_1,  
> > > _write_cmd_source_1},
> > > +    [GPIO_EFGH_DATA_READ] = {1, read_data, NULL},
> > > +    [GPIO_EFGH_INPUT_MASK] = {1,
> > > read_input_mask,  _write_input_mask},
> > > +    /* Set IJKL */
> > > +    [GPIO_IJKL_DATA_VALUE] = {2, read_data_value,
> > > _write_data_value},
> > > +    [GPIO_IJKL_DIRECTION] = {2, read_direction, _write_direction},
> > > +    [GPIO_IJKL_INT_ENABLE] = {2, read_int_enable,
> > > _write_int_enable},
> > > +    [GPIO_IJKL_INT_SENS_0] = {2, read_int_sens_0,
> > > _write_int_sens_0},
> > > +    [GPIO_IJKL_INT_SENS_1] = {2, read_int_sens_1,
> > > _write_int_sens_1},
> > > +    [GPIO_IJKL_INT_SENS_2] = {2, read_int_sens_2,
> > > _write_int_sens_2},
> > > +    [GPIO_IJKL_INT_STATUS] = {2, read_int_status,
> > > _write_int_status},
> > > +    [GPIO_IJKL_RESET_TOLERANT] = {2, read_reset_tol,
> > > _write_reset_tol},
> > > +    [GPIO_IJKL_DEBOUNCE_1] = {2, read_debounce_1,
> > > _write_debounce_1},
> > > +    [GPIO_IJKL_DEBOUNCE_2] = {2, read_debounce_2,
> > > _write_debounce_2},
> > > +    [GPIO_IJKL_COMMAND_SRC_0] = {2, read_cmd_source_0, 
> > > _write_cmd_source_0},
> > > +    [GPIO_IJKL_COMMAND_SRC_1] = {2, read_cmd_source_1, 
> > > _write_cmd_source_1},
> > > +    [GPIO_IJKL_DATA_READ] = {2, read_data, NULL},
> > > +    [GPIO_IJKL_INPUT_MASK] = {2, read_input_mask,
> > > _write_input_mask},
> > > +    /* Set MNOP */
> > > +    [GPIO_MNOP_DATA_VALUE] = {3, read_data_value,
> > > _write_data_value},
> > > +    [GPIO_MNOP_DIRECTION] = {3, read_direction, _write_direction},
> > > +    [GPIO_MNOP_INT_ENABLE] = {3, read_int_enable,
> > > _write_int_enable},
> > > +    [GPIO_MNOP_INT_SENS_0] = {3, read_int_sens_0,
> > > _write_int_sens_0},
> > > +    [GPIO_MNOP_INT_SENS_1] = {3, read_int_sens_1,
> > > _write_int_sens_1},
> > > +    [GPIO_MNOP_INT_SENS_2] = {3, read_int_sens_2,
> > > _write_int_sens_2},
> > > +    [GPIO_MNOP_INT_STATUS] = {3, read_int_status,
> > > _write_int_status},
> > > +    [GPIO_MNOP_RESET_TOLERANT] = {3, read_reset_tol,  
> > > _write_reset_tol},
> > > +    [GPIO_MNOP_DEBOUNCE_1] = {3, read_debounce_1,
> > > _write_debounce_1},
> > > +    [GPIO_MNOP_DEBOUNCE_2] = {3, read_debounce_2,
> > > _write_debounce_2},
> > > +    [GPIO_MNOP_COMMAND_SRC_0] = {3, read_cmd_source_0, 
> > > _write_cmd_source_0},
> > > +    [GPIO_MNOP_COMMAND_SRC_1] = {3, read_cmd_source_1, 
> > > _write_cmd_source_1},
> > > +    [GPIO_MNOP_DATA_READ] = {3, read_data, NULL},
> > > +    [GPIO_MNOP_INPUT_MASK] = {3, read_input_mask,
> > > _write_input_mask},
> > > +    /* Set QRST */
> > > +    [GPIO_QRST_DATA_VALUE] = {4, read_data_value,
> > > _write_data_value},
> > > +    [GPIO_QRST_DIRECTION] = {4, read_direction, _write_direction},
> > > +    [GPIO_QRST_INT_ENABLE] = {4, read_int_enable,
> > > _write_int_enable},
> > > +    [GPIO_QRST_INT_SENS_0] = {4, read_int_sens_0,
> > > _write_int_sens_0},
> > > +    [GPIO_QRST_INT_SENS_1] = {4, read_int_sens_1,
> > > _write_int_sens_1},
> > > +    [GPIO_QRST_INT_SENS_2] = {4, read_int_sens_2,
> > > _write_int_sens_2},
> > > +    [GPIO_QRST_INT_STATUS] = {4, read_int_status,
> > > _write_int_status},
> > > +    [GPIO_QRST_RESET_TOLERANT] = {4, read_reset_tol,
> > > _write_reset_tol},
> > > +    [GPIO_QRST_DEBOUNCE_1] = {4, read_debounce_1,
> > > _write_debounce_1},
> > > +    [GPIO_QRST_DEBOUNCE_2] = {4, read_debounce_2,
> > > _write_debounce_2},
> > > +    [GPIO_QRST_COMMAND_SRC_0] = {4, read_cmd_source_0, 
> > > _write_cmd_source_0},
> > > +    [GPIO_QRST_COMMAND_SRC_1] = {4, read_cmd_source_1, 
> > > _write_cmd_source_1},
> > > +    [GPIO_QRST_DATA_READ] = {4, read_data, NULL},
> > > +    [GPIO_QRST_INPUT_MASK] = {4,
> > > read_input_mask,  _write_input_mask},
> > > +    /* Set UVWX */
> > > +    [GPIO_UVWX_DATA_VALUE] = {5, read_data_value,
> > > _write_data_value},
> > > +    [GPIO_UWVX_DIRECTION] = {5, read_direction, _write_direction},
> > > +    [GPIO_UVWX_INT_ENABLE] = {5, read_int_enable,
> > > _write_int_enable},
> > > +    [GPIO_UVWX_INT_SENS_0] = {5, read_int_sens_0,
> > > _write_int_sens_0},
> > > +    [GPIO_UVWX_INT_SENS_1] = {5, read_int_sens_1,
> > > _write_int_sens_1},
> > > +    [GPIO_UVWX_INT_SENS_2] = {5, read_int_sens_2,
> > > _write_int_sens_2},
> > > +    [GPIO_UVWX_INT_STATUS] = {5, read_int_status,
> > > _write_int_status},
> > > +    [GPIO_UVWX_RESET_TOLERANT] = {5, read_reset_tol,
> > > _write_reset_tol},
> > > +    [GPIO_UVWX_DEBOUNCE_1] = {5, read_debounce_1,
> > > _write_debounce_1},
> > > +    [GPIO_UVWX_DEBOUNCE_2] = {5, read_debounce_2,
> > > _write_debounce_2},
> > > +    [GPIO_UVWX_COMMAND_SRC_0] = {5, read_cmd_source_0, 
> > > _write_cmd_source_0},
> > > +    [GPIO_UVWX_COMMAND_SRC_1] = {5, read_cmd_source_1, 
> > > _write_cmd_source_1},
> > > +    [GPIO_UVWX_DATA_READ] = {5, read_data, NULL},
> > > +    [GPIO_UVWX_INPUT_MASK] = {5, read_input_mask,
> > > _write_input_mask},
> > > +    /* Set YZAAAB */
> > > +    [GPIO_YZAAAB_DATA_VALUE] = {6, read_data_value,
> > > _write_data_value},
> > > +    [GPIO_YZAAAB_DIRECTION] = {6, read_direction,
> > > _write_direction},
> > > +    [GPIO_YZAAAB_INT_ENABLE] = {6, read_int_enable,
> > > _write_int_enable},
> > > +    [GPIO_YZAAAB_INT_SENS_0] = {6, read_int_sens_0,
> > > _write_int_sens_0},
> > > +    [GPIO_YZAAAB_INT_SENS_1] = {6, read_int_sens_1,
> > > _write_int_sens_1},
> > > +    [GPIO_YZAAAB_INT_SENS_2] = {6, read_int_sens_2,
> > > _write_int_sens_2},
> > > +    [GPIO_YZAAAB_INT_STATUS] = {6, read_int_status,
> > > _write_int_status},
> > > +    [GPIO_YZAAAB_RESET_TOLERANT] = {6, read_reset_tol, 
> > > _write_reset_tol},
> > > +    [GPIO_YZAAAB_DEBOUNCE_1] = {6, read_debounce_1,
> > > _write_debounce_1},
> > > +    [GPIO_YZAAAB_DEBOUNCE_2] = {6, read_debounce_2,
> > > _write_debounce_2},
> > > +    [GPIO_YZAAAB_COMMAND_SRC_0] = {6, read_cmd_source_0, 
> > > _write_cmd_source_0},
> > > +    [GPIO_YZAAAB_COMMAND_SRC_1] = {6, read_cmd_source_1, 
> > > _write_cmd_source_1},
> > > +    [GPIO_YZAAAB_DATA_READ] = {6, read_data, NULL},
> > > +    [GPIO_YZAAAB_INPUT_MASK] = {6, read_input_mask,
> > > _write_input_mask},
> > > +    /* Set AC */
> > > +    [GPIO_AC_DATA_VALUE] = {7, read_data_value,
> > > _write_data_value},
> > > +    [GPIO_AC_DIRECTION] = {7, read_direction, _write_direction},
> > > +    [GPIO_AC_INT_ENABLE] = {7, read_int_enable,
> > > _write_int_enable},
> > > +    [GPIO_AC_INT_SENS_0] = {7, read_int_sens_0,
> > > _write_int_sens_0},
> > > +    [GPIO_AC_INT_SENS_1] = {7, read_int_sens_1,
> > > _write_int_sens_1},
> > > +    [GPIO_AC_INT_SENS_2] = {7, read_int_sens_2,
> > > _write_int_sens_2},
> > > +    [GPIO_AC_INT_STATUS] = {7, read_int_status,
> > > _write_int_status},
> > > +    [GPIO_AC_RESET_TOLERANT] = {7, read_reset_tol,
> > > _write_reset_tol},
> > > +    [GPIO_AC_DEBOUNCE_1] = {7, read_debounce_1,
> > > _write_debounce_1},
> > > +    [GPIO_AC_DEBOUNCE_2] = {7, read_debounce_2,
> > > _write_debounce_2},
> > > +    [GPIO_AC_COMMAND_SRC_0] = {7, read_cmd_source_0, 
> > > _write_cmd_source_0},
> > > +    [GPIO_AC_COMMAND_SRC_1] = {7, read_cmd_source_1, 
> > > _write_cmd_source_1},
> > > +    [GPIO_AC_DATA_READ] = {7, read_data, NULL},
> > > +    [GPIO_AC_INPUT_MASK] = {7,
> > > read_input_mask,     _write_input_mask},
> > > +    /* Debounce registers */
> > > +    [GPIO_DEBOUNCE_TIME_1] = {-1, NULL, NULL},
> > > +};
> > > +
> > > +static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset,
> > > uint32_t 
> > > size)
> > > +{
> > > +    AspeedGPIOState *s = ASPEED_GPIO(opaque);
> > > +    uint32_t val = 0;
> > > +
> > > +    if (size != 4) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    if (gpios[offset].get == NULL) {
> > 
> > With the AST2600 we might need to make gpios class-specific rather
> > than
> > a static global to the compilation unit (to deal with the 1.8V GPIO
> > registers
> > later in the address-space). Not sure though, haven't thought about
> > it for
> > more than this paragraph.
> 
> 
> Yup, I agree. Still trying to sort out a nice way to deal with the
> AST2600 differences so I figured I'd leave it like this for now? 

Okay.

> 
> > 
> > > +        qemu_log_mask(LOG_GUEST_ERROR, "no getter for offset
> > > %lx", 
> > > offset);
> > 
> > Really this is a model error. We should only encounter it if we
> > haven't
> > completely described the controller. So maybe it should be an assert
> > or some kind of non-guest warning?
> >
> 
> I don't think this is quite true. There are values in the gpio memory
> space
> that do not have a register associated with them, ie registers with
> offsets 0x138 and 0x140 exist, but there is no 0x13c. So if the
> kernel tries to access this address, this isn't really a gpio model
> error? 

Ah, clearly I hadn't looked closely enough :) Well spotted.

> 
> 
> > > +        return 0;
> > > +    }
> > > +
> > > +    val = gpios[offset].get(&s->sets[gpios[offset].set_idx]);
> > 
> > The indexing here is starting to get a bit hard to follow. I think we
> > could
> > improve readability by introducing some locals, and rewriting in
> > terms
> > of those:
> > 
> > const struct AspeedGPIO *reg = &gpios[offset];
> > const struct GPIORegs *set = &s->sets[gpio.set_idx];
> > 
> > if (!reg.get) {
> >     ...
> > }
> > 
> > val = reg.get(set);
> > 
> > I think it makes the relationship of the structs a bit clearer too,
> > and I think
> > it also suggests we could improve the names:
> > 
> > s/struct AspeedGPIO/struct AspeedGPIOReg/
> > s/struct GPIORegs/struct GPIOProperties/
> > 
> 
> I agree with the local variables, and renaming AspeedGPIO-
> >AspeedGPIOReg
> but I think GPIOSet makes more sense than GPIOProperties.
> 

Okay.

> 
> 
> > GPIOProperties clashes a bit with GPIOSetProperties though, so maybe
> > we
> > need to think of a better name for that too?
> > 
> > > +    return (uint64_t) val;
> > > +}
> > > +
> > > +static void aspeed_gpio_write(void *opaque, hwaddr offset,
> > > uint64_t 
> > > data,
> > > +static GPIOSetProperties ast2400_set_props[] = {
> 	
> [snip]
> 
> > > +    [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
> > > +    [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
> > > +    [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
> > > +    [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
> > > +    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
> > > +    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> > > +    [6] = {0x0000000f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
> > > +};
> > > +
> > > +static GPIOSetProperties ast2500_set_props[] = {
> > > +    [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
> > > +    [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
> > > +    [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
> > > +    [3] = {0xffffffff,  0xffffffff,  {"M", "N", "O", "P"} },
> > > +    [4] = {0xffffffff,  0xffffffff,  {"Q", "R", "S", "T"} },
> > > +    [5] = {0xffffffff,  0x0000ffff,  {"U", "V", "W", "X"} },
> > > +    [6] = {0xffffff0f,  0x0fffff0f,  {"Y", "Z", "AA", "AB"} },
> > > +    [7] = {0x000000ff,  0x000000ff,  {"AC"} },
> > > +};
> > > +
> > > +static AspeedGPIOController controllers[] = {
> > > +    {
> > > +        .name           = TYPE_ASPEED_GPIO "-ast2500",
> > > +        .props          = ast2500_set_props,
> > > +        .nr_gpio_pins   = 228,
> > > +        .nr_gpio_sets   = 8,
> > > +        .gap            = 220,
> > > +    }, {
> > > +        .name           = TYPE_ASPEED_GPIO "-ast2400",
> > > +        .props          = ast2400_set_props,
> > > +        .nr_gpio_pins   = 216,
> > > +        .nr_gpio_sets   = 7,
> > > +        .gap            = 196,
> > > +    }
> > > +};
> > > +
> > > +static const TypeInfo aspeed_gpio_info = {
> > > +    .name = TYPE_ASPEED_GPIO,
> > > +    .parent = TYPE_SYS_BUS_DEVICE,
> > > +    .instance_size = sizeof(AspeedGPIOState),
> > > +    .class_size     = sizeof(AspeedGPIOClass),
> > > +    .instance_init = aspeed_gpio_init,
> > > +    .abstract       = true,
> > > +};
> > > +
> > > +static void aspeed_gpio_register_types(void)
> > > +{
> > > +    int i;
> > > +
> > > +    type_register_static(&aspeed_gpio_info);
> > > +    for (i = 0; i < ARRAY_SIZE(controllers); i++) {
> > > +        TypeInfo t = {
> > > +            .name = controllers[i].name,
> > > +            .parent = TYPE_ASPEED_GPIO,
> > > +            .class_init = aspeed_gpio_class_init,
> > > +            .class_data = (void *)&controllers[i],
> > > +        };
> > > +        type_register(&t);
> > > +    };
> > 
> > I think we want separate classes for the 2400 and 2500, especially
> > with
> > the 2600 on the way.
> 
> I thought this was defining seperate classes that just happen to have
> the same init function? I don't fully grasp classes in qemu yet, so
> likely to be wrong :) 

Yeah, I'm a bit out of the loop here too, was just expecting to see explicit
typedef names for each variant and didn't. This is probably fine as is,
though someone with a better grip on it should weigh in.

Andrew


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH 2/2] aspeed: add a GPIO controller to the SoC
  2019-07-15  6:19 Rashmica Gupta
@ 2019-07-15  6:19 ` Rashmica Gupta
  0 siblings, 0 replies; 11+ messages in thread
From: Rashmica Gupta @ 2019-07-15  6:19 UTC (permalink / raw)
  To: qemu-arm; +Cc: peter.maydell, andrew, qemu-devel, clg, Rashmica Gupta, joel

Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 hw/arm/aspeed_soc.c         | 17 +++++++++++++++++
 include/hw/arm/aspeed_soc.h |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index c6fb3700f2..ff422c8ad1 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -124,6 +124,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .spis_num     = 1,
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
+        .gpio_typename = "aspeed.gpio-ast2400",
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
         .memmap       = aspeed_soc_ast2400_memmap,
@@ -136,6 +137,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .spis_num     = 1,
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
+        .gpio_typename = "aspeed.gpio-ast2400",
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
         .memmap       = aspeed_soc_ast2400_memmap,
@@ -148,6 +150,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .spis_num     = 1,
         .fmc_typename = "aspeed.smc.fmc",
         .spi_typename = aspeed_soc_ast2400_typenames,
+        .gpio_typename = "aspeed.gpio-ast2400",
         .wdts_num     = 2,
         .irqmap       = aspeed_soc_ast2400_irqmap,
         .memmap       = aspeed_soc_ast2400_memmap,
@@ -160,6 +163,7 @@ static const AspeedSoCInfo aspeed_socs[] = {
         .spis_num     = 2,
         .fmc_typename = "aspeed.smc.ast2500-fmc",
         .spi_typename = aspeed_soc_ast2500_typenames,
+        .gpio_typename = "aspeed.gpio-ast2500",
         .wdts_num     = 3,
         .irqmap       = aspeed_soc_ast2500_irqmap,
         .memmap       = aspeed_soc_ast2500_memmap,
@@ -246,6 +250,9 @@ static void aspeed_soc_init(Object *obj)
 
     sysbus_init_child_obj(obj, "xdma", OBJECT(&s->xdma), sizeof(s->xdma),
                           TYPE_ASPEED_XDMA);
+
+    sysbus_init_child_obj(obj, "gpio", OBJECT(&s->gpio), sizeof(s->gpio),
+                          sc->info->gpio_typename);
 }
 
 static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -425,6 +432,16 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
                     sc->info->memmap[ASPEED_XDMA]);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->xdma), 0,
                        aspeed_soc_get_irq(s, ASPEED_XDMA));
+
+    /* GPIO */
+    object_property_set_bool(OBJECT(&s->gpio), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->gpio), 0, sc->info->memmap[ASPEED_GPIO]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
+                       aspeed_soc_get_irq(s, ASPEED_GPIO));
 }
 static Property aspeed_soc_properties[] = {
     DEFINE_PROP_UINT32("num-cpus", AspeedSoCState, num_cpus, 0),
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index cef605ad6b..fa04abddd8 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -22,6 +22,7 @@
 #include "hw/ssi/aspeed_smc.h"
 #include "hw/watchdog/wdt_aspeed.h"
 #include "hw/net/ftgmac100.h"
+#include "hw/gpio/aspeed_gpio.h"
 
 #define ASPEED_SPIS_NUM  2
 #define ASPEED_WDTS_NUM  3
@@ -47,6 +48,7 @@ typedef struct AspeedSoCState {
     AspeedSDMCState sdmc;
     AspeedWDTState wdt[ASPEED_WDTS_NUM];
     FTGMAC100State ftgmac100[ASPEED_MACS_NUM];
+    AspeedGPIOState gpio;
 } AspeedSoCState;
 
 #define TYPE_ASPEED_SOC "aspeed-soc"
@@ -60,6 +62,7 @@ typedef struct AspeedSoCInfo {
     int spis_num;
     const char *fmc_typename;
     const char **spi_typename;
+    const char *gpio_typename;
     int wdts_num;
     const int *irqmap;
     const hwaddr *memmap;
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-07-15  6:20 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-18  8:51 [Qemu-devel] [PATCH 0/2] Add Aspeed GPIO controller model Rashmica Gupta
2019-06-18  8:51 ` [Qemu-devel] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model Rashmica Gupta
2019-06-18  9:04   ` Cédric Le Goater
2019-07-03  4:16   ` Andrew Jeffery
2019-07-15  2:36     ` Rashmica Gupta
2019-07-15  2:43       ` Andrew Jeffery
2019-06-18  8:51 ` [Qemu-devel] [PATCH 2/2] aspeed: add a GPIO controller to the SoC Rashmica Gupta
2019-06-18  9:21   ` Cédric Le Goater
2019-06-19  0:19     ` Rashmica Gupta
2019-06-18  9:24 ` [Qemu-devel] [PATCH 0/2] Add Aspeed GPIO controller model Cédric Le Goater
2019-07-15  6:19 Rashmica Gupta
2019-07-15  6:19 ` [Qemu-devel] [PATCH 2/2] aspeed: add a GPIO controller to the SoC Rashmica Gupta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).