diff mbox series

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

Message ID 20230109120833.3330-14-philmd@linaro.org
State New
Headers show
Series hw: Remove implicit sysbus_mmio_map() from pflash APIs | expand

Commit Message

Philippe Mathieu-Daudé Jan. 9, 2023, 12:08 p.m. UTC 
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(-)

Comments

Peter Maydell Jan. 13, 2023, 1:55 p.m. UTC | #1 
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
diff mbox series

Patch

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);
 
     /* Create the main clock source, and feed slcr with it */
     zynq_machine->ps_clk = CLOCK(object_new(TYPE_CLOCK));