Re: [Qemu-ppc] [Qemu-devel] [PATCH v4 32/40] hw/ppc: Use the IEC binary prefix definitions

[Philippe Mathieu-Daudé]

Hi Zoltan,

On 06/11/2018 05:09 AM, BALATON Zoltan wrote:
> On Sun, 10 Jun 2018, Philippe Mathieu-Daudé wrote:
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index 2a98c10664..b35e3fff1c 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -12,8 +12,9 @@
>>  */
>>
>> #include "qemu/osdep.h"
>> +#include "qemu/units.h"
>> #include "qemu-common.h"
>> -#include "qemu/cutils.h"
>> +#include "qemu/units.h"
> 
> I think one of these includes of qemu/units.h is enough.

Oops :) Rebase mistake.

> 
>> #include "qemu/error-report.h"
>> #include "qapi/error.h"
>> #include "hw/hw.h"
>> @@ -46,7 +47,7 @@
>> /* from Sam460 U-Boot include/configs/Sam460ex.h */
>> #define FLASH_BASE             0xfff00000
>> #define FLASH_BASE_H           0x4
>> -#define FLASH_SIZE             (1 << 20)
>> +#define FLASH_SIZE             (1 * MiB)
>> #define UBOOT_LOAD_BASE        0xfff80000
>> #define UBOOT_SIZE             0x00080000
>> #define UBOOT_ENTRY            0xfffffffc
>> @@ -71,7 +72,8 @@
>>
>> /* FIXME: See u-boot.git 8ac41e, also fix in ppc440_uc.c */
>> static const unsigned int ppc460ex_sdram_bank_sizes[] = {
>> -    1024 << 20, 512 << 20, 256 << 20, 128 << 20, 64 << 20, 32 << 20, 0
>> +    1 * GiB, 512 * MiB, 256 * MiB, 128 * MiB,
>> +    64 * MiB, 32 * MiB, 0
>> };
> 
> You said elsewhere:
> 
> On Sun, 10 Jun 2018, Philippe Mathieu-Daudé wrote:
>> Each use this saves 3 char of the 80 columns limit!

This was versus the M_BYTE/G_BYTE definitions, now I noticed the ML ate
this patch...
(it supposed to be 31/41 here)
http://patchew.org/QEMU/address@hidden/

Where the diff was:

 static const unsigned int ppc460ex_sdram_bank_sizes[] = {
-    1024 << 20, 512 << 20, 256 << 20, 128 << 20, 64 << 20, 32 << 20, 0
+    1 * G_BYTE, 512 * M_BYTE, 256 * M_BYTE, 128 * M_BYTE,
+    64 * M_BYTE, 32 * M_BYTE, 0
 };

> Except when not but at least should not be longer than before and maybe
> more readable. But why are you splitting this line into two? I prefer
> one line for readability if it's not over the line limit.

Sure, as it fits.

> 
>> struct boot_info {
>> @@ -225,7 +227,7 @@ static int sam460ex_load_uboot(void)
>>     fl_sectors = (bios_size + 65535) >> 16;
>>
>>     if (!pflash_cfi01_register(base, NULL, "sam460ex.flash", bios_size,
>> -                               blk, (64 * 1024), fl_sectors,
>> +                               blk, 64 * KiB, fl_sectors,
>>                                1, 0x89, 0x18, 0x0000, 0x0, 1)) {
>>         error_report("qemu: Error registering flash memory.");
>>         /* XXX: return an error instead? */
>> @@ -359,14 +361,14 @@ static void main_cpu_reset(void *opaque)
>>
>>     /* either we have a kernel to boot or we jump to U-Boot */
>>     if (bi->entry != UBOOT_ENTRY) {
>> -        env->gpr[1] = (16 << 20) - 8;
>> +        env->gpr[1] = (16 * MiB) - 8;
>>         env->gpr[3] = FDT_ADDR;
>>         env->nip = bi->entry;
>>
>>         /* Create a mapping for the kernel.  */
>>         mmubooke_create_initial_mapping(env, 0, 0);
>>         env->gpr[6] = tswap32(EPAPR_MAGIC);
>> -        env->gpr[7] = (16 << 20) - 8; /*bi->ima_size;*/
>> +        env->gpr[7] = (16 * MiB) - 8; /* bi->ima_size; */
>>
>>     } else {
>>         env->nip = UBOOT_ENTRY;
>> @@ -479,8 +481,8 @@ static void sam460ex_init(MachineState *machine)
>>     /* 256K of L2 cache as memory */
>>     ppc4xx_l2sram_init(env);
>>     /* FIXME: remove this after fixing l2sram mapping in ppc440_uc.c? */
>> -    memory_region_init_ram(l2cache_ram, NULL, "ppc440.l2cache_ram",
>> 256 << 10,
>> -                           &error_abort);
>> +    memory_region_init_ram(l2cache_ram, NULL, "ppc440.l2cache_ram",
>> +                           256 * KiB, &error_abort);
> 
> Line is split differently here too for no apparent reason.

Same explanation:

+    memory_region_init_ram(l2cache_ram, NULL, "ppc440.l2cache_ram",
+                           256 * K_BYTE, &error_abort);

I'll update.

> 
> Regards,
> BALATON Zoltan