[U-Boot] [PATCH 1/2] ARM: bcm2835: add mailbox driver
Stephen Warren
swarren at wwwdotorg.org
Thu Oct 18 22:34:27 CEST 2012
On 10/18/2012 02:23 PM, Albert ARIBAUD wrote:
> Hi Stephen,
>
> On Mon, 15 Oct 2012 23:10:35 -0600, Stephen Warren
> <swarren at 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(®s->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);
More information about the U-Boot
mailing list