Re: [Qemu-devel] [PATCH v9 5/6] arm: SoC model for Calxeda Highbank

[Mitsyanko Igor <i.mitsyanko@samsung.com>]

On 01/12/2012 05:09 PM, Andreas Färber wrote:
> Am 12.01.2012 13:47, schrieb Mitsyanko Igor:
>> On 01/11/2012 08:31 PM, Mark Langsdorf wrote:
>>> +    sysram = g_new(MemoryRegion, 1);
>>> +    memory_region_init_ram(sysram, "highbank.sysram", 0x8000);
>>> +    memory_region_add_subregion(sysmem, 0xfff88000, sysram);
>>> +    if (bios_name != NULL) {
>>> +        sysboot_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS,
>>> bios_name);
>>> +        if (sysboot_filename != NULL) {
>>> +            uint32_t filesize = get_image_size(sysboot_filename);
>>> +            if (load_image_targphys("sysram.bin", 0xfff88000,
>>> filesize)<   0) {
>>> +                hw_error("Unable to load %s\n", bios_name);
>>> +            }
>>
>> Probably should be
>> if (load_image_targphys(sysboot_filename, 0xfff88000, 0x8000)<   0) {
>> and then you don't need "uint32_t filesize" at all.
>
> You need it either way; if you use 0x8000 there, you need to check if
> filesize is actually 0x8000. Doing it this way allows to load smaller
> files; a check for larger files should be added though. Thanks for
> making me aware.
>

Why do we need to check if filesize is 0x8000? load_image_targphys() 
will call get_image_size() and check that size is not more then 0x8000 
automatically, so it would operate on any file with size<=0x8000 and 
return error if size>0x8000, just like we need it to. Well, OK, I know 
load_image_targphys() is currently broken and doesn't use max_sz 
argument, but recently I saw a patch in mailing list which fixes this.

>>> +    dev = qdev_create(NULL, "l2x0");
>>> +    qdev_init_nofail(dev);
>>> +    busdev = sysbus_from_qdev(dev);
>>> +    sysbus_mmio_map(busdev, 0, 0xfff12000);
>>
>>> +    dev = qdev_create(NULL, "highbank-regs");
>>> +    qdev_init_nofail(dev);
>>> +    busdev = sysbus_from_qdev(dev);
>>> +    sysbus_mmio_map(busdev, 0, 0xfff3c000);
>>> +
>>
>> You can use sysbus_create_simple() here (of course, if you didn't avoid
>> it intentionally for some reason).
>
> Depends on how you read this:
>
> /* Legacy helper function for creating devices.  */
> DeviceState *sysbus_create_varargs(const char *name,
>                                   target_phys_addr_t addr, ...);
> DeviceState *sysbus_try_create_varargs(const char *name,
>                                         target_phys_addr_t addr, ...);
> static inline DeviceState *sysbus_create_simple(const char *name,
>                                                target_phys_addr_t addr,
>                                                qemu_irq irq)
> {
>      return sysbus_create_varargs(name, addr, irq, NULL);
> }
>
> I interpret it as sysbus_create_simple() using deprecated
> sysbus_create_varargs() and therefore being deprecated, too.
>
> Andreas
>

Sorry, never paid attention that these functions are deprecated.
-- 
Mitsyanko Igor
ASWG, Moscow R&D center, Samsung Electronics
email: i.mitsyanko@samsung.com