On 17 January 2015 at 02:57, Stephen Boyd <sboyd@xxxxxxxxxxxxxx> wrote:
> On 01/12, Tomeu Vizoso wrote:
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 7eddfd8..2793bd7 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -1013,8 +1015,8 @@ static unsigned long clk_core_round_rate_nolock(struct clk_core *clk,
>>
>> if (clk->ops->determine_rate) {
>> parent_hw = parent ? parent->hw : NULL;
>> - return clk->ops->determine_rate(clk->hw, rate, &parent_rate,
>> - &parent_hw);
>> + return clk->ops->determine_rate(clk->hw, rate, 0, ULONG_MAX,
>> + &parent_rate, &parent_hw);
>> } else if (clk->ops->round_rate)
>> return clk->ops->round_rate(clk->hw, rate, &parent_rate);
>> else if (clk->flags & CLK_SET_RATE_PARENT)
>> @@ -1453,8 +1458,20 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *clk,
>>
>> /* find the closest rate and parent clk/rate */
>> if (clk->ops->determine_rate) {
>> + hlist_for_each_entry(clk_user, &clk->clks, child_node) {
>> + floor_rate = max(floor_rate,
>> + clk_user->floor_constraint);
>> + }
>> +
>> + hlist_for_each_entry(clk_user, &clk->clks, child_node) {
>> + ceiling_rate = min(ceiling_rate,
>> + clk_user->ceiling_constraint);
>> + }
>
> I would think we need to do this in the clk_round_rate() path as
> well. We can't just pass 0 and ULONG_MAX there or we'll determine
> one rate here and another rate in round_rate(), violating the
> contract between set_rate() and round_rate().
Right, I have added a test for this.
>> +
>> parent_hw = parent ? parent->hw : NULL;
>> new_rate = clk->ops->determine_rate(clk->hw, rate,
>> + floor_rate,
>> + ceiling_rate,
>> &best_parent_rate,
>> &parent_hw);
>> parent = parent_hw ? parent_hw->core : NULL;
>
> We should enforce a constraint if the clk is using the
> round_rate() op too. If the .round_rate() op returns some rate
> within range it should be ok. Otherwise we can fail the rate
> change because it's out of range.
Ok.
> We'll also need to introduce some sort of
> clk_core_determine_rate(core, rate, min, max) so that clock
> providers can ask parent clocks to find a rate within some range
> that they can tolerate. If we update __clk_mux_determine_rate()
> we can see how that would work out.
Ok, I'm testing this case as well now.
>> @@ -1660,13 +1657,92 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
> [...]
>> + */
>> +int clk_set_rate(struct clk *clk, unsigned long rate)
>> +{
>> + return clk_core_set_rate(clk->core, rate);
>
> clk could be NULL.
>
>> +}
>> EXPORT_SYMBOL_GPL(clk_set_rate);
>>
>> +int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
>> +{
>> + int ret = 0;
>
> Check for NULL clk.
>
>> +
>> +/**
>> + * clk_set_floor_rate - set a minimum clock rate for a clock source
>> + * @clk: clock source
>> + * @rate: desired minimum clock rate in Hz
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_set_floor_rate(struct clk *clk, unsigned long rate)
>> +{
>> + return clk_set_rate_range(clk, rate, clk->ceiling_constraint);
>
> clk could be NULL.
>
>> +}
>> +EXPORT_SYMBOL_GPL(clk_set_floor_rate);
>> +
>> +/**
>> + * clk_set_ceiling_rate - set a maximum clock rate for a clock source
>> + * @clk: clock source
>> + * @rate: desired maximum clock rate in Hz
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_set_ceiling_rate(struct clk *clk, unsigned long rate)
>> +{
>> + return clk_set_rate_range(clk, clk->floor_constraint, rate);
>
> clk could be NULL.
>
>> +}
>> +EXPORT_SYMBOL_GPL(clk_set_ceiling_rate);
>> +
>> static struct clk_core *clk_core_get_parent(struct clk_core *core)
>> {
>> struct clk_core *parent;
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index 2e65419..ae5c800 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -175,9 +175,12 @@ struct clk_ops {
>> unsigned long parent_rate);
>> long (*round_rate)(struct clk_hw *hw, unsigned long rate,
>> unsigned long *parent_rate);
>> - long (*determine_rate)(struct clk_hw *hw, unsigned long rate,
>> - unsigned long *best_parent_rate,
>> - struct clk_hw **best_parent_hw);
>> + long (*determine_rate)(struct clk_hw *hw,
>> + unsigned long rate,
>> + unsigned long floor_rate,
>> + unsigned long ceiling_rate,
>
> I wonder if we should call this min_rate and max_rate?
I have gone ahead and replaced all floor/ceiling instances for
min/max. I don't even remember why I went with the formers any more.
>> + unsigned long *best_parent_rate,
>> + struct clk_hw **best_parent_hw);
>> int (*set_parent)(struct clk_hw *hw, u8 index);
>> u8 (*get_parent)(struct clk_hw *hw);
>> int (*set_rate)(struct clk_hw *hw, unsigned long rate,
>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>> index c7f258a..f99ae67 100644
>> --- a/include/linux/clk.h
>> +++ b/include/linux/clk.h
>> @@ -302,6 +302,34 @@ long clk_round_rate(struct clk *clk, unsigned long rate);
>> int clk_set_rate(struct clk *clk, unsigned long rate);
>>
>> /**
>> + * clk_set_rate_range - set a rate range for a clock source
>> + * @clk: clock source
>> + * @min: desired minimum clock rate in Hz
>> + * @max: desired maximum clock rate in Hz
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max);
>> +
>> +/**
>> + * clk_set_floor_rate - set a minimum clock rate for a clock source
>> + * @clk: clock source
>> + * @rate: desired minimum clock rate in Hz
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_set_floor_rate(struct clk *clk, unsigned long rate);
>
> And this called clk_set_max_rate()?
>
>> +
>> +/**
>> + * clk_set_ceiling_rate - set a maximum clock rate for a clock source
>> + * @clk: clock source
>> + * @rate: desired maximum clock rate in Hz
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_set_ceiling_rate(struct clk *clk, unsigned long rate);
>
> And this called clk_set_min_rate()?
>
>> +
>> +/**
>> * clk_set_parent - set the parent clock source for this clock
>> * @clk: clock source
>> * @parent: parent clock source
Thanks for the review!
Tomeu
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> 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/
