[PATCH RFC] clk: add support for automatic parent handling

[Uwe Kleine-König]

Hello Thomas,

On Wed, Apr 20, 2011 at 09:52:15PM +0200, Thomas Gleixner wrote:
> On Wed, 20 Apr 2011, Uwe Kleine-König wrote:
> > On Wed, Apr 20, 2011 at 06:16:39PM +0200, Thomas Gleixner wrote:
> > > And this whole mess should be written:
> > > 
> > >     ret = clk_prepare(clk->parent);
> > >     if (ret)
> > > 		return ret;
> > > 
> > > Which returns 0 when there is no parent and it also returns 0 when
> > > there is no prepare callback for the parent. Why the hell do we need
> > > all this ERRPTR checking mess and all this conditional crap ?
> >
> > struct clk has no parent member, there is only clk_get_parent(). If
> 
> Which is a complete failure to begin with. Why the heck do you need a
> callback to figure that out?
> 
> Because someone claimed, that you need a callback to make it safe from
> changing? Or what's the reason for this?
If there were a pointer tracking the parent you still needed to program
out the get_parent function anyhow to set the pointer initially. But I
agree that in other situations having a pointer is more comfortable and
saves ugly error handling e.g. in set_parent.

> > there is no parent it returns ERR_PTR(-ENOSYS) and if you pass that
> > to clk_prepare it tries to dereference it. So either it must not be
> > called with an error pointer or clk_prepare et al. need adaption to
> > handle
> 
> The whole clk struct is an utter failure.
> 
> It's simply the least common denominator of all clk madness in the
> tree. Which results in a half documented data structure and a handful
> helper functions of which most of them are exported, though the
> functionality in them is less than the overhead of the EXPORT_SYMBOL.
> 
> That's not an abstraction and neither it's a framework. It's a half
> arsed conglomorate of primitives w/o the minimal documentation how
> those primitives should be used and why they are there in the first
> place.
> 
> This is new code and it should be aimed to cleanup things not to
> shuffle things around in a frenzy.
> 
> So what's wrong with that code:
> 
> 1) struct clk is just a collection of function pointers and two locks
> 
>    It lacks:
>    
>    - a flag field for properties
Up to now there are no properties to distinguish. When the first comes
up such a field is easily added, no?

>    - a field for the parent
I can agree here, see above.

>    - a field for the current clock rate
Then you need a mechanism to notify the children if the rate or parent
of a clock changes. I assume you won't allow that for a clock that has
more than one (enabled) consumer. Knowing only very few platforms I
don't know if that is something you can forbid.

>    - a field for the base register
This is not needed for all clocks (at least not a say void __iomem *). I
think it's fine here to leave that to the platforms as it is now.

>    - a struct for the offsets of the most common registers relative to
>      base
I'm not sure I understand you right here. If this is about not
duplicating data/code for similar clocks, this works (IMHO) fine with
the current approach (ie. sharing the ops pointer).

>    optionally a set of common register accessor functions like I did
>    for the generic irq chip.
Agreed, this can (and should) come when the base API is fixed.

> 2) The "framework" API is just a set of low level primitive helper
>    functions
> 
>    It lacks:
> 
>    - proper refcounting. clk_get() / clk_put() should do that at the
>      framework level.
For most clocks it's not important if they are referenced somewhere.
What should be done with that reference count at the framework level?
The only thing I currently can come up with now is to module_get an
optional owner. I'm not convinced this is worth to include in the
framework. Still it should be possible to do, and it is.
 
>    - the ability to propagate enable/disable/prepare/unprepare down
>      through the parent tree
Ack, I'm working on that.

>    - proper mechanisms to sanity check clk_set_parent(),
>      clk_set_rate() et. al.
> 
>      This is not a implementation detail inside a specific clock. It's
>      a problem which is common at least on the tree level:
> 
>                     rootclk
>                  /          \
>               clk1          clk2   
>              /   \
>            clk3  clk4
>           /
>          clk5
> 
>     So now you want to change the rate of clk1. Can you do that?
>     
>     No, but where is the sanity check which prevents that ?
I assume your ascii art should imply that clk3 and clk4 are enabled?
What is the policy you suggest?
This was discussed at some point[1] with the result that the user of
set_rate should be aware of the clk tree and the changes it introduces
for now and that some policy could still be added later.  Ditto for
set_parent. 
It's easy enough to implement such a policy on top of Jeremy's patches.
I'm not able to see already now all the requirements of all users of the
clk-API. So I agree with you that it probably isn't (yet) the perfect
framework, but I think it's a good base to see these requirements.

>         In the clk1->set_rate() callback ?
> 
> 	Great, that's the wrong place.
> 
>     So now you want to switch the parent of clk3 from clk1 to
>     clk2. Can you do that?
> 
>     No, but where is the sanity check which prevents that ?
> 
>     	In the clk3->set_parent() callback ?
> 
> 	Again, the wrong place.
> 
>     And these are not problems of a particular clk implementation,
>     these are general problems and those want to be addressed in a
>     real framework.
> 
>     It's debatable, whether you want to be able to change clocks which
>     have active childs or not. If not the decision function is pretty
>     simple. If yes, you need a list of child clks which you want to
>     consult first before committing the change.
> 
> So unless you fix this stuff you should not even think about
> converting anything to this "framework". That's just waste of time and
> effort. Aside of declaring it stable and useful ....
With stable I didn't intend to say something about the quality (though I
still think it's a good thing), but that I won't rewrite the commits
such that others can base their work on top of them.
Before your mail nobody spoke up against these patches and some people
see a value in them and want to convert their platform to it. So
providing a branch seems sensible to me.
I don't say that this common struct clk is perfect already now and I
expect it to change here and there. It allows to consolidate some code
and so is a good thing. I think nobody disagrees that when having
converted most or all platforms to it we're not done. It's just the
first step. Maybe one of the next is to add a "class" of clocks that
looks similar to this:

	struct clk_generic {
		struct clk clk;
		unsigned long rate;
		struct clk *parent;
		void __iomem *iobase;

		sometype enable_regoffset;
		size_t enable_shift;

		struct clk **possible_parents;
		size_t num_possible_parents;
		sometype parent_regoffset;
		size_t parent_shift;
	};

and that could be used for a big part of the needed clocks.

Best regards
Uwe

[1] see for example
    http://thread.gmane.org/gmane.linux.ports.sh.devel/9755/focus=1100223
    and the replies to it.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |