Re: [PATCH v5 24/79] arm/musicpal: use memdev for RAM

[Igor Mammedov]

On Mon, 17 Feb 2020 11:11:29 -0800
Richard Henderson <address@hidden> wrote:

> On 2/17/20 9:33 AM, 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>
> > Reviewed-by: Andrew Jones <address@hidden>
> > ---  
> 
> Reviewed-by: Richard Henderson <address@hidden>
Thanks!

> 
> > @@ -1589,16 +1590,21 @@ static void musicpal_init(MachineState *machine)
> >      int i;
> >      unsigned long flash_size;
> >      DriveInfo *dinfo;
> > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> >      MemoryRegion *address_space_mem = get_system_memory();
> > -    MemoryRegion *ram = g_new(MemoryRegion, 1);
> >      MemoryRegion *sram = g_new(MemoryRegion, 1);
> >  
> > +    /* For now we use a fixed - the original - RAM size */
> > +    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);
> > +    }  
> 
> If for some reason you need to re-spin this series again, and considering my
> comment re arm/imx25_pdk, I think it would be worthwhile to create a common
> helper for this:
> 

This check is temporary, I plan to replace it with a similar
check in generic machine code and clean it up. The reason it
is not done in this series is that generalizing it is not
related to this series. Hence I'd prefer to keep current
approach in this series to avoid touching already reviewed
patches and generalize it later.


> void machine_memory_check_fixed_size(MachineState *machine)
> {
>     MachineClass *mc = MACHINE_GET_CLASS(machine);
> 
>     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);
>     }
> }
> 
> That would keep the language consistent across the boards.
> 
> 
> r~
>