Re: [PATCH v2 04/11] hw/arm/aspeed: Use the IEC binary prefix definitions

[Cédric Le Goater]

On 1/18/23 09:37, Philippe Mathieu-Daudé wrote:
> On 18/1/23 08:32, Cédric Le Goater wrote:
> > On 1/18/23 07:53, Joel Stanley wrote:
> > > On Fri, 30 Dec 2022 at 11:35, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > > > IEC binary prefixes ease code review: the unit is explicit.
> > >
> > > I strongly prefer the existing code; it tells you the size without
> > > having to do maths.
>
> I guess it depends on one's mindset and culture / education,
> maybe I'm too young for the full hexadecimal world and am more
> custom to decimal notation with binary prefixes.
>
> 0x16400 is just another magic number for me, while 89 * KiB is a size.
>
> > you mean that it matches better with the address space representation
> > in the code and the 'info mtree' output ? If so, I agree. We can keep
> > this patch out, it is not fundamental.
> >
> > The hex representation of values has its advantages compared to the
> > macros because hex is generally what you get in debug outputs and
> > it is easier to compare and manipulate.  Some Linux dev feel the
> > same.
>
> > > > aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
> > > >       sc->name         = "ast2600-a3";
> > > >       sc->cpu_type     = ARM_CPU_TYPE_NAME("cortex-a7");
> > > >       sc->silicon_rev  = AST2600_A3_SILICON_REV;
> > > > -    sc->sram_size    = 0x16400;
> > > > +    sc->sram_size    = 89 * KiB;
> To keep everybody happy I'll respin using:
>
>           sc->sram_size    = 0x16400; /* 89 * KiB */

I had some minor/aesthetic comments. If you could also address them please, I 
will
include this series in the aspeed-8.0 PR.

Thanks,

C.