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

[Jassi Brar]

On Wed, Jun 10, 2015 at 12:01 AM, Eric Anholt <eric at anholt.net> wrote:
> 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.
>
tx_done means if the last submitted tx was done. And if there was
exactly 1 tx submitted at any time, that means the status should be
checked for EMPTY.

But I understand either there is no EMPTY bit-filed to look at or you
are not going to do that. And since the code apparently already works,
I am going to merge the patch.