Date:   Fri, 24 Feb 2023 10:49:07 +0800
From:   Yu Tu <yu.tu@...ogic.com>
To:     Jerome Brunet <jbrunet@...libre.com>, linux-clk@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Kevin Hilman <khilman@...libre.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc:     kelvin.zhang@...ogic.com, qi.duan@...ogic.com
Subject: Re: [PATCH] clk: meson: vid-pll-div: added meson_vid_pll_div_ops
 support to enable vid_pll_div to meet clock setting requirements, especially
 for late chip



On 2023/2/23 18:11, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 

Hi Jerome,

> 
> On Thu 23 Feb 2023 at 14:27, Yu Tu <yu.tu@...ogic.com> wrote:
> 
> Title is way too long, 75 char max

I will change to "clk: meson: vid-pll-div: added meson_vid_pll_div_ops 
support". I wonder if you have a better suggestion, please let me know 
if you have.

> 
>> The previous chip only provides "ro_ops" for the vid_pll_div clock,
> 
> The driver does. Other chip could use RW ops I suppose.

Your suppose is right.

> 
>> which is not satisfied with the operation requirements of the later
>> chip for this clock, so the ops that can be set for the clock is added.
>>
> 
> What requirements ? What "late" chip ? all this is quite vague.

I will change to "Since the previous code only provides "ro_ops" for the 
vid_pll_div clock,In fact, the clock can be set. So add "ops" that can 
set the clock, especially for later chips like S4 SOC and so on."

Is that ok with you?

> 
>> Signed-off-by: Yu Tu <yu.tu@...ogic.com>
>> ---
>>   drivers/clk/meson/vid-pll-div.c | 59 +++++++++++++++++++++++++++++++++
>>   drivers/clk/meson/vid-pll-div.h |  1 +
>>   2 files changed, 60 insertions(+)
>>
>> diff --git a/drivers/clk/meson/vid-pll-div.c b/drivers/clk/meson/vid-pll-div.c
>> index daff235bc763..e75fa6f75efe 100644
>> --- a/drivers/clk/meson/vid-pll-div.c
>> +++ b/drivers/clk/meson/vid-pll-div.c
>> @@ -89,6 +89,65 @@ static unsigned long meson_vid_pll_div_recalc_rate(struct clk_hw *hw,
>>   	return DIV_ROUND_UP_ULL(parent_rate * div->multiplier, div->divider);
>>   }
>>   
>> +static int meson_vid_pll_div_determine_rate(struct clk_hw *hw,
>> +					    struct clk_rate_request *req)
>> +{
>> +	unsigned long best = 0, now = 0;
>> +	unsigned int i, best_i = 0;
>> +
>> +	for (i = 0 ; i < ARRAY_SIZE(vid_pll_div_table) ; ++i) {
> 
> It would be nice to actually describe how this vid pll work so we can
> stop using precompute "magic" values and actually use the IP to its full
> capacity.

Thank you for your advice. I'm going to define a macro to represent this 
table size.

> 
>> +		now = DIV_ROUND_CLOSEST_ULL(req->best_parent_rate *
> 
> This effectively stops rate propagation. That's not how determine_rate()
> call back should work. Have a look a clk-divider.c and how it calls
> clk_hw_round_rate().
> 

I understand that this should be changed to
" parent_rate = clk_hw_round_rate(req->best_parent_hw,
				 DIV_ROUND_CLOSEST_ULL(rate * vid_pll_div_table[i].divider, 
vid_pll_div_table[i].multiplier));

now = DIV_ROUND_CLOSEST_ULL(parent_rate * 
vid_pll_div_table[i].multiplier, vid_pll_div_table[i].divider);"

I don't know if it is correct, please give me a comment.

>> +					    vid_pll_div_table[i].multiplier,
>> +					    vid_pll_div_table[i].divider);
>> +		if (req->rate == now) {
>> +			return 0;
>> +		} else if (abs(now - req->rate) < abs(best - req->rate)) {
>> +			best = now;
>> +			best_i = i;
>> +		}
>> +	}
>> +
>> +	if (best_i < ARRAY_SIZE(vid_pll_div_table))
>> +		req->rate = DIV_ROUND_CLOSEST_ULL(req->best_parent_rate *
>> +						  vid_pll_div_table[best_i].multiplier,
>> +						  vid_pll_div_table[best_i].divider);
>> +	else
> 
> What is the point of this 'if' clause ?
> It looks like the 'else' part is dead code.

I'm going to delete these.

> 
>> +		req->rate = meson_vid_pll_div_recalc_rate(hw, req->best_parent_rate);
>> +
>> +	return 0;
>> +}
>> +
>> +static int meson_vid_pll_div_set_rate(struct clk_hw *hw, unsigned long rate,
>> +				      unsigned long parent_rate)
>> +{
>> +	struct clk_regmap *clk = to_clk_regmap(hw);
>> +	struct meson_vid_pll_div_data *pll_div = meson_vid_pll_div_data(clk);
>> +	int i;
>> +
>> +	for (i = 0 ; i < ARRAY_SIZE(vid_pll_div_table) ; ++i) {
>> +		if (DIV_ROUND_CLOSEST_ULL(parent_rate * vid_pll_div_table[i].multiplier,
>> +					  vid_pll_div_table[i].divider) == rate) {
> 
> This assumes the set_rate() is going to have a perfect match and
> otherwise fail. You should not assume that. Have a look at clk-divider.c
> for examples.

Thank you for your advice. I will do a bestdiv match like the 
clk-divider.c file.

> 
>> +			meson_parm_write(clk->map, &pll_div->val, vid_pll_div_table[i].shift_val);
>> +			meson_parm_write(clk->map, &pll_div->sel, vid_pll_div_table[i].shift_sel);
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (i >= ARRAY_SIZE(vid_pll_div_table)) {
>> +		pr_debug("%s: Invalid rate value for vid_pll_div\n", __func__);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +const struct clk_ops meson_vid_pll_div_ops = {
>> +	.recalc_rate	= meson_vid_pll_div_recalc_rate,
>> +	.determine_rate	= meson_vid_pll_div_determine_rate,
>> +	.set_rate	= meson_vid_pll_div_set_rate,
>> +};
>> +EXPORT_SYMBOL_GPL(meson_vid_pll_div_ops);
>> +
>>   const struct clk_ops meson_vid_pll_div_ro_ops = {
>>   	.recalc_rate	= meson_vid_pll_div_recalc_rate,
>>   };
>> diff --git a/drivers/clk/meson/vid-pll-div.h b/drivers/clk/meson/vid-pll-div.h
>> index c0128e33ccf9..3ab729b85fde 100644
>> --- a/drivers/clk/meson/vid-pll-div.h
>> +++ b/drivers/clk/meson/vid-pll-div.h
>> @@ -16,5 +16,6 @@ struct meson_vid_pll_div_data {
>>   };
>>   
>>   extern const struct clk_ops meson_vid_pll_div_ro_ops;
>> +extern const struct clk_ops meson_vid_pll_div_ops;
>>   
>>   #endif /* __MESON_VID_PLL_DIV_H */
>>
>> base-commit: 8a9fbf00acfeeeaac8efab8091bb464bd71b70ea
> 

Powered by blists - more mailing lists