[PATCH 5/6] arm: mx50: add core functions support
Richard Zhao
richard.zhao at freescale.com
Fri Dec 10 04:30:00 EST 2010
Hi Uwe,
On Fri, Dec 10, 2010 at 09:42:48AM +0100, Uwe Kleine-König wrote:
> Hello Richard,
>
> 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.
>
> > > > +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.
>
> > > > +}
> > > > +
> > > > +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?
>
> > > > +static unsigned long pfd_round_rate(struct clk *clk, unsigned long rate)
> > > > +{
> > > > + u32 frac;
> > > > + u64 tmp;
> > > > + tmp = (u64)clk_get_rate(clk->parent) * 18;
> > > This can overflow e.g. when parent = apll
> > 480M * 18 = 0x202FBF000, u64 can not overflow.
> ah, missed that u64.
>
> > >
> > > > + do_div(tmp, rate);
> > > > + frac = tmp;
> > > > + frac = frac < 18 ? 18 : frac;
> > > > + frac = frac > 35 ? 35 : frac;
> > > I remember having seen something like:
> > >
> > > #define CLAMP(x, low, high) ((x) < (low) ? (low) : ((x) > (high) ? (high) : (x)))
> > >
> > > Maybe this is something for <linux/kernel.h>, probably with better
> > > typing.
> > Yes, thanks. it's clamp(val, min, max).
> ah, anticipatory obedience :-)
>
> > > > + /* clear clk frac bits */
> > > > + __raw_writel(MXC_ANADIG_PFD_FRAC_MASK << clk->enable_shift,
> > > > + apll_base + (int)clk->enable_reg + 8);
> > > What is 8?
> > Its read/set/clear reg are three different reg. The ip is from mxs series.
> > I can add a MXC_SET_OFFSET and MXC_CLEAR_OFFSET macro in crm-regs-mx50.h, if you need it.
> ah, ok. The (suggested) mxs barebox port uses 4 and 8, too. I wonder
> if it makes sense to introduce
>
> #define __raw_setl(val, addr) __raw_writel(val, addr + 4)
>
> and analogous __raw_clearl. That together with a more defensive way to
> define it. (I.e. cast addr to char* (?) and __iomem (?) and use the
> usual parenthesis.)
Yes, if the offset is alway 4 or 8. I'm not sure yet. I will check with Shawn.
And sharing code between mxc and mxs is another problem.
>
> > > > diff --git a/arch/arm/mach-mx5/crm_regs-mx50.h b/arch/arm/mach-mx5/crm_regs-mx50.h
> > > > new file mode 100644
> > > > index 0000000..f57e93c
> > > > --- /dev/null
> > > > +++ b/arch/arm/mach-mx5/crm_regs-mx50.h
> > > > @@ -0,0 +1,607 @@
> > > > +/*
> > > > + * 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
> > > > + * Version 2 or later at the following locations:
> > > Does that mean GPL v1 may be applied or not?
> > Does it need to mention all GPL version?
> I like specifying the exact terms. Some people consider saying "version
> X or later" OK. So "licensed under the GNU General Public License"
> without a version might be OK for you. But then saying where to find a
> copy of the GPL v2 or later seems strange to me.
/*
* 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 saw other files have such header too.
> IMHO that's a bad excuse. Feel free to make your work better than the
> people before you!
>
> > > > +/*
> > > > + * Define the MX50 memory map.
> > > > + */
> > > > +static struct map_desc mx50_io_desc[] __initdata = {
> > > > + imx_map_entry(MX50, AIPS1, MT_DEVICE),
> > > > + imx_map_entry(MX50, SPBA0, MT_DEVICE),
> > > > + imx_map_entry(MX50, AIPS2, MT_DEVICE),
> > > Did you verify that the exiting function works for mx50?
> > Yes, I calculated the physical->virtual map, no overlap.
> > And ccm and uart works.
> OK, I added mx50 to my script that generates the comment for IMX_IOP2V.
> (And it confirms your findings :-)
>
> > > > +void __init mx50_init_irq(void)
> > > > +{
> > > > + unsigned long tzic_addr;
> > > > + void __iomem *tzic_virt;
> > > > +
> > > > + tzic_addr = MX50_TZIC_BASE_ADDR;
> > > > +
> > > > + 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.
>
> > > > +/*
> > > > + * various IOMUX alternate output functions (1-7)
> > > > + */
> > > > +enum iomux_config {
> > > > + IOMUX_CONFIG_ALT0,
> > > > + IOMUX_CONFIG_ALT1,
> > > > + IOMUX_CONFIG_ALT2,
> > > > + IOMUX_CONFIG_ALT3,
> > > > + IOMUX_CONFIG_ALT4,
> > > > + IOMUX_CONFIG_ALT5,
> > > > + IOMUX_CONFIG_ALT6,
> > > > + IOMUX_CONFIG_ALT7,
> > > > + IOMUX_CONFIG_GPIO, /* added to help user use GPIO mode */
> > > > + IOMUX_CONFIG_SION = 0x1 << 4, /* LOOPBACK:MUX SION bit */
> > > > +};
> > > Shouldn't that go into iomux-v3.h?
> > Yes. I added it here because I saw other file also defined it.
> > I can send out a new patch to move it.
> Yes, that's a seperate patch at the beginning of the series, please.
>
> > > > diff --git a/arch/arm/plat-mxc/include/mach/irqs.h b/arch/arm/plat-mxc/include/mach/irqs.h
> > > > index e42e9e4..a085bf2 100644
> > > > --- a/arch/arm/plat-mxc/include/mach/irqs.h
> > > > +++ b/arch/arm/plat-mxc/include/mach/irqs.h
> > > > @@ -29,6 +29,8 @@
> > > > #define MXC_GPIO_IRQS (32 * 4)
> > > > #elif defined CONFIG_ARCH_MX25
> > > > #define MXC_GPIO_IRQS (32 * 4)
> > > > +#elif defined CONFIG_SOC_IMX50
> > > > +#define MXC_GPIO_IRQS (32 * 6)
> > > > #elif defined CONFIG_SOC_IMX51
> > > > #define MXC_GPIO_IRQS (32 * 4)
> > > > #elif defined CONFIG_ARCH_MXC91231
> > > That is if you have a kernel with support for mx25 and mx50 you end up
> > > with MXC_GPIO_IRQS being defined as (32 * 4).
> > ok, I will move it to the first place. Anyway, I think it's just a work around for mult-SoC support.
> It's needed as long was we need to specify the number of irqs. Unless
> when using sparse irq it's good to keep this number low. So this is the
> strait forward approach.
>
> > > > diff --git a/arch/arm/plat-mxc/include/mach/memory.h b/arch/arm/plat-mxc/include/mach/memory.h
> > > > index 9a9a000..5022c32 100644
> > > > --- a/arch/arm/plat-mxc/include/mach/memory.h
> > > > +++ b/arch/arm/plat-mxc/include/mach/memory.h
> > > > @@ -16,6 +16,7 @@
> > > > #define MX25_PHYS_OFFSET UL(0x80000000)
> > > > #define MX27_PHYS_OFFSET UL(0xa0000000)
> > > > #define MX3x_PHYS_OFFSET UL(0x80000000)
> > > > +#define MX50_PHYS_OFFSET UL(0x70000000)
> > > > #define MX51_PHYS_OFFSET UL(0x90000000)
> > > > #define MX53_PHYS_OFFSET UL(0x70000000)
> > > > #define MXC91231_PHYS_OFFSET UL(0x90000000)
> > > > @@ -33,6 +34,8 @@
> > > > # 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.
>
> Best regards
> 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