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

[Stephen Warren]

On 06/11/2015 06:57 AM, Jassi Brar wrote:
> On Thu, Jun 11, 2015 at 12:25 AM, Eric Anholt <eric at anholt.net> wrote:
>> 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.
>>
> 1) The framework doesn't batch messages together, so the controller
> will ever have only one message to transmit at a time.
> 
> 
> 2) There seems nothing special about your '8-entry FIFO', there are
> controllers with even deeper fifos.  *** All data in FIFO is
> associated with one signal to/from remote ***
>  I just noticed the driver picks one entry from fifo as one message,
> which apparently works (because any message is exactly 32bits wide)
> but that is plain wrong. What if some platform passes 64 or 256 bits
> wide messages?  I strongly suggest you revisit that part of the code.
> Every other driver reads out the whole fifo as one packet received
> from remote (similarly for sending).

At least the RPi firmware protocol always passes 32-bit messages through
the mailbox itself. Those messages are often/typically/always(?)
pointers to memory structures that carry any larger data that's
required. So, I think there's zero risk of needing larger message in the
mailbox itself.