Re: [PATCH] mailbox/bcm2835: Fix mailbox full detection.

[Eric Anholt]

Stephen Warren <swarren@xxxxxxxxxxxxx> writes:

> On 05/13/2015 02:10 PM, Eric Anholt wrote:
>> With the VC reader blocked and the ARM writing, MAIL0_STA reads empty
>> permanently while MAIL1_STA goes from empty (0x40000000) to non-empty
>> (0x00000001-0x00000007) to full (0x80000008).
>>
>> This bug ended up having no effect on us, because all of our
>> transactions in the client driver were synchronous and under a mutex.
>
> If you could get someone at the RPi Foundation or Broadcom to update the 
> register descriptions and example code at the following URLs, that would 
> be rather useful. Otherwise, this code will appear incorrect when 
> compared against the documentation:
>
> https://github.com/raspberrypi/firmware/wiki/Mailboxes
> ("Mailbox registers" at the bottom)
>
> https://github.com/raspberrypi/firmware/wiki/Accessing-mailboxes
> ("Sample code")

Since it's a wiki, I went ahead and edited the first one.  Hopefully
that clarifies how the c++ in the other page is supposed to be used.

>> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
>
>> @@ -117,7 +118,7 @@ static bool bcm2835_last_tx_done(struct mbox_chan *link)
>>   	bool ret;
>>
>>   	spin_lock(&mbox->lock);
>> -	ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);
>> +	ret = !(readl(mbox->regs + MAIL1_STA) & ARM_MS_FULL);
>
> What does "tx done" mean semantically?
>
> If "tx done" means "remote side received all our messages", then surely 
> this should check MAIL1_STA for emptiness, which is different to the 
> "not full" check implemented here?
>
> If "tx done" means "there's space to transmit more messages", then 
> consider this:

The mailbox core appears to use this hook as "there's space to transmit
more messages."  The name does seem really confusing.

Attachment: signature.asc
Description: PGP signature