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

[Peter Delevoryas]

> On Jun 23, 2022, at 8:39 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> 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.

Ah yes, I noticed that this was odd too, and that the DRAM probably
should have been mapped inside the SoC code. I didn’t know exactly
how to solve it easily though. Thanks for sending a patch Cedric!!

> 
> Thanks,
> 
> C.
> 
>> thanks
>> -- PMM
>