[PATCH 1/4] ARM: BCM2835: Add a function for doing an rmb() between device reads.

[PATCH 1/4] ARM: BCM2835: Add a function for doing an rmb() between device reads.

[Stephen Warren]

On 03/12/2015 08:32 PM, Eric Anholt wrote:
> Stephen Warren was concerned that the rmb() present in the new mailbox
> driver was unnecessary, and after seeing the docs, that it was just so
> surprising that somebody would come along and remove it later.  The
> explanation for the need for the rmb() is long enough that we won't
> want to place it at every callsite.  Make a wrapper with the whole
> explanation in it, so that anyone wondering what's going on sees the
> docs right there.

> diff --git a/include/soc/bcm2835/peripheral-workaround.h b/include/soc/bcm2835/peripheral-workaround.h

> +static inline void bcm2835_peripheral_read_workaround(void)
> +{
> +#ifdef CONFIG_ARCH_BCM2835

Would this header be included if that wasn't defined? Perhaps that'll be
answered by a later patch...

> +	/*
> +	 * The BCM2835 bus is unusual in that it doesn't guarantee
> +	 * ordering between reads from different peripherals (where
> +	 * peripherals roughly correspond to Linux devices).  From
> +	 * BCM2835 ARM Peripherals.pdf, page 7:

Many buses don't guarantee ordering; that's quite common. The issue is
that the CPU then doesn't match up the correct read request and
response, thus causing it to swap the results of read requests. That's
the unusual part. It would be useful to spell that out more explicitly
in this introduction, even though it is called out in the example below.

BTW, the ARM mailing list is linux-arm-kernel at lists.infradead.org not
linux-arm-kernel at vger.kernel.org.

[PATCH 1/4] ARM: BCM2835: Add a function for doing an rmb() between device reads.

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>> Stephen Warren was concerned that the rmb() present in the new mailbox
>> driver was unnecessary, and after seeing the docs, that it was just so
>> surprising that somebody would come along and remove it later.  The
>> explanation for the need for the rmb() is long enough that we won't
>> want to place it at every callsite.  Make a wrapper with the whole
>> explanation in it, so that anyone wondering what's going on sees the
>> docs right there.
>
>> diff --git a/include/soc/bcm2835/peripheral-workaround.h b/include/soc/bcm2835/peripheral-workaround.h
>
>> +static inline void bcm2835_peripheral_read_workaround(void)
>> +{
>> +#ifdef CONFIG_ARCH_BCM2835
>
> Would this header be included if that wasn't defined? Perhaps that'll be
> answered by a later patch...

Well, we may find we need workaround rmb()s in, say, dwc2.  I don't
think we'd want to have them unconditional on other architectures when
bcm2835 isn't included in the build.

>> +	/*
>> +	 * The BCM2835 bus is unusual in that it doesn't guarantee
>> +	 * ordering between reads from different peripherals (where
>> +	 * peripherals roughly correspond to Linux devices).  From
>> +	 * BCM2835 ARM Peripherals.pdf, page 7:
>
> Many buses don't guarantee ordering; that's quite common. The issue is
> that the CPU then doesn't match up the correct read request and
> response, thus causing it to swap the results of read requests. That's
> the unusual part. It would be useful to spell that out more explicitly
> in this introduction, even though it is called out in the example below.
>
> BTW, the ARM mailing list is linux-arm-kernel at lists.infradead.org not
> linux-arm-kernel at vger.kernel.org.

Fixed in my send-email script, thanks!
-------------- 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-arm-kernel/attachments/20150317/34fcb781/attachment-0001.sig>

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/12/2015 08:32 PM, Eric Anholt wrote:
> From: Lubomir Rintel <lkundrak@v3.sk>
> 
> Implement BCM2835 mailbox support as a device registered with the
> general purpose mailbox framework. Implementation based on commits by
> Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the
> implementation.
> 
> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Cc: Jassi Brar <jassisinghbrar@gmail.com>
> Acked-by: Lee Jones <lee.jones@linaro.org>

Acks often don't carry over when there are significant changes.

> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c

> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
> +#define MBOX_CHAN(msg)			((msg) & 0xf)
> +#define MBOX_DATA28(msg)		((msg) & ~0xf)

Even the concept of storing channel IDs in the LSBs feels like it might
be RPi-firmware-specific rather than HW-specific?

> +static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
> +{
> +	struct bcm2835_mbox *mbox = (struct bcm2835_mbox *) dev_id;
> +	struct device *dev = mbox->dev;
> +
> +	bcm2835_peripheral_read_workaround();
> +
> +	while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
> +		u32 msg = readl(mbox->regs + MAIL0_RD);
> +		unsigned int chan = MBOX_CHAN(msg);
> +		u32 data = MBOX_DATA28(msg);
> +
> +		if (!mbox->channel[chan].started) {
> +			dev_err(dev, "Reply on stopped channel %d\n", chan);
> +			continue;
> +		}
> +		dev_dbg(dev, "Reply 0x%08X\n", msg);

I think for complete safety, you'd need to put the peripheral workaround
call here too. Consider what happens if some module registers to receive
mailbox responses, then e.g. changes a GPIO right in the callback. Are
we going to add the workaround call to the bcm2835 GPIO, SDHCI, and USB
drivers too? If so, that might obviate the need to perform the
workaround here.

Either way though, wouldn't we need to at least move the workaround
that's already present in this function so that it happens before the
readl() inside the while() above every time, to handle the switch from
any HW access inside mbox_chan_received_data() to the HW access to the
mbox module here?

> +		mbox_chan_received_data(mbox->channel[chan].link, &data);
> +	}
> +	return IRQ_HANDLED;
> +}

> +static int bcm2835_mbox_probe(struct platform_device *pdev)
...
> +	/* Enable the interrupt on data reception */
> +	writel(ARM_MC_IHAVEDATAIRQEN, mbox->regs + MAIL0_CNF);

I think that bcm2835_mbox_remove() needs to undo that, so that it
guarantees the IRQ handler won't be called after the function returns,
but before the devm IRQ unregister occurs.

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Lee Jones]

