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

[BALATON Zoltan]

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;
> >   }