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

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

[Eric Anholt]

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.

Suggested-by: Phil Elwell <phil@raspberrypi.org>
Signed-off-by: Eric Anholt <eric@anholt.net>
---

This is a port of a patch by Noralf from the downstream Raspberry Pi
tree.  Noralf said he didn't want attribution on this patch, even if
it looks a lot like his.

 drivers/mailbox/bcm2835-mailbox.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
index 4b13268..0b47dd4 100644
--- a/drivers/mailbox/bcm2835-mailbox.c
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -49,6 +49,7 @@
 #define MAIL0_STA	(ARM_0_MAIL0 + 0x18)
 #define MAIL0_CNF	(ARM_0_MAIL0 + 0x1C)
 #define MAIL1_WRT	(ARM_0_MAIL1 + 0x00)
+#define MAIL1_STA	(ARM_0_MAIL1 + 0x18)
 
 /* Status register: FIFO state. */
 #define ARM_MS_FULL		BIT(31)
@@ -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);
 	spin_unlock(&mbox->lock);
 	return ret;
 }
-- 
2.1.4

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

[Stephen Warren]

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

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

If "tx done" means "there's space to transmit more messages", then 
consider this:

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

... and I guess I'll need to fix U-Boot for the same issue.

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

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]

Stephen Warren <swarren@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?
>
> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

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

[Jassi Brar]

On Fri, May 29, 2015 at 3:16 AM, Eric Anholt <eric@anholt.net> wrote:
> Stephen Warren <swarren@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.

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

[Jassi Brar]

On Fri, May 29, 2015 at 2:15 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 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")
>
>> 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?
>
If only one message is active at any time we may have either FULL or
EMPTY status(?), so that might explain why this patch works as well.
But yes, it should check for emptiness ideally.