Re: [PATCH v1 1/1] hw/gpio: Add ASPEED GPIO model for AST1030

qemu-arm

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1 1/1] hw/gpio: Add ASPEED GPIO model for AST1030

From:	Cédric Le Goater
Subject:	Re: [PATCH v1 1/1] hw/gpio: Add ASPEED GPIO model for AST1030
Date:	Wed, 11 May 2022 08:14:55 +0200
User-agent:	Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0

Hello Jamin,

(Adding a few people that could help with the review)

On 3/21/22 10:14, Jamin Lin wrote:

1. Add GPIO read/write trace event.


Do we really need the "DEVICE(s)->canonical_path" parameter ?
That would be patch 1.

2. Support GPIO index mode for write operation.
It did not support GPIO index mode for read operation.


these changes would be in patch 2.

3. AST1030 integrates one set of Parallel GPIO Controller
with maximum 151 control pins, which are 21 groups
(A~U, exclude pin: M6 M7 Q5 Q6 Q7 R0 R1 R4 R5 R6 R7 S0 S3 S4
S5 S6 S7 ) and the group T and U are input only.


and a last patch 3.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>



Some minor comments below,

Thanks,

C.

---
  hw/gpio/aspeed_gpio.c         | 250 ++++++++++++++++++++++++++++++++--
  hw/gpio/trace-events          |   5 +
  include/hw/gpio/aspeed_gpio.h |  16 ++-
  3 files changed, 255 insertions(+), 16 deletions(-)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index c63634d3d3..3f0bd036b7 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -15,6 +15,8 @@
  #include "qapi/visitor.h"
  #include "hw/irq.h"
  #include "migration/vmstate.h"
+#include "trace.h"
+#include "hw/registerfields.h"

#define GPIOS_PER_GROUP 8@@ -203,6 +205,28 @@

  #define GPIO_1_8V_MEM_SIZE            0x1D8
  #define GPIO_1_8V_REG_ARRAY_SIZE      (GPIO_1_8V_MEM_SIZE >> 2)

+/*

+ * GPIO index mode support
+ * It only supports write operation
+ */
+REG32(GPIO_INDEX_REG, 0x2AC)
+    FIELD(GPIO_INDEX_REG, NUMBER, 0, 8)
+    FIELD(GPIO_INDEX_REG, COMMAND, 12, 1)
+    FIELD(GPIO_INDEX_REG, TYPE, 16, 4)
+    FIELD(GPIO_INDEX_REG, DATA_VALUE, 20, 1)
+    FIELD(GPIO_INDEX_REG, DIRECTION, 20, 1)
+    FIELD(GPIO_INDEX_REG, INT_ENABLE, 20, 1)
+    FIELD(GPIO_INDEX_REG, INT_SENS_0, 21, 1)
+    FIELD(GPIO_INDEX_REG, INT_SENS_1, 22, 1)
+    FIELD(GPIO_INDEX_REG, INT_SENS_2, 23, 1)
+    FIELD(GPIO_INDEX_REG, INT_STATUS, 24, 1)
+    FIELD(GPIO_INDEX_REG, DEBOUNCE_1, 20, 1)
+    FIELD(GPIO_INDEX_REG, DEBOUNCE_2, 21, 1)
+    FIELD(GPIO_INDEX_REG, RESET_TOLERANT, 20, 1)
+    FIELD(GPIO_INDEX_REG, COMMAND_SRC_0, 20, 1)
+    FIELD(GPIO_INDEX_REG, COMMAND_SRC_1, 21, 1)
+    FIELD(GPIO_INDEX_REG, INPUT_MASK, 20, 1)


That's a good idea. We should start switching the models to the registerfields
interface.

  static int aspeed_evaluate_irq(GPIOSets *regs, int gpio_prev_high, int gpio)
  {
      uint32_t falling_edge = 0, rising_edge = 0;
@@ -523,11 +547,16 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr 
offset, uint32_t size)
      uint64_t idx = -1;
      const AspeedGPIOReg *reg;
      GPIOSets *set;
+    uint32_t value = 0;
+    uint64_t debounce_value;

idx = offset >> 2;

      if (idx >= GPIO_DEBOUNCE_TIME_1 && idx <= GPIO_DEBOUNCE_TIME_3) {
          idx -= GPIO_DEBOUNCE_TIME_1;
-        return (uint64_t) s->debounce_regs[idx];
+        debounce_value = (uint64_t) s->debounce_regs[idx];
+        trace_aspeed_gpio_read(DEVICE(s)->canonical_path,
+                               offset, debounce_value);
+        return debounce_value;
      }

reg = &agc->reg_table[idx];

@@ -540,38 +569,193 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr 
offset, uint32_t size)
      set = &s->sets[reg->set_idx];
      switch (reg->type) {
      case gpio_reg_data_value:
-        return set->data_value;
+         value = set->data_value;
+         break;
      case gpio_reg_direction:
-        return set->direction;
+        value = set->direction;
+        break;
      case gpio_reg_int_enable:
-        return set->int_enable;
+        value = set->int_enable;
+        break;
      case gpio_reg_int_sens_0:
-        return set->int_sens_0;
+        value = set->int_sens_0;
+        break;
      case gpio_reg_int_sens_1:
-        return set->int_sens_1;
+        value = set->int_sens_1;
+        break;
      case gpio_reg_int_sens_2:
-        return set->int_sens_2;
+        value = set->int_sens_2;
+        break;
      case gpio_reg_int_status:
-        return set->int_status;
+        value = set->int_status;
+        break;
      case gpio_reg_reset_tolerant:
-        return set->reset_tol;
+        value = set->reset_tol;
+        break;
      case gpio_reg_debounce_1:
-        return set->debounce_1;
+        value = set->debounce_1;
+        break;
      case gpio_reg_debounce_2:
-        return set->debounce_2;
+        value = set->debounce_2;
+        break;
      case gpio_reg_cmd_source_0:
-        return set->cmd_source_0;
+        value = set->cmd_source_0;
+        break;
      case gpio_reg_cmd_source_1:
-        return set->cmd_source_1;
+        value = set->cmd_source_1;
+        break;
      case gpio_reg_data_read:
-        return set->data_read;
+        value = set->data_read;
+        break;
      case gpio_reg_input_mask:
-        return set->input_mask;
+        value = set->input_mask;
+        break;
      default:
          qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset 0x%"
                        HWADDR_PRIx"\n", __func__, offset);
          return 0;
      }
+
+    trace_aspeed_gpio_read(DEVICE(s)->canonical_path, offset, value);
+    return value;
+}
+
+static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
+                                                uint64_t data, uint32_t size)
+{
+
+    AspeedGPIOState *s = ASPEED_GPIO(opaque);
+    AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
+    const GPIOSetProperties *props;
+    GPIOSets *set;
+    uint32_t reg_idx_number = FIELD_EX32(data, GPIO_INDEX_REG, NUMBER);
+    uint32_t reg_idx_type = FIELD_EX32(data, GPIO_INDEX_REG, TYPE);
+    uint32_t reg_idx_command = FIELD_EX32(data, GPIO_INDEX_REG, COMMAND);
+    uint32_t set_idx = reg_idx_number / ASPEED_GPIOS_PER_SET;
+    uint32_t pin_idx = reg_idx_number % ASPEED_GPIOS_PER_SET;
+    uint32_t group_idx = pin_idx / GPIOS_PER_GROUP;
+    uint32_t reg_value = 0;
+    uint32_t cleared;
+
+    set = &s->sets[set_idx];
+    props = &agc->props[set_idx];
+
+    if (reg_idx_command)
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%" HWADDR_PRIx "data 0x%"
+            HWADDR_PRIx "index mode wrong command 0x%x\n",


The format should use PRIx64 for the data

+            __func__, offset, data, reg_idx_command);
+
+    switch (reg_idx_type) {
+    case gpio_reg_idx_data:
+        reg_value = set->data_read;
+        reg_value = deposit32(reg_value, pin_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, DATA_VALUE));
+        reg_value &= props->output;
+        reg_value = update_value_control_source(set, set->data_value,
+                                                reg_value);
+        set->data_read = reg_value;
+        aspeed_gpio_update(s, set, reg_value);
+        return;
+    case gpio_reg_idx_direction:
+        reg_value = set->direction;
+        reg_value = deposit32(reg_value, pin_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, DIRECTION));
+        /*
+         *   where data is the value attempted to be written to the pin:
+         *    pin type      | input mask | output mask | expected value
+         *    ------------------------------------------------------------
+         *   bidirectional  |   1       |   1        |  data
+         *   input only     |   1       |   0        |   0
+         *   output only    |   0       |   1        |   1
+         *   no pin         |   0       |   0        |   0
+         *
+         *  which is captured by:
+         *  data = ( data | ~input) & output;
+         */
+        reg_value = (reg_value | ~props->input) & props->output;
+        set->direction = update_value_control_source(set, set->direction,
+                                                     reg_value);
+        break;
+    case gpio_reg_idx_interrupt:
+        reg_value = set->int_enable;
+        reg_value = deposit32(reg_value, pin_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, INT_ENABLE));
+        set->int_enable = update_value_control_source(set, set->int_enable,
+                                                      reg_value);
+        reg_value = set->int_sens_0;
+        reg_value = deposit32(reg_value, pin_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, INT_SENS_0));
+        set->int_sens_0 = update_value_control_source(set, set->int_sens_0,
+                                                      reg_value);
+        reg_value = set->int_sens_1;
+        reg_value = deposit32(reg_value, pin_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, INT_SENS_1));
+        set->int_sens_1 = update_value_control_source(set, set->int_sens_1,
+                                                      reg_value);
+        reg_value = set->int_sens_2;
+        reg_value = deposit32(reg_value, pin_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, INT_SENS_2));
+        set->int_sens_2 = update_value_control_source(set, set->int_sens_2,
+                                                      reg_value);
+        /* set interrupt status */
+        reg_value = set->int_status;
+        reg_value = deposit32(reg_value, pin_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, INT_STATUS));
+        cleared = ctpop32(reg_value & set->int_status);
+        if (s->pending && cleared) {
+            assert(s->pending >= cleared);
+            s->pending -= cleared;
+        }
+        set->int_status &= ~reg_value;
+        break;
+    case gpio_reg_idx_debounce:
+        reg_value = set->debounce_1;
+        reg_value = deposit32(reg_value, pin_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, DEBOUNCE_1));
+        set->debounce_1 = update_value_control_source(set, set->debounce_1,
+                                                      reg_value);
+        reg_value = set->debounce_2;
+        reg_value = deposit32(reg_value, pin_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, DEBOUNCE_2));
+        set->debounce_2 = update_value_control_source(set, set->debounce_2,
+                                                      reg_value);
+        return;
+    case gpio_reg_idx_tolerance:
+        reg_value = set->reset_tol;
+        reg_value = deposit32(reg_value, pin_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, 
RESET_TOLERANT));
+        set->reset_tol = update_value_control_source(set, set->reset_tol,
+                                                     reg_value);
+        return;
+    case gpio_reg_idx_cmd_src:
+        reg_value = set->cmd_source_0;
+        reg_value = deposit32(reg_value, GPIOS_PER_GROUP * group_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, COMMAND_SRC_0));
+        set->cmd_source_0 = reg_value & ASPEED_CMD_SRC_MASK;
+        reg_value = set->cmd_source_1;
+        reg_value = deposit32(reg_value, GPIOS_PER_GROUP * group_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, COMMAND_SRC_1));
+        set->cmd_source_1 = reg_value & ASPEED_CMD_SRC_MASK;
+        return;
+    case gpio_reg_idx_input_mask:
+        reg_value = set->input_mask;
+        reg_value = deposit32(reg_value, pin_idx, 1,
+                              FIELD_EX32(data, GPIO_INDEX_REG, INPUT_MASK));
+        /*
+         * feeds into interrupt generation
+         * 0: read from data value reg will be updated
+         * 1: read from data value reg will not be updated
+         */
+        set->input_mask = reg_value & props->input;
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: offset 0x%" HWADDR_PRIx "data 0x%"
+            HWADDR_PRIx "index mode wrong type 0x%x\n",
+            __func__, offset, data, reg_idx_type);
+        return;
+    }
+    aspeed_gpio_update(s, set, set->data_value);
+    return;
  }

static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,

@@ -585,7 +769,16 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, 
uint64_t data,
      GPIOSets *set;
      uint32_t cleared;

+ trace_aspeed_gpio_write(DEVICE(s)->canonical_path, offset, data);

+
      idx = offset >> 2;
+
+    /* check gpio index mode */
+    if (idx == R_GPIO_INDEX_REG) {
+        aspeed_gpio_write_index_mode(opaque, offset, data, size);
+        return;
+    }
+
      if (idx >= GPIO_DEBOUNCE_TIME_1 && idx <= GPIO_DEBOUNCE_TIME_3) {
          idx -= GPIO_DEBOUNCE_TIME_1;
          s->debounce_regs[idx] = (uint32_t) data;
@@ -795,6 +988,15 @@ static GPIOSetProperties 
ast2600_1_8v_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
      [1] = {0x0000000f,  0x0000000f,  {"18E"} },
  };

+static GPIOSetProperties ast1030_set_props[ASPEED_GPIO_MAX_NR_SETS] = {

+    [0] = {0xffffffff,  0xffffffff,  {"A", "B", "C", "D"} },
+    [1] = {0xffffffff,  0xffffffff,  {"E", "F", "G", "H"} },
+    [2] = {0xffffffff,  0xffffffff,  {"I", "J", "K", "L"} },
+    [3] = {0xffffff3f,  0xffffff3f,  {"M", "N", "O", "P"} },
+    [4] = {0xff060c1f,  0x00060c1f,  {"Q", "R", "S", "T"} },
+    [5] = {0x000000ff,  0x00000000,  {"U"} },
+};
+
  static const MemoryRegionOps aspeed_gpio_ops = {
      .read       = aspeed_gpio_read,
      .write      = aspeed_gpio_write,
@@ -947,6 +1149,16 @@ static void 
aspeed_gpio_ast2600_1_8v_class_init(ObjectClass *klass, void *data)
      agc->reg_table = aspeed_1_8v_gpios;
  }

