[PATCH v3 00/13] hw/arm/raspi: Dynamically create machines based on the board revision

[PATCH v3 00/13] hw/arm/raspi: Dynamically create machines based on the board revision

[Philippe Mathieu-Daudé]

Hi,

This series is a preparatory to easily add the raspi0/raspi1/raspi4
boards (see [1]).

Igor has been working in his "refactor main RAM allocation to use
hostmem backend" series, and now v4 [2] is almost reviewed.

His raspi patch [3] clashes with my work, Since it is easier for
him to apply his on top of mine, I am sending these patches first.

Since v2:
- Split of bigger series (30 patches was scary)
- addressed Zoltan review comments

Phil.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg677145.html
[2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg675738.html
[3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg675752.html
Supersedes: <20200206011756.2413-1-f4bug@amsat.org>

Philippe Mathieu-Daudé (13):
  hw/arm/raspi: Use BCM2708 machine type with pre Device Tree kernels
  hw/arm/raspi: Correct the board descriptions
  hw/arm/raspi: Extract the version from the board revision
  hw/arm/raspi: Extract the RAM size from the board revision
  hw/arm/raspi: Extract the processor type from the board revision
  hw/arm/raspi: Trivial code movement
  hw/arm/raspi: Make machines children of abstract RaspiMachineClass
  hw/arm/raspi: Make board_rev a field of RaspiMachineClass
  hw/arm/raspi: Let class_init() directly call raspi_machine_init()
  hw/arm/raspi: Set default RAM size to size encoded in board revision
  hw/arm/raspi: Extract the board model from the board revision
  hw/arm/raspi: Use a unique raspi_machine_class_init() method
  hw/arm/raspi: Extract the cores count from the board revision

 hw/arm/raspi.c | 190 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 143 insertions(+), 47 deletions(-)

-- 
2.21.1

[PATCH v3 01/13] hw/arm/raspi: Use BCM2708 machine type with pre Device Tree kernels

[Philippe Mathieu-Daudé]

When booting without device tree, the Linux kernels uses the $R1
register to determine the machine type. The list of values is
registered at [1].

There are two entries for the Raspberry Pi:

- https://www.arm.linux.org.uk/developer/machines/list.php?mid=3138
  name: MACH_TYPE_BCM2708
  value: 0xc42 (3138)
  status: Active, not mainlined
  date: 15 Oct 2010

- https://www.arm.linux.org.uk/developer/machines/list.php?mid=4828
  name: MACH_TYPE_BCM2835
  value: 4828
  status: Active, mainlined
  date: 6 Dec 2013

QEMU always used the non-mainlined type MACH_TYPE_BCM2708.
The value 0xc43 is registered to 'MX51_GGC' (processor i.MX51), and
0xc44 to 'Western Digital Sharespace NAS' (processor Marvell 88F5182).

The Raspberry Pi foundation bootloader only sets the BCM2708 machine
type, see [2] or [3]:

 133 9:
 134     mov r0, #0
 135     ldr r1, =3138       @ BCM2708 machine id
 136     ldr r2, atags       @ ATAGS
 137     bx  r4

U-Boot only uses MACH_TYPE_BCM2708 (see [4]):

 25 /*
 26  * 2835 is a SKU in a series for which the 2708 is the first or primary SoC,
 27  * so 2708 has historically been used rather than a dedicated 2835 ID.
 28  *
 29  * We don't define a machine type for bcm2709/bcm2836 since the RPi Foundation
 30  * chose to use someone else's previously registered machine ID (3139, MX51_GGC)
 31  * rather than obtaining a valid ID:-/
 32  *
 33  * For the bcm2837, hopefully a machine type is not needed, since everything
 34  * is DT.
 35  */

While the definition MACH_BCM2709 with value 0xc43 was introduced in
a commit described "Add 2709 platform for Raspberry Pi 2" out of the
mainline Linux kernel, it does not seem used, and the platform is
introduced with Device Tree support anyway (see [5] and [6]).

Remove the unused values (0xc43 introduced in commit 1df7d1f9303aef
"raspi: add raspberry pi 2 machine" and 0xc44 in commit bade58166f4
"raspi: Raspberry Pi 3 support"), keeping only MACH_TYPE_BCM2708.

[1] https://www.arm.linux.org.uk/developer/machines/
[2] https://github.com/raspberrypi/tools/blob/920c7ed2e/armstubs/armstub7.S#L135
[3] https://github.com/raspberrypi/tools/blob/49719d554/armstubs/armstub7.S#L64
[4] https://gitlab.denx.de/u-boot/u-boot/blob/v2015.04/include/configs/rpi-common.h#L18
[5] https://github.com/raspberrypi/linux/commit/d9fac63adac#diff-6722037d79570df5b392a49e0e006573R526
[6] http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-February/001268.html

Cc: Zoltán Baldaszti <bztemail@gmail.com>
Cc: Pekka Enberg <penberg@iki.fi>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Kshitij Soni <kshitij.soni@broadcom.com>
Cc: Michael Chan <michael.chan@broadcom.com>
Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
v3: Improved MACH_TYPE_BCM2708 comment (Zoltán)
---
 hw/arm/raspi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 3996f6c63a..f2ccabc662 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -29,8 +29,8 @@
 #define FIRMWARE_ADDR_3 0x80000 /* Pi 3 loads kernel.img here by default */
 #define SPINTABLE_ADDR  0xd8 /* Pi 3 bootloader spintable */
 
-/* Table of Linux board IDs for different Pi versions */
-static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};
+/* Registered machine type (matches RPi Foundation bootloader and U-Boot) */
+#define MACH_TYPE_BCM2708   3138
 
 typedef struct RasPiState {
     BCM283XState soc;
@@ -116,7 +116,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
     static struct arm_boot_info binfo;
     int r;
 
-    binfo.board_id = raspi_boardid[version];
+    binfo.board_id = MACH_TYPE_BCM2708;
     binfo.ram_size = ram_size;
     binfo.nb_cpus = machine->smp.cpus;
 
-- 
2.21.1

[PATCH v3 02/13] hw/arm/raspi: Correct the board descriptions

[Philippe Mathieu-Daudé]

We hardcode the board revision as 0xa21041 for the raspi2, and
0xa02082 for the raspi3:

  166 static void raspi_init(MachineState *machine, int version)
  167 {
  ...
  194     int board_rev = version == 3 ? 0xa02082 : 0xa21041;

These revision codes are for the 2B and 3B models, see:
https://www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md

Correct the board description.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/raspi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index f2ccabc662..818146fdbb 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -221,7 +221,7 @@ static void raspi2_init(MachineState *machine)
 
 static void raspi2_machine_init(MachineClass *mc)
 {
-    mc->desc = "Raspberry Pi 2";
+    mc->desc = "Raspberry Pi 2B";
     mc->init = raspi2_init;
     mc->block_default_type = IF_SD;
     mc->no_parallel = 1;
@@ -243,7 +243,7 @@ static void raspi3_init(MachineState *machine)
 
 static void raspi3_machine_init(MachineClass *mc)
 {
-    mc->desc = "Raspberry Pi 3";
+    mc->desc = "Raspberry Pi 3B";
     mc->init = raspi3_init;
     mc->block_default_type = IF_SD;
     mc->no_parallel = 1;
-- 
2.21.1

[PATCH v3 03/13] hw/arm/raspi: Extract the version from the board revision

[Philippe Mathieu-Daudé]

The board revision encode the board version. Add a helper
to extract the version, and use it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/raspi.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 818146fdbb..f285e2988f 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -16,6 +16,7 @@
 #include "qapi/error.h"
 #include "cpu.h"
 #include "hw/arm/bcm2836.h"
+#include "hw/registerfields.h"
 #include "qemu/error-report.h"
 #include "hw/boards.h"
 #include "hw/loader.h"
@@ -37,6 +38,28 @@ typedef struct RasPiState {
     MemoryRegion ram;
 } RasPiState;
 
+/*
+ * Board revision codes:
+ * www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/
+ */
+FIELD(REV_CODE, REVISION,           0, 4);
+FIELD(REV_CODE, TYPE,               4, 8);
+FIELD(REV_CODE, PROCESSOR,         12, 4);
+FIELD(REV_CODE, MANUFACTURER,      16, 4);
+FIELD(REV_CODE, MEMORY_SIZE,       20, 3);
+FIELD(REV_CODE, STYLE,             23, 1);
+
+static int board_processor_id(uint32_t board_rev)
+{
+    assert(FIELD_EX32(board_rev, REV_CODE, STYLE)); /* Only new style */
+    return FIELD_EX32(board_rev, REV_CODE, PROCESSOR);
+}
+
+static int board_version(uint32_t board_rev)
+{
+    return board_processor_id(board_rev) + 1;
+}
+
 static void write_smpboot(ARMCPU *cpu, const struct arm_boot_info *info)
 {
     static const uint32_t smpboot[] = {
@@ -164,9 +187,10 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
     arm_load_kernel(ARM_CPU(first_cpu), machine, &binfo);
 }
 
-static void raspi_init(MachineState *machine, int version)
+static void raspi_init(MachineState *machine, uint32_t board_rev)
 {
     RasPiState *s = g_new0(RasPiState, 1);
+    int version = board_version(board_rev);
     uint32_t vcram_size;
     DriveInfo *di;
     BlockBackend *blk;
@@ -192,7 +216,6 @@ static void raspi_init(MachineState *machine, int version)
     /* Setup the SOC */
     object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
                                    &error_abort);
-    int board_rev = version == 3 ? 0xa02082 : 0xa21041;
     object_property_set_int(OBJECT(&s->soc), board_rev, "board-rev",
                             &error_abort);
     object_property_set_bool(OBJECT(&s->soc), true, "realized", &error_abort);
@@ -216,7 +239,7 @@ static void raspi_init(MachineState *machine, int version)
 
 static void raspi2_init(MachineState *machine)
 {
-    raspi_init(machine, 2);
+    raspi_init(machine, 0xa21041);
 }
 
 static void raspi2_machine_init(MachineClass *mc)
@@ -238,7 +261,7 @@ DEFINE_MACHINE("raspi2", raspi2_machine_init)
 #ifdef TARGET_AARCH64
 static void raspi3_init(MachineState *machine)
 {
-    raspi_init(machine, 3);
+    raspi_init(machine, 0xa02082);
 }
 
 static void raspi3_machine_init(MachineClass *mc)
-- 
2.21.1

[PATCH v3 04/13] hw/arm/raspi: Extract the RAM size from the board revision

[Philippe Mathieu-Daudé]

The board revision encode the amount of RAM. Add a helper
to extract the RAM size, and use it.
Since the amount of RAM is fixed (it is impossible to physically
modify to have more or less RAM), do not allow sizes different
than the one anounced by the manufacturer.

Acked-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
v3: Try to make error message clearer (Zoltan)
I used the same string as "ppc/ppc405_boards: add RAM size checks"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg675801.html
---
 hw/arm/raspi.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index f285e2988f..dcd8d2d6d3 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -13,6 +13,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "cpu.h"
 #include "hw/arm/bcm2836.h"
@@ -49,6 +50,12 @@ FIELD(REV_CODE, MANUFACTURER,      16, 4);
 FIELD(REV_CODE, MEMORY_SIZE,       20, 3);
 FIELD(REV_CODE, STYLE,             23, 1);
 
+static uint64_t board_ram_size(uint32_t board_rev)
+{
+    assert(FIELD_EX32(board_rev, REV_CODE, STYLE)); /* Only new style */
+    return 256 * MiB << FIELD_EX32(board_rev, REV_CODE, MEMORY_SIZE);
+}
+
 static int board_processor_id(uint32_t board_rev)
 {
     assert(FIELD_EX32(board_rev, REV_CODE, STYLE)); /* Only new style */
@@ -191,15 +198,17 @@ static void raspi_init(MachineState *machine, uint32_t board_rev)
 {
     RasPiState *s = g_new0(RasPiState, 1);
     int version = board_version(board_rev);
+    uint64_t ram_size = board_ram_size(board_rev);
     uint32_t vcram_size;
     DriveInfo *di;
     BlockBackend *blk;
     BusState *bus;
     DeviceState *carddev;
 
-    if (machine->ram_size > 1 * GiB) {
-        error_report("Requested ram size is too large for this machine: "
-                     "maximum is 1GB");
+    if (machine->ram_size != ram_size) {
+        char *size_str = size_to_str(ram_size);
+        error_report("Invalid RAM size, should be %s", size_str);
+        g_free(size_str);
         exit(1);
     }
 
-- 
2.21.1

[PATCH v3 05/13] hw/arm/raspi: Extract the processor type from the board revision

[Philippe Mathieu-Daudé]

The board revision encode the processor type. Add a helper
to extract the type, and use it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/raspi.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index dcd8d2d6d3..7a2ca97347 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -67,6 +67,21 @@ static int board_version(uint32_t board_rev)
     return board_processor_id(board_rev) + 1;
 }
 
+static const char *board_soc_type(uint32_t board_rev)
+{
+    static const char *soc_types[] = {
+        NULL, TYPE_BCM2836, TYPE_BCM2837,
+    };
+    int proc_id = board_processor_id(board_rev);
+
+    if (proc_id >= ARRAY_SIZE(soc_types) || !soc_types[proc_id]) {
+        error_report("Unsupported processor id '%d' (board revision: 0x%x)",
+                     proc_id, board_rev);
+        exit(1);
+    }
+    return soc_types[proc_id];
+}
+
 static void write_smpboot(ARMCPU *cpu, const struct arm_boot_info *info)
 {
     static const uint32_t smpboot[] = {
@@ -213,8 +228,7 @@ static void raspi_init(MachineState *machine, uint32_t board_rev)
     }
 
     object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            version == 3 ? TYPE_BCM2837 : TYPE_BCM2836,
-                            &error_abort, NULL);
+                            board_soc_type(board_rev), &error_abort, NULL);
 
     /* Allocate and map RAM */
     memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
-- 
2.21.1

[PATCH v3 06/13] hw/arm/raspi: Trivial code movement

[Philippe Mathieu-Daudé]

There is no point in creating the SoC object before allocating the RAM.
Move the call to keep all the SoC-related calls together.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/raspi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 7a2ca97347..b3e6f72b55 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -227,9 +227,6 @@ static void raspi_init(MachineState *machine, uint32_t board_rev)
         exit(1);
     }
 
-    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
-                            board_soc_type(board_rev), &error_abort, NULL);
-
     /* Allocate and map RAM */
     memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
                                          machine->ram_size);
@@ -237,6 +234,8 @@ static void raspi_init(MachineState *machine, uint32_t board_rev)
     memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram, 0);
 
     /* Setup the SOC */
+    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
+                            board_soc_type(board_rev), &error_abort, NULL);
     object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
                                    &error_abort);
     object_property_set_int(OBJECT(&s->soc), board_rev, "board-rev",
-- 
2.21.1

[PATCH v3 07/13] hw/arm/raspi: Make machines children of abstract RaspiMachineClass

[Philippe Mathieu-Daudé]

QOM'ify RaspiMachineState. Now machines inherit of RaspiMachineClass.

Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
v3: Fixed typo in description (Zoltán)
---
 hw/arm/raspi.c | 56 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 49 insertions(+), 7 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index b3e6f72b55..62b8df3c2e 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -34,10 +34,28 @@
 /* Registered machine type (matches RPi Foundation bootloader and U-Boot) */
 #define MACH_TYPE_BCM2708   3138
 
-typedef struct RasPiState {
+typedef struct RaspiMachineState {
+    /*< private >*/
+    MachineState parent_obj;
+    /*< public >*/
     BCM283XState soc;
     MemoryRegion ram;
-} RasPiState;
+} RaspiMachineState;
+
+typedef struct RaspiMachineClass {
+    /*< private >*/
+    MachineClass parent_obj;
+    /*< public >*/
+} RaspiMachineClass;
+
+#define TYPE_RASPI_MACHINE       MACHINE_TYPE_NAME("raspi-common")
+#define RASPI_MACHINE(obj) \
+    OBJECT_CHECK(RaspiMachineState, (obj), TYPE_RASPI_MACHINE)
+
+#define RASPI_MACHINE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(RaspiMachineClass, (klass), TYPE_RASPI_MACHINE)
+#define RASPI_MACHINE_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(RaspiMachineClass, (obj), TYPE_RASPI_MACHINE)
 
 /*
  * Board revision codes:
@@ -211,7 +229,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
 
 static void raspi_init(MachineState *machine, uint32_t board_rev)
 {
-    RasPiState *s = g_new0(RasPiState, 1);
+    RaspiMachineState *s = RASPI_MACHINE(machine);
     int version = board_version(board_rev);
     uint64_t ram_size = board_ram_size(board_rev);
     uint32_t vcram_size;
@@ -264,8 +282,10 @@ static void raspi2_init(MachineState *machine)
     raspi_init(machine, 0xa21041);
 }
 
-static void raspi2_machine_init(MachineClass *mc)
+static void raspi2_machine_class_init(ObjectClass *oc, void *data)
 {
+    MachineClass *mc = MACHINE_CLASS(oc);
+
     mc->desc = "Raspberry Pi 2B";
     mc->init = raspi2_init;
     mc->block_default_type = IF_SD;
@@ -278,7 +298,6 @@ static void raspi2_machine_init(MachineClass *mc)
     mc->default_ram_size = 1 * GiB;
     mc->ignore_memory_transaction_failures = true;
 };
-DEFINE_MACHINE("raspi2", raspi2_machine_init)
 
 #ifdef TARGET_AARCH64
 static void raspi3_init(MachineState *machine)
@@ -286,8 +305,10 @@ static void raspi3_init(MachineState *machine)
     raspi_init(machine, 0xa02082);
 }
 
-static void raspi3_machine_init(MachineClass *mc)
+static void raspi3_machine_class_init(ObjectClass *oc, void *data)
 {
+    MachineClass *mc = MACHINE_CLASS(oc);
+
     mc->desc = "Raspberry Pi 3B";
     mc->init = raspi3_init;
     mc->block_default_type = IF_SD;
@@ -299,5 +320,26 @@ static void raspi3_machine_init(MachineClass *mc)
     mc->default_cpus = BCM283X_NCPUS;
     mc->default_ram_size = 1 * GiB;
 }
-DEFINE_MACHINE("raspi3", raspi3_machine_init)
 #endif
+
+static const TypeInfo raspi_machine_types[] = {
+    {
+        .name           = MACHINE_TYPE_NAME("raspi2"),
+        .parent         = TYPE_RASPI_MACHINE,
+        .class_init     = raspi2_machine_class_init,
+#ifdef TARGET_AARCH64
+    }, {
+        .name           = MACHINE_TYPE_NAME("raspi3"),
+        .parent         = TYPE_RASPI_MACHINE,
+        .class_init     = raspi3_machine_class_init,
+#endif
+    }, {
+        .name           = TYPE_RASPI_MACHINE,
+        .parent         = TYPE_MACHINE,
+        .instance_size  = sizeof(RaspiMachineState),
+        .class_size     = sizeof(RaspiMachineClass),
+        .abstract       = true,
+    }
+};
+
+DEFINE_TYPES(raspi_machine_types)
-- 
2.21.1

[PATCH v3 08/13] hw/arm/raspi: Make board_rev a field of RaspiMachineClass

[Philippe Mathieu-Daudé]

We want to have a common class_init(). The only value that
matters (and changes) is the board revision.
Pass the board_rev as class_data to class_init().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/raspi.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 62b8df3c2e..fbfcd29732 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -46,6 +46,7 @@ typedef struct RaspiMachineClass {
     /*< private >*/
     MachineClass parent_obj;
     /*< public >*/
+    uint32_t board_rev;
 } RaspiMachineClass;
 
 #define TYPE_RASPI_MACHINE       MACHINE_TYPE_NAME("raspi-common")
@@ -227,9 +228,11 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
     arm_load_kernel(ARM_CPU(first_cpu), machine, &binfo);
 }
 
