Re: [Qemu-devel] [PATCH v3 4/7] bcm2835_peripherals: add rollup device for bcm2835 peripherals

[Peter Crosthwaite]

On Tue, Jan 5, 2016 at 10:07 PM, Andrew Baumann
 wrote:
>> From: Alistair Francis [mailto:alistai...@gmail.com]
>> Sent: Tuesday, 5 January 2016 18:14
>> On Thu, Dec 31, 2015 at 4:31 PM, Andrew Baumann
>>  wrote:
>> > This device maintains all the non-CPU peripherals on bcm2835 (Pi1)
>> > which are also present on bcm2836 (Pi2). It also implements the
>> > private address spaces used for DMA and mailboxes.
> [...]
>> > +obj = object_property_get_link(OBJECT(dev), "ram", );
>> > +if (obj == NULL) {
>> > +error_setg(errp, "%s: required ram link not found: %s",
>> > +   __func__, error_get_pretty(err));
>> > +return;
>> > +}
>>
>> I only had a quick read of this patch, but this RAM linking looks fine
>> to me. Out of curiosity is there a reason you use
>> object_property_get_link() instead of object_property_add_link() in
>> the init?
>

The const link system removes the need for the object to have storage
for the link pointer in state. This means you don't need the state
field or add_link(), but the only way to get the pointer for your own
use is to get_link() on yourself. This is slightly simpler but has the
disadvantage that you cannot unlink and then relink something else (I
think?).

I don't have an opinion over which way is more correct so both are
fine for me but if the QOM people have a preferred style we should
probably make the two patches consistent.

Regards,
Peter

> I'm not sure I understand your question... it wouldn't work the other way. I 
> allocate the ram and add the link using object_property_add_const_link() in 
> hw/arm/raspi.c. This file needs to consume the ram to setup alias mappings, 
> so it is using get_link(). (Note there's also level of indirection; raspi 
> creates bcm2836, which does nothing but get the link set by its parent and 
> add it to its bcm2835_peripherals child.)
>
> I suppose I could do it the other way around (allocate and set link in 
> bcm2835_peripherals, based on a size passed from the board), but it seemed 
> more logical to treat the RAM as created/owned of the board rather than the 
> SoC.
>
> Cheers,
> Andrew

Re: [Qemu-devel] [PATCH v3 4/7] bcm2835_peripherals: add rollup device for bcm2835 peripherals

[Alistair Francis]

On Wed, Jan 6, 2016 at 5:32 AM, Peter Crosthwaite
 wrote:
> On Tue, Jan 5, 2016 at 10:07 PM, Andrew Baumann
>  wrote:
>>> From: Alistair Francis [mailto:alistai...@gmail.com]
>>> Sent: Tuesday, 5 January 2016 18:14
>>> On Thu, Dec 31, 2015 at 4:31 PM, Andrew Baumann
>>>  wrote:
>>> > This device maintains all the non-CPU peripherals on bcm2835 (Pi1)
>>> > which are also present on bcm2836 (Pi2). It also implements the
>>> > private address spaces used for DMA and mailboxes.
>> [...]
>>> > +obj = object_property_get_link(OBJECT(dev), "ram", );
>>> > +if (obj == NULL) {
>>> > +error_setg(errp, "%s: required ram link not found: %s",
>>> > +   __func__, error_get_pretty(err));
>>> > +return;
>>> > +}
>>>
>>> I only had a quick read of this patch, but this RAM linking looks fine
>>> to me. Out of curiosity is there a reason you use
>>> object_property_get_link() instead of object_property_add_link() in
>>> the init?
>>
>
> The const link system removes the need for the object to have storage
> for the link pointer in state. This means you don't need the state
> field or add_link(), but the only way to get the pointer for your own
> use is to get_link() on yourself. This is slightly simpler but has the
> disadvantage that you cannot unlink and then relink something else (I
> think?).

Ok, that makes sense.

Thanks,

Alistair

