pflash_cfi01_register() creates a TYPE_CFI_PFLASH01 device, sets
properties, realizes, and wires up.
We have three modified copies of it, because their users need to set
additional properties, or have the wiring done differently.
Factor out their common part into pflash_cfi01_create().
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/arm/vexpress.c | 22 +++++-----------------
hw/arm/virt.c | 26 +++++++++-----------------
hw/block/pflash_cfi01.c | 39 +++++++++++++++++++++++++++------------
hw/xtensa/xtfpga.c | 18 +++++++-----------
include/hw/block/flash.h | 8 ++++++++
5 files changed, 56 insertions(+), 57 deletions(-)
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 00913f2655..b23c63ed24 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -515,26 +515,14 @@ static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
static PFlashCFI01 *ve_pflash_cfi01_register(hwaddr base, const char *name,
DriveInfo *di)
{
- DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
+ DeviceState *dev;
- if (di) {
- qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di),
- &error_abort);
- }
-
- qdev_prop_set_uint32(dev, "num-blocks",
- VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE);
- qdev_prop_set_uint64(dev, "sector-length", VEXPRESS_FLASH_SECT_SIZE);
- qdev_prop_set_uint8(dev, "width", 4);
+ dev = DEVICE(pflash_cfi01_create(name, VEXPRESS_FLASH_SIZE,
+ di ? blk_by_legacy_dinfo(di) : NULL,
+ VEXPRESS_FLASH_SECT_SIZE,
+ 4, 0x89, 0x18, 0x00, 0x00, false));
qdev_prop_set_uint8(dev, "device-width", 2);
- qdev_prop_set_bit(dev, "big-endian", false);
- qdev_prop_set_uint16(dev, "id0", 0x89);
- qdev_prop_set_uint16(dev, "id1", 0x18);
- qdev_prop_set_uint16(dev, "id2", 0x00);
- qdev_prop_set_uint16(dev, "id3", 0x00);
- qdev_prop_set_string(dev, "name", name);
qdev_init_nofail(dev);
-
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
return CFI_PFLASH01(dev);
}
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b7d53b2b87..7787918483 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -48,6 +48,7 @@
#include "exec/address-spaces.h"
#include "qemu/bitops.h"
#include "qemu/error-report.h"
+#include "qemu/units.h"
#include "hw/pci-host/gpex.h"
#include "hw/arm/sysbus-fdt.h"
#include "hw/platform-bus.h"
@@ -875,29 +876,20 @@ static void create_one_flash(const char *name, hwaddr flashbase,
* parameters as the flash devices on the Versatile Express board.
*/
DriveInfo *dinfo = drive_get_next(IF_PFLASH);
- DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
- SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
- const uint64_t sectorlength = 256 * 1024;
+ DeviceState *dev;
+ SysBusDevice *sbd;
- if (dinfo) {
- qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo),
- &error_abort);
- }
+ dev = DEVICE(pflash_cfi01_create(name, flashsize,
+ dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
+ 256 * KiB,
+ 4, 0x89, 0x18, 0x00, 0x00, false));
- qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength);
- qdev_prop_set_uint64(dev, "sector-length", sectorlength);
- qdev_prop_set_uint8(dev, "width", 4);
qdev_prop_set_uint8(dev, "device-width", 2);
- qdev_prop_set_bit(dev, "big-endian", false);
- qdev_prop_set_uint16(dev, "id0", 0x89);
- qdev_prop_set_uint16(dev, "id1", 0x18);
- qdev_prop_set_uint16(dev, "id2", 0x00);
- qdev_prop_set_uint16(dev, "id3", 0x00);
- qdev_prop_set_string(dev, "name", name);
qdev_init_nofail(dev);
+ sbd = SYS_BUS_DEVICE(dev);
memory_region_add_subregion(sysmem, flashbase,
- sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
+ sysbus_mmio_get_region(sbd, 0));
if (file) {
char *fn;
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 2e161f937f..00c2efd0d7 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -920,15 +920,14 @@ 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)
+PFlashCFI01 *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_create(NULL, TYPE_CFI_PFLASH01);
@@ -945,12 +944,28 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
qdev_prop_set_uint16(dev, "id2", id2);
qdev_prop_set_uint16(dev, "id3", id3);
qdev_prop_set_string(dev, "name", name);
- qdev_init_nofail(dev);
-
- sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
return CFI_PFLASH01(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)
+{
+ PFlashCFI01 *dev = pflash_cfi01_create(name, size, blk,
+ sector_len, bank_width,
+ id0, id1, id2, id3, be);
+
+ qdev_init_nofail(DEVICE(dev));
+ sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
+ return dev;
+}
+
MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
{
return &fl->mem;
diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index a726d5632a..0e96e73ee2 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -167,21 +167,17 @@ static PFlashCFI01 *xtfpga_flash_init(MemoryRegion *address_space,
DriveInfo *dinfo, int be)
{
SysBusDevice *s;
- DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
+ PFlashCFI01 *dev = pflash_cfi01_create("xtfpga.io.flash",
+ board->flash->size,
+ blk_by_legacy_dinfo(dinfo),
+ board->flash->sector_size,
+ 2, 0, 0, 0, 0, be);
- qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo),
- &error_abort);
- qdev_prop_set_uint32(dev, "num-blocks",
- board->flash->size / board->flash->sector_size);
- qdev_prop_set_uint64(dev, "sector-length", board->flash->sector_size);
- qdev_prop_set_uint8(dev, "width", 2);
- qdev_prop_set_bit(dev, "big-endian", be);
- qdev_prop_set_string(dev, "name", "xtfpga.io.flash");
- qdev_init_nofail(dev);
+ qdev_init_nofail(DEVICE(dev));
s = SYS_BUS_DEVICE(dev);
memory_region_add_subregion(address_space, board->flash->base,
sysbus_mmio_get_region(s, 0));
- return CFI_PFLASH01(dev);
+ return dev;
}
static uint64_t translate_phys_addr(void *opaque, uint64_t addr)
diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
index 24b13eb525..dbb25ba382 100644
--- a/include/hw/block/flash.h
+++ b/include/hw/block/flash.h
@@ -13,6 +13,14 @@
typedef struct PFlashCFI01 PFlashCFI01;
+PFlashCFI01 *pflash_cfi01_create(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);
PFlashCFI01 *pflash_cfi01_register(hwaddr base,
const char *name,
hwaddr size,
--
2.17.2
On Mon, Feb 18, 2019 at 5:07 AM Markus Armbruster <armbru@redhat.com> wrote: > > pflash_cfi01_register() creates a TYPE_CFI_PFLASH01 device, sets > properties, realizes, and wires up. > > We have three modified copies of it, because their users need to set > additional properties, or have the wiring done differently. > > Factor out their common part into pflash_cfi01_create(). > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/arm/vexpress.c | 22 +++++----------------- > hw/arm/virt.c | 26 +++++++++----------------- > hw/block/pflash_cfi01.c | 39 +++++++++++++++++++++++++++------------ > hw/xtensa/xtfpga.c | 18 +++++++----------- I was told that it's better this way when I did that initially: http://lists.gnu.org/archive/html/qemu-devel/2015-09/msg06927.html Has the idea of "better" changed since then? I'm fine with the code either way, just curious. -- Thanks. -- Max
Max Filippov <jcmvbkbc@gmail.com> writes: > On Mon, Feb 18, 2019 at 5:07 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> pflash_cfi01_register() creates a TYPE_CFI_PFLASH01 device, sets >> properties, realizes, and wires up. >> >> We have three modified copies of it, because their users need to set >> additional properties, or have the wiring done differently. >> >> Factor out their common part into pflash_cfi01_create(). >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> hw/arm/vexpress.c | 22 +++++----------------- >> hw/arm/virt.c | 26 +++++++++----------------- >> hw/block/pflash_cfi01.c | 39 +++++++++++++++++++++++++++------------ >> hw/xtensa/xtfpga.c | 18 +++++++----------- > > I was told that it's better this way when I did that initially: > http://lists.gnu.org/archive/html/qemu-devel/2015-09/msg06927.html > > Has the idea of "better" changed since then? > I'm fine with the code either way, just curious. I'm not sure about our collective idea of "better". I simply saw a helper function duplicated with minor variations because it fails to capture what's truly common, so I fixed that. If we think helper functions capturing device creation and property setting are bad, and open coding would be better. then we should get rid of pflash_cfi01_register() and pflash_cfi02_register() everywhere, not just avoid it in these three places. Cc'ing the usual QOM suspects for guidance.
On Mon, 18 Feb 2019 at 13:08, Markus Armbruster <armbru@redhat.com> wrote:
>
> pflash_cfi01_register() creates a TYPE_CFI_PFLASH01 device, sets
> properties, realizes, and wires up.
>
> We have three modified copies of it, because their users need to set
> additional properties, or have the wiring done differently.
>
> Factor out their common part into pflash_cfi01_create().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/arm/vexpress.c | 22 +++++-----------------
> hw/arm/virt.c | 26 +++++++++-----------------
> hw/block/pflash_cfi01.c | 39 +++++++++++++++++++++++++++------------
> hw/xtensa/xtfpga.c | 18 +++++++-----------
> include/hw/block/flash.h | 8 ++++++++
> 5 files changed, 56 insertions(+), 57 deletions(-)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 00913f2655..b23c63ed24 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -515,26 +515,14 @@ static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
> static PFlashCFI01 *ve_pflash_cfi01_register(hwaddr base, const char *name,
> DriveInfo *di)
> {
> - DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
> + DeviceState *dev;
>
> - if (di) {
> - qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di),
> - &error_abort);
> - }
> -
> - qdev_prop_set_uint32(dev, "num-blocks",
> - VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE);
> - qdev_prop_set_uint64(dev, "sector-length", VEXPRESS_FLASH_SECT_SIZE);
> - qdev_prop_set_uint8(dev, "width", 4);
> + dev = DEVICE(pflash_cfi01_create(name, VEXPRESS_FLASH_SIZE,
> + di ? blk_by_legacy_dinfo(di) : NULL,
> + VEXPRESS_FLASH_SECT_SIZE,
> + 4, 0x89, 0x18, 0x00, 0x00, false));
> qdev_prop_set_uint8(dev, "device-width", 2);
> - qdev_prop_set_bit(dev, "big-endian", false);
> - qdev_prop_set_uint16(dev, "id0", 0x89);
> - qdev_prop_set_uint16(dev, "id1", 0x18);
> - qdev_prop_set_uint16(dev, "id2", 0x00);
> - qdev_prop_set_uint16(dev, "id3", 0x00);
> - qdev_prop_set_string(dev, "name", name);
> qdev_init_nofail(dev);
> -
> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> return CFI_PFLASH01(dev);
> }
I prefer this code the way it stands. In particular the
"call another function but then set the 'device-width'
property here" looks dubious. But broadly speaking the
"do all the property setting directly rather than calling
a helper function" is the style choice I think we should
be aiming for. (The prevalence of the other approach is
due to a mix of (1) older code we haven't updated and
(2) property-setting being annoyingly heavyweight
syntax.)
thanks
-- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
> On Mon, 18 Feb 2019 at 13:08, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> pflash_cfi01_register() creates a TYPE_CFI_PFLASH01 device, sets
>> properties, realizes, and wires up.
>>
>> We have three modified copies of it, because their users need to set
>> additional properties, or have the wiring done differently.
>>
>> Factor out their common part into pflash_cfi01_create().
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> hw/arm/vexpress.c | 22 +++++-----------------
>> hw/arm/virt.c | 26 +++++++++-----------------
>> hw/block/pflash_cfi01.c | 39 +++++++++++++++++++++++++++------------
>> hw/xtensa/xtfpga.c | 18 +++++++-----------
>> include/hw/block/flash.h | 8 ++++++++
>> 5 files changed, 56 insertions(+), 57 deletions(-)
>>
>> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
>> index 00913f2655..b23c63ed24 100644
>> --- a/hw/arm/vexpress.c
>> +++ b/hw/arm/vexpress.c
>> @@ -515,26 +515,14 @@ static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
>> static PFlashCFI01 *ve_pflash_cfi01_register(hwaddr base, const char *name,
>> DriveInfo *di)
>> {
>> - DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
>> + DeviceState *dev;
>>
>> - if (di) {
>> - qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di),
>> - &error_abort);
>> - }
>> -
>> - qdev_prop_set_uint32(dev, "num-blocks",
>> - VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE);
>> - qdev_prop_set_uint64(dev, "sector-length", VEXPRESS_FLASH_SECT_SIZE);
>> - qdev_prop_set_uint8(dev, "width", 4);
>> + dev = DEVICE(pflash_cfi01_create(name, VEXPRESS_FLASH_SIZE,
>> + di ? blk_by_legacy_dinfo(di) : NULL,
>> + VEXPRESS_FLASH_SECT_SIZE,
>> + 4, 0x89, 0x18, 0x00, 0x00, false));
>> qdev_prop_set_uint8(dev, "device-width", 2);
>> - qdev_prop_set_bit(dev, "big-endian", false);
>> - qdev_prop_set_uint16(dev, "id0", 0x89);
>> - qdev_prop_set_uint16(dev, "id1", 0x18);
>> - qdev_prop_set_uint16(dev, "id2", 0x00);
>> - qdev_prop_set_uint16(dev, "id3", 0x00);
>> - qdev_prop_set_string(dev, "name", name);
>> qdev_init_nofail(dev);
>> -
>> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
>> return CFI_PFLASH01(dev);
>> }
>
> I prefer this code the way it stands. In particular the
> "call another function but then set the 'device-width'
> property here" looks dubious. But broadly speaking the
> "do all the property setting directly rather than calling
> a helper function" is the style choice I think we should
> be aiming for. (The prevalence of the other approach is
> due to a mix of (1) older code we haven't updated and
> (2) property-setting being annoyingly heavyweight
> syntax.)
Okay, no problem.
On 02/18/19 13:56, Markus Armbruster wrote:
> pflash_cfi01_register() creates a TYPE_CFI_PFLASH01 device, sets
> properties, realizes, and wires up.
>
> We have three modified copies of it, because their users need to set
> additional properties, or have the wiring done differently.
>
> Factor out their common part into pflash_cfi01_create().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/arm/vexpress.c | 22 +++++-----------------
> hw/arm/virt.c | 26 +++++++++-----------------
> hw/block/pflash_cfi01.c | 39 +++++++++++++++++++++++++++------------
> hw/xtensa/xtfpga.c | 18 +++++++-----------
> include/hw/block/flash.h | 8 ++++++++
> 5 files changed, 56 insertions(+), 57 deletions(-)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 00913f2655..b23c63ed24 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -515,26 +515,14 @@ static void vexpress_modify_dtb(const struct arm_boot_info *info, void *fdt)
> static PFlashCFI01 *ve_pflash_cfi01_register(hwaddr base, const char *name,
> DriveInfo *di)
> {
> - DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
> + DeviceState *dev;
>
> - if (di) {
> - qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di),
> - &error_abort);
> - }
> -
> - qdev_prop_set_uint32(dev, "num-blocks",
> - VEXPRESS_FLASH_SIZE / VEXPRESS_FLASH_SECT_SIZE);
> - qdev_prop_set_uint64(dev, "sector-length", VEXPRESS_FLASH_SECT_SIZE);
> - qdev_prop_set_uint8(dev, "width", 4);
> + dev = DEVICE(pflash_cfi01_create(name, VEXPRESS_FLASH_SIZE,
> + di ? blk_by_legacy_dinfo(di) : NULL,
> + VEXPRESS_FLASH_SECT_SIZE,
> + 4, 0x89, 0x18, 0x00, 0x00, false));
> qdev_prop_set_uint8(dev, "device-width", 2);
> - qdev_prop_set_bit(dev, "big-endian", false);
> - qdev_prop_set_uint16(dev, "id0", 0x89);
> - qdev_prop_set_uint16(dev, "id1", 0x18);
> - qdev_prop_set_uint16(dev, "id2", 0x00);
> - qdev_prop_set_uint16(dev, "id3", 0x00);
> - qdev_prop_set_string(dev, "name", name);
> qdev_init_nofail(dev);
> -
> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> return CFI_PFLASH01(dev);
> }
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b7d53b2b87..7787918483 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -48,6 +48,7 @@
> #include "exec/address-spaces.h"
> #include "qemu/bitops.h"
> #include "qemu/error-report.h"
> +#include "qemu/units.h"
> #include "hw/pci-host/gpex.h"
> #include "hw/arm/sysbus-fdt.h"
> #include "hw/platform-bus.h"
> @@ -875,29 +876,20 @@ static void create_one_flash(const char *name, hwaddr flashbase,
> * parameters as the flash devices on the Versatile Express board.
> */
> DriveInfo *dinfo = drive_get_next(IF_PFLASH);
> - DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
> - SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> - const uint64_t sectorlength = 256 * 1024;
> + DeviceState *dev;
> + SysBusDevice *sbd;
>
> - if (dinfo) {
> - qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo),
> - &error_abort);
> - }
> + dev = DEVICE(pflash_cfi01_create(name, flashsize,
> + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> + 256 * KiB,
> + 4, 0x89, 0x18, 0x00, 0x00, false));
>
> - qdev_prop_set_uint32(dev, "num-blocks", flashsize / sectorlength);
> - qdev_prop_set_uint64(dev, "sector-length", sectorlength);
> - qdev_prop_set_uint8(dev, "width", 4);
> qdev_prop_set_uint8(dev, "device-width", 2);
> - qdev_prop_set_bit(dev, "big-endian", false);
> - qdev_prop_set_uint16(dev, "id0", 0x89);
> - qdev_prop_set_uint16(dev, "id1", 0x18);
> - qdev_prop_set_uint16(dev, "id2", 0x00);
> - qdev_prop_set_uint16(dev, "id3", 0x00);
> - qdev_prop_set_string(dev, "name", name);
> qdev_init_nofail(dev);
>
> + sbd = SYS_BUS_DEVICE(dev);
> memory_region_add_subregion(sysmem, flashbase,
> - sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
> + sysbus_mmio_get_region(sbd, 0));
>
> if (file) {
> char *fn;
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 2e161f937f..00c2efd0d7 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -920,15 +920,14 @@ 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)
> +PFlashCFI01 *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_create(NULL, TYPE_CFI_PFLASH01);
>
> @@ -945,12 +944,28 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base,
> qdev_prop_set_uint16(dev, "id2", id2);
> qdev_prop_set_uint16(dev, "id3", id3);
> qdev_prop_set_string(dev, "name", name);
> - qdev_init_nofail(dev);
> -
> - sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> return CFI_PFLASH01(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)
> +{
> + PFlashCFI01 *dev = pflash_cfi01_create(name, size, blk,
> + sector_len, bank_width,
> + id0, id1, id2, id3, be);
> +
> + qdev_init_nofail(DEVICE(dev));
> + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> + return dev;
> +}
> +
> MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl)
> {
> return &fl->mem;
> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
> index a726d5632a..0e96e73ee2 100644
> --- a/hw/xtensa/xtfpga.c
> +++ b/hw/xtensa/xtfpga.c
> @@ -167,21 +167,17 @@ static PFlashCFI01 *xtfpga_flash_init(MemoryRegion *address_space,
> DriveInfo *dinfo, int be)
> {
> SysBusDevice *s;
> - DeviceState *dev = qdev_create(NULL, TYPE_CFI_PFLASH01);
> + PFlashCFI01 *dev = pflash_cfi01_create("xtfpga.io.flash",
> + board->flash->size,
> + blk_by_legacy_dinfo(dinfo),
> + board->flash->sector_size,
> + 2, 0, 0, 0, 0, be);
>
> - qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo),
> - &error_abort);
> - qdev_prop_set_uint32(dev, "num-blocks",
> - board->flash->size / board->flash->sector_size);
> - qdev_prop_set_uint64(dev, "sector-length", board->flash->sector_size);
> - qdev_prop_set_uint8(dev, "width", 2);
> - qdev_prop_set_bit(dev, "big-endian", be);
> - qdev_prop_set_string(dev, "name", "xtfpga.io.flash");
> - qdev_init_nofail(dev);
> + qdev_init_nofail(DEVICE(dev));
> s = SYS_BUS_DEVICE(dev);
> memory_region_add_subregion(address_space, board->flash->base,
> sysbus_mmio_get_region(s, 0));
> - return CFI_PFLASH01(dev);
> + return dev;
> }
>
> static uint64_t translate_phys_addr(void *opaque, uint64_t addr)
> diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h
> index 24b13eb525..dbb25ba382 100644
> --- a/include/hw/block/flash.h
> +++ b/include/hw/block/flash.h
> @@ -13,6 +13,14 @@
>
> typedef struct PFlashCFI01 PFlashCFI01;
>
> +PFlashCFI01 *pflash_cfi01_create(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);
> PFlashCFI01 *pflash_cfi01_register(hwaddr base,
> const char *name,
> hwaddr size,
>
Writing this patch must have been "fun"; my eyes are bleeding from
cross-referencing the new parameters with the removed properties. :/
I think the patch is correct.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
© 2016 - 2026 Red Hat, Inc.