On Mon, 16 Mar 2015, Stephen Warren wrote:

> On 03/12/2015 08:32 PM, Eric Anholt wrote:
> > From: Lubomir Rintel <lkundrak@v3.sk>
> > 
> > Implement BCM2835 mailbox support as a device registered with the
> > general purpose mailbox framework. Implementation based on commits by
> > Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the
> > implementation.
> > 
> > [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
> > [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
> > Signed-off-by: Suman Anna <s-anna@ti.com>
> > Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
> > Signed-off-by: Eric Anholt <eric@anholt.net>
> > Cc: Jassi Brar <jassisinghbrar@gmail.com>
> > Acked-by: Lee Jones <lee.jones@linaro.org>
> 
> Acks often don't carry over when there are significant changes.

Did I even Ack this?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Lee Jones <lee.jones@linaro.org> writes:

> On Mon, 16 Mar 2015, Stephen Warren wrote:
>
>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>> > From: Lubomir Rintel <lkundrak@v3.sk>
>> > 
>> > Implement BCM2835 mailbox support as a device registered with the
>> > general purpose mailbox framework. Implementation based on commits by
>> > Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the
>> > implementation.
>> > 
>> > [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
>> > [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>> > 
>> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>> > Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
>> > Signed-off-by: Suman Anna <s-anna@ti.com>
>> > Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
>> > Signed-off-by: Eric Anholt <eric@anholt.net>
>> > Cc: Jassi Brar <jassisinghbrar@gmail.com>
>> > Acked-by: Lee Jones <lee.jones@linaro.org>
>> 
>> Acks often don't carry over when there are significant changes.
>
> Did I even Ack this?

Yeah, I don't see any record of it, looks like I misread the mail
thread.
-------------- 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-arm-kernel/attachments/20150317/68198f4c/attachment.sig>

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
>
>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) & 0xf))
>> +#define MBOX_CHAN(msg)			((msg) & 0xf)
>> +#define MBOX_DATA28(msg)		((msg) & ~0xf)
>
> Even the concept of storing channel IDs in the LSBs feels like it might
> be RPi-firmware-specific rather than HW-specific?

I guess?  If we found another firmware protocol, we could have that
device's dt just specify a different compatible string.  But in the
absence of another firmware to talk to, I'm not sure what you want here.

Note that Roku's kernel code dump doesn't even communicate through the
mailbox.  vcio.c exists, but is disconnected from the build.
-------------- 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-arm-kernel/attachments/20150318/40d8a5ac/attachment.sig>

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/18/2015 05:28 PM, Eric Anholt wrote:
> Stephen Warren <swarren@wwwdotorg.org> writes:
> 
>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>> b/drivers/mailbox/bcm2835-mailbox.c
>> 
>>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &
>>> 0xf)) +#define MBOX_CHAN(msg)			((msg) & 0xf) +#define
>>> MBOX_DATA28(msg)		((msg) & ~0xf)
>> 
>> Even the concept of storing channel IDs in the LSBs feels like it
>> might be RPi-firmware-specific rather than HW-specific?
> 
> I guess?  If we found another firmware protocol, we could have
> that device's dt just specify a different compatible string.  But
> in the absence of another firmware to talk to, I'm not sure what
> you want here.