+static void aspeed_gpio_1030_class_init(ObjectClass *klass, void *data)

+{
+    AspeedGPIOClass *agc = ASPEED_GPIO_CLASS(klass);
+
+    agc->props = ast1030_set_props;
+    agc->nr_gpio_pins = 151;
+    agc->nr_gpio_sets = 6;
+    agc->reg_table = aspeed_3_3v_gpios;
+}
+
  static const TypeInfo aspeed_gpio_info = {
      .name           = TYPE_ASPEED_GPIO,
      .parent         = TYPE_SYS_BUS_DEVICE,
@@ -984,6 +1196,13 @@ static const TypeInfo aspeed_gpio_ast2600_1_8v_info = {
      .instance_init  = aspeed_gpio_init,
  };

+static const TypeInfo aspeed_gpio_ast1030_info = {

+    .name           = TYPE_ASPEED_GPIO "-ast1030",
+    .parent         = TYPE_ASPEED_GPIO,
+    .class_init     = aspeed_gpio_1030_class_init,
+    .instance_init  = aspeed_gpio_init,
+};
+
  static void aspeed_gpio_register_types(void)
  {
      type_register_static(&aspeed_gpio_info);
@@ -991,6 +1210,7 @@ static void aspeed_gpio_register_types(void)
      type_register_static(&aspeed_gpio_ast2500_info);
      type_register_static(&aspeed_gpio_ast2600_3_3v_info);
      type_register_static(&aspeed_gpio_ast2600_1_8v_info);
+    type_register_static(&aspeed_gpio_ast1030_info);
  }

type_init(aspeed_gpio_register_types);

diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
index 1dab99c560..717d6cf7cc 100644
--- a/hw/gpio/trace-events
+++ b/hw/gpio/trace-events
@@ -27,3 +27,8 @@ sifive_gpio_read(uint64_t offset, uint64_t r) "offset 0x%" PRIx64 
" value 0x%" P
  sifive_gpio_write(uint64_t offset, uint64_t value) "offset 0x%" PRIx64 " value 
0x%" PRIx64
  sifive_gpio_set(int64_t line, int64_t value) "line %" PRIi64 " value %" PRIi64
  sifive_gpio_update_output_irq(int64_t line, int64_t value) "line %" PRIi64 " value 
%" PRIi64
+
+# aspeed_gpio.c
+aspeed_gpio_read(const char *id, uint64_t offset, uint64_t value) " %s offset: 0x%04" 
PRIx64 " value 0x%08" PRIx64
+aspeed_gpio_write(const char *id, uint64_t offset, uint64_t value) "%s offset: 0x%04" 
PRIx64 " value 0x%08" PRIx64
+
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
index 801846befb..cb7671149f 100644
--- a/include/hw/gpio/aspeed_gpio.h
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -50,10 +50,24 @@ enum GPIORegType {
      gpio_reg_input_mask,
  };

+/* GPIO index mode */

+enum GPIORegIndexType {
+    gpio_reg_idx_data = 0,
+    gpio_reg_idx_direction,
+    gpio_reg_idx_interrupt,
+    gpio_reg_idx_debounce,
+    gpio_reg_idx_tolerance,
+    gpio_reg_idx_cmd_src,
+    gpio_reg_idx_input_mask,
+    gpio_reg_idx_reserved,
+    gpio_reg_idx_new_w_cmd_src,
+    gpio_reg_idx_new_r_cmd_src,
+};
+
  typedef struct AspeedGPIOReg {
      uint16_t set_idx;
      enum GPIORegType type;
- } AspeedGPIOReg;
+} AspeedGPIOReg;

struct AspeedGPIOClass {

      SysBusDevice parent_obj;

[Prev in Thread]

Current Thread

[Next in Thread]

Re: [PATCH v1 1/1] hw/gpio: Add ASPEED GPIO model for AST1030, Cédric Le Goater <=
- Re: [PATCH v1 1/1] hw/gpio: Add ASPEED GPIO model for AST1030, Jamin Lin, 2022/05/25

Prev by Date: Re: [PATCH] hw/adc/zynq-xadc: Use qemu_irq typedef
Next by Date: Re: [PATCH 1/1] docs: aspeed: Add fby35 board
Previous by thread: QEMU 32-bit vs. 64-bit binaries (was: [PATCH] mos6522: fix linking error when CONFIG_MOS6522 is not set)
Next by thread: Re: [PATCH v1 1/1] hw/gpio: Add ASPEED GPIO model for AST1030
Index(es):
- Date
- Thread