[Qemu-devel] [PATCH] BCM2837 and machine raspi3

bzt bzt posted 1 patch 6 years, 5 months ago
Failed in applying to current master (apply log)
hw/arm/raspi.c                  |  79 ++++++++++++++++--
include/hw/arm/bcm2836.h        |   6 ++
include/hw/arm/bcm2837.h        |  19 +++++
include/hw/arm/raspi_platform.h |   2 +-
8 files changed, 287 insertions(+), 16 deletions(-)
create mode 100644 hw/arm/bcm2837.c
create mode 100644 include/hw/arm/bcm2837.h
[Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 5 months ago
Dear All,

I've added support for "-M raspi3" to qemu. This is my first patch, I hope
it's okay. The github repo is here: https://github.com/bztsrc/qemu-raspi3
in case my patch does not work for some reason.

From 1f10f957b57f336728097803bf8339a5577dd3c2 Mon Sep 17 00:00:00 2001
From: bzt <bztemail@gmail.com>
Date: Sun, 22 Oct 2017 14:59:20 +0200
Subject: [PATCH] BCM2837 and machine raspi3

Signed-off-by: bzt <bztemail@gmail.com>
---
 hw/arm/Makefile.objs            |   2 +-
 hw/arm/bcm2835_peripherals.c    |  10 ++-
 hw/arm/bcm2836.c                |   6 --
 hw/arm/bcm2837.c                | 179
++++++++++++++++++++++++++++++++++++++++
 hw/arm/raspi.c                  |  79 ++++++++++++++++--
 include/hw/arm/bcm2836.h        |   6 ++
 include/hw/arm/bcm2837.h        |  19 +++++
 include/hw/arm/raspi_platform.h |   2 +-
 8 files changed, 287 insertions(+), 16 deletions(-)
 create mode 100644 hw/arm/bcm2837.c
 create mode 100644 include/hw/arm/bcm2837.h

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 2794e08..72b60e1 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -11,7 +11,7 @@ obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o
pxa2xx_pic.o
 obj-$(CONFIG_DIGIC) += digic.o
 obj-y += omap1.o omap2.o strongarm.o
 obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o
-obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o raspi.o
+obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o bcm2837.o raspi.o
 obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
 obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o xlnx-zcu102.o
 obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index 12e0dd1..f79ce36 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -212,7 +212,15 @@ static void bcm2835_peripherals_realize(DeviceState
*dev, Error **errp)
         error_propagate(errp, err);
         return;
     }
-
+    // check if parameters are valid
+    if (ram_size < vcram_size + 64*1024*1024) {
+        error_setg(errp, "%s: not enough ram for VideoCore",
+                   __func__);
+        return;
+    }
+    // if vcram_size is bigger than ram_size, this will silently overflow
+    // and generate a not very informative "Parameter 'vcram-base' expects
+    // uint32_t" message...
     object_property_set_uint(OBJECT(&s->fb), ram_size - vcram_size,
                              "vcram-base", &err);
     if (err) {
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 8c43291..db40c8e 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -17,12 +17,6 @@
 #include "hw/sysbus.h"
 #include "exec/address-spaces.h"

-/* Peripheral base address seen by the CPU */
-#define BCM2836_PERI_BASE       0x3F000000
-
-/* "QA7" (Pi2) interrupt controller and mailboxes etc. */
-#define BCM2836_CONTROL_BASE    0x40000000
-
 static void bcm2836_init(Object *obj)
 {
     BCM2836State *s = BCM2836(obj);
diff --git a/hw/arm/bcm2837.c b/hw/arm/bcm2837.c
new file mode 100644
index 0000000..1bab93b
--- /dev/null
+++ b/hw/arm/bcm2837.c
@@ -0,0 +1,179 @@
+/*
+ * Raspberry Pi emulation (c) 2012 Gregory Estrade
+ * Upstreaming code cleanup [including bcm2835_*] (c) 2013 Jan Petrous
+ *
+ * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft
+ * Written by Andrew Baumann
+ *
+ * Raspberry Pi 3 emulation 2017 by bzt
+ *
+ * This code is licensed under the GNU GPLv2 and later.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
+#include "cpu.h"
+#include "hw/arm/bcm2836.h"
+#include "hw/arm/bcm2837.h"
+#include "hw/arm/raspi_platform.h"
+#include "hw/sysbus.h"
+#include "exec/address-spaces.h"
+
+/* According to
https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2837/README.md
+ * The underlying architecture of the BCM2837 is identical to the BCM2836.
The only significant
+ * difference is the replacement of the ARMv7 quad core cluster with a
quad-core ARM Cortex A53
+ * (ARMv8) cluster. So we use cortex-a53- here. */
+
+static void bcm2837_init(Object *obj)
+{
+    BCM2836State *s = BCM2837(obj);
+    int n;
+
+    for (n = 0; n < BCM2836_NCPUS; n++) {
+        object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
+                          "cortex-a53-" TYPE_ARM_CPU);
+        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
+                                  &error_abort);
+    }
+
+    object_initialize(&s->control, sizeof(s->control),
TYPE_BCM2836_CONTROL);
+    object_property_add_child(obj, "control", OBJECT(&s->control), NULL);
+    qdev_set_parent_bus(DEVICE(&s->control), sysbus_get_default());
+
+    object_initialize(&s->peripherals, sizeof(s->peripherals),
+                      TYPE_BCM2835_PERIPHERALS);
+    object_property_add_child(obj, "peripherals", OBJECT(&s->peripherals),
+                              &error_abort);
+    object_property_add_alias(obj, "board-rev", OBJECT(&s->peripherals),
+                              "board-rev", &error_abort);
+    object_property_add_alias(obj, "vcram-size", OBJECT(&s->peripherals),
+                              "vcram-size", &error_abort);
+    qdev_set_parent_bus(DEVICE(&s->peripherals), sysbus_get_default());
+}
+
+static void bcm2837_realize(DeviceState *dev, Error **errp)
+{
+    BCM2836State *s = BCM2837(dev);
+    Object *obj;
+    Error *err = NULL;
+    int n;
+
+    /* common peripherals from bcm2835 */
+
+    obj = object_property_get_link(OBJECT(dev), "ram", &err);
+    if (obj == NULL) {
+        error_setg(errp, "%s: required ram link not found: %s",
+                   __func__, error_get_pretty(err));
+        return;
+    }
+
+    object_property_add_const_link(OBJECT(&s->peripherals), "ram", obj,
&err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    object_property_set_bool(OBJECT(&s->peripherals), true, "realized",
&err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    object_property_add_alias(OBJECT(s), "sd-bus", OBJECT(&s->peripherals),
+                              "sd-bus", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->peripherals), 0,
+                            BCM2836_PERI_BASE, 1);
+
+    /* bcm2836 interrupt controller (and mailboxes, etc.) */
+    object_property_set_bool(OBJECT(&s->control), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, BCM2836_CONTROL_BASE);
+
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 0,
+        qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-irq", 0));
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 1,
+        qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-fiq", 0));
+
+    for (n = 0; n < BCM2836_NCPUS; n++) {
+        /* Mirror bcm2836, which has clusterid set to 0xf
+         * TODO: this should be converted to a property of ARM_CPU
+         */
+        s->cpus[n].mp_affinity = 0xF00 | n;
+
+        /* set periphbase/CBAR value for CPU-local registers */
+        object_property_set_int(OBJECT(&s->cpus[n]),
+                                BCM2836_PERI_BASE + MCORE_OFFSET,
+                                "reset-cbar", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+
+        /* start powered off if not enabled */
+        object_property_set_bool(OBJECT(&s->cpus[n]), n >= s->enabled_cpus,
+                                 "start-powered-off", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+
+        object_property_set_bool(OBJECT(&s->cpus[n]), true, "realized",
&err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+
+        /* Connect irq/fiq outputs from the interrupt controller. */
+        qdev_connect_gpio_out_named(DEVICE(&s->control), "irq", n,
+                qdev_get_gpio_in(DEVICE(&s->cpus[n]), ARM_CPU_IRQ));
+        qdev_connect_gpio_out_named(DEVICE(&s->control), "fiq", n,
+                qdev_get_gpio_in(DEVICE(&s->cpus[n]), ARM_CPU_FIQ));
+
+        /* Connect timers from the CPU to the interrupt controller */
+        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_PHYS,
+                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpnsirq",
n));
+        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_VIRT,
+                qdev_get_gpio_in_named(DEVICE(&s->control), "cntvirq", n));
+        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_HYP,
+                qdev_get_gpio_in_named(DEVICE(&s->control), "cnthpirq",
n));
+        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_SEC,
+                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpsirq",
n));
+    }
+}
+
+static Property bcm2837_props[] = {
+    DEFINE_PROP_UINT32("enabled-cpus", BCM2836State, enabled_cpus,
BCM2836_NCPUS),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static void bcm2837_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    dc->props = bcm2837_props;
+    dc->realize = bcm2837_realize;
+}
+
+static const TypeInfo bcm2837_type_info = {
+    .name = TYPE_BCM2837,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(BCM2836State),
+    .instance_init = bcm2837_init,
+    .class_init = bcm2837_class_init,
+};
+
+static void bcm2837_register_types(void)
+{
+    type_register_static(&bcm2837_type_info);
+}
+
+type_init(bcm2837_register_types)
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 5941c9f..726a426 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -5,6 +5,8 @@
  * Rasperry Pi 2 emulation Copyright (c) 2015, Microsoft
  * Written by Andrew Baumann
  *
+ * Raspberry Pi 3 emulation 2017 by bzt
+ *
  * This code is licensed under the GNU GPLv2 and later.
  */

@@ -13,6 +15,7 @@
 #include "qemu-common.h"
 #include "cpu.h"
 #include "hw/arm/bcm2836.h"
+#include "hw/arm/bcm2837.h"
 #include "qemu/error-report.h"
 #include "hw/boards.h"
 #include "hw/loader.h"
@@ -22,10 +25,11 @@
 #define SMPBOOT_ADDR    0x300 /* this should leave enough space for ATAGS
*/
 #define MVBAR_ADDR      0x400 /* secure vectors */
 #define BOARDSETUP_ADDR (MVBAR_ADDR + 0x20) /* board setup code */
-#define FIRMWARE_ADDR   0x8000 /* Pi loads kernel.img here by default */
+#define FIRMWARE_ADDR_2    0x8000 /* Pi 2 loads kernel.img here by default
*/
+#define FIRMWARE_ADDR_3   0x80000 /* Pi 3 loads kernel8.img here by
default */

 /* Table of Linux board IDs for different Pi versions */
-static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43};
+static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};

 typedef struct RasPiState {
     BCM2836State soc;
@@ -73,6 +77,7 @@ static void reset_secondary(ARMCPU *cpu, const struct
arm_boot_info *info)
 static void setup_boot(MachineState *machine, int version, size_t ram_size)
 {
     static struct arm_boot_info binfo;
+    hwaddr entry;
     int r;

     binfo.board_id = raspi_boardid[version];
@@ -83,11 +88,12 @@ static void setup_boot(MachineState *machine, int
version, size_t ram_size)
     binfo.secure_board_setup = true;
     binfo.secure_boot = true;

-    /* Pi2 requires SMP setup */
-    if (version == 2) {
+    /* Pi2 and Pi3 requires SMP setup */
+    if (version == 2 || version == 3) {
         binfo.smp_loader_start = SMPBOOT_ADDR;
         binfo.write_secondary_boot = write_smpboot;
         binfo.secondary_cpu_reset_hook = reset_secondary;
+        entry = version == 2 ? FIRMWARE_ADDR_2 : FIRMWARE_ADDR_3;
     }

     /* If the user specified a "firmware" image (e.g. UEFI), we bypass
@@ -95,14 +101,14 @@ static void setup_boot(MachineState *machine, int
version, size_t ram_size)
      */
     if (machine->firmware) {
         /* load the firmware image (typically kernel.img) */
-        r = load_image_targphys(machine->firmware, FIRMWARE_ADDR,
-                                ram_size - FIRMWARE_ADDR);
+        r = load_image_targphys(machine->firmware, entry,
+                                ram_size - entry);
         if (r < 0) {
             error_report("Failed to load firmware from %s",
machine->firmware);
             exit(1);
         }

-        binfo.entry = FIRMWARE_ADDR;
+        binfo.entry = entry;
         binfo.firmware_loaded = true;
     } else {
         binfo.kernel_filename = machine->kernel_filename;
@@ -171,3 +177,62 @@ static void raspi2_machine_init(MachineClass *mc)
     mc->ignore_memory_transaction_failures = true;
 };
 DEFINE_MACHINE("raspi2", raspi2_machine_init)
+
+static void raspi3_init(MachineState *machine)
+{
+    RasPiState *s = g_new0(RasPiState, 1);
+    uint32_t vcram_size;
+    DriveInfo *di;
+    BlockBackend *blk;
+    BusState *bus;
+    DeviceState *carddev;
+
+    object_initialize(&s->soc, sizeof(s->soc), TYPE_BCM2837);
+    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
+                              &error_abort);
+
+    /* Allocate and map RAM */
+    memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
+                                         machine->ram_size);
+    /* FIXME: Remove when we have custom CPU address space support */
+    memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram,
0);
+
+    /* Setup the SOC */
+    object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
+                                   &error_abort);
+    object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
+                            &error_abort);
+    object_property_set_int(OBJECT(&s->soc), 0xa02082, "board-rev",
+                            &error_abort);
+    object_property_set_bool(OBJECT(&s->soc), true, "realized",
&error_abort);
+
+    /* Create and plug in the SD cards */
+    di = drive_get_next(IF_SD);
+    blk = di ? blk_by_legacy_dinfo(di) : NULL;
+    bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
+    if (bus == NULL) {
+        error_report("No SD bus found in SOC object");
+        exit(1);
+    }
+    carddev = qdev_create(bus, TYPE_SD_CARD);
+    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
+    object_property_set_bool(OBJECT(carddev), true, "realized",
&error_fatal);
+
+    vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
+                                          &error_abort);
+    setup_boot(machine, 3, machine->ram_size - vcram_size);
+}
+
+static void raspi3_machine_init(MachineClass *mc)
+{
+    mc->desc = "Raspberry Pi 3";
+    mc->init = raspi3_init;
+    mc->block_default_type = IF_SD;
+    mc->no_parallel = 1;
+    mc->no_floppy = 1;
+    mc->no_cdrom = 1;
+    mc->max_cpus = BCM2836_NCPUS;
+    mc->default_ram_size = 1024 * 1024 * 1024;
+    mc->ignore_memory_transaction_failures = true;
+};
+DEFINE_MACHINE("raspi3", raspi3_machine_init)
diff --git a/include/hw/arm/bcm2836.h b/include/hw/arm/bcm2836.h
index 76de199..ee6b9dc 100644
--- a/include/hw/arm/bcm2836.h
+++ b/include/hw/arm/bcm2836.h
@@ -20,6 +20,12 @@

 #define BCM2836_NCPUS 4

