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

[Jassi Brar]

On Fri, May 29, 2015 at 3:16 AM, Eric Anholt <eric at anholt.net> wrote:
> Stephen Warren <swarren at wwwdotorg.org> 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?
>>
Mailbox core queues messages internally. The controller driver is
provided one message at a time.

>> 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.
>
'tx_done'  => 'is tx done' / 'tx is done'.  'tx' is the last submitted
message to be transferred.
So 'tx_done' marks if the last transmitted message was received by the
remote. That event could be indicated by some register's bit cleared
or even a message(ack) returned by remote (client does
mbox_client_txdone).  And if last tx was done, we should be able to
send next message.