diff mbox series

[4/5] hw/sh4/r2d: Use the IEC binary prefix definitions

Message ID 20230109120154.2868-5-philmd@linaro.org (mailing list archive)
State New, archived
Headers show
Series hw: Cleanups around PFLASH use | expand

Commit Message

Philippe Mathieu-Daudé Jan. 9, 2023, 12:01 p.m. UTC
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(-)

Comments

BALATON Zoltan Jan. 9, 2023, 12:46 p.m. UTC | #1
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 */
>
Philippe Mathieu-Daudé Jan. 9, 2023, 1:12 p.m. UTC | #2
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.
BALATON Zoltan Jan. 9, 2023, 2:02 p.m. UTC | #3
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 mbox series

Patch

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 */