Re: [Qemu-arm] [Qemu-devel] [Qemu-devel PATCH 4/5] msf2: Add Smartfusion2 SoC.

[Philippe Mathieu-Daudé]

On 05/12/2017 12:17 AM, sundeep subbaraya wrote:
> Hi Philippe,
>
> On Wed, May 10, 2017 at 5:20 PM, Philippe Mathieu-Daudé <address@hidden>
> wrote:
>
> > Hi Subbaraya,
> >
> >
> > On 05/09/2017 01:44 PM, Subbaraya Sundeep wrote:
> >
> > > Smartfusion2 SoC has hardened Microcontroller subsystem
> > > and flash based FPGA fabric. This patch adds support for
> > > Microcontroller subsystem in the SoC.
> > >
> > > Signed-off-by: Subbaraya Sundeep <address@hidden>
> > > ---
> > >  default-configs/arm-softmmu.mak |   1 +
> > >  hw/arm/Makefile.objs            |   2 +-
> > >  hw/arm/msf2-soc.c               | 188 ++++++++++++++++++++++++++++++
> > > ++++++++++
> > >  include/hw/arm/msf2-soc.h       |  60 +++++++++++++
> > >  4 files changed, 250 insertions(+), 1 deletion(-)
> > >  create mode 100644 hw/arm/msf2-soc.c
> > >  create mode 100644 include/hw/arm/msf2-soc.h
> > >
> > > diff --git a/default-configs/arm-softmmu.mak
> > > b/default-configs/arm-softmmu.mak
> > > index 78d7af0..7062512 100644
> > > --- a/default-configs/arm-softmmu.mak
> > > +++ b/default-configs/arm-softmmu.mak
> > > @@ -122,3 +122,4 @@ CONFIG_ACPI=y
> > >  CONFIG_SMBIOS=y
> > >  CONFIG_ASPEED_SOC=y
> > >  CONFIG_GPIO_KEY=y
> > > +CONFIG_MSF2=y
> > > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> > > index 4c5c4ee..ae5e4a3 100644
> > > --- a/hw/arm/Makefile.objs
> > > +++ b/hw/arm/Makefile.objs
> > > @@ -1,7 +1,7 @@
> > >  obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
> > >  obj-$(CONFIG_DIGIC) += digic_boards.o
> > >  obj-y += integratorcp.o mainstone.o musicpal.o nseries.o
> > > -obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
> > > +obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o msf2-soc.o
> >
> > Not a big deal, but since you added CONFIG_MSF2 why not using it here and
> > the Makefiles you touched (misc/ssi/timer)?
> >
> > obj-$(CONFIG_MSF2) += msf2-soc.o
> >
> >   OK. Will change it.
>
> >  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
[...]
> > > +    MemoryRegion *system_memory = get_system_memory();
> > > +    MemoryRegion *nvm = g_new(MemoryRegion, 1);
> > > +    MemoryRegion *nvm_alias = g_new(MemoryRegion, 1);
> > > +    MemoryRegion *sram = g_new(MemoryRegion, 1);
> > > +    MemoryRegion *ddr = g_new(MemoryRegion, 1);
> > > +
> > > +    memory_region_init_ram(nvm, NULL, "MSF2.envm", ENVM_SIZE,
> > > +                           &error_fatal);
> >
> > Maybe you can name it "eNVM" to match the documentation.
> >
> > Also envm_size should be a per-model property.
>
> Ok.
>
> > +    memory_region_init_alias(nvm_alias, NULL, "MSF2.flash.alias",
> > > +                             nvm, 0, ENVM_SIZE);
> >
> > Hmmm well this would be the "Cache Matrix Remap" which happens to be
> > mapped by default to eNVM on cold reset.
> > Naming it "MSF2.flash.alias" is pretty confusing.
>
> Exactly it is Cache Matrix Remap.
> AFAIK currently we cannot remap memory during runtime in Qemu.
> So I handled default remap with alias.
> Please suggest the name. MSF2.eNVM.alias sounds fine?

Hmm Peter, Francis?

Personally I prefer "bus_remap.alias" which is explicit.

"eNVM.alias" is only true on Cold Reset.

> > +    vmstate_register_ram_global(nvm);
> > > +
> > > +    memory_region_set_readonly(nvm, true);
> > > +    memory_region_set_readonly(nvm_alias, true);
> > > +
> > > +    memory_region_add_subregion(system_memory, ENVM_BASE_ADDRESS, nvm);
> > > +    memory_region_add_subregion(system_memory, 0, nvm_alias);
> > > +
> > > +    memory_region_init_ram(ddr, NULL, "MSF2.ddr", DDR_SIZE,
> > > +                           &error_fatal);
> >
> > Wrong, there is no DDR on this SoC.
> DDR controller is there in Smartfusion2 (different from Smartfusion). As
> you said below this
> should be in board file.