>
> I don't have an opinion over which way is more correct so both are
> fine for me but if the QOM people have a preferred style we should
> probably make the two patches consistent.
>
> Regards,
> Peter
>
>> I'm not sure I understand your question... it wouldn't work the other way. I 
>> allocate the ram and add the link using object_property_add_const_link() in 
>> hw/arm/raspi.c. This file needs to consume the ram to setup alias mappings, 
>> so it is using get_link(). (Note there's also level of indirection; raspi 
>> creates bcm2836, which does nothing but get the link set by its parent and 
>> add it to its bcm2835_peripherals child.)
>>
>> I suppose I could do it the other way around (allocate and set link in 
>> bcm2835_peripherals, based on a size passed from the board), but it seemed 
>> more logical to treat the RAM as created/owned of the board rather than the 
>> SoC.
>>
>> Cheers,
>> Andrew

Re: [Qemu-devel] [PATCH v3 4/7] bcm2835_peripherals: add rollup device for bcm2835 peripherals

[Andrew Baumann]

> From: Alistair Francis [mailto:alistai...@gmail.com]
> Sent: Tuesday, 5 January 2016 18:14
> On Thu, Dec 31, 2015 at 4:31 PM, Andrew Baumann
>  wrote:
> > This device maintains all the non-CPU peripherals on bcm2835 (Pi1)
> > which are also present on bcm2836 (Pi2). It also implements the
> > private address spaces used for DMA and mailboxes.
[...]
> > +obj = object_property_get_link(OBJECT(dev), "ram", );
> > +if (obj == NULL) {
> > +error_setg(errp, "%s: required ram link not found: %s",
> > +   __func__, error_get_pretty(err));
> > +return;
> > +}
> 
> I only had a quick read of this patch, but this RAM linking looks fine
> to me. Out of curiosity is there a reason you use
> object_property_get_link() instead of object_property_add_link() in
> the init?

I'm not sure I understand your question... it wouldn't work the other way. I 
allocate the ram and add the link using object_property_add_const_link() in 
hw/arm/raspi.c. This file needs to consume the ram to setup alias mappings, so 
it is using get_link(). (Note there's also level of indirection; raspi creates 
bcm2836, which does nothing but get the link set by its parent and add it to 
its bcm2835_peripherals child.)

I suppose I could do it the other way around (allocate and set link in 
bcm2835_peripherals, based on a size passed from the board), but it seemed more 
logical to treat the RAM as created/owned of the board rather than the SoC.

Cheers,
Andrew

Re: [Qemu-devel] [PATCH v3 4/7] bcm2835_peripherals: add rollup device for bcm2835 peripherals

[Alistair Francis]

On Thu, Dec 31, 2015 at 4:31 PM, Andrew Baumann
 wrote:
> This device maintains all the non-CPU peripherals on bcm2835 (Pi1)
> which are also present on bcm2836 (Pi2). It also implements the
> private address spaces used for DMA and mailboxes.
>
> Signed-off-by: Andrew Baumann 
> ---
>
> Notes:
> v3:
>  * clean up raspi_platform.h
>  * s/_/-/ in type/property/child names
>  * use memory_region_init where appropriate rather than 
> memory_region_init_io
>  * pass ram as link property
>
> v2 changes:
>  * adapted to use common SDHCI emulation
>
>  hw/arm/Makefile.objs |   1 +
>  hw/arm/bcm2835_peripherals.c | 205 
> +++
>  include/hw/arm/bcm2835_peripherals.h |  42 +++
>  include/hw/arm/raspi_platform.h  | 128 ++
>  4 files changed, 376 insertions(+)
>  create mode 100644 hw/arm/bcm2835_peripherals.c
>  create mode 100644 include/hw/arm/bcm2835_peripherals.h
>  create mode 100644 include/hw/arm/raspi_platform.h
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 2195b60..82cc142 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -11,6 +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
>  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/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> new file mode 100644
> index 000..879a41d
> --- /dev/null
> +++ b/hw/arm/bcm2835_peripherals.c
> @@ -0,0 +1,205 @@
> +/*
> + * 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 "hw/arm/bcm2835_peripherals.h"
> +#include "hw/misc/bcm2835_mbox_defs.h"
> +#include "hw/arm/raspi_platform.h"
> +
> +/* Peripheral base address on the VC (GPU) system bus */
> +#define BCM2835_VC_PERI_BASE 0x7e00
> +
> +/* Capabilities for SD controller: no DMA, high-speed, default clocks etc. */
> +#define BCM2835_SDHC_CAPAREG 0x52034b4
> +
> +static void bcm2835_peripherals_init(Object *obj)
> +{
> +BCM2835PeripheralState *s = BCM2835_PERIPHERALS(obj);
> +
> +/* Memory region for peripheral devices, which we export to our parent */
> +memory_region_init_io(>peri_mr, obj, NULL, s, "bcm2835-peripherals",
> +  0x100);
> +object_property_add_child(obj, "peripheral-io", OBJECT(>peri_mr), 
> NULL);
> +sysbus_init_mmio(SYS_BUS_DEVICE(s), >peri_mr);
> +
> +/* Internal memory region for peripheral bus addresses (not exported) */
> +memory_region_init(>gpu_bus_mr, obj, "bcm2835-gpu", (uint64_t)1 << 
> 32);
> +object_property_add_child(obj, "gpu-bus", OBJECT(>gpu_bus_mr), NULL);
> +
> +/* Internal memory region for request/response communication with
> + * mailbox-addressable peripherals (not exported)
> + */
> +memory_region_init(>mbox_mr, obj, "bcm2835-mbox",
> +   MBOX_CHAN_COUNT << MBOX_AS_CHAN_SHIFT);
> +
> +/* Interrupt Controller */
> +object_initialize(>ic, sizeof(s->ic), TYPE_BCM2835_IC);
> +object_property_add_child(obj, "ic", OBJECT(>ic), NULL);
> +qdev_set_parent_bus(DEVICE(>ic), sysbus_get_default());
> +
> +/* UART0 */
> +s->uart0 = SYS_BUS_DEVICE(object_new("pl011"));
> +object_property_add_child(obj, "uart0", OBJECT(s->uart0), NULL);
> +qdev_set_parent_bus(DEVICE(s->uart0), sysbus_get_default());
> +
> +/* Mailboxes */
> +object_initialize(>mboxes, sizeof(s->mboxes), TYPE_BCM2835_MBOX);
> +object_property_add_child(obj, "mbox", OBJECT(>mboxes), NULL);
> +qdev_set_parent_bus(DEVICE(>mboxes), sysbus_get_default());
> +
> +object_property_add_const_link(OBJECT(>mboxes), "mbox-mr",
> +   OBJECT(>mbox_mr), _abort);
> +
> +/* Property channel */
> +object_initialize(>property, sizeof(s->property), 
> TYPE_BCM2835_PROPERTY);
> +object_property_add_child(obj, "property", OBJECT(>property), NULL);
> +qdev_set_parent_bus(DEVICE(>property), sysbus_get_default());
> +
> +object_property_add_const_link(OBJECT(>property), "dma-mr",
> +   OBJECT(>gpu_bus_mr), _abort);
> +
> +/* Extended Mass Media Controller */
> +object_initialize(>sdhci, sizeof(s->sdhci), TYPE_SYSBUS_SDHCI);
> +object_property_add_child(obj, "sdhci", OBJECT(>sdhci), NULL);
> +qdev_set_parent_bus(DEVICE(>sdhci), 

[Qemu-devel] [PATCH v3 4/7] bcm2835_peripherals: add rollup device for bcm2835 peripherals

[Andrew Baumann]

This device maintains all the non-CPU peripherals on bcm2835 (Pi1)
which are also present on bcm2836 (Pi2). It also implements the
private address spaces used for DMA and mailboxes.

Signed-off-by: Andrew Baumann 
---

Notes:
v3:
 * clean up raspi_platform.h
 * s/_/-/ in type/property/child names
 * use memory_region_init where appropriate rather than 
memory_region_init_io
 * pass ram as link property

v2 changes:
 * adapted to use common SDHCI emulation

 hw/arm/Makefile.objs |   1 +
 hw/arm/bcm2835_peripherals.c | 205 +++
 include/hw/arm/bcm2835_peripherals.h |  42 +++
 include/hw/arm/raspi_platform.h  | 128 ++
 4 files changed, 376 insertions(+)
 create mode 100644 hw/arm/bcm2835_peripherals.c
 create mode 100644 include/hw/arm/bcm2835_peripherals.h
 create mode 100644 include/hw/arm/raspi_platform.h

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 2195b60..82cc142 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -11,6 +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
 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/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
new file mode 100644
index 000..879a41d
--- /dev/null
+++ b/hw/arm/bcm2835_peripherals.c
@@ -0,0 +1,205 @@
+/*
+ * 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 "hw/arm/bcm2835_peripherals.h"
+#include "hw/misc/bcm2835_mbox_defs.h"
+#include "hw/arm/raspi_platform.h"
+
+/* Peripheral base address on the VC (GPU) system bus */
+#define BCM2835_VC_PERI_BASE 0x7e00
+
+/* Capabilities for SD controller: no DMA, high-speed, default clocks etc. */
+#define BCM2835_SDHC_CAPAREG 0x52034b4
+
+static void bcm2835_peripherals_init(Object *obj)
+{
+BCM2835PeripheralState *s = BCM2835_PERIPHERALS(obj);
+
+/* Memory region for peripheral devices, which we export to our parent */
+memory_region_init_io(>peri_mr, obj, NULL, s, "bcm2835-peripherals",
+  0x100);
+object_property_add_child(obj, "peripheral-io", OBJECT(>peri_mr), NULL);
+sysbus_init_mmio(SYS_BUS_DEVICE(s), >peri_mr);
+
+/* Internal memory region for peripheral bus addresses (not exported) */
+memory_region_init(>gpu_bus_mr, obj, "bcm2835-gpu", (uint64_t)1 << 32);
+object_property_add_child(obj, "gpu-bus", OBJECT(>gpu_bus_mr), NULL);
+
+/* Internal memory region for request/response communication with
+ * mailbox-addressable peripherals (not exported)
+ */
+memory_region_init(>mbox_mr, obj, "bcm2835-mbox",
+   MBOX_CHAN_COUNT << MBOX_AS_CHAN_SHIFT);
+
+/* Interrupt Controller */
+object_initialize(>ic, sizeof(s->ic), TYPE_BCM2835_IC);
+object_property_add_child(obj, "ic", OBJECT(>ic), NULL);
+qdev_set_parent_bus(DEVICE(>ic), sysbus_get_default());
+
+/* UART0 */
+s->uart0 = SYS_BUS_DEVICE(object_new("pl011"));
+object_property_add_child(obj, "uart0", OBJECT(s->uart0), NULL);
+qdev_set_parent_bus(DEVICE(s->uart0), sysbus_get_default());
+
+/* Mailboxes */
+object_initialize(>mboxes, sizeof(s->mboxes), TYPE_BCM2835_MBOX);
+object_property_add_child(obj, "mbox", OBJECT(>mboxes), NULL);
+qdev_set_parent_bus(DEVICE(>mboxes), sysbus_get_default());
+
+object_property_add_const_link(OBJECT(>mboxes), "mbox-mr",
+   OBJECT(>mbox_mr), _abort);
+
+/* Property channel */
+object_initialize(>property, sizeof(s->property), 
TYPE_BCM2835_PROPERTY);
+object_property_add_child(obj, "property", OBJECT(>property), NULL);
+qdev_set_parent_bus(DEVICE(>property), sysbus_get_default());
+
+object_property_add_const_link(OBJECT(>property), "dma-mr",
+   OBJECT(>gpu_bus_mr), _abort);
+
+/* Extended Mass Media Controller */
+object_initialize(>sdhci, sizeof(s->sdhci), TYPE_SYSBUS_SDHCI);
+object_property_add_child(obj, "sdhci", OBJECT(>sdhci), NULL);
+qdev_set_parent_bus(DEVICE(>sdhci), sysbus_get_default());
+}
+
+static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
+{
+BCM2835PeripheralState *s = BCM2835_PERIPHERALS(dev);
+Object *obj;
+MemoryRegion *ram;
+Error *err = NULL;
+uint32_t ram_size;
+int n;
+
+obj = object_property_get_link(OBJECT(dev), "ram", );
+