Re: [PATCH 2/4] clk: Implement clk_set_rate

From: Sascha Hauer
Date: Thu May 26 2011 - 02:54:37 EST


[put the lists back on cc]

On Thu, May 26, 2011 at 09:37:47AM +0800, Jeremy Kerr wrote:
> Hi Sascha,
>
> > > +
> > > +propagate:
> > > + ret = clk->ops->set_rate(clk->hw, new_rate, &parent_rate);
> > > +
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /* ops->set_rate may require the parent's rate to change (to
> > > + * parent_rate), we need to propagate the set_rate call to the
> > > + * parent.
> > > + */
> > > + if (ret == CLK_SET_RATE_PROPAGATE) {
> > > + new_rate = parent_rate;
> > > + clk = clk->parent;
> > > + goto propagate;
> > > + }
> >
> > I'm unsure about this one. Every clock should have the ability to stop
> > or continue the rate propagation to the parent. This suggests to leave
> > the decision whether or not to propagate to the core and not to the
> > individual clocks.
>
> Maybe I'm not understanding you correctly, but that's exactly what this
> code does. The decision to propagate is left up to the
> implementation-specific set_rate callback - if it returns
> CLK_SET_RATE_PROPAGATE (and populate the parent_rate argument with the
> requested parent rate), then we propagate the rate change to the parent.

I understood how the code is meant to work. It's just that IMO the place
where the propagation flag is stored is the wrong one, given that it's a
flag that all clocks (can) have.

>
> > Right now each mux/div/gate needs an individual propagate flag. By
> > adding the flag to the core the building block implementations could be
> > simpler and the knowledge about propagatability might become handy for
> > the core later.
>
> We could do this with a flag too, yes. But then there's no way of
> altering the rate (which we need to do with a divider) as we propagate
> it upwards. The current set_rate code lets us do that.

Hm, the core could pass a NULL pointer as the third argument to
set_rate to indicate that the parent rate is not allowed to change.
Then we could initialize &parent_rate to zero before calling set_rate.
If the set_rate function does not change it, we don't have to propagate,
otherwise yes. Or instead we could just use the CLK_SET_RATE_PROPAGATE
return value like we do in the current version.

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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/