[Cbe-oss-dev] Axon End Point (axonep.ko) driver for review: patch file 1 of 1

[Arnd Bergmann]

On Tuesday 01 April 2008, H Brett Bolen wrote:
> This is a new patch that creates an axonep.ko ( Axon Companion
> Chipset 'End-Point') driver.  This driver will be used to
> access and export axon resources to child drivers.  This driver
> will communicate with a 'host' axonpci.ko driver which sees the
> axon as a pci device.  Axonep.ko and Axonpci.ko will allow
> child drivers to support shared memory, dma ( via dmax), and
> notification services.
> 
> This driver will be followed by a series of child drivers
> to export functionality to userspace, and a network driver.
> 
> Signed-off-by: Brett Bolen <brettb at linux.vnet.ibm.com>

Nice work!

You should probably split the patch into two separate drivers:

* One small driver that is shared with the axonpci driver, to just
provide the bus abstraction (register the bus_type).

* Another driver registering the of_device and creating the
child devices, and handling all OF specific functions.

> +/* Device base on Axon PLB5 */
> +#define PLB5_DEV_BASE_HI		0x40000044u
> +#define PLB5_DEV_BASE_LO		0x00000000u
> +
> +#define BE_DEV_BASE_OFFSET		0x0000004400000000ul

These seem to all be unused in the driver, and therefore should
not be in the header file.

> +/**
> + * axonep_driver - struct used by child driver to access axon
> + * resources.
> + */
> +struct axonep_driver {
> +	char *version;
> +	char *name;
> +	char *owner;
> +	struct module *module;
> +	struct device_driver driver;
> +	int (*probe)(struct axonep_driver *, struct axonep_instance*);
> +	int (*remove)(struct axonep_driver *, struct axonep_instance*);
> +	int (*device_irq)(struct axonep_driver *, int type, int flags);
> +};

I like it how you call the interrupt handler through a driver callback,
but shouldn't that take an axonep_instance argument instead of the
axonep_driver argument?

The name, owner and module fields are probably not necessary here,
as the driver can set those in the embedded device_driver itself.

I don't like the concept of driver versions that much, as the intention
is to have the driver in the mainline kernel, and then the driver version
is the same as the kernel version.

> +static struct list_head	 axonep_list;

Why do you need the axonep_list? Normally you should be able to just
iterate through the devices using driver_for_each_device.

> +/* handle dcr interface chage */
> +#if 1
> +#define DCR_ADDR(base,offset)	(base+offset)
> +#else
> +#define DCR_ADDR(base,offset)	(offset)
> +#endif

ok for now, but this should go away for the upstream version, as
we don't want to carry compatibility code for older kernels there.

