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

[Murali Iyer]

On Thu, 2008-03-13 at 05:55 +0100, Arnd Bergmann wrote:

> This driver looks pretty good, nice work!
thanks

> 
> Do I understand this correctly that this is unidirectional? What do you use
> for the opposite way?
Yep. this is from host to cell. For opposite the UTL stuff to generate
PCIe interrupt.. we are using shared one and need to start using MSI
but having lack of some support host systems...
and, this driver is extracting mails what host has sent _not_ the
sending part itself.



> 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.
I suspect it has to be aligned 16 bytes boundary.. need to check.

> 
> > +	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.
need to check whether dev-tree has it otherwise will request firmware
to add it and infact this code could be removed if firmware writes it.


> > +/*
> > + * 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!
yep

> 
> > +#include <linux/types.h>
> > +#include <linux/bitops.h>
> > +#include <linux/cdev.h>
> 
> You should not need cdev.h here.
stale code. was needed for 2.6.20 stuff when this was providing char
dev...

> 
> > +#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.
yep. but sync between probes needed prior to that.

-- 
Bye,

Murali