diff mbox series

[v2,16/21] hw/arm: Open-code pflash_cfi01_register()

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

Commit Message

Philippe Mathieu-Daudé Jan. 9, 2023, 12:08 p.m. UTC 
pflash_cfi01_register() hides an implicit sysbus mapping of
MMIO region #0. This is not practical in a heterogeneous world
where multiple cores use different address spaces. In order to
remove pflash_cfi01_register() from the pflash API, open-code it
as a qdev creation call followed by an explicit sysbus mapping.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/collie.c      | 16 ++++++++++++----
 hw/arm/gumstix.c     | 32 ++++++++++++++++++++++++++------
 hw/arm/mainstone.c   | 19 ++++++++++++++-----
 hw/arm/omap_sx1.c    | 31 +++++++++++++++++++++++--------
 hw/arm/versatilepb.c | 18 +++++++++++++-----
 hw/arm/z2.c          | 17 ++++++++++++++---
 6 files changed, 102 insertions(+), 31 deletions(-)

Comments

Peter Maydell Jan. 13, 2023, 2:03 p.m. UTC | #1 
On Mon, 9 Jan 2023 at 13:13, 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 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      | 16 ++++++++++++----
>  hw/arm/gumstix.c     | 32 ++++++++++++++++++++++++++------
>  hw/arm/mainstone.c   | 19 ++++++++++++++-----
>  hw/arm/omap_sx1.c    | 31 +++++++++++++++++++++++--------
>  hw/arm/versatilepb.c | 18 +++++++++++++-----
>  hw/arm/z2.c          | 17 ++++++++++++++---
>  6 files changed, 102 insertions(+), 31 deletions(-)

When we exand out these uses of pflash_cfi01_register() can
we add a brief todo comment:

/*
 * TODO: we should set device-width to avoid the legacy
 * back-compat behaviour of cfi01. What does the hardware do?
 */

(feel free to edit if you can get it down to 1 line...)

I don't think it's worth trying to track down the right answer
for all these old boards, I just want something so that if
somebody cut-n-pastes this into new board code we can see it
in code review and say "oh, you need to set device-width".

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/arm/collie.c b/hw/arm/collie.c
index 9edff59370..b0183f8ea2 100644
--- a/hw/arm/collie.c
+++ b/hw/arm/collie.c
@@ -11,11 +11,13 @@ 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "qemu/cutils.h"
+#include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "hw/boards.h"
 #include "strongarm.h"
 #include "hw/arm/boot.h"
 #include "hw/block/flash.h"
+#include "hw/qdev-properties.h"
 #include "exec/address-spaces.h"
 #include "cpu.h"
 #include "qom/object.h"
@@ -56,10 +58,16 @@  static void collie_init(MachineState *machine)
 
     for (unsigned i = 0; i < 2; i++) {
         DriveInfo *dinfo = drive_get(IF_PFLASH, 0, i);
-        pflash_cfi01_register(i ? SA_CS1 : SA_CS0,
-                              i ? "collie.fl2" : "collie.fl1", FLASH_SIZE,
-                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                              FLASH_SECTOR_SIZE, 4, 0x00, 0x00, 0x00, 0x00, 0);
+        DeviceState *dev = qdev_new(TYPE_PFLASH_CFI01);
+        qdev_prop_set_string(dev, "name", i ? "collie.fl2" : "collie.fl1");
+        qdev_prop_set_drive(dev, "drive",
+                            dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+        qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint8(dev, "width", 4);
+        qdev_prop_set_bit(dev, "big-endian", false);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, i ? SA_CS1 : SA_CS0);
     }
 
     sysbus_create_simple("scoop", 0x40800000, NULL);
diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c
index 2ca4140c9f..639317394d 100644
--- a/hw/arm/gumstix.c
+++ b/hw/arm/gumstix.c
@@ -37,10 +37,12 @@ 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 #include "hw/arm/pxa.h"
 #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"