> +/**
> + * dump_image - hexdump data
> + * @ptr: pointer to data
> + * @offfset: start offset
> + * @len: length to dump
> + * @banner: banner to print
> + *
> + * output form "addr: hex[0..7] | ascii[0..7]\n"
> + */
> +static void dump_image(void *buf, int offset, int len, char *banner)
> +{

For similar reasons, this could probably go away as well, the function
is currently unused because of if(0) around its only caller, but
useful for debugging I guess.

> +/**
> + * axonep_register_driver - Registers a new Axon child device driver
> + * @adrv: pointer to the driver definition structure
> + *
> + * This function is called by child drivers to register
> + * itself on the axon bus.  Note: axon device support
> + * will be needed at a later date.
> + */
> +int axonep_register_driver(struct axonep_driver *adrv)
> +{
> +	int rc;
> +	struct axonep_instance *ipriv;
> +	struct list_head  *pos, *next;
> +
> +	/* initialize common driver fields */
> +	adrv->driver.name = adrv->name;
> +	adrv->driver.bus = &axonep_bus_type;
> +
> +	/* register with core */
> +	rc = driver_register(&adrv->driver);
> +	pr_debug("%s: registering '%s' @ %p returns %d\n",
> +		 __FUNCTION__, adrv->name, adrv, rc);
> +
> +	/* notify child driver of each axon */
> +	list_for_each_safe(pos, next, &axonep_list) {
> +		if (adrv->probe) {
> +			ipriv = list_entry(pos, struct axonep_instance, list);
> +			if (!ipriv)
> +				pr_info("%s: bad ipriv\n", __FUNCTION__);
> +			else
> +				adrv->probe( adrv, ipriv);
> +		}
> +	}
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(axonep_register_driver);

This looks a bit strange. The adrv->probe function should have been
called earlier as a side-effect of driver_register. I think what you
want here is a helper function

int axonep_probe(struct device *dev)
{
	struct axonep_instance *ipriv = container_of(dev, struct axonep_instance, dev);
	struct axonep_driver *adrv = container_of(dev->drv, struct axonep_driver, driver);

	return adrv->probe(adrv, ipriv);
}

and assign that to adrv->driver.probe before calling driver_register.

> +/**
> + * axonep_unregister_driver - Unregisters a new Axon device driver
> + * @adrv: pointer to the driver definition structure
> + */
> +void axonep_unregister_driver(struct axonep_driver *adrv)
> +{
> +	struct list_head *pos, *next;
> +	pr_debug("%s: unregistering '%s' \n",
> +		 __FUNCTION__, adrv->name);
> +
> +	/* notify child driver of each axon */
> +	list_for_each_safe(pos, next, &axonep_list) {
> +		if (adrv->remove) {
> +			struct axonep_instance *ipriv;
> +			ipriv = list_entry(pos, struct axonep_instance, list);
> +			adrv->remove(adrv, ipriv);
> +		}
> +	}
> +	driver_unregister(&adrv->driver);
> +}
> +EXPORT_SYMBOL_GPL(axonep_unregister_driver);

same thing here.

> +/* axonep_get_handle - given of_device, return axon handle
> + * @ofdev - of_device to match
> + *
> + * if the of_device and the axon instance's of_device share
> + * a common grandparent of_device then the corresponding
> + * axon priv instance is returned to be used as a 'handle'
> + * to get axon base resources.
> + */
> +struct axonep_instance *axonep_get_handle(struct of_device *ofdev)
> +{
> +	struct list_head *pos, *next;
> +	struct device_node *parent, *grandparent;
> +
> +	parent = of_get_parent(ofdev->node);
> +	if (!parent) {
> +		pr_info("%s: no parent\n", __FUNCTION__);
> +		return NULL;
> +	}
> +	grandparent = of_get_parent(parent);
> +	if (!grandparent) {
> +		pr_info("%s: no grandparent\n", __FUNCTION__);
> +		return NULL;
> +	}

instead of doing two explicit levels here, I think it would be better
to loop through the parents until you get to an axon device, or to the
device tree root.

> +	/* search for axon of_dev */
> +	list_for_each_safe(pos, next, &axonep_list) {
> +		struct device_node    *axon_parent;
> +		struct device_node    *axon_grandparent;
> +		struct axonep_instance *ipriv;
> +		ipriv = list_entry(pos, struct axonep_instance, list);
> +		if (ipriv) {
> +			axon_parent = of_get_parent(ipriv->ofdev->node);
> +			if (!axon_parent) {
> +				pr_info("%s: no axon parent\n", __FUNCTION__);
> +				return NULL;
> +			}
> +			axon_grandparent = of_get_parent(axon_parent);
> +			if (!axon_grandparent) {
> +				pr_info("%s: no axon grandparent\n",
> +					__FUNCTION__);
> +				return NULL;
> +			}
> +			if (axon_grandparent == grandparent) {
> +				/* success */
> +				return ipriv;
> +			}
> +		}
> +	}

This is missing some of_node_put. A better way to do it without
the global list would be to put the pointer to the ipriv into
the driver_data of the of_device.

> +	/* grab memory */
> +	if (!request_mem_region(priv->rmem2.paddr,
> +				priv->rmem2.size,
> +				"axon_rsma"))	{
> +		rc = -ENOMEM;
> +		dev_info(&priv->ofdev->dev,
> +			 "request_mem_region failed for rsma\n");
> +	} else {
> +		rc = 0;
> +		priv->rmem2.ptr = (void __iomem *)
> +			ioremap(priv->rmem2.paddr,
> +				priv->rmem2.size);
> +	}
> +	return rc;
> +}

I find it hard to follow the pointers to the memory areas you
are ioremapping here. It would be easier to read if you just
do an of_iomap of the resources at device detection time,
and then reference that as virtual addresses, instead of
passing down physical addresses.

> +/**
> + * axonep_probe - called during of registration
> + * @ofdev: of device tree entry
> + * @match: matching of device id;
> + */
> +static int __devinit axonep_of_probe(struct of_device *ofdev,
> +				     const struct of_device_id *match)
> +{
> +	int		 rc = 0, rc2 = 0;
> +	int		 current_axon;
> +	struct axonep_instance *priv;
> +	struct resource	       res;
> +	static int	       num_found = 0;
> +
> +	dev_info(&ofdev->dev, "%s:: Found %s\n", __FUNCTION__, MODULE_NAME);
> +
> +	rc = of_address_to_resource(ofdev->node, 0, &res);
> +	if (rc) {
> +		dev_info(&ofdev->dev, "%s: of_address_to_resource() failed "
> +			 "errno = %d\n",
> +			 __FUNCTION__, rc);
> +		return -ENODEV;
> +	}

AFAICS, this is the place where the address comes from, right?
So just call the high-level of_iomap here and ignore the resource.
Note that most drivers don't even bother calling request_mem_resource
like you do.

If you want to properly request the resources, you should probably
write a new of_request_resource() function to go into 
arch/powerpc/kernel/prom_parse.c and call that one. There is already
a macio_request_resource that you can basically copy.

> +/**
> + * open firmware device id, and driver struct
> + */
> +static struct of_device_id pcie_device_id[] = {
> +	{
> +		.type = "pcie-endpoint",
> +#ifdef	 OF_COMPATIBLE
> +		.compatible = "ibm,axon-pciep"	  /* qs22 only */
> +#endif
> +	},
> +	{}
> +};

You can have two elements in here, one for the old "device-type" method,
and one for the proper way of doing with with "compatible".
That will save you the #ifdef.

	Arnd <><