[PATCH v6 00/20] clk: help OF platforms with their stdout (earlycon) clocks during early boot

[PATCH v6 00/20] clk: help OF platforms with their stdout (earlycon) clocks during early boot

[André Draszik]

Hi,

On at least two plaforms, i.MX and the Exynos-derivative gs101,
earlycon depends on the bootloader setup stdout clocks being retained.
In some cases stdout UART clocks (or their parents) can get disabled
during loading of other drivers (e.g. i2c or UART driver init) causing
earlycon to stop to work sometime into the boot, halting the whole
system, see e.g. [3].

Code exists in the i.MX clock drivers to deal with that by temporarily
bumping the reference count of the relevant stdout clocks during early
boot.

While gs101 doesn't have such code, some UART clocks had been marked
'critical' for a different reason originally, and by accident
worked-around the same problem. v4 of this series proposed addition of
similar code to gs101 to be able to drop the 'critical' flag from its
clocks, but Stephen suggested to move all this into the clk core
instead.

This series now does that:
* instead of duplicating such code to gs101, teaches the clk core to
  deal with stdout clocks during early boot, similar to the existing
  support in i.MX
  This is hooked into of_clk_add_hw_provider() and
  of_clk_add_provider()
* updates gs101 to remove the 'critical' flag from affected clocks, as
  not necessary. This is essentially the last remaining patch [1] with
  all review comments addressed, from the series [2] that was sent
  earlier this year, see lore links below.
* updates i.MX to remove the now-unnecessary platform specific code in
  its clock drivers. I believe this also plugs a memory and extra clock
  reference leak at least on imx7ulp, see below.

Note 1: For the avoidance of doubt, any of the above is relevant only
        if earlycon and OF are enabled, behaviour is based on the
        'earlycon' kernel command line parameter.
        As this is meant to also replace i.MX specific code, the
        'earlyprintk' is also supported (since it was supported on
        i.MX)

Note 2: On i.MX, at least clk-imx7ulp.c calls
        imx_register_uart_clocks() twice (once for each compatible),
        but imx_register_uart_clocks() can not handle that and will
        leak memory and clock references in that case.
        The new clk core code handles multiple invocations without such
        issues.

Note 3: I am not in a position to test any of the i.MX changes and
        would appreciate feedback. In particular with these changes
        stdout clocks are enabled when of_clk_add_hw_provider() or
        of_clk_add_provider() return, but:
        * some i.MX platforms did some reparenting or frequency changes
          in the old approach before enabling stdout clocks. I believe
          they're all unrelated to stdout, though
        * some i.MX platforms enabled stdout clocks before the call to
          of_clk_add_hw_provider() or of_clk_add_provider(). Again, I
          don't think that difference is going to be relevant.

Signed-off-by: André Draszik <andre.draszik@linaro.org>

[1] https://lore.kernel.org/all/20240130093812.1746512-6-andre.draszik@linaro.org/
[2] https://lore.kernel.org/all/20240130093812.1746512-1-andre.draszik@linaro.org/
[3] https://lore.kernel.org/all/d45de3b2bb6b48653842cf1f74e58889ed6783ae.camel@linaro.org/

Changes in v6:
- drop a stray #include from drivers/clk/samsung/clk-gs101.c
- Link to v5: https://lore.kernel.org/r/20240808-gs101-non-essential-clocks-2-v5-0-11cffef0634e@linaro.org

Changes in v5:
- move stdout uart clock handling from gs101 into clk core (Stephen)
- update i.MX to drop now-unnecessary code
- update series' subject due to changed scope
- Link to v4: https://lore.kernel.org/r/20240712-gs101-non-essential-clocks-2-v4-0-310aee0de46e@linaro.org

Changes in v4:
- new patch "clk: samsung: gs101: allow earlycon to work unconditionally"
- update commit message for patch 2
- Link to v3: https://lore.kernel.org/r/20240710-gs101-non-essential-clocks-2-v3-0-5dcb8d040d1c@linaro.org

---
André Draszik (20):
      clk: bump stdout clock usage for earlycon
      clk: samsung: gs101: don't mark non-essential (UART) clocks critical
      clk: imx: imx25: drop call to imx_register_uart_clocks()
      clk: imx: imx27: drop call to imx_register_uart_clocks()
      clk: imx: imx35: drop call to imx_register_uart_clocks()
      clk: imx: imx5: drop call to imx_register_uart_clocks()
      clk: imx: imx6q: drop call to imx_register_uart_clocks()
      clk: imx: imx6sl: drop call to imx_register_uart_clocks()
      clk: imx: imx6sll: drop call to imx_register_uart_clocks()
      clk: imx: imx6sx: drop call to imx_register_uart_clocks()
      clk: imx: imx6ul: drop call to imx_register_uart_clocks()
      clk: imx: imx7d: drop call to imx_register_uart_clocks()
      clk: imx: imx7ulp: drop calls to imx_register_uart_clocks()
      clk: imx: imx8mm: drop call to imx_register_uart_clocks()
      clk: imx: imx8mn: drop call to imx_register_uart_clocks()
      clk: imx: imx8mp: drop call to imx_register_uart_clocks()
      clk: imx: imx8mq: drop call to imx_register_uart_clocks()
      clk: imx: imx8ulp: drop call to imx_register_uart_clocks()
      clk: imx: imx93: drop call to imx_register_uart_clocks()
      clk: imx: drop imx_register_uart_clocks()

 drivers/clk/clk.c               | 129 ++++++++++++++++++++++++++++++++++++++++
 drivers/clk/imx/clk-imx25.c     |   2 -
 drivers/clk/imx/clk-imx27.c     |   2 -
 drivers/clk/imx/clk-imx35.c     |   2 -
 drivers/clk/imx/clk-imx5.c      |   6 --
 drivers/clk/imx/clk-imx6q.c     |   2 -
 drivers/clk/imx/clk-imx6sl.c    |   2 -
 drivers/clk/imx/clk-imx6sll.c   |   2 -
 drivers/clk/imx/clk-imx6sx.c    |   2 -
 drivers/clk/imx/clk-imx6ul.c    |   2 -
 drivers/clk/imx/clk-imx7d.c     |   2 -
 drivers/clk/imx/clk-imx7ulp.c   |   4 --
 drivers/clk/imx/clk-imx8mm.c    |   2 -
 drivers/clk/imx/clk-imx8mn.c    |   2 -
 drivers/clk/imx/clk-imx8mp.c    |   2 -
 drivers/clk/imx/clk-imx8mq.c    |   2 -
 drivers/clk/imx/clk-imx8ulp.c   |   2 -
 drivers/clk/imx/clk-imx93.c     |   2 -
 drivers/clk/imx/clk.c           |  72 ----------------------
 drivers/clk/imx/clk.h           |   7 ---
 drivers/clk/samsung/clk-gs101.c |   6 +-
 21 files changed, 131 insertions(+), 123 deletions(-)
---
base-commit: 1e391b34f6aa043c7afa40a2103163a0ef06d179
change-id: 20240430-gs101-non-essential-clocks-2-6a3280fa1be8

Best regards,
-- 
André Draszik <andre.draszik@linaro.org>

[PATCH v6 01/20] clk: bump stdout clock usage for earlycon

[André Draszik]

On some platforms, earlycon depends on the bootloader setup stdout
clocks being retained. In some cases stdout UART clocks (or their
parents) can get disabled during loading of other drivers (e.g. i2c)
causing earlycon to stop to work sometime into the boot, halting the
whole system.

Since there are at least two platforms where that is the case, i.MX and
the Exynos-derivative gs101, this patch adds some logic to the clk core
to detect these clocks if earlycon is enabled, to bump their usage
count as part of of_clk_add_hw_provider() and of_clk_add_provider(),
and to release them again at the end of init.

This way code duplication in affected platforms can be avoided.

The general idea is based on similar code in the i.MX clock driver, but
this here is a bit more generic as in general (e.g. on gs101) clocks
can come from various different clock units (driver instances) and
therefore it can be necessary to run this code multiple times until all
required stdout clocks have probed.

Signed-off-by: André Draszik <andre.draszik@linaro.org>

---
v6:
* drop a stray #include from drivers/clk/samsung/clk-gs101.c
---
 drivers/clk/clk.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7264cf6165ce..03c5d80e833c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4923,6 +4923,131 @@ static void clk_core_reparent_orphans(void)
 	clk_prepare_unlock();
 }
 
+/**
+ * struct of_clk_stdout_clks - holds data that is required for handling extra
+ * references to stdout clocks during early boot.
+ *
+ * On some platforms, earlycon depends on the bootloader setup stdout clocks
+ * being retained. In some cases stdout UART clocks (or their parents) can get
+ * disabled during loading of other drivers (e.g. i2c) causing earlycon to stop
+ * to work sometime into the boot, halting the system.
+ *
+ * Having logic to detect these clocks if earlycon is enabled helps with those
+ * cases by bumping their usage count during init. The extra usage count is
+ * later dropped at the end of init.
+ *
+ * @bump_refs: whether or not to add the extra stdout clock references
+ * @lock: mutex protecting access
+ * @have_all: whether or not we have acquired all clocks, to handle cases of
+ *            clocks coming from different drivers / instances
+ * @clks: clocks associated with stdout
+ * @n_clks: number of clocks associated with stdout
+ */
+static struct of_clk_stdout_clks {
+	bool bump_refs;
+
+	struct mutex lock;
+	bool have_all;
+	struct clk **clks;
+	size_t n_clks;
+} of_clk_stdout_clks = {
+	.lock = __MUTEX_INITIALIZER(of_clk_stdout_clks.lock),
+};
+
+static int __init of_clk_bump_stdout_clocks_param(char *str)
+{
+	of_clk_stdout_clks.bump_refs = true;
+	return 0;
+}
+__setup("earlycon", of_clk_bump_stdout_clocks_param);
+__setup_param("earlyprintk", of_clk_keep_stdout_clocks_earlyprintk,
+	      of_clk_bump_stdout_clocks_param, 0);
+
+static void of_clk_bump_stdout_clocks(void)
+{
+	size_t n_clks;
+
+	/*
+	 * We only need to run this code if required to do so and only ever
+	 * before late initcalls have run. Otherwise it'd be impossible to know
+	 * when to drop the extra clock references again.
+	 *
+	 * This generally means that this only works if on affected platforms
+	 * the clock drivers have been built-in (as opposed to being modules).
+	 */
+	if (!of_clk_stdout_clks.bump_refs)
+		return;
+
+	n_clks = of_clk_get_parent_count(of_stdout);
+	if (!n_clks || !of_stdout)
+		return;
+
+	mutex_lock(&of_clk_stdout_clks.lock);
+
+	/*
+	 * We only need to keep trying if we have not succeeded previously,
+	 * i.e. if not all required clocks were ready during previous attempts.
+	 */
+	if (of_clk_stdout_clks.have_all)
+		goto out_unlock;
+
+	if (!of_clk_stdout_clks.clks) {
+		of_clk_stdout_clks.n_clks = n_clks;
+
+		of_clk_stdout_clks.clks = kcalloc(of_clk_stdout_clks.n_clks,
+					      sizeof(*of_clk_stdout_clks.clks),
+					      GFP_KERNEL);
+		if (!of_clk_stdout_clks.clks)
+			goto out_unlock;
+	}
+
+	/* assume that this time we'll be able to grab all required clocks */
+	of_clk_stdout_clks.have_all = true;
+	for (size_t i = 0; i < n_clks; ++i) {
+		struct clk *clk;
+
+		/* we might have grabbed this clock in a previous attempt */
+		if (of_clk_stdout_clks.clks[i])
+			continue;
+
+		clk = of_clk_get(of_stdout, i);
+		if (IS_ERR(clk)) {
+			/* retry next time if clock has not probed yet */
+			of_clk_stdout_clks.have_all = false;
+			continue;
+		}
+
+		if (clk_prepare_enable(clk)) {
+			clk_put(clk);
+			continue;
+		}
+		of_clk_stdout_clks.clks[i] = clk;
+	}
+
+out_unlock:
+	mutex_unlock(&of_clk_stdout_clks.lock);
+}
+
+static int __init of_clk_drop_stdout_clocks(void)
+{
+	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
+		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
+		clk_put(of_clk_stdout_clks.clks[i]);
+	}
+
+	kfree(of_clk_stdout_clks.clks);
+
+	/*
+	 * Do not try to acquire stdout clocks after late initcalls, e.g.
+	 * during further module loading, as we then wouldn't have a way to
+	 * drop the references (and associated allocations) ever again.
+	 */
+	of_clk_stdout_clks.bump_refs = false;
+
+	return 0;
+}
+late_initcall_sync(of_clk_drop_stdout_clocks);
+
 /**
  * struct of_clk_provider - Clock provider registration structure
  * @link: Entry in global list of clock providers
@@ -5031,6 +5156,8 @@ int of_clk_add_provider(struct device_node *np,
 
 	fwnode_dev_initialized(&np->fwnode, true);
 
+	of_clk_bump_stdout_clocks();
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_clk_add_provider);
@@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct device_node *np,
 
 	fwnode_dev_initialized(&np->fwnode, true);
 
+	of_clk_bump_stdout_clocks();
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_clk_add_hw_provider);

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 02/20] clk: samsung: gs101: don't mark non-essential (UART) clocks critical

[André Draszik]

The peric0_top1_ipclk_0 and peric0_top1_pclk_0 are the clocks going to
peric0/uart_usi, with pclk being the bus clock. Without pclk running,
any bus access will hang.
Unfortunately, in commit d97b6c902a40 ("arm64: dts: exynos: gs101:
update USI UART to use peric0 clocks") the gs101 DT ended up specifying
an incorrect pclk in the respective node and instead the two clocks
here were marked as critical.

As a side-effect and by accident, having them 'critical' also
worked-around a problem where earlycon stops to work sometime into the
boot for two reasons:
    * peric0_top1_ipclk_0 requires its parent gout_cmu_peric0_ip to be
      running, but because earlycon doesn't deal with clocks that
      parent will be disabled when none of the other drivers that
      actually deal with clocks correctly require it to be running and
      the real serial driver (which does deal with clocks) hasn't taken
      over yet
    * hand-over between earlycon and serial driver appears to be
      fragile and clocks get enabled and disabled a few times, which
      also causes register access to hang while earlycon is still
      active
(A wordier explanation can also be found in [1])

Since then, the DT has been updated to use the correct clock in
commit 21e4e8807bfc ("arm64: dts: exynos: gs101: use correct clocks for
usi_uart"). Furthermore, the clk core now helps OF platforms with their
stdout (earlycon) clocks during early boot and thereby avoids the
problem described above.

The driver here can now be corrected and the work-arounds removed. Do
so.

Link: https://lore.kernel.org/all/d45de3b2bb6b48653842cf1f74e58889ed6783ae.camel@linaro.org/ [1]
Fixes: 893f133a040b ("clk: samsung: gs101: add support for cmu_peric0")
Signed-off-by: André Draszik <andre.draszik@linaro.org>
Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

---
v5: update commit message

v4:
- the earlycon issue described in the commit message in previous
  versions of this patch is gone with "clk: samsung: gs101: allow
  earlycon to work unconditionally", so no need to mention anything

v3:
- add git commit SHA1s (Krzysztof)
- add link to wordier description of earlycon issue

v2:
- commit message typo fixed
- collect Reviewed-by: tags
---
 drivers/clk/samsung/clk-gs101.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
index 85098c61c15e..9769c00b6ca8 100644
--- a/drivers/clk/samsung/clk-gs101.c
+++ b/drivers/clk/samsung/clk-gs101.c
@@ -3946,20 +3946,18 @@ static const struct samsung_gate_clock peric0_gate_clks[] __initconst = {
 	     "gout_peric0_peric0_top0_pclk_9", "mout_peric0_bus_user",
 	     CLK_CON_GAT_GOUT_BLK_PERIC0_UID_PERIC0_TOP0_IPCLKPORT_PCLK_9,
 	     21, 0, 0),
-	/* Disabling this clock makes the system hang. Mark the clock as critical. */
 	GATE(CLK_GOUT_PERIC0_PERIC0_TOP1_IPCLK_0,
 	     "gout_peric0_peric0_top1_ipclk_0", "dout_peric0_usi0_uart",
 	     CLK_CON_GAT_GOUT_BLK_PERIC0_UID_PERIC0_TOP1_IPCLKPORT_IPCLK_0,
-	     21, CLK_IS_CRITICAL, 0),
+	     21, 0, 0),
 	GATE(CLK_GOUT_PERIC0_PERIC0_TOP1_IPCLK_2,
 	     "gout_peric0_peric0_top1_ipclk_2", "dout_peric0_usi14_usi",
 	     CLK_CON_GAT_GOUT_BLK_PERIC0_UID_PERIC0_TOP1_IPCLKPORT_IPCLK_2,
 	     21, CLK_SET_RATE_PARENT, 0),
