Hi Philippe
> Looking at this machine in more detail, I think it would be better modelled as:
>
> struct Ast2700FcMachineState {
> MachineState parent_obj;
>
> MemoryRegion dram;
> Aspeed2700SCUState scu;
>
> Aspeed27x0SoCState psp;
> MemoryRegion psp_memory;
> MemoryRegion psp_bootrom;
>
> Aspeed27x0CoprocessorState ssp;
> Clock *ssp_sysclk;
> MemoryRegion ssp_memory;
>
> Aspeed27x0CoprocessorState tsp;
> Clock *tsp_sysclk;
> MemoryRegion tsp_memory;
> };
>
> - dram and scu are shared within psp/ssp/tsp
> (ca35_dram renamed as generic dram)
> (ca35_memory renamed as psp_memory)
> (ca35_boot_rom renamed as psp_bootrom)
>
> and:
>
> struct Aspeed27x0SoCState {
> AspeedSoCState parent;
>
> ARMCPU cpu[ASPEED_CPUS_NUM];
> AspeedINTCState intc[ASPEED_INTC_NUM];
> AspeedINTCState intcioexp[ASPEED_IOEXP_NUM];
> GICv3State gic;
> MemoryRegion dram_empty;
> Aspeed2700SCUState *scu;
> };
>
> - scu becomes a link property
>
> struct Ast2700FcMachineState {
> MachineState parent_obj;
>
> MemoryRegion dram;
> Aspeed2700SCUState scu;
>
> Aspeed27x0SoCState psp;
> MemoryRegion psp_memory;
> MemoryRegion psp_bootrom;
>
> Aspeed27x0CoprocessorState ssp;
> Clock *ssp_sysclk;
> MemoryRegion ssp_memory;
>
> Aspeed27x0CoprocessorState tsp;
> Clock *tsp_sysclk;
> MemoryRegion tsp_memory;
> };
>
> My personal style preference being:
>
> struct Ast2700FcMachineState {
> MachineState parent_obj;
>
> MemoryRegion dram;
> Aspeed2700SCUState scu;
>
> struct {
> Aspeed27x0SoCState mpcore;
> MemoryRegion memory;
> MemoryRegion bootrom;
> } psp;
>
> struct {
> Aspeed27x0CoprocessorState mcu;
> Clock *sysclk;
> MemoryRegion memory;
> } ssp;
>
> struct {
> Aspeed27x0CoprocessorState mcu;
> Clock *sysclk;
> MemoryRegion memory;
> } tsp;
> };
Thanks for the suggestion. I've reworked Ast2700FCState to match your preferred nested-struct style — psp/ssp/tsp are now grouped structs (mpcore/memory/bootrom for psp, mcu/sysclk/memory for ssp and tsp),
and ca35_dram is renamed to the generic dram as you suggested. Diff attached/inline below. Built and boot-tested both ast2700fc and ast2700a2-evb to confirm nothing regressed.
One part I couldn't do yet: making Aspeed27x0SoCState.scu a link property.
The blocker is hw-strap1/hw-strap2: they're property aliases created in instance_init(), pointing at the embedded scu. Both ast2700fc.c and hw/arm/aspeed.c set them right after object creation, before realize — but a link wouldn't be populated that early, so the alias would point at NULL.
hw/arm/aspeed.c isn't AST2700-specific — it's shared by every Aspeed BMC board. Fixing the timing there would touch all 24 of them (2400/2500/2600/1030/1040/2700 combined). "I do not prefer to change it to a link property."
Thanks,
Jamin
diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
index 058cea42ed..417162bb02 100644
--- a/hw/arm/aspeed_ast27x0-fc.c
+++ b/hw/arm/aspeed_ast27x0-fc.c
@@ -34,18 +34,25 @@ static struct arm_boot_info ast2700fc_board_info = {
struct Ast2700FCState {
MachineState parent_obj;
- MemoryRegion ca35_memory;
- MemoryRegion ca35_dram;
- MemoryRegion ca35_boot_rom;
- MemoryRegion ssp_memory;
- MemoryRegion tsp_memory;
-
- Clock *ssp_sysclk;
- Clock *tsp_sysclk;
-
- Aspeed27x0SoCState ca35;
- Aspeed27x0CoprocessorState ssp;
- Aspeed27x0CoprocessorState tsp;
+ MemoryRegion dram;
+
+ struct {
+ Aspeed27x0SoCState mpcore;
+ MemoryRegion memory;
+ MemoryRegion bootrom;
+ } psp;
+
+ struct {
+ Aspeed27x0CoprocessorState mcu;
+ Clock *sysclk;
+ MemoryRegion memory;
+ } ssp;
+
+ struct {
+ Aspeed27x0CoprocessorState mcu;
+ Clock *sysclk;
+ MemoryRegion memory;
+ } tsp;
};
#define AST2700FC_BMC_RAM_SIZE (2 * GiB)
@@ -67,23 +74,23 @@ static bool ast2700fc_ca35_init(MachineState *machine, Error **errp)
DeviceState *dev = NULL;
uint64_t rom_size;
- object_initialize_child(OBJECT(s), "ca35", &s->ca35, "ast2700-a2");
- soc = ASPEED_SOC(&s->ca35);
+ object_initialize_child(OBJECT(s), "ca35", &s->psp.mpcore, "ast2700-a2");
+ soc = ASPEED_SOC(&s->psp.mpcore);
sc = ASPEED_SOC_GET_CLASS(soc);
- memory_region_init(&s->ca35_memory, OBJECT(&s->ca35), "ca35-memory",
+ memory_region_init(&s->psp.memory, OBJECT(&s->psp.mpcore), "psp-memory",
UINT64_MAX);
- memory_region_add_subregion(get_system_memory(), 0, &s->ca35_memory);
+ memory_region_add_subregion(get_system_memory(), 0, &s->psp.memory);
- if (!memory_region_init_ram(&s->ca35_dram, OBJECT(&s->ca35), "ca35-dram",
+ if (!memory_region_init_ram(&s->dram, OBJECT(&s->psp.mpcore), "dram",
AST2700FC_BMC_RAM_SIZE, errp)) {
return false;
}
- object_property_set_link(OBJECT(&s->ca35), "memory",
- OBJECT(&s->ca35_memory), &error_abort);
- object_property_set_link(OBJECT(&s->ca35), "dram", OBJECT(&s->ca35_dram),
- &error_abort);
- object_property_set_int(OBJECT(&s->ca35), "ram-size",
+ object_property_set_link(OBJECT(&s->psp.mpcore), "memory",
+ OBJECT(&s->psp.memory), &error_abort);
+ object_property_set_link(OBJECT(&s->psp.mpcore), "dram",
+ OBJECT(&s->dram), &error_abort);
+ object_property_set_int(OBJECT(&s->psp.mpcore), "ram-size",
AST2700FC_BMC_RAM_SIZE, &error_abort);
for (int i = 0; i < sc->macs_num; i++) {
@@ -92,9 +99,9 @@ static bool ast2700fc_ca35_init(MachineState *machine, Error **errp)
break;
}
}
- object_property_set_int(OBJECT(&s->ca35), "hw-strap1",
+ object_property_set_int(OBJECT(&s->psp.mpcore), "hw-strap1",
AST2700FC_HW_STRAP1, &error_abort);
- object_property_set_int(OBJECT(&s->ca35), "hw-strap2",
+ object_property_set_int(OBJECT(&s->psp.mpcore), "hw-strap2",
AST2700FC_HW_STRAP2, &error_abort);
aspeed_soc_uart_set_chr(soc->uart, ASPEED_DEV_UART12, sc->uarts_base,
sc->uarts_num, serial_hd(0));
@@ -102,7 +109,7 @@ static bool ast2700fc_ca35_init(MachineState *machine, Error **errp)
sc->uarts_num, serial_hd(1));
aspeed_soc_uart_set_chr(soc->uart, ASPEED_DEV_UART7, sc->uarts_base,
sc->uarts_num, serial_hd(2));
- if (!qdev_realize(DEVICE(&s->ca35), NULL, errp)) {
+ if (!qdev_realize(DEVICE(&s->psp.mpcore), NULL, errp)) {
return false;
}
@@ -122,7 +129,7 @@ static bool ast2700fc_ca35_init(MachineState *machine, Error **errp)
if (fmc0) {
rom_size = memory_region_size(&soc->spi_boot);
- aspeed_install_boot_rom(soc, fmc0, &s->ca35_boot_rom, rom_size);
+ aspeed_install_boot_rom(soc, fmc0, &s->psp.bootrom, rom_size);
}
/* VBOOTROM */
@@ -134,68 +141,68 @@ static bool ast2700fc_ca35_init(MachineState *machine, Error **errp)
return true;
}
-static bool ast2700fc_ssp_init(Ast2700FCState *s, AspeedSoCState *psp,
+static bool ast2700fc_ssp_init(Ast2700FCState *s, AspeedSoCState *soc,
Error **errp)
{
- s->ssp_sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
- clock_set_hz(s->ssp_sysclk, 200000000ULL);
+ s->ssp.sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
+ clock_set_hz(s->ssp.sysclk, 200000000ULL);
- object_initialize_child(OBJECT(s), "ssp", &s->ssp,
+ object_initialize_child(OBJECT(s), "ssp", &s->ssp.mcu,
TYPE_ASPEED27X0SSP_COPROCESSOR);
- memory_region_init(&s->ssp_memory, OBJECT(&s->ssp), "ssp-memory",
+ memory_region_init(&s->ssp.memory, OBJECT(&s->ssp.mcu), "ssp-memory",
UINT64_MAX);
- qdev_connect_clock_in(DEVICE(&s->ssp), "sysclk", s->ssp_sysclk);
- object_property_set_link(OBJECT(&s->ssp), "memory",
- OBJECT(&s->ssp_memory), &error_abort);
+ qdev_connect_clock_in(DEVICE(&s->ssp.mcu), "sysclk", s->ssp.sysclk);
+ object_property_set_link(OBJECT(&s->ssp.mcu), "memory",
+ OBJECT(&s->ssp.memory), &error_abort);
- object_property_set_link(OBJECT(&s->ssp), "uart",
- OBJECT(&psp->uart[4]), &error_abort);
- object_property_set_int(OBJECT(&s->ssp), "uart-dev", ASPEED_DEV_UART4,
+ object_property_set_link(OBJECT(&s->ssp.mcu), "uart",
+ OBJECT(&soc->uart[4]), &error_abort);
+ object_property_set_int(OBJECT(&s->ssp.mcu), "uart-dev", ASPEED_DEV_UART4,
&error_abort);
- object_property_set_link(OBJECT(&s->ssp), "sram",
- OBJECT(&psp->sram), &error_abort);
- object_property_set_link(OBJECT(&s->ssp), "scu",
- OBJECT(&s->ca35.scu), &error_abort);
- object_property_set_link(OBJECT(&s->ssp), "scuio",
- OBJECT(&psp->scuio), &error_abort);
- object_property_set_link(OBJECT(&s->ssp), "fmc",
- OBJECT(&psp->fmc), &error_abort);
- if (!qdev_realize(DEVICE(&s->ssp), NULL, errp)) {
+ object_property_set_link(OBJECT(&s->ssp.mcu), "sram",
+ OBJECT(&soc->sram), &error_abort);
+ object_property_set_link(OBJECT(&s->ssp.mcu), "scu",
+ OBJECT(&s->psp.mpcore.scu), &error_abort);
+ object_property_set_link(OBJECT(&s->ssp.mcu), "scuio",
+ OBJECT(&soc->scuio), &error_abort);
+ object_property_set_link(OBJECT(&s->ssp.mcu), "fmc",
+ OBJECT(&soc->fmc), &error_abort);
+ if (!qdev_realize(DEVICE(&s->ssp.mcu), NULL, errp)) {
return false;
}
return true;
}
-static bool ast2700fc_tsp_init(Ast2700FCState *s, AspeedSoCState *psp,
+static bool ast2700fc_tsp_init(Ast2700FCState *s, AspeedSoCState *soc,
Error **errp)
{
- s->tsp_sysclk = clock_new(OBJECT(s), "TSP_SYSCLK");
- clock_set_hz(s->tsp_sysclk, 200000000ULL);
+ s->tsp.sysclk = clock_new(OBJECT(s), "TSP_SYSCLK");
+ clock_set_hz(s->tsp.sysclk, 200000000ULL);
- object_initialize_child(OBJECT(s), "tsp", &s->tsp,
+ object_initialize_child(OBJECT(s), "tsp", &s->tsp.mcu,
TYPE_ASPEED27X0TSP_COPROCESSOR);
- memory_region_init(&s->tsp_memory, OBJECT(&s->tsp), "tsp-memory",
+ memory_region_init(&s->tsp.memory, OBJECT(&s->tsp.mcu), "tsp-memory",
UINT64_MAX);
- qdev_connect_clock_in(DEVICE(&s->tsp), "sysclk", s->tsp_sysclk);
- object_property_set_link(OBJECT(&s->tsp), "memory",
- OBJECT(&s->tsp_memory), &error_abort);
+ qdev_connect_clock_in(DEVICE(&s->tsp.mcu), "sysclk", s->tsp.sysclk);
+ object_property_set_link(OBJECT(&s->tsp.mcu), "memory",
+ OBJECT(&s->tsp.memory), &error_abort);
- object_property_set_link(OBJECT(&s->tsp), "uart",
- OBJECT(&psp->uart[7]), &error_abort);
- object_property_set_int(OBJECT(&s->tsp), "uart-dev", ASPEED_DEV_UART7,
+ object_property_set_link(OBJECT(&s->tsp.mcu), "uart",
+ OBJECT(&soc->uart[7]), &error_abort);
+ object_property_set_int(OBJECT(&s->tsp.mcu), "uart-dev", ASPEED_DEV_UART7,
&error_abort);
- object_property_set_link(OBJECT(&s->tsp), "sram",
- OBJECT(&psp->sram), &error_abort);
- object_property_set_link(OBJECT(&s->tsp), "scu",
- OBJECT(&s->ca35.scu), &error_abort);
- object_property_set_link(OBJECT(&s->tsp), "scuio",
- OBJECT(&psp->scuio), &error_abort);
- object_property_set_link(OBJECT(&s->tsp), "fmc",
- OBJECT(&psp->fmc), &error_abort);
- if (!qdev_realize(DEVICE(&s->tsp), NULL, errp)) {
+ object_property_set_link(OBJECT(&s->tsp.mcu), "sram",
+ OBJECT(&soc->sram), &error_abort);
+ object_property_set_link(OBJECT(&s->tsp.mcu), "scu",
+ OBJECT(&s->psp.mpcore.scu), &error_abort);
+ object_property_set_link(OBJECT(&s->tsp.mcu), "scuio",
+ OBJECT(&soc->scuio), &error_abort);
+ object_property_set_link(OBJECT(&s->tsp.mcu), "fmc",
+ OBJECT(&soc->fmc), &error_abort);
+ if (!qdev_realize(DEVICE(&s->tsp.mcu), NULL, errp)) {
return false;
}
@@ -214,7 +221,7 @@ static void ast2700fc_init(MachineState *machine)
* SRAM, SCU and SCUIO. Therefore the PSP SoC must be realized
* before the coprocessors are initialized.
*/
- psp = ASPEED_SOC(&s->ca35);
+ psp = ASPEED_SOC(&s->psp.mpcore);
ast2700fc_ssp_init(s, psp, &error_abort);
ast2700fc_tsp_init(s, psp, &error_abort);
}
On 7/16/26 08:22, Jamin Lin wrote:
> Hi Philippe
>
>> Looking at this machine in more detail, I think it would be better modelled as:
>>
>> struct Ast2700FcMachineState {
>> MachineState parent_obj;
>>
>> MemoryRegion dram;
>> Aspeed2700SCUState scu;
>>
>> Aspeed27x0SoCState psp;
>> MemoryRegion psp_memory;
>> MemoryRegion psp_bootrom;
>>
>> Aspeed27x0CoprocessorState ssp;
>> Clock *ssp_sysclk;
>> MemoryRegion ssp_memory;
>>
>> Aspeed27x0CoprocessorState tsp;
>> Clock *tsp_sysclk;
>> MemoryRegion tsp_memory;
>> };
>>
>> - dram and scu are shared within psp/ssp/tsp
>> (ca35_dram renamed as generic dram)
>> (ca35_memory renamed as psp_memory)
>> (ca35_boot_rom renamed as psp_bootrom)
>>
>> and:
>>
>> struct Aspeed27x0SoCState {
>> AspeedSoCState parent;
>>
>> ARMCPU cpu[ASPEED_CPUS_NUM];
>> AspeedINTCState intc[ASPEED_INTC_NUM];
>> AspeedINTCState intcioexp[ASPEED_IOEXP_NUM];
>> GICv3State gic;
>> MemoryRegion dram_empty;
>> Aspeed2700SCUState *scu;
>> };
>>
>> - scu becomes a link property
>>
>> struct Ast2700FcMachineState {
>> MachineState parent_obj;
>>
>> MemoryRegion dram;
>> Aspeed2700SCUState scu;
>>
>> Aspeed27x0SoCState psp;
>> MemoryRegion psp_memory;
>> MemoryRegion psp_bootrom;
>>
>> Aspeed27x0CoprocessorState ssp;
>> Clock *ssp_sysclk;
>> MemoryRegion ssp_memory;
>>
>> Aspeed27x0CoprocessorState tsp;
>> Clock *tsp_sysclk;
>> MemoryRegion tsp_memory;
>> };
>>
>> My personal style preference being:
>>
>> struct Ast2700FcMachineState {
>> MachineState parent_obj;
>>
>> MemoryRegion dram;
>> Aspeed2700SCUState scu;
>>
>> struct {
>> Aspeed27x0SoCState mpcore;
>> MemoryRegion memory;
>> MemoryRegion bootrom;
>> } psp;
>>
>> struct {
>> Aspeed27x0CoprocessorState mcu;
>> Clock *sysclk;
>> MemoryRegion memory;
>> } ssp;
>>
>> struct {
>> Aspeed27x0CoprocessorState mcu;
>> Clock *sysclk;
>> MemoryRegion memory;
>> } tsp;
>> };
>
> Thanks for the suggestion. I've reworked Ast2700FCState to match your preferred nested-struct style — psp/ssp/tsp are now grouped structs (mpcore/memory/bootrom for psp, mcu/sysclk/memory for ssp and tsp),
> and ca35_dram is renamed to the generic dram as you suggested. Diff attached/inline below. Built and boot-tested both ast2700fc and ast2700a2-evb to confirm nothing regressed.
>
> One part I couldn't do yet: making Aspeed27x0SoCState.scu a link property.
>
> The blocker is hw-strap1/hw-strap2: they're property aliases created in instance_init(), pointing at the embedded scu. Both ast2700fc.c and hw/arm/aspeed.c set them right after object creation, before realize — but a link wouldn't be populated that early, so the alias would point at NULL.
>
> hw/arm/aspeed.c isn't AST2700-specific — it's shared by every Aspeed BMC board. Fixing the timing there would touch all 24 of them (2400/2500/2600/1030/1040/2700 combined). "I do not prefer to change it to a link property."
>
> Thanks,
> Jamin
>
> diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
> index 058cea42ed..417162bb02 100644
> --- a/hw/arm/aspeed_ast27x0-fc.c
> +++ b/hw/arm/aspeed_ast27x0-fc.c
> @@ -34,18 +34,25 @@ static struct arm_boot_info ast2700fc_board_info = {
> struct Ast2700FCState {
> MachineState parent_obj;
>
> - MemoryRegion ca35_memory;
> - MemoryRegion ca35_dram;
> - MemoryRegion ca35_boot_rom;
> - MemoryRegion ssp_memory;
> - MemoryRegion tsp_memory;
> -
> - Clock *ssp_sysclk;
> - Clock *tsp_sysclk;
> -
> - Aspeed27x0SoCState ca35;
> - Aspeed27x0CoprocessorState ssp;
> - Aspeed27x0CoprocessorState tsp;
> + MemoryRegion dram;
> +
> + struct {
> + Aspeed27x0SoCState mpcore;
> + MemoryRegion memory;
> + MemoryRegion bootrom;
> + } psp;
> +
> + struct {
> + Aspeed27x0CoprocessorState mcu;
> + Clock *sysclk;
> + MemoryRegion memory;
> + } ssp;
> +
> + struct {
> + Aspeed27x0CoprocessorState mcu;
> + Clock *sysclk;
> + MemoryRegion memory;
> + } tsp;
I prefer the current version which reflects "a bit" better
the topology with the QOM tree. This is just personal test,
plus the intuition that Aspeed27x0CoprocessorState could
grow.
Thanks,
C.
© 2016 - 2026 Red Hat, Inc.