diff mbox series

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

Message ID 20240205-pinephone-pll-fixes-v2-3-96a46a2d8c9b@oltmanns.dev
State New
Headers
Series Pinephone video out fixes (flipping between two frames) |

Commit Message

Frank Oltmanns Feb. 5, 2024, 3:22 p.m. UTC 
  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@oltmanns.dev>
---
 drivers/clk/sunxi-ng/ccu_nkm.c | 13 +++++++++++++
 drivers/clk/sunxi-ng/ccu_nkm.h |  2 ++
 2 files changed, 15 insertions(+)

Comments

Jernej Škrabec Feb. 5, 2024, 5:56 p.m. UTC | #1 
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@oltmanns.dev>
> ---
>  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.

> +
>  	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.

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;
>  
> 
>
Frank Oltmanns Feb. 5, 2024, 8:34 p.m. UTC | #2 
On 2024-02-05 at 18:56:09 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> 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@oltmanns.dev>
>> ---
>>  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;
>>
>>
>>
Jernej Škrabec Feb. 6, 2024, 5:51 p.m. UTC | #3 
Dne ponedeljek, 05. februar 2024 ob 21:34:04 CET je Frank Oltmanns napisal(a):
> 
> On 2024-02-05 at 18:56:09 +0100, Jernej Škrabec <jernej.skrabec@gmail.com> 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@oltmanns.dev>
> >> ---
> >>  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.

Good point.

> 
> 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).

Nevermind, this applies only for immediate return.

Best regards,
Jernej

> 
> >
> >> +
> >>  	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;
> >>
> >>
> >>
>
Maxime Ripard Feb. 8, 2024, 12:16 p.m. UTC | #4 
On Mon, Feb 05, 2024 at 04:22:26PM +0100, Frank Oltmanns wrote:
> 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@oltmanns.dev>
> ---
>  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;
> +

This is provided by the clock range already. If you call
clk_hw_set_rate_range, it should work just fine.

Maxime
Frank Oltmanns Feb. 18, 2024, 8:29 a.m. UTC | #5 
Hi Maxime,

On 2024-02-08 at 13:16:27 +0100, Maxime Ripard <mripard@kernel.org> wrote:
> [[PGP Signed Part:Undecided]]
> On Mon, Feb 05, 2024 at 04:22:26PM +0100, Frank Oltmanns wrote:
>> 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@oltmanns.dev>
>> ---
>>  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;
>> +
>
> This is provided by the clock range already. If you call
> clk_hw_set_rate_range, it should work just fine.

I have to admit, that I don't know that much about sunxi-ng or the CCF
and therefore humbly request some guidance.

I've looked at other examples of clk_hw_set_rate_range() usage and it
seems there is not a lot of adoption for this functionality even though
it was already introduced mid-2015. This makes me wonder, why that is.

Anyhow, it seems in all examples I found, clk_hw_set_rate_range() is
called immediately after registering the clk_hw. So, in the case of
sunxi-ng, we'd need to do that in sunxi_ccu_probe, which is a common
function for all sunxi-ng clock types. Correct?

If so, surely, you don't want me to introduce clock type specific code
to a common function, so I assume you want min_rate and max_rate to
become members of struct ccu_common. Correct?

If so, since there already are some clock types in sunxi-ng that support
having a minimum and maximum rate, these clocks should be refactored
eventually. Correct?

Finally, in sunxi-ng there is a feature of having a fixed_post_div (see,
e.g., the first to lines of the diff above). It seems to me that CCF
cannot know about these post_divs, so we'd also need to transfer the
fixed_post_div to ccu_common and use that when calling
clk_hw_set_rate_range. Correct?

The fact that you casually dropped the two sentences above and me
deducing you want a somewhat large refactoring of the functionality for
sunxi-ng, makes me wonder if I completely misunderstood your request.

Best regards,
  Frank

>
> Maxime
>
> [[End of PGP Signed Part]]
Frank Oltmanns Feb. 21, 2024, 10:38 a.m. UTC | #6 
Hi Jernej,
hi Maxime,

On 2024-02-05 at 16:22:26 +0100, Frank Oltmanns <frank@oltmanns.dev> wrote:
> 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.

I should point out that limiting PLL-MIPI also fixes a regression
that was introduced in 6.5, specifically
ca1170b69968233b34d26432245eddf7d265186b "clk: sunxi-ng: a64: force
select PLL_MIPI in TCON0 mux". This has been bisected and reported by
Diego [1].

I don't know the procedure (yet), but probably the fix (if and when
accepted) should be backported at least to 6.6 (first broken LTS), 6.7
(stable), and 6.8 (next stable).

