On 6/16/19 1:41 PM, Hongbo Zhang wrote: > On Mon, 3 Jun 2019 at 18:54, Philippe Mathieu-Daudé wrote: >> >> Hi Hongbo, Ard. >> >> On 4/18/19 6:04 AM, Hongbo Zhang wrote: >>> Following the previous patch, this patch adds peripheral devices to the >>> newly introduced SBSA-ref machine. >>> >>> Signed-off-by: Hongbo Zhang >>> --- >>> hw/arm/sbsa-ref.c | 451 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 451 insertions(+) >>> >>> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c [...] >>> +static void create_one_flash(const char *name, hwaddr flashbase, >>> + hwaddr flashsize, const char *file, >>> + MemoryRegion *sysmem) >>> +{ >>> + /* >>> + * Create and map a single flash device. We use the same >>> + * parameters as the flash devices on the Versatile Express board. >>> + */ >>> + DriveInfo *dinfo = drive_get_next(IF_PFLASH); >>> + DeviceState *dev = qdev_create(NULL, "cfi.pflash01"); >> >> Please use TYPE_PFLASH_CFI01 instead of "cfi.pflash01". >> >> I wanted to ask "does it has to be CFI01?" because this device model is >> in bad shape, but I guess I answered myself looking at the EDK2 platform >> code: >> >> - P30_CFI_ADDR_VENDOR_ID is not used >> - NorFlashDxe::NorFlashReadCfiData() is not implemented >> - All commands in NorFlashDxe uses: >> SEND_NOR_COMMAND(..., P30_CMD_...) >> which are specific to the Intel P30 Nor flash family (CFI01). >> >>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >>> + const uint64_t sectorlength = 256 * 1024; >>> + >>> + if (dinfo) { >>> + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo), >>> + &error_abort); >>> + } >>> + >>> + qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength); >>> + qdev_prop_set_uint64(dev, "sector-length", sectorlength); >>> + qdev_prop_set_uint8(dev, "width", 4); >>> + qdev_prop_set_uint8(dev, "device-width", 2); >>> + qdev_prop_set_bit(dev, "big-endian", false); >>> + qdev_prop_set_uint16(dev, "id0", 0x89); >>> + qdev_prop_set_uint16(dev, "id1", 0x18); >>> + qdev_prop_set_uint16(dev, "id2", 0x00); >>> + qdev_prop_set_uint16(dev, "id3", 0x00); >>> + qdev_prop_set_string(dev, "name", name); >>> + qdev_init_nofail(dev); >>> + >>> + memory_region_add_subregion(sysmem, flashbase, >>> + sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0)); >>> + >>> + if (file) { >>> + char *fn; >>> + int image_size; >>> + >>> + if (drive_get(IF_PFLASH, 0, 0)) { >>> + error_report("The contents of the first flash device may be " >>> + "specified with -bios or with -drive if=pflash... " >>> + "but you cannot use both options at once"); >>> + exit(1); >>> + } >>> + fn = qemu_find_file(QEMU_FILE_TYPE_BIOS, file); >>> + if (!fn) { >>> + error_report("Could not find ROM image '%s'", file); >>> + exit(1); >>> + } >>> + image_size = load_image_mr(fn, sysbus_mmio_get_region(sbd, 0)); >>> + g_free(fn); >>> + if (image_size < 0) { >>> + error_report("Could not load ROM image '%s'", file); >>> + exit(1); >>> + } >>> + } >>> +} >>> + >>> +static void create_flash(const SBSAMachineState *vms, >>> + MemoryRegion *sysmem, >>> + MemoryRegion *secure_sysmem) >>> +{ >>> + /* >>> + * Create one secure and nonsecure flash devices to fill SBSA_FLASH >>> + * space in the memmap, file passed via -bios goes in the first one. >>> + */ >>> + hwaddr flashsize = vms->memmap[SBSA_FLASH].size / 2; >>> + hwaddr flashbase = vms->memmap[SBSA_FLASH].base; >>> + >>> + create_one_flash("sbsa-ref.flash0", flashbase, flashsize, >>> + bios_name, secure_sysmem); >>> + create_one_flash("sbsa-ref.flash1", flashbase + flashsize, flashsize, >>> + NULL, sysmem); >> >> static const MemMapEntry base_memmap[] = { >> /* Space up to 0x8000000 is reserved for a boot ROM */ >> [VIRT_FLASH] = { 0, 0x08000000 }, >> > Hi Philippe, > Thank you for the long comments. > Some parts of this machine are based on the 'virt' machine, but I use > this flash memory map: > [SBSA_FLASH] = { 0, 0x20000000 }, > that are 256M *2 flashes. > Franky I didn't consider the product part number etc, just use the > original design in 'virt' and change the size large enough as I think. I guess we are very lucky... The Micron PC28F00BP33EF is a 2Gib symmetrical blocks NOR flash, and the only CFI01 one I could find a datasheet :) "The 2Gb device employs a virtual chip enable feature, which combines two 1Gb die with a common chip enable". > Peter, Ard, do we need more considerations here? > >> So you are creating 2 identical flashes of 128MiB/2 = 64 MiB each which >> are the biggest flash you can have: >> >> "The P30 family provides density upgrades from 64-Mbit through >> 512-Mbit." On Intel, the 512-Mib case is particular in that it is built >> of 2x 256-Mib on the same die, with a virtual chip enable. It is simpler >> to use a Micron or Numonyx model. >> >> I plan to use a whitelist of supported (and tested...) models, the one >> you use seems the Micron JS28F512P30EF ('E' for 'Symetrically Blocked', >> since the current model doesn't support bottom/top blocks layout), or in >> short: 28F512P30E. >> Ard, is that OK? >> >> Checking EDK2 git history, the driver is part of ArmPlatformPkg, >> imported in commit 1d5d0ae92d9541, based on 'Versatile Express'. >> >> On the Versatile Express and the RealView Emulation Baseboard user >> guides, I only find reference of "64MB of NOR flash" with no specific model. >> >> Peter, do you have physical access to tell me what flashes are used on >> real hardware? I googled for Linux console boot log but the kernel >> doesn't seem to care about detecting/mapping the flash. >> >> QEMU added the flash to the Versatile board in commit 964c695a54ceda3a, >> with the following description: >> >> - add support for the 64MB NOR CFI01 flash available at >> 0x34000000 on the versatilepb board >> >> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0225d/BBAJIHEC.html >> >> However on this link I only see "SSMC Chip Select 1, normally NOR flash >> (During boot remapping, this can be NOR flash, Disk-on-Chip, or static >> expansion memory)". Again, nothing specific (which makes sense, why >> restrict the users to a particuliar family, as long as the pinout matches). >> >> The Manufacturer/Device ID used in QEMU (0x0089, 0x0018) correspond to >> the Micron 28F128J3D (128-Mbit, 128 symmetrical blocks of 128-KiB). >> Neither the flash size (64 vs 16) nor the block size (256 vs 128) match. >> >> The safer fix here is to find a CFI01 flash of 256 sectors of 256-KiB >> and update the Manufacturer/Device IDs in QEMU. Luckily this matches the >> 28F512P30E cited previously :) >> >> Regards, >> >> Phil. [...]