* [PATCH 0/2] hw/arm/gumstix: Trivial cleanups @ 2020-02-23 23:10 Philippe Mathieu-Daudé 2020-02-23 23:10 ` [PATCH 1/2] hw/arm/gumstix: Simplify since the machines are little-endian only Philippe Mathieu-Daudé 2020-02-23 23:10 ` [PATCH 2/2] hw/arm/gumstix: Use the IEC binary prefix definitions Philippe Mathieu-Daudé 0 siblings, 2 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-23 23:10 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, qemu-trivial, Michael Tokarev, Laurent Vivier, Philippe Mathieu-Daudé, qemu-arm, Philippe Mathieu-Daudé From: Philippe Mathieu-Daudé <f4bug@amsat.org> Two house keeping patches while looking at the Gumstix boards, to make the code slightly more readable/documented. Philippe Mathieu-Daudé (2): hw/arm/gumstix: Simplify since the machines are little-endian only hw/arm/gumstix: Use the IEC binary prefix definitions hw/arm/gumstix.c | 42 ++++++++++++++---------------------------- 1 file changed, 14 insertions(+), 28 deletions(-) -- 2.21.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] hw/arm/gumstix: Simplify since the machines are little-endian only 2020-02-23 23:10 [PATCH 0/2] hw/arm/gumstix: Trivial cleanups Philippe Mathieu-Daudé @ 2020-02-23 23:10 ` Philippe Mathieu-Daudé 2020-02-25 11:12 ` Peter Maydell 2020-02-23 23:10 ` [PATCH 2/2] hw/arm/gumstix: Use the IEC binary prefix definitions Philippe Mathieu-Daudé 1 sibling, 1 reply; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-23 23:10 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, qemu-trivial, Michael Tokarev, Laurent Vivier, Philippe Mathieu-Daudé, qemu-arm, Philippe Mathieu-Daudé From: Philippe Mathieu-Daudé <f4bug@amsat.org> As the Connex and Verdex machines only boot in little-endian, we can simplify the code. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/arm/gumstix.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c index f26a0e8010..94904d717b 100644 --- a/hw/arm/gumstix.c +++ b/hw/arm/gumstix.c @@ -51,7 +51,6 @@ static void connex_init(MachineState *machine) { PXA2xxState *cpu; DriveInfo *dinfo; - int be; MemoryRegion *address_space_mem = get_system_memory(); uint32_t connex_rom = 0x01000000; @@ -66,14 +65,9 @@ static void connex_init(MachineState *machine) exit(1); } -#ifdef TARGET_WORDS_BIGENDIAN - be = 1; -#else - be = 0; -#endif if (!pflash_cfi01_register(0x00000000, "connext.rom", connex_rom, dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, - sector_len, 2, 0, 0, 0, 0, be)) { + sector_len, 2, 0, 0, 0, 0, 0)) { error_report("Error registering flash memory"); exit(1); } @@ -87,7 +81,6 @@ static void verdex_init(MachineState *machine) { PXA2xxState *cpu; DriveInfo *dinfo; - int be; MemoryRegion *address_space_mem = get_system_memory(); uint32_t verdex_rom = 0x02000000; @@ -102,14 +95,9 @@ static void verdex_init(MachineState *machine) exit(1); } -#ifdef TARGET_WORDS_BIGENDIAN - be = 1; -#else - be = 0; -#endif if (!pflash_cfi01_register(0x00000000, "verdex.rom", verdex_rom, dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, - sector_len, 2, 0, 0, 0, 0, be)) { + sector_len, 2, 0, 0, 0, 0, 0)) { error_report("Error registering flash memory"); exit(1); } @@ -152,6 +140,9 @@ static const TypeInfo verdex_type = { static void gumstix_machine_init(void) { + if (target_words_bigendian()) { + return; + } type_register_static(&connex_type); type_register_static(&verdex_type); } -- 2.21.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] hw/arm/gumstix: Simplify since the machines are little-endian only 2020-02-23 23:10 ` [PATCH 1/2] hw/arm/gumstix: Simplify since the machines are little-endian only Philippe Mathieu-Daudé @ 2020-02-25 11:12 ` Peter Maydell 2020-02-25 11:16 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 7+ messages in thread From: Peter Maydell @ 2020-02-25 11:12 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: QEMU Trivial, Michael Tokarev, Philippe Mathieu-Daudé, QEMU Developers, qemu-arm, Laurent Vivier On Sun, 23 Feb 2020 at 23:10, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > From: Philippe Mathieu-Daudé <f4bug@amsat.org> > > As the Connex and Verdex machines only boot in little-endian, > we can simplify the code. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > static void gumstix_machine_init(void) > { > + if (target_words_bigendian()) { > + return; > + } I don't think we really need to do this, do we? We don't in any of the other arm boards that just assume little-endian. I think TARGET_WORDS_BIGENDIAN will only be true on Arm for the BE linux-user targets, never for softmmu. Also, there's a warning comment in the header file for the declaration of target_words_bigendian() that says you probably don't want to use it... If you drop this, then otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] hw/arm/gumstix: Simplify since the machines are little-endian only 2020-02-25 11:12 ` Peter Maydell @ 2020-02-25 11:16 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-25 11:16 UTC (permalink / raw) To: Peter Maydell Cc: QEMU Trivial, Michael Tokarev, Philippe Mathieu-Daudé, QEMU Developers, qemu-arm, Laurent Vivier On 2/25/20 12:12 PM, Peter Maydell wrote: > On Sun, 23 Feb 2020 at 23:10, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> From: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> As the Connex and Verdex machines only boot in little-endian, >> we can simplify the code. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- > >> static void gumstix_machine_init(void) >> { >> + if (target_words_bigendian()) { >> + return; >> + } > > I don't think we really need to do this, do we? We don't > in any of the other arm boards that just assume little-endian. > I think TARGET_WORDS_BIGENDIAN will only be true on Arm for > the BE linux-user targets, never for softmmu. > > Also, there's a warning comment in the header file for > the declaration of target_words_bigendian() that says > you probably don't want to use it... Yes you (and the comment) are right, we should not use this call in target-specific code. > > If you drop this, then otherwise > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Thanks! > > thanks > -- PMM > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] hw/arm/gumstix: Use the IEC binary prefix definitions 2020-02-23 23:10 [PATCH 0/2] hw/arm/gumstix: Trivial cleanups Philippe Mathieu-Daudé 2020-02-23 23:10 ` [PATCH 1/2] hw/arm/gumstix: Simplify since the machines are little-endian only Philippe Mathieu-Daudé @ 2020-02-23 23:10 ` Philippe Mathieu-Daudé 2020-02-25 11:16 ` Peter Maydell 1 sibling, 1 reply; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-23 23:10 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, qemu-trivial, Michael Tokarev, Laurent Vivier, Philippe Mathieu-Daudé, qemu-arm, Philippe Mathieu-Daudé From: Philippe Mathieu-Daudé <f4bug@amsat.org> 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é <f4bug@amsat.org> --- 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"); @@ -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"); @@ -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"); -- 2.21.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] hw/arm/gumstix: Use the IEC binary prefix definitions 2020-02-23 23:10 ` [PATCH 2/2] hw/arm/gumstix: Use the IEC binary prefix definitions Philippe Mathieu-Daudé @ 2020-02-25 11:16 ` Peter Maydell 2020-02-25 11:20 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 7+ messages in thread From: Peter Maydell @ 2020-02-25 11:16 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: QEMU Trivial, Michael Tokarev, Philippe Mathieu-Daudé, QEMU Developers, qemu-arm, Laurent Vivier On Sun, 23 Feb 2020 at 23:10, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > From: Philippe Mathieu-Daudé <f4bug@amsat.org> > > 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é <f4bug@amsat.org> 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? > @@ -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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] hw/arm/gumstix: Use the IEC binary prefix definitions 2020-02-25 11:16 ` Peter Maydell @ 2020-02-25 11:20 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-02-25 11:20 UTC (permalink / raw) To: Peter Maydell Cc: QEMU Trivial, Michael Tokarev, Philippe Mathieu-Daudé, QEMU Developers, qemu-arm, Laurent Vivier On 2/25/20 12:16 PM, Peter Maydell wrote: > On Sun, 23 Feb 2020 at 23:10, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> From: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> 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é <f4bug@amsat.org> > > 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-25 11:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-02-23 23:10 [PATCH 0/2] hw/arm/gumstix: Trivial cleanups Philippe Mathieu-Daudé 2020-02-23 23:10 ` [PATCH 1/2] hw/arm/gumstix: Simplify since the machines are little-endian only Philippe Mathieu-Daudé 2020-02-25 11:12 ` Peter Maydell 2020-02-25 11:16 ` Philippe Mathieu-Daudé 2020-02-23 23:10 ` [PATCH 2/2] hw/arm/gumstix: Use the IEC binary prefix definitions Philippe Mathieu-Daudé 2020-02-25 11:16 ` Peter Maydell 2020-02-25 11:20 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).