[U-Boot] [PATCH 1/2] ARM: bcm2835: add mailbox driver

[U-Boot] [PATCH 1/2] ARM: bcm2835: add mailbox driver

[Stephen Warren]

The BCM2835 SoC contains (at least) two CPUs; the VideoCore (a/k/a "GPU")
and the ARM CPU. The ARM CPU is often thought of as the main CPU.
However, the VideoCore actually controls the initial SoC boot, and hides
much of the hardware behind a protocol. This protocol is transported
using the SoC's mailbox hardware module. The mailbox supports two forms
of communication: Raw 28-bit values, and a so-called "property"
interface, where the 28-bit value is a physical pointer to a memory
buffer that contains various "tags".

Here, we add a very simplistic driver for the mailbox module, and define
a few structures for the property messages.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
 arch/arm/cpu/arm1176/bcm2835/Makefile    |    2 +-
 arch/arm/cpu/arm1176/bcm2835/mbox.c      |   92 ++++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-bcm2835/mbox.h |   87 ++++++++++++++++++++++++++++
 3 files changed, 180 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/cpu/arm1176/bcm2835/mbox.c
 create mode 100644 arch/arm/include/asm/arch-bcm2835/mbox.h

diff --git a/arch/arm/cpu/arm1176/bcm2835/Makefile b/arch/arm/cpu/arm1176/bcm2835/Makefile
index 95da6a8..135de42 100644
--- a/arch/arm/cpu/arm1176/bcm2835/Makefile
+++ b/arch/arm/cpu/arm1176/bcm2835/Makefile
@@ -17,7 +17,7 @@ include $(TOPDIR)/config.mk
 LIB	= $(obj)lib$(SOC).o
 
 SOBJS	:= lowlevel_init.o
-COBJS	:= init.o reset.o timer.o
+COBJS	:= init.o reset.o timer.o mbox.o
 
 SRCS	:= $(SOBJS:.o=.c) $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(SOBJS) $(COBJS))
diff --git a/arch/arm/cpu/arm1176/bcm2835/mbox.c b/arch/arm/cpu/arm1176/bcm2835/mbox.c
new file mode 100644
index 0000000..f5c060b
--- /dev/null
+++ b/arch/arm/cpu/arm1176/bcm2835/mbox.c
@@ -0,0 +1,92 @@
+/*
+ * (C) Copyright 2012 Stephen Warren
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/mbox.h>
+
+#define TIMEOUT (100 * 1000) /* 100mS in uS */
+
+int bcm2835_mbox_call_raw(u32 chan, u32 send, u32 *recv)
+{
+	struct bcm2835_mbox_regs *regs =
+		(struct bcm2835_mbox_regs *)BCM2835_MBOX_PHYSADDR;
+	ulong endtime = get_timer(0) + TIMEOUT;
+	u32 val;
+
+	if (send & BCM2835_CHAN_MASK)
+		return -1;
+
+	/* Drain any stale responses */
+
+	for (;;) {
+		val = readl(&regs->status);
+		if (val & BCM2835_MBOX_STATUS_RD_EMPTY)
+			break;
+		if (get_timer(0) >= endtime)
+			return -1;
+		val = readl(&regs->read);
+	}
+
+	/* Send the request */
+
+	/* FIXME: timeout */
+	for (;;) {
+		val = readl(&regs->status);
+		if (!(val & BCM2835_MBOX_STATUS_WR_FULL))
+			break;
+		if (get_timer(0) >= endtime)
+			return -1;
+	}
+
+	writel(BCM2835_MBOX_PACK(chan, send), &regs->write);
+
+	/* Wait for the response */
+
+	/* FIXME: timeout */
+	for (;;) {
+		val = readl(&regs->status);
+		if (!(val & BCM2835_MBOX_STATUS_RD_EMPTY))
+			break;
+		if (get_timer(0) >= endtime)
+			return -1;
+	}
+
+	val = readl(&regs->read);
+
+	/* Validate the response */
+
+	if (BCM2835_MBOX_UNPACK_CHAN(val) != chan)
+		return -1;
+
+	*recv = BCM2835_MBOX_UNPACK_DATA(val);
+
+	return 0;
+}
+
+int bcm2835_mbox_call_prop(u32 chan, struct bcm2835_mbox_prop_hdr *buffer)
+{
+	int ret;
+	u32 rbuffer;
+
+	ret = bcm2835_mbox_call_raw(chan, (u32)buffer, &rbuffer);
+	if (ret)
+		return ret;
+	if (rbuffer != (u32)buffer)
+		return -1;
+
+	return 0;
+}
diff --git a/arch/arm/include/asm/arch-bcm2835/mbox.h b/arch/arm/include/asm/arch-bcm2835/mbox.h
new file mode 100644
index 0000000..907fabd
--- /dev/null
+++ b/arch/arm/include/asm/arch-bcm2835/mbox.h
@@ -0,0 +1,87 @@
+/*
+ * (C) Copyright 2012 Stephen Warren
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _BCM2835_MBOX_H
+#define _BCM2835_MBOX_H
+
+#include <linux/compiler.h>
+
+/* Raw mailbox HW */
+
+#define BCM2835_MBOX_PHYSADDR	0x2000b880
+
+struct bcm2835_mbox_regs {
+	u32 read;
+	u32 rsvd0[5];
+	u32 status;
+	u32 config;
+	u32 write;
+};
+
+#define BCM2835_MBOX_STATUS_WR_FULL	0x80000000
+#define BCM2835_MBOX_STATUS_RD_EMPTY	0x40000000
+
+#define BCM2835_CHAN_MASK		0xf
+
+#define BCM2835_MBOX_PACK(chan, data)	(((data) & (~BCM2835_CHAN_MASK)) | \
+					 (chan & BCM2835_CHAN_MASK))
+#define BCM2835_MBOX_UNPACK_CHAN(val)	((val) & BCM2835_CHAN_MASK)
+#define BCM2835_MBOX_UNPACK_DATA(val)	((val) & (~BCM2835_CHAN_MASK))
+
+/* Property mailbox buffer structures */
+
+#define BCM2835_MBOX_PROP_CHAN		8
+
+struct bcm2835_mbox_prop_hdr {
+	u32 buf_size;
+	u32 code;
+} __packed;
+
+#define BCM2835_MBOX_REQ_CODE		0
+#define BCM2835_MBOX_RESP_CODE_SUCCESS	0x80000000
+
+struct bcm2835_mbox_tag_hdr {
+	u32 tag;
+	u32 val_buf_size;
+	u32 val_len;
+} __packed;
+
+#define BCM2835_MBOX_TAG_VAL_LEN_RESPONSE	0x80000000
+
+#define BCM2835_MBOX_TAG_GET_ARM_MEMORY		0x00010005
+
+struct bcm2835_mbox_req_get_arm_mem {
+} __packed;
+
+struct bcm2835_mbox_resp_get_arm_mem {
+	u32 mem_base;
+	u32 mem_size;
+} __packed;
+
+struct bcm2835_mbox_buf_get_arm_mem {
+	struct bcm2835_mbox_prop_hdr hdr;
+	struct bcm2835_mbox_tag_hdr tag_hdr;
+	union {
+		struct bcm2835_mbox_req_get_arm_mem req;
+		struct bcm2835_mbox_resp_get_arm_mem resp;
+	} body;
+	u32 end_tag;
+} __packed;
+
+int bcm2835_mbox_call_raw(u32 chan, u32 send, u32 *recv);
+int bcm2835_mbox_call_prop(u32 chan, struct bcm2835_mbox_prop_hdr *buffer);
+
+#endif
-- 
1.7.9.5

[U-Boot] [PATCH 2/2] ARM: rpi_b: use bcm2835 mbox driver to get memory size

[Stephen Warren]

The firmware running on the bcm2835 SoC's VideoCore CPU determines how
much of the system RAM is available for use by the ARM CPU. Previously,
U-Boot assumed that only 128MB was available, since this was the
smallest value configured by any public firmware. However, we can now
query the actual value at run-time from the firmware using the mbox
property protocol.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
 board/raspberrypi/rpi_b/rpi_b.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/board/raspberrypi/rpi_b/rpi_b.c b/board/raspberrypi/rpi_b/rpi_b.c
index 688b0aa..88f8c58 100644
--- a/board/raspberrypi/rpi_b/rpi_b.c
+++ b/board/raspberrypi/rpi_b/rpi_b.c
@@ -15,13 +15,31 @@
  */
 
 #include <common.h>
+#include <asm/arch/mbox.h>
 #include <asm/global_data.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
 int dram_init(void)
 {
-	gd->ram_size = CONFIG_SYS_SDRAM_SIZE;
+	ALLOC_ALIGN_BUFFER(struct bcm2835_mbox_buf_get_arm_mem, buf, 1, 16);
+
+	memset(buf, 0, sizeof(*buf));
+	buf->hdr.buf_size = sizeof(*buf);
+	buf->hdr.code = BCM2835_MBOX_REQ_CODE;
+	buf->tag_hdr.tag = BCM2835_MBOX_TAG_GET_ARM_MEMORY;
+	buf->tag_hdr.val_buf_size = sizeof(buf->body);
+	buf->tag_hdr.val_len = sizeof(buf->body.req);
+
+	bcm2835_mbox_call_prop(BCM2835_MBOX_PROP_CHAN, &buf->hdr);
+
+	if ((buf->hdr.code != BCM2835_MBOX_RESP_CODE_SUCCESS) ||
+	    (!(buf->tag_hdr.val_len & BCM2835_MBOX_TAG_VAL_LEN_RESPONSE))) {
+		printf("BCM2835_MBOX_TAG_GET_ARM_MEMORY failed\n");
+		return -1;
+	}
+
+	gd->ram_size = buf->body.resp.mem_size;
 
 	return 0;
 }
-- 
1.7.9.5

[U-Boot] [PATCH 1/2] ARM: bcm2835: add mailbox driver

[Albert ARIBAUD]

Hi Stephen,

On Mon, 15 Oct 2012 23:10:35 -0600, Stephen Warren
<swarren@wwwdotorg.org> wrote:

