[PATCH] mailbox: Enable BCM2835 mailbox support

[Ross Oldfield]

Thanks,

> 
> +#define MAIL0_CNF	(ARM_0_MAIL0 + 0x1C)
> +#define MAIL1_WRT	(ARM_0_MAIL1 + 0x00)
> +
> +#define MBOX_CHAN_COUNT		16
> +


I'd like to add:
#define MAIL1_STA (ARM_0_MAIL1 + 0x18)
... because ...

> 
> +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);
> +	if (readl(mbox->regs + MAIL0_STA) & ARM_MS_FULL) {
> +		rmb(); /* Finished last mailbox read. */
> +		ret = -EBUSY;
> +		goto end;
> +	}

The conditional here cannot ever be non-zero. Each of the two FIFOs we 
are using here ( MAIL0 and MAIL1 ) have space for eight u32 entries in 
them and the procotol is to only ever send or receive a single u32 at a 
time. Something very bad must have happened for eight replies to be 
waiting in the FIFO.

What we should be checking for is "has the remote end actually read the 
last u32 we wrote". This is the same as saying "is the MAIL1_WRT FIFO 
empty?" because if it is empty then it stands to reason that the remote 
end read it already and so is not busy. So this conditional should be:
if (!(readl(mbox->regs + MAIL1_STA) & ARM_MS_EMPTY)) {
	rmb(); /* Finished last mailbox read. */
	ret =- EBUSY


Similarly,

> +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. */
> +	spin_unlock(&mbox->lock);
> +	return ret;
> +}

Again this should be checking for MAIL1 FIFO being empty as the tx is 
"done" when the remote end has removed the last u32 written.

So,

ret = !!(readl(mbox->regs + MAIL1_STA) & ARM_MS_EMPTY);

... is better here I think.


As an aside, it turns out that we _may_ be able to make this a 
non-polling driver because it turns out this peripheral has the ability 
to generate an interrupt when the MAIL1 FIFO is empty. At the moment on 
probe we set bit 1 of MAIL0_CNF to get an interrupt for "MAIL0 FIFO is 
not empty". There are additonal options in the configuration register:

MAILn_CNF:
10 R/Wc Error: Read from empty mailbox
9  R/Wc Error: Write to full mailbox
8  R/Wc Error: None owner read mailbox
7  -    <Unused, read as zero>
6  R    Opposite Mailbox is empty interrupt pending
5  R    My Mailbox has space interrupt pending
4  R    My Mailbox has data interrupt pending
3  R/W  Reset mailbox (set then clear this bit)
2  R/W  Opp. Mailbox is empty interrupt enable
1  R/W  Mailbox has space interrupt enable
0  R/W  Mailbox has data interrupt enable

So by setting bit two we can now get interrupted when the remote end 
has emptied MAIL1 and can check in the irq handler by reading bit 6 
that this was the interrupt cause.



Thanks again,

Ross