Re: [PATCH v2 13/21] hw/arm/xilinx_zynq: Open-code pflash_cfi02_register()

[Peter Maydell]

On Mon, 9 Jan 2023 at 12:20, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> pflash_cfi02_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_cfi02_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/arm/xilinx_zynq.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 3190cc0b8d..201ca697ec 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -218,11 +218,21 @@ static void zynq_init(MachineState *machine)
>      DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
>
>      /* AMD */
> -    pflash_cfi02_register(0xe2000000, "zynq.pflash", FLASH_SIZE,
> -                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> -                          FLASH_SECTOR_SIZE, 1,
> -                          1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
> -                          0);
> +    dev = qdev_new(TYPE_PFLASH_CFI02);
> +    qdev_prop_set_string(dev, "name", "zynq.pflash");
> +    qdev_prop_set_drive(dev, "drive",
> +                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
> +    qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE);
> +    qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE);
> +    qdev_prop_set_uint8(dev, "device-width", 1);
> +    qdev_prop_set_uint8(dev, "mappings", 1);
> +    qdev_prop_set_uint8(dev, "big-endian", false);
> +    qdev_prop_set_uint16(dev, "id0", 0x0066);
> +    qdev_prop_set_uint16(dev, "id1", 0x0022);
> +    qdev_prop_set_uint16(dev, "unlock-addr0", 0x555);
> +    qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa);
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xe2000000);

What's the difference between setting "mappings" to 0 vs 1?

I was expecting that this could leave the "mappings" property
unset and at its default value, but the default is 0, not 1.
I think if I'm reading the cfi02 code right that the device
treats both 0 and 1 identically, though (meaning "don't set up
the mappings that repeat mirrors of the device across
the memory region"). If that's the case then we could just
drop the setting of 'mappings' here and add a note in the
commit message that 0 (the default) and 1 behave the same
so we don't need to explicitly set the property.

(I think this is the only use which sets mappings to 1,
and no users set it to 0.)

Either way
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM