Re: [U-Boot] [PATCH 1/2] ARM: bcm2835: add mailbox driver
Hi Stephen,
On Mon, 15 Oct 2012 23:10:35 -0600, Stephen Warren
swar...@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 swar...@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 000..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 000..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
+ *
Re: [U-Boot] [PATCH 1/2] ARM: bcm2835: add mailbox driver
On 10/18/2012 02:23 PM, Albert ARIBAUD wrote:
Hi Stephen,
On Mon, 15 Oct 2012 23:10:35 -0600, Stephen Warren
swar...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] ARM: bcm2835: add mailbox driver
Hi Stephen,
On Thu, 18 Oct 2012 14:34:27 -0600, Stephen Warren
swar...@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
swar...@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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
