| Message ID | 20230109120154.2868-5-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw: Cleanups around PFLASH use | expand |
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 */ >
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.
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
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 */
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(-)