Re: [PATCH 08/14] aspeed: Replace direct get_system_memory() calls

[Cédric Le Goater]

On 6/23/22 14:57, Peter Maydell wrote:
> On Thu, 23 Jun 2022 at 13:37, Peter Delevoryas <pdel@fb.com> wrote:
> > Note: sysbus_mmio_map(), sysbus_mmio_map_overlap(), and others are still
> > using get_system_memory indirectly.
> >
> > Signed-off-by: Peter Delevoryas <pdel@fb.com>
> > ---
> >   hw/arm/aspeed.c         | 8 ++++----
> >   hw/arm/aspeed_ast10x0.c | 5 ++---
> >   hw/arm/aspeed_ast2600.c | 2 +-
> >   hw/arm/aspeed_soc.c     | 6 +++---
> >   4 files changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index 8dae155183..3aa74e88fb 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -371,7 +371,7 @@ static void aspeed_machine_init(MachineState *machine)
> >                            amc->uart_default);
> >       qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
> >
> > -    memory_region_add_subregion(get_system_memory(),
> > +    memory_region_add_subregion(bmc->soc.system_memory,
> >                                   sc->memmap[ASPEED_DEV_SDRAM],
> >                                   &bmc->ram_container);
>
> This is board code, it shouldn't be reaching into the internals
> of the SoC object like this. The board code probably already
> has the right MemoryRegion because it was the one that passed
> it to the SoC link porperty in the first place.

It's a bit messy currently. May be I got it wrong initially. The
board allocates a ram container region in which the machine ram
region is mapped. See commit ad1a9782186d ("aspeed: add a RAM memory
region container")

There is an extra region after ram in the ram container to catch
invalid access done by FW. That's how FW determines the size of
ram. See commit ebe31c0a8ef7 ("aspeed: add a max_ram_size property
to the memory controller")

But I think I can safely move all the RAM logic under the board.
I will send a patch.

Thanks,

C.

> thanks
> -- PMM