Re: [PATCH REPOST v3 20/80] arm/mps2-tz: use memdev for RAM

[Andrew Jones]

On Thu, Jan 23, 2020 at 02:39:48PM +0100, Igor Mammedov wrote:
> On Thu, 23 Jan 2020 13:59:09 +0100
> Andrew Jones <address@hidden> wrote:
> 
> > On Thu, Jan 23, 2020 at 12:37:45PM +0100, Igor Mammedov wrote:
> > > memory_region_allocate_system_memory() API is going away, so
> > > replace it with memdev allocated MemoryRegion. The later is
> > > initialized by generic code, so board only needs to opt in
> > > to memdev scheme by providing
> > >   MachineClass::default_ram_id
> > > and using MachineState::ram instead of manually initializing
> > > RAM memory region.
> > > 
> > > PS:
> > >  while at it add check for user supplied RAM size and error
> > >  out if it mismatches board expected value.
> > > 
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > > v2:
> > >   * fix format string causing build failure on 32-bit host
> > >     (Philippe Mathieu-Daudé <address@hidden>)
> > > 
> > > CC: address@hidden
> > > CC: address@hidden
> > > CC: address@hidden
> > > ---
> > >  hw/arm/mps2-tz.c | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
> > > index f8b620b..06dacf6 100644
> > > --- a/hw/arm/mps2-tz.c
> > > +++ b/hw/arm/mps2-tz.c
> > > @@ -39,6 +39,7 @@
> > >  
> > >  #include "qemu/osdep.h"
> > >  #include "qemu/units.h"
> > > +#include "qemu/cutils.h"
> > >  #include "qapi/error.h"
> > >  #include "qemu/error-report.h"
> > >  #include "hw/arm/boot.h"
> > > @@ -79,7 +80,6 @@ typedef struct {
> > >      MachineState parent;
> > >  
> > >      ARMSSE iotkit;
> > > -    MemoryRegion psram;
> > >      MemoryRegion ssram[3];
> > >      MemoryRegion ssram1_m;
> > >      MPS2SCC scc;
> > > @@ -388,6 +388,13 @@ static void mps2tz_common_init(MachineState *machine)
> > >          exit(1);
> > >      }
> > >  
> > > +    if (machine->ram_size != mc->default_ram_size) {
> > > +        char *sz = size_to_str(mc->default_ram_size);
> > > +        error_report("Invalid RAM size, should be %s", sz);
> > > +        g_free(sz);
> > > +        exit(EXIT_FAILURE);
> > > +    }
> > > +
> > >      sysbus_init_child_obj(OBJECT(machine), "iotkit", &mms->iotkit,
> > >                            sizeof(mms->iotkit), mmc->armsse_type);
> > >      iotkitdev = DEVICE(&mms->iotkit);
> > > @@ -458,9 +465,7 @@ static void mps2tz_common_init(MachineState *machine)
> > >       * tradeoffs. For QEMU they're all just RAM, though. We arbitrarily
> > >       * call the 16MB our "system memory", as it's the largest lump.
> > >       */
> > > -    memory_region_allocate_system_memory(&mms->psram,
> > > -                                         NULL, "mps.ram", 16 * MiB);
> > > -    memory_region_add_subregion(system_memory, 0x80000000, &mms->psram);
> > > +    memory_region_add_subregion(system_memory, 0x80000000, machine->ram);
> > >  
> > >      /* The overflow IRQs for all UARTs are ORed together.
> > >       * Tx, Rx and "combined" IRQs are sent to the NVIC separately.
> > > @@ -642,6 +647,7 @@ static void mps2tz_class_init(ObjectClass *oc, void 
> > > *data)
> > >  
> > >      mc->init = mps2tz_common_init;
> > >      iic->check = mps2_tz_idau_check;
> > > +    mc->default_ram_id = "mps.ram";
> > >  }
> > >  
> > >  static void mps2tz_an505_class_init(ObjectClass *oc, void *data)
> > > @@ -657,6 +663,7 @@ static void mps2tz_an505_class_init(ObjectClass *oc, 
> > > void *data)
> > >      mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m33");
> > >      mmc->scc_id = 0x41045050;
> > >      mmc->armsse_type = TYPE_IOTKIT;
> > > +    mc->default_ram_size = 16 * MiB;  
> > 
> > Shouldn't this line be added to mps2tz_class_init ?
> 
> Yep, it should be there

With that change you can add my r-b.

Thanks,
drew