> The BCM2835 SoC contains (at least) two CPUs; the VideoCore (a/k/a "GPU")
> and the ARM CPU. The ARM CPU is often thought of as the main CPU.
> However, the VideoCore actually controls the initial SoC boot, and hides
> much of the hardware behind a protocol. This protocol is transported
> using the SoC's mailbox hardware module. The mailbox supports two forms
> of communication: Raw 28-bit values, and a so-called "property"
> interface, where the 28-bit value is a physical pointer to a memory
> buffer that contains various "tags".
> 
> Here, we add a very simplistic driver for the mailbox module, and define
> a few structures for the property messages.
> 
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
>  arch/arm/cpu/arm1176/bcm2835/Makefile    |    2 +-
>  arch/arm/cpu/arm1176/bcm2835/mbox.c      |   92 ++++++++++++++++++++++++++++++
>  arch/arm/include/asm/arch-bcm2835/mbox.h |   87 ++++++++++++++++++++++++++++
>  3 files changed, 180 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/cpu/arm1176/bcm2835/mbox.c
>  create mode 100644 arch/arm/include/asm/arch-bcm2835/mbox.h
> 
> diff --git a/arch/arm/cpu/arm1176/bcm2835/Makefile b/arch/arm/cpu/arm1176/bcm2835/Makefile
> index 95da6a8..135de42 100644
> --- a/arch/arm/cpu/arm1176/bcm2835/Makefile
> +++ b/arch/arm/cpu/arm1176/bcm2835/Makefile
> @@ -17,7 +17,7 @@ include $(TOPDIR)/config.mk
>  LIB	= $(obj)lib$(SOC).o
>  
>  SOBJS	:= lowlevel_init.o
> -COBJS	:= init.o reset.o timer.o
> +COBJS	:= init.o reset.o timer.o mbox.o
>  
>  SRCS	:= $(SOBJS:.o=.c) $(COBJS:.o=.c)
>  OBJS	:= $(addprefix $(obj),$(SOBJS) $(COBJS))
> diff --git a/arch/arm/cpu/arm1176/bcm2835/mbox.c b/arch/arm/cpu/arm1176/bcm2835/mbox.c
> new file mode 100644
> index 0000000..f5c060b
> --- /dev/null
> +++ b/arch/arm/cpu/arm1176/bcm2835/mbox.c
> @@ -0,0 +1,92 @@
> +/*
> + * (C) Copyright 2012 Stephen Warren
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <common.h>
> +#include <asm/io.h>
> +#include <asm/arch/mbox.h>
> +
> +#define TIMEOUT (100 * 1000) /* 100mS in uS */
> +
> +int bcm2835_mbox_call_raw(u32 chan, u32 send, u32 *recv)
> +{
> +	struct bcm2835_mbox_regs *regs =
> +		(struct bcm2835_mbox_regs *)BCM2835_MBOX_PHYSADDR;
> +	ulong endtime = get_timer(0) + TIMEOUT;
> +	u32 val;
> +
> +	if (send & BCM2835_CHAN_MASK)
> +		return -1;
> +
> +	/* Drain any stale responses */
> +
> +	for (;;) {
> +		val = readl(&regs->status);
> +		if (val & BCM2835_MBOX_STATUS_RD_EMPTY)
> +			break;
> +		if (get_timer(0) >= endtime)
> +			return -1;
> +		val = readl(&regs->read);
> +	}
> +
> +	/* Send the request */
> +
> +	/* FIXME: timeout */

Develop this FIXME to indicate what exactly should be fixed.

> +	for (;;) {
> +		val = readl(&regs->status);
> +		if (!(val & BCM2835_MBOX_STATUS_WR_FULL))
> +			break;
> +		if (get_timer(0) >= endtime)
> +			return -1;
> +	}
> +
> +	writel(BCM2835_MBOX_PACK(chan, send), &regs->write);
> +
> +	/* Wait for the response */
> +
> +	/* FIXME: timeout */

Ditto.

> +	for (;;) {
> +		val = readl(&regs->status);
> +		if (!(val & BCM2835_MBOX_STATUS_RD_EMPTY))
> +			break;
> +		if (get_timer(0) >= endtime)
> +			return -1;
> +	}
> +
> +	val = readl(&regs->read);
> +
> +	/* Validate the response */
> +
> +	if (BCM2835_MBOX_UNPACK_CHAN(val) != chan)
> +		return -1;
> +
> +	*recv = BCM2835_MBOX_UNPACK_DATA(val);
> +
> +	return 0;
> +}
> +
> +int bcm2835_mbox_call_prop(u32 chan, struct bcm2835_mbox_prop_hdr *buffer)
> +{
> +	int ret;
> +	u32 rbuffer;
> +
> +	ret = bcm2835_mbox_call_raw(chan, (u32)buffer, &rbuffer);
> +	if (ret)
> +		return ret;
> +	if (rbuffer != (u32)buffer)
> +		return -1;
> +
> +	return 0;
> +}
> diff --git a/arch/arm/include/asm/arch-bcm2835/mbox.h b/arch/arm/include/asm/arch-bcm2835/mbox.h
> new file mode 100644
> index 0000000..907fabd
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-bcm2835/mbox.h
> @@ -0,0 +1,87 @@
> +/*
> + * (C) Copyright 2012 Stephen Warren
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _BCM2835_MBOX_H
> +#define _BCM2835_MBOX_H
> +
> +#include <linux/compiler.h>
> +
> +/* Raw mailbox HW */
> +
> +#define BCM2835_MBOX_PHYSADDR	0x2000b880
> +
> +struct bcm2835_mbox_regs {
> +	u32 read;
> +	u32 rsvd0[5];
> +	u32 status;
> +	u32 config;
> +	u32 write;
> +};
> +
> +#define BCM2835_MBOX_STATUS_WR_FULL	0x80000000
> +#define BCM2835_MBOX_STATUS_RD_EMPTY	0x40000000
> +
> +#define BCM2835_CHAN_MASK		0xf
> +
> +#define BCM2835_MBOX_PACK(chan, data)	(((data) & (~BCM2835_CHAN_MASK)) | \
> +					 (chan & BCM2835_CHAN_MASK))
> +#define BCM2835_MBOX_UNPACK_CHAN(val)	((val) & BCM2835_CHAN_MASK)
> +#define BCM2835_MBOX_UNPACK_DATA(val)	((val) & (~BCM2835_CHAN_MASK))
> +
> +/* Property mailbox buffer structures */
> +
> +#define BCM2835_MBOX_PROP_CHAN		8
> +
> +struct bcm2835_mbox_prop_hdr {
> +	u32 buf_size;
> +	u32 code;
> +} __packed;

Remove __packed here, as all fields are 32 bits and thus no padding
would happen anyway.

> +#define BCM2835_MBOX_REQ_CODE		0
> +#define BCM2835_MBOX_RESP_CODE_SUCCESS	0x80000000
> +
> +struct bcm2835_mbox_tag_hdr {
> +	u32 tag;
> +	u32 val_buf_size;
> +	u32 val_len;
> +} __packed;

Ditto.

> +#define BCM2835_MBOX_TAG_VAL_LEN_RESPONSE	0x80000000
> +
> +#define BCM2835_MBOX_TAG_GET_ARM_MEMORY		0x00010005
> +
> +struct bcm2835_mbox_req_get_arm_mem {
> +} __packed;

Ditto (for an empty struct).

> +struct bcm2835_mbox_resp_get_arm_mem {
> +	u32 mem_base;
> +	u32 mem_size;
> +} __packed;

Ditto.

> +struct bcm2835_mbox_buf_get_arm_mem {
> +	struct bcm2835_mbox_prop_hdr hdr;
> +	struct bcm2835_mbox_tag_hdr tag_hdr;
> +	union {
> +		struct bcm2835_mbox_req_get_arm_mem req;
> +		struct bcm2835_mbox_resp_get_arm_mem resp;
> +	} body;
> +	u32 end_tag;
> +} __packed;

Ditto.

Also, what is the point of bcm2835_mbox_buf_get_arm_mem which is not
referenced in the API below?

> +int bcm2835_mbox_call_raw(u32 chan, u32 send, u32 *recv);
> +int bcm2835_mbox_call_prop(u32 chan, struct bcm2835_mbox_prop_hdr *buffer);
> +
> +#endif

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 2/2] ARM: rpi_b: use bcm2835 mbox driver to get memory size

[Albert ARIBAUD]

Hi Stephen,

On Mon, 15 Oct 2012 23:10:36 -0600, Stephen Warren
<swarren@wwwdotorg.org> wrote:

> The firmware running on the bcm2835 SoC's VideoCore CPU determines how
> much of the system RAM is available for use by the ARM CPU. Previously,
> U-Boot assumed that only 128MB was available, since this was the
> smallest value configured by any public firmware. However, we can now
> query the actual value at run-time from the firmware using the mbox
> property protocol.
> 
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
>  board/raspberrypi/rpi_b/rpi_b.c |   20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/board/raspberrypi/rpi_b/rpi_b.c b/board/raspberrypi/rpi_b/rpi_b.c
> index 688b0aa..88f8c58 100644
> --- a/board/raspberrypi/rpi_b/rpi_b.c
> +++ b/board/raspberrypi/rpi_b/rpi_b.c
> @@ -15,13 +15,31 @@
>   */
>  
>  #include <common.h>
> +#include <asm/arch/mbox.h>
>  #include <asm/global_data.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
>  int dram_init(void)
>  {
> -	gd->ram_size = CONFIG_SYS_SDRAM_SIZE;
> +	ALLOC_ALIGN_BUFFER(struct bcm2835_mbox_buf_get_arm_mem, buf, 1, 16);
> +
> +	memset(buf, 0, sizeof(*buf));
> +	buf->hdr.buf_size = sizeof(*buf);
> +	buf->hdr.code = BCM2835_MBOX_REQ_CODE;
> +	buf->tag_hdr.tag = BCM2835_MBOX_TAG_GET_ARM_MEMORY;
> +	buf->tag_hdr.val_buf_size = sizeof(buf->body);
> +	buf->tag_hdr.val_len = sizeof(buf->body.req);
> +
> +	bcm2835_mbox_call_prop(BCM2835_MBOX_PROP_CHAN, &buf->hdr);
> +
> +	if ((buf->hdr.code != BCM2835_MBOX_RESP_CODE_SUCCESS) ||
> +	    (!(buf->tag_hdr.val_len & BCM2835_MBOX_TAG_VAL_LEN_RESPONSE))) {
> +		printf("BCM2835_MBOX_TAG_GET_ARM_MEMORY failed\n");
> +		return -1;
> +	}
> +
> +	gd->ram_size = buf->body.resp.mem_size;
>  
>  	return 0;
>  }

