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 > > [email protected]/ > > > > 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.
