Re: [PATCH 6/13] powerpc: Add arch/powerpc support for Marvell/mv64x60 hostbridge

["Dale Farnsworth" <dale@farnsworth.org>]

On Thu, Apr 26, 2007 at 12:42:39AM +0000, Arnd Bergmann wrote:
> > Index: linux-2.6-powerpc-df/arch/powerpc/sysdev/mv64x60.c
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6-powerpc-df/arch/powerpc/sysdev/mv64x60.c	2007-04-17 12:48:22.000000000 -0700
> > @@ -0,0 +1,343 @@
> > +/*
> > + * Common routines for the Marvell mv64360/mv64460 host bridges (Discovery)
> > + *
> > +/* Interrupt Controller Interface Registers */
> 
> I'd make this a file that _only_ deals with the interrupt controller
> code, and has a respective name, e.g. mv64x60_irq.c

Does anyone else feel strongly about this?  I can go either way.
On the one hand, separating out the irq handling might make it
marginally easier to understand.  On the other hand, it's nice
to keep all of the mv64x60 support together. I see examples
doing it both ways currently in arch/powerpc/sysdev.

> > +/*
> > + * The bootwrapper sets the coherency of the DMA windows according to
> > + * the setting in the device tree.  For the kernel, coherency is a
> > + * compile-time configuration option.  Fail if there is a mismatch.
> > + */
> > +
> > +#ifdef CONFIG_NOT_COHERENT_CACHE
> > +#define KERNEL_COHERENCY	0
> > +#else
> > +#define KERNEL_COHERENCY	1
> > +#endif
> > +
> > +int __init mv64x60_verify_cache_coherency(void)
> > +{
> > +	struct device_node *np;
> > +	const void *prop;
> > +	int devtree_coherency;
> > +
> > +	np = of_find_node_by_path("/");
> > +	prop = of_get_property(np, "coherency-off", NULL);
> > +	of_node_put(np);
> > +
> > +	devtree_coherency = prop ? 0 : 1;
> > +
> > +	if (devtree_coherency != KERNEL_COHERENCY) {
> > +		printk(KERN_ERR
> > +			"kernel coherency:%s != device tree_coherency:%s\n",
> > +			KERNEL_COHERENCY ? "on" : "off",
> > +			devtree_coherency ? "on" : "off");
> > +		BUG();
> > +	}
> > +
> > +	return 0;
> > +
> > +late_initcall(mv64x60_verify_cache_coherency);
> 
> Not sure where to best put this function, but probably not in the same file as
> your interrupt handling code.
> 
> Note that the way your function is written, there is nothing specific to
> mv64x60 in it, and it will actually get called on other machines when the
> file is built-in.
> 
> Maybe just rename it to verify_cache_coherency() and put it into
> arch/powerpc/kernel/setup-common.c.

Good point.  I'll break it out and submit it separately.

Thanks,
-Dale