Re: [PATCH 2/2] hw/arm/gumstix: Use the IEC binary prefix definitions

[Philippe Mathieu-Daudé]

On 2/25/20 12:16 PM, Peter Maydell wrote:
> On Sun, 23 Feb 2020 at 23:10, Philippe Mathieu-Daudé <address@hidden> wrote:
> > From: Philippe Mathieu-Daudé <address@hidden>
> >
> > IEC binary prefixes ease code review: the unit is explicit.
> >
> > Add a comment describing the Connex uses a Numonyx RC28F128J3F75
> > flash, and the Verdex uses a Micron RC28F256P30TFA.
> >
> > Correct the Verdex machine description (we model the 'Pro' board).
> >
> > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>
> This patch is doing somewhat more than the commit Subject line
> suggests. I don't think it's particularly strongly in need
> of splitting into more patches, but could you make the
> subject line a bit closer to what the patch is doing?
>
> > ---
> >   hw/arm/gumstix.c | 23 +++++++++--------------
> >   1 file changed, 9 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c
> > index 94904d717b..ca918fda0c 100644
> > --- a/hw/arm/gumstix.c
> > +++ b/hw/arm/gumstix.c
> > @@ -35,6 +35,7 @@
> >    */
> >
> >   #include "qemu/osdep.h"
> > +#include "qemu/units.h"
> >   #include "qemu/error-report.h"
> >   #include "hw/arm/pxa.h"
> >   #include "net/net.h"
> > @@ -45,18 +46,14 @@
> >   #include "sysemu/qtest.h"
> >   #include "cpu.h"
> >
> > -static const int sector_len = 128 * 1024;
> > +static const int sector_len = 128 * KiB;
> >
> >   static void connex_init(MachineState *machine)
> >   {
> >       PXA2xxState *cpu;
> >       DriveInfo *dinfo;
> > -    MemoryRegion *address_space_mem = get_system_memory();
> >
> > -    uint32_t connex_rom = 0x01000000;
> > -    uint32_t connex_ram = 0x04000000;
> > -
> > -    cpu = pxa255_init(address_space_mem, connex_ram);
> > +    cpu = pxa255_init(get_system_memory(), 64 * MiB);
> >
> >       dinfo = drive_get(IF_PFLASH, 0, 0);
> >       if (!dinfo && !qtest_enabled()) {
> > @@ -65,7 +62,8 @@ static void connex_init(MachineState *machine)
> >           exit(1);
> >       }
> >
> > -    if (!pflash_cfi01_register(0x00000000, "connext.rom", connex_rom,
> > +    /* Numonyx RC28F128J3F75 */
> > +    if (!pflash_cfi01_register(0x00000000, "connext.rom", 16 * MiB,
> >                                  dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> >                                  sector_len, 2, 0, 0, 0, 0, 0)) {
> >           error_report("Error registering flash memory");
>
> Previously the variable names were helpfully acting as a
> bit of documentation of what the 64MB and 16MB things were.
> Perhaps we could instead have
>
> #define CONNEX_ROM_SIZE (16 * MiB)
> #define CONNEX_RAM_SIZE (64 * MiB)
>
> and then use those?

OK.

FYI long term plan for the pflash API is to use a white-list of tested 
flash models, and restrict models when boards have flash soldered.

> > @@ -81,12 +79,8 @@ static void verdex_init(MachineState *machine)
> >   {
> >       PXA2xxState *cpu;
> >       DriveInfo *dinfo;
> > -    MemoryRegion *address_space_mem = get_system_memory();
> >
> > -    uint32_t verdex_rom = 0x02000000;
> > -    uint32_t verdex_ram = 0x10000000;
> > -
> > -    cpu = pxa270_init(address_space_mem, verdex_ram, machine->cpu_type);
> > +    cpu = pxa270_init(get_system_memory(), 256 * MiB, machine->cpu_type);
> >
> >       dinfo = drive_get(IF_PFLASH, 0, 0);
> >       if (!dinfo && !qtest_enabled()) {
> > @@ -95,7 +89,8 @@ static void verdex_init(MachineState *machine)
> >           exit(1);
> >       }
> >
> > -    if (!pflash_cfi01_register(0x00000000, "verdex.rom", verdex_rom,
> > +    /* Micron RC28F256P30TFA */
> > +    if (!pflash_cfi01_register(0x00000000, "verdex.rom", 32 * MiB,
> >                                  dinfo ? blk_by_legacy_dinfo(dinfo) : NULL,
> >                                  sector_len, 2, 0, 0, 0, 0, 0)) {
> >           error_report("Error registering flash memory");
>
> Similarly here.
>
> > @@ -126,7 +121,7 @@ static void verdex_class_init(ObjectClass *oc, void *data)
> >   {
> >       MachineClass *mc = MACHINE_CLASS(oc);
> >
> > -    mc->desc = "Gumstix Verdex (PXA270)";
> > +    mc->desc = "Gumstix Verdex Pro XL6P COMs (PXA270)";
> >       mc->init = verdex_init;
> >       mc->ignore_memory_transaction_failures = true;
> >       mc->default_cpu_type = ARM_CPU_TYPE_NAME("pxa270-c0");
> > --
>
> thanks
> -- PMM