My suggestion:
 - In V3 of this series, I will reorder the patches, so that what is now
   PATCH 3 and 4 becomes 1 and 2 respectively, so that they can be
   applied to 6.6 more easily.
 - Maxime, IIUC you requested some refactoring for handling the rate
   limits [2]. I propose, we use my current proposal as-is, and I will
   do a follow-up series for the refactoring.

Please let me know how you would like me to proceed.

Thanks,
  Frank

[1]: https://groups.google.com/g/linux-sunxi/c/Rh-Uqqa66bw
[2]: https://lore.kernel.org/all/exb2lvjcozak5fayrgyenrd3ntii4jfxgvqork4klyz5pky2aq@dj2zyw5su6pv/

>
> Signed-off-by: Frank Oltmanns <frank@oltmanns.dev>
> ---
>  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;
> +
>  	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;
> +
>  	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;
Maxime Ripard Feb. 22, 2024, 10:26 a.m. UTC | #7 
Hi,

On Sun, Feb 18, 2024 at 09:29:15AM +0100, Frank Oltmanns wrote:
> Hi Maxime,
> 
> On 2024-02-08 at 13:16:27 +0100, Maxime Ripard <mripard@kernel.org> wrote:
> > [[PGP Signed Part:Undecided]]
> > On Mon, Feb 05, 2024 at 04:22:26PM +0100, Frank Oltmanns wrote:
> >> 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@oltmanns.dev>
> >> ---
> >>  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;
> >> +
> >
> > This is provided by the clock range already. If you call
> > clk_hw_set_rate_range, it should work just fine.
> 
> I have to admit, that I don't know that much about sunxi-ng or the CCF
> and therefore humbly request some guidance.
> 
> I've looked at other examples of clk_hw_set_rate_range() usage and it
> seems there is not a lot of adoption for this functionality even though
> it was already introduced mid-2015. This makes me wonder, why that is.

There's no reason, really. I would expect a big part of it to be "if it
works don't fix it" :)

> Anyhow, it seems in all examples I found, clk_hw_set_rate_range() is
> called immediately after registering the clk_hw. So, in the case of
> sunxi-ng, we'd need to do that in sunxi_ccu_probe, which is a common
> function for all sunxi-ng clock types. Correct?

Yup.

> If so, surely, you don't want me to introduce clock type specific code
> to a common function, so I assume you want min_rate and max_rate to
> become members of struct ccu_common. Correct?

Yes, that would be reasonable indeed.

> If so, since there already are some clock types in sunxi-ng that support
> having a minimum and maximum rate, these clocks should be refactored
> eventually. Correct?

I guess. I don't consider it to be a pre-requisite to your patch though.

> Finally, in sunxi-ng there is a feature of having a fixed_post_div (see,
> e.g., the first to lines of the diff above). It seems to me that CCF
> cannot know about these post_divs, so we'd also need to transfer the
> fixed_post_div to ccu_common and use that when calling
> clk_hw_set_rate_range. Correct?

Not really, no. The fixed post divider is an additional divider that
needs to be considered for the clock rate.

See the A64's periph0 PLL for example. Its fixed post divider is 2, and
its rate is 24MHz * N * K / 2. The rate should be bounded by its minimal
and maximal rate taking the post divider into account. The CCF doesn't
have to know about it.

Maxime
Maxime Ripard Feb. 22, 2024, 10:28 a.m. UTC | #8 
On Wed, Feb 21, 2024 at 11:38:39AM +0100, Frank Oltmanns wrote:
> Hi Jernej,
> hi Maxime,
> 
> On 2024-02-05 at 16:22:26 +0100, Frank Oltmanns <frank@oltmanns.dev> wrote:
> > 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.
> 
> I should point out that limiting PLL-MIPI also fixes a regression
> that was introduced in 6.5, specifically
> ca1170b69968233b34d26432245eddf7d265186b "clk: sunxi-ng: a64: force
> select PLL_MIPI in TCON0 mux". This has been bisected and reported by
> Diego [1].
> 
> I don't know the procedure (yet), but probably the fix (if and when
> accepted) should be backported at least to 6.6 (first broken LTS), 6.7
> (stable), and 6.8 (next stable).

https://www.kernel.org/doc/html/next/process/stable-kernel-rules.html#procedure-for-submitting-patches-to-the-stable-tree

> My suggestion:
>  - In V3 of this series, I will reorder the patches, so that what is now
>    PATCH 3 and 4 becomes 1 and 2 respectively, so that they can be
>    applied to 6.6 more easily.
>  - Maxime, IIUC you requested some refactoring for handling the rate
>    limits [2]. I propose, we use my current proposal as-is, and I will
>    do a follow-up series for the refactoring.

I'd really like to not introduce a new ad-hoc implementation of range
handling. It's fine for older users to not be converted yet, but we
shouldn't introduce more users.

Maxime
diff mbox series

Patch

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;
+
 	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;
+
 	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;