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

[H Brett Bolen]

Arnd Bergmann wrote:
>
>
> 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.
>
I thought we would need two buses -- in case we needed both host and cell
connected axons in the same computer.
>> +/* 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.

I have a mailbox and sma drivers running from the same directory,
I try the minimum hw defines needed for all drivers in this shared
file.  I will verify these are needed.
>
>> +/**
>> + * 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?
Good point -- I will make this change.  I think this is more relevant on 
the host
side.
>
> 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.
>
Ok.
>> +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.

Ok.  I will try to use it.  Currently I use the list to handle driver 
callbacks in
register and unregister.  This will change the probe/remove functions.
>
>> +/* 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.
>

Right.
>> +/**
>> + * 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.
Yes, there is no need for this in a fully working and implemented kernel.
>
>> +/**
>> + * 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.
>
This is very interesting.  So basically, I use a wrapper to call the 
child driver probe/remove.

I need to implement a bus match function, and then axonep_probe will get 
called automatically.

There are three kinds of axon_devices I can see:
   
    Cell side    1.  no of_dev entry -- shared memory
                 2.  of_dev entry -- axon mailbox
    Host Side    3.  no of tree -- all devices

Our current thinking is that we need to create axon_devices for all 
support child
drivers, and once bus_match is supported, then the probe and remove 
functions
will get called automatically.   Does this sound correct?
>> +/**
>> + * 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.

Ok.
>
>> +/* 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.
It look like mailbox, ep, dmax share the same parent (pl5).  I could 
just check the parent
if these were the only axon drivers I am interested in.

Can you elaborate on your suggestion? 


>
>> +	/* 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.
That would make it much simpler.... will do.
>
>> +	/* 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.

Currently we are using physical addresses that the other side sends via
the scratchpad.  We are looking at different ways to do this, including
getting these address from the of_dev tree and pci cfg space. 

One note, we do need to keep the remote physical address for the shared
memory driver.  We use it when we map in the memory area to userspace.
For some reason __pa()/virt_to_phys() functions did not return the correct
physical addresses, and the mmap would fail.

>
>> +/**
>> + * 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.

I will investigate.
>
>> +/**
>> + * 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.

Good idea.

Many thanks.

b\375