[PATCH 5/6] arm: mx50: add core functions support

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

Hello Richard,

On Fri, Dec 10, 2010 at 05:30:00PM +0800, Richard Zhao wrote:
> On Fri, Dec 10, 2010 at 09:42:48AM +0100, Uwe Kleine-K?nig wrote:
> > On Fri, Dec 10, 2010 at 03:08:35PM +0800, Richard Zhao wrote:
> > > On Thu, Dec 09, 2010 at 09:25:35AM +0100, Uwe Kleine-K?nig wrote:
> > > > On Thu, Dec 09, 2010 at 10:08:35PM +0800, Richard Zhao wrote:
> > > > > +#define WAIT(exp, timeout) \
> > > > > +({ \
> > > > > +     struct timespec nstimeofday; \
> > > > > +     struct timespec curtime; \
> > > > > +     int result = 1; \
> > > > > +     getnstimeofday(&nstimeofday); \
> > > > > +     while (!(exp)) { \
> > > > > +             getnstimeofday(&curtime); \
> > > > > +             if ((curtime.tv_nsec - nstimeofday.tv_nsec) > (timeout)) { \
> > > > > +                     result = 0; \
> > > > > +                     break; \
> > > > > +             } \
> > > > > +     } \
> > > > > +     result; \
> > > > this is broken.  Consider getnstimeofday(&nstimeofday) returns with
> > > > nstimeofday = { .ts_sec = 42, .ts_nsec = 999999999, }.  If timeout is >0
> > > > the break is never taken.  And furthermore I'm sure that getnstimeofday
> > > > isn't the right function for that job.
> > > So, what do I suppose to use? udelay is not accurate here.
> > I'd just busy-loop bounded by a loop count.  Do you need exact timing
> > here?
> Yes. If the time is too short, the system may hang.
What is the unit of the needed time?  If it's clock ticks a busy loop is
fine.  Why udelay isn't accurate enough?

> > > > > +static void __calc_pre_post_dividers(u32 div, u32 *pre, u32 *post)
> > > > > +{
> > > > > +     u32 min_pre, temp_pre, old_err, err;
> > > > > +
> > > > > +     if (div >= 512) {
> > > > > +             *pre = 8;
> > > > > +             *post = 64;
> > > > > +     } else if (div >= 8) {
> > > > > +             min_pre = (div - 1) / 64 + 1;
> > > > > +             old_err = 8;
> > > > > +             for (temp_pre = 8; temp_pre >= min_pre; temp_pre--) {
> > > > > +                     err = div % temp_pre;
> > > > > +                     if (err == 0) {
> > > > > +                             *pre = temp_pre;
> > > > > +                             break;
> > > > > +                     }
> > > > > +                     err = temp_pre - err;
> > > > > +                     if (err < old_err) {
> > > > > +                             old_err = err;
> > > > > +                             *pre = temp_pre;
> > > > > +                     }
> > > > > +             }
> > > > > +             *post = (div + *pre - 1) / *pre;
> > > > > +     } else if (div < 8) {
> > > > > +             *pre = div;
> > > > > +             *post = 1;
> > > > > +     }
> > > > You seem to have copied an old version of this function.  The similarity
> > > > to arch/arm/mach-mx5/clock-mx51.c makes me wonder if you shouldn't copy
> > > > the Copyright lines from there, too.
> > > It's freescale code. clock-mx51.c is originally freescale code too.
> > If you copied from Freescale only that's obviously OK.  Still I wonder
> > about the years.  clock-mx51 has
> > 
> >          * Copyright 2008-2010 Freescale Semiconductor, Inc. All Rights Reserved.
> >          * Copyright (C) 2009-2010 Amit Kucheria <amit.kucheria@canonical.com>
> > 
> > while your suggested clock-mx50.c has
> > 
> >          * Copyright (C) 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> It's because this file was created in 2010.
INAL, but I think it's in your (and/or your employer's) interest to have
an early date here.

> > > > > +static int _clk_enable(struct clk *clk)
> > > > > +{
> > > > > +     u32 reg;
> > > > > +     reg = __raw_readl(clk->enable_reg);
> > > > > +     reg |= MXC_CCM_CCGRx_CG_MASK << clk->enable_shift;
> > > > > +     __raw_writel(reg, clk->enable_reg);
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > These functions are refactored in arch/arm/mach-mx5/clock-mx51.c in the
> > > > meantime, too.
> > > Do you mean _clk_ccgr_xxx? Do I need to abstract common file clock.c, or
> > > just repeat the function?
> > See Sascha's comment.  I agree that it makes sense to get the
> > common clock stuff in first.  With that I have the hope that we can
> > simplify here considerably.
> Where should I put the common code? plat-mxc or mach-mx5?
> Is it ok for you if I only share the code between mx5x?
hmm, don't know yet.  This needs a deeper comparison in my opinion.

> /*
>  * Copyright 2008-2010 Freescale Semiconductor, Inc. All Rights Reserved.
>  *
>  * The code contained herein is licensed under the GNU General Public
>  * License. You may obtain a copy of the GNU General Public License
>  * at the following locations:
>  *
>  * http://www.opensource.org/licenses/gpl-license.html
>  * http://www.gnu.org/copyleft/gpl.html
>  */
> Is above ok?
I think this is something Freescale has to decide.  I prefer the
statement I wrote e.g. in arch/arm/plat-mxc/devices/platform-imx-dma.c,
but as I said, it's up to you.

> > > > > +     tzic_virt = ioremap(tzic_addr, SZ_16K);
> > > > > +     if (!tzic_virt)
> > > > > +             panic("unable to map TZIC interrupt controller\n");
> > > > Is it really necessary for soc code to map the irq controller?  If yes
> > > > that needs to be changed.
> > > TZIC is in on-chip memory section. It's a large memory region. It's hare to
> > > use current memory map routines to map it. I mean imx_map_entry.
> > Hmm, MX50_TZIC_BASE_ADDR is at 0x0fffc000.  If you add it to the
> > statically mapped memory, your map looks as follows:
> > 
> >  *      TZIC    0x0fffc000+0x004000     ->      0xf4bfc000+0x004000
> >  *      SPBA0   0x50000000+0x100000     ->      0xf5400000+0x100000
> >  *      AIPS1   0x53f00000+0x100000     ->      0xf5700000+0x100000
> >  *      AIPS2   0x63f00000+0x100000     ->      0xf5300000+0x100000
> > 
> > which is OK, isn't it?
> Great! I will add a macro MX50_TZIC_SIZE.
And maybe do the same for mx51.

> > > > >  #  define PHYS_OFFSET                MX3x_PHYS_OFFSET
> > > > >  # elif defined CONFIG_ARCH_MXC91231
> > > > >  #  define PHYS_OFFSET                MXC91231_PHYS_OFFSET
> > > > > +# elif defined CONFIG_SOC_IMX50
> > > > > +#  define PHYS_OFFSET                MX50_PHYS_OFFSET
> > > > I think introducing ARCH_MX50 would be nice and using it e.g. here.
> > > Why? Isn't it redundant? They're all soc level macro.
> > Right from a technical pov.  For maintainability they have a different
> > meaning.  That is, ARCH_... needs further addressing for multi-SOC.  I
> > really like being able to grep for these places.
> Is it the only reason that be easy to grep? I really think we only need one of
> them, either a or b.
IMHO the reason is good enough.

Uwe

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