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

[Albert ARIBAUD]

Hi Stephen,

On Thu, 18 Oct 2012 14:34:27 -0600, Stephen Warren
<swarren at 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 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.

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.