-	/* Disabling this clock makes the system hang. Mark the clock as critical. */
 	GATE(CLK_GOUT_PERIC0_PERIC0_TOP1_PCLK_0,
 	     "gout_peric0_peric0_top1_pclk_0", "mout_peric0_bus_user",
 	     CLK_CON_GAT_GOUT_BLK_PERIC0_UID_PERIC0_TOP1_IPCLKPORT_PCLK_0,
-	     21, CLK_IS_CRITICAL, 0),
+	     21, 0, 0),
 	GATE(CLK_GOUT_PERIC0_PERIC0_TOP1_PCLK_2,
 	     "gout_peric0_peric0_top1_pclk_2", "mout_peric0_bus_user",
 	     CLK_CON_GAT_GOUT_BLK_PERIC0_UID_PERIC0_TOP1_IPCLKPORT_PCLK_2,

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 03/20] clk: imx: imx25: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx25.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
index c566be848c2d..0474778f988f 100644
--- a/drivers/clk/imx/clk-imx25.c
+++ b/drivers/clk/imx/clk-imx25.c
@@ -219,8 +219,6 @@ static void __init __mx25_clocks_init(void __iomem *ccm_base)
 	 */
 	clk_set_parent(clk[cko_sel], clk[ipg]);
 
-	imx_register_uart_clocks();
-
 	imx_print_silicon_rev("i.MX25", mx25_revision());
 }
 

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 04/20] clk: imx: imx27: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx27.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx27.c b/drivers/clk/imx/clk-imx27.c
index 99618ded0939..53018e80ca51 100644
--- a/drivers/clk/imx/clk-imx27.c
+++ b/drivers/clk/imx/clk-imx27.c
@@ -164,8 +164,6 @@ static void __init _mx27_clocks_init(unsigned long fref)
 
 	clk_prepare_enable(clk[IMX27_CLK_EMI_AHB_GATE]);
 
-	imx_register_uart_clocks();
-
 	imx_print_silicon_rev("i.MX27", mx27_revision());
 }
 

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 05/20] clk: imx: imx35: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx35.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx35.c b/drivers/clk/imx/clk-imx35.c
index 3b6fdb4e0be7..00355a187340 100644
--- a/drivers/clk/imx/clk-imx35.c
+++ b/drivers/clk/imx/clk-imx35.c
@@ -234,8 +234,6 @@ static void __init _mx35_clocks_init(void)
 	 */
 	clk_prepare_enable(clk[scc_gate]);
 
-	imx_register_uart_clocks();
-
 	imx_print_silicon_rev("i.MX35", mx35_revision());
 }
 

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 06/20] clk: imx: imx5: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx5.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/clk/imx/clk-imx5.c b/drivers/clk/imx/clk-imx5.c
index b82044911603..163bed084a57 100644
--- a/drivers/clk/imx/clk-imx5.c
+++ b/drivers/clk/imx/clk-imx5.c
@@ -357,8 +357,6 @@ static void __init mx50_clocks_init(struct device_node *np)
 
 	r = clk_round_rate(clk[IMX5_CLK_USBOH3_PER_GATE], 54000000);
 	clk_set_rate(clk[IMX5_CLK_USBOH3_PER_GATE], r);
-
-	imx_register_uart_clocks();
 }
 CLK_OF_DECLARE(imx50_ccm, "fsl,imx50-ccm", mx50_clocks_init);
 
@@ -463,8 +461,6 @@ static void __init mx51_clocks_init(struct device_node *np)
 	val = readl(MXC_CCM_CLPCR);
 	val |= 1 << 23;
 	writel(val, MXC_CCM_CLPCR);
-
-	imx_register_uart_clocks();
 }
 CLK_OF_DECLARE(imx51_ccm, "fsl,imx51-ccm", mx51_clocks_init);
 
@@ -608,7 +604,5 @@ static void __init mx53_clocks_init(struct device_node *np)
 
 	r = clk_round_rate(clk[IMX5_CLK_USBOH3_PER_GATE], 54000000);
 	clk_set_rate(clk[IMX5_CLK_USBOH3_PER_GATE], r);
-
-	imx_register_uart_clocks();
 }
 CLK_OF_DECLARE(imx53_ccm, "fsl,imx53-ccm", mx53_clocks_init);

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 07/20] clk: imx: imx6q: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx6q.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
index bf4c1d9c9928..b436463e06d4 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -986,7 +986,5 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	}
 
 	clk_set_parent(hws[IMX6QDL_CLK_ENET_REF_SEL]->clk, hws[IMX6QDL_CLK_ENET_REF]->clk);
-
-	imx_register_uart_clocks();
 }
 CLK_OF_DECLARE(imx6q, "fsl,imx6q-ccm", imx6q_clocks_init);

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 08/20] clk: imx: imx6sl: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx6sl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6sl.c b/drivers/clk/imx/clk-imx6sl.c
index 47b8667cfa3f..bc3e414eeadd 100644
--- a/drivers/clk/imx/clk-imx6sl.c
+++ b/drivers/clk/imx/clk-imx6sl.c
@@ -439,7 +439,5 @@ static void __init imx6sl_clocks_init(struct device_node *ccm_node)
 
 	clk_set_parent(hws[IMX6SL_CLK_LCDIF_AXI_SEL]->clk,
 		       hws[IMX6SL_CLK_PLL2_PFD2]->clk);
-
-	imx_register_uart_clocks();
 }
 CLK_OF_DECLARE(imx6sl, "fsl,imx6sl-ccm", imx6sl_clocks_init);

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 09/20] clk: imx: imx6sll: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx6sll.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6sll.c b/drivers/clk/imx/clk-imx6sll.c
index 2fa70bf35e45..4310adc5d54f 100644
--- a/drivers/clk/imx/clk-imx6sll.c
+++ b/drivers/clk/imx/clk-imx6sll.c
@@ -340,8 +340,6 @@ static void __init imx6sll_clocks_init(struct device_node *ccm_node)
 
 	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_hw_data);
 
-	imx_register_uart_clocks();
-
 	/* Lower the AHB clock rate before changing the clock source. */
 	clk_set_rate(hws[IMX6SLL_CLK_AHB]->clk, 99000000);
 

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 10/20] clk: imx: imx6sx: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx6sx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6sx.c b/drivers/clk/imx/clk-imx6sx.c
index 69f8f6f9ca49..4e42e30245b3 100644
--- a/drivers/clk/imx/clk-imx6sx.c
+++ b/drivers/clk/imx/clk-imx6sx.c
@@ -555,7 +555,5 @@ static void __init imx6sx_clocks_init(struct device_node *ccm_node)
 
 	clk_set_parent(hws[IMX6SX_CLK_QSPI1_SEL]->clk, hws[IMX6SX_CLK_PLL2_BUS]->clk);
 	clk_set_parent(hws[IMX6SX_CLK_QSPI2_SEL]->clk, hws[IMX6SX_CLK_PLL2_BUS]->clk);
