[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 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(&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);