* [PATCH 0/5] hw: Cleanups around PFLASH use
@ 2023-01-09 12:01 Philippe Mathieu-Daudé
2023-01-09 12:01 ` [PATCH 1/5] hw/ppc/sam460ex: Remove unreachable code calling pflash_cfi01_register() Philippe Mathieu-Daudé
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-09 12:01 UTC (permalink / raw)
To: qemu-devel
Cc: Edgar E. Iglesias, Yoshinori Sato, Aurelien Jarno,
Philippe Mathieu-Daudé, Jiaxun Yang, Magnus Damm, qemu-ppc,
BALATON Zoltan
Trivial cleanups before PFLASH refactor:
- Remove unreachable error path calling pflash_cfi01_register()
- Add FLASH_SECTOR_SIZE definitions
- Use more IEC binary prefix definition
Philippe Mathieu-Daudé (5):
hw/ppc/sam460ex: Remove unreachable code calling
pflash_cfi01_register()
hw/microblaze/petalogix: Add FLASH_SECTOR_SIZE definitions
hw/mips/malta: Add the FLASH_SECTOR_SIZE definition
hw/sh4/r2d: Use the IEC binary prefix definitions
hw/sh4/r2d: Add the FLASH_SECTOR_SIZE definition
hw/microblaze/petalogix_ml605_mmu.c | 3 ++-
hw/microblaze/petalogix_s3adsp1800_mmu.c | 3 ++-
hw/mips/malta.c | 5 +++--
hw/ppc/sam460ex.c | 12 ++++--------
hw/sh4/r2d.c | 13 +++++++------
5 files changed, 18 insertions(+), 18 deletions(-)
--
2.38.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/5] hw/ppc/sam460ex: Remove unreachable code calling pflash_cfi01_register()
2023-01-09 12:01 [PATCH 0/5] hw: Cleanups around PFLASH use Philippe Mathieu-Daudé
@ 2023-01-09 12:01 ` Philippe Mathieu-Daudé
2023-01-09 12:38 ` BALATON Zoltan
2023-01-09 16:42 ` Bernhard Beschow
2023-01-09 12:01 ` [PATCH 2/5] hw/microblaze/petalogix: Add FLASH_SECTOR_SIZE definitions Philippe Mathieu-Daudé
` (3 subsequent siblings)
4 siblings, 2 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-09 12:01 UTC (permalink / raw)
To: qemu-devel
Cc: Edgar E. Iglesias, Yoshinori Sato, Aurelien Jarno,
Philippe Mathieu-Daudé, Jiaxun Yang, Magnus Damm, qemu-ppc,
BALATON Zoltan
Since its QOM'ification in commit 368a354f02 ("pflash_cfi0x:
QOMified") the pflash_cfi01_register() function does not fail.
This call was later converted with a script to use &error_fatal,
still unable to fail. Remove the unreachable code.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/ppc/sam460ex.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 4a22ce3761..cf7213f7c9 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -103,14 +103,10 @@ 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)) {
- error_report("Error registering flash memory");
- /* XXX: return an error instead? */
- exit(1);
- }
+ 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));
if (!dinfo) {
/*error_report("No flash image given with the 'pflash' parameter,"
--
2.38.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/5] hw/microblaze/petalogix: Add FLASH_SECTOR_SIZE definitions
2023-01-09 12:01 [PATCH 0/5] hw: Cleanups around PFLASH use Philippe Mathieu-Daudé
2023-01-09 12:01 ` [PATCH 1/5] hw/ppc/sam460ex: Remove unreachable code calling pflash_cfi01_register() Philippe Mathieu-Daudé
@ 2023-01-09 12:01 ` Philippe Mathieu-Daudé
2023-01-09 12:01 ` [PATCH 3/5] hw/mips/malta: Add the FLASH_SECTOR_SIZE definition Philippe Mathieu-Daudé
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-09 12:01 UTC (permalink / raw)
To: qemu-devel
Cc: Edgar E. Iglesias, Yoshinori Sato, Aurelien Jarno,
Philippe Mathieu-Daudé, Jiaxun Yang, Magnus Damm, qemu-ppc,
BALATON Zoltan
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/microblaze/petalogix_ml605_mmu.c | 3 ++-
hw/microblaze/petalogix_s3adsp1800_mmu.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index a24fadddca..1888900156 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -45,6 +45,7 @@
#define LMB_BRAM_SIZE (128 * KiB)
#define FLASH_SIZE (32 * MiB)
+#define FLASH_SECTOR_SIZE (64 * KiB)
#define BINARY_DEVICE_TREE_FILE "petalogix-ml605.dtb"
@@ -107,7 +108,7 @@ petalogix_ml605_init(MachineState *machine)
* 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);
+ FLASH_SECTOR_SIZE, 2, 0x89, 0x18, 0x0000, 0x0, 0);
dev = qdev_new("xlnx.xps-intc");
diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c b/hw/microblaze/petalogix_s3adsp1800_mmu.c
index 9d959d1ad8..d14eff2514 100644
--- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
+++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
@@ -40,6 +40,7 @@
#define LMB_BRAM_SIZE (128 * KiB)
#define FLASH_SIZE (16 * MiB)
+#define FLASH_SECTOR_SIZE (64 * KiB)
#define BINARY_DEVICE_TREE_FILE "petalogix-s3adsp1800.dtb"
@@ -87,7 +88,7 @@ petalogix_s3adsp1800_init(MachineState *machine)
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);
+ FLASH_SECTOR_SIZE, 1, 0x89, 0x18, 0x0000, 0x0, 1);
dev = qdev_new("xlnx.xps-intc");
qdev_prop_set_uint32(dev, "kind-of-intr",
--
2.38.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/5] hw/mips/malta: Add the FLASH_SECTOR_SIZE definition
2023-01-09 12:01 [PATCH 0/5] hw: Cleanups around PFLASH use Philippe Mathieu-Daudé
2023-01-09 12:01 ` [PATCH 1/5] hw/ppc/sam460ex: Remove unreachable code calling pflash_cfi01_register() Philippe Mathieu-Daudé
2023-01-09 12:01 ` [PATCH 2/5] hw/microblaze/petalogix: Add FLASH_SECTOR_SIZE definitions Philippe Mathieu-Daudé
@ 2023-01-09 12:01 ` Philippe Mathieu-Daudé
2023-01-09 12:01 ` [PATCH 4/5] hw/sh4/r2d: Use the IEC binary prefix definitions Philippe Mathieu-Daudé
2023-01-09 12:01 ` [PATCH 5/5] hw/sh4/r2d: Add the FLASH_SECTOR_SIZE definition Philippe Mathieu-Daudé
4 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-09 12:01 UTC (permalink / raw)
To: qemu-devel
Cc: Edgar E. Iglesias, Yoshinori Sato, Aurelien Jarno,
Philippe Mathieu-Daudé, Jiaxun Yang, Magnus Damm, qemu-ppc,
BALATON Zoltan
Ease code review by using the IEC binary prefix definition
for the FLASH_SIZE.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/mips/malta.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index c0a2e0ab04..e645ba1322 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -69,7 +69,8 @@
#define FPGA_ADDRESS 0x1f000000ULL
#define RESET_ADDRESS 0x1fc00000ULL
-#define FLASH_SIZE 0x400000
+#define FLASH_SIZE (4 * MiB)
+#define FLASH_SECTOR_SIZE (64 * KiB)
typedef struct {
MemoryRegion iomem;
@@ -1289,7 +1290,7 @@ void mips_malta_init(MachineState *machine)
fl = pflash_cfi01_register(FLASH_ADDRESS, "mips_malta.bios",
FLASH_SIZE,
dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
- 65536,
+ FLASH_SECTOR_SIZE,
4, 0x0000, 0x0000, 0x0000, 0x0000, be);
bios = pflash_cfi01_get_memory(fl);
fl_idx++;
--
2.38.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/5] hw/sh4/r2d: Use the IEC binary prefix definitions
2023-01-09 12:01 [PATCH 0/5] hw: Cleanups around PFLASH use Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2023-01-09 12:01 ` [PATCH 3/5] hw/mips/malta: Add the FLASH_SECTOR_SIZE definition Philippe Mathieu-Daudé
@ 2023-01-09 12:01 ` Philippe Mathieu-Daudé
2023-01-09 12:46 ` BALATON Zoltan
2023-01-09 12:01 ` [PATCH 5/5] hw/sh4/r2d: Add the FLASH_SECTOR_SIZE definition Philippe Mathieu-Daudé
4 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-09 12:01 UTC (permalink / raw)
To: qemu-devel
Cc: Edgar E. Iglesias, Yoshinori Sato, Aurelien Jarno,
Philippe Mathieu-Daudé, Jiaxun Yang, Magnus Damm, qemu-ppc,
BALATON Zoltan
IEC binary prefixes ease code review: the unit is explicit.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/sh4/r2d.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index 39fc4f19d9..b3667e9b12 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -47,10 +47,10 @@
#define FLASH_BASE 0x00000000
#define FLASH_SIZE (16 * MiB)
-#define SDRAM_BASE 0x0c000000 /* Physical location of SDRAM: Area 3 */
-#define SDRAM_SIZE 0x04000000
+#define SDRAM_BASE (192 * MiB) /* Physical location of SDRAM: Area 3 */
+#define SDRAM_SIZE (64 * MiB)
-#define SM501_VRAM_SIZE 0x800000
+#define SM501_VRAM_SIZE (8 * MiB)
#define BOOT_PARAMS_OFFSET 0x0010000
/* CONFIG_BOOT_LINK_OFFSET of Linux kernel */
--
2.38.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/5] hw/sh4/r2d: Add the FLASH_SECTOR_SIZE definition
2023-01-09 12:01 [PATCH 0/5] hw: Cleanups around PFLASH use Philippe Mathieu-Daudé
` (3 preceding siblings ...)
2023-01-09 12:01 ` [PATCH 4/5] hw/sh4/r2d: Use the IEC binary prefix definitions Philippe Mathieu-Daudé
@ 2023-01-09 12:01 ` Philippe Mathieu-Daudé
2023-01-09 12:49 ` BALATON Zoltan
4 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-09 12:01 UTC (permalink / raw)
To: qemu-devel
Cc: Edgar E. Iglesias, Yoshinori Sato, Aurelien Jarno,
Philippe Mathieu-Daudé, Jiaxun Yang, Magnus Damm, qemu-ppc,
BALATON Zoltan
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/sh4/r2d.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index b3667e9b12..6e0c65124a 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -45,7 +45,8 @@
#include "hw/block/flash.h"
#define FLASH_BASE 0x00000000
-#define FLASH_SIZE (16 * MiB)
+#define FLASH_SIZE (16 * MiB)
+#define FLASH_SECTOR_SIZE (128 * KiB)
#define SDRAM_BASE (192 * MiB) /* Physical location of SDRAM: Area 3 */
#define SDRAM_SIZE (64 * MiB)
@@ -304,8 +305,8 @@ static void r2d_init(MachineState *machine)
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);
+ FLASH_SECTOR_SIZE, 1, 2,
+ 0x0001, 0x227e, 0x2220, 0x2200, 0x555, 0x2aa, 0);
/* NIC: rtl8139 on-board, and 2 slots. */
for (i = 0; i < nb_nics; i++)
--
2.38.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] hw/ppc/sam460ex: Remove unreachable code calling pflash_cfi01_register()
2023-01-09 12:01 ` [PATCH 1/5] hw/ppc/sam460ex: Remove unreachable code calling pflash_cfi01_register() Philippe Mathieu-Daudé
@ 2023-01-09 12:38 ` BALATON Zoltan
2023-01-09 16:42 ` Bernhard Beschow
1 sibling, 0 replies; 13+ messages in thread
From: BALATON Zoltan @ 2023-01-09 12:38 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]
On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
> Since its QOM'ification in commit 368a354f02 ("pflash_cfi0x:
> QOMified") the pflash_cfi01_register() function does not fail.
>
> This call was later converted with a script to use &error_fatal,
> still unable to fail. Remove the unreachable code.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ppc/sam460ex.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 4a22ce3761..cf7213f7c9 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -103,14 +103,10 @@ 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)) {
> - error_report("Error registering flash memory");
> - /* XXX: return an error instead? */
> - exit(1);
> - }
> + 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));
>
> if (!dinfo) {
> /*error_report("No flash image given with the 'pflash' parameter,"
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/5] hw/sh4/r2d: Use the IEC binary prefix definitions
2023-01-09 12:01 ` [PATCH 4/5] hw/sh4/r2d: Use the IEC binary prefix definitions Philippe Mathieu-Daudé
@ 2023-01-09 12:46 ` BALATON Zoltan
2023-01-09 13:12 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 13+ messages in thread
From: BALATON Zoltan @ 2023-01-09 12:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Edgar E. Iglesias, Yoshinori Sato, Aurelien Jarno,
Jiaxun Yang, Magnus Damm, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]
On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
> IEC binary prefixes ease code review: the unit is explicit.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/sh4/r2d.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index 39fc4f19d9..b3667e9b12 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -47,10 +47,10 @@
> #define FLASH_BASE 0x00000000
> #define FLASH_SIZE (16 * MiB)
>
> -#define SDRAM_BASE 0x0c000000 /* Physical location of SDRAM: Area 3 */
> -#define SDRAM_SIZE 0x04000000
> +#define SDRAM_BASE (192 * MiB) /* Physical location of SDRAM: Area 3 */
> +#define SDRAM_SIZE (64 * MiB)
I don't think changing these help as the docs probably have memory map
with the hex numbers rather than sizes so it's easier to match as it is
now.
> -#define SM501_VRAM_SIZE 0x800000
> +#define SM501_VRAM_SIZE (8 * MiB)
This one is OK but since it's only used once in
qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE);
you might as well just inline it there and remove the define which is then
pretty clear and easier to see without needing to look up the define far
away from its usage.
Regards,
BALATON Zoltan
> #define BOOT_PARAMS_OFFSET 0x0010000
> /* CONFIG_BOOT_LINK_OFFSET of Linux kernel */
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 5/5] hw/sh4/r2d: Add the FLASH_SECTOR_SIZE definition
2023-01-09 12:01 ` [PATCH 5/5] hw/sh4/r2d: Add the FLASH_SECTOR_SIZE definition Philippe Mathieu-Daudé
@ 2023-01-09 12:49 ` BALATON Zoltan
0 siblings, 0 replies; 13+ messages in thread
From: BALATON Zoltan @ 2023-01-09 12:49 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Edgar E. Iglesias, Yoshinori Sato, Aurelien Jarno,
Jiaxun Yang, Magnus Damm, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]
On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/sh4/r2d.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index b3667e9b12..6e0c65124a 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -45,7 +45,8 @@
> #include "hw/block/flash.h"
>
> #define FLASH_BASE 0x00000000
> -#define FLASH_SIZE (16 * MiB)
> +#define FLASH_SIZE (16 * MiB)
> +#define FLASH_SECTOR_SIZE (128 * KiB)
I'm not a fan of single use defines when it makes harder to see what value
is used than using a constant. This one is border line case as the
pflash_cfi02_register() function has so many arguments that it's not clear
which is which but I'm not sure it worth the churn but I don't care too
much either.
Regards,
BALATON Zoltan
> #define SDRAM_BASE (192 * MiB) /* Physical location of SDRAM: Area 3 */
> #define SDRAM_SIZE (64 * MiB)
> @@ -304,8 +305,8 @@ static void r2d_init(MachineState *machine)
> 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);
> + FLASH_SECTOR_SIZE, 1, 2,
> + 0x0001, 0x227e, 0x2220, 0x2200, 0x555, 0x2aa, 0);
>
> /* NIC: rtl8139 on-board, and 2 slots. */
> for (i = 0; i < nb_nics; i++)
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/5] hw/sh4/r2d: Use the IEC binary prefix definitions
2023-01-09 12:46 ` BALATON Zoltan
@ 2023-01-09 13:12 ` Philippe Mathieu-Daudé
2023-01-09 14:02 ` BALATON Zoltan
0 siblings, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-09 13:12 UTC (permalink / raw)
To: BALATON Zoltan, Peter Maydell
Cc: qemu-devel, Edgar E. Iglesias, Yoshinori Sato, Aurelien Jarno,
Jiaxun Yang, Magnus Damm, qemu-ppc
On 9/1/23 13:46, BALATON Zoltan wrote:
> On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
>> IEC binary prefixes ease code review: the unit is explicit.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/sh4/r2d.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
>> index 39fc4f19d9..b3667e9b12 100644
>> --- a/hw/sh4/r2d.c
>> +++ b/hw/sh4/r2d.c
>> @@ -47,10 +47,10 @@
>> #define FLASH_BASE 0x00000000
>> #define FLASH_SIZE (16 * MiB)
>>
>> -#define SDRAM_BASE 0x0c000000 /* Physical location of SDRAM: Area 3 */
>> -#define SDRAM_SIZE 0x04000000
>> +#define SDRAM_BASE (192 * MiB) /* Physical location of
>> SDRAM: Area 3 */
>> +#define SDRAM_SIZE (64 * MiB)
>
> I don't think changing these help as the docs probably have memory map
> with the hex numbers rather than sizes so it's easier to match as it is
> now.
>
>> -#define SM501_VRAM_SIZE 0x800000
>> +#define SM501_VRAM_SIZE (8 * MiB)
>
> This one is OK but since it's only used once in
>
> qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE);
>
> you might as well just inline it there and remove the define which is
> then pretty clear and easier to see without needing to look up the
> define far away from its usage.
I did this change after Peter's feedbacks on:
https://lore.kernel.org/qemu-devel/CAFEAcA8MSO4YMEq2FqvpJKUVYE_1CqaB2KLD1YN-YebOhJVgEg@mail.gmail.com/
But maybe I misunderstood him. Peter, looking at it again, maybe you
asked for a definition because when using pflash_cfi01_register() it
isn't explicit what means each argument? So in this case, since the
properties gives a hint on what is the value ("vram-size") it would
be OK to inline?
Thanks,
Phil.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/5] hw/sh4/r2d: Use the IEC binary prefix definitions
2023-01-09 13:12 ` Philippe Mathieu-Daudé
@ 2023-01-09 14:02 ` BALATON Zoltan
0 siblings, 0 replies; 13+ messages in thread
From: BALATON Zoltan @ 2023-01-09 14:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, qemu-devel, Edgar E. Iglesias, Yoshinori Sato,
Aurelien Jarno, Jiaxun Yang, Magnus Damm, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]
On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
> On 9/1/23 13:46, BALATON Zoltan wrote:
>> On Mon, 9 Jan 2023, Philippe Mathieu-Daudé wrote:
>>> IEC binary prefixes ease code review: the unit is explicit.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> hw/sh4/r2d.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
>>> index 39fc4f19d9..b3667e9b12 100644
>>> --- a/hw/sh4/r2d.c
>>> +++ b/hw/sh4/r2d.c
>>> @@ -47,10 +47,10 @@
>>> #define FLASH_BASE 0x00000000
>>> #define FLASH_SIZE (16 * MiB)
>>>
>>> -#define SDRAM_BASE 0x0c000000 /* Physical location of SDRAM: Area 3 */
>>> -#define SDRAM_SIZE 0x04000000
>>> +#define SDRAM_BASE (192 * MiB) /* Physical location of SDRAM:
>>> Area 3 */
>>> +#define SDRAM_SIZE (64 * MiB)
>>
>> I don't think changing these help as the docs probably have memory map with
>> the hex numbers rather than sizes so it's easier to match as it is now.
>>
>>> -#define SM501_VRAM_SIZE 0x800000
>>> +#define SM501_VRAM_SIZE (8 * MiB)
>>
>> This one is OK but since it's only used once in
>>
>> qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE);
>>
>> you might as well just inline it there and remove the define which is then
>> pretty clear and easier to see without needing to look up the define far
>> away from its usage.
>
> I did this change after Peter's feedbacks on:
> https://lore.kernel.org/qemu-devel/CAFEAcA8MSO4YMEq2FqvpJKUVYE_1CqaB2KLD1YN-YebOhJVgEg@mail.gmail.com/
>
> But maybe I misunderstood him. Peter, looking at it again, maybe you
> asked for a definition because when using pflash_cfi01_register() it
> isn't explicit what means each argument? So in this case, since the
> properties gives a hint on what is the value ("vram-size") it would
> be OK to inline?
I think since you'll change it later to set properties then it will be
clear again without defines so I don't think single use defines are needed
in this case. It's just the many arguments of pflash_cfi01_register()
function that made it unclear but that will be gone by the end of the
series.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] hw/ppc/sam460ex: Remove unreachable code calling pflash_cfi01_register()
2023-01-09 12:01 ` [PATCH 1/5] hw/ppc/sam460ex: Remove unreachable code calling pflash_cfi01_register() Philippe Mathieu-Daudé
2023-01-09 12:38 ` BALATON Zoltan
@ 2023-01-09 16:42 ` Bernhard Beschow
2023-01-09 16:51 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 13+ messages in thread
From: Bernhard Beschow @ 2023-01-09 16:42 UTC (permalink / raw)
To: qemu-devel, Philippe Mathieu-Daudé
Cc: Edgar E. Iglesias, Yoshinori Sato, Aurelien Jarno, Jiaxun Yang,
Magnus Damm, qemu-ppc, BALATON Zoltan
Am 9. Januar 2023 12:01:50 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Since its QOM'ification in commit 368a354f02 ("pflash_cfi0x:
>QOMified") the pflash_cfi01_register() function does not fail.
>
>This call was later converted with a script to use &error_fatal,
>still unable to fail. Remove the unreachable code.
>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---
> hw/ppc/sam460ex.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
It looks like there is more: https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg02491.html
Best regards,
Bernhard
>
>diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>index 4a22ce3761..cf7213f7c9 100644
>--- a/hw/ppc/sam460ex.c
>+++ b/hw/ppc/sam460ex.c
>@@ -103,14 +103,10 @@ 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)) {
>- error_report("Error registering flash memory");
>- /* XXX: return an error instead? */
>- exit(1);
>- }
>+ 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));
>
> if (!dinfo) {
> /*error_report("No flash image given with the 'pflash' parameter,"
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] hw/ppc/sam460ex: Remove unreachable code calling pflash_cfi01_register()
2023-01-09 16:42 ` Bernhard Beschow
@ 2023-01-09 16:51 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-01-09 16:51 UTC (permalink / raw)
To: Bernhard Beschow, qemu-devel
Cc: Edgar E. Iglesias, Yoshinori Sato, Aurelien Jarno, Jiaxun Yang,
Magnus Damm, qemu-ppc, BALATON Zoltan
On 9/1/23 17:42, Bernhard Beschow wrote:
> Am 9. Januar 2023 12:01:50 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> Since its QOM'ification in commit 368a354f02 ("pflash_cfi0x:
>> QOMified") the pflash_cfi01_register() function does not fail.
>>
>> This call was later converted with a script to use &error_fatal,
>> still unable to fail. Remove the unreachable code.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/ppc/sam460ex.c | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> It looks like there is more: https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg02491.html
Doh, my short-term memory scares me :\
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-01-09 16:52 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-09 12:01 [PATCH 0/5] hw: Cleanups around PFLASH use Philippe Mathieu-Daudé
2023-01-09 12:01 ` [PATCH 1/5] hw/ppc/sam460ex: Remove unreachable code calling pflash_cfi01_register() Philippe Mathieu-Daudé
2023-01-09 12:38 ` BALATON Zoltan
2023-01-09 16:42 ` Bernhard Beschow
2023-01-09 16:51 ` Philippe Mathieu-Daudé
2023-01-09 12:01 ` [PATCH 2/5] hw/microblaze/petalogix: Add FLASH_SECTOR_SIZE definitions Philippe Mathieu-Daudé
2023-01-09 12:01 ` [PATCH 3/5] hw/mips/malta: Add the FLASH_SECTOR_SIZE definition Philippe Mathieu-Daudé
2023-01-09 12:01 ` [PATCH 4/5] hw/sh4/r2d: Use the IEC binary prefix definitions Philippe Mathieu-Daudé
2023-01-09 12:46 ` BALATON Zoltan
2023-01-09 13:12 ` Philippe Mathieu-Daudé
2023-01-09 14:02 ` BALATON Zoltan
2023-01-09 12:01 ` [PATCH 5/5] hw/sh4/r2d: Add the FLASH_SECTOR_SIZE definition Philippe Mathieu-Daudé
2023-01-09 12:49 ` BALATON Zoltan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).