Re: [PATCH v2 3/6] clk: sunxi-ng: nkm: Support minimum and maximum rate

[Frank Oltmanns]

On 2024-02-05 at 18:56:09 +0100, Jernej Škrabec <jernej.skrabec@xxxxxxxxx> wrote:
> Dne ponedeljek, 05. februar 2024 ob 16:22:26 CET je Frank Oltmanns napisal(a):
>> According to the Allwinner User Manual, the Allwinner A64 requires
>> PLL-MIPI to run at 500MHz-1.4GHz. Add support for that to ccu_nkm.
>>
>> Signed-off-by: Frank Oltmanns <frank@xxxxxxxxxxxx>
>> ---
>>  drivers/clk/sunxi-ng/ccu_nkm.c | 13 +++++++++++++
>>  drivers/clk/sunxi-ng/ccu_nkm.h |  2 ++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.c b/drivers/clk/sunxi-ng/ccu_nkm.c
>> index 1168d894d636..7d135908d6e0 100644
>> --- a/drivers/clk/sunxi-ng/ccu_nkm.c
>> +++ b/drivers/clk/sunxi-ng/ccu_nkm.c
>> @@ -181,6 +181,12 @@ static unsigned long ccu_nkm_round_rate(struct ccu_mux_internal *mux,
>>  	if (nkm->common.features & CCU_FEATURE_FIXED_POSTDIV)
>>  		rate *= nkm->fixed_post_div;
>>
>> +	if (nkm->min_rate && rate < nkm->min_rate)
>> +		rate = nkm->min_rate;
>> +
>> +	if (nkm->max_rate && rate > nkm->max_rate)
>> +		rate = nkm->max_rate;
>
> Please take a look at ccu_nm_round_rate() code. You need to consider postdiv
> and you can return immediately.

There is a difference here insofar that ccu_nm is always connected to a
fixed rate parent (at least that's my understanding). Therefore, in
ccu_nm_round_rate() we can be sure that the min or max rate can really
be set. In ccu_nkm we don't have that luxury, we actually have to find a
rate that is approximately equal to the min and max rate, based on the
parent rate. Therefore, we can't return immediately.

Also, I'm not sure what you mean about me needing to consider postdiv.
That's what I did. The check is after multiplying with the postdiv. It's
the same as in ccu_nm_round_rate() (just minus the immediate return).

>
>> +
>>  	if (!clk_hw_can_set_rate_parent(&nkm->common.hw))
>>  		rate = ccu_nkm_find_best(*parent_rate, rate, &_nkm, &nkm->common);
>>  	else
>> @@ -220,6 +226,13 @@ static int ccu_nkm_set_rate(struct clk_hw *hw, unsigned long rate,
>>  	_nkm.min_m = 1;
>>  	_nkm.max_m = nkm->m.max ?: 1 << nkm->m.width;
>>
>> +
>> +	if (nkm->min_rate && rate < nkm->min_rate)
>> +		rate = nkm->min_rate;
>> +
>> +	if (nkm->max_rate && rate > nkm->max_rate)
>> +		rate = nkm->max_rate;
>> +
>
> No need for this, clk subsystem calls round rate before setting actual clock
> rate.

I'll remove the checks in V3.

Best regards,
  Frank

>
> Best regards,
> Jernej
>
>>  	ccu_nkm_find_best(parent_rate, rate, &_nkm, &nkm->common);
>>
>>  	spin_lock_irqsave(nkm->common.lock, flags);
>> diff --git a/drivers/clk/sunxi-ng/ccu_nkm.h b/drivers/clk/sunxi-ng/ccu_nkm.h
>> index c409212ee40e..358a9df6b6a0 100644
>> --- a/drivers/clk/sunxi-ng/ccu_nkm.h
>> +++ b/drivers/clk/sunxi-ng/ccu_nkm.h
>> @@ -27,6 +27,8 @@ struct ccu_nkm {
>>  	struct ccu_mux_internal	mux;
>>
>>  	unsigned int		fixed_post_div;
>> +	unsigned long		min_rate;
>> +	unsigned long		max_rate;
>>  	unsigned long		max_m_n_ratio;
>>  	unsigned long		min_parent_m_ratio;
>>
>>
>>