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

[Philippe Mathieu-Daudé]

Hi Jamin,

On 7/7/26 08:09, Jamin Lin wrote:

The original AST2700 FC support series [1] is fairly large, making it
difficult to review all changes together.

To help speed up the review process, this series extracts the SCU/FMC-related
refactoring into a standalone prerequisite series.

Jamin Lin (8):
   hw/misc/aspeed_scu: Introduce Aspeed2700SCUState
   hw/arm/aspeed: Use Aspeed2700SCUState for AST2700 users
   hw/arm/aspeed_ast27x0: Move SCU link into AST27x0 coprocessors
   hw/misc/aspeed_scu: Add separate reset handler for AST2700 SCUIO
   hw/arm/aspeed_ast27x0: Pass realized PSP SoC to SSP/TSP initialization
   hw/arm/ast27x0: Share single SCUIO instance across PSP, SSP, and TSP
   hw/arm/ast27x0: Share FMC controller with SSP and TSP
   hw/ssi/aspeed_smc: Add Data FIFO-based flash access support for
     AST2700

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;
};