Re: [Qemu-arm] [PATCH] raspi: Add Raspberry Pi 1 support

[Peter Maydell]

On 8 April 2017 at 06:43, Omar Rizwan <address@hidden> wrote:
> Signed-off-by: Omar Rizwan <address@hidden>
> ---
>  hw/arm/Makefile.objs         |   2 +-
>  hw/arm/bcm2835.c             | 119 
> +++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/bcm2835_peripherals.c |   1 +
>  hw/arm/raspi.c               |  47 ++++++++++++++---
>  include/hw/arm/bcm2835.h     |  31 +++++++++++
>  5 files changed, 191 insertions(+), 9 deletions(-)
>  create mode 100644 hw/arm/bcm2835.c
>  create mode 100644 include/hw/arm/bcm2835.h

Hi; thanks for this patch. I have a few comments below to add to the ones
from Andrew.


> +/* Peripheral base address seen by the CPU */
> +#define BCM2835_PERI_BASE       0x20000000
> +
> +static void bcm2835_init(Object *obj)
> +{
> +    BCM2835State *s = BCM2835(obj);
> +
> +    object_initialize(&s->cpu, sizeof(s->cpu), "arm1176-" TYPE_ARM_CPU);
> +    object_property_add_child(obj, "cpu", OBJECT(&s->cpu), &error_abort);
> +
> +    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 bcm2835_realize(DeviceState *dev, Error **errp)
> +{
> +    BCM2835State *s = BCM2835(dev);
> +    Object *obj;
> +    Error *err = NULL;
> +
> +    /* 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,
> +                            BCM2835_PERI_BASE, 1);
> +
> +    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->peripherals), 1,
> +                            0x40000000, 1);

If this mapping is correct, can we have a #define for this address, like we
do for BCM2835_PERI_BASE ?

(I see from Andrew's review that it's not clear whether we should be
mapping this here.)

> +
> +    object_property_set_bool(OBJECT(&s->cpu), true, "realized", &err);
> +    if (err) {
> +        error_report_err(err);
> +        exit(1);
> +    }
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 0,
> +                       qdev_get_gpio_in(DEVICE(&s->cpu), ARM_CPU_IRQ));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 1,
> +                       qdev_get_gpio_in(DEVICE(&s->cpu), ARM_CPU_FIQ));
> +
> +}
> +
> +static void bcm2835_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = bcm2835_realize;
> +
> +    /*
> +     * Reason: creates an ARM CPU, thus use after free(), see
> +     * arm_cpu_class_init()
> +     */
> +    dc->cannot_destroy_with_object_finalize_yet = true;

This assignment and its attached comment can be deleted, because
the issue with arm_cpu_class_init() has now been fixed upstream
(there's a patchset on the list that removes the DeviceClass
field entirely).

> +}
> +
> +static const TypeInfo bcm2835_type_info = {
> +    .name = TYPE_BCM2835,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(BCM2835State),
> +    .instance_init = bcm2835_init,
> +    .class_init = bcm2835_class_init,
> +};
> +
> +static void bcm2835_register_types(void)
> +{
> +    type_register_static(&bcm2835_type_info);
> +}
> +
> +type_init(bcm2835_register_types)
> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> index 369ef1e..a6cd54a 100644
> --- a/hw/arm/bcm2835_peripherals.c
> +++ b/hw/arm/bcm2835_peripherals.c
> @@ -34,6 +34,7 @@ static void bcm2835_peripherals_init(Object *obj)
>      /* Internal memory region for peripheral bus addresses (not exported) */
>      memory_region_init(&s->gpu_bus_mr, obj, "bcm2835-gpu", (uint64_t)1 << 
> 32);
>      object_property_add_child(obj, "gpu-bus", OBJECT(&s->gpu_bus_mr), NULL);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->gpu_bus_mr);

If we determine that this is correct, then we should delete the "(not exported)"
part of the comment above, because this added call to sysbus_init_mmio()
is exporting the memory region.

>      /* Internal memory region for request/response communication with
>       * mailbox-addressable peripherals (not exported)
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 2b295f1..dca6077 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -2,9 +2,11 @@
>   * Raspberry Pi emulation (c) 2012 Gregory Estrade
>   * Upstreaming code cleanup [including bcm2835_*] (c) 2013 Jan Petrous
>   *
> - * Rasperry Pi 2 emulation Copyright (c) 2015, Microsoft
> + * Raspberry Pi 2 emulation Copyright (c) 2015, Microsoft
>   * Written by Andrew Baumann
>   *
> + * Raspberry Pi 1 rebase onto master (c) 2017, Omar Rizwan
> + *
>   * This code is licensed under the GNU GPLv2 and later.
>   */
>
> @@ -12,6 +14,7 @@
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
> +#include "hw/arm/bcm2835.h"
>  #include "hw/arm/bcm2836.h"
>  #include "qemu/error-report.h"
>  #include "hw/boards.h"
> @@ -27,6 +30,11 @@
>  /* Table of Linux board IDs for different Pi versions */
>  static const int raspi_boardid[] = {[1] = 0xc42, [2] = 0xc43};
>
> +/* Table of board revisions:
> + * 
> https://github.com/AndrewFromMelbourne/raspberry_pi_revision/blob/2764c19983fb7b1ef8cb21031e94c293703afa2e/README.md
> + */
> +static const uint32_t raspi_boardrev[] = {[1] = 0x10, [2] = 0xa21041};

So we have three things here that are per-board:
 * the boardid value
 * the boardrev value
 * the soc_type value

which we have stored in two arrays plus passing a different parameter
to the raspi_machine_init function.

Perhaps this would be better as a
typedef struct RPiInfo {
    const char *soc_type;
    uint32_t boardrev;
    uint32_t boardid;
} RPiInfo;

static const RPiInfo rpi_boards[] = {
    {
         /* Raspberry Pi 1 */
         .soc_type = TYPE_BCM2835,
         .boardrev = 0x10,
         .boardid = 0xc42,
    }, {
         /* Raspberry Pi 2 */
         ...
    }
};

?

thanks
-- PMM