Re: [PATCH v1 5/6] riscv/virt: Add the PFlash CFI01 device

[Bin Meng]

On Fri, Sep 20, 2019 at 6:36 AM Alistair Francis
<address@hidden> wrote:
>
> Add the CFI01 PFlash to the RISC-V virt board. This is the same PFlash
> from the ARM Virt board and the implementation is based on the ARM Virt
> board. This allows users to specify flash files from the command line.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
>  hw/riscv/Kconfig        |  1 +
>  hw/riscv/virt.c         | 81 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/riscv/virt.h |  3 ++
>  3 files changed, 85 insertions(+)
>
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index fb19b2df3a..b12660b9f8 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -36,4 +36,5 @@ config RISCV_VIRT
>      select SERIAL
>      select VIRTIO_MMIO
>      select PCI_EXPRESS_GENERIC_BRIDGE
> +    select PFLASH_CFI01
>      select SIFIVE
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index d36f5625ec..ca002ecea7 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -26,6 +26,7 @@
>  #include "hw/boards.h"
>  #include "hw/loader.h"
>  #include "hw/sysbus.h"
> +#include "hw/qdev-properties.h"
>  #include "hw/char/serial.h"
>  #include "target/riscv/cpu.h"
>  #include "hw/riscv/riscv_hart.h"
> @@ -61,12 +62,72 @@ static const struct MemmapEntry {
>      [VIRT_PLIC] =        {  0xc000000,     0x4000000 },
>      [VIRT_UART0] =       { 0x10000000,         0x100 },
>      [VIRT_VIRTIO] =      { 0x10001000,        0x1000 },
> +    [VIRT_FLASH] =       { 0x20000000,     0x2000000 },
>      [VIRT_DRAM] =        { 0x80000000,           0x0 },
>      [VIRT_PCIE_MMIO] =   { 0x40000000,    0x40000000 },
>      [VIRT_PCIE_PIO] =    { 0x03000000,    0x00010000 },
>      [VIRT_PCIE_ECAM] =   { 0x30000000,    0x10000000 },
>  };
>
> +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
> +
> +static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> +                                       const char *name,
> +                                       const char *alias_prop_name)
> +{
> +    /*
> +     * Create a single flash device.  We use the same parameters as
> +     * the flash devices on the ARM virt board.
> +     */
> +    DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
> +
> +    qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
> +    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);

alias_prop_name is unused? ARM virt has 2 more calls in the same function here.

> +
> +    return PFLASH_CFI01(dev);
> +}
> +
> +static void virt_flash_create(RISCVVirtState *s)
> +{
> +    s->flash[0] = virt_flash_create1(s, "virt.flash0", "pflash0");
> +    s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");

I don't think we should mirror what is used on ARM virt board to
create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
they need distinguish secure and non-secure. For sifive_u, only one is
enough.

> +}
> +
> +static void virt_flash_map1(PFlashCFI01 *flash,
> +                            hwaddr base, hwaddr size,
> +                            MemoryRegion *sysmem)
> +{
> +    DeviceState *dev = DEVICE(flash);
> +
> +    assert(size % VIRT_FLASH_SECTOR_SIZE == 0);
> +    assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> +    qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
> +    qdev_init_nofail(dev);
> +
> +    memory_region_add_subregion(sysmem, base,
> +                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev),
> +                                                       0));
> +}
> +
> +static void virt_flash_map(RISCVVirtState *s,
> +                           MemoryRegion *sysmem)
> +{
> +    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> +    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
> +
> +    virt_flash_map1(s->flash[0], flashbase, flashsize,
> +                    sysmem);
> +    virt_flash_map1(s->flash[1], flashbase + flashsize, flashsize,
> +                    sysmem);
> +}
> +
>  static void create_pcie_irq_map(void *fdt, char *nodename,
>                                  uint32_t plic_phandle)
>  {
> @@ -121,6 +182,8 @@ static void create_fdt(RISCVVirtState *s, const struct 
> MemmapEntry *memmap,
>      char *nodename;
>      uint32_t plic_phandle, phandle = 1;
>      int i;
> +    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> +    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
>
>      fdt = s->fdt = create_device_tree(&s->fdt_size);
>      if (!fdt) {
> @@ -316,6 +379,15 @@ static void create_fdt(RISCVVirtState *s, const struct 
> MemmapEntry *memmap,
>          qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
>      }
>      g_free(nodename);
> +
> +    nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
> +    qemu_fdt_add_subnode(s->fdt, nodename);
> +    qemu_fdt_setprop_string(s->fdt, nodename, "compatible", "cfi-flash");
> +    qemu_fdt_setprop_sized_cells(s->fdt, nodename, "reg",
> +                                 2, flashbase, 2, flashsize,
> +                                 2, flashbase + flashsize, 2, flashsize);
> +    qemu_fdt_setprop_cell(s->fdt, nodename, "bank-width", 4);
> +    g_free(nodename);
>  }
>
>
> @@ -496,6 +568,15 @@ static void riscv_virt_board_init(MachineState *machine)
>          0, qdev_get_gpio_in(DEVICE(s->plic), UART0_IRQ), 399193,
>          serial_hd(0), DEVICE_LITTLE_ENDIAN);
>
> +    virt_flash_create(s);
> +
> +    /* Map legacy -drive if=pflash to machine properties */
> +    for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
> +        pflash_cfi01_legacy_drive(s->flash[i],
> +                                  drive_get(IF_PFLASH, 0, i));
> +    }
> +    virt_flash_map(s, system_memory);
> +
>      g_free(plic_hart_config);
>  }
>
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 6e5fbe5d3b..2ca8bd3512 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -21,6 +21,7 @@
>
>  #include "hw/riscv/riscv_hart.h"
>  #include "hw/sysbus.h"
> +#include "hw/block/flash.h"
>
>  typedef struct {
>      /*< private >*/
> @@ -29,6 +30,7 @@ typedef struct {
>      /*< public >*/
>      RISCVHartArrayState soc;
>      DeviceState *plic;
> +    PFlashCFI01 *flash[2];
>      void *fdt;
>      int fdt_size;
>  } RISCVVirtState;
> @@ -41,6 +43,7 @@ enum {
>      VIRT_PLIC,
>      VIRT_UART0,
>      VIRT_VIRTIO,
> +    VIRT_FLASH,
>      VIRT_DRAM,
>      VIRT_PCIE_MMIO,
>      VIRT_PCIE_PIO,
> --

Regards,
Bin