I would expect the mailbox driver to expose a single channel that just
transports 32-bit values, since the HW doesn't impose any kind of
structure on the values it transports AFAIK. Clients of the mailbox
driver would formulate the messages they send through the mailox using
the macros above.

I'm not sure whether the mailbox core allows multiple clients for the
same mailbox channel though? This HW appears to require it.

> Note that Roku's kernel code dump doesn't even communicate through
> the mailbox.  vcio.c exists, but is disconnected from the build.

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Jassi Brar]

On Fri, Mar 20, 2015 at 10:18 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>> Stephen Warren <swarren@wwwdotorg.org> writes:
>>
>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>>
>>>> +#define MBOX_MSG(chan, data28)             (((data28) & ~0xf) | ((chan) &
>>>> 0xf)) +#define MBOX_CHAN(msg)                       ((msg) & 0xf) +#define
>>>> MBOX_DATA28(msg)            ((msg) & ~0xf)
>>>
>>> Even the concept of storing channel IDs in the LSBs feels like it
>>> might be RPi-firmware-specific rather than HW-specific?
>>
>> I guess?  If we found another firmware protocol, we could have
>> that device's dt just specify a different compatible string.  But
>> in the absence of another firmware to talk to, I'm not sure what
>> you want here.
>
> I would expect the mailbox driver to expose a single channel that just
> transports 32-bit values, since the HW doesn't impose any kind of
> structure on the values it transports AFAIK. Clients of the mailbox
> driver would formulate the messages they send through the mailox using
> the macros above.
>
Yes, it should be so.

> I'm not sure whether the mailbox core allows multiple clients for the
> same mailbox channel though? This HW appears to require it.
>
There were tradeoffs to granting shared vs exclusive access, we chose
latter. The platform could have a global client that serializes
requests and dispatch received data to exact destination rather than
mbox core iterating over all users of the channel. Some platform may
require to execute 2 or more commands 'atomically', which would be
messy if channels are shared.

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Jassi Brar <jassisinghbrar@gmail.com> writes:

> On Fri, Mar 20, 2015 at 10:18 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>>> Stephen Warren <swarren@wwwdotorg.org> writes:
>>>
>>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>>>
>>>>> +#define MBOX_MSG(chan, data28)             (((data28) & ~0xf) | ((chan) &
>>>>> 0xf)) +#define MBOX_CHAN(msg)                       ((msg) & 0xf) +#define
>>>>> MBOX_DATA28(msg)            ((msg) & ~0xf)
>>>>
>>>> Even the concept of storing channel IDs in the LSBs feels like it
>>>> might be RPi-firmware-specific rather than HW-specific?
>>>
>>> I guess?  If we found another firmware protocol, we could have
>>> that device's dt just specify a different compatible string.  But
>>> in the absence of another firmware to talk to, I'm not sure what
>>> you want here.
>>
>> I would expect the mailbox driver to expose a single channel that just
>> transports 32-bit values, since the HW doesn't impose any kind of
>> structure on the values it transports AFAIK. Clients of the mailbox
>> driver would formulate the messages they send through the mailox using
>> the macros above.
>>
> Yes, it should be so.

I'm getting pretty frustrated here.

So, if I build a global client serializing requests and stop presenting
channels, what exactly is the generic mailbox infrastructure gaining me?
I don't need add_to_rbuf.  I don't need tpoll_txdone.  I don't need
tx_tick.  I don't need peek_data.  I don't need channels.  It doesn't
even handle waiting on requests for me, and I keep having to do it in
clients.  There's nothing left that the generic code is doing for me.

If I have to do any more changes ot this driver (we're at 27 hours of
just my work on it so far...), I'd rather go back to the trivial driver
we had before that simply, obviously did what we wanted.
-------------- 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-arm-kernel/attachments/20150320/c18c25a5/attachment.sig>

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Eric Anholt]

