Hi Cédric

> Subject: Re: [PATCH v5 00/10] Add SSP/TSP power control and DRAM remap
> support for AST2700
> 
> Hello Jamin,
> 
> On 7/8/26 11:20, Jamin Lin wrote:
> > This series improves AST2700 platform support by aligning SSP/TSP
> > power and reset behavior with hardware, and enabling DRAM remapping
> > required for proper firmware boot flow.
> >
> > This series depends on:
> > [v2,0/8] Refactor AST2700 SCU preparation for coprocessors
> > https://patchwork.kernel.org/project/qemu-devel/cover/20260707060919.3
> > 50637-1-jamin_lin@aspeedtech.com/
> >
> > v1:
> >    1. The changes move DRAM/SDMC initialization earlier to support
> > memory aliasing, add DRAM aliases for SSP/TSP SDRAM remap, and
> > implement SSP/TSP reset, power-on, and remap controls via SCU registers.
> >    2. With these updates, SSP and TSP can be booted via PSP and load
> > their binaries from DRAM. Functional tests and documentation are
> > updated accordingly.
> >
> > v2:
> >    Fix "make check" failure caused by both AST2700 and AST1700 realizing
> the same
> >    TYPE_AST2700_SCU model.
> >
> > v3:
> >   1. Drop "Move DRAM and SDMC initialization earlier to support memory
> aliasing"
> >   2. Support SPI/FMC FIFO Mode
> >   3. Add unimplemented devices
> >
> > v4:
> >   1. Introduce Aspeed2700SCU subclass and separate from generic SCU.
> >   2. Add separate reset handler for AST2700 SCUIO
> >   3. Add AST2700 SCUIO RNG control and data registers
> >   4. Share single SCUIO instance across PSP, SSP, and TSP
> >   5. Fix AST2700 FC hardware strap settings
> >
> > v5:
> >   1. To speed up the review process, move the following patches into a
> separate
> >      patch series, as they are not directly related to the main topic of this
> series:
> >
> >      [v4,01/21] hw/misc/aspeed_scu: Introduce Aspeed2700SCU subclass
> and separate from generic SCU
> >      [v4,02/21] hw/misc/aspeed_scu: Add separate reset handler for
> AST2700 SCUIO
> >      [v4,11/21] hw/arm/ast27x0: Share FMC controller with SSP and TSP
> >      [v4,12/21] hw/arm/aspeed_ast27x0: Add unimplemented Privilege
> Controller MMIO regions for SSP/TSP (Merged)
> >      [v4,13/21] hw/arm/aspeed_ast27x0: Add unimplemented OTP
> controller MMIO regions for SSP/TSP (Merged)
> >      [v4,14/21] hw/block/m25p80: Implement volatile status register write
> enable for Winbond
> >      [v4,15/21] hw/ssi/aspeed_smc: Add Data FIFO-based flash access
> support for AST2700
> >      [v4,16/21] hw/misc/aspeed_scu: Drop noisy unhandled read logs for
> AST2700 SCU/SCUIO (Merged)
> >      [v4,17/21] hw/misc/aspeed_scu: Add AST2700 SCUIO RNG control and
> data registers (Merged)
> >      [v4,18/21] hw/arm/ast27x0: Share single SCUIO instance across PSP,
> SSP, and TSP
> >      [v4,19/21] hw/arm/aspeed_ast27x0-fc: Fix hardware strap settings
> > (Merged)
> >
> >    2. Add memory_region_transaction_begin() and
> memory_region_transaction_commit()
> >       to protect the transaction.
> >
> > Jamin Lin (10):
> >    hw/arm/ast27x0: Start SSP in powered-off state to match hardware
> >      behavior
> >    hw/arm/ast27x0: Start TSP in powered-off state to match hardware
> >      behavior
> >    hw/arm/ast27x0: Add DRAM alias for SSP SDRAM remap
> >    hw/arm/ast27x0: Add DRAM alias for TSP SDRAM remap
> >    hw/misc/aspeed_scu: Implement SSP reset and power-on control via SCU
> >      registers
> >    hw/misc/aspeed_scu: Implement TSP reset and power-on control via
> SCU
> >      registers
> >    hw/misc/aspeed_scu: Add SCU support for SSP SDRAM remap
> >    hw/misc/aspeed_scu: Add SCU support for TSP SDRAM remap
> >    tests/functional/aarch64/test_aspeed_ast2700fc: Boot SSP/TSP via PSP
> >      and load binaries from DRAM
> >    docs: Add support vbootrom and update Manual boot for ast2700fc
> >
> >   docs/system/arm/aspeed.rst                    |  42 ++-
> >   include/hw/misc/aspeed_scu.h                  |   5 +
> >   hw/arm/aspeed_ast27x0-fc.c                    |   4 +
> >   hw/arm/aspeed_ast27x0-ssp.c                   |  13 +
> >   hw/arm/aspeed_ast27x0-tsp.c                   |  10 +
> >   hw/arm/aspeed_ast27x0.c                       |   6 +
> >   hw/misc/aspeed_scu.c                          | 285
> ++++++++++++++++++
> >   .../aarch64/test_aspeed_ast2700fc.py          |  29 +-
> >   8 files changed, 374 insertions(+), 20 deletions(-)
> >
> 
> Here are some general comments on the design,
> 
> 
> * Aspeed SCU Models
> 
> Aspeed2700SCUState is used by 3 instances:
> - 1 PSP SCU, then linked in SSP and TSP
> - 2 I/O expander SCUs (all aspeed.scu-ast2700)
> 
> AspeedSCUState (the parent) is used by SCUIO (aspeed.scuio-ast2700)
> 
> The I/O expander embeds Aspeed2700SCUState and gets the full
> dram_remap_alias[3], ssp_cpuid, tsp_cpuid, and dram link.
> none of which make sense for I/O.
> 
> Also, I doubt the whole register space is implemented for all SoCs.
> This model seems overused to me and The I/O expander would need a fix to
> separate the SCU type.
> 
> * remap Memory Regions
> 
> I think we need to reorganize the code.
> 
> The dram_remap Memory regions belong to the SSP and TSP coprocessor SoC
> states. Makes more sense to me. The only reason they are under SCU today is
> because it makes things easier for the memory region controls.
> 
> So :
> 
>    include/hw/arm/aspeed_coprocessor.h:
>    @@ -57,6 +57,8 @@ struct Aspeed27x0CoprocessorState {
>         MemoryRegion scu_alias;
>         MemoryRegion scuio_alias;
>         MemoryRegion fmc_alias;
>    +    MemoryRegion dram_remap[2];
>    +    MemoryRegion *dram;
>         Aspeed2700SCUState *scu;
>         AspeedSCUState *scuio;
>         AspeedSMCState *fmc;
> 
>    include/hw/misc/aspeed_scu.h:
>    @@ -45,8 +45,8 @@ struct AspeedSCUState {
>     struct Aspeed2700SCUState {
>         AspeedSCUState parent_obj;
> 
>    +    MemoryRegion *ssp_remap[2];
>    +    MemoryRegion *tsp_remap;
> 
>     static const Property aspeed_2700_scu_properties[] = {
>    +    DEFINE_PROP_LINK("ssp-remap-0", Aspeed2700SCUState,
> ssp_remap[0],
>    +                     TYPE_MEMORY_REGION, MemoryRegion *),
>    +    DEFINE_PROP_LINK("ssp-remap-1", Aspeed2700SCUState,
> ssp_remap[1],
>    +                     TYPE_MEMORY_REGION, MemoryRegion *),
>    +    DEFINE_PROP_LINK("tsp-remap", Aspeed2700SCUState, tsp_remap,
>    +                     TYPE_MEMORY_REGION, MemoryRegion *),
>      };
> 
> The TSP and SSP models would initialize the alias(es) always :
> 
>    hw/arm/aspeed_ast27x0-ssp.c:
>    +    /* DRAM remap aliases for PSP to access SSP SDRAM */
>    +    memory_region_init_alias(&a->dram_remap[0], OBJECT(a),
>    +                             "ssp.dram.remap1", a->dram, 0,
> 0x1a77e000);
>    +    memory_region_init_alias(&a->dram_remap[1], OBJECT(a),
>    +                             "ssp.dram.remap2", a->dram,
> 0x2c000000, 0x05880000);
>    +    memory_region_add_subregion(&s->sdram, 0, &a->dram_remap[1]);
>    +    memory_region_add_subregion(&s->sdram,
>    +            memory_region_size(&a->dram_remap[1]),
> &a->dram_remap[0]);
>    +    object_property_set_link(OBJECT(a->scu), "ssp-remap-0",
>    +                             OBJECT(&a->dram_remap[0]),
> &error_abort);
>    +    object_property_set_link(OBJECT(a->scu), "ssp-remap-1",
>    +                             OBJECT(&a->dram_remap[1]),
> &error_abort);
> 
>         /* INTC */
>         if (!sysbus_realize(SYS_BUS_DEVICE(&a->intc[0]), errp)) {
>    @@ -323,6 +330,8 @@ static const Property aspeed_27x0_coproc
>                          TYPE_ASPEED_SCU, AspeedSCUState *),
>         DEFINE_PROP_LINK("fmc", Aspeed27x0CoprocessorState, fmc,
> TYPE_ASPEED_SMC,
>                          AspeedSMCState *),
>    +    DEFINE_PROP_LINK("dram", Aspeed27x0CoprocessorState, dram,
>    +                     TYPE_MEMORY_REGION, MemoryRegion *),
>     };
> 
>    hw/arm/aspeed_ast27x0-fc.c:
>         object_property_set_link(OBJECT(&s->ssp), "sram",
>                                  OBJECT(&psp->sram), &error_abort);
>    +    object_property_set_link(OBJECT(&s->ssp), "dram",
>    +                             OBJECT(psp->dram_mr),
> &error_abort);
>         object_property_set_link(OBJECT(&s->ssp), "scu",
>                                  OBJECT(&s->ca35.scu),
> &error_abort);
>         object_property_set_link(OBJECT(&s->ssp), "scuio",
> 
> 
> In SCU, we can drop the aspeed_2700_scu_realize() changes and the control
> bits become :
> 
>    hw/misc/aspeed_scu.c:
>         case AST2700_SCU_SSP_CTRL_1:
>         case AST2700_SCU_SSP_CTRL_2:
>    +        mr = (reg == AST2700_SCU_SSP_CTRL_1) ? a->ssp_remap[0] :
> a->ssp_remap[1];
>             if (a->ssp_cpuid < 0 || mr == NULL) {
>                 return;
>             }
> 
> 
> * CPU index
> 
> As for the "tsp-cpuid" and "ssp-cpuid" properties, we have a chicken&egg issue
> because the shared SCU model needs to be realized and the SSP and TSP CPUs
> too.
> 
> I would be tempted to simply bypass QOM to avoid hardcoding cpu_index
> values:
> 
>    hw/arm/aspeed_ast27x0-ssp.c:
>          object_property_set_bool(OBJECT(&a->armv7m),
> "start-powered-off", true,
>                                   &error_abort);
>          sysbus_realize(SYS_BUS_DEVICE(&a->armv7m), &error_abort);
>     +    a->scu->ssp_cpuid = CPU(a->armv7m.cpu)->cpu_index;
> 
>          /* SDRAM */
>          sdram_name = g_strdup_printf("aspeed.sdram.%d",
> 
>    hw/arm/aspeed_ast27x0-tsp.c:
>          object_property_set_bool(OBJECT(&a->armv7m),
> "start-powered-off", true,
>                                   &error_abort);
>          sysbus_realize(SYS_BUS_DEVICE(&a->armv7m), &error_abort);
>     +    a->scu->tsp_cpuid = CPU(a->armv7m.cpu)->cpu_index;
> 
>          /* SDRAM */
>          sdram_name = g_strdup_printf("aspeed.sdram.%d",
> 

Thanks for the review and the helpful suggestions.
I'll address them in the next revision.
I really appreciate your feedback and support.

Thanks,
Jamin

> Or keep it that way for now.
> 
> 
> Thanks,
> 
> C.