-
-	imx_register_uart_clocks();
 }
 CLK_OF_DECLARE(imx6sx, "fsl,imx6sx-ccm", imx6sx_clocks_init);

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 11/20] clk: imx: imx6ul: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx6ul.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6ul.c b/drivers/clk/imx/clk-imx6ul.c
index 05c7a82b751f..b828cecd3690 100644
--- a/drivers/clk/imx/clk-imx6ul.c
+++ b/drivers/clk/imx/clk-imx6ul.c
@@ -544,8 +544,6 @@ static void __init imx6ul_clocks_init(struct device_node *ccm_node)
 
 	clk_set_parent(hws[IMX6UL_CLK_ENET1_REF_SEL]->clk, hws[IMX6UL_CLK_ENET1_REF_125M]->clk);
 	clk_set_parent(hws[IMX6UL_CLK_ENET2_REF_SEL]->clk, hws[IMX6UL_CLK_ENET2_REF_125M]->clk);
-
-	imx_register_uart_clocks();
 }
 
 CLK_OF_DECLARE(imx6ul, "fsl,imx6ul-ccm", imx6ul_clocks_init);

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 12/20] clk: imx: imx7d: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx7d.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index 99adc55e3f5d..789cc3afd062 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -882,7 +882,5 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
 	hws[IMX7D_USB1_MAIN_480M_CLK] = imx_clk_hw_fixed_factor("pll_usb1_main_clk", "osc", 20, 1);
 	hws[IMX7D_USB_MAIN_480M_CLK] = imx_clk_hw_fixed_factor("pll_usb_main_clk", "osc", 20, 1);
 
-	imx_register_uart_clocks();
-
 }
 CLK_OF_DECLARE(imx7d, "fsl,imx7d-ccm", imx7d_clocks_init);

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 13/20] clk: imx: imx7ulp: drop calls to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

This should also plug a memory and clock reference leak due to multiple
calls to imx_register_uart_clocks(), one for each clock unit.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx7ulp.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/clk/imx/clk-imx7ulp.c b/drivers/clk/imx/clk-imx7ulp.c
index f4a48a42637f..517fb388ce85 100644
--- a/drivers/clk/imx/clk-imx7ulp.c
+++ b/drivers/clk/imx/clk-imx7ulp.c
@@ -175,8 +175,6 @@ static void __init imx7ulp_clk_pcc2_init(struct device_node *np)
 	imx_check_clk_hws(hws, clk_data->num);
 
 	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_data);
-
-	imx_register_uart_clocks();
 }
 CLK_OF_DECLARE(imx7ulp_clk_pcc2, "fsl,imx7ulp-pcc2", imx7ulp_clk_pcc2_init);
 
@@ -222,8 +220,6 @@ static void __init imx7ulp_clk_pcc3_init(struct device_node *np)
 	imx_check_clk_hws(hws, clk_data->num);
 
 	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_data);
-
-	imx_register_uart_clocks();
 }
 CLK_OF_DECLARE(imx7ulp_clk_pcc3, "fsl,imx7ulp-pcc3", imx7ulp_clk_pcc3_init);
 

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 14/20] clk: imx: imx8mm: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx8mm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index 342049b847b9..3f649da0230d 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -609,8 +609,6 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
 		goto unregister_hws;
 	}
 
-	imx_register_uart_clocks();
-
 	return 0;
 
 unregister_hws:

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 15/20] clk: imx: imx8mn: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx8mn.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
index ab77e148e70c..af7f4c6b51e9 100644
--- a/drivers/clk/imx/clk-imx8mn.c
+++ b/drivers/clk/imx/clk-imx8mn.c
@@ -603,8 +603,6 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
 		goto unregister_hws;
 	}
 
-	imx_register_uart_clocks();
-
 	return 0;
 
 unregister_hws:

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 16/20] clk: imx: imx8mp: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx8mp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 516dbd170c8a..1c128c051199 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -721,8 +721,6 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	imx_register_uart_clocks();
-
 	return 0;
 }
 

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 17/20] clk: imx: imx8mq: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx8mq.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
index f70ed231b92d..db9aaec42800 100644
--- a/drivers/clk/imx/clk-imx8mq.c
+++ b/drivers/clk/imx/clk-imx8mq.c
@@ -604,8 +604,6 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
 		goto unregister_hws;
 	}
 
-	imx_register_uart_clocks();
-
 	return 0;
 
 unregister_hws:

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 18/20] clk: imx: imx8ulp: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx8ulp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8ulp.c b/drivers/clk/imx/clk-imx8ulp.c
index 535b6364ca7e..5531e51a1e80 100644
--- a/drivers/clk/imx/clk-imx8ulp.c
+++ b/drivers/clk/imx/clk-imx8ulp.c
@@ -385,8 +385,6 @@ static int imx8ulp_clk_pcc3_init(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	imx_register_uart_clocks();
-
 	/* register the pcc3 reset controller */
 	return imx8ulp_pcc_reset_init(pdev, base, pcc3_resets, ARRAY_SIZE(pcc3_resets));
 }

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 19/20] clk: imx: imx93: drop call to imx_register_uart_clocks()

[André Draszik]

The clk core now does something similar for us as part of
of_clk_add_provider() and of_clk_add_hw_provider() and this i.MX
specific call isn't necessary anymore.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk-imx93.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx93.c b/drivers/clk/imx/clk-imx93.c
index c6a9bc8ecc1f..9b8006b15d0c 100644
--- a/drivers/clk/imx/clk-imx93.c
+++ b/drivers/clk/imx/clk-imx93.c
@@ -343,8 +343,6 @@ static int imx93_clocks_probe(struct platform_device *pdev)
 		goto unregister_hws;
 	}
 
-	imx_register_uart_clocks();
-
 	return 0;
 
 unregister_hws:

-- 
2.46.0.rc2.264.g509ed76dc8-goog

[PATCH v6 20/20] clk: imx: drop imx_register_uart_clocks()

[André Draszik]

There are no users of this anymore in the tree and the clk core
implements something similar now, we can remove
imx_register_uart_clocks().

Do so.

Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
 drivers/clk/imx/clk.c | 72 ---------------------------------------------------
 drivers/clk/imx/clk.h |  7 -----
 2 files changed, 79 deletions(-)

diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
index df83bd939492..5f998e56a4ed 100644
--- a/drivers/clk/imx/clk.c
+++ b/drivers/clk/imx/clk.c
@@ -154,77 +154,5 @@ void imx_cscmr1_fixup(u32 *val)
 	return;
 }
 
