qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Andrew Jeffery
Subject: Re: [Qemu-arm] [PATCH 1/2] hw/gpio: Add basic Aspeed GPIO model
Date: Mon, 15 Jul 2019 12:13:26 +0930
User-agent: Cyrus-JMAP/3.1.6-731-g19d3b16-fmstable-20190627v1


On Mon, 15 Jul 2019, at 12:06, Rashmica Gupta wrote:
> 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?

Right - if you've written correctly already then there's not much benefit.

> 
> 
> > > +    [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? 

Okay.

> 
> > 
> > > +        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? 

Ah, clearly I hadn't looked closely enough :) Well spotted.

> 
> 
> > > +        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.
> 

Okay.

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

Yeah, I'm a bit out of the loop here too, was just expecting to see explicit
typedef names for each variant and didn't. This is probably fine as is,
though someone with a better grip on it should weigh in.

Andrew



reply via email to

[Prev in Thread] Current Thread [Next in Thread]