Re: [PATCH v5 11/79] arm/collie: use memdev for RAM

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/19/20 2:44 PM, Igor Mammedov wrote:
> On Tue, 18 Feb 2020 18:16:14 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> Hi Igor,
>>
>> 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.
>>>
>>> PS:
>>>    - while at it add check for user supplied RAM size and error
>>>      out if it mismatches board expected value.
>>>    - introduce RAM_ADDR_UFMT to avoid build errors on 32-bit hosts
>>>      when specifying format string for ram_addr_t type
>>
>> Since v3 the 2nd comment is not valid anymore (also in other patches
>> from this series).
> 
> I looks like the last remnant of RAM_ADDR_UFMT
> I'll remove it in v6

Thanks.

> 
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>>> ---
>>> v2:
>>>     * fix format string causing build failure on 32-bit host
>>>       (Philippe Mathieu-Daudé <philmd@redhat.com>)
>>> v3:
>>>     * instead of RAM_ADDR_UFMT adding use size_to_str()
>>>        Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>    hw/arm/collie.c | 17 ++++++++++++-----
>>>    1 file changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/arm/collie.c b/hw/arm/collie.c
>>> index 970a4405cc..024893fc9e 100644
>>> --- a/hw/arm/collie.c
>>> +++ b/hw/arm/collie.c
>>> @@ -10,6 +10,7 @@
>>>     */
>>>    #include "qemu/osdep.h"
>>>    #include "qemu/units.h"
>>> +#include "qemu/cutils.h"
>>>    #include "hw/sysbus.h"
>>>    #include "hw/boards.h"
>>>    #include "strongarm.h"
>>> @@ -20,20 +21,24 @@
>>>    
>>>    static struct arm_boot_info collie_binfo = {
>>>        .loader_start = SA_SDCS0,
>>> -    .ram_size = 0x20000000,
>>
>> This doesn't seem correct, this field is used do_cpu_reset() ->
>> set_kernel_args() (see hw/arm/boot.c).
> 
> Thanks,
> fixed in v6.
> 
> I'll respin series as already several amended patches accumulated by this time.

With the arm_boot_info ram_size unmodified, please add
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>>
>>>    };
>>>    
>>>    static void collie_init(MachineState *machine)
>>>    {
>>>        StrongARMState *s;
>>>        DriveInfo *dinfo;
>>> -    MemoryRegion *sdram = g_new(MemoryRegion, 1);
>>> +    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);
>>> +    }
>>>    
>>>        s = sa1110_init(machine->cpu_type);
>>>    
>>> -    memory_region_allocate_system_memory(sdram, NULL, "strongarm.sdram",
>>> -                                         collie_binfo.ram_size);
>>> -    memory_region_add_subregion(get_system_memory(), SA_SDCS0, sdram);
>>> +    memory_region_add_subregion(get_system_memory(), SA_SDCS0, machine->ram);
>>>    
>>>        dinfo = drive_get(IF_PFLASH, 0, 0);
>>>        pflash_cfi01_register(SA_CS0, "collie.fl1", 0x02000000,
>>> @@ -57,6 +62,8 @@ static void collie_machine_init(MachineClass *mc)
>>>        mc->init = collie_init;
>>>        mc->ignore_memory_transaction_failures = true;
>>>        mc->default_cpu_type = ARM_CPU_TYPE_NAME("sa1110");
>>> +    mc->default_ram_size = 0x20000000;
>>> +    mc->default_ram_id = "strongarm.sdram";
>>>    }
>>>    
>>>    DEFINE_MACHINE("collie", collie_machine_init)
>>>    
>>
>>
>