+/* Peripheral base address seen by the CPU */
+#define BCM2836_PERI_BASE       0x3F000000
+
+/* "QA7" (Pi2/Pi8) interrupt controller and mailboxes etc. */
+#define BCM2836_CONTROL_BASE    0x40000000
+
 typedef struct BCM2836State {
     /*< private >*/
     DeviceState parent_obj;
diff --git a/include/hw/arm/bcm2837.h b/include/hw/arm/bcm2837.h
new file mode 100644
index 0000000..5c7be8a
--- /dev/null
+++ b/include/hw/arm/bcm2837.h
@@ -0,0 +1,19 @@
+/*
+ * Raspberry Pi emulation (c) 2012 Gregory Estrade
+ * Upstreaming code cleanup [including bcm2835_*] (c) 2013 Jan Petrous
+ *
+ * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft
+ * Written by Andrew Baumann
+ *
+ * This code is licensed under the GNU GPLv2 and later.
+ */
+
+#ifndef BCM2837_H
+#define BCM2837_H
+
+#include "hw/arm/bcm2836.h"
+
+#define TYPE_BCM2837 "bcm2837"
+#define BCM2837(obj) OBJECT_CHECK(BCM2836State, (obj), TYPE_BCM2837)
+
+#endif /* BCM2837_H */
diff --git a/include/hw/arm/raspi_platform.h
b/include/hw/arm/raspi_platform.h
index 6467e88..9e6910b 100644
--- a/include/hw/arm/raspi_platform.h
+++ b/include/hw/arm/raspi_platform.h
@@ -1,5 +1,5 @@
 /*
- * bcm2708 aka bcm2835/2836 aka Raspberry Pi/Pi2 SoC platform defines
+ * bcm2708 aka bcm2835/2836/2837 aka Raspberry Pi/Pi2 SoC platform defines
  *
  * These definitions are derived from those in Raspbian Linux at
  * arch/arm/mach-{bcm2708,bcm2709}/include/mach/platform.h
-- 
2.14.2
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by KONRAD Frederic 6 years, 5 months ago
Hi,

Thanks for your patch.

I'd split the patch as there are different piece of work here.

eg:
bcm2835: checking that the parameters are valid
adding bcm2837
adding raspi3
etc..

And you should run your patch through ./script/checkpatch.

See: https://wiki.qemu.org/Contribute/SubmitAPatch

Thanks,
Fred

On 10/22/2017 03:20 PM, bzt bzt wrote:
> Dear All,
> 
> I've added support for "-M raspi3" to qemu. This is my first patch, I hope
> it's okay. The github repo is here: https://github.com/bztsrc/qemu-raspi3
> in case my patch does not work for some reason.
> 
>  From 1f10f957b57f336728097803bf8339a5577dd3c2 Mon Sep 17 00:00:00 2001
> From: bzt <bztemail@gmail.com>
> Date: Sun, 22 Oct 2017 14:59:20 +0200
> Subject: [PATCH] BCM2837 and machine raspi3
> 
> Signed-off-by: bzt <bztemail@gmail.com>
> ---
>   hw/arm/Makefile.objs            |   2 +-
>   hw/arm/bcm2835_peripherals.c    |  10 ++-
>   hw/arm/bcm2836.c                |   6 --
>   hw/arm/bcm2837.c                | 179
> ++++++++++++++++++++++++++++++++++++++++
>   hw/arm/raspi.c                  |  79 ++++++++++++++++--
>   include/hw/arm/bcm2836.h        |   6 ++
>   include/hw/arm/bcm2837.h        |  19 +++++
>   include/hw/arm/raspi_platform.h |   2 +-
>   8 files changed, 287 insertions(+), 16 deletions(-)
>   create mode 100644 hw/arm/bcm2837.c
>   create mode 100644 include/hw/arm/bcm2837.h
> 
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 2794e08..72b60e1 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -11,7 +11,7 @@ obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o
> pxa2xx_pic.o
>   obj-$(CONFIG_DIGIC) += digic.o
>   obj-y += omap1.o omap2.o strongarm.o
>   obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o
> -obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o raspi.o
> +obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o bcm2837.o raspi.o
>   obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
>   obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o xlnx-zcu102.o
>   obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> index 12e0dd1..f79ce36 100644
> --- a/hw/arm/bcm2835_peripherals.c
> +++ b/hw/arm/bcm2835_peripherals.c
> @@ -212,7 +212,15 @@ static void bcm2835_peripherals_realize(DeviceState
> *dev, Error **errp)
>           error_propagate(errp, err);
>           return;
>       }
> -
> +    // check if parameters are valid
> +    if (ram_size < vcram_size + 64*1024*1024) {
> +        error_setg(errp, "%s: not enough ram for VideoCore",
> +                   __func__);
> +        return;
> +    }
> +    // if vcram_size is bigger than ram_size, this will silently overflow
> +    // and generate a not very informative "Parameter 'vcram-base' expects
> +    // uint32_t" message...
>       object_property_set_uint(OBJECT(&s->fb), ram_size - vcram_size,
>                                "vcram-base", &err);
>       if (err) {
> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 8c43291..db40c8e 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c
> @@ -17,12 +17,6 @@
>   #include "hw/sysbus.h"
>   #include "exec/address-spaces.h"
> 
> -/* Peripheral base address seen by the CPU */
> -#define BCM2836_PERI_BASE       0x3F000000
> -
> -/* "QA7" (Pi2) interrupt controller and mailboxes etc. */
> -#define BCM2836_CONTROL_BASE    0x40000000
> -
>   static void bcm2836_init(Object *obj)
>   {
>       BCM2836State *s = BCM2836(obj);
> diff --git a/hw/arm/bcm2837.c b/hw/arm/bcm2837.c
> new file mode 100644
> index 0000000..1bab93b
> --- /dev/null
> +++ b/hw/arm/bcm2837.c
> @@ -0,0 +1,179 @@
> +/*
> + * Raspberry Pi emulation (c) 2012 Gregory Estrade
> + * Upstreaming code cleanup [including bcm2835_*] (c) 2013 Jan Petrous
> + *
> + * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft
> + * Written by Andrew Baumann
> + *
> + * Raspberry Pi 3 emulation 2017 by bzt
> + *
> + * This code is licensed under the GNU GPLv2 and later.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "cpu.h"
> +#include "hw/arm/bcm2836.h"
> +#include "hw/arm/bcm2837.h"
> +#include "hw/arm/raspi_platform.h"
> +#include "hw/sysbus.h"
> +#include "exec/address-spaces.h"
> +
> +/* According to
> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2837/README.md
> + * The underlying architecture of the BCM2837 is identical to the BCM2836.
> The only significant
> + * difference is the replacement of the ARMv7 quad core cluster with a
> quad-core ARM Cortex A53
> + * (ARMv8) cluster. So we use cortex-a53- here. */
> +
> +static void bcm2837_init(Object *obj)
> +{
> +    BCM2836State *s = BCM2837(obj);
> +    int n;
> +
> +    for (n = 0; n < BCM2836_NCPUS; n++) {
> +        object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
> +                          "cortex-a53-" TYPE_ARM_CPU);
> +        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
> +                                  &error_abort);
> +    }
> +
> +    object_initialize(&s->control, sizeof(s->control),
> TYPE_BCM2836_CONTROL);
> +    object_property_add_child(obj, "control", OBJECT(&s->control), NULL);
> +    qdev_set_parent_bus(DEVICE(&s->control), sysbus_get_default());
> +
> +    object_initialize(&s->peripherals, sizeof(s->peripherals),
> +                      TYPE_BCM2835_PERIPHERALS);
> +    object_property_add_child(obj, "peripherals", OBJECT(&s->peripherals),
> +                              &error_abort);
> +    object_property_add_alias(obj, "board-rev", OBJECT(&s->peripherals),
> +                              "board-rev", &error_abort);
> +    object_property_add_alias(obj, "vcram-size", OBJECT(&s->peripherals),
> +                              "vcram-size", &error_abort);
> +    qdev_set_parent_bus(DEVICE(&s->peripherals), sysbus_get_default());
> +}
> +
> +static void bcm2837_realize(DeviceState *dev, Error **errp)
> +{
> +    BCM2836State *s = BCM2837(dev);
> +    Object *obj;
> +    Error *err = NULL;
> +    int n;
> +
> +    /* common peripherals from bcm2835 */
> +
> +    obj = object_property_get_link(OBJECT(dev), "ram", &err);
> +    if (obj == NULL) {
> +        error_setg(errp, "%s: required ram link not found: %s",
> +                   __func__, error_get_pretty(err));
> +        return;
> +    }
> +
> +    object_property_add_const_link(OBJECT(&s->peripherals), "ram", obj,
> &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    object_property_set_bool(OBJECT(&s->peripherals), true, "realized",
> &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    object_property_add_alias(OBJECT(s), "sd-bus", OBJECT(&s->peripherals),
> +                              "sd-bus", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->peripherals), 0,
> +                            BCM2836_PERI_BASE, 1);
> +
> +    /* bcm2836 interrupt controller (and mailboxes, etc.) */
> +    object_property_set_bool(OBJECT(&s->control), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0, BCM2836_CONTROL_BASE);
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 0,
> +        qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-irq", 0));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 1,
> +        qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-fiq", 0));
> +
> +    for (n = 0; n < BCM2836_NCPUS; n++) {
> +        /* Mirror bcm2836, which has clusterid set to 0xf
> +         * TODO: this should be converted to a property of ARM_CPU
> +         */
> +        s->cpus[n].mp_affinity = 0xF00 | n;
> +
> +        /* set periphbase/CBAR value for CPU-local registers */
> +        object_property_set_int(OBJECT(&s->cpus[n]),
> +                                BCM2836_PERI_BASE + MCORE_OFFSET,
> +                                "reset-cbar", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +
> +        /* start powered off if not enabled */
> +        object_property_set_bool(OBJECT(&s->cpus[n]), n >= s->enabled_cpus,
> +                                 "start-powered-off", &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +
> +        object_property_set_bool(OBJECT(&s->cpus[n]), true, "realized",
> &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +
> +        /* Connect irq/fiq outputs from the interrupt controller. */
> +        qdev_connect_gpio_out_named(DEVICE(&s->control), "irq", n,
> +                qdev_get_gpio_in(DEVICE(&s->cpus[n]), ARM_CPU_IRQ));
> +        qdev_connect_gpio_out_named(DEVICE(&s->control), "fiq", n,
> +                qdev_get_gpio_in(DEVICE(&s->cpus[n]), ARM_CPU_FIQ));
> +
> +        /* Connect timers from the CPU to the interrupt controller */
> +        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_PHYS,
> +                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpnsirq",
> n));
> +        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_VIRT,
> +                qdev_get_gpio_in_named(DEVICE(&s->control), "cntvirq", n));
> +        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_HYP,
> +                qdev_get_gpio_in_named(DEVICE(&s->control), "cnthpirq",
> n));
> +        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_SEC,
> +                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpsirq",
> n));
> +    }
> +}
> +
> +static Property bcm2837_props[] = {
> +    DEFINE_PROP_UINT32("enabled-cpus", BCM2836State, enabled_cpus,
> BCM2836_NCPUS),
> +    DEFINE_PROP_END_OF_LIST()
> +};
> +
> +static void bcm2837_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    dc->props = bcm2837_props;
> +    dc->realize = bcm2837_realize;
> +}
> +
> +static const TypeInfo bcm2837_type_info = {
> +    .name = TYPE_BCM2837,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(BCM2836State),
> +    .instance_init = bcm2837_init,
> +    .class_init = bcm2837_class_init,
> +};
> +
> +static void bcm2837_register_types(void)
> +{
> +    type_register_static(&bcm2837_type_info);
> +}
> +
> +type_init(bcm2837_register_types)
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 5941c9f..726a426 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -5,6 +5,8 @@
>    * Rasperry Pi 2 emulation Copyright (c) 2015, Microsoft
>    * Written by Andrew Baumann
>    *
> + * Raspberry Pi 3 emulation 2017 by bzt
> + *
>    * This code is licensed under the GNU GPLv2 and later.
>    */
> 
> @@ -13,6 +15,7 @@
>   #include "qemu-common.h"
>   #include "cpu.h"
>   #include "hw/arm/bcm2836.h"
> +#include "hw/arm/bcm2837.h"
>   #include "qemu/error-report.h"
>   #include "hw/boards.h"
>   #include "hw/loader.h"
> @@ -22,10 +25,11 @@
>   #define SMPBOOT_ADDR    0x300 /* this should leave enough space for ATAGS
> */
>   #define MVBAR_ADDR      0x400 /* secure vectors */
>   #define BOARDSETUP_ADDR (MVBAR_ADDR + 0x20) /* board setup code */
> -#define FIRMWARE_ADDR   0x8000 /* Pi loads kernel.img here by default */
> +#define FIRMWARE_ADDR_2    0x8000 /* Pi 2 loads kernel.img here by default
> */
> +#define FIRMWARE_ADDR_3   0x80000 /* Pi 3 loads kernel8.img here by
> default */
> 
>   /* Table of Linux board IDs for different Pi versions */
> -static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43};
> +static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};
> 
>   typedef struct RasPiState {
>       BCM2836State soc;
> @@ -73,6 +77,7 @@ static void reset_secondary(ARMCPU *cpu, const struct
> arm_boot_info *info)
>   static void setup_boot(MachineState *machine, int version, size_t ram_size)
>   {
>       static struct arm_boot_info binfo;
> +    hwaddr entry;
>       int r;
> 
>       binfo.board_id = raspi_boardid[version];
> @@ -83,11 +88,12 @@ static void setup_boot(MachineState *machine, int
> version, size_t ram_size)
>       binfo.secure_board_setup = true;
>       binfo.secure_boot = true;
> 
> -    /* Pi2 requires SMP setup */
> -    if (version == 2) {
> +    /* Pi2 and Pi3 requires SMP setup */
> +    if (version == 2 || version == 3) {
>           binfo.smp_loader_start = SMPBOOT_ADDR;
>           binfo.write_secondary_boot = write_smpboot;
>           binfo.secondary_cpu_reset_hook = reset_secondary;
> +        entry = version == 2 ? FIRMWARE_ADDR_2 : FIRMWARE_ADDR_3;
>       }
> 
>       /* If the user specified a "firmware" image (e.g. UEFI), we bypass
> @@ -95,14 +101,14 @@ static void setup_boot(MachineState *machine, int
> version, size_t ram_size)
>        */
>       if (machine->firmware) {
>           /* load the firmware image (typically kernel.img) */
> -        r = load_image_targphys(machine->firmware, FIRMWARE_ADDR,
> -                                ram_size - FIRMWARE_ADDR);
> +        r = load_image_targphys(machine->firmware, entry,
> +                                ram_size - entry);
>           if (r < 0) {
>               error_report("Failed to load firmware from %s",
> machine->firmware);
>               exit(1);
>           }
> 
> -        binfo.entry = FIRMWARE_ADDR;
> +        binfo.entry = entry;
>           binfo.firmware_loaded = true;
>       } else {
>           binfo.kernel_filename = machine->kernel_filename;
> @@ -171,3 +177,62 @@ static void raspi2_machine_init(MachineClass *mc)
>       mc->ignore_memory_transaction_failures = true;
>   };
>   DEFINE_MACHINE("raspi2", raspi2_machine_init)
> +
> +static void raspi3_init(MachineState *machine)
> +{
> +    RasPiState *s = g_new0(RasPiState, 1);
> +    uint32_t vcram_size;
> +    DriveInfo *di;
> +    BlockBackend *blk;
> +    BusState *bus;
> +    DeviceState *carddev;
> +
> +    object_initialize(&s->soc, sizeof(s->soc), TYPE_BCM2837);
> +    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> +                              &error_abort);
> +
> +    /* Allocate and map RAM */
> +    memory_region_allocate_system_memory(&s->ram, OBJECT(machine), "ram",
> +                                         machine->ram_size);
> +    /* FIXME: Remove when we have custom CPU address space support */
> +    memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram,
> 0);
> +
> +    /* Setup the SOC */
> +    object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram),
> +                                   &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
> +                            &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), 0xa02082, "board-rev",
> +                            &error_abort);
> +    object_property_set_bool(OBJECT(&s->soc), true, "realized",
> &error_abort);
> +
> +    /* Create and plug in the SD cards */
> +    di = drive_get_next(IF_SD);
> +    blk = di ? blk_by_legacy_dinfo(di) : NULL;
> +    bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
> +    if (bus == NULL) {
> +        error_report("No SD bus found in SOC object");
> +        exit(1);
> +    }
> +    carddev = qdev_create(bus, TYPE_SD_CARD);
> +    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> +    object_property_set_bool(OBJECT(carddev), true, "realized",
> &error_fatal);
> +
> +    vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
> +                                          &error_abort);
> +    setup_boot(machine, 3, machine->ram_size - vcram_size);
> +}
> +
> +static void raspi3_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "Raspberry Pi 3";
> +    mc->init = raspi3_init;
> +    mc->block_default_type = IF_SD;
> +    mc->no_parallel = 1;
> +    mc->no_floppy = 1;
> +    mc->no_cdrom = 1;
> +    mc->max_cpus = BCM2836_NCPUS;
> +    mc->default_ram_size = 1024 * 1024 * 1024;
> +    mc->ignore_memory_transaction_failures = true;
> +};
> +DEFINE_MACHINE("raspi3", raspi3_machine_init)
> diff --git a/include/hw/arm/bcm2836.h b/include/hw/arm/bcm2836.h
> index 76de199..ee6b9dc 100644
> --- a/include/hw/arm/bcm2836.h
> +++ b/include/hw/arm/bcm2836.h
> @@ -20,6 +20,12 @@
> 
>   #define BCM2836_NCPUS 4
> 
> +/* Peripheral base address seen by the CPU */
> +#define BCM2836_PERI_BASE       0x3F000000
> +
> +/* "QA7" (Pi2/Pi8) interrupt controller and mailboxes etc. */
> +#define BCM2836_CONTROL_BASE    0x40000000
> +
>   typedef struct BCM2836State {
>       /*< private >*/
>       DeviceState parent_obj;
> diff --git a/include/hw/arm/bcm2837.h b/include/hw/arm/bcm2837.h
> new file mode 100644
> index 0000000..5c7be8a
> --- /dev/null
> +++ b/include/hw/arm/bcm2837.h
> @@ -0,0 +1,19 @@
> +/*
> + * Raspberry Pi emulation (c) 2012 Gregory Estrade
> + * Upstreaming code cleanup [including bcm2835_*] (c) 2013 Jan Petrous
> + *
> + * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft
> + * Written by Andrew Baumann
> + *
> + * This code is licensed under the GNU GPLv2 and later.
> + */
> +
> +#ifndef BCM2837_H
> +#define BCM2837_H
> +
> +#include "hw/arm/bcm2836.h"
> +
> +#define TYPE_BCM2837 "bcm2837"
> +#define BCM2837(obj) OBJECT_CHECK(BCM2836State, (obj), TYPE_BCM2837)
> +
> +#endif /* BCM2837_H */
> diff --git a/include/hw/arm/raspi_platform.h
> b/include/hw/arm/raspi_platform.h
> index 6467e88..9e6910b 100644
> --- a/include/hw/arm/raspi_platform.h
> +++ b/include/hw/arm/raspi_platform.h
> @@ -1,5 +1,5 @@
>   /*
> - * bcm2708 aka bcm2835/2836 aka Raspberry Pi/Pi2 SoC platform defines
> + * bcm2708 aka bcm2835/2836/2837 aka Raspberry Pi/Pi2 SoC platform defines
>    *
>    * These definitions are derived from those in Raspbian Linux at
>    * arch/arm/mach-{bcm2708,bcm2709}/include/mach/platform.h
> 

Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 5 months ago
Okay, thanks! Sorry I haven't splitted.

Best wishes,
Zoli (bzt)

On Mon, Oct 23, 2017 at 11:34 AM, KONRAD Frederic <
frederic.konrad@adacore.com> wrote:

> Hi,
>
> Thanks for your patch.
>
> I'd split the patch as there are different piece of work here.
>
> eg:
> bcm2835: checking that the parameters are valid
> adding bcm2837
> adding raspi3
> etc..
>
> And you should run your patch through ./script/checkpatch.
>
> See: https://wiki.qemu.org/Contribute/SubmitAPatch
>
> Thanks,
> Fred
>
>
> On 10/22/2017 03:20 PM, bzt bzt wrote:
>
>> Dear All,
>>
>> I've added support for "-M raspi3" to qemu. This is my first patch, I hope
>> it's okay. The github repo is here: https://github.com/bztsrc/qemu-raspi3
>> in case my patch does not work for some reason.
>>
>>  From 1f10f957b57f336728097803bf8339a5577dd3c2 Mon Sep 17 00:00:00 2001
>> From: bzt <bztemail@gmail.com>
>> Date: Sun, 22 Oct 2017 14:59:20 +0200
>> Subject: [PATCH] BCM2837 and machine raspi3
>>
>> Signed-off-by: bzt <bztemail@gmail.com>
>> ---
>>   hw/arm/Makefile.objs            |   2 +-
>>   hw/arm/bcm2835_peripherals.c    |  10 ++-
>>   hw/arm/bcm2836.c                |   6 --
>>   hw/arm/bcm2837.c                | 179
>> ++++++++++++++++++++++++++++++++++++++++
>>   hw/arm/raspi.c                  |  79 ++++++++++++++++--
>>   include/hw/arm/bcm2836.h        |   6 ++
>>   include/hw/arm/bcm2837.h        |  19 +++++
>>   include/hw/arm/raspi_platform.h |   2 +-
>>   8 files changed, 287 insertions(+), 16 deletions(-)
>>   create mode 100644 hw/arm/bcm2837.c
>>   create mode 100644 include/hw/arm/bcm2837.h
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index 2794e08..72b60e1 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -11,7 +11,7 @@ obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o
>> pxa2xx_pic.o
>>   obj-$(CONFIG_DIGIC) += digic.o
>>   obj-y += omap1.o omap2.o strongarm.o
>>   obj-$(CONFIG_ALLWINNER_A10) += allwinner-a10.o cubieboard.o
>> -obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o raspi.o
>> +obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o bcm2837.o raspi.o
>>   obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
>>   obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o xlnx-zcu102.o
>>   obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
>> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
>> index 12e0dd1..f79ce36 100644
>> --- a/hw/arm/bcm2835_peripherals.c
>> +++ b/hw/arm/bcm2835_peripherals.c
>> @@ -212,7 +212,15 @@ static void bcm2835_peripherals_realize(DeviceState
>> *dev, Error **errp)
>>           error_propagate(errp, err);
>>           return;
>>       }
>> -
>> +    // check if parameters are valid
>> +    if (ram_size < vcram_size + 64*1024*1024) {
>> +        error_setg(errp, "%s: not enough ram for VideoCore",
>> +                   __func__);
>> +        return;
>> +    }
>> +    // if vcram_size is bigger than ram_size, this will silently overflow
>> +    // and generate a not very informative "Parameter 'vcram-base'
>> expects
>> +    // uint32_t" message...
>>       object_property_set_uint(OBJECT(&s->fb), ram_size - vcram_size,
>>                                "vcram-base", &err);
>>       if (err) {
>> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
>> index 8c43291..db40c8e 100644
>> --- a/hw/arm/bcm2836.c
>> +++ b/hw/arm/bcm2836.c
>> @@ -17,12 +17,6 @@
>>   #include "hw/sysbus.h"
>>   #include "exec/address-spaces.h"
>>
>> -/* Peripheral base address seen by the CPU */
>> -#define BCM2836_PERI_BASE       0x3F000000
>> -
>> -/* "QA7" (Pi2) interrupt controller and mailboxes etc. */
>> -#define BCM2836_CONTROL_BASE    0x40000000
>> -
>>   static void bcm2836_init(Object *obj)
>>   {
>>       BCM2836State *s = BCM2836(obj);
>> diff --git a/hw/arm/bcm2837.c b/hw/arm/bcm2837.c
>> new file mode 100644
>> index 0000000..1bab93b
>> --- /dev/null
>> +++ b/hw/arm/bcm2837.c
>> @@ -0,0 +1,179 @@
>> +/*
>> + * Raspberry Pi emulation (c) 2012 Gregory Estrade
>> + * Upstreaming code cleanup [including bcm2835_*] (c) 2013 Jan Petrous
>> + *
>> + * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft
>> + * Written by Andrew Baumann
>> + *
>> + * Raspberry Pi 3 emulation 2017 by bzt
>> + *
>> + * This code is licensed under the GNU GPLv2 and later.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu-common.h"
>> +#include "cpu.h"
>> +#include "hw/arm/bcm2836.h"
>> +#include "hw/arm/bcm2837.h"
>> +#include "hw/arm/raspi_platform.h"
>> +#include "hw/sysbus.h"
>> +#include "exec/address-spaces.h"
>> +
>> +/* According to
>> https://www.raspberrypi.org/documentation/hardware/raspberry
>> pi/bcm2837/README.md
>> + * The underlying architecture of the BCM2837 is identical to the
>> BCM2836.
>> The only significant
>> + * difference is the replacement of the ARMv7 quad core cluster with a
>> quad-core ARM Cortex A53
>> + * (ARMv8) cluster. So we use cortex-a53- here. */
>> +
>> +static void bcm2837_init(Object *obj)
>> +{
>> +    BCM2836State *s = BCM2837(obj);
>> +    int n;
>> +
>> +    for (n = 0; n < BCM2836_NCPUS; n++) {
>> +        object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
>> +                          "cortex-a53-" TYPE_ARM_CPU);
>> +        object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
>> +                                  &error_abort);
>> +    }
>> +
>> +    object_initialize(&s->control, sizeof(s->control),
>> TYPE_BCM2836_CONTROL);
>> +    object_property_add_child(obj, "control", OBJECT(&s->control), NULL);
>> +    qdev_set_parent_bus(DEVICE(&s->control), sysbus_get_default());
>> +
>> +    object_initialize(&s->peripherals, sizeof(s->peripherals),
>> +                      TYPE_BCM2835_PERIPHERALS);
>> +    object_property_add_child(obj, "peripherals",
>> OBJECT(&s->peripherals),
>> +                              &error_abort);
>> +    object_property_add_alias(obj, "board-rev", OBJECT(&s->peripherals),
>> +                              "board-rev", &error_abort);
>> +    object_property_add_alias(obj, "vcram-size", OBJECT(&s->peripherals),
>> +                              "vcram-size", &error_abort);
>> +    qdev_set_parent_bus(DEVICE(&s->peripherals), sysbus_get_default());
>> +}
>> +
>> +static void bcm2837_realize(DeviceState *dev, Error **errp)
>> +{
>> +    BCM2836State *s = BCM2837(dev);
>> +    Object *obj;
>> +    Error *err = NULL;
>> +    int n;
>> +
>> +    /* common peripherals from bcm2835 */
>> +
>> +    obj = object_property_get_link(OBJECT(dev), "ram", &err);
>> +    if (obj == NULL) {
>> +        error_setg(errp, "%s: required ram link not found: %s",
>> +                   __func__, error_get_pretty(err));
>> +        return;
>> +    }
>> +
>> +    object_property_add_const_link(OBJECT(&s->peripherals), "ram", obj,
>> &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +
>> +    object_property_set_bool(OBJECT(&s->peripherals), true, "realized",
>> &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +
>> +    object_property_add_alias(OBJECT(s), "sd-bus",
>> OBJECT(&s->peripherals),
>> +                              "sd-bus", &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +
>> +    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->peripherals), 0,
>> +                            BCM2836_PERI_BASE, 1);
>> +
>> +    /* bcm2836 interrupt controller (and mailboxes, etc.) */
>> +    object_property_set_bool(OBJECT(&s->control), true, "realized",
>> &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->control), 0,
>> BCM2836_CONTROL_BASE);
>> +
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 0,
>> +        qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-irq", 0));
>> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 1,
>> +        qdev_get_gpio_in_named(DEVICE(&s->control), "gpu-fiq", 0));
>> +
>> +    for (n = 0; n < BCM2836_NCPUS; n++) {
>> +        /* Mirror bcm2836, which has clusterid set to 0xf
>> +         * TODO: this should be converted to a property of ARM_CPU
>> +         */
>> +        s->cpus[n].mp_affinity = 0xF00 | n;
>> +
>> +        /* set periphbase/CBAR value for CPU-local registers */
>> +        object_property_set_int(OBJECT(&s->cpus[n]),
>> +                                BCM2836_PERI_BASE + MCORE_OFFSET,
>> +                                "reset-cbar", &err);
>> +        if (err) {
>> +            error_propagate(errp, err);
>> +            return;
>> +        }
>> +
>> +        /* start powered off if not enabled */
>> +        object_property_set_bool(OBJECT(&s->cpus[n]), n >=
>> s->enabled_cpus,
>> +                                 "start-powered-off", &err);
>> +        if (err) {
>> +            error_propagate(errp, err);
>> +            return;
>> +        }
>> +
>> +        object_property_set_bool(OBJECT(&s->cpus[n]), true, "realized",
>> &err);
>> +        if (err) {
>> +            error_propagate(errp, err);
>> +            return;
>> +        }
>> +
>> +        /* Connect irq/fiq outputs from the interrupt controller. */
>> +        qdev_connect_gpio_out_named(DEVICE(&s->control), "irq", n,
>> +                qdev_get_gpio_in(DEVICE(&s->cpus[n]), ARM_CPU_IRQ));
>> +        qdev_connect_gpio_out_named(DEVICE(&s->control), "fiq", n,
>> +                qdev_get_gpio_in(DEVICE(&s->cpus[n]), ARM_CPU_FIQ));
>> +
>> +        /* Connect timers from the CPU to the interrupt controller */
>> +        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_PHYS,
>> +                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpnsirq",
>> n));
>> +        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_VIRT,
>> +                qdev_get_gpio_in_named(DEVICE(&s->control), "cntvirq",
>> n));
>> +        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_HYP,
>> +                qdev_get_gpio_in_named(DEVICE(&s->control), "cnthpirq",
>> n));
>> +        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_SEC,
>> +                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpsirq",
>> n));
>> +    }
>> +}
>> +
>> +static Property bcm2837_props[] = {
>> +    DEFINE_PROP_UINT32("enabled-cpus", BCM2836State, enabled_cpus,
>> BCM2836_NCPUS),
>> +    DEFINE_PROP_END_OF_LIST()
>> +};
>> +
>> +static void bcm2837_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    dc->props = bcm2837_props;
>> +    dc->realize = bcm2837_realize;
>> +}
>> +
>> +static const TypeInfo bcm2837_type_info = {
>> +    .name = TYPE_BCM2837,
>> +    .parent = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(BCM2836State),
>> +    .instance_init = bcm2837_init,
>> +    .class_init = bcm2837_class_init,
>> +};
>> +
>> +static void bcm2837_register_types(void)
>> +{
>> +    type_register_static(&bcm2837_type_info);
>> +}
>> +
>> +type_init(bcm2837_register_types)
>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>> index 5941c9f..726a426 100644
>> --- a/hw/arm/raspi.c
>> +++ b/hw/arm/raspi.c
>> @@ -5,6 +5,8 @@
>>    * Rasperry Pi 2 emulation Copyright (c) 2015, Microsoft
>>    * Written by Andrew Baumann
>>    *
>> + * Raspberry Pi 3 emulation 2017 by bzt
>> + *
>>    * This code is licensed under the GNU GPLv2 and later.
>>    */
>>
>> @@ -13,6 +15,7 @@
>>   #include "qemu-common.h"
>>   #include "cpu.h"
>>   #include "hw/arm/bcm2836.h"
>> +#include "hw/arm/bcm2837.h"
>>   #include "qemu/error-report.h"
>>   #include "hw/boards.h"
>>   #include "hw/loader.h"
>> @@ -22,10 +25,11 @@
>>   #define SMPBOOT_ADDR    0x300 /* this should leave enough space for
>> ATAGS
>> */
>>   #define MVBAR_ADDR      0x400 /* secure vectors */
>>   #define BOARDSETUP_ADDR (MVBAR_ADDR + 0x20) /* board setup code */
>> -#define FIRMWARE_ADDR   0x8000 /* Pi loads kernel.img here by default */
>> +#define FIRMWARE_ADDR_2    0x8000 /* Pi 2 loads kernel.img here by
>> default
>> */
>> +#define FIRMWARE_ADDR_3   0x80000 /* Pi 3 loads kernel8.img here by
>> default */
>>
>>   /* Table of Linux board IDs for different Pi versions */
>> -static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43};
>> +static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] =
>> 0xc44};
>>
>>   typedef struct RasPiState {
>>       BCM2836State soc;
>> @@ -73,6 +77,7 @@ static void reset_secondary(ARMCPU *cpu, const struct
>> arm_boot_info *info)
>>   static void setup_boot(MachineState *machine, int version, size_t
>> ram_size)
>>   {
>>       static struct arm_boot_info binfo;
>> +    hwaddr entry;
>>       int r;
>>
>>       binfo.board_id = raspi_boardid[version];
>> @@ -83,11 +88,12 @@ static void setup_boot(MachineState *machine, int
>> version, size_t ram_size)
>>       binfo.secure_board_setup = true;
>>       binfo.secure_boot = true;
>>
>> -    /* Pi2 requires SMP setup */
>> -    if (version == 2) {
>> +    /* Pi2 and Pi3 requires SMP setup */
>> +    if (version == 2 || version == 3) {
>>           binfo.smp_loader_start = SMPBOOT_ADDR;
>>           binfo.write_secondary_boot = write_smpboot;
>>           binfo.secondary_cpu_reset_hook = reset_secondary;
>> +        entry = version == 2 ? FIRMWARE_ADDR_2 : FIRMWARE_ADDR_3;
>>       }
>>
>>       /* If the user specified a "firmware" image (e.g. UEFI), we bypass
>> @@ -95,14 +101,14 @@ static void setup_boot(MachineState *machine, int
>> version, size_t ram_size)
>>        */
>>       if (machine->firmware) {
>>           /* load the firmware image (typically kernel.img) */
>> -        r = load_image_targphys(machine->firmware, FIRMWARE_ADDR,
>> -                                ram_size - FIRMWARE_ADDR);
>> +        r = load_image_targphys(machine->firmware, entry,
>> +                                ram_size - entry);
>>           if (r < 0) {
>>               error_report("Failed to load firmware from %s",
>> machine->firmware);
>>               exit(1);
>>           }
>>
>> -        binfo.entry = FIRMWARE_ADDR;
>> +        binfo.entry = entry;
>>           binfo.firmware_loaded = true;
>>       } else {
>>           binfo.kernel_filename = machine->kernel_filename;
>> @@ -171,3 +177,62 @@ static void raspi2_machine_init(MachineClass *mc)
>>       mc->ignore_memory_transaction_failures = true;
>>   };
>>   DEFINE_MACHINE("raspi2", raspi2_machine_init)
>> +
>> +static void raspi3_init(MachineState *machine)
>> +{
>> +    RasPiState *s = g_new0(RasPiState, 1);
>> +    uint32_t vcram_size;
>> +    DriveInfo *di;
>> +    BlockBackend *blk;
>> +    BusState *bus;
>> +    DeviceState *carddev;
>> +
>> +    object_initialize(&s->soc, sizeof(s->soc), TYPE_BCM2837);
>> +    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
>> +                              &error_abort);
>> +
>> +    /* Allocate and map RAM */
>> +    memory_region_allocate_system_memory(&s->ram, OBJECT(machine),
>> "ram",
>> +                                         machine->ram_size);
>> +    /* FIXME: Remove when we have custom CPU address space support */
>> +    memory_region_add_subregion_overlap(get_system_memory(), 0, &s->ram,
>> 0);
>> +
>> +    /* Setup the SOC */
>> +    object_property_add_const_link(OBJECT(&s->soc), "ram",
>> OBJECT(&s->ram),
>> +                                   &error_abort);
>> +    object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
>> +                            &error_abort);
>> +    object_property_set_int(OBJECT(&s->soc), 0xa02082, "board-rev",
>> +                            &error_abort);
>> +    object_property_set_bool(OBJECT(&s->soc), true, "realized",
>> &error_abort);
>> +
>> +    /* Create and plug in the SD cards */
>> +    di = drive_get_next(IF_SD);
>> +    blk = di ? blk_by_legacy_dinfo(di) : NULL;
>> +    bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
>> +    if (bus == NULL) {
>> +        error_report("No SD bus found in SOC object");
>> +        exit(1);
>> +    }
>> +    carddev = qdev_create(bus, TYPE_SD_CARD);
>> +    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
>> +    object_property_set_bool(OBJECT(carddev), true, "realized",
>> &error_fatal);
>> +
>> +    vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
>> +                                          &error_abort);
>> +    setup_boot(machine, 3, machine->ram_size - vcram_size);
>> +}
>> +
>> +static void raspi3_machine_init(MachineClass *mc)
>> +{
>> +    mc->desc = "Raspberry Pi 3";
>> +    mc->init = raspi3_init;
>> +    mc->block_default_type = IF_SD;
>> +    mc->no_parallel = 1;
>> +    mc->no_floppy = 1;
>> +    mc->no_cdrom = 1;
>> +    mc->max_cpus = BCM2836_NCPUS;
>> +    mc->default_ram_size = 1024 * 1024 * 1024;
>> +    mc->ignore_memory_transaction_failures = true;
>> +};
>> +DEFINE_MACHINE("raspi3", raspi3_machine_init)
>> diff --git a/include/hw/arm/bcm2836.h b/include/hw/arm/bcm2836.h
>> index 76de199..ee6b9dc 100644
>> --- a/include/hw/arm/bcm2836.h
>> +++ b/include/hw/arm/bcm2836.h
>> @@ -20,6 +20,12 @@
>>
>>   #define BCM2836_NCPUS 4
>>
>> +/* Peripheral base address seen by the CPU */
>> +#define BCM2836_PERI_BASE       0x3F000000
>> +
>> +/* "QA7" (Pi2/Pi8) interrupt controller and mailboxes etc. */
>> +#define BCM2836_CONTROL_BASE    0x40000000
>> +
>>   typedef struct BCM2836State {
>>       /*< private >*/
>>       DeviceState parent_obj;
>> diff --git a/include/hw/arm/bcm2837.h b/include/hw/arm/bcm2837.h
>> new file mode 100644
>> index 0000000..5c7be8a
>> --- /dev/null
>> +++ b/include/hw/arm/bcm2837.h
>> @@ -0,0 +1,19 @@
>> +/*
>> + * Raspberry Pi emulation (c) 2012 Gregory Estrade
>> + * Upstreaming code cleanup [including bcm2835_*] (c) 2013 Jan Petrous
>> + *
>> + * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft
>> + * Written by Andrew Baumann
>> + *
>> + * This code is licensed under the GNU GPLv2 and later.
>> + */
>> +
>> +#ifndef BCM2837_H
>> +#define BCM2837_H
>> +
>> +#include "hw/arm/bcm2836.h"
>> +
>> +#define TYPE_BCM2837 "bcm2837"
>> +#define BCM2837(obj) OBJECT_CHECK(BCM2836State, (obj), TYPE_BCM2837)
>> +
>> +#endif /* BCM2837_H */
>> diff --git a/include/hw/arm/raspi_platform.h
>> b/include/hw/arm/raspi_platform.h
>> index 6467e88..9e6910b 100644
>> --- a/include/hw/arm/raspi_platform.h
>> +++ b/include/hw/arm/raspi_platform.h
>> @@ -1,5 +1,5 @@
>>   /*
>> - * bcm2708 aka bcm2835/2836 aka Raspberry Pi/Pi2 SoC platform defines
>> + * bcm2708 aka bcm2835/2836/2837 aka Raspberry Pi/Pi2 SoC platform
>> defines
>>    *
>>    * These definitions are derived from those in Raspbian Linux at
>>    * arch/arm/mach-{bcm2708,bcm2709}/include/mach/platform.h
>>
>>
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by Andrew Baumann via Qemu-devel 6 years, 5 months ago
> From: Qemu-devel On Behalf Of bzt bzt
> Sent: Sunday, 22 October 2017 06:21
> 
> I've added support for "-M raspi3" to qemu. This is my first patch, I hope
> it's okay. The github repo is here: https://github.com/bztsrc/qemu-raspi3
> in case my patch does not work for some reason.

Thanks for the patch!

[...]

> Subject: [PATCH] BCM2837 and machine raspi3

*add

[...]

> --- /dev/null
> +++ b/hw/arm/bcm2837.c
> @@ -0,0 +1,179 @@
> +/*
> + * Raspberry Pi emulation (c) 2012 Gregory Estrade
> + * Upstreaming code cleanup [including bcm2835_*] (c) 2013 Jan Petrous
> + *
> + * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft
> + * Written by Andrew Baumann
> + *
> + * Raspberry Pi 3 emulation 2017 by bzt
> + *
> + * This code is licensed under the GNU GPLv2 and later.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "cpu.h"
> +#include "hw/arm/bcm2836.h"
> +#include "hw/arm/bcm2837.h"
> +#include "hw/arm/raspi_platform.h"
> +#include "hw/sysbus.h"
> +#include "exec/address-spaces.h"
> +
> +/* According to
> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2837/
> README.md
> + * The underlying architecture of the BCM2837 is identical to the BCM2836.
> The only significant
> + * difference is the replacement of the ARMv7 quad core cluster with a
> quad-core ARM Cortex A53
> + * (ARMv8) cluster. So we use cortex-a53- here. */

Are there any changes from bcm2836 other than the CPU model?

Duplicating the whole file just to have a different CPU seems like a bad idea to me. I would suggest parameterising the CPU model in bcm2836 (and maybe noting in header comments etc. that it can also model 2837), and then instantiating it from raspi.c with the correct CPU type depending on which machine is being used. That will also reduce the size of the patch significantly, which will make it easier to get it reviewed.

(Alternatively, if there are minor but non-trivial differences between 2836 and 2837 other than the CPU model, you may want to create something like bcm2835_peripherals that contains all the functionality common to both. But I suspect that isn't the case here.)

> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
[...]
> @@ -171,3 +177,62 @@ static void raspi2_machine_init(MachineClass *mc)
>      mc->ignore_memory_transaction_failures = true;
>  };
>  DEFINE_MACHINE("raspi2", raspi2_machine_init)
> +
> +static void raspi3_init(MachineState *machine)
> +{
> +    RasPiState *s = g_new0(RasPiState, 1);
> +    uint32_t vcram_size;
> +    DriveInfo *di;
> +    BlockBackend *blk;
> +    BusState *bus;
> +    DeviceState *carddev;
> +
> +    object_initialize(&s->soc, sizeof(s->soc), TYPE_BCM2837);
> +    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> +                              &error_abort);
> +
> +    /* Allocate and map RAM */
> +    memory_region_allocate_system_memory(&s->ram, OBJECT(machine),
> "ram",
> +                                         machine->ram_size);
> +    /* FIXME: Remove when we have custom CPU address space support */
> +    memory_region_add_subregion_overlap(get_system_memory(), 0, &s-
> >ram,
> 0);
> +
> +    /* Setup the SOC */
> +    object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s-
> >ram),
> +                                   &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
> +                            &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), 0xa02082, "board-rev",
> +                            &error_abort);
> +    object_property_set_bool(OBJECT(&s->soc), true, "realized",
> &error_abort);
> +
> +    /* Create and plug in the SD cards */
> +    di = drive_get_next(IF_SD);
> +    blk = di ? blk_by_legacy_dinfo(di) : NULL;
> +    bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
> +    if (bus == NULL) {
> +        error_report("No SD bus found in SOC object");
> +        exit(1);
> +    }
> +    carddev = qdev_create(bus, TYPE_SD_CARD);
> +    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> +    object_property_set_bool(OBJECT(carddev), true, "realized",
> &error_fatal);
> +
> +    vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
> +                                          &error_abort);
> +    setup_boot(machine, 3, machine->ram_size - vcram_size);
> +}

