Re: [Qemu-arm] [Qemu devel v5 PATCH 5/5] msf2: Add Emcraft's Smartfusion2 SOM kit.

[Alistair Francis]

On Sun, May 28, 2017 at 10:26 PM, sundeep subbaraya
<address@hidden> wrote:
> Hi Alistair,
>
> On Sat, May 27, 2017 at 5:30 AM, Alistair Francis <address@hidden>
> wrote:
>>
>> On Tue, May 16, 2017 at 8:38 AM, Subbaraya Sundeep
>> <address@hidden> wrote:
>> > Emulated Emcraft's Smartfusion2 System On Module starter
>> > kit.
>> >
>> > Signed-off-by: Subbaraya Sundeep <address@hidden>
>> > ---
>> >  hw/arm/Makefile.objs |  1 +
>> >  hw/arm/msf2-som.c    | 89
>> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 90 insertions(+)
>> >  create mode 100644 hw/arm/msf2-som.c
>> >
>> > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> > index c828061..4b02093 100644
>> > --- a/hw/arm/Makefile.objs
>> > +++ b/hw/arm/Makefile.objs
>> > @@ -5,6 +5,7 @@ obj-y += omap_sx1.o palm.o realview.o spitz.o
>> > stellaris.o
>> >  obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
>> >  obj-$(CONFIG_ACPI) += virt-acpi-build.o
>> >  obj-y += netduino2.o
>> > +obj-y += msf2-som.o
>>
>> This should be obj-$(CONFIG_MSF2).
>
>
> Ok will change it.
>>
>>
>> >  obj-y += sysbus-fdt.o
>> >
>> >  obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
>> > diff --git a/hw/arm/msf2-som.c b/hw/arm/msf2-som.c
>> > new file mode 100644
>> > index 0000000..cd2b759
>> > --- /dev/null
>> > +++ b/hw/arm/msf2-som.c
>> > @@ -0,0 +1,89 @@
>> > +/*
>> > + * SmartFusion2 SOM starter kit(from Emcraft) emulation.
>> > + *
>> > + * Copyright (c) 2017 Subbaraya Sundeep <address@hidden>
>> > + *
>> > + * Permission is hereby granted, free of charge, to any person
>> > obtaining a copy
>> > + * of this software and associated documentation files (the
>> > "Software"), to deal
>> > + * in the Software without restriction, including without limitation
>> > the rights
>> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
>> > sell
>> > + * copies of the Software, and to permit persons to whom the Software
>> > is
>> > + * furnished to do so, subject to the following conditions:
>> > + *
>> > + * The above copyright notice and this permission notice shall be
>> > included in
>> > + * all copies or substantial portions of the Software.
>> > + *
>> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> > EXPRESS OR
>> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> > MERCHANTABILITY,
>> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
>> > SHALL
>> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> > OTHER
>> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> > ARISING FROM,
>> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> > DEALINGS IN
>> > + * THE SOFTWARE.
>> > + */
>> > +
>> > +#include "qemu/osdep.h"
>> > +#include "qapi/error.h"
>> > +#include "hw/boards.h"
>> > +#include "hw/arm/msf2-soc.h"
>> > +#include "hw/arm/arm.h"
>> > +#include "exec/address-spaces.h"
>> > +
>> > +#define DDR_BASE_ADDRESS      0xA0000000
>> > +#define DDR_SIZE              (64 * M_BYTE)
>> > +
>> > +static void emcraft_sf2_init(MachineState *machine)
>> > +{
>> > +    DeviceState *dev;
>> > +    DeviceState *spi_flash;
>> > +    MSF2State *soc;
>> > +    DriveInfo *dinfo = drive_get_next(IF_MTD);
>> > +    qemu_irq cs_line;
>> > +    SSIBus *spi_bus;
>> > +    MemoryRegion *sysmem = get_system_memory();
>> > +    MemoryRegion *ddr = g_new(MemoryRegion, 1);
>> > +
>> > +    memory_region_init_ram(ddr, NULL, "ddr-ram", DDR_SIZE,
>> > +                           &error_fatal);
>> > +    vmstate_register_ram_global(ddr);
>> > +    memory_region_add_subregion(sysmem, DDR_BASE_ADDRESS, ddr);
>>
>> The user can use -m to specify the amount of RAM to create in the
>> machine. Unless this board only ever includes 64MB of RAM you should
>> use that option (you will need to sanity check it though). If the
>> board only ever has 64MB it might be worth printing a warning to the
>> user if they specify an something. Although there might be a default
>> if they don't use -m, which makes it hard to print out a warning
>> message.
>
>
> This -m confuses me. Why is it necessary for an embedded board? RAM chip
> is fixed and not extendable. Whereas normal PC may have extra RAM slots.
> If another board has more RAM then we would instantiate another machine for
> it
> with that RAM size. Please explain. Maybe I am thinking in wrong direction.

I agree with you, it doesn't make sense for every board. For some
embedded boards it does make sense (ZynqMP can have a customisable
amount of memory) but for most it doesn't make too much sense.

In saying that it is a commonly used QEMU option, if you can find a
way to report a warning if the user tries to specify a custom amount
of memory I think that would be beneficial as QEMU will just ignore
their input. I have a feeling that the ram_size variable will be set
even if the user doesn't specify anything, which we don't want to
report a warning on.

Thanks,
Alistair

>
> Thanks,
> Sundeep