[PATCH 00/20] hw: Remove implicit sysbus_mmio_map() from pflash APIs

[PATCH 00/20] hw: Remove implicit sysbus_mmio_map() from pflash APIs

[Philippe Mathieu-Daudé]

Paving the road toward heterogeneous QEMU, the limitations of
having a single machine sysbus become more apparent.

The sysbus_mmio_map() API forces the caller to map a sysbus
device to an address on the system bus (system bus here is
the root MemoryRegion returned by get_system_memory() ).

This is not practical when each core has its own address
space and group of cores have access to a part of the
peripherals.

Experimenting with the PFLASH devices. Here the fix is
quite easy, we split the pflash_cfi_register() -- which
does the implicit sysbus mapping -- into an explicit qdev
pflash_cfi_create() followed by the sysbus_mmio_map() call.

Since we were touching the PFLASH API, we restricted the
PFlashCFI0X structures to their models. The API now deals
with a generic qdev pointer (DeviceState*).

First 15 patches deal with the CFI type 1 model, then the
last 5 with the type 2.

The patch logic is mostly:

- extract pflash_cfi_create() from pflash_cfi_register()
- open-code pflash_cfi_register() for each hw/${ARCH}/
- remove unused pflash_cfi_register()
- reduce PFlashCFI0x structure scope

Please review,

Phil.

Philippe Mathieu-Daudé (20):
  hw/block: Pass DeviceState to pflash_cfi01_get_blk()
  hw/block: Use pflash_cfi01_get_blk() in pflash_cfi01_legacy_drive()
  hw/block: Pass DeviceState to pflash_cfi01_get_memory()
  hw/arm: Use generic DeviceState instead of PFlashCFI01
  hw/loongarch: Use generic DeviceState instead of PFlashCFI01
  hw/riscv: Use generic DeviceState instead of PFlashCFI01
  hw/i386: Use generic DeviceState instead of PFlashCFI01
  hw/xtensa: Use generic DeviceState instead of PFlashCFI01
  hw/block: Factor pflash_cfi01_create() out of pflash_cfi01_register()
  hw/arm: Open-code pflash_cfi01_register()
  hw/microblaze: Open-code pflash_cfi01_register()
  hw/mips: Open-code pflash_cfi01_register()
  hw/ppc: Open-code pflash_cfi01_register()
  hw/block: Remove unused pflash_cfi01_register()
  hw/block: Make PFlashCFI01 QOM declaration internal
  hw/block: Factor pflash_cfi02_create() out of pflash_cfi02_register()
  hw/arm: Open-code pflash_cfi02_register()
  hw/sh4: Open-code pflash_cfi02_register()
  hw/block: Remove unused pflash_cfi02_register()
  hw/block: Make PFlashCFI02 QOM declaration internal

 hw/arm/collie.c                          | 15 ++++---
 hw/arm/digic_boards.c                    | 14 +++---
 hw/arm/gumstix.c                         | 19 +++++---
 hw/arm/mainstone.c                       | 13 +++---
 hw/arm/musicpal.c                        | 13 +++---
 hw/arm/omap_sx1.c                        | 22 +++++----
 hw/arm/sbsa-ref.c                        |  8 ++--
 hw/arm/versatilepb.c                     | 13 +++---
 hw/arm/vexpress.c                        | 12 +++--
 hw/arm/virt.c                            |  6 +--
 hw/arm/xilinx_zynq.c                     | 10 ++---
 hw/arm/z2.c                              | 10 +++--
 hw/block/pflash_cfi01.c                  | 35 ++++++++-------
 hw/block/pflash_cfi02.c                  | 25 +++++------
 hw/i386/pc_sysfw.c                       |  6 +--
 hw/loongarch/virt.c                      |  9 ++--
 hw/microblaze/petalogix_ml605_mmu.c      |  8 ++--
 hw/microblaze/petalogix_s3adsp1800_mmu.c |  8 ++--
 hw/mips/malta.c                          | 13 +++---
 hw/ppc/e500.c                            |  2 +-
 hw/ppc/sam460ex.c                        | 12 +++--
 hw/ppc/virtex_ml507.c                    |  7 +--
 hw/riscv/virt.c                          |  7 +--
 hw/sh4/r2d.c                             |  9 ++--
 hw/xtensa/xtfpga.c                       |  6 +--
 include/hw/arm/virt.h                    |  3 +-
 include/hw/block/flash.h                 | 57 ++++++++++++------------
 include/hw/i386/pc.h                     |  3 +-
 include/hw/loongarch/virt.h              |  3 +-
 include/hw/riscv/virt.h                  |  3 +-
 30 files changed, 200 insertions(+), 171 deletions(-)

-- 
2.38.1

[PATCH 01/20] hw/block: Pass DeviceState to pflash_cfi01_get_blk()

[Philippe Mathieu-Daudé]

The point of a getter() function is to not expose the structure
internal fields. Otherwise callers could simply access the
PFlashCFI01::blk field.

Have the callers pass a DeviceState* argument. The QOM
type check is done in the callee.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/sbsa-ref.c        | 2 +-
 hw/arm/virt.c            | 2 +-
 hw/block/pflash_cfi01.c  | 4 +++-
 hw/i386/pc_sysfw.c       | 4 ++--
 include/hw/block/flash.h | 2 +-
 5 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 4bb444684f..65b9acba04 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -346,7 +346,7 @@ static bool sbsa_firmware_init(SBSAMachineState *sms,
 
     sbsa_flash_map(sms, sysmem, secure_sysmem);
 
-    pflash_blk0 = pflash_cfi01_get_blk(sms->flash[0]);
+    pflash_blk0 = pflash_cfi01_get_blk(DEVICE(sms->flash[0]));
 
     bios_name = MACHINE(sms)->firmware;
     if (bios_name) {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ea2413a0ba..954e3ca5ce 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1233,7 +1233,7 @@ static bool virt_firmware_init(VirtMachineState *vms,
 
     virt_flash_map(vms, sysmem, secure_sysmem);
 
-    pflash_blk0 = pflash_cfi01_get_blk(vms->flash[0]);
+    pflash_blk0 = pflash_cfi01_get_blk(DEVICE(vms->flash[0]));
 
     bios_name = MACHINE(vms)->firmware;
     if (bios_name) {
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 0cbc2fb4cb..458c50ec45 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -984,8 +984,10 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
     return PFLASH_CFI01(dev);
 }
 
-BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl)
+BlockBackend *pflash_cfi01_get_blk(DeviceState *dev)
 {
+    PFlashCFI01 *fl = PFLASH_CFI01(dev);
+
     return fl->blk;
 }
 
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index c8d9e71b88..4b85c48ec8 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -152,7 +152,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
 
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
         system_flash = pcms->flash[i];
-        blk = pflash_cfi01_get_blk(system_flash);
+        blk = pflash_cfi01_get_blk(DEVICE(system_flash));
         if (!blk) {
             break;
         }
@@ -216,7 +216,7 @@ void pc_system_firmware_init(PCMachineState *pcms,
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
         pflash_cfi01_legacy_drive(pcms->flash[i],
                                   drive_get(IF_PFLASH, 0, i));
-        pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]);
+        pflash_blk[i] = pflash_cfi01_get_blk(DEVICE(pcms->flash[i]));
     }
 
     /* Reject gaps */
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 86d8363bb0..961b6e9f74 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -21,7 +21,7 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
                                    uint16_t id0, uint16_t id1,
                                    uint16_t id2, uint16_t id3,
                                    int be);
-BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl);
+BlockBackend *pflash_cfi01_get_blk(DeviceState *dev);
 MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl);
 void pflash_cfi01_legacy_drive(PFlashCFI01 *dev, DriveInfo *dinfo);
 
-- 
2.38.1

Re: [PATCH 01/20] hw/block: Pass DeviceState to pflash_cfi01_get_blk()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:40 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> The point of a getter() function is to not expose the structure
> internal fields. Otherwise callers could simply access the
> PFlashCFI01::blk field.
>
> Have the callers pass a DeviceState* argument. The QOM
> type check is done in the callee.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/sbsa-ref.c        | 2 +-
>  hw/arm/virt.c            | 2 +-
>  hw/block/pflash_cfi01.c  | 4 +++-
>  hw/i386/pc_sysfw.c       | 4 ++--
>  include/hw/block/flash.h | 2 +-
>  5 files changed, 8 insertions(+), 6 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 02/20] hw/block: Use pflash_cfi01_get_blk() in pflash_cfi01_legacy_drive()

[Philippe Mathieu-Daudé]