-static void raspi_init(MachineState *machine, uint32_t board_rev)
+static void raspi_init(MachineState *machine)
 {
+    RaspiMachineClass *mc = RASPI_MACHINE_GET_CLASS(machine);
     RaspiMachineState *s = RASPI_MACHINE(machine);
+    uint32_t board_rev = mc->board_rev;
     int version = board_version(board_rev);
     uint64_t ram_size = board_ram_size(board_rev);
     uint32_t vcram_size;
@@ -279,13 +282,16 @@ static void raspi_init(MachineState *machine, uint32_t board_rev)
 
 static void raspi2_init(MachineState *machine)
 {
-    raspi_init(machine, 0xa21041);
+    raspi_init(machine);
 }
 
 static void raspi2_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
+    uint32_t board_rev = (uint32_t)(uintptr_t)data;
 
+    rmc->board_rev = board_rev;
     mc->desc = "Raspberry Pi 2B";
     mc->init = raspi2_init;
     mc->block_default_type = IF_SD;
@@ -302,13 +308,16 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
 #ifdef TARGET_AARCH64
 static void raspi3_init(MachineState *machine)
 {
-    raspi_init(machine, 0xa02082);
+    raspi_init(machine);
 }
 
 static void raspi3_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
+    uint32_t board_rev = (uint32_t)(uintptr_t)data;
 
+    rmc->board_rev = board_rev;
     mc->desc = "Raspberry Pi 3B";
     mc->init = raspi3_init;
     mc->block_default_type = IF_SD;
@@ -327,11 +336,13 @@ static const TypeInfo raspi_machine_types[] = {
         .name           = MACHINE_TYPE_NAME("raspi2"),
         .parent         = TYPE_RASPI_MACHINE,
         .class_init     = raspi2_machine_class_init,
+        .class_data     = (void *)0xa21041,
 #ifdef TARGET_AARCH64
     }, {
         .name           = MACHINE_TYPE_NAME("raspi3"),
         .parent         = TYPE_RASPI_MACHINE,
         .class_init     = raspi3_machine_class_init,
+        .class_data     = (void *)0xa02082,
 #endif
     }, {
         .name           = TYPE_RASPI_MACHINE,
-- 
2.21.1

[PATCH v3 09/13] hw/arm/raspi: Let class_init() directly call raspi_machine_init()

[Philippe Mathieu-Daudé]

raspi_machine_init() access to board_rev via RaspiMachineClass.
raspi2_init() and raspi3_init() do nothing. Call raspi_machine_init
directly.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Squash with previous?
---
 hw/arm/raspi.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index fbfcd29732..1628b0dda7 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -228,7 +228,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
     arm_load_kernel(ARM_CPU(first_cpu), machine, &binfo);
 }
 
-static void raspi_init(MachineState *machine)
+static void raspi_machine_init(MachineState *machine)
 {
     RaspiMachineClass *mc = RASPI_MACHINE_GET_CLASS(machine);
     RaspiMachineState *s = RASPI_MACHINE(machine);
@@ -280,11 +280,6 @@ static void raspi_init(MachineState *machine)
     setup_boot(machine, version, machine->ram_size - vcram_size);
 }
 
-static void raspi2_init(MachineState *machine)
-{
-    raspi_init(machine);
-}
-
 static void raspi2_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -293,7 +288,7 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
 
     rmc->board_rev = board_rev;
     mc->desc = "Raspberry Pi 2B";
-    mc->init = raspi2_init;
+    mc->init = raspi_machine_init;
     mc->block_default_type = IF_SD;
     mc->no_parallel = 1;
     mc->no_floppy = 1;
@@ -306,11 +301,6 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
 };
 
 #ifdef TARGET_AARCH64
-static void raspi3_init(MachineState *machine)
-{
-    raspi_init(machine);
-}
-
 static void raspi3_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -319,7 +309,7 @@ static void raspi3_machine_class_init(ObjectClass *oc, void *data)
 
     rmc->board_rev = board_rev;
     mc->desc = "Raspberry Pi 3B";
-    mc->init = raspi3_init;
+    mc->init = raspi_machine_init;
     mc->block_default_type = IF_SD;
     mc->no_parallel = 1;
     mc->no_floppy = 1;
-- 
2.21.1

[PATCH v3 10/13] hw/arm/raspi: Set default RAM size to size encoded in board revision

[Philippe Mathieu-Daudé]

We added a helper to extract the RAM size from the board
revision, and made board_rev a field of RaspiMachineClass.
The class_init() can now use the helper to extract from the
board revision the board-specific amount of RAM.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/raspi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 1628b0dda7..f0dcffbc2e 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -296,7 +296,7 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = BCM283X_NCPUS;
     mc->min_cpus = BCM283X_NCPUS;
     mc->default_cpus = BCM283X_NCPUS;
-    mc->default_ram_size = 1 * GiB;
+    mc->default_ram_size = board_ram_size(board_rev);
     mc->ignore_memory_transaction_failures = true;
 };
 
@@ -317,7 +317,7 @@ static void raspi3_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = BCM283X_NCPUS;
     mc->min_cpus = BCM283X_NCPUS;
     mc->default_cpus = BCM283X_NCPUS;
-    mc->default_ram_size = 1 * GiB;
+    mc->default_ram_size = board_ram_size(board_rev);
 }
 #endif
 
-- 
2.21.1

[PATCH v3 11/13] hw/arm/raspi: Extract the board model from the board revision

[Philippe Mathieu-Daudé]

The board revision encode the model type. Add a helper
to extract the model, and use it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/raspi.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index f0dcffbc2e..0537fc0a2d 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -101,6 +101,20 @@ static const char *board_soc_type(uint32_t board_rev)
     return soc_types[proc_id];
 }
 
+static const char *board_type(uint32_t board_rev)
+{
+    static const char *types[] = {
+        "A", "B", "A+", "B+", "2B", "Alpha", "CM1", NULL, "3B", "Zero",
+        "CM3", NULL, "Zero W", "3B+", "3A+", NULL, "CM3+", "4B",
+    };
+    assert(FIELD_EX32(board_rev, REV_CODE, STYLE)); /* Only new style */
+    int bt = FIELD_EX32(board_rev, REV_CODE, TYPE);
+    if (bt >= ARRAY_SIZE(types) || !types[bt]) {
+        return "Unknown";
+    }
+    return types[bt];
+}
+
 static void write_smpboot(ARMCPU *cpu, const struct arm_boot_info *info)
 {
     static const uint32_t smpboot[] = {
@@ -287,7 +301,7 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
     uint32_t board_rev = (uint32_t)(uintptr_t)data;
 
     rmc->board_rev = board_rev;
-    mc->desc = "Raspberry Pi 2B";
+    mc->desc = g_strdup_printf("Raspberry Pi %s", board_type(board_rev));
     mc->init = raspi_machine_init;
     mc->block_default_type = IF_SD;
     mc->no_parallel = 1;
@@ -308,7 +322,7 @@ static void raspi3_machine_class_init(ObjectClass *oc, void *data)
     uint32_t board_rev = (uint32_t)(uintptr_t)data;
 
     rmc->board_rev = board_rev;
-    mc->desc = "Raspberry Pi 3B";
+    mc->desc = g_strdup_printf("Raspberry Pi %s", board_type(board_rev));
     mc->init = raspi_machine_init;
     mc->block_default_type = IF_SD;
     mc->no_parallel = 1;
-- 
2.21.1

[PATCH v3 12/13] hw/arm/raspi: Use a unique raspi_machine_class_init() method

[Philippe Mathieu-Daudé]

With the exception of the ignore_memory_transaction_failures
flag set for the raspi2, both machine_class_init() methods
are now identical. Merge them to keep a unique method.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/raspi.c | 31 ++++++-------------------------
 1 file changed, 6 insertions(+), 25 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 0537fc0a2d..bee6ca0a08 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -294,7 +294,7 @@ static void raspi_machine_init(MachineState *machine)
     setup_boot(machine, version, machine->ram_size - vcram_size);
 }
 
-static void raspi2_machine_class_init(ObjectClass *oc, void *data)
+static void raspi_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
@@ -311,41 +311,22 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
     mc->min_cpus = BCM283X_NCPUS;
     mc->default_cpus = BCM283X_NCPUS;
     mc->default_ram_size = board_ram_size(board_rev);
-    mc->ignore_memory_transaction_failures = true;
+    if (board_version(board_rev) == 2) {
+        mc->ignore_memory_transaction_failures = true;
+    }
 };
 