Stephen Warren <swarren@wwwdotorg.org> writes:

> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>> Stephen Warren <swarren@wwwdotorg.org> writes:
>> 
>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>> 
>>>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &
>>>> 0xf)) +#define MBOX_CHAN(msg)			((msg) & 0xf) +#define
>>>> MBOX_DATA28(msg)		((msg) & ~0xf)
>>> 
>>> Even the concept of storing channel IDs in the LSBs feels like it
>>> might be RPi-firmware-specific rather than HW-specific?
>> 
>> I guess?  If we found another firmware protocol, we could have
>> that device's dt just specify a different compatible string.  But
>> in the absence of another firmware to talk to, I'm not sure what
>> you want here.
>
> I would expect the mailbox driver to expose a single channel that just
> transports 32-bit values, since the HW doesn't impose any kind of
> structure on the values it transports AFAIK. Clients of the mailbox
> driver would formulate the messages they send through the mailox using
> the macros above.
>
> I'm not sure whether the mailbox core allows multiple clients for the
> same mailbox channel though? This HW appears to require it.

Yeah, that's the problem.
-------------- 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-arm-kernel/attachments/20150320/bb6c83cb/attachment-0001.sig>

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/20/2015 11:24 AM, Eric Anholt wrote:
> Stephen Warren <swarren@wwwdotorg.org> writes:
>
>> On 03/18/2015 05:28 PM, Eric Anholt wrote:
>>> Stephen Warren <swarren@wwwdotorg.org> writes:
>>>
>>>> On 03/12/2015 08:32 PM, Eric Anholt wrote:
>>>>> diff --git a/drivers/mailbox/bcm2835-mailbox.c
>>>>> b/drivers/mailbox/bcm2835-mailbox.c
>>>>
>>>>> +#define MBOX_MSG(chan, data28)		(((data28) & ~0xf) | ((chan) &
>>>>> 0xf)) +#define MBOX_CHAN(msg)			((msg) & 0xf) +#define
>>>>> MBOX_DATA28(msg)		((msg) & ~0xf)
>>>>
>>>> Even the concept of storing channel IDs in the LSBs feels like it
>>>> might be RPi-firmware-specific rather than HW-specific?
>>>
>>> I guess?  If we found another firmware protocol, we could have
>>> that device's dt just specify a different compatible string.  But
>>> in the absence of another firmware to talk to, I'm not sure what
>>> you want here.
>>
>> I would expect the mailbox driver to expose a single channel that just
>> transports 32-bit values, since the HW doesn't impose any kind of
>> structure on the values it transports AFAIK. Clients of the mailbox
>> driver would formulate the messages they send through the mailox using
>> the macros above.
>>
>> I'm not sure whether the mailbox core allows multiple clients for the
>> same mailbox channel though? This HW appears to require it.
>
> Yeah, that's the problem.

I expect you'd end up representing the low-level mbox HW and the remote 
firmware as separate nodes in DT. There's certainly precedent for 
representing firmware in DT as a separate node. Perhaps something like:

// Exports just one channel, since there's 1 channel in HW
// Driver solely transports u32s through the registers and nothing else
// Driver allows a single client on the channel
mailbox: mailbox at 7e00b800 {
	compatible = "brcm,bcm2835-mbox";
	reg = <0x7e00b880 0x40>;
	interrupts = <0 1>;
	#mbox-cells = <1>;
};

// implements all RPI- (rather than bcm2835-)specific aspects of the
// firmware interface, such as merging data and channel ID into the data
// sent into the mailbox registers, handling multiple clients on a
// channel, etc.
firmware {
	compatible = "raspberrypi,firmware";
	mboxes = <&mailbox 8>;
};

The driver for the FW node could either register itself with all the 
relevant subsystems (power domains, clocks, ...) or in turn exports its 
services to other nodes like:

   firmware {
   	compatible = "raspberrypi,firmware";
   	mboxes = <&mailbox 8>;
+ 	#fw-cells = <1>; # or whatever's needed
   };

clocks {
	compatible = "raspberrypi,clocks";
	fw = <&firmware 0>; // or whatever channel # clocks are on
};

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/18/2015 04:39 PM, Eric Anholt wrote:
> Lee Jones <lee@kernel.org> writes:
> 
>> On Thu, 12 Mar 2015, Eric Anholt wrote:
>> 
>>> From: Lubomir Rintel <lkundrak@v3.sk>
>>> 
>>> Implement BCM2835 mailbox support as a device registered with
>>> the general purpose mailbox framework. Implementation based on
>>> commits by Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on
>>> which to base the implementation.
>>> 
>>> [1]
>>> http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
>>>
>>> 
[2]
http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>>> 
>>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk> Signed-off-by:
>>> Craig McGeachie <slapdau@yahoo.com.au> Signed-off-by: Suman
>>> Anna <s-anna@ti.com> Signed-off-by: Jassi Brar
>>> <jassisinghbrar@gmail.com> Signed-off-by: Eric Anholt
>>> <eric@anholt.net> Cc: Jassi Brar <jassisinghbrar@gmail.com> 
>>> Acked-by: Lee Jones <lee.jones@linaro.org> ---
>>> 
>>> 
>>> v2: Squashed Craig's work for review, carried over to new
>>> version of Mailbox framework (changes by Lubomir)
>>> 
>>> v3: Fix multi-line comment style.  Refer to the documentation
>>> by filename.  Only declare one MODULE_AUTHOR.  Alphabetize
>>> includes. Drop some excessive dev_dbg()s (changes by anholt).
>>> 
>>> v4: Use the new bcm2835_peripheral_read_workaround(), drop the
>> 
>> Can you explain to me why this is required (and don't just point
>> me in the direction of the other patch ;) ).  You appear to be
>> using the non-relaxed variants of readl and writel, which already
>> do memory barriers, so I'm a little perplexed as to how the
>> problem can arise.
> 
> Hmm.
> 
> A shorter restatement of the architecture requirement would be, I
> think, "Don't let there be two outstanding reads of different
> peripherals on the AXI bus, or the CPU might mis-assign the read
> results.  Use rmb() to wait for the previous bus reads when you
> need to prevent this"
> 
> arch/arm/include/asm/io.h's readl() does __iormb() after each 
> __raw_readl().  Imagine taking an interrupt for a new peripheral
> between the driver's __raw_readl() and the following __iormb(): Now
> you've got two __raw_readl()s in between iormb()s and you can
> theoretically get unordered reads.
> 
> We could hope that the architecture IRQ handler would happen to do
> an incidental rmb(), resolving the need to protect from interrupt
> handling inside of device drivers.  The interrupt controller's
> presence at 0x7e00b200 sounds like it's an AXI peripheral, so it
> would need to be ensuring ordering of reads.  However, it's doing
> readl_relaxed()s.  So my rmb() at the start of my irq handler is
> silly -- if somebody got interrupted between readl and rmb, we've
> already had a chance to get the wrong result inside of the IRQ
> chip's status read.
> 
> My new idea for handling this would be to:
> 
> 1) Assume drivers don't exit with reads outstanding.  This means
> they don't do a readl_relaxed() from an AXI peripheral at the end
> of a path without doing something with the result.
> 
> 2) Make bcm2835_handle_irq() do this rmb() at the top, with the
> big explanation, to avoid a race against the interrupted code
> device being inside a readl() before the __iormb().  We don't worry
> about the 1-2 readl_relaxed()s inside of bcm2835_handle_irq(),
> because their return values get waited on before continuing on to
> calling the device driver, so the device driver knows its IRQ
> handler is being entered with no AXI reads outstanding.

I /think/ that sounds reasonable. I suppose if we do find any code
that initiates a read but doesn't use the result before returning or
calling into some other code, we can always patch that up with an
extra barrier at that time.

[PATCH 3/4 v4] mailbox: Enable BCM2835 mailbox support

[Stephen Warren]

