On 6/16/19 1:41 PM, Hongbo Zhang wrote:
> On Mon, 3 Jun 2019 at 18:54, Philippe Mathieu-Daudé <phi...@redhat.com> 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 <hongbo.zh...@linaro.org>
>>> ---
>>> 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.
[...]
signature.asc
Description: OpenPGP digital signature