There IS a DDRC in this SoC, but here you are registering a DDR 'ram' 
memory region, not a controller. This SoC can be used without any DDR, 
enough using embedded eNVM and eSRAM.

Now it happens your SoM board provides a DDR chip connected to this SoC.

> > +    vmstate_register_ram_global(ddr);
> > > +    memory_region_add_subregion(system_memory, DDR_BASE_ADDRESS, ddr);
> > > +
> > > +    memory_region_init_ram(sram, NULL, "MSF2.sram", SRAM_SIZE,
> > > +                           &error_fatal);
> >
> > I'd rather like to see it named "eSRAM" somehow, so there is no confusion
> > possible with external SRAM a SoM/board can map at 0x60000000.
> >
> > Same comment than envm_size, sram_size should be a per-model property.
> >
> > OK
>
> > +    vmstate_register_ram_global(sram);
> > > +    memory_region_add_subregion(system_memory, SRAM_BASE_ADDRESS, sram);
> > > +
> > > +    armv7m = DEVICE(&s->armv7m);
> > > +    qdev_prop_set_uint32(armv7m, "num-irq", 96);
> >
> > Can you point me to your datasheet? I thought the SF2 had 240 IRQs.
>
>
> Please go to link:
> https://www.microsemi.com/document-portal/search_form
> and provide search keyword as "UG0331". You can the download the spec.
> It has 81 irqs I remember when I have given 81 qemu complained not multiple
> of 4.
> I checked again with 81 and it is fine. I will change it to 81.

Ok :)

> +    qdev_prop_set_string(armv7m, "cpu-model", "cortex-m3");
> > > +    object_property_set_link(OBJECT(&s->armv7m),
> > > OBJECT(get_system_memory()),
> > > +                                     "memory", &error_abort);
[...]
> > > +#define MSF2_NUM_SPIS         2
> > > +#define MSF2_NUM_UARTS        2
> > > +
> > > +#define ENVM_BASE_ADDRESS     0x60000000
> > > +#define ENVM_SIZE             (128 * 1024)
> >
> > The SoC design ENVM_SIZE is 1MB, 128K seems your particular model.
>
>  M2S010 SoC device has 256K eNMV. My bad it should be 256 instead of 128.
>  The board I have contains M2S010 device in SOM.

What I mean here is on the SF2 the eNVM "region size" (as seen by the 
AHB Bus) is 1MB. Now your SoC M2S010 provides 256KB in this dedicated 1M 
region.

I find your #define confusing, what about:

#define ENVM_REGION_SIZE      (1 * M_BYTE)
#define M2S010_ENVM_SIZE      (256 * K_BYTE)

(?_BYTE from "qemu/cutils.h")

I wanted to clarify this because you named your functions as it can 
models any SF2 but actually you restrict it to your M2S010.

Maybe to start you can rename msf2_soc_x() -> m2s010_sf2_soc_x() then if 
needed this can be generalized to other SoCs from SF2 family?

> > +
> > > +#define DDR_BASE_ADDRESS      0xA0000000
> > > +#define DDR_SIZE              (64 * 1024 * 1024)
> >
> > This belongs to the SoM.
> > Yes will change it.
> >
> > > +
> > > +#define SRAM_BASE_ADDRESS     0x20000000
> > > +#define SRAM_SIZE             (64 * 1024)
> >
> > Indeed this SoC is designed to have up to 64K of SRAM.
> > Luckily your model provides 64K.
>
>
> Yes it is 64k :)

Same here, what you have is:

#define ESRAM_REGION_SIZE     (64 * K_BYTE)
#define M2S010_ESRAM_SIZE     (64 * K_BYTE)

which happens to be equal but does not mean the same, do you see the 
difference?

> +
> > > +typedef struct MSF2State {
> > > +    /*< private >*/
> > > +    SysBusDevice parent_obj;
> > > +    /*< public >*/
> > > +
> > > +    ARMv7MState armv7m;
> > > +
> > > +    MSF2SysregState sysreg;
> > > +    MSF2TimerState timer;
> > > +    MSF2SpiState spi[MSF2_NUM_SPIS];
> > > +} MSF2State;
> > > +
> > > +#endif