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

[sundeep subbaraya]

Hi Philippe and Alistair,

On Mon, May 15, 2017 at 10:24 PM, Alistair Francis <address@hidden>
wrote:

>  On Thu, May 11, 2017 at 10:02 PM, Philippe Mathieu-Daudé
> <address@hidden> wrote:
> > 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.
>
> Yeah, I'm pretty sure you can't remap memory (unless that is some new
> feature I missed).
>
> Creating an alias seems like the right idea (you can even
> enable/disable it as needed to pretend we have dynamic remapping).
>
> As for names usually just copy the data sheet. MSF2.eNVM.alias sounds
> fine to me.
>

I will let it be MSF2.eNVM.alias and add comment about default remapping on
reset.

Thanks,
Sundeep

>
> Thanks,
>
> Alistair
>
> >
> >>>
> >>> +    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
>