Hi Cédric > Subject: Re: [PATCH v4 01/21] hw/misc/aspeed_scu: Introduce Aspeed2700SCU > subclass and separate from generic SCU > > On 4/17/26 05:28, Jamin Lin wrote: > > Introduce a dedicated Aspeed2700SCUState subclass to isolate > > AST2700-specific behavior from the generic AspeedSCUState > implementation. > > > > Previously, AST17x0/27x0 platforms reused AspeedSCUState directly, > > including in coprocessor paths. With AST2700 introducing additional > requirements (e.g. > > future coprocessor features such as DRAM remapping), continuing to > > extend the generic SCU risks impacting other SoCs. > > > > To address this, add Aspeed2700SCUState as a subclass of > > AspeedSCUState and migrate all AST2700-related users to it, including: > > - AST1700 SoC > > - AST27x0 SoC > > - AST27x0 FC > > - AST27x0 coprocessors > > > > As part of this refactoring: > > - Remove SCU linkage from the common AspeedCoprocessorState > > - Add a dedicated 'scu' link property to Aspeed27x0CoprocessorState > > - Update SCU memory aliasing to reference the subclass > > (parent_obj.iomem) > > > > This separation keeps the common code generic while enabling > > AST2700-specific extensions in both SCU and coprocessor paths. > > > > No functional change. > > I think this patch needs to be broken down further. > Thanks for the review and suggestion. Will separate more patches for these changes. Thanks, Jamin > > > > Signed-off-by: Jamin Lin <[email protected]> > > --- > > include/hw/arm/aspeed_ast1700.h | 2 +- > > include/hw/arm/aspeed_coprocessor.h | 5 +++-- > > include/hw/arm/aspeed_soc.h | 1 + > > include/hw/misc/aspeed_scu.h | 5 +++++ > > hw/arm/aspeed_ast27x0-fc.c | 4 ++-- > > hw/arm/aspeed_ast27x0-ssp.c | 14 ++++++++++---- > > hw/arm/aspeed_ast27x0-tsp.c | 14 ++++++++++---- > > hw/arm/aspeed_ast27x0.c | 16 ++++++++-------- > > hw/arm/aspeed_coprocessor_common.c | 2 -- > > hw/misc/aspeed_scu.c | 8 +++++++- > > 10 files changed, 47 insertions(+), 24 deletions(-) > > > > diff --git a/include/hw/arm/aspeed_ast1700.h > > b/include/hw/arm/aspeed_ast1700.h index f7bd4e8650..39c5977cf1 100644 > > --- a/include/hw/arm/aspeed_ast1700.h > > +++ b/include/hw/arm/aspeed_ast1700.h > > @@ -41,7 +41,7 @@ struct AspeedAST1700SoCState { > > MemoryRegion sram; > > AspeedSMCState spi; > > AspeedADCState adc; > > - AspeedSCUState scu; > > + Aspeed2700SCUState scu; > > AspeedGPIOState gpio; > > AspeedSGPIOState sgpiom[AST1700_SGPIO_NUM]; > > AspeedI2CState i2c; > > diff --git a/include/hw/arm/aspeed_coprocessor.h > > b/include/hw/arm/aspeed_coprocessor.h > > index 4a50f688ec..4db995d251 100644 > > --- a/include/hw/arm/aspeed_coprocessor.h > > +++ b/include/hw/arm/aspeed_coprocessor.h > > @@ -20,10 +20,8 @@ struct AspeedCoprocessorState { > > MemoryRegion *sram; > > MemoryRegion sram_alias; > > MemoryRegion uart_alias; > > - MemoryRegion scu_alias; > > Clock *sysclk; > > > > - AspeedSCUState *scu; > > AspeedSCUState scuio; > > AspeedTimerCtrlState timerctrl; > > SerialMM *uart; > > @@ -50,6 +48,9 @@ struct Aspeed27x0CoprocessorState { > > UnimplementedDeviceState scuio; > > > > ARMv7MState armv7m; > > + > > + MemoryRegion scu_alias; > > + Aspeed2700SCUState *scu; > > }; > > > > #define TYPE_ASPEED27X0SSP_COPROCESSOR > "aspeed27x0ssp-coprocessor" > > diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h > > index d7b3647ca1..adf2f2898a 100644 > > --- a/include/hw/arm/aspeed_soc.h > > +++ b/include/hw/arm/aspeed_soc.h > > @@ -151,6 +151,7 @@ struct Aspeed27x0SoCState { > > AspeedINTCState intcioexp[ASPEED_IOEXP_NUM]; > > GICv3State gic; > > MemoryRegion dram_empty; > > + Aspeed2700SCUState scu; > > }; > > > > #define TYPE_ASPEED27X0_SOC "aspeed27x0-soc" > > diff --git a/include/hw/misc/aspeed_scu.h > > b/include/hw/misc/aspeed_scu.h index d003955428..fd30313e69 100644 > > --- a/include/hw/misc/aspeed_scu.h > > +++ b/include/hw/misc/aspeed_scu.h > > @@ -20,6 +20,7 @@ OBJECT_DECLARE_TYPE(AspeedSCUState, > AspeedSCUClass, ASPEED_SCU) > > #define TYPE_ASPEED_2500_SCU TYPE_ASPEED_SCU "-ast2500" > > #define TYPE_ASPEED_2600_SCU TYPE_ASPEED_SCU "-ast2600" > > #define TYPE_ASPEED_2700_SCU TYPE_ASPEED_SCU "-ast2700" > > +OBJECT_DECLARE_SIMPLE_TYPE(Aspeed2700SCUState, > ASPEED_2700_SCU) > > #define TYPE_ASPEED_2700_SCUIO TYPE_ASPEED_SCU "io" "-ast2700" > > #define TYPE_ASPEED_1030_SCU TYPE_ASPEED_SCU "-ast1030" > > > > @@ -41,6 +42,10 @@ struct AspeedSCUState { > > uint32_t hw_prot_key; > > }; > > > > +struct Aspeed2700SCUState { > > + AspeedSCUState parent_obj; > > +}; > > + > > #define AST2400_A1_SILICON_REV 0x02010303U > > #define AST2500_A1_SILICON_REV 0x04010303U > > #define AST2600_A3_SILICON_REV 0x05030303U > > diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c > > index 5eb6680da9..46f0c97329 100644 > > --- a/hw/arm/aspeed_ast27x0-fc.c > > +++ b/hw/arm/aspeed_ast27x0-fc.c > > @@ -156,7 +156,7 @@ static bool ast2700fc_ssp_init(MachineState > *machine, Error **errp) > > object_property_set_link(OBJECT(&s->ssp), "sram", > > OBJECT(&psp->sram), &error_abort); > > object_property_set_link(OBJECT(&s->ssp), "scu", > > - OBJECT(&psp->scu), &error_abort); > > + OBJECT(&s->ca35.scu), &error_abort); > > if (!qdev_realize(DEVICE(&s->ssp), NULL, errp)) { > > return false; > > } > > @@ -188,7 +188,7 @@ static bool ast2700fc_tsp_init(MachineState > *machine, Error **errp) > > object_property_set_link(OBJECT(&s->tsp), "sram", > > OBJECT(&psp->sram), &error_abort); > > object_property_set_link(OBJECT(&s->tsp), "scu", > > - OBJECT(&psp->scu), &error_abort); > > + OBJECT(&s->ca35.scu), &error_abort); > > if (!qdev_realize(DEVICE(&s->tsp), NULL, errp)) { > > return false; > > } > > diff --git a/hw/arm/aspeed_ast27x0-ssp.c b/hw/arm/aspeed_ast27x0-ssp.c > > index 8b84300e0f..9a2079668b 100644 > > --- a/hw/arm/aspeed_ast27x0-ssp.c > > +++ b/hw/arm/aspeed_ast27x0-ssp.c > > @@ -186,11 +186,11 @@ static void > aspeed_soc_ast27x0ssp_realize(DeviceState *dev_soc, Error **errp) > > &s->sram_alias); > > > > /* SCU */ > > - memory_region_init_alias(&s->scu_alias, OBJECT(s), "scu.alias", > > - &s->scu->iomem, 0, > > - > memory_region_size(&s->scu->iomem)); > > + memory_region_init_alias(&a->scu_alias, OBJECT(a), "scu.alias", > > + &a->scu->parent_obj.iomem, 0, > > a->scu->parent_obj.iomem seems wrong > > > + > > + memory_region_size(&a->scu->parent_obj.iomem)); > > memory_region_add_subregion(s->memory, > sc->memmap[ASPEED_DEV_SCU], > > - &s->scu_alias); > > + &a->scu_alias); > > > > /* INTC */ > > if (!sysbus_realize(SYS_BUS_DEVICE(&a->intc[0]), errp)) { @@ > > -257,6 +257,11 @@ static void aspeed_soc_ast27x0ssp_realize(DeviceState > *dev_soc, Error **errp) > > > sc->memmap[ASPEED_DEV_SCUIO], 0x1000); > > } > > > > +static const Property aspeed_27x0_coprocessor_properties[] = { > > + DEFINE_PROP_LINK("scu", Aspeed27x0CoprocessorState, scu, > > + TYPE_ASPEED_2700_SCU, Aspeed2700SCUState > *), }; > > + > > static void aspeed_soc_ast27x0ssp_class_init(ObjectClass *klass, > > const void *data) > > { > > @@ -270,6 +275,7 @@ static void > aspeed_soc_ast27x0ssp_class_init(ObjectClass *klass, > > /* Reason: The Aspeed Coprocessor can only be instantiated from a > board */ > > dc->user_creatable = false; > > dc->realize = aspeed_soc_ast27x0ssp_realize; > > + device_class_set_props(dc, aspeed_27x0_coprocessor_properties); > > > > sc->valid_cpu_types = valid_cpu_types; > > sc->irqmap = aspeed_soc_ast27x0ssp_irqmap; diff --git > > a/hw/arm/aspeed_ast27x0-tsp.c b/hw/arm/aspeed_ast27x0-tsp.c index > > e7c7b74491..674307103e 100644 > > --- a/hw/arm/aspeed_ast27x0-tsp.c > > +++ b/hw/arm/aspeed_ast27x0-tsp.c > > @@ -186,11 +186,11 @@ static void > aspeed_soc_ast27x0tsp_realize(DeviceState *dev_soc, Error **errp) > > &s->sram_alias); > > > > /* SCU */ > > - memory_region_init_alias(&s->scu_alias, OBJECT(s), "scu.alias", > > - &s->scu->iomem, 0, > > - > memory_region_size(&s->scu->iomem)); > > + memory_region_init_alias(&a->scu_alias, OBJECT(a), "scu.alias", > > + &a->scu->parent_obj.iomem, 0, > > + > > + memory_region_size(&a->scu->parent_obj.iomem)); > > memory_region_add_subregion(s->memory, > sc->memmap[ASPEED_DEV_SCU], > > - &s->scu_alias); > > + &a->scu_alias); > > > > /* INTC */ > > if (!sysbus_realize(SYS_BUS_DEVICE(&a->intc[0]), errp)) { @@ > > -257,6 +257,11 @@ static void aspeed_soc_ast27x0tsp_realize(DeviceState > *dev_soc, Error **errp) > > > sc->memmap[ASPEED_DEV_SCUIO], 0x1000); > > } > > > > +static const Property aspeed_27x0_coprocessor_properties[] = { > > + DEFINE_PROP_LINK("scu", Aspeed27x0CoprocessorState, scu, > > + TYPE_ASPEED_2700_SCU, Aspeed2700SCUState > *), > > a TYPE_ASPEED_2700_SCU is still a TYPE_ASPEED_SCU. Could you keep the > property removed below ? > > > +}; > > + > > static void aspeed_soc_ast27x0tsp_class_init(ObjectClass *klass, > > const void *data) > > { > > @@ -270,6 +275,7 @@ static void > aspeed_soc_ast27x0tsp_class_init(ObjectClass *klass, > > /* Reason: The Aspeed Coprocessor can only be instantiated from a > board */ > > dc->user_creatable = false; > > dc->realize = aspeed_soc_ast27x0tsp_realize; > > + device_class_set_props(dc, aspeed_27x0_coprocessor_properties); > > > > sc->valid_cpu_types = valid_cpu_types; > > sc->irqmap = aspeed_soc_ast27x0tsp_irqmap; diff --git > > a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index > > 87dcb82e1b..e84975a9c4 100644 > > --- a/hw/arm/aspeed_ast27x0.c > > +++ b/hw/arm/aspeed_ast27x0.c > > @@ -425,12 +425,12 @@ static void aspeed_soc_ast2700_init(Object *obj) > > > > object_initialize_child(obj, "gic", &a->gic, > > gicv3_class_name()); > > > > - object_initialize_child(obj, "scu", &s->scu, TYPE_ASPEED_2700_SCU); > > - qdev_prop_set_uint32(DEVICE(&s->scu), "silicon-rev", > > + object_initialize_child(obj, "scu", &a->scu, TYPE_ASPEED_2700_SCU); > > + qdev_prop_set_uint32(DEVICE(&a->scu), "silicon-rev", > > sc->silicon_rev); > > - object_property_add_alias(obj, "hw-strap1", OBJECT(&s->scu), > > + object_property_add_alias(obj, "hw-strap1", OBJECT(&a->scu), > > "hw-strap1"); > > - object_property_add_alias(obj, "hw-prot-key", OBJECT(&s->scu), > > + object_property_add_alias(obj, "hw-prot-key", OBJECT(&a->scu), > > "hw-prot-key"); > > > > object_initialize_child(obj, "scuio", &s->scuio, > > TYPE_ASPEED_2700_SCUIO); @@ -794,10 +794,10 @@ static void > aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp) > > > sc->memmap[ASPEED_DEV_VBOOTROM], > > &s->vbootrom); > > > > /* SCU */ > > - if (!sysbus_realize(SYS_BUS_DEVICE(&s->scu), errp)) { > > + if (!sysbus_realize(SYS_BUS_DEVICE(&a->scu), errp)) { > > return; > > } > > - aspeed_mmio_map(s->memory, SYS_BUS_DEVICE(&s->scu), 0, > > + aspeed_mmio_map(s->memory, SYS_BUS_DEVICE(&a->scu), 0, > > sc->memmap[ASPEED_DEV_SCU]); > > > > /* SCU1 */ > > @@ -915,7 +915,7 @@ static void aspeed_soc_ast2700_realize(DeviceState > *dev, Error **errp) > > AspeedWDTClass *awc = > ASPEED_WDT_GET_CLASS(&s->wdt[i]); > > hwaddr wdt_offset = sc->memmap[ASPEED_DEV_WDT] + i * > > awc->iosize; > > > > - object_property_set_link(OBJECT(&s->wdt[i]), "scu", > OBJECT(&s->scu), > > + object_property_set_link(OBJECT(&s->wdt[i]), "scu", > > + OBJECT(&a->scu), > > &error_abort); > > if (!sysbus_realize(SYS_BUS_DEVICE(&s->wdt[i]), errp)) { > > return; > > @@ -1018,7 +1018,7 @@ static void > aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp) > > aspeed_soc_ast2700_get_irq(s, > > ASPEED_DEV_EMMC)); > > > > /* Timer */ > > - object_property_set_link(OBJECT(&s->timerctrl), "scu", > OBJECT(&s->scu), > > + object_property_set_link(OBJECT(&s->timerctrl), "scu", > > + OBJECT(&a->scu), > > &error_abort); > > if (!sysbus_realize(SYS_BUS_DEVICE(&s->timerctrl), errp)) { > > return; > > diff --git a/hw/arm/aspeed_coprocessor_common.c > > b/hw/arm/aspeed_coprocessor_common.c > > index a0a4c73d08..43026d2a55 100644 > > --- a/hw/arm/aspeed_coprocessor_common.c > > +++ b/hw/arm/aspeed_coprocessor_common.c > > @@ -27,8 +27,6 @@ static const Property aspeed_coprocessor_properties[] > = { > > TYPE_MEMORY_REGION, MemoryRegion *), > > DEFINE_PROP_LINK("sram", AspeedCoprocessorState, sram, > TYPE_MEMORY_REGION, > > MemoryRegion *), > > - DEFINE_PROP_LINK("scu", AspeedCoprocessorState, scu, > TYPE_ASPEED_SCU, > > - AspeedSCUState *), > > > > DEFINE_PROP_LINK("uart", AspeedCoprocessorState, uart, > TYPE_SERIAL_MM, > > SerialMM *), > > DEFINE_PROP_INT32("uart-dev", AspeedCoprocessorState, uart_dev, > > 0), diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c index > > e4160356e4..0a2dad4537 100644 > > --- a/hw/misc/aspeed_scu.c > > +++ b/hw/misc/aspeed_scu.c > > @@ -959,6 +959,11 @@ static void aspeed_ast2700_scu_reset(DeviceState > *dev) > > s->regs[AST2700_HW_STRAP1] = s->hw_strap1; > > } > > > > +static void aspeed_2700_scu_realize(DeviceState *dev, Error **errp) { > > + aspeed_scu_realize(dev, errp); > > +} > > + > > static void aspeed_2700_scu_class_init(ObjectClass *klass, const void > *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); @@ -966,6 +971,7 @@ > > 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; > > asc->resets = ast2700_a0_resets; > > asc->calc_hpll = aspeed_2600_scu_calc_hpll; > > asc->get_apb = aspeed_2700_scu_get_apb_freq; @@ -1097,7 > +1103,7 > > @@ static void aspeed_2700_scuio_class_init(ObjectClass *klass, const void > *data) > > static const TypeInfo aspeed_2700_scu_info = { > > .name = TYPE_ASPEED_2700_SCU, > > .parent = TYPE_ASPEED_SCU, > > - .instance_size = sizeof(AspeedSCUState), > > + .instance_size = sizeof(Aspeed2700SCUState), > > .class_init = aspeed_2700_scu_class_init, > > }; > >