-#ifndef MODULE
-
-static bool imx_keep_uart_clocks;
-static int imx_enabled_uart_clocks;
-static struct clk **imx_uart_clocks;
-
-static int __init imx_keep_uart_clocks_param(char *str)
-{
-	imx_keep_uart_clocks = 1;
-
-	return 0;
-}
-__setup_param("earlycon", imx_keep_uart_earlycon,
-	      imx_keep_uart_clocks_param, 0);
-__setup_param("earlyprintk", imx_keep_uart_earlyprintk,
-	      imx_keep_uart_clocks_param, 0);
-
-void imx_register_uart_clocks(void)
-{
-	unsigned int num __maybe_unused;
-
-	imx_enabled_uart_clocks = 0;
-
-/* i.MX boards use device trees now.  For build tests without CONFIG_OF, do nothing */
-#ifdef CONFIG_OF
-	if (imx_keep_uart_clocks) {
-		int i;
-
-		num = of_clk_get_parent_count(of_stdout);
-		if (!num)
-			return;
-
-		if (!of_stdout)
-			return;
-
-		imx_uart_clocks = kcalloc(num, sizeof(struct clk *), GFP_KERNEL);
-		if (!imx_uart_clocks)
-			return;
-
-		for (i = 0; i < num; i++) {
-			imx_uart_clocks[imx_enabled_uart_clocks] = of_clk_get(of_stdout, i);
-
-			/* Stop if there are no more of_stdout references */
-			if (IS_ERR(imx_uart_clocks[imx_enabled_uart_clocks]))
-				return;
-
-			/* Only enable the clock if it's not NULL */
-			if (imx_uart_clocks[imx_enabled_uart_clocks])
-				clk_prepare_enable(imx_uart_clocks[imx_enabled_uart_clocks++]);
-		}
-	}
-#endif
-}
-
-static int __init imx_clk_disable_uart(void)
-{
-	if (imx_keep_uart_clocks && imx_enabled_uart_clocks) {
-		int i;
-
-		for (i = 0; i < imx_enabled_uart_clocks; i++) {
-			clk_disable_unprepare(imx_uart_clocks[i]);
-			clk_put(imx_uart_clocks[i]);
-		}
-	}
-
-	kfree(imx_uart_clocks);
-
-	return 0;
-}
-late_initcall_sync(imx_clk_disable_uart);
-#endif
-
 MODULE_DESCRIPTION("Common clock support for NXP i.MX SoC family");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index aa5202f284f3..314730f848f7 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -11,13 +11,6 @@ extern bool mcore_booted;
 
 void imx_check_clocks(struct clk *clks[], unsigned int count);
 void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
-#ifndef MODULE
-void imx_register_uart_clocks(void);
-#else
-static inline void imx_register_uart_clocks(void)
-{
-}
-#endif
 void imx_mmdc_mask_handshake(void __iomem *ccm_base, unsigned int chn);
 void imx_unregister_hw_clocks(struct clk_hw *hws[], unsigned int count);
 

-- 
2.46.0.rc2.264.g509ed76dc8-goog

Re: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon

[Stephen Boyd]

Quoting André Draszik (2024-08-08 07:42:42)
> On some platforms, earlycon depends on the bootloader setup stdout
> clocks being retained. In some cases stdout UART clocks (or their
> parents) can get disabled during loading of other drivers (e.g. i2c)
> causing earlycon to stop to work sometime into the boot, halting the
> whole system.
> 
> Since there are at least two platforms where that is the case, i.MX and
> the Exynos-derivative gs101, this patch adds some logic to the clk core
> to detect these clocks if earlycon is enabled, to bump their usage
> count as part of of_clk_add_hw_provider() and of_clk_add_provider(),
> and to release them again at the end of init.
> 
> This way code duplication in affected platforms can be avoided.
> 
> The general idea is based on similar code in the i.MX clock driver, but
> this here is a bit more generic as in general (e.g. on gs101) clocks
> can come from various different clock units (driver instances) and
> therefore it can be necessary to run this code multiple times until all
> required stdout clocks have probed.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>
> 
> ---

Thanks for doing this!

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 7264cf6165ce..03c5d80e833c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4923,6 +4923,131 @@ static void clk_core_reparent_orphans(void)
>         clk_prepare_unlock();
>  }
>  
> +/**
> + * struct of_clk_stdout_clks - holds data that is required for handling extra
> + * references to stdout clocks during early boot.
> + *
> + * On some platforms, earlycon depends on the bootloader setup stdout clocks
> + * being retained. In some cases stdout UART clocks (or their parents) can get
> + * disabled during loading of other drivers (e.g. i2c) causing earlycon to stop
> + * to work sometime into the boot, halting the system.
> + *
> + * Having logic to detect these clocks if earlycon is enabled helps with those
> + * cases by bumping their usage count during init. The extra usage count is
> + * later dropped at the end of init.
> + *
> + * @bump_refs: whether or not to add the extra stdout clock references
> + * @lock: mutex protecting access
> + * @have_all: whether or not we have acquired all clocks, to handle cases of
> + *            clocks coming from different drivers / instances
> + * @clks: clocks associated with stdout
> + * @n_clks: number of clocks associated with stdout
> + */
> +static struct of_clk_stdout_clks {
> +       bool bump_refs;
> +
> +       struct mutex lock;
> +       bool have_all;
> +       struct clk **clks;
> +       size_t n_clks;
> +} of_clk_stdout_clks = {

This can be initdata?

> +       .lock = __MUTEX_INITIALIZER(of_clk_stdout_clks.lock),
> +};
> +
> +static int __init of_clk_bump_stdout_clocks_param(char *str)
> +{
> +       of_clk_stdout_clks.bump_refs = true;
> +       return 0;
> +}
> +__setup("earlycon", of_clk_bump_stdout_clocks_param);
> +__setup_param("earlyprintk", of_clk_keep_stdout_clocks_earlyprintk,
> +             of_clk_bump_stdout_clocks_param, 0);
> +
> +static void of_clk_bump_stdout_clocks(void)

This can be __init?

> +{
> +       size_t n_clks;
> +
> +       /*
> +        * We only need to run this code if required to do so and only ever
> +        * before late initcalls have run. Otherwise it'd be impossible to know
> +        * when to drop the extra clock references again.
> +        *
> +        * This generally means that this only works if on affected platforms
> +        * the clock drivers have been built-in (as opposed to being modules).
> +        */
> +       if (!of_clk_stdout_clks.bump_refs)
> +               return;
> +
> +       n_clks = of_clk_get_parent_count(of_stdout);
> +       if (!n_clks || !of_stdout)
> +               return;
> +
> +       mutex_lock(&of_clk_stdout_clks.lock);
> +
> +       /*
> +        * We only need to keep trying if we have not succeeded previously,
> +        * i.e. if not all required clocks were ready during previous attempts.
> +        */
> +       if (of_clk_stdout_clks.have_all)
> +               goto out_unlock;
> +
> +       if (!of_clk_stdout_clks.clks) {
> +               of_clk_stdout_clks.n_clks = n_clks;
> +
> +               of_clk_stdout_clks.clks = kcalloc(of_clk_stdout_clks.n_clks,
> +                                             sizeof(*of_clk_stdout_clks.clks),
> +                                             GFP_KERNEL);
> +               if (!of_clk_stdout_clks.clks)
> +                       goto out_unlock;
> +       }
> +
> +       /* assume that this time we'll be able to grab all required clocks */
> +       of_clk_stdout_clks.have_all = true;
> +       for (size_t i = 0; i < n_clks; ++i) {
> +               struct clk *clk;
> +
> +               /* we might have grabbed this clock in a previous attempt */
> +               if (of_clk_stdout_clks.clks[i])
> +                       continue;
> +
> +               clk = of_clk_get(of_stdout, i);
> +               if (IS_ERR(clk)) {
> +                       /* retry next time if clock has not probed yet */
> +                       of_clk_stdout_clks.have_all = false;
> +                       continue;
> +               }
> +
> +               if (clk_prepare_enable(clk)) {
> +                       clk_put(clk);
> +                       continue;
> +               }
> +               of_clk_stdout_clks.clks[i] = clk;
> +       }
> +
> +out_unlock:
> +       mutex_unlock(&of_clk_stdout_clks.lock);
> +}
> +
> +static int __init of_clk_drop_stdout_clocks(void)
> +{
> +       for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> +               clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> +               clk_put(of_clk_stdout_clks.clks[i]);
> +       }
> +
> +       kfree(of_clk_stdout_clks.clks);
> +
> +       /*
> +        * Do not try to acquire stdout clocks after late initcalls, e.g.
> +        * during further module loading, as we then wouldn't have a way to
> +        * drop the references (and associated allocations) ever again.
> +        */
> +       of_clk_stdout_clks.bump_refs = false;
> +
> +       return 0;
> +}
> +late_initcall_sync(of_clk_drop_stdout_clocks);
> +
>  /**
>   * struct of_clk_provider - Clock provider registration structure
>   * @link: Entry in global list of clock providers
> @@ -5031,6 +5156,8 @@ int of_clk_add_provider(struct device_node *np,
>  
>         fwnode_dev_initialized(&np->fwnode, true);
>  
> +       of_clk_bump_stdout_clocks();

This can be a wrapper function that isn't marked __init but which calls
the init function with __ref. That lets us free up as much code as
possible. We need to set a bool in of_clk_drop_stdout_clocks() that when
false doesn't call the __init functions that are wrapped though, i.e.
'bump_refs'. Here's the structure:

	static bool bump_stdout_clks __ro_after_init = true;

	static int __init _of_clk_bump_stdout_clks(void)
	{
		...
	}

	static int __ref of_clk_bump_stdout_clks(void)
	{
		if (bump_stdout_clks)
			return _of_clk_bump_stdout_clks();

		return 0;
	}

	static int __init of_clk_drop_stdout_clks(void)
	{
		bump_stdout_clks = false;
		...
	}
	late_initcall_sync(of_clk_drop_stdout_clks);

> +
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_clk_add_provider);

RE: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon

[Peng Fan]

> Subject: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon
> 
> On some platforms, earlycon depends on the bootloader setup stdout
> clocks being retained. In some cases stdout UART clocks (or their
> parents) can get disabled during loading of other drivers (e.g. i2c)
> causing earlycon to stop to work sometime into the boot, halting the
> whole system.
> 
> Since there are at least two platforms where that is the case, i.MX and
> the Exynos-derivative gs101, this patch adds some logic to the clk core
> to detect these clocks if earlycon is enabled, to bump their usage count
> as part of of_clk_add_hw_provider() and of_clk_add_provider(), and to
> release them again at the end of init.
> 
> This way code duplication in affected platforms can be avoided.
> 
> The general idea is based on similar code in the i.MX clock driver, but
> this here is a bit more generic as in general (e.g. on gs101) clocks can
> come from various different clock units (driver instances) and therefore
> it can be necessary to run this code multiple times until all required
> stdout clocks have probed.
> 
> Signed-off-by: André Draszik <andre.draszik@linaro.org>
> 
> ---
> v6:
> * drop a stray #include from drivers/clk/samsung/clk-gs101.c
> ---
>  drivers/clk/clk.c | 129
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
> 7264cf6165ce..03c5d80e833c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4923,6 +4923,131 @@ static void
> clk_core_reparent_orphans(void)
>  	clk_prepare_unlock();
>  }
> 
> +/**
> + * struct of_clk_stdout_clks - holds data that is required for handling
> +extra
> + * references to stdout clocks during early boot.
> + *
> + * On some platforms, earlycon depends on the bootloader setup
> stdout
> +clocks
> + * being retained. In some cases stdout UART clocks (or their parents)
> +can get
> + * disabled during loading of other drivers (e.g. i2c) causing earlycon
> +to stop
> + * to work sometime into the boot, halting the system.
> + *
> + * Having logic to detect these clocks if earlycon is enabled helps
> +with those
> + * cases by bumping their usage count during init. The extra usage
> +count is
> + * later dropped at the end of init.
> + *
> + * @bump_refs: whether or not to add the extra stdout clock
> references
> + * @lock: mutex protecting access
> + * @have_all: whether or not we have acquired all clocks, to handle
> cases of
> + *            clocks coming from different drivers / instances
> + * @clks: clocks associated with stdout
> + * @n_clks: number of clocks associated with stdout  */ static struct
> +of_clk_stdout_clks {
> +	bool bump_refs;
> +
> +	struct mutex lock;
> +	bool have_all;
> +	struct clk **clks;
> +	size_t n_clks;
> +} of_clk_stdout_clks = {
> +	.lock = __MUTEX_INITIALIZER(of_clk_stdout_clks.lock),
> +};
> +
> +static int __init of_clk_bump_stdout_clocks_param(char *str) {
> +	of_clk_stdout_clks.bump_refs = true;
> +	return 0;
> +}
> +__setup("earlycon", of_clk_bump_stdout_clocks_param);
> +__setup_param("earlyprintk", of_clk_keep_stdout_clocks_earlyprintk,
> +	      of_clk_bump_stdout_clocks_param, 0);
> +
> +static void of_clk_bump_stdout_clocks(void) {
> +	size_t n_clks;
> +
> +	/*
> +	 * We only need to run this code if required to do so and only
> ever
> +	 * before late initcalls have run. Otherwise it'd be impossible to
> know
> +	 * when to drop the extra clock references again.
> +	 *
> +	 * This generally means that this only works if on affected
> platforms
> +	 * the clock drivers have been built-in (as opposed to being
> modules).
> +	 */
> +	if (!of_clk_stdout_clks.bump_refs)
> +		return;
> +
> +	n_clks = of_clk_get_parent_count(of_stdout);
> +	if (!n_clks || !of_stdout)
> +		return;
> +
> +	mutex_lock(&of_clk_stdout_clks.lock);
> +
> +	/*
> +	 * We only need to keep trying if we have not succeeded
> previously,
> +	 * i.e. if not all required clocks were ready during previous
> attempts.
> +	 */
> +	if (of_clk_stdout_clks.have_all)
> +		goto out_unlock;
> +
> +	if (!of_clk_stdout_clks.clks) {
> +		of_clk_stdout_clks.n_clks = n_clks;
> +
> +		of_clk_stdout_clks.clks =
> kcalloc(of_clk_stdout_clks.n_clks,
> +
> sizeof(*of_clk_stdout_clks.clks),
> +					      GFP_KERNEL);
> +		if (!of_clk_stdout_clks.clks)
> +			goto out_unlock;
> +	}
> +
> +	/* assume that this time we'll be able to grab all required
> clocks */
> +	of_clk_stdout_clks.have_all = true;
> +	for (size_t i = 0; i < n_clks; ++i) {
> +		struct clk *clk;
> +
> +		/* we might have grabbed this clock in a previous
> attempt */
> +		if (of_clk_stdout_clks.clks[i])
> +			continue;
> +
> +		clk = of_clk_get(of_stdout, i);
> +		if (IS_ERR(clk)) {
> +			/* retry next time if clock has not probed yet
> */
> +			of_clk_stdout_clks.have_all = false;
> +			continue;
> +		}
> +
> +		if (clk_prepare_enable(clk)) {
> +			clk_put(clk);
> +			continue;
> +		}
> +		of_clk_stdout_clks.clks[i] = clk;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&of_clk_stdout_clks.lock);
> +}
> +
> +static int __init of_clk_drop_stdout_clocks(void) {
> +	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> +		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> +		clk_put(of_clk_stdout_clks.clks[i]);
> +	}
> +
> +	kfree(of_clk_stdout_clks.clks);
> +
> +	/*
> +	 * Do not try to acquire stdout clocks after late initcalls, e.g.
> +	 * during further module loading, as we then wouldn't have a
> way to
> +	 * drop the references (and associated allocations) ever again.
> +	 */
> +	of_clk_stdout_clks.bump_refs = false;
> +
> +	return 0;
> +}
> +late_initcall_sync(of_clk_drop_stdout_clocks);

If the uart driver is built as module, this might break earlycon.
Before uart driver loaded, clk disabled per my understanding.

