| Message ID | 20200813222625.243136-9-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qom: Automated conversion of type checking boilerplate | expand |
On Thu, Aug 13, 2020 at 3:29 PM Eduardo Habkost <ehabkost@redhat.com> wrote: > > Some of the enum constant names conflict with the QOM type check > macros. This needs to be addressed to allow us to transform the > QOM type check macros into functions generated by > OBJECT_DECLARE_TYPE(). > > Rename all the constants to IBEX_DEV_*, to avoid conflicts. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > include/hw/riscv/opentitan.h | 38 ++++++++-------- > hw/riscv/opentitan.c | 84 ++++++++++++++++++------------------ > 2 files changed, 61 insertions(+), 61 deletions(-) > > diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h > index 8f29b9cbbf..835a80f896 100644 > --- a/include/hw/riscv/opentitan.h > +++ b/include/hw/riscv/opentitan.h > @@ -49,25 +49,25 @@ typedef struct OpenTitanState { > } OpenTitanState; > > enum { > - IBEX_ROM, > - IBEX_RAM, > - IBEX_FLASH, > - IBEX_UART, > - IBEX_GPIO, > - IBEX_SPI, > - IBEX_FLASH_CTRL, > - IBEX_RV_TIMER, > - IBEX_AES, > - IBEX_HMAC, > - IBEX_PLIC, > - IBEX_PWRMGR, > - IBEX_RSTMGR, > - IBEX_CLKMGR, > - IBEX_PINMUX, > - IBEX_ALERT_HANDLER, > - IBEX_NMI_GEN, > - IBEX_USBDEV, > - IBEX_PADCTRL, > + IBEX_DEV_ROM, > + IBEX_DEV_RAM, > + IBEX_DEV_FLASH, > + IBEX_DEV_UART, > + IBEX_DEV_GPIO, > + IBEX_DEV_SPI, > + IBEX_DEV_FLASH_CTRL, > + IBEX_DEV_RV_TIMER, > + IBEX_DEV_AES, > + IBEX_DEV_HMAC, > + IBEX_DEV_PLIC, > + IBEX_DEV_PWRMGR, > + IBEX_DEV_RSTMGR, > + IBEX_DEV_CLKMGR, > + IBEX_DEV_PINMUX, > + IBEX_DEV_ALERT_HANDLER, > + IBEX_DEV_NMI_GEN, > + IBEX_DEV_USBDEV, > + IBEX_DEV_PADCTRL, > }; > > enum { > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c > index a8f0039e51..23ba3b4bfc 100644 > --- a/hw/riscv/opentitan.c > +++ b/hw/riscv/opentitan.c > @@ -32,25 +32,25 @@ static const struct MemmapEntry { > hwaddr base; > hwaddr size; > } ibex_memmap[] = { > - [IBEX_ROM] = { 0x00008000, 16 * KiB }, > - [IBEX_RAM] = { 0x10000000, 0x10000 }, > - [IBEX_FLASH] = { 0x20000000, 0x80000 }, > - [IBEX_UART] = { 0x40000000, 0x10000 }, > - [IBEX_GPIO] = { 0x40010000, 0x10000 }, > - [IBEX_SPI] = { 0x40020000, 0x10000 }, > - [IBEX_FLASH_CTRL] = { 0x40030000, 0x10000 }, > - [IBEX_PINMUX] = { 0x40070000, 0x10000 }, > - [IBEX_RV_TIMER] = { 0x40080000, 0x10000 }, > - [IBEX_PLIC] = { 0x40090000, 0x10000 }, > - [IBEX_PWRMGR] = { 0x400A0000, 0x10000 }, > - [IBEX_RSTMGR] = { 0x400B0000, 0x10000 }, > - [IBEX_CLKMGR] = { 0x400C0000, 0x10000 }, > - [IBEX_AES] = { 0x40110000, 0x10000 }, > - [IBEX_HMAC] = { 0x40120000, 0x10000 }, > - [IBEX_ALERT_HANDLER] = { 0x40130000, 0x10000 }, > - [IBEX_NMI_GEN] = { 0x40140000, 0x10000 }, > - [IBEX_USBDEV] = { 0x40150000, 0x10000 }, > - [IBEX_PADCTRL] = { 0x40160000, 0x10000 } > + [IBEX_DEV_ROM] = { 0x00008000, 16 * KiB }, > + [IBEX_DEV_RAM] = { 0x10000000, 0x10000 }, > + [IBEX_DEV_FLASH] = { 0x20000000, 0x80000 }, > + [IBEX_DEV_UART] = { 0x40000000, 0x10000 }, > + [IBEX_DEV_GPIO] = { 0x40010000, 0x10000 }, > + [IBEX_DEV_SPI] = { 0x40020000, 0x10000 }, > + [IBEX_DEV_FLASH_CTRL] = { 0x40030000, 0x10000 }, > + [IBEX_DEV_PINMUX] = { 0x40070000, 0x10000 }, > + [IBEX_DEV_RV_TIMER] = { 0x40080000, 0x10000 }, > + [IBEX_DEV_PLIC] = { 0x40090000, 0x10000 }, > + [IBEX_DEV_PWRMGR] = { 0x400A0000, 0x10000 }, > + [IBEX_DEV_RSTMGR] = { 0x400B0000, 0x10000 }, > + [IBEX_DEV_CLKMGR] = { 0x400C0000, 0x10000 }, > + [IBEX_DEV_AES] = { 0x40110000, 0x10000 }, > + [IBEX_DEV_HMAC] = { 0x40120000, 0x10000 }, > + [IBEX_DEV_ALERT_HANDLER] = { 0x40130000, 0x10000 }, > + [IBEX_DEV_NMI_GEN] = { 0x40140000, 0x10000 }, > + [IBEX_DEV_USBDEV] = { 0x40150000, 0x10000 }, > + [IBEX_DEV_PADCTRL] = { 0x40160000, 0x10000 } > }; > > static void opentitan_board_init(MachineState *machine) > @@ -66,12 +66,12 @@ static void opentitan_board_init(MachineState *machine) > qdev_realize(DEVICE(&s->soc), NULL, &error_abort); > > memory_region_init_ram(main_mem, NULL, "riscv.lowrisc.ibex.ram", > - memmap[IBEX_RAM].size, &error_fatal); > + memmap[IBEX_DEV_RAM].size, &error_fatal); > memory_region_add_subregion(sys_mem, > - memmap[IBEX_RAM].base, main_mem); > + memmap[IBEX_DEV_RAM].base, main_mem); > > if (machine->firmware) { > - riscv_load_firmware(machine->firmware, memmap[IBEX_RAM].base, NULL); > + riscv_load_firmware(machine->firmware, memmap[IBEX_DEV_RAM].base, NULL); > } > > if (machine->kernel_filename) { > @@ -115,28 +115,28 @@ static void lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp) > > /* Boot ROM */ > memory_region_init_rom(&s->rom, OBJECT(dev_soc), "riscv.lowrisc.ibex.rom", > - memmap[IBEX_ROM].size, &error_fatal); > + memmap[IBEX_DEV_ROM].size, &error_fatal); > memory_region_add_subregion(sys_mem, > - memmap[IBEX_ROM].base, &s->rom); > + memmap[IBEX_DEV_ROM].base, &s->rom); > > /* Flash memory */ > memory_region_init_rom(&s->flash_mem, OBJECT(dev_soc), "riscv.lowrisc.ibex.flash", > - memmap[IBEX_FLASH].size, &error_fatal); > - memory_region_add_subregion(sys_mem, memmap[IBEX_FLASH].base, > + memmap[IBEX_DEV_FLASH].size, &error_fatal); > + memory_region_add_subregion(sys_mem, memmap[IBEX_DEV_FLASH].base, > &s->flash_mem); > > /* PLIC */ > if (!sysbus_realize(SYS_BUS_DEVICE(&s->plic), errp)) { > return; > } > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->plic), 0, memmap[IBEX_PLIC].base); > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->plic), 0, memmap[IBEX_DEV_PLIC].base); > > /* UART */ > qdev_prop_set_chr(DEVICE(&(s->uart)), "chardev", serial_hd(0)); > if (!sysbus_realize(SYS_BUS_DEVICE(&s->uart), errp)) { > return; > } > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart), 0, memmap[IBEX_UART].base); > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart), 0, memmap[IBEX_DEV_UART].base); > sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), > 0, qdev_get_gpio_in(DEVICE(&s->plic), > IBEX_UART_TX_WATERMARK_IRQ)); > @@ -151,33 +151,33 @@ static void lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp) > IBEX_UART_RX_OVERFLOW_IRQ)); > > create_unimplemented_device("riscv.lowrisc.ibex.gpio", > - memmap[IBEX_GPIO].base, memmap[IBEX_GPIO].size); > + memmap[IBEX_DEV_GPIO].base, memmap[IBEX_DEV_GPIO].size); > create_unimplemented_device("riscv.lowrisc.ibex.spi", > - memmap[IBEX_SPI].base, memmap[IBEX_SPI].size); > + memmap[IBEX_DEV_SPI].base, memmap[IBEX_DEV_SPI].size); > create_unimplemented_device("riscv.lowrisc.ibex.flash_ctrl", > - memmap[IBEX_FLASH_CTRL].base, memmap[IBEX_FLASH_CTRL].size); > + memmap[IBEX_DEV_FLASH_CTRL].base, memmap[IBEX_DEV_FLASH_CTRL].size); > create_unimplemented_device("riscv.lowrisc.ibex.rv_timer", > - memmap[IBEX_RV_TIMER].base, memmap[IBEX_RV_TIMER].size); > + memmap[IBEX_DEV_RV_TIMER].base, memmap[IBEX_DEV_RV_TIMER].size); > create_unimplemented_device("riscv.lowrisc.ibex.pwrmgr", > - memmap[IBEX_PWRMGR].base, memmap[IBEX_PWRMGR].size); > + memmap[IBEX_DEV_PWRMGR].base, memmap[IBEX_DEV_PWRMGR].size); > create_unimplemented_device("riscv.lowrisc.ibex.rstmgr", > - memmap[IBEX_RSTMGR].base, memmap[IBEX_RSTMGR].size); > + memmap[IBEX_DEV_RSTMGR].base, memmap[IBEX_DEV_RSTMGR].size); > create_unimplemented_device("riscv.lowrisc.ibex.clkmgr", > - memmap[IBEX_CLKMGR].base, memmap[IBEX_CLKMGR].size); > + memmap[IBEX_DEV_CLKMGR].base, memmap[IBEX_DEV_CLKMGR].size); > create_unimplemented_device("riscv.lowrisc.ibex.aes", > - memmap[IBEX_AES].base, memmap[IBEX_AES].size); > + memmap[IBEX_DEV_AES].base, memmap[IBEX_DEV_AES].size); > create_unimplemented_device("riscv.lowrisc.ibex.hmac", > - memmap[IBEX_HMAC].base, memmap[IBEX_HMAC].size); > + memmap[IBEX_DEV_HMAC].base, memmap[IBEX_DEV_HMAC].size); > create_unimplemented_device("riscv.lowrisc.ibex.pinmux", > - memmap[IBEX_PINMUX].base, memmap[IBEX_PINMUX].size); > + memmap[IBEX_DEV_PINMUX].base, memmap[IBEX_DEV_PINMUX].size); > create_unimplemented_device("riscv.lowrisc.ibex.alert_handler", > - memmap[IBEX_ALERT_HANDLER].base, memmap[IBEX_ALERT_HANDLER].size); > + memmap[IBEX_DEV_ALERT_HANDLER].base, memmap[IBEX_DEV_ALERT_HANDLER].size); > create_unimplemented_device("riscv.lowrisc.ibex.nmi_gen", > - memmap[IBEX_NMI_GEN].base, memmap[IBEX_NMI_GEN].size); > + memmap[IBEX_DEV_NMI_GEN].base, memmap[IBEX_DEV_NMI_GEN].size); > create_unimplemented_device("riscv.lowrisc.ibex.usbdev", > - memmap[IBEX_USBDEV].base, memmap[IBEX_USBDEV].size); > + memmap[IBEX_DEV_USBDEV].base, memmap[IBEX_DEV_USBDEV].size); > create_unimplemented_device("riscv.lowrisc.ibex.padctrl", > - memmap[IBEX_PADCTRL].base, memmap[IBEX_PADCTRL].size); > + memmap[IBEX_DEV_PADCTRL].base, memmap[IBEX_DEV_PADCTRL].size); > } > > static void lowrisc_ibex_soc_class_init(ObjectClass *oc, void *data) > -- > 2.26.2 > >
On 8/14/20 12:25 AM, Eduardo Habkost wrote: > Some of the enum constant names conflict with the QOM type check > macros. This needs to be addressed to allow us to transform the > QOM type check macros into functions generated by > OBJECT_DECLARE_TYPE(). > > Rename all the constants to IBEX_DEV_*, to avoid conflicts. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > include/hw/riscv/opentitan.h | 38 ++++++++-------- > hw/riscv/opentitan.c | 84 ++++++++++++++++++------------------ > 2 files changed, 61 insertions(+), 61 deletions(-) > > diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h > index 8f29b9cbbf..835a80f896 100644 > --- a/include/hw/riscv/opentitan.h > +++ b/include/hw/riscv/opentitan.h > @@ -49,25 +49,25 @@ typedef struct OpenTitanState { > } OpenTitanState; > > enum { > - IBEX_ROM, > - IBEX_RAM, > - IBEX_FLASH, > - IBEX_UART, > - IBEX_GPIO, > - IBEX_SPI, > - IBEX_FLASH_CTRL, > - IBEX_RV_TIMER, > - IBEX_AES, > - IBEX_HMAC, > - IBEX_PLIC, > - IBEX_PWRMGR, > - IBEX_RSTMGR, > - IBEX_CLKMGR, > - IBEX_PINMUX, > - IBEX_ALERT_HANDLER, > - IBEX_NMI_GEN, > - IBEX_USBDEV, > - IBEX_PADCTRL, > + IBEX_DEV_ROM, > + IBEX_DEV_RAM, > + IBEX_DEV_FLASH, > + IBEX_DEV_UART, > + IBEX_DEV_GPIO, > + IBEX_DEV_SPI, > + IBEX_DEV_FLASH_CTRL, > + IBEX_DEV_RV_TIMER, > + IBEX_DEV_AES, > + IBEX_DEV_HMAC, > + IBEX_DEV_PLIC, > + IBEX_DEV_PWRMGR, > + IBEX_DEV_RSTMGR, > + IBEX_DEV_CLKMGR, > + IBEX_DEV_PINMUX, > + IBEX_DEV_ALERT_HANDLER, > + IBEX_DEV_NMI_GEN, > + IBEX_DEV_USBDEV, > + IBEX_DEV_PADCTRL, Similarly, why is this enum in a public header and not local to opentitan.c, only place where it is used? > }; > > enum { > diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c > index a8f0039e51..23ba3b4bfc 100644 > --- a/hw/riscv/opentitan.c > +++ b/hw/riscv/opentitan.c > @@ -32,25 +32,25 @@ static const struct MemmapEntry { > hwaddr base; > hwaddr size; > } ibex_memmap[] = { > - [IBEX_ROM] = { 0x00008000, 16 * KiB }, > - [IBEX_RAM] = { 0x10000000, 0x10000 }, > - [IBEX_FLASH] = { 0x20000000, 0x80000 }, > - [IBEX_UART] = { 0x40000000, 0x10000 }, > - [IBEX_GPIO] = { 0x40010000, 0x10000 }, > - [IBEX_SPI] = { 0x40020000, 0x10000 }, > - [IBEX_FLASH_CTRL] = { 0x40030000, 0x10000 }, > - [IBEX_PINMUX] = { 0x40070000, 0x10000 }, > - [IBEX_RV_TIMER] = { 0x40080000, 0x10000 }, > - [IBEX_PLIC] = { 0x40090000, 0x10000 }, > - [IBEX_PWRMGR] = { 0x400A0000, 0x10000 }, > - [IBEX_RSTMGR] = { 0x400B0000, 0x10000 }, > - [IBEX_CLKMGR] = { 0x400C0000, 0x10000 }, > - [IBEX_AES] = { 0x40110000, 0x10000 }, > - [IBEX_HMAC] = { 0x40120000, 0x10000 }, > - [IBEX_ALERT_HANDLER] = { 0x40130000, 0x10000 }, > - [IBEX_NMI_GEN] = { 0x40140000, 0x10000 }, > - [IBEX_USBDEV] = { 0x40150000, 0x10000 }, > - [IBEX_PADCTRL] = { 0x40160000, 0x10000 } > + [IBEX_DEV_ROM] = { 0x00008000, 16 * KiB }, > + [IBEX_DEV_RAM] = { 0x10000000, 0x10000 }, > + [IBEX_DEV_FLASH] = { 0x20000000, 0x80000 }, > + [IBEX_DEV_UART] = { 0x40000000, 0x10000 }, > + [IBEX_DEV_GPIO] = { 0x40010000, 0x10000 }, > + [IBEX_DEV_SPI] = { 0x40020000, 0x10000 }, > + [IBEX_DEV_FLASH_CTRL] = { 0x40030000, 0x10000 }, > + [IBEX_DEV_PINMUX] = { 0x40070000, 0x10000 }, > + [IBEX_DEV_RV_TIMER] = { 0x40080000, 0x10000 }, > + [IBEX_DEV_PLIC] = { 0x40090000, 0x10000 }, > + [IBEX_DEV_PWRMGR] = { 0x400A0000, 0x10000 }, > + [IBEX_DEV_RSTMGR] = { 0x400B0000, 0x10000 }, > + [IBEX_DEV_CLKMGR] = { 0x400C0000, 0x10000 }, > + [IBEX_DEV_AES] = { 0x40110000, 0x10000 }, > + [IBEX_DEV_HMAC] = { 0x40120000, 0x10000 }, > + [IBEX_DEV_ALERT_HANDLER] = { 0x40130000, 0x10000 }, > + [IBEX_DEV_NMI_GEN] = { 0x40140000, 0x10000 }, > + [IBEX_DEV_USBDEV] = { 0x40150000, 0x10000 }, > + [IBEX_DEV_PADCTRL] = { 0x40160000, 0x10000 } > }; > > static void opentitan_board_init(MachineState *machine) > @@ -66,12 +66,12 @@ static void opentitan_board_init(MachineState *machine) > qdev_realize(DEVICE(&s->soc), NULL, &error_abort); > > memory_region_init_ram(main_mem, NULL, "riscv.lowrisc.ibex.ram", > - memmap[IBEX_RAM].size, &error_fatal); > + memmap[IBEX_DEV_RAM].size, &error_fatal); > memory_region_add_subregion(sys_mem, > - memmap[IBEX_RAM].base, main_mem); > + memmap[IBEX_DEV_RAM].base, main_mem); > > if (machine->firmware) { > - riscv_load_firmware(machine->firmware, memmap[IBEX_RAM].base, NULL); > + riscv_load_firmware(machine->firmware, memmap[IBEX_DEV_RAM].base, NULL); > } > > if (machine->kernel_filename) { > @@ -115,28 +115,28 @@ static void lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp) > > /* Boot ROM */ > memory_region_init_rom(&s->rom, OBJECT(dev_soc), "riscv.lowrisc.ibex.rom", > - memmap[IBEX_ROM].size, &error_fatal); > + memmap[IBEX_DEV_ROM].size, &error_fatal); > memory_region_add_subregion(sys_mem, > - memmap[IBEX_ROM].base, &s->rom); > + memmap[IBEX_DEV_ROM].base, &s->rom); > > /* Flash memory */ > memory_region_init_rom(&s->flash_mem, OBJECT(dev_soc), "riscv.lowrisc.ibex.flash", > - memmap[IBEX_FLASH].size, &error_fatal); > - memory_region_add_subregion(sys_mem, memmap[IBEX_FLASH].base, > + memmap[IBEX_DEV_FLASH].size, &error_fatal); > + memory_region_add_subregion(sys_mem, memmap[IBEX_DEV_FLASH].base, > &s->flash_mem); > > /* PLIC */ > if (!sysbus_realize(SYS_BUS_DEVICE(&s->plic), errp)) { > return; > } > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->plic), 0, memmap[IBEX_PLIC].base); > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->plic), 0, memmap[IBEX_DEV_PLIC].base); > > /* UART */ > qdev_prop_set_chr(DEVICE(&(s->uart)), "chardev", serial_hd(0)); > if (!sysbus_realize(SYS_BUS_DEVICE(&s->uart), errp)) { > return; > } > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart), 0, memmap[IBEX_UART].base); > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart), 0, memmap[IBEX_DEV_UART].base); > sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), > 0, qdev_get_gpio_in(DEVICE(&s->plic), > IBEX_UART_TX_WATERMARK_IRQ)); > @@ -151,33 +151,33 @@ static void lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp) > IBEX_UART_RX_OVERFLOW_IRQ)); > > create_unimplemented_device("riscv.lowrisc.ibex.gpio", > - memmap[IBEX_GPIO].base, memmap[IBEX_GPIO].size); > + memmap[IBEX_DEV_GPIO].base, memmap[IBEX_DEV_GPIO].size); > create_unimplemented_device("riscv.lowrisc.ibex.spi", > - memmap[IBEX_SPI].base, memmap[IBEX_SPI].size); > + memmap[IBEX_DEV_SPI].base, memmap[IBEX_DEV_SPI].size); > create_unimplemented_device("riscv.lowrisc.ibex.flash_ctrl", > - memmap[IBEX_FLASH_CTRL].base, memmap[IBEX_FLASH_CTRL].size); > + memmap[IBEX_DEV_FLASH_CTRL].base, memmap[IBEX_DEV_FLASH_CTRL].size); > create_unimplemented_device("riscv.lowrisc.ibex.rv_timer", > - memmap[IBEX_RV_TIMER].base, memmap[IBEX_RV_TIMER].size); > + memmap[IBEX_DEV_RV_TIMER].base, memmap[IBEX_DEV_RV_TIMER].size); > create_unimplemented_device("riscv.lowrisc.ibex.pwrmgr", > - memmap[IBEX_PWRMGR].base, memmap[IBEX_PWRMGR].size); > + memmap[IBEX_DEV_PWRMGR].base, memmap[IBEX_DEV_PWRMGR].size); > create_unimplemented_device("riscv.lowrisc.ibex.rstmgr", > - memmap[IBEX_RSTMGR].base, memmap[IBEX_RSTMGR].size); > + memmap[IBEX_DEV_RSTMGR].base, memmap[IBEX_DEV_RSTMGR].size); > create_unimplemented_device("riscv.lowrisc.ibex.clkmgr", > - memmap[IBEX_CLKMGR].base, memmap[IBEX_CLKMGR].size); > + memmap[IBEX_DEV_CLKMGR].base, memmap[IBEX_DEV_CLKMGR].size); > create_unimplemented_device("riscv.lowrisc.ibex.aes", > - memmap[IBEX_AES].base, memmap[IBEX_AES].size); > + memmap[IBEX_DEV_AES].base, memmap[IBEX_DEV_AES].size); > create_unimplemented_device("riscv.lowrisc.ibex.hmac", > - memmap[IBEX_HMAC].base, memmap[IBEX_HMAC].size); > + memmap[IBEX_DEV_HMAC].base, memmap[IBEX_DEV_HMAC].size); > create_unimplemented_device("riscv.lowrisc.ibex.pinmux", > - memmap[IBEX_PINMUX].base, memmap[IBEX_PINMUX].size); > + memmap[IBEX_DEV_PINMUX].base, memmap[IBEX_DEV_PINMUX].size); > create_unimplemented_device("riscv.lowrisc.ibex.alert_handler", > - memmap[IBEX_ALERT_HANDLER].base, memmap[IBEX_ALERT_HANDLER].size); > + memmap[IBEX_DEV_ALERT_HANDLER].base, memmap[IBEX_DEV_ALERT_HANDLER].size); > create_unimplemented_device("riscv.lowrisc.ibex.nmi_gen", > - memmap[IBEX_NMI_GEN].base, memmap[IBEX_NMI_GEN].size); > + memmap[IBEX_DEV_NMI_GEN].base, memmap[IBEX_DEV_NMI_GEN].size); > create_unimplemented_device("riscv.lowrisc.ibex.usbdev", > - memmap[IBEX_USBDEV].base, memmap[IBEX_USBDEV].size); > + memmap[IBEX_DEV_USBDEV].base, memmap[IBEX_DEV_USBDEV].size); > create_unimplemented_device("riscv.lowrisc.ibex.padctrl", > - memmap[IBEX_PADCTRL].base, memmap[IBEX_PADCTRL].size); > + memmap[IBEX_DEV_PADCTRL].base, memmap[IBEX_DEV_PADCTRL].size); > } > > static void lowrisc_ibex_soc_class_init(ObjectClass *oc, void *data) >
On Sat, Aug 15, 2020 at 1:56 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 8/14/20 12:25 AM, Eduardo Habkost wrote: > > Some of the enum constant names conflict with the QOM type check > > macros. This needs to be addressed to allow us to transform the > > QOM type check macros into functions generated by > > OBJECT_DECLARE_TYPE(). > > > > Rename all the constants to IBEX_DEV_*, to avoid conflicts. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > include/hw/riscv/opentitan.h | 38 ++++++++-------- > > hw/riscv/opentitan.c | 84 ++++++++++++++++++------------------ > > 2 files changed, 61 insertions(+), 61 deletions(-) > > > > diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h > > index 8f29b9cbbf..835a80f896 100644 > > --- a/include/hw/riscv/opentitan.h > > +++ b/include/hw/riscv/opentitan.h > > @@ -49,25 +49,25 @@ typedef struct OpenTitanState { > > } OpenTitanState; > > > > enum { > > - IBEX_ROM, > > - IBEX_RAM, > > - IBEX_FLASH, > > - IBEX_UART, > > - IBEX_GPIO, > > - IBEX_SPI, > > - IBEX_FLASH_CTRL, > > - IBEX_RV_TIMER, > > - IBEX_AES, > > - IBEX_HMAC, > > - IBEX_PLIC, > > - IBEX_PWRMGR, > > - IBEX_RSTMGR, > > - IBEX_CLKMGR, > > - IBEX_PINMUX, > > - IBEX_ALERT_HANDLER, > > - IBEX_NMI_GEN, > > - IBEX_USBDEV, > > - IBEX_PADCTRL, > > + IBEX_DEV_ROM, > > + IBEX_DEV_RAM, > > + IBEX_DEV_FLASH, > > + IBEX_DEV_UART, > > + IBEX_DEV_GPIO, > > + IBEX_DEV_SPI, > > + IBEX_DEV_FLASH_CTRL, > > + IBEX_DEV_RV_TIMER, > > + IBEX_DEV_AES, > > + IBEX_DEV_HMAC, > > + IBEX_DEV_PLIC, > > + IBEX_DEV_PWRMGR, > > + IBEX_DEV_RSTMGR, > > + IBEX_DEV_CLKMGR, > > + IBEX_DEV_PINMUX, > > + IBEX_DEV_ALERT_HANDLER, > > + IBEX_DEV_NMI_GEN, > > + IBEX_DEV_USBDEV, > > + IBEX_DEV_PADCTRL, > > Similarly, why is this enum in a public header and not local > to opentitan.c, only place where it is used? > I believe the reason is that opentitan.c implements both SoC and board stuff. These enums are helpful to define SoC peripherals hence technically another board that uses the same SoC may utilize these macros. Unfortunately this is the case for all RISC-V boards so far. Should we clean this up, for example, splitting SoC and board codes into 2 files? Regards, Bin
On Mon, Aug 17, 2020 at 2:53 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Sat, Aug 15, 2020 at 1:56 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > On 8/14/20 12:25 AM, Eduardo Habkost wrote: > > > Some of the enum constant names conflict with the QOM type check > > > macros. This needs to be addressed to allow us to transform the > > > QOM type check macros into functions generated by > > > OBJECT_DECLARE_TYPE(). > > > > > > Rename all the constants to IBEX_DEV_*, to avoid conflicts. > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > include/hw/riscv/opentitan.h | 38 ++++++++-------- > > > hw/riscv/opentitan.c | 84 ++++++++++++++++++------------------ > > > 2 files changed, 61 insertions(+), 61 deletions(-) > > > > > > diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h > > > index 8f29b9cbbf..835a80f896 100644 > > > --- a/include/hw/riscv/opentitan.h > > > +++ b/include/hw/riscv/opentitan.h > > > @@ -49,25 +49,25 @@ typedef struct OpenTitanState { > > > } OpenTitanState; > > > > > > enum { > > > - IBEX_ROM, > > > - IBEX_RAM, > > > - IBEX_FLASH, > > > - IBEX_UART, > > > - IBEX_GPIO, > > > - IBEX_SPI, > > > - IBEX_FLASH_CTRL, > > > - IBEX_RV_TIMER, > > > - IBEX_AES, > > > - IBEX_HMAC, > > > - IBEX_PLIC, > > > - IBEX_PWRMGR, > > > - IBEX_RSTMGR, > > > - IBEX_CLKMGR, > > > - IBEX_PINMUX, > > > - IBEX_ALERT_HANDLER, > > > - IBEX_NMI_GEN, > > > - IBEX_USBDEV, > > > - IBEX_PADCTRL, > > > + IBEX_DEV_ROM, > > > + IBEX_DEV_RAM, > > > + IBEX_DEV_FLASH, > > > + IBEX_DEV_UART, > > > + IBEX_DEV_GPIO, > > > + IBEX_DEV_SPI, > > > + IBEX_DEV_FLASH_CTRL, > > > + IBEX_DEV_RV_TIMER, > > > + IBEX_DEV_AES, > > > + IBEX_DEV_HMAC, > > > + IBEX_DEV_PLIC, > > > + IBEX_DEV_PWRMGR, > > > + IBEX_DEV_RSTMGR, > > > + IBEX_DEV_CLKMGR, > > > + IBEX_DEV_PINMUX, > > > + IBEX_DEV_ALERT_HANDLER, > > > + IBEX_DEV_NMI_GEN, > > > + IBEX_DEV_USBDEV, > > > + IBEX_DEV_PADCTRL, > > > > Similarly, why is this enum in a public header and not local > > to opentitan.c, only place where it is used? > > > > I believe the reason is that opentitan.c implements both SoC and board > stuff. These enums are helpful to define SoC peripherals hence > technically another board that uses the same SoC may utilize these > macros. > > Unfortunately this is the case for all RISC-V boards so far. Should we > clean this up, for example, splitting SoC and board codes into 2 > files? Yeah the hw/riscv directory is a bit of a mess. We need to look at moving non machine parts out (like sifive_uart for example) and then it's probably worth splitting SoC and machine. It does result in some extra files (some of which are very simple) but the current setup is pretty confusing. Are you volunteering Bin? Otherwise I can look at it. Alistair > > Regards, > Bin >
Hi Alistair, On Sat, Aug 22, 2020 at 3:04 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Mon, Aug 17, 2020 at 2:53 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > On Sat, Aug 15, 2020 at 1:56 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > > > On 8/14/20 12:25 AM, Eduardo Habkost wrote: > > > > Some of the enum constant names conflict with the QOM type check > > > > macros. This needs to be addressed to allow us to transform the > > > > QOM type check macros into functions generated by > > > > OBJECT_DECLARE_TYPE(). > > > > > > > > Rename all the constants to IBEX_DEV_*, to avoid conflicts. > > > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > > --- > > > > include/hw/riscv/opentitan.h | 38 ++++++++-------- > > > > hw/riscv/opentitan.c | 84 ++++++++++++++++++------------------ > > > > 2 files changed, 61 insertions(+), 61 deletions(-) > > > > > > > > diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h > > > > index 8f29b9cbbf..835a80f896 100644 > > > > --- a/include/hw/riscv/opentitan.h > > > > +++ b/include/hw/riscv/opentitan.h > > > > @@ -49,25 +49,25 @@ typedef struct OpenTitanState { > > > > } OpenTitanState; > > > > > > > > enum { > > > > - IBEX_ROM, > > > > - IBEX_RAM, > > > > - IBEX_FLASH, > > > > - IBEX_UART, > > > > - IBEX_GPIO, > > > > - IBEX_SPI, > > > > - IBEX_FLASH_CTRL, > > > > - IBEX_RV_TIMER, > > > > - IBEX_AES, > > > > - IBEX_HMAC, > > > > - IBEX_PLIC, > > > > - IBEX_PWRMGR, > > > > - IBEX_RSTMGR, > > > > - IBEX_CLKMGR, > > > > - IBEX_PINMUX, > > > > - IBEX_ALERT_HANDLER, > > > > - IBEX_NMI_GEN, > > > > - IBEX_USBDEV, > > > > - IBEX_PADCTRL, > > > > + IBEX_DEV_ROM, > > > > + IBEX_DEV_RAM, > > > > + IBEX_DEV_FLASH, > > > > + IBEX_DEV_UART, > > > > + IBEX_DEV_GPIO, > > > > + IBEX_DEV_SPI, > > > > + IBEX_DEV_FLASH_CTRL, > > > > + IBEX_DEV_RV_TIMER, > > > > + IBEX_DEV_AES, > > > > + IBEX_DEV_HMAC, > > > > + IBEX_DEV_PLIC, > > > > + IBEX_DEV_PWRMGR, > > > > + IBEX_DEV_RSTMGR, > > > > + IBEX_DEV_CLKMGR, > > > > + IBEX_DEV_PINMUX, > > > > + IBEX_DEV_ALERT_HANDLER, > > > > + IBEX_DEV_NMI_GEN, > > > > + IBEX_DEV_USBDEV, > > > > + IBEX_DEV_PADCTRL, > > > > > > Similarly, why is this enum in a public header and not local > > > to opentitan.c, only place where it is used? > > > > > > > I believe the reason is that opentitan.c implements both SoC and board > > stuff. These enums are helpful to define SoC peripherals hence > > technically another board that uses the same SoC may utilize these > > macros. > > > > Unfortunately this is the case for all RISC-V boards so far. Should we > > clean this up, for example, splitting SoC and board codes into 2 > > files? > > Yeah the hw/riscv directory is a bit of a mess. We need to look at > moving non machine parts out (like sifive_uart for example) and then > it's probably worth splitting SoC and machine. It does result in some > extra files (some of which are very simple) but the current setup is > pretty confusing. > > Are you volunteering Bin? Otherwise I can look at it. > Yes, I will be working on a series to clean hw/riscv directory. Regards, Bin
diff --git a/include/hw/riscv/opentitan.h b/include/hw/riscv/opentitan.h index 8f29b9cbbf..835a80f896 100644 --- a/include/hw/riscv/opentitan.h +++ b/include/hw/riscv/opentitan.h @@ -49,25 +49,25 @@ typedef struct OpenTitanState { } OpenTitanState; enum { - IBEX_ROM, - IBEX_RAM, - IBEX_FLASH, - IBEX_UART, - IBEX_GPIO, - IBEX_SPI, - IBEX_FLASH_CTRL, - IBEX_RV_TIMER, - IBEX_AES, - IBEX_HMAC, - IBEX_PLIC, - IBEX_PWRMGR, - IBEX_RSTMGR, - IBEX_CLKMGR, - IBEX_PINMUX, - IBEX_ALERT_HANDLER, - IBEX_NMI_GEN, - IBEX_USBDEV, - IBEX_PADCTRL, + IBEX_DEV_ROM, + IBEX_DEV_RAM, + IBEX_DEV_FLASH, + IBEX_DEV_UART, + IBEX_DEV_GPIO, + IBEX_DEV_SPI, + IBEX_DEV_FLASH_CTRL, + IBEX_DEV_RV_TIMER, + IBEX_DEV_AES, + IBEX_DEV_HMAC, + IBEX_DEV_PLIC, + IBEX_DEV_PWRMGR, + IBEX_DEV_RSTMGR, + IBEX_DEV_CLKMGR, + IBEX_DEV_PINMUX, + IBEX_DEV_ALERT_HANDLER, + IBEX_DEV_NMI_GEN, + IBEX_DEV_USBDEV, + IBEX_DEV_PADCTRL, }; enum { diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c index a8f0039e51..23ba3b4bfc 100644 --- a/hw/riscv/opentitan.c +++ b/hw/riscv/opentitan.c @@ -32,25 +32,25 @@ static const struct MemmapEntry { hwaddr base; hwaddr size; } ibex_memmap[] = { - [IBEX_ROM] = { 0x00008000, 16 * KiB }, - [IBEX_RAM] = { 0x10000000, 0x10000 }, - [IBEX_FLASH] = { 0x20000000, 0x80000 }, - [IBEX_UART] = { 0x40000000, 0x10000 }, - [IBEX_GPIO] = { 0x40010000, 0x10000 }, - [IBEX_SPI] = { 0x40020000, 0x10000 }, - [IBEX_FLASH_CTRL] = { 0x40030000, 0x10000 }, - [IBEX_PINMUX] = { 0x40070000, 0x10000 }, - [IBEX_RV_TIMER] = { 0x40080000, 0x10000 }, - [IBEX_PLIC] = { 0x40090000, 0x10000 }, - [IBEX_PWRMGR] = { 0x400A0000, 0x10000 }, - [IBEX_RSTMGR] = { 0x400B0000, 0x10000 }, - [IBEX_CLKMGR] = { 0x400C0000, 0x10000 }, - [IBEX_AES] = { 0x40110000, 0x10000 }, - [IBEX_HMAC] = { 0x40120000, 0x10000 }, - [IBEX_ALERT_HANDLER] = { 0x40130000, 0x10000 }, - [IBEX_NMI_GEN] = { 0x40140000, 0x10000 }, - [IBEX_USBDEV] = { 0x40150000, 0x10000 }, - [IBEX_PADCTRL] = { 0x40160000, 0x10000 } + [IBEX_DEV_ROM] = { 0x00008000, 16 * KiB }, + [IBEX_DEV_RAM] = { 0x10000000, 0x10000 }, + [IBEX_DEV_FLASH] = { 0x20000000, 0x80000 }, + [IBEX_DEV_UART] = { 0x40000000, 0x10000 }, + [IBEX_DEV_GPIO] = { 0x40010000, 0x10000 }, + [IBEX_DEV_SPI] = { 0x40020000, 0x10000 }, + [IBEX_DEV_FLASH_CTRL] = { 0x40030000, 0x10000 }, + [IBEX_DEV_PINMUX] = { 0x40070000, 0x10000 }, + [IBEX_DEV_RV_TIMER] = { 0x40080000, 0x10000 }, + [IBEX_DEV_PLIC] = { 0x40090000, 0x10000 }, + [IBEX_DEV_PWRMGR] = { 0x400A0000, 0x10000 }, + [IBEX_DEV_RSTMGR] = { 0x400B0000, 0x10000 }, + [IBEX_DEV_CLKMGR] = { 0x400C0000, 0x10000 }, + [IBEX_DEV_AES] = { 0x40110000, 0x10000 }, + [IBEX_DEV_HMAC] = { 0x40120000, 0x10000 }, + [IBEX_DEV_ALERT_HANDLER] = { 0x40130000, 0x10000 }, + [IBEX_DEV_NMI_GEN] = { 0x40140000, 0x10000 }, + [IBEX_DEV_USBDEV] = { 0x40150000, 0x10000 }, + [IBEX_DEV_PADCTRL] = { 0x40160000, 0x10000 } }; static void opentitan_board_init(MachineState *machine) @@ -66,12 +66,12 @@ static void opentitan_board_init(MachineState *machine) qdev_realize(DEVICE(&s->soc), NULL, &error_abort); memory_region_init_ram(main_mem, NULL, "riscv.lowrisc.ibex.ram", - memmap[IBEX_RAM].size, &error_fatal); + memmap[IBEX_DEV_RAM].size, &error_fatal); memory_region_add_subregion(sys_mem, - memmap[IBEX_RAM].base, main_mem); + memmap[IBEX_DEV_RAM].base, main_mem); if (machine->firmware) { - riscv_load_firmware(machine->firmware, memmap[IBEX_RAM].base, NULL); + riscv_load_firmware(machine->firmware, memmap[IBEX_DEV_RAM].base, NULL); } if (machine->kernel_filename) { @@ -115,28 +115,28 @@ static void lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp) /* Boot ROM */ memory_region_init_rom(&s->rom, OBJECT(dev_soc), "riscv.lowrisc.ibex.rom", - memmap[IBEX_ROM].size, &error_fatal); + memmap[IBEX_DEV_ROM].size, &error_fatal); memory_region_add_subregion(sys_mem, - memmap[IBEX_ROM].base, &s->rom); + memmap[IBEX_DEV_ROM].base, &s->rom); /* Flash memory */ memory_region_init_rom(&s->flash_mem, OBJECT(dev_soc), "riscv.lowrisc.ibex.flash", - memmap[IBEX_FLASH].size, &error_fatal); - memory_region_add_subregion(sys_mem, memmap[IBEX_FLASH].base, + memmap[IBEX_DEV_FLASH].size, &error_fatal); + memory_region_add_subregion(sys_mem, memmap[IBEX_DEV_FLASH].base, &s->flash_mem); /* PLIC */ if (!sysbus_realize(SYS_BUS_DEVICE(&s->plic), errp)) { return; } - sysbus_mmio_map(SYS_BUS_DEVICE(&s->plic), 0, memmap[IBEX_PLIC].base); + sysbus_mmio_map(SYS_BUS_DEVICE(&s->plic), 0, memmap[IBEX_DEV_PLIC].base); /* UART */ qdev_prop_set_chr(DEVICE(&(s->uart)), "chardev", serial_hd(0)); if (!sysbus_realize(SYS_BUS_DEVICE(&s->uart), errp)) { return; } - sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart), 0, memmap[IBEX_UART].base); + sysbus_mmio_map(SYS_BUS_DEVICE(&s->uart), 0, memmap[IBEX_DEV_UART].base); sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0, qdev_get_gpio_in(DEVICE(&s->plic), IBEX_UART_TX_WATERMARK_IRQ)); @@ -151,33 +151,33 @@ static void lowrisc_ibex_soc_realize(DeviceState *dev_soc, Error **errp) IBEX_UART_RX_OVERFLOW_IRQ)); create_unimplemented_device("riscv.lowrisc.ibex.gpio", - memmap[IBEX_GPIO].base, memmap[IBEX_GPIO].size); + memmap[IBEX_DEV_GPIO].base, memmap[IBEX_DEV_GPIO].size); create_unimplemented_device("riscv.lowrisc.ibex.spi", - memmap[IBEX_SPI].base, memmap[IBEX_SPI].size); + memmap[IBEX_DEV_SPI].base, memmap[IBEX_DEV_SPI].size); create_unimplemented_device("riscv.lowrisc.ibex.flash_ctrl", - memmap[IBEX_FLASH_CTRL].base, memmap[IBEX_FLASH_CTRL].size); + memmap[IBEX_DEV_FLASH_CTRL].base, memmap[IBEX_DEV_FLASH_CTRL].size); create_unimplemented_device("riscv.lowrisc.ibex.rv_timer", - memmap[IBEX_RV_TIMER].base, memmap[IBEX_RV_TIMER].size); + memmap[IBEX_DEV_RV_TIMER].base, memmap[IBEX_DEV_RV_TIMER].size); create_unimplemented_device("riscv.lowrisc.ibex.pwrmgr", - memmap[IBEX_PWRMGR].base, memmap[IBEX_PWRMGR].size); + memmap[IBEX_DEV_PWRMGR].base, memmap[IBEX_DEV_PWRMGR].size); create_unimplemented_device("riscv.lowrisc.ibex.rstmgr", - memmap[IBEX_RSTMGR].base, memmap[IBEX_RSTMGR].size); + memmap[IBEX_DEV_RSTMGR].base, memmap[IBEX_DEV_RSTMGR].size); create_unimplemented_device("riscv.lowrisc.ibex.clkmgr", - memmap[IBEX_CLKMGR].base, memmap[IBEX_CLKMGR].size); + memmap[IBEX_DEV_CLKMGR].base, memmap[IBEX_DEV_CLKMGR].size); create_unimplemented_device("riscv.lowrisc.ibex.aes", - memmap[IBEX_AES].base, memmap[IBEX_AES].size); + memmap[IBEX_DEV_AES].base, memmap[IBEX_DEV_AES].size); create_unimplemented_device("riscv.lowrisc.ibex.hmac", - memmap[IBEX_HMAC].base, memmap[IBEX_HMAC].size); + memmap[IBEX_DEV_HMAC].base, memmap[IBEX_DEV_HMAC].size); create_unimplemented_device("riscv.lowrisc.ibex.pinmux", - memmap[IBEX_PINMUX].base, memmap[IBEX_PINMUX].size); + memmap[IBEX_DEV_PINMUX].base, memmap[IBEX_DEV_PINMUX].size); create_unimplemented_device("riscv.lowrisc.ibex.alert_handler", - memmap[IBEX_ALERT_HANDLER].base, memmap[IBEX_ALERT_HANDLER].size); + memmap[IBEX_DEV_ALERT_HANDLER].base, memmap[IBEX_DEV_ALERT_HANDLER].size); create_unimplemented_device("riscv.lowrisc.ibex.nmi_gen", - memmap[IBEX_NMI_GEN].base, memmap[IBEX_NMI_GEN].size); + memmap[IBEX_DEV_NMI_GEN].base, memmap[IBEX_DEV_NMI_GEN].size); create_unimplemented_device("riscv.lowrisc.ibex.usbdev", - memmap[IBEX_USBDEV].base, memmap[IBEX_USBDEV].size); + memmap[IBEX_DEV_USBDEV].base, memmap[IBEX_DEV_USBDEV].size); create_unimplemented_device("riscv.lowrisc.ibex.padctrl", - memmap[IBEX_PADCTRL].base, memmap[IBEX_PADCTRL].size); + memmap[IBEX_DEV_PADCTRL].base, memmap[IBEX_DEV_PADCTRL].size); } static void lowrisc_ibex_soc_class_init(ObjectClass *oc, void *data)
Some of the enum constant names conflict with the QOM type check macros. This needs to be addressed to allow us to transform the QOM type check macros into functions generated by OBJECT_DECLARE_TYPE(). Rename all the constants to IBEX_DEV_*, to avoid conflicts. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- include/hw/riscv/opentitan.h | 38 ++++++++-------- hw/riscv/opentitan.c | 84 ++++++++++++++++++------------------ 2 files changed, 61 insertions(+), 61 deletions(-)