Hi Cédric,
Thanks for reviewing. I will adjust the code based the feedback.
Best Regards,
Kane
> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Tuesday, November 11, 2025 12:08 AM
> To: Kane Chen <kane_chen@aspeedtech.com>; Peter Maydell
> <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy
> Lee <leetroy@gmail.com>; Jamin Lin <jamin_lin@aspeedtech.com>; Andrew
> Jeffery <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>;
> open list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC
> here <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>
> 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 <kane_chen@aspeedtech.com>
> >
> > Map the SRAM device to AST1700 model
> >
> > Signed-off-by: Kane-Chen-AS <kane_chen@aspeedtech.com>
> > ---
> > 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];