By using pflash_cfi01_get_blk(), pflash_cfi01_legacy_drive()
doesn't require any knowledge of the PFlashCFI01 structure.
Thus we can pass a generic DeviceState pointer.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/sbsa-ref.c        | 2 +-
 hw/arm/virt.c            | 2 +-
 hw/block/pflash_cfi01.c  | 6 +++---
 hw/i386/pc_sysfw.c       | 2 +-
 hw/riscv/virt.c          | 2 +-
 include/hw/block/flash.h | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 65b9acba04..1d29e8ca7f 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -340,7 +340,7 @@ static bool sbsa_firmware_init(SBSAMachineState *sms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(sms->flash); i++) {
-        pflash_cfi01_legacy_drive(sms->flash[i],
+        pflash_cfi01_legacy_drive(DEVICE(sms->flash[i]),
                                   drive_get(IF_PFLASH, 0, i));
     }
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 954e3ca5ce..57726b0f52 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1227,7 +1227,7 @@ static bool virt_firmware_init(VirtMachineState *vms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(vms->flash); i++) {
-        pflash_cfi01_legacy_drive(vms->flash[i],
+        pflash_cfi01_legacy_drive(DEVICE(vms->flash[i]),
                                   drive_get(IF_PFLASH, 0, i));
     }
 
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 458c50ec45..8beba24989 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -1002,7 +1002,7 @@ MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
  * Else if @fl's property "drive" is already set, fatal error.
  * Else set it to the BlockBackend with @dinfo.
  */
-void pflash_cfi01_legacy_drive(PFlashCFI01 *fl, DriveInfo *dinfo)
+void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo)
 {
     Location loc;
 
@@ -1012,11 +1012,11 @@ void pflash_cfi01_legacy_drive(PFlashCFI01 *fl, DriveInfo *dinfo)
 
     loc_push_none(&loc);
     qemu_opts_loc_restore(dinfo->opts);
-    if (fl->blk) {
+    if (pflash_cfi01_get_blk(dev)) {
         error_report("clashes with -machine");
         exit(1);
     }
-    qdev_prop_set_drive_err(DEVICE(fl), "drive", blk_by_legacy_dinfo(dinfo),
+    qdev_prop_set_drive_err(dev, "drive", blk_by_legacy_dinfo(dinfo),
                             &error_fatal);
     loc_pop(&loc);
 }
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 4b85c48ec8..c08cba6628 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -214,7 +214,7 @@ void pc_system_firmware_init(PCMachineState *pcms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
-        pflash_cfi01_legacy_drive(pcms->flash[i],
+        pflash_cfi01_legacy_drive(DEVICE(pcms->flash[i]),
                                   drive_get(IF_PFLASH, 0, i));
         pflash_blk[i] = pflash_cfi01_get_blk(DEVICE(pcms->flash[i]));
     }
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index a5bc7353b4..400bd9329f 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1517,7 +1517,7 @@ static void virt_machine_init(MachineState *machine)
 
     for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
         /* Map legacy -drive if=pflash to machine properties */
-        pflash_cfi01_legacy_drive(s->flash[i],
+        pflash_cfi01_legacy_drive(DEVICE(s->flash[i]),
                                   drive_get(IF_PFLASH, 0, i));
     }
     virt_flash_map(s, system_memory);
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 961b6e9f74..701a2c1701 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -23,7 +23,7 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
                                    int be);
 BlockBackend *pflash_cfi01_get_blk(DeviceState *dev);
 MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl);
-void pflash_cfi01_legacy_drive(PFlashCFI01 *dev, DriveInfo *dinfo);
+void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo);
 
 /* pflash_cfi02.c */
 
-- 
2.38.1

Re: [PATCH 02/20] hw/block: Use pflash_cfi01_get_blk() in pflash_cfi01_legacy_drive()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:10 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> By using pflash_cfi01_get_blk(), pflash_cfi01_legacy_drive()
> doesn't require any knowledge of the PFlashCFI01 structure.
> Thus we can pass a generic DeviceState pointer.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/sbsa-ref.c        | 2 +-
>  hw/arm/virt.c            | 2 +-
>  hw/block/pflash_cfi01.c  | 6 +++---
>  hw/i386/pc_sysfw.c       | 2 +-
>  hw/riscv/virt.c          | 2 +-
>  include/hw/block/flash.h | 2 +-
>  6 files changed, 8 insertions(+), 8 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 03/20] hw/block: Pass DeviceState to pflash_cfi01_get_memory()

[Philippe Mathieu-Daudé]

The point of a getter() function is to not expose the structure
internal fields. Otherwise callers could simply access the
PFlashCFI01::mem field.

Have the callers pass a DeviceState* argument. The QOM
type check is done in the callee.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/pflash_cfi01.c  | 4 +++-
 hw/i386/pc_sysfw.c       | 2 +-
 hw/mips/malta.c          | 3 ++-
 hw/ppc/e500.c            | 2 +-
 hw/xtensa/xtfpga.c       | 2 +-
 include/hw/block/flash.h | 2 +-
 6 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 8beba24989..866ea596ea 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -991,8 +991,10 @@ BlockBackend *pflash_cfi01_get_blk(DeviceState *dev)
     return fl->blk;
 }
 
-MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
+MemoryRegion *pflash_cfi01_get_memory(DeviceState *dev)
 {
+    PFlashCFI01 *fl = PFLASH_CFI01(dev);
+
     return &fl->mem;
 }
 
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index c08cba6628..60db0efb41 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -187,7 +187,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
                         0x100000000ULL - total_size);
 
         if (i == 0) {
-            flash_mem = pflash_cfi01_get_memory(system_flash);
+            flash_mem = pflash_cfi01_get_memory(DEVICE(system_flash));
             pc_isa_bios_init(rom_memory, flash_mem, size);
 
             /* Encrypt the pflash boot ROM */
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index c0a2e0ab04..43fbb97799 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1291,7 +1291,8 @@ void mips_malta_init(MachineState *machine)
                                dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
                                65536,
                                4, 0x0000, 0x0000, 0x0000, 0x0000, be);
-    bios = pflash_cfi01_get_memory(fl);
+    dev = DEVICE(fl);
+    bios = pflash_cfi01_get_memory(dev);
     fl_idx++;
     if (kernel_filename) {
         ram_low_size = MIN(ram_size, 256 * MiB);
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 9fa1f8e6cf..b127068431 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1144,7 +1144,7 @@ void ppce500_init(MachineState *machine)
         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
         memory_region_add_subregion(&pms->pbus_dev->mmio, 0,
-                                    pflash_cfi01_get_memory(PFLASH_CFI01(dev)));
+                                    pflash_cfi01_get_memory(dev));
     }
 
     /*
diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index 2a5556a35f..bce3a543b0 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -459,7 +459,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
         }
     } else {
         if (flash) {
-            MemoryRegion *flash_mr = pflash_cfi01_get_memory(flash);
+            MemoryRegion *flash_mr = pflash_cfi01_get_memory(DEVICE(flash));
             MemoryRegion *flash_io = g_malloc(sizeof(*flash_io));
             uint32_t size = env->config->sysrom.location[0].size;
 
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 701a2c1701..25affdf7a5 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -22,7 +22,7 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
                                    uint16_t id2, uint16_t id3,
                                    int be);
 BlockBackend *pflash_cfi01_get_blk(DeviceState *dev);
-MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl);
+MemoryRegion *pflash_cfi01_get_memory(DeviceState *dev);
 void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo);
 
 /* pflash_cfi02.c */
-- 
2.38.1

Re: [PATCH 03/20] hw/block: Pass DeviceState to pflash_cfi01_get_memory()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:39 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> The point of a getter() function is to not expose the structure
> internal fields. Otherwise callers could simply access the
> PFlashCFI01::mem field.
>
> Have the callers pass a DeviceState* argument. The QOM
> type check is done in the callee.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/block/pflash_cfi01.c  | 4 +++-
>  hw/i386/pc_sysfw.c       | 2 +-
>  hw/mips/malta.c          | 3 ++-
>  hw/ppc/e500.c            | 2 +-
>  hw/xtensa/xtfpga.c       | 2 +-
>  include/hw/block/flash.h | 2 +-
>  6 files changed, 9 insertions(+), 6 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 04/20] hw/arm: Use generic DeviceState instead of PFlashCFI01

[Philippe Mathieu-Daudé]

Nothing here requires access to PFlashCFI01 internal fields:
use the inherited generic DeviceState.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/sbsa-ref.c     | 12 ++++++------
 hw/arm/vexpress.c     | 12 +++++-------
 hw/arm/virt.c         | 10 +++++-----
 include/hw/arm/virt.h |  3 +--
 4 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 1d29e8ca7f..8e60e0e58d 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -91,7 +91,7 @@ struct SBSAMachineState {
     int fdt_size;
     int psci_conduit;
     DeviceState *gic;
-    PFlashCFI01 *flash[2];
+    DeviceState *flash[2];
 };
 
 #define TYPE_SBSA_MACHINE   MACHINE_TYPE_NAME("sbsa-ref")
@@ -264,7 +264,7 @@ static void create_fdt(SBSAMachineState *sms)
 
 #define SBSA_FLASH_SECTOR_SIZE (256 * KiB)
 
