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 <fly...@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 <fly...@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
signature.asc
Description: OpenPGP digital signature
