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

[Jassi Brar]

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