Similarly, this looks like a clone of raspi2_init. I think it would be better to have one function, parametrised if needed.

Regards,
Andrew
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 5 months ago
Hi Andrew!

On Mon, Oct 23, 2017 at 6:34 PM, Andrew Baumann <
Andrew.Baumann@microsoft.com> wrote:

> > From: Qemu-devel On Behalf Of bzt bzt
> > Sent: Sunday, 22 October 2017 06:21
> >
> > I've added support for "-M raspi3" to qemu. This is my first patch, I
> hope
> > it's okay. The github repo is here: https://github.com/bztsrc/
> qemu-raspi3
> > in case my patch does not work for some reason.
>
> Thanks for the patch!
>

Welcome!


>
> [...]
>
> > Subject: [PATCH] BCM2837 and machine raspi3
>
> *add
>
> [...]
>
> > --- /dev/null
> > +++ b/hw/arm/bcm2837.c
> > @@ -0,0 +1,179 @@
> > +/*
> > + * Raspberry Pi emulation (c) 2012 Gregory Estrade
> > + * Upstreaming code cleanup [including bcm2835_*] (c) 2013 Jan Petrous
> > + *
> > + * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft
> > + * Written by Andrew Baumann
> > + *
> > + * Raspberry Pi 3 emulation 2017 by bzt
> > + *
> > + * This code is licensed under the GNU GPLv2 and later.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "qemu-common.h"
> > +#include "cpu.h"
> > +#include "hw/arm/bcm2836.h"
> > +#include "hw/arm/bcm2837.h"
> > +#include "hw/arm/raspi_platform.h"
> > +#include "hw/sysbus.h"
> > +#include "exec/address-spaces.h"
> > +
> > +/* According to
> > https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2837/
> > README.md
> > + * The underlying architecture of the BCM2837 is identical to the
> BCM2836.
> > The only significant
> > + * difference is the replacement of the ARMv7 quad core cluster with a
> > quad-core ARM Cortex A53
> > + * (ARMv8) cluster. So we use cortex-a53- here. */
>
> Are there any changes from bcm2836 other than the CPU model?


> Duplicating the whole file just to have a different CPU seems like a bad
> idea to me. I would suggest parameterising the CPU model in bcm2836 (and
> maybe noting in header comments etc. that it can also model 2837), and then
> instantiating it from raspi.c with the correct CPU type depending on which
> machine is being used. That will also reduce the size of the patch
> significantly, which will make it easier to get it reviewed.
>

For now, there's no more difference in the emulation than the CPU, but
BCM2837 has a lot more to offer (40 bit address space, wifi, bluetooth
etc.). So sooner or later it will need it's own source file to support all
of that without getting the code messy. I have asked Peter whether it does
worth writing future proof code, or keep the change at the bare minimum,
but had no answer yet.


>
> (Alternatively, if there are minor but non-trivial differences between
> 2836 and 2837 other than the CPU model, you may want to create something
> like bcm2835_peripherals that contains all the functionality common to
> both. But I suspect that isn't the case here.)
>

Well, bcm2837 is backward compatible with the bcm2836, but has a lot more
registers and a different boot up process (different address and no atags).
The common functionality is already in bcm2835_peripherals, and the MMIO
address change from bcm2835 to bcm2836 has already been added by you.
What's different is a lot more devices, which imho would be unwise to add
to bcm2835 or either to bcm2836. So to be future proof I've created a
separated file, but you are right at the current level of emulation it's
not necessary.


>
> > --- a/hw/arm/raspi.c
> > +++ b/hw/arm/raspi.c
> [...]
> > @@ -171,3 +177,62 @@ static void raspi2_machine_init(MachineClass *mc)
> >      mc->ignore_memory_transaction_failures = true;
> >  };
> >  DEFINE_MACHINE("raspi2", raspi2_machine_init)
> > +
> > +static void raspi3_init(MachineState *machine)
> > +{
> > +    RasPiState *s = g_new0(RasPiState, 1);
> > +    uint32_t vcram_size;
> > +    DriveInfo *di;
> > +    BlockBackend *blk;
> > +    BusState *bus;
> > +    DeviceState *carddev;
> > +
> > +    object_initialize(&s->soc, sizeof(s->soc), TYPE_BCM2837);
> > +    object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> > +                              &error_abort);
> > +
> > +    /* Allocate and map RAM */
> > +    memory_region_allocate_system_memory(&s->ram, OBJECT(machine),
> > "ram",
> > +                                         machine->ram_size);
> > +    /* FIXME: Remove when we have custom CPU address space support */
> > +    memory_region_add_subregion_overlap(get_system_memory(), 0, &s-
> > >ram,
> > 0);
> > +
> > +    /* Setup the SOC */
> > +    object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s-
> > >ram),
> > +                                   &error_abort);
> > +    object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
> > +                            &error_abort);
> > +    object_property_set_int(OBJECT(&s->soc), 0xa02082, "board-rev",
> > +                            &error_abort);
> > +    object_property_set_bool(OBJECT(&s->soc), true, "realized",
> > &error_abort);
> > +
> > +    /* Create and plug in the SD cards */
> > +    di = drive_get_next(IF_SD);
> > +    blk = di ? blk_by_legacy_dinfo(di) : NULL;
> > +    bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
> > +    if (bus == NULL) {
> > +        error_report("No SD bus found in SOC object");
> > +        exit(1);
> > +    }
> > +    carddev = qdev_create(bus, TYPE_SD_CARD);
> > +    qdev_prop_set_drive(carddev, "drive", blk, &error_fatal);
> > +    object_property_set_bool(OBJECT(carddev), true, "realized",
> > &error_fatal);
> > +
> > +    vcram_size = object_property_get_uint(OBJECT(&s->soc),
> "vcram-size",
> > +                                          &error_abort);
> > +    setup_boot(machine, 3, machine->ram_size - vcram_size);
> > +}
>
> Similarly, this looks like a clone of raspi2_init. I think it would be
> better to have one function, parametrised if needed.
>

For now, yes. But the extra devices bcm2837 have will need extra
initialization, and I though it would be clearer to separate in advance.
Again, I've asked Peter about this, but had no response. As I see having a
parametrised single function is simplier for now, but could lead to a messy
code later when all the SoC features will be added. But I'm ain't no expert
on qemu source, this is my first patch, so I'm open to suggestions! :-)

Summa summarum, which one is preferred? Think of the future or keep it at a
bare minimum?

Cheers,
bzt


>
> Regards,
> Andrew
>
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by BALATON Zoltan 6 years, 5 months ago
On Tue, 24 Oct 2017, bzt bzt wrote:
> On Mon, Oct 23, 2017 at 6:34 PM, Andrew Baumann <Andrew.Baumann@microsoft.com> wrote:
>>> +/* According to
>>> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2837/
>>> README.md
>>> + * The underlying architecture of the BCM2837 is identical to the
>> BCM2836.
>>> The only significant
>>> + * difference is the replacement of the ARMv7 quad core cluster with a
>>> quad-core ARM Cortex A53
>>> + * (ARMv8) cluster. So we use cortex-a53- here. */
>>
>> Are there any changes from bcm2836 other than the CPU model?
>
>
>> Duplicating the whole file just to have a different CPU seems like a bad
>> idea to me. I would suggest parameterising the CPU model in bcm2836 (and
>> maybe noting in header comments etc. that it can also model 2837), and then
>> instantiating it from raspi.c with the correct CPU type depending on which
>> machine is being used. That will also reduce the size of the patch
>> significantly, which will make it easier to get it reviewed.
>>
>
> For now, there's no more difference in the emulation than the CPU, but
> BCM2837 has a lot more to offer (40 bit address space, wifi, bluetooth
> etc.). So sooner or later it will need it's own source file to support all
> of that without getting the code messy. I have asked Peter whether it does
> worth writing future proof code, or keep the change at the bare minimum,
> but had no answer yet.
>
>
>>
>> (Alternatively, if there are minor but non-trivial differences between
>> 2836 and 2837 other than the CPU model, you may want to create something
>> like bcm2835_peripherals that contains all the functionality common to
>> both. But I suspect that isn't the case here.)
>>
>
> Well, bcm2837 is backward compatible with the bcm2836, but has a lot more
> registers and a different boot up process (different address and no atags).
> The common functionality is already in bcm2835_peripherals, and the MMIO
> address change from bcm2835 to bcm2836 has already been added by you.
> What's different is a lot more devices, which imho would be unwise to add
> to bcm2835 or either to bcm2836. So to be future proof I've created a
> separated file, but you are right at the current level of emulation it's
> not necessary.
[...]
>> Similarly, this looks like a clone of raspi2_init. I think it would be
>> better to have one function, parametrised if needed.
>>
>
> For now, yes. But the extra devices bcm2837 have will need extra
> initialization, and I though it would be clearer to separate in advance.
> Again, I've asked Peter about this, but had no response. As I see having a
> parametrised single function is simplier for now, but could lead to a messy
> code later when all the SoC features will be added. But I'm ain't no expert
> on qemu source, this is my first patch, so I'm open to suggestions! :-)
>
> Summa summarum, which one is preferred? Think of the future or keep it at a
> bare minimum?

I don't feel I have a deciding word in this but to share my opinion I 
think it's better to keep it in one file avoiding too much code 
duplication as long as those features that would make the implementations 
different aren't added now. Then when those features are added the files 
could be split or the best way can be decided at that point based on how 
those features will be implemented so no need to think that far ahead now. 
This would make patches easier to review now and maintaining the common 
code easier until they are different enough so a split needs to be done.

Regrads,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 5 months ago
On Tue, Oct 24, 2017 at 1:05 PM, BALATON Zoltan <balaton@eik.bme.hu> wrote:
[...]

> Summa summarum, which one is preferred? Think of the future or keep it at a
>> bare minimum?
>>
>
> I don't feel I have a deciding word in this but to share my opinion I
> think it's better to keep it in one file avoiding too much code duplication
> as long as those features that would make the implementations different
> aren't added now. Then when those features are added the files could be
> split or the best way can be decided at that point based on how those
> features will be implemented so no need to think that far ahead now. This
> would make patches easier to review now and maintaining the common code
> easier until they are different enough so a split needs to be done.
>
> Regrads,
> BALATON Zoltan
>

Okay, in this case here's a minimalistic version without the BCM2837 class
(just a flag added to BCM2836State). Feel free to choose which one to
apply. :-)

Cheers,
Zoli (bzt)

From a4d5c240d3b872a7ff1068de83d2080cd00e4517 Mon Sep 17 00:00:00 2001
From: bzt <bztemail@gmail.com>
Date: Tue, 24 Oct 2017 18:07:28 +0200
Subject: [PATCH] machine raspi3

