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

[Arnd Bergmann]

On Saturday 08 March 2008, Murali Iyer wrote:

> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/errno.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/poll.h>
> +#include <linux/proc_fs.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/version.h>
> +
> +#include <linux/interrupt.h>
> +#include <linux/uaccess.h>
> +#include <linux/triblade/axon_if.h>
> +#include <linux/triblade/axon_ioctl.h>
> +#include <asm/dcr.h>

I don't think you need uaccess.h or poll.h in this driver

> +
> +extern int	axon_local_sma_pages;
> +extern int	axon_major;
> +extern const char	*axon_name;
> +extern int	num_axons_found;
> +extern int	axon_local_d2d_pages;

These should be in a header file, if you really need them.

> +static struct of_device_id pcie_device_id[] = {
> +	{
> +		.type	= "pcie-endpoint",
> +/*		.compatible = "ibm,axon-pciep" */
> +	},
> +	{}
> +};

Please change this to really use the compatible property.
Don't match on the device-type, as that is meant for
other purposes.

> +/* architecture specific helpers */
> +int __init axon_arch_mbx_init(void)
> +{
> +#ifndef mailbox_disable
> +	return mbx_init();	/* call mailbox driver */
> +#else
> +	return 0;
> +#endif
> +}
> +
> +void __exit axon_arch_mbx_exit(void)
> +{
> +#ifndef mailbox_disable
> +	mbx_exit();		/* call mailbox driver */
> +#endif
> +}

It would be better to just have a separate module_init/module_exit function
in each driver. Instead of using this #ifdef, you then simply don't load
that driver if you don't want it.

For the built-in driver case, you'll have to use *_initcall with
different levels to get the dependencies right. In case of loadable
modules, the order should work out automatically because of symbol
dependencies in the module loader.

> +/* architecture specific helpers */
> +int __init axon_arch_dmax_init(void)
> +{
> +#ifndef dmax_disable
> +	return dmax_init();	/* call dmax driver */
> +#else
> +	return 0;
> +#endif
> +}
> +
> +void __exit axon_arch_dmax_exit(void)
> +{
> +#ifndef dmax_disable
> +	dmax_exit();		/* call dmax driver */
> +#endif
> +}

same here.

> +void axon_arch_map_remote_mem(struct axon *axon_priv)
> +{
> +	axon_sb_map_remote_sma ( axon_priv );
> +}
> +
> +unsigned long int axon_arch_get_rsma_page(struct axon *axon_priv)
> +{
> +	return (axon_priv->remote_sma_paddr >> PAGE_SHIFT);
> +}
> +
> +void *axon_arch_alloc_sma(struct axon *axon_priv)
> +{
> +	void *tmp;
> +	/* force the size to be a power of 2 * PAGE_SIZE */
> +	tmp = dma_alloc_coherent(&axon_priv->axon_device->dev,
> +				 (1<<axon_priv->raw_sma_order)*PAGE_SIZE,
> +				 &axon_priv->local_sma_dma_handle, GFP_KERNEL);
> +	pr_debug("dma handle for sma: %lx\n", axon_priv->local_sma_dma_handle);
> +	return tmp;
> +}

These (and more like them below) should use a data structure with function
pointers for the abstraction, instead of defining the same symbols
as the other driver.

> +int axon_arch_map_single(struct axon *axon_priv, void *addr,
> +			size_t size, int dir, dma_addr_t *handle)
> +{
> +	*handle = dma_map_single(&axon_priv->axon_device->dev,
> +				addr, size, dir);
> +	return dma_mapping_error(*handle);
> +}

As mentioned before, the dma API abstractions should not be needed,
if you implement the dma_mapping_ops so that they are used correctly
on each device.

> +		/* clear the interrupt status reg */
> +		iowrite32be(PCIE_UTL_ISR_CLEARALL,
> +			    axon_priv->pciereg_base_addr + PCIE_UTL_ISR_CLR);
> +
> +		/* commit isr status write */
> +		wmb();

wmb() probably doesn't do what you think it does. iowrite32be is strongly
ordered to start with.

> +		/* enable interrupt generation */
> +		iowrite32be(PCIE_UTL_ISR_ENABLEALL,
> +			    axon_priv->pciereg_base_addr + PCIE_UTL_ISR_MSK);
> +	}

The UTL registers are owned by the firmware. I fear that reprogramming them
in this way might interfere with the error handling on QS2x where the firmware
routes all UTL interrupts to the ppc405 service processor. Have you checked
that this still works correctly?

> +	if (axon_priv->remote_sma_address == NULL)	{
> +		/* remote SMA has been allocated */
> +		axon_priv->remote_sma_size =
> +			SPREG_RD_REMOTE(axon_priv->scratchpad,
> +					remote_sp.sma_size);
> +		raw_phys_addr_hi = SPREG_RD_REMOTE(axon_priv->scratchpad,
> +						   remote_sp.sma_phys_hi);
> +		raw_phys_addr_lo = SPREG_RD_REMOTE(axon_priv->scratchpad,
> +						   remote_sp.sma_phys_lo);
> +		pr_debug("remote sma parms: size = 0x%08x, phys addr = 0x%lx\n",
> +		    axon_priv->remote_sma_size, (((u64) raw_phys_addr_hi)<<32)
> +		    + raw_phys_addr_lo);

You have a lot of code inside of the if() body. I think it would be better
to make that a function by itself.
Also, you might just want to return an error in case of a NULL sma address
instead of trying to handle the rest of the devices without them.

> +#define BE_AXON_BASE_PADDR_MASK 0x0000030000000000ul

What is this? Should it be in the device tree?

> +#define MAX_NUM_AXONS	2

Should not be hardcoded.

	Arnd <><