Hi, Interesting suggestion Philippe. I modelled the initial code of the H3 SoC mostly in-line with the aspeed SoC, to let it use commonly applied code structures where possible. And it looks like in this series, aspeed_soc.h is also getting the same change. I'll see if I can find some time to make a small patch to move the enum to the local allwinner-h3.c file like you suggested.
For Eduardo's work, I'm OK with a rename for now to AW_H3_DEV_*. I see it is also done like that in aspeed_soc.h and similar files. I do have one follow-up question: which value(s) from the enum are actually in conflict? Could you add that in the commit message perhaps, for clarity? With that answered: Reviewed-by: Niek Linnenbank <nieklinnenb...@gmail.com> Regards, Niek On Fri, Aug 14, 2020 at 7:54 PM Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > +Niek as maintainer. > > 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 AW_H3_DEV_*, to avoid conflicts. > > > > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com> > > --- > > include/hw/arm/allwinner-h3.h | 62 ++++++++--------- > > hw/arm/allwinner-h3.c | 124 +++++++++++++++++----------------- > > hw/arm/orangepi.c | 6 +- > > 3 files changed, 96 insertions(+), 96 deletions(-) > > > > diff --git a/include/hw/arm/allwinner-h3.h > b/include/hw/arm/allwinner-h3.h > > index 82e4e59216..626139dcb3 100644 > > --- a/include/hw/arm/allwinner-h3.h > > +++ b/include/hw/arm/allwinner-h3.h > > @@ -61,37 +61,37 @@ > > * @see AwH3State > > */ > > enum { > > - AW_H3_SRAM_A1, > > - AW_H3_SRAM_A2, > > - AW_H3_SRAM_C, > > - AW_H3_SYSCTRL, > > - AW_H3_MMC0, > > - AW_H3_SID, > > - AW_H3_EHCI0, > > - AW_H3_OHCI0, > > - AW_H3_EHCI1, > > - AW_H3_OHCI1, > > - AW_H3_EHCI2, > > - AW_H3_OHCI2, > > - AW_H3_EHCI3, > > - AW_H3_OHCI3, > > - AW_H3_CCU, > > - AW_H3_PIT, > > - AW_H3_UART0, > > - AW_H3_UART1, > > - AW_H3_UART2, > > - AW_H3_UART3, > > - AW_H3_EMAC, > > - AW_H3_DRAMCOM, > > - AW_H3_DRAMCTL, > > - AW_H3_DRAMPHY, > > - AW_H3_GIC_DIST, > > - AW_H3_GIC_CPU, > > - AW_H3_GIC_HYP, > > - AW_H3_GIC_VCPU, > > - AW_H3_RTC, > > - AW_H3_CPUCFG, > > - AW_H3_SDRAM > > + AW_H3_DEV_SRAM_A1, > > + AW_H3_DEV_SRAM_A2, > > + AW_H3_DEV_SRAM_C, > > + AW_H3_DEV_SYSCTRL, > > My 2 cents: > These are not devices, but peripheral blocks or IP cores. > Anyway, short change could be to name them E_AW_H3_xxx, > but I don't understand why these are exposed. This enum > should be locally declared in hw/arm/allwinner-h3.c, > and the SoC might provide a get_sdram_base_address() > method so the machine can access it. > > > + AW_H3_DEV_MMC0, > > + AW_H3_DEV_SID, > > + AW_H3_DEV_EHCI0, > > + AW_H3_DEV_OHCI0, > > + AW_H3_DEV_EHCI1, > > + AW_H3_DEV_OHCI1, > > + AW_H3_DEV_EHCI2, > > + AW_H3_DEV_OHCI2, > > + AW_H3_DEV_EHCI3, > > + AW_H3_DEV_OHCI3, > > + AW_H3_DEV_CCU, > > + AW_H3_DEV_PIT, > > + AW_H3_DEV_UART0, > > + AW_H3_DEV_UART1, > > + AW_H3_DEV_UART2, > > + AW_H3_DEV_UART3, > > + AW_H3_DEV_EMAC, > > + AW_H3_DEV_DRAMCOM, > > + AW_H3_DEV_DRAMCTL, > > + AW_H3_DEV_DRAMPHY, > > + AW_H3_DEV_GIC_DIST, > > + AW_H3_DEV_GIC_CPU, > > + AW_H3_DEV_GIC_HYP, > > + AW_H3_DEV_GIC_VCPU, > > + AW_H3_DEV_RTC, > > + AW_H3_DEV_CPUCFG, > > + AW_H3_DEV_SDRAM > > }; > > > > /** Total number of CPU cores in the H3 SoC */ > > diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c > > index ff92ded82c..341abe6718 100644 > > --- a/hw/arm/allwinner-h3.c > > +++ b/hw/arm/allwinner-h3.c > > @@ -35,37 +35,37 @@ > > > > /* Memory map */ > > const hwaddr allwinner_h3_memmap[] = { > > - [AW_H3_SRAM_A1] = 0x00000000, > > - [AW_H3_SRAM_A2] = 0x00044000, > > - [AW_H3_SRAM_C] = 0x00010000, > > - [AW_H3_SYSCTRL] = 0x01c00000, > > - [AW_H3_MMC0] = 0x01c0f000, > > - [AW_H3_SID] = 0x01c14000, > > - [AW_H3_EHCI0] = 0x01c1a000, > > - [AW_H3_OHCI0] = 0x01c1a400, > > - [AW_H3_EHCI1] = 0x01c1b000, > > - [AW_H3_OHCI1] = 0x01c1b400, > > - [AW_H3_EHCI2] = 0x01c1c000, > > - [AW_H3_OHCI2] = 0x01c1c400, > > - [AW_H3_EHCI3] = 0x01c1d000, > > - [AW_H3_OHCI3] = 0x01c1d400, > > - [AW_H3_CCU] = 0x01c20000, > > - [AW_H3_PIT] = 0x01c20c00, > > - [AW_H3_UART0] = 0x01c28000, > > - [AW_H3_UART1] = 0x01c28400, > > - [AW_H3_UART2] = 0x01c28800, > > - [AW_H3_UART3] = 0x01c28c00, > > - [AW_H3_EMAC] = 0x01c30000, > > - [AW_H3_DRAMCOM] = 0x01c62000, > > - [AW_H3_DRAMCTL] = 0x01c63000, > > - [AW_H3_DRAMPHY] = 0x01c65000, > > - [AW_H3_GIC_DIST] = 0x01c81000, > > - [AW_H3_GIC_CPU] = 0x01c82000, > > - [AW_H3_GIC_HYP] = 0x01c84000, > > - [AW_H3_GIC_VCPU] = 0x01c86000, > > - [AW_H3_RTC] = 0x01f00000, > > - [AW_H3_CPUCFG] = 0x01f01c00, > > - [AW_H3_SDRAM] = 0x40000000 > > + [AW_H3_DEV_SRAM_A1] = 0x00000000, > > + [AW_H3_DEV_SRAM_A2] = 0x00044000, > > + [AW_H3_DEV_SRAM_C] = 0x00010000, > > + [AW_H3_DEV_SYSCTRL] = 0x01c00000, > > + [AW_H3_DEV_MMC0] = 0x01c0f000, > > + [AW_H3_DEV_SID] = 0x01c14000, > > + [AW_H3_DEV_EHCI0] = 0x01c1a000, > > + [AW_H3_DEV_OHCI0] = 0x01c1a400, > > + [AW_H3_DEV_EHCI1] = 0x01c1b000, > > + [AW_H3_DEV_OHCI1] = 0x01c1b400, > > + [AW_H3_DEV_EHCI2] = 0x01c1c000, > > + [AW_H3_DEV_OHCI2] = 0x01c1c400, > > + [AW_H3_DEV_EHCI3] = 0x01c1d000, > > + [AW_H3_DEV_OHCI3] = 0x01c1d400, > > + [AW_H3_DEV_CCU] = 0x01c20000, > > + [AW_H3_DEV_PIT] = 0x01c20c00, > > + [AW_H3_DEV_UART0] = 0x01c28000, > > + [AW_H3_DEV_UART1] = 0x01c28400, > > + [AW_H3_DEV_UART2] = 0x01c28800, > > + [AW_H3_DEV_UART3] = 0x01c28c00, > > + [AW_H3_DEV_EMAC] = 0x01c30000, > > + [AW_H3_DEV_DRAMCOM] = 0x01c62000, > > + [AW_H3_DEV_DRAMCTL] = 0x01c63000, > > + [AW_H3_DEV_DRAMPHY] = 0x01c65000, > > + [AW_H3_DEV_GIC_DIST] = 0x01c81000, > > + [AW_H3_DEV_GIC_CPU] = 0x01c82000, > > + [AW_H3_DEV_GIC_HYP] = 0x01c84000, > > + [AW_H3_DEV_GIC_VCPU] = 0x01c86000, > > + [AW_H3_DEV_RTC] = 0x01f00000, > > + [AW_H3_DEV_CPUCFG] = 0x01f01c00, > > + [AW_H3_DEV_SDRAM] = 0x40000000 > > }; > > > > /* List of unimplemented devices */ > > @@ -183,7 +183,7 @@ void allwinner_h3_bootrom_setup(AwH3State *s, > BlockBackend *blk) > > } > > > > rom_add_blob("allwinner-h3.bootrom", buffer, rom_size, > > - rom_size, s->memmap[AW_H3_SRAM_A1], > > + rom_size, s->memmap[AW_H3_DEV_SRAM_A1], > > NULL, NULL, NULL, NULL, false); > > } > > > > @@ -262,10 +262,10 @@ static void allwinner_h3_realize(DeviceState *dev, > Error **errp) > > qdev_prop_set_bit(DEVICE(&s->gic), "has-virtualization-extensions", > true); > > sysbus_realize(SYS_BUS_DEVICE(&s->gic), &error_fatal); > > > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 0, > s->memmap[AW_H3_GIC_DIST]); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 1, > s->memmap[AW_H3_GIC_CPU]); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 2, > s->memmap[AW_H3_GIC_HYP]); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 3, > s->memmap[AW_H3_GIC_VCPU]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 0, > s->memmap[AW_H3_DEV_GIC_DIST]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 1, > s->memmap[AW_H3_DEV_GIC_CPU]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 2, > s->memmap[AW_H3_DEV_GIC_HYP]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->gic), 3, > s->memmap[AW_H3_DEV_GIC_VCPU]); > > > > /* > > * Wire the outputs from each CPU's generic timer and the GICv3 > > @@ -312,7 +312,7 @@ static void allwinner_h3_realize(DeviceState *dev, > Error **errp) > > > > /* Timer */ > > sysbus_realize(SYS_BUS_DEVICE(&s->timer), &error_fatal); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->timer), 0, s->memmap[AW_H3_PIT]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->timer), 0, > s->memmap[AW_H3_DEV_PIT]); > > sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer), 0, > > qdev_get_gpio_in(DEVICE(&s->gic), > AW_H3_GIC_SPI_TIMER0)); > > sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer), 1, > > @@ -325,32 +325,32 @@ static void allwinner_h3_realize(DeviceState *dev, > Error **errp) > > 32 * KiB, &error_abort); > > memory_region_init_ram(&s->sram_c, OBJECT(dev), "sram C", > > 44 * KiB, &error_abort); > > - memory_region_add_subregion(get_system_memory(), > s->memmap[AW_H3_SRAM_A1], > > + memory_region_add_subregion(get_system_memory(), > s->memmap[AW_H3_DEV_SRAM_A1], > > &s->sram_a1); > > - memory_region_add_subregion(get_system_memory(), > s->memmap[AW_H3_SRAM_A2], > > + memory_region_add_subregion(get_system_memory(), > s->memmap[AW_H3_DEV_SRAM_A2], > > &s->sram_a2); > > - memory_region_add_subregion(get_system_memory(), > s->memmap[AW_H3_SRAM_C], > > + memory_region_add_subregion(get_system_memory(), > s->memmap[AW_H3_DEV_SRAM_C], > > &s->sram_c); > > > > /* Clock Control Unit */ > > sysbus_realize(SYS_BUS_DEVICE(&s->ccu), &error_fatal); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccu), 0, s->memmap[AW_H3_CCU]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccu), 0, > s->memmap[AW_H3_DEV_CCU]); > > > > /* System Control */ > > sysbus_realize(SYS_BUS_DEVICE(&s->sysctrl), &error_fatal); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->sysctrl), 0, > s->memmap[AW_H3_SYSCTRL]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->sysctrl), 0, > s->memmap[AW_H3_DEV_SYSCTRL]); > > > > /* CPU Configuration */ > > sysbus_realize(SYS_BUS_DEVICE(&s->cpucfg), &error_fatal); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->cpucfg), 0, > s->memmap[AW_H3_CPUCFG]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->cpucfg), 0, > s->memmap[AW_H3_DEV_CPUCFG]); > > > > /* Security Identifier */ > > sysbus_realize(SYS_BUS_DEVICE(&s->sid), &error_fatal); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->sid), 0, s->memmap[AW_H3_SID]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->sid), 0, > s->memmap[AW_H3_DEV_SID]); > > > > /* SD/MMC */ > > sysbus_realize(SYS_BUS_DEVICE(&s->mmc0), &error_fatal); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->mmc0), 0, s->memmap[AW_H3_MMC0]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->mmc0), 0, > s->memmap[AW_H3_DEV_MMC0]); > > sysbus_connect_irq(SYS_BUS_DEVICE(&s->mmc0), 0, > > qdev_get_gpio_in(DEVICE(&s->gic), > AW_H3_GIC_SPI_MMC0)); > > > > @@ -364,63 +364,63 @@ static void allwinner_h3_realize(DeviceState *dev, > Error **errp) > > qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]); > > } > > sysbus_realize(SYS_BUS_DEVICE(&s->emac), &error_fatal); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->emac), 0, s->memmap[AW_H3_EMAC]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->emac), 0, > s->memmap[AW_H3_DEV_EMAC]); > > sysbus_connect_irq(SYS_BUS_DEVICE(&s->emac), 0, > > qdev_get_gpio_in(DEVICE(&s->gic), > AW_H3_GIC_SPI_EMAC)); > > > > /* Universal Serial Bus */ > > - sysbus_create_simple(TYPE_AW_H3_EHCI, s->memmap[AW_H3_EHCI0], > > + sysbus_create_simple(TYPE_AW_H3_EHCI, s->memmap[AW_H3_DEV_EHCI0], > > qdev_get_gpio_in(DEVICE(&s->gic), > > AW_H3_GIC_SPI_EHCI0)); > > - sysbus_create_simple(TYPE_AW_H3_EHCI, s->memmap[AW_H3_EHCI1], > > + sysbus_create_simple(TYPE_AW_H3_EHCI, s->memmap[AW_H3_DEV_EHCI1], > > qdev_get_gpio_in(DEVICE(&s->gic), > > AW_H3_GIC_SPI_EHCI1)); > > - sysbus_create_simple(TYPE_AW_H3_EHCI, s->memmap[AW_H3_EHCI2], > > + sysbus_create_simple(TYPE_AW_H3_EHCI, s->memmap[AW_H3_DEV_EHCI2], > > qdev_get_gpio_in(DEVICE(&s->gic), > > AW_H3_GIC_SPI_EHCI2)); > > - sysbus_create_simple(TYPE_AW_H3_EHCI, s->memmap[AW_H3_EHCI3], > > + sysbus_create_simple(TYPE_AW_H3_EHCI, s->memmap[AW_H3_DEV_EHCI3], > > qdev_get_gpio_in(DEVICE(&s->gic), > > AW_H3_GIC_SPI_EHCI3)); > > > > - sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI0], > > + sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_DEV_OHCI0], > > qdev_get_gpio_in(DEVICE(&s->gic), > > AW_H3_GIC_SPI_OHCI0)); > > - sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI1], > > + sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_DEV_OHCI1], > > qdev_get_gpio_in(DEVICE(&s->gic), > > AW_H3_GIC_SPI_OHCI1)); > > - sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI2], > > + sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_DEV_OHCI2], > > qdev_get_gpio_in(DEVICE(&s->gic), > > AW_H3_GIC_SPI_OHCI2)); > > - sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_OHCI3], > > + sysbus_create_simple("sysbus-ohci", s->memmap[AW_H3_DEV_OHCI3], > > qdev_get_gpio_in(DEVICE(&s->gic), > > AW_H3_GIC_SPI_OHCI3)); > > > > /* UART0. For future clocktree API: All UARTS are connected to > APB2_CLK. */ > > - serial_mm_init(get_system_memory(), s->memmap[AW_H3_UART0], 2, > > + serial_mm_init(get_system_memory(), s->memmap[AW_H3_DEV_UART0], 2, > > qdev_get_gpio_in(DEVICE(&s->gic), > AW_H3_GIC_SPI_UART0), > > 115200, serial_hd(0), DEVICE_NATIVE_ENDIAN); > > /* UART1 */ > > - serial_mm_init(get_system_memory(), s->memmap[AW_H3_UART1], 2, > > + serial_mm_init(get_system_memory(), s->memmap[AW_H3_DEV_UART1], 2, > > qdev_get_gpio_in(DEVICE(&s->gic), > AW_H3_GIC_SPI_UART1), > > 115200, serial_hd(1), DEVICE_NATIVE_ENDIAN); > > /* UART2 */ > > - serial_mm_init(get_system_memory(), s->memmap[AW_H3_UART2], 2, > > + serial_mm_init(get_system_memory(), s->memmap[AW_H3_DEV_UART2], 2, > > qdev_get_gpio_in(DEVICE(&s->gic), > AW_H3_GIC_SPI_UART2), > > 115200, serial_hd(2), DEVICE_NATIVE_ENDIAN); > > /* UART3 */ > > - serial_mm_init(get_system_memory(), s->memmap[AW_H3_UART3], 2, > > + serial_mm_init(get_system_memory(), s->memmap[AW_H3_DEV_UART3], 2, > > qdev_get_gpio_in(DEVICE(&s->gic), > AW_H3_GIC_SPI_UART3), > > 115200, serial_hd(3), DEVICE_NATIVE_ENDIAN); > > > > /* DRAMC */ > > sysbus_realize(SYS_BUS_DEVICE(&s->dramc), &error_fatal); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->dramc), 0, > s->memmap[AW_H3_DRAMCOM]); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->dramc), 1, > s->memmap[AW_H3_DRAMCTL]); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->dramc), 2, > s->memmap[AW_H3_DRAMPHY]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->dramc), 0, > s->memmap[AW_H3_DEV_DRAMCOM]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->dramc), 1, > s->memmap[AW_H3_DEV_DRAMCTL]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->dramc), 2, > s->memmap[AW_H3_DEV_DRAMPHY]); > > > > /* RTC */ > > sysbus_realize(SYS_BUS_DEVICE(&s->rtc), &error_fatal); > > - sysbus_mmio_map(SYS_BUS_DEVICE(&s->rtc), 0, s->memmap[AW_H3_RTC]); > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->rtc), 0, > s->memmap[AW_H3_DEV_RTC]); > > > > /* Unimplemented devices */ > > for (i = 0; i < ARRAY_SIZE(unimplemented); i++) { > > diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c > > index 1679468232..17a568a2b4 100644 > > --- a/hw/arm/orangepi.c > > +++ b/hw/arm/orangepi.c > > @@ -79,7 +79,7 @@ static void orangepi_init(MachineState *machine) > > object_property_set_int(OBJECT(&h3->emac), "phy-addr", 1, > &error_abort); > > > > /* DRAMC */ > > - object_property_set_uint(OBJECT(h3), "ram-addr", > h3->memmap[AW_H3_SDRAM], > > + object_property_set_uint(OBJECT(h3), "ram-addr", > h3->memmap[AW_H3_DEV_SDRAM], > > &error_abort); > > object_property_set_int(OBJECT(h3), "ram-size", machine->ram_size / > MiB, > > &error_abort); > > @@ -98,7 +98,7 @@ static void orangepi_init(MachineState *machine) > > qdev_realize_and_unref(carddev, bus, &error_fatal); > > > > /* SDRAM */ > > - memory_region_add_subregion(get_system_memory(), > h3->memmap[AW_H3_SDRAM], > > + memory_region_add_subregion(get_system_memory(), > h3->memmap[AW_H3_DEV_SDRAM], > > machine->ram); > > > > /* Load target kernel or start using BootROM */ > > @@ -106,7 +106,7 @@ static void orangepi_init(MachineState *machine) > > /* Use Boot ROM to copy data from SD card to SRAM */ > > allwinner_h3_bootrom_setup(h3, blk); > > } > > - orangepi_binfo.loader_start = h3->memmap[AW_H3_SDRAM]; > > + orangepi_binfo.loader_start = h3->memmap[AW_H3_DEV_SDRAM]; > > orangepi_binfo.ram_size = machine->ram_size; > > arm_load_kernel(ARM_CPU(first_cpu), machine, &orangepi_binfo); > > } > > > > -- Niek Linnenbank