Signed-off-by: bzt <bztemail@gmail.com>
---
 hw/arm/bcm2836.c         |  2 +-
 hw/arm/raspi.c           | 54
++++++++++++++++++++++++++++++++++++++----------
 include/hw/arm/bcm2836.h |  1 +
 3 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 8c43291..768f3c2 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -30,7 +30,7 @@ static void bcm2836_init(Object *obj)

     for (n = 0; n < BCM2836_NCPUS; n++) {
         object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
-                          "cortex-a15-" TYPE_ARM_CPU);
+            s->version == 2 ? "cortex-a15-" TYPE_ARM_CPU : "cortex-a53-"
TYPE_ARM_CPU);
         object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
                                   &error_abort);
     }
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 5941c9f..a74d57d 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -5,6 +5,8 @@
  * Rasperry Pi 2 emulation Copyright (c) 2015, Microsoft
  * Written by Andrew Baumann
  *
+ * Raspberry Pi 3 emulation 2017 by bzt
+ *
  * This code is licensed under the GNU GPLv2 and later.
  */

@@ -22,10 +24,13 @@
 #define SMPBOOT_ADDR    0x300 /* this should leave enough space for ATAGS
*/
 #define MVBAR_ADDR      0x400 /* secure vectors */
 #define BOARDSETUP_ADDR (MVBAR_ADDR + 0x20) /* board setup code */
-#define FIRMWARE_ADDR   0x8000 /* Pi loads kernel.img here by default */
+#define FIRMWARE_ADDR_2    0x8000 /* Pi 2 loads kernel.img here by default
*/
+#define FIRMWARE_ADDR_3   0x80000 /* Pi 3 loads kernel8.img here by
default */

 /* Table of Linux board IDs for different Pi versions */
-static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43};
+static const int raspi_boardid[]  = {[1] = 0xc42, [2] = 0xc43, [3] =
0xc44};
+/* Board revision IDs */
+static const int raspi_boardrev[] = {[1] = 0, [2] = 0xa21041, [3] =
0xa02082};

 typedef struct RasPiState {
     BCM2836State soc;
@@ -73,6 +78,7 @@ static void reset_secondary(ARMCPU *cpu, const struct
arm_boot_info *info)
 static void setup_boot(MachineState *machine, int version, size_t ram_size)
 {
     static struct arm_boot_info binfo;
+    hwaddr entry = 0;
     int r;

     binfo.board_id = raspi_boardid[version];
@@ -83,11 +89,12 @@ static void setup_boot(MachineState *machine, int
version, size_t ram_size)
     binfo.secure_board_setup = true;
     binfo.secure_boot = true;

-    /* Pi2 requires SMP setup */
-    if (version == 2) {
+    /* Pi2 and Pi3 requires SMP setup */
+    if (version == 2 || version == 3) {
         binfo.smp_loader_start = SMPBOOT_ADDR;
         binfo.write_secondary_boot = write_smpboot;
         binfo.secondary_cpu_reset_hook = reset_secondary;
+        entry = version == 2 ? FIRMWARE_ADDR_2 : FIRMWARE_ADDR_3;
     }

     /* If the user specified a "firmware" image (e.g. UEFI), we bypass
@@ -95,14 +102,14 @@ static void setup_boot(MachineState *machine, int
version, size_t ram_size)
      */
     if (machine->firmware) {
         /* load the firmware image (typically kernel.img) */
-        r = load_image_targphys(machine->firmware, FIRMWARE_ADDR,
-                                ram_size - FIRMWARE_ADDR);
+        r = load_image_targphys(machine->firmware, entry,
+                                ram_size - entry);
         if (r < 0) {
             error_report("Failed to load firmware from %s",
machine->firmware);
             exit(1);
         }

-        binfo.entry = FIRMWARE_ADDR;
+        binfo.entry = entry;
         binfo.firmware_loaded = true;
     } else {
         binfo.kernel_filename = machine->kernel_filename;
@@ -113,7 +120,7 @@ static void setup_boot(MachineState *machine, int
version, size_t ram_size)
     arm_load_kernel(ARM_CPU(first_cpu), &binfo);
 }

-static void raspi2_init(MachineState *machine)
+static void raspi_init(MachineState *machine, int version)
 {
     RasPiState *s = g_new0(RasPiState, 1);
     uint32_t vcram_size;
@@ -122,6 +129,7 @@ static void raspi2_init(MachineState *machine)
     BusState *bus;
     DeviceState *carddev;

+    s->soc.version = version;
     object_initialize(&s->soc, sizeof(s->soc), TYPE_BCM2836);
     object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
                               &error_abort);
@@ -137,8 +145,8 @@ static void raspi2_init(MachineState *machine)
                                    &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
                             &error_abort);
-    object_property_set_int(OBJECT(&s->soc), 0xa21041, "board-rev",
-                            &error_abort);
+    object_property_set_int(OBJECT(&s->soc), raspi_boardrev[version],
+                            "board-rev", &error_abort);
     object_property_set_bool(OBJECT(&s->soc), true, "realized",
&error_abort);

     /* Create and plug in the SD cards */
@@ -155,7 +163,12 @@ static void raspi2_init(MachineState *machine)

     vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
                                           &error_abort);
-    setup_boot(machine, 2, machine->ram_size - vcram_size);
+    setup_boot(machine, version, machine->ram_size - vcram_size);
+}
+
+static void raspi2_init(MachineState *machine)
+{
+    raspi_init(machine, 2);
 }

 static void raspi2_machine_init(MachineClass *mc)
@@ -171,3 +184,22 @@ static void raspi2_machine_init(MachineClass *mc)
     mc->ignore_memory_transaction_failures = true;
 };
 DEFINE_MACHINE("raspi2", raspi2_machine_init)
+
+static void raspi3_init(MachineState *machine)
+{
+    raspi_init(machine, 3);
+}
+
+static void raspi3_machine_init(MachineClass *mc)
+{
+    mc->desc = "Raspberry Pi 3";
+    mc->init = raspi3_init;
+    mc->block_default_type = IF_SD;
+    mc->no_parallel = 1;
+    mc->no_floppy = 1;
+    mc->no_cdrom = 1;
+    mc->max_cpus = BCM2836_NCPUS;
+    mc->default_ram_size = 1024 * 1024 * 1024;
+    mc->ignore_memory_transaction_failures = true;
+};
+DEFINE_MACHINE("raspi3", raspi3_machine_init)
diff --git a/include/hw/arm/bcm2836.h b/include/hw/arm/bcm2836.h
index 76de199..3c538f6 100644
--- a/include/hw/arm/bcm2836.h
+++ b/include/hw/arm/bcm2836.h
@@ -26,6 +26,7 @@ typedef struct BCM2836State {
     /*< public >*/

     uint32_t enabled_cpus;
+    uint version;

     ARMCPU cpus[BCM2836_NCPUS];
     BCM2836ControlState control;
-- 
2.14.2
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by Andrew Baumann via Qemu-devel 6 years, 5 months ago
> From: bzt bzt [mailto:bztemail@gmail.com]
> Sent: Tuesday, 24 October 2017 02:54
> On Mon, Oct 23, 2017 at 6:34 PM, Andrew Baumann wrote:
> 	Are there any changes from bcm2836 other than the CPU model?
> 
> 
> 	Duplicating the whole file just to have a different CPU seems like a bad
> idea to me. I would suggest parameterising the CPU model in bcm2836 (and
> maybe noting in header comments etc. that it can also model 2837), and then
> instantiating it from raspi.c with the correct CPU type depending on which
> machine is being used. That will also reduce the size of the patch significantly,
> which will make it easier to get it reviewed.
> 
> 
> 
> For now, there's no more difference in the emulation than the CPU, but
> BCM2837 has a lot more to offer (40 bit address space, wifi, bluetooth etc.). So
> sooner or later it will need it's own source file to support all of that without
> getting the code messy. I have asked Peter whether it does worth writing
> future proof code, or keep the change at the bare minimum, but had no answer
> yet.

I see. The address space size sounds like it would affect the SoC (although is there really 40 bits of usable physical address space beyond the core?). If it's like pi2, however, the wifi and BT are both behind USB, so that would be handled more naturally in the board model (raspi.c) than the SoC.

> Well, bcm2837 is backward compatible with the bcm2836, but has a lot more
> registers and a different boot up process (different address and no atags). The
> common functionality is already in bcm2835_peripherals, and the MMIO
> address change from bcm2835 to bcm2836 has already been added by you.
> What's different is a lot more devices, which imho would be unwise to add to
> bcm2835 or either to bcm2836. So to be future proof I've created a separated
> file, but you are right at the current level of emulation it's not necessary.

Right, but as I wrote above if those devices are behind USB that's not part of the SoC model, and belongs in the board logic. If "more registers" just refers to the CPU, then that's irrelevant. If there are really more devices / device registers on the system bus, then that calls for a deeper change in the SoC model and perhaps a new implementation.

> 	Similarly, this looks like a clone of raspi2_init. I think it would be better
> to have one function, parametrised if needed.
> 
> 
> 
> For now, yes. But the extra devices bcm2837 have will need extra initialization,
> and I though it would be clearer to separate in advance. Again, I've asked Peter
> about this, but had no response. As I see having a parametrised single function
> is simplier for now, but could lead to a messy code later when all the SoC
> features will be added. But I'm ain't no expert on qemu source, this is my first
> patch, so I'm open to suggestions! :-)

I'm more open to the need for evolution in the machine init logic (raspi.c), so splitting there makes more sense to me than in bcm2836/7.

I see you just posted another patch. FWIW, this isn't quite what I was proposing -- rather than have bcm2736.c take a version number that is 2 or 3, I would just pass it the CPU model to instantiate. But at this point I think it's best waiting for one of the Qemu maintainers to chime in and see what they prefer.

Andrew
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 5 months ago
Hi Andrew!

On Tue, Oct 24, 2017 at 6:44 PM, Andrew Baumann <
Andrew.Baumann@microsoft.com> wrote:
[...]

> I see. The address space size sounds like it would affect the SoC
> (although is there really 40 bits of usable physical address space beyond
> the core?). If it's like pi2, however, the wifi and BT are both behind USB,
> so that would be handled more naturally in the board model (raspi.c) than
> the SoC.
>
[...]

>
> Right, but as I wrote above if those devices are behind USB that's not
> part of the SoC model, and belongs in the board logic. If "more registers"
> just refers to the CPU, then that's irrelevant. If there are really more
> devices / device registers on the system bus, then that calls for a deeper
> change in the SoC model and perhaps a new implementation.
>

True, but even if it's behind USB, somehow the USB system should know which
one to emulate. Originally for clear code I was thinking along the line of
if( TYPE(soc) == TYPE_BCM2836 ) {
...
}
if( TYPE(soc) == TYPE_BCM2837 ) {
...
}
when the two are not separated and shares common code. But we are not there
yet, let's focus on one step at a time :-)

[...]

> I'm more open to the need for evolution in the machine init logic
> (raspi.c), so splitting there makes more sense to me than in bcm2836/7.
>
> I see you just posted another patch. FWIW, this isn't quite what I was
> proposing -- rather than have bcm2736.c take a version number that is 2 or
> 3, I would just pass it the CPU model to instantiate. But at this point I
> think it's best waiting for one of the Qemu maintainers to chime in and see
> what they prefer.
>

Again you are right. But I saw Alistair's patch
https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg04153.html When it
makes through, the bcm2836.c could simply use mc->default_cpu_type and
would be no need for anything else in BCM2836State, neither a version nor a
CPU model.

Okay, for now let's see what the maintainers prefer!

Cheers,
bzt


> Andrew
>
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 4 months ago
Dear Andrew,

A month passed, and the maintainers didn't gave a damn about raspi3 support
in qemu. Any ideas?

On Wed, Oct 25, 2017 at 10:52 AM, bzt bzt <bztemail@gmail.com> wrote:

> Hi Andrew!
>
> On Tue, Oct 24, 2017 at 6:44 PM, Andrew Baumann <
> Andrew.Baumann@microsoft.com> wrote:
> [...]
>
>> I see. The address space size sounds like it would affect the SoC
>> (although is there really 40 bits of usable physical address space beyond
>> the core?). If it's like pi2, however, the wifi and BT are both behind USB,
>> so that would be handled more naturally in the board model (raspi.c) than
>> the SoC.
>>
> [...]
>
>>
>> Right, but as I wrote above if those devices are behind USB that's not
>> part of the SoC model, and belongs in the board logic. If "more registers"
>> just refers to the CPU, then that's irrelevant. If there are really more
>> devices / device registers on the system bus, then that calls for a deeper
>> change in the SoC model and perhaps a new implementation.
>>
>
> True, but even if it's behind USB, somehow the USB system should know
> which one to emulate. Originally for clear code I was thinking along the
> line of
> if( TYPE(soc) == TYPE_BCM2836 ) {
> ...
> }
> if( TYPE(soc) == TYPE_BCM2837 ) {
> ...
> }
> when the two are not separated and shares common code. But we are not
> there yet, let's focus on one step at a time :-)
>
> [...]
>
>> I'm more open to the need for evolution in the machine init logic
>> (raspi.c), so splitting there makes more sense to me than in bcm2836/7.
>>
>> I see you just posted another patch. FWIW, this isn't quite what I was
>> proposing -- rather than have bcm2736.c take a version number that is 2 or
>> 3, I would just pass it the CPU model to instantiate. But at this point I
>> think it's best waiting for one of the Qemu maintainers to chime in and see
>> what they prefer.
>>
>
> Again you are right. But I saw Alistair's patch
> https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg04153.html When
> it makes through, the bcm2836.c could simply use mc->default_cpu_type and
> would be no need for anything else in BCM2836State, neither a version nor a
> CPU model.
>
> Okay, for now let's see what the maintainers prefer!
>
> Cheers,
> bzt
>
>
>> Andrew
>>
>
>
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by Peter Maydell 6 years, 4 months ago
On 25 November 2017 at 16:43, bzt bzt <bztemail@gmail.com> wrote:
> Dear Andrew,
>
> A month passed, and the maintainers didn't gave a damn about raspi3 support
> in qemu. Any ideas?

Sorry this patch fell through the cracks, but it looked to me
to be in the state "patch got code review comments, waiting for
submitter to resend a new version" (from about the point Konrad
said "split this up, it's too big", which I agree with.)
QEMU gets a lot of patches every day, so our code process relies
on the submitter to prod things if what they're interested in
seems to have been accidentally ignored:
https://wiki.qemu.org/Contribute/SubmitAPatch#If_your_patch_seems_to_have_been_ignored

FWIW, as far as I'm concerned Andrew pretty much is the maintainer
as far as the raspi boards are concerned, so as far as those
bits of code goes I'm happy to take his opinion on it.

PS: if you send a new patch, send it as a new top level
thread -- if you send it as a reply/followup to the
first patch it's liable to not be noticed.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by Andrew Baumann via Qemu-devel 6 years, 4 months ago
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Saturday, 25 November 2017 10:05

> On 25 November 2017 at 16:43, bzt bzt <bztemail@gmail.com> wrote:
> > Dear Andrew,
> >
> > A month passed, and the maintainers didn't gave a damn about raspi3
> support
> > in qemu. Any ideas?
> 
> Sorry this patch fell through the cracks, but it looked to me
> to be in the state "patch got code review comments, waiting for
> submitter to resend a new version" (from about the point Konrad
> said "split this up, it's too big", which I agree with.)
> QEMU gets a lot of patches every day, so our code process relies
> on the submitter to prod things if what they're interested in
> seems to have been accidentally ignored:
> https://wiki.qemu.org/Contribute/SubmitAPatch#If_your_patch_seems_to_hav
> e_been_ignored
> 
> FWIW, as far as I'm concerned Andrew pretty much is the maintainer
> as far as the raspi boards are concerned, so as far as those
> bits of code goes I'm happy to take his opinion on it.

In that case, IIRC my high-level suggestion was to either parameterise bcm2836 to take a CPU model string, or else move the CPU creation out of bcm2836.c into the board file. From what I've understood thus far about pi3, it does not seem necessary to have a separate bcm2837 implementation. I suspect at that point the patch would be small enough that it didn't require further splitting.

> PS: if you send a new patch, send it as a new top level
> thread -- if you send it as a reply/followup to the
> first patch it's liable to not be noticed.

Please also CC me on the new patch as I am not very reliable at monitoring qemu-devel.

Thanks!
Andrew
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 4 months ago
Hi Andrew!

[...]

>
> In that case, IIRC my high-level suggestion was to either parameterise
> bcm2836 to take a CPU model string, or else move the CPU creation out of
> bcm2836.c into the board file. From what I've understood thus far about
> pi3, it does not seem necessary to have a separate bcm2837 implementation.
> I suspect at that point the patch would be small enough that it didn't
> require further splitting.
>

Yes, I agree. I've provided a parameterised version on Oct 24, which does
not have a separate bcm2837 implementation. Is that patch ok? Or should I
wait for Alistair's patch (
https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg04153.html)?
I would prefer the latter, if I may, and I'm willing to rewrite raspi3
support using the CPU model string in mc->default_cpu_type introduced by
that patch. (Although I have a question. I'm not sure what's the preferred
way to get MachineClass* object in bcm2836. Use a MachineState* cast on
it's Object* argument with MACHINE_GET_CLASS() or should I use the
parameterless qdev_get_machine() instead?)


