Sorry for the late reply! I agree with most of your feedback and will
send out
a v2 shortly with those changes. I have a few replies below
[snip]
> > +static const struct AspeedGPIO gpios[0x1f0] = {
> > + /* Set ABCD */
> > + [GPIO_ABCD_DATA_VALUE] = {0, read_data_value,
> > _write_data_value},
>
> Maybe there would be less tedium (and risk of minor bugs) here if we
> add
> a table for looking up the read/write callbacks from a type, and just
> stash
> the type in struct AspeedGPIO (or a pointer to the corresponding type
> ops),
> e.g:
>
> enum gpio_property { gpio_direction = 0, gpio_int_enable, ... };
>
> struct aspeed_gpio_reg_ops {
> int (*read)(...);
> int (*write)(...);
> };
>
> struct aspeed_gpio_reg_ops gpio_reg_ops[] = {
> [gpio_direction] = { .read = read_direction, .write =
> write_direction };
> ...
> };
>
> Then we have:
>
> static const struct AspeedGPIO gpios[...] = {
> [GPIO_ABCD_DIRECTION] = { .set = 0, .type = gpio_direction },
> ...
> };
>
> Not wedded to the idea, just thinking out loud. What do you think?
>
I'm not a huge fan of the extra indirection. It is less error prone but
is this a place that is likely to be updated regularly?
> > + [GPIO_ABCD_DIRECTION] = {0, read_direction, _write_direction},
> > + [GPIO_ABCD_INT_ENABLE] = {0, read_int_enable,
> > _write_int_enable},
> > + [GPIO_ABCD_INT_SENS_0] = {0, read_int_sens_0,
> > _write_int_sens_0},
> > + [GPIO_ABCD_INT_SENS_1] = {0, read_int_sens_1,
> > _write_int_sens_1},
> > + [GPIO_ABCD_INT_SENS_2] = {0, read_int_sens_2,
> > _write_int_sens_2},
> > + [GPIO_ABCD_INT_STATUS] = {0, read_int_status,
> > _write_int_status},
> > + [GPIO_ABCD_RESET_TOLERANT] = {0, read_reset_tol,
> > _write_reset_tol},
> > + [GPIO_ABCD_DEBOUNCE_1] = {0, read_debounce_1,
> > _write_debounce_1},
> > + [GPIO_ABCD_DEBOUNCE_2] = {0, read_debounce_2,
> > _write_debounce_2},
> > + [GPIO_ABCD_COMMAND_SRC_0] = {0, read_cmd_source_0,
> > _write_cmd_source_0},
> > + [GPIO_ABCD_COMMAND_SRC_1] = {0, read_cmd_source_1,
> > _write_cmd_source_1},
> > + [GPIO_ABCD_DATA_READ] = {0, read_data, NULL},
> > + [GPIO_ABCD_INPUT_MASK] = {0, read_input_mask,
> > _write_input_mask},
> > + /* Set EFGH */
> > + [GPIO_EFGH_DATA_VALUE] = {1, read_data_value,
> > _write_data_value},
> > + [GPIO_EFGH_DIRECTION] = {1, read_direction, _write_direction
> > },
> > + [GPIO_EFGH_INT_ENABLE] = {1, read_int_enable,
> > _write_int_enable},
> > + [GPIO_EFGH_INT_SENS_0] = {1, read_int_sens_0,
> > _write_int_sens_0},
> > + [GPIO_EFGH_INT_SENS_1] = {1, read_int_sens_1,
> > _write_int_sens_1},
> > + [GPIO_EFGH_INT_SENS_2] = {1, read_int_sens_2,
> > _write_int_sens_2},
> > + [GPIO_EFGH_INT_STATUS] = {1, read_int_status,
> > _write_int_status},
> > + [GPIO_EFGH_RESET_TOL] = {1,
> > read_reset_tol, _write_reset_tol},
> > + [GPIO_EFGH_DEBOUNCE_1] = {1,
> > read_debounce_1, _write_debounce_1},
> > + [GPIO_EFGH_DEBOUNCE_2] = {1,
> > read_debounce_2, _write_debounce_2},
> > + [GPIO_EFGH_COMMAND_SRC_0] = {1, read_cmd_source_0,
> > _write_cmd_source_0},
> > + [GPIO_EFGH_COMMAND_SRC_1] = {1, read_cmd_source_1,
> > _write_cmd_source_1},
> > + [GPIO_EFGH_DATA_READ] = {1, read_data, NULL},
> > + [GPIO_EFGH_INPUT_MASK] = {1,
> > read_input_mask, _write_input_mask},
> > + /* Set IJKL */
> > + [GPIO_IJKL_DATA_VALUE] = {2, read_data_value,
> > _write_data_value},
> > + [GPIO_IJKL_DIRECTION] = {2, read_direction, _write_direction},
> > + [GPIO_IJKL_INT_ENABLE] = {2, read_int_enable,
> > _write_int_enable},
> > + [GPIO_IJKL_INT_SENS_0] = {2, read_int_sens_0,
> > _write_int_sens_0},
> > + [GPIO_IJKL_INT_SENS_1] = {2, read_int_sens_1,
> > _write_int_sens_1},
> > + [GPIO_IJKL_INT_SENS_2] = {2, read_int_sens_2,
> > _write_int_sens_2},
> > + [GPIO_IJKL_INT_STATUS] = {2, read_int_status,
> > _write_int_status},
> > + [GPIO_IJKL_RESET_TOLERANT] = {2, read_reset_tol,
> > _write_reset_tol},
> > + [GPIO_IJKL_DEBOUNCE_1] = {2, read_debounce_1,
> > _write_debounce_1},
> > + [GPIO_IJKL_DEBOUNCE_2] = {2, read_debounce_2,
> > _write_debounce_2},
> > + [GPIO_IJKL_COMMAND_SRC_0] = {2, read_cmd_source_0,
> > _write_cmd_source_0},
> > + [GPIO_IJKL_COMMAND_SRC_1] = {2, read_cmd_source_1,
> > _write_cmd_source_1},
> > + [GPIO_IJKL_DATA_READ] = {2, read_data, NULL},
> > + [GPIO_IJKL_INPUT_MASK] = {2, read_input_mask,
> > _write_input_mask},
> > + /* Set MNOP */
> > + [GPIO_MNOP_DATA_VALUE] = {3, read_data_value,
> > _write_data_value},
> > + [GPIO_MNOP_DIRECTION] = {3, read_direction, _write_direction},
> > + [GPIO_MNOP_INT_ENABLE] = {3, read_int_enable,
> > _write_int_enable},
> > + [GPIO_MNOP_INT_SENS_0] = {3, read_int_sens_0,
> > _write_int_sens_0},
> > + [GPIO_MNOP_INT_SENS_1] = {3, read_int_sens_1,
> > _write_int_sens_1},
> > + [GPIO_MNOP_INT_SENS_2] = {3, read_int_sens_2,
> > _write_int_sens_2},
> > + [GPIO_MNOP_INT_STATUS] = {3, read_int_status,
> > _write_int_status},
> > + [GPIO_MNOP_RESET_TOLERANT] = {3, read_reset_tol,
> > _write_reset_tol},
> > + [GPIO_MNOP_DEBOUNCE_1] = {3, read_debounce_1,
> > _write_debounce_1},
> > + [GPIO_MNOP_DEBOUNCE_2] = {3, read_debounce_2,
> > _write_debounce_2},
> > + [GPIO_MNOP_COMMAND_SRC_0] = {3, read_cmd_source_0,
> > _write_cmd_source_0},
> > + [GPIO_MNOP_COMMAND_SRC_1] = {3, read_cmd_source_1,
> > _write_cmd_source_1},
> > + [GPIO_MNOP_DATA_READ] = {3, read_data, NULL},
> > + [GPIO_MNOP_INPUT_MASK] = {3, read_input_mask,
> > _write_input_mask},
> > + /* Set QRST */
> > + [GPIO_QRST_DATA_VALUE] = {4, read_data_value,
> > _write_data_value},
> > + [GPIO_QRST_DIRECTION] = {4, read_direction, _write_direction},
> > + [GPIO_QRST_INT_ENABLE] = {4, read_int_enable,
> > _write_int_enable},
> > + [GPIO_QRST_INT_SENS_0] = {4, read_int_sens_0,
> > _write_int_sens_0},
> > + [GPIO_QRST_INT_SENS_1] = {4, read_int_sens_1,
> > _write_int_sens_1},
> > + [GPIO_QRST_INT_SENS_2] = {4, read_int_sens_2,
> > _write_int_sens_2},
> > + [GPIO_QRST_INT_STATUS] = {4, read_int_status,
> > _write_int_status},
> > + [GPIO_QRST_RESET_TOLERANT] = {4, read_reset_tol,
> > _write_reset_tol},
> > + [GPIO_QRST_DEBOUNCE_1] = {4, read_debounce_1,
> > _write_debounce_1},
> > + [GPIO_QRST_DEBOUNCE_2] = {4, read_debounce_2,
> > _write_debounce_2},
> > + [GPIO_QRST_COMMAND_SRC_0] = {4, read_cmd_source_0,
> > _write_cmd_source_0},
> > + [GPIO_QRST_COMMAND_SRC_1] = {4, read_cmd_source_1,
> > _write_cmd_source_1},
> > + [GPIO_QRST_DATA_READ] = {4, read_data, NULL},
> > + [GPIO_QRST_INPUT_MASK] = {4,
> > read_input_mask, _write_input_mask},
> > + /* Set UVWX */
> > + [GPIO_UVWX_DATA_VALUE] = {5, read_data_value,
> > _write_data_value},
> > + [GPIO_UWVX_DIRECTION] = {5, read_direction, _write_direction},
> > + [GPIO_UVWX_INT_ENABLE] = {5, read_int_enable,
> > _write_int_enable},
> > + [GPIO_UVWX_INT_SENS_0] = {5, read_int_sens_0,
> > _write_int_sens_0},
> > + [GPIO_UVWX_INT_SENS_1] = {5, read_int_sens_1,
> > _write_int_sens_1},
> > + [GPIO_UVWX_INT_SENS_2] = {5, read_int_sens_2,
> > _write_int_sens_2},
> > + [GPIO_UVWX_INT_STATUS] = {5, read_int_status,
> > _write_int_status},
> > + [GPIO_UVWX_RESET_TOLERANT] = {5, read_reset_tol,
> > _write_reset_tol},
> > + [GPIO_UVWX_DEBOUNCE_1] = {5, read_debounce_1,
> > _write_debounce_1},
> > + [GPIO_UVWX_DEBOUNCE_2] = {5, read_debounce_2,
> > _write_debounce_2},
> > + [GPIO_UVWX_COMMAND_SRC_0] = {5, read_cmd_source_0,
> > _write_cmd_source_0},
> > + [GPIO_UVWX_COMMAND_SRC_1] = {5, read_cmd_source_1,
> > _write_cmd_source_1},
> > + [GPIO_UVWX_DATA_READ] = {5, read_data, NULL},
> > + [GPIO_UVWX_INPUT_MASK] = {5, read_input_mask,
> > _write_input_mask},
> > + /* Set YZAAAB */
> > + [GPIO_YZAAAB_DATA_VALUE] = {6, read_data_value,
> > _write_data_value},
> > + [GPIO_YZAAAB_DIRECTION] = {6, read_direction,
> > _write_direction},
> > + [GPIO_YZAAAB_INT_ENABLE] = {6, read_int_enable,
> > _write_int_enable},
> > + [GPIO_YZAAAB_INT_SENS_0] = {6, read_int_sens_0,
> > _write_int_sens_0},
> > + [GPIO_YZAAAB_INT_SENS_1] = {6, read_int_sens_1,
> > _write_int_sens_1},
> > + [GPIO_YZAAAB_INT_SENS_2] = {6, read_int_sens_2,
> > _write_int_sens_2},
> > + [GPIO_YZAAAB_INT_STATUS] = {6, read_int_status,
> > _write_int_status},
> > + [GPIO_YZAAAB_RESET_TOLERANT] = {6, read_reset_tol,
> > _write_reset_tol},
> > + [GPIO_YZAAAB_DEBOUNCE_1] = {6, read_debounce_1,
> > _write_debounce_1},
> > + [GPIO_YZAAAB_DEBOUNCE_2] = {6, read_debounce_2,
> > _write_debounce_2},
> > + [GPIO_YZAAAB_COMMAND_SRC_0] = {6, read_cmd_source_0,
> > _write_cmd_source_0},
> > + [GPIO_YZAAAB_COMMAND_SRC_1] = {6, read_cmd_source_1,
> > _write_cmd_source_1},
> > + [GPIO_YZAAAB_DATA_READ] = {6, read_data, NULL},
> > + [GPIO_YZAAAB_INPUT_MASK] = {6, read_input_mask,
> > _write_input_mask},
> > + /* Set AC */
> > + [GPIO_AC_DATA_VALUE] = {7, read_data_value,
> > _write_data_value},
> > + [GPIO_AC_DIRECTION] = {7, read_direction, _write_direction},
> > + [GPIO_AC_INT_ENABLE] = {7, read_int_enable,
> > _write_int_enable},
> > + [GPIO_AC_INT_SENS_0] = {7, read_int_sens_0,
> > _write_int_sens_0},
> > + [GPIO_AC_INT_SENS_1] = {7, read_int_sens_1,
> > _write_int_sens_1},
> > + [GPIO_AC_INT_SENS_2] = {7, read_int_sens_2,
> > _write_int_sens_2},
> > + [GPIO_AC_INT_STATUS] = {7, read_int_status,
> > _write_int_status},
> > + [GPIO_AC_RESET_TOLERANT] = {7, read_reset_tol,
> > _write_reset_tol},
> > + [GPIO_AC_DEBOUNCE_1] = {7, read_debounce_1,
> > _write_debounce_1},
> > + [GPIO_AC_DEBOUNCE_2] = {7, read_debounce_2,
> > _write_debounce_2},
> > + [GPIO_AC_COMMAND_SRC_0] = {7, read_cmd_source_0,
> > _write_cmd_source_0},
> > + [GPIO_AC_COMMAND_SRC_1] = {7, read_cmd_source_1,
> > _write_cmd_source_1},
> > + [GPIO_AC_DATA_READ] = {7, read_data, NULL},
> > + [GPIO_AC_INPUT_MASK] = {7,
> > read_input_mask, _write_input_mask},
> > + /* Debounce registers */
> > + [GPIO_DEBOUNCE_TIME_1] = {-1, NULL, NULL},
> > +};
> > +
> > +static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset,
> > uint32_t
> > size)
> > +{
> > + AspeedGPIOState *s = ASPEED_GPIO(opaque);
> > + uint32_t val = 0;
> > +
> > + if (size != 4) {
> > + return 0;
> > + }
> > +
> > + if (gpios[offset].get == NULL) {
>
> With the AST2600 we might need to make gpios class-specific rather
> than
> a static global to the compilation unit (to deal with the 1.8V GPIO
> registers
> later in the address-space). Not sure though, haven't thought about
> it for
> more than this paragraph.
Yup, I agree. Still trying to sort out a nice way to deal with the
AST2600 differences so I figured I'd leave it like this for now?
>
> > + qemu_log_mask(LOG_GUEST_ERROR, "no getter for offset
> > %lx",
> > offset);
>
> Really this is a model error. We should only encounter it if we
> haven't
> completely described the controller. So maybe it should be an assert
> or some kind of non-guest warning?
>
I don't think this is quite true. There are values in the gpio memory
space
that do not have a register associated with them, ie registers with
offsets 0x138 and 0x140 exist, but there is no 0x13c. So if the
kernel tries to access this address, this isn't really a gpio model
error?
> > + return 0;
> > + }
> > +
> > + val = gpios[offset].get(&s->sets[gpios[offset].set_idx]);
>
> The indexing here is starting to get a bit hard to follow. I think we
> could
> improve readability by introducing some locals, and rewriting in
> terms
> of those:
>
> const struct AspeedGPIO *reg = &gpios[offset];
> const struct GPIORegs *set = &s->sets[gpio.set_idx];
>
> if (!reg.get) {
> ...
> }
>
> val = reg.get(set);
>
> I think it makes the relationship of the structs a bit clearer too,
> and I think
> it also suggests we could improve the names:
>
> s/struct AspeedGPIO/struct AspeedGPIOReg/
> s/struct GPIORegs/struct GPIOProperties/
>
I agree with the local variables, and renaming AspeedGPIO-
>AspeedGPIOReg
but I think GPIOSet makes more sense than GPIOProperties.
> GPIOProperties clashes a bit with GPIOSetProperties though, so maybe
> we
> need to think of a better name for that too?
>
> > + return (uint64_t) val;
> > +}
> > +
> > +static void aspeed_gpio_write(void *opaque, hwaddr offset,
> > uint64_t
> > data,
> > +static GPIOSetProperties ast2400_set_props[] = {
[snip]
> > + [0] = {0xffffffff, 0xffffffff, {"A", "B", "C", "D"} },
> > + [1] = {0xffffffff, 0xffffffff, {"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, 0x0000ffff, {"U", "V", "W", "X"} },
> > + [6] = {0x0000000f, 0x0fffff0f, {"Y", "Z", "AA", "AB"} },
> > +};
> > +
> > +static GPIOSetProperties ast2500_set_props[] = {
> > + [0] = {0xffffffff, 0xffffffff, {"A", "B", "C", "D"} },
> > + [1] = {0xffffffff, 0xffffffff, {"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, 0x0000ffff, {"U", "V", "W", "X"} },
> > + [6] = {0xffffff0f, 0x0fffff0f, {"Y", "Z", "AA", "AB"} },
> > + [7] = {0x000000ff, 0x000000ff, {"AC"} },
> > +};
> > +
> > +static AspeedGPIOController controllers[] = {
> > + {
> > + .name = TYPE_ASPEED_GPIO "-ast2500",
> > + .props = ast2500_set_props,
> > + .nr_gpio_pins = 228,
> > + .nr_gpio_sets = 8,
> > + .gap = 220,
> > + }, {
> > + .name = TYPE_ASPEED_GPIO "-ast2400",
> > + .props = ast2400_set_props,
> > + .nr_gpio_pins = 216,
> > + .nr_gpio_sets = 7,
> > + .gap = 196,
> > + }
> > +};
> > +
> > +static const TypeInfo aspeed_gpio_info = {
> > + .name = TYPE_ASPEED_GPIO,
> > + .parent = TYPE_SYS_BUS_DEVICE,
> > + .instance_size = sizeof(AspeedGPIOState),
> > + .class_size = sizeof(AspeedGPIOClass),
> > + .instance_init = aspeed_gpio_init,
> > + .abstract = true,
> > +};
> > +
> > +static void aspeed_gpio_register_types(void)
> > +{
> > + int i;
> > +
> > + type_register_static(&aspeed_gpio_info);
> > + for (i = 0; i < ARRAY_SIZE(controllers); i++) {
> > + TypeInfo t = {
> > + .name = controllers[i].name,
> > + .parent = TYPE_ASPEED_GPIO,
> > + .class_init = aspeed_gpio_class_init,
> > + .class_data = (void *)&controllers[i],
> > + };
> > + type_register(&t);
> > + };
>
> I think we want separate classes for the 2400 and 2500, especially
> with
> the 2600 on the way.
I thought this was defining seperate classes that just happen to have
the same init function? I don't fully grasp classes in qemu yet, so
likely to be wrong :)
