From mboxrd@z Thu Jan 1 00:00:00 1970 From: u.kleine-koenig@pengutronix.de (Uwe =?iso-8859-1?Q?Kleine-K=F6nig?=) Date: Fri, 10 Dec 2010 11:56:53 +0100 Subject: [PATCH 5/6] arm: mx50: add core functions support In-Reply-To: <20101210093000.GA29588@b20223-2.ap.freescale.net> References: <1291903716-31388-1-git-send-email-richard.zhao@freescale.com> <1291903716-31388-5-git-send-email-richard.zhao@freescale.com> <20101209082535.GK17441@pengutronix.de> <20101210070835.GB29383@b20223-2.ap.freescale.net> <20101210084248.GF17441@pengutronix.de> <20101210093000.GA29588@b20223-2.ap.freescale.net> Message-ID: <20101210105653.GI17441@pengutronix.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > > > > 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/ |