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

[Eric Anholt]

Jassi Brar <jassisinghbrar at gmail.com> writes:

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

Thanks!

There is an empty bit (see the top of the file), but we're supposed to
be able to have multiple messages outstanding in our 8-entry FIFO, and
it may be imporant to do so for future higher-bandwidth protocols.  The
transmit is successfully done the moment we write the data register.
-------------- 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/20150610/e7989455/attachment.sig>