-#ifdef TARGET_AARCH64
-static void raspi3_machine_class_init(ObjectClass *oc, void *data)
-{
-    MachineClass *mc = MACHINE_CLASS(oc);
-    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
-    uint32_t board_rev = (uint32_t)(uintptr_t)data;
-
-    rmc->board_rev = board_rev;
-    mc->desc = g_strdup_printf("Raspberry Pi %s", board_type(board_rev));
-    mc->init = raspi_machine_init;
-    mc->block_default_type = IF_SD;
-    mc->no_parallel = 1;
-    mc->no_floppy = 1;
-    mc->no_cdrom = 1;
-    mc->max_cpus = BCM283X_NCPUS;
-    mc->min_cpus = BCM283X_NCPUS;
-    mc->default_cpus = BCM283X_NCPUS;
-    mc->default_ram_size = board_ram_size(board_rev);
-}
-#endif
-
 static const TypeInfo raspi_machine_types[] = {
     {
         .name           = MACHINE_TYPE_NAME("raspi2"),
         .parent         = TYPE_RASPI_MACHINE,
-        .class_init     = raspi2_machine_class_init,
+        .class_init     = raspi_machine_class_init,
         .class_data     = (void *)0xa21041,
 #ifdef TARGET_AARCH64
     }, {
         .name           = MACHINE_TYPE_NAME("raspi3"),
         .parent         = TYPE_RASPI_MACHINE,
-        .class_init     = raspi3_machine_class_init,
+        .class_init     = raspi_machine_class_init,
         .class_data     = (void *)0xa02082,
 #endif
     }, {
-- 
2.21.1

[PATCH v3 13/13] hw/arm/raspi: Extract the cores count from the board revision

[Philippe Mathieu-Daudé]

The board revision encode the count of ARM cores. Add a helper
to extract the number of cores, and use it. This will be helpful
when we add the Raspi0/1 that have a single core.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/raspi.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index bee6ca0a08..90ad9b8115 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -101,6 +101,21 @@ static const char *board_soc_type(uint32_t board_rev)
     return soc_types[proc_id];
 }
 
+static int cores_count(uint32_t board_rev)
+{
+    static const int soc_cores_count[] = {
+        0, BCM283X_NCPUS, BCM283X_NCPUS,
+    };
+    int proc_id = board_processor_id(board_rev);
+
+    if (proc_id >= ARRAY_SIZE(soc_cores_count) || !soc_cores_count[proc_id]) {
+        error_report("Unsupported processor id '%d' (board revision: 0x%x)",
+                     proc_id, board_rev);
+        exit(1);
+    }
+    return soc_cores_count[proc_id];
+}
+
 static const char *board_type(uint32_t board_rev)
 {
     static const char *types[] = {
@@ -307,9 +322,7 @@ static void raspi_machine_class_init(ObjectClass *oc, void *data)
     mc->no_parallel = 1;
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
-    mc->max_cpus = BCM283X_NCPUS;
-    mc->min_cpus = BCM283X_NCPUS;
-    mc->default_cpus = BCM283X_NCPUS;
+    mc->default_cpus = mc->min_cpus = mc->max_cpus = cores_count(board_rev);
     mc->default_ram_size = board_ram_size(board_rev);
     if (board_version(board_rev) == 2) {
         mc->ignore_memory_transaction_failures = true;
-- 
2.21.1

Re: [PATCH v3 02/13] hw/arm/raspi: Correct the board descriptions

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]

Hi Philippe,


On Sat, Feb 8, 2020 at 5:57 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> We hardcode the board revision as 0xa21041 for the raspi2, and
> 0xa02082 for the raspi3:
>
>   166 static void raspi_init(MachineState *machine, int version)
>   167 {
>   ...
>   194     int board_rev = version == 3 ? 0xa02082 : 0xa21041;
>
> These revision codes are for the 2B and 3B models, see:
>
> https://www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
>
> Correct the board description.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/raspi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index f2ccabc662..818146fdbb 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -221,7 +221,7 @@ static void raspi2_init(MachineState *machine)
>
>  static void raspi2_machine_init(MachineClass *mc)
>  {
> -    mc->desc = "Raspberry Pi 2";
> +    mc->desc = "Raspberry Pi 2B";
>      mc->init = raspi2_init;
>      mc->block_default_type = IF_SD;
>      mc->no_parallel = 1;
> @@ -243,7 +243,7 @@ static void raspi3_init(MachineState *machine)
>
>  static void raspi3_machine_init(MachineClass *mc)
>  {
> -    mc->desc = "Raspberry Pi 3";
> +    mc->desc = "Raspberry Pi 3B";
>

Could this patch be replaced by patch #11 "hw/arm/raspi: Extract the board
model from the board revision"?

Regards,
Niek


>      mc->init = raspi3_init;
>      mc->block_default_type = IF_SD;
>      mc->no_parallel = 1;
> --
> 2.21.1
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 2649 bytes --]

Re: [PATCH v3 01/13] hw/arm/raspi: Use BCM2708 machine type with pre Device Tree kernels

[Niek Linnenbank]

[-- Attachment #1: Type: text/plain, Size: 4348 bytes --]

On Sat, Feb 8, 2020 at 5:57 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> When booting without device tree, the Linux kernels uses the $R1
> register to determine the machine type. The list of values is
> registered at [1].
>
> There are two entries for the Raspberry Pi:
>
> - https://www.arm.linux.org.uk/developer/machines/list.php?mid=3138
>   name: MACH_TYPE_BCM2708
>   value: 0xc42 (3138)
>   status: Active, not mainlined
>   date: 15 Oct 2010
>
> - https://www.arm.linux.org.uk/developer/machines/list.php?mid=4828
>   name: MACH_TYPE_BCM2835
>   value: 4828
>   status: Active, mainlined
>   date: 6 Dec 2013
>
> QEMU always used the non-mainlined type MACH_TYPE_BCM2708.
> The value 0xc43 is registered to 'MX51_GGC' (processor i.MX51), and
> 0xc44 to 'Western Digital Sharespace NAS' (processor Marvell 88F5182).
>
> The Raspberry Pi foundation bootloader only sets the BCM2708 machine
> type, see [2] or [3]:
>
>  133 9:
>  134     mov r0, #0
>  135     ldr r1, =3138       @ BCM2708 machine id
>  136     ldr r2, atags       @ ATAGS
>  137     bx  r4
>
> U-Boot only uses MACH_TYPE_BCM2708 (see [4]):
>
>  25 /*
>  26  * 2835 is a SKU in a series for which the 2708 is the first or
> primary SoC,
>  27  * so 2708 has historically been used rather than a dedicated 2835 ID.
>  28  *
>  29  * We don't define a machine type for bcm2709/bcm2836 since the RPi
> Foundation
>  30  * chose to use someone else's previously registered machine ID (3139,
> MX51_GGC)
>  31  * rather than obtaining a valid ID:-/
>  32  *
>  33  * For the bcm2837, hopefully a machine type is not needed, since
> everything
>  34  * is DT.
>  35  */
>
> While the definition MACH_BCM2709 with value 0xc43 was introduced in
> a commit described "Add 2709 platform for Raspberry Pi 2" out of the
> mainline Linux kernel, it does not seem used, and the platform is
> introduced with Device Tree support anyway (see [5] and [6]).
>
> Remove the unused values (0xc43 introduced in commit 1df7d1f9303aef
> "raspi: add raspberry pi 2 machine" and 0xc44 in commit bade58166f4
> "raspi: Raspberry Pi 3 support"), keeping only MACH_TYPE_BCM2708.
>
> [1] https://www.arm.linux.org.uk/developer/machines/
> [2]
> https://github.com/raspberrypi/tools/blob/920c7ed2e/armstubs/armstub7.S#L135
> [3]
> https://github.com/raspberrypi/tools/blob/49719d554/armstubs/armstub7.S#L64
> [4]
> https://gitlab.denx.de/u-boot/u-boot/blob/v2015.04/include/configs/rpi-common.h#L18
> [5]
> https://github.com/raspberrypi/linux/commit/d9fac63adac#diff-6722037d79570df5b392a49e0e006573R526
> [6]
> http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-February/001268.html
>
> Cc: Zoltán Baldaszti <bztemail@gmail.com>
> Cc: Pekka Enberg <penberg@iki.fi>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Kshitij Soni <kshitij.soni@broadcom.com>
> Cc: Michael Chan <michael.chan@broadcom.com>
> Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>

Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>


> ---
> v3: Improved MACH_TYPE_BCM2708 comment (Zoltán)
> ---
>  hw/arm/raspi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 3996f6c63a..f2ccabc662 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -29,8 +29,8 @@
>  #define FIRMWARE_ADDR_3 0x80000 /* Pi 3 loads kernel.img here by default
> */
>  #define SPINTABLE_ADDR  0xd8 /* Pi 3 bootloader spintable */
>
> -/* Table of Linux board IDs for different Pi versions */
> -static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] =
> 0xc44};
> +/* Registered machine type (matches RPi Foundation bootloader and U-Boot)
> */
> +#define MACH_TYPE_BCM2708   3138
>
>  typedef struct RasPiState {
>      BCM283XState soc;
> @@ -116,7 +116,7 @@ static void setup_boot(MachineState *machine, int
> version, size_t ram_size)
>      static struct arm_boot_info binfo;
>      int r;
>
> -    binfo.board_id = raspi_boardid[version];
> +    binfo.board_id = MACH_TYPE_BCM2708;
>      binfo.ram_size = ram_size;
>      binfo.nb_cpus = machine->smp.cpus;
>
> --
> 2.21.1
>
>

-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 6779 bytes --]

Re: [PATCH v3 02/13] hw/arm/raspi: Correct the board descriptions

[Philippe Mathieu-Daudé]

On 2/9/20 11:51 PM, Niek Linnenbank wrote:
> Hi Philippe,
> 
> 
> On Sat, Feb 8, 2020 at 5:57 PM Philippe Mathieu-Daudé <f4bug@amsat.org
> <mailto:f4bug@amsat.org>> wrote:
> 
>     We hardcode the board revision as 0xa21041 for the raspi2, and
>     0xa02082 for the raspi3:
> 
>       166 static void raspi_init(MachineState *machine, int version)
>       167 {
>       ...
>       194     int board_rev = version == 3 ? 0xa02082 : 0xa21041;
> 
>     These revision codes are for the 2B and 3B models, see:
>     https://www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md
> 
>     Correct the board description.
> 
>     Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org
>     <mailto:f4bug@amsat.org>>
>     ---
>      hw/arm/raspi.c | 4 ++--
>      1 file changed, 2 insertions(+), 2 deletions(-)
> 
>     diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>     index f2ccabc662..818146fdbb 100644
>     --- a/hw/arm/raspi.c
>     +++ b/hw/arm/raspi.c
>     @@ -221,7 +221,7 @@ static void raspi2_init(MachineState *machine)
> 
>      static void raspi2_machine_init(MachineClass *mc)
>      {
>     -    mc->desc = "Raspberry Pi 2";
>     +    mc->desc = "Raspberry Pi 2B";
>          mc->init = raspi2_init;
>          mc->block_default_type = IF_SD;
>          mc->no_parallel = 1;
>     @@ -243,7 +243,7 @@ static void raspi3_init(MachineState *machine)
> 
>      static void raspi3_machine_init(MachineClass *mc)
>      {
>     -    mc->desc = "Raspberry Pi 3";
>     +    mc->desc = "Raspberry Pi 3B";
> 
> 
> Could this patch be replaced by patch #11 "hw/arm/raspi: Extract the
> board model from the board revision"?

It has to be changed before patch #8, and while patch #8 is tiny, it is
complex. I prefer to keep #8 as simple as possible, by making this
trivial change first (I don't want to do 2 different changes in the same
patch). If you want I can move this #2 as #7 just before #8, but I'm not
sure this makes things clearer.
> 
>          mc->init = raspi3_init;
>          mc->block_default_type = IF_SD;
>          mc->no_parallel = 1;
>     -- 
>     2.21.1
> 
> 
> 
> -- 
> Niek Linnenbank
> 

Re: [PATCH v3 07/13] hw/arm/raspi: Make machines children of abstract RaspiMachineClass

[Igor Mammedov]

On Sat,  8 Feb 2020 17:56:39 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> QOM'ify RaspiMachineState. Now machines inherit of RaspiMachineClass.
> 
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Acked-by: Igor Mammedov <imammedo@redhat.com>

> ---
> v3: Fixed typo in description (Zoltán)
> ---
>  hw/arm/raspi.c | 56 +++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 49 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index b3e6f72b55..62b8df3c2e 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -34,10 +34,28 @@
>  /* Registered machine type (matches RPi Foundation bootloader and U-Boot) */
>  #define MACH_TYPE_BCM2708   3138
>  
> -typedef struct RasPiState {
> +typedef struct RaspiMachineState {
> +    /*< private >*/
> +    MachineState parent_obj;
> +    /*< public >*/
>      BCM283XState soc;
>      MemoryRegion ram;
> -} RasPiState;
> +} RaspiMachineState;
> +
> +typedef struct RaspiMachineClass {
> +    /*< private >*/
> +    MachineClass parent_obj;
> +    /*< public >*/
> +} RaspiMachineClass;
> +
> +#define TYPE_RASPI_MACHINE       MACHINE_TYPE_NAME("raspi-common")
> +#define RASPI_MACHINE(obj) \
> +    OBJECT_CHECK(RaspiMachineState, (obj), TYPE_RASPI_MACHINE)
> +
> +#define RASPI_MACHINE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(RaspiMachineClass, (klass), TYPE_RASPI_MACHINE)
> +#define RASPI_MACHINE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(RaspiMachineClass, (obj), TYPE_RASPI_MACHINE)
>  
>  /*
>   * Board revision codes:
> @@ -211,7 +229,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
>  
>  static void raspi_init(MachineState *machine, uint32_t board_rev)
>  {
> -    RasPiState *s = g_new0(RasPiState, 1);
> +    RaspiMachineState *s = RASPI_MACHINE(machine);
>      int version = board_version(board_rev);
>      uint64_t ram_size = board_ram_size(board_rev);
>      uint32_t vcram_size;
> @@ -264,8 +282,10 @@ static void raspi2_init(MachineState *machine)
>      raspi_init(machine, 0xa21041);
>  }
>  
> -static void raspi2_machine_init(MachineClass *mc)
> +static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>  {
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
>      mc->desc = "Raspberry Pi 2B";
>      mc->init = raspi2_init;
>      mc->block_default_type = IF_SD;
> @@ -278,7 +298,6 @@ static void raspi2_machine_init(MachineClass *mc)
>      mc->default_ram_size = 1 * GiB;
>      mc->ignore_memory_transaction_failures = true;
>  };
> -DEFINE_MACHINE("raspi2", raspi2_machine_init)
>  
>  #ifdef TARGET_AARCH64
>  static void raspi3_init(MachineState *machine)
> @@ -286,8 +305,10 @@ static void raspi3_init(MachineState *machine)
>      raspi_init(machine, 0xa02082);
>  }
>  
> -static void raspi3_machine_init(MachineClass *mc)
> +static void raspi3_machine_class_init(ObjectClass *oc, void *data)
>  {
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
>      mc->desc = "Raspberry Pi 3B";
>      mc->init = raspi3_init;
>      mc->block_default_type = IF_SD;
> @@ -299,5 +320,26 @@ static void raspi3_machine_init(MachineClass *mc)
>      mc->default_cpus = BCM283X_NCPUS;
>      mc->default_ram_size = 1 * GiB;
>  }
> -DEFINE_MACHINE("raspi3", raspi3_machine_init)
>  #endif
> +
> +static const TypeInfo raspi_machine_types[] = {
> +    {
> +        .name           = MACHINE_TYPE_NAME("raspi2"),
> +        .parent         = TYPE_RASPI_MACHINE,
> +        .class_init     = raspi2_machine_class_init,
> +#ifdef TARGET_AARCH64
> +    }, {
> +        .name           = MACHINE_TYPE_NAME("raspi3"),
> +        .parent         = TYPE_RASPI_MACHINE,
> +        .class_init     = raspi3_machine_class_init,
> +#endif
> +    }, {
> +        .name           = TYPE_RASPI_MACHINE,
> +        .parent         = TYPE_MACHINE,
> +        .instance_size  = sizeof(RaspiMachineState),
> +        .class_size     = sizeof(RaspiMachineClass),
> +        .abstract       = true,
> +    }
> +};
> +
> +DEFINE_TYPES(raspi_machine_types)

Re: [PATCH v3 08/13] hw/arm/raspi: Make board_rev a field of RaspiMachineClass

[Igor Mammedov]

On Sat,  8 Feb 2020 17:56:40 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> We want to have a common class_init(). The only value that
> matters (and changes) is the board revision.
> Pass the board_rev as class_data to class_init().
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/raspi.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 62b8df3c2e..fbfcd29732 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -46,6 +46,7 @@ typedef struct RaspiMachineClass {
>      /*< private >*/
>      MachineClass parent_obj;
>      /*< public >*/
> +    uint32_t board_rev;
>  } RaspiMachineClass;
>  
>  #define TYPE_RASPI_MACHINE       MACHINE_TYPE_NAME("raspi-common")
> @@ -227,9 +228,11 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
>      arm_load_kernel(ARM_CPU(first_cpu), machine, &binfo);
>  }
>  
> -static void raspi_init(MachineState *machine, uint32_t board_rev)
> +static void raspi_init(MachineState *machine)
>  {
> +    RaspiMachineClass *mc = RASPI_MACHINE_GET_CLASS(machine);
>      RaspiMachineState *s = RASPI_MACHINE(machine);
> +    uint32_t board_rev = mc->board_rev;
>      int version = board_version(board_rev);
>      uint64_t ram_size = board_ram_size(board_rev);
>      uint32_t vcram_size;
> @@ -279,13 +282,16 @@ static void raspi_init(MachineState *machine, uint32_t board_rev)
>  
>  static void raspi2_init(MachineState *machine)
>  {
> -    raspi_init(machine, 0xa21041);
> +    raspi_init(machine);
>  }
>  
>  static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
>  
> +    rmc->board_rev = board_rev;

instead of doing a bit obscure ".class_data     = (void *)0xa21041," and
using it here, I'd just do

       rmc->board_rev = 0xa21041;

using value specific for each leaf class

with this change
 Reviewed-by: Igor Mammedov <imammedo@redhat.com>

>      mc->desc = "Raspberry Pi 2B";
>      mc->init = raspi2_init;
>      mc->block_default_type = IF_SD;
> @@ -302,13 +308,16 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>  #ifdef TARGET_AARCH64
>  static void raspi3_init(MachineState *machine)
>  {
> -    raspi_init(machine, 0xa02082);
> +    raspi_init(machine);
>  }
>  
>  static void raspi3_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
>  
> +    rmc->board_rev = board_rev;
>      mc->desc = "Raspberry Pi 3B";
>      mc->init = raspi3_init;
>      mc->block_default_type = IF_SD;
> @@ -327,11 +336,13 @@ static const TypeInfo raspi_machine_types[] = {
>          .name           = MACHINE_TYPE_NAME("raspi2"),
>          .parent         = TYPE_RASPI_MACHINE,
>          .class_init     = raspi2_machine_class_init,
> +        .class_data     = (void *)0xa21041,
>  #ifdef TARGET_AARCH64
>      }, {
>          .name           = MACHINE_TYPE_NAME("raspi3"),
>          .parent         = TYPE_RASPI_MACHINE,
>          .class_init     = raspi3_machine_class_init,
> +        .class_data     = (void *)0xa02082,
>  #endif
>      }, {
>          .name           = TYPE_RASPI_MACHINE,

Re: [PATCH v3 09/13] hw/arm/raspi: Let class_init() directly call raspi_machine_init()

[Igor Mammedov]

On Sat,  8 Feb 2020 17:56:41 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> raspi_machine_init() access to board_rev via RaspiMachineClass.
> raspi2_init() and raspi3_init() do nothing. Call raspi_machine_init
> directly.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Squash with previous?
> ---
>  hw/arm/raspi.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index fbfcd29732..1628b0dda7 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -228,7 +228,7 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
>      arm_load_kernel(ARM_CPU(first_cpu), machine, &binfo);
>  }
>  
> -static void raspi_init(MachineState *machine)
> +static void raspi_machine_init(MachineState *machine)
>  {
>      RaspiMachineClass *mc = RASPI_MACHINE_GET_CLASS(machine);
>      RaspiMachineState *s = RASPI_MACHINE(machine);
> @@ -280,11 +280,6 @@ static void raspi_init(MachineState *machine)
>      setup_boot(machine, version, machine->ram_size - vcram_size);
>  }
>  
> -static void raspi2_init(MachineState *machine)
> -{
> -    raspi_init(machine);
> -}
> -
>  static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -293,7 +288,7 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>  
>      rmc->board_rev = board_rev;
>      mc->desc = "Raspberry Pi 2B";
> -    mc->init = raspi2_init;
> +    mc->init = raspi_machine_init;

[...]
> @@ -319,7 +309,7 @@ static void raspi3_machine_class_init(ObjectClass *oc, void *data)
>  
>      rmc->board_rev = board_rev;
>      mc->desc = "Raspberry Pi 3B";
> -    mc->init = raspi3_init;
> +    mc->init = raspi_machine_init;
[...]

you could set it once in base class_init,
since there is not reason lest to do it per leaf class.

Either with this change or without

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

Re: [PATCH v3 06/13] hw/arm/raspi: Trivial code movement

[Igor Mammedov]

On Sat,  8 Feb 2020 17:56:38 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> There is no point in creating the SoC object before allocating the RAM.
> Move the call to keep all the SoC-related calls together.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Acked-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/arm/raspi.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 7a2ca97347..b3e6f72b55 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -227,9 +227,6 @@ static void raspi_init(MachineState *machine, uint32_t board_rev)
>          exit(1);
>      }
>  
> -    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> -                            board_soc_type(board_rev), &error_abort, NULL);
> -
>      /* Allocate and map RAM */
>      memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
>                                           machine->ram_size);
> @@ -237,6 +234,8 @@ static void raspi_init(MachineState *machine, uint32_t board_rev)
>      memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram, 0);
>  
>      /* Setup the SOC */
> +    object_initialize_child(OBJECT(machine), "soc", &s->soc, sizeof(s->soc),
> +                            board_soc_type(board_rev), &error_abort, NULL);
>      object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
>                                     &error_abort);
>      object_property_set_int(OBJECT(&s->soc), board_rev, "board-rev",

Re: [PATCH v3 12/13] hw/arm/raspi: Use a unique raspi_machine_class_init() method

[Igor Mammedov]

On Sat,  8 Feb 2020 17:56:44 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> With the exception of the ignore_memory_transaction_failures
> flag set for the raspi2, both machine_class_init() methods
> are now identical. Merge them to keep a unique method.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/arm/raspi.c | 31 ++++++-------------------------
>  1 file changed, 6 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 0537fc0a2d..bee6ca0a08 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -294,7 +294,7 @@ static void raspi_machine_init(MachineState *machine)
>      setup_boot(machine, version, machine->ram_size - vcram_size);
>  }
>  
> -static void raspi2_machine_class_init(ObjectClass *oc, void *data)
> +static void raspi_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
>      RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> @@ -311,41 +311,22 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>      mc->min_cpus = BCM283X_NCPUS;
>      mc->default_cpus = BCM283X_NCPUS;
>      mc->default_ram_size = board_ram_size(board_rev);
> -    mc->ignore_memory_transaction_failures = true;
> +    if (board_version(board_rev) == 2) {
> +        mc->ignore_memory_transaction_failures = true;
> +    }
>  };
>  
> -#ifdef TARGET_AARCH64
> -static void raspi3_machine_class_init(ObjectClass *oc, void *data)
> -{
> -    MachineClass *mc = MACHINE_CLASS(oc);
> -    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> -    uint32_t board_rev = (uint32_t)(uintptr_t)data;
> -
> -    rmc->board_rev = board_rev;
> -    mc->desc = g_strdup_printf("Raspberry Pi %s", board_type(board_rev));
> -    mc->init = raspi_machine_init;
> -    mc->block_default_type = IF_SD;
> -    mc->no_parallel = 1;
> -    mc->no_floppy = 1;
> -    mc->no_cdrom = 1;
> -    mc->max_cpus = BCM283X_NCPUS;
> -    mc->min_cpus = BCM283X_NCPUS;
> -    mc->default_cpus = BCM283X_NCPUS;
> -    mc->default_ram_size = board_ram_size(board_rev);
> -}
> -#endif
> -
>  static const TypeInfo raspi_machine_types[] = {
>      {
>          .name           = MACHINE_TYPE_NAME("raspi2"),
>          .parent         = TYPE_RASPI_MACHINE,
> -        .class_init     = raspi2_machine_class_init,
> +        .class_init     = raspi_machine_class_init,
>          .class_data     = (void *)0xa21041,
>  #ifdef TARGET_AARCH64
>      }, {
>          .name           = MACHINE_TYPE_NAME("raspi3"),
>          .parent         = TYPE_RASPI_MACHINE,
> -        .class_init     = raspi3_machine_class_init,
> +        .class_init     = raspi_machine_class_init,
>          .class_data     = (void *)0xa02082,
>  #endif
>      }, {

Re: [PATCH v3 13/13] hw/arm/raspi: Extract the cores count from the board revision

[Igor Mammedov]

On Sat,  8 Feb 2020 17:56:45 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> The board revision encode the count of ARM cores. Add a helper
s/encode/encodes/
Or better
the count of ARM cores is encoded in ...


> to extract the number of cores, and use it. This will be helpful
> when we add the Raspi0/1 that have a single core.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/raspi.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index bee6ca0a08..90ad9b8115 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -101,6 +101,21 @@ static const char *board_soc_type(uint32_t board_rev)
>      return soc_types[proc_id];
>  }
>  
> +static int cores_count(uint32_t board_rev)
> +{
> +    static const int soc_cores_count[] = {
> +        0, BCM283X_NCPUS, BCM283X_NCPUS,
> +    };
> +    int proc_id = board_processor_id(board_rev);
> +
> +    if (proc_id >= ARRAY_SIZE(soc_cores_count) || !soc_cores_count[proc_id]) {
> +        error_report("Unsupported processor id '%d' (board revision: 0x%x)",
> +                     proc_id, board_rev);
> +        exit(1);
> +    }
> +    return soc_cores_count[proc_id];
> +}
> +
>  static const char *board_type(uint32_t board_rev)
>  {
>      static const char *types[] = {
> @@ -307,9 +322,7 @@ static void raspi_machine_class_init(ObjectClass *oc, void *data)
>      mc->no_parallel = 1;
>      mc->no_floppy = 1;
>      mc->no_cdrom = 1;
> -    mc->max_cpus = BCM283X_NCPUS;
> -    mc->min_cpus = BCM283X_NCPUS;
> -    mc->default_cpus = BCM283X_NCPUS;
> +    mc->default_cpus = mc->min_cpus = mc->max_cpus = cores_count(board_rev);
>      mc->default_ram_size = board_ram_size(board_rev);
>      if (board_version(board_rev) == 2) {
>          mc->ignore_memory_transaction_failures = true;

Re: [PATCH v3 08/13] hw/arm/raspi: Make board_rev a field of RaspiMachineClass

[Philippe Mathieu-Daudé]

On 2/10/20 10:50 AM, Igor Mammedov wrote:
> On Sat,  8 Feb 2020 17:56:40 +0100
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
>> We want to have a common class_init(). The only value that
>> matters (and changes) is the board revision.
>> Pass the board_rev as class_data to class_init().
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/arm/raspi.c | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>> index 62b8df3c2e..fbfcd29732 100644
>> --- a/hw/arm/raspi.c
>> +++ b/hw/arm/raspi.c
>> @@ -46,6 +46,7 @@ typedef struct RaspiMachineClass {
>>       /*< private >*/
>>       MachineClass parent_obj;
>>       /*< public >*/
>> +    uint32_t board_rev;
>>   } RaspiMachineClass;
>>   
>>   #define TYPE_RASPI_MACHINE       MACHINE_TYPE_NAME("raspi-common")
>> @@ -227,9 +228,11 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
>>       arm_load_kernel(ARM_CPU(first_cpu), machine, &binfo);
>>   }
>>   
>> -static void raspi_init(MachineState *machine, uint32_t board_rev)
>> +static void raspi_init(MachineState *machine)
>>   {
>> +    RaspiMachineClass *mc = RASPI_MACHINE_GET_CLASS(machine);
>>       RaspiMachineState *s = RASPI_MACHINE(machine);
>> +    uint32_t board_rev = mc->board_rev;
>>       int version = board_version(board_rev);
>>       uint64_t ram_size = board_ram_size(board_rev);
>>       uint32_t vcram_size;
>> @@ -279,13 +282,16 @@ static void raspi_init(MachineState *machine, uint32_t board_rev)
>>   
>>   static void raspi2_init(MachineState *machine)
>>   {
>> -    raspi_init(machine, 0xa21041);
>> +    raspi_init(machine);
>>   }
>>   
>>   static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
>> +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
>>   
>> +    rmc->board_rev = board_rev;
> 
> instead of doing a bit obscure ".class_data     = (void *)0xa21041," and
> using it here, I'd just do
> 
>         rmc->board_rev = 0xa21041;
> 
> using value specific for each leaf class

Leaf classes are removed in patch #12 "Use a unique 
raspi_machine_class_init() method", see more uses of .class_data from v2:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg677164.html
https://www.mail-archive.com/qemu-devel@nongnu.org/msg677166.html

Are you disagreeing with them? Then we should document .class_data as 
deprecated and show example of good code.

> with this change
>   Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
>>       mc->desc = "Raspberry Pi 2B";
>>       mc->init = raspi2_init;
>>       mc->block_default_type = IF_SD;
>> @@ -302,13 +308,16 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>>   #ifdef TARGET_AARCH64
>>   static void raspi3_init(MachineState *machine)
>>   {
>> -    raspi_init(machine, 0xa02082);
>> +    raspi_init(machine);
>>   }
>>   
>>   static void raspi3_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
>> +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
>>   
>> +    rmc->board_rev = board_rev;
>>       mc->desc = "Raspberry Pi 3B";
>>       mc->init = raspi3_init;
>>       mc->block_default_type = IF_SD;
>> @@ -327,11 +336,13 @@ static const TypeInfo raspi_machine_types[] = {
>>           .name           = MACHINE_TYPE_NAME("raspi2"),
>>           .parent         = TYPE_RASPI_MACHINE,
>>           .class_init     = raspi2_machine_class_init,
>> +        .class_data     = (void *)0xa21041,
>>   #ifdef TARGET_AARCH64
>>       }, {
>>           .name           = MACHINE_TYPE_NAME("raspi3"),
>>           .parent         = TYPE_RASPI_MACHINE,
>>           .class_init     = raspi3_machine_class_init,
>> +        .class_data     = (void *)0xa02082,
>>   #endif
>>       }, {
>>           .name           = TYPE_RASPI_MACHINE,
> 

Re: [PATCH v3 08/13] hw/arm/raspi: Make board_rev a field of RaspiMachineClass

[Igor Mammedov]

On Mon, 10 Feb 2020 11:03:40 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 2/10/20 10:50 AM, Igor Mammedov wrote:
> > On Sat,  8 Feb 2020 17:56:40 +0100
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >   
> >> We want to have a common class_init(). The only value that
> >> matters (and changes) is the board revision.
> >> Pass the board_rev as class_data to class_init().
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >>   hw/arm/raspi.c | 17 ++++++++++++++---
> >>   1 file changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> >> index 62b8df3c2e..fbfcd29732 100644
> >> --- a/hw/arm/raspi.c
> >> +++ b/hw/arm/raspi.c
> >> @@ -46,6 +46,7 @@ typedef struct RaspiMachineClass {
> >>       /*< private >*/
> >>       MachineClass parent_obj;
> >>       /*< public >*/
> >> +    uint32_t board_rev;
> >>   } RaspiMachineClass;
> >>   
> >>   #define TYPE_RASPI_MACHINE       MACHINE_TYPE_NAME("raspi-common")
> >> @@ -227,9 +228,11 @@ static void setup_boot(MachineState *machine, int version, size_t ram_size)
> >>       arm_load_kernel(ARM_CPU(first_cpu), machine, &binfo);
> >>   }
> >>   
> >> -static void raspi_init(MachineState *machine, uint32_t board_rev)
> >> +static void raspi_init(MachineState *machine)
> >>   {
> >> +    RaspiMachineClass *mc = RASPI_MACHINE_GET_CLASS(machine);
> >>       RaspiMachineState *s = RASPI_MACHINE(machine);
> >> +    uint32_t board_rev = mc->board_rev;
> >>       int version = board_version(board_rev);
> >>       uint64_t ram_size = board_ram_size(board_rev);
> >>       uint32_t vcram_size;
> >> @@ -279,13 +282,16 @@ static void raspi_init(MachineState *machine, uint32_t board_rev)
> >>   
> >>   static void raspi2_init(MachineState *machine)
> >>   {
> >> -    raspi_init(machine, 0xa21041);
> >> +    raspi_init(machine);
> >>   }
> >>   
> >>   static void raspi2_machine_class_init(ObjectClass *oc, void *data)
> >>   {
> >>       MachineClass *mc = MACHINE_CLASS(oc);
> >> +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> >> +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
> >>   
> >> +    rmc->board_rev = board_rev;  
> > 
> > instead of doing a bit obscure ".class_data     = (void *)0xa21041," and
> > using it here, I'd just do
> > 
> >         rmc->board_rev = 0xa21041;
> > 
> > using value specific for each leaf class  
> 
> Leaf classes are removed in patch #12 "Use a unique 
> raspi_machine_class_init() method", see more uses of .class_data from v2:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg677164.html
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg677166.html
> 
> Are you disagreeing with them? Then we should document .class_data as 
> deprecated and show example of good code.

we sometimes use .class_data when creating many derived types
(typical example CPU types (x86) ) where it's impractical to code
leaf class_init functions. I'd use .class_data in cases where I
can't get away with explicit .class_init

However in case of machines (even various versioned ones) practice
was to use parent class_init to set common data and leaf class_inits
to set board (version) specific features.


> > with this change
> >   Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> >   
> >>       mc->desc = "Raspberry Pi 2B";
> >>       mc->init = raspi2_init;
> >>       mc->block_default_type = IF_SD;
> >> @@ -302,13 +308,16 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
> >>   #ifdef TARGET_AARCH64
> >>   static void raspi3_init(MachineState *machine)
> >>   {
> >> -    raspi_init(machine, 0xa02082);
> >> +    raspi_init(machine);
> >>   }
> >>   
> >>   static void raspi3_machine_class_init(ObjectClass *oc, void *data)
> >>   {
> >>       MachineClass *mc = MACHINE_CLASS(oc);
> >> +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> >> +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
> >>   
> >> +    rmc->board_rev = board_rev;
> >>       mc->desc = "Raspberry Pi 3B";
> >>       mc->init = raspi3_init;
> >>       mc->block_default_type = IF_SD;
> >> @@ -327,11 +336,13 @@ static const TypeInfo raspi_machine_types[] = {
> >>           .name           = MACHINE_TYPE_NAME("raspi2"),
> >>           .parent         = TYPE_RASPI_MACHINE,
> >>           .class_init     = raspi2_machine_class_init,
> >> +        .class_data     = (void *)0xa21041,
> >>   #ifdef TARGET_AARCH64
> >>       }, {
> >>           .name           = MACHINE_TYPE_NAME("raspi3"),
> >>           .parent         = TYPE_RASPI_MACHINE,
> >>           .class_init     = raspi3_machine_class_init,
> >> +        .class_data     = (void *)0xa02082,
> >>   #endif
> >>       }, {
> >>           .name           = TYPE_RASPI_MACHINE,  
> >   
> 

Re: [PATCH v3 03/13] hw/arm/raspi: Extract the version from the board revision

[Peter Maydell]

On Sat, 8 Feb 2020 at 16:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The board revision encode the board version. Add a helper
> to extract the version, and use it.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/raspi.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 818146fdbb..f285e2988f 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -16,6 +16,7 @@
>  #include "qapi/error.h"
>  #include "cpu.h"
>  #include "hw/arm/bcm2836.h"
> +#include "hw/registerfields.h"
>  #include "qemu/error-report.h"
>  #include "hw/boards.h"
>  #include "hw/loader.h"
> @@ -37,6 +38,28 @@ typedef struct RasPiState {
>      MemoryRegion ram;
>  } RasPiState;
>
> +/*
> + * Board revision codes:
> + * www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/
> + */
> +FIELD(REV_CODE, REVISION,           0, 4);
> +FIELD(REV_CODE, TYPE,               4, 8);
> +FIELD(REV_CODE, PROCESSOR,         12, 4);
> +FIELD(REV_CODE, MANUFACTURER,      16, 4);
> +FIELD(REV_CODE, MEMORY_SIZE,       20, 3);
> +FIELD(REV_CODE, STYLE,             23, 1);
> +
> +static int board_processor_id(uint32_t board_rev)
> +{
> +    assert(FIELD_EX32(board_rev, REV_CODE, STYLE)); /* Only new style */
> +    return FIELD_EX32(board_rev, REV_CODE, PROCESSOR);
> +}
> +
> +static int board_version(uint32_t board_rev)
> +{
> +    return board_processor_id(board_rev) + 1;

This uses the 'processor' field, which basically means the SoC
(0 for BCM2835, 1 for BCM2836, 2 for BCMM2837, 3 for BCM2711).
We use 'version' for a wider range of things in our code here:
 * do we need SMP setup?
 * which address does the firmware image go?
 * do we need to set up SMC vectors so no-op SMC works?
 * as well as "which SoC do we instantiate"?

We think of 'version' as basically "raspi 2 or 3?", but
according to the table in your url you can get a version of
the raspi 2b with a BCM2837 SoC, which confuses this idea.

Anyway, since what we have in this patch works OK for the set
of board models we support, I'm happy to leave the patch as-is,
but maybe worth checking and considering what in our code we
should really be making conditional on "actually the SoC type"
and what on something else...

thanks
-- PMM

Re: [PATCH v3 03/13] hw/arm/raspi: Extract the version from the board revision

[Philippe Mathieu-Daudé]

On 2/13/20 2:40 PM, Peter Maydell wrote:
> On Sat, 8 Feb 2020 at 16:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> The board revision encode the board version. Add a helper
>> to extract the version, and use it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/arm/raspi.c | 31 +++++++++++++++++++++++++++----
>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>> index 818146fdbb..f285e2988f 100644
>> --- a/hw/arm/raspi.c
>> +++ b/hw/arm/raspi.c
>> @@ -16,6 +16,7 @@
>>   #include "qapi/error.h"
>>   #include "cpu.h"
>>   #include "hw/arm/bcm2836.h"
>> +#include "hw/registerfields.h"
>>   #include "qemu/error-report.h"
>>   #include "hw/boards.h"
>>   #include "hw/loader.h"
>> @@ -37,6 +38,28 @@ typedef struct RasPiState {
>>       MemoryRegion ram;
>>   } RasPiState;
>>
>> +/*
>> + * Board revision codes:
>> + * www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/
>> + */
>> +FIELD(REV_CODE, REVISION,           0, 4);
>> +FIELD(REV_CODE, TYPE,               4, 8);
>> +FIELD(REV_CODE, PROCESSOR,         12, 4);
>> +FIELD(REV_CODE, MANUFACTURER,      16, 4);
>> +FIELD(REV_CODE, MEMORY_SIZE,       20, 3);
>> +FIELD(REV_CODE, STYLE,             23, 1);
>> +
>> +static int board_processor_id(uint32_t board_rev)
>> +{
>> +    assert(FIELD_EX32(board_rev, REV_CODE, STYLE)); /* Only new style */
>> +    return FIELD_EX32(board_rev, REV_CODE, PROCESSOR);
>> +}
>> +
>> +static int board_version(uint32_t board_rev)
>> +{
>> +    return board_processor_id(board_rev) + 1;
> 
> This uses the 'processor' field, which basically means the SoC
> (0 for BCM2835, 1 for BCM2836, 2 for BCMM2837, 3 for BCM2711).
> We use 'version' for a wider range of things in our code here:
>   * do we need SMP setup?
>   * which address does the firmware image go?
>   * do we need to set up SMC vectors so no-op SMC works?
>   * as well as "which SoC do we instantiate"?
> 
> We think of 'version' as basically "raspi 2 or 3?", but
> according to the table in your url you can get a version of
> the raspi 2b with a BCM2837 SoC, which confuses this idea.
> 
> Anyway, since what we have in this patch works OK for the set
> of board models we support, I'm happy to leave the patch as-is,
> but maybe worth checking and considering what in our code we
> should really be making conditional on "actually the SoC type"
> and what on something else...

Yes you are right, version = (2, 3) was too simple, I replaced the 
'version' check by 'processor_id' in the series introducing the raspi4 
(with other cleanups).
Eventually this file will only use the board_rev fields directly.

Re: [PATCH v3 12/13] hw/arm/raspi: Use a unique raspi_machine_class_init() method

[Peter Maydell]

On Sat, 8 Feb 2020 at 16:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> With the exception of the ignore_memory_transaction_failures
> flag set for the raspi2, both machine_class_init() methods
> are now identical. Merge them to keep a unique method.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/arm/raspi.c | 31 ++++++-------------------------
>  1 file changed, 6 insertions(+), 25 deletions(-)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 0537fc0a2d..bee6ca0a08 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -294,7 +294,7 @@ static void raspi_machine_init(MachineState *machine)
>      setup_boot(machine, version, machine->ram_size - vcram_size);
>  }
>
> -static void raspi2_machine_class_init(ObjectClass *oc, void *data)
> +static void raspi_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
>      RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> @@ -311,41 +311,22 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>      mc->min_cpus = BCM283X_NCPUS;
>      mc->default_cpus = BCM283X_NCPUS;
>      mc->default_ram_size = board_ram_size(board_rev);
> -    mc->ignore_memory_transaction_failures = true;
> +    if (board_version(board_rev) == 2) {
> +        mc->ignore_memory_transaction_failures = true;
> +    }
>  };

This isn't really the correct condition here. What we want is:
 * for the board named 'raspi2' which was introduced before
   we added the transaction-failure support to Arm CPU emulation,
   disable signaling transaction failures
 * for any other board, leave it enabled (whether that new
   board is BCM2836 based or anything else)

(This kind of follows on from my remark on patch 3: we should
be suspicious of anything that's conditional on board_version();
it should probably be testing something else.)

The natural way to implement this is to have the .class_data
be a pointer to a struct which is in an array and defines
relevant per-class stuff, the same way we do in
bcm2836_register_types(). That way the struct can indicate
both the board revision number and also "is this a legacy
board that needs transaction-failures disabled?".

The other approach here, as discussed on IRC, is that if
we're confident we really have all the devices in the SoC
either present or stubbed out with unimplemented-device
then we could disable ignore_memory_transaction_failures
for raspi2. (The flag is only there because I didn't want
to try to do the auditing and fielding of potential bug
reports if I changed the behaviour of a bunch of our
existing not-very-maintained board models: the real
correct behaviour in almost all cases would be to allow
transaction failures and just make sure we have stub devices
as needed.)

That said, this does give the right answer for our current boards,
so I'm ok with taking this series if you want to address this
in a followup patch.

thanks
-- PMM

Re: [PATCH v3 00/13] hw/arm/raspi: Dynamically create machines based on the board revision

[Peter Maydell]

On Sat, 8 Feb 2020 at 16:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi,
>
> This series is a preparatory to easily add the raspi0/raspi1/raspi4
> boards (see [1]).
>
> Igor has been working in his "refactor main RAM allocation to use
> hostmem backend" series, and now v4 [2] is almost reviewed.
>
> His raspi patch [3] clashes with my work, Since it is easier for
> him to apply his on top of mine, I am sending these patches first.
>
> Since v2:
> - Split of bigger series (30 patches was scary)
> - addressed Zoltan review comments
>
> Phil.
>
> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg677145.html
> [2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg675738.html
> [3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg675752.html
> Supersedes: <20200206011756.2413-1-f4bug@amsat.org>

I had one or two minor comments but I'm happy if we address
those in follow-up patches.

Applied to target-arm.next with the commit message tweak for
patch 13 suggested by Igor.

thanks
-- PMM

Re: [PATCH v3 12/13] hw/arm/raspi: Use a unique raspi_machine_class_init() method

[Philippe Mathieu-Daudé]

On 2/13/20 2:59 PM, Peter Maydell wrote:
> On Sat, 8 Feb 2020 at 16:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> With the exception of the ignore_memory_transaction_failures
>> flag set for the raspi2, both machine_class_init() methods
>> are now identical. Merge them to keep a unique method.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   hw/arm/raspi.c | 31 ++++++-------------------------
>>   1 file changed, 6 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>> index 0537fc0a2d..bee6ca0a08 100644
>> --- a/hw/arm/raspi.c
>> +++ b/hw/arm/raspi.c
>> @@ -294,7 +294,7 @@ static void raspi_machine_init(MachineState *machine)
>>       setup_boot(machine, version, machine->ram_size - vcram_size);
>>   }
>>
>> -static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>> +static void raspi_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>>       RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
>> @@ -311,41 +311,22 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>>       mc->min_cpus = BCM283X_NCPUS;
>>       mc->default_cpus = BCM283X_NCPUS;
>>       mc->default_ram_size = board_ram_size(board_rev);
>> -    mc->ignore_memory_transaction_failures = true;
>> +    if (board_version(board_rev) == 2) {
>> +        mc->ignore_memory_transaction_failures = true;
>> +    }
>>   };
> 
> This isn't really the correct condition here. What we want is:
>   * for the board named 'raspi2' which was introduced before
>     we added the transaction-failure support to Arm CPU emulation,
>     disable signaling transaction failures
>   * for any other board, leave it enabled (whether that new
>     board is BCM2836 based or anything else)
> 
> (This kind of follows on from my remark on patch 3: we should
> be suspicious of anything that's conditional on board_version();
> it should probably be testing something else.)
> 
> The natural way to implement this is to have the .class_data
> be a pointer to a struct which is in an array and defines
> relevant per-class stuff, the same way we do in
> bcm2836_register_types(). That way the struct can indicate
> both the board revision number and also "is this a legacy
> board that needs transaction-failures disabled?".

IIUC Igor insists explaining that he doesn't accept anymore a 
".class_data pointer to a struct which is in an array and defines 
relevant per-class stuff" and we should not use this pattern anymore.

> The other approach here, as discussed on IRC, is that if
> we're confident we really have all the devices in the SoC
> either present or stubbed out with unimplemented-device
> then we could disable ignore_memory_transaction_failures
> for raspi2. (The flag is only there because I didn't want
> to try to do the auditing and fielding of potential bug
> reports if I changed the behaviour of a bunch of our
> existing not-very-maintained board models: the real
> correct behaviour in almost all cases would be to allow
> transaction failures and just make sure we have stub devices
> as needed.)

Yes, the plan is to add all the unimplemented peripherals (patches 
ready, but out of scope of this series) and remove this flag.

> That said, this does give the right answer for our current boards,
> so I'm ok with taking this series if you want to address this
> in a followup patch.

If you are OK, I prefer to address this in a later series than delaying 
this one more longer.

Thanks!

Re: [PATCH v3 12/13] hw/arm/raspi: Use a unique raspi_machine_class_init() method

[Peter Maydell]

On Thu, 13 Feb 2020 at 14:16, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 2/13/20 2:59 PM, Peter Maydell wrote:
> > The natural way to implement this is to have the .class_data
> > be a pointer to a struct which is in an array and defines
> > relevant per-class stuff, the same way we do in
> > bcm2836_register_types(). That way the struct can indicate
> > both the board revision number and also "is this a legacy
> > board that needs transaction-failures disabled?".
>
> IIUC Igor insists explaining that he doesn't accept anymore a
> ".class_data pointer to a struct which is in an array and defines
> relevant per-class stuff" and we should not use this pattern anymore.

Huh? How else would you do this? I'm kinda dubious about the
pattern this patch series uses of just stuffing a 32-bit board
ID number into the class_data field, to be honest -- I let that
pass partly not to hold up the series but partly because I
expect that we'll need to turn it back into a proper pointer
to a data struct soonish.

thnaks
-- PMM

Re: [PATCH v3 12/13] hw/arm/raspi: Use a unique raspi_machine_class_init() method

[Philippe Mathieu-Daudé]

On 2/13/20 3:32 PM, Peter Maydell wrote:
> On Thu, 13 Feb 2020 at 14:16, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> On 2/13/20 2:59 PM, Peter Maydell wrote:
>>> The natural way to implement this is to have the .class_data
>>> be a pointer to a struct which is in an array and defines
>>> relevant per-class stuff, the same way we do in
>>> bcm2836_register_types(). That way the struct can indicate
>>> both the board revision number and also "is this a legacy
>>> board that needs transaction-failures disabled?".
>>
>> IIUC Igor insists explaining that he doesn't accept anymore a
>> ".class_data pointer to a struct which is in an array and defines
>> relevant per-class stuff" and we should not use this pattern anymore.
> 
> Huh? How else would you do this? I'm kinda dubious about the
> pattern this patch series uses of just stuffing a 32-bit board
> ID number into the class_data field, to be honest -- I let that
> pass partly not to hold up the series but partly because I
> expect that we'll need to turn it back into a proper pointer
> to a data struct soonish.

https://www.mail-archive.com/qemu-devel@nongnu.org/msg678305.html

Igor> we sometimes use .class_data when creating many
       derived types (typical example CPU types (x86))
       where it's impractical to code leaf class_init
       functions. I'd use .class_data in cases where I
       can't get away with explicit .class_init

Which I understand as:

- avoid .class_data (pointers to structures)
- explicitly set ObjectClass::fields in .class_init()
   by open-coding all.

Re: [PATCH v3 12/13] hw/arm/raspi: Use a unique raspi_machine_class_init() method

[Philippe Mathieu-Daudé]

On Thu, Feb 13, 2020 at 3:16 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
> On 2/13/20 2:59 PM, Peter Maydell wrote:
> > On Sat, 8 Feb 2020 at 16:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> With the exception of the ignore_memory_transaction_failures
> >> flag set for the raspi2, both machine_class_init() methods
> >> are now identical. Merge them to keep a unique method.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>   hw/arm/raspi.c | 31 ++++++-------------------------
> >>   1 file changed, 6 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> >> index 0537fc0a2d..bee6ca0a08 100644
> >> --- a/hw/arm/raspi.c
> >> +++ b/hw/arm/raspi.c
> >> @@ -294,7 +294,7 @@ static void raspi_machine_init(MachineState *machine)
> >>       setup_boot(machine, version, machine->ram_size - vcram_size);
> >>   }
> >>
> >> -static void raspi2_machine_class_init(ObjectClass *oc, void *data)
> >> +static void raspi_machine_class_init(ObjectClass *oc, void *data)
> >>   {
> >>       MachineClass *mc = MACHINE_CLASS(oc);
> >>       RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> >> @@ -311,41 +311,22 @@ static void raspi2_machine_class_init(ObjectClass *oc, void *data)
> >>       mc->min_cpus = BCM283X_NCPUS;
> >>       mc->default_cpus = BCM283X_NCPUS;
> >>       mc->default_ram_size = board_ram_size(board_rev);
> >> -    mc->ignore_memory_transaction_failures = true;
> >> +    if (board_version(board_rev) == 2) {
> >> +        mc->ignore_memory_transaction_failures = true;
> >> +    }
> >>   };
> >
> > This isn't really the correct condition here. What we want is:
> >   * for the board named 'raspi2' which was introduced before
> >     we added the transaction-failure support to Arm CPU emulation,
> >     disable signaling transaction failures
> >   * for any other board, leave it enabled (whether that new
> >     board is BCM2836 based or anything else)
> >
> > (This kind of follows on from my remark on patch 3: we should
> > be suspicious of anything that's conditional on board_version();
> > it should probably be testing something else.)
> >
> > The natural way to implement this is to have the .class_data
> > be a pointer to a struct which is in an array and defines
> > relevant per-class stuff, the same way we do in
> > bcm2836_register_types(). That way the struct can indicate
> > both the board revision number and also "is this a legacy
> > board that needs transaction-failures disabled?".
>
> IIUC Igor insists explaining that he doesn't accept anymore a
> ".class_data pointer to a struct which is in an array and defines
> relevant per-class stuff" and we should not use this pattern anymore.
>
> > The other approach here, as discussed on IRC, is that if
> > we're confident we really have all the devices in the SoC
> > either present or stubbed out with unimplemented-device
> > then we could disable ignore_memory_transaction_failures
> > for raspi2. (The flag is only there because I didn't want
> > to try to do the auditing and fielding of potential bug
> > reports if I changed the behaviour of a bunch of our
> > existing not-very-maintained board models: the real
> > correct behaviour in almost all cases would be to allow
> > transaction failures and just make sure we have stub devices
> > as needed.)
>
> Yes, the plan is to add all the unimplemented peripherals (patches
> ready, but out of scope of this series) and remove this flag.

I found my 'ready' patch, it is already merged as commit 00cbd5bd74b1 =)

    hw/arm/bcm2835: Add various unimplemented peripherals

    Base addresses and sizes taken from the "BCM2835 ARM Peripherals"
    datasheet from February 06 2012:
    https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

I'm successfully running U-boot and Linux on raspi0/1/2/3 so I guess
it is safe to remove ignore_memory_transaction_failures for the
raspi2.
It might be insufficient to run proprietary firmware (on the raspi2),
I have no idea if people use QEMU for that.

> > That said, this does give the right answer for our current boards,
> > so I'm ok with taking this series if you want to address this
> > in a followup patch.
>
> If you are OK, I prefer to address this in a later series than delaying
> this one more longer.
>
> Thanks!
>
>