Stephen Warren <swarren@xxxxxxxxxxxxx> writes:
> On 03/02/2015 01:54 PM, Eric Anholt wrote:
>> From: Lubomir Rintel <lkundrak@xxxxx>
>>
>> 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.
>
>> diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
>
>> +/* Mailboxes */
>> +#define ARM_0_MAIL0 0x00
>> +#define ARM_0_MAIL1 0x20
>> +
>> +/*
>> + * Mailbox registers. We basically only support mailbox 0 & 1. We
>> + * deliver to the VC in mailbox 1, it delivers to us in mailbox 0. See
>> + * BCM2835-ARM-Peripherals.pdf section 1.3 for an explanation about
>> + * the placement of memory barriers.
>> + */
>> +#define MAIL0_RD (ARM_0_MAIL0 + 0x00)
>> +#define MAIL0_POL (ARM_0_MAIL0 + 0x10)
>> +#define MAIL0_STA (ARM_0_MAIL0 + 0x18)
>> +#define MAIL0_CNF (ARM_0_MAIL0 + 0x1C)
>> +#define MAIL1_WRT (ARM_0_MAIL1 + 0x00)
>
> That implies there are more mailboxes. I wonder if we should
> parameterize which to use via some DT properties? I guess we can defer
> that though; we can default to the current values and add properties
> later if we want to use something else.
Yeah, until there's something to talk to in another mailbox, this seems
fine.
>> +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;
>> +
>> + while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
>> + u32 msg = readl(mbox->regs + MAIL0_RD);
>> + unsigned int chan = MBOX_CHAN(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);
>> + mbox_chan_received_data(mbox->channel[chan].link,
>> + (void *) MBOX_DATA28(msg));
>> + }
>> + rmb(); /* Finished last mailbox read. */
>
> I know the PDF mentioned in the comment earlier in the patch says to put
> in barriers between accesses to different peripherals, which this seems
> compliant with, but I don't see quite what this barrier achieves. I
> think the PDF is talking generalities, not imposing a rule that must be
> blindly followed. Besides, if there's a context-switch you can't
> actually implement the rules the PDF suggests. What read operation is
> this barrier attempting to ensure happens after reading all mailbox
> messages and any associated DRAM buffer?
Looking at this bit of code in particular:
"As interrupts can appear anywhere in the code so you should safeguard
those. If an interrupt routine reads from a peripheral the routine
should start with a memory read barrier. If an interrupt routine writes
to a peripheral the routine should end with a memory write barrier."
So it seems like the IRQ handler at least wants:
diff --git a/drivers/mailbox/bcm2835-mailbox.c b/drivers/mailbox/bcm2835-mailbox.c
index 604beb7..7e528f6 100644
--- a/drivers/mailbox/bcm2835-mailbox.c
+++ b/drivers/mailbox/bcm2835-mailbox.c
@@ -88,6 +88,13 @@ 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-ARM-Peripherals.pdf says "If an interrupt routine
+ * reads from a peripheral the routine should start with a
+ * memory read barrier."
+ */
+ rmb();
+
while (!(readl(mbox->regs + MAIL0_STA) & ARM_MS_EMPTY)) {
u32 msg = readl(mbox->regs + MAIL0_RD);
unsigned int chan = MBOX_CHAN(msg);
@@ -100,7 +107,6 @@ static irqreturn_t bcm2835_mbox_irq(int irq, void *dev_id)
mbox_chan_received_data(mbox->channel[chan].link,
(void *) MBOX_DATA28(msg));
}
- rmb(); /* Finished last mailbox read. */
return IRQ_HANDLED;
}
--
> If any barrier is needed, shouldn't it be between the mailbox read and
> the processing of that message (which could at least in some cases read
> an SDRAM buffer). So, the producer would do roughly:
>
> p1) Fill in DRAM buffer
> p2) Write memory barrier so the MBOX write happens after the above
> p3) Send mbox message to tell the consumer to process the buffer
>
> ... and the consumer:
>
> c1) Read MBOX register to know which DRAM buffer to handle
> c2) rmb() to make sure we read from the DRAM buffer after the MBOX read
> c3) Read the DRAM buffer
>
> Even then, since (c3) is data-dependent on (c1), I don't think the rmb()
> in (c2) there actually does anything useful.
I'm not sure if this is already covered by "The GPU has special logic to
cope with data arriving out-of-order", but I think I agree we should
probably do it for safety.
>> +static int bcm2835_send_data(struct mbox_chan *link, void *data)
>> +{
>> + struct bcm2835_channel *chan = to_channel(link);
>> + struct bcm2835_mbox *mbox = chan->mbox;
>> + int ret = 0;
>> +
>> + if (!chan->started)
>> + return -ENODEV;
>> + spin_lock(&mbox->lock);
>
> Is it safe to read chan->started without the channel lock held?
The channel's lock is held by our caller (msg_submit() of mailbox.c).
>> + if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
>> + rmb(); /* Finished last mailbox read. */
>
> This also doesn't seem useful?
This (and the next one) seems to fall under:
"You should place:
• A memory write barrier before the first write to a peripheral.
• A memory read barrier after the last read of a peripheral.
It is not required to put a memory barrier instruction after each read
or write access. Only at those places in the code where it is possible
that a peripheral read or write may be followed by a read or write of a
different peripheral. This is normally at the entry and exit points of
the peripheral service code."
>> +static int bcm2835_startup(struct mbox_chan *link)
>> +{
>> + struct bcm2835_channel *chan = to_channel(link);
>> +
>> + chan->started = true;
>> + return 0;
>> +}
>> +
>> +static void bcm2835_shutdown(struct mbox_chan *link)
>> +{
>> + struct bcm2835_channel *chan = to_channel(link);
>> +
>> + chan->started = false;
>> +}
>
> Don't we need to hold chan->lock when adjusting chan->started? Or is
> start/stop intended to be asynchronous to any operations currently in
> progress on the channel?
Only one client can be on a channel at a time, which is controlled by
con_mutex at channel request time, and these hooks are when the client
appears/disappears.
The started check in the irq handler is necessary so that we drop any
stray mbox messages instead of NULL dereffing in
mbox_chan_received_data(). I think the check in bcm2846_send_data()
could just be dropped (we know we have a client if a client is trying to
send a message). I haven't followed quite what bcm2835_last_tx_done()
is doing.
>> +static bool bcm2835_last_tx_done(struct mbox_chan *link)
>> +{
>> + struct bcm2835_channel *chan = to_channel(link);
>> + struct bcm2835_mbox *mbox = chan->mbox;
>> + bool ret;
>> +
>> + if (!chan->started)
>> + return false;
>> + spin_lock(&mbox->lock);
>> + ret = !(readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL);
>> + rmb(); /* Finished last mailbox read. */
>
> That barrier doesn't seem useful?
Same barrier comment.
> What are the semantics of "tx done"? This seems to be testing that the
> TX mailbox isn't completely full, which is more about whether the
> consumer side is backed up rather than whether our producer-side TX
> operations are done.
Take a look at poll_txdone() -- it does look like we're doing the right
thing here, just that the method would be better named as
"ready_to_send" or something.
>> +static int request_mailbox_irq(struct bcm2835_mbox *mbox)
>
>> + if (irq <= 0) {
>> + dev_err(dev, "Can't get IRQ number for mailbox\n");
>> + return -ENODEV;
>> + }
>
> I expect devm_request_irq() checkes that condition.
Chasing things down, it looks like you'd get a silent error, but then
we've already got a whine if devm_request_irq fails anyway.
>> + ret = devm_request_irq(dev, irq, bcm2835_mbox_irq, 0, dev_name(dev),
>> + mbox);
>> + if (ret) {
>> + dev_err(dev, "Failed to register a mailbox IRQ handler\n");
>
> Printing ret might be useful to know why.
Yeah.
> Are you sure devm_request_irq() is appropriate? The IRQ handler will be
> unregistered *after* bcm2835_mbox_remove() is called, and I think
> without any guarantee re: the order that other devm_ allocations are
> cleaned up. If bcm2835_mbox_remove() can't guarantee that no IRQ will
> fire after it exits, then bcm2835_mbox_irq() might just get called after
> some allocations are torn down, thus causing the IRQ handler to touch
> free'd memory.
It looks like we should
writel(0, mbox->regs + MAIL0_CNF);
in the unload.
> Both request_mailbox_iomem and request_mailbox_irq are small enough
> they're typically written inline into the main probe() function.
Sounds good.
>> +static int bcm2835_mbox_probe(struct platform_device *pdev)
>
>> + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
>> + if (mbox == NULL) {
>> + dev_err(dev, "Failed to allocate mailbox memory\n");
>
> devm_kzalloc() already prints an error, so no need to add another here,
> even if it does nicely document the fact that you remembered error
> messages:-)
Wait, it does? I couldn't find where it would, when I was looking into
the checkpatch warnings.
>> + mbox->controller.txdone_poll = true;
>> + mbox->controller.txpoll_period = 5;
>> + mbox->controller.ops = &bcm2835_mbox_chan_ops;
>> + mbox->controller.dev = dev;
>> + mbox->controller.num_chans = MBOX_CHAN_COUNT;
>> + mbox->controller.chans = devm_kzalloc(dev,
>> + sizeof(struct mbox_chan) * MBOX_CHAN_COUNT,
>> + GFP_KERNEL);
>
> It'd be common to say "sizeof(*mbox->controller.chans) so the type can't
> mismatch what's being assigned to.
Agreed.
Attachment:
signature.asc
Description: PGP signature
