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

[Maxime Ripard]

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

Attachment: signature.asc
Description: PGP signature