Re: [PATCH 2/4] clk: ti: fapll: Fix clock speed calculation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: ti: fapll: set_frac_rate uses incorrect 10000000 base causing clock mismatch
- [High] clk: ti: fapll: mismatched set/get fractional bases and off-by-one denominator
--

commit 1dcae77615d6eede1d731b41214f02cb5834c02c
Author: Andrew Goodbody <andrew.goodbody@linaro.org>

clk: ti: fapll: Fix clock speed calculation

The fractional part of the divider was being treated as 1/10000000 wheras
it is in fact 1/16777215. This led to incorrect clock speeds being
reported when a fractional divider was in use. Correct it.

> The fractional part of the divider was being treated as 1/10000000 wheras

[Severity: Low]
This isn't a bug, but there is a typo (wheras) in the commit message.

diff --git a/drivers/clk/ti/fapll.c b/drivers/clk/ti/fapll.c
index ae2f6fed10f66..008a61f45ac14 100644
--- a/drivers/clk/ti/fapll.c
+++ b/drivers/clk/ti/fapll.c
@@ -336,8 +336,8 @@ static unsigned long ti_fapll_synth_recalc_rate(struct clk_hw *hw,
 		v = readl_relaxed(synth->freq);
 		synth_int_div = (v >> 24) & 0xf;
 		synth_frac_div = v & 0xffffff;
-		synth_div_freq = (synth_int_div * 10000000) + synth_frac_div;
-		rate *= 10000000;
+		synth_div_freq = (synth_int_div * 16777215) + synth_frac_div;
+		rate *= 16777215;

[Severity: High]
Is the new 16777215 denominator an off-by-one error?

Standard 24-bit hardware dividers typically use a base of 16,777,216 (2^24).
While 0xffffff is the maximum 24-bit value, the fractional denominator is
usually the full 2^24.

[Severity: High]
Does this create a mismatch between calculating the rate and setting the rate?

Looking at ti_fapll_synth_set_frac_rate() which was not updated in this patch,
it still uses the 10000000 base to calculate the divider:

drivers/clk/ti/fapll.c:ti_fapll_synth_set_frac_rate() {
	for (; post_div_m < SYNTH_MAX_DIV_M; post_div_m++) {
		synth_int_div = DIV_ROUND_UP_ULL((u64)parent_rate *
						 SYNTH_PHASE_K *
						 10000000,
						 rate * post_div_m);
		synth_frac_div = synth_int_div % 10000000;
		synth_int_div /= 10000000;

		if (synth_int_div <= SYNTH_MAX_INT_DIV)
			break;
	}
	...
}

If a clock rate is set using ti_fapll_synth_set_frac_rate(), the programmed
hardware fractional divider will use the 10000000 base. However,
ti_fapll_synth_recalc_rate() will interpret that same register using the new
16777215 denominator. Should ti_fapll_synth_set_frac_rate() also be updated
to use the new denominator?

 		do_div(rate, synth_div_freq);
 		rate *= SYNTH_PHASE_K;
 	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-patch-ektor5-dm8168-clk-v1-0-8b99c5cd5541@linaro.org?part=2