[PATCH 5/6] arm: mx50: add core functions support
Richard Zhao
linuxzsc at gmail.com
Fri Dec 10 07:51:11 EST 2010
Hi Uwe,
2010/12/10 Uwe Kleine-König <u.kleine-koenig at pengutronix.de>:
> 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?
In most cases, it's some specific clock cycles.
udelay may depend on calibrate_delay(). I first used udelay, but it
won't work anyway, so I changed it to getnstimeofday.
I need to add a abs here to avoid overflow.
>
>> > > > > +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.
I'll check with professional.
>
>> > > > > +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.
ok. I will look at mx3 and mx5.
>
>> /*
>> * 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.
I will check with professional, and use the official one.
>
>> > > > > + 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.
ok
>
>> > > > > # 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.
Both you and Sascha have made your decision? Then, OK.
No other platforms use SOC_XXX, I really can not understand the meaning.
>
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
Thanks
Richard
>
More information about the linux-arm-kernel
mailing list