[PATCH 0/2] clk: Fix issues related to CLK_IGNORE_UNUSED failures and amlogic glitch free mux

[PATCH 0/2] clk: Fix issues related to CLK_IGNORE_UNUSED failures and amlogic glitch free mux

[Chuan Liu via B4 Relay]

If CLK_OPS_PARENT_ENABLE is configured for clock,
clk_core_disable_unprepare() is called in clk_disable_unused_subtree().
Even clocks that are configured with CLK_IGNORE_UNUSED are disabled,
resulting in the failure of CLK_IGNORE_UNUSED.

To ensure that amlogic glitch free mux can switch clock channels
properly, add flag CLK_OPS_PARENT_ENABLE to glitch free mux. The issue
that CLK_OPS_PARENT_ENABLE in CCF causes CLK_IGNORE_UNUSED to fail is
also exposed.

glitch free mux channel switchover failure issue(Test vpu_clk on S4):
step 1:
$ cat /sys/kernel/debug/clk/vpu/clk_parent 
vpu_0
$ cat /sys/kernel/debug/clk/vpu_0/clk_rate 
399999994
$ cat /sys/kernel/debug/clk/vpu_1/clk_rate 
666666656
$ echo 1 > /sys/kernel/debug/clk/vpu/clk_prepare_enable 
$ cat /sys/kernel/debug/meson-clk-msr/clks/cts_vpu_clk
399987500       +/-12500Hz

step 2:
$ echo 0 > /sys/kernel/debug/clk/vpu/clk_prepare_enable 
$ echo 1 > /sys/kernel/debug/clk/vpu/clk_parent 
$ cat /sys/kernel/debug/clk/vpu/clk_parent 
vpu_1
$ cat /sys/kernel/debug/clk/vpu/clk_rate 
666666656
$ echo 1 > /sys/kernel/debug/clk/vpu/clk_prepare_enable 
$ cat /sys/kernel/debug/meson-clk-msr/clks/cts_vpu_clk
0       +/-3125Hz

In step2, vpu_0 is disabled, and the vpu is not switched to vpu_1. At
this time, the vpu is still connected to vpu_0 and vpu_0 is disabled at
this time, resulting in the clk-measure not measuring the clock.

Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
Chuan Liu (2):
      clk: Fix the CLK_IGNORE_UNUSED failure issue
      clk: meson: Fix glitch free mux related issues

 drivers/clk/clk.c                  | 67 ++++++++++++++++++++++++++++++++++++--
 drivers/clk/meson/a1-peripherals.c | 12 +++----
 drivers/clk/meson/axg.c            | 16 +++++----
 drivers/clk/meson/c3-peripherals.c |  6 ++--
 drivers/clk/meson/g12a.c           | 18 ++++++----
 drivers/clk/meson/gxbb.c           | 18 ++++++----
 drivers/clk/meson/s4-peripherals.c | 32 +++++++++---------
 7 files changed, 122 insertions(+), 47 deletions(-)
---
base-commit: e736da1956cf0880a02ec5023f3487eea7611b5f
change-id: 20240929-fix_glitch_free-290c88923c31

Best regards,
-- 
Chuan Liu <chuan.liu@amlogic.com>

[PATCH 1/2] clk: Fix the CLK_IGNORE_UNUSED failure issue

[Chuan Liu via B4 Relay]

From: Chuan Liu <chuan.liu@amlogic.com>

When the clk_disable_unused_subtree() function disables an unused clock,
if CLK_OPS_PARENT_ENABLE is configured on the clock,
clk_core_prepare_enable() and clk_core_disable_unprepare() are called
directly, and these two functions do not determine CLK_IGNORE_UNUSED,
This causes the clock to be disabled even if CLK_IGNORE_UNUSED is
configured when clk_core_disable_unprepare() is called.

Two new functions clk_disable_unprepare_unused() and
clk_prepare_enable_unused() are added to resolve the preceding
situation. The CLK_IGNORE_UNUSED judgment logic is added to these two
functions. To prevent clock configuration CLK_IGNORE_UNUSED from
possible failure.

Change-Id: I56943e17b86436254f07d9b8cdbc35599328d519
Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
 drivers/clk/clk.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 285ed1ad8a37..5d3316699b57 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -94,6 +94,7 @@ struct clk_core {
 	struct hlist_node	debug_node;
 #endif
 	struct kref		ref;
+	bool			ignore_enabled;
 };
 
 #define CREATE_TRACE_POINTS
@@ -1479,6 +1480,68 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
 	}
 }
 
+static void __init clk_disable_unprepare_unused(struct clk_core *core)
+{
+	unsigned long flags;
+
+	lockdep_assert_held(&prepare_lock);
+
+	if (!core)
+		return;
+
+	if ((core->enable_count == 0) && core->ops->disable &&
+	    !core->ignore_enabled) {
+		flags = clk_enable_lock();
+		core->ops->disable(core->hw);
+		clk_enable_unlock(flags);
+	}
+
+	if ((core->prepare_count == 0) && core->ops->unprepare &&
+	    !core->ignore_enabled)
+		core->ops->unprepare(core->hw);
+
+	core->ignore_enabled = false;
+
+	clk_disable_unprepare_unused(core->parent);
+}
+
+static int __init clk_prepare_enable_unused(struct clk_core *core)
+{
+	int ret = 0;
+	unsigned long flags;
+
+	lockdep_assert_held(&prepare_lock);
+
+	if (!core)
+		return 0;
+
+	ret = clk_prepare_enable_unused(core->parent);
+	if (ret)
+		return ret;
+
+	if ((core->flags & CLK_IGNORE_UNUSED) && clk_core_is_enabled(core))
+		core->ignore_enabled = true;
+
+	if ((core->prepare_count == 0) && core->ops->prepare) {
+		ret = core->ops->prepare(core->hw);
+		if (ret)
+			goto disable_unprepare;
+	}
+
+	if ((core->enable_count == 0) && core->ops->enable) {
+		flags = clk_enable_lock();
+		ret = core->ops->enable(core->hw);
+		clk_enable_unlock(flags);
+		if (ret)
+			goto disable_unprepare;
+	}
+
+	return 0;
+disable_unprepare:
+	clk_disable_unprepare_unused(core->parent);
+	return ret;
+}
+
 static void __init clk_disable_unused_subtree(struct clk_core *core)
 {
 	struct clk_core *child;
@@ -1490,7 +1553,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
 		clk_disable_unused_subtree(child);
 
 	if (core->flags & CLK_OPS_PARENT_ENABLE)
-		clk_core_prepare_enable(core->parent);
+		clk_prepare_enable_unused(core->parent);
 
 	flags = clk_enable_lock();
 
@@ -1517,7 +1580,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
 unlock_out:
 	clk_enable_unlock(flags);
 	if (core->flags & CLK_OPS_PARENT_ENABLE)
-		clk_core_disable_unprepare(core->parent);
+		clk_disable_unprepare_unused(core->parent);
 }
 
 static bool clk_ignore_unused __initdata;

-- 
2.42.0

[PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Chuan Liu via B4 Relay]

From: Chuan Liu <chuan.liu@amlogic.com>

glitch free mux has two clock channels (channel 0 and channel 1) with
the same configuration. When the frequency needs to be changed, the two
channels ping-pong to ensure clock continuity and suppress glitch.

Channel 0 of glitch free mux is not only the clock source for the mux,
but also the working clock for glitch free mux. Therefore, when glitch
free mux switches, it is necessary to ensure that channel 0 has a clock
input, otherwise glitch free mux will not work and cannot switch to the
target channel.

Add flag CLK_SET_RATE_GATE to channels 0 and 1 to implement Ping-Pong
switchover to suppress glitch.

glitch free mux Add flag CLK_OPS_PARENT_ENABLE to ensure that channel 0
has clock input when switching channels.

Change-Id: I6fa6ff92f7b2e0a13dd7a27feb0e8568be3ca9f9
Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
 drivers/clk/meson/a1-peripherals.c | 12 ++++++------
 drivers/clk/meson/axg.c            | 16 ++++++++++------
 drivers/clk/meson/c3-peripherals.c |  6 +++---
 drivers/clk/meson/g12a.c           | 18 +++++++++++-------
 drivers/clk/meson/gxbb.c           | 18 +++++++++++-------
 drivers/clk/meson/s4-peripherals.c | 32 ++++++++++++++++----------------
 6 files changed, 57 insertions(+), 45 deletions(-)