> > PS: if you send a new patch, send it as a new top level
> > thread -- if you send it as a reply/followup to the
> > first patch it's liable to not be noticed.
>
> Please also CC me on the new patch as I am not very reliable at monitoring
> qemu-devel.
>

Noted :-) Thank you for your help! I'm sure qemu users appreciate it very
much!

bzt


>
> Thanks!
> Andrew
>
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by Peter Maydell 6 years, 4 months ago
On 28 November 2017 at 11:26, bzt bzt <bztemail@gmail.com> wrote:
> (Although I have a question. I'm not sure what's the preferred
> way to get MachineClass* object in bcm2836. Use a MachineState* cast on it's
> Object* argument with MACHINE_GET_CLASS() or should I use the parameterless
> qdev_get_machine() instead?)

bcm2836.c should be a self contained object, which its caller creates
and configures. The MachineClass is the board model object, and the
SoC object shouldn't have to access it. If there's something (like the
CPU model string) that the SoC needs, the SoC object should have a
property which the board model sets.

hw/arm/stm32f205_soc.c has an example of this (as do some of the
other SoC objects -- 'git grep cpu-type hw/arm' should bring them up).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 4 months ago
On Tue, Nov 28, 2017 at 12:56 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 28 November 2017 at 11:26, bzt bzt <bztemail@gmail.com> wrote:
> > (Although I have a question. I'm not sure what's the preferred
> > way to get MachineClass* object in bcm2836. Use a MachineState* cast on
> it's
> > Object* argument with MACHINE_GET_CLASS() or should I use the
> parameterless
> > qdev_get_machine() instead?)
>
> bcm2836.c should be a self contained object, which its caller creates
> and configures. The MachineClass is the board model object, and the
> SoC object shouldn't have to access it. If there's something (like the
> CPU model string) that the SoC needs, the SoC object should have a
> property which the board model sets.
>
> hw/arm/stm32f205_soc.c has an example of this (as do some of the
> other SoC objects -- 'git grep cpu-type hw/arm' should bring them up).
>

Okay, thanks a lot! I'll check them!


>
> thanks
> -- PMM
>
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by Andrew Baumann via Qemu-devel 6 years, 4 months ago
> From: bzt bzt [mailto:bztemail@gmail.com]
> Sent: Tuesday, 28 November 2017 03:27

> Yes, I agree. I've provided a parameterised version on Oct 24, which does not
> have a separate bcm2837 implementation. Is that patch ok?

That patch was moving in the right direction, but I think there were two problems with it. First, the right way to pass a parameter is via a property field (as Peter explained). Second, IMO the parameter should be the CPU model string to instantiate and not the Pi version number.

> Or should I wait
> for Alistair's patch (https://lists.gnu.org/archive/html/qemu-devel/2017-
> 10/msg04153.html)?

Alistair's patch doesn't actually help you as far as I can see, since it doesn't change what CPU bcm2836 instantiates. I think it just means that if the user specifies a different CPU type they get an error rather than being silently ignored.

Andrew
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 4 months ago
On Tue, Nov 28, 2017 at 6:54 PM, Andrew Baumann <
Andrew.Baumann@microsoft.com> wrote:

> > From: bzt bzt [mailto:bztemail@gmail.com]
> > Sent: Tuesday, 28 November 2017 03:27
>
> > Yes, I agree. I've provided a parameterised version on Oct 24, which
> does not
> > have a separate bcm2837 implementation. Is that patch ok?
>
> That patch was moving in the right direction, but I think there were two
> problems with it. First, the right way to pass a parameter is via a
> property field (as Peter explained). Second, IMO the parameter should be
> the CPU model string to instantiate and not the Pi version number.
>

Thanks!


>
> > Or should I wait
> > for Alistair's patch (https://lists.gnu.org/
> archive/html/qemu-devel/2017-
> > 10/msg04153.html)?
>
> Alistair's patch doesn't actually help you as far as I can see, since it
> doesn't change what CPU bcm2836 instantiates. I think it just means that if
> the user specifies a different CPU type they get an error rather than being
> silently ignored.
>

It does add the CPU model string, which can and should be used in bcm2836
to decide which CPU to instatiate (regardless the method of passing that
parameter). If I add yet another CPU model string, the two patch will be in
conflict containing redundant information. Also I'm sure would raspi3 use
different cpus, we should always instantiate the one the user specified.
Although here is only one possible CPU model, still that seems to be the
correct behavior. Don't get me wrong, I don't criticize, I just want to
find the best solution. Setting the CPU model only once for each version
seems right. I was thinking: if there's going to be a user specified CPU
model string anyway, why not use that one?

bzt


>
> Andrew
>
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 2 months ago
Dear All,

Since you still haven't merged Alistair's patch, I decided to include it in
my own.
I've shrinked the number of modified files to two, that's the bare minimum
to support "-M raspi3" switch. Bcm2836.c modified minimally, the rest is in
raspi.c. I've renamed raspi2_init() to raspi_init() 'cos now it initializes
raspi3 as well, no additional function.

diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 8c43291112..5119e00051 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -15,6 +15,7 @@
 #include "hw/arm/bcm2836.h"
 #include "hw/arm/raspi_platform.h"
 #include "hw/sysbus.h"
+#include "hw/boards.h"
 #include "exec/address-spaces.h"

 /* Peripheral base address seen by the CPU */
@@ -30,7 +31,7 @@ static void bcm2836_init(Object *obj)

     for (n = 0; n < BCM2836_NCPUS; n++) {
         object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
-                          "cortex-a15-" TYPE_ARM_CPU);
+                          current_machine->cpu_type);
         object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
                                   &error_abort);
     }
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index cd5fa8c3dc..24a9243440 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -5,6 +5,8 @@
  * Rasperry Pi 2 emulation Copyright (c) 2015, Microsoft
  * Written by Andrew Baumann
  *
+ * Raspberry Pi 3 emulation Copyright (c) 2018 by bzt
+ *
  * This code is licensed under the GNU GPLv2 and later.
  */

@@ -22,10 +24,11 @@
 #define SMPBOOT_ADDR    0x300 /* this should leave enough space for ATAGS
*/
 #define MVBAR_ADDR      0x400 /* secure vectors */
 #define BOARDSETUP_ADDR (MVBAR_ADDR + 0x20) /* board setup code */
-#define FIRMWARE_ADDR   0x8000 /* Pi loads kernel.img here by default */
+#define FIRMWARE_ADDR_2    0x8000 /* Pi 2 loads kernel.img here by default
*/
+#define FIRMWARE_ADDR_3   0x80000 /* Pi 3 loads kernel8.img here by
default */

 /* Table of Linux board IDs for different Pi versions */
-static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43};
+static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};

 typedef struct RasPiState {
     BCM2836State soc;
@@ -83,8 +86,8 @@ static void setup_boot(MachineState *machine, int
version, size_t ram_size)
     binfo.secure_board_setup = true;
     binfo.secure_boot = true;

-    /* Pi2 requires SMP setup */
-    if (version == 2) {
+    /* Pi2 and Pi3 requires SMP setup */
+    if (version >= 2) {
         binfo.smp_loader_start = SMPBOOT_ADDR;
         binfo.write_secondary_boot = write_smpboot;
         binfo.secondary_cpu_reset_hook = reset_secondary;
@@ -94,15 +97,15 @@ static void setup_boot(MachineState *machine, int
version, size_t ram_size)
      * the normal Linux boot process
      */
     if (machine->firmware) {
+        binfo.entry = version==3? FIRMWARE_ADDR_3 : FIRMWARE_ADDR_2;
         /* load the firmware image (typically kernel.img) */
-        r = load_image_targphys(machine->firmware, FIRMWARE_ADDR,
-                                ram_size - FIRMWARE_ADDR);
+        r = load_image_targphys(machine->firmware, binfo.entry,
+                                ram_size - binfo.entry);
         if (r < 0) {
             error_report("Failed to load firmware from %s",
machine->firmware);
             exit(1);
         }

-        binfo.entry = FIRMWARE_ADDR;
         binfo.firmware_loaded = true;
     } else {
         binfo.kernel_filename = machine->kernel_filename;
@@ -113,7 +116,7 @@ static void setup_boot(MachineState *machine, int
version, size_t ram_size)
     arm_load_kernel(ARM_CPU(first_cpu), &binfo);
 }

-static void raspi2_init(MachineState *machine)
+static void raspi_init(MachineState *machine)
 {
     RasPiState *s = g_new0(RasPiState, 1);
     uint32_t vcram_size;
@@ -121,6 +124,7 @@ static void raspi2_init(MachineState *machine)
     BlockBackend *blk;
     BusState *bus;
     DeviceState *carddev;
+    int version = machine->cpu_type==ARM_CPU_TYPE_NAME("cortex-a15")? 2 :
3;

     object_initialize(&s->soc, sizeof(s->soc), TYPE_BCM2836);
     object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
@@ -137,8 +141,8 @@ static void raspi2_init(MachineState *machine)
                                    &error_abort);
     object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
                             &error_abort);
-    object_property_set_int(OBJECT(&s->soc), 0xa21041, "board-rev",
-                            &error_abort);
+    object_property_set_int(OBJECT(&s->soc), version==3 ? 0xa02082 :
0xa21041,
+                            "board-rev", &error_abort);
     object_property_set_bool(OBJECT(&s->soc), true, "realized",
&error_abort);

     /* Create and plug in the SD cards */
@@ -155,21 +159,39 @@ static void raspi2_init(MachineState *machine)

     vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
                                           &error_abort);
-    setup_boot(machine, 2, machine->ram_size - vcram_size);
+    setup_boot(machine, version, machine->ram_size - vcram_size);
 }

 static void raspi2_machine_init(MachineClass *mc)
 {
     mc->desc = "Raspberry Pi 2";
-    mc->init = raspi2_init;
+    mc->init = raspi_init;
     mc->block_default_type = IF_SD;
     mc->no_parallel = 1;
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->max_cpus = BCM2836_NCPUS;
     mc->min_cpus = BCM2836_NCPUS;
     mc->default_cpus = BCM2836_NCPUS;
+    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
     mc->default_ram_size = 1024 * 1024 * 1024;
     mc->ignore_memory_transaction_failures = true;
 };
 DEFINE_MACHINE("raspi2", raspi2_machine_init)
+
+static void raspi3_machine_init(MachineClass *mc)
+{
+    mc->desc = "Raspberry Pi 3";
+    mc->init = raspi_init;
+    mc->block_default_type = IF_SD;
+    mc->no_parallel = 1;
+    mc->no_floppy = 1;
+    mc->no_cdrom = 1;
+    mc->max_cpus = BCM2836_NCPUS;
+    mc->min_cpus = BCM2836_NCPUS;
+    mc->default_cpus = BCM2836_NCPUS;
+    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
+    mc->default_ram_size = 1024 * 1024 * 1024;
+    mc->ignore_memory_transaction_failures = true;
+};
+DEFINE_MACHINE("raspi3", raspi3_machine_init)

Hope it's simple enough to be reviewed easily, and it uses CPU type strings
as requested.

Cheers,
bzt


On Wed, Nov 29, 2017 at 8:52 AM, bzt bzt <bztemail@gmail.com> wrote:

>
>
> On Tue, Nov 28, 2017 at 6:54 PM, Andrew Baumann <
> Andrew.Baumann@microsoft.com> wrote:
>
>> > From: bzt bzt [mailto:bztemail@gmail.com]
>> > Sent: Tuesday, 28 November 2017 03:27
>>
>> > Yes, I agree. I've provided a parameterised version on Oct 24, which
>> does not
>> > have a separate bcm2837 implementation. Is that patch ok?
>>
>> That patch was moving in the right direction, but I think there were two
>> problems with it. First, the right way to pass a parameter is via a
>> property field (as Peter explained). Second, IMO the parameter should be
>> the CPU model string to instantiate and not the Pi version number.
>>
>
> Thanks!
>
>
>>
>> > Or should I wait
>> > for Alistair's patch (https://lists.gnu.org/archive
>> /html/qemu-devel/2017-
>> > 10/msg04153.html)?
>>
>> Alistair's patch doesn't actually help you as far as I can see, since it
>> doesn't change what CPU bcm2836 instantiates. I think it just means that if
>> the user specifies a different CPU type they get an error rather than being
>> silently ignored.
>>
>
> It does add the CPU model string, which can and should be used in bcm2836
> to decide which CPU to instatiate (regardless the method of passing that
> parameter). If I add yet another CPU model string, the two patch will be in
> conflict containing redundant information. Also I'm sure would raspi3 use
> different cpus, we should always instantiate the one the user specified.
> Although here is only one possible CPU model, still that seems to be the
> correct behavior. Don't get me wrong, I don't criticize, I just want to
> find the best solution. Setting the CPU model only once for each version
> seems right. I was thinking: if there's going to be a user specified CPU
> model string anyway, why not use that one?
>
> bzt
>
>
>>
>> Andrew
>>
>
>
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by BALATON Zoltan 6 years, 2 months ago
Hello,

On Thu, 18 Jan 2018, bzt bzt wrote:
> Since you still haven't merged Alistair's patch, I decided to include it in
> my own.

Which patch exactly are you referring to? Please be patient and don't get 
upset. Merging a patch can take some time, especially if code freeze and 
holiday season are in the middle and some backlog is accumulated. I 
suggest to follow instructions described here:

https://wiki.qemu.org/Contribute/SubmitAPatch

and send this version as a new message with proper format and commit 
message so it can be easily found and tested by relevant people and don't 
give up if they are busy and can't reply immediately. Just politely remind 
them after a week or two as suggested on the above wiki page.

Rationale for the above: Replying to an older thread may not show up at 
the end if someone uses threaded view but gets burried in some old thread 
so new patches should be new top level threads, only replies to the same 
patch or series should be sent as replies. Also your patch has been 
mangled by the email client and some lines are broken. This is a problem 
because it cannot be tested by using git am so it's more difficult to 
check. Please make sure you send it unflowed so the patch is not changed 
by the client (or use git send-email).

All in all, I think your work is interesting and the lack of reply only 
means that people who could review and merge it were too busy not lack of 
interest so keep up improving the patch and don't give up.

Regards,
BALATON Zoltan

> I've shrinked the number of modified files to two, that's the bare minimum
> to support "-M raspi3" switch. Bcm2836.c modified minimally, the rest is in
> raspi.c. I've renamed raspi2_init() to raspi_init() 'cos now it initializes
> raspi3 as well, no additional function.
>
> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 8c43291112..5119e00051 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c
> @@ -15,6 +15,7 @@
> #include "hw/arm/bcm2836.h"
> #include "hw/arm/raspi_platform.h"
> #include "hw/sysbus.h"
> +#include "hw/boards.h"
> #include "exec/address-spaces.h"
>
> /* Peripheral base address seen by the CPU */
> @@ -30,7 +31,7 @@ static void bcm2836_init(Object *obj)
>
>     for (n = 0; n < BCM2836_NCPUS; n++) {
>         object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
> -                          "cortex-a15-" TYPE_ARM_CPU);
> +                          current_machine->cpu_type);
>         object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
>                                   &error_abort);
>     }
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index cd5fa8c3dc..24a9243440 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -5,6 +5,8 @@
>  * Rasperry Pi 2 emulation Copyright (c) 2015, Microsoft
>  * Written by Andrew Baumann
>  *
> + * Raspberry Pi 3 emulation Copyright (c) 2018 by bzt
> + *
>  * This code is licensed under the GNU GPLv2 and later.
>  */
>
> @@ -22,10 +24,11 @@
> #define SMPBOOT_ADDR    0x300 /* this should leave enough space for ATAGS
> */
> #define MVBAR_ADDR      0x400 /* secure vectors */
> #define BOARDSETUP_ADDR (MVBAR_ADDR + 0x20) /* board setup code */
> -#define FIRMWARE_ADDR   0x8000 /* Pi loads kernel.img here by default */
> +#define FIRMWARE_ADDR_2    0x8000 /* Pi 2 loads kernel.img here by default
> */
> +#define FIRMWARE_ADDR_3   0x80000 /* Pi 3 loads kernel8.img here by
> default */
>
> /* Table of Linux board IDs for different Pi versions */
> -static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43};
> +static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};
>
> typedef struct RasPiState {
>     BCM2836State soc;
> @@ -83,8 +86,8 @@ static void setup_boot(MachineState *machine, int
> version, size_t ram_size)
>     binfo.secure_board_setup = true;
>     binfo.secure_boot = true;
>
> -    /* Pi2 requires SMP setup */
> -    if (version == 2) {
> +    /* Pi2 and Pi3 requires SMP setup */
> +    if (version >= 2) {
>         binfo.smp_loader_start = SMPBOOT_ADDR;
>         binfo.write_secondary_boot = write_smpboot;
>         binfo.secondary_cpu_reset_hook = reset_secondary;
> @@ -94,15 +97,15 @@ static void setup_boot(MachineState *machine, int
> version, size_t ram_size)
>      * the normal Linux boot process
>      */
>     if (machine->firmware) {
> +        binfo.entry = version==3? FIRMWARE_ADDR_3 : FIRMWARE_ADDR_2;
>         /* load the firmware image (typically kernel.img) */
> -        r = load_image_targphys(machine->firmware, FIRMWARE_ADDR,
> -                                ram_size - FIRMWARE_ADDR);
> +        r = load_image_targphys(machine->firmware, binfo.entry,
> +                                ram_size - binfo.entry);
>         if (r < 0) {
>             error_report("Failed to load firmware from %s",
> machine->firmware);
>             exit(1);
>         }
>
> -        binfo.entry = FIRMWARE_ADDR;
>         binfo.firmware_loaded = true;
>     } else {
>         binfo.kernel_filename = machine->kernel_filename;
> @@ -113,7 +116,7 @@ static void setup_boot(MachineState *machine, int
> version, size_t ram_size)
>     arm_load_kernel(ARM_CPU(first_cpu), &binfo);
> }
>
> -static void raspi2_init(MachineState *machine)
> +static void raspi_init(MachineState *machine)
> {
>     RasPiState *s = g_new0(RasPiState, 1);
>     uint32_t vcram_size;
> @@ -121,6 +124,7 @@ static void raspi2_init(MachineState *machine)
>     BlockBackend *blk;
>     BusState *bus;
>     DeviceState *carddev;
> +    int version = machine->cpu_type==ARM_CPU_TYPE_NAME("cortex-a15")? 2 :
> 3;
>
>     object_initialize(&s->soc, sizeof(s->soc), TYPE_BCM2836);
>     object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> @@ -137,8 +141,8 @@ static void raspi2_init(MachineState *machine)
>                                    &error_abort);
>     object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
>                             &error_abort);
> -    object_property_set_int(OBJECT(&s->soc), 0xa21041, "board-rev",
> -                            &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), version==3 ? 0xa02082 :
> 0xa21041,
> +                            "board-rev", &error_abort);
>     object_property_set_bool(OBJECT(&s->soc), true, "realized",
> &error_abort);
>
>     /* Create and plug in the SD cards */
> @@ -155,21 +159,39 @@ static void raspi2_init(MachineState *machine)
>
>     vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
>                                           &error_abort);
> -    setup_boot(machine, 2, machine->ram_size - vcram_size);
> +    setup_boot(machine, version, machine->ram_size - vcram_size);
> }
>
> static void raspi2_machine_init(MachineClass *mc)
> {
>     mc->desc = "Raspberry Pi 2";
> -    mc->init = raspi2_init;
> +    mc->init = raspi_init;
>     mc->block_default_type = IF_SD;
>     mc->no_parallel = 1;
>     mc->no_floppy = 1;
>     mc->no_cdrom = 1;
>     mc->max_cpus = BCM2836_NCPUS;
>     mc->min_cpus = BCM2836_NCPUS;
>     mc->default_cpus = BCM2836_NCPUS;
> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>     mc->default_ram_size = 1024 * 1024 * 1024;
>     mc->ignore_memory_transaction_failures = true;
> };
> DEFINE_MACHINE("raspi2", raspi2_machine_init)
> +
> +static void raspi3_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "Raspberry Pi 3";
> +    mc->init = raspi_init;
> +    mc->block_default_type = IF_SD;
> +    mc->no_parallel = 1;
> +    mc->no_floppy = 1;
> +    mc->no_cdrom = 1;
> +    mc->max_cpus = BCM2836_NCPUS;
> +    mc->min_cpus = BCM2836_NCPUS;
> +    mc->default_cpus = BCM2836_NCPUS;
> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> +    mc->default_ram_size = 1024 * 1024 * 1024;
> +    mc->ignore_memory_transaction_failures = true;
> +};
> +DEFINE_MACHINE("raspi3", raspi3_machine_init)
>
> Hope it's simple enough to be reviewed easily, and it uses CPU type strings
> as requested.
>
> Cheers,
> bzt
>
>
> On Wed, Nov 29, 2017 at 8:52 AM, bzt bzt <bztemail@gmail.com> wrote:
>
>>
>>
>> On Tue, Nov 28, 2017 at 6:54 PM, Andrew Baumann <
>> Andrew.Baumann@microsoft.com> wrote:
>>
>>>> From: bzt bzt [mailto:bztemail@gmail.com]
>>>> Sent: Tuesday, 28 November 2017 03:27
>>>
>>>> Yes, I agree. I've provided a parameterised version on Oct 24, which
>>> does not
>>>> have a separate bcm2837 implementation. Is that patch ok?
>>>
>>> That patch was moving in the right direction, but I think there were two
>>> problems with it. First, the right way to pass a parameter is via a
>>> property field (as Peter explained). Second, IMO the parameter should be
>>> the CPU model string to instantiate and not the Pi version number.
>>>
>>
>> Thanks!
>>
>>
>>>
>>>> Or should I wait
>>>> for Alistair's patch (https://lists.gnu.org/archive
>>> /html/qemu-devel/2017-
>>>> 10/msg04153.html)?
>>>
>>> Alistair's patch doesn't actually help you as far as I can see, since it
>>> doesn't change what CPU bcm2836 instantiates. I think it just means that if
>>> the user specifies a different CPU type they get an error rather than being
>>> silently ignored.
>>>
>>
>> It does add the CPU model string, which can and should be used in bcm2836
>> to decide which CPU to instatiate (regardless the method of passing that
>> parameter). If I add yet another CPU model string, the two patch will be in
>> conflict containing redundant information. Also I'm sure would raspi3 use
>> different cpus, we should always instantiate the one the user specified.
>> Although here is only one possible CPU model, still that seems to be the
>> correct behavior. Don't get me wrong, I don't criticize, I just want to
>> find the best solution. Setting the CPU model only once for each version
>> seems right. I was thinking: if there's going to be a user specified CPU
>> model string anyway, why not use that one?
>>
>> bzt
>>
>>
>>>
>>> Andrew
>>>
>>
>>
>
>

Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 2 months ago
On Mon, Jan 22, 2018 at 12:41 PM, BALATON Zoltan <balaton@eik.bme.hu> wrote:

> Hello,
>
> On Thu, 18 Jan 2018, bzt bzt wrote:
>
>> Since you still haven't merged Alistair's patch, I decided to include it
>> in
>> my own.
>>
>
> Which patch exactly are you referring to?


The one I linked earlier, which also added CPU model to the mc class.


> Please be patient and don't get upset.


I'm not in-patent and I'm not upset. I can emulate raspi3, and I'm very
happy with it! Other users may be upset about you (plural) because they
can't use binary distributions and have to compile qemu from my source if
they want to have raspi3 support.


> Merging a patch can take some time, especially if code freeze and holiday
> season are in the middle and some backlog is accumulated. I suggest to
> follow instructions described here:
>
> https://wiki.qemu.org/Contribute/SubmitAPatch
>
> and send this version as a new message with proper format and commit
> message so it can be easily found and tested by relevant people and don't
> give up if they are busy and can't reply immediately. Just politely remind
> them after a week or two as suggested on the above wiki page.
>

There's a huge misunderstanding here. I have a working qemu for about half
a year now, and I don't need it merged. It's not my goal to submit a patch
to qemu in any way, I just did that because I had modified an Open Source
software and wanted to share it with the community. If you don't merge my
patch, I don't care. Other users will.


>
> Rationale for the above: Replying to an older thread may not show up at
> the end if someone uses threaded view but gets burried in some old thread
> so new patches should be new top level threads, only replies to the same
> patch or series should be sent as replies. Also your patch has been mangled
> by the email client and some lines are broken. This is a problem because it
> cannot be tested by using git am so it's more difficult to check. Please
> make sure you send it unflowed so the patch is not changed by the client
> (or use git send-email).
>

Again, if you want to provide raspi3 support (as more and more users
require it), use my patch or my git repo. I don't have to or want to do
anything more about it.


> All in all, I think your work is interesting and the lack of reply only
> means that people who could review and merge it were too busy not lack of
> interest so keep up improving the patch and don't give up.
>

I won't improve my patch any further, 'cos it's working perfectly for me.
Don't try to fix what's not broken!

Regards,
bzt


> Regards,
> BALATON Zoltan
>
>
> I've shrinked the number of modified files to two, that's the bare minimum
>> to support "-M raspi3" switch. Bcm2836.c modified minimally, the rest is
>> in
>> raspi.c. I've renamed raspi2_init() to raspi_init() 'cos now it
>> initializes
>> raspi3 as well, no additional function.
>>
>> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
>> index 8c43291112..5119e00051 100644
>> --- a/hw/arm/bcm2836.c
>> +++ b/hw/arm/bcm2836.c
>> @@ -15,6 +15,7 @@
>> #include "hw/arm/bcm2836.h"
>> #include "hw/arm/raspi_platform.h"
>> #include "hw/sysbus.h"
>> +#include "hw/boards.h"
>> #include "exec/address-spaces.h"
>>
>> /* Peripheral base address seen by the CPU */
>> @@ -30,7 +31,7 @@ static void bcm2836_init(Object *obj)
>>
>>     for (n = 0; n < BCM2836_NCPUS; n++) {
>>         object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
>> -                          "cortex-a15-" TYPE_ARM_CPU);
>> +                          current_machine->cpu_type);
>>         object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
>>                                   &error_abort);
>>     }
>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>> index cd5fa8c3dc..24a9243440 100644
>> --- a/hw/arm/raspi.c
>> +++ b/hw/arm/raspi.c
>> @@ -5,6 +5,8 @@
>>  * Rasperry Pi 2 emulation Copyright (c) 2015, Microsoft
>>  * Written by Andrew Baumann
>>  *
>> + * Raspberry Pi 3 emulation Copyright (c) 2018 by bzt
>> + *
>>  * This code is licensed under the GNU GPLv2 and later.
>>  */
>>
>> @@ -22,10 +24,11 @@
>> #define SMPBOOT_ADDR    0x300 /* this should leave enough space for ATAGS
>> */
>> #define MVBAR_ADDR      0x400 /* secure vectors */
>> #define BOARDSETUP_ADDR (MVBAR_ADDR + 0x20) /* board setup code */
>> -#define FIRMWARE_ADDR   0x8000 /* Pi loads kernel.img here by default */
>> +#define FIRMWARE_ADDR_2    0x8000 /* Pi 2 loads kernel.img here by
>> default
>> */
>> +#define FIRMWARE_ADDR_3   0x80000 /* Pi 3 loads kernel8.img here by
>> default */
>>
>> /* Table of Linux board IDs for different Pi versions */
>> -static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43};
>> +static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] =
>> 0xc44};
>>
>> typedef struct RasPiState {
>>     BCM2836State soc;
>> @@ -83,8 +86,8 @@ static void setup_boot(MachineState *machine, int
>> version, size_t ram_size)
>>     binfo.secure_board_setup = true;
>>     binfo.secure_boot = true;
>>
>> -    /* Pi2 requires SMP setup */
>> -    if (version == 2) {
>> +    /* Pi2 and Pi3 requires SMP setup */
>> +    if (version >= 2) {
>>         binfo.smp_loader_start = SMPBOOT_ADDR;
>>         binfo.write_secondary_boot = write_smpboot;
>>         binfo.secondary_cpu_reset_hook = reset_secondary;
>> @@ -94,15 +97,15 @@ static void setup_boot(MachineState *machine, int
>> version, size_t ram_size)
>>      * the normal Linux boot process
>>      */
>>     if (machine->firmware) {
>> +        binfo.entry = version==3? FIRMWARE_ADDR_3 : FIRMWARE_ADDR_2;
>>         /* load the firmware image (typically kernel.img) */
>> -        r = load_image_targphys(machine->firmware, FIRMWARE_ADDR,
>> -                                ram_size - FIRMWARE_ADDR);
>> +        r = load_image_targphys(machine->firmware, binfo.entry,
>> +                                ram_size - binfo.entry);
>>         if (r < 0) {
>>             error_report("Failed to load firmware from %s",
>> machine->firmware);
>>             exit(1);
>>         }
>>
>> -        binfo.entry = FIRMWARE_ADDR;
>>         binfo.firmware_loaded = true;
>>     } else {
>>         binfo.kernel_filename = machine->kernel_filename;
>> @@ -113,7 +116,7 @@ static void setup_boot(MachineState *machine, int
>> version, size_t ram_size)
>>     arm_load_kernel(ARM_CPU(first_cpu), &binfo);
>> }
>>
>> -static void raspi2_init(MachineState *machine)
>> +static void raspi_init(MachineState *machine)
>> {
>>     RasPiState *s = g_new0(RasPiState, 1);
>>     uint32_t vcram_size;
>> @@ -121,6 +124,7 @@ static void raspi2_init(MachineState *machine)
>>     BlockBackend *blk;
>>     BusState *bus;
>>     DeviceState *carddev;
>> +    int version = machine->cpu_type==ARM_CPU_TYPE_NAME("cortex-a15")? 2
>> :
>> 3;
>>
>>     object_initialize(&s->soc, sizeof(s->soc), TYPE_BCM2836);
>>     object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
>> @@ -137,8 +141,8 @@ static void raspi2_init(MachineState *machine)
>>                                    &error_abort);
>>     object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
>>                             &error_abort);
>> -    object_property_set_int(OBJECT(&s->soc), 0xa21041, "board-rev",
>> -                            &error_abort);
>> +    object_property_set_int(OBJECT(&s->soc), version==3 ? 0xa02082 :
>> 0xa21041,
>> +                            "board-rev", &error_abort);
>>     object_property_set_bool(OBJECT(&s->soc), true, "realized",
>> &error_abort);
>>
>>     /* Create and plug in the SD cards */
>> @@ -155,21 +159,39 @@ static void raspi2_init(MachineState *machine)
>>
>>     vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
>>                                           &error_abort);
>> -    setup_boot(machine, 2, machine->ram_size - vcram_size);
>> +    setup_boot(machine, version, machine->ram_size - vcram_size);
>> }
>>
>> static void raspi2_machine_init(MachineClass *mc)
>> {
>>     mc->desc = "Raspberry Pi 2";
>> -    mc->init = raspi2_init;
>> +    mc->init = raspi_init;
>>     mc->block_default_type = IF_SD;
>>     mc->no_parallel = 1;
>>     mc->no_floppy = 1;
>>     mc->no_cdrom = 1;
>>     mc->max_cpus = BCM2836_NCPUS;
>>     mc->min_cpus = BCM2836_NCPUS;
>>     mc->default_cpus = BCM2836_NCPUS;
>> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>>     mc->default_ram_size = 1024 * 1024 * 1024;
>>     mc->ignore_memory_transaction_failures = true;
>> };
>> DEFINE_MACHINE("raspi2", raspi2_machine_init)
>> +
>> +static void raspi3_machine_init(MachineClass *mc)
>> +{
>> +    mc->desc = "Raspberry Pi 3";
>> +    mc->init = raspi_init;
>> +    mc->block_default_type = IF_SD;
>> +    mc->no_parallel = 1;
>> +    mc->no_floppy = 1;
>> +    mc->no_cdrom = 1;
>> +    mc->max_cpus = BCM2836_NCPUS;
>> +    mc->min_cpus = BCM2836_NCPUS;
>> +    mc->default_cpus = BCM2836_NCPUS;
>> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
>> +    mc->default_ram_size = 1024 * 1024 * 1024;
>> +    mc->ignore_memory_transaction_failures = true;
>> +};
>> +DEFINE_MACHINE("raspi3", raspi3_machine_init)
>>
>> Hope it's simple enough to be reviewed easily, and it uses CPU type
>> strings
>> as requested.
>>
>> Cheers,
>> bzt
>>
>>
>> On Wed, Nov 29, 2017 at 8:52 AM, bzt bzt <bztemail@gmail.com> wrote:
>>
>>
>>>
>>> On Tue, Nov 28, 2017 at 6:54 PM, Andrew Baumann <
>>> Andrew.Baumann@microsoft.com> wrote:
>>>
>>> From: bzt bzt [mailto:bztemail@gmail.com]
>>>>> Sent: Tuesday, 28 November 2017 03:27
>>>>>
>>>>
>>>> Yes, I agree. I've provided a parameterised version on Oct 24, which
>>>>>
>>>> does not
>>>>
>>>>> have a separate bcm2837 implementation. Is that patch ok?
>>>>>
>>>>
>>>> That patch was moving in the right direction, but I think there were two
>>>> problems with it. First, the right way to pass a parameter is via a
>>>> property field (as Peter explained). Second, IMO the parameter should be
>>>> the CPU model string to instantiate and not the Pi version number.
>>>>
>>>>
>>> Thanks!
>>>
>>>
>>>
>>>> Or should I wait
>>>>> for Alistair's patch (https://lists.gnu.org/archive
>>>>>
>>>> /html/qemu-devel/2017-
>>>>
>>>>> 10/msg04153.html)?
>>>>>
>>>>
>>>> Alistair's patch doesn't actually help you as far as I can see, since it
>>>> doesn't change what CPU bcm2836 instantiates. I think it just means
>>>> that if
>>>> the user specifies a different CPU type they get an error rather than
>>>> being
>>>> silently ignored.
>>>>
>>>>
>>> It does add the CPU model string, which can and should be used in bcm2836
>>> to decide which CPU to instatiate (regardless the method of passing that
>>> parameter). If I add yet another CPU model string, the two patch will be
>>> in
>>> conflict containing redundant information. Also I'm sure would raspi3 use
>>> different cpus, we should always instantiate the one the user specified.
>>> Although here is only one possible CPU model, still that seems to be the
>>> correct behavior. Don't get me wrong, I don't criticize, I just want to
>>> find the best solution. Setting the CPU model only once for each version
>>> seems right. I was thinking: if there's going to be a user specified CPU
>>> model string anyway, why not use that one?
>>>
>>> bzt
>>>
>>>
>>>
>>>> Andrew
>>>>
>>>>
>>>
>>>
>>
>>
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by Peter Maydell 6 years, 2 months ago
On 23 January 2018 at 11:13, bzt bzt <bztemail@gmail.com> wrote:
> There's a huge misunderstanding here. I have a working qemu for about half a
> year now, and I don't need it merged. It's not my goal to submit a patch to
> qemu in any way, I just did that because I had modified an Open Source
> software and wanted to share it with the community. If you don't merge my
> patch, I don't care. Other users will.

Unfortunately we legally can't merge your patch (even if somebody
else is willing to do the work of cleaning it up) because you
haven't provided a signed-off-by line...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 2 months ago
On Tue, Jan 23, 2018 at 12:22 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 23 January 2018 at 11:13, bzt bzt <bztemail@gmail.com> wrote:
> > There's a huge misunderstanding here. I have a working qemu for about
> half a
> > year now, and I don't need it merged. It's not my goal to submit a patch
> to
> > qemu in any way, I just did that because I had modified an Open Source
> > software and wanted to share it with the community. If you don't merge my
> > patch, I don't care. Other users will.
>
> Unfortunately we legally can't merge your patch (even if somebody
> else is willing to do the work of cleaning it up) because you
> haven't provided a signed-off-by line...
>

You have to came up with a better excuse for your incompetence.

Date: Sun, 22 Oct 2017 14:59:20 +0200
Subject: [PATCH] BCM2837 and machine raspi3

Signed-off-by: bzt <bztemail@gmail.com>

And no, I don't feel bad at all about not letting you take advantage on my
programming skills. Now you're on your own, when users starts to demand
raspi3 support in qemu don't expect me to help.

Have a nice day,
bzt


>
> thanks
> -- PMM
>
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by Peter Maydell 6 years, 2 months ago
On 18 January 2018 at 21:39, bzt bzt <bztemail@gmail.com> wrote:
> Dear All,
>
> Since you still haven't merged Alistair's patch, I decided to include it in
> my own.
> I've shrinked the number of modified files to two, that's the bare minimum
> to support "-M raspi3" switch. Bcm2836.c modified minimally, the rest is in
> raspi.c. I've renamed raspi2_init() to raspi_init() 'cos now it initializes
> raspi3 as well, no additional function.
>

Can you send this as a proper patch email, not as a reply to
an existing email thread, please? (This makes our automated tooling
much happier.)

In particular we can't apply patches if they don't come with
the right Signed-off-by: lines from everybody who contributed
code to them.

I've made some code review comments below.

> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 8c43291112..5119e00051 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c
> @@ -15,6 +15,7 @@
>  #include "hw/arm/bcm2836.h"
>  #include "hw/arm/raspi_platform.h"
>  #include "hw/sysbus.h"
> +#include "hw/boards.h"
>  #include "exec/address-spaces.h"
>
>  /* Peripheral base address seen by the CPU */
> @@ -30,7 +31,7 @@ static void bcm2836_init(Object *obj)
>
>      for (n = 0; n < BCM2836_NCPUS; n++) {
>          object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
> -                          "cortex-a15-" TYPE_ARM_CPU);
> +                          current_machine->cpu_type);

This SoC code shouldn't be looking at the current_machine settings.
It should have a QOM property that specifies the cpu type, and
the raspi.c code should set that property. (Call the property
'cpu-type' -- if you grep in hw/arm for that string you'll find
some examples of existing devices/boards that do this.)

>          object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
>                                    &error_abort);
>      }
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index cd5fa8c3dc..24a9243440 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -5,6 +5,8 @@
>   * Rasperry Pi 2 emulation Copyright (c) 2015, Microsoft
>   * Written by Andrew Baumann
>   *
> + * Raspberry Pi 3 emulation Copyright (c) 2018 by bzt
> + *
>   * This code is licensed under the GNU GPLv2 and later.
>   */
>
> @@ -22,10 +24,11 @@
>  #define SMPBOOT_ADDR    0x300 /* this should leave enough space for ATAGS
> */
>  #define MVBAR_ADDR      0x400 /* secure vectors */
>  #define BOARDSETUP_ADDR (MVBAR_ADDR + 0x20) /* board setup code */
> -#define FIRMWARE_ADDR   0x8000 /* Pi loads kernel.img here by default */
> +#define FIRMWARE_ADDR_2    0x8000 /* Pi 2 loads kernel.img here by default
> */
> +#define FIRMWARE_ADDR_3   0x80000 /* Pi 3 loads kernel8.img here by default
> */
>
>  /* Table of Linux board IDs for different Pi versions */
> -static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43};
> +static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43, [3] = 0xc44};
>
>  typedef struct RasPiState {
>      BCM2836State soc;
> @@ -83,8 +86,8 @@ static void setup_boot(MachineState *machine, int version,
> size_t ram_size)
>      binfo.secure_board_setup = true;
>      binfo.secure_boot = true;
>
> -    /* Pi2 requires SMP setup */
> -    if (version == 2) {
> +    /* Pi2 and Pi3 requires SMP setup */
> +    if (version >= 2) {
>          binfo.smp_loader_start = SMPBOOT_ADDR;
>          binfo.write_secondary_boot = write_smpboot;
>          binfo.secondary_cpu_reset_hook = reset_secondary;
> @@ -94,15 +97,15 @@ static void setup_boot(MachineState *machine, int
> version, size_t ram_size)
>       * the normal Linux boot process
>       */
>      if (machine->firmware) {
> +        binfo.entry = version==3? FIRMWARE_ADDR_3 : FIRMWARE_ADDR_2;

There should be more spaces in this : "version == 3 ? ...".
If you run your patch through scripts/checkpatch.pl it should
help with this kind of style nit (though it doesn't always find
everything and sometimes it gets confused, so it's not always right).

>          /* load the firmware image (typically kernel.img) */
> -        r = load_image_targphys(machine->firmware, FIRMWARE_ADDR,
> -                                ram_size - FIRMWARE_ADDR);
> +        r = load_image_targphys(machine->firmware, binfo.entry,
> +                                ram_size - binfo.entry);
>          if (r < 0) {
>              error_report("Failed to load firmware from %s",
> machine->firmware);
>              exit(1);
>          }
>
> -        binfo.entry = FIRMWARE_ADDR;
>          binfo.firmware_loaded = true;
>      } else {
>          binfo.kernel_filename = machine->kernel_filename;
> @@ -113,7 +116,7 @@ static void setup_boot(MachineState *machine, int
> version, size_t ram_size)
>      arm_load_kernel(ARM_CPU(first_cpu), &binfo);
>  }
>
> -static void raspi2_init(MachineState *machine)
> +static void raspi_init(MachineState *machine)
>  {
>      RasPiState *s = g_new0(RasPiState, 1);
>      uint32_t vcram_size;
> @@ -121,6 +124,7 @@ static void raspi2_init(MachineState *machine)
>      BlockBackend *blk;
>      BusState *bus;
>      DeviceState *carddev;
> +    int version = machine->cpu_type==ARM_CPU_TYPE_NAME("cortex-a15")? 2 :
> 3;

This will get confused if the user passes a -cpu argument. Instead
we should just have the machine type directly determine the version.
There are several ways to do this, but the simplest way is to have
raspi2_machine_init() set mc->init to raspi2_init() and raspi3_machine_init()
set mc->niit to raspi3_init(), and thenthose function calls
raspi_init(machine, 2) or raspi_init(machine, 3).

(The other approach would be to make the board set up a subclass
of MachineState and then have raspi2 and raspi3 be subclasses of
that, which gives you a place to define raspi-specific data fields
like "version". You can see that approach in hw/arm/vexpress.c. But
that seems like a lot of effort to go to given where we've started,
so I don't really recommend it.)

>      object_initialize(&s->soc, sizeof(s->soc), TYPE_BCM2836);
>      object_property_add_child(OBJECT(machine), "soc", OBJECT(&s->soc),
> @@ -137,8 +141,8 @@ static void raspi2_init(MachineState *machine)
>                                     &error_abort);
>      object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus",
>                              &error_abort);
> -    object_property_set_int(OBJECT(&s->soc), 0xa21041, "board-rev",
> -                            &error_abort);
> +    object_property_set_int(OBJECT(&s->soc), version==3 ? 0xa02082 :
> 0xa21041,
> +                            "board-rev", &error_abort);
>      object_property_set_bool(OBJECT(&s->soc), true, "realized",
> &error_abort);
>
>      /* Create and plug in the SD cards */
> @@ -155,21 +159,39 @@ static void raspi2_init(MachineState *machine)
>
>      vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
>                                            &error_abort);
> -    setup_boot(machine, 2, machine->ram_size - vcram_size);
> +    setup_boot(machine, version, machine->ram_size - vcram_size);
>  }
>
>  static void raspi2_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Raspberry Pi 2";
> -    mc->init = raspi2_init;
> +    mc->init = raspi_init;
>      mc->block_default_type = IF_SD;
>      mc->no_parallel = 1;
>      mc->no_floppy = 1;
>      mc->no_cdrom = 1;
>      mc->max_cpus = BCM2836_NCPUS;
>      mc->min_cpus = BCM2836_NCPUS;
>      mc->default_cpus = BCM2836_NCPUS;
> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>      mc->default_ram_size = 1024 * 1024 * 1024;
>      mc->ignore_memory_transaction_failures = true;
>  };
>  DEFINE_MACHINE("raspi2", raspi2_machine_init)
> +
> +static void raspi3_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "Raspberry Pi 3";
> +    mc->init = raspi_init;
> +    mc->block_default_type = IF_SD;
> +    mc->no_parallel = 1;
> +    mc->no_floppy = 1;
> +    mc->no_cdrom = 1;
> +    mc->max_cpus = BCM2836_NCPUS;
> +    mc->min_cpus = BCM2836_NCPUS;
> +    mc->default_cpus = BCM2836_NCPUS;
> +    mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> +    mc->default_ram_size = 1024 * 1024 * 1024;
> +    mc->ignore_memory_transaction_failures = true;
> +};
> +DEFINE_MACHINE("raspi3", raspi3_machine_init)


I don't want to add new machine types which set the
ignore_memory_transaction_failures flag to true -- this is
only for existing board models where we don't know what
guest code that used to run would break when the flag was
flipped. For new boards we know that no code runs on them
so can leave the flag at its correct default.

This may mean that you need to add some extra dummy
devices to the the SoC model using
create_unimplemented_device() so that your guest image
doesn't crash trying to probe those devices.

I appreciate that this is a bit annoying for a case like this
where it's adding a new variant of an existing SoC, but
this is one of the situations where the only practical
opportunity to get the implementation right without breaking
existing QEMU users is at the point where we add the new
board model.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by bzt bzt 6 years, 2 months ago
Hi,

On Mon, Jan 22, 2018 at 1:12 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 18 January 2018 at 21:39, bzt bzt <bztemail@gmail.com> wrote:
> > Dear All,
> >
> > Since you still haven't merged Alistair's patch, I decided to include it
> in
> > my own.
> > I've shrinked the number of modified files to two, that's the bare
> minimum
> > to support "-M raspi3" switch. Bcm2836.c modified minimally, the rest is
> in
> > raspi.c. I've renamed raspi2_init() to raspi_init() 'cos now it
> initializes
> > raspi3 as well, no additional function.
> >
>
> Can you send this as a proper patch email, not as a reply to
> an existing email thread, please? (This makes our automated tooling
> much happier.)
>

Only if you're not demanding any further nonsense modifications.


>
> In particular we can't apply patches if they don't come with
> the right Signed-off-by: lines from everybody who contributed
> code to them.
>
> I've made some code review comments below.
>
> > diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> > index 8c43291112..5119e00051 100644
> > --- a/hw/arm/bcm2836.c
> > +++ b/hw/arm/bcm2836.c
> > @@ -15,6 +15,7 @@
> >  #include "hw/arm/bcm2836.h"
> >  #include "hw/arm/raspi_platform.h"
> >  #include "hw/sysbus.h"
> > +#include "hw/boards.h"
> >  #include "exec/address-spaces.h"
> >
> >  /* Peripheral base address seen by the CPU */
> > @@ -30,7 +31,7 @@ static void bcm2836_init(Object *obj)
> >
> >      for (n = 0; n < BCM2836_NCPUS; n++) {
> >          object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
> > -                          "cortex-a15-" TYPE_ARM_CPU);
> > +                          current_machine->cpu_type);
>
> This SoC code shouldn't be looking at the current_machine settings.
> It should have a QOM property that specifies the cpu type, and

the raspi.c code should set that property. (Call the property
>
'cpu-type' -- if you grep in hw/arm for that string you'll find
> some examples of existing devices/boards that do this.)
>

I'll give you some time to think about why it is nonsense that you asking
for.
Hint: you cannot add properties to an uninitialized object, you can't add
the CPUs in the realize phase (will cause segfaults, I've tried) and SoC
initialization will do call bcm2836_init().

  [...]

> There should be more spaces in this : "version == 3 ? ...".
> If you run your patch through scripts/checkpatch.pl it should
> help with this kind of style nit (though it doesn't always find
> everything and sometimes it gets confused, so it's not always right).
>

Sure thing.


> [...]
> This will get confused if the user passes a -cpu argument. Instead
> we should just have the machine type directly determine the version.
> There are several ways to do this, but the simplest way is to have
> raspi2_machine_init() set mc->init to raspi2_init() and
> raspi3_machine_init()
> set mc->niit to raspi3_init(), and thenthose function calls
> raspi_init(machine, 2) or raspi_init(machine, 3).
>

Auch. Originally there were two raspi_init() functions (with two different
version values), I've reduced the number to one because you asked for it.
I've also tried to access the MachineClass object from raspi_init(), but
it's impossible, MACHINE_CLASS() drops and assert, find_machine_class()
segfaults. Nice model interface you have there.


>
> (The other approach would be to make the board set up a subclass
> of MachineState and then have raspi2 and raspi3 be subclasses of
> that, which gives you a place to define raspi-specific data fields
> like "version". You can see that approach in hw/arm/vexpress.c. But
> that seems like a lot of effort to go to given where we've started,
> so I don't really recommend it.)
>

Again, that is EXACTLY what I originally had, and you (the maintainers)
said there shouldn't be a new BCM2837 class, so I've removed it.
Look, if you can't understand how my patch works, just say so, no shame in
that.


> [...]
>
> I don't want to add new machine types which set the
> ignore_memory_transaction_failures flag to true -- this is
> only for existing board models where we don't know what
> guest code that used to run would break when the flag was
> flipped. For new boards we know that no code runs on them
> so can leave the flag at its correct default.
>

Just copy'n'pasted raspi2, only modified the CPU model in which they differ.


>
> This may mean that you need to add some extra dummy
> devices to the the SoC model using
> create_unimplemented_device() so that your guest image
> doesn't crash trying to probe those devices.
>

This may mean your users will be very upset because mainline qemu won't
emulate raspi3 in the foreseeable future, because it's sure like hell I
won't fix something that's not broken.


>
> I appreciate that this is a bit annoying for a case like this
> where it's adding a new variant of an existing SoC, but
> this is one of the situations where the only practical
> opportunity to get the implementation right without breaking
> existing QEMU users is at the point where we add the new
> board model.
>

It is not annoying at all, it's simply silly. Are you really thought I
would do *your* job?
"Only two things are infinite, the universe and the human stupidity, and
I'm not sure about the former!". Actually I'm very entertained :-D :-D :-D

But seriously, I've showed you a way how to add raspi3, the rest is up to
you. I won't do anything more about it, I'm not a paid qemu maintainer
whatsoever!
Again, my patch is working for me, it does not broke any existing raspi2
machines, and I'm very happy about using it for more than half a year now!
These are the facts, everything else is just talking.

Cheers
bzt


> thanks
> -- PMM
>
Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by BALATON Zoltan 6 years, 2 months ago
On Tue, 23 Jan 2018, bzt bzt wrote:
> It is not annoying at all, it's simply silly. Are you really thought I
> would do *your* job?
> "Only two things are infinite, the universe and the human stupidity, and
> I'm not sure about the former!". Actually I'm very entertained :-D :-D :-D
>
> But seriously, I've showed you a way how to add raspi3, the rest is up to
> you. I won't do anything more about it, I'm not a paid qemu maintainer
> whatsoever!
> Again, my patch is working for me, it does not broke any existing raspi2
> machines, and I'm very happy about using it for more than half a year now!
> These are the facts, everything else is just talking.

OK, sorry for trying to help. I'm not a paid QEMU developer and don't use 
ARM emulation so I don't really care about it. Please exclude me from this 
thread in all furhter correspondence.

Thank you,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] BCM2837 and machine raspi3
Posted by Peter Maydell 6 years, 2 months ago
On 23 January 2018 at 11:49, bzt bzt <bztemail@gmail.com> wrote:
> Hi,
>
> On Mon, Jan 22, 2018 at 1:12 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>> Can you send this as a proper patch email, not as a reply to
>> an existing email thread, please? (This makes our automated tooling
>> much happier.)
>
>
> Only if you're not demanding any further nonsense modifications.

I think that patch review will be much more pleasant for
both of us if you tone down the combativeness here.
Yes, sometimes code review comments aren't right because
the reviewer has missed or misunderstood a detail -- but
there's no need to throw insults around as a result.

If you don't have the time or don't want to engage with the
community's review processes, that's fine. But this hostility
is not pleasant to interact with and off-putting to other
people reading it. Please stop it.

(Sometimes a submitter doesn't have the time to work with our
review process, and that's fine. It just means that the patches
will sit on the mailing list and perhaps in future somebody
who does have the time as well as the interest in the feature
will come along and pick up the work.)

thanks
-- PMM