Hi Cédric
> Subject: Re: [PATCH v4 05/21] hw/arm/ast27x0: Add DRAM alias for SSP
> SDRAM remap
>
> On 4/17/26 05:28, Jamin Lin wrote:
> > This commit adds two MemoryRegion aliases to support PSP access to SSP
> > SDRAM through shared memory remapping.
> >
> > The SSP exposes two DRAM aliases:
> > - remap1 maps PSP DRAM at 0x400000000 to SSP SDRAM offset
> 0x5880000
> > - remap2 maps PSP DRAM at 0x42C000000 to SSP SDRAM offset 0x0
> >
> > These mappings follow the default SCU register configuration used by
> > the ASPEED SDK firmware, which defines the memory window mapping
> > between PSP and the SSP.
> >
> > Set SSP CPUID 4.
> >
> > Signed-off-by: Jamin Lin <[email protected]>
> > ---
> > include/hw/misc/aspeed_scu.h | 4 ++++
> > hw/arm/aspeed_ast27x0-fc.c | 2 ++
> > hw/arm/aspeed_ast27x0-ssp.c | 6 ++++++
> > hw/arm/aspeed_ast27x0.c | 4 ++++
> > hw/misc/aspeed_scu.c | 36
> ++++++++++++++++++++++++++++++++++++
> > 5 files changed, 52 insertions(+)
> >
> > diff --git a/include/hw/misc/aspeed_scu.h
> > b/include/hw/misc/aspeed_scu.h index fd30313e69..106ab4b04a 100644
> > --- a/include/hw/misc/aspeed_scu.h
> > +++ b/include/hw/misc/aspeed_scu.h
> > @@ -44,6 +44,10 @@ struct AspeedSCUState {
> >
> > struct Aspeed2700SCUState {
> > AspeedSCUState parent_obj;
> > +
> > + MemoryRegion dram_remap_alias[3];
> > + MemoryRegion *dram;
> > + int ssp_cpuid;
> > };
> >
> > #define AST2400_A1_SILICON_REV 0x02010303U
> > diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
> > index 46f0c97329..0688750e78 100644
> > --- a/hw/arm/aspeed_ast27x0-fc.c
> > +++ b/hw/arm/aspeed_ast27x0-fc.c
> > @@ -100,6 +100,8 @@ 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));
> > + object_property_set_int(OBJECT(&s->ca35), "ssp-cpuid", 4,
> > + &error_abort);
> > if (!qdev_realize(DEVICE(&s->ca35), NULL, errp)) {
> > return false;
> > }
> > diff --git a/hw/arm/aspeed_ast27x0-ssp.c b/hw/arm/aspeed_ast27x0-ssp.c
> > index edfcf1badb..0500c02755 100644
> > --- a/hw/arm/aspeed_ast27x0-ssp.c
> > +++ b/hw/arm/aspeed_ast27x0-ssp.c
> > @@ -199,6 +199,12 @@ static void
> aspeed_soc_ast27x0ssp_realize(DeviceState *dev_soc, Error **errp)
> > memory_region_add_subregion(s->memory,
> sc->memmap[ASPEED_DEV_SCU],
> > &a->scu_alias);
> >
> > + /* SDRAM remap alias used by PSP to access SSP SDRAM */
> > + memory_region_add_subregion(&s->sdram, 0,
> &a->scu->dram_remap_alias[1]);
> > + memory_region_add_subregion(&s->sdram,
> > + memory_region_size(&a->scu->dram_remap_alias[1]),
> > + &a->scu->dram_remap_alias[0]);
> > +
> > /* INTC */
> > if (!sysbus_realize(SYS_BUS_DEVICE(&a->intc[0]), errp)) {
> > return;
> > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> > e84975a9c4..e38b829cc7 100644
> > --- a/hw/arm/aspeed_ast27x0.c
> > +++ b/hw/arm/aspeed_ast27x0.c
> > @@ -432,6 +432,8 @@ static void aspeed_soc_ast2700_init(Object *obj)
> > "hw-strap1");
> > object_property_add_alias(obj, "hw-prot-key", OBJECT(&a->scu),
> > "hw-prot-key");
> > + object_property_add_alias(obj, "ssp-cpuid", OBJECT(&a->scu),
> > + "ssp-cpuid");
> >
> > object_initialize_child(obj, "scuio", &s->scuio,
> TYPE_ASPEED_2700_SCUIO);
> > qdev_prop_set_uint32(DEVICE(&s->scuio), "silicon-rev", @@ -794,6
> > +796,8 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error
> **errp)
> >
> sc->memmap[ASPEED_DEV_VBOOTROM],
> > &s->vbootrom);
> >
> > /* SCU */
> > + object_property_set_link(OBJECT(&a->scu), "dram",
> OBJECT(s->dram_mr),
> > + &error_abort);
> > if (!sysbus_realize(SYS_BUS_DEVICE(&a->scu), errp)) {
> > return;
> > }
> > diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c index
> > 51696443bd..7080bd6241 100644
> > --- a/hw/misc/aspeed_scu.c
> > +++ b/hw/misc/aspeed_scu.c
> > @@ -20,6 +20,7 @@
> > #include "qemu/guest-random.h"
> > #include "qemu/module.h"
> > #include "trace.h"
> > +#include "qemu/units.h"
> >
> > #define TO_REG(offset) ((offset) >> 2)
> >
> > @@ -961,9 +962,42 @@ static void aspeed_ast2700_scu_reset(DeviceState
> > *dev)
> >
> > static void aspeed_2700_scu_realize(DeviceState *dev, Error **errp)
> > {
> > + Aspeed2700SCUState *a = ASPEED_2700_SCU(dev);
> > +
> > aspeed_scu_realize(dev, errp);
> > +
> > + if (a->ssp_cpuid > 0) {
>
> hmm, this 'ssp_cpuid' atribute looks like an easy way to avoid a sub class.
>
I prefer creating an AST2700-specific subclass because it contains several
configurations that are unique to AST2700, such as:
SSP support
DRAM remapping
Keeping these settings in the AST2700 subclass avoids affecting the SCU
implementation used by other BMCs. It also improves code readability and
maintainability.
Therefore, I strongly suggest moving these configurations into
aspeed_2700_scu_realize().
>
> > + if (!a->dram) {
> > + error_setg(errp, TYPE_ASPEED_2700_SCU ": 'dram' link not
> set");
> > + return;
> > + }
> > + }
> > +
> > + if (a->ssp_cpuid > 0) {
>
> double if.
>
Will fix it.
Thanks,
Jamin
> > + /*
> > + * The SSP coprocessor uses two memory aliases (remap1 and
> remap2)
> > + * to access shared memory regions in the PSP DRAM:
> > + *
> > + * - remap1 maps PSP DRAM at 0x400000000 (size: 0x1A77E000)
> to
> > + * SSP SDRAM offset 0x5880000
> > + * - remap2 maps PSP DRAM at 0x42C000000 (size: 0x05880000)
> to
> > + * SSP SDRAM offset 0x0
> > + */
> > + memory_region_init_alias(&a->dram_remap_alias[0], OBJECT(a),
> > + "ssp.dram.remap1", a->dram,
> > + 0, 0x1a77e000);
> > + memory_region_init_alias(&a->dram_remap_alias[1], OBJECT(a),
> > + "ssp.dram.remap2", a->dram,
> > + 0x2c000000, 0x05880000);
> > + }
> > }
> >
> > +static const Property aspeed_2700_scu_properties[] = {
> > + DEFINE_PROP_INT32("ssp-cpuid", Aspeed2700SCUState, ssp_cpuid, -1),
> > + DEFINE_PROP_LINK("dram", Aspeed2700SCUState, dram,
> TYPE_MEMORY_REGION,
> > + MemoryRegion *), };
> > +
> > static void aspeed_2700_scu_class_init(ObjectClass *klass, const void
> *data)
> > {
> > DeviceClass *dc = DEVICE_CLASS(klass); @@ -972,6 +1006,8 @@
> > static void aspeed_2700_scu_class_init(ObjectClass *klass, const void *data)
> > dc->desc = "ASPEED 2700 System Control Unit";
> > device_class_set_legacy_reset(dc, aspeed_ast2700_scu_reset);
> > dc->realize = aspeed_2700_scu_realize;
> > + device_class_set_props(dc, aspeed_2700_scu_properties);
> > +
> > asc->resets = ast2700_a0_resets;
> > asc->calc_hpll = aspeed_2600_scu_calc_hpll;
> > asc->get_apb = aspeed_2700_scu_get_apb_freq;
