Re: [PATCH v2 0/8] Refactor AST2700 SCU preparation for coprocessors

[Cédric Le Goater]

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.