@@ -58,6 +60,7 @@  static void connex_init(MachineState *machine)
 {
     PXA2xxState *cpu;
     DriveInfo *dinfo;
+    DeviceState *dev;
 
     cpu = pxa255_init(CONNEX_RAM_SIZE);
 
@@ -69,9 +72,17 @@  static void connex_init(MachineState *machine)
     }
 
     /* Numonyx RC28F128J3F75 */
-    pflash_cfi01_register(0x00000000, "connext.rom", CONNEX_FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          FLASH_SECTOR_SIZE, 2, 0, 0, 0, 0, 0);
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "connext.rom");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks",
+                         CONNEX_FLASH_SIZE / FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 2);
+    qdev_prop_set_bit(dev, "big-endian", false);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    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,
@@ -82,6 +93,7 @@  static void verdex_init(MachineState *machine)
 {
     PXA2xxState *cpu;
     DriveInfo *dinfo;
+    DeviceState *dev;
 
     cpu = pxa270_init(VERDEX_RAM_SIZE, machine->cpu_type);
 
@@ -93,9 +105,17 @@  static void verdex_init(MachineState *machine)
     }
 
     /* Micron RC28F256P30TFA */
-    pflash_cfi01_register(0x00000000, "verdex.rom", VERDEX_FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          FLASH_SECTOR_SIZE, 2, 0, 0, 0, 0, 0);
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "verdex.rom");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks",
+                         VERDEX_FLASH_SIZE / FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 2);
+    qdev_prop_set_bit(dev, "big-endian", false);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    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 68329c4617..b07193a375 100644
--- a/hw/arm/mainstone.c
+++ b/hw/arm/mainstone.c
@@ -129,12 +129,21 @@  static void mainstone_common_init(MachineState *machine,
 
     /* There are two 32MiB flash devices on the board */
     for (i = 0; i < 2; i ++) {
+        DeviceState *dev;
+
         dinfo = drive_get(IF_PFLASH, 0, i);
-        pflash_cfi01_register(mainstone_flash_base[i],
-                              i ? "mainstone.flash1" : "mainstone.flash0",
-                              MAINSTONE_FLASH_SIZE,
-                              dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                              FLASH_SECTOR_SIZE, 4, 0, 0, 0, 0, 0);
+        dev = qdev_new(TYPE_PFLASH_CFI01);
+        qdev_prop_set_string(dev, "name",
+                             i ? "mainstone.flash1" : "mainstone.flash0");
+        qdev_prop_set_drive(dev, "drive",
+                            dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+        qdev_prop_set_uint32(dev, "num-blocks",
+                             MAINSTONE_FLASH_SIZE / FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+        qdev_prop_set_uint8(dev, "width", 4);
+        qdev_prop_set_bit(dev, "big-endian", false);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        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 1d156bc344..7925ddd67e 100644
--- a/hw/arm/omap_sx1.c
+++ b/hw/arm/omap_sx1.c
@@ -31,8 +31,10 @@ 
 #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 "hw/qdev-properties.h"
 #include "sysemu/qtest.h"
 #include "exec/address-spaces.h"
 #include "cpu.h"
@@ -110,6 +112,7 @@  static void sx1_init(MachineState *machine, const int version)
     static uint32_t cs1val = 0x00215070;
     static uint32_t cs2val = 0x00001139;
     static uint32_t cs3val = 0x00001139;
+    DeviceState *dev;
     DriveInfo *dinfo;
     int fl_idx;
     uint32_t flash_size = FLASH0_SIZE;
@@ -152,10 +155,16 @@  static void sx1_init(MachineState *machine, const int version)
 
     fl_idx = 0;
     if ((dinfo = drive_get(IF_PFLASH, 0, fl_idx)) != NULL) {
-        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 = qdev_new(TYPE_PFLASH_CFI01);
+        qdev_prop_set_string(dev, "name", "omap_sx1.flash0-1");
+        qdev_prop_set_drive(dev, "drive",
+                            dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+        qdev_prop_set_uint32(dev, "num-blocks", flash_size / SECTOR_SIZE);
+        qdev_prop_set_uint64(dev, "sector-length", SECTOR_SIZE);
+        qdev_prop_set_uint8(dev, "width", 4);
+        qdev_prop_set_bit(dev, "big-endian", false);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, OMAP_CS0_BASE);
         fl_idx++;
     }
 
@@ -171,10 +180,16 @@  static void sx1_init(MachineState *machine, const int version)
         memory_region_add_subregion(address_space,
                                 OMAP_CS1_BASE + FLASH1_SIZE, &cs[1]);
 
-        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 = qdev_new(TYPE_PFLASH_CFI01);
+        qdev_prop_set_string(dev, "name", "omap_sx1.flash1-1");
+        qdev_prop_set_drive(dev, "drive",
+                            dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+        qdev_prop_set_uint32(dev, "num-blocks", FLASH1_SIZE / SECTOR_SIZE);
+        qdev_prop_set_uint64(dev, "sector-length", SECTOR_SIZE);
+        qdev_prop_set_uint8(dev, "width", 4);
+        qdev_prop_set_bit(dev, "big-endian", false);
+        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+        sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, OMAP_CS1_BASE);
         fl_idx++;
     } else {
         memory_region_init_io(&cs[1], NULL, &static_ops, &cs1val,
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 43172d72ea..e5da688fe4 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -385,11 +385,19 @@  static void versatile_init(MachineState *machine, int board_id)
     /* 0x34000000 NOR Flash */
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    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 = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "versatile.flash");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks",
+                         VERSATILE_FLASH_SIZE / VERSATILE_FLASH_SECT_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", VERSATILE_FLASH_SECT_SIZE);
+    qdev_prop_set_uint8(dev, "width", 4);
+    qdev_prop_set_bit(dev, "big-endian", false);
+    qdev_prop_set_uint16(dev, "id0", 0x0089);
+    qdev_prop_set_uint16(dev, "id1", 0x0018);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, VERSATILE_FLASH_ADDR);
 
     versatile_binfo.ram_size = machine->ram_size;
     versatile_binfo.board_id = board_id;
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index dc25304290..867aef7d87 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -13,14 +13,17 @@ 
 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
+#include "qapi/error.h"
 #include "hw/arm/pxa.h"
 #include "hw/arm/boot.h"
 #include "hw/i2c/i2c.h"
 #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 "hw/qdev-properties.h"
 #include "ui/console.h"
 #include "hw/audio/wm8750.h"
 #include "audio/audio.h"
@@ -307,14 +310,22 @@  static void z2_init(MachineState *machine)
     void *z2_lcd;
     I2CBus *bus;
     DeviceState *wm;
+    DeviceState *dev;
 
     /* Setup CPU & memory */
     mpu = pxa270_init(z2_binfo.ram_size, machine->cpu_type);
 
     dinfo = drive_get(IF_PFLASH, 0, 0);
-    pflash_cfi01_register(Z2_FLASH_BASE, "z2.flash0", Z2_FLASH_SIZE,
-                          dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
-                          FLASH_SECTOR_SIZE, 4, 0, 0, 0, 0, 0);
+    dev = qdev_new(TYPE_PFLASH_CFI01);
+    qdev_prop_set_string(dev, "name", "z2.flash0");
+    qdev_prop_set_drive(dev, "drive",
+                        dinfo ? blk_by_legacy_dinfo(dinfo) : NULL);
+    qdev_prop_set_uint32(dev, "num-blocks", Z2_FLASH_SIZE / FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint64(dev, "sector-length", FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 4);
+    qdev_prop_set_bit(dev, "big-endian", false);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, Z2_FLASH_BASE);
 
     /* setup keypad */
     pxa27x_register_keypad(mpu->kp, map, 0x100);