Re: [PATCH v4 3/8] clk: add support for clocks provided by SCP(System Control Processor)

[Stephen Boyd <sboyd@codeaurora.org>]

On 07/07, Sudeep Holla wrote:
> 
> 
> On 06/07/15 20:52, Stephen Boyd wrote:
> >>
> >
> >If I have time I may try to start doing the clk_register() conversion,
> >but it will take a while so I doubt it will be in v4.3. I'm asking if
> >you can add a clk_hw based API that does something like
> >clk_set_rate_range() without requiring a struct clk pointer. i.e.
> >clk_hw_set_rate_range(struct clk_hw *hw, min, max) that constraints the
> >min/max rate of the clock. This way, the driver is only using clk
> >provider APIs and not clk consumer APIs.
> >
> 
> I understand the intention of separating clk provider helpers/APIs
> and clk consumer APIs. Since {min,max}_rate are part of struct clk
> itself, I was thinking that you would have moved it to struct clk_core
> as part of the rework you mentioned and hence asked about the patches.
> 
> IIUC, if {min,max}_rate remain part of struct clk, then how are we
> restricting that operation to just the clk providers ? clk consumer
> can still directly modify or use clk_set_rate_range.
> 
> Do we continue to provide that feature for both provider and consumer ?
> If so I assume {min,max}_rate range requested by consumer should be
> within the limits set by provider and do we maintain both the limits ?
> 
> Sorry if I am missing something fundamental since I don't have much
> knowledge of clk layer internals.
> 

Yes struct clk would have min/max, and struct clk_core would have
min/max. Then some sort of provider API (or possibly even
clk_init_data) would take the min/max fields and copy them over
to struct clk_core. Then during set_rate operations we would
aggregate the constraints from struct clk like we already do and
add in the constrains in struct clk_core.

One downside to adding new fields to clk_init_data is that there
are drivers out there that aren't initializing that structure to
0, and they're putting it on the stack, so stack junk can come
through. Furthermore, min/max would mean that every driver needs
to specify some large number for max or we have to special case
min == max == 0 and ignore it. Somehow it needs to be opt-in. If
we want to go down the clk_init_data route then perhaps we need
some sort of rate_constraint struct pointer in there that drivers
can optionally setup.

	struct clk_rate_constraint {
		unsigned long min;
		unsigned long max;
	};

	struct clk_init_data {
		...
		struct clk_rate_constraint *rate_constraint;
	};

I haven't thought it through completely, but I can probably write
up some patch tomorrow after I sleep on it.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project