[RFC][PATCH 00/11] ARM: imx: Add initial i.MX28 support

[u.kleine-koenig@pengutronix.de (Uwe Kleine-König)]

Hello Shawn,

On Tue, Nov 16, 2010 at 08:42:29PM +0800, Shawn Guo wrote:
> On Tue, Nov 16, 2010 at 6:15 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > Hello Shawn,
> >
> > On Mon, Nov 15, 2010 at 10:36:24PM +0800, Shawn Guo wrote:
> >> This patchset adds the initial support of i.MX28, and the target device
> >> is i.MX28 EVK Rev C. It's based on the imx-single-kernel branch below,
> >> and adding codes into arch/arm/mxc and arch/arm/mach-imx.
> >
> > Without further looking at the patch I agree with Uwe: The i.MX23/28
> > are totally different from the other i.MX SoCs and should not be
> > integrated in current i.MX support. And definitely it should not be
> > integrated by adding cpu_is_* into each and every function.
> >
> cpu_is_* is not being added into each and every function. Uwe had this
> comment on only two files, arch/arm/plat-mxc/gpio.c and
I consider the irq handling very ugly, but I didn't feel like repeating
the same argument gives much value.  (And OK, the irq handling was ugly
already before, that's on my plate to evaluate.)  I didn't recheck where
I thought the runtime checks were non-optimal, but seeing to much of
them makes me feel that this is the wrong approach.  This is more a
subjective thing and for that it's independant if there are one, two or
five instances.

> arch/arm/plat-mxc/time.c. As there are big percentage of
> infrastructural common codes that are IP block independent, I though
> it's good to use the same file.  But since you are not in this
> position, I would propose another option. Like what I'm doing with
> iomux-pinctrl, we can create arch/arm/plat-mxc/gpio-pinctrl.c and
> arch/arm/plat-mxc/time-timrot.c for PINCTRL based gpio and TIMROT
> based time implementation.
> 
> The philosophy behind this is we treat arch/arm/plat-mxc as the
> Freescale IP block pool.  The driver in this folder is IP block
> oriented other than SoC oriented.  And SoC is actually a collection of
> IP blocks.  MX23/MX28 integrates PINCTRL, TIMROT, ICOLL, and
> MX21/MX25/MX27/MX31/MX35/MX51 integrates GPIO/IOMUXC, GPT and
> AVIC/TZIC.  Accordingly, MX23/MX28 builds gpio-pinctrl.c, timrot.c and
> icoll.c in, and other i.MX builds gpio.c, iomux-v1//v2/v3,
> avic.c/tzic.c in.  The only thing we need to deal with is picking up
> the file name to stand for the IP block.
For me (not having seen mx50) integrating mx28 into mach-imx seems
wrong.  Currently they are completely orthogonal for the components that
matter for architecture support (i.e. timer, irq handling, clocks, pin
muxing, power management and to a slightly lesser degree gpio and memory
mapping).

> > Now you have two choices: Add the code to plat-stmp3xxx (for the
> > interested reader: the i.MX23/28 are based on former Sigmatel SoCs), or
> > add a new mach-mxs like you did in the Freescale Kernel.
> > I'm not sure which choice of these two is best. It would be good to get
> > some other opinions from people who already had the situation that a
> > vendor got sold and we get the same old metal with new names.
> >
> Yes, MX23/28 are based on former Sigmatel SoCs.  The imx-single-kernel
> demonstrates the possibility of single image for different SoCs from
> same vendor.  My patchset somehow adds the the possibility for SoCs
> even from different vendors, even though I'm unsure if single image
> for SoCs from different vendor is something ARM Kernel will possibly
> go in the future?
That's one of the goals of Linaro.  I'd prefer to get an
"really-all-i.MX" image via this more global approach.

> The plat-stmp3xxx is not an option for me because of the Freescale
> marketing concern.  For the choice of what Freescale Kernel is doing,
I prefer technical arguments here.  Just because some guys in the
marketing department consider giving similar names to completely
different products *I* don't believe grouping them together in the
source code makes more sense.  (It just reinforces my opinion about
marketing guys :-)

> there is no mach-mxs.  Instead, there are arch/arm/plat-mxs,
> arch/arm/mach-mx23 and arch/arm/mach-mx28.  Going this way, MX23/28
> will be totally separated from i.MX family, though their name tells
Adding mach-mx23, mach-mx28 and plat-mxs is (IMHO) too much.  mx23 and
mx28 are similar enough to put both of the two into a single mach
directory.  So seeing the naming choices Freescale did for their kernel
mach-mxs looks reasonable to me.

> the relationship with i.MX family.  Also, Freescale is merging the
This wouldn't be worse than what we had before I merged mach-mx1 and
mach-mx2 into mach-imx for 2.6.36: mach-mx1, mach-mx2, mach-mx25,
mach-mx3 and mach-mx5.

> Sigmatel IP pool into i.MX one and will only maintain the same pool.
> In another words, Sigmatel IP block are getting consolidated with i.MX
> ones. You can still say MX28 are former Sigmatel based, but it gets
> i.MX IP blocks, FEC(ENET), FlexCAN integrated.  If MX28 is not so
This is natural and OK.  But these components are no reason to put mx28
into mach-imx.  The respective drivers live in drivers/net and
drivers/net/can.  So at best they can share the imx_add_device_du_jour
functions.  Here I'd suggest to make these functions available for all
ARMs (or even all archs).

> typical, MX50 can be the one.  It primarily derives from MX51/MX53 and
> will go into plat-mxc and mach-mx5 in common understanding, but it
Let's discuss that when you come up with mx50 patches :-)

> gets a lot of MX28 (Sigmatel) based IP blocks integrated, LCDIF, PXP,
> GPMI, DCP, APHB DMA, and so on.  SoC is merging these two families
> into one.  It makes no sense for SW to keeps them separated.
I don't want to separate things artificially that belong together, but I
don't want to put things together that are really seperate, too.
 
> Can you please think about the IP oriented way I mentioned above to
> see if there is any problem that stops us going?  Also do you want to
> eventually include MX23/28 into imx-single-kernel image?  If you do,
> don't you think my proposal will make it a little bit easier?
It might be easier, yes, but there are also some other criterias that
matter, like maintainability, efficiency of compiled code etc.  And I
think when taking these into account using a different machine directory
is the right choice.

Best regards
Uwe

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