Re: [Qemu-arm] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model

[Rashmica Gupta]

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 :)