-static PFlashCFI01 *sbsa_flash_create1(SBSAMachineState *sms,
+static DeviceState *sbsa_flash_create1(SBSAMachineState *sms,
                                         const char *name,
                                         const char *alias_prop_name)
 {
@@ -286,7 +286,7 @@ static PFlashCFI01 *sbsa_flash_create1(SBSAMachineState *sms,
     object_property_add_child(OBJECT(sms), name, OBJECT(dev));
     object_property_add_alias(OBJECT(sms), alias_prop_name,
                               OBJECT(dev), "drive");
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 static void sbsa_flash_create(SBSAMachineState *sms)
@@ -295,7 +295,7 @@ static void sbsa_flash_create(SBSAMachineState *sms)
     sms->flash[1] = sbsa_flash_create1(sms, "sbsa.flash1", "pflash1");
 }
 
-static void sbsa_flash_map1(PFlashCFI01 *flash,
+static void sbsa_flash_map1(DeviceState *flash,
                             hwaddr base, hwaddr size,
                             MemoryRegion *sysmem)
 {
@@ -340,13 +340,13 @@ static bool sbsa_firmware_init(SBSAMachineState *sms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(sms->flash); i++) {
-        pflash_cfi01_legacy_drive(DEVICE(sms->flash[i]),
+        pflash_cfi01_legacy_drive(sms->flash[i],
                                   drive_get(IF_PFLASH, 0, i));
     }
 
     sbsa_flash_map(sms, sysmem, secure_sysmem);
 
-    pflash_blk0 = pflash_cfi01_get_blk(DEVICE(sms->flash[0]));
+    pflash_blk0 = pflash_cfi01_get_blk(sms->flash[0]);
 
     bios_name = MACHINE(sms)->firmware;
     if (bios_name) {
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index e1d1983ae6..94eeff73d9 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -508,7 +508,7 @@ static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
 /* Open code a private version of pflash registration since we
  * need to set non-default device width for VExpress platform.
  */
-static PFlashCFI01 *ve_pflash_cfi01_register(hwaddr base, const char *name,
+static DeviceState *ve_pflash_cfi01_register(hwaddr base, const char *name,
                                              DriveInfo *di)
 {
     DeviceState *dev = qdev_new(TYPE_PFLASH_CFI01);
@@ -531,7 +531,7 @@ static PFlashCFI01 *ve_pflash_cfi01_register(hwaddr base, const char *name,
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 static void vexpress_common_init(MachineState *machine)
@@ -543,7 +543,6 @@ static void vexpress_common_init(MachineState *machine)
     qemu_irq pic[64];
     uint32_t sys_id;
     DriveInfo *dinfo;
-    PFlashCFI01 *pflash0;
     I2CBus *i2c;
     ram_addr_t vram_size, sram_size;
     MemoryRegion *sysmem = get_system_memory();
@@ -657,16 +656,15 @@ static void vexpress_common_init(MachineState *machine)
     sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash0 = ve_pflash_cfi01_register(map[VE_NORFLASH0], "vexpress.flash0",
-                                       dinfo);
-    if (!pflash0) {
+    dev = ve_pflash_cfi01_register(map[VE_NORFLASH0], "vexpress.flash0", dinfo);
+    if (!dev) {
         error_report("vexpress: error registering flash 0");
         exit(1);
     }
 
     if (map[VE_NORFLASHALIAS] != -1) {
         /* Map flash 0 as an alias into low memory */
-        flash0mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(pflash0), 0);
+        flash0mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
         memory_region_init_alias(flashalias, NULL, "vexpress.flashalias",
                                  flash0mem, 0, VEXPRESS_FLASH_SIZE);
         memory_region_add_subregion(sysmem, map[VE_NORFLASHALIAS], flashalias);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 57726b0f52..e47070105d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1105,7 +1105,7 @@ static void create_virtio_devices(const VirtMachineState *vms)
 
 #define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
 
-static PFlashCFI01 *virt_flash_create1(VirtMachineState *vms,
+static DeviceState *virt_flash_create1(VirtMachineState *vms,
                                         const char *name,
                                         const char *alias_prop_name)
 {
@@ -1127,7 +1127,7 @@ static PFlashCFI01 *virt_flash_create1(VirtMachineState *vms,
     object_property_add_child(OBJECT(vms), name, OBJECT(dev));
     object_property_add_alias(OBJECT(vms), alias_prop_name,
                               OBJECT(dev), "drive");
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 static void virt_flash_create(VirtMachineState *vms)
@@ -1136,7 +1136,7 @@ static void virt_flash_create(VirtMachineState *vms)
     vms->flash[1] = virt_flash_create1(vms, "virt.flash1", "pflash1");
 }
 
-static void virt_flash_map1(PFlashCFI01 *flash,
+static void virt_flash_map1(DeviceState *flash,
                             hwaddr base, hwaddr size,
                             MemoryRegion *sysmem)
 {
@@ -1227,13 +1227,13 @@ static bool virt_firmware_init(VirtMachineState *vms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(vms->flash); i++) {
-        pflash_cfi01_legacy_drive(DEVICE(vms->flash[i]),
+        pflash_cfi01_legacy_drive(vms->flash[i],
                                   drive_get(IF_PFLASH, 0, i));
     }
 
     virt_flash_map(vms, sysmem, secure_sysmem);
 
-    pflash_blk0 = pflash_cfi01_get_blk(DEVICE(vms->flash[0]));
+    pflash_blk0 = pflash_cfi01_get_blk(vms->flash[0]);
 
     bios_name = MACHINE(vms)->firmware;
     if (bios_name) {
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index c7dd59d7f1..817b43b248 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -34,7 +34,6 @@
 #include "qemu/notify.h"
 #include "hw/boards.h"
 #include "hw/arm/boot.h"
-#include "hw/block/flash.h"
 #include "sysemu/kvm.h"
 #include "hw/intc/arm_gicv3_common.h"
 #include "qom/object.h"
@@ -142,7 +141,7 @@ struct VirtMachineState {
     Notifier machine_done;
     DeviceState *platform_bus_dev;
     FWCfgState *fw_cfg;
-    PFlashCFI01 *flash[2];
+    DeviceState *flash[2];
     bool secure;
     bool highmem;
     bool highmem_compact;
-- 
2.38.1

Re: [PATCH 04/20] hw/arm: Use generic DeviceState instead of PFlashCFI01

[Bin Meng]

On Thu, Jan 5, 2023 at 6:35 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Nothing here requires access to PFlashCFI01 internal fields:
> use the inherited generic DeviceState.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/arm/sbsa-ref.c     | 12 ++++++------
>  hw/arm/vexpress.c     | 12 +++++-------
>  hw/arm/virt.c         | 10 +++++-----
>  include/hw/arm/virt.h |  3 +--
>  4 files changed, 17 insertions(+), 20 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 05/20] hw/loongarch: Use generic DeviceState instead of PFlashCFI01

[Philippe Mathieu-Daudé]

Nothing here requires access to PFlashCFI01 internal fields:
use the inherited generic DeviceState.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/loongarch/virt.c         | 9 ++++-----
 include/hw/loongarch/virt.h | 3 +--
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index c8a495ea30..cd5aa26f49 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -45,7 +45,7 @@
 #include "sysemu/block-backend.h"
 #include "hw/block/flash.h"
 
-static void virt_flash_create(LoongArchMachineState *lams)
+static DeviceState *virt_flash_create(LoongArchMachineState *lams)
 {
     DeviceState *dev = qdev_new(TYPE_PFLASH_CFI01);
 
@@ -62,14 +62,13 @@ static void virt_flash_create(LoongArchMachineState *lams)
     object_property_add_alias(OBJECT(lams), "pflash",
                               OBJECT(dev), "drive");
 
-    lams->flash = PFLASH_CFI01(dev);
+    return dev;
 }
 
 static void virt_flash_map(LoongArchMachineState *lams,
                            MemoryRegion *sysmem)
 {
-    PFlashCFI01 *flash = lams->flash;
-    DeviceState *dev = DEVICE(flash);
+    DeviceState *dev = lams->flash;
     hwaddr base = VIRT_FLASH_BASE;
     hwaddr size = VIRT_FLASH_SIZE;
 
@@ -899,7 +898,7 @@ static void loongarch_machine_initfn(Object *obj)
     lams->acpi = ON_OFF_AUTO_AUTO;
     lams->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
     lams->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
-    virt_flash_create(lams);
+    lams->flash = virt_flash_create(lams);
 }
 
 static bool memhp_type_supported(DeviceState *dev)
diff --git a/include/hw/loongarch/virt.h b/include/hw/loongarch/virt.h
index f5f818894e..519b25c722 100644
--- a/include/hw/loongarch/virt.h
+++ b/include/hw/loongarch/virt.h
@@ -12,7 +12,6 @@
 #include "hw/boards.h"
 #include "qemu/queue.h"
 #include "hw/intc/loongarch_ipi.h"
-#include "hw/block/flash.h"
 
 #define LOONGARCH_MAX_VCPUS     4
 
@@ -52,7 +51,7 @@ struct LoongArchMachineState {
     int          fdt_size;
     DeviceState *platform_bus_dev;
     PCIBus       *pci_bus;
-    PFlashCFI01  *flash;
+    DeviceState  *flash;
 };
 
 #define TYPE_LOONGARCH_MACHINE  MACHINE_TYPE_NAME("virt")
-- 
2.38.1

Re: [PATCH 05/20] hw/loongarch: Use generic DeviceState instead of PFlashCFI01

[Bin Meng]

On Thu, Jan 5, 2023 at 6:16 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Nothing here requires access to PFlashCFI01 internal fields:
> use the inherited generic DeviceState.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/loongarch/virt.c         | 9 ++++-----
>  include/hw/loongarch/virt.h | 3 +--
>  2 files changed, 5 insertions(+), 7 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 06/20] hw/riscv: Use generic DeviceState instead of PFlashCFI01

[Philippe Mathieu-Daudé]

Nothing here requires access to PFlashCFI01 internal fields:
use the inherited generic DeviceState.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/riscv/virt.c         | 9 +++++----
 include/hw/riscv/virt.h | 3 +--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 400bd9329f..b421a9dc12 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -46,6 +46,7 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "sysemu/tpm.h"
+#include "hw/block/flash.h"
 #include "hw/pci/pci.h"
 #include "hw/pci-host/gpex.h"
 #include "hw/display/ramfb.h"
@@ -106,7 +107,7 @@ static MemMapEntry virt_high_pcie_memmap;
 
 #define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
 
-static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
+static DeviceState *virt_flash_create1(RISCVVirtState *s,
                                        const char *name,
                                        const char *alias_prop_name)
 {
@@ -130,7 +131,7 @@ static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
     object_property_add_alias(OBJECT(s), alias_prop_name,
                               OBJECT(dev), "drive");
 
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 static void virt_flash_create(RISCVVirtState *s)
@@ -139,7 +140,7 @@ static void virt_flash_create(RISCVVirtState *s)
     s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
 }
 
-static void virt_flash_map1(PFlashCFI01 *flash,
+static void virt_flash_map1(DeviceState *flash,
                             hwaddr base, hwaddr size,
                             MemoryRegion *sysmem)
 {
@@ -1517,7 +1518,7 @@ static void virt_machine_init(MachineState *machine)
 
     for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
         /* Map legacy -drive if=pflash to machine properties */
-        pflash_cfi01_legacy_drive(DEVICE(s->flash[i]),
+        pflash_cfi01_legacy_drive(s->flash[i],
                                   drive_get(IF_PFLASH, 0, i));
     }
     virt_flash_map(s, system_memory);
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index be4ab8fe7f..b700a46763 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -21,7 +21,6 @@
 
 #include "hw/riscv/riscv_hart.h"
 #include "hw/sysbus.h"
-#include "hw/block/flash.h"
 #include "qom/object.h"
 
 #define VIRT_CPUS_MAX_BITS             9
@@ -49,7 +48,7 @@ struct RISCVVirtState {
     DeviceState *platform_bus_dev;
     RISCVHartArrayState soc[VIRT_SOCKETS_MAX];
     DeviceState *irqchip[VIRT_SOCKETS_MAX];
-    PFlashCFI01 *flash[2];
+    DeviceState *flash[2];
     FWCfgState *fw_cfg;
 
     int fdt_size;
-- 
2.38.1

Re: [PATCH 06/20] hw/riscv: Use generic DeviceState instead of PFlashCFI01

[Daniel Henrique Barboza]

On 1/4/23 19:04, Philippe Mathieu-Daudé wrote:
> Nothing here requires access to PFlashCFI01 internal fields:
> use the inherited generic DeviceState.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>


>   hw/riscv/virt.c         | 9 +++++----
>   include/hw/riscv/virt.h | 3 +--
>   2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 400bd9329f..b421a9dc12 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -46,6 +46,7 @@
>   #include "sysemu/sysemu.h"
>   #include "sysemu/kvm.h"
>   #include "sysemu/tpm.h"
> +#include "hw/block/flash.h"
>   #include "hw/pci/pci.h"
>   #include "hw/pci-host/gpex.h"
>   #include "hw/display/ramfb.h"
> @@ -106,7 +107,7 @@ static MemMapEntry virt_high_pcie_memmap;
>   
>   #define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
>   
> -static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> +static DeviceState *virt_flash_create1(RISCVVirtState *s,
>                                          const char *name,
>                                          const char *alias_prop_name)
>   {
> @@ -130,7 +131,7 @@ static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
>       object_property_add_alias(OBJECT(s), alias_prop_name,
>                                 OBJECT(dev), "drive");
>   
> -    return PFLASH_CFI01(dev);
> +    return dev;
>   }
>   
>   static void virt_flash_create(RISCVVirtState *s)
> @@ -139,7 +140,7 @@ static void virt_flash_create(RISCVVirtState *s)
>       s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
>   }
>   
> -static void virt_flash_map1(PFlashCFI01 *flash,
> +static void virt_flash_map1(DeviceState *flash,
>                               hwaddr base, hwaddr size,
>                               MemoryRegion *sysmem)
>   {
> @@ -1517,7 +1518,7 @@ static void virt_machine_init(MachineState *machine)
>   
>       for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
>           /* Map legacy -drive if=pflash to machine properties */
> -        pflash_cfi01_legacy_drive(DEVICE(s->flash[i]),
> +        pflash_cfi01_legacy_drive(s->flash[i],
>                                     drive_get(IF_PFLASH, 0, i));
>       }
>       virt_flash_map(s, system_memory);
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index be4ab8fe7f..b700a46763 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -21,7 +21,6 @@
>   
>   #include "hw/riscv/riscv_hart.h"
>   #include "hw/sysbus.h"
> -#include "hw/block/flash.h"
>   #include "qom/object.h"
>   
>   #define VIRT_CPUS_MAX_BITS             9
> @@ -49,7 +48,7 @@ struct RISCVVirtState {
>       DeviceState *platform_bus_dev;
>       RISCVHartArrayState soc[VIRT_SOCKETS_MAX];
>       DeviceState *irqchip[VIRT_SOCKETS_MAX];
> -    PFlashCFI01 *flash[2];
> +    DeviceState *flash[2];
>       FWCfgState *fw_cfg;
>   
>       int fdt_size;

Re: [PATCH 06/20] hw/riscv: Use generic DeviceState instead of PFlashCFI01

[Bin Meng]

On Thu, Jan 5, 2023 at 6:47 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Nothing here requires access to PFlashCFI01 internal fields:
> use the inherited generic DeviceState.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/riscv/virt.c         | 9 +++++----
>  include/hw/riscv/virt.h | 3 +--
>  2 files changed, 6 insertions(+), 6 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 07/20] hw/i386: Use generic DeviceState instead of PFlashCFI01

[Philippe Mathieu-Daudé]

Nothing here requires access to PFlashCFI01 internal fields:
use the inherited generic DeviceState.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_sysfw.c   | 14 +++++++-------
 include/hw/i386/pc.h |  3 +--
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 60db0efb41..1a12207dd1 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -71,7 +71,7 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
     memory_region_set_readonly(isa_bios, true);
 }
 
-static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms,
+static DeviceState *pc_pflash_create(PCMachineState *pcms,
                                      const char *name,
                                      const char *alias_prop_name)
 {
@@ -88,7 +88,7 @@ static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms,
      * will be removed with object_unparent.
      */
     object_unref(OBJECT(dev));
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 void pc_system_flash_create(PCMachineState *pcms)
@@ -143,7 +143,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
     int i;
     BlockBackend *blk;
     int64_t size;
-    PFlashCFI01 *system_flash;
+    DeviceState *system_flash;
     MemoryRegion *flash_mem;
     void *flash_ptr;
     int flash_size;
@@ -152,7 +152,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
 
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
         system_flash = pcms->flash[i];
-        blk = pflash_cfi01_get_blk(DEVICE(system_flash));
+        blk = pflash_cfi01_get_blk(system_flash);
         if (!blk) {
             break;
         }
@@ -187,7 +187,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
                         0x100000000ULL - total_size);
 
         if (i == 0) {
-            flash_mem = pflash_cfi01_get_memory(DEVICE(system_flash));
+            flash_mem = pflash_cfi01_get_memory(system_flash);
             pc_isa_bios_init(rom_memory, flash_mem, size);
 
             /* Encrypt the pflash boot ROM */
@@ -214,9 +214,9 @@ void pc_system_firmware_init(PCMachineState *pcms,
 
     /* Map legacy -drive if=pflash to machine properties */
     for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
-        pflash_cfi01_legacy_drive(DEVICE(pcms->flash[i]),
+        pflash_cfi01_legacy_drive(pcms->flash[i],
                                   drive_get(IF_PFLASH, 0, i));
-        pflash_blk[i] = pflash_cfi01_get_blk(DEVICE(pcms->flash[i]));
+        pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]);
     }
 
     /* Reject gaps */
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 991f905f5d..70abe61805 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -6,7 +6,6 @@
 #include "qemu/uuid.h"
 #include "hw/boards.h"
 #include "hw/block/fdc.h"
-#include "hw/block/flash.h"
 #include "hw/i386/x86.h"
 
 #include "hw/acpi/acpi_dev_interface.h"
@@ -35,7 +34,7 @@ typedef struct PCMachineState {
     /* Pointers to devices and objects: */
     PCIBus *bus;
     I2CBus *smbus;
-    PFlashCFI01 *flash[2];
+    DeviceState *flash[2];
     ISADevice *pcspk;
     DeviceState *iommu;
 
-- 
2.38.1

Re: [PATCH 07/20] hw/i386: Use generic DeviceState instead of PFlashCFI01

[Bin Meng]

On Thu, Jan 5, 2023 at 6:53 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Nothing here requires access to PFlashCFI01 internal fields:
> use the inherited generic DeviceState.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/i386/pc_sysfw.c   | 14 +++++++-------
>  include/hw/i386/pc.h |  3 +--
>  2 files changed, 8 insertions(+), 9 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 08/20] hw/xtensa: Use generic DeviceState instead of PFlashCFI01

[Philippe Mathieu-Daudé]

Nothing here requires access to PFlashCFI01 internal fields:
use the inherited generic DeviceState.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/xtensa/xtfpga.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index bce3a543b0..b039416fde 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -165,7 +165,7 @@ static void xtfpga_net_init(MemoryRegion *address_space,
     memory_region_add_subregion(address_space, buffers, ram);
 }
 
-static PFlashCFI01 *xtfpga_flash_init(MemoryRegion *address_space,
+static DeviceState *xtfpga_flash_init(MemoryRegion *address_space,
                                       const XtfpgaBoardDesc *board,
                                       DriveInfo *dinfo, int be)
 {
@@ -183,7 +183,7 @@ static PFlashCFI01 *xtfpga_flash_init(MemoryRegion *address_space,
     sysbus_realize_and_unref(s, &error_fatal);
     memory_region_add_subregion(address_space, board->flash->base,
                                 sysbus_mmio_get_region(s, 0));
-    return PFLASH_CFI01(dev);
+    return dev;
 }
 
 static uint64_t translate_phys_addr(void *opaque, uint64_t addr)
@@ -231,7 +231,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
     XtensaMxPic *mx_pic = NULL;
     qemu_irq *extints;
     DriveInfo *dinfo;
-    PFlashCFI01 *flash = NULL;
+    DeviceState *flash = NULL;
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *dtb_filename = machine->dtb;
@@ -459,7 +459,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board, MachineState *machine)
         }
     } else {
         if (flash) {
-            MemoryRegion *flash_mr = pflash_cfi01_get_memory(DEVICE(flash));
+            MemoryRegion *flash_mr = pflash_cfi01_get_memory(flash);
             MemoryRegion *flash_io = g_malloc(sizeof(*flash_io));
             uint32_t size = env->config->sysrom.location[0].size;
 
-- 
2.38.1

Re: [PATCH 08/20] hw/xtensa: Use generic DeviceState instead of PFlashCFI01

[Bin Meng]

On Thu, Jan 5, 2023 at 6:37 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Nothing here requires access to PFlashCFI01 internal fields:
> use the inherited generic DeviceState.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/xtensa/xtfpga.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 09/20] hw/block: Factor pflash_cfi01_create() out of pflash_cfi01_register()

[Philippe Mathieu-Daudé]

Currently pflash_cfi01_register():

 1/ creates a TYPE_PFLASH_CFI01 qdev instance
 2/ maps the first MMIO region to the system bus

The first minor issue is the implicit sysbus mapping is not
obvious (the function name could mention it), and the function
is not documented.

Another issue is we are forced to map on sysbus, thus code
wanting to simply instantiate this device are forced to open
code the qdev creation.

This is a problem in a heterogeneous system where not all cores
has access to the sysbus, or if we want to map the pflash on
different address spaces.

To clarify this API, extract the qdev creation in a new helper
named pflash_cfi01_create().

We don't document pflash_cfi01_register() because we are going
to remove it in a few commits.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/pflash_cfi01.c  | 34 +++++++++++++++++++++++++---------
 include/hw/block/flash.h | 14 +++++++++++++-
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 866ea596ea..6a8f9e6319 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -953,15 +953,13 @@ static void pflash_cfi01_register_types(void)
 
 type_init(pflash_cfi01_register_types)
 
-PFlashCFI01 *pflash_cfi01_register(hwaddr base,
-                                   const char *name,
-                                   hwaddr size,
-                                   BlockBackend *blk,
-                                   uint32_t sector_len,
-                                   int bank_width,
-                                   uint16_t id0, uint16_t id1,
-                                   uint16_t id2, uint16_t id3,
-                                   int be)
+DeviceState *pflash_cfi01_create(const char *name,
+                                 hwaddr size,
+                                 BlockBackend *blk, uint32_t sector_len,
+                                 int bank_width,
+                                 uint16_t id0, uint16_t id1,
+                                 uint16_t id2, uint16_t id3,
+                                 int be)
 {
     DeviceState *dev = qdev_new(TYPE_PFLASH_CFI01);
 
@@ -980,7 +978,25 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
     qdev_prop_set_string(dev, "name", name);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
+    return dev;
+}
+
+PFlashCFI01 *pflash_cfi01_register(hwaddr base,
+                                   const char *name,
+                                   hwaddr size,
+                                   BlockBackend *blk,
+                                   uint32_t sector_len,
+                                   int bank_width,
+                                   uint16_t id0, uint16_t id1,
+                                   uint16_t id2, uint16_t id3,
+                                   int be)
+{
+    DeviceState *dev;
+
+    dev = pflash_cfi01_create(name, size, blk, sector_len, bank_width,
+                              id0, id1, id2, id3, be);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+
     return PFLASH_CFI01(dev);
 }
 
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 25affdf7a5..40ba857f69 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -11,7 +11,19 @@
 #define TYPE_PFLASH_CFI01 "cfi.pflash01"
 OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI01, PFLASH_CFI01)
 
-
+/**
+ * Create and realize a parallel NOR flash (CFI type 1) on the heap.
+ *
+ * Create the device state structure, initialize it, and drop the
+ * reference to it (the device is realized).
+ */
+DeviceState *pflash_cfi01_create(const char *name,
+                                 hwaddr size,
+                                 BlockBackend *blk, uint32_t sector_len,
+                                 int bank_width,
+                                 uint16_t id0, uint16_t id1,
+                                 uint16_t id2, uint16_t id3,
+                                 int be);
 PFlashCFI01 *pflash_cfi01_register(hwaddr base,
                                    const char *name,
                                    hwaddr size,
-- 
2.38.1

Re: [PATCH 09/20] hw/block: Factor pflash_cfi01_create() out of pflash_cfi01_register()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:16 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Currently pflash_cfi01_register():
>
>  1/ creates a TYPE_PFLASH_CFI01 qdev instance
>  2/ maps the first MMIO region to the system bus
>
> The first minor issue is the implicit sysbus mapping is not
> obvious (the function name could mention it), and the function
> is not documented.
>
> Another issue is we are forced to map on sysbus, thus code
> wanting to simply instantiate this device are forced to open
> code the qdev creation.
>
> This is a problem in a heterogeneous system where not all cores
> has access to the sysbus, or if we want to map the pflash on
> different address spaces.
>
> To clarify this API, extract the qdev creation in a new helper
> named pflash_cfi01_create().
>
> We don't document pflash_cfi01_register() because we are going
> to remove it in a few commits.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/block/pflash_cfi01.c  | 34 +++++++++++++++++++++++++---------
>  include/hw/block/flash.h | 14 +++++++++++++-
>  2 files changed, 38 insertions(+), 10 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 10/20] hw/arm: Open-code pflash_cfi01_register()

[Philippe Mathieu-Daudé]

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 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/arm/collie.c      | 15 +++++++++------
 hw/arm/gumstix.c     | 19 +++++++++++++------
 hw/arm/mainstone.c   | 13 ++++++++-----
 hw/arm/omap_sx1.c    | 22 ++++++++++++++--------
 hw/arm/versatilepb.c | 13 ++++++++-----
 hw/arm/z2.c          | 10 +++++++---
 6 files changed, 59 insertions(+), 33 deletions(-)

diff --git a/hw/arm/collie.c b/hw/arm/collie.c
index 8df31e2793..1fbb1a5773 100644
--- a/hw/arm/collie.c
+++ b/hw/arm/collie.c
@@ -39,6 +39,7 @@ static void collie_init(MachineState *machine)
     DriveInfo *dinfo;
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     CollieMachineState *cms = COLLIE_MACHINE(machine);
+    DeviceState *dev;
 
     if (machine->ram_size != mc->default_ram_size) {
         char *sz = size_to_str(mc->default_ram_size);
@@ -52,14 +53,16 @@ static void collie_init(MachineState *machine)
     memory_region_add_subregion(get_system_memory(), SA_SDCS0, machine->ram);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash_cfi01_register(SA_CS0, "collie.fl1", 0x02000000,
-                    dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                    64 * KiB, 4, 0x00, 0x00, 0x00, 0x00, 0);
+    dev = pflash_cfi01_create("collie.fl1", 0x02000000,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              64 * KiB, 4, 0x00, 0x00, 0x00, 0x00, 0);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, SA_CS0);
 
     dinfo = drive_get(IF_PFLASH, 0, 1);
-    pflash_cfi01_register(SA_CS1, "collie.fl2", 0x02000000,
-                    dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                    64 * KiB, 4, 0x00, 0x00, 0x00, 0x00, 0);
+    dev = pflash_cfi01_create("collie.fl2", 0x02000000,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              64 * KiB, 4, 0x00, 0x00, 0x00, 0x00, 0);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, SA_CS1);
 
     sysbus_create_simple("scoop", 0x40800000, NULL);
 
diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c
index 3a4bc332c4..7b80a7d0a4 100644
--- a/hw/arm/gumstix.c
+++ b/hw/arm/gumstix.c
@@ -40,6 +40,7 @@
 #include "net/net.h"
 #include "hw/block/flash.h"
 #include "hw/net/smc91c111.h"
+#include "hw/sysbus.h"
 #include "hw/boards.h"
 #include "exec/address-spaces.h"
 #include "sysemu/qtest.h"
@@ -51,6 +52,7 @@ static void connex_init(MachineState *machine)
 {
     PXA2xxState *cpu;
     DriveInfo *dinfo;
+    DeviceState *dev;
     MemoryRegion *address_space_mem = get_system_memory();
 
     uint32_t connex_rom = 0x01000000;
@@ -65,12 +67,14 @@ static void connex_init(MachineState *machine)
         exit(1);
     }
 
-    if (!pflash_cfi01_register(0x00000000, "connext.rom", connex_rom,
-                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                               sector_len, 2, 0, 0, 0, 0, 0)) {
+    dev = pflash_cfi01_create("connext.rom", connex_rom,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              sector_len, 2, 0, 0, 0, 0, 0);
+    if (!dev) {
         error_report("Error registering flash memory");
         exit(1);
     }
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000);
 
     /* Interrupt line of NIC is connected to GPIO line 36 */
     smc91c111_init(&nd_table[0], 0x04000300,
@@ -81,6 +85,7 @@ static void verdex_init(MachineState *machine)
 {
     PXA2xxState *cpu;
     DriveInfo *dinfo;
+    DeviceState *dev;
     MemoryRegion *address_space_mem = get_system_memory();
 
     uint32_t verdex_rom = 0x02000000;
@@ -95,12 +100,14 @@ static void verdex_init(MachineState *machine)
         exit(1);
     }
 
-    if (!pflash_cfi01_register(0x00000000, "verdex.rom", verdex_rom,
-                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                               sector_len, 2, 0, 0, 0, 0, 0)) {
+    dev = pflash_cfi01_create("verdex.rom", verdex_rom,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              sector_len, 2, 0, 0, 0, 0, 0);
+    if (!dev) {
         error_report("Error registering flash memory");
         exit(1);
     }
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000);
 
     /* Interrupt line of NIC is connected to GPIO line 99 */
     smc91c111_init(&nd_table[0], 0x04000300,
diff --git a/hw/arm/mainstone.c b/hw/arm/mainstone.c
index 8454b65458..ac34be3709 100644
--- a/hw/arm/mainstone.c
+++ b/hw/arm/mainstone.c
@@ -129,15 +129,18 @@ static void mainstone_common_init(MemoryRegion *address_space_mem,
 
     /* There are two 32MiB flash devices on the board */
     for (i = 0; i < 2; i ++) {
+        DeviceState *dev;
+
         dinfo = drive_get(IF_PFLASH, 0, i);
-        if (!pflash_cfi01_register(mainstone_flash_base[i],
-                                   i ? "mainstone.flash1" : "mainstone.flash0",
-                                   MAINSTONE_FLASH,
-                                   dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                                   sector_len, 4, 0, 0, 0, 0, 0)) {
+        dev = pflash_cfi01_create(i ? "mainstone.flash1" : "mainstone.flash0",
+                                  MAINSTONE_FLASH,
+                                  dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                                  sector_len, 4, 0, 0, 0, 0, 0);
+        if (!dev) {
             error_report("Error registering flash memory");
             exit(1);
         }
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, mainstone_flash_base[i]);
     }
 
     mst_irq = sysbus_create_simple("mainstone-fpga", MST_FPGA_PHYS,
diff --git a/hw/arm/omap_sx1.c b/hw/arm/omap_sx1.c
index 57829b3744..718e50c062 100644
--- a/hw/arm/omap_sx1.c
+++ b/hw/arm/omap_sx1.c
@@ -30,6 +30,7 @@
 #include "ui/console.h"
 #include "hw/arm/omap.h"
 #include "hw/boards.h"
+#include "hw/sysbus.h"
 #include "hw/arm/boot.h"
 #include "hw/block/flash.h"
 #include "sysemu/qtest.h"
@@ -114,6 +115,7 @@ static void sx1_init(MachineState *machine, const int version)
     DriveInfo *dinfo;
     int fl_idx;
     uint32_t flash_size = flash0_size;
+    DeviceState *dev;
 
     if (machine->ram_size != mc->default_ram_size) {
         char *sz = size_to_str(mc->default_ram_size);
@@ -153,10 +155,12 @@ static void sx1_init(MachineState *machine, const int version)
 
     fl_idx = 0;
     if ((dinfo = drive_get(IF_PFLASH, 0, fl_idx)) != NULL) {
-        if (!pflash_cfi01_register(OMAP_CS0_BASE,
-                                   "omap_sx1.flash0-1", flash_size,
-                                   blk_by_legacy_dinfo(dinfo),
-                                   sector_size, 4, 0, 0, 0, 0, 0)) {
+        dev = pflash_cfi01_create("omap_sx1.flash0-1", flash_size,
+                                  blk_by_legacy_dinfo(dinfo),
+                                  sector_size, 4, 0, 0, 0, 0, 0);
+        if (dev) {
+            sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, OMAP_CS0_BASE);
+        } else {
             fprintf(stderr, "qemu: Error registering flash memory %d.\n",
                            fl_idx);
         }
@@ -175,10 +179,12 @@ static void sx1_init(MachineState *machine, const int version)
         memory_region_add_subregion(address_space,
                                 OMAP_CS1_BASE + flash1_size, &cs[1]);
 
-        if (!pflash_cfi01_register(OMAP_CS1_BASE,
-                                   "omap_sx1.flash1-1", flash1_size,
-                                   blk_by_legacy_dinfo(dinfo),
-                                   sector_size, 4, 0, 0, 0, 0, 0)) {
+        dev = pflash_cfi01_create("omap_sx1.flash1-1", flash1_size,
+                                  blk_by_legacy_dinfo(dinfo),
+                                  sector_size, 4, 0, 0, 0, 0, 0);
+        if (dev) {
+            sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, OMAP_CS1_BASE);
+        } else {
             fprintf(stderr, "qemu: Error registering flash memory %d.\n",
                            fl_idx);
         }
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index ecc1f6cf74..c5c7cf6dde 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -385,11 +385,14 @@ static void versatile_init(MachineState *machine, int board_id)
     /* 0x34000000 NOR Flash */
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    if (!pflash_cfi01_register(VERSATILE_FLASH_ADDR, "versatile.flash",
-                          VERSATILE_FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          VERSATILE_FLASH_SECT_SIZE,
-                          4, 0x0089, 0x0018, 0x0000, 0x0, 0)) {
+    dev = pflash_cfi01_create("versatile.flash",
+                              VERSATILE_FLASH_SIZE,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              VERSATILE_FLASH_SECT_SIZE,
+                              4, 0x0089, 0x0018, 0x0000, 0x0, 0);
+    if (dev) {
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, VERSATILE_FLASH_ADDR);
+    } else {
         fprintf(stderr, "qemu: Error registering flash memory.\n");
     }
 
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index 9c1e876207..d28d75aa0f 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -18,6 +18,7 @@
 #include "hw/irq.h"
 #include "hw/ssi/ssi.h"
 #include "migration/vmstate.h"
+#include "hw/sysbus.h"
 #include "hw/boards.h"
 #include "hw/block/flash.h"
 #include "ui/console.h"
@@ -306,17 +307,20 @@ static void z2_init(MachineState *machine)
     void *z2_lcd;
     I2CBus *bus;
     DeviceState *wm;
+    DeviceState *dev;
 
     /* Setup CPU & memory */
     mpu = pxa270_init(address_space_mem, z2_binfo.ram_size, machine->cpu_type);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    if (!pflash_cfi01_register(Z2_FLASH_BASE, "z2.flash0", Z2_FLASH_SIZE,
-                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                               sector_len, 4, 0, 0, 0, 0, 0)) {
+    dev = pflash_cfi01_create("z2.flash0", Z2_FLASH_SIZE,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              sector_len, 4, 0, 0, 0, 0, 0);
+    if (!dev) {
         error_report("Error registering flash memory");
         exit(1);
     }
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, Z2_FLASH_BASE);
 
     /* setup keypad */
     pxa27x_register_keypad(mpu->kp, map, 0x100);
-- 
2.38.1

Re: [PATCH 10/20] hw/arm: Open-code pflash_cfi01_register()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:43 AM Philippe Mathieu-Daudé <philmd@linaro.org> 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 to remove pflash_cfi01_register() from the pflash API,

duplicated "to remove"

> 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/collie.c      | 15 +++++++++------
>  hw/arm/gumstix.c     | 19 +++++++++++++------
>  hw/arm/mainstone.c   | 13 ++++++++-----
>  hw/arm/omap_sx1.c    | 22 ++++++++++++++--------
>  hw/arm/versatilepb.c | 13 ++++++++-----
>  hw/arm/z2.c          | 10 +++++++---
>  6 files changed, 59 insertions(+), 33 deletions(-)
>

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 11/20] hw/microblaze: Open-code pflash_cfi01_register()

[Philippe Mathieu-Daudé]

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 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/microblaze/petalogix_ml605_mmu.c      | 8 ++++----
 hw/microblaze/petalogix_s3adsp1800_mmu.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index a24fadddca..d5ff71218d 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -105,10 +105,10 @@ petalogix_ml605_init(MachineState *machine)
     dinfo = drive_get(IF_PFLASH, 0, 0);
     /* 5th parameter 2 means bank-width
      * 10th paremeter 0 means little-endian */
-    pflash_cfi01_register(FLASH_BASEADDR, "petalogix_ml605.flash", FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          64 * KiB, 2, 0x89, 0x18, 0x0000, 0x0, 0);
-
+    dev = pflash_cfi01_create("petalogix_ml605.flash", FLASH_SIZE,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              64 * KiB, 2, 0x89, 0x18, 0x0000, 0x0, 0);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, FLASH_BASEADDR);
 
     dev = qdev_new("xlnx.xps-intc");
     qdev_prop_set_uint32(dev, "kind-of-intr", 1 << TIMER_IRQ);
diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
index 9d959d1ad8..426ff1de93 100644
--- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
+++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
@@ -84,10 +84,10 @@ petalogix_s3adsp1800_init(MachineState *machine)
     memory_region_add_subregion(sysmem, ddr_base, phys_ram);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash_cfi01_register(FLASH_BASEADDR,
-                          "petalogix_s3adsp1800.flash", FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1);
+    dev = pflash_cfi01_create("petalogix_s3adsp1800.flash", FLASH_SIZE,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, FLASH_BASEADDR);
 
     dev = qdev_new("xlnx.xps-intc");
     qdev_prop_set_uint32(dev, "kind-of-intr",
-- 
2.38.1

Re: [PATCH 11/20] hw/microblaze: Open-code pflash_cfi01_register()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:37 AM Philippe Mathieu-Daudé <philmd@linaro.org> 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 to remove pflash_cfi01_register() from the pflash API,

duplicated "to remove"

> open-code it as a qdev creation call followed by an explicit
> sysbus mapping.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/microblaze/petalogix_ml605_mmu.c      | 8 ++++----
>  hw/microblaze/petalogix_s3adsp1800_mmu.c | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
>

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 12/20] hw/mips: Open-code pflash_cfi01_register()

[Philippe Mathieu-Daudé]

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 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/mips/malta.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index 43fbb97799..e690f13bdb 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -1222,7 +1222,6 @@ void mips_malta_init(MachineState *machine)
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
     char *filename;
-    PFlashCFI01 *fl;
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *ram_low_preio = g_new(MemoryRegion, 1);
     MemoryRegion *ram_low_postio;
@@ -1286,12 +1285,11 @@ void mips_malta_init(MachineState *machine)
 
     /* Load firmware in flash / BIOS. */
     dinfo = drive_get(IF_PFLASH, 0, fl_idx);
-    fl = pflash_cfi01_register(FLASH_ADDRESS, "mips_malta.bios",
-                               FLASH_SIZE,
-                               dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                               65536,
-                               4, 0x0000, 0x0000, 0x0000, 0x0000, be);
-    dev = DEVICE(fl);
+    dev = pflash_cfi01_create("mips_malta.bios", FLASH_SIZE,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              65536, 4,
+                              0x0000, 0x0000, 0x0000, 0x0000, be);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, FLASH_ADDRESS);
     bios = pflash_cfi01_get_memory(dev);
     fl_idx++;
     if (kernel_filename) {
-- 
2.38.1

Re: [PATCH 12/20] hw/mips: Open-code pflash_cfi01_register()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:51 AM Philippe Mathieu-Daudé <philmd@linaro.org> 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 to remove pflash_cfi01_register() from the pflash API,

same comments in previous patches

> open-code it as a qdev creation call followed by an explicit
> sysbus mapping.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/mips/malta.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 13/20] hw/ppc: Open-code pflash_cfi01_register()

[Philippe Mathieu-Daudé]

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 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     | 12 ++++++++----
 hw/ppc/virtex_ml507.c |  7 ++++---
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 4a22ce3761..a82c9e0642 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -82,6 +82,8 @@ struct boot_info {
 
 static int sam460ex_load_uboot(void)
 {
+    DeviceState *dev;
+
     /*
      * This first creates 1MiB of flash memory mapped at the end of
      * the 32-bit address space (0xFFF00000..0xFFFFFFFF).
@@ -103,14 +105,16 @@ static int sam460ex_load_uboot(void)
     DriveInfo *dinfo;
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    if (!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 = pflash_cfi01_create("sam460ex.flash", FLASH_SIZE,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1);
+    if (!dev) {
         error_report("Error registering flash memory");
         /* XXX: return an error instead? */
         exit(1);
     }
+    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..ee211664a8 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -233,9 +233,10 @@ 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 = pflash_cfi01_create("virtex.flash", FLASH_SIZE,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              64 * KiB, 1, 0x89, 0x18, 0x0000, 0x0, 1);
+    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");
-- 
2.38.1

Re: [PATCH 13/20] hw/ppc: Open-code pflash_cfi01_register()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:49 AM Philippe Mathieu-Daudé <philmd@linaro.org> 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 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     | 12 ++++++++----
>  hw/ppc/virtex_ml507.c |  7 ++++---
>  2 files changed, 12 insertions(+), 7 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 14/20] hw/block: Remove unused pflash_cfi01_register()

[Philippe Mathieu-Daudé]

We converted all caller of pflash_cfi01_register() by open
coding a call to pflash_cfi01_create() followed by an explicit
call to sysbus_mmio_map(); we can now remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/pflash_cfi01.c  | 19 -------------------
 include/hw/block/flash.h |  9 ---------
 2 files changed, 28 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 6a8f9e6319..9df79b102b 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -981,25 +981,6 @@ DeviceState *pflash_cfi01_create(const char *name,
     return dev;
 }
 
-PFlashCFI01 *pflash_cfi01_register(hwaddr base,
-                                   const char *name,
-                                   hwaddr size,
-                                   BlockBackend *blk,
-                                   uint32_t sector_len,
-                                   int bank_width,
-                                   uint16_t id0, uint16_t id1,
-                                   uint16_t id2, uint16_t id3,
-                                   int be)
-{
-    DeviceState *dev;
-
-    dev = pflash_cfi01_create(name, size, blk, sector_len, bank_width,
-                              id0, id1, id2, id3, be);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
-
-    return PFLASH_CFI01(dev);
-}
-
 BlockBackend *pflash_cfi01_get_blk(DeviceState *dev)
 {
     PFlashCFI01 *fl = PFLASH_CFI01(dev);
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 40ba857f69..858c0a1b6e 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -24,15 +24,6 @@ DeviceState *pflash_cfi01_create(const char *name,
                                  uint16_t id0, uint16_t id1,
                                  uint16_t id2, uint16_t id3,
                                  int be);
-PFlashCFI01 *pflash_cfi01_register(hwaddr base,
-                                   const char *name,
-                                   hwaddr size,
-                                   BlockBackend *blk,
-                                   uint32_t sector_len,
-                                   int width,
-                                   uint16_t id0, uint16_t id1,
-                                   uint16_t id2, uint16_t id3,
-                                   int be);
 BlockBackend *pflash_cfi01_get_blk(DeviceState *dev);
 MemoryRegion *pflash_cfi01_get_memory(DeviceState *dev);
 void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo);
-- 
2.38.1

Re: [PATCH 14/20] hw/block: Remove unused pflash_cfi01_register()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:48 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> We converted all caller of pflash_cfi01_register() by open
> coding a call to pflash_cfi01_create() followed by an explicit
> call to sysbus_mmio_map(); we can now remove it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/block/pflash_cfi01.c  | 19 -------------------
>  include/hw/block/flash.h |  9 ---------
>  2 files changed, 28 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 15/20] hw/block: Make PFlashCFI01 QOM declaration internal

[Philippe Mathieu-Daudé]

Convert the QOM PFlashCFI01 to a forward/opaque pointer declaration.
Only pflash_cfi01.c is able to poke at the internal fields.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/pflash_cfi01.c  | 2 ++
 include/hw/block/flash.h | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 9df79b102b..2af9273fc1 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -59,6 +59,8 @@
 #define PFLASH_BE          0
 #define PFLASH_SECURE      1
 
+OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI01, PFLASH_CFI01)
+
 struct PFlashCFI01 {
     /*< private >*/
     SysBusDevice parent_obj;
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 858c0a1b6e..321aede8ef 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -9,7 +9,6 @@
 /* pflash_cfi01.c */
 
 #define TYPE_PFLASH_CFI01 "cfi.pflash01"
-OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI01, PFLASH_CFI01)
 
 /**
  * Create and realize a parallel NOR flash (CFI type 1) on the heap.
-- 
2.38.1

Re: [PATCH 15/20] hw/block: Make PFlashCFI01 QOM declaration internal

[Bin Meng]

On Thu, Jan 5, 2023 at 6:52 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Convert the QOM PFlashCFI01 to a forward/opaque pointer declaration.
> Only pflash_cfi01.c is able to poke at the internal fields.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/block/pflash_cfi01.c  | 2 ++
>  include/hw/block/flash.h | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 16/20] hw/block: Factor pflash_cfi02_create() out of pflash_cfi02_register()

[Philippe Mathieu-Daudé]

Currently pflash_cfi02_register():

 1/ creates a TYPE_PFLASH_CFI02 qdev instance
 2/ maps the first MMIO region to the system bus

The first minor issue is the implicit sysbus mapping is not
obvious (the function name could mention it), and the function
is not documented.

Another issue is we are forced to map on sysbus, thus code
wanting to simply instantiate this device are forced to open
code the qdev creation.

This is a problem in a heterogeneous system where not all cores
has access to the sysbus, or if we want to map the pflash on
different address spaces.

To clarify this API, extract the qdev creation in a new helper
named pflash_cfi02_create().

We don't document pflash_cfi02_register() because we are going
to remove it in a pair of commits.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/pflash_cfi02.c  | 55 ++++++++++++++++++++++++++--------------
 include/hw/block/flash.h | 14 +++++++++-
 2 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 2a99b286b0..176f93b512 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -994,6 +994,37 @@ static void pflash_cfi02_register_types(void)
 
 type_init(pflash_cfi02_register_types)
 
+DeviceState *pflash_cfi02_create(const char *name, hwaddr size,
+                                 BlockBackend *blk, uint32_t sector_len,
+                                 int nb_mappings, int bank_width,
+                                 uint16_t id0, uint16_t id1,
+                                 uint16_t id2, uint16_t id3,
+                                 uint16_t unlock_addr0, uint16_t unlock_addr1,
+                                 int be)
+{
+    DeviceState *dev = qdev_new(TYPE_PFLASH_CFI02);
+
+    if (blk) {
+        qdev_prop_set_drive(dev, "drive", blk);
+    }
+    assert(QEMU_IS_ALIGNED(size, sector_len));
+    qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
+    qdev_prop_set_uint32(dev, "sector-length", sector_len);
+    qdev_prop_set_uint8(dev, "width", bank_width);
+    qdev_prop_set_uint8(dev, "mappings", nb_mappings);
+    qdev_prop_set_uint8(dev, "big-endian", !!be);
+    qdev_prop_set_uint16(dev, "id0", id0);
+    qdev_prop_set_uint16(dev, "id1", id1);
+    qdev_prop_set_uint16(dev, "id2", id2);
+    qdev_prop_set_uint16(dev, "id3", id3);
+    qdev_prop_set_uint16(dev, "unlock-addr0", unlock_addr0);
+    qdev_prop_set_uint16(dev, "unlock-addr1", unlock_addr1);
+    qdev_prop_set_string(dev, "name", name);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+
+    return dev;
+}
+
 PFlashCFI02 *pflash_cfi02_register(hwaddr base,
                                    const char *name,
                                    hwaddr size,
@@ -1006,26 +1037,12 @@ PFlashCFI02 *pflash_cfi02_register(hwaddr base,
                                    uint16_t unlock_addr1,
                                    int be)
 {
-    DeviceState *dev = qdev_new(TYPE_PFLASH_CFI02);
-
-    if (blk) {
-        qdev_prop_set_drive(dev, "drive", blk);
-    }
-    assert(QEMU_IS_ALIGNED(size, sector_len));
-    qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
-    qdev_prop_set_uint32(dev, "sector-length", sector_len);
-    qdev_prop_set_uint8(dev, "width", width);
-    qdev_prop_set_uint8(dev, "mappings", nb_mappings);
-    qdev_prop_set_uint8(dev, "big-endian", !!be);
-    qdev_prop_set_uint16(dev, "id0", id0);
-    qdev_prop_set_uint16(dev, "id1", id1);
-    qdev_prop_set_uint16(dev, "id2", id2);
-    qdev_prop_set_uint16(dev, "id3", id3);
-    qdev_prop_set_uint16(dev, "unlock-addr0", unlock_addr0);
-    qdev_prop_set_uint16(dev, "unlock-addr1", unlock_addr1);
-    qdev_prop_set_string(dev, "name", name);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    DeviceState *dev;
 
+    dev = pflash_cfi02_create(name, size, blk, sector_len,
+                              nb_mappings, width, id0, id1, id2, id3,
+                              unlock_addr0, unlock_addr1, be);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+
     return PFLASH_CFI02(dev);
 }
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 321aede8ef..78b078955e 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -32,7 +32,19 @@ void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo);
 #define TYPE_PFLASH_CFI02 "cfi.pflash02"
 OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI02, PFLASH_CFI02)
 
-
+/**
+ * Create and realize a parallel NOR flash (CFI type 2) on the heap.
+ *
+ * Create the device state structure, initialize it, and drop the
+ * reference to it (the device is realized).
+ */
+DeviceState *pflash_cfi02_create(const char *name, hwaddr size,
+                                 BlockBackend *blk, uint32_t sector_len,
+                                 int nb_mappings, int bank_width,
+                                 uint16_t id0, uint16_t id1,
+                                 uint16_t id2, uint16_t id3,
+                                 uint16_t unlock_addr0, uint16_t unlock_addr1,
+                                 int be);
 PFlashCFI02 *pflash_cfi02_register(hwaddr base,
                                    const char *name,
                                    hwaddr size,
-- 
2.38.1

Re: [PATCH 16/20] hw/block: Factor pflash_cfi02_create() out of pflash_cfi02_register()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:51 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Currently pflash_cfi02_register():
>
>  1/ creates a TYPE_PFLASH_CFI02 qdev instance
>  2/ maps the first MMIO region to the system bus
>
> The first minor issue is the implicit sysbus mapping is not
> obvious (the function name could mention it), and the function
> is not documented.
>
> Another issue is we are forced to map on sysbus, thus code
> wanting to simply instantiate this device are forced to open
> code the qdev creation.
>
> This is a problem in a heterogeneous system where not all cores
> has access to the sysbus, or if we want to map the pflash on
> different address spaces.
>
> To clarify this API, extract the qdev creation in a new helper
> named pflash_cfi02_create().
>
> We don't document pflash_cfi02_register() because we are going
> to remove it in a pair of commits.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/block/pflash_cfi02.c  | 55 ++++++++++++++++++++++++++--------------
>  include/hw/block/flash.h | 14 +++++++++-
>  2 files changed, 49 insertions(+), 20 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 17/20] hw/arm: Open-code pflash_cfi02_register()

[Philippe Mathieu-Daudé]

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 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/digic_boards.c | 14 ++++++++------
 hw/arm/musicpal.c     | 13 +++++++------
 hw/arm/xilinx_zynq.c  | 10 +++++-----
 3 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/hw/arm/digic_boards.c b/hw/arm/digic_boards.c
index 4093af09cb..98b0002d16 100644
--- a/hw/arm/digic_boards.c
+++ b/hw/arm/digic_boards.c
@@ -115,13 +115,15 @@ static void digic4_add_k8p3215uqb_rom(DigicState *s, hwaddr addr,
 {
 #define FLASH_K8P3215UQB_SIZE (4 * 1024 * 1024)
 #define FLASH_K8P3215UQB_SECTOR_SIZE (64 * 1024)
+    DeviceState *dev;
 
-    pflash_cfi02_register(addr, "pflash", FLASH_K8P3215UQB_SIZE,
-                          NULL, FLASH_K8P3215UQB_SECTOR_SIZE,
-                          DIGIC4_ROM_MAX_SIZE / FLASH_K8P3215UQB_SIZE,
-                          4,
-                          0x00EC, 0x007E, 0x0003, 0x0001,
-                          0x0555, 0x2aa, 0);
+    dev = pflash_cfi02_create("pflash", FLASH_K8P3215UQB_SIZE,
+                              NULL, FLASH_K8P3215UQB_SECTOR_SIZE,
+                              DIGIC4_ROM_MAX_SIZE / FLASH_K8P3215UQB_SIZE,
+                              4,
+                              0x00EC, 0x007E, 0x0003, 0x0001,
+                              0x0555, 0x2aa, 0);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
 
     digic_load_rom(s, addr, FLASH_K8P3215UQB_SIZE, filename);
 }
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index b65c020115..9f75d69b7f 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1275,12 +1275,13 @@ static void musicpal_init(MachineState *machine)
          * 0xFF800000 (if there is 8 MB flash). So remap flash access if the
          * image is smaller than 32 MB.
          */
-        pflash_cfi02_register(0x100000000ULL - MP_FLASH_SIZE_MAX,
-                              "musicpal.flash", flash_size,
-                              blk, 0x10000,
-                              MP_FLASH_SIZE_MAX / flash_size,
-                              2, 0x00BF, 0x236D, 0x0000, 0x0000,
-                              0x5555, 0x2AAA, 0);
+        dev = pflash_cfi02_create("musicpal.flash", flash_size,
+                                  blk, 0x10000,
+                                  MP_FLASH_SIZE_MAX / flash_size,
+                                  2, 0x00BF, 0x236D, 0x0000, 0x0000,
+                                  0x5555, 0x2AAA, 0);
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
+                        0x100000000ULL - MP_FLASH_SIZE_MAX);
     }
     sysbus_create_simple(TYPE_MV88W8618_FLASHCFG, MP_FLASHCFG_BASE, NULL);
 
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 3190cc0b8d..e55aff5532 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -218,11 +218,11 @@ 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 = pflash_cfi02_create("zynq.pflash", FLASH_SIZE,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              FLASH_SECTOR_SIZE, 1, 1,
+                              0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa, 0);
+    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));
-- 
2.38.1

Re: [PATCH 17/20] hw/arm: Open-code pflash_cfi02_register()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:48 AM 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 to remove pflash_cfi02_register() from the pflash API,

same issue as the cfi 01 commits

> 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/digic_boards.c | 14 ++++++++------
>  hw/arm/musicpal.c     | 13 +++++++------
>  hw/arm/xilinx_zynq.c  | 10 +++++-----
>  3 files changed, 20 insertions(+), 17 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 18/20] hw/sh4: Open-code pflash_cfi02_register()

[Philippe Mathieu-Daudé]

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 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/sh4/r2d.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index 39fc4f19d9..43a8c56d14 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -302,10 +302,11 @@ static void r2d_init(MachineState *machine)
      * addressable in words of 16bit.
      */
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash_cfi02_register(0x0, "r2d.flash", FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          64 * KiB, 1, 2, 0x0001, 0x227e, 0x2220, 0x2200,
-                          0x555, 0x2aa, 0);
+    dev = pflash_cfi02_create("r2d.flash", FLASH_SIZE,
+                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+                              64 * KiB, 1, 2, 0x0001, 0x227e, 0x2220, 0x2200,
+                              0x555, 0x2aa, 0);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000);
 
     /* NIC: rtl8139 on-board, and 2 slots. */
     for (i = 0; i < nb_nics; i++)
-- 
2.38.1

Re: [PATCH 18/20] hw/sh4: Open-code pflash_cfi02_register()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:47 AM 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 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/sh4/r2d.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 19/20] hw/block: Remove unused pflash_cfi02_register()

[Philippe Mathieu-Daudé]

We converted all caller of pflash_cfi02_register() by open
coding a call to pflash_cfi02_create() followed by an explicit
call to sysbus_mmio_map(); we can now remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/pflash_cfi02.c  | 22 ----------------------
 include/hw/block/flash.h | 12 ------------
 2 files changed, 34 deletions(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 176f93b512..a9dcabdeb2 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -1024,25 +1024,3 @@ DeviceState *pflash_cfi02_create(const char *name, hwaddr size,
 
     return dev;
 }
-
-PFlashCFI02 *pflash_cfi02_register(hwaddr base,
-                                   const char *name,
-                                   hwaddr size,
-                                   BlockBackend *blk,
-                                   uint32_t sector_len,
-                                   int nb_mappings, int width,
-                                   uint16_t id0, uint16_t id1,
-                                   uint16_t id2, uint16_t id3,
-                                   uint16_t unlock_addr0,
-                                   uint16_t unlock_addr1,
-                                   int be)
-{
-    DeviceState *dev;
-
-    dev = pflash_cfi02_create(name, size, blk, sector_len,
-                              nb_mappings, width, id0, id1, id2, id3,
-                              unlock_addr0, unlock_addr1, be);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
-
-    return PFLASH_CFI02(dev);
-}
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 78b078955e..64ee40c561 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -45,18 +45,6 @@ DeviceState *pflash_cfi02_create(const char *name, hwaddr size,
                                  uint16_t id2, uint16_t id3,
                                  uint16_t unlock_addr0, uint16_t unlock_addr1,
                                  int be);
-PFlashCFI02 *pflash_cfi02_register(hwaddr base,
-                                   const char *name,
-                                   hwaddr size,
-                                   BlockBackend *blk,
-                                   uint32_t sector_len,
-                                   int nb_mappings,
-                                   int width,
-                                   uint16_t id0, uint16_t id1,
-                                   uint16_t id2, uint16_t id3,
-                                   uint16_t unlock_addr0,
-                                   uint16_t unlock_addr1,
-                                   int be);
 
 /* nand.c */
 DeviceState *nand_init(BlockBackend *blk, int manf_id, int chip_id);
-- 
2.38.1

Re: [PATCH 19/20] hw/block: Remove unused pflash_cfi02_register()

[Bin Meng]

On Thu, Jan 5, 2023 at 6:28 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> We converted all caller of pflash_cfi02_register() by open
> coding a call to pflash_cfi02_create() followed by an explicit
> call to sysbus_mmio_map(); we can now remove it.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/block/pflash_cfi02.c  | 22 ----------------------
>  include/hw/block/flash.h | 12 ------------
>  2 files changed, 34 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[PATCH 20/20] hw/block: Make PFlashCFI02 QOM declaration internal

[Philippe Mathieu-Daudé]

Convert the QOM PFlashCFI02 to a forward/opaque pointer declaration.
Only pflash_cfi02.c is able to poke at the internal fields.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/block/pflash_cfi02.c  | 2 ++
 include/hw/block/flash.h | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index a9dcabdeb2..90b5feb36c 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -63,6 +63,8 @@ enum {
     WCYCLE_AUTOSELECT_CFI   = 8,
 };
 
+OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI02, PFLASH_CFI02)
+
 struct PFlashCFI02 {
     /*< private >*/
     SysBusDevice parent_obj;
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 64ee40c561..aefbaa9493 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -30,7 +30,6 @@ void pflash_cfi01_legacy_drive(DeviceState *dev, DriveInfo *dinfo);
 /* pflash_cfi02.c */
 
 #define TYPE_PFLASH_CFI02 "cfi.pflash02"
-OBJECT_DECLARE_SIMPLE_TYPE(PFlashCFI02, PFLASH_CFI02)
 
 /**
  * Create and realize a parallel NOR flash (CFI type 2) on the heap.
-- 
2.38.1

Re: [PATCH 20/20] hw/block: Make PFlashCFI02 QOM declaration internal

[Bin Meng]

On Thu, Jan 5, 2023 at 6:50 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Convert the QOM PFlashCFI02 to a forward/opaque pointer declaration.
> Only pflash_cfi02.c is able to poke at the internal fields.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/block/pflash_cfi02.c  | 2 ++
>  include/hw/block/flash.h | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 00/20] hw: Remove implicit sysbus_mmio_map() from pflash APIs

[Peter Maydell]

On Wed, 4 Jan 2023 at 22:04, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Paving the road toward heterogeneous QEMU, the limitations of
> having a single machine sysbus become more apparent.
>
> The sysbus_mmio_map() API forces the caller to map a sysbus
> device to an address on the system bus (system bus here is
> the root MemoryRegion returned by get_system_memory() ).
>
> This is not practical when each core has its own address
> space and group of cores have access to a part of the
> peripherals.
>
> Experimenting with the PFLASH devices. Here the fix is
> quite easy, we split the pflash_cfi_register() -- which
> does the implicit sysbus mapping -- into an explicit qdev
> pflash_cfi_create() followed by the sysbus_mmio_map() call.

pflash_cfi_register() is a legacy convenience function. If
you don't like the sysbus_mmio_map() it does then you can
create, configure, realize and map the device directly.
This is what hw/arm/virt.c does, for instance (it wants to
map the flash devices into either secure or non secure RAM).
(This also lets you embed the device struct into some other
struct if you want rather than using qdev_new(), though
we don't have any code that does that currently.)

thanks
-- PMM

Re: [PATCH 00/20] hw: Remove implicit sysbus_mmio_map() from pflash APIs

[Philippe Mathieu-Daudé]

On 6/1/23 18:51, Peter Maydell wrote:
> On Wed, 4 Jan 2023 at 22:04, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Paving the road toward heterogeneous QEMU, the limitations of
>> having a single machine sysbus become more apparent.
>>
>> The sysbus_mmio_map() API forces the caller to map a sysbus
>> device to an address on the system bus (system bus here is
>> the root MemoryRegion returned by get_system_memory() ).
>>
>> This is not practical when each core has its own address
>> space and group of cores have access to a part of the
>> peripherals.
>>
>> Experimenting with the PFLASH devices. Here the fix is
>> quite easy, we split the pflash_cfi_register() -- which
>> does the implicit sysbus mapping -- into an explicit qdev
>> pflash_cfi_create() followed by the sysbus_mmio_map() call.
> 
> pflash_cfi_register() is a legacy convenience function. If
> you don't like the sysbus_mmio_map() it does then you can
> create, configure, realize and map the device directly.
> This is what hw/arm/virt.c does, for instance (it wants to
> map the flash devices into either secure or non secure RAM).

Good point, thanks!