Re: [PATCH v5 17/79] arm/integratorcp: use memdev for RAM

[Philippe Mathieu-Daudé]

On 2/18/20 5:41 PM, Igor Mammedov wrote:
> On Tue, 18 Feb 2020 07:55:14 +0100
> Philippe Mathieu-Daudé <address@hidden> wrote:
>
> > On 2/17/20 6:33 PM, 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.
> > >
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > Reviewed-by: Andrew Jones <address@hidden>
> > > ---
> > > CC: address@hidden
> > > ---
> > >    hw/arm/integratorcp.c | 9 ++++-----
> > >    1 file changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
> > > index 0cd94d9f09..cc845b8534 100644
> > > --- a/hw/arm/integratorcp.c
> > > +++ b/hw/arm/integratorcp.c
> > > @@ -585,7 +585,6 @@ static void integratorcp_init(MachineState *machine)
> > >        Object *cpuobj;
> > >        ARMCPU *cpu;
> > >        MemoryRegion *address_space_mem = get_system_memory();
> > > -    MemoryRegion *ram = g_new(MemoryRegion, 1);
> > >        MemoryRegion *ram_alias = g_new(MemoryRegion, 1);
> > >        qemu_irq pic[32];
> > >        DeviceState *dev, *sic, *icp;
> > > @@ -605,14 +604,13 @@ static void integratorcp_init(MachineState *machine)
> > >    
> > >        cpu = ARM_CPU(cpuobj);
> > >    
> > > -    memory_region_allocate_system_memory(ram, NULL, "integrator.ram",
> > > -                                         ram_size);
> > >        /* ??? On a real system the first 1Mb is mapped as SSRAM or boot flash.  
> > > */
> > >        /* ??? RAM should repeat to fill physical memory space.  */
> > >        /* SDRAM at address zero*/
> > > -    memory_region_add_subregion(address_space_mem, 0, ram);
> > > +    memory_region_add_subregion(address_space_mem, 0, machine->ram);
> > >        /* And again at address 0x80000000 */
> > > -    memory_region_init_alias(ram_alias, NULL, "ram.alias", ram, 0, ram_size);
> > > +    memory_region_init_alias(ram_alias, NULL, "ram.alias", machine->ram,
> > > +                             0, ram_size);
> > >        memory_region_add_subregion(address_space_mem, 0x80000000, ram_alias);
> > >    
> > >        dev = qdev_create(NULL, TYPE_INTEGRATOR_CM);
> > > @@ -660,6 +658,7 @@ static void integratorcp_machine_init(MachineClass *mc)
> > >        mc->init = integratorcp_init;
> > >        mc->ignore_memory_transaction_failures = true;
> > >        mc->default_cpu_type = ARM_CPU_TYPE_NAME("arm926");
> > > +    mc->default_ram_id = "integrator.ram";
> > >    }
> > >    
> > >    DEFINE_MACHINE("integratorcp", integratorcp_machine_init)
> > >    
> >
> > Looking at integratorcm_realize() this machine seems to handle at most
> > 512MiB.
>
> According to
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Cegeadbj.html
>
> it supports "optionally, 16 to 256MB of SDRAM plugged into the DIMM socket"
> so I'm not sure that realize is valid reference here.
> (well I don't know anything about arm boards, but it probably should be
> double checked by maintainer).
>
> PS:
> It should not hold this series (as check wasn't there to begin with),
> I'll post a patch on top to add check once we decide to what limit it
> should be set.

This is certainly not a blocking comment (neither am I waiting for you 
to fix this, I was just making a comment while reviewing the whole). 
Sorry if this was not clear.