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

[Jassi Brar]

On Fri, Jun 12, 2015 at 9:22 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 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.
>
I already noted that the driver works on the platform it was developed
on. Agreed.

However, some other platform using this controller may need, say,
64-bit data along with shared-memory(SHM) message passing. Or if some
protocol is trivial enough, 256bits may be enough to avoid the need of
any SHM.
 There is some reason to most controllers having fifos many words deep.

Anyways, I will pick the driver as such, with your ack.