* [PATCH v6 1/8] hw/gpio/aspeed: Fix coding style
2024-09-30 8:52 [PATCH v6 0/8] Support GPIO for AST2700 Jamin Lin via
@ 2024-09-30 8:52 ` Jamin Lin via
2024-09-30 8:52 ` [PATCH v6 2/8] hw/gpio/aspeed: Support to set the different memory size Jamin Lin via
` (6 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Jamin Lin via @ 2024-09-30 8:52 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Andrew Jeffery, Joel Stanley, Thomas Huth, Laurent Vivier,
Paolo Bonzini, open list:ASPEED BMCs,
open list:All patches CC here
Cc: jamin_lin, troy_lee, yunlin.tang, Cédric Le Goater
Fix coding style issues from checkpatch.pl
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
hw/gpio/aspeed_gpio.c | 6 +++---
include/hw/gpio/aspeed_gpio.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 71756664dd..00fb72a509 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -340,7 +340,8 @@ static void aspeed_gpio_set_pin_level(AspeedGPIOState *s, uint32_t set_idx,
value &= ~pin_mask;
}
- aspeed_gpio_update(s, &s->sets[set_idx], value, ~s->sets[set_idx].direction);
+ aspeed_gpio_update(s, &s->sets[set_idx], value,
+ ~s->sets[set_idx].direction);
}
/*
@@ -629,7 +630,6 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size)
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;
@@ -963,7 +963,7 @@ static void aspeed_gpio_set_pin(Object *obj, Visitor *v, const char *name,
aspeed_gpio_set_pin_level(s, set_idx, pin, level);
}
-/****************** Setup functions ******************/
+/* Setup functions */
static const GPIOSetProperties ast2400_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
[0] = {0xffffffff, 0xffffffff, {"A", "B", "C", "D"} },
[1] = {0xffffffff, 0xffffffff, {"E", "F", "G", "H"} },
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
index 90a12ae318..39febda9ea 100644
--- a/include/hw/gpio/aspeed_gpio.h
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -88,7 +88,7 @@ struct AspeedGPIOState {
qemu_irq irq;
qemu_irq gpios[ASPEED_GPIO_MAX_NR_SETS][ASPEED_GPIOS_PER_SET];
-/* Parallel GPIO Registers */
+ /* Parallel GPIO Registers */
uint32_t debounce_regs[ASPEED_GPIO_NR_DEBOUNCE_REGS];
struct GPIOSets {
uint32_t data_value; /* Reflects pin values */
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 2/8] hw/gpio/aspeed: Support to set the different memory size
2024-09-30 8:52 [PATCH v6 0/8] Support GPIO for AST2700 Jamin Lin via
2024-09-30 8:52 ` [PATCH v6 1/8] hw/gpio/aspeed: Fix coding style Jamin Lin via
@ 2024-09-30 8:52 ` Jamin Lin via
2024-09-30 8:52 ` [PATCH v6 3/8] hw/gpio/aspeed: Support different memory region ops Jamin Lin via
` (5 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Jamin Lin via @ 2024-09-30 8:52 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Andrew Jeffery, Joel Stanley, Thomas Huth, Laurent Vivier,
Paolo Bonzini, open list:ASPEED BMCs,
open list:All patches CC here
Cc: jamin_lin, troy_lee, yunlin.tang, Cédric Le Goater
According to the datasheet of ASPEED SOCs, a GPIO controller owns 4KB of
register space for AST2700, AST2500, AST2400 and AST1030; owns 2KB of
register space for AST2600 1.8v and owns 2KB of register space for
AST2600 3.3v.
It set the memory region size 2KB by default and it does not compatible
register space for AST2700.
Introduce a new class attribute to set the GPIO controller memory size
for different ASPEED SOCs.
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
hw/gpio/aspeed_gpio.c | 7 ++++++-
include/hw/gpio/aspeed_gpio.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 00fb72a509..564459ad4f 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -1047,7 +1047,7 @@ static void aspeed_gpio_realize(DeviceState *dev, Error **errp)
}
memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s,
- TYPE_ASPEED_GPIO, 0x800);
+ TYPE_ASPEED_GPIO, agc->mem_size);
sysbus_init_mmio(sbd, &s->iomem);
}
@@ -1130,6 +1130,7 @@ static void aspeed_gpio_ast2400_class_init(ObjectClass *klass, void *data)
agc->nr_gpio_sets = 7;
agc->reg_table = aspeed_3_3v_gpios;
agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
+ agc->mem_size = 0x1000;
}
static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
@@ -1141,6 +1142,7 @@ static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
agc->nr_gpio_sets = 8;
agc->reg_table = aspeed_3_3v_gpios;
agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
+ agc->mem_size = 0x1000;
}
static void aspeed_gpio_ast2600_3_3v_class_init(ObjectClass *klass, void *data)
@@ -1152,6 +1154,7 @@ static void aspeed_gpio_ast2600_3_3v_class_init(ObjectClass *klass, void *data)
agc->nr_gpio_sets = 7;
agc->reg_table = aspeed_3_3v_gpios;
agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
+ agc->mem_size = 0x800;
}
static void aspeed_gpio_ast2600_1_8v_class_init(ObjectClass *klass, void *data)
@@ -1163,6 +1166,7 @@ static void aspeed_gpio_ast2600_1_8v_class_init(ObjectClass *klass, void *data)
agc->nr_gpio_sets = 2;
agc->reg_table = aspeed_1_8v_gpios;
agc->reg_table_count = GPIO_1_8V_REG_ARRAY_SIZE;
+ agc->mem_size = 0x800;
}
static void aspeed_gpio_1030_class_init(ObjectClass *klass, void *data)
@@ -1174,6 +1178,7 @@ static void aspeed_gpio_1030_class_init(ObjectClass *klass, void *data)
agc->nr_gpio_sets = 6;
agc->reg_table = aspeed_3_3v_gpios;
agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
+ agc->mem_size = 0x1000;
}
static const TypeInfo aspeed_gpio_info = {
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
index 39febda9ea..8cd2ff5496 100644
--- a/include/hw/gpio/aspeed_gpio.h
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -76,6 +76,7 @@ struct AspeedGPIOClass {
uint32_t nr_gpio_sets;
const AspeedGPIOReg *reg_table;
unsigned reg_table_count;
+ uint64_t mem_size;
};
struct AspeedGPIOState {
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 3/8] hw/gpio/aspeed: Support different memory region ops
2024-09-30 8:52 [PATCH v6 0/8] Support GPIO for AST2700 Jamin Lin via
2024-09-30 8:52 ` [PATCH v6 1/8] hw/gpio/aspeed: Fix coding style Jamin Lin via
2024-09-30 8:52 ` [PATCH v6 2/8] hw/gpio/aspeed: Support to set the different memory size Jamin Lin via
@ 2024-09-30 8:52 ` Jamin Lin via
2024-09-30 8:52 ` [PATCH v6 4/8] hw/gpio/aspeed: Fix clear incorrect interrupt status for GPIO index mode Jamin Lin via
` (4 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Jamin Lin via @ 2024-09-30 8:52 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Andrew Jeffery, Joel Stanley, Thomas Huth, Laurent Vivier,
Paolo Bonzini, open list:ASPEED BMCs,
open list:All patches CC here
Cc: jamin_lin, troy_lee, yunlin.tang, Cédric Le Goater
It set "aspeed_gpio_ops" struct which containing read and write callbacks
to be used when I/O is performed on the GPIO region.
Besides, in the previous design of ASPEED SOCs, one register is used for
setting one function for 32 GPIO pins.
ex: GPIO000 is used for setting data value for GPIO A, B, C and D in AST2600.
ex: GPIO004 is used for setting direction for GPIO A, B, C and D in AST2600.
However, the register set have a significant change in AST2700.
Each GPIO pin has their own control register. In other words, users are able to
set one GPIO pin’s direction, interrupt enable, input mask and so on
in one register. The aspeed_gpio_read/aspeed_gpio_write callback functions
are not compatible AST2700.
Introduce a new "const MemoryRegionOps *" attribute in AspeedGPIOClass and
use it in aspeed_gpio_realize function.
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
hw/gpio/aspeed_gpio.c | 7 ++++++-
include/hw/gpio/aspeed_gpio.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 564459ad4f..8725606aec 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -1046,7 +1046,7 @@ static void aspeed_gpio_realize(DeviceState *dev, Error **errp)
}
}
- memory_region_init_io(&s->iomem, OBJECT(s), &aspeed_gpio_ops, s,
+ memory_region_init_io(&s->iomem, OBJECT(s), agc->reg_ops, s,
TYPE_ASPEED_GPIO, agc->mem_size);
sysbus_init_mmio(sbd, &s->iomem);
@@ -1131,6 +1131,7 @@ static void aspeed_gpio_ast2400_class_init(ObjectClass *klass, void *data)
agc->reg_table = aspeed_3_3v_gpios;
agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
agc->mem_size = 0x1000;
+ agc->reg_ops = &aspeed_gpio_ops;
}
static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
@@ -1143,6 +1144,7 @@ static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
agc->reg_table = aspeed_3_3v_gpios;
agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
agc->mem_size = 0x1000;
+ agc->reg_ops = &aspeed_gpio_ops;
}
static void aspeed_gpio_ast2600_3_3v_class_init(ObjectClass *klass, void *data)
@@ -1155,6 +1157,7 @@ static void aspeed_gpio_ast2600_3_3v_class_init(ObjectClass *klass, void *data)
agc->reg_table = aspeed_3_3v_gpios;
agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
agc->mem_size = 0x800;
+ agc->reg_ops = &aspeed_gpio_ops;
}
static void aspeed_gpio_ast2600_1_8v_class_init(ObjectClass *klass, void *data)
@@ -1167,6 +1170,7 @@ static void aspeed_gpio_ast2600_1_8v_class_init(ObjectClass *klass, void *data)
agc->reg_table = aspeed_1_8v_gpios;
agc->reg_table_count = GPIO_1_8V_REG_ARRAY_SIZE;
agc->mem_size = 0x800;
+ agc->reg_ops = &aspeed_gpio_ops;
}
static void aspeed_gpio_1030_class_init(ObjectClass *klass, void *data)
@@ -1179,6 +1183,7 @@ static void aspeed_gpio_1030_class_init(ObjectClass *klass, void *data)
agc->reg_table = aspeed_3_3v_gpios;
agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
agc->mem_size = 0x1000;
+ agc->reg_ops = &aspeed_gpio_ops;
}
static const TypeInfo aspeed_gpio_info = {
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
index 8cd2ff5496..e1e6c54333 100644
--- a/include/hw/gpio/aspeed_gpio.h
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -77,6 +77,7 @@ struct AspeedGPIOClass {
const AspeedGPIOReg *reg_table;
unsigned reg_table_count;
uint64_t mem_size;
+ const MemoryRegionOps *reg_ops;
};
struct AspeedGPIOState {
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 4/8] hw/gpio/aspeed: Fix clear incorrect interrupt status for GPIO index mode
2024-09-30 8:52 [PATCH v6 0/8] Support GPIO for AST2700 Jamin Lin via
` (2 preceding siblings ...)
2024-09-30 8:52 ` [PATCH v6 3/8] hw/gpio/aspeed: Support different memory region ops Jamin Lin via
@ 2024-09-30 8:52 ` Jamin Lin via
2024-09-30 8:52 ` [PATCH v6 5/8] hw/gpio/aspeed: Add AST2700 support Jamin Lin via
` (3 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Jamin Lin via @ 2024-09-30 8:52 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Andrew Jeffery, Joel Stanley, Thomas Huth, Laurent Vivier,
Paolo Bonzini, open list:ASPEED BMCs,
open list:All patches CC here
Cc: jamin_lin, troy_lee, yunlin.tang
The interrupt status field is W1C, where a set bit on read indicates an
interrupt is pending. If the bit extracted from data is set it should
clear the corresponding bit in reg_value. However, if the extracted
bit is clear then the value of the corresponding bit in reg_value
should be unchanged.
SHARED_FIELD_EX32() extracts the interrupt status bit from the write
(data). reg_value is set to the set's interrupt status, which means
that for any pin with an interrupt pending, the corresponding bit is
set. The deposit32() call updates the bit at pin_idx in the
reg_value, using the value extracted from the write (data).
The result is that if multiple interrupt status bits
were pending and the write was acknowledging specific one bit,
then the all interrupt status bits will be cleared.
However, it is index mode and should only clear the corresponding bit.
For example, say we have an interrupt pending for GPIOA0, where the
following statements are true:
set->int_status == 0b01
s->pending == 1
Before it is acknowledged, an interrupt becomes pending for GPIOA1:
set->int_status == 0b11
s->pending == 2
A write is issued to acknowledge the interrupt for GPIOA0. This causes
the following sequence:
reg_value == 0b11
pending == 2
s->pending == 0
set->int_status == 0b00
It should only clear bit 0 in index mode and the correct result
should be as following.
set->int_status == 0b11
s->pending == 2
pending == 1
s->pending == 1
set->int_status == 0b10
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Suggested-by: Andrew Jeffery <andrew@codeconstruct.com.au>
Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
hw/gpio/aspeed_gpio.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 8725606aec..16c18ea2f7 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -641,7 +641,7 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
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;
+ uint32_t pending = 0;
set = &s->sets[set_idx];
props = &agc->props[set_idx];
@@ -703,16 +703,23 @@ static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset,
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;
+ /* interrupt status */
+ if (FIELD_EX32(data, GPIO_INDEX_REG, INT_STATUS)) {
+ /* pending is either 1 or 0 for a 1-bit field */
+ pending = extract32(set->int_status, pin_idx, 1);
+
+ assert(s->pending >= pending);
+
+ /* No change to s->pending if pending is 0 */
+ s->pending -= pending;
+
+ /*
+ * The write acknowledged the interrupt regardless of whether it
+ * was pending or not. The post-condition is that it mustn't be
+ * pending. Unconditionally clear the status bit.
+ */
+ set->int_status = deposit32(set->int_status, pin_idx, 1, 0);
}
- set->int_status &= ~reg_value;
break;
case gpio_reg_idx_debounce:
reg_value = set->debounce_1;
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 5/8] hw/gpio/aspeed: Add AST2700 support
2024-09-30 8:52 [PATCH v6 0/8] Support GPIO for AST2700 Jamin Lin via
` (3 preceding siblings ...)
2024-09-30 8:52 ` [PATCH v6 4/8] hw/gpio/aspeed: Fix clear incorrect interrupt status for GPIO index mode Jamin Lin via
@ 2024-09-30 8:52 ` Jamin Lin via
2024-09-30 8:52 ` [PATCH v6 6/8] aspeed/soc: Correct GPIO irq 130 for AST2700 Jamin Lin via
` (2 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Jamin Lin via @ 2024-09-30 8:52 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Andrew Jeffery, Joel Stanley, Thomas Huth, Laurent Vivier,
Paolo Bonzini, open list:ASPEED BMCs,
open list:All patches CC here
Cc: jamin_lin, troy_lee, yunlin.tang
AST2700 integrates two set of Parallel GPIO Controller with maximum 212
control pins, which are 27 groups. (H, exclude pin: H7 H6 H5 H4)
In the previous design of ASPEED SOCs, one register is used for setting
one function for one set which are 32 pins and 4 groups.
ex: GPIO000 is used for setting data value for GPIO A, B, C and D in AST2600.
ex: GPIO004 is used for setting direction for GPIO A, B, C and D in AST2600.
However, the register set have a significant change since AST2700.
Each GPIO pin has their own individual control register.
In other words, users are able to set one GPIO pin’s direction,
interrupt enable, input mask and so on in the same one register.
Currently, aspeed_gpio_read and aspeed_gpio_write callback functions
are not compatible AST2700.
Introduce new aspeed_gpio_2700_read and aspeed_gpio_2700_write callback
functions and aspeed_gpio_2700_ops memory region operation for AST2700.
Introduce a new ast2700 class to support AST2700.
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
hw/gpio/aspeed_gpio.c | 380 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 380 insertions(+)
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 16c18ea2f7..a5b3f454e8 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -227,6 +227,38 @@ REG32(GPIO_INDEX_REG, 0x2AC)
FIELD(GPIO_INDEX_REG, COMMAND_SRC_1, 21, 1)
FIELD(GPIO_INDEX_REG, INPUT_MASK, 20, 1)
+/* AST2700 GPIO Register Address Offsets */
+REG32(GPIO_2700_DEBOUNCE_TIME_1, 0x000)
+REG32(GPIO_2700_DEBOUNCE_TIME_2, 0x004)
+REG32(GPIO_2700_DEBOUNCE_TIME_3, 0x008)
+REG32(GPIO_2700_INT_STATUS_1, 0x100)
+REG32(GPIO_2700_INT_STATUS_2, 0x104)
+REG32(GPIO_2700_INT_STATUS_3, 0x108)
+REG32(GPIO_2700_INT_STATUS_4, 0x10C)
+REG32(GPIO_2700_INT_STATUS_5, 0x110)
+REG32(GPIO_2700_INT_STATUS_6, 0x114)
+REG32(GPIO_2700_INT_STATUS_7, 0x118)
+/* GPIOA0 - GPIOAA7 Control Register */
+REG32(GPIO_A0_CONTROL, 0x180)
+ SHARED_FIELD(GPIO_CONTROL_OUT_DATA, 0, 1)
+ SHARED_FIELD(GPIO_CONTROL_DIRECTION, 1, 1)
+ SHARED_FIELD(GPIO_CONTROL_INT_ENABLE, 2, 1)
+ SHARED_FIELD(GPIO_CONTROL_INT_SENS_0, 3, 1)
+ SHARED_FIELD(GPIO_CONTROL_INT_SENS_1, 4, 1)
+ SHARED_FIELD(GPIO_CONTROL_INT_SENS_2, 5, 1)
+ SHARED_FIELD(GPIO_CONTROL_RESET_TOLERANCE, 6, 1)
+ SHARED_FIELD(GPIO_CONTROL_DEBOUNCE_1, 7, 1)
+ SHARED_FIELD(GPIO_CONTROL_DEBOUNCE_2, 8, 1)
+ SHARED_FIELD(GPIO_CONTROL_INPUT_MASK, 9, 1)
+ SHARED_FIELD(GPIO_CONTROL_BLINK_COUNTER_1, 10, 1)
+ SHARED_FIELD(GPIO_CONTROL_BLINK_COUNTER_2, 11, 1)
+ SHARED_FIELD(GPIO_CONTROL_INT_STATUS, 12, 1)
+ SHARED_FIELD(GPIO_CONTROL_IN_DATA, 13, 1)
+ SHARED_FIELD(GPIO_CONTROL_RESERVED, 14, 18)
+REG32(GPIO_AA7_CONTROL, 0x4DC)
+#define GPIO_2700_MEM_SIZE 0x4E0
+#define GPIO_2700_REG_ARRAY_SIZE (GPIO_2700_MEM_SIZE >> 2)
+
static int aspeed_evaluate_irq(GPIOSets *regs, int gpio_prev_high, int gpio)
{
uint32_t falling_edge = 0, rising_edge = 0;
@@ -970,6 +1002,316 @@ static void aspeed_gpio_set_pin(Object *obj, Visitor *v, const char *name,
aspeed_gpio_set_pin_level(s, set_idx, pin, level);
}
+static uint64_t aspeed_gpio_2700_read_control_reg(AspeedGPIOState *s,
+ uint32_t pin)
+{
+ AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
+ GPIOSets *set;
+ uint64_t value = 0;
+ uint32_t set_idx;
+ uint32_t pin_idx;
+
+ set_idx = pin / ASPEED_GPIOS_PER_SET;
+ pin_idx = pin % ASPEED_GPIOS_PER_SET;
+
+ if (set_idx >= agc->nr_gpio_sets) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: set index: %d, out of bounds\n",
+ __func__, set_idx);
+ return 0;
+ }
+
+ set = &s->sets[set_idx];
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_OUT_DATA,
+ extract32(set->data_read, pin_idx, 1));
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_DIRECTION,
+ extract32(set->direction, pin_idx, 1));
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_INT_ENABLE,
+ extract32(set->int_enable, pin_idx, 1));
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_INT_SENS_0,
+ extract32(set->int_sens_0, pin_idx, 1));
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_INT_SENS_1,
+ extract32(set->int_sens_1, pin_idx, 1));
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_INT_SENS_2,
+ extract32(set->int_sens_2, pin_idx, 1));
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_RESET_TOLERANCE,
+ extract32(set->reset_tol, pin_idx, 1));
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_DEBOUNCE_1,
+ extract32(set->debounce_1, pin_idx, 1));
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_DEBOUNCE_2,
+ extract32(set->debounce_2, pin_idx, 1));
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_INPUT_MASK,
+ extract32(set->input_mask, pin_idx, 1));
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_INT_STATUS,
+ extract32(set->int_status, pin_idx, 1));
+ value = SHARED_FIELD_DP32(value, GPIO_CONTROL_IN_DATA,
+ extract32(set->data_value, pin_idx, 1));
+ return value;
+}
+
+static void aspeed_gpio_2700_write_control_reg(AspeedGPIOState *s,
+ uint32_t pin, uint64_t data)
+{
+ AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
+ const GPIOSetProperties *props;
+ GPIOSets *set;
+ uint32_t set_idx;
+ uint32_t pin_idx;
+ uint32_t group_value = 0;
+ uint32_t pending = 0;
+
+ set_idx = pin / ASPEED_GPIOS_PER_SET;
+ pin_idx = pin % ASPEED_GPIOS_PER_SET;
+
+ if (set_idx >= agc->nr_gpio_sets) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: set index: %d, out of bounds\n",
+ __func__, set_idx);
+ return;
+ }
+
+ set = &s->sets[set_idx];
+ props = &agc->props[set_idx];
+
+ /* direction */
+ group_value = set->direction;
+ group_value = deposit32(group_value, pin_idx, 1,
+ SHARED_FIELD_EX32(data, GPIO_CONTROL_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;
+ */
+ group_value = (group_value | ~props->input) & props->output;
+ set->direction = update_value_control_source(set, set->direction,
+ group_value);
+
+ /* out data */
+ group_value = set->data_read;
+ group_value = deposit32(group_value, pin_idx, 1,
+ SHARED_FIELD_EX32(data, GPIO_CONTROL_OUT_DATA));
+ group_value &= props->output;
+ group_value = update_value_control_source(set, set->data_read,
+ group_value);
+ set->data_read = group_value;
+
+ /* interrupt enable */
+ group_value = set->int_enable;
+ group_value = deposit32(group_value, pin_idx, 1,
+ SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_ENABLE));
+ set->int_enable = update_value_control_source(set, set->int_enable,
+ group_value);
+
+ /* interrupt sensitivity type 0 */
+ group_value = set->int_sens_0;
+ group_value = deposit32(group_value, pin_idx, 1,
+ SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_SENS_0));
+ set->int_sens_0 = update_value_control_source(set, set->int_sens_0,
+ group_value);
+
+ /* interrupt sensitivity type 1 */
+ group_value = set->int_sens_1;
+ group_value = deposit32(group_value, pin_idx, 1,
+ SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_SENS_1));
+ set->int_sens_1 = update_value_control_source(set, set->int_sens_1,
+ group_value);
+
+ /* interrupt sensitivity type 2 */
+ group_value = set->int_sens_2;
+ group_value = deposit32(group_value, pin_idx, 1,
+ SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_SENS_2));
+ set->int_sens_2 = update_value_control_source(set, set->int_sens_2,
+ group_value);
+
+ /* reset tolerance enable */
+ group_value = set->reset_tol;
+ group_value = deposit32(group_value, pin_idx, 1,
+ SHARED_FIELD_EX32(data, GPIO_CONTROL_RESET_TOLERANCE));
+ set->reset_tol = update_value_control_source(set, set->reset_tol,
+ group_value);
+
+ /* debounce 1 */
+ group_value = set->debounce_1;
+ group_value = deposit32(group_value, pin_idx, 1,
+ SHARED_FIELD_EX32(data, GPIO_CONTROL_DEBOUNCE_1));
+ set->debounce_1 = update_value_control_source(set, set->debounce_1,
+ group_value);
+
+ /* debounce 2 */
+ group_value = set->debounce_2;
+ group_value = deposit32(group_value, pin_idx, 1,
+ SHARED_FIELD_EX32(data, GPIO_CONTROL_DEBOUNCE_2));
+ set->debounce_2 = update_value_control_source(set, set->debounce_2,
+ group_value);
+
+ /* input mask */
+ group_value = set->input_mask;
+ group_value = deposit32(group_value, pin_idx, 1,
+ SHARED_FIELD_EX32(data, GPIO_CONTROL_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 = group_value & props->input;
+
+ /* blink counter 1 */
+ /* blink counter 2 */
+ /* unimplement */
+
+ /* interrupt status */
+ if (SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS)) {
+ /* pending is either 1 or 0 for a 1-bit field */
+ pending = extract32(set->int_status, pin_idx, 1);
+
+ assert(s->pending >= pending);
+
+ /* No change to s->pending if pending is 0 */
+ s->pending -= pending;
+
+ /*
+ * The write acknowledged the interrupt regardless of whether it
+ * was pending or not. The post-condition is that it mustn't be
+ * pending. Unconditionally clear the status bit.
+ */
+ set->int_status = deposit32(set->int_status, pin_idx, 1, 0);
+ }
+
+ aspeed_gpio_update(s, set, set->data_value, UINT32_MAX);
+ return;
+}
+
+static uint64_t aspeed_gpio_2700_read(void *opaque, hwaddr offset,
+ uint32_t size)
+{
+ AspeedGPIOState *s = ASPEED_GPIO(opaque);
+ AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
+ GPIOSets *set;
+ uint64_t value;
+ uint64_t reg;
+ uint32_t pin;
+ uint32_t idx;
+
+ reg = offset >> 2;
+
+ if (reg >= agc->reg_table_count) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: offset 0x%" PRIx64 " out of bounds\n",
+ __func__, offset);
+ return 0;
+ }
+
+ switch (reg) {
+ case R_GPIO_2700_DEBOUNCE_TIME_1 ... R_GPIO_2700_DEBOUNCE_TIME_3:
+ idx = reg - R_GPIO_2700_DEBOUNCE_TIME_1;
+
+ if (idx >= ASPEED_GPIO_NR_DEBOUNCE_REGS) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: debounce index: %d, out of bounds\n",
+ __func__, idx);
+ return 0;
+ }
+
+ value = (uint64_t) s->debounce_regs[idx];
+ break;
+ case R_GPIO_2700_INT_STATUS_1 ... R_GPIO_2700_INT_STATUS_7:
+ idx = reg - R_GPIO_2700_INT_STATUS_1;
+
+ if (idx >= agc->nr_gpio_sets) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: interrupt status index: %d, out of bounds\n",
+ __func__, idx);
+ return 0;
+ }
+
+ set = &s->sets[idx];
+ value = (uint64_t) set->int_status;
+ break;
+ case R_GPIO_A0_CONTROL ... R_GPIO_AA7_CONTROL:
+ pin = reg - R_GPIO_A0_CONTROL;
+
+ if (pin >= agc->nr_gpio_pins) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid pin number: %d\n",
+ __func__, pin);
+ return 0;
+ }
+
+ value = aspeed_gpio_2700_read_control_reg(s, pin);
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset 0x%"
+ PRIx64"\n", __func__, offset);
+ return 0;
+ }
+
+ trace_aspeed_gpio_read(offset, value);
+ return value;
+}
+
+static void aspeed_gpio_2700_write(void *opaque, hwaddr offset,
+ uint64_t data, uint32_t size)
+{
+ AspeedGPIOState *s = ASPEED_GPIO(opaque);
+ AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
+ uint64_t reg;
+ uint32_t pin;
+ uint32_t idx;
+
+ trace_aspeed_gpio_write(offset, data);
+
+ reg = offset >> 2;
+
+ if (reg >= agc->reg_table_count) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: offset 0x%" PRIx64 " out of bounds\n",
+ __func__, offset);
+ return;
+ }
+
+ switch (reg) {
+ case R_GPIO_2700_DEBOUNCE_TIME_1 ... R_GPIO_2700_DEBOUNCE_TIME_3:
+ idx = reg - R_GPIO_2700_DEBOUNCE_TIME_1;
+
+ if (idx >= ASPEED_GPIO_NR_DEBOUNCE_REGS) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: debounce index: %d out of bounds\n",
+ __func__, idx);
+ return;
+ }
+
+ s->debounce_regs[idx] = (uint32_t) data;
+ break;
+ case R_GPIO_A0_CONTROL ... R_GPIO_AA7_CONTROL:
+ pin = reg - R_GPIO_A0_CONTROL;
+
+ if (pin >= agc->nr_gpio_pins) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid pin number: %d\n",
+ __func__, pin);
+ return;
+ }
+
+ if (SHARED_FIELD_EX32(data, GPIO_CONTROL_RESERVED)) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid reserved data: 0x%"
+ PRIx64"\n", __func__, data);
+ return;
+ }
+
+ aspeed_gpio_2700_write_control_reg(s, pin, data);
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset 0x%"
+ PRIx64"\n", __func__, offset);
+ break;
+ }
+
+ return;
+}
+
/* Setup functions */
static const GPIOSetProperties ast2400_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
[0] = {0xffffffff, 0xffffffff, {"A", "B", "C", "D"} },
@@ -1016,6 +1358,16 @@ static GPIOSetProperties ast1030_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
[5] = {0x000000ff, 0x00000000, {"U"} },
};
+static GPIOSetProperties ast2700_set_props[ASPEED_GPIO_MAX_NR_SETS] = {
+ [0] = {0xffffffff, 0xffffffff, {"A", "B", "C", "D"} },
+ [1] = {0x0fffffff, 0x0fffffff, {"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, 0xffffffff, {"U", "V", "W", "X"} },
+ [6] = {0x00ffffff, 0x00ffffff, {"Y", "Z", "AA"} },
+};
+
static const MemoryRegionOps aspeed_gpio_ops = {
.read = aspeed_gpio_read,
.write = aspeed_gpio_write,
@@ -1024,6 +1376,14 @@ static const MemoryRegionOps aspeed_gpio_ops = {
.valid.max_access_size = 4,
};
+static const MemoryRegionOps aspeed_gpio_2700_ops = {
+ .read = aspeed_gpio_2700_read,
+ .write = aspeed_gpio_2700_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid.min_access_size = 4,
+ .valid.max_access_size = 4,
+};
+
static void aspeed_gpio_reset(DeviceState *dev)
{
AspeedGPIOState *s = ASPEED_GPIO(dev);
@@ -1193,6 +1553,18 @@ static void aspeed_gpio_1030_class_init(ObjectClass *klass, void *data)
agc->reg_ops = &aspeed_gpio_ops;
}
+static void aspeed_gpio_2700_class_init(ObjectClass *klass, void *data)
+{
+ AspeedGPIOClass *agc = ASPEED_GPIO_CLASS(klass);
+
+ agc->props = ast2700_set_props;
+ agc->nr_gpio_pins = 216;
+ agc->nr_gpio_sets = 7;
+ agc->reg_table_count = GPIO_2700_REG_ARRAY_SIZE;
+ agc->mem_size = 0x1000;
+ agc->reg_ops = &aspeed_gpio_2700_ops;
+}
+
static const TypeInfo aspeed_gpio_info = {
.name = TYPE_ASPEED_GPIO,
.parent = TYPE_SYS_BUS_DEVICE,
@@ -1237,6 +1609,13 @@ static const TypeInfo aspeed_gpio_ast1030_info = {
.instance_init = aspeed_gpio_init,
};
+static const TypeInfo aspeed_gpio_ast2700_info = {
+ .name = TYPE_ASPEED_GPIO "-ast2700",
+ .parent = TYPE_ASPEED_GPIO,
+ .class_init = aspeed_gpio_2700_class_init,
+ .instance_init = aspeed_gpio_init,
+};
+
static void aspeed_gpio_register_types(void)
{
type_register_static(&aspeed_gpio_info);
@@ -1245,6 +1624,7 @@ static void aspeed_gpio_register_types(void)
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_register_static(&aspeed_gpio_ast2700_info);
}
type_init(aspeed_gpio_register_types);
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v6 6/8] aspeed/soc: Correct GPIO irq 130 for AST2700
2024-09-30 8:52 [PATCH v6 0/8] Support GPIO for AST2700 Jamin Lin via
` (4 preceding siblings ...)
2024-09-30 8:52 ` [PATCH v6 5/8] hw/gpio/aspeed: Add AST2700 support Jamin Lin via
@ 2024-09-30 8:52 ` Jamin Lin via
2024-09-30 9:11 ` [SPAM] " Cédric Le Goater
2024-09-30 8:52 ` [PATCH v6 7/8] aspeed/soc: Support GPIO " Jamin Lin via
2024-09-30 8:52 ` [PATCH v6 8/8] hw/gpio/aspeed: Add test case " Jamin Lin via
7 siblings, 1 reply; 15+ messages in thread
From: Jamin Lin via @ 2024-09-30 8:52 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Andrew Jeffery, Joel Stanley, Thomas Huth, Laurent Vivier,
Paolo Bonzini, open list:ASPEED BMCs,
open list:All patches CC here
Cc: jamin_lin, troy_lee, yunlin.tang
The register set of GPIO have a significant change since AST2700.
Each GPIO pin has their own individual control register and users are able to
set one GPIO pin’s direction, interrupt enable, input mask and so on in the
same one control register.
AST2700 does not have GPIO18_XXX registers for GPIO 1.8v, removes
ASPEED_DEV_GPIO_1_8V. It is enough to only have ASPEED_DEV_GPIO
device in AST2700.
The AST2700 GPIO controller interrupt is connected to GICINT130_INTC at
bit 18. Therefore, correct GPIO irq 130.
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
hw/arm/aspeed_ast27x0.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 761ee11657..99135edc1e 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -87,8 +87,7 @@ static const int aspeed_soc_ast2700_irqmap[] = {
[ASPEED_DEV_ADC] = 130,
[ASPEED_DEV_XDMA] = 5,
[ASPEED_DEV_EMMC] = 15,
- [ASPEED_DEV_GPIO] = 11,
- [ASPEED_DEV_GPIO_1_8V] = 130,
+ [ASPEED_DEV_GPIO] = 130,
[ASPEED_DEV_RTC] = 13,
[ASPEED_DEV_TIMER1] = 16,
[ASPEED_DEV_TIMER2] = 17,
@@ -124,7 +123,7 @@ static const int aspeed_soc_ast2700_gic128_intcmap[] = {
static const int aspeed_soc_ast2700_gic130_intcmap[] = {
[ASPEED_DEV_I2C] = 0,
[ASPEED_DEV_ADC] = 16,
- [ASPEED_DEV_GPIO_1_8V] = 18,
+ [ASPEED_DEV_GPIO] = 18,
};
/* GICINT 131 */
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [SPAM] [PATCH v6 6/8] aspeed/soc: Correct GPIO irq 130 for AST2700
2024-09-30 8:52 ` [PATCH v6 6/8] aspeed/soc: Correct GPIO irq 130 for AST2700 Jamin Lin via
@ 2024-09-30 9:11 ` Cédric Le Goater
0 siblings, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2024-09-30 9:11 UTC (permalink / raw)
To: Jamin Lin, Peter Maydell, Steven Lee, Troy Lee, Andrew Jeffery,
Joel Stanley, Thomas Huth, Laurent Vivier, Paolo Bonzini,
open list:ASPEED BMCs, open list:All patches CC here
Cc: troy_lee, yunlin.tang
On 9/30/24 10:52, Jamin Lin wrote:
> The register set of GPIO have a significant change since AST2700.
> Each GPIO pin has their own individual control register and users are able to
> set one GPIO pin’s direction, interrupt enable, input mask and so on in the
> same one control register.
>
> AST2700 does not have GPIO18_XXX registers for GPIO 1.8v, removes
> ASPEED_DEV_GPIO_1_8V. It is enough to only have ASPEED_DEV_GPIO
> device in AST2700.
>
> The AST2700 GPIO controller interrupt is connected to GICINT130_INTC at
> bit 18. Therefore, correct GPIO irq 130.
>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Thanks,
C.
> ---
> hw/arm/aspeed_ast27x0.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index 761ee11657..99135edc1e 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -87,8 +87,7 @@ static const int aspeed_soc_ast2700_irqmap[] = {
> [ASPEED_DEV_ADC] = 130,
> [ASPEED_DEV_XDMA] = 5,
> [ASPEED_DEV_EMMC] = 15,
> - [ASPEED_DEV_GPIO] = 11,
> - [ASPEED_DEV_GPIO_1_8V] = 130,
> + [ASPEED_DEV_GPIO] = 130,
> [ASPEED_DEV_RTC] = 13,
> [ASPEED_DEV_TIMER1] = 16,
> [ASPEED_DEV_TIMER2] = 17,
> @@ -124,7 +123,7 @@ static const int aspeed_soc_ast2700_gic128_intcmap[] = {
> static const int aspeed_soc_ast2700_gic130_intcmap[] = {
> [ASPEED_DEV_I2C] = 0,
> [ASPEED_DEV_ADC] = 16,
> - [ASPEED_DEV_GPIO_1_8V] = 18,
> + [ASPEED_DEV_GPIO] = 18,
> };
>
> /* GICINT 131 */
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v6 7/8] aspeed/soc: Support GPIO for AST2700
2024-09-30 8:52 [PATCH v6 0/8] Support GPIO for AST2700 Jamin Lin via
` (5 preceding siblings ...)
2024-09-30 8:52 ` [PATCH v6 6/8] aspeed/soc: Correct GPIO irq 130 for AST2700 Jamin Lin via
@ 2024-09-30 8:52 ` Jamin Lin via
2024-09-30 9:11 ` [SPAM] " Cédric Le Goater
2024-09-30 8:52 ` [PATCH v6 8/8] hw/gpio/aspeed: Add test case " Jamin Lin via
7 siblings, 1 reply; 15+ messages in thread
From: Jamin Lin via @ 2024-09-30 8:52 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Andrew Jeffery, Joel Stanley, Thomas Huth, Laurent Vivier,
Paolo Bonzini, open list:ASPEED BMCs,
open list:All patches CC here
Cc: jamin_lin, troy_lee, yunlin.tang
Add GPIO model for AST2700 GPIO support. The GPIO controller registers base
address is start at 0x14C0_B000 and its address space is 0x1000.
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
hw/arm/aspeed_ast27x0.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 99135edc1e..dca660eb6b 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -62,6 +62,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
[ASPEED_GIC_REDIST] = 0x12280000,
[ASPEED_DEV_ADC] = 0x14C00000,
[ASPEED_DEV_I2C] = 0x14C0F000,
+ [ASPEED_DEV_GPIO] = 0x14C0B000,
};
#define AST2700_MAX_IRQ 288
@@ -372,6 +373,9 @@ static void aspeed_soc_ast2700_init(Object *obj)
snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
object_initialize_child(obj, "i2c", &s->i2c, typename);
+
+ snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
+ object_initialize_child(obj, "gpio", &s->gpio, typename);
}
/*
@@ -657,6 +661,15 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
}
+ /* GPIO */
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->gpio), errp)) {
+ return;
+ }
+ aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->gpio), 0,
+ sc->memmap[ASPEED_DEV_GPIO]);
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_GPIO));
+
create_unimplemented_device("ast2700.dpmcu", 0x11000000, 0x40000);
create_unimplemented_device("ast2700.iomem0", 0x12000000, 0x01000000);
create_unimplemented_device("ast2700.iomem1", 0x14000000, 0x01000000);
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [SPAM] [PATCH v6 7/8] aspeed/soc: Support GPIO for AST2700
2024-09-30 8:52 ` [PATCH v6 7/8] aspeed/soc: Support GPIO " Jamin Lin via
@ 2024-09-30 9:11 ` Cédric Le Goater
0 siblings, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2024-09-30 9:11 UTC (permalink / raw)
To: Jamin Lin, Peter Maydell, Steven Lee, Troy Lee, Andrew Jeffery,
Joel Stanley, Thomas Huth, Laurent Vivier, Paolo Bonzini,
open list:ASPEED BMCs, open list:All patches CC here
Cc: troy_lee, yunlin.tang
On 9/30/24 10:52, Jamin Lin wrote:
> Add GPIO model for AST2700 GPIO support. The GPIO controller registers base
> address is start at 0x14C0_B000 and its address space is 0x1000.
>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Thanks,
C.
> ---
> hw/arm/aspeed_ast27x0.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index 99135edc1e..dca660eb6b 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -62,6 +62,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
> [ASPEED_GIC_REDIST] = 0x12280000,
> [ASPEED_DEV_ADC] = 0x14C00000,
> [ASPEED_DEV_I2C] = 0x14C0F000,
> + [ASPEED_DEV_GPIO] = 0x14C0B000,
> };
>
> #define AST2700_MAX_IRQ 288
> @@ -372,6 +373,9 @@ static void aspeed_soc_ast2700_init(Object *obj)
>
> snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
> object_initialize_child(obj, "i2c", &s->i2c, typename);
> +
> + snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
> + object_initialize_child(obj, "gpio", &s->gpio, typename);
> }
>
> /*
> @@ -657,6 +661,15 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
> }
>
> + /* GPIO */
> + if (!sysbus_realize(SYS_BUS_DEVICE(&s->gpio), errp)) {
> + return;
> + }
> + aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->gpio), 0,
> + sc->memmap[ASPEED_DEV_GPIO]);
> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
> + aspeed_soc_get_irq(s, ASPEED_DEV_GPIO));
> +
> create_unimplemented_device("ast2700.dpmcu", 0x11000000, 0x40000);
> create_unimplemented_device("ast2700.iomem0", 0x12000000, 0x01000000);
> create_unimplemented_device("ast2700.iomem1", 0x14000000, 0x01000000);
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v6 8/8] hw/gpio/aspeed: Add test case for AST2700
2024-09-30 8:52 [PATCH v6 0/8] Support GPIO for AST2700 Jamin Lin via
` (6 preceding siblings ...)
2024-09-30 8:52 ` [PATCH v6 7/8] aspeed/soc: Support GPIO " Jamin Lin via
@ 2024-09-30 8:52 ` Jamin Lin via
2024-09-30 16:36 ` Thomas Huth
7 siblings, 1 reply; 15+ messages in thread
From: Jamin Lin via @ 2024-09-30 8:52 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell, Steven Lee, Troy Lee,
Andrew Jeffery, Joel Stanley, Thomas Huth, Laurent Vivier,
Paolo Bonzini, open list:ASPEED BMCs,
open list:All patches CC here
Cc: jamin_lin, troy_lee, yunlin.tang
Add test case to test GPIO output and input pins from A0 to D7 for AST2700.
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
tests/qtest/aspeed_gpio-test.c | 77 ++++++++++++++++++++++++++++++++--
tests/qtest/meson.build | 3 ++
2 files changed, 76 insertions(+), 4 deletions(-)
diff --git a/tests/qtest/aspeed_gpio-test.c b/tests/qtest/aspeed_gpio-test.c
index d38f51d719..03b3b1c2b2 100644
--- a/tests/qtest/aspeed_gpio-test.c
+++ b/tests/qtest/aspeed_gpio-test.c
@@ -33,6 +33,10 @@
#define GPIO_ABCD_DATA_VALUE 0x000
#define GPIO_ABCD_DIRECTION 0x004
+/* AST2700 */
+#define AST2700_GPIO_BASE 0x14C0B000
+#define GPIOA0_CONTROL 0x180
+
static void test_set_colocated_pins(const void *data)
{
QTestState *s = (QTestState *)data;
@@ -72,17 +76,82 @@ static void test_set_input_pins(const void *data)
g_assert_cmphex(value, ==, 0xffffffff);
}
+static void test_2700_output_pins(const void *data)
+{
+ QTestState *s = (QTestState *)data;
+ uint32_t offset = 0;
+ uint32_t value = 0;
+ uint32_t pin = 0;
+
+ for (char c = 'A'; c <= 'D'; c++) {
+ for (int i = 0; i < 8; i++) {
+ offset = AST2700_GPIO_BASE + GPIOA0_CONTROL + (pin * 4);
+
+ /* output direction and output hi */
+ qtest_writel(s, offset, 0x00000003);
+ value = qtest_readl(s, offset);
+ g_assert_cmphex(value, ==, 0x00000003);
+
+ /* output direction and output low */
+ qtest_writel(s, offset, 0x00000002);
+ value = qtest_readl(s, offset);
+ g_assert_cmphex(value, ==, 0x00000002);
+ pin++;
+ }
+ }
+}
+
+static void test_2700_input_pins(const void *data)
+{
+ QTestState *s = (QTestState *)data;
+ char name[16];
+ uint32_t offset = 0;
+ uint32_t value = 0;
+ uint32_t pin = 0;
+
+ for (char c = 'A'; c <= 'D'; c++) {
+ for (int i = 0; i < 8; i++) {
+ sprintf(name, "gpio%c%d", c, i);
+ offset = AST2700_GPIO_BASE + GPIOA0_CONTROL + (pin * 4);
+ /* input direction */
+ qtest_writel(s, offset, 0);
+
+ /* set input */
+ qtest_qom_set_bool(s, "/machine/soc/gpio", name, true);
+ value = qtest_readl(s, offset);
+ g_assert_cmphex(value, ==, 0x00002000);
+
+ /* clear input */
+ qtest_qom_set_bool(s, "/machine/soc/gpio", name, false);
+ value = qtest_readl(s, offset);
+ g_assert_cmphex(value, ==, 0);
+ pin++;
+ }
+ }
+}
+
+
int main(int argc, char **argv)
{
+ const char *arch = qtest_get_arch();
QTestState *s;
int r;
g_test_init(&argc, &argv, NULL);
- s = qtest_init("-machine ast2600-evb");
- qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
- test_set_colocated_pins);
- qtest_add_data_func("/ast2600/gpio/set_input_pins", s, test_set_input_pins);
+ if (strcmp(arch, "aarch64") == 0) {
+ s = qtest_init("-machine ast2700-evb");
+ qtest_add_data_func("/ast2700/gpio/input_pins",
+ s, test_2700_input_pins);
+ qtest_add_data_func("/ast2700/gpio/out_pins", s, test_2700_output_pins);
+ } else {
+ s = qtest_init("-machine ast2600-evb");
+ qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
+ test_set_colocated_pins);
+ qtest_add_data_func("/ast2600/gpio/set_input_pins", s,
+ test_set_input_pins);
+ }
+
r = g_test_run();
qtest_quit(s);
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 310865e49c..292980e3ad 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -209,6 +209,8 @@ qtests_aspeed = \
['aspeed_hace-test',
'aspeed_smc-test',
'aspeed_gpio-test']
+qtests_aspeed64 = \
+ ['aspeed_gpio-test']
qtests_stm32l4x5 = \
['stm32l4x5_exti-test',
@@ -247,6 +249,7 @@ qtests_aarch64 = \
(config_all_devices.has_key('CONFIG_RASPI') ? ['bcm2835-dma-test', 'bcm2835-i2c-test'] : []) + \
(config_all_accel.has_key('CONFIG_TCG') and \
config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : []) + \
+ (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed64 : []) + \
['arm-cpu-features',
'numa-test',
'boot-serial-test',
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v6 8/8] hw/gpio/aspeed: Add test case for AST2700
2024-09-30 8:52 ` [PATCH v6 8/8] hw/gpio/aspeed: Add test case " Jamin Lin via
@ 2024-09-30 16:36 ` Thomas Huth
2024-09-30 16:48 ` Cédric Le Goater
0 siblings, 1 reply; 15+ messages in thread
From: Thomas Huth @ 2024-09-30 16:36 UTC (permalink / raw)
To: Jamin Lin, Cédric Le Goater, Peter Maydell, Steven Lee,
Troy Lee, Andrew Jeffery, Joel Stanley, Laurent Vivier,
Paolo Bonzini, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee, yunlin.tang
On 30/09/2024 10.52, Jamin Lin wrote:
> Add test case to test GPIO output and input pins from A0 to D7 for AST2700.
>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
> tests/qtest/aspeed_gpio-test.c | 77 ++++++++++++++++++++++++++++++++--
> tests/qtest/meson.build | 3 ++
> 2 files changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qtest/aspeed_gpio-test.c b/tests/qtest/aspeed_gpio-test.c
> index d38f51d719..03b3b1c2b2 100644
> --- a/tests/qtest/aspeed_gpio-test.c
> +++ b/tests/qtest/aspeed_gpio-test.c
> @@ -33,6 +33,10 @@
> #define GPIO_ABCD_DATA_VALUE 0x000
> #define GPIO_ABCD_DIRECTION 0x004
>
> +/* AST2700 */
> +#define AST2700_GPIO_BASE 0x14C0B000
> +#define GPIOA0_CONTROL 0x180
> +
> static void test_set_colocated_pins(const void *data)
> {
> QTestState *s = (QTestState *)data;
> @@ -72,17 +76,82 @@ static void test_set_input_pins(const void *data)
> g_assert_cmphex(value, ==, 0xffffffff);
> }
>
> +static void test_2700_output_pins(const void *data)
> +{
> + QTestState *s = (QTestState *)data;
> + uint32_t offset = 0;
> + uint32_t value = 0;
> + uint32_t pin = 0;
> +
> + for (char c = 'A'; c <= 'D'; c++) {
> + for (int i = 0; i < 8; i++) {
> + offset = AST2700_GPIO_BASE + GPIOA0_CONTROL + (pin * 4);
> +
> + /* output direction and output hi */
> + qtest_writel(s, offset, 0x00000003);
> + value = qtest_readl(s, offset);
> + g_assert_cmphex(value, ==, 0x00000003);
> +
> + /* output direction and output low */
> + qtest_writel(s, offset, 0x00000002);
> + value = qtest_readl(s, offset);
> + g_assert_cmphex(value, ==, 0x00000002);
> + pin++;
> + }
> + }
> +}
> +
> +static void test_2700_input_pins(const void *data)
> +{
> + QTestState *s = (QTestState *)data;
> + char name[16];
> + uint32_t offset = 0;
> + uint32_t value = 0;
> + uint32_t pin = 0;
> +
> + for (char c = 'A'; c <= 'D'; c++) {
> + for (int i = 0; i < 8; i++) {
> + sprintf(name, "gpio%c%d", c, i);
> + offset = AST2700_GPIO_BASE + GPIOA0_CONTROL + (pin * 4);
> + /* input direction */
> + qtest_writel(s, offset, 0);
> +
> + /* set input */
> + qtest_qom_set_bool(s, "/machine/soc/gpio", name, true);
> + value = qtest_readl(s, offset);
> + g_assert_cmphex(value, ==, 0x00002000);
> +
> + /* clear input */
> + qtest_qom_set_bool(s, "/machine/soc/gpio", name, false);
> + value = qtest_readl(s, offset);
> + g_assert_cmphex(value, ==, 0);
> + pin++;
> + }
> + }
> +}
As far as I can see, there is nothing in these two functions that requires
any of the other code in this file ...
> +
> int main(int argc, char **argv)
> {
> + const char *arch = qtest_get_arch();
> QTestState *s;
> int r;
>
> g_test_init(&argc, &argv, NULL);
>
> - s = qtest_init("-machine ast2600-evb");
> - qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
> - test_set_colocated_pins);
> - qtest_add_data_func("/ast2600/gpio/set_input_pins", s, test_set_input_pins);
> + if (strcmp(arch, "aarch64") == 0) {
> + s = qtest_init("-machine ast2700-evb");
> + qtest_add_data_func("/ast2700/gpio/input_pins",
> + s, test_2700_input_pins);
> + qtest_add_data_func("/ast2700/gpio/out_pins", s, test_2700_output_pins);
> + } else {
> + s = qtest_init("-machine ast2600-evb");
> + qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
> + test_set_colocated_pins);
> + qtest_add_data_func("/ast2600/gpio/set_input_pins", s,
> + test_set_input_pins);
> + }
... so the more I look at this, the more I think your new test should reside
in a separate file that only gets executed for aarch64, while this file here
should stay for arm 32-bit. Or is there a real compelling reason for putting
your code in this file here?
Thomas
> r = g_test_run();
> qtest_quit(s);
>
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 310865e49c..292980e3ad 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -209,6 +209,8 @@ qtests_aspeed = \
> ['aspeed_hace-test',
> 'aspeed_smc-test',
> 'aspeed_gpio-test']
> +qtests_aspeed64 = \
> + ['aspeed_gpio-test']
>
> qtests_stm32l4x5 = \
> ['stm32l4x5_exti-test',
> @@ -247,6 +249,7 @@ qtests_aarch64 = \
> (config_all_devices.has_key('CONFIG_RASPI') ? ['bcm2835-dma-test', 'bcm2835-i2c-test'] : []) + \
> (config_all_accel.has_key('CONFIG_TCG') and \
> config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : []) + \
> + (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed64 : []) + \
> ['arm-cpu-features',
> 'numa-test',
> 'boot-serial-test',
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v6 8/8] hw/gpio/aspeed: Add test case for AST2700
2024-09-30 16:36 ` Thomas Huth
@ 2024-09-30 16:48 ` Cédric Le Goater
2024-09-30 17:23 ` Thomas Huth
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2024-09-30 16:48 UTC (permalink / raw)
To: Thomas Huth, Jamin Lin, Peter Maydell, Steven Lee, Troy Lee,
Andrew Jeffery, Joel Stanley, Laurent Vivier, Paolo Bonzini,
open list:ASPEED BMCs, open list:All patches CC here
Cc: troy_lee, yunlin.tang
On 9/30/24 18:36, Thomas Huth wrote:
> On 30/09/2024 10.52, Jamin Lin wrote:
>> Add test case to test GPIO output and input pins from A0 to D7 for AST2700.
>>
>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>> ---
>> tests/qtest/aspeed_gpio-test.c | 77 ++++++++++++++++++++++++++++++++--
>> tests/qtest/meson.build | 3 ++
>> 2 files changed, 76 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qtest/aspeed_gpio-test.c b/tests/qtest/aspeed_gpio-test.c
>> index d38f51d719..03b3b1c2b2 100644
>> --- a/tests/qtest/aspeed_gpio-test.c
>> +++ b/tests/qtest/aspeed_gpio-test.c
>> @@ -33,6 +33,10 @@
>> #define GPIO_ABCD_DATA_VALUE 0x000
>> #define GPIO_ABCD_DIRECTION 0x004
>> +/* AST2700 */
>> +#define AST2700_GPIO_BASE 0x14C0B000
>> +#define GPIOA0_CONTROL 0x180
>> +
>> static void test_set_colocated_pins(const void *data)
>> {
>> QTestState *s = (QTestState *)data;
>> @@ -72,17 +76,82 @@ static void test_set_input_pins(const void *data)
>> g_assert_cmphex(value, ==, 0xffffffff);
>> }
>> +static void test_2700_output_pins(const void *data)
>> +{
>> + QTestState *s = (QTestState *)data;
>> + uint32_t offset = 0;
>> + uint32_t value = 0;
>> + uint32_t pin = 0;
>> +
>> + for (char c = 'A'; c <= 'D'; c++) {
>> + for (int i = 0; i < 8; i++) {
>> + offset = AST2700_GPIO_BASE + GPIOA0_CONTROL + (pin * 4);
>> +
>> + /* output direction and output hi */
>> + qtest_writel(s, offset, 0x00000003);
>> + value = qtest_readl(s, offset);
>> + g_assert_cmphex(value, ==, 0x00000003);
>> +
>> + /* output direction and output low */
>> + qtest_writel(s, offset, 0x00000002);
>> + value = qtest_readl(s, offset);
>> + g_assert_cmphex(value, ==, 0x00000002);
>> + pin++;
>> + }
>> + }
>> +}
>> +
>> +static void test_2700_input_pins(const void *data)
>> +{
>> + QTestState *s = (QTestState *)data;
>> + char name[16];
>> + uint32_t offset = 0;
>> + uint32_t value = 0;
>> + uint32_t pin = 0;
>> +
>> + for (char c = 'A'; c <= 'D'; c++) {
>> + for (int i = 0; i < 8; i++) {
>> + sprintf(name, "gpio%c%d", c, i);
>> + offset = AST2700_GPIO_BASE + GPIOA0_CONTROL + (pin * 4);
>> + /* input direction */
>> + qtest_writel(s, offset, 0);
>> +
>> + /* set input */
>> + qtest_qom_set_bool(s, "/machine/soc/gpio", name, true);
>> + value = qtest_readl(s, offset);
>> + g_assert_cmphex(value, ==, 0x00002000);
>> +
>> + /* clear input */
>> + qtest_qom_set_bool(s, "/machine/soc/gpio", name, false);
>> + value = qtest_readl(s, offset);
>> + g_assert_cmphex(value, ==, 0);
>> + pin++;
>> + }
>> + }
>> +}
>
> As far as I can see, there is nothing in these two functions that requires any of the other code in this file ...
>
>> +
>> int main(int argc, char **argv)
>> {
>> + const char *arch = qtest_get_arch();
>> QTestState *s;
>> int r;
>> g_test_init(&argc, &argv, NULL);
>> - s = qtest_init("-machine ast2600-evb");
>> - qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
>> - test_set_colocated_pins);
>> - qtest_add_data_func("/ast2600/gpio/set_input_pins", s, test_set_input_pins);
>> + if (strcmp(arch, "aarch64") == 0) {
>> + s = qtest_init("-machine ast2700-evb");
>> + qtest_add_data_func("/ast2700/gpio/input_pins",
>> + s, test_2700_input_pins);
>> + qtest_add_data_func("/ast2700/gpio/out_pins", s, test_2700_output_pins);
>> + } else {
>> + s = qtest_init("-machine ast2600-evb");
>> + qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
>> + test_set_colocated_pins);
>> + qtest_add_data_func("/ast2600/gpio/set_input_pins", s,
>> + test_set_input_pins);
>> + }
>
> ... so the more I look at this, the more I think your new test should reside in a separate file that only gets executed for aarch64, while this file here should stay for arm 32-bit. Or is there a real compelling reason for putting your code in this file here?
Because it is related to the Aspeed GPIO controllers. Unless we
want to introduce a new set of test files for 64-bit Aspeed SoC
controllers ? why not.
I am ok with both options. Option 1 is simpler to implement, but
there may be reasons to separate the tests based on architecture,
although I'm not aware of any at the moment. Would you rather prefer
option 2 ? How should we name the test file ?
Thanks,
C.
> Thomas
>
>
>> r = g_test_run();
>> qtest_quit(s);
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index 310865e49c..292980e3ad 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -209,6 +209,8 @@ qtests_aspeed = \
>> ['aspeed_hace-test',
>> 'aspeed_smc-test',
>> 'aspeed_gpio-test']
>> +qtests_aspeed64 = \
>> + ['aspeed_gpio-test']
>> qtests_stm32l4x5 = \
>> ['stm32l4x5_exti-test',
>> @@ -247,6 +249,7 @@ qtests_aarch64 = \
>> (config_all_devices.has_key('CONFIG_RASPI') ? ['bcm2835-dma-test', 'bcm2835-i2c-test'] : []) + \
>> (config_all_accel.has_key('CONFIG_TCG') and \
>> config_all_devices.has_key('CONFIG_TPM_TIS_I2C') ? ['tpm-tis-i2c-test'] : []) + \
>> + (config_all_devices.has_key('CONFIG_ASPEED_SOC') ? qtests_aspeed64 : []) + \
>> ['arm-cpu-features',
>> 'numa-test',
>> 'boot-serial-test',
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v6 8/8] hw/gpio/aspeed: Add test case for AST2700
2024-09-30 16:48 ` Cédric Le Goater
@ 2024-09-30 17:23 ` Thomas Huth
2024-10-01 1:15 ` Jamin Lin
0 siblings, 1 reply; 15+ messages in thread
From: Thomas Huth @ 2024-09-30 17:23 UTC (permalink / raw)
To: Cédric Le Goater, Jamin Lin, Peter Maydell, Steven Lee,
Troy Lee, Andrew Jeffery, Joel Stanley, Laurent Vivier,
Paolo Bonzini, open list:ASPEED BMCs,
open list:All patches CC here
Cc: troy_lee, yunlin.tang
On 30/09/2024 18.48, Cédric Le Goater wrote:
> On 9/30/24 18:36, Thomas Huth wrote:
>> On 30/09/2024 10.52, Jamin Lin wrote:
>>> Add test case to test GPIO output and input pins from A0 to D7 for AST2700.
>>>
>>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>>> ---
>>> tests/qtest/aspeed_gpio-test.c | 77 ++++++++++++++++++++++++++++++++--
>>> tests/qtest/meson.build | 3 ++
>>> 2 files changed, 76 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/qtest/aspeed_gpio-test.c b/tests/qtest/aspeed_gpio-test.c
>>> index d38f51d719..03b3b1c2b2 100644
>>> --- a/tests/qtest/aspeed_gpio-test.c
>>> +++ b/tests/qtest/aspeed_gpio-test.c
>>> @@ -33,6 +33,10 @@
>>> #define GPIO_ABCD_DATA_VALUE 0x000
>>> #define GPIO_ABCD_DIRECTION 0x004
>>> +/* AST2700 */
>>> +#define AST2700_GPIO_BASE 0x14C0B000
>>> +#define GPIOA0_CONTROL 0x180
>>> +
>>> static void test_set_colocated_pins(const void *data)
>>> {
>>> QTestState *s = (QTestState *)data;
>>> @@ -72,17 +76,82 @@ static void test_set_input_pins(const void *data)
>>> g_assert_cmphex(value, ==, 0xffffffff);
>>> }
>>> +static void test_2700_output_pins(const void *data)
>>> +{
>>> + QTestState *s = (QTestState *)data;
>>> + uint32_t offset = 0;
>>> + uint32_t value = 0;
>>> + uint32_t pin = 0;
>>> +
>>> + for (char c = 'A'; c <= 'D'; c++) {
>>> + for (int i = 0; i < 8; i++) {
>>> + offset = AST2700_GPIO_BASE + GPIOA0_CONTROL + (pin * 4);
>>> +
>>> + /* output direction and output hi */
>>> + qtest_writel(s, offset, 0x00000003);
>>> + value = qtest_readl(s, offset);
>>> + g_assert_cmphex(value, ==, 0x00000003);
>>> +
>>> + /* output direction and output low */
>>> + qtest_writel(s, offset, 0x00000002);
>>> + value = qtest_readl(s, offset);
>>> + g_assert_cmphex(value, ==, 0x00000002);
>>> + pin++;
>>> + }
>>> + }
>>> +}
>>> +
>>> +static void test_2700_input_pins(const void *data)
>>> +{
>>> + QTestState *s = (QTestState *)data;
>>> + char name[16];
>>> + uint32_t offset = 0;
>>> + uint32_t value = 0;
>>> + uint32_t pin = 0;
>>> +
>>> + for (char c = 'A'; c <= 'D'; c++) {
>>> + for (int i = 0; i < 8; i++) {
>>> + sprintf(name, "gpio%c%d", c, i);
>>> + offset = AST2700_GPIO_BASE + GPIOA0_CONTROL + (pin * 4);
>>> + /* input direction */
>>> + qtest_writel(s, offset, 0);
>>> +
>>> + /* set input */
>>> + qtest_qom_set_bool(s, "/machine/soc/gpio", name, true);
>>> + value = qtest_readl(s, offset);
>>> + g_assert_cmphex(value, ==, 0x00002000);
>>> +
>>> + /* clear input */
>>> + qtest_qom_set_bool(s, "/machine/soc/gpio", name, false);
>>> + value = qtest_readl(s, offset);
>>> + g_assert_cmphex(value, ==, 0);
>>> + pin++;
>>> + }
>>> + }
>>> +}
>>
>> As far as I can see, there is nothing in these two functions that requires
>> any of the other code in this file ...
>>
>>> +
>>> int main(int argc, char **argv)
>>> {
>>> + const char *arch = qtest_get_arch();
>>> QTestState *s;
>>> int r;
>>> g_test_init(&argc, &argv, NULL);
>>> - s = qtest_init("-machine ast2600-evb");
>>> - qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
>>> - test_set_colocated_pins);
>>> - qtest_add_data_func("/ast2600/gpio/set_input_pins", s,
>>> test_set_input_pins);
>>> + if (strcmp(arch, "aarch64") == 0) {
>>> + s = qtest_init("-machine ast2700-evb");
>>> + qtest_add_data_func("/ast2700/gpio/input_pins",
>>> + s, test_2700_input_pins);
>>> + qtest_add_data_func("/ast2700/gpio/out_pins", s,
>>> test_2700_output_pins);
>>> + } else {
>>> + s = qtest_init("-machine ast2600-evb");
>>> + qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
>>> + test_set_colocated_pins);
>>> + qtest_add_data_func("/ast2600/gpio/set_input_pins", s,
>>> + test_set_input_pins);
>>> + }
>>
>> ... so the more I look at this, the more I think your new test should
>> reside in a separate file that only gets executed for aarch64, while this
>> file here should stay for arm 32-bit. Or is there a real compelling reason
>> for putting your code in this file here?
>
> Because it is related to the Aspeed GPIO controllers. Unless we
> want to introduce a new set of test files for 64-bit Aspeed SoC
> controllers ? why not.
>
> I am ok with both options. Option 1 is simpler to implement, but
> there may be reasons to separate the tests based on architecture,
> although I'm not aware of any at the moment. Would you rather prefer
> option 2 ? How should we name the test file ?
Since all arm 32-bit tests are currently completely separate from the
aarch64 tests, I think a separate file would be better, indeed.
Simply call it ast2700-gpio-test.c ?
Thomas
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH v6 8/8] hw/gpio/aspeed: Add test case for AST2700
2024-09-30 17:23 ` Thomas Huth
@ 2024-10-01 1:15 ` Jamin Lin
0 siblings, 0 replies; 15+ messages in thread
From: Jamin Lin @ 2024-10-01 1:15 UTC (permalink / raw)
To: Thomas Huth, Cédric Le Goater, Peter Maydell, Steven Lee,
Troy Lee, Andrew Jeffery, Joel Stanley, Laurent Vivier,
Paolo Bonzini, open list:ASPEED BMCs,
open list:All patches CC here
Cc: Troy Lee, Yunlin Tang
Hi Thomas, Cedric
> Subject: Re: [PATCH v6 8/8] hw/gpio/aspeed: Add test case for AST2700
>
> On 30/09/2024 18.48, Cédric Le Goater wrote:
> > On 9/30/24 18:36, Thomas Huth wrote:
> >> On 30/09/2024 10.52, Jamin Lin wrote:
> >>> Add test case to test GPIO output and input pins from A0 to D7 for
> AST2700.
> >>>
> >>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> >>> ---
> >>> tests/qtest/aspeed_gpio-test.c | 77
> >>> ++++++++++++++++++++++++++++++++--
> >>> tests/qtest/meson.build | 3 ++
> >>> 2 files changed, 76 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/tests/qtest/aspeed_gpio-test.c
> >>> b/tests/qtest/aspeed_gpio-test.c index d38f51d719..03b3b1c2b2 100644
> >>> --- a/tests/qtest/aspeed_gpio-test.c
> >>> +++ b/tests/qtest/aspeed_gpio-test.c
> >>> @@ -33,6 +33,10 @@
> >>> #define GPIO_ABCD_DATA_VALUE 0x000
> >>> #define GPIO_ABCD_DIRECTION 0x004
> >>> +/* AST2700 */
> >>> +#define AST2700_GPIO_BASE 0x14C0B000 #define GPIOA0_CONTROL
> 0x180
> >>> +
> >>> static void test_set_colocated_pins(const void *data)
> >>> {
> >>> QTestState *s = (QTestState *)data; @@ -72,17 +76,82 @@ static
> >>> void test_set_input_pins(const void *data)
> >>> g_assert_cmphex(value, ==, 0xffffffff);
> >>> }
> >>> +static void test_2700_output_pins(const void *data) {
> >>> + QTestState *s = (QTestState *)data;
> >>> + uint32_t offset = 0;
> >>> + uint32_t value = 0;
> >>> + uint32_t pin = 0;
> >>> +
> >>> + for (char c = 'A'; c <= 'D'; c++) {
> >>> + for (int i = 0; i < 8; i++) {
> >>> + offset = AST2700_GPIO_BASE + GPIOA0_CONTROL +
> (pin *
> >>> +4);
> >>> +
> >>> + /* output direction and output hi */
> >>> + qtest_writel(s, offset, 0x00000003);
> >>> + value = qtest_readl(s, offset);
> >>> + g_assert_cmphex(value, ==, 0x00000003);
> >>> +
> >>> + /* output direction and output low */
> >>> + qtest_writel(s, offset, 0x00000002);
> >>> + value = qtest_readl(s, offset);
> >>> + g_assert_cmphex(value, ==, 0x00000002);
> >>> + pin++;
> >>> + }
> >>> + }
> >>> +}
> >>> +
> >>> +static void test_2700_input_pins(const void *data) {
> >>> + QTestState *s = (QTestState *)data;
> >>> + char name[16];
> >>> + uint32_t offset = 0;
> >>> + uint32_t value = 0;
> >>> + uint32_t pin = 0;
> >>> +
> >>> + for (char c = 'A'; c <= 'D'; c++) {
> >>> + for (int i = 0; i < 8; i++) {
> >>> + sprintf(name, "gpio%c%d", c, i);
> >>> + offset = AST2700_GPIO_BASE + GPIOA0_CONTROL +
> (pin *
> >>> +4);
> >>> + /* input direction */
> >>> + qtest_writel(s, offset, 0);
> >>> +
> >>> + /* set input */
> >>> + qtest_qom_set_bool(s, "/machine/soc/gpio", name,
> true);
> >>> + value = qtest_readl(s, offset);
> >>> + g_assert_cmphex(value, ==, 0x00002000);
> >>> +
> >>> + /* clear input */
> >>> + qtest_qom_set_bool(s, "/machine/soc/gpio", name,
> >>> +false);
> >>> + value = qtest_readl(s, offset);
> >>> + g_assert_cmphex(value, ==, 0);
> >>> + pin++;
> >>> + }
> >>> + }
> >>> +}
> >>
> >> As far as I can see, there is nothing in these two functions that
> >> requires any of the other code in this file ...
> >>
> >>> +
> >>> int main(int argc, char **argv)
> >>> {
> >>> + const char *arch = qtest_get_arch();
> >>> QTestState *s;
> >>> int r;
> >>> g_test_init(&argc, &argv, NULL);
> >>> - s = qtest_init("-machine ast2600-evb");
> >>> - qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
> >>> - test_set_colocated_pins);
> >>> - qtest_add_data_func("/ast2600/gpio/set_input_pins", s,
> >>> test_set_input_pins);
> >>> + if (strcmp(arch, "aarch64") == 0) {
> >>> + s = qtest_init("-machine ast2700-evb");
> >>> + qtest_add_data_func("/ast2700/gpio/input_pins",
> >>> + s, test_2700_input_pins);
> >>> + qtest_add_data_func("/ast2700/gpio/out_pins", s,
> >>> test_2700_output_pins);
> >>> + } else {
> >>> + s = qtest_init("-machine ast2600-evb");
> >>> + qtest_add_data_func("/ast2600/gpio/set_colocated_pins", s,
> >>> + test_set_colocated_pins);
> >>> + qtest_add_data_func("/ast2600/gpio/set_input_pins", s,
> >>> + test_set_input_pins);
> >>> + }
> >>
> >> ... so the more I look at this, the more I think your new test should
> >> reside in a separate file that only gets executed for aarch64, while
> >> this file here should stay for arm 32-bit. Or is there a real
> >> compelling reason for putting your code in this file here?
> >
> > Because it is related to the Aspeed GPIO controllers. Unless we want
> > to introduce a new set of test files for 64-bit Aspeed SoC controllers
> > ? why not.
> >
> > I am ok with both options. Option 1 is simpler to implement, but there
> > may be reasons to separate the tests based on architecture, although
> > I'm not aware of any at the moment. Would you rather prefer option 2 ?
> > How should we name the test file ?
>
> Since all arm 32-bit tests are currently completely separate from the
> aarch64 tests, I think a separate file would be better, indeed.
> Simply call it ast2700-gpio-test.c ?
>
Thanks for suggestion.
Will move it into new file, ast2700-gptio-test.c.
Jamin
> Thomas
^ permalink raw reply [flat|nested] 15+ messages in thread