| Message ID | 20240923094206.1455783-5-jamin_lin@aspeedtech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support GPIO for AST2700 | expand |
Hi Jamin, On Mon, 2024-09-23 at 17:42 +0800, Jamin Lin wrote: > 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/aspeed_gpio_write callback functions > are not compatible AST2700. > Introduce new aspeed_gpio_2700_read/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> > --- > hw/gpio/aspeed_gpio.c | 373 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 373 insertions(+) > > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c > index f23ffae34d..e3d5556dc1 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; > @@ -964,6 +996,309 @@ 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_read_control_reg(AspeedGPIOState *s, uint32_t pin) This function is specific to the AST2700 and I think the name should reflect that. > +{ > + 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_write_control_reg(AspeedGPIOState *s, Also should reflect it's specific to the AST2700? > + uint32_t pin, uint32_t type, uint64_t data) > +{ > + AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s); > + const GPIOSetProperties *props; > + GPIOSets *set; > + uint32_t cleared; > + uint32_t set_idx; > + uint32_t pin_idx; > + uint32_t group_value = 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 */ > + group_value = set->int_status; > + group_value = deposit32(group_value, pin_idx, 1, > + SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS)); This makes me a bit wary. 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 group_value. However, if the extracted bit is clear then the value of the corresponding bit in group_value should be unchanged. SHARED_FIELD_EX32() extracts the interrupt status bit from the write (data). group_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 group, using the value extracted from the write (data). However, the result is that if the interrupt was pending and the write was acknowledging it, then the update has no effect. Alternatively, if the interrupt was pending but the write was acknowledging it, then the update will mark the interrupt as pending. Or, if the interrupt was pending but the write was _not_ acknowledging it, then the interrupt will _no longer_ be marked pending. If this is intentional it feels a bit hard to follow. > + cleared = ctpop32(group_value & set->int_status); Can this rather be expressed as ``` cleared = SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS); ``` > + if (s->pending && cleared) { > + assert(s->pending >= cleared); > + s->pending -= cleared; We're only ever going to be subtracting 1, as each GPIO has its own register. This feels overly abstract. > + } > + set->int_status &= ~group_value; This feels like it misbehaves in the face of multiple pending interrupts. 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: group_value == 0b11 cleared == 2 s->pending = 0 set->int_status == 0b00 It seems the pending interrupt for GPIOA1 is lost? > + > + 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_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 type; > + 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_write_control_reg(s, pin, type, data); > + break; > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset 0x%" > + PRIx64"\n", __func__, offset); > + break; > + } > + > + return; > +} > + > /****************** Setup functions ******************/ Bit of a nitpick, but I'm not personally a fan of banner comments like this. Andrew
Hi Andrew, > Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support > > Hi Jamin, > > On Mon, 2024-09-23 at 17:42 +0800, Jamin Lin wrote: > > 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/aspeed_gpio_write callback functions are > > not compatible AST2700. > > Introduce new aspeed_gpio_2700_read/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> > > --- > > hw/gpio/aspeed_gpio.c | 373 > > ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 373 insertions(+) > > > > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c index > > f23ffae34d..e3d5556dc1 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; @@ -964,6 +996,309 @@ > > 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_read_control_reg(AspeedGPIOState *s, > > +uint32_t pin) > > This function is specific to the AST2700 and I think the name should reflect > that. > Will rename it. > > +{ > > + 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_write_control_reg(AspeedGPIOState *s, > > Also should reflect it's specific to the AST2700? > Will rename it. > > + uint32_t pin, uint32_t type, uint64_t data) { > > + AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s); > > + const GPIOSetProperties *props; > > + GPIOSets *set; > > + uint32_t cleared; > > + uint32_t set_idx; > > + uint32_t pin_idx; > > + uint32_t group_value = 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 */ > > + group_value = set->int_status; > > + group_value = deposit32(group_value, pin_idx, 1, > > + SHARED_FIELD_EX32(data, > > + GPIO_CONTROL_INT_STATUS)); > > This makes me a bit wary. > > 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 group_value. However, if the extracted bit is clear then > the value of the corresponding bit in group_value should be unchanged. > > SHARED_FIELD_EX32() extracts the interrupt status bit from the write (data). > group_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 group, using the value extracted from the > write (data). > > However, the result is that if the interrupt was pending and the write was > acknowledging it, then the update has no effect. Alternatively, if the interrupt > was pending but the write was acknowledging it, then the update will mark the > interrupt as pending. Or, if the interrupt was pending but the write was _not_ > acknowledging it, then the interrupt will _no longer_ be marked pending. If > this is intentional it feels a bit hard to follow. > > > + cleared = ctpop32(group_value & set->int_status); > > Can this rather be expressed as > > ``` > cleared = SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS); ``` > > > + if (s->pending && cleared) { > > + assert(s->pending >= cleared); > > + s->pending -= cleared; > > We're only ever going to be subtracting 1, as each GPIO has its own register. > This feels overly abstract. > > > + } > > + set->int_status &= ~group_value; > > This feels like it misbehaves in the face of multiple pending interrupts. > > 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: > > group_value == 0b11 > cleared == 2 > s->pending = 0 > set->int_status == 0b00 > > It seems the pending interrupt for GPIOA1 is lost? > Thanks for review and input. I should check "int_status" bit of write data in write callback function. If 1 clear status flag(group value), else should not change group value. I am checking and testing this issue and will update to you or directly resend the new patch series. > > + > > + 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_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 type; > > + 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_write_control_reg(s, pin, type, data); > > + break; > > + default: > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset > 0x%" > > + PRIx64"\n", __func__, offset); > > + break; > > + } > > + > > + return; > > +} > > + > > /****************** Setup functions ******************/ > > Bit of a nitpick, but I'm not personally a fan of banner comments like this. > Did you mean change as following? A. /************ Setup functions *****************/ 1. /* Setup functions */ 2. /* * Setup functions */ Thanks-Jamin > Andrew
Hi Andrew, > Subject: RE: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support > > Hi Andrew, > > > Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support > > > > Hi Jamin, > > > > On Mon, 2024-09-23 at 17:42 +0800, Jamin Lin wrote: > > > 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/aspeed_gpio_write callback functions are > > > not compatible AST2700. > > > Introduce new aspeed_gpio_2700_read/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> > > > --- > > > hw/gpio/aspeed_gpio.c | 373 > > > ++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 373 insertions(+) > > > > > > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c index > > > f23ffae34d..e3d5556dc1 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; @@ -964,6 +996,309 > > > @@ 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_read_control_reg(AspeedGPIOState *s, > > > +uint32_t pin) > > > > This function is specific to the AST2700 and I think the name should > > reflect that. > > > Will rename it. > > > > +{ > > > + 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_write_control_reg(AspeedGPIOState *s, > > > > Also should reflect it's specific to the AST2700? > > > Will rename it. > > > + uint32_t pin, uint32_t type, uint64_t data) { > > > + AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s); > > > + const GPIOSetProperties *props; > > > + GPIOSets *set; > > > + uint32_t cleared; > > > + uint32_t set_idx; > > > + uint32_t pin_idx; > > > + uint32_t group_value = 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 */ > > > + group_value = set->int_status; > > > + group_value = deposit32(group_value, pin_idx, 1, > > > + SHARED_FIELD_EX32(data, > > > + GPIO_CONTROL_INT_STATUS)); > > > > This makes me a bit wary. > > > > 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 group_value. However, if the > > extracted bit is clear then the value of the corresponding bit in group_value > should be unchanged. > > > > SHARED_FIELD_EX32() extracts the interrupt status bit from the write (data). > > group_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 group, using the > > value extracted from the write (data). > > > > However, the result is that if the interrupt was pending and the write > > was acknowledging it, then the update has no effect. Alternatively, if > > the interrupt was pending but the write was acknowledging it, then the > > update will mark the interrupt as pending. Or, if the interrupt was > > pending but the write was _not_ acknowledging it, then the interrupt > > will _no longer_ be marked pending. If this is intentional it feels a bit hard to > follow. > > > > > + cleared = ctpop32(group_value & set->int_status); > > > > Can this rather be expressed as > > > > ``` > > cleared = SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS); ``` > > > > > + if (s->pending && cleared) { > > > + assert(s->pending >= cleared); > > > + s->pending -= cleared; > > > > We're only ever going to be subtracting 1, as each GPIO has its own register. > > This feels overly abstract. > > > > > + } > > > + set->int_status &= ~group_value; > > > > This feels like it misbehaves in the face of multiple pending interrupts. > > > > 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: > > > > group_value == 0b11 > > cleared == 2 > > s->pending = 0 > > set->int_status == 0b00 > > > > It seems the pending interrupt for GPIOA1 is lost? > > > Thanks for review and input. > I should check "int_status" bit of write data in write callback function. If 1 clear > status flag(group value), else should not change group value. > I am checking and testing this issue and will update to you or directly resend > the new patch series. I appreciate your review and finding this issue. My changes as following. If you agree, I will add them in v2 patch. Thanks-Jamin static void aspeed_gpio_2700_write_control_reg(AspeedGPIOState *s, uint32_t pin, uint32_t type, uint64_t data) { --- /* interrupt status */ if (SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS)) { cleared = extract32(set->int_status, pin_idx, 1); if (cleared) { if (s->pending) { assert(s->pending >= cleared); s->pending -= cleared; } set->int_status = deposit32(set->int_status, pin_idx, 1, 0); } } ---- } By the way, I found the same issue in "aspeed_gpio_write_index_mode" and my changes as following. If you agree this change, I will create a new patch in v2 patch series. static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset, uint64_t data, uint32_t size) { --- case gpio_reg_idx_interrupt: if (FIELD_EX32(data, GPIO_INDEX_REG, INT_STATUS)) { cleared = extract32(set->int_status, pin_idx, 1); if (cleared) { if (s->pending) { assert(s->pending >= cleared); s->pending -= cleared; } set->int_status = deposit32(set->int_status, pin_idx, 1, 0); } } break; --- } Thanks-Jamin > > > + > > > + 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_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 type; > > > + 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_write_control_reg(s, pin, type, data); > > > + break; > > > + default: > > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset > > 0x%" > > > + PRIx64"\n", __func__, offset); > > > + break; > > > + } > > > + > > > + return; > > > +} > > > + > > > /****************** Setup functions ******************/ > > > > Bit of a nitpick, but I'm not personally a fan of banner comments like this. > > > Did you mean change as following? > > A. > > /************ Setup functions *****************/ > > 1. /* Setup functions */ > 2. /* > * Setup functions > */ > > Thanks-Jamin > > > Andrew
On Tue, 2024-09-24 at 03:03 +0000, Jamin Lin wrote: > Hi Andrew, > > > Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support > > > > Hi Jamin, > > > > On Mon, 2024-09-23 at 17:42 +0800, Jamin Lin wrote: > > > > > + > > > + /* interrupt status */ > > > + group_value = set->int_status; > > > + group_value = deposit32(group_value, pin_idx, 1, > > > + SHARED_FIELD_EX32(data, > > > + GPIO_CONTROL_INT_STATUS)); > > > > This makes me a bit wary. > > > > 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 group_value. However, if the extracted bit is clear then > > the value of the corresponding bit in group_value should be unchanged. > > > > SHARED_FIELD_EX32() extracts the interrupt status bit from the write (data). > > group_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 group, using the value extracted from the > > write (data). > > > > However, the result is that if the interrupt was pending and the write was > > acknowledging it, then the update has no effect. Alternatively, if the interrupt > > was pending but the write was acknowledging it, then the update will mark the > > interrupt as pending. Or, if the interrupt was pending but the write was _not_ > > acknowledging it, then the interrupt will _no longer_ be marked pending. If > > this is intentional it feels a bit hard to follow. > > > > > + cleared = ctpop32(group_value & set->int_status); > > > > Can this rather be expressed as > > > > ``` > > cleared = SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS); ``` > > > > > + if (s->pending && cleared) { > > > + assert(s->pending >= cleared); > > > + s->pending -= cleared; > > > > We're only ever going to be subtracting 1, as each GPIO has its own register. > > This feels overly abstract. > > > > > + } > > > + set->int_status &= ~group_value; > > > > This feels like it misbehaves in the face of multiple pending interrupts. > > > > 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: > > > > group_value == 0b11 > > cleared == 2 > > s->pending = 0 > > set->int_status == 0b00 > > > > It seems the pending interrupt for GPIOA1 is lost? > > > Thanks for review and input. > I should check "int_status" bit of write data in write callback function. If 1 clear status flag(group value), else should not change group value. > I am checking and testing this issue and will update to you or directly resend the new patch series. Happy to take a look in a v2 of the series :) > > > + > > > /****************** Setup functions ******************/ > > > > Bit of a nitpick, but I'm not personally a fan of banner comments like this. > > > Did you mean change as following? > > A. > > /************ Setup functions *****************/ > > 1. /* Setup functions */ > 2. /* > * Setup functions > */ Either is fine, but I prefer 1. Cheers, Andrew
On Tue, 2024-09-24 at 06:48 +0000, Jamin Lin wrote: > Hi Andrew, > > > Subject: RE: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support > > > > Hi Andrew, > > > > > Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support > > > > > > Hi Jamin, > > > > > > > > > > + } > > > > + set->int_status &= ~group_value; > > > > > > This feels like it misbehaves in the face of multiple pending interrupts. > > > > > > 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: > > > > > > group_value == 0b11 > > > cleared == 2 > > > s->pending = 0 > > > set->int_status == 0b00 > > > > > > It seems the pending interrupt for GPIOA1 is lost? > > > > > Thanks for review and input. > > I should check "int_status" bit of write data in write callback function. If 1 clear > > status flag(group value), else should not change group value. > > I am checking and testing this issue and will update to you or directly resend > > the new patch series. > > I appreciate your review and finding this issue. > My changes as following. > If you agree, I will add them in v2 patch. > Thanks-Jamin > > static void aspeed_gpio_2700_write_control_reg(AspeedGPIOState *s, > uint32_t pin, uint32_t type, uint64_t data) > { > --- > /* interrupt status */ > if (SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS)) { > cleared = extract32(set->int_status, pin_idx, 1); > if (cleared) { > if (s->pending) { > assert(s->pending >= cleared); > s->pending -= cleared; > } > set->int_status = deposit32(set->int_status, pin_idx, 1, 0); > } > } > ---- > } The logic is easier to follow. Not sure about calling the value extracted from set->int_status 'cleared' though, seems confusing on first pass. It would feel more appropriate if it were called 'pending'. I think 'cleared' is derived from `SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS)`. Anyway, that's just some quibbling over names. > > By the way, I found the same issue in "aspeed_gpio_write_index_mode" and my changes as following. > If you agree this change, I will create a new patch in v2 patch series. > > static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset, > uint64_t data, uint32_t size) > { > --- > case gpio_reg_idx_interrupt: > if (FIELD_EX32(data, GPIO_INDEX_REG, INT_STATUS)) { > cleared = extract32(set->int_status, pin_idx, 1); > if (cleared) { > if (s->pending) { > assert(s->pending >= cleared); > s->pending -= cleared; > } > set->int_status = deposit32(set->int_status, pin_idx, 1, 0); > } > } > break; > --- > } I'll take a look in v2. Cheers, Andrew
Hi Andrew, > Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support > > On Tue, 2024-09-24 at 06:48 +0000, Jamin Lin wrote: > > Hi Andrew, > > > > > Subject: RE: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support > > > > > > Hi Andrew, > > > > > > > Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support > > > > > > > > Hi Jamin, > > > > > > > > > > > > > + } > > > > > + set->int_status &= ~group_value; > > > > > > > > This feels like it misbehaves in the face of multiple pending interrupts. > > > > > > > > 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: > > > > > > > > group_value == 0b11 > > > > cleared == 2 > > > > s->pending = 0 > > > > set->int_status == 0b00 > > > > > > > > It seems the pending interrupt for GPIOA1 is lost? > > > > > > > Thanks for review and input. > > > I should check "int_status" bit of write data in write callback > > > function. If 1 clear status flag(group value), else should not change group > value. > > > I am checking and testing this issue and will update to you or > > > directly resend the new patch series. > > > > I appreciate your review and finding this issue. > > My changes as following. > > If you agree, I will add them in v2 patch. > > Thanks-Jamin > > > > static void aspeed_gpio_2700_write_control_reg(AspeedGPIOState *s, > > uint32_t pin, uint32_t type, uint64_t > > data) { > > --- > > /* interrupt status */ > > if (SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS)) { > > cleared = extract32(set->int_status, pin_idx, 1); > > if (cleared) { > > if (s->pending) { > > assert(s->pending >= cleared); > > s->pending -= cleared; > > } > > set->int_status = deposit32(set->int_status, pin_idx, 1, 0); > > } > > } > > ---- > > } > > The logic is easier to follow. Not sure about calling the value extracted from > set->int_status 'cleared' though, seems confusing on first pass. It would feel > more appropriate if it were called 'pending'. > I think 'cleared' is derived from `SHARED_FIELD_EX32(data, > GPIO_CONTROL_INT_STATUS)`. Anyway, that's just some quibbling over > names. Got it. Will update it. Thanks for suggestion and review. > > > > > By the way, I found the same issue in "aspeed_gpio_write_index_mode" and > my changes as following. > > If you agree this change, I will create a new patch in v2 patch series. > > > > static void aspeed_gpio_write_index_mode(void *opaque, hwaddr offset, > > uint64_t data, > > uint32_t size) { > > --- > > case gpio_reg_idx_interrupt: > > if (FIELD_EX32(data, GPIO_INDEX_REG, INT_STATUS)) { > > cleared = extract32(set->int_status, pin_idx, 1); > > if (cleared) { > > if (s->pending) { > > assert(s->pending >= cleared); > > s->pending -= cleared; > > } > > set->int_status = deposit32(set->int_status, pin_idx, 1, > 0); > > } > > } > > break; > > --- > > } > > I'll take a look in v2. > > Cheers, > > Andrew
Hi Andrew, > Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support > > On Tue, 2024-09-24 at 03:03 +0000, Jamin Lin wrote: > > Hi Andrew, > > > > > Subject: Re: [PATCH 4/5] hw/gpio/aspeed: Add AST2700 support > > > > > > Hi Jamin, > > > > > > On Mon, 2024-09-23 at 17:42 +0800, Jamin Lin wrote: > > > > > > > + > > > > + /* interrupt status */ > > > > + group_value = set->int_status; > > > > + group_value = deposit32(group_value, pin_idx, 1, > > > > + SHARED_FIELD_EX32(data, > > > > + GPIO_CONTROL_INT_STATUS)); > > > > > > This makes me a bit wary. > > > > > > 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 group_value. However, if the > > > extracted bit is clear then the value of the corresponding bit in group_value > should be unchanged. > > > > > > SHARED_FIELD_EX32() extracts the interrupt status bit from the write > (data). > > > group_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 group, using > > > the value extracted from the write (data). > > > > > > However, the result is that if the interrupt was pending and the > > > write was acknowledging it, then the update has no effect. > > > Alternatively, if the interrupt was pending but the write was > > > acknowledging it, then the update will mark the interrupt as > > > pending. Or, if the interrupt was pending but the write was _not_ > > > acknowledging it, then the interrupt will _no longer_ be marked pending. If > this is intentional it feels a bit hard to follow. > > > > > > > + cleared = ctpop32(group_value & set->int_status); > > > > > > Can this rather be expressed as > > > > > > ``` > > > cleared = SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS); ``` > > > > > > > + if (s->pending && cleared) { > > > > + assert(s->pending >= cleared); > > > > + s->pending -= cleared; > > > > > > We're only ever going to be subtracting 1, as each GPIO has its own > register. > > > This feels overly abstract. > > > > > > > + } > > > > + set->int_status &= ~group_value; > > > > > > This feels like it misbehaves in the face of multiple pending interrupts. > > > > > > 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: > > > > > > group_value == 0b11 > > > cleared == 2 > > > s->pending = 0 > > > set->int_status == 0b00 > > > > > > It seems the pending interrupt for GPIOA1 is lost? > > > > > Thanks for review and input. > > I should check "int_status" bit of write data in write callback function. If 1 > clear status flag(group value), else should not change group value. > > I am checking and testing this issue and will update to you or directly resend > the new patch series. > > Happy to take a look in a v2 of the series :) > > > > > + > > > > /****************** Setup functions ******************/ > > > > > > Bit of a nitpick, but I'm not personally a fan of banner comments like this. > > > > > Did you mean change as following? > > > > A. > > > > /************ Setup functions *****************/ > > > > 1. /* Setup functions */ > > 2. /* > > * Setup functions > > */ > > Either is fine, but I prefer 1. > Thanks for suggestion. Will update it in V2 Thanks-Jamin > Cheers, > > Andrew
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c index f23ffae34d..e3d5556dc1 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; @@ -964,6 +996,309 @@ 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_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_write_control_reg(AspeedGPIOState *s, + uint32_t pin, uint32_t type, uint64_t data) +{ + AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s); + const GPIOSetProperties *props; + GPIOSets *set; + uint32_t cleared; + uint32_t set_idx; + uint32_t pin_idx; + uint32_t group_value = 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 */ + group_value = set->int_status; + group_value = deposit32(group_value, pin_idx, 1, + SHARED_FIELD_EX32(data, GPIO_CONTROL_INT_STATUS)); + cleared = ctpop32(group_value & set->int_status); + if (s->pending && cleared) { + assert(s->pending >= cleared); + s->pending -= cleared; + } + set->int_status &= ~group_value; + + 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_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 type; + 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_write_control_reg(s, pin, type, 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"} }, @@ -1010,6 +1345,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, @@ -1018,6 +1363,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); @@ -1187,6 +1540,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, @@ -1231,6 +1596,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); @@ -1239,6 +1611,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);
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/aspeed_gpio_write callback functions are not compatible AST2700. Introduce new aspeed_gpio_2700_read/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> --- hw/gpio/aspeed_gpio.c | 373 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 373 insertions(+)