Re: [PATCH 1/5] clk: sunxi-ng: nkm: Support constraints on m/n ratio and parent rate

From: Frank Oltmanns <frank@oltmanns.dev>
To: "Jernej Škrabec" <jernej.skrabec@gmail.com>
Cc: "Neil Armstrong" <neil.armstrong@linaro.org>,
	"Purism Kernel Team" <kernel@puri.sm>,
	"Samuel Holland" <samuel@sholland.org>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"David Airlie" <airlied@gmail.com>,
	"Michael Turquette" <mturquette@baylibre.com>,
	linux-kernel@vger.kernel.org,
	"Maxime Ripard" <mripard@kernel.org>,
	linux-clk@vger.kernel.org, linux-sunxi@lists.linux.dev,
	"Chen-Yu Tsai" <wens@csie.org>, "Ondrej Jirman" <megi@xff.cz>,
	dri-devel@lists.freedesktop.org,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"Jessica Zhang" <quic_jesszhan@quicinc.com>,
	"Sam Ravnborg" <sam@ravnborg.org>,
	"Guido Günther" <agx@sigxcpu.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/5] clk: sunxi-ng: nkm: Support constraints on m/n ratio and parent rate
Date: Wed, 20 Dec 2023 07:58:07 +0100	[thread overview]
Message-ID: <87msu59wrc.fsf@oltmanns.dev> (raw)
In-Reply-To: <5736273.DvuYhMxLoT@jernej-laptop>

Hi Jernej!

On 2023-12-19 at 17:46:08 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> Hi Frank!
>
> Dne ponedeljek, 18. december 2023 ob 14:35:19 CET je Frank Oltmanns napisal(a):
>> The Allwinner A64 manual lists the following constraints for the
>> PLL-MIPI clock:
>>  - M/N >= 3
>
> This should be "<="

Yes, good catch! I will fix it in V2.

>
>>  - (PLL_VIDEO0)/M >= 24MHz
>>
>> The PLL-MIPI clock is implemented as ccu_nkm. Therefore, add support for
>> these constraints.
>>
>> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
>> ---
>>  drivers/clk/sunxi-ng/ccu_nkm.c | 23 +++++++++++++++++++++++
>>  drivers/clk/sunxi-ng/ccu_nkm.h |  8 ++++++++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
>> index eed64547ad42..2af5c1ebd527 100644
>> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
>> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
>> @@ -16,6 +16,20 @@ struct _ccu_nkm {
>>  	unsigned long	m, min_m, max_m;
>>  };
>>
>> +static bool ccu_nkm_is_valid_rate(struct ccu_common *common, unsigned long parent,
>> +				  unsigned long n, unsigned long m)
>> +{
>> +	struct ccu_nkm *nkm = container_of(common, struct ccu_nkm, common);
>> +
>> +	if (nkm->max_mn_ratio && (m > nkm->max_mn_ratio * n))
>> +		return false;
>> +
>> +	if (nkm->parent_wo_nk && (parent < nkm->parent_wo_nk * m))
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>>  static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common,
>>  						       struct clk_hw *parent_hw,
>>  						       unsigned long *parent, unsigned long rate,
>> @@ -32,6 +46,9 @@ static unsigned long ccu_nkm_find_best_with_parent_adj(struct ccu_common *common
>>
>>  				tmp_parent = clk_hw_round_rate(parent_hw, rate * _m / (_n * _k));
>>
>> +				if (!ccu_nkm_is_valid_rate(common, tmp_parent, _n, _m))
>> +					continue;
>> +
>>  				tmp_rate = tmp_parent * _n * _k / _m;
>>
>>  				if (ccu_is_better_rate(common, rate, tmp_rate, best_rate) ||
>> @@ -65,6 +82,12 @@ static unsigned long ccu_nkm_find_best(unsigned long parent, unsigned long rate,
>>  	for (_k = nkm->min_k; _k <= nkm->max_k; _k++) {
>>  		for (_n = nkm->min_n; _n <= nkm->max_n; _n++) {
>>  			for (_m = nkm->min_m; _m <= nkm->max_m; _m++) {
>> +				if ((common->reg == 0x040) && (_m > 3 * _n))
>> +					break;
>> +
>> +				if ((common->reg == 0x040) && (parent < 24000000 * _m))
>> +					continue;
>> +
>
> You already figured this part.
>
>>  				unsigned long tmp_rate;
>>
>>  				tmp_rate = parent * _n * _k / _m;
>> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
>> index 6601defb3f38..d3d3eaf55faf 100644
>> --- a/drivers/clk/sunxi-ng/ccu_nkm.h
>> +++ b/drivers/clk/sunxi-ng/ccu_nkm.h
>> @@ -16,6 +16,12 @@
>>   * struct ccu_nkm - Definition of an N-K-M clock
>>   *
>>   * Clocks based on the formula parent * N * K / M
>> + *
>> + * @max_mn_ratio:	Maximum value for M / N.
>> + * @parent_wo_nk:	The minimum rate the parent must provide after applying the divisor,
>> + *			but without applying the multipliers, i.e. the contstraint
>> + *			   (parent rate)/M >= parent_wo_nk
>> + *			must be fulfilled.
>>   */
>>  struct ccu_nkm {
>>  	u32			enable;
>> @@ -27,6 +33,8 @@ struct ccu_nkm {
>>  	struct ccu_mux_internal	mux;
>>
>>  	unsigned int		fixed_post_div;
>> +	unsigned long		max_mn_ratio;
>> +	unsigned long           parent_wo_nk;
>
> What about max_m_n_ratio and max_parent_m_ratio, to be consistent? This
> should also allow to simplify description.

Jernej, thank you so much! This is brilliant! I was racking my brain for
a good name but failed. Now, that I see your proposal, I don't know why
I hadn't come up with it. It's the obvious choice.

I'd say with the new names we should be able to get rid of the comments
describing the new struct members (also in ccu_nm.h). What are your
thoughts on that?

Best regards,
  Frank

>
> Best regards,
> Jernej
>
>>
>>  	struct ccu_common	common;
>>  };
>>
>>