From mboxrd@z Thu Jan 1 00:00:00 1970 From: richard.zhao@freescale.com (Richard Zhao) Date: Fri, 17 Dec 2010 14:49:20 +0800 Subject: [RFC] i.MX clock support In-Reply-To: <20101215140631.GD6017@pengutronix.de> References: <20101213102538.GB6017@pengutronix.de> <20101213150120.GE26210@pengutronix.de> <20101213154142.GP6017@pengutronix.de> <20101215111237.GA6017@pengutronix.de> <20101215120912.GA1921@b20223-2.ap.freescale.net> <20101215140631.GD6017@pengutronix.de> Message-ID: <20101217064920.GA20902@shlinux1.ap.freescale.net> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Dec 15, 2010 at 03:06:31PM +0100, Sascha Hauer wrote: > On Wed, Dec 15, 2010 at 08:09:12PM +0800, Richard Zhao wrote: > > On Wed, Dec 15, 2010 at 12:12:37PM +0100, Sascha Hauer wrote: > > > On Wed, Dec 15, 2010 at 07:20:08AM +0800, Richard Zhao wrote: > > > > 2010/12/13 Sascha Hauer : > > > > > On Mon, Dec 13, 2010 at 04:01:20PM +0100, Uwe Kleine-K?nig wrote: > > > > >> Hi Sascha, > > > > >> > > > > >> On Mon, Dec 13, 2010 at 11:25:38AM +0100, Sascha Hauer wrote: > > > > >> > I am not willing to accept patches for adding i.MX50 support in the mess > > > > >> > we currently have. These patches offer a way to cleanup the clock support > > > > >> > and the i.MX50 may be a good test bed for an implementation without > > > > >> > old cruft to worry about. That said the following patch is not set in > > > > >> > stone, it's a request for comments and I'm of course open to other > > > > >> > suggestions, but it's clear that we have to do something. > > > > >> Full ack. > > > > >> > > > > >> > +#define to_clk_divider(clk) (container_of(clk, struct clk_divider, clk)) > > > > >> > + > > > > >> > +static unsigned long clk_divider_get_rate(struct clk *clk) > > > > >> > +{ > > > > >> > + ? struct clk_divider *divider = to_clk_divider(clk); > > > > >> > + > > > > >> > + ? unsigned long rate = clk_get_rate(divider->parent); > > > > >> > + ? unsigned int div = 1; > > > > >> > + > > > > >> > + ? if (divider->reg) { > > > > >> > + ? ? ? ? ? div = readl(divider->reg) >> divider->shift; > > > > >> > + ? ? ? ? ? div &= (1 << divider->width) - 1; > > > > >> > + ? ? ? ? ? div++; > > > > >> > + ? } > > > > >> > + > > > > >> > + ? return rate / div / divider->div * divider->mult; > > > > >> Maybe you need to spend more effort to exactness e.g. by using > > > > >> DIV_ROUND_CLOSEST and/or reordering? > > > > >> (You didn't describe div and mult in struct clk_divider (below), so this > > > > >> is a bit guess work for me here.) > > > > > > > > > > Ok, this needs some work. My original idea was to have seperate fixed > > > > > dividers and configurable dividers. Then I decided to combine these into > > > > > one divider. The end result was a mixure of both. We have a struct > > > > > clk_divider_fixed, which is described but unused. > > > > > > > > > >> > > > > >> > +} > > > > >> > + > > > > >> > +static long clk_divider_round_rate(struct clk *clk, unsigned long rate) > > > > >> > +{ > > > > >> > + ? struct clk_divider *divider = to_clk_divider(clk); > > > > >> > + ? unsigned long parent_rate = clk_get_rate(divider->parent); > > > > >> > + ? unsigned int max_div, div; > > > > >> > + > > > > >> > + ? if (rate > parent_rate) > > > > >> > + ? ? ? ? ? return parent_rate; > > > > >> > + > > > > >> > + ? max_div = 1 << divider->width; > > > > >> > + > > > > >> > + ? div = parent_rate / rate; > > > > >> > + ? div = max(div, max_div); > > > > >> > + > > > > >> > + ? return parent_rate / div / divider->div * divider->mult; > > > > >> ditto > > > > >> > > > > >> > +} > > > > >> > + > > > > >> > +static int clk_divider_set_rate(struct clk *clk, unsigned long rate) > > > > >> > +{ > > > > >> > + ? struct clk_divider *divider = to_clk_divider(clk); > > > > >> > + ? unsigned long parent_rate = clk_get_rate(divider->parent); > > > > >> > + ? unsigned int max_div, div; > > > > >> > + ? u32 val; > > > > >> > + > > > > >> > + ? parent_rate /= divider->div; > > > > >> > + ? parent_rate *= divider->mult; > > > > >> > + > > > > >> > + ? if (rate > parent_rate) > > > > >> > + ? ? ? ? ? rate = parent_rate; > > > > >> > + > > > > >> > + ? max_div = 1 << divider->width; > > > > >> > + > > > > >> > + ? div = parent_rate / rate; > > > > >> > + > > > > >> > + ? div = max(div, max_div); > > > > >> > + ? div--; > > > > >> > + > > > > >> > + ? val = readl(divider->reg); > > > > >> > + ? val &= ~(((1 << divider->width) - 1) << divider->shift); > > > > >> > + ? val |= div << divider->shift; > > > > >> > + ? writel(val, divider->reg); > > > > >> > + > > > > >> > + ? return 0; > > > > >> You could spend more efforts here, but I think this is OK for now. > > > > >> > > > > >> > [...] > > > > >> > +struct clk_ops clk_multiplexer_ops = { > > > > >> > + ? .enable = clk_parent_enable, > > > > >> > + ? .disable = clk_parent_disable, > > > > >> > + ? .get_rate = clk_parent_get_rate, > > > > >> > + ? .round_rate = clk_parent_round_rate, > > > > >> > + ? .set_rate = clk_parent_set_rate, > > > > >> Oh, this might have surprising effects if the parent is "public". > > > > >> Is this intended? > > > > > > > > > > I have no idea what the best way is here. We could remove it and wait > > > > > if somebody comes up with a good reason to add it again. > > > > How about adding a child_count. If child_count >1, we stop its child > > > > calling its set_rate/set_parent. In such way, we have to register > > > > every clock, which is easier to debug. child_count maybe none zero > > > > intend, in case there're some clocks in physical we don't set up in > > > > software. > > > > > > Instead of a child count I would rather suggest a flag > > > allowing/disallowing the set_rate function propagating to the parent. > > flag works too. > > > Currently propagating stops at a multiplexer which might be enough for > > > most cases already. > > But some clocks can not set_parent, which mean it doesn't need > > any multiplexer. > > Examples? In case of clocks passed to drivers they all seem to be > multiplexed somewhere. Examples for mx50: gpc_dvfs_clk cpu_clk sdma_clk[0] sdma_clk[1] uart1_clk uart2_clk uart3_clk uart4_clk uart5_clk ... > I think the upper clocks shouldn't be messed with > in drivers, only core and board code should change something here, and > if they do, all kinds of funny things can happen. Yes, we can export as least clock as possible to lookup. >I think it's nearly > impossible to abstract all constrains and possible sideeffects into > the clock framework. > Sascha > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >