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