diff --git a/drivers/clk/meson/a1-peripherals.c b/drivers/clk/meson/a1-peripherals.c
index 7aa6abb2eb1f..7f515e002adb 100644
--- a/drivers/clk/meson/a1-peripherals.c
+++ b/drivers/clk/meson/a1-peripherals.c
@@ -423,7 +423,7 @@ static struct clk_regmap dspa_a = {
 			&dspa_a_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -471,7 +471,7 @@ static struct clk_regmap dspa_b = {
 			&dspa_b_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -489,7 +489,7 @@ static struct clk_regmap dspa_sel = {
 			&dspa_b.hw,
 		},
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
@@ -569,7 +569,7 @@ static struct clk_regmap dspb_a = {
 			&dspb_a_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -617,7 +617,7 @@ static struct clk_regmap dspb_b = {
 			&dspb_b_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -635,7 +635,7 @@ static struct clk_regmap dspb_sel = {
 			&dspb_b.hw,
 		},
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
index 1b08daf579b2..e2d3266f4b45 100644
--- a/drivers/clk/meson/axg.c
+++ b/drivers/clk/meson/axg.c
@@ -1077,7 +1077,8 @@ static struct clk_regmap axg_vpu_0 = {
 		 * We want to avoid CCF to disable the VPU clock if
 		 * display has been set by Bootloader
 		 */
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1126,7 +1127,8 @@ static struct clk_regmap axg_vpu_1 = {
 		 * We want to avoid CCF to disable the VPU clock if
 		 * display has been set by Bootloader
 		 */
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1144,7 +1146,7 @@ static struct clk_regmap axg_vpu = {
 			&axg_vpu_1.hw
 		},
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_NO_REPARENT,
+		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
@@ -1194,7 +1196,8 @@ static struct clk_regmap axg_vapb_0 = {
 			&axg_vapb_0_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1242,7 +1245,8 @@ static struct clk_regmap axg_vapb_1 = {
 			&axg_vapb_1_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1260,7 +1264,7 @@ static struct clk_regmap axg_vapb_sel = {
 			&axg_vapb_1.hw
 		},
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_NO_REPARENT,
+		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
diff --git a/drivers/clk/meson/c3-peripherals.c b/drivers/clk/meson/c3-peripherals.c
index 7dcbf4ebee07..27343a73a521 100644
--- a/drivers/clk/meson/c3-peripherals.c
+++ b/drivers/clk/meson/c3-peripherals.c
@@ -1364,7 +1364,7 @@ static struct clk_regmap hcodec_0 = {
 			&hcodec_0_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1411,7 +1411,7 @@ static struct clk_regmap hcodec_1 = {
 			&hcodec_1_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1431,7 +1431,7 @@ static struct clk_regmap hcodec = {
 		.ops = &clk_regmap_mux_ops,
 		.parent_data = hcodec_parent_data,
 		.num_parents = ARRAY_SIZE(hcodec_parent_data),
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
index d3539fe9f7af..21a25001e904 100644
--- a/drivers/clk/meson/g12a.c
+++ b/drivers/clk/meson/g12a.c
@@ -2746,7 +2746,8 @@ static struct clk_regmap g12a_vpu_0 = {
 		.ops = &clk_regmap_gate_ops,
 		.parent_hws = (const struct clk_hw *[]) { &g12a_vpu_0_div.hw },
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -2790,7 +2791,8 @@ static struct clk_regmap g12a_vpu_1 = {
 		.ops = &clk_regmap_gate_ops,
 		.parent_hws = (const struct clk_hw *[]) { &g12a_vpu_1_div.hw },
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -2812,7 +2814,7 @@ static struct clk_regmap g12a_vpu = {
 			&g12a_vpu_1.hw,
 		},
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_NO_REPARENT,
+		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
@@ -3035,7 +3037,8 @@ static struct clk_regmap g12a_vapb_0 = {
 			&g12a_vapb_0_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -3083,7 +3086,8 @@ static struct clk_regmap g12a_vapb_1 = {
 			&g12a_vapb_1_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -3105,7 +3109,7 @@ static struct clk_regmap g12a_vapb_sel = {
 			&g12a_vapb_1.hw,
 		},
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_NO_REPARENT,
+		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
@@ -4039,7 +4043,7 @@ static struct clk_regmap g12a_mali = {
 		.ops = &clk_regmap_mux_ops,
 		.parent_hws = g12a_mali_parent_hws,
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
index 262c318edbd5..812b3e20c366 100644
--- a/drivers/clk/meson/gxbb.c
+++ b/drivers/clk/meson/gxbb.c
@@ -1132,7 +1132,7 @@ static struct clk_regmap gxbb_mali = {
 		.ops = &clk_regmap_mux_ops,
 		.parent_hws = gxbb_mali_parent_hws,
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
@@ -1543,7 +1543,8 @@ static struct clk_regmap gxbb_vpu_0 = {
 		.ops = &clk_regmap_gate_ops,
 		.parent_hws = (const struct clk_hw *[]) { &gxbb_vpu_0_div.hw },
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1591,7 +1592,8 @@ static struct clk_regmap gxbb_vpu_1 = {
 		.ops = &clk_regmap_gate_ops,
 		.parent_hws = (const struct clk_hw *[]) { &gxbb_vpu_1_div.hw },
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1613,7 +1615,7 @@ static struct clk_regmap gxbb_vpu = {
 			&gxbb_vpu_1.hw
 		},
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_NO_REPARENT,
+		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
@@ -1674,7 +1676,8 @@ static struct clk_regmap gxbb_vapb_0 = {
 			&gxbb_vapb_0_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1726,7 +1729,8 @@ static struct clk_regmap gxbb_vapb_1 = {
 			&gxbb_vapb_1_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
+			 CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1748,7 +1752,7 @@ static struct clk_regmap gxbb_vapb_sel = {
 			&gxbb_vapb_1.hw
 		},
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_NO_REPARENT,
+		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
diff --git a/drivers/clk/meson/s4-peripherals.c b/drivers/clk/meson/s4-peripherals.c
index c930cf0614a0..cf10be40141d 100644
--- a/drivers/clk/meson/s4-peripherals.c
+++ b/drivers/clk/meson/s4-peripherals.c
@@ -1404,7 +1404,7 @@ static struct clk_regmap s4_mali_mux = {
 		.ops = &clk_regmap_mux_ops,
 		.parent_hws = s4_mali_parent_hws,
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
@@ -1466,7 +1466,7 @@ static struct clk_regmap s4_vdec_p0 = {
 			&s4_vdec_p0_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1516,7 +1516,7 @@ static struct clk_regmap s4_vdec_p1 = {
 			&s4_vdec_p1_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1536,7 +1536,7 @@ static struct clk_regmap s4_vdec_mux = {
 		.ops = &clk_regmap_mux_ops,
 		.parent_hws = s4_vdec_mux_parent_hws,
 		.num_parents = ARRAY_SIZE(s4_vdec_mux_parent_hws),
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
@@ -1586,7 +1586,7 @@ static struct clk_regmap s4_hevcf_p0 = {
 			&s4_hevcf_p0_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1636,7 +1636,7 @@ static struct clk_regmap s4_hevcf_p1 = {
 			&s4_hevcf_p1_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1656,7 +1656,7 @@ static struct clk_regmap s4_hevcf_mux = {
 		.ops = &clk_regmap_mux_ops,
 		.parent_hws = s4_hevcf_mux_parent_hws,
 		.num_parents = ARRAY_SIZE(s4_hevcf_mux_parent_hws),
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
@@ -1712,7 +1712,7 @@ static struct clk_regmap s4_vpu_0 = {
 		.ops = &clk_regmap_gate_ops,
 		.parent_hws = (const struct clk_hw *[]) { &s4_vpu_0_div.hw },
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1756,7 +1756,7 @@ static struct clk_regmap s4_vpu_1 = {
 		.ops = &clk_regmap_gate_ops,
 		.parent_hws = (const struct clk_hw *[]) { &s4_vpu_1_div.hw },
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1774,7 +1774,7 @@ static struct clk_regmap s4_vpu = {
 			&s4_vpu_1.hw,
 		},
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
@@ -1921,7 +1921,7 @@ static struct clk_regmap s4_vpu_clkc_p0 = {
 			&s4_vpu_clkc_p0_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1969,7 +1969,7 @@ static struct clk_regmap s4_vpu_clkc_p1 = {
 			&s4_vpu_clkc_p1_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -1989,7 +1989,7 @@ static struct clk_regmap s4_vpu_clkc_mux = {
 		.ops = &clk_regmap_mux_ops,
 		.parent_hws = s4_vpu_mux_parent_hws,
 		.num_parents = ARRAY_SIZE(s4_vpu_mux_parent_hws),
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 
@@ -2049,7 +2049,7 @@ static struct clk_regmap s4_vapb_0 = {
 			&s4_vapb_0_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -2097,7 +2097,7 @@ static struct clk_regmap s4_vapb_1 = {
 			&s4_vapb_1_div.hw
 		},
 		.num_parents = 1,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
 	},
 };
 
@@ -2115,7 +2115,7 @@ static struct clk_regmap s4_vapb = {
 			&s4_vapb_1.hw
 		},
 		.num_parents = 2,
-		.flags = CLK_SET_RATE_PARENT,
+		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
 	},
 };
 

-- 
2.42.0

Re: [PATCH 1/2] clk: Fix the CLK_IGNORE_UNUSED failure issue

[Jerome Brunet]

On Sun 29 Sep 2024 at 14:10, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:

> From: Chuan Liu <chuan.liu@amlogic.com>
>
> When the clk_disable_unused_subtree() function disables an unused clock,
> if CLK_OPS_PARENT_ENABLE is configured on the clock,
> clk_core_prepare_enable() and clk_core_disable_unprepare() are called
> directly, and these two functions do not determine CLK_IGNORE_UNUSED,
> This causes the clock to be disabled even if CLK_IGNORE_UNUSED is
> configured when clk_core_disable_unprepare() is called.
>
> Two new functions clk_disable_unprepare_unused() and
> clk_prepare_enable_unused() are added to resolve the preceding
> situation. The CLK_IGNORE_UNUSED judgment logic is added to these two
> functions. To prevent clock configuration CLK_IGNORE_UNUSED from
> possible failure.
>
> Change-Id: I56943e17b86436254f07d9b8cdbc35599328d519
> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
>  drivers/clk/clk.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 285ed1ad8a37..5d3316699b57 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -94,6 +94,7 @@ struct clk_core {
>  	struct hlist_node	debug_node;
>  #endif
>  	struct kref		ref;
> +	bool			ignore_enabled;
>  };
>  
>  #define CREATE_TRACE_POINTS
> @@ -1479,6 +1480,68 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
>  	}
>  }
>  
> +static void __init clk_disable_unprepare_unused(struct clk_core *core)
> +{
> +	unsigned long flags;
> +
> +	lockdep_assert_held(&prepare_lock);
> +
> +	if (!core)
> +		return;
> +
> +	if ((core->enable_count == 0) && core->ops->disable &&
> +	    !core->ignore_enabled) {
> +		flags = clk_enable_lock();

Used core->enable_count without taking the lock

> +		core->ops->disable(core->hw);

If the there is any CLK_IS_CRITICAL in the path, it is game over.
You've basically disregarded all the other CCF flags which are equally
important to the ones you are dealing with.

> +		clk_enable_unlock(flags);
> +	}
> +
> +	if ((core->prepare_count == 0) && core->ops->unprepare &&
> +	    !core->ignore_enabled)
> +		core->ops->unprepare(core->hw);
> +
> +	core->ignore_enabled = false;
> +
> +	clk_disable_unprepare_unused(core->parent);

Here you are disabling the parent of any CLK_IGNORE_UNUSED clock.
IMO, the problem is not solved. It just shifted.

> +}
> +
> +static int __init clk_prepare_enable_unused(struct clk_core *core)
> +{
> +	int ret = 0;
> +	unsigned long flags;
> +
> +	lockdep_assert_held(&prepare_lock);
> +
> +	if (!core)
> +		return 0;
> +
> +	ret = clk_prepare_enable_unused(core->parent);
> +	if (ret)
> +		return ret;

That's adding another recursion in CCF, something Stephen would like to remove

> +
> +	if ((core->flags & CLK_IGNORE_UNUSED) && clk_core_is_enabled(core))
> +		core->ignore_enabled = true;
> +
> +	if ((core->prepare_count == 0) && core->ops->prepare) {
> +		ret = core->ops->prepare(core->hw);
> +		if (ret)
> +			goto disable_unprepare;
> +	}
> +
> +	if ((core->enable_count == 0) && core->ops->enable) {
> +		flags = clk_enable_lock();
> +		ret = core->ops->enable(core->hw);
> +		clk_enable_unlock(flags);
> +		if (ret)
> +			goto disable_unprepare;
> +	}
> +
> +	return 0;
> +disable_unprepare:
> +	clk_disable_unprepare_unused(core->parent);
> +	return ret;
> +}
> +
>  static void __init clk_disable_unused_subtree(struct clk_core *core)
>  {
>  	struct clk_core *child;
> @@ -1490,7 +1553,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>  		clk_disable_unused_subtree(child);
>  
>  	if (core->flags & CLK_OPS_PARENT_ENABLE)
> -		clk_core_prepare_enable(core->parent);
> +		clk_prepare_enable_unused(core->parent);
>  
>  	flags = clk_enable_lock();
>  
> @@ -1517,7 +1580,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>  unlock_out:
>  	clk_enable_unlock(flags);
>  	if (core->flags & CLK_OPS_PARENT_ENABLE)
> -		clk_core_disable_unprepare(core->parent);
> +		clk_disable_unprepare_unused(core->parent);
>  }
>  
>  static bool clk_ignore_unused __initdata;

-- 
Jerome

Re: [PATCH 0/2] clk: Fix issues related to CLK_IGNORE_UNUSED failures and amlogic glitch free mux

[Jerome Brunet]

On Sun 29 Sep 2024 at 14:10, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:

> If CLK_OPS_PARENT_ENABLE is configured for clock,
> clk_core_disable_unprepare() is called in clk_disable_unused_subtree().
> Even clocks that are configured with CLK_IGNORE_UNUSED are disabled,
> resulting in the failure of CLK_IGNORE_UNUSED.
>
> To ensure that amlogic glitch free mux can switch clock channels
> properly, add flag CLK_OPS_PARENT_ENABLE to glitch free mux. The issue
> that CLK_OPS_PARENT_ENABLE in CCF causes CLK_IGNORE_UNUSED to fail is
> also exposed.

The problem is that you are mixing problems together and it makes things
rather difficult to follow. There are 2 distinct problem, there should
have been to distinct patchset, even if you reference the CCF one in the
Amlogic change.

CLK_IGNORE_UNUSED is no guarantee that a clock will stay on, no matter
what happens in the clock tree. I explained that to you several times,
and it is the very reason why you are being asked to justify each usage
of the flag. Most of the time, using it is wrong.

That being said, there seems to be problems with CLK_OPS_PARENT_ENABLE
in clk_disable_unused_subtree(). As it is, I think
* a clock with CLK_IGNORE_UNUSED and CLK_OPS_PARENT_ENABLE, would 'leak'
  an enable, essentially making the parent subtree critical.
* All parents of a CLK_OPS_PARENT_ENABLE clock would have its
  CLK_IGNORE_UNUSED ignored as a result of the enable/disable sequence
  (note that in any other circumstance, enable/disable should indeed
  disable an CLK_IGNORE_UNUSED clock).
* Parent of ignored clocks may still get disabled.

I'll be sending a proposal to address these problems soon.

>
> glitch free mux channel switchover failure issue(Test vpu_clk on S4):
> step 1:
> $ cat /sys/kernel/debug/clk/vpu/clk_parent 
> vpu_0
> $ cat /sys/kernel/debug/clk/vpu_0/clk_rate 
> 399999994
> $ cat /sys/kernel/debug/clk/vpu_1/clk_rate 
> 666666656
> $ echo 1 > /sys/kernel/debug/clk/vpu/clk_prepare_enable 
> $ cat /sys/kernel/debug/meson-clk-msr/clks/cts_vpu_clk
> 399987500       +/-12500Hz
>
> step 2:
> $ echo 0 > /sys/kernel/debug/clk/vpu/clk_prepare_enable 
> $ echo 1 > /sys/kernel/debug/clk/vpu/clk_parent 
> $ cat /sys/kernel/debug/clk/vpu/clk_parent 
> vpu_1
> $ cat /sys/kernel/debug/clk/vpu/clk_rate 
> 666666656
> $ echo 1 > /sys/kernel/debug/clk/vpu/clk_prepare_enable 
> $ cat /sys/kernel/debug/meson-clk-msr/clks/cts_vpu_clk
> 0       +/-3125Hz
>
> In step2, vpu_0 is disabled, and the vpu is not switched to vpu_1. At
> this time, the vpu is still connected to vpu_0 and vpu_0 is disabled at
> this time, resulting in the clk-measure not measuring the clock.
>

If keep the display on and managed at all time is critical, then may you
should consider having the module managing it as built-in.

You would not need CLK_IGNORE_USED at all because device would be
present before clk_disable_unused() is executed.

Past the late init, if no device actively use the clock, disabling it is
the sane thing to do because nothing says it will ever be used.

> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
> Chuan Liu (2):
>       clk: Fix the CLK_IGNORE_UNUSED failure issue
>       clk: meson: Fix glitch free mux related issues
>
>  drivers/clk/clk.c                  | 67 ++++++++++++++++++++++++++++++++++++--
>  drivers/clk/meson/a1-peripherals.c | 12 +++----
>  drivers/clk/meson/axg.c            | 16 +++++----
>  drivers/clk/meson/c3-peripherals.c |  6 ++--
>  drivers/clk/meson/g12a.c           | 18 ++++++----
>  drivers/clk/meson/gxbb.c           | 18 ++++++----
>  drivers/clk/meson/s4-peripherals.c | 32 +++++++++---------
>  7 files changed, 122 insertions(+), 47 deletions(-)
> ---
> base-commit: e736da1956cf0880a02ec5023f3487eea7611b5f
> change-id: 20240929-fix_glitch_free-290c88923c31
>
> Best regards,

-- 
Jerome

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Jerome Brunet]

On Sun 29 Sep 2024 at 14:10, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:

> From: Chuan Liu <chuan.liu@amlogic.com>
>
> glitch free mux has two clock channels (channel 0 and channel 1) with
> the same configuration. When the frequency needs to be changed, the two
> channels ping-pong to ensure clock continuity and suppress glitch.
>
> Channel 0 of glitch free mux is not only the clock source for the mux,
> but also the working clock for glitch free mux. Therefore, when glitch
> free mux switches, it is necessary to ensure that channel 0 has a clock
> input, otherwise glitch free mux will not work and cannot switch to the
> target channel.
>
> Add flag CLK_SET_RATE_GATE to channels 0 and 1 to implement Ping-Pong
> switchover to suppress glitch.
>
> glitch free mux Add flag CLK_OPS_PARENT_ENABLE to ensure that channel 0
> has clock input when switching channels.

Several 'glitch_free' are not touched by your change. Why ?
I thinking about the mali glitch free mux for example.

>
> Change-Id: I6fa6ff92f7b2e0a13dd7a27feb0e8568be3ca9f9
> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
>  drivers/clk/meson/a1-peripherals.c | 12 ++++++------
>  drivers/clk/meson/axg.c            | 16 ++++++++++------
>  drivers/clk/meson/c3-peripherals.c |  6 +++---
>  drivers/clk/meson/g12a.c           | 18 +++++++++++-------
>  drivers/clk/meson/gxbb.c           | 18 +++++++++++-------
>  drivers/clk/meson/s4-peripherals.c | 32 ++++++++++++++++----------------
>  6 files changed, 57 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/clk/meson/a1-peripherals.c b/drivers/clk/meson/a1-peripherals.c
> index 7aa6abb2eb1f..7f515e002adb 100644
> --- a/drivers/clk/meson/a1-peripherals.c
> +++ b/drivers/clk/meson/a1-peripherals.c
> @@ -423,7 +423,7 @@ static struct clk_regmap dspa_a = {
>  			&dspa_a_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -471,7 +471,7 @@ static struct clk_regmap dspa_b = {
>  			&dspa_b_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -489,7 +489,7 @@ static struct clk_regmap dspa_sel = {
>  			&dspa_b.hw,
>  		},
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> @@ -569,7 +569,7 @@ static struct clk_regmap dspb_a = {
>  			&dspb_a_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -617,7 +617,7 @@ static struct clk_regmap dspb_b = {
>  			&dspb_b_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -635,7 +635,7 @@ static struct clk_regmap dspb_sel = {
>  			&dspb_b.hw,
>  		},
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
> index 1b08daf579b2..e2d3266f4b45 100644
> --- a/drivers/clk/meson/axg.c
> +++ b/drivers/clk/meson/axg.c
> @@ -1077,7 +1077,8 @@ static struct clk_regmap axg_vpu_0 = {
>  		 * We want to avoid CCF to disable the VPU clock if
>  		 * display has been set by Bootloader
>  		 */
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1126,7 +1127,8 @@ static struct clk_regmap axg_vpu_1 = {
>  		 * We want to avoid CCF to disable the VPU clock if
>  		 * display has been set by Bootloader
>  		 */
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1144,7 +1146,7 @@ static struct clk_regmap axg_vpu = {
>  			&axg_vpu_1.hw
>  		},
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_NO_REPARENT,
> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> @@ -1194,7 +1196,8 @@ static struct clk_regmap axg_vapb_0 = {
>  			&axg_vapb_0_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1242,7 +1245,8 @@ static struct clk_regmap axg_vapb_1 = {
>  			&axg_vapb_1_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1260,7 +1264,7 @@ static struct clk_regmap axg_vapb_sel = {
>  			&axg_vapb_1.hw
>  		},
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_NO_REPARENT,
> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> diff --git a/drivers/clk/meson/c3-peripherals.c b/drivers/clk/meson/c3-peripherals.c
> index 7dcbf4ebee07..27343a73a521 100644
> --- a/drivers/clk/meson/c3-peripherals.c
> +++ b/drivers/clk/meson/c3-peripherals.c
> @@ -1364,7 +1364,7 @@ static struct clk_regmap hcodec_0 = {
>  			&hcodec_0_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1411,7 +1411,7 @@ static struct clk_regmap hcodec_1 = {
>  			&hcodec_1_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1431,7 +1431,7 @@ static struct clk_regmap hcodec = {
>  		.ops = &clk_regmap_mux_ops,
>  		.parent_data = hcodec_parent_data,
>  		.num_parents = ARRAY_SIZE(hcodec_parent_data),
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index d3539fe9f7af..21a25001e904 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -2746,7 +2746,8 @@ static struct clk_regmap g12a_vpu_0 = {
>  		.ops = &clk_regmap_gate_ops,
>  		.parent_hws = (const struct clk_hw *[]) { &g12a_vpu_0_div.hw },
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -2790,7 +2791,8 @@ static struct clk_regmap g12a_vpu_1 = {
>  		.ops = &clk_regmap_gate_ops,
>  		.parent_hws = (const struct clk_hw *[]) { &g12a_vpu_1_div.hw },
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -2812,7 +2814,7 @@ static struct clk_regmap g12a_vpu = {
>  			&g12a_vpu_1.hw,
>  		},
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_NO_REPARENT,
> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> @@ -3035,7 +3037,8 @@ static struct clk_regmap g12a_vapb_0 = {
>  			&g12a_vapb_0_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -3083,7 +3086,8 @@ static struct clk_regmap g12a_vapb_1 = {
>  			&g12a_vapb_1_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -3105,7 +3109,7 @@ static struct clk_regmap g12a_vapb_sel = {
>  			&g12a_vapb_1.hw,
>  		},
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_NO_REPARENT,
> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> @@ -4039,7 +4043,7 @@ static struct clk_regmap g12a_mali = {
>  		.ops = &clk_regmap_mux_ops,
>  		.parent_hws = g12a_mali_parent_hws,
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
> index 262c318edbd5..812b3e20c366 100644
> --- a/drivers/clk/meson/gxbb.c
> +++ b/drivers/clk/meson/gxbb.c
> @@ -1132,7 +1132,7 @@ static struct clk_regmap gxbb_mali = {
>  		.ops = &clk_regmap_mux_ops,
>  		.parent_hws = gxbb_mali_parent_hws,
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> @@ -1543,7 +1543,8 @@ static struct clk_regmap gxbb_vpu_0 = {
>  		.ops = &clk_regmap_gate_ops,
>  		.parent_hws = (const struct clk_hw *[]) { &gxbb_vpu_0_div.hw },
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1591,7 +1592,8 @@ static struct clk_regmap gxbb_vpu_1 = {
>  		.ops = &clk_regmap_gate_ops,
>  		.parent_hws = (const struct clk_hw *[]) { &gxbb_vpu_1_div.hw },
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1613,7 +1615,7 @@ static struct clk_regmap gxbb_vpu = {
>  			&gxbb_vpu_1.hw
>  		},
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_NO_REPARENT,
> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> @@ -1674,7 +1676,8 @@ static struct clk_regmap gxbb_vapb_0 = {
>  			&gxbb_vapb_0_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1726,7 +1729,8 @@ static struct clk_regmap gxbb_vapb_1 = {
>  			&gxbb_vapb_1_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED |
> +			 CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1748,7 +1752,7 @@ static struct clk_regmap gxbb_vapb_sel = {
>  			&gxbb_vapb_1.hw
>  		},
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_NO_REPARENT,
> +		.flags = CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> diff --git a/drivers/clk/meson/s4-peripherals.c b/drivers/clk/meson/s4-peripherals.c
> index c930cf0614a0..cf10be40141d 100644
> --- a/drivers/clk/meson/s4-peripherals.c
> +++ b/drivers/clk/meson/s4-peripherals.c
> @@ -1404,7 +1404,7 @@ static struct clk_regmap s4_mali_mux = {
>  		.ops = &clk_regmap_mux_ops,
>  		.parent_hws = s4_mali_parent_hws,
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> @@ -1466,7 +1466,7 @@ static struct clk_regmap s4_vdec_p0 = {
>  			&s4_vdec_p0_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1516,7 +1516,7 @@ static struct clk_regmap s4_vdec_p1 = {
>  			&s4_vdec_p1_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1536,7 +1536,7 @@ static struct clk_regmap s4_vdec_mux = {
>  		.ops = &clk_regmap_mux_ops,
>  		.parent_hws = s4_vdec_mux_parent_hws,
>  		.num_parents = ARRAY_SIZE(s4_vdec_mux_parent_hws),
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> @@ -1586,7 +1586,7 @@ static struct clk_regmap s4_hevcf_p0 = {
>  			&s4_hevcf_p0_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1636,7 +1636,7 @@ static struct clk_regmap s4_hevcf_p1 = {
>  			&s4_hevcf_p1_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1656,7 +1656,7 @@ static struct clk_regmap s4_hevcf_mux = {
>  		.ops = &clk_regmap_mux_ops,
>  		.parent_hws = s4_hevcf_mux_parent_hws,
>  		.num_parents = ARRAY_SIZE(s4_hevcf_mux_parent_hws),
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> @@ -1712,7 +1712,7 @@ static struct clk_regmap s4_vpu_0 = {
>  		.ops = &clk_regmap_gate_ops,
>  		.parent_hws = (const struct clk_hw *[]) { &s4_vpu_0_div.hw },
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1756,7 +1756,7 @@ static struct clk_regmap s4_vpu_1 = {
>  		.ops = &clk_regmap_gate_ops,
>  		.parent_hws = (const struct clk_hw *[]) { &s4_vpu_1_div.hw },
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1774,7 +1774,7 @@ static struct clk_regmap s4_vpu = {
>  			&s4_vpu_1.hw,
>  		},
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> @@ -1921,7 +1921,7 @@ static struct clk_regmap s4_vpu_clkc_p0 = {
>  			&s4_vpu_clkc_p0_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1969,7 +1969,7 @@ static struct clk_regmap s4_vpu_clkc_p1 = {
>  			&s4_vpu_clkc_p1_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -1989,7 +1989,7 @@ static struct clk_regmap s4_vpu_clkc_mux = {
>  		.ops = &clk_regmap_mux_ops,
>  		.parent_hws = s4_vpu_mux_parent_hws,
>  		.num_parents = ARRAY_SIZE(s4_vpu_mux_parent_hws),
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };
>  
> @@ -2049,7 +2049,7 @@ static struct clk_regmap s4_vapb_0 = {
>  			&s4_vapb_0_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -2097,7 +2097,7 @@ static struct clk_regmap s4_vapb_1 = {
>  			&s4_vapb_1_div.hw
>  		},
>  		.num_parents = 1,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
>  	},
>  };
>  
> @@ -2115,7 +2115,7 @@ static struct clk_regmap s4_vapb = {
>  			&s4_vapb_1.hw
>  		},
>  		.num_parents = 2,
> -		.flags = CLK_SET_RATE_PARENT,
> +		.flags = CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
>  	},
>  };

-- 
Jerome

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Martin Blumenstingl]

Hello,

On Sun, Sep 29, 2024 at 8:10 AM Chuan Liu via B4 Relay
<devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>
> From: Chuan Liu <chuan.liu@amlogic.com>
>
> glitch free mux has two clock channels (channel 0 and channel 1) with
> the same configuration. When the frequency needs to be changed, the two
> channels ping-pong to ensure clock continuity and suppress glitch.
You describe the solution to this below:
> Add flag CLK_SET_RATE_GATE to channels 0 and 1 to implement Ping-Pong
> switchover to suppress glitch.
It would be great to have this change in a separate patch.
The clocks to which you're adding CLK_SET_RATE_GATE aren't switched at
runtime in mainline kernels (at least I think so).

> Channel 0 of glitch free mux is not only the clock source for the mux,
> but also the working clock for glitch free mux. Therefore, when glitch
> free mux switches, it is necessary to ensure that channel 0 has a clock
> input, otherwise glitch free mux will not work and cannot switch to the
> target channel.
[...]
> glitch free mux Add flag CLK_OPS_PARENT_ENABLE to ensure that channel 0
> has clock input when switching channels.
This describes a second problem. I think it's best to have this in a
separate commit/patch.
Also you're updating some mali clocks (e.g. on G12 and GX) but not all
of them (Meson8b for example is missing).

I still have some questions to the CLK_OPS_PARENT_ENABLE approach -
please share your findings on this.

There's multiple clocks involved in a glitch-free mux hierarchy:
- a number of clock inputs (e.g. fclk, xtal, ...)
- two muxes (one for every channel of the glitch-free mux)
- two dividers (one for every channel of the glitch-free mux)
- two gates (one for every channel of the glitch-free mux)
- the glitch-free mux

When switching from channel 0 (which is active and enabled) CCF
(common clock framework) will:
a) on channel 1:
- find the best input clock
- choose the best input clock in the mux
- set the divider
- enable the gate
b) switch the glitch-free mux
c) on channel 2:
- disable the gate

To me it's not clear at which level the problem occurs (glitch-free
mux, gate, divider, mux, input clock).
Also I don't understand why enabling the clocks with
CLK_OPS_PARENT_ENABLE solves any problem since CCF is doing things
automatically for us.
Can you please explain (preferably with an example) what problem is
solved with this approach?

Last but not least: if we're running into bugs when
CLK_OPS_PARENT_ENABLE is missing then that patch should carry a Fixes
tag.


Best regards,
Martin

[RFC PATCH] clk: core: refine disable unused clocks

[Jerome Brunet]

As it as been pointed out numerous times, flagging a clock with
CLK_IGNORE_UNUSED does _not_ guarantee that clock left enabled will stay
on. The clock will get disabled if any enable/disable cycle happens on it
or its parent clocks.

Because enable/disable cycles will disable unused clocks,
clk_disable_unused() should not trigger such cycle. Doing so disregard
the flag if set for any parent clocks. This is problematic with
CLK_OPS_PARENT_ENABLE handling.

To solve this, and a couple other issues, pass the parent status to the
child while walking the subtree, and return whether child ignored disable,
or not.

* Knowing the parent status allows to safely disable clocks with
  CLK_OPS_PARENT_ENABLE when the parent is enabled. Otherwise it means
  that, while the clock is not gated it is effectively disabled. Turning on
  the parents to sanitize the sitation would bring back our initial
  problem, so just let it sanitize itself when the clock gets used.

* If a clock is not actively used (enabled_count == 0), does not have
  CLK_IGNORE_UNUSED but the hw enabled all the way to the root clock, and a
  child ignored the disable, it should ignore the disable too. Doing so
  avoids disabling what is feading the children. Let the flag trickle down
  the tree. This has the added benefit to transfer the information to the
  unprepare path, so we don't unprepare the parent of a clock that ignored
  a disable.

* An enabled clock must be prepared in CCF but we can't rely solely on
  counts at clk_disable_unused() stage. Make sure an enabled clock is
  considered prepared too, even if does not implement the related callback.
  Also make sure only disabled clocks get unprepared.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---

 This is sent as an RFC to continue the discussion started by Chuan.
 It is not meant to be applied as it is.


 drivers/clk/clk.c | 92 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 60 insertions(+), 32 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d02451f951cf..41c4504a41f1 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -332,17 +332,6 @@ static bool clk_core_is_enabled(struct clk_core *core)
 		}
 	}
 
-	/*
-	 * This could be called with the enable lock held, or from atomic
-	 * context. If the parent isn't enabled already, we can't do
-	 * anything here. We can also assume this clock isn't enabled.
-	 */
-	if ((core->flags & CLK_OPS_PARENT_ENABLE) && core->parent)
-		if (!clk_core_is_enabled(core->parent)) {
-			ret = false;
-			goto done;
-		}
-
 	ret = core->ops->is_enabled(core->hw);
 done:
 	if (core->rpm_enabled)
@@ -1454,22 +1443,39 @@ static void clk_core_disable_unprepare(struct clk_core *core)
 	clk_core_unprepare_lock(core);
 }
 
-static void __init clk_unprepare_unused_subtree(struct clk_core *core)
+static bool __init clk_unprepare_unused_subtree(struct clk_core *core,
+						bool parent_prepared)
 {
 	struct clk_core *child;
+	bool prepared;
 
 	lockdep_assert_held(&prepare_lock);
 
+	/*
+	 * Relying on count is not possible at this stage, so consider
+	 * prepared an enabled clock, in case only .is_enabled() is
+	 * implemented
+	 */
+	if (parent_prepared)
+		prepared = (clk_core_is_prepared(core) ||
+			    clk_core_is_enabled(core));
+	else
+		prepared = false;
+
 	hlist_for_each_entry(child, &core->children, child_node)
-		clk_unprepare_unused_subtree(child);
+		if (clk_unprepare_unused_subtree(child, prepared) &&
+		    prepared && !core->prepare_count)
+			core->flags |= CLK_IGNORE_UNUSED;
 
-	if (core->prepare_count)
-		return;
+	if (core->flags & CLK_IGNORE_UNUSED || core->prepare_count)
+		goto out;
 
-	if (core->flags & CLK_IGNORE_UNUSED)
-		return;
+	if (!parent_prepared && (core->flags & CLK_OPS_PARENT_ENABLE))
+		goto out;
 
-	if (clk_core_is_prepared(core)) {
+	/* Do not unprepare an enabled clock */
+	if (clk_core_is_prepared(core) &&
+	    !clk_core_is_enabled(core)) {
 		trace_clk_unprepare(core);
 		if (core->ops->unprepare_unused)
 			core->ops->unprepare_unused(core->hw);
@@ -1477,27 +1483,50 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
 			core->ops->unprepare(core->hw);
 		trace_clk_unprepare_complete(core);
 	}
+
+out:
+	return (core->flags & CLK_IGNORE_UNUSED) && prepared;
 }
 
-static void __init clk_disable_unused_subtree(struct clk_core *core)
+static bool __init clk_disable_unused_subtree(struct clk_core *core,
+					      bool parent_enabled)
 {
 	struct clk_core *child;
 	unsigned long flags;
+	bool enabled;
 
 	lockdep_assert_held(&prepare_lock);
 
-	hlist_for_each_entry(child, &core->children, child_node)
-		clk_disable_unused_subtree(child);
+	flags = clk_enable_lock();
 
-	if (core->flags & CLK_OPS_PARENT_ENABLE)
-		clk_core_prepare_enable(core->parent);
+	/* Check if the clock is enabled from root to this clock */
+	if (parent_enabled)
+		enabled = clk_core_is_enabled(core);
+	else
+		enabled = false;
 
-	flags = clk_enable_lock();
+	hlist_for_each_entry(child, &core->children, child_node)
+		/*
+		 * If any child ignored disable, this clock should too,
+		 * unless there is, valid reason for the clock to be enabled
+		 */
+		if (clk_disable_unused_subtree(child, enabled) &&
+		    enabled && !core->enable_count)
+			core->flags |= CLK_IGNORE_UNUSED;
 
-	if (core->enable_count)
+	if (core->flags & CLK_IGNORE_UNUSED || core->enable_count)
 		goto unlock_out;
 
-	if (core->flags & CLK_IGNORE_UNUSED)
+	/*
+	 * If the parent is disabled but the gate is open, we should sanitize
+	 * the situation. This will avoid an unexpected enable of the clock as
+	 * soon as the parent is enabled, without control of CCF.
+	 *
+	 * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
+	 * forcefully enabling a whole part of the subtree.  Just let the
+	 * situation resolve it self on the first enable of the clock
+	 */
+	if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
 		goto unlock_out;
 
 	/*
@@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
 
 unlock_out:
 	clk_enable_unlock(flags);
-	if (core->flags & CLK_OPS_PARENT_ENABLE)
-		clk_core_disable_unprepare(core->parent);
+	return (core->flags & CLK_IGNORE_UNUSED) && enabled;
 }
 
 static bool clk_ignore_unused __initdata;
@@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
 	clk_prepare_lock();
 
 	hlist_for_each_entry(core, &clk_root_list, child_node)
-		clk_disable_unused_subtree(core);
+		clk_disable_unused_subtree(core, true);
 
 	hlist_for_each_entry(core, &clk_orphan_list, child_node)
-		clk_disable_unused_subtree(core);
+		clk_disable_unused_subtree(core, true);
 
 	hlist_for_each_entry(core, &clk_root_list, child_node)
-		clk_unprepare_unused_subtree(core);
+		clk_unprepare_unused_subtree(core, true);
 
 	hlist_for_each_entry(core, &clk_orphan_list, child_node)
-		clk_unprepare_unused_subtree(core);
+		clk_unprepare_unused_subtree(core, true);
 
 	clk_prepare_unlock();
 
-- 
2.45.2

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Chuan Liu]

Hi Martin,


On 2024/10/1 4:08, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
>
> Hello,
>
> On Sun, Sep 29, 2024 at 8:10 AM Chuan Liu via B4 Relay
> <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>> From: Chuan Liu <chuan.liu@amlogic.com>
>>
>> glitch free mux has two clock channels (channel 0 and channel 1) with
>> the same configuration. When the frequency needs to be changed, the two
>> channels ping-pong to ensure clock continuity and suppress glitch.
> You describe the solution to this below:
>> Add flag CLK_SET_RATE_GATE to channels 0 and 1 to implement Ping-Pong
>> switchover to suppress glitch.
> It would be great to have this change in a separate patch.
> The clocks to which you're adding CLK_SET_RATE_GATE aren't switched at
> runtime in mainline kernels (at least I think so).


Okay, I will separate it into two patches and submit it in the next version.


>
>> Channel 0 of glitch free mux is not only the clock source for the mux,
>> but also the working clock for glitch free mux. Therefore, when glitch
>> free mux switches, it is necessary to ensure that channel 0 has a clock
>> input, otherwise glitch free mux will not work and cannot switch to the
>> target channel.
> [...]
>> glitch free mux Add flag CLK_OPS_PARENT_ENABLE to ensure that channel 0
>> has clock input when switching channels.
> This describes a second problem. I think it's best to have this in a
> separate commit/patch.
> Also you're updating some mali clocks (e.g. on G12 and GX) but not all
> of them (Meson8b for example is missing).


Yes, M8 missed it, I will complete it in the next version.


>
> I still have some questions to the CLK_OPS_PARENT_ENABLE approach -
> please share your findings on this.
>
> There's multiple clocks involved in a glitch-free mux hierarchy:
> - a number of clock inputs (e.g. fclk, xtal, ...)
> - two muxes (one for every channel of the glitch-free mux)
> - two dividers (one for every channel of the glitch-free mux)
> - two gates (one for every channel of the glitch-free mux)
> - the glitch-free mux
>
> When switching from channel 0 (which is active and enabled) CCF
> (common clock framework) will:
> a) on channel 1:
> - find the best input clock
> - choose the best input clock in the mux
> - set the divider
> - enable the gate
> b) switch the glitch-free mux
> c) on channel 2:
> - disable the gate
>
> To me it's not clear at which level the problem occurs (glitch-free
> mux, gate, divider, mux, input clock).
> Also I don't understand why enabling the clocks with
> CLK_OPS_PARENT_ENABLE solves any problem since CCF is doing things
> automatically for us.
> Can you please explain (preferably with an example) what problem is
> solved with this approach?


If CLK_OPS_PARENT_ENABLE is configured in mux, 'new_parent' and
'old_parent' will be enabled first when __clk_set_parent_before() is
called. And disable them at __clk_set_parent_after(). Our glitch free
only has two clock sources, so adding this flag ensures that both
channels 0 and 1 are enabled when mux switches.

In fact, we just need to make sure that channel 0 is enabled. The
purpose of CLK_OPS_PARENT_ENABLE may not be to solve our situation,
but adding this flag does solve our current problem.


>
> Last but not least: if we're running into bugs when
> CLK_OPS_PARENT_ENABLE is missing then that patch should carry a Fixes
> tag.


Thanks for the heads-up. I'll keep an eye on it in the next version.


>
> Best regards,
> Martin

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Jerome Brunet]

On Tue 08 Oct 2024 at 13:44, Chuan Liu <chuan.liu@amlogic.com> wrote:

> Hi Martin,
>
>
> On 2024/10/1 4:08, Martin Blumenstingl wrote:
>> [ EXTERNAL EMAIL ]
>>
>> Hello,
>>
>> On Sun, Sep 29, 2024 at 8:10 AM Chuan Liu via B4 Relay
>> <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>>> From: Chuan Liu <chuan.liu@amlogic.com>
>>>
>>> glitch free mux has two clock channels (channel 0 and channel 1) with
>>> the same configuration. When the frequency needs to be changed, the two
>>> channels ping-pong to ensure clock continuity and suppress glitch.
>> You describe the solution to this below:
>>> Add flag CLK_SET_RATE_GATE to channels 0 and 1 to implement Ping-Pong
>>> switchover to suppress glitch.
>> It would be great to have this change in a separate patch.
>> The clocks to which you're adding CLK_SET_RATE_GATE aren't switched at
>> runtime in mainline kernels (at least I think so).
>
>
> Okay, I will separate it into two patches and submit it in the next version.
>
>
>>
>>> Channel 0 of glitch free mux is not only the clock source for the mux,
>>> but also the working clock for glitch free mux. Therefore, when glitch
>>> free mux switches, it is necessary to ensure that channel 0 has a clock
>>> input, otherwise glitch free mux will not work and cannot switch to the
>>> target channel.
>> [...]
>>> glitch free mux Add flag CLK_OPS_PARENT_ENABLE to ensure that channel 0
>>> has clock input when switching channels.
>> This describes a second problem. I think it's best to have this in a
>> separate commit/patch.
>> Also you're updating some mali clocks (e.g. on G12 and GX) but not all
>> of them (Meson8b for example is missing).
>
>
> Yes, M8 missed it, I will complete it in the next version.
>
>
>>
>> I still have some questions to the CLK_OPS_PARENT_ENABLE approach -
>> please share your findings on this.
>>
>> There's multiple clocks involved in a glitch-free mux hierarchy:
>> - a number of clock inputs (e.g. fclk, xtal, ...)
>> - two muxes (one for every channel of the glitch-free mux)
>> - two dividers (one for every channel of the glitch-free mux)
>> - two gates (one for every channel of the glitch-free mux)
>> - the glitch-free mux
>>
>> When switching from channel 0 (which is active and enabled) CCF
>> (common clock framework) will:
>> a) on channel 1:
>> - find the best input clock
>> - choose the best input clock in the mux
>> - set the divider
>> - enable the gate
>> b) switch the glitch-free mux
>> c) on channel 2:
>> - disable the gate
>>
>> To me it's not clear at which level the problem occurs (glitch-free
>> mux, gate, divider, mux, input clock).
>> Also I don't understand why enabling the clocks with
>> CLK_OPS_PARENT_ENABLE solves any problem since CCF is doing things
>> automatically for us.
>> Can you please explain (preferably with an example) what problem is
>> solved with this approach?
>
>
> If CLK_OPS_PARENT_ENABLE is configured in mux, 'new_parent' and
> 'old_parent' will be enabled first when __clk_set_parent_before() is
> called. And disable them at __clk_set_parent_after(). Our glitch free
> only has two clock sources, so adding this flag ensures that both
> channels 0 and 1 are enabled when mux switches.
>
> In fact, we just need to make sure that channel 0 is enabled. The
> purpose of CLK_OPS_PARENT_ENABLE may not be to solve our situation,
> but adding this flag does solve our current problem.

This is last point is important.

It is OK to use a side effect of CLK_OPS_PARENT_ENABLE but it needs to
be documented somehow, so what really matters is still known 2y from now.

Make sure the information appears in the code comments at least once.

>
>
>>
>> Last but not least: if we're running into bugs when
>> CLK_OPS_PARENT_ENABLE is missing then that patch should carry a Fixes
>> tag.
>
>
> Thanks for the heads-up. I'll keep an eye on it in the next version.
>
>
>>
>> Best regards,
>> Martin

-- 
Jerome

Re: [RFC PATCH] clk: core: refine disable unused clocks

[Chuan Liu]

hi Jerome:

     Tranks for your REF. I looked at your patch and there are some 
parts that I don't quite understand: The original intention of 
CLK_OPS_PARENT_ENABLE was to solve the issue of "parents need enable 
_during _gate/ungate, set rate and re-parent" when setting a clock. After setting 
the clock, it can still be disabled. However, from what I see in your 
patch, the handling logic seems more like "parents need _always _ gate 
during clock gate period"?

On 10/4/2024 9:39 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> As it as been pointed out numerous times, flagging a clock with
> CLK_IGNORE_UNUSED does _not_ guarantee that clock left enabled will stay
> on. The clock will get disabled if any enable/disable cycle happens on it
> or its parent clocks.
>
> Because enable/disable cycles will disable unused clocks,
> clk_disable_unused() should not trigger such cycle. Doing so disregard
> the flag if set for any parent clocks. This is problematic with
> CLK_OPS_PARENT_ENABLE handling.
>
> To solve this, and a couple other issues, pass the parent status to the
> child while walking the subtree, and return whether child ignored disable,
> or not.
>
> * Knowing the parent status allows to safely disable clocks with
>    CLK_OPS_PARENT_ENABLE when the parent is enabled. Otherwise it means
>    that, while the clock is not gated it is effectively disabled. Turning on
>    the parents to sanitize the sitation would bring back our initial
>    problem, so just let it sanitize itself when the clock gets used.
>
> * If a clock is not actively used (enabled_count == 0), does not have
>    CLK_IGNORE_UNUSED but the hw enabled all the way to the root clock, and a
>    child ignored the disable, it should ignore the disable too. Doing so
>    avoids disabling what is feading the children. Let the flag trickle down
>    the tree. This has the added benefit to transfer the information to the
>    unprepare path, so we don't unprepare the parent of a clock that ignored
>    a disable.
>
> * An enabled clock must be prepared in CCF but we can't rely solely on
>    counts at clk_disable_unused() stage. Make sure an enabled clock is
>    considered prepared too, even if does not implement the related callback.
>    Also make sure only disabled clocks get unprepared.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>
>   This is sent as an RFC to continue the discussion started by Chuan.
>   It is not meant to be applied as it is.
>
>
>   drivers/clk/clk.c | 92 ++++++++++++++++++++++++++++++-----------------
>   1 file changed, 60 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index d02451f951cf..41c4504a41f1 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -332,17 +332,6 @@ static bool clk_core_is_enabled(struct clk_core *core)
>                  }
>          }
>
> -       /*
> -        * This could be called with the enable lock held, or from atomic
> -        * context. If the parent isn't enabled already, we can't do
> -        * anything here. We can also assume this clock isn't enabled.
> -        */
> -       if ((core->flags & CLK_OPS_PARENT_ENABLE) && core->parent)

This judgment of CLK_OPS_PARENT_ENABLE seems redundant. According to
normal logic, if the parent is disabled, its children will also be
forced to disable. This seems unrelated to whether CLK_OPS_PARENT_ENABLE
is configured.😳

> -               if (!clk_core_is_enabled(core->parent)) {
> -                       ret = false;
> -                       goto done;
> -               }
> -
>          ret = core->ops->is_enabled(core->hw);
>   done:
>          if (core->rpm_enabled)
> @@ -1454,22 +1443,39 @@ static void clk_core_disable_unprepare(struct clk_core *core)
>          clk_core_unprepare_lock(core);
>   }
>
> -static void __init clk_unprepare_unused_subtree(struct clk_core *core)
> +static bool __init clk_unprepare_unused_subtree(struct clk_core *core,
> +                                               bool parent_prepared)
>   {
>          struct clk_core *child;
> +       bool prepared;
>
>          lockdep_assert_held(&prepare_lock);
>
> +       /*
> +        * Relying on count is not possible at this stage, so consider
> +        * prepared an enabled clock, in case only .is_enabled() is
> +        * implemented
> +        */
> +       if (parent_prepared)
> +               prepared = (clk_core_is_prepared(core) ||
> +                           clk_core_is_enabled(core));
> +       else
> +               prepared = false;
> +
>          hlist_for_each_entry(child, &core->children, child_node)
> -               clk_unprepare_unused_subtree(child);
> +               if (clk_unprepare_unused_subtree(child, prepared) &&
> +                   prepared && !core->prepare_count)
> +                       core->flags |= CLK_IGNORE_UNUSED;
>
> -       if (core->prepare_count)
> -               return;
> +       if (core->flags & CLK_IGNORE_UNUSED || core->prepare_count)
> +               goto out;
>
> -       if (core->flags & CLK_IGNORE_UNUSED)
> -               return;
> +       if (!parent_prepared && (core->flags & CLK_OPS_PARENT_ENABLE))
> +               goto out;
>
> -       if (clk_core_is_prepared(core)) {
> +       /* Do not unprepare an enabled clock */
> +       if (clk_core_is_prepared(core) &&
> +           !clk_core_is_enabled(core)) {
>                  trace_clk_unprepare(core);
>                  if (core->ops->unprepare_unused)
>                          core->ops->unprepare_unused(core->hw);
> @@ -1477,27 +1483,50 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
>                          core->ops->unprepare(core->hw);
>                  trace_clk_unprepare_complete(core);
>          }
> +
> +out:
> +       return (core->flags & CLK_IGNORE_UNUSED) && prepared;
>   }
>
> -static void __init clk_disable_unused_subtree(struct clk_core *core)
> +static bool __init clk_disable_unused_subtree(struct clk_core *core,
> +                                             bool parent_enabled)
>   {
>          struct clk_core *child;
>          unsigned long flags;
> +       bool enabled;
>
>          lockdep_assert_held(&prepare_lock);
>
> -       hlist_for_each_entry(child, &core->children, child_node)
> -               clk_disable_unused_subtree(child);
> +       flags = clk_enable_lock();
>
> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
> -               clk_core_prepare_enable(core->parent);
> +       /* Check if the clock is enabled from root to this clock */
> +       if (parent_enabled)
> +               enabled = clk_core_is_enabled(core);
> +       else
> +               enabled = false;
>
> -       flags = clk_enable_lock();
> +       hlist_for_each_entry(child, &core->children, child_node)
> +               /*
> +                * If any child ignored disable, this clock should too,
> +                * unless there is, valid reason for the clock to be enabled
> +                */
> +               if (clk_disable_unused_subtree(child, enabled) &&
> +                   enabled && !core->enable_count)
> +                       core->flags |= CLK_IGNORE_UNUSED;
>
> -       if (core->enable_count)
> +       if (core->flags & CLK_IGNORE_UNUSED || core->enable_count)
>                  goto unlock_out;
>
> -       if (core->flags & CLK_IGNORE_UNUSED)
> +       /*
> +        * If the parent is disabled but the gate is open, we should sanitize
> +        * the situation. This will avoid an unexpected enable of the clock as
> +        * soon as the parent is enabled, without control of CCF.
> +        *
> +        * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
> +        * forcefully enabling a whole part of the subtree.  Just let the
> +        * situation resolve it self on the first enable of the clock
> +        */
> +       if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
>                  goto unlock_out;
>
>          /*
> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>
>   unlock_out:
>          clk_enable_unlock(flags);
> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
> -               clk_core_disable_unprepare(core->parent);
> +       return (core->flags & CLK_IGNORE_UNUSED) && enabled;
>   }
>
>   static bool clk_ignore_unused __initdata;
> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
>          clk_prepare_lock();
>
>          hlist_for_each_entry(core, &clk_root_list, child_node)
> -               clk_disable_unused_subtree(core);
> +               clk_disable_unused_subtree(core, true);
>
>          hlist_for_each_entry(core, &clk_orphan_list, child_node)
> -               clk_disable_unused_subtree(core);
> +               clk_disable_unused_subtree(core, true);
>
>          hlist_for_each_entry(core, &clk_root_list, child_node)
> -               clk_unprepare_unused_subtree(core);
> +               clk_unprepare_unused_subtree(core, true);
>
>          hlist_for_each_entry(core, &clk_orphan_list, child_node)
> -               clk_unprepare_unused_subtree(core);
> +               clk_unprepare_unused_subtree(core, true);
>
>          clk_prepare_unlock();
>
> --
> 2.45.2
>

Re: [RFC PATCH] clk: core: refine disable unused clocks

[Jerome Brunet]

On Fri 08 Nov 2024 at 15:59, Chuan Liu <chuan.liu@amlogic.com> wrote:

> hi Jerome:
>
>     Tranks for your REF. I looked at your patch and there are some parts
> that I don't quite understand: The original intention of
> CLK_OPS_PARENT_ENABLE was to solve the issue of "parents need enable
> _during _gate/ungate, set rate and re-parent" when setting a clock. After
> setting the clock, it can still be disabled. However, from what I see in
> your patch, the handling logic seems more like "parents need _always _ gate
> during clock gate period"?

As explained in the description, the problem with CLK_IGNORE_UNUSED and
CLK_OPS_PARENT_ENABLE is that you'll get cycle of enable/disable, which
will disable any parent clock that may have a been enabled and expected
to be ignored.

IOW, the CCF changes the state of the tree while inspecting it.
This change solves that.

>
> On 10/4/2024 9:39 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> As it as been pointed out numerous times, flagging a clock with
>> CLK_IGNORE_UNUSED does _not_ guarantee that clock left enabled will stay
>> on. The clock will get disabled if any enable/disable cycle happens on it
>> or its parent clocks.
>>
>> Because enable/disable cycles will disable unused clocks,
>> clk_disable_unused() should not trigger such cycle. Doing so disregard
>> the flag if set for any parent clocks. This is problematic with
>> CLK_OPS_PARENT_ENABLE handling.
>>
>> To solve this, and a couple other issues, pass the parent status to the
>> child while walking the subtree, and return whether child ignored disable,
>> or not.
>>
>> * Knowing the parent status allows to safely disable clocks with
>>    CLK_OPS_PARENT_ENABLE when the parent is enabled. Otherwise it means
>>    that, while the clock is not gated it is effectively disabled. Turning on
>>    the parents to sanitize the sitation would bring back our initial
>>    problem, so just let it sanitize itself when the clock gets used.
>>
>> * If a clock is not actively used (enabled_count == 0), does not have
>>    CLK_IGNORE_UNUSED but the hw enabled all the way to the root clock, and a
>>    child ignored the disable, it should ignore the disable too. Doing so
>>    avoids disabling what is feading the children. Let the flag trickle down
>>    the tree. This has the added benefit to transfer the information to the
>>    unprepare path, so we don't unprepare the parent of a clock that ignored
>>    a disable.
>>
>> * An enabled clock must be prepared in CCF but we can't rely solely on
>>    counts at clk_disable_unused() stage. Make sure an enabled clock is
>>    considered prepared too, even if does not implement the related callback.
>>    Also make sure only disabled clocks get unprepared.
>>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>
>>   This is sent as an RFC to continue the discussion started by Chuan.
>>   It is not meant to be applied as it is.
>>
>>
>>   drivers/clk/clk.c | 92 ++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 60 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index d02451f951cf..41c4504a41f1 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -332,17 +332,6 @@ static bool clk_core_is_enabled(struct clk_core *core)
>>                  }
>>          }
>>
>> -       /*
>> -        * This could be called with the enable lock held, or from atomic
>> -        * context. If the parent isn't enabled already, we can't do
>> -        * anything here. We can also assume this clock isn't enabled.
>> -        */
>> -       if ((core->flags & CLK_OPS_PARENT_ENABLE) && core->parent)
>
> This judgment of CLK_OPS_PARENT_ENABLE seems redundant. According to
> normal logic, if the parent is disabled, its children will also be
> forced to disable. This seems unrelated to whether CLK_OPS_PARENT_ENABLE
> is configured.😳

It's removed.

>
>> -               if (!clk_core_is_enabled(core->parent)) {
>> -                       ret = false;
>> -                       goto done;
>> -               }
>> -
>>          ret = core->ops->is_enabled(core->hw);
>>   done:
>>          if (core->rpm_enabled)
>> @@ -1454,22 +1443,39 @@ static void clk_core_disable_unprepare(struct clk_core *core)
>>          clk_core_unprepare_lock(core);
>>   }
>>
>> -static void __init clk_unprepare_unused_subtree(struct clk_core *core)
>> +static bool __init clk_unprepare_unused_subtree(struct clk_core *core,
>> +                                               bool parent_prepared)
>>   {
>>          struct clk_core *child;
>> +       bool prepared;
>>
>>          lockdep_assert_held(&prepare_lock);
>>
>> +       /*
>> +        * Relying on count is not possible at this stage, so consider
>> +        * prepared an enabled clock, in case only .is_enabled() is
>> +        * implemented
>> +        */
>> +       if (parent_prepared)
>> +               prepared = (clk_core_is_prepared(core) ||
>> +                           clk_core_is_enabled(core));
>> +       else
>> +               prepared = false;
>> +
>>          hlist_for_each_entry(child, &core->children, child_node)
>> -               clk_unprepare_unused_subtree(child);
>> +               if (clk_unprepare_unused_subtree(child, prepared) &&
>> +                   prepared && !core->prepare_count)
>> +                       core->flags |= CLK_IGNORE_UNUSED;
>>
>> -       if (core->prepare_count)
>> -               return;
>> +       if (core->flags & CLK_IGNORE_UNUSED || core->prepare_count)
>> +               goto out;
>>
>> -       if (core->flags & CLK_IGNORE_UNUSED)
>> -               return;
>> +       if (!parent_prepared && (core->flags & CLK_OPS_PARENT_ENABLE))
>> +               goto out;
>>
>> -       if (clk_core_is_prepared(core)) {
>> +       /* Do not unprepare an enabled clock */
>> +       if (clk_core_is_prepared(core) &&
>> +           !clk_core_is_enabled(core)) {
>>                  trace_clk_unprepare(core);
>>                  if (core->ops->unprepare_unused)
>>                          core->ops->unprepare_unused(core->hw);
>> @@ -1477,27 +1483,50 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
>>                          core->ops->unprepare(core->hw);
>>                  trace_clk_unprepare_complete(core);
>>          }
>> +
>> +out:
>> +       return (core->flags & CLK_IGNORE_UNUSED) && prepared;
>>   }
>>
>> -static void __init clk_disable_unused_subtree(struct clk_core *core)
>> +static bool __init clk_disable_unused_subtree(struct clk_core *core,
>> +                                             bool parent_enabled)
>>   {
>>          struct clk_core *child;
>>          unsigned long flags;
>> +       bool enabled;
>>
>>          lockdep_assert_held(&prepare_lock);
>>
>> -       hlist_for_each_entry(child, &core->children, child_node)
>> -               clk_disable_unused_subtree(child);
>> +       flags = clk_enable_lock();
>>
>> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
>> -               clk_core_prepare_enable(core->parent);
>> +       /* Check if the clock is enabled from root to this clock */
>> +       if (parent_enabled)
>> +               enabled = clk_core_is_enabled(core);
>> +       else
>> +               enabled = false;
>>
>> -       flags = clk_enable_lock();
>> +       hlist_for_each_entry(child, &core->children, child_node)
>> +               /*
>> +                * If any child ignored disable, this clock should too,
>> +                * unless there is, valid reason for the clock to be enabled
>> +                */
>> +               if (clk_disable_unused_subtree(child, enabled) &&
>> +                   enabled && !core->enable_count)
>> +                       core->flags |= CLK_IGNORE_UNUSED;
>>
>> -       if (core->enable_count)
>> +       if (core->flags & CLK_IGNORE_UNUSED || core->enable_count)
>>                  goto unlock_out;
>>
>> -       if (core->flags & CLK_IGNORE_UNUSED)
>> +       /*
>> +        * If the parent is disabled but the gate is open, we should sanitize
>> +        * the situation. This will avoid an unexpected enable of the clock as
>> +        * soon as the parent is enabled, without control of CCF.
>> +        *
>> +        * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
>> +        * forcefully enabling a whole part of the subtree.  Just let the
>> +        * situation resolve it self on the first enable of the clock
>> +        */
>> +       if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
>>                  goto unlock_out;
>>
>>          /*
>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>
>>   unlock_out:
>>          clk_enable_unlock(flags);
>> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
>> -               clk_core_disable_unprepare(core->parent);
>> +       return (core->flags & CLK_IGNORE_UNUSED) && enabled;
>>   }
>>
>>   static bool clk_ignore_unused __initdata;
>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
>>          clk_prepare_lock();
>>
>>          hlist_for_each_entry(core, &clk_root_list, child_node)
>> -               clk_disable_unused_subtree(core);
>> +               clk_disable_unused_subtree(core, true);
>>
>>          hlist_for_each_entry(core, &clk_orphan_list, child_node)
>> -               clk_disable_unused_subtree(core);
>> +               clk_disable_unused_subtree(core, true);
>>
>>          hlist_for_each_entry(core, &clk_root_list, child_node)
>> -               clk_unprepare_unused_subtree(core);
>> +               clk_unprepare_unused_subtree(core, true);
>>
>>          hlist_for_each_entry(core, &clk_orphan_list, child_node)
>> -               clk_unprepare_unused_subtree(core);
>> +               clk_unprepare_unused_subtree(core, true);
>>
>>          clk_prepare_unlock();
>>
>> --
>> 2.45.2
>>

-- 
Jerome

Re: [RFC PATCH] clk: core: refine disable unused clocks

[Chuan Liu]

On 11/8/2024 4:38 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 08 Nov 2024 at 15:59, Chuan Liu <chuan.liu@amlogic.com> wrote:
>
>> hi Jerome:
>>
>>      Tranks for your REF. I looked at your patch and there are some parts
>> that I don't quite understand: The original intention of
>> CLK_OPS_PARENT_ENABLE was to solve the issue of "parents need enable
>> _during _gate/ungate, set rate and re-parent" when setting a clock. After
>> setting the clock, it can still be disabled. However, from what I see in
>> your patch, the handling logic seems more like "parents need _always _ gate
>> during clock gate period"?
> As explained in the description, the problem with CLK_IGNORE_UNUSED and
> CLK_OPS_PARENT_ENABLE is that you'll get cycle of enable/disable, which
> will disable any parent clock that may have a been enabled and expected
> to be ignored.
>
> IOW, the CCF changes the state of the tree while inspecting it.
> This change solves that.

Ok, I understand your idea now... I have gotten myself tangled up.

>> On 10/4/2024 9:39 PM, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> As it as been pointed out numerous times, flagging a clock with
>>> CLK_IGNORE_UNUSED does _not_ guarantee that clock left enabled will stay
>>> on. The clock will get disabled if any enable/disable cycle happens on it
>>> or its parent clocks.
>>>
>>> Because enable/disable cycles will disable unused clocks,
>>> clk_disable_unused() should not trigger such cycle. Doing so disregard
>>> the flag if set for any parent clocks. This is problematic with
>>> CLK_OPS_PARENT_ENABLE handling.
>>>
>>> To solve this, and a couple other issues, pass the parent status to the
>>> child while walking the subtree, and return whether child ignored disable,
>>> or not.
>>>
>>> * Knowing the parent status allows to safely disable clocks with
>>>     CLK_OPS_PARENT_ENABLE when the parent is enabled. Otherwise it means
>>>     that, while the clock is not gated it is effectively disabled. Turning on
>>>     the parents to sanitize the sitation would bring back our initial
>>>     problem, so just let it sanitize itself when the clock gets used.
>>>
>>> * If a clock is not actively used (enabled_count == 0), does not have
>>>     CLK_IGNORE_UNUSED but the hw enabled all the way to the root clock, and a
>>>     child ignored the disable, it should ignore the disable too. Doing so
>>>     avoids disabling what is feading the children. Let the flag trickle down
>>>     the tree. This has the added benefit to transfer the information to the
>>>     unprepare path, so we don't unprepare the parent of a clock that ignored
>>>     a disable.
>>>
>>> * An enabled clock must be prepared in CCF but we can't rely solely on
>>>     counts at clk_disable_unused() stage. Make sure an enabled clock is
>>>     considered prepared too, even if does not implement the related callback.
>>>     Also make sure only disabled clocks get unprepared.
>>>
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>> ---
>>>
>>>    This is sent as an RFC to continue the discussion started by Chuan.
>>>    It is not meant to be applied as it is.
>>>
>>>
>>>    drivers/clk/clk.c | 92 ++++++++++++++++++++++++++++++-----------------
>>>    1 file changed, 60 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>> index d02451f951cf..41c4504a41f1 100644
>>> --- a/drivers/clk/clk.c
>>> +++ b/drivers/clk/clk.c
>>> @@ -332,17 +332,6 @@ static bool clk_core_is_enabled(struct clk_core *core)
>>>                   }
>>>           }
>>>
>>> -       /*
>>> -        * This could be called with the enable lock held, or from atomic
>>> -        * context. If the parent isn't enabled already, we can't do
>>> -        * anything here. We can also assume this clock isn't enabled.
>>> -        */
>>> -       if ((core->flags & CLK_OPS_PARENT_ENABLE) && core->parent)
>> This judgment of CLK_OPS_PARENT_ENABLE seems redundant. According to
>> normal logic, if the parent is disabled, its children will also be
>> forced to disable. This seems unrelated to whether CLK_OPS_PARENT_ENABLE
>> is configured.😳
> It's removed.

Yes, I just want to express that judging CLK_OPS_PARENT_ENABLE seems unnecessary here.


>
>>> -               if (!clk_core_is_enabled(core->parent)) {
>>> -                       ret = false;
>>> -                       goto done;
>>> -               }
>>> -
>>>           ret = core->ops->is_enabled(core->hw);
>>>    done:
>>>           if (core->rpm_enabled)
>>> @@ -1454,22 +1443,39 @@ static void clk_core_disable_unprepare(struct clk_core *core)
>>>           clk_core_unprepare_lock(core);
>>>    }

[ . . . ]

>>> -       if (core->flags & CLK_IGNORE_UNUSED)
>>> +       /*
>>> +        * If the parent is disabled but the gate is open, we should sanitize
>>> +        * the situation. This will avoid an unexpected enable of the clock as
>>> +        * soon as the parent is enabled, without control of CCF.
>>> +        *
>>> +        * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
>>> +        * forcefully enabling a whole part of the subtree.  Just let the
>>> +        * situation resolve it self on the first enable of the clock
>>> +        */
>>> +       if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))

At first, I couldn't grasp the logic behind the 'return' here. Now it's
clear. This approach is equivalent to completely giving up on
handling clocks with CLK_OPS_PARENT_ENABLE feature in
clk_disable_unused_subtree().

>>>                   goto unlock_out;
>>>
>>>           /*
>>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>>
>>>    unlock_out:
>>>           clk_enable_unlock(flags);
>>> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
>>> -               clk_core_disable_unprepare(core->parent);
>>> +       return (core->flags & CLK_IGNORE_UNUSED) && enabled;
>>>    }
>>>
>>>    static bool clk_ignore_unused __initdata;
>>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
>>>           clk_prepare_lock();
>>>
>>>           hlist_for_each_entry(core, &clk_root_list, child_node)
>>> -               clk_disable_unused_subtree(core);
>>> +               clk_disable_unused_subtree(core, true);
>>>
>>>           hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>> -               clk_disable_unused_subtree(core);
>>> +               clk_disable_unused_subtree(core, true);
>>>
>>>           hlist_for_each_entry(core, &clk_root_list, child_node)
>>> -               clk_unprepare_unused_subtree(core);
>>> +               clk_unprepare_unused_subtree(core, true);
>>>
>>>           hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>> -               clk_unprepare_unused_subtree(core);
>>> +               clk_unprepare_unused_subtree(core, true);
>>>
>>>           clk_prepare_unlock();
>>>
>>> --
>>> 2.45.2
>>>
> --
> Jerome

Re: [RFC PATCH] clk: core: refine disable unused clocks

[Jerome Brunet]

On Fri 08 Nov 2024 at 17:23, Chuan Liu <chuan.liu@amlogic.com> wrote:

>>>> -       if (core->flags & CLK_IGNORE_UNUSED)
>>>> +       /*
>>>> +        * If the parent is disabled but the gate is open, we should sanitize
>>>> +        * the situation. This will avoid an unexpected enable of the clock as
>>>> +        * soon as the parent is enabled, without control of CCF.
>>>> +        *
>>>> +        * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
>>>> +        * forcefully enabling a whole part of the subtree.  Just let the
>>>> +        * situation resolve it self on the first enable of the clock
>>>> +        */
>>>> +       if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
>
> At first, I couldn't grasp the logic behind the 'return' here. Now it's
> clear. This approach is equivalent to completely giving up on
> handling clocks with CLK_OPS_PARENT_ENABLE feature in
> clk_disable_unused_subtree().
>

No. It's handled correctly as long as the tree is in coherent state.

What is not done anymore is fixing up an inconsistent tree, by this I
mean: A clock with CLK_OPS_PARENT_ENABLE, which report enabled from its
own registers but has its parent disabled.

In that particular case, clk_disable_unused_subtree() won't be turning on
everything to properly disable that one clock. That is the root cause of
the problem you reported initially. The clock is disabled anyway.

Every other case are properly handled (at least I think).

>>>>                   goto unlock_out;
>>>>
>>>>           /*
>>>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>>>
>>>>    unlock_out:
>>>>           clk_enable_unlock(flags);
>>>> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
>>>> -               clk_core_disable_unprepare(core->parent);
>>>> +       return (core->flags & CLK_IGNORE_UNUSED) && enabled;
>>>>    }
>>>>
>>>>    static bool clk_ignore_unused __initdata;
>>>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
>>>>           clk_prepare_lock();
>>>>
>>>>           hlist_for_each_entry(core, &clk_root_list, child_node)
>>>> -               clk_disable_unused_subtree(core);
>>>> +               clk_disable_unused_subtree(core, true);
>>>>
>>>>           hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>> -               clk_disable_unused_subtree(core);
>>>> +               clk_disable_unused_subtree(core, true);
>>>>
>>>>           hlist_for_each_entry(core, &clk_root_list, child_node)
>>>> -               clk_unprepare_unused_subtree(core);
>>>> +               clk_unprepare_unused_subtree(core, true);
>>>>
>>>>           hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>> -               clk_unprepare_unused_subtree(core);
>>>> +               clk_unprepare_unused_subtree(core, true);
>>>>
>>>>           clk_prepare_unlock();
>>>>
>>>> --
>>>> 2.45.2
>>>>
>> --
>> Jerome

-- 
Jerome

Re: [RFC PATCH] clk: core: refine disable unused clocks

[Chuan Liu]

On 11/8/2024 5:59 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 08 Nov 2024 at 17:23, Chuan Liu <chuan.liu@amlogic.com> wrote:
>
>>>>> -       if (core->flags & CLK_IGNORE_UNUSED)
>>>>> +       /*
>>>>> +        * If the parent is disabled but the gate is open, we should sanitize
>>>>> +        * the situation. This will avoid an unexpected enable of the clock as
>>>>> +        * soon as the parent is enabled, without control of CCF.
>>>>> +        *
>>>>> +        * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
>>>>> +        * forcefully enabling a whole part of the subtree.  Just let the
>>>>> +        * situation resolve it self on the first enable of the clock
>>>>> +        */
>>>>> +       if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
>> At first, I couldn't grasp the logic behind the 'return' here. Now it's
>> clear. This approach is equivalent to completely giving up on
>> handling clocks with CLK_OPS_PARENT_ENABLE feature in
>> clk_disable_unused_subtree().
>>
> No. It's handled correctly as long as the tree is in coherent state.
>
> What is not done anymore is fixing up an inconsistent tree, by this I
> mean: A clock with CLK_OPS_PARENT_ENABLE, which report enabled from its
> own registers but has its parent disabled.
>
> In that particular case, clk_disable_unused_subtree() won't be turning on
> everything to properly disable that one clock. That is the root cause of
> the problem you reported initially. The clock is disabled anyway.
>
> Every other case are properly handled (at least I think).

name              en_sts            flags
clk_a                1          CLK_IGNORE_UNUSED
     clk_b            0                0
         clk_c        1         CLK_OPS_PARENT_ENABLE

Based on the above case:
1. When 'clk_c' is configured with CLK_OPS_PARENT_ENABLE, disabling
'clk_c' requires enabling 'clk_b' first (disabling 'clk_c' before
disabling 'clk_b'). How can to ensure that during the period of
disabling 'clk_c', 'clk_b' remains enabled?

2. 'clk_c' is not configured with CLK_IGNORE_UNUSED, it should be
disabled later. However, here it goes to a 'goto' statement and then
return 'false', ultimately resulting in 'clk_c' not being disabled?

>>>>>                    goto unlock_out;
>>>>>
>>>>>            /*
>>>>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>>>>
>>>>>     unlock_out:
>>>>>            clk_enable_unlock(flags);
>>>>> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
>>>>> -               clk_core_disable_unprepare(core->parent);
>>>>> +       return (core->flags & CLK_IGNORE_UNUSED) && enabled;
>>>>>     }
>>>>>
>>>>>     static bool clk_ignore_unused __initdata;
>>>>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
>>>>>            clk_prepare_lock();
>>>>>
>>>>>            hlist_for_each_entry(core, &clk_root_list, child_node)
>>>>> -               clk_disable_unused_subtree(core);
>>>>> +               clk_disable_unused_subtree(core, true);
>>>>>
>>>>>            hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>>> -               clk_disable_unused_subtree(core);
>>>>> +               clk_disable_unused_subtree(core, true);
>>>>>
>>>>>            hlist_for_each_entry(core, &clk_root_list, child_node)
>>>>> -               clk_unprepare_unused_subtree(core);
>>>>> +               clk_unprepare_unused_subtree(core, true);
>>>>>
>>>>>            hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>>> -               clk_unprepare_unused_subtree(core);
>>>>> +               clk_unprepare_unused_subtree(core, true);
>>>>>
>>>>>            clk_prepare_unlock();
>>>>>
>>>>> --
>>>>> 2.45.2
>>>>>
>>> --
>>> Jerome
> --
> Jerome

Re: [PATCH 1/2] clk: Fix the CLK_IGNORE_UNUSED failure issue

[Chuan Liu]

Hi Jerome:

     Thanks for your suggestion.


On 9/30/2024 8:27 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Sun 29 Sep 2024 at 14:10, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>
>> From: Chuan Liu <chuan.liu@amlogic.com>
>>
>> When the clk_disable_unused_subtree() function disables an unused clock,
>> if CLK_OPS_PARENT_ENABLE is configured on the clock,
>> clk_core_prepare_enable() and clk_core_disable_unprepare() are called
>> directly, and these two functions do not determine CLK_IGNORE_UNUSED,
>> This causes the clock to be disabled even if CLK_IGNORE_UNUSED is
>> configured when clk_core_disable_unprepare() is called.
>>
>> Two new functions clk_disable_unprepare_unused() and
>> clk_prepare_enable_unused() are added to resolve the preceding
>> situation. The CLK_IGNORE_UNUSED judgment logic is added to these two
>> functions. To prevent clock configuration CLK_IGNORE_UNUSED from
>> possible failure.
>>
>> Change-Id: I56943e17b86436254f07d9b8cdbc35599328d519
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>>   drivers/clk/clk.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 65 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 285ed1ad8a37..5d3316699b57 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -94,6 +94,7 @@ struct clk_core {
>>        struct hlist_node       debug_node;
>>   #endif
>>        struct kref             ref;
>> +     bool                    ignore_enabled;
>>   };
>>
>>   #define CREATE_TRACE_POINTS
>> @@ -1479,6 +1480,68 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
>>        }
>>   }
>>
>> +static void __init clk_disable_unprepare_unused(struct clk_core *core)
>> +{
>> +     unsigned long flags;
>> +
>> +     lockdep_assert_held(&prepare_lock);
>> +
>> +     if (!core)
>> +             return;
>> +
>> +     if ((core->enable_count == 0) && core->ops->disable &&
>> +         !core->ignore_enabled) {
>> +             flags = clk_enable_lock();
> Used core->enable_count without taking the lock

My understanding is that adding a spinlock here is to ensure that
the disabling of the clock can be completed without interference.

>
>> +             core->ops->disable(core->hw);
> If the there is any CLK_IS_CRITICAL in the path, it is game over.
> You've basically disregarded all the other CCF flags which are equally
> important to the ones you are dealing with.

if clock is CLK_IS_CRITICAL then its enable_count > 0 does not go into
the 'if'.

>
>> +             clk_enable_unlock(flags);
>> +     }
>> +
>> +     if ((core->prepare_count == 0) && core->ops->unprepare &&
>> +         !core->ignore_enabled)
>> +             core->ops->unprepare(core->hw);
>> +
>> +     core->ignore_enabled = false;
>> +
>> +     clk_disable_unprepare_unused(core->parent);
> Here you are disabling the parent of any CLK_IGNORE_UNUSED clock.
> IMO, the problem is not solved. It just shifted.

Yes, it does not take into account the situation where its parent is
disabled without configuring CLK_IGNORE_UNUSED.

>
>> +}
>> +
>> +static int __init clk_prepare_enable_unused(struct clk_core *core)
>> +{
>> +     int ret = 0;
>> +     unsigned long flags;
>> +
>> +     lockdep_assert_held(&prepare_lock);
>> +
>> +     if (!core)
>> +             return 0;
>> +
>> +     ret = clk_prepare_enable_unused(core->parent);
>> +     if (ret)
>> +             return ret;
> That's adding another recursion in CCF, something Stephen would like to remove

This patch is meant to throw out an idea and bring attention to the
problem that was discovered.

>> +
>> +     if ((core->flags & CLK_IGNORE_UNUSED) && clk_core_is_enabled(core))
>> +             core->ignore_enabled = true;
>> +
>> +     if ((core->prepare_count == 0) && core->ops->prepare) {
>> +             ret = core->ops->prepare(core->hw);
>> +             if (ret)
>> +                     goto disable_unprepare;
>> +     }
>> +
>> +     if ((core->enable_count == 0) && core->ops->enable) {
>> +             flags = clk_enable_lock();
>> +             ret = core->ops->enable(core->hw);
>> +             clk_enable_unlock(flags);
>> +             if (ret)
>> +                     goto disable_unprepare;
>> +     }
>> +
>> +     return 0;
>> +disable_unprepare:
>> +     clk_disable_unprepare_unused(core->parent);
>> +     return ret;
>> +}
>> +
>>   static void __init clk_disable_unused_subtree(struct clk_core *core)
>>   {
>>        struct clk_core *child;
>> @@ -1490,7 +1553,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>                clk_disable_unused_subtree(child);
>>
>>        if (core->flags & CLK_OPS_PARENT_ENABLE)
>> -             clk_core_prepare_enable(core->parent);
>> +             clk_prepare_enable_unused(core->parent);
>>
>>        flags = clk_enable_lock();
>>
>> @@ -1517,7 +1580,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>   unlock_out:
>>        clk_enable_unlock(flags);
>>        if (core->flags & CLK_OPS_PARENT_ENABLE)
>> -             clk_core_disable_unprepare(core->parent);
>> +             clk_disable_unprepare_unused(core->parent);
>>   }
>>
>>   static bool clk_ignore_unused __initdata;
> --
> Jerome

Re: [RFC PATCH] clk: core: refine disable unused clocks

[Jerome Brunet]

On Fri 08 Nov 2024 at 19:49, Chuan Liu <chuan.liu@amlogic.com> wrote:

> On 11/8/2024 5:59 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On Fri 08 Nov 2024 at 17:23, Chuan Liu <chuan.liu@amlogic.com> wrote:
>>
>>>>>> -       if (core->flags & CLK_IGNORE_UNUSED)
>>>>>> +       /*
>>>>>> +        * If the parent is disabled but the gate is open, we should sanitize
>>>>>> +        * the situation. This will avoid an unexpected enable of the clock as
>>>>>> +        * soon as the parent is enabled, without control of CCF.
>>>>>> +        *
>>>>>> +        * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
>>>>>> +        * forcefully enabling a whole part of the subtree.  Just let the
>>>>>> +        * situation resolve it self on the first enable of the clock
>>>>>> +        */
>>>>>> +       if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
>>> At first, I couldn't grasp the logic behind the 'return' here. Now it's
>>> clear. This approach is equivalent to completely giving up on
>>> handling clocks with CLK_OPS_PARENT_ENABLE feature in
>>> clk_disable_unused_subtree().
>>>
>> No. It's handled correctly as long as the tree is in coherent state.
>>
>> What is not done anymore is fixing up an inconsistent tree, by this I
>> mean: A clock with CLK_OPS_PARENT_ENABLE, which report enabled from its
>> own registers but has its parent disabled.
>>
>> In that particular case, clk_disable_unused_subtree() won't be turning on
>> everything to properly disable that one clock. That is the root cause of
>> the problem you reported initially. The clock is disabled anyway.
>>
>> Every other case are properly handled (at least I think).
>
> name              en_sts            flags
> clk_a                1          CLK_IGNORE_UNUSED
>     clk_b            0                0
>         clk_c        1         CLK_OPS_PARENT_ENABLE
>
> Based on the above case:
> 1. When 'clk_c' is configured with CLK_OPS_PARENT_ENABLE, disabling
> 'clk_c' requires enabling 'clk_b' first (disabling 'clk_c' before
> disabling 'clk_b'). How can to ensure that during the period of
> disabling 'clk_c', 'clk_b' remains enabled?

That's perfect example of incoherent state.
How can 'clk_c' be enabled if its parent is disable. That makes no
sense, so there is no point enabling a whole subtree for this IMO.

>
> 2. 'clk_c' is not configured with CLK_IGNORE_UNUSED, it should be
> disabled later. However, here it goes to a 'goto' statement and then
> return 'false', ultimately resulting in 'clk_c' not being disabled?

We've discussed that 2 times already. This discussion is going in
circles now.

>
>>>>>>                    goto unlock_out;
>>>>>>
>>>>>>            /*
>>>>>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>>>>>
>>>>>>     unlock_out:
>>>>>>            clk_enable_unlock(flags);
>>>>>> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
>>>>>> -               clk_core_disable_unprepare(core->parent);
>>>>>> +       return (core->flags & CLK_IGNORE_UNUSED) && enabled;
>>>>>>     }
>>>>>>
>>>>>>     static bool clk_ignore_unused __initdata;
>>>>>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
>>>>>>            clk_prepare_lock();
>>>>>>
>>>>>>            hlist_for_each_entry(core, &clk_root_list, child_node)
>>>>>> -               clk_disable_unused_subtree(core);
>>>>>> +               clk_disable_unused_subtree(core, true);
>>>>>>
>>>>>>            hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>>>> -               clk_disable_unused_subtree(core);
>>>>>> +               clk_disable_unused_subtree(core, true);
>>>>>>
>>>>>>            hlist_for_each_entry(core, &clk_root_list, child_node)
>>>>>> -               clk_unprepare_unused_subtree(core);
>>>>>> +               clk_unprepare_unused_subtree(core, true);
>>>>>>
>>>>>>            hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>>>> -               clk_unprepare_unused_subtree(core);
>>>>>> +               clk_unprepare_unused_subtree(core, true);
>>>>>>
>>>>>>            clk_prepare_unlock();
>>>>>>
>>>>>> --
>>>>>> 2.45.2
>>>>>>
>>>> --
>>>> Jerome
>> --
>> Jerome

-- 
Jerome

Re: [RFC PATCH] clk: core: refine disable unused clocks

[Chuan Liu]

On 11/12/2024 4:36 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 08 Nov 2024 at 19:49, Chuan Liu <chuan.liu@amlogic.com> wrote:
>
>> On 11/8/2024 5:59 PM, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Fri 08 Nov 2024 at 17:23, Chuan Liu <chuan.liu@amlogic.com> wrote:
>>>
>>>>>>> -       if (core->flags & CLK_IGNORE_UNUSED)
>>>>>>> +       /*
>>>>>>> +        * If the parent is disabled but the gate is open, we should sanitize
>>>>>>> +        * the situation. This will avoid an unexpected enable of the clock as
>>>>>>> +        * soon as the parent is enabled, without control of CCF.
>>>>>>> +        *
>>>>>>> +        * Doing so is not possible with a CLK_OPS_PARENT_ENABLE clock without
>>>>>>> +        * forcefully enabling a whole part of the subtree.  Just let the
>>>>>>> +        * situation resolve it self on the first enable of the clock
>>>>>>> +        */
>>>>>>> +       if (!parent_enabled && (core->flags & CLK_OPS_PARENT_ENABLE))
>>>> At first, I couldn't grasp the logic behind the 'return' here. Now it's
>>>> clear. This approach is equivalent to completely giving up on
>>>> handling clocks with CLK_OPS_PARENT_ENABLE feature in
>>>> clk_disable_unused_subtree().

Referring to the situation of 'clk_c' below, combined with your
previous explanation:

* Knowing the parent status allows to safely disable clocks with
   CLK_OPS_PARENT_ENABLE when the parent is enabled. Otherwise it means
   that, while the clock is not gated it is effectively disabled. Turning on
   the parents to sanitize the sitation would bring back our initial
   problem, so just let it sanitize itself when the clock gets used.

Do you mean 'clk_c' cases should be sanitized before clk_disable_unused()
(such as during driver probe(), etc.)? Dropped in clk_disable_unused_subtree()?
This is actually my biggest confusion.🙁

>>>>
>>> No. It's handled correctly as long as the tree is in coherent state.
>>>
>>> What is not done anymore is fixing up an inconsistent tree, by this I
>>> mean: A clock with CLK_OPS_PARENT_ENABLE, which report enabled from its
>>> own registers but has its parent disabled.
>>>
>>> In that particular case, clk_disable_unused_subtree() won't be turning on
>>> everything to properly disable that one clock. That is the root cause of
>>> the problem you reported initially. The clock is disabled anyway.
>>>
>>> Every other case are properly handled (at least I think).
>> name              en_sts            flags
>> clk_a                1          CLK_IGNORE_UNUSED
>>      clk_b            0                0
>>          clk_c        1         CLK_OPS_PARENT_ENABLE
>>
>> Based on the above case:
>> 1. When 'clk_c' is configured with CLK_OPS_PARENT_ENABLE, disabling
>> 'clk_c' requires enabling 'clk_b' first (disabling 'clk_c' before
>> disabling 'clk_b'). How can to ensure that during the period of
>> disabling 'clk_c', 'clk_b' remains enabled?
> That's perfect example of incoherent state.
> How can 'clk_c' be enabled if its parent is disable. That makes no
> sense, so there is no point enabling a whole subtree for this IMO.
>
>> 2. 'clk_c' is not configured with CLK_IGNORE_UNUSED, it should be
>> disabled later. However, here it goes to a 'goto' statement and then
>> return 'false', ultimately resulting in 'clk_c' not being disabled?
> We've discussed that 2 times already. This discussion is going in
> circles now.
>
>>>>>>>                     goto unlock_out;
>>>>>>>
>>>>>>>             /*
>>>>>>> @@ -1516,8 +1545,7 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>>>>>>>
>>>>>>>      unlock_out:
>>>>>>>             clk_enable_unlock(flags);
>>>>>>> -       if (core->flags & CLK_OPS_PARENT_ENABLE)
>>>>>>> -               clk_core_disable_unprepare(core->parent);
>>>>>>> +       return (core->flags & CLK_IGNORE_UNUSED) && enabled;
>>>>>>>      }
>>>>>>>
>>>>>>>      static bool clk_ignore_unused __initdata;
>>>>>>> @@ -1550,16 +1578,16 @@ static int __init clk_disable_unused(void)
>>>>>>>             clk_prepare_lock();
>>>>>>>
>>>>>>>             hlist_for_each_entry(core, &clk_root_list, child_node)
>>>>>>> -               clk_disable_unused_subtree(core);
>>>>>>> +               clk_disable_unused_subtree(core, true);
>>>>>>>
>>>>>>>             hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>>>>> -               clk_disable_unused_subtree(core);
>>>>>>> +               clk_disable_unused_subtree(core, true);
>>>>>>>
>>>>>>>             hlist_for_each_entry(core, &clk_root_list, child_node)
>>>>>>> -               clk_unprepare_unused_subtree(core);
>>>>>>> +               clk_unprepare_unused_subtree(core, true);
>>>>>>>
>>>>>>>             hlist_for_each_entry(core, &clk_orphan_list, child_node)
>>>>>>> -               clk_unprepare_unused_subtree(core);
>>>>>>> +               clk_unprepare_unused_subtree(core, true);
>>>>>>>
>>>>>>>             clk_prepare_unlock();
>>>>>>>
>>>>>>> --
>>>>>>> 2.45.2
>>>>>>>
>>>>> --
>>>>> Jerome
>>> --
>>> Jerome
> --
> Jerome

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Chuan Liu]

Hi Jerome & Martin:

Sorry for the imprecise description of the glitch-free mux earlier.

Recently, while troubleshooting a CPU hang issue caused by glitches,
I realized there was a discrepancy from our previous understanding,
so I'd like to clarify it here.

On 10/8/2024 2:02 PM, Jerome Brunet wrote:

> [ EXTERNAL EMAIL ]
>
> On Tue 08 Oct 2024 at 13:44, Chuan Liu <chuan.liu@amlogic.com> wrote:
>
>> Hi Martin,
>>
>>
>> On 2024/10/1 4:08, Martin Blumenstingl wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Hello,
>>>
>>> On Sun, Sep 29, 2024 at 8:10 AM Chuan Liu via B4 Relay
>>> <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>>>> From: Chuan Liu <chuan.liu@amlogic.com>
>>>>
>>>> glitch free mux has two clock channels (channel 0 and channel 1) with
>>>> the same configuration. When the frequency needs to be changed, the two
>>>> channels ping-pong to ensure clock continuity and suppress glitch.
>>> You describe the solution to this below:
>>>> Add flag CLK_SET_RATE_GATE to channels 0 and 1 to implement Ping-Pong
>>>> switchover to suppress glitch.
>>> It would be great to have this change in a separate patch.
>>> The clocks to which you're adding CLK_SET_RATE_GATE aren't switched at
>>> runtime in mainline kernels (at least I think so).
>>
>> Okay, I will separate it into two patches and submit it in the next version.
>>
>>
>>>> Channel 0 of glitch free mux is not only the clock source for the mux,
>>>> but also the working clock for glitch free mux. Therefore, when glitch
>>>> free mux switches, it is necessary to ensure that channel 0 has a clock
>>>> input, otherwise glitch free mux will not work and cannot switch to the
>>>> target channel.
>>> [...]
>>>> glitch free mux Add flag CLK_OPS_PARENT_ENABLE to ensure that channel 0
>>>> has clock input when switching channels.
>>> This describes a second problem. I think it's best to have this in a
>>> separate commit/patch.
>>> Also you're updating some mali clocks (e.g. on G12 and GX) but not all
>>> of them (Meson8b for example is missing).
>>
>> Yes, M8 missed it, I will complete it in the next version.
>>
>>
>>> I still have some questions to the CLK_OPS_PARENT_ENABLE approach -
>>> please share your findings on this.
>>>
>>> There's multiple clocks involved in a glitch-free mux hierarchy:
>>> - a number of clock inputs (e.g. fclk, xtal, ...)
>>> - two muxes (one for every channel of the glitch-free mux)
>>> - two dividers (one for every channel of the glitch-free mux)
>>> - two gates (one for every channel of the glitch-free mux)
>>> - the glitch-free mux
>>>
>>> When switching from channel 0 (which is active and enabled) CCF
>>> (common clock framework) will:
>>> a) on channel 1:
>>> - find the best input clock
>>> - choose the best input clock in the mux
>>> - set the divider
>>> - enable the gate
>>> b) switch the glitch-free mux
>>> c) on channel 2:
>>> - disable the gate
>>>
>>> To me it's not clear at which level the problem occurs (glitch-free
>>> mux, gate, divider, mux, input clock).
>>> Also I don't understand why enabling the clocks with
>>> CLK_OPS_PARENT_ENABLE solves any problem since CCF is doing things
>>> automatically for us.
>>> Can you please explain (preferably with an example) what problem is
>>> solved with this approach?
>>
>> If CLK_OPS_PARENT_ENABLE is configured in mux, 'new_parent' and
>> 'old_parent' will be enabled first when __clk_set_parent_before() is
>> called. And disable them at __clk_set_parent_after(). Our glitch free
>> only has two clock sources, so adding this flag ensures that both
>> channels 0 and 1 are enabled when mux switches.
>>
>> In fact, we just need to make sure that channel 0 is enabled. The

It is indeed necessary to enable the glitch-free mux on *both* channel 0 
and channel 1 to allow proper switching. multiple original channel clock 
cycles and new channel clock cycles will be filtered during mux 
switching. An example of the clock waveform is shown below: __ __ __ __ 
__ __ __ __ ori: ↑ |__↑ |__↑ |__↑ |__↑ |__↑ |__↑ |__↑ |__↑ ^ 1 * cycle 
original channel. _ _ _ _ _ _ _ _ _ _ _ _ new: ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ 
|_↑ |_↑ |_↑ |_↑ |_↑ |_↑ ^ 5 * cycles new channel. __ __ _ _ _ _ out: ↑ 
|__↑ |______________________↑ |_↑ |_↑ |_↑ |_↑ ^ ^ start switching mux. 
switch to new channel.

>> purpose of CLK_OPS_PARENT_ENABLE may not be to solve our situation,
>> but adding this flag does solve our current problem.
> This is last point is important.
>
> It is OK to use a side effect of CLK_OPS_PARENT_ENABLE but it needs to
> be documented somehow, so what really matters is still known 2y from now.
>
> Make sure the information appears in the code comments at least once.
>
>>
>>> Last but not least: if we're running into bugs when
>>> CLK_OPS_PARENT_ENABLE is missing then that patch should carry a Fixes
>>> tag.
>>
>> Thanks for the heads-up. I'll keep an eye on it in the next version.
>>
>>
>>> Best regards,
>>> Martin
> --
> Jerome

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Chuan Liu]

On 9/28/2025 2:05 PM, Chuan Liu wrote:
> Hi Jerome & Martin:
>
> Sorry for the imprecise description of the glitch-free mux earlier.
>
> Recently, while troubleshooting a CPU hang issue caused by glitches,
> I realized there was a discrepancy from our previous understanding,
> so I'd like to clarify it here.
>
> On 10/8/2024 2:02 PM, Jerome Brunet wrote:
>
>> [ EXTERNAL EMAIL ]
>>
>> On Tue 08 Oct 2024 at 13:44, Chuan Liu <chuan.liu@amlogic.com> wrote:
>>
>>> Hi Martin,
>>>
>>>
>>> On 2024/10/1 4:08, Martin Blumenstingl wrote:
>>>> [ EXTERNAL EMAIL ]
>>>>
>>>> Hello,
>>>>
>>>> On Sun, Sep 29, 2024 at 8:10 AM Chuan Liu via B4 Relay
>>>> <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>>>>> From: Chuan Liu <chuan.liu@amlogic.com>
>>>>>
>>>>> glitch free mux has two clock channels (channel 0 and channel 1) with
>>>>> the same configuration. When the frequency needs to be changed, 
>>>>> the two
>>>>> channels ping-pong to ensure clock continuity and suppress glitch.
>>>> You describe the solution to this below:
>>>>> Add flag CLK_SET_RATE_GATE to channels 0 and 1 to implement Ping-Pong
>>>>> switchover to suppress glitch.
>>>> It would be great to have this change in a separate patch.
>>>> The clocks to which you're adding CLK_SET_RATE_GATE aren't switched at
>>>> runtime in mainline kernels (at least I think so).
>>>
>>> Okay, I will separate it into two patches and submit it in the next 
>>> version.
>>>
>>>
>>>>> Channel 0 of glitch free mux is not only the clock source for the 
>>>>> mux,
>>>>> but also the working clock for glitch free mux. Therefore, when 
>>>>> glitch
>>>>> free mux switches, it is necessary to ensure that channel 0 has a 
>>>>> clock
>>>>> input, otherwise glitch free mux will not work and cannot switch 
>>>>> to the
>>>>> target channel.
>>>> [...]
>>>>> glitch free mux Add flag CLK_OPS_PARENT_ENABLE to ensure that 
>>>>> channel 0
>>>>> has clock input when switching channels.
>>>> This describes a second problem. I think it's best to have this in a
>>>> separate commit/patch.
>>>> Also you're updating some mali clocks (e.g. on G12 and GX) but not all
>>>> of them (Meson8b for example is missing).
>>>
>>> Yes, M8 missed it, I will complete it in the next version.
>>>
>>>
>>>> I still have some questions to the CLK_OPS_PARENT_ENABLE approach -
>>>> please share your findings on this.
>>>>
>>>> There's multiple clocks involved in a glitch-free mux hierarchy:
>>>> - a number of clock inputs (e.g. fclk, xtal, ...)
>>>> - two muxes (one for every channel of the glitch-free mux)
>>>> - two dividers (one for every channel of the glitch-free mux)
>>>> - two gates (one for every channel of the glitch-free mux)
>>>> - the glitch-free mux
>>>>
>>>> When switching from channel 0 (which is active and enabled) CCF
>>>> (common clock framework) will:
>>>> a) on channel 1:
>>>> - find the best input clock
>>>> - choose the best input clock in the mux
>>>> - set the divider
>>>> - enable the gate
>>>> b) switch the glitch-free mux
>>>> c) on channel 2:
>>>> - disable the gate
>>>>
>>>> To me it's not clear at which level the problem occurs (glitch-free
>>>> mux, gate, divider, mux, input clock).
>>>> Also I don't understand why enabling the clocks with
>>>> CLK_OPS_PARENT_ENABLE solves any problem since CCF is doing things
>>>> automatically for us.
>>>> Can you please explain (preferably with an example) what problem is
>>>> solved with this approach?
>>>
>>> If CLK_OPS_PARENT_ENABLE is configured in mux, 'new_parent' and
>>> 'old_parent' will be enabled first when __clk_set_parent_before() is
>>> called. And disable them at __clk_set_parent_after(). Our glitch free
>>> only has two clock sources, so adding this flag ensures that both
>>> channels 0 and 1 are enabled when mux switches.
>>>
>>> In fact, we just need to make sure that channel 0 is enabled. The
>
> It is indeed necessary to enable the glitch-free mux on *both* channel 
> 0 and channel 1 to allow proper switching. multiple original channel 
> clock cycles and new channel clock cycles will be filtered during mux 
> switching. An example of the clock waveform is shown below: __ __ __ 
> __ __ __ __ __ ori: ↑ |__↑ |__↑ |__↑ |__↑ |__↑ |__↑ |__↑ |__↑ ^ 1 * 
> cycle original channel. _ _ _ _ _ _ _ _ _ _ _ _ new: ↑ |_↑ |_↑ |_↑ |_↑ 
> |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ ^ 5 * cycles new channel. __ __ _ _ _ 
> _ out: ↑ |__↑ |______________________↑ |_↑ |_↑ |_↑ |_↑ ^ ^ start 
> switching mux. switch to new channel.
>

Sorry, there seems to be something wrong with the following format parsing.


An example of the clock waveform is shown below:


        __    __    __    __    __    __    __    __
ori:  ↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑
                   ^
                   1 * cycle original channel.
        _   _   _   _   _   _   _   _   _   _   _   _
new:  ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑
                                       ^
                                       5 * cycles new channel.
        __    __                        _   _   _   _
out:  ↑  |__↑  |______________________↑ |_↑ |_↑ |_↑ |_↑
              ^                        ^
              start switching mux.     switch to new channel.


>>> purpose of CLK_OPS_PARENT_ENABLE may not be to solve our situation,
>>> but adding this flag does solve our current problem.
>> This is last point is important.
>>
>> It is OK to use a side effect of CLK_OPS_PARENT_ENABLE but it needs to
>> be documented somehow, so what really matters is still known 2y from 
>> now.
>>
>> Make sure the information appears in the code comments at least once.
>>
>>>
>>>> Last but not least: if we're running into bugs when
>>>> CLK_OPS_PARENT_ENABLE is missing then that patch should carry a Fixes
>>>> tag.
>>>
>>> Thanks for the heads-up. I'll keep an eye on it in the next version.
>>>
>>>
>>>> Best regards,
>>>> Martin
>> -- 
>> Jerome

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Martin Blumenstingl]

Hello,

On Sun, Sep 28, 2025 at 8:41 AM Chuan Liu <chuan.liu@amlogic.com> wrote:
>
>
> On 9/28/2025 2:05 PM, Chuan Liu wrote:
> > Hi Jerome & Martin:
> >
> > Sorry for the imprecise description of the glitch-free mux earlier.
> >
> > Recently, while troubleshooting a CPU hang issue caused by glitches,
> > I realized there was a discrepancy from our previous understanding,
> > so I'd like to clarify it here.
[...]
> An example of the clock waveform is shown below:
>
>
>         __    __    __    __    __    __    __    __
> ori:  ↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑
>                    ^
>                    1 * cycle original channel.
>         _   _   _   _   _   _   _   _   _   _   _   _
> new:  ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑
>                                        ^
>                                        5 * cycles new channel.
>         __    __                        _   _   _   _
> out:  ↑  |__↑  |______________________↑ |_↑ |_↑ |_↑ |_↑
>               ^                        ^
>               start switching mux.     switch to new channel.
Thank you for the detailed report!
This is indeed problematic behavior. I guess the result is somewhat
random: depending on load (power draw), silicon lottery (quality),
temperature, voltage supply, ... - one may or may not see crashes
caused by this.

Based on the previous discussion on this topic, my suggestion is to
split the original patch:
- one to add CLK_SET_RATE_GATE where needed (I think the meson8b.c
driver already has this where needed) to actually enable the
glitch-free mux behavior
- another one with the CLK_OPS_PARENT_ENABLE change (meson8b.c would
also need to be updated) to prevent the glitch-free mux from
temporarily outputting an electrical low signal. Jerome also asked to
document the behavior so we don't forget why we set this flag

Both patches should get the proper "Fixes" tags.
I think it would also be great if you could include the waveform
example in at least the commit message as it helps understand the
problem.

Let's also give Jerome some time to comment before you send patches.


Best regards,
Martin

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Chuan Liu]

Hi Martin:

Thanks for the detailed explanation.


On 9/29/2025 4:55 AM, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
>
> Hello,
>
> On Sun, Sep 28, 2025 at 8:41 AM Chuan Liu <chuan.liu@amlogic.com> wrote:
>>
>> On 9/28/2025 2:05 PM, Chuan Liu wrote:
>>> Hi Jerome & Martin:
>>>
>>> Sorry for the imprecise description of the glitch-free mux earlier.
>>>
>>> Recently, while troubleshooting a CPU hang issue caused by glitches,
>>> I realized there was a discrepancy from our previous understanding,
>>> so I'd like to clarify it here.
> [...]
>> An example of the clock waveform is shown below:
>>
>>
>>          __    __    __    __    __    __    __    __
>> ori:  ↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑
>>                     ^
>>                     1 * cycle original channel.
>>          _   _   _   _   _   _   _   _   _   _   _   _
>> new:  ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑
>>                                         ^
>>                                         5 * cycles new channel.
>>          __    __                        _   _   _   _
>> out:  ↑  |__↑  |______________________↑ |_↑ |_↑ |_↑ |_↑
>>                ^                        ^
>>                start switching mux.     switch to new channel.
> Thank you for the detailed report!
> This is indeed problematic behavior. I guess the result is somewhat
> random: depending on load (power draw), silicon lottery (quality),
> temperature, voltage supply, ... - one may or may not see crashes
> caused by this.


Yes, our glitch-free mux is designed to prevent glitches caused by
excessively short high or low levels in the clock output.


>
> Based on the previous discussion on this topic, my suggestion is to
> split the original patch:
> - one to add CLK_SET_RATE_GATE where needed (I think the meson8b.c
> driver already has this where needed) to actually enable the
> glitch-free mux behavior
> - another one with the CLK_OPS_PARENT_ENABLE change (meson8b.c would
> also need to be updated) to prevent the glitch-free mux from
> temporarily outputting an electrical low signal. Jerome also asked to
> document the behavior so we don't forget why we set this flag
>
> Both patches should get the proper "Fixes" tags.
> I think it would also be great if you could include the waveform
> example in at least the commit message as it helps understand the
> problem.
>
> Let's also give Jerome some time to comment before you send patches.


A V2 version was submitted later with changes based on your suggestions.
Regarding the "Fixes" tag, Jerome had proposed some modifications.

[PATCH v2 0/3] clk: Fix issues related to CLK_IGNORE_UNUSED failures and 
amlogic glitch free mux - Chuan Liu via B4 Relay 
<https://lore.kernel.org/all/20241111-fix_glitch_free-v2-0-0099fd9ad3e5@amlogic.com/>


Adding CLK_OPS_PARENT_ENABLE causes the CLK_IGNORE_UNUSED configuration
of it's parent clocks on the chain to become ineffective, so this patch
depends on fixing that issue before it can proceed.


Jerome and I have submitted patches to address the issue of
CLK_IGNORE_UNUSED becoming ineffective. I originally planned to wait
for progress on this patch and then incorporate Jerome's feedback before
sending the V3 version.

Hi Jerome, sorry if this caused any misunderstanding on your part; I
will provide timely feedback moving forward.


>
>
> Best regards,
> Martin

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Jerome Brunet]

On Sun 28 Sep 2025 at 22:55, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hello,
>
> On Sun, Sep 28, 2025 at 8:41 AM Chuan Liu <chuan.liu@amlogic.com> wrote:
>>
>>
>> On 9/28/2025 2:05 PM, Chuan Liu wrote:
>> > Hi Jerome & Martin:
>> >
>> > Sorry for the imprecise description of the glitch-free mux earlier.
>> >
>> > Recently, while troubleshooting a CPU hang issue caused by glitches,
>> > I realized there was a discrepancy from our previous understanding,
>> > so I'd like to clarify it here.
> [...]
>> An example of the clock waveform is shown below:
>>
>>
                     1                  2
                     v                  v
>>         __    __    __    __    __    __    __    __
>> ori:  ↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑
>>                    ^
>>                    1 * cycle original channel.
>>         _   _   _   _   _   _   _   _   _   _   _   _
>> new:  ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑
>>                                        ^
>>                                        5 * cycles new channel.
>>         __    __                        _   _   _   _
>> out:  ↑  |__↑  |______________________↑ |_↑ |_↑ |_↑ |_↑
>>               ^                        ^
>>               start switching mux.     switch to new channel.

Ok ... but when is it safe to disable the "ori" clock ?
Can you do it at '1' already ? or do you have to wait for '2' ?


> Thank you for the detailed report!
> This is indeed problematic behavior. I guess the result is somewhat
> random: depending on load (power draw), silicon lottery (quality),
> temperature, voltage supply, ... - one may or may not see crashes
> caused by this.
>
> Based on the previous discussion on this topic, my suggestion is to
> split the original patch:
> - one to add CLK_SET_RATE_GATE where needed (I think the meson8b.c
> driver already has this where needed) to actually enable the
> glitch-free mux behavior
> - another one with the CLK_OPS_PARENT_ENABLE change (meson8b.c would
> also need to be updated) to prevent the glitch-free mux from
> temporarily outputting an electrical low signal. Jerome also asked to
> document the behavior so we don't forget why we set this flag

Yes please split the changes and visit all the controllers shipping this
type of muxes.

>
> Both patches should get the proper "Fixes" tags.

... and proper fixes tag maybe different depending on the controller so
there might more that just 2 changes.

> I think it would also be great if you could include the waveform
> example in at least the commit message as it helps understand the
> problem.
>
> Let's also give Jerome some time to comment before you send patches.
>
>
> Best regards,
> Martin

-- 
Jerome

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Chuan Liu]

On 9/29/2025 4:48 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Sun 28 Sep 2025 at 22:55, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>
>> Hello,
>>
>> On Sun, Sep 28, 2025 at 8:41 AM Chuan Liu <chuan.liu@amlogic.com> wrote:
>>>
>>> On 9/28/2025 2:05 PM, Chuan Liu wrote:
>>>> Hi Jerome & Martin:
>>>>
>>>> Sorry for the imprecise description of the glitch-free mux earlier.
>>>>
>>>> Recently, while troubleshooting a CPU hang issue caused by glitches,
>>>> I realized there was a discrepancy from our previous understanding,
>>>> so I'd like to clarify it here.
>> [...]
>>> An example of the clock waveform is shown below:
>>>
>>>
>                       1                  2
>                       v                  v
>>>          __    __    __    __    __    __    __    __
>>> ori:  ↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑
>>>                     ^
>>>                     1 * cycle original channel.
>>>          _   _   _   _   _   _   _   _   _   _   _   _
>>> new:  ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑
>>>                                         ^
>>>                                         5 * cycles new channel.
>>>          __    __                        _   _   _   _
>>> out:  ↑  |__↑  |______________________↑ |_↑ |_↑ |_↑ |_↑
>>>                ^                        ^
>>>                start switching mux.     switch to new channel.
> Ok ... but when is it safe to disable the "ori" clock ?
> Can you do it at '1' already ? or do you have to wait for '2' ?


It should wait for "2", because there is a state machine in the
glitch-free mux, this state machine is driven by the working clock
provided by its channel 0.


>
>> Thank you for the detailed report!
>> This is indeed problematic behavior. I guess the result is somewhat
>> random: depending on load (power draw), silicon lottery (quality),
>> temperature, voltage supply, ... - one may or may not see crashes
>> caused by this.
>>
>> Based on the previous discussion on this topic, my suggestion is to
>> split the original patch:
>> - one to add CLK_SET_RATE_GATE where needed (I think the meson8b.c
>> driver already has this where needed) to actually enable the
>> glitch-free mux behavior
>> - another one with the CLK_OPS_PARENT_ENABLE change (meson8b.c would
>> also need to be updated) to prevent the glitch-free mux from
>> temporarily outputting an electrical low signal. Jerome also asked to
>> document the behavior so we don't forget why we set this flag
> Yes please split the changes and visit all the controllers shipping this
> type of muxes.
>
>> Both patches should get the proper "Fixes" tags.
> ... and proper fixes tag maybe different depending on the controller so
> there might more that just 2 changes.
>
>> I think it would also be great if you could include the waveform
>> example in at least the commit message as it helps understand the
>> problem.
>>
>> Let's also give Jerome some time to comment before you send patches.
>>
>>
>> Best regards,
>> Martin
> --
> Jerome

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Jerome Brunet]

On Mon 29 Sep 2025 at 11:15, Chuan Liu <chuan.liu@amlogic.com> wrote:

> Hi Martin:
>
> Thanks for the detailed explanation.
>
>
> On 9/29/2025 4:55 AM, Martin Blumenstingl wrote:
>> [ EXTERNAL EMAIL ]
>>
>> Hello,
>>
>> On Sun, Sep 28, 2025 at 8:41 AM Chuan Liu <chuan.liu@amlogic.com> wrote:
>>>
>>> On 9/28/2025 2:05 PM, Chuan Liu wrote:
>>>> Hi Jerome & Martin:
>>>>
>>>> Sorry for the imprecise description of the glitch-free mux earlier.
>>>>
>>>> Recently, while troubleshooting a CPU hang issue caused by glitches,
>>>> I realized there was a discrepancy from our previous understanding,
>>>> so I'd like to clarify it here.
>> [...]
>>> An example of the clock waveform is shown below:
>>>
>>>
>>>          __    __    __    __    __    __    __    __
>>> ori:  ↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑
>>>                     ^
>>>                     1 * cycle original channel.
>>>          _   _   _   _   _   _   _   _   _   _   _   _
>>> new:  ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑
>>>                                         ^
>>>                                         5 * cycles new channel.
>>>          __    __                        _   _   _   _
>>> out:  ↑  |__↑  |______________________↑ |_↑ |_↑ |_↑ |_↑
>>>                ^                        ^
>>>                start switching mux.     switch to new channel.
>> Thank you for the detailed report!
>> This is indeed problematic behavior. I guess the result is somewhat
>> random: depending on load (power draw), silicon lottery (quality),
>> temperature, voltage supply, ... - one may or may not see crashes
>> caused by this.
>
>
> Yes, our glitch-free mux is designed to prevent glitches caused by
> excessively short high or low levels in the clock output.
>
>
>>
>> Based on the previous discussion on this topic, my suggestion is to
>> split the original patch:
>> - one to add CLK_SET_RATE_GATE where needed (I think the meson8b.c
>> driver already has this where needed) to actually enable the
>> glitch-free mux behavior
>> - another one with the CLK_OPS_PARENT_ENABLE change (meson8b.c would
>> also need to be updated) to prevent the glitch-free mux from
>> temporarily outputting an electrical low signal. Jerome also asked to
>> document the behavior so we don't forget why we set this flag
>>
>> Both patches should get the proper "Fixes" tags.
>> I think it would also be great if you could include the waveform
>> example in at least the commit message as it helps understand the
>> problem.
>>
>> Let's also give Jerome some time to comment before you send patches.
>
>
> A V2 version was submitted later with changes based on your suggestions.
> Regarding the "Fixes" tag, Jerome had proposed some modifications.
>
> [PATCH v2 0/3] clk: Fix issues related to CLK_IGNORE_UNUSED failures and
> amlogic glitch free mux - Chuan Liu via B4 Relay
> <https://lore.kernel.org/all/20241111-fix_glitch_free-v2-0-0099fd9ad3e5@amlogic.com/>
>

The comments I've provided on this still stands.

>
> Adding CLK_OPS_PARENT_ENABLE causes the CLK_IGNORE_UNUSED configuration
> of it's parent clocks on the chain to become ineffective, so this patch
> depends on fixing that issue before it can proceed.

Unused clocks are NOT a configuration.

They are by-product of the bootloader. You cannot rely on them. If
anything depends on them, you have a(nother) problem to solve. 

>
>
> Jerome and I have submitted patches to address the issue of
> CLK_IGNORE_UNUSED becoming ineffective. I originally planned to wait
> for progress on this patch and then incorporate Jerome's feedback before
> sending the V3 version.

I've provided a suggestion but this something happening in clock core.
I suggest that you split this out of your series so things that need to
go through Stephen are not mixed with Amlogic stuff.

But again, you cannot rely on the state of clock just because it has
CLK_IGNORE_UNUSED:

* Nothing says it is enabled to begin with
* Nothing says it will stay on if a consumer comes and goes
* ... and yes, it does not survive CCF usage checking down the road.

It is unreliable and it is not meant to be more than that, AFAIK

>
> Hi Jerome, sorry if this caused any misunderstanding on your part; I
> will provide timely feedback moving forward.
>
>
>>
>>
>> Best regards,
>> Martin

-- 
Jerome

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Jerome Brunet]

On Mon 29 Sep 2025 at 17:31, Chuan Liu <chuan.liu@amlogic.com> wrote:

> On 9/29/2025 4:48 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On Sun 28 Sep 2025 at 22:55, Martin Blumenstingl
>> <martin.blumenstingl@googlemail.com> wrote:
>>
>>> Hello,
>>>
>>> On Sun, Sep 28, 2025 at 8:41 AM Chuan Liu <chuan.liu@amlogic.com> wrote:
>>>>
>>>> On 9/28/2025 2:05 PM, Chuan Liu wrote:
>>>>> Hi Jerome & Martin:
>>>>>
>>>>> Sorry for the imprecise description of the glitch-free mux earlier.
>>>>>
>>>>> Recently, while troubleshooting a CPU hang issue caused by glitches,
>>>>> I realized there was a discrepancy from our previous understanding,
>>>>> so I'd like to clarify it here.
>>> [...]
>>>> An example of the clock waveform is shown below:
>>>>
>>>>
>>                       1                  2
>>                       v                  v
>>>>          __    __    __    __    __    __    __    __
>>>> ori:  ↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑
>>>>                     ^
>>>>                     1 * cycle original channel.
>>>>          _   _   _   _   _   _   _   _   _   _   _   _
>>>> new:  ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑
>>>>                                         ^
>>>>                                         5 * cycles new channel.
>>>>          __    __                        _   _   _   _
>>>> out:  ↑  |__↑  |______________________↑ |_↑ |_↑ |_↑ |_↑
>>>>                ^                        ^
>>>>                start switching mux.     switch to new channel.
>> Ok ... but when is it safe to disable the "ori" clock ?
>> Can you do it at '1' already ? or do you have to wait for '2' ?
>
>
> It should wait for "2", because there is a state machine in the
> glitch-free mux, this state machine is driven by the working clock
> provided by its channel 0.

Then I don't think the 2 flags are enough to make it safe

Nothing guarantees that CCF will wait for those 5 cycles to turn off
the clock noted 'ori' above.

I think you need new specific ops for this mux

Something that would
* protect both parents before changing the mux
* do the actual change
* wait for it to settle
* remove the protection

>
>
>>
>>> Thank you for the detailed report!
>>> This is indeed problematic behavior. I guess the result is somewhat
>>> random: depending on load (power draw), silicon lottery (quality),
>>> temperature, voltage supply, ... - one may or may not see crashes
>>> caused by this.
>>>
>>> Based on the previous discussion on this topic, my suggestion is to
>>> split the original patch:
>>> - one to add CLK_SET_RATE_GATE where needed (I think the meson8b.c
>>> driver already has this where needed) to actually enable the
>>> glitch-free mux behavior
>>> - another one with the CLK_OPS_PARENT_ENABLE change (meson8b.c would
>>> also need to be updated) to prevent the glitch-free mux from
>>> temporarily outputting an electrical low signal. Jerome also asked to
>>> document the behavior so we don't forget why we set this flag
>> Yes please split the changes and visit all the controllers shipping this
>> type of muxes.
>>
>>> Both patches should get the proper "Fixes" tags.
>> ... and proper fixes tag maybe different depending on the controller so
>> there might more that just 2 changes.
>>
>>> I think it would also be great if you could include the waveform
>>> example in at least the commit message as it helps understand the
>>> problem.
>>>
>>> Let's also give Jerome some time to comment before you send patches.
>>>
>>>
>>> Best regards,
>>> Martin
>> --
>> Jerome

-- 
Jerome

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Chuan Liu]

Hi Jerome,


On 9/29/2025 8:55 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Mon 29 Sep 2025 at 17:31, Chuan Liu <chuan.liu@amlogic.com> wrote:
>
>> On 9/29/2025 4:48 PM, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Sun 28 Sep 2025 at 22:55, Martin Blumenstingl
>>> <martin.blumenstingl@googlemail.com> wrote:
>>>
>>>> Hello,
>>>>
>>>> On Sun, Sep 28, 2025 at 8:41 AM Chuan Liu <chuan.liu@amlogic.com> wrote:
>>>>> On 9/28/2025 2:05 PM, Chuan Liu wrote:
>>>>>> Hi Jerome & Martin:
>>>>>>
>>>>>> Sorry for the imprecise description of the glitch-free mux earlier.
>>>>>>
>>>>>> Recently, while troubleshooting a CPU hang issue caused by glitches,
>>>>>> I realized there was a discrepancy from our previous understanding,
>>>>>> so I'd like to clarify it here.
>>>> [...]
>>>>> An example of the clock waveform is shown below:
>>>>>
>>>>>
>>>                        1                  2
>>>                        v                  v
>>>>>           __    __    __    __    __    __    __    __
>>>>> ori:  ↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑
>>>>>                      ^
>>>>>                      1 * cycle original channel.
>>>>>           _   _   _   _   _   _   _   _   _   _   _   _
>>>>> new:  ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑
>>>>>                                          ^
>>>>>                                          5 * cycles new channel.
>>>>>           __    __                        _   _   _   _
>>>>> out:  ↑  |__↑  |______________________↑ |_↑ |_↑ |_↑ |_↑
>>>>>                 ^                        ^
>>>>>                 start switching mux.     switch to new channel.
>>> Ok ... but when is it safe to disable the "ori" clock ?
>>> Can you do it at '1' already ? or do you have to wait for '2' ?
>>
>> It should wait for "2", because there is a state machine in the
>> glitch-free mux, this state machine is driven by the working clock
>> provided by its channel 0.
> Then I don't think the 2 flags are enough to make it safe
>
> Nothing guarantees that CCF will wait for those 5 cycles to turn off
> the clock noted 'ori' above.
>
> I think you need new specific ops for this mux
>
> Something that would
> * protect both parents before changing the mux
> * do the actual change
> * wait for it to settle
> * remove the protection


Got it, thanks for your suggestion. I will try to address it in this way
moving forward, but it may take some time, as I'm currently working on
the bring-up of a new SoC.

By the way, On the new SoC, we have standardized the clock tree and PLL
design at the chip level. As a result, future drivers won't need to
include a large amount of redundant register bit definitions. This
should also help improve the generality, memory footprint, and
performance of our drivers.


>>
>>>> Thank you for the detailed report!
>>>> This is indeed problematic behavior. I guess the result is somewhat
>>>> random: depending on load (power draw), silicon lottery (quality),
>>>> temperature, voltage supply, ... - one may or may not see crashes
>>>> caused by this.
>>>>
>>>> Based on the previous discussion on this topic, my suggestion is to
>>>> split the original patch:
>>>> - one to add CLK_SET_RATE_GATE where needed (I think the meson8b.c
>>>> driver already has this where needed) to actually enable the
>>>> glitch-free mux behavior
>>>> - another one with the CLK_OPS_PARENT_ENABLE change (meson8b.c would
>>>> also need to be updated) to prevent the glitch-free mux from
>>>> temporarily outputting an electrical low signal. Jerome also asked to
>>>> document the behavior so we don't forget why we set this flag
>>> Yes please split the changes and visit all the controllers shipping this
>>> type of muxes.
>>>
>>>> Both patches should get the proper "Fixes" tags.
>>> ... and proper fixes tag maybe different depending on the controller so
>>> there might more that just 2 changes.
>>>
>>>> I think it would also be great if you could include the waveform
>>>> example in at least the commit message as it helps understand the
>>>> problem.
>>>>
>>>> Let's also give Jerome some time to comment before you send patches.
>>>>
>>>>
>>>> Best regards,
>>>> Martin
>>> --
>>> Jerome
> --
> Jerome

Re: [PATCH 2/2] clk: meson: Fix glitch free mux related issues

[Chuan Liu]

On 9/29/2025 8:36 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Mon 29 Sep 2025 at 11:15, Chuan Liu <chuan.liu@amlogic.com> wrote:
>
>> Hi Martin:
>>
>> Thanks for the detailed explanation.
>>
>>
>> On 9/29/2025 4:55 AM, Martin Blumenstingl wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Hello,
>>>
>>> On Sun, Sep 28, 2025 at 8:41 AM Chuan Liu <chuan.liu@amlogic.com> wrote:
>>>> On 9/28/2025 2:05 PM, Chuan Liu wrote:
>>>>> Hi Jerome & Martin:
>>>>>
>>>>> Sorry for the imprecise description of the glitch-free mux earlier.
>>>>>
>>>>> Recently, while troubleshooting a CPU hang issue caused by glitches,
>>>>> I realized there was a discrepancy from our previous understanding,
>>>>> so I'd like to clarify it here.
>>> [...]
>>>> An example of the clock waveform is shown below:
>>>>
>>>>
>>>>           __    __    __    __    __    __    __    __
>>>> ori:  ↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑  |__↑
>>>>                      ^
>>>>                      1 * cycle original channel.
>>>>           _   _   _   _   _   _   _   _   _   _   _   _
>>>> new:  ↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑ |_↑
>>>>                                          ^
>>>>                                          5 * cycles new channel.
>>>>           __    __                        _   _   _   _
>>>> out:  ↑  |__↑  |______________________↑ |_↑ |_↑ |_↑ |_↑
>>>>                 ^                        ^
>>>>                 start switching mux.     switch to new channel.
>>> Thank you for the detailed report!
>>> This is indeed problematic behavior. I guess the result is somewhat
>>> random: depending on load (power draw), silicon lottery (quality),
>>> temperature, voltage supply, ... - one may or may not see crashes
>>> caused by this.
>>
>> Yes, our glitch-free mux is designed to prevent glitches caused by
>> excessively short high or low levels in the clock output.
>>
>>
>>> Based on the previous discussion on this topic, my suggestion is to
>>> split the original patch:
>>> - one to add CLK_SET_RATE_GATE where needed (I think the meson8b.c
>>> driver already has this where needed) to actually enable the
>>> glitch-free mux behavior
>>> - another one with the CLK_OPS_PARENT_ENABLE change (meson8b.c would
>>> also need to be updated) to prevent the glitch-free mux from
>>> temporarily outputting an electrical low signal. Jerome also asked to
>>> document the behavior so we don't forget why we set this flag
>>>
>>> Both patches should get the proper "Fixes" tags.
>>> I think it would also be great if you could include the waveform
>>> example in at least the commit message as it helps understand the
>>> problem.
>>>
>>> Let's also give Jerome some time to comment before you send patches.
>>
>> A V2 version was submitted later with changes based on your suggestions.
>> Regarding the "Fixes" tag, Jerome had proposed some modifications.
>>
>> [PATCH v2 0/3] clk: Fix issues related to CLK_IGNORE_UNUSED failures and
>> amlogic glitch free mux - Chuan Liu via B4 Relay
>> <https://lore.kernel.org/all/20241111-fix_glitch_free-v2-0-0099fd9ad3e5@amlogic.com/>
>>
> The comments I've provided on this still stands.
>
>> Adding CLK_OPS_PARENT_ENABLE causes the CLK_IGNORE_UNUSED configuration
>> of it's parent clocks on the chain to become ineffective, so this patch
>> depends on fixing that issue before it can proceed.
> Unused clocks are NOT a configuration.
>
> They are by-product of the bootloader. You cannot rely on them. If
> anything depends on them, you have a(nother) problem to solve.
>
>>
>> Jerome and I have submitted patches to address the issue of
>> CLK_IGNORE_UNUSED becoming ineffective. I originally planned to wait
>> for progress on this patch and then incorporate Jerome's feedback before
>> sending the V3 version.
> I've provided a suggestion but this something happening in clock core.
> I suggest that you split this out of your series so things that need to
> go through Stephen are not mixed with Amlogic stuff.
>
> But again, you cannot rely on the state of clock just because it has
> CLK_IGNORE_UNUSED:
>
> * Nothing says it is enabled to begin with
> * Nothing says it will stay on if a consumer comes and goes
> * ... and yes, it does not survive CCF usage checking down the road.
>
> It is unreliable and it is not meant to be more than that, AFAIK


ok, I see what you mean, I will try to do that later.


>> Hi Jerome, sorry if this caused any misunderstanding on your part; I
>> will provide timely feedback moving forward.
>>
>>
>>>
>>> Best regards,
>>> Martin
> --
> Jerome