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

[Eric Anholt]

Jassi Brar <jassisinghbrar at gmail.com> writes:

> 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.

Since the tx_done hook is functionally asking "should I send the next
message?", the patch should be taken as is.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-rpi-kernel/attachments/20150609/b72d7c3d/attachment.sig>