| Message ID | 20230109120833.3330-11-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Remove implicit sysbus_mmio_map() from pflash APIs | expand |
On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: > pflash_cfi02_register() hides an implicit sysbus mapping of > MMIO region #0. This is not practical in a heterogeneous world > where multiple cores use different address spaces. In order to > remove pflash_cfi02_register() from the pflash API, open-code it > as a qdev creation call followed by an explicit sysbus mapping. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/sh4/r2d.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c > index 6e0c65124a..9d31fad807 100644 > --- a/hw/sh4/r2d.c > +++ b/hw/sh4/r2d.c > @@ -303,10 +303,23 @@ 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, > - FLASH_SECTOR_SIZE, 1, 2, > - 0x0001, 0x227e, 0x2220, 0x2200, 0x555, 0x2aa, 0); > + dev = qdev_new(TYPE_PFLASH_CFI02); > + qdev_prop_set_string(dev, "name", "r2d.flash"); > + qdev_prop_set_drive(dev, "drive", > + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL); > + qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE); > + qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE); > + qdev_prop_set_uint8(dev, "device-width", 2); > + qdev_prop_set_uint8(dev, "mappings", 1); > + qdev_prop_set_uint8(dev, "big-endian", false); > + qdev_prop_set_uint16(dev, "id0", 0x0001); > + qdev_prop_set_uint16(dev, "id1", 0x227e); > + qdev_prop_set_uint16(dev, "id2", 0x2220); > + qdev_prop_set_uint16(dev, "id3", 0x2200); > + qdev_prop_set_uint16(dev, "unlock-addr0", 0x555); > + qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa); > + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000); Instead of 0x00000000 can you just write 0 or if you really want then maybe 0x0? With the lot of zeros it's hard to tell it's not 0x00008000 or something so it's best to keep is simple if there's no good reason to obfuscate it. Regards, BALATON Zoltan > > /* NIC: rtl8139 on-board, and 2 slots. */ > for (i = 0; i < nb_nics; i++) >
On 9/1/23 14:40, BALATON Zoltan wrote: > On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: >> pflash_cfi02_register() hides an implicit sysbus mapping of >> MMIO region #0. This is not practical in a heterogeneous world >> where multiple cores use different address spaces. In order to >> remove pflash_cfi02_register() from the pflash API, open-code it >> as a qdev creation call followed by an explicit sysbus mapping. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/sh4/r2d.c | 21 +++++++++++++++++---- >> 1 file changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c >> index 6e0c65124a..9d31fad807 100644 >> --- a/hw/sh4/r2d.c >> +++ b/hw/sh4/r2d.c >> @@ -303,10 +303,23 @@ 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, >> - FLASH_SECTOR_SIZE, 1, 2, >> - 0x0001, 0x227e, 0x2220, 0x2200, 0x555, >> 0x2aa, 0); >> + dev = qdev_new(TYPE_PFLASH_CFI02); >> + qdev_prop_set_string(dev, "name", "r2d.flash"); >> + qdev_prop_set_drive(dev, "drive", >> + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL); >> + qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / >> FLASH_SECTOR_SIZE); >> + qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE); >> + qdev_prop_set_uint8(dev, "device-width", 2); >> + qdev_prop_set_uint8(dev, "mappings", 1); >> + qdev_prop_set_uint8(dev, "big-endian", false); >> + qdev_prop_set_uint16(dev, "id0", 0x0001); >> + qdev_prop_set_uint16(dev, "id1", 0x227e); >> + qdev_prop_set_uint16(dev, "id2", 0x2220); >> + qdev_prop_set_uint16(dev, "id3", 0x2200); >> + qdev_prop_set_uint16(dev, "unlock-addr0", 0x555); >> + qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa); >> + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >> + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000); > > Instead of 0x00000000 can you just write 0 or if you really want then > maybe 0x0? With the lot of zeros it's hard to tell it's not 0x00008000 > or something so it's best to keep is simple if there's no good reason to > obfuscate it. OK, maybe 0x0 to differentiate between the MMIO index and the base address.
On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: > On 9/1/23 14:40, BALATON Zoltan wrote: >> On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote: >>> pflash_cfi02_register() hides an implicit sysbus mapping of >>> MMIO region #0. This is not practical in a heterogeneous world >>> where multiple cores use different address spaces. In order to >>> remove pflash_cfi02_register() from the pflash API, open-code it >>> as a qdev creation call followed by an explicit sysbus mapping. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/sh4/r2d.c | 21 +++++++++++++++++---- >>> 1 file changed, 17 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c >>> index 6e0c65124a..9d31fad807 100644 >>> --- a/hw/sh4/r2d.c >>> +++ b/hw/sh4/r2d.c >>> @@ -303,10 +303,23 @@ 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, >>> - FLASH_SECTOR_SIZE, 1, 2, >>> - 0x0001, 0x227e, 0x2220, 0x2200, 0x555, 0x2aa, >>> 0); >>> + dev = qdev_new(TYPE_PFLASH_CFI02); >>> + qdev_prop_set_string(dev, "name", "r2d.flash"); >>> + qdev_prop_set_drive(dev, "drive", >>> + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL); >>> + qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / >>> FLASH_SECTOR_SIZE); >>> + qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE); >>> + qdev_prop_set_uint8(dev, "device-width", 2); >>> + qdev_prop_set_uint8(dev, "mappings", 1); >>> + qdev_prop_set_uint8(dev, "big-endian", false); >>> + qdev_prop_set_uint16(dev, "id0", 0x0001); >>> + qdev_prop_set_uint16(dev, "id1", 0x227e); >>> + qdev_prop_set_uint16(dev, "id2", 0x2220); >>> + qdev_prop_set_uint16(dev, "id3", 0x2200); >>> + qdev_prop_set_uint16(dev, "unlock-addr0", 0x555); >>> + qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa); >>> + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); >>> + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000); >> >> Instead of 0x00000000 can you just write 0 or if you really want then maybe >> 0x0? With the lot of zeros it's hard to tell it's not 0x00008000 or >> something so it's best to keep is simple if there's no good reason to >> obfuscate it. > > OK, maybe 0x0 to differentiate between the MMIO index and the base address. OK, you can also add: Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c index 6e0c65124a..9d31fad807 100644 --- a/hw/sh4/r2d.c +++ b/hw/sh4/r2d.c @@ -303,10 +303,23 @@ 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, - FLASH_SECTOR_SIZE, 1, 2, - 0x0001, 0x227e, 0x2220, 0x2200, 0x555, 0x2aa, 0); + dev = qdev_new(TYPE_PFLASH_CFI02); + qdev_prop_set_string(dev, "name", "r2d.flash"); + qdev_prop_set_drive(dev, "drive", + dinfo ? blk_by_legacy_dinfo(dinfo) : NULL); + qdev_prop_set_uint32(dev, "num-blocks", FLASH_SIZE / FLASH_SECTOR_SIZE); + qdev_prop_set_uint32(dev, "sector-length", FLASH_SECTOR_SIZE); + qdev_prop_set_uint8(dev, "device-width", 2); + qdev_prop_set_uint8(dev, "mappings", 1); + qdev_prop_set_uint8(dev, "big-endian", false); + qdev_prop_set_uint16(dev, "id0", 0x0001); + qdev_prop_set_uint16(dev, "id1", 0x227e); + qdev_prop_set_uint16(dev, "id2", 0x2220); + qdev_prop_set_uint16(dev, "id3", 0x2200); + qdev_prop_set_uint16(dev, "unlock-addr0", 0x555); + qdev_prop_set_uint16(dev, "unlock-addr1", 0x2aa); + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x00000000); /* NIC: rtl8139 on-board, and 2 slots. */ for (i = 0; i < nb_nics; i++)
pflash_cfi02_register() hides an implicit sysbus mapping of MMIO region #0. This is not practical in a heterogeneous world where multiple cores use different address spaces. In order to remove pflash_cfi02_register() from the pflash API, open-code it as a qdev creation call followed by an explicit sysbus mapping. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/sh4/r2d.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)