[Qemu-devel] [PATCH] Add BCM2835 devices to Arm hardware.

[Qemu-devel] [PATCH] Add BCM2835 devices to Arm hardware.

[John Bradley]

>From 0b39a04030d5a2cea4fcd2159d365580ca155b78 Mon Sep 17 00:00:00 2001
From: John Bradley <flypie@rocketmail.com>
Date: Wed, 17 May 2017 18:57:21 +0100
Subject: [PATCH] Add BCM2835 devices to Arm hardware.

Signed-off-by: John Bradley <flypie@rocketmail.com>
---
hw/arm/bcm2835.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 114 insertions(+)

diff --git a/hw/arm/bcm2835.c b/hw/arm/bcm2835.c
new file mode 100644
index 0000000000..e5744c1620
--- /dev/null
+++ b/hw/arm/bcm2835.c
@@ -0,0 +1,114 @@
+/*
+ * 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.
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "hw/arm/bcm2835.h"
+#include "hw/arm/raspi_platform.h"
+#include "hw/sysbus.h"
+#include "exec/address-spaces.h"
+
+
+/* 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->cpus[0], sizeof(s->cpus[0]), "arm1176-" TYPE_ARM_CPU);
+    object_property_add_child(obj, "cpu", OBJECT(&s->cpus[0]), &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);
+
+    object_property_set_bool(OBJECT(&s->cpus[0]), 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->cpus[0]), ARM_CPU_IRQ));
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->peripherals), 1,
+                       qdev_get_gpio_in(DEVICE(&s->cpus[0]), ARM_CPU_FIQ));
+}
+
+static Property bcm2835_props[] = {
+    DEFINE_PROP_UINT32("enabled-cpus", BCM2835State, enabled_cpus, BCM2835_NCPUS),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static void bcm2835_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->props = bcm2835_props;
+    dc->realize = bcm2835_realize;
+}
+
+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)


 
John Bradley
Tel: 07896 839635
Skype: flypie125
125B Grove Street
Edge Hill 
Liverpool L7 7AF

Re: [Qemu-devel] [PATCH] Add BCM2835 devices to Arm hardware.

[Eric Blake]

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

On 05/17/2017 04:05 PM, John Bradley via Qemu-devel wrote:
>>From 0b39a04030d5a2cea4fcd2159d365580ca155b78 Mon Sep 17 00:00:00 2001
> From: John Bradley <flypie@rocketmail.com>
> Date: Wed, 17 May 2017 18:57:21 +0100
> Subject: [PATCH] Add BCM2835 devices to Arm hardware.
> 
> Signed-off-by: John Bradley <flypie@rocketmail.com>
> ---
> hw/arm/bcm2835.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 114 insertions(+)
> 

> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "hw/arm/bcm2835.h"

This file does not exist in master. Since it is obvious that this patch
depends on other patches you have pending, you should submit everything
as a patch series (where the patch adding bcm2835.h would be labeled
1/2, and this patch labeled 2/2, as well as with a 0/2 cover letter - or
0/N for however many N patches you plan to submit).  Posting each
dependent patch as a top-level thread with no documentation on how they
interrelate is just going to waste reviewer's time, as well as trigger
more bot-related compile failure messages to you, that could have been
prevented if it had been properly submitted as a series.

It looks like you are not using 'git send-email' to send your patches.
That makes it highly likely that your patches will be corrupted to the
point that they cannot be applied by automated tooling.  While a one-off
patch submission can usually be manually beaten into correct form by the
maintainer, it is much harder to offload this burden onto others when
you plan to submit a series - so you should really fix your workflow to
get 'git send-email' working properly.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] Add BCM2835 devices to Arm hardware.

[Eric Blake]

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

On 05/17/2017 04:05 PM, John Bradley via Qemu-devel wrote:
>>From 0b39a04030d5a2cea4fcd2159d365580ca155b78 Mon Sep 17 00:00:00 2001
> From: John Bradley <flypie@rocketmail.com>
> Date: Wed, 17 May 2017 18:57:21 +0100
> Subject: [PATCH] Add BCM2835 devices to Arm hardware.
> 

The subject line is not typical; you are missing a 'category: ' prefix,
and most commits don't end in '.'.  I'd suggest:

bcm2835: Add new Arm device

Your commit message is sparse. While the subject does a good one-line
summary of WHAT, the commit body is where you state WHY and/or go into
more details.


> Signed-off-by: John Bradley <flypie@rocketmail.com>
> ---
> hw/arm/bcm2835.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 114 insertions(+)

Adding a new .c file without touching any Makefile snippets is pointless
- your new code does not compile.  Therefore, we cannot validate that it
is useful.  Also, it pays to double-check that MAINTAINERS will cover
the new file (if not, you need to add a section, as we are trying to
avoid adding new files without a listed maintainer).

I'm doing a rough code review (as I can't compile this in isolation, and
don't know what the prerequisites are - but at least this patch is small
enough to be reviewable):

> 
> diff --git a/hw/arm/bcm2835.c b/hw/arm/bcm2835.c
> new file mode 100644
> index 0000000000..e5744c1620
> --- /dev/null
> +++ b/hw/arm/bcm2835.c
> @@ -0,0 +1,114 @@
> +/*
> + * Raspberry Pi emulation (c) 2012 Gregory Estrade
> + * Upstreaming code cleanup [including bcm2835_*] (c) 2013 Jan Petrous

Then where is Jan Petrous' Signed-off-by:?

> + *
> + * Rasperry Pi 2 emulation and refactoring Copyright (c) 2015, Microsoft
> + * Written by Andrew Baumann

Then where is Andrew Baumann's Signed-off-by:?

> + *
> + * This code is licensed under the GNU GPLv2 and later.

That's not the usual spelling for GPLv2+ as used in other files,
although we haven't been very consistent so it's probably okay.

$ git grep 'or later' | wc
    603    9704   58672
$ git grep 'and later' | wc
     71     734    6032



> + */
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "hw/arm/bcm2835.h"
> +#include "hw/arm/raspi_platform.h"
> +#include "hw/sysbus.h"
> +#include "exec/address-spaces.h"
> +
> +
> +/* 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->cpus[0], sizeof(s->cpus[0]), "arm1176-" TYPE_ARM_CPU);
> +    object_property_add_child(obj, "cpu", OBJECT(&s->cpus[0]), &error_abort);
> +
> +    object_initialize(&s->peripherals, sizeof(s->peripherals),
> +                      TYPE_BCM2835_PERIPHERALS);
> +    object_property_add_child(obj, "peripherals", OBJECT(&s->peripherals),
> +                              &error_abort);

Does this use of error_abort even compile without an #include
"qapi/error.h"?


> +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) {

I personally prefer 'if (!obj)' (less typing), but your use of '(obj ==
NULL)' is fine.

> +        error_setg(errp, "%s: required ram link not found: %s",
> +                   __func__, error_get_pretty(err));

error_setg() already includes __func__ as part of its boilerplate; your
explicit use of __func__ is redundant and makes your error look stupid.

It looks like you are trying to add details to an existing error -
rather than calling error_setg(, error_get_pretty(err)), you should
instead use:

obj = (, errp);
if (obj == NULL) {
    error_prepend(errp, "required ram link not found: ");

> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->peripherals), 0,
> +                            BCM2835_PERI_BASE, 1);
> +
> +    object_property_set_bool(OBJECT(&s->cpus[0]), true, "realized", &err);
> +    if (err) {
> +        error_report_err(err);
> +        exit(1);

It's weird to mix error_propagate() (return the error to the caller to
deal with) and error_report_err() (report the error to the end user and
exit) in the same function.  You should probably NOT be using
error_report_err() or exit().

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]