Re: [PATCH v3 07/12] clk: samsung: gs101: add support for cmu_peric0

On Wed, Jan 17, 2024 at 8:49 AM Tudor Ambarus [off-list ref] wrote:

Hi, Sam,

Thanks for reviewing the series!

On 1/16/24 17:42, Sam Protsenko wrote:

cut

quoted

quoted

Few clocks are marked as critical because when either of them is
disabled, the system hangs even if their clock parents are enabled.

Reviewed-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---

cut

quoted

quoted

diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
index 782993951fff..f3f0f5feb28d 100644
--- a/drivers/clk/samsung/clk-gs101.c
+++ b/drivers/clk/samsung/clk-gs101.c

cut

quoted

quoted

+static const struct samsung_gate_clock peric0_gate_clks[] __initconst = {
+       /* Disabling this clock makes the system hang. Mark the clock as critical. */
+       GATE(CLK_GOUT_PERIC0_PERIC0_CMU_PERIC0_PCLK,
+            "gout_peric0_peric0_cmu_peric0_pclk", "mout_peric0_bus_user",
+            CLK_CON_GAT_CLK_BLK_PERIC0_UID_PERIC0_CMU_PERIC0_IPCLKPORT_PCLK,
+            21, CLK_IS_CRITICAL, 0),

Why not just CLK_IGNORE_UNUSED? As I understand this gate clock can be

When either of the clocks that I marked as critical is disabled, the
system hangs, even if their clock parent is enabled. I tested this by
enabling the clock debugfs with write permissions. I prepared-enabled
the parent clock to increase their user count so that when the child
gets disabled to not disable the parent as well. When disabling the
child the system hung, even if its parent was enabled. Thus I considered
that the child is critical. I mentioned this in the commit message as
well. Please tell if get this wrong.

Ok, if you already tested this particular clock with CLK_IGNORE_UNUSED
and it didn't help:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

quoted

used to disable PCLK (bus clock) provided to the whole CMU_PERIC0.
Aren't there any valid cases for disabling this clock, like during
some PM transitions? For Exynos850 clock driver I marked all clocks of

They aren't, because if one switches off any of these clocks that are
marked as critical, the system hangs and it will not be able to resume.

quoted

this kind as CLK_IGNORE_UNUSED and it works fine. In other words: I'd
say CLK_IS_CRITICAL flag is more "strong" than CLK_IGNORE_UNUSED, and
requires better and more specific explanation, to make sure we are not
abusing it. And I'm not sure this is the case.

Is the explanation from the commit message enough?

quoted

The same goes for the rest of clocks marked as CLK_IS_CRITICAL in this
patch. Please check if maybe using CLK_IGNORE_UNUSED makes sense for
any of those as well.

I've already checked and all behave as described above.

Thanks,
ta