[PATCH v2 0/4] clk: ti: fapll/dm816x: clock tree and calculation fixes

[PATCH v2 0/4] clk: ti: fapll/dm816x: clock tree and calculation fixes

[Ettore Chimenti]

During TI DM8168-EVM board testing we found that the clock hierarchy and
clock calculation was broken already on 6.12:

    main_pll_clk1       <--- main PLL should be parent of other PLLs.
       sysclk1_ck                   
       main_pll_clk7            
		...
       main_pll_clk6            
		...
       main_pll_clk5            
		...
       main_pll_clk4            
		...

This series brings fixes and additions for TI DM816x / FAPLL clock tree
and frequency calculation.

Summary:
- Fix clock tree hierarchy by not using ti_dt_clk_name() helper, that
  causes the first PLL as the parent of all clocks in DM816x.
- Correct rate calculation logic in fapll driver (1/2^24, was 1/10^7).
- Add sys_ck alias required for DM816x clock lookup, fixing a boot
  error.
- Add error logging for clocks failing registration to ease debugging.

Fixing clock tree hierarchy is critical as it fixes DM8168-EVM boot
failure in 7.3-rc7. These fixes are affecting DM816x SoCs only.

Signed-off-by: Ettore Chimenti <ettore.chimenti@linaro.org>
---
Changes in v2:
- Fixed off-by-one error in clock calculation (Sashiko).
- Fixed clock calculation also in the set rate path.
- Added Fixes tag to clock calculation fix patch.
- Link to v1: https://patch.msgid.link/20261015-patch-ektor5-dm8168-clk-v1-0-8b99c5cd5541@linaro.org

---
Andrew Goodbody (1):
      clk: ti: fapll: Fix clock speed calculation

Ettore Chimenti (1):
      clk-816x: add sys_ck clock as sys_clkin_ck alias

Rouven Czerwinski (2):
      clk: ti: fapll: remove ti_dt_clk_name helper
      clk: ti: fapll: print clock name that fails to register

 drivers/clk/ti/clk-816x.c |  1 +
 drivers/clk/ti/fapll.c    | 22 +++++++++-------------
 2 files changed, 10 insertions(+), 13 deletions(-)
---
base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
change-id: 20261014-patch-ektor5-dm8168-clk-e426101415b0

Best regards,
--  
Ettore Chimenti <ettore.chimenti@linaro.org>

[PATCH v2 1/4] clk: ti: fapll: remove ti_dt_clk_name helper

[Ettore Chimenti]

From: Rouven Czerwinski <rouven.czerwinski@linaro.org>

The fapll driver uses multiple `clock-output-names`, so we cannot use the
DT helper in this driver because that would assign the first clock
output name as the parent name. This change do not affect other SoCs as
the only consumer is DM816x.

Fixes: 9e56a7d4263c ("clk: ti: Update pll and clockdomain clocks to use ti_dt_clk_name()")
Signed-off-by: Rouven Czerwinski <rouven.czerwinski@linaro.org>
Tested-by: Andrew Goodbody <andrew.goodbody@linaro.org>
Signed-off-by: Ettore Chimenti <ettore.chimenti@linaro.org>
---
 drivers/clk/ti/fapll.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/ti/fapll.c b/drivers/clk/ti/fapll.c
index e54b1df3436e..ae2f6fed10f6 100644
--- a/drivers/clk/ti/fapll.c
+++ b/drivers/clk/ti/fapll.c
@@ -10,8 +10,6 @@
 #include <linux/of_address.h>
 #include <linux/clk/ti.h>
 
-#include "clock.h"
-
 /* FAPLL Control Register PLL_CTRL */
 #define FAPLL_MAIN_MULT_N_SHIFT	16
 #define FAPLL_MAIN_DIV_P_SHIFT	8
@@ -541,7 +539,6 @@ static void __init ti_fapll_setup(struct device_node *node)
 	struct clk_init_data *init = NULL;
 	const char *parent_name[2];
 	struct clk *pll_clk;
-	const char *name;
 	int i;
 
 	fd = kzalloc_obj(*fd);
@@ -559,8 +556,7 @@ static void __init ti_fapll_setup(struct device_node *node)
 		goto free;
 
 	init->ops = &ti_fapll_ops;
-	name = ti_dt_clk_name(node);
-	init->name = name;
+	init->name = node->name;
 
 	init->num_parents = of_clk_get_parent_count(node);
 	if (init->num_parents != 2) {
@@ -592,7 +588,7 @@ static void __init ti_fapll_setup(struct device_node *node)
 	if (fapll_is_ddr_pll(fd->base))
 		fd->bypass_bit_inverted = true;
 
-	fd->name = name;
+	fd->name = node->name;
 	fd->hw.init = init;
 
 	/* Register the parent PLL */
@@ -639,7 +635,7 @@ static void __init ti_fapll_setup(struct device_node *node)
 				freq = NULL;
 		}
 		synth_clk = ti_fapll_synth_setup(fd, freq, div, output_instance,
-						 output_name, name, pll_clk);
+						 output_name, node->name, pll_clk);
 		if (IS_ERR(synth_clk))
 			continue;
 

-- 
2.56.0

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

[Ettore Chimenti]

From: Andrew Goodbody <andrew.goodbody@linaro.org>

The fractional part of the divider was being treated as 1/10000000
whereas it is in fact 1/2^24 (1/16777216). This led to incorrect clock
speeds being reported when a fractional divider was in use. Correct it.
As well as fixing the clock speed calculation when read, also update the
calculation on set in a similar manner.

Fixes: 163152cbbe32 ("clk: ti: Add support for FAPLL on dm816x")
Signed-off-by: Andrew Goodbody <andrew.goodbody@linaro.org>
Signed-off-by: Ettore Chimenti <ettore.chimenti@linaro.org>
---
 drivers/clk/ti/fapll.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/ti/fapll.c b/drivers/clk/ti/fapll.c
index ae2f6fed10f6..8ab9acd57b60 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 * 16777216) + synth_frac_div;
+		rate *= 16777216;
 		do_div(rate, synth_div_freq);
 		rate *= SYNTH_PHASE_K;
 	}
@@ -378,10 +378,10 @@ static u32 ti_fapll_synth_set_frac_rate(struct fapll_synth *synth,
 	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,
+						 16777216,
 						 rate * post_div_m);
-		synth_frac_div = synth_int_div % 10000000;
-		synth_int_div /= 10000000;
+		synth_frac_div = synth_int_div % 16777216;
+		synth_int_div /= 16777216;
 
 		if (synth_int_div <= SYNTH_MAX_INT_DIV)
 			break;

-- 
2.56.0

[PATCH v2 3/4] clk: ti: fapll: print clock name that fails to register

[Ettore Chimenti]

From: Rouven Czerwinski <rouven.czerwinski@linaro.org>

Improve the error message when a clock fails to register by including
the clock's name and its parent's name for better debugging.

Signed-off-by: Rouven Czerwinski <rouven.czerwinski@linaro.org>
Signed-off-by: Ettore Chimenti <ettore.chimenti@linaro.org>
---
 drivers/clk/ti/fapll.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/ti/fapll.c b/drivers/clk/ti/fapll.c
