[RFC PATCH 08/10] imx: Add MX53 core support

[Uwe Kleine-König]

On Mon, Apr 19, 2010 at 07:53:11AM -0700, Herring Robert-RA7055 wrote:
> Sasha,
I'm always annoyed when people mistype my name, but I don't want to be
appear too picky about it.  So I hint you for Sascha (with a 'c') here
to save him from appearing too picky :-)

> > -----Original Message-----
> > From: Sascha Hauer [mailto:s.hauer at pengutronix.de] 
> > Sent: Saturday, April 17, 2010 9:35 PM
> > To: Herring Robert-RA7055
> > Cc: linux-arm-kernel at lists.infradead.org; amit.kucheria at canonical.com
> > Subject: Re: [RFC PATCH 08/10] imx: Add MX53 core support
> > 
> > On Fri, Apr 16, 2010 at 02:35:22PM -0500, Rob Herring wrote:
> > 
> > [snip]
> > 
> > > +
> > > +void __init mx5x_init_irq(void)
> > 
> > What does the x stand for? Obviously not 1, so how about 
> > mx53_init_irq?
> 
> All new MX5 chips will have memory map similar to MX53 rather than MX51.
> So this function will cover at least MX53 and MX50.
I agree with Sascha here. mx5x should only be used if x can be 1, too.
Otherwise you need to think about another name.  I don't have a good
idea though.  If I had to choose a name I might use fish names to group
them, only fearing we will start to discuss which fishes to choose :-)
 
> > > +{
> > > +	void __iomem *tzic_virt;
> > > +
> > > +	tzic_virt = ioremap(MX5x_TZIC_BASE_ADDR, SZ_4K);
> > > +	if (!tzic_virt)
> > > +		panic("unable to map TZIC interrupt controller\n");
> > > +	tzic_init_irq(tzic_virt);
> > > +}
> > > +
> > > diff --git a/arch/arm/plat-mxc/devices.c 
> > b/arch/arm/plat-mxc/devices.c
> > > index 56f2fb5..7c4c722 100644
> > > --- a/arch/arm/plat-mxc/devices.c
> > > +++ b/arch/arm/plat-mxc/devices.c
> > > @@ -20,13 +20,26 @@
> > >  #include <linux/init.h>
> > >  #include <linux/platform_device.h>
> > >  #include <mach/common.h>
> > > +#include <mach/hardware.h>
> > >  
> > >  int __init mxc_register_device(struct platform_device 
> > *pdev, void *data)
> > >  {
> > >  	int ret;
> > > +	int i;
> > >  
> > >  	pdev->dev.platform_data = data;
> > >  
> > > +	if (cpu_is_mx53()) {
> > > +		for (i = 0; i < pdev->num_resources; i++) {
> > > +			struct resource *r = &pdev->resource[i];
> > > +			if (resource_type(r) != IORESOURCE_MEM)
> > > +				continue;
> > > +
> > > +			r->start = MX5x_FIXUP_ADDR(r->start);
> > > +			r->end = MX5x_FIXUP_ADDR(r->end);
> > > +		}
> > > +	}
> > 
> > This MX5x_FIXUP_ADDR stuff really looks painful. Please just 
> > use proper
> > MX53_ defines and use them where appropriate. We can really effort a
> > duplicate set of resources when both i.MX51 and i.MX53 support are
> > compiled in. You might have a look how Uwe did this in
> > arch/arm/mach-mx2/devices.c.
> > 
> 
> I agree it is a bit ugly, but it is all in effort to run a single kernel
> image. It is a 10 line change vs. 100's of lines of code
> duplicated/renamed. At that point, the question would be why even put
> them in the same mach dir because very little code would be common. MX2
> situation is a bit different as the top-level memory map is the same,
> but the peripherals are mixed up. Taking the MX2 approach, you will then
> have namespace collision with the device names, so all those need to be
> renamed too.
> 
> What if I make the runtime fix-up only be done when building both chips?
on an mx51-only build it's already optimised by the compiler,
nevertheless I second Sascha here.  MX5x_FIXUP_ADDR is obfuscating.
Still more in an if (cpu_is_mx53()) block.

My long-term plan here is to use functions instead of static (and
probably duplicated) device structs.  A while back I sent a patch:

	http://mid.gmane.org/1258403708-10287-16-git-send-email-u.kleine-koenig@pengutronix.de

I couldn't convice Sascha to consider it good, but I still think it's
the best approach.  Only the existing devices are created and the
functions like imx_add_mxc_nand live in .init.text and so don't occupy
memory after booting.

Today I'd let imx_add_mxc_nand return a pointer to the created device to
be able to hand it over to e.g. a regulator.

So I recommend using proper MX53_... defines, too, until I convinced
Sascha to like my approach ;-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |