| Message ID | 218478942.134698.1495055111655@mail.yahoo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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().
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)