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

[Jassi Brar]

On Fri, May 29, 2015 at 12:36 PM, Jassi Brar <jassisinghbrar at gmail.com> wrote:
> 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.
>
Should this patch be taken as such or does it work the 'right' way --
checking the EMPTY status.