Hi Cédric, Thanks for reviewing. I will adjust the code based the feedback.
Best Regards, Kane > -----Original Message----- > From: Cédric Le Goater <[email protected]> > Sent: Tuesday, November 11, 2025 12:08 AM > To: Kane Chen <[email protected]>; Peter Maydell > <[email protected]>; Steven Lee <[email protected]>; Troy > Lee <[email protected]>; Jamin Lin <[email protected]>; Andrew > Jeffery <[email protected]>; Joel Stanley <[email protected]>; > open list:ASPEED BMCs <[email protected]>; open list:All patches CC > here <[email protected]> > Cc: Troy Lee <[email protected]> > Subject: Re: [PATCH v2 08/17] hw/arm/aspeed: Attach SRAM device to > AST1700 model > > On 11/5/25 04:58, Kane Chen wrote: > > From: Kane-Chen-AS <[email protected]> > > > > Map the SRAM device to AST1700 model > > > > Signed-off-by: Kane-Chen-AS <[email protected]> > > --- > > include/hw/misc/aspeed_ast1700.h | 1 + > > hw/misc/aspeed_ast1700.c | 25 > +++++++++++++++++++++++++ > > 2 files changed, 26 insertions(+) > > > > diff --git a/include/hw/misc/aspeed_ast1700.h > > b/include/hw/misc/aspeed_ast1700.h > > index e105ceb027..391c8687f5 100644 > > --- a/include/hw/misc/aspeed_ast1700.h > > +++ b/include/hw/misc/aspeed_ast1700.h > > @@ -32,6 +32,7 @@ struct AspeedAST1700SoCState { > > > > AspeedLTPIState ltpi; > > SerialMM uart; > > + MemoryRegion sram; > > }; > > > > #endif /* ASPEED_AST1700_H */ > > diff --git a/hw/misc/aspeed_ast1700.c b/hw/misc/aspeed_ast1700.c index > > 1c2d367cdb..6f7ff625b5 100644 > > --- a/hw/misc/aspeed_ast1700.c > > +++ b/hw/misc/aspeed_ast1700.c > > @@ -15,14 +15,18 @@ > > #include "migration/vmstate.h" > > #include "hw/misc/aspeed_ast1700.h" > > > > +#define AST1700_BOARD1_MEM_ADDR 0x30000000 > > #define AST2700_SOC_LTPI_SIZE 0x01000000 > > +#define AST1700_SOC_SRAM_SIZE 0x00040000 > > > > enum { > > + ASPEED_AST1700_DEV_SRAM, > > ASPEED_AST1700_DEV_UART12, > > ASPEED_AST1700_DEV_LTPI_CTRL, > > }; > > > > static const hwaddr aspeed_ast1700_io_memmap[] = { > > + [ASPEED_AST1700_DEV_SRAM] = 0x00BC0000, > > [ASPEED_AST1700_DEV_UART12] = 0x00C33B00, > > [ASPEED_AST1700_DEV_LTPI_CTRL] = 0x00C34000, > > }; > > @@ -31,12 +35,33 @@ static void aspeed_ast1700_realize(DeviceState > *dev, Error **errp) > > AspeedAST1700SoCState *s = ASPEED_AST1700(dev); > > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > > hwaddr uart_base; > > + Error *err = NULL; > > This variable could be avoided. > > > + int board_idx; > > + char sram_name[32]; > > + > > + if (s->mapped_base == AST1700_BOARD1_MEM_ADDR) { > > + board_idx = 0; > > + } else { > > + board_idx = 1; > > + } > > That's a bit hacky. > > An "index" property set at the SoC level would avoid this weak heuristic. > > > > > > /* Occupy memory space for all controllers in AST1700 */ > > memory_region_init(&s->iomem, OBJECT(s), > TYPE_ASPEED_AST1700, > > AST2700_SOC_LTPI_SIZE); > > sysbus_init_mmio(sbd, &s->iomem); > > > > + /* SRAM */ > > + snprintf(sram_name, sizeof(sram_name), "aspeed.ioexp-sram.%d", > board_idx); > > + memory_region_init_ram(&s->sram, OBJECT(s), sram_name, > > + AST1700_SOC_SRAM_SIZE, &err); > > Just pass 'errp' and test the return value of memory_region_init_ram() > > > Thanks, > > C. > > > > > + if (err != NULL) { > > + error_propagate(errp, err); > > + return; > > + } > > + memory_region_add_subregion(&s->iomem, > > + > aspeed_ast1700_io_memmap[ASPEED_AST1700_DEV_SRAM], > > + &s->sram); > > + > > /* UART */ > > uart_base = s->mapped_base + > > > aspeed_ast1700_io_memmap[ASPEED_AST1700_DEV_UART12];