index 8ab9acd57b60..d360e9d986c3 100644
--- a/drivers/clk/ti/fapll.c
+++ b/drivers/clk/ti/fapll.c
@@ -520,7 +520,7 @@ static struct clk * __init ti_fapll_synth_setup(struct fapll_data *fd,
 
 	clk = clk_register(NULL, &synth->hw);
 	if (IS_ERR(clk)) {
-		pr_err("failed to register clock\n");
+		pr_err("failed to register clock '%s' for parent '%s'\n", name, parent);
 		goto free;
 	}
 

-- 
2.56.0

[PATCH v2 4/4] clk-816x: add sys_ck clock as sys_clkin_ck alias

[Ettore Chimenti]

This patch adds a new clock alias for the DM816x system, mapping
'sys_ck' to 'sys_clkin_ck'. This change is necessary because the voltage
management driver searches for 'sys_ck' in these systems and fails
without this alias. By including 'sys_ck' as an alias for
'sys_clkin_ck', we ensure compatibility with existing drivers that may
reference 'sys_ck'.

Tested-by: Andrew Goodbody <andrew.goodbody@linaro.org>
Signed-off-by: Ettore Chimenti <ettore.chimenti@linaro.org>
---
 drivers/clk/ti/clk-816x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/ti/clk-816x.c b/drivers/clk/ti/clk-816x.c
index 3b8e483aec92..fe6f66fc5a05 100644
--- a/drivers/clk/ti/clk-816x.c
+++ b/drivers/clk/ti/clk-816x.c
@@ -57,6 +57,7 @@ static struct ti_dt_clk dm816x_clks[] = {
 	DT_CLK(NULL, "timer_sys_ck", "sys_clkin_ck"),
 	DT_CLK(NULL, "timer_32k_ck", "sysclk18_ck"),
 	DT_CLK(NULL, "timer_ext_ck", "tclkin_ck"),
+	DT_CLK(NULL, "sys_ck", "sys_clkin_ck"),
 	{ .node_name = NULL },
 };
 

-- 
2.56.0

Re: [PATCH v2 4/4] clk-816x: add sys_ck clock as sys_clkin_ck alias

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: ti: dm816x: adding sys_ck alias triggers invalid OMAP3 VC/VP init
--

commit 1a28e92700d48db3c2d33b03820789ccb46fb7f1
Author: Ettore Chimenti <ettore.chimenti@linaro.org>

clk-816x: add sys_ck clock as sys_clkin_ck alias

This commit adds a clock alias mapping sys_ck to sys_clkin_ck in
dm816x_clks[] so that the OMAP voltage management driver can find sys_ck on
DM816x systems.

> diff --git a/drivers/clk/ti/clk-816x.c b/drivers/clk/ti/clk-816x.c
> index 3b8e483aec928..fe6f66fc5a05d 100644
> --- a/drivers/clk/ti/clk-816x.c
> +++ b/drivers/clk/ti/clk-816x.c
> @@ -57,6 +57,7 @@ static struct ti_dt_clk dm816x_clks[] = {
>  	DT_CLK(NULL, "timer_sys_ck", "sys_clkin_ck"),
>  	DT_CLK(NULL, "timer_32k_ck", "sysclk18_ck"),
>  	DT_CLK(NULL, "timer_ext_ck", "tclkin_ck"),
> +	DT_CLK(NULL, "sys_ck", "sys_clkin_ck"),

[Severity: Medium]
Does adding the sys_ck alias in dm816x_clks[] cause omap_voltage_late_init()
to run OMAP34xx/36xx voltage controller and voltage processor initialization
on DM816x where that hardware and PMIC data are absent?

On DM816x boot, ti816x_init_early() (as well as ti814x_init_early() on TI814x)
calls omap3xxx_voltagedomains_init() in
arch/arm/mach-omap2/voltagedomains3xxx_data.c:

	if (soc_is_am35xx())
		voltdms = voltagedomains_am35xx;
	else
		voltdms = voltagedomains_omap3;

Because only soc_is_am35xx() is checked, TI81xx falls back to
voltagedomains_omap3, which registers omap3_voltdm_mpu and omap3_voltdm_core
with .scalable = true, .vc, and .vp populated.

Later, omap2_common_pm_late_init() calls omap3_twl_init(), which returns
-ENODEV because cpu_is_omap34xx() is false on DM816x, leaving voltdm->pmic as
NULL. It then calls omap_voltage_late_init() in
arch/arm/mach-omap2/voltage.c:

	list_for_each_entry(voltdm, &voltdm_list, node) {
		struct clk *sys_ck;

		if (!voltdm->scalable)
			continue;

		sys_ck = clk_get(NULL, voltdm->sys_clk.name);
		if (IS_ERR(sys_ck)) {
			pr_warn("%s: Could not get sys clk.\n", __func__);
			return -EINVAL;
		}
		voltdm->sys_clk.rate = clk_get_rate(sys_ck);
		WARN_ON(!voltdm->sys_clk.rate);
		clk_put(sys_ck);

		if (voltdm->vc) {
			voltdm->scale = omap_vc_bypass_scale;
			omap_vc_init_channel(voltdm);
		}

		if (voltdm->vp) {
			voltdm->scale = omap_vp_forceupdate_scale;
			omap_vp_init(voltdm);
		}
	}

When clk_get(NULL, "sys_ck") succeeds, omap_voltage_late_init() proceeds to
call omap_vc_init_channel() in arch/arm/mach-omap2/vc.c and omap_vp_init() in
arch/arm/mach-omap2/vp.c for both mpu_iva and core:

	if (!voltdm->pmic || !voltdm->pmic->uv_to_vsel) {
		pr_err("%s: No PMIC info for vdd_%s\n", __func__, voltdm->name);
		return;
	}

This trades the single pr_warn() in omap_voltage_late_init() for four pr_err()
messages on boot and leaves voltdm->scale set to omap_vp_forceupdate_scale on
uninitialized VP instances.

Should TI81xx instead use non-scalable voltage domains in
omap3xxx_voltagedomains_init(), similar to how commit ddf536d0d7e1 ("ARM:
OMAP: AM3517/3505: fix crash on boot due to incorrect voltagedomain data")
handled AM35xx with voltagedomains_am35xx?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-patch-ektor5-dm8168-clk-v2-0-dc6bf0fabae4@linaro.org?part=4