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(®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); _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot