On 03/12/2015 08:32 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. > > [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@xxxxx> > Signed-off-by: Craig McGeachie <slapdau@xxxxxxxxxxxx> > Signed-off-by: Suman Anna <s-anna@xxxxxx> > Signed-off-by: Jassi Brar <jassisinghbrar@xxxxxxxxx> > Signed-off-by: Eric Anholt <eric@xxxxxxxxxx> > Cc: Jassi Brar <jassisinghbrar@xxxxxxxxx> > Acked-by: Lee Jones <lee.jones@xxxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
