On Thu, May 11, 2017 at 10:02 PM, Philippe Mathieu-Daudé <f4...@amsat.org> 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é <f4...@amsat.org> >> 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 <sundeep.l...@gmail.com> >>>> --- >>>> 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. 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
