[Cbe-oss-dev] [patch 08/16] powerpc and x86_64: Initial port of PCIe endpoint device driver for Axon

[Arnd Bergmann]

On Saturday 08 March 2008, Murali Iyer wrote:

> Index: linux-2.6.23.1/drivers/misc/triblade/axon_mailbox.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6.23.1/drivers/misc/triblade/axon_mailbox.c
> @@ -0,0 +1,618 @@
> +/**
> + *  axon_mailbox - kernel suport for internal mailbox communication.
> + *
> + *  The mailbox is a mechanism that allows the Axon-attached Cell processor
> + *  to receive status messages concerning various processes and data
> + *  transactions in its IO and environment. The processes send mailbox buffers
> + *  of up to 128 bits (16 Bytes) that Axon places in a FIFO structure
> + *  residing in the XDR memory (attached to the Cell). Communication between
> + *  the Axon and the Cell is carried out through a standard write transaction
> + *  through the BE interface. A mailbox buffer can be sent either by external
> + *  devices (e.g., devices attached to the PCI-E) or internal devices
> + *  (e.g., command completion by the DMA).

This driver looks pretty good, nice work!

Do I understand this correctly that this is unidirectional? What do you use
for the opposite way?

> +static int num_mbx_found;

Should not be needed, if you don't have a device, who should call you?

> +/**
> + * mbx_find_base -
> + *
> + */
> +static int mbx_find_base(struct device_node *dn, struct mbx_dev *mbx_p)
> +{
> +
> +	mbx_p->mbxreg_base_addr = (void __iomem *) of_iomap(dn, 0);

The cast should not be needed.

> +/**
> + * mbx_hw_setup -
> + *
> + * During chip initialization, the following operations must be
> + * carried out to prepare the mailbox for operation:
> + *	- Configure the mailbox buffer target address in the
> + *	  MailboxMessageAddress Register
> + *	- Configure the XDR location of the mailbox FIFO in the
> + *	  MailboxBaseAddresss Register
> + *	- Configure the mailbox size and the interrupt mask and
> + *	  set the enable bit in the MailboxControl Register
> + * There is no need to initialize the read and write pointers.
> + *
> + */
> +static int mbx_hw_setup(struct of_device *ofdev, struct mbx_dev *mbx_p)
> +{
> +	/* Get Mailbox buffer memory assigned */
> +	mbx_p->mbx_buf =  dma_alloc_coherent(&ofdev->dev, MAX_MBX_FIFO,
> +					     &mbx_p->buf_da, GFP_KERNEL);
> +	if (!mbx_p->mbx_buf) {
> +		printk(KERN_ERR "Unable to get dma pages\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* mailbox needs to be 64KB aligned */
> +	if ((u64)mbx_p->mbx_buf & 0xffff) {
> +		printk(KERN_ERR "Mailbox buffer not 64KB aligned.\n");
> +		return -ENOMEM;
> +	}

Nothing in the kernel guarantees that the memory returned from
dma_alloc_coherent() is aligned. Even if it is, the DMA address
may not be aligned in the same way. If you require strict alignment,
your best option would be to allocate an extra 64kb and manually
align on the bus address.

> +	pr_debug("mailbox buffer= %p, buf_da= 0x%lx\n",
> +	    mbx_p->mbx_buf, mbx_p->buf_da);
> +
> +	/* Write default mailbox location as per AXON specs. */
> +	set_mailbox_dcr(mbx_p, MAILBOX_MESSAGE_HI, PLB5_DEV_BASE_HI);
> +	set_mailbox_dcr(mbx_p, MAILBOX_MESSAGE_LO, DEV_MBX_MSG0_OFFSET);

This is hardcoding addresses that the driver should not have to
know about. It would be better to get that from the device tree
so that the mailbox driver can be reused for possible future designs.

> +	if (mbx_p->mbxreg_base_addr != 0) {
> +		iounmap((void __iomem *)mbx_p->mbxreg_base_addr);
> +		mbx_p->mbxreg_base_addr = 0;
> +	}

No cast here please. mbxreg_base_addr already needs to be an __iomem
pointer, if it isn't, you have other problems.

> +/*
> + * NOTE: We do not have a "compatible" entry in the device tree yet.
> + * Once that is added, it needs to be updated here.
> + * This should be along the lines of "ibm,mailbox-axon"
> + */
> +static struct of_device_id mbx_device_id[] = {
> +	{
> +		.type	= "mailbox"
> +	},
> +	{}
> +};

Right, that comment is important. You cannot match on the device_type here,
as it does not have an official binding. We need to make sure to not
deliver firmware that gets this wrong!

> +#include <linux/types.h>
> +#include <linux/bitops.h>
> +#include <linux/cdev.h>

You should not need cdev.h here.

> +#include <linux/platform_device.h>
> +
> +/*  function prototypes */
> +int mbx_init(void);	/* mailbox driver initialization */
> +void mbx_exit(void);	/* mailbox driver exit */
> +

Nor these prototypes. Just make the mailbox driver a standalone module
and mark these functions module_init/module_exit.

	Arnd <><