> +
>  /**
>   * struct of_clk_provider - Clock provider registration structure
>   * @link: Entry in global list of clock providers @@ -5031,6 +5156,8
> @@ int of_clk_add_provider(struct device_node *np,
> 
>  	fwnode_dev_initialized(&np->fwnode, true);
> 
> +	of_clk_bump_stdout_clocks();
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_clk_add_provider);
> @@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct
> device_node *np,
> 
>  	fwnode_dev_initialized(&np->fwnode, true);
> 
> +	of_clk_bump_stdout_clocks();

If clock driver is built as module,  the will make the
clocks will be always enabled, if my understanding is correct.


Regards,
Peng.

> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_clk_add_hw_provider);
> 
> --
> 2.46.0.rc2.264.g509ed76dc8-goog

Re: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon

[André Draszik]

Hi Stephen,

On Thu, 2024-08-08 at 13:14 -0700, Stephen Boyd wrote:
> > +static struct of_clk_stdout_clks {
> > +       bool bump_refs;
> > +
> > +       struct mutex lock;
> > +       bool have_all;
> > +       struct clk **clks;
> > +       size_t n_clks;
> > +} of_clk_stdout_clks = {
> 
> This can be initdata?

With the __ref wrapper you suggested below that's indeed possible. Without,
it's not, and I wasn't aware of __ref. Thanks for the pointer!

> > +       .lock = __MUTEX_INITIALIZER(of_clk_stdout_clks.lock),
> > +};
> > +
> > +static int __init of_clk_bump_stdout_clocks_param(char *str)
> > +{
> > +       of_clk_stdout_clks.bump_refs = true;
> > +       return 0;
> > +}
> > +__setup("earlycon", of_clk_bump_stdout_clocks_param);
> > +__setup_param("earlyprintk", of_clk_keep_stdout_clocks_earlyprintk,
> > +             of_clk_bump_stdout_clocks_param, 0);
> > +
> > +static void of_clk_bump_stdout_clocks(void)
> 
> This can be __init?

dito.

Cheers,
Andre'

> 
> > +{
> > +       size_t n_clks;
> > +
> > +       /*
> > +        * We only need to run this code if required to do so and only ever
> > +        * before late initcalls have run. Otherwise it'd be impossible to know
> > +        * when to drop the extra clock references again.
> > +        *
> > +        * This generally means that this only works if on affected platforms
> > +        * the clock drivers have been built-in (as opposed to being modules).
> > +        */
> > +       if (!of_clk_stdout_clks.bump_refs)
> > +               return;
> > +
> > +       n_clks = of_clk_get_parent_count(of_stdout);
> > +       if (!n_clks || !of_stdout)
> > +               return;
> > +
> > +       mutex_lock(&of_clk_stdout_clks.lock);
> > +
> > +       /*
> > +        * We only need to keep trying if we have not succeeded previously,
> > +        * i.e. if not all required clocks were ready during previous attempts.
> > +        */
> > +       if (of_clk_stdout_clks.have_all)
> > +               goto out_unlock;
> > +
> > +       if (!of_clk_stdout_clks.clks) {
> > +               of_clk_stdout_clks.n_clks = n_clks;
> > +
> > +               of_clk_stdout_clks.clks = kcalloc(of_clk_stdout_clks.n_clks,
> > +                                             sizeof(*of_clk_stdout_clks.clks),
> > +                                             GFP_KERNEL);
> > +               if (!of_clk_stdout_clks.clks)
> > +                       goto out_unlock;
> > +       }
> > +
> > +       /* assume that this time we'll be able to grab all required clocks */
> > +       of_clk_stdout_clks.have_all = true;
> > +       for (size_t i = 0; i < n_clks; ++i) {
> > +               struct clk *clk;
> > +
> > +               /* we might have grabbed this clock in a previous attempt */
> > +               if (of_clk_stdout_clks.clks[i])
> > +                       continue;
> > +
> > +               clk = of_clk_get(of_stdout, i);
> > +               if (IS_ERR(clk)) {
> > +                       /* retry next time if clock has not probed yet */
> > +                       of_clk_stdout_clks.have_all = false;
> > +                       continue;
> > +               }
> > +
> > +               if (clk_prepare_enable(clk)) {
> > +                       clk_put(clk);
> > +                       continue;
> > +               }
> > +               of_clk_stdout_clks.clks[i] = clk;
> > +       }
> > +
> > +out_unlock:
> > +       mutex_unlock(&of_clk_stdout_clks.lock);
> > +}
> > +
> > +static int __init of_clk_drop_stdout_clocks(void)
> > +{
> > +       for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > +               clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > +               clk_put(of_clk_stdout_clks.clks[i]);
> > +       }
> > +
> > +       kfree(of_clk_stdout_clks.clks);
> > +
> > +       /*
> > +        * Do not try to acquire stdout clocks after late initcalls, e.g.
> > +        * during further module loading, as we then wouldn't have a way to
> > +        * drop the references (and associated allocations) ever again.
> > +        */
> > +       of_clk_stdout_clks.bump_refs = false;
> > +
> > +       return 0;
> > +}
> > +late_initcall_sync(of_clk_drop_stdout_clocks);
> > +
> >  /**
> >   * struct of_clk_provider - Clock provider registration structure
> >   * @link: Entry in global list of clock providers
> > @@ -5031,6 +5156,8 @@ int of_clk_add_provider(struct device_node *np,
> >  
> >         fwnode_dev_initialized(&np->fwnode, true);
> >  
> > +       of_clk_bump_stdout_clocks();
> 
> This can be a wrapper function that isn't marked __init but which calls
> the init function with __ref. That lets us free up as much code as
> possible. We need to set a bool in of_clk_drop_stdout_clocks() that when
> false doesn't call the __init functions that are wrapped though, i.e.
> 'bump_refs'. Here's the structure:
> 
> 	static bool bump_stdout_clks __ro_after_init = true;
> 
> 	static int __init _of_clk_bump_stdout_clks(void)
> 	{
> 		...
> 	}
> 
> 	static int __ref of_clk_bump_stdout_clks(void)
> 	{
> 		if (bump_stdout_clks)
> 			return _of_clk_bump_stdout_clks();
> 
> 		return 0;
> 	}
> 
> 	static int __init of_clk_drop_stdout_clks(void)
> 	{
> 		bump_stdout_clks = false;
> 		...
> 	}
> 	late_initcall_sync(of_clk_drop_stdout_clks);
> 
> > +
> >         return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(of_clk_add_provider);

Re: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon

[André Draszik]

Hi Peng,

On Fri, 2024-08-09 at 07:16 +0000, Peng Fan wrote:
> > +static int __init of_clk_drop_stdout_clocks(void) {
> > +	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > +		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > +		clk_put(of_clk_stdout_clks.clks[i]);
> > +	}
> > +
> > +	kfree(of_clk_stdout_clks.clks);
> > +
> > +	/*
> > +	 * Do not try to acquire stdout clocks after late initcalls, e.g.
> > +	 * during further module loading, as we then wouldn't have a
> > way to
> > +	 * drop the references (and associated allocations) ever again.
> > +	 */
> > +	of_clk_stdout_clks.bump_refs = false;
> > +
> > +	return 0;
> > +}
> > +late_initcall_sync(of_clk_drop_stdout_clocks);
> 
> If the uart driver is built as module, this might break earlycon.
> Before uart driver loaded, clk disabled per my understanding.

You're right.

With this in mind, I'm not sure then if a generic solution is possible...

I guess it has to be duplicated into the platforms after all and platforms
can enable this if they opt to disallow uart as module?

Any other suggestions?

