Re: [Qemu-devel] [PATCH v7 2/2] hw/arm: Add arm SBSA reference machine, devices part

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7355 bytes --]

On 6/16/19 1:41 PM, Hongbo Zhang wrote:
> On Mon, 3 Jun 2019 at 18:54, Philippe Mathieu-Daudé <philmd@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.zhang@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.
[...]


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]