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

[Mitsyanko Igor]

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: address@hidden