> > +
> >  /**
> >   * struct of_clk_provider - Clock provider registration structure
> >   * @link: Entry in global list of clock providers @@ -5031,6 +5156,8
> > @@ int of_clk_add_provider(struct device_node *np,
> > 
> >  	fwnode_dev_initialized(&np->fwnode, true);
> > 
> > +	of_clk_bump_stdout_clocks();
> > +
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(of_clk_add_provider);
> > @@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct
> > device_node *np,
> > 
> >  	fwnode_dev_initialized(&np->fwnode, true);
> > 
> > +	of_clk_bump_stdout_clocks();
> 
> If clock driver is built as module,  the will make the
> clocks will be always enabled, if my understanding is correct.

until late_initcall_sync(), at which point it'll be disabled before the uart
driver has probed, yes :-(

Cheers,
Andre'

Re: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon

[André Draszik]

On Fri, 2024-08-09 at 10:02 +0100, André Draszik wrote:
> Hi Peng,
> 
> On Fri, 2024-08-09 at 07:16 +0000, Peng Fan wrote:
> > > +static int __init of_clk_drop_stdout_clocks(void) {
> > > +	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > > +		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > > +		clk_put(of_clk_stdout_clks.clks[i]);
> > > +	}
> > > +
> > > +	kfree(of_clk_stdout_clks.clks);
> > > +
> > > +	/*
> > > +	 * Do not try to acquire stdout clocks after late initcalls, e.g.
> > > +	 * during further module loading, as we then wouldn't have a
> > > way to
> > > +	 * drop the references (and associated allocations) ever again.
> > > +	 */
> > > +	of_clk_stdout_clks.bump_refs = false;
> > > +
> > > +	return 0;
> > > +}
> > > +late_initcall_sync(of_clk_drop_stdout_clocks);
> > 
> > If the uart driver is built as module, this might break earlycon.
> > Before uart driver loaded, clk disabled per my understanding.
> 
> You're right.
> 
> With this in mind, I'm not sure then if a generic solution is possible...
> 
> I guess it has to be duplicated into the platforms after all and platforms
> can enable this if they opt to disallow uart as module?
> 
> Any other suggestions?
> 
> > > +
> > >  /**
> > >   * struct of_clk_provider - Clock provider registration structure
> > >   * @link: Entry in global list of clock providers @@ -5031,6 +5156,8
> > > @@ int of_clk_add_provider(struct device_node *np,
> > > 
> > >  	fwnode_dev_initialized(&np->fwnode, true);
> > > 
> > > +	of_clk_bump_stdout_clocks();
> > > +
> > >  	return ret;
> > >  }
> > >  EXPORT_SYMBOL_GPL(of_clk_add_provider);
> > > @@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct
> > > device_node *np,
> > > 
> > >  	fwnode_dev_initialized(&np->fwnode, true);
> > > 
> > > +	of_clk_bump_stdout_clocks();
> > 
> > If clock driver is built as module,  the will make the
> > clocks will be always enabled, if my understanding is correct.
> 
> until late_initcall_sync(), at which point it'll be disabled before the uart
> driver has probed, yes :-(

Sorry, ignore that. If clock driver is built as module, the code to bump
the clocks is disabled by the time this code runs (due to setting the flag
as part of late_initcall_sync(of_clk_drop_stdout_clocks)), in other words
it will not bump the clocks at all in that case and behaviour is as before.

Did I miss something?

Cheers,
Andre'

RE: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon

[Peng Fan]

> Subject: Re: [PATCH v6 01/20] clk: bump stdout clock usage for
> earlycon
> 
> On Fri, 2024-08-09 at 10:02 +0100, André Draszik wrote:
> > Hi Peng,
> >
> > On Fri, 2024-08-09 at 07:16 +0000, Peng Fan wrote:
> > > > +static int __init of_clk_drop_stdout_clocks(void) {
> > > > +	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > > > +		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > > > +		clk_put(of_clk_stdout_clks.clks[i]);
> > > > +	}
> > > > +
> > > > +	kfree(of_clk_stdout_clks.clks);
> > > > +
> > > > +	/*
> > > > +	 * Do not try to acquire stdout clocks after late initcalls, e.g.
> > > > +	 * during further module loading, as we then wouldn't have a
> > > > way to
> > > > +	 * drop the references (and associated allocations) ever again.
> > > > +	 */
> > > > +	of_clk_stdout_clks.bump_refs = false;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +late_initcall_sync(of_clk_drop_stdout_clocks);
> > >
> > > If the uart driver is built as module, this might break earlycon.
> > > Before uart driver loaded, clk disabled per my understanding.
> >
> > You're right.
> >
> > With this in mind, I'm not sure then if a generic solution is possible...
> >
> > I guess it has to be duplicated into the platforms after all and
> > platforms can enable this if they opt to disallow uart as module?
> >
> > Any other suggestions?
> >
> > > > +
> > > >  /**
> > > >   * struct of_clk_provider - Clock provider registration structure
> > > >   * @link: Entry in global list of clock providers @@ -5031,6
> > > > +5156,8 @@ int of_clk_add_provider(struct device_node *np,
> > > >
> > > >  	fwnode_dev_initialized(&np->fwnode, true);
> > > >
> > > > +	of_clk_bump_stdout_clocks();
> > > > +
> > > >  	return ret;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(of_clk_add_provider);
> > > > @@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct
> > > > device_node *np,
> > > >
> > > >  	fwnode_dev_initialized(&np->fwnode, true);
> > > >
> > > > +	of_clk_bump_stdout_clocks();
> > >
> > > If clock driver is built as module,  the will make the clocks will
> > > be always enabled, if my understanding is correct.
> >
> > until late_initcall_sync(), at which point it'll be disabled before
> > the uart driver has probed, yes :-(
> 
> Sorry, ignore that. If clock driver is built as module, the code to bump
> the clocks is disabled by the time this code runs (due to setting the flag
> as part of late_initcall_sync(of_clk_drop_stdout_clocks)), in other
> words it will not bump the clocks at all in that case and behaviour is as
> before.

Just checked again, you are right.
If the clock driver is built at module and "earlycon" is set in bootargs,
of_clk_stdout_clks.bump_refs will be true. late_initcall_sync will
set it back to false.

Regards,
Peng.

> 
> Did I miss something?
> 
> Cheers,
> Andre'

RE: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon

[Peng Fan]

> Subject: Re: [PATCH v6 01/20] clk: bump stdout clock usage for
> earlycon
> 
> Hi Peng,
> 
> On Fri, 2024-08-09 at 07:16 +0000, Peng Fan wrote:
> > > +static int __init of_clk_drop_stdout_clocks(void) {
> > > +	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > > +		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > > +		clk_put(of_clk_stdout_clks.clks[i]);
> > > +	}
> > > +
> > > +	kfree(of_clk_stdout_clks.clks);
> > > +
> > > +	/*
> > > +	 * Do not try to acquire stdout clocks after late initcalls, e.g.
> > > +	 * during further module loading, as we then wouldn't have a
> > > way to
> > > +	 * drop the references (and associated allocations) ever again.
> > > +	 */
> > > +	of_clk_stdout_clks.bump_refs = false;
> > > +
> > > +	return 0;
> > > +}
> > > +late_initcall_sync(of_clk_drop_stdout_clocks);
> >
> > If the uart driver is built as module, this might break earlycon.
> > Before uart driver loaded, clk disabled per my understanding.
> 
> You're right.
> 
> With this in mind, I'm not sure then if a generic solution is possible...
> 
> I guess it has to be duplicated into the platforms after all and platforms
> can enable this if they opt to disallow uart as module?
> 
> Any other suggestions?

I not have good idea here. Put of_clk_drop_stdout_clocks after
bootconsole disabled might be ok.

Regards,
Peng.

> 
> > > +
> > >  /**
> > >   * struct of_clk_provider - Clock provider registration structure
> > >   * @link: Entry in global list of clock providers @@ -5031,6
> > > +5156,8 @@ int of_clk_add_provider(struct device_node *np,
> > >
> > >  	fwnode_dev_initialized(&np->fwnode, true);
> > >
> > > +	of_clk_bump_stdout_clocks();
> > > +
> > >  	return ret;
> > >  }
> > >  EXPORT_SYMBOL_GPL(of_clk_add_provider);
> > > @@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct
> device_node
> > > *np,
> > >
> > >  	fwnode_dev_initialized(&np->fwnode, true);
> > >
> > > +	of_clk_bump_stdout_clocks();
> >
> > If clock driver is built as module,  the will make the clocks will be
> > always enabled, if my understanding is correct.
> 
> until late_initcall_sync(), at which point it'll be disabled before the uart
> driver has probed, yes :-(
> 
> Cheers,
> Andre'