Re: [PATCH v2 19/21] hw/ppc: Open-code pflash_cfi01_register()

[BALATON Zoltan]

On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
> pflash_cfi01_register() hides an implicit sysbus mapping of
> MMIO region #0. This is not practical in a heterogeneous world
> where multiple cores use different address spaces. In order to
> remove pflash_cfi01_register() from the pflash API, open-code it
> as a qdev creation call followed by an explicit sysbus mapping.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/ppc/sam460ex.c     | 19 ++++++++++++++-----
> hw/ppc/virtex_ml507.c | 15 ++++++++++++---
> 2 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index cf7213f7c9..d2bf11d774 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -99,14 +99,23 @@ static int sam460ex_load_uboot(void)
>      *
>      * TODO Figure out what we really need here, and clean this up.
>      */
> -
> +    DeviceState *dev;
>     DriveInfo *dinfo;
>
>     dinfo = drive_get(IF_PFLASH, 0, 0);
> -    pflash_cfi01_register(FLASH_BASE | ((hwaddr)FLASH_BASE_H << 32),
> -                          "sam460ex.flash", FLASH_SIZE,
> -                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> -                          64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1));
> +    dev = qdev_new(TYPE_PFLASH_CFI01);
> +    qdev_prop_set_string(dev, "name", "sam460ex.flash");
> +    qdev_prop_set_drive(dev, "drive",
> +                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
> +    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / (64 * KiB));
> +    qdev_prop_set_uint64(dev, "sector-length", 64 * KiB);
> +    qdev_prop_set_uint8(dev, "width", 1);
> +    qdev_prop_set_bit(dev, "big-endian", true);
> +    qdev_prop_set_uint16(dev, "id0", 0x0089);
> +    qdev_prop_set_uint16(dev, "id1", 0x0018);

Can you drop unneeded zeros? Otherwise

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Regards,
BALATON Zoltan

> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
> +                    FLASH_BASE | ((hwaddr)FLASH_BASE_H << 32));
>
>     if (!dinfo) {
>         /*error_report("No flash image given with the 'pflash' parameter,"
> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> index f2f81bd425..2532806922 100644
> --- a/hw/ppc/virtex_ml507.c
> +++ b/hw/ppc/virtex_ml507.c
> @@ -233,9 +233,18 @@ static void virtex_init(MachineState *machine)
>     memory_region_add_subregion(address_space_mem, ram_base, machine->ram);
>
>     dinfo = drive_get(IF_PFLASH, 0, 0);
> -    pflash_cfi01_register(PFLASH_BASEADDR, "virtex.flash", FLASH_SIZE,
> -                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> -                          64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1);
> +    dev = qdev_new(TYPE_PFLASH_CFI01);
> +    qdev_prop_set_string(dev, "name", "virtex.flash");
> +    qdev_prop_set_drive(dev, "drive",
> +                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
> +    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / (64 * KiB));
> +    qdev_prop_set_uint64(dev, "sector-length", 64 * KiB);
> +    qdev_prop_set_uint8(dev, "width", 1);
> +    qdev_prop_set_bit(dev, "big-endian", true);
> +    qdev_prop_set_uint16(dev, "id0", 0x0089);
> +    qdev_prop_set_uint16(dev, "id1", 0x0018);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, PFLASH_BASEADDR);
>
>     cpu_irq = qdev_get_gpio_in(DEVICE(cpu), PPC40x_INPUT_INT);
>     dev = qdev_new("xlnx.xps-intc");