In the previous design, the SSP coprocessor (aspeed27x0ssp-soc) was initialized
and realized at the machine level (e.g., AST2700FC). However, to make sure the
coprocessors can work together properly—such as using the same SRAM, sharing
the SCU, and having consistent memory remapping—we need to change how these
devices are set up.
This commit moves the SSP coprocessor initialization and realization into the
AST2700 SoC (aspeed_soc_ast2700_init() and aspeed_soc_ast2700_realize()).
By doing so, the SSP becomes a proper child of the SoC device, rather than
the machine.
This is a preparation step for future commits that will support shared SCU,
SRAM, and memory remap logic—specifically enabling PSP DRAM remap for SSP SDRAM
access.
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
include/hw/arm/aspeed_soc.h | 27 +++++++++++++-----------
hw/arm/aspeed_ast27x0-fc.c | 30 ++------------------------
hw/arm/aspeed_ast27x0.c | 42 +++++++++++++++++++++++++++++++++++++
3 files changed, 59 insertions(+), 40 deletions(-)
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 217ef0eafd..2831da91ab 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -128,6 +128,19 @@ struct Aspeed2600SoCState {
#define TYPE_ASPEED2600_SOC "aspeed2600-soc"
OBJECT_DECLARE_SIMPLE_TYPE(Aspeed2600SoCState, ASPEED2600_SOC)
+struct Aspeed27x0SSPSoCState {
+ AspeedSoCState parent;
+ AspeedINTCState intc[2];
+ UnimplementedDeviceState ipc[2];
+ UnimplementedDeviceState scuio;
+ MemoryRegion memory;
+
+ ARMv7MState armv7m;
+};
+
+#define TYPE_ASPEED27X0SSP_SOC "aspeed27x0ssp-soc"
+OBJECT_DECLARE_SIMPLE_TYPE(Aspeed27x0SSPSoCState, ASPEED27X0SSP_SOC)
+
struct Aspeed27x0SoCState {
AspeedSoCState parent;
@@ -135,6 +148,8 @@ struct Aspeed27x0SoCState {
AspeedINTCState intc[2];
GICv3State gic;
MemoryRegion dram_empty;
+
+ Aspeed27x0SSPSoCState ssp;
};
#define TYPE_ASPEED27X0_SOC "aspeed27x0-soc"
@@ -146,18 +161,6 @@ struct Aspeed10x0SoCState {
ARMv7MState armv7m;
};
-struct Aspeed27x0SSPSoCState {
- AspeedSoCState parent;
- AspeedINTCState intc[2];
- UnimplementedDeviceState ipc[2];
- UnimplementedDeviceState scuio;
-
- ARMv7MState armv7m;
-};
-
-#define TYPE_ASPEED27X0SSP_SOC "aspeed27x0ssp-soc"
-OBJECT_DECLARE_SIMPLE_TYPE(Aspeed27x0SSPSoCState, ASPEED27X0SSP_SOC)
-
struct Aspeed27x0TSPSoCState {
AspeedSoCState parent;
AspeedINTCState intc[2];
diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
index e2eee6183f..c9b338fe78 100644
--- a/hw/arm/aspeed_ast27x0-fc.c
+++ b/hw/arm/aspeed_ast27x0-fc.c
@@ -37,14 +37,11 @@ struct Ast2700FCState {
MemoryRegion ca35_memory;
MemoryRegion ca35_dram;
MemoryRegion ca35_boot_rom;
- MemoryRegion ssp_memory;
MemoryRegion tsp_memory;
- Clock *ssp_sysclk;
Clock *tsp_sysclk;
Aspeed27x0SoCState ca35;
- Aspeed27x0SSPSoCState ssp;
Aspeed27x0TSPSoCState tsp;
bool mmio_exec;
@@ -158,6 +155,8 @@ static void ast2700fc_ca35_init(MachineState *machine)
return;
}
aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART12, serial_hd(0));
+ aspeed_soc_uart_set_chr(ASPEED_SOC(&s->ca35.ssp), ASPEED_DEV_UART4,
+ serial_hd(1));
if (!qdev_realize(DEVICE(&s->ca35), NULL, &error_abort)) {
return;
}
@@ -196,30 +195,6 @@ static void ast2700fc_ca35_init(MachineState *machine)
arm_load_kernel(ARM_CPU(first_cpu), machine, &ast2700fc_board_info);
}
-static void ast2700fc_ssp_init(MachineState *machine)
-{
- AspeedSoCState *soc;
- Ast2700FCState *s = AST2700A1FC(machine);
- s->ssp_sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
- clock_set_hz(s->ssp_sysclk, 200000000ULL);
-
- object_initialize_child(OBJECT(s), "ssp", &s->ssp, TYPE_ASPEED27X0SSP_SOC);
- memory_region_init(&s->ssp_memory, OBJECT(&s->ssp), "ssp-memory",
- UINT64_MAX);
-
- qdev_connect_clock_in(DEVICE(&s->ssp), "sysclk", s->ssp_sysclk);
- if (!object_property_set_link(OBJECT(&s->ssp), "memory",
- OBJECT(&s->ssp_memory), &error_abort)) {
- return;
- }
-
- soc = ASPEED_SOC(&s->ssp);
- aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART4, serial_hd(1));
- if (!qdev_realize(DEVICE(&s->ssp), NULL, &error_abort)) {
- return;
- }
-}
-
static void ast2700fc_tsp_init(MachineState *machine)
{
AspeedSoCState *soc;
@@ -247,7 +222,6 @@ static void ast2700fc_tsp_init(MachineState *machine)
static void ast2700fc_init(MachineState *machine)
{
ast2700fc_ca35_init(machine);
- ast2700fc_ssp_init(machine);
ast2700fc_tsp_init(machine);
}
diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 6aa3841b69..ffbc32fef2 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -22,6 +22,8 @@
#include "hw/intc/arm_gicv3.h"
#include "qobject/qlist.h"
#include "qemu/log.h"
+#include "hw/qdev-clock.h"
+#include "hw/boards.h"
#define AST2700_SOC_IO_SIZE 0x00FE0000
#define AST2700_SOC_IOMEM_SIZE 0x01000000
@@ -410,6 +412,8 @@ static bool aspeed_soc_ast2700_dram_init(DeviceState *dev, Error **errp)
static void aspeed_soc_ast2700_init(Object *obj)
{
+ MachineState *ms = MACHINE(qdev_get_machine());
+ MachineClass *mc = MACHINE_GET_CLASS(ms);
Aspeed27x0SoCState *a = ASPEED27X0_SOC(obj);
AspeedSoCState *s = ASPEED_SOC(obj);
AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
@@ -426,6 +430,11 @@ static void aspeed_soc_ast2700_init(Object *obj)
aspeed_soc_cpu_type(sc));
}
+ /* Coprocessors */
+ if (mc->default_cpus > sc->num_cpus) {
+ object_initialize_child(obj, "ssp", &a->ssp, TYPE_ASPEED27X0SSP_SOC);
+ }
+
object_initialize_child(obj, "gic", &a->gic, gicv3_class_name());
object_initialize_child(obj, "scu", &s->scu, TYPE_ASPEED_2700_SCU);
@@ -610,9 +619,35 @@ static bool aspeed_soc_ast2700_gic_realize(DeviceState *dev, Error **errp)
return true;
}
+static bool aspeed_soc_ast2700_ssp_realize(DeviceState *dev, Error **errp)
+{
+ Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
+ AspeedSoCState *s = ASPEED_SOC(dev);
+ Clock *sysclk;
+
+ sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
+ clock_set_hz(sysclk, 200000000ULL);
+ qdev_connect_clock_in(DEVICE(&a->ssp), "sysclk", sysclk);
+
+ memory_region_init(&a->ssp.memory, OBJECT(&a->ssp), "ssp-memory",
+ UINT64_MAX);
+ if (!object_property_set_link(OBJECT(&a->ssp), "memory",
+ OBJECT(&a->ssp.memory), &error_abort)) {
+ return false;
+ }
+
+ if (!qdev_realize(DEVICE(&a->ssp), NULL, &error_abort)) {
+ return false;
+ }
+
+ return true;
+}
+
static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
{
int i;
+ MachineState *ms = MACHINE(qdev_get_machine());
+ MachineClass *mc = MACHINE_GET_CLASS(ms);
Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
AspeedSoCState *s = ASPEED_SOC(dev);
AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
@@ -719,6 +754,13 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->scuio), 0,
sc->memmap[ASPEED_DEV_SCUIO]);
+ /* Coprocessors */
+ if (mc->default_cpus > sc->num_cpus) {
+ if (!aspeed_soc_ast2700_ssp_realize(dev, errp)) {
+ return;
+ }
+ }
+
/* UART */
if (!aspeed_soc_uart_realize(s, errp)) {
return;
--
2.43.0
On 7/17/25 05:40, Jamin Lin wrote:
> In the previous design, the SSP coprocessor (aspeed27x0ssp-soc) was initialized
> and realized at the machine level (e.g., AST2700FC). However, to make sure the
> coprocessors can work together properly—such as using the same SRAM, sharing
> the SCU, and having consistent memory remapping—we need to change how these
> devices are set up.
>
> This commit moves the SSP coprocessor initialization and realization into the
> AST2700 SoC (aspeed_soc_ast2700_init() and aspeed_soc_ast2700_realize()).
> By doing so, the SSP becomes a proper child of the SoC device, rather than
> the machine.
>
> This is a preparation step for future commits that will support shared SCU,
> SRAM, and memory remap logic—specifically enabling PSP DRAM remap for SSP SDRAM
> access.
>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
> include/hw/arm/aspeed_soc.h | 27 +++++++++++++-----------
> hw/arm/aspeed_ast27x0-fc.c | 30 ++------------------------
> hw/arm/aspeed_ast27x0.c | 42 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 59 insertions(+), 40 deletions(-)
>
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 217ef0eafd..2831da91ab 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -128,6 +128,19 @@ struct Aspeed2600SoCState {
> #define TYPE_ASPEED2600_SOC "aspeed2600-soc"
> OBJECT_DECLARE_SIMPLE_TYPE(Aspeed2600SoCState, ASPEED2600_SOC)
>
> +struct Aspeed27x0SSPSoCState {
> + AspeedSoCState parent;
> + AspeedINTCState intc[2];
> + UnimplementedDeviceState ipc[2];
> + UnimplementedDeviceState scuio;
> + MemoryRegion memory;
> +
> + ARMv7MState armv7m;
> +};
> +
> +#define TYPE_ASPEED27X0SSP_SOC "aspeed27x0ssp-soc"
> +OBJECT_DECLARE_SIMPLE_TYPE(Aspeed27x0SSPSoCState, ASPEED27X0SSP_SOC)
> +
> struct Aspeed27x0SoCState {
> AspeedSoCState parent;
>
> @@ -135,6 +148,8 @@ struct Aspeed27x0SoCState {
> AspeedINTCState intc[2];
> GICv3State gic;
> MemoryRegion dram_empty;
> +
> + Aspeed27x0SSPSoCState ssp;
> };
>
> #define TYPE_ASPEED27X0_SOC "aspeed27x0-soc"
> @@ -146,18 +161,6 @@ struct Aspeed10x0SoCState {
> ARMv7MState armv7m;
> };
>
> -struct Aspeed27x0SSPSoCState {
> - AspeedSoCState parent;
> - AspeedINTCState intc[2];
> - UnimplementedDeviceState ipc[2];
> - UnimplementedDeviceState scuio;
> -
> - ARMv7MState armv7m;
> -};
> -
> -#define TYPE_ASPEED27X0SSP_SOC "aspeed27x0ssp-soc"
> -OBJECT_DECLARE_SIMPLE_TYPE(Aspeed27x0SSPSoCState, ASPEED27X0SSP_SOC)
> -
> struct Aspeed27x0TSPSoCState {
> AspeedSoCState parent;
> AspeedINTCState intc[2];
> diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
> index e2eee6183f..c9b338fe78 100644
> --- a/hw/arm/aspeed_ast27x0-fc.c
> +++ b/hw/arm/aspeed_ast27x0-fc.c
> @@ -37,14 +37,11 @@ struct Ast2700FCState {
> MemoryRegion ca35_memory;
> MemoryRegion ca35_dram;
> MemoryRegion ca35_boot_rom;
> - MemoryRegion ssp_memory;
> MemoryRegion tsp_memory;
>
> - Clock *ssp_sysclk;
> Clock *tsp_sysclk;
>
> Aspeed27x0SoCState ca35;
> - Aspeed27x0SSPSoCState ssp;
> Aspeed27x0TSPSoCState tsp;
>
> bool mmio_exec;
> @@ -158,6 +155,8 @@ static void ast2700fc_ca35_init(MachineState *machine)
> return;
> }
> aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART12, serial_hd(0));
> + aspeed_soc_uart_set_chr(ASPEED_SOC(&s->ca35.ssp), ASPEED_DEV_UART4,
> + serial_hd(1));
hmm, I wonder if the second uart shouldn't be set from the init handler
of the Aspeed27x0SSPSoCState child object.
> if (!qdev_realize(DEVICE(&s->ca35), NULL, &error_abort)) {
> return;
> }
> @@ -196,30 +195,6 @@ static void ast2700fc_ca35_init(MachineState *machine)
> arm_load_kernel(ARM_CPU(first_cpu), machine, &ast2700fc_board_info);
> }
>
> -static void ast2700fc_ssp_init(MachineState *machine)
> -{
> - AspeedSoCState *soc;
> - Ast2700FCState *s = AST2700A1FC(machine);
> - s->ssp_sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
> - clock_set_hz(s->ssp_sysclk, 200000000ULL);
> -
> - object_initialize_child(OBJECT(s), "ssp", &s->ssp, TYPE_ASPEED27X0SSP_SOC);
> - memory_region_init(&s->ssp_memory, OBJECT(&s->ssp), "ssp-memory",
> - UINT64_MAX);
> -
> - qdev_connect_clock_in(DEVICE(&s->ssp), "sysclk", s->ssp_sysclk);
> - if (!object_property_set_link(OBJECT(&s->ssp), "memory",
> - OBJECT(&s->ssp_memory), &error_abort)) {
> - return;
> - }
> -
> - soc = ASPEED_SOC(&s->ssp);
> - aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART4, serial_hd(1));
> - if (!qdev_realize(DEVICE(&s->ssp), NULL, &error_abort)) {
> - return;
> - }
> -}
> -
> static void ast2700fc_tsp_init(MachineState *machine)
> {
> AspeedSoCState *soc;
> @@ -247,7 +222,6 @@ static void ast2700fc_tsp_init(MachineState *machine)
> static void ast2700fc_init(MachineState *machine)
> {
> ast2700fc_ca35_init(machine);
> - ast2700fc_ssp_init(machine);
> ast2700fc_tsp_init(machine);
> }
>
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index 6aa3841b69..ffbc32fef2 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -22,6 +22,8 @@
> #include "hw/intc/arm_gicv3.h"
> #include "qobject/qlist.h"
> #include "qemu/log.h"
> +#include "hw/qdev-clock.h"
> +#include "hw/boards.h"
>
> #define AST2700_SOC_IO_SIZE 0x00FE0000
> #define AST2700_SOC_IOMEM_SIZE 0x01000000
> @@ -410,6 +412,8 @@ static bool aspeed_soc_ast2700_dram_init(DeviceState *dev, Error **errp)
>
> static void aspeed_soc_ast2700_init(Object *obj)
> {
> + MachineState *ms = MACHINE(qdev_get_machine());
Calling qdev_get_machine() in a device model is a no-no. Please don't.
> + MachineClass *mc = MACHINE_GET_CLASS(ms);
> Aspeed27x0SoCState *a = ASPEED27X0_SOC(obj);
> AspeedSoCState *s = ASPEED_SOC(obj);
> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> @@ -426,6 +430,11 @@ static void aspeed_soc_ast2700_init(Object *obj)
> aspeed_soc_cpu_type(sc));
> }
>
> + /* Coprocessors */
> + if (mc->default_cpus > sc->num_cpus) {
That's a hack.
We need to find another way to conditionally create the co-processors
if that's what you want to do. A SoC class attribute would be a better
way.
> + object_initialize_child(obj, "ssp", &a->ssp, TYPE_ASPEED27X0SSP_SOC);
> + }
> +
> object_initialize_child(obj, "gic", &a->gic, gicv3_class_name());
>
> object_initialize_child(obj, "scu", &s->scu, TYPE_ASPEED_2700_SCU);
> @@ -610,9 +619,35 @@ static bool aspeed_soc_ast2700_gic_realize(DeviceState *dev, Error **errp)
> return true;
> }
>
> +static bool aspeed_soc_ast2700_ssp_realize(DeviceState *dev, Error **errp)
I would pass 'Aspeed27x0SoCState *' instead.
> +{
> + Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
> + AspeedSoCState *s = ASPEED_SOC(dev);
> + Clock *sysclk;
> +
> + sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
> + clock_set_hz(sysclk, 200000000ULL);
> + qdev_connect_clock_in(DEVICE(&a->ssp), "sysclk", sysclk);
> +
> + memory_region_init(&a->ssp.memory, OBJECT(&a->ssp), "ssp-memory",
> + UINT64_MAX);
> + if (!object_property_set_link(OBJECT(&a->ssp), "memory",
> + OBJECT(&a->ssp.memory), &error_abort)) {
please use errp instead.
> + return false;
> + }
> +
> + if (!qdev_realize(DEVICE(&a->ssp), NULL, &error_abort)) {
same here.
Thanks,
C.
> + return false;
> + }
> +
> + return true;
> +}
> +
> static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
> {
> int i;
> + MachineState *ms = MACHINE(qdev_get_machine());
> + MachineClass *mc = MACHINE_GET_CLASS(ms);
> Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
> AspeedSoCState *s = ASPEED_SOC(dev);
> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> @@ -719,6 +754,13 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
> aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->scuio), 0,
> sc->memmap[ASPEED_DEV_SCUIO]);
>
> + /* Coprocessors */
> + if (mc->default_cpus > sc->num_cpus) {
> + if (!aspeed_soc_ast2700_ssp_realize(dev, errp)) {
> + return;
> + }
> + }
> +
> /* UART */
> if (!aspeed_soc_uart_realize(s, errp)) {
> return;
Hi Cédric
> Subject: Re: [SPAM] [PATCH v1 02/21] hw/arm/ast27x0: Move SSP coprocessor
> initialization from machine to SoC leve
>
> On 7/17/25 05:40, Jamin Lin wrote:
> > In the previous design, the SSP coprocessor (aspeed27x0ssp-soc) was
> > initialized and realized at the machine level (e.g., AST2700FC).
> > However, to make sure the coprocessors can work together properly—such
> > as using the same SRAM, sharing the SCU, and having consistent memory
> > remapping—we need to change how these devices are set up.
> >
> > This commit moves the SSP coprocessor initialization and realization
> > into the
> > AST2700 SoC (aspeed_soc_ast2700_init() and
> aspeed_soc_ast2700_realize()).
> > By doing so, the SSP becomes a proper child of the SoC device, rather
> > than the machine.
> >
> > This is a preparation step for future commits that will support shared
> > SCU, SRAM, and memory remap logic—specifically enabling PSP DRAM
> remap
> > for SSP SDRAM access.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> > include/hw/arm/aspeed_soc.h | 27 +++++++++++++-----------
> > hw/arm/aspeed_ast27x0-fc.c | 30 ++------------------------
> > hw/arm/aspeed_ast27x0.c | 42
> +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 59 insertions(+), 40 deletions(-)
> >
> > diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> > index 217ef0eafd..2831da91ab 100644
> > --- a/include/hw/arm/aspeed_soc.h
> > +++ b/include/hw/arm/aspeed_soc.h
> > @@ -128,6 +128,19 @@ struct Aspeed2600SoCState {
> > #define TYPE_ASPEED2600_SOC "aspeed2600-soc"
> > OBJECT_DECLARE_SIMPLE_TYPE(Aspeed2600SoCState,
> ASPEED2600_SOC)
> >
> > +struct Aspeed27x0SSPSoCState {
> > + AspeedSoCState parent;
> > + AspeedINTCState intc[2];
> > + UnimplementedDeviceState ipc[2];
> > + UnimplementedDeviceState scuio;
> > + MemoryRegion memory;
> > +
> > + ARMv7MState armv7m;
> > +};
> > +
> > +#define TYPE_ASPEED27X0SSP_SOC "aspeed27x0ssp-soc"
> > +OBJECT_DECLARE_SIMPLE_TYPE(Aspeed27x0SSPSoCState,
> ASPEED27X0SSP_SOC)
> > +
> > struct Aspeed27x0SoCState {
> > AspeedSoCState parent;
> >
> > @@ -135,6 +148,8 @@ struct Aspeed27x0SoCState {
> > AspeedINTCState intc[2];
> > GICv3State gic;
> > MemoryRegion dram_empty;
> > +
> > + Aspeed27x0SSPSoCState ssp;
> > };
> >
> > #define TYPE_ASPEED27X0_SOC "aspeed27x0-soc"
> > @@ -146,18 +161,6 @@ struct Aspeed10x0SoCState {
> > ARMv7MState armv7m;
> > };
> >
> > -struct Aspeed27x0SSPSoCState {
> > - AspeedSoCState parent;
> > - AspeedINTCState intc[2];
> > - UnimplementedDeviceState ipc[2];
> > - UnimplementedDeviceState scuio;
> > -
> > - ARMv7MState armv7m;
> > -};
> > -
> > -#define TYPE_ASPEED27X0SSP_SOC "aspeed27x0ssp-soc"
> > -OBJECT_DECLARE_SIMPLE_TYPE(Aspeed27x0SSPSoCState,
> ASPEED27X0SSP_SOC)
> > -
> > struct Aspeed27x0TSPSoCState {
> > AspeedSoCState parent;
> > AspeedINTCState intc[2];
> > diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
> > index e2eee6183f..c9b338fe78 100644
> > --- a/hw/arm/aspeed_ast27x0-fc.c
> > +++ b/hw/arm/aspeed_ast27x0-fc.c
> > @@ -37,14 +37,11 @@ struct Ast2700FCState {
> > MemoryRegion ca35_memory;
> > MemoryRegion ca35_dram;
> > MemoryRegion ca35_boot_rom;
> > - MemoryRegion ssp_memory;
> > MemoryRegion tsp_memory;
> >
> > - Clock *ssp_sysclk;
> > Clock *tsp_sysclk;
> >
> > Aspeed27x0SoCState ca35;
> > - Aspeed27x0SSPSoCState ssp;
> > Aspeed27x0TSPSoCState tsp;
> >
> > bool mmio_exec;
> > @@ -158,6 +155,8 @@ static void ast2700fc_ca35_init(MachineState
> *machine)
> > return;
> > }
> > aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART12, serial_hd(0));
> > + aspeed_soc_uart_set_chr(ASPEED_SOC(&s->ca35.ssp),
> ASPEED_DEV_UART4,
> > + serial_hd(1));
>
> hmm, I wonder if the second uart shouldn't be set from the init handler of the
> Aspeed27x0SSPSoCState child object.
Will try it.
>
>
> > if (!qdev_realize(DEVICE(&s->ca35), NULL, &error_abort)) {
> > return;
> > }
> > @@ -196,30 +195,6 @@ static void ast2700fc_ca35_init(MachineState
> *machine)
> > arm_load_kernel(ARM_CPU(first_cpu), machine,
> &ast2700fc_board_info);
> > }
> >
> > -static void ast2700fc_ssp_init(MachineState *machine) -{
> > - AspeedSoCState *soc;
> > - Ast2700FCState *s = AST2700A1FC(machine);
> > - s->ssp_sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
> > - clock_set_hz(s->ssp_sysclk, 200000000ULL);
> > -
> > - object_initialize_child(OBJECT(s), "ssp", &s->ssp,
> TYPE_ASPEED27X0SSP_SOC);
> > - memory_region_init(&s->ssp_memory, OBJECT(&s->ssp),
> "ssp-memory",
> > - UINT64_MAX);
> > -
> > - qdev_connect_clock_in(DEVICE(&s->ssp), "sysclk", s->ssp_sysclk);
> > - if (!object_property_set_link(OBJECT(&s->ssp), "memory",
> > - OBJECT(&s->ssp_memory),
> &error_abort)) {
> > - return;
> > - }
> > -
> > - soc = ASPEED_SOC(&s->ssp);
> > - aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART4, serial_hd(1));
> > - if (!qdev_realize(DEVICE(&s->ssp), NULL, &error_abort)) {
> > - return;
> > - }
> > -}
> > -
> > static void ast2700fc_tsp_init(MachineState *machine)
> > {
> > AspeedSoCState *soc;
> > @@ -247,7 +222,6 @@ static void ast2700fc_tsp_init(MachineState
> *machine)
> > static void ast2700fc_init(MachineState *machine)
> > {
> > ast2700fc_ca35_init(machine);
> > - ast2700fc_ssp_init(machine);
> > ast2700fc_tsp_init(machine);
> > }
> >
> > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> > 6aa3841b69..ffbc32fef2 100644
> > --- a/hw/arm/aspeed_ast27x0.c
> > +++ b/hw/arm/aspeed_ast27x0.c
> > @@ -22,6 +22,8 @@
> > #include "hw/intc/arm_gicv3.h"
> > #include "qobject/qlist.h"
> > #include "qemu/log.h"
> > +#include "hw/qdev-clock.h"
> > +#include "hw/boards.h"
> >
> > #define AST2700_SOC_IO_SIZE 0x00FE0000
> > #define AST2700_SOC_IOMEM_SIZE 0x01000000
> > @@ -410,6 +412,8 @@ static bool
> > aspeed_soc_ast2700_dram_init(DeviceState *dev, Error **errp)
> >
> > static void aspeed_soc_ast2700_init(Object *obj)
> > {
> > + MachineState *ms = MACHINE(qdev_get_machine());
>
> Calling qdev_get_machine() in a device model is a no-no. Please don't.
Will remove it.
>
> > + MachineClass *mc = MACHINE_GET_CLASS(ms);
> > Aspeed27x0SoCState *a = ASPEED27X0_SOC(obj);
> > AspeedSoCState *s = ASPEED_SOC(obj);
> > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); @@ -426,6
> +430,11
> > @@ static void aspeed_soc_ast2700_init(Object *obj)
> > aspeed_soc_cpu_type(sc));
> > }
> >
> > + /* Coprocessors */
> > + if (mc->default_cpus > sc->num_cpus) {
>
> That's a hack.
>
> We need to find another way to conditionally create the co-processors if that's
> what you want to do. A SoC class attribute would be a better way.
>
If I understanding your suggestion correctly, I should create new SOC variant(ex: ast2700-fc) and set a class attribute in its class_init.
Then, the SOC's init/realize will check the coprocessor features and initialize SSP/TSP objects accordingly.
If yes, I will add a new class attribute(ex: sc->coprocessor =2)
> > + object_initialize_child(obj, "ssp", &a->ssp,
> TYPE_ASPEED27X0SSP_SOC);
> > + }
> > +
> > object_initialize_child(obj, "gic", &a->gic,
> > gicv3_class_name());
> >
> > object_initialize_child(obj, "scu", &s->scu,
> > TYPE_ASPEED_2700_SCU); @@ -610,9 +619,35 @@ static bool
> aspeed_soc_ast2700_gic_realize(DeviceState *dev, Error **errp)
> > return true;
> > }
> >
> > +static bool aspeed_soc_ast2700_ssp_realize(DeviceState *dev, Error
> > +**errp)
>
> I would pass 'Aspeed27x0SoCState *' instead.
>
Will do
> > +{
> > + Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
> > + AspeedSoCState *s = ASPEED_SOC(dev);
> > + Clock *sysclk;
> > +
> > + sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
> > + clock_set_hz(sysclk, 200000000ULL);
> > + qdev_connect_clock_in(DEVICE(&a->ssp), "sysclk", sysclk);
> > +
> > + memory_region_init(&a->ssp.memory, OBJECT(&a->ssp),
> "ssp-memory",
> > + UINT64_MAX);
> > + if (!object_property_set_link(OBJECT(&a->ssp), "memory",
> > + OBJECT(&a->ssp.memory),
> > + &error_abort)) {
>
> please use errp instead.
>
Will do
>
> > + return false;
> > + }
> > +
> > + if (!qdev_realize(DEVICE(&a->ssp), NULL, &error_abort)) {
>
> same here.
>
Will do
Thanks for review and suggestions.
Jamin
>
> Thanks,
>
> C.
>
>
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
> > {
> > int i;
> > + MachineState *ms = MACHINE(qdev_get_machine());
> > + MachineClass *mc = MACHINE_GET_CLASS(ms);
> > Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
> > AspeedSoCState *s = ASPEED_SOC(dev);
> > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); @@ -719,6
> +754,13
> > @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
> > aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->scuio), 0,
> > sc->memmap[ASPEED_DEV_SCUIO]);
> >
> > + /* Coprocessors */
> > + if (mc->default_cpus > sc->num_cpus) {
> > + if (!aspeed_soc_ast2700_ssp_realize(dev, errp)) {
> > + return;
> > + }
> > + }
> > +
> > /* UART */
> > if (!aspeed_soc_uart_realize(s, errp)) {
> > return;
On 9/2/25 10:41, Jamin Lin wrote:
> Hi Cédric
>
>> Subject: Re: [SPAM] [PATCH v1 02/21] hw/arm/ast27x0: Move SSP coprocessor
>> initialization from machine to SoC leve
>>
>> On 7/17/25 05:40, Jamin Lin wrote:
>>> In the previous design, the SSP coprocessor (aspeed27x0ssp-soc) was
>>> initialized and realized at the machine level (e.g., AST2700FC).
>>> However, to make sure the coprocessors can work together properly—such
>>> as using the same SRAM, sharing the SCU, and having consistent memory
>>> remapping—we need to change how these devices are set up.
>>>
>>> This commit moves the SSP coprocessor initialization and realization
>>> into the
>>> AST2700 SoC (aspeed_soc_ast2700_init() and
>> aspeed_soc_ast2700_realize()).
>>> By doing so, the SSP becomes a proper child of the SoC device, rather
>>> than the machine.
>>>
>>> This is a preparation step for future commits that will support shared
>>> SCU, SRAM, and memory remap logic—specifically enabling PSP DRAM
>> remap
>>> for SSP SDRAM access.
>>>
>>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>>> ---
>>> include/hw/arm/aspeed_soc.h | 27 +++++++++++++-----------
>>> hw/arm/aspeed_ast27x0-fc.c | 30 ++------------------------
>>> hw/arm/aspeed_ast27x0.c | 42
>> +++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 59 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
>>> index 217ef0eafd..2831da91ab 100644
>>> --- a/include/hw/arm/aspeed_soc.h
>>> +++ b/include/hw/arm/aspeed_soc.h
>>> @@ -128,6 +128,19 @@ struct Aspeed2600SoCState {
>>> #define TYPE_ASPEED2600_SOC "aspeed2600-soc"
>>> OBJECT_DECLARE_SIMPLE_TYPE(Aspeed2600SoCState,
>> ASPEED2600_SOC)
>>>
>>> +struct Aspeed27x0SSPSoCState {
>>> + AspeedSoCState parent;
>>> + AspeedINTCState intc[2];
>>> + UnimplementedDeviceState ipc[2];
>>> + UnimplementedDeviceState scuio;
>>> + MemoryRegion memory;
>>> +
>>> + ARMv7MState armv7m;
>>> +};
>>> +
>>> +#define TYPE_ASPEED27X0SSP_SOC "aspeed27x0ssp-soc"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(Aspeed27x0SSPSoCState,
>> ASPEED27X0SSP_SOC)
>>> +
>>> struct Aspeed27x0SoCState {
>>> AspeedSoCState parent;
>>>
>>> @@ -135,6 +148,8 @@ struct Aspeed27x0SoCState {
>>> AspeedINTCState intc[2];
>>> GICv3State gic;
>>> MemoryRegion dram_empty;
>>> +
>>> + Aspeed27x0SSPSoCState ssp;
>>> };
>>>
>>> #define TYPE_ASPEED27X0_SOC "aspeed27x0-soc"
>>> @@ -146,18 +161,6 @@ struct Aspeed10x0SoCState {
>>> ARMv7MState armv7m;
>>> };
>>>
>>> -struct Aspeed27x0SSPSoCState {
>>> - AspeedSoCState parent;
>>> - AspeedINTCState intc[2];
>>> - UnimplementedDeviceState ipc[2];
>>> - UnimplementedDeviceState scuio;
>>> -
>>> - ARMv7MState armv7m;
>>> -};
>>> -
>>> -#define TYPE_ASPEED27X0SSP_SOC "aspeed27x0ssp-soc"
>>> -OBJECT_DECLARE_SIMPLE_TYPE(Aspeed27x0SSPSoCState,
>> ASPEED27X0SSP_SOC)
>>> -
>>> struct Aspeed27x0TSPSoCState {
>>> AspeedSoCState parent;
>>> AspeedINTCState intc[2];
>>> diff --git a/hw/arm/aspeed_ast27x0-fc.c b/hw/arm/aspeed_ast27x0-fc.c
>>> index e2eee6183f..c9b338fe78 100644
>>> --- a/hw/arm/aspeed_ast27x0-fc.c
>>> +++ b/hw/arm/aspeed_ast27x0-fc.c
>>> @@ -37,14 +37,11 @@ struct Ast2700FCState {
>>> MemoryRegion ca35_memory;
>>> MemoryRegion ca35_dram;
>>> MemoryRegion ca35_boot_rom;
>>> - MemoryRegion ssp_memory;
>>> MemoryRegion tsp_memory;
>>>
>>> - Clock *ssp_sysclk;
>>> Clock *tsp_sysclk;
>>>
>>> Aspeed27x0SoCState ca35;
>>> - Aspeed27x0SSPSoCState ssp;
>>> Aspeed27x0TSPSoCState tsp;
>>>
>>> bool mmio_exec;
>>> @@ -158,6 +155,8 @@ static void ast2700fc_ca35_init(MachineState
>> *machine)
>>> return;
>>> }
>>> aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART12, serial_hd(0));
>>> + aspeed_soc_uart_set_chr(ASPEED_SOC(&s->ca35.ssp),
>> ASPEED_DEV_UART4,
>>> + serial_hd(1));
>>
>> hmm, I wonder if the second uart shouldn't be set from the init handler of the
>> Aspeed27x0SSPSoCState child object.
>
> Will try it.
>
>>
>>
>>> if (!qdev_realize(DEVICE(&s->ca35), NULL, &error_abort)) {
>>> return;
>>> }
>>> @@ -196,30 +195,6 @@ static void ast2700fc_ca35_init(MachineState
>> *machine)
>>> arm_load_kernel(ARM_CPU(first_cpu), machine,
>> &ast2700fc_board_info);
>>> }
>>>
>>> -static void ast2700fc_ssp_init(MachineState *machine) -{
>>> - AspeedSoCState *soc;
>>> - Ast2700FCState *s = AST2700A1FC(machine);
>>> - s->ssp_sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
>>> - clock_set_hz(s->ssp_sysclk, 200000000ULL);
>>> -
>>> - object_initialize_child(OBJECT(s), "ssp", &s->ssp,
>> TYPE_ASPEED27X0SSP_SOC);
>>> - memory_region_init(&s->ssp_memory, OBJECT(&s->ssp),
>> "ssp-memory",
>>> - UINT64_MAX);
>>> -
>>> - qdev_connect_clock_in(DEVICE(&s->ssp), "sysclk", s->ssp_sysclk);
>>> - if (!object_property_set_link(OBJECT(&s->ssp), "memory",
>>> - OBJECT(&s->ssp_memory),
>> &error_abort)) {
>>> - return;
>>> - }
>>> -
>>> - soc = ASPEED_SOC(&s->ssp);
>>> - aspeed_soc_uart_set_chr(soc, ASPEED_DEV_UART4, serial_hd(1));
>>> - if (!qdev_realize(DEVICE(&s->ssp), NULL, &error_abort)) {
>>> - return;
>>> - }
>>> -}
>>> -
>>> static void ast2700fc_tsp_init(MachineState *machine)
>>> {
>>> AspeedSoCState *soc;
>>> @@ -247,7 +222,6 @@ static void ast2700fc_tsp_init(MachineState
>> *machine)
>>> static void ast2700fc_init(MachineState *machine)
>>> {
>>> ast2700fc_ca35_init(machine);
>>> - ast2700fc_ssp_init(machine);
>>> ast2700fc_tsp_init(machine);
>>> }
>>>
>>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
>>> 6aa3841b69..ffbc32fef2 100644
>>> --- a/hw/arm/aspeed_ast27x0.c
>>> +++ b/hw/arm/aspeed_ast27x0.c
>>> @@ -22,6 +22,8 @@
>>> #include "hw/intc/arm_gicv3.h"
>>> #include "qobject/qlist.h"
>>> #include "qemu/log.h"
>>> +#include "hw/qdev-clock.h"
>>> +#include "hw/boards.h"
>>>
>>> #define AST2700_SOC_IO_SIZE 0x00FE0000
>>> #define AST2700_SOC_IOMEM_SIZE 0x01000000
>>> @@ -410,6 +412,8 @@ static bool
>>> aspeed_soc_ast2700_dram_init(DeviceState *dev, Error **errp)
>>>
>>> static void aspeed_soc_ast2700_init(Object *obj)
>>> {
>>> + MachineState *ms = MACHINE(qdev_get_machine());
>>
>> Calling qdev_get_machine() in a device model is a no-no. Please don't.
>
> Will remove it.
>
>>
>>> + MachineClass *mc = MACHINE_GET_CLASS(ms);
>>> Aspeed27x0SoCState *a = ASPEED27X0_SOC(obj);
>>> AspeedSoCState *s = ASPEED_SOC(obj);
>>> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); @@ -426,6
>> +430,11
>>> @@ static void aspeed_soc_ast2700_init(Object *obj)
>>> aspeed_soc_cpu_type(sc));
>>> }
>>>
>>> + /* Coprocessors */
>>> + if (mc->default_cpus > sc->num_cpus) {
>>
>> That's a hack.
>>
>> We need to find another way to conditionally create the co-processors if that's
>> what you want to do. A SoC class attribute would be a better way.
>>
>
> If I understanding your suggestion correctly, I should create new SOC variant(ex: ast2700-fc) and set a class attribute in its class_init.
Yes. Something like that.
Thanks,
C.
> Then, the SOC's init/realize will check the coprocessor features and initialize SSP/TSP objects accordingly.
> If yes, I will add a new class attribute(ex: sc->coprocessor =2)
>
>>> + object_initialize_child(obj, "ssp", &a->ssp,
>> TYPE_ASPEED27X0SSP_SOC);
>>> + }
>>> +
>>> object_initialize_child(obj, "gic", &a->gic,
>>> gicv3_class_name());
>>>
>>> object_initialize_child(obj, "scu", &s->scu,
>>> TYPE_ASPEED_2700_SCU); @@ -610,9 +619,35 @@ static bool
>> aspeed_soc_ast2700_gic_realize(DeviceState *dev, Error **errp)
>>> return true;
>>> }
>>>
>>> +static bool aspeed_soc_ast2700_ssp_realize(DeviceState *dev, Error
>>> +**errp)
>>
>> I would pass 'Aspeed27x0SoCState *' instead.
>>
>
>
> Will do
>
>>> +{
>>> + Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
>>> + AspeedSoCState *s = ASPEED_SOC(dev);
>>> + Clock *sysclk;
>>> +
>>> + sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
>>> + clock_set_hz(sysclk, 200000000ULL);
>>> + qdev_connect_clock_in(DEVICE(&a->ssp), "sysclk", sysclk);
>>> +
>>> + memory_region_init(&a->ssp.memory, OBJECT(&a->ssp),
>> "ssp-memory",
>>> + UINT64_MAX);
>>> + if (!object_property_set_link(OBJECT(&a->ssp), "memory",
>>> + OBJECT(&a->ssp.memory),
>>> + &error_abort)) {
>>
>> please use errp instead.
>>
> Will do
>>
>>> + return false;
>>> + }
>>> +
>>> + if (!qdev_realize(DEVICE(&a->ssp), NULL, &error_abort)) {
>>
>> same here.
>>
> Will do
>
> Thanks for review and suggestions.
> Jamin
>>
>> Thanks,
>>
>> C.
>>
>>
>>> + return false;
>>> + }
>>> +
>>> + return true;
>>> +}
>>> +
>>> static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
>>> {
>>> int i;
>>> + MachineState *ms = MACHINE(qdev_get_machine());
>>> + MachineClass *mc = MACHINE_GET_CLASS(ms);
>>> Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
>>> AspeedSoCState *s = ASPEED_SOC(dev);
>>> AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); @@ -719,6
>> +754,13
>>> @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
>>> aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->scuio), 0,
>>> sc->memmap[ASPEED_DEV_SCUIO]);
>>>
>>> + /* Coprocessors */
>>> + if (mc->default_cpus > sc->num_cpus) {
>>> + if (!aspeed_soc_ast2700_ssp_realize(dev, errp)) {
>>> + return;
>>> + }
>>> + }
>>> +
>>> /* UART */
>>> if (!aspeed_soc_uart_realize(s, errp)) {
>>> return;
>
Cédric Le Goater <clg@kaod.org> writes:
> On 7/17/25 05:40, Jamin Lin wrote:
>> In the previous design, the SSP coprocessor (aspeed27x0ssp-soc) was initialized
>> and realized at the machine level (e.g., AST2700FC). However, to make sure the
>> coprocessors can work together properly—such as using the same SRAM, sharing
>> the SCU, and having consistent memory remapping—we need to change how these
>> devices are set up.
>>
>> This commit moves the SSP coprocessor initialization and realization into the
>> AST2700 SoC (aspeed_soc_ast2700_init() and aspeed_soc_ast2700_realize()).
>> By doing so, the SSP becomes a proper child of the SoC device, rather than
>> the machine.
>>
>> This is a preparation step for future commits that will support shared SCU,
>> SRAM, and memory remap logic—specifically enabling PSP DRAM remap for SSP SDRAM
>> access.
>>
>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
[...]
>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
>> index 6aa3841b69..ffbc32fef2 100644
>> --- a/hw/arm/aspeed_ast27x0.c
>> +++ b/hw/arm/aspeed_ast27x0.c
[...]
>> @@ -610,9 +619,35 @@ static bool aspeed_soc_ast2700_gic_realize(DeviceState *dev, Error **errp)
>> return true;
>> }
>> +static bool aspeed_soc_ast2700_ssp_realize(DeviceState *dev, Error **errp)
>
> I would pass 'Aspeed27x0SoCState *' instead.
>
>> +{
>> + Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
>> + AspeedSoCState *s = ASPEED_SOC(dev);
>> + Clock *sysclk;
>> +
>> + sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
>> + clock_set_hz(sysclk, 200000000ULL);
>> + qdev_connect_clock_in(DEVICE(&a->ssp), "sysclk", sysclk);
>> +
>> + memory_region_init(&a->ssp.memory, OBJECT(&a->ssp), "ssp-memory",
>> + UINT64_MAX);
>> + if (!object_property_set_link(OBJECT(&a->ssp), "memory",
>> + OBJECT(&a->ssp.memory), &error_abort)) {
>
> please use errp instead.
>
>> + return false;
>> + }
object_property_set_link() can return false only when it fails, and it
sets an error when it fails. Since you pass &error_abort, it cannot
fail (it aborts instead). Therefore the return value is always true,
and the return statement is dead code.
If object_property_set_link() is not expected to fail, i.e. failure
would be a programming error, use
object_property_set_link(..., &error_abort);
If failure is not a programming error, passing &error_abort is wrong,
and you need to pass errp instead.
>> +
>> + if (!qdev_realize(DEVICE(&a->ssp), NULL, &error_abort)) {
>
> same here.
Same argument.
[...]
Hi Markus,
> Subject: Re: [SPAM] [PATCH v1 02/21] hw/arm/ast27x0: Move SSP coprocessor
> initialization from machine to SoC leve
>
> Cédric Le Goater <clg@kaod.org> writes:
>
> > On 7/17/25 05:40, Jamin Lin wrote:
> >> In the previous design, the SSP coprocessor (aspeed27x0ssp-soc) was
> >> initialized and realized at the machine level (e.g., AST2700FC).
> >> However, to make sure the coprocessors can work together
> >> properly—such as using the same SRAM, sharing the SCU, and having
> >> consistent memory remapping—we need to change how these devices are
> set up.
> >>
> >> This commit moves the SSP coprocessor initialization and realization
> >> into the
> >> AST2700 SoC (aspeed_soc_ast2700_init() and
> aspeed_soc_ast2700_realize()).
> >> By doing so, the SSP becomes a proper child of the SoC device, rather
> >> than the machine.
> >>
> >> This is a preparation step for future commits that will support
> >> shared SCU, SRAM, and memory remap logic—specifically enabling PSP
> >> DRAM remap for SSP SDRAM access.
> >>
> >> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>
> [...]
>
> >> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> >> 6aa3841b69..ffbc32fef2 100644
> >> --- a/hw/arm/aspeed_ast27x0.c
> >> +++ b/hw/arm/aspeed_ast27x0.c
>
> [...]
>
> >> @@ -610,9 +619,35 @@ static bool
> aspeed_soc_ast2700_gic_realize(DeviceState *dev, Error **errp)
> >> return true;
> >> }
> >> +static bool aspeed_soc_ast2700_ssp_realize(DeviceState *dev, Error
> >> **errp)
> >
> > I would pass 'Aspeed27x0SoCState *' instead.
> >
> >> +{
> >> + Aspeed27x0SoCState *a = ASPEED27X0_SOC(dev);
> >> + AspeedSoCState *s = ASPEED_SOC(dev);
> >> + Clock *sysclk;
> >> +
> >> + sysclk = clock_new(OBJECT(s), "SSP_SYSCLK");
> >> + clock_set_hz(sysclk, 200000000ULL);
> >> + qdev_connect_clock_in(DEVICE(&a->ssp), "sysclk", sysclk);
> >> +
> >> + memory_region_init(&a->ssp.memory, OBJECT(&a->ssp),
> "ssp-memory",
> >> + UINT64_MAX);
> >> + if (!object_property_set_link(OBJECT(&a->ssp), "memory",
> >> + OBJECT(&a->ssp.memory),
> >> + &error_abort)) {
> >
> > please use errp instead.
> >
> >> + return false;
> >> + }
>
> object_property_set_link() can return false only when it fails, and it sets an
> error when it fails. Since you pass &error_abort, it cannot fail (it aborts
> instead). Therefore the return value is always true, and the return statement
> is dead code.
>
> If object_property_set_link() is not expected to fail, i.e. failure would be a
> programming error, use
>
> object_property_set_link(..., &error_abort);
>
> If failure is not a programming error, passing &error_abort is wrong, and you
> need to pass errp instead.
>
> >> +
> >> + if (!qdev_realize(DEVICE(&a->ssp), NULL, &error_abort)) {
> >
> > same here.
>
Thanks for the detailed explanation - that clarifies the intent and the
difference between using "error_abort" and "errp".
Appreciate your help.
Thanks again Jamin
> Same argument.
>
> [...]
To: Jamin Lin <jamin_lin@aspeedtech.com>
Subject: Re: [SPAM] [PATCH v1 02/21] hw/arm/ast27x0: Move SSP coprocessor initialization from machine to SoC leve
Gcc: nnml:mail.redhat.xlst.qemu-devel
From: Markus Armbruster <armbru@redhat.com>
--text follows this line--
Jamin Lin <jamin_lin@aspeedtech.com> writes:
> Hi Markus,
[...]
>> object_property_set_link() can return false only when it fails, and it sets an
>> error when it fails. Since you pass &error_abort, it cannot fail (it aborts
>> instead). Therefore the return value is always true, and the return statement
>> is dead code.
>>
>> If object_property_set_link() is not expected to fail, i.e. failure would be a
>> programming error, use
>>
>> object_property_set_link(..., &error_abort);
>>
>> If failure is not a programming error, passing &error_abort is wrong, and you
>> need to pass errp instead.
>>
>> >> +
>> >> + if (!qdev_realize(DEVICE(&a->ssp), NULL, &error_abort)) {
>> >
>> > same here.
>>
>
> Thanks for the detailed explanation - that clarifies the intent and the
> difference between using "error_abort" and "errp".
> Appreciate your help.
> Thanks again Jamin
You're welcome!
© 2016 - 2025 Red Hat, Inc.