Re: [RESEND PATCH 8/9] hw/riscv: microchip_pfsoc: Correct DDR memory map

[Bin Meng]

Hi Alistair,

On Wed, Oct 28, 2020 at 5:07 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Oct 27, 2020 at 7:48 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > When system memory is larger than 1 GiB (high memory), PolarFire SoC
> > maps it at address 0x10_0000_0000. Address 0xC000_0000 and above is
> > aliased to the same 1 GiB low memory with different cache attributes.
> >
> > At present QEMU maps the system memory contiguously from 0x8000_0000.
> > This corrects the wrong QEMU logic. Note address 0x14_0000_0000 is
> > the alias to the high memory, and even physical memory is only 1 GiB,
> > the HSS codes still tries to probe the high memory alias address.
> > It seems there is no issue on the real hardware, so we will have to
> > take that into the consideration in our emulation.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> >  hw/riscv/microchip_pfsoc.c         | 49 +++++++++++++++++++++++++++---
> >  include/hw/riscv/microchip_pfsoc.h |  5 ++-
> >  2 files changed, 48 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> > index b9c2f73e7c..c595c9c967 100644
> > --- a/hw/riscv/microchip_pfsoc.c
> > +++ b/hw/riscv/microchip_pfsoc.c
> > @@ -102,7 +102,10 @@ static const struct MemmapEntry {
> >      [MICROCHIP_PFSOC_ENVM_CFG] =        { 0x20200000,     0x1000 },
> >      [MICROCHIP_PFSOC_ENVM_DATA] =       { 0x20220000,    0x20000 },
> >      [MICROCHIP_PFSOC_IOSCB] =           { 0x30000000, 0x10000000 },
> > -    [MICROCHIP_PFSOC_DRAM] =            { 0x80000000,        0x0 },
> > +    [MICROCHIP_PFSOC_DRAM_LO] =         { 0x80000000, 0x40000000 },
> > +    [MICROCHIP_PFSOC_DRAM_LO_ALIAS] =   { 0xc0000000, 0x40000000 },
> > +    [MICROCHIP_PFSOC_DRAM_HI] =       { 0x1000000000,        0x0 },
> > +    [MICROCHIP_PFSOC_DRAM_HI_ALIAS] = { 0x1400000000,        0x0 },
> >  };
> >
> >  static void microchip_pfsoc_soc_instance_init(Object *obj)
> > @@ -405,7 +408,11 @@ static void 
> > microchip_icicle_kit_machine_init(MachineState *machine)
> >      const struct MemmapEntry *memmap = microchip_pfsoc_memmap;
> >      MicrochipIcicleKitState *s = MICROCHIP_ICICLE_KIT_MACHINE(machine);
> >      MemoryRegion *system_memory = get_system_memory();
> > -    MemoryRegion *main_mem = g_new(MemoryRegion, 1);
> > +    MemoryRegion *mem_low = g_new(MemoryRegion, 1);
> > +    MemoryRegion *mem_low_alias = g_new(MemoryRegion, 1);
> > +    MemoryRegion *mem_high = g_new(MemoryRegion, 1);
> > +    MemoryRegion *mem_high_alias = g_new(MemoryRegion, 1);
> > +    uint64_t mem_high_size;
> >      DriveInfo *dinfo = drive_get_next(IF_SD);
> >
> >      /* Sanity check on RAM size */
> > @@ -422,10 +429,42 @@ static void 
> > microchip_icicle_kit_machine_init(MachineState *machine)
> >      qdev_realize(DEVICE(&s->soc), NULL, &error_abort);
> >
> >      /* Register RAM */
> > -    memory_region_init_ram(main_mem, NULL, "microchip.icicle.kit.ram",
> > -                           machine->ram_size, &error_fatal);
> > +    memory_region_init_ram(mem_low, NULL, "microchip.icicle.kit.ram_low",
> > +                           memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> > +                           &error_fatal);
> > +    memory_region_init_alias(mem_low_alias, NULL,
> > +                             "microchip.icicle.kit.ram_low.alias",
> > +                             mem_low, 0,
> > +                             memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].size);
> > +    memory_region_add_subregion(system_memory,
> > +                                memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> > +                                mem_low);
> > +    memory_region_add_subregion(system_memory,
> > +                                memmap[MICROCHIP_PFSOC_DRAM_LO_ALIAS].base,
> > +                                mem_low_alias);
> > +
> > +    /*
> > +     * Map 1 GiB high memory because HSS will do memory test against the 
> > high
> > +     * memory address range regardless of physical memory installed.
> > +     *
> > +     * See memory_tests() in mss_ddr.c in the HSS source code.
> > +     */
> > +    mem_high_size = machine->ram_size - 1 * GiB;
> > +    if (mem_high_size < 1 * GiB) {
> > +        mem_high_size = 1 * GiB;
>
> This now means that the machine requires at least 2GB of memory!

Yes, unfortunately :(

>
> Can you increase the default_ram_size so we will return an error if
> the user specified less than 2GB instead of just increasing it without
> their knowledge?
>

Sure.

Regards,
Bin