On 03/19/2015 01:58 AM, Lee Jones wrote:
> On Wed, 18 Mar 2015, Eric Anholt wrote:
> 
>> Lee Jones <lee@kernel.org> writes:
>>
>>> On Thu, 12 Mar 2015, Eric Anholt wrote:
>>>
>>>> From: Lubomir Rintel <lkundrak@v3.sk>
>>>>
>>>> Implement BCM2835 mailbox support as a device registered with the
>>>> general purpose mailbox framework. Implementation based on commits by
>>>> Lubomir Rintel [1], Suman Anna and Jassi Brar [2] on which to base the
>>>> implementation.
>>>>
>>>> [1] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-April/000528.html
>>>> [2] http://lists.infradead.org/pipermail/linux-rpi-kernel/2013-May/000546.html
>>>>
>>>> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>>>> Signed-off-by: Craig McGeachie <slapdau@yahoo.com.au>
>>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>>> Signed-off-by: Jassi Brar <jassisinghbrar@gmail.com>
>>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>>> Cc: Jassi Brar <jassisinghbrar@gmail.com>
>>>> Acked-by: Lee Jones <lee.jones@linaro.org>
>>>> ---
>>>>
>>>>
>>>> v2: Squashed Craig's work for review, carried over to new version of
>>>>     Mailbox framework (changes by Lubomir)
>>>>
>>>> v3: Fix multi-line comment style.  Refer to the documentation by
>>>>     filename.  Only declare one MODULE_AUTHOR.  Alphabetize includes.
>>>>     Drop some excessive dev_dbg()s (changes by anholt).
>>>>
>>>> v4: Use the new bcm2835_peripheral_read_workaround(), drop the
>>>
>>> Can you explain to me why this is required (and don't just point me in
>>> the direction of the other patch ;) ).  You appear to be using the
>>> non-relaxed variants of readl and writel, which already do memory
>>> barriers, so I'm a little perplexed as to how the problem can arise.
>>
>> Hmm.
>>
>> A shorter restatement of the architecture requirement would be, I think,
>> "Don't let there be two outstanding reads of different peripherals on
>> the AXI bus, or the CPU might mis-assign the read results.  Use rmb() to
>> wait for the previous bus reads when you need to prevent this"
>>
>> arch/arm/include/asm/io.h's readl() does __iormb() after each
>> __raw_readl().  Imagine taking an interrupt for a new peripheral between
>> the driver's __raw_readl() and the following __iormb(): Now you've got
>> two __raw_readl()s in between iormb()s and you can theoretically get
>> unordered reads.
>>
>> We could hope that the architecture IRQ handler would happen to do an
>> incidental rmb(), resolving the need to protect from interrupt handling
>> inside of device drivers.  The interrupt controller's presence at
>> 0x7e00b200 sounds like it's an AXI peripheral, so it would need to be
>> ensuring ordering of reads.  However, it's doing readl_relaxed()s.  So
>> my rmb() at the start of my irq handler is silly -- if somebody got
>> interrupted between readl and rmb, we've already had a chance to get the
>> wrong result inside of the IRQ chip's status read.
>>
>> My new idea for handling this would be to:
>>
>> 1) Assume drivers don't exit with reads outstanding.  This means they
>> don't do a readl_relaxed() from an AXI peripheral at the end of a path
>> without doing something with the result.
>>
>> 2) Make bcm2835_handle_irq() do this rmb() at the top, with the big
>> explanation, to avoid a race against the interrupted code device being
>> inside a readl() before the __iormb().  We don't worry about the 1-2
>> readl_relaxed()s inside of bcm2835_handle_irq(), because their return
>> values get waited on before continuing on to calling the device driver,
>> so the device driver knows its IRQ handler is being entered with no AXI
>> reads outstanding.
> 
> That's a fantastic explanation.  Thanks for taking the time to
> write this out so diligently.
> 
> Doing this at a sub-arch level sounds a little wrong to me.  I don't
> think Broadcom are the only vendor who do not ensure correct read
> order,

It seems like quite a surprising bug to me, and AFAIK there's nothing
already upstream that requires a similar WAR.

> and writing <vendor>_peripheral_read_workarounds() all over the
> place sounds less than graceful. 

It sounds like Eric's latest plan is to put the one WAR into the
bcm2835-specific IRQ controller driver. Assuming we don't find any
unusual code elsewhere, we shouldn't need to put WARs all over the place.

> Granted, if there were a greater
> need and this can't be fixed another way we could knock off the
> <vendor>_ part and make the call generic, but is there no way we can
> deal with this at the architecture level?
> 
> The whole point of doing readl_relaxed()s is that you can be assured
> that the architecture guarantee ordering.  If that's not the case,
> then we need to be using readl()s in the IRQ handler instead. 

Doing a single dsb at the start of the IRQ handler should be enough,
assuming that the code consumes each read result in a control path
before issuing another readl_relaxed, there will never be multiple reads
outstanding.

[PATCH 4/4] ARM: bcm2835: Add the mailbox to the device tree.

[Stephen Warren]

On 03/12/2015 08:32 PM, Eric Anholt wrote:
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Acked-by: Lee Jones <lee.jones@linaro.org>

Patches 2, 4,
Acked-by: Stephen Warren <swarren@wwwdotorg.org>