[PATCH 5/6] arm: mx50: add core functions support
Uwe Kleine-König
u.kleine-koenig at pengutronix.de
Fri Dec 10 05:56:53 EST 2010
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 at 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/ |
More information about the linux-arm-kernel
mailing list