Hi Cédric
> > >>> 3. Migrate all ASPEED coprocessors (e.g. SSP/TSP) to a common
> > >> AspeedCoprocessorState.
> > >>
> > >> Is 'AspeedCoprocessorState' a new model structure minimizing the
> > >> number of sub controllers ? if so, looks good. Could be merged fairly
> quickly.
> > >>
> > >
> > > Yes, I am planning to use the new AspeedCoprocessorState instead of
> > AspeedSoCState for SSP and TSP.
> > > struct Aspeed27x0SSPSoCState {
> > > AspeedSoCState parent; -------> Change to AspeedCoprocessorState
> > > AspeedINTCState intc[2];
> > > UnimplementedDeviceState ipc[2];
> > > UnimplementedDeviceState scuio;
> > >
> > > ARMv7MState armv7m;
> > > };
> > >
> > > struct Aspeed27x0TSPSoCState {
> > > AspeedSoCState parent; -------> Change to AspeedCoprocessorState
> > > AspeedINTCState intc[2];
> > > UnimplementedDeviceState ipc[2];
> > > UnimplementedDeviceState scuio;
> > >
> > > ARMv7MState armv7m;
> > > };
> > > This change consolidates SSP and TSP under a common coprocessor
> > > model, reducing duplication and aligning them with the new
> > AspeedCoprocessorState abstraction.
> >
> > Aspeed27x0TSPSoCState and Aspeed27x0SSPSoCState look similar. Could
> > they be merged ?
> >
> Thanks for your suggestion.
> Will try it.
> Jamin
> > Thanks,
I am considering making the following APIs common so that both the Coprocessor
and SoC can use them.
The Coprocessor state is AspeedCoprocessor
The general SoC state is AspeedSoC
A. MMIO Mapping
Currently:
void aspeed_mmio_map(AspeedSoCState *s, SysBusDevice *dev, int n, hwaddr addr)
{
memory_region_add_subregion(s->memory, addr,
sysbus_mmio_get_region(dev, n));
}
void aspeed_mmio_map_unimplemented(AspeedSoCState *s, SysBusDevice *dev,
const char *name, hwaddr addr, uint64_t size)
{
qdev_prop_set_string(DEVICE(dev), "name", name);
qdev_prop_set_uint64(DEVICE(dev), "size", size);
sysbus_realize(dev, &error_abort);
memory_region_add_subregion_overlap(s->memory, addr,
sysbus_mmio_get_region(dev, 0), -1000);
}
Proposal:
Replace AspeedSoCState *s with MemoryRegion *memory.
This way, the functions can be reused for both AspeedSoC and AspeedCoprocessor.
void aspeed_mmio_map(MemoryRegion *memory, SysBusDevice *dev, int n, hwaddr
addr);
void aspeed_mmio_map_unimplemented(MemoryRegion *memory, SysBusDevice *dev,
const char *name, hwaddr addr, uint64_t
size);
B. UART Index Helpers
Currently:
static inline int aspeed_uart_first(AspeedSoCClass *sc)
{
return aspeed_uart_index(sc->uarts_base);
}
static inline int aspeed_uart_last(AspeedSoCClass *sc)
{
return aspeed_uart_first(sc) + sc->uarts_num - 1;
}
Proposal:
Make them independent of AspeedSoCClass, so they can be reused:
static inline int aspeed_uart_first(int uarts_base);
static inline int aspeed_uart_last(int uarts_base, int uarts_num);
C. UART Realization
This case looks more complex. Possible approaches:
1. Create a new API specifically for the coprocessor.
2. Add extra parameters to the existing APIs instead of relying on
AspeedSoCState.
For example, the current SoC UART realization is tightly bound to
AspeedSoCState.
Making it generic would require additional parameters (such as the UART array,
base index, memmap, and IRQ handler) so it could be reused by both SoC and
Coprocessor.
Could you please give me any suggestion?
bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp)
{
AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
SerialMM *smm;
for (int i = 0, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) {
smm = &s->uart[i];
/* Chardev property is set by the machine. */
qdev_prop_set_uint8(DEVICE(smm), "regshift", 2);
qdev_prop_set_uint32(DEVICE(smm), "baudbase", 38400);
qdev_set_legacy_instance_id(DEVICE(smm), sc->memmap[uart], 2);
qdev_prop_set_uint8(DEVICE(smm), "endianness", DEVICE_LITTLE_ENDIAN);
if (!sysbus_realize(SYS_BUS_DEVICE(smm), errp)) {
return false;
}
sysbus_connect_irq(SYS_BUS_DEVICE(smm), 0, aspeed_soc_get_irq(s, uart));
aspeed_mmio_map(s, SYS_BUS_DEVICE(smm), 0, sc->memmap[uart]);
}
return true;
}
void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr)
{
AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
int uart_first = aspeed_uart_first(sc);
int uart_index = aspeed_uart_index(dev);
int i = uart_index - uart_first;
g_assert(0 <= i && i < ARRAY_SIZE(s->uart) && i < sc->uarts_num);
qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
}
D. Common APIs in Multiple SoCs
The following APIs are already widely used across AST2400, AST2500, AST2600,
AST2700, and AST1030 SoC realizations:
Are you agree if I create a new aspeed_coprocessor_cpu_type and
aspeed_coprocessor_get_irq for Coprocessor?
Could you please give me any suggestion?
const char *aspeed_soc_cpu_type(AspeedSoCClass *sc)
{
assert(sc->valid_cpu_types);
assert(sc->valid_cpu_types[0]);
assert(!sc->valid_cpu_types[1]);
return sc->valid_cpu_types[0];
}
qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int dev)
{
return ASPEED_SOC_GET_CLASS(s)->get_irq(s, dev);
}
Thanks-Jamin
> >
> > C.
