| Message ID | 20150716193114.GA17952@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/07/15 20:31, Stephen Boyd wrote: > On 07/16, Sudeep Holla wrote: >> On 08/07/15 02:46, Stephen Boyd wrote: >>> >>> 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. >>> >> >> I am hoping to get this series for v4.3. In order to avoid using >> consumer API, I can revert back to the min,max check I had in the >> round_rate earlier if that's fine with you ? Let me know so that I can >> post the next version based on that. All the other comments are already >> addressed. > > Ok. I'm fine with the consumer API being used, but it would be > nice if we didn't have to do so. Try out the patch below, > hopefully it's good enough for your purposes. It may need to be > more robust, and we may still want to use the init_data structure > to avoid races with providers and consumers, but we can leave > that for later after sweeping all the structure users. > Agreed, I would avoid using clk consumer API or use it with TODO so that I remember to remove it soon. Anyways, thanks for the patch, I tested it and works fine to me. You can add Tested-by if you decide to push it. >> >> Also since this series depends on SCPI, I was thinking to get it merged >> via ARM-SoC, but that might conflict with the round_rate prototype >> change. Do do plan to share a stable base with arm-soc guys or you >> expect all the changes to be contained in clk tree ? >> > > We can share a stable branch for the determine_rate change with > arm-soc. We already have it on a separate branch but haven't > published it so far because nobody has asked. > determine_rate change shouldn't affect SCPI clock driver but I remember seeing round_rate change too on the list which returns value using the argument from Boris. Is that planned for v4.3 ? I would need the stable branch from this clk_hw_set_rate_range if you decide to push. Let me know your preferences. I will post the updated version of the patch accordingly. Regards, Sudeep
On 07/17/2015 04:17 AM, Sudeep Holla wrote: > > > On 16/07/15 20:31, Stephen Boyd wrote: >> On 07/16, Sudeep Holla wrote: >>> On 08/07/15 02:46, Stephen Boyd wrote: >>>> >>>> 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. >>>> >>> >>> I am hoping to get this series for v4.3. In order to avoid using >>> consumer API, I can revert back to the min,max check I had in the >>> round_rate earlier if that's fine with you ? Let me know so that I can >>> post the next version based on that. All the other comments are already >>> addressed. >> >> Ok. I'm fine with the consumer API being used, but it would be >> nice if we didn't have to do so. Try out the patch below, >> hopefully it's good enough for your purposes. It may need to be >> more robust, and we may still want to use the init_data structure >> to avoid races with providers and consumers, but we can leave >> that for later after sweeping all the structure users. >> > > Agreed, I would avoid using clk consumer API or use it with TODO so that > I remember to remove it soon. Anyways, thanks for the patch, I tested it > and works fine to me. You can add Tested-by if you decide to push it. Thanks. I pushed it to -next last night but it probably hasn't shown up yet. > >>> >>> Also since this series depends on SCPI, I was thinking to get it merged >>> via ARM-SoC, but that might conflict with the round_rate prototype >>> change. Do do plan to share a stable base with arm-soc guys or you >>> expect all the changes to be contained in clk tree ? >>> >> >> We can share a stable branch for the determine_rate change with >> arm-soc. We already have it on a separate branch but haven't >> published it so far because nobody has asked. >> > > determine_rate change shouldn't affect SCPI clock driver but I remember > seeing round_rate change too on the list which returns value using the > argument from Boris. Is that planned for v4.3 ? I would need the stable > branch from this clk_hw_set_rate_range if you decide to push. Let me > know your preferences. I will post the updated version of the patch > accordingly. > We're not going to change round_rate() so it sounds like you don't need a stable branch. But you would need this new consumer API. So you still need a branch right?
On 17/07/15 19:13, Stephen Boyd wrote: > On 07/17/2015 04:17 AM, Sudeep Holla wrote: [...] >> >> determine_rate change shouldn't affect SCPI clock driver but I remember >> seeing round_rate change too on the list which returns value using the >> argument from Boris. Is that planned for v4.3 ? I would need the stable >> branch from this clk_hw_set_rate_range if you decide to push. Let me >> know your preferences. I will post the updated version of the patch >> accordingly. >> > > We're not going to change round_rate() so it sounds like you don't need > a stable branch. But you would need this new consumer API. So you still > need a branch right? > I am fine either way. If no one else need the stable branch to be shared with arm-soc, I prefer to use clock consumer API for now to avoid all the troubles to you guys and switch to provider API later. I will post it once the both this driver and that provider API is merged, if that's fine with you ? Regards, Sudeep
On 07/20/2015 01:54 AM, Sudeep Holla wrote: > > > On 17/07/15 19:13, Stephen Boyd wrote: >> On 07/17/2015 04:17 AM, Sudeep Holla wrote: > > [...] > >>> >>> determine_rate change shouldn't affect SCPI clock driver but I remember >>> seeing round_rate change too on the list which returns value using the >>> argument from Boris. Is that planned for v4.3 ? I would need the stable >>> branch from this clk_hw_set_rate_range if you decide to push. Let me >>> know your preferences. I will post the updated version of the patch >>> accordingly. >>> >> >> We're not going to change round_rate() so it sounds like you don't need >> a stable branch. But you would need this new consumer API. So you still >> need a branch right? >> > > I am fine either way. If no one else need the stable branch to be shared > with arm-soc, I prefer to use clock consumer API for now to avoid all > the troubles to you guys and switch to provider API later. I will post > it once the both this driver and that provider API is merged, if that's > fine with you ? Ok. Sounds fine as long as we don't forget.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a1d34a2ed9c6..8760b743bb70 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -58,6 +58,8 @@ struct clk_core { unsigned long flags; unsigned int enable_count; unsigned int prepare_count; + unsigned long min_rate; + unsigned long max_rate; unsigned long accuracy; int phase; struct hlist_head children; @@ -512,8 +514,8 @@ static void clk_core_get_boundaries(struct clk_core *core, { struct clk *clk_user; - *min_rate = 0; - *max_rate = ULONG_MAX; + *min_rate = core->min_rate; + *max_rate = core->max_rate; hlist_for_each_entry(clk_user, &core->clks, clks_node) *min_rate = max(*min_rate, clk_user->min_rate); @@ -522,6 +524,13 @@ static void clk_core_get_boundaries(struct clk_core *core, *max_rate = min(*max_rate, clk_user->max_rate); } +void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate, + unsigned long max_rate) +{ + hw->core->min_rate = min_rate; + hw->core->max_rate = max_rate; +} + /* * Helper for finding best parent to provide a given frequency. This can be used * directly as a determine_rate callback (e.g. for a mux), or from a more @@ -2496,6 +2505,8 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) core->hw = hw; core->flags = hw->init->flags; core->num_parents = hw->init->num_parents; + core->min_rate = 0; + core->max_rate = ULONG_MAX; hw->core = core; /* allocate local copy in case parent_names is __initdata */ diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 2116e2b8a5f2..d62e7eab1dbe 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -619,6 +619,8 @@ int __clk_determine_rate(struct clk_hw *core, struct clk_rate_request *req); int __clk_mux_determine_rate_closest(struct clk_hw *hw, struct clk_rate_request *req); void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent); +void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate, + unsigned long max_rate); static inline void __clk_hw_set_clk(struct clk_hw *dst, struct clk_hw *src) {