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

[Murali Iyer]

On Thu, 2008-03-13 at 06:12 +0100, Arnd Bergmann wrote:
> > +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.
it is commented out since some of the system is _not_ up to
date with the firmware.. need to have a working code :)
will be uncommented.

> 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.
that is the plan and adds complexity to sync up probe completions.
> > +void __exit axon_arch_dmax_exit(void)
> > +{
> > +#ifndef dmax_disable
> > +	dmax_exit();		/* call dmax driver */
> > +#endif
> > +}
> 
> same here.
we are still debating and sure this will change but don't know
what form it will take..

> 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.
yep. basic plumbing will take care of this.

> 
> > +		/* 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?
I wouldn't put it saying owned by. need to share the resources.. We need
to access this register and have not come across any issues... we found
a way to use ppc405.. I am not 100% convinced the reason behind.. that
is me.


> > +#define BE_AXON_BASE_PADDR_MASK 0x0000030000000000ul
> 
> What is this? Should it be in the device tree?
its a mask (bit 41 & 42 of BE address).. could be done
differently..


> 
> > +#define MAX_NUM_AXONS	2
> 
> Should not be hardcoded.
may be..  I think this will go away in the new driver...


Bye,

Murali