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

[Arnd Bergmann]

On Wednesday 02 April 2008, H Brett Bolen wrote:
> 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.

No, the point of having your own bus_type is to pull these two together:
Each implementation (pci and of) registers devices on the bus, and the
drivers register themselves on the bus to get access to these devices.

If you had two buses, you would also need two device_driver structures
in each of the drivers.

> > 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.

Right. The wrapper in my example is just a small helper, and you can
also leave that out and do the same thing in each driver, if you
just provide a adrv->device_driver.probe() function in those. It's your
choice, but many other buses have these small helpers.

> 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?

Yes.

Creating the axon_devices is up to the axonep/axonpci drivers. For the
case of the mailbox, you can choose to either register device_drivers
for both of_device and axon_device in the mailbox driver, or to
just always create an axon_device in the axonep driver so that the
mailbox driver gets simpler.

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

These drivers should not really care how they are connected, as that
is up to the implementation of the bus. Suppose you have a simulated axon
that does not simulate the plb5 bus (why should it) and simply puts the
mailbox device directly under the axon node.
By walking up the device tree an arbitrary number of levels, you don't
have to care, and the code might even get simpler.

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

Ok, that makes sense, but you have to be careful to do the address
space conversion at the right place. The OF device tree is set up
by the firmware, which I assume does not know about mappings on the
remote side.

> 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.

physical addresses have no meaning on the bus, as you must have found out
when dealing with the DMA driver. Both sides have an IOMMU that translates
physical addresses to bus addresses. You have to call dma_map_single instead
of virt_to_phys to get a bus address that you can pass to the remote end.

	Arnd <><