Re: [Qemu-devel] [PATCH v4 8/8] raspi: add raspberry pi 2 machine

[Peter Crosthwaite]

On Fri, Jan 15, 2016 at 3:58 PM, Andrew Baumann
<address@hidden> wrote:
> Signed-off-by: Andrew Baumann <address@hidden>
> ---
>
> Notes:
>     Pi1 requires more peripherals, and will be added in a later patch
>     series.
>
>     v4:
>     * drop header comment from versatile
>     * made smpboot and board setup blobs relocatable (within limits:
>       we can't use ARMv7 MOVW for Pi1, so it's messier than highbank)
>     * move board setup blob to common code
>     * modify SCR using read-or-write
>     * s/RaspiMachineState/RaspiState/
>     * style tweaks
>
>     v3:
>      * fix board setup to remain Pi1 compatible
>      * pass ram property
>
>  hw/arm/Makefile.objs |   2 +-
>  hw/arm/raspi.c       | 156 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 157 insertions(+), 1 deletion(-)
>  create mode 100644 hw/arm/raspi.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index f55f8d2..a711e4d 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
> +obj-$(CONFIG_RASPI) += bcm2835_peripherals.o bcm2836.o raspi.o
>  obj-$(CONFIG_STM32F205_SOC) += stm32f205_soc.o
>  obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp.o xlnx-ep108.o
>  obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> new file mode 100644
> index 0000000..2110725
> --- /dev/null
> +++ b/hw/arm/raspi.c
> @@ -0,0 +1,156 @@
> +/*
> + * Raspberry Pi emulation (c) 2012 Gregory Estrade
> + * Upstreaming code cleanup [including bcm2835_*] (c) 2013 Jan Petrous
> + *
> + * Rasperry Pi 2 emulation Copyright (c) 2015, Microsoft
> + * Written by Andrew Baumann
> + *
> + * This code is licensed under the GNU GPLv2 and later.
> + */
> +
> +#include "hw/arm/bcm2836.h"
> +#include "qemu/error-report.h"
> +#include "hw/boards.h"
> +#include "hw/loader.h"
> +#include "hw/arm/arm.h"
> +#include "sysemu/sysemu.h"
> +
> +#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 */
> +
> +/* Table of Linux board IDs for different Pi versions */
> +static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43};
> +
> +typedef struct RaspiState {

A quick google search, I see the camel case form for rpi is usually
"RasPi". Should we follow?

> +    union {

union not needed.

> +        BCM2836State pi2;
> +    } soc;
> +    MemoryRegion ram;
> +} RaspiState;
> +
> +static void write_smpboot(ARMCPU *cpu, const struct arm_boot_info *info)
> +{
> +    static const uint32_t smpboot[] = {
> +        0xE1A0E00F, /*    mov     lr, pc */
> +        0xE3A0FE00 + (BOARDSETUP_ADDR >> 4), /* mov pc, BOARDSETUP_ADDR */
> +        0xEE100FB0, /*    mrc     p15, 0, r0, c0, c0, 5;get core ID */
> +        0xE7E10050, /*    ubfx    r0, r0, #0, #2       ;extract LSB */
> +        0xE59F5014, /*    ldr     r5, =0x400000CC      ;load mbox base */
> +        0xE320F001, /* 1: yield */
> +        0xE7953200, /*    ldr     r3, [r5, r0, lsl #4] ;read mbox for our 
> core*/
> +        0xE3530000, /*    cmp     r3, #0               ;spin while zero */
> +        0x0AFFFFFB, /*    beq     1b */
> +        0xE7853200, /*    str     r3, [r5, r0, lsl #4] ;clear mbox */
> +        0xE12FFF13, /*    bx      r3                   ;jump to target */
> +        0x400000CC, /* (constant: mailbox 3 read/clear base) */

lower case hex for consistency with other blobbing boards (exynos,
zynq, arm_boot).

> +    };
> +

> +    /* check that we don't overrun board setup vectors */
> +    assert(SMPBOOT_ADDR + sizeof(smpboot) <= MVBAR_ADDR);
> +    /* check that board setup address is correctly relocated */
> +    assert((BOARDSETUP_ADDR & 0xf) == 0 && (BOARDSETUP_ADDR >> 4) < 0x100);

QEMU_BUILD_BUG_ON (both asserts should be convertible)

> +
> +    rom_add_blob_fixed("raspi_smpboot", smpboot, sizeof(smpboot),
> +                       info->smp_loader_start);
> +}
> +
> +static void write_board_setup(ARMCPU *cpu, const struct arm_boot_info *info)
> +{
> +    arm_write_secure_board_setup_dummy_smc(cpu, info, MVBAR_ADDR);
> +}
> +
> +static void reset_secondary(ARMCPU *cpu, const struct arm_boot_info *info)
> +{
> +    CPUState *cs = CPU(cpu);
> +    cpu_set_pc(cs, info->smp_loader_start);
> +}
> +
> +static void setup_boot(MachineState *machine, int version, size_t ram_size)
> +{
> +    static struct arm_boot_info binfo;
> +    int r;
> +
> +    binfo.board_id = raspi_boardid[version];
> +    binfo.ram_size = ram_size;
> +    binfo.nb_cpus = smp_cpus;
> +    binfo.board_setup_addr = BOARDSETUP_ADDR;
> +    binfo.write_board_setup = write_board_setup;
> +    binfo.secure_board_setup = true;
> +    binfo.secure_boot = true;
> +
> +    /* Pi2 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;
> +    }
> +
> +    /* If the user specified a "firmware" image (e.g. UEFI), we bypass
> +     * the normal Linux boot process
> +     */
> +    if (machine->firmware) {

Others on the qemu-arm list follow UEFI support better than I so Peter
or someone else should take a look at this section I think.

> +        /* load the firmware image (typically kernel.img) */
> +        r = load_image_targphys(machine->firmware, FIRMWARE_ADDR,
> +                                ram_size - FIRMWARE_ADDR);
> +        if (r < 0) {
> +            error_report("Failed to load firmware from %s", 
> machine->firmware);
> +            exit(1);
> +        }
> +
> +        /* set variables so arm_load_kernel does the right thing */

This comment doesn't add too much. I think it can be dropped
(everything in a binfo is to make arm_load_kernel DTRT).

> +        binfo.entry = FIRMWARE_ADDR;
> +        binfo.firmware_loaded = true;
> +    } else {
> +        /* Just let arm_load_kernel do everything for us... */
> +        binfo.kernel_filename = machine->kernel_filename;
> +        binfo.kernel_cmdline = machine->kernel_cmdline;
> +        binfo.initrd_filename = machine->initrd_filename;
> +    }
> +
> +    arm_load_kernel(ARM_CPU(first_cpu), &binfo);
> +}
> +
> +static void raspi2_init(MachineState *machine)
> +{
> +    RaspiState *s = g_new0(RaspiState, 1);
> +
> +    /* Initialise the SOC */

Drop comment.

> +    object_initialize(&s->soc.pi2, sizeof(s->soc.pi2), TYPE_BCM2836);
> +    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_bool(OBJECT(&s->soc), true, "realized", 
> &error_abort);
> +
> +    /* Prepare to boot */

Comment still doesn't add much so should drop.

Otherwise:

Reviewed-by: Peter Crosthwaite <address@hidden>

Thanks for your continued persistence and sorry about the recent
delays through review process.

Regards,
Peter

> +    setup_boot(machine, 2, machine->ram_size);
> +}
> +
> +static void raspi2_machine_init(MachineClass *mc)
> +{
> +    mc->desc = "Raspberry Pi 2";
> +    mc->init = raspi2_init;
> +    mc->block_default_type = IF_SD;
> +    mc->no_parallel = 1;
> +    mc->no_floppy = 1;
> +    mc->no_cdrom = 1;
> +    mc->max_cpus = BCM2836_NCPUS;
> +
> +    /* XXX: Temporary restriction in RAM size from the full 1GB. Since
> +     * we do not yet support the framebuffer / GPU, we need to limit
> +     * RAM usable by the OS to sit below the peripherals.
> +     */
> +    mc->default_ram_size = 0x3F000000; /* BCM2836_PERI_BASE */
> +};
> +DEFINE_MACHINE("raspi2", raspi2_machine_init)
> --
> 2.5.3
>