[PATCH 2/2] hw/arm/gumstix: Use the IEC binary prefix definitions
From: Philippe Mathieu-Daudé
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é
---
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 = 0x0100;
-uint32_t connex_ram = 0x0400;
-
-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(0x, "connext.rom", connex_rom,
+/* Numonyx RC28F128J3F75 */
+if (!pflash_cfi01_register(0x, "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 = 0x0200;
-uint32_t verdex_ram = 0x1000;
-
-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(0x, "verdex.rom", verdex_rom,
+/* Micron RC28F256P30TFA */
+if (!pflash_cfi01_register(0x, "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
Re: [PATCH 2/2] hw/arm/gumstix: Use the IEC binary prefix definitions
On Sun, 23 Feb 2020 at 23:10, Philippe Mathieu-Daudé wrote:
>
> From: Philippe Mathieu-Daudé
>
> 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é
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 = 0x0100;
> -uint32_t connex_ram = 0x0400;
> -
> -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(0x, "connext.rom", connex_rom,
> +/* Numonyx RC28F128J3F75 */
> +if (!pflash_cfi01_register(0x, "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 = 0x0200;
> -uint32_t verdex_ram = 0x1000;
> -
> -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(0x, "verdex.rom", verdex_rom,
> +/* Micron RC28F256P30TFA */
> +if (!pflash_cfi01_register(0x, "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
Re: [PATCH 2/2] hw/arm/gumstix: Use the IEC binary prefix definitions
On 2/25/20 12:16 PM, Peter Maydell wrote:
On Sun, 23 Feb 2020 at 23:10, Philippe Mathieu-Daudé wrote:
From: Philippe Mathieu-Daudé
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é
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 = 0x0100;
-uint32_t connex_ram = 0x0400;
-
-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(0x, "connext.rom", connex_rom,
+/* Numonyx RC28F128J3F75 */
+if (!pflash_cfi01_register(0x, "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 = 0x0200;
-uint32_t verdex_ram = 0x1000;
-
-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(0x, "verdex.rom", verdex_rom,
+/* Micron RC28F256P30TFA */
+if (!pflash_cfi01_register(0x, "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
