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

[Andrew Baumann <Andrew.Baumann@microsoft.com>]

> 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