[RFC] i.MX clock support

[Richard Zhao]

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 <s.hauer at pengutronix.de>:
> > > > > 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 |
>