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_...@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
