diff mbox series

[08/20] ppc4xx_sdram: Drop extra zeros for readability

Message ID f6d9eec237e5cc84a314d8eb67294212f93076ef.1660926381.git.balaton@eik.bme.hu
State New
Headers show
Series ppc4xx_sdram QOMify and clean ups | expand

Commit Message

BALATON Zoltan Aug. 19, 2022, 4:55 p.m. UTC 
Constants that are written zero padded for no good reason are hard to
read, it's easier to see what is meant if it's just 0 or 1 instead.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc4xx_devs.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

Comments

Cédric Le Goater Sept. 7, 2022, 1:30 p.m. UTC | #1 
On 8/19/22 18:55, BALATON Zoltan wrote:
> Constants that are written zero padded for no good reason are hard to
> read, it's easier to see what is meant if it's just 0 or 1 instead.

I would keep the 0x prefix though.

Thanks,

C.


> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/ppc4xx_devs.c | 40 ++++++++++++++++++++--------------------
>   1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index 375834a52b..bfe7b2d3a6 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -49,31 +49,31 @@ static uint32_t sdram_ddr_bcr(hwaddr ram_base, hwaddr ram_size)
>   
>       switch (ram_size) {
>       case 4 * MiB:
> -        bcr = 0x00000000;
> +        bcr = 0;
>           break;
>       case 8 * MiB:
> -        bcr = 0x00020000;
> +        bcr = 0x20000;
>           break;
>       case 16 * MiB:
> -        bcr = 0x00040000;
> +        bcr = 0x40000;
>           break;
>       case 32 * MiB:
> -        bcr = 0x00060000;
> +        bcr = 0x60000;
>           break;
>       case 64 * MiB:
> -        bcr = 0x00080000;
> +        bcr = 0x80000;
>           break;
>       case 128 * MiB:
> -        bcr = 0x000A0000;
> +        bcr = 0xA0000;
>           break;
>       case 256 * MiB:
> -        bcr = 0x000C0000;
> +        bcr = 0xC0000;
>           break;
>       default:
>           qemu_log_mask(LOG_GUEST_ERROR,
>                         "%s: invalid RAM size 0x%" HWADDR_PRIx "\n", __func__,
>                         ram_size);
> -        return 0x00000000;
> +        return 0;
>       }
>       bcr |= ram_base & 0xFF800000;
>       bcr |= 1;
> @@ -104,7 +104,7 @@ static target_ulong sdram_size(uint32_t bcr)
>   static void sdram_set_bcr(Ppc4xxSdramDdrState *sdram, int i,
>                             uint32_t bcr, int enabled)
>   {
> -    if (sdram->bank[i].bcr & 0x00000001) {
> +    if (sdram->bank[i].bcr & 1) {
>           /* Unmap RAM */
>           trace_ppc4xx_sdram_unmap(sdram_base(sdram->bank[i].bcr),
>                                    sdram_size(sdram->bank[i].bcr));
> @@ -115,7 +115,7 @@ static void sdram_set_bcr(Ppc4xxSdramDdrState *sdram, int i,
>           object_unparent(OBJECT(&sdram->bank[i].container));
>       }
>       sdram->bank[i].bcr = bcr & 0xFFDEE001;
> -    if (enabled && (bcr & 0x00000001)) {
> +    if (enabled && (bcr & 1)) {
>           trace_ppc4xx_sdram_map(sdram_base(bcr), sdram_size(bcr));
>           memory_region_init(&sdram->bank[i].container, NULL, "sdram-container",
>                              sdram_size(bcr));
> @@ -136,7 +136,7 @@ static void sdram_map_bcr(Ppc4xxSdramDdrState *sdram)
>               sdram_set_bcr(sdram, i, sdram_ddr_bcr(sdram->bank[i].base,
>                                                     sdram->bank[i].size), 1);
>           } else {
> -            sdram_set_bcr(sdram, i, 0x00000000, 0);
> +            sdram_set_bcr(sdram, i, 0, 0);
>           }
>       }
>   }
> @@ -213,7 +213,7 @@ static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
>           break;
>       default:
>           /* Avoid gcc warning */
> -        ret = 0x00000000;
> +        ret = 0;
>           break;
>       }
>   
> @@ -306,18 +306,18 @@ static void ppc4xx_sdram_ddr_reset(DeviceState *dev)
>   {
>       Ppc4xxSdramDdrState *sdram = PPC4xx_SDRAM_DDR(dev);
>   
> -    sdram->addr = 0x00000000;
> -    sdram->bear = 0x00000000;
> -    sdram->besr0 = 0x00000000; /* No error */
> -    sdram->besr1 = 0x00000000; /* No error */
> -    sdram->cfg = 0x00000000;
> -    sdram->ecccfg = 0x00000000; /* No ECC */
> -    sdram->eccesr = 0x00000000; /* No error */
> +    sdram->addr = 0;
> +    sdram->bear = 0;
> +    sdram->besr0 = 0; /* No error */
> +    sdram->besr1 = 0; /* No error */
> +    sdram->cfg = 0;
> +    sdram->ecccfg = 0; /* No ECC */
> +    sdram->eccesr = 0; /* No error */
>       sdram->pmit = 0x07C00000;
>       sdram->rtr = 0x05F00000;
>       sdram->tr = 0x00854009;
>       /* We pre-initialize RAM banks */
> -    sdram->status = 0x00000000;
> +    sdram->status = 0;
>       sdram->cfg = 0x00800000;
>   }
>
BALATON Zoltan Sept. 7, 2022, 2:32 p.m. UTC | #2 
On Wed, 7 Sep 2022, Cédric Le Goater wrote:
> On 8/19/22 18:55, BALATON Zoltan wrote:
>> Constants that are written zero padded for no good reason are hard to
>> read, it's easier to see what is meant if it's just 0 or 1 instead.
>
> I would keep the 0x prefix though.

I'm not a fan of 0x0 or 0x prefix for numbers below 0xa as it's more 
confusing than just having the simple number since these are the same in 
decimal and hex so I always think it might be 0xC or something not just 0 
when I see a prefix and have to double check. So unless there's a good 
reaon to write them in hex it's simpler to only use the 0x when really 
needed. Maybe if you really want the 0x I could keep it in the switch 
below just for consistency with other cases there but wouldn't have them 
elsewhere. Is it really not acceptable for you as in this patch?

Regards,
BALATON Zoltan

> Thanks,
>
> C.
>
>
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/ppc/ppc4xx_devs.c | 40 ++++++++++++++++++++--------------------
>>   1 file changed, 20 insertions(+), 20 deletions(-)
>> 
>> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
>> index 375834a52b..bfe7b2d3a6 100644
>> --- a/hw/ppc/ppc4xx_devs.c
>> +++ b/hw/ppc/ppc4xx_devs.c
>> @@ -49,31 +49,31 @@ static uint32_t sdram_ddr_bcr(hwaddr ram_base, hwaddr 
>> ram_size)
>>         switch (ram_size) {
>>       case 4 * MiB:
>> -        bcr = 0x00000000;
>> +        bcr = 0;
>>           break;
>>       case 8 * MiB:
>> -        bcr = 0x00020000;
>> +        bcr = 0x20000;
>>           break;
>>       case 16 * MiB:
>> -        bcr = 0x00040000;
>> +        bcr = 0x40000;
>>           break;
>>       case 32 * MiB:
>> -        bcr = 0x00060000;
>> +        bcr = 0x60000;
>>           break;
>>       case 64 * MiB:
>> -        bcr = 0x00080000;
>> +        bcr = 0x80000;
>>           break;
>>       case 128 * MiB:
>> -        bcr = 0x000A0000;
>> +        bcr = 0xA0000;
>>           break;
>>       case 256 * MiB:
>> -        bcr = 0x000C0000;
>> +        bcr = 0xC0000;
>>           break;
>>       default:
>>           qemu_log_mask(LOG_GUEST_ERROR,
>>                         "%s: invalid RAM size 0x%" HWADDR_PRIx "\n", 
>> __func__,
>>                         ram_size);
>> -        return 0x00000000;
>> +        return 0;
>>       }
>>       bcr |= ram_base & 0xFF800000;
>>       bcr |= 1;
>> @@ -104,7 +104,7 @@ static target_ulong sdram_size(uint32_t bcr)
>>   static void sdram_set_bcr(Ppc4xxSdramDdrState *sdram, int i,
>>                             uint32_t bcr, int enabled)
>>   {
>> -    if (sdram->bank[i].bcr & 0x00000001) {
>> +    if (sdram->bank[i].bcr & 1) {
>>           /* Unmap RAM */
>>           trace_ppc4xx_sdram_unmap(sdram_base(sdram->bank[i].bcr),
>>                                    sdram_size(sdram->bank[i].bcr));
>> @@ -115,7 +115,7 @@ static void sdram_set_bcr(Ppc4xxSdramDdrState *sdram, 
>> int i,
>>           object_unparent(OBJECT(&sdram->bank[i].container));
>>       }
>>       sdram->bank[i].bcr = bcr & 0xFFDEE001;
>> -    if (enabled && (bcr & 0x00000001)) {
>> +    if (enabled && (bcr & 1)) {
>>           trace_ppc4xx_sdram_map(sdram_base(bcr), sdram_size(bcr));
>>           memory_region_init(&sdram->bank[i].container, NULL, 
>> "sdram-container",
>>                              sdram_size(bcr));
>> @@ -136,7 +136,7 @@ static void sdram_map_bcr(Ppc4xxSdramDdrState *sdram)
>>               sdram_set_bcr(sdram, i, sdram_ddr_bcr(sdram->bank[i].base,
>>                                                     sdram->bank[i].size), 
>> 1);
>>           } else {
>> -            sdram_set_bcr(sdram, i, 0x00000000, 0);
>> +            sdram_set_bcr(sdram, i, 0, 0);
>>           }
>>       }
>>   }
>> @@ -213,7 +213,7 @@ static uint32_t sdram_ddr_dcr_read(void *opaque, int 
>> dcrn)
>>           break;
>>       default:
>>           /* Avoid gcc warning */
>> -        ret = 0x00000000;
>> +        ret = 0;
>>           break;
>>       }
>>   @@ -306,18 +306,18 @@ static void ppc4xx_sdram_ddr_reset(DeviceState 
>> *dev)
>>   {
>>       Ppc4xxSdramDdrState *sdram = PPC4xx_SDRAM_DDR(dev);
>>   -    sdram->addr = 0x00000000;
>> -    sdram->bear = 0x00000000;
>> -    sdram->besr0 = 0x00000000; /* No error */
>> -    sdram->besr1 = 0x00000000; /* No error */
>> -    sdram->cfg = 0x00000000;
>> -    sdram->ecccfg = 0x00000000; /* No ECC */
>> -    sdram->eccesr = 0x00000000; /* No error */
>> +    sdram->addr = 0;
>> +    sdram->bear = 0;
>> +    sdram->besr0 = 0; /* No error */
>> +    sdram->besr1 = 0; /* No error */
>> +    sdram->cfg = 0;
>> +    sdram->ecccfg = 0; /* No ECC */
>> +    sdram->eccesr = 0; /* No error */
>>       sdram->pmit = 0x07C00000;
>>       sdram->rtr = 0x05F00000;
>>       sdram->tr = 0x00854009;
>>       /* We pre-initialize RAM banks */
>> -    sdram->status = 0x00000000;
>> +    sdram->status = 0;
>>       sdram->cfg = 0x00800000;
>>   }
>> 
>
>
>
diff mbox series

Patch

diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index 375834a52b..bfe7b2d3a6 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -49,31 +49,31 @@  static uint32_t sdram_ddr_bcr(hwaddr ram_base, hwaddr ram_size)
 
     switch (ram_size) {
     case 4 * MiB:
-        bcr = 0x00000000;
+        bcr = 0;
         break;
     case 8 * MiB:
-        bcr = 0x00020000;
+        bcr = 0x20000;
         break;
     case 16 * MiB:
-        bcr = 0x00040000;
+        bcr = 0x40000;
         break;
     case 32 * MiB:
-        bcr = 0x00060000;
+        bcr = 0x60000;
         break;
     case 64 * MiB:
-        bcr = 0x00080000;
+        bcr = 0x80000;
         break;
     case 128 * MiB:
-        bcr = 0x000A0000;
+        bcr = 0xA0000;
         break;
     case 256 * MiB:
-        bcr = 0x000C0000;
+        bcr = 0xC0000;
         break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: invalid RAM size 0x%" HWADDR_PRIx "\n", __func__,
                       ram_size);
-        return 0x00000000;
+        return 0;
     }
     bcr |= ram_base & 0xFF800000;
     bcr |= 1;
@@ -104,7 +104,7 @@  static target_ulong sdram_size(uint32_t bcr)
 static void sdram_set_bcr(Ppc4xxSdramDdrState *sdram, int i,
                           uint32_t bcr, int enabled)
 {
-    if (sdram->bank[i].bcr & 0x00000001) {
+    if (sdram->bank[i].bcr & 1) {
         /* Unmap RAM */
         trace_ppc4xx_sdram_unmap(sdram_base(sdram->bank[i].bcr),
                                  sdram_size(sdram->bank[i].bcr));
@@ -115,7 +115,7 @@  static void sdram_set_bcr(Ppc4xxSdramDdrState *sdram, int i,
         object_unparent(OBJECT(&sdram->bank[i].container));
     }
     sdram->bank[i].bcr = bcr & 0xFFDEE001;
-    if (enabled && (bcr & 0x00000001)) {
+    if (enabled && (bcr & 1)) {
         trace_ppc4xx_sdram_map(sdram_base(bcr), sdram_size(bcr));
         memory_region_init(&sdram->bank[i].container, NULL, "sdram-container",
                            sdram_size(bcr));
@@ -136,7 +136,7 @@  static void sdram_map_bcr(Ppc4xxSdramDdrState *sdram)
             sdram_set_bcr(sdram, i, sdram_ddr_bcr(sdram->bank[i].base,
                                                   sdram->bank[i].size), 1);
         } else {
-            sdram_set_bcr(sdram, i, 0x00000000, 0);
+            sdram_set_bcr(sdram, i, 0, 0);
         }
     }
 }
@@ -213,7 +213,7 @@  static uint32_t sdram_ddr_dcr_read(void *opaque, int dcrn)
         break;
     default:
         /* Avoid gcc warning */
-        ret = 0x00000000;
+        ret = 0;
         break;
     }
 
@@ -306,18 +306,18 @@  static void ppc4xx_sdram_ddr_reset(DeviceState *dev)
 {
     Ppc4xxSdramDdrState *sdram = PPC4xx_SDRAM_DDR(dev);
 
-    sdram->addr = 0x00000000;
-    sdram->bear = 0x00000000;
-    sdram->besr0 = 0x00000000; /* No error */
-    sdram->besr1 = 0x00000000; /* No error */
-    sdram->cfg = 0x00000000;
-    sdram->ecccfg = 0x00000000; /* No ECC */
-    sdram->eccesr = 0x00000000; /* No error */
+    sdram->addr = 0;
+    sdram->bear = 0;
+    sdram->besr0 = 0; /* No error */
+    sdram->besr1 = 0; /* No error */
+    sdram->cfg = 0;
+    sdram->ecccfg = 0; /* No ECC */
+    sdram->eccesr = 0; /* No error */
     sdram->pmit = 0x07C00000;
     sdram->rtr = 0x05F00000;
     sdram->tr = 0x00854009;
     /* We pre-initialize RAM banks */
-    sdram->status = 0x00000000;
+    sdram->status = 0;
     sdram->cfg = 0x00800000;
 }