Either move the buffer processing into mbox (as an opaque "ask mbox for
memory size" function) or move the memory request struct in here (this
is in order to have a self-consistent mbox interface where either the
mbox API does not know at all about the mem size request, or it knows
all about it).

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/2] ARM: bcm2835: add mailbox driver

[Stephen Warren]

On 10/18/2012 02:23 PM, Albert ARIBAUD wrote:
> Hi Stephen,
> 
> On Mon, 15 Oct 2012 23:10:35 -0600, Stephen Warren
> <swarren@wwwdotorg.org> wrote:
> 
>> The BCM2835 SoC contains (at least) two CPUs; the VideoCore (a/k/a "GPU")
>> and the ARM CPU. The ARM CPU is often thought of as the main CPU.
>> However, the VideoCore actually controls the initial SoC boot, and hides
>> much of the hardware behind a protocol. This protocol is transported
>> using the SoC's mailbox hardware module. The mailbox supports two forms
>> of communication: Raw 28-bit values, and a so-called "property"
>> interface, where the 28-bit value is a physical pointer to a memory
>> buffer that contains various "tags".
>>
>> Here, we add a very simplistic driver for the mailbox module, and define
>> a few structures for the property messages.

>> +++ b/arch/arm/cpu/arm1176/bcm2835/mbox.c

>> +int bcm2835_mbox_call_raw(u32 chan, u32 send, u32 *recv)

>> +	/* FIXME: timeout */
> 
> Develop this FIXME to indicate what exactly should be fixed.
> 
>> +	for (;;) {
>> +		val = readl(&regs->status);
>> +		if (!(val & BCM2835_MBOX_STATUS_WR_FULL))
>> +			break;
>> +		if (get_timer(0) >= endtime)
>> +			return -1;
>> +	}

The FIXME is actually already implemented by the last if test in the
loop; I simply forgot to remove the comment when I implemented it:-(

I'll repost with those removed. I've also learned a few things about
better constructing the message buffers while implementing a more
complex mbox client, so I might re-organize some of the tag structures
below a little too...

>> diff --git a/arch/arm/include/asm/arch-bcm2835/mbox.h b/arch/arm/include/asm/arch-bcm2835/mbox.h

>> +struct bcm2835_mbox_prop_hdr {
>> +	u32 buf_size;
>> +	u32 code;
>> +} __packed;
> 
> Remove __packed here, as all fields are 32 bits and thus no padding
> would happen anyway.

I'd prefer to keep it for consistency; see below.

>> +struct bcm2835_mbox_buf_get_arm_mem {
>> +	struct bcm2835_mbox_prop_hdr hdr;
>> +	struct bcm2835_mbox_tag_hdr tag_hdr;
>> +	union {
>> +		struct bcm2835_mbox_req_get_arm_mem req;
>> +		struct bcm2835_mbox_resp_get_arm_mem resp;
>> +	} body;
>> +	u32 end_tag;
>> +} __packed;
> 
> Ditto.

Here, multiple structs are packed into another struct. Isn't the
alignment requirement for a struct larger than for a u32, such that
without __packed, gaps may be left between those component structs and
unions if __packed isn't specified? I certainly added __packed during
development in order to make the code work, although it's possible there
was actually some other bug and I could have gone back and reverted
adding __packed...

Assuming __packed is needed here, I'd prefer to specify it for all
message buffer structs for consistency; that way, if someone chooses the
"wrong" thing to cut/paste, they'll still pick up the required __packed
attribute.

> Also, what is the point of bcm2835_mbox_buf_get_arm_mem which is not
> referenced in the API below?

The idea is that you'll actually pass a struct
bcm2835_mbox_buf_get_arm_mem (or one of tens of other message-specific
struct types) to bcm2835_mbox_call_prop, rather than just a message
header. I could use void * in the prototype below, but chose struct
bcm2835_mbox_prop_hdr as it is at least a requirement that all message
buffers start with that header.

>> +int bcm2835_mbox_call_raw(u32 chan, u32 send, u32 *recv);
>> +int bcm2835_mbox_call_prop(u32 chan, struct bcm2835_mbox_prop_hdr *buffer);

[U-Boot] [PATCH 2/2] ARM: rpi_b: use bcm2835 mbox driver to get memory size

[Stephen Warren]

On 10/18/2012 02:28 PM, Albert ARIBAUD wrote:
> Hi Stephen,
> 
> On Mon, 15 Oct 2012 23:10:36 -0600, Stephen Warren
> <swarren@wwwdotorg.org> wrote:
> 
>> The firmware running on the bcm2835 SoC's VideoCore CPU determines how
>> much of the system RAM is available for use by the ARM CPU. Previously,
>> U-Boot assumed that only 128MB was available, since this was the
>> smallest value configured by any public firmware. However, we can now
>> query the actual value at run-time from the firmware using the mbox
>> property protocol.
>>
>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>> ---
>>  board/raspberrypi/rpi_b/rpi_b.c |   20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/board/raspberrypi/rpi_b/rpi_b.c b/board/raspberrypi/rpi_b/rpi_b.c
>> index 688b0aa..88f8c58 100644
>> --- a/board/raspberrypi/rpi_b/rpi_b.c
>> +++ b/board/raspberrypi/rpi_b/rpi_b.c
>> @@ -15,13 +15,31 @@
>>   */
>>  
>>  #include <common.h>
>> +#include <asm/arch/mbox.h>
>>  #include <asm/global_data.h>
>>  
>>  DECLARE_GLOBAL_DATA_PTR;
>>  
>>  int dram_init(void)
>>  {
>> -	gd->ram_size = CONFIG_SYS_SDRAM_SIZE;
>> +	ALLOC_ALIGN_BUFFER(struct bcm2835_mbox_buf_get_arm_mem, buf, 1, 16);
>> +
>> +	memset(buf, 0, sizeof(*buf));
>> +	buf->hdr.buf_size = sizeof(*buf);
>> +	buf->hdr.code = BCM2835_MBOX_REQ_CODE;
>> +	buf->tag_hdr.tag = BCM2835_MBOX_TAG_GET_ARM_MEMORY;
>> +	buf->tag_hdr.val_buf_size = sizeof(buf->body);
>> +	buf->tag_hdr.val_len = sizeof(buf->body.req);
>> +
>> +	bcm2835_mbox_call_prop(BCM2835_MBOX_PROP_CHAN, &buf->hdr);
>> +
>> +	if ((buf->hdr.code != BCM2835_MBOX_RESP_CODE_SUCCESS) ||
>> +	    (!(buf->tag_hdr.val_len & BCM2835_MBOX_TAG_VAL_LEN_RESPONSE))) {
>> +		printf("BCM2835_MBOX_TAG_GET_ARM_MEMORY failed\n");
>> +		return -1;
>> +	}
>> +
>> +	gd->ram_size = buf->body.resp.mem_size;
>>  
>>  	return 0;
>>  }
> 
> Either move the buffer processing into mbox (as an opaque "ask mbox for
> memory size" function)

I don't like that idea, because soon there will be a bunch of other
message types (e.g. display configuration, power on/off), and multiple
"tags" can be packed into a single "message", and it should really be up
to the client driver to decide which sets of messages it sends at once,
not the mbox driver.

> ... or move the memory request struct in here (this
> is in order to have a self-consistent mbox interface where either the
> mbox API does not know at all about the mem size request, or it knows
> all about it).

I also don't entirely like that idea - what if multiple different
drivers want to send the same message (e.g. power on for I2C and power
on for SPI both use the same message structure, just with a different
device ID). Putting the message buffer definition into a single location
makes sense here.

I suppose one could define a "bcm2835 power" driver for that case, and
only define the message structure there? I guess if we go that route,
moving all the struct definitions out of the mbox driver would be
do-able, although I still like the idea of a single central list so it
can be easily matched up with the documentation...

[U-Boot] [PATCH 1/2] ARM: bcm2835: add mailbox driver

[Albert ARIBAUD]

Hi Stephen,

On Thu, 18 Oct 2012 14:34:27 -0600, Stephen Warren
<swarren@wwwdotorg.org> wrote:

> On 10/18/2012 02:23 PM, Albert ARIBAUD wrote:
> > Hi Stephen,
> > 
> > On Mon, 15 Oct 2012 23:10:35 -0600, Stephen Warren
> > <swarren@wwwdotorg.org> wrote:
> > 
> >> The BCM2835 SoC contains (at least) two CPUs; the VideoCore (a/k/a "GPU")
> >> and the ARM CPU. The ARM CPU is often thought of as the main CPU.
> >> However, the VideoCore actually controls the initial SoC boot, and hides
> >> much of the hardware behind a protocol. This protocol is transported
> >> using the SoC's mailbox hardware module. The mailbox supports two forms
> >> of communication: Raw 28-bit values, and a so-called "property"
> >> interface, where the 28-bit value is a physical pointer to a memory
> >> buffer that contains various "tags".
> >>
> >> Here, we add a very simplistic driver for the mailbox module, and define
> >> a few structures for the property messages.
> 
> >> +++ b/arch/arm/cpu/arm1176/bcm2835/mbox.c
> 
> >> +int bcm2835_mbox_call_raw(u32 chan, u32 send, u32 *recv)
> 
> >> +	/* FIXME: timeout */
> > 
> > Develop this FIXME to indicate what exactly should be fixed.
> > 
> >> +	for (;;) {
> >> +		val = readl(&regs->status);
> >> +		if (!(val & BCM2835_MBOX_STATUS_WR_FULL))
> >> +			break;
> >> +		if (get_timer(0) >= endtime)
> >> +			return -1;
> >> +	}
> 
> The FIXME is actually already implemented by the last if test in the
> loop; I simply forgot to remove the comment when I implemented it:-(
> 
> I'll repost with those removed. I've also learned a few things about
> better constructing the message buffers while implementing a more
> complex mbox client, so I might re-organize some of the tag structures
> below a little too...
> 
> >> diff --git a/arch/arm/include/asm/arch-bcm2835/mbox.h b/arch/arm/include/asm/arch-bcm2835/mbox.h
> 
> >> +struct bcm2835_mbox_prop_hdr {
> >> +	u32 buf_size;
> >> +	u32 code;
> >> +} __packed;
> > 
> > Remove __packed here, as all fields are 32 bits and thus no padding
> > would happen anyway.
> 
> I'd prefer to keep it for consistency; see below.
> 
> >> +struct bcm2835_mbox_buf_get_arm_mem {
> >> +	struct bcm2835_mbox_prop_hdr hdr;
> >> +	struct bcm2835_mbox_tag_hdr tag_hdr;
> >> +	union {
> >> +		struct bcm2835_mbox_req_get_arm_mem req;
> >> +		struct bcm2835_mbox_resp_get_arm_mem resp;
> >> +	} body;
> >> +	u32 end_tag;
> >> +} __packed;
> > 
> > Ditto.
> 
> Here, multiple structs are packed into another struct. Isn't the
> alignment requirement for a struct larger than for a u32, such that
> without __packed, gaps may be left between those component structs and
> unions if __packed isn't specified? I certainly added __packed during
> development in order to make the code work, although it's possible there
> was actually some other bug and I could have gone back and reverted
> adding __packed...
> 
> Assuming __packed is needed here, I'd prefer to specify it for all
> message buffer structs for consistency; that way, if someone chooses the
> "wrong" thing to cut/paste, they'll still pick up the required __packed
> attribute.

Strictly speaking, the alignment of a struct is that of its first
member (recursively if that member is a struct). This is according to
the C99 TC3 (last publically available draft), section 67.2.1, 
paragraph 14. In practice, struct members are aligned within the
struct, so to maintain these alignments, the struct alignment must be
the greater of the alignments of its members. Thus here it can never go
beyond u32.

> > Also, what is the point of bcm2835_mbox_buf_get_arm_mem which is not
> > referenced in the API below?
> 
> The idea is that you'll actually pass a struct
> bcm2835_mbox_buf_get_arm_mem (or one of tens of other message-specific
> struct types) to bcm2835_mbox_call_prop, rather than just a message
> header. I could use void * in the prototype below, but chose struct
> bcm2835_mbox_prop_hdr as it is at least a requirement that all message
> buffers start with that header.
> 
> >> +int bcm2835_mbox_call_raw(u32 chan, u32 send, u32 *recv);
> >> +int bcm2835_mbox_call_prop(u32 chan, struct bcm2835_mbox_prop_hdr *buffer);

Understood. This, plus your anwser to 2/2, leads me to asking for a 3rd
option: that the mbox driver header file make it clear (through
comments) how this struct (and future others) is used.

Amicalement,
-- 
Albert.