[PATCH v2 0/3] clk: socfpga: register the SP timer clocks early

[PATCH v2 0/3] clk: socfpga: register the SP timer clocks early

[Adrian Ng Ho Yin]

The DW APB timers on Stratix10, Agilex (including eASIC N5X) and Agilex5
are probed through TIMER_OF_DECLARE() from time_init(), but their clock,
l4_sp_clk, comes from a clock manager that is a platform driver
registered at core_initcall(). The timer's clk_get() returns
-EPROBE_DEFER, which timer_probe() does not report, and the timer is
never registered because TIMER_OF_DECLARE() callbacks are not retried.

Rather than moving the whole clock manager to CLK_OF_DECLARE() as v1 did,
this version keeps the platform driver and, following
clk-mt8173-infracfg.c, uses CLK_OF_DECLARE_DRIVER() to register only
l4_sp_clk and the clocks its rate depends on from of_clk_init():

  Agilex/N5X: boot_clk, main_pll, periph_pll, main_pll_c1, peri_pll_c1,
              noc_free_clk, l4_sp_clk
  Agilex5:    boot_clk, main_pll, periph_pll, main_pll_c3, peri_pll_c1,
              noc_free_clk, l4_sp_clk
  Stratix10:  boot_clk, main_pll, periph_pll, main_noc_base_clk,
              peri_noc_base_clk, noc_free_clk, l4_sp_clk

All other clock IDs return -EPROBE_DEFER until the platform driver probes,
registers the remaining clocks into the same provider and turns any
unused IDs into -ENOENT. If the early registration fails, probe falls
back to registering everything as before.

Boot-tested on Agilex7 (FM61), Stratix10 L-tile and Agilex5 (065B Premium
devkit), comparing each board with and without this series:

  - with the series, timer1 is registered as a clocksource within 0.5 ms
    of boot and timer0 as the broadcast clockevent device; without it,
    neither is registered and bc_hrtimer is used for broadcast instead
  - the clock manager still binds as a platform device, no devices are
    left deferred, and clocks registered at probe (EMAC, SD/MMC, USB,
    DMA, watchdog) have their expected rates and consumers

eASIC N5X is compile-tested only.

Changes in v2:
- Keep the clock managers as platform drivers instead of converting
  them to CLK_OF_DECLARE() (Brian)
- Register only l4_sp_clk and its parents early, using
  CLK_OF_DECLARE_DRIVER() as in clk-mt8173-infracfg.c, instead of the
  whole controller (Jerome)
- Return the result of of_clk_add_hw_provider() from probe
- Update subjects and commit messages to match the new approach

v1: https://lore.kernel.org/all/cover.1789096458.git.adrian.ho.yin.ng@altera.com/

Adrian Ng Ho Yin (3):
  clk: socfpga: agilex: register the SP timer clocks early
  clk: socfpga: agilex5: register the SP timer clocks early
  clk: socfpga: stratix10: register the SP timer clocks early

 drivers/clk/socfpga/clk-agilex.c  | 222 +++++++++++++++++++++++-------
 drivers/clk/socfpga/clk-agilex5.c | 138 +++++++++++++++----
 drivers/clk/socfpga/clk-s10.c     | 133 ++++++++++++++----
 3 files changed, 385 insertions(+), 108 deletions(-)


base-commit: 1e7f56205813a2c48cdb3e9a4b0a24f49fd9a548
-- 
2.49.GIT

[PATCH v2 1/3] clk: socfpga: agilex: register the SP timer clocks early

[Adrian Ng Ho Yin]

The DW APB timers are probed through TIMER_OF_DECLARE() from time_init()
and cannot defer, but the clock manager is a platform driver registered
at core_initcall(). The timer's clk_get() therefore fails and the timer
is never brought up.

Register only l4_sp_clk and its parents (boot_clk, main_pll, periph_pll,
main_pll_c1, peri_pll_c1 and noc_free_clk) from of_clk_init() using
CLK_OF_DECLARE_DRIVER(), with every other clock left as -EPROBE_DEFER.
The remaining clocks are still registered when the platform driver
probes, which reuses the early provider.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/clk/socfpga/clk-agilex.c | 222 +++++++++++++++++++++++--------
 1 file changed, 169 insertions(+), 53 deletions(-)

diff --git a/drivers/clk/socfpga/clk-agilex.c b/drivers/clk/socfpga/clk-agilex.c
index 2bdea1997b5e..91a80b7b1886 100644
--- a/drivers/clk/socfpga/clk-agilex.c
+++ b/drivers/clk/socfpga/clk-agilex.c
@@ -4,7 +4,9 @@
  */
 #include <linux/slab.h>
 #include <linux/clk-provider.h>
+#include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 
 #include <dt-bindings/clock/agilex-clock.h>
@@ -232,24 +234,49 @@ static const struct stratix10_pll_clock agilex_pll_clks[] = {
 	  0, 0x9c},
 };
 
+/*
+ * The SP timers are probed from time_init() and cannot defer, so their
+ * l4_sp_clk and its parents are registered from of_clk_init(). Everything
+ * else is registered when the platform driver probes.
+ */
+static const struct n5x_perip_c_clock n5x_early_perip_c_clks[] = {
+	{ AGILEX_MAIN_PLL_C1_CLK, "main_pll_c1", "main_pll", NULL, 1, 0, 0x54, 8},
+	{ AGILEX_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0, 0xA8, 8},
+};
+
+static const struct stratix10_perip_c_clock agilex_early_perip_c_clks[] = {
+	{ AGILEX_MAIN_PLL_C1_CLK, "main_pll_c1", "main_pll", NULL, 1, 0, 0x5C},
+	{ AGILEX_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0, 0xB0},
+};
+
+static const struct stratix10_perip_cnt_clock agilex_early_perip_cnt_clks[] = {
+	{ AGILEX_NOC_FREE_CLK, "noc_free_clk", NULL, noc_free_mux, ARRAY_SIZE(noc_free_mux),
+	  0, 0x40, 0, 0, 0},
+};
+
+static const struct stratix10_gate_clock agilex_early_gate_clks[] = {
+	/*
+	 * The l4_sp_clk feeds a 100 MHz clock to various peripherals, one of them
+	 * being the SP timers, thus cannot get gated.
+	 */
+	{ AGILEX_L4_SP_CLK, "l4_sp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), CLK_IS_CRITICAL, 0x24,
+	  3, 0x44, 16, 2, 0x30, 1, 0},
+};
+
 static const struct n5x_perip_c_clock n5x_main_perip_c_clks[] = {
 	{ AGILEX_MAIN_PLL_C0_CLK, "main_pll_c0", "main_pll", NULL, 1, 0, 0x54, 0},
-	{ AGILEX_MAIN_PLL_C1_CLK, "main_pll_c1", "main_pll", NULL, 1, 0, 0x54, 8},
 	{ AGILEX_MAIN_PLL_C2_CLK, "main_pll_c2", "main_pll", NULL, 1, 0, 0x54, 16},
 	{ AGILEX_MAIN_PLL_C3_CLK, "main_pll_c3", "main_pll", NULL, 1, 0, 0x54, 24},
 	{ AGILEX_PERIPH_PLL_C0_CLK, "peri_pll_c0", "periph_pll", NULL, 1, 0, 0xA8, 0},
-	{ AGILEX_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0, 0xA8, 8},
 	{ AGILEX_PERIPH_PLL_C2_CLK, "peri_pll_c2", "periph_pll", NULL, 1, 0, 0xA8, 16},
 	{ AGILEX_PERIPH_PLL_C3_CLK, "peri_pll_c3", "periph_pll", NULL, 1, 0, 0xA8, 24},
 };
 
 static const struct stratix10_perip_c_clock agilex_main_perip_c_clks[] = {
 	{ AGILEX_MAIN_PLL_C0_CLK, "main_pll_c0", "main_pll", NULL, 1, 0, 0x58},
-	{ AGILEX_MAIN_PLL_C1_CLK, "main_pll_c1", "main_pll", NULL, 1, 0, 0x5C},
 	{ AGILEX_MAIN_PLL_C2_CLK, "main_pll_c2", "main_pll", NULL, 1, 0, 0x64},
 	{ AGILEX_MAIN_PLL_C3_CLK, "main_pll_c3", "main_pll", NULL, 1, 0, 0x68},
 	{ AGILEX_PERIPH_PLL_C0_CLK, "peri_pll_c0", "periph_pll", NULL, 1, 0, 0xAC},
-	{ AGILEX_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0, 0xB0},
 	{ AGILEX_PERIPH_PLL_C2_CLK, "peri_pll_c2", "periph_pll", NULL, 1, 0, 0xB8},
 	{ AGILEX_PERIPH_PLL_C3_CLK, "peri_pll_c3", "periph_pll", NULL, 1, 0, 0xBC},
 };
@@ -257,8 +284,6 @@ static const struct stratix10_perip_c_clock agilex_main_perip_c_clks[] = {
 static const struct stratix10_perip_cnt_clock agilex_main_perip_cnt_clks[] = {
 	{ AGILEX_MPU_FREE_CLK, "mpu_free_clk", NULL, mpu_free_mux, ARRAY_SIZE(mpu_free_mux),
 	   0, 0x3C, 0, 0, 0},
-	{ AGILEX_NOC_FREE_CLK, "noc_free_clk", NULL, noc_free_mux, ARRAY_SIZE(noc_free_mux),
-	  0, 0x40, 0, 0, 0},
 	{ AGILEX_L3_MAIN_FREE_CLK, "l3_main_free_clk", "noc_free_clk", NULL,
 	  1, 0, 0, 1, 0, 0},
 	{ AGILEX_L4_SYS_FREE_CLK, "l4_sys_free_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0,
@@ -292,12 +317,6 @@ static const struct stratix10_gate_clock agilex_gate_clks[] = {
 	  1, 0x44, 0, 2, 0x30, 1, 0},
 	{ AGILEX_L4_MP_CLK, "l4_mp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x24,
 	  2, 0x44, 8, 2, 0x30, 1, 0},
-	/*
-	 * The l4_sp_clk feeds a 100 MHz clock to various peripherals, one of them
-	 * being the SP timers, thus cannot get gated.
-	 */
-	{ AGILEX_L4_SP_CLK, "l4_sp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), CLK_IS_CRITICAL, 0x24,
-	  3, 0x44, 16, 2, 0x30, 1, 0},
 	{ AGILEX_CS_AT_CLK, "cs_at_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x24,
 	  4, 0x44, 24, 2, 0x30, 1, 0},
 	{ AGILEX_CS_TRACE_CLK, "cs_trace_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x24,
@@ -454,32 +473,148 @@ static int n5x_clk_register_pll(const struct stratix10_pll_clock *clks,
 	return 0;
 }
 
-static int agilex_clkmgr_init(struct platform_device *pdev)
+static struct stratix10_clock_data *agilex_clk_data;
+
+static struct stratix10_clock_data *agilex_clk_data_alloc(void __iomem *base)
+{
+	struct stratix10_clock_data *clk_data;
+	int i;
+
+	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, AGILEX_NUM_CLKS),
+			   GFP_KERNEL);
+	if (!clk_data)
+		return NULL;
+
+	clk_data->base = base;
+	clk_data->clk_data.num = AGILEX_NUM_CLKS;
+
+	for (i = 0; i < AGILEX_NUM_CLKS; i++)
+		clk_data->clk_data.hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+	return clk_data;
+}
+
+static void agilex_clk_register_early(struct stratix10_clock_data *clk_data)
+{
+	agilex_clk_register_pll(agilex_pll_clks, ARRAY_SIZE(agilex_pll_clks), clk_data);
+
+	agilex_clk_register_c_perip(agilex_early_perip_c_clks,
+				    ARRAY_SIZE(agilex_early_perip_c_clks), clk_data);
+
+	agilex_clk_register_cnt_perip(agilex_early_perip_cnt_clks,
+				      ARRAY_SIZE(agilex_early_perip_cnt_clks),
+				      clk_data);
+
+	agilex_clk_register_gate(agilex_early_gate_clks,
+				 ARRAY_SIZE(agilex_early_gate_clks), clk_data);
+}
+
+static void n5x_clk_register_early(struct stratix10_clock_data *clk_data)
+{
+	n5x_clk_register_pll(agilex_pll_clks, ARRAY_SIZE(agilex_pll_clks), clk_data);
+
+	n5x_clk_register_c_perip(n5x_early_perip_c_clks,
+				 ARRAY_SIZE(n5x_early_perip_c_clks), clk_data);
+
+	agilex_clk_register_cnt_perip(agilex_early_perip_cnt_clks,
+				      ARRAY_SIZE(agilex_early_perip_cnt_clks),
+				      clk_data);
+
+	agilex_clk_register_gate(agilex_early_gate_clks,
+				 ARRAY_SIZE(agilex_early_gate_clks), clk_data);
+}
+
+static void __init
+agilex_clkmgr_of_init(struct device_node *np,
+		      void (*register_early)(struct stratix10_clock_data *))
+{
+	struct stratix10_clock_data *clk_data;
+	void __iomem *base;
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_err("%s: failed to map clock registers\n", __func__);
+		return;
+	}
+
+	clk_data = agilex_clk_data_alloc(base);
+	if (!clk_data) {
+		iounmap(base);
+		return;
+	}
+
+	register_early(clk_data);
+
+	if (of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data)) {
+		pr_err("%s: failed to add clock provider\n", __func__);
+		return;
+	}
+
+	agilex_clk_data = clk_data;
+}
+
+static void __init agilex_clkmgr_early_init(struct device_node *np)
+{
+	agilex_clkmgr_of_init(np, agilex_clk_register_early);
+}
+
+CLK_OF_DECLARE_DRIVER(agilex_clkmgr, "intel,agilex-clkmgr",
+		      agilex_clkmgr_early_init);
+
+static void __init n5x_clkmgr_early_init(struct device_node *np)
+{
+	agilex_clkmgr_of_init(np, n5x_clk_register_early);
+}
+
+CLK_OF_DECLARE_DRIVER(n5x_clkmgr, "intel,easic-n5x-clkmgr",
+		      n5x_clkmgr_early_init);
+
+static struct stratix10_clock_data *
+agilex_clkmgr_get_clk_data(struct platform_device *pdev,
+			   void (*register_early)(struct stratix10_clock_data *))
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
 	struct stratix10_clock_data *clk_data;
 	void __iomem *base;
-	int i, num_clks;
+
+	if (agilex_clk_data)
+		return agilex_clk_data;
 
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	num_clks = AGILEX_NUM_CLKS;
+		return ERR_CAST(base);
 
-	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
-				num_clks), GFP_KERNEL);
+	clk_data = agilex_clk_data_alloc(base);
 	if (!clk_data)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
-	clk_data->clk_data.num = num_clks;
-	clk_data->base = base;
+	register_early(clk_data);
 
-	for (i = 0; i < num_clks; i++)
-		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+	return clk_data;
+}
 
-	agilex_clk_register_pll(agilex_pll_clks, ARRAY_SIZE(agilex_pll_clks), clk_data);
+static int agilex_clkmgr_add_provider(struct platform_device *pdev,
+				      struct stratix10_clock_data *clk_data)
+{
+	int i;
+
+	for (i = 0; i < AGILEX_NUM_CLKS; i++)
+		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
+			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+
+	if (clk_data == agilex_clk_data)
+		return 0;
+
+	return of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get,
+				      &clk_data->clk_data);
+}
+
+static int agilex_clkmgr_init(struct platform_device *pdev)
+{
+	struct stratix10_clock_data *clk_data;
+
+	clk_data = agilex_clkmgr_get_clk_data(pdev, agilex_clk_register_early);
+	if (IS_ERR(clk_data))
+		return PTR_ERR(clk_data);
 
 	agilex_clk_register_c_perip(agilex_main_perip_c_clks,
 				 ARRAY_SIZE(agilex_main_perip_c_clks), clk_data);
@@ -490,36 +625,17 @@ static int agilex_clkmgr_init(struct platform_device *pdev)
 
 	agilex_clk_register_gate(agilex_gate_clks, ARRAY_SIZE(agilex_gate_clks),
 			      clk_data);
-	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
-	return 0;
+
+	return agilex_clkmgr_add_provider(pdev, clk_data);
 }
 
 static int n5x_clkmgr_init(struct platform_device *pdev)
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
 	struct stratix10_clock_data *clk_data;
-	void __iomem *base;
-	int i, num_clks;
-
-	base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	num_clks = AGILEX_NUM_CLKS;
-
-	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
-				num_clks), GFP_KERNEL);
-	if (!clk_data)
-		return -ENOMEM;
-
-	clk_data->base = base;
-	clk_data->clk_data.num = num_clks;
 
-	for (i = 0; i < num_clks; i++)
-		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
-
-	n5x_clk_register_pll(agilex_pll_clks, ARRAY_SIZE(agilex_pll_clks), clk_data);
+	clk_data = agilex_clkmgr_get_clk_data(pdev, n5x_clk_register_early);
+	if (IS_ERR(clk_data))
+		return PTR_ERR(clk_data);
 
 	n5x_clk_register_c_perip(n5x_main_perip_c_clks,
 				 ARRAY_SIZE(n5x_main_perip_c_clks), clk_data);
@@ -530,8 +646,8 @@ static int n5x_clkmgr_init(struct platform_device *pdev)
 
 	agilex_clk_register_gate(agilex_gate_clks, ARRAY_SIZE(agilex_gate_clks),
 			      clk_data);
-	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
-	return 0;
+
+	return agilex_clkmgr_add_provider(pdev, clk_data);
 }
 
 static int agilex_clkmgr_probe(struct platform_device *pdev)
-- 
2.49.GIT

[PATCH v2 2/3] clk: socfpga: agilex5: register the SP timer clocks early

[Adrian Ng Ho Yin]

The DW APB timers are probed through TIMER_OF_DECLARE() from time_init()
and cannot defer, but the clock manager is a platform driver registered
at core_initcall(). The timer's clk_get() therefore fails and the timer
is never brought up.

Register only l4_sp_clk and its parents (boot_clk, main_pll, periph_pll,
main_pll_c3, peri_pll_c1 and noc_free_clk) from of_clk_init() using
CLK_OF_DECLARE_DRIVER(), with every other clock left as -EPROBE_DEFER.
The remaining clocks are still registered when the platform driver
probes, which reuses the early provider.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/clk/socfpga/clk-agilex5.c | 138 +++++++++++++++++++++++-------
 1 file changed, 109 insertions(+), 29 deletions(-)

diff --git a/drivers/clk/socfpga/clk-agilex5.c b/drivers/clk/socfpga/clk-agilex5.c
index f7f0ad884f64..e439950dd6da 100644
--- a/drivers/clk/socfpga/clk-agilex5.c
+++ b/drivers/clk/socfpga/clk-agilex5.c
@@ -5,7 +5,9 @@
  */
 #include <linux/slab.h>
 #include <linux/clk-provider.h>
+#include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include  <dt-bindings/clock/intel,agilex5-clkmgr.h>
 #include "stratix10-clk.h"
@@ -233,7 +235,29 @@ static const struct agilex5_pll_clock agilex5_pll_clks[] = {
 	},
 };
 
-/* Main PLL C0, C1, C2, C3 and Peri PLL C0, C1, C2, C3. With ping-pong counter. */
+/*
+ * The SP timers are probed from time_init() and cannot defer, so their
+ * l4_sp_clk and its parents are registered from of_clk_init(). Everything
+ * else is registered when the platform driver probes.
+ */
+static const struct stratix10_perip_c_clock agilex5_early_perip_c_clks[] = {
+	{ AGILEX5_MAIN_PLL_C3_CLK, "main_pll_c3", "main_pll", NULL, 1, 0,
+	  0x68 },
+	{ AGILEX5_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0,
+	  0xB4 },
+};
+
+static const struct agilex5_perip_cnt_clock agilex5_early_perip_cnt_clks[] = {
+	{ AGILEX5_NOC_FREE_CLK, "noc_free_clk", noc_free_mux,
+	ARRAY_SIZE(noc_free_mux), 0, 0x40, 0, 0, 0 },
+};
+
+static const struct agilex5_gate_clock agilex5_early_gate_clks[] = {
+	{ AGILEX5_L4_SP_CLK, "l4_sp_clk", noc_mux, ARRAY_SIZE(noc_mux),
+	  CLK_IS_CRITICAL, 0x24, 3, 0x44, 6, 2, 0x30, 1, 0 },
+};
+
+/* Main PLL C0, C1, C2 and Peri PLL C0, C2, C3. With ping-pong counter. */
 static const struct stratix10_perip_c_clock agilex5_main_perip_c_clks[] = {
 	{ AGILEX5_MAIN_PLL_C0_CLK, "main_pll_c0", "main_pll", NULL, 1, 0,
 	  0x5C },
@@ -241,12 +265,8 @@ static const struct stratix10_perip_c_clock agilex5_main_perip_c_clks[] = {
 	  0x60 },
 	{ AGILEX5_MAIN_PLL_C2_CLK, "main_pll_c2", "main_pll", NULL, 1, 0,
 	  0x64 },
-	{ AGILEX5_MAIN_PLL_C3_CLK, "main_pll_c3", "main_pll", NULL, 1, 0,
-	  0x68 },
 	{ AGILEX5_PERIPH_PLL_C0_CLK, "peri_pll_c0", "periph_pll", NULL, 1, 0,
 	  0xB0 },
-	{ AGILEX5_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0,
-	  0xB4 },
 	{ AGILEX5_PERIPH_PLL_C2_CLK, "peri_pll_c2", "periph_pll", NULL, 1, 0,
 	  0xB8 },
 	{ AGILEX5_PERIPH_PLL_C3_CLK, "peri_pll_c3", "periph_pll", NULL, 1, 0,
@@ -265,8 +285,6 @@ static const struct agilex5_perip_cnt_clock agilex5_main_perip_cnt_clks[] = {
 	ARRAY_SIZE(core3_free_mux), 0, 0x0110, 0, 0, 0},
 	{ AGILEX5_DSU_FREE_CLK, "dsu_free_clk", dsu_free_mux,
 	ARRAY_SIZE(dsu_free_mux), 0, 0xfc, 0, 0, 0},
-	{ AGILEX5_NOC_FREE_CLK, "noc_free_clk", noc_free_mux,
-	ARRAY_SIZE(noc_free_mux), 0, 0x40, 0, 0, 0 },
 	{ AGILEX5_EMAC_A_FREE_CLK, "emaca_free_clk", emaca_free_mux,
 	ARRAY_SIZE(emaca_free_mux), 0, 0xD4, 0, 0x88, 0 },
 	{ AGILEX5_EMAC_B_FREE_CLK, "emacb_free_clk", emacb_free_mux,
@@ -313,8 +331,6 @@ static const struct agilex5_gate_clock agilex5_gate_clks[] = {
 	  0x24, 2, 0x44, 4, 2, 0x30, 1, 0 },
 	{ AGILEX5_L4_SYS_FREE_CLK, "l4_sys_free_clk", noc_mux,
 	  ARRAY_SIZE(noc_mux), 0, 0, 0, 0x44, 2, 2, 0x30, 1, 0 },
-	{ AGILEX5_L4_SP_CLK, "l4_sp_clk", noc_mux, ARRAY_SIZE(noc_mux),
-	  CLK_IS_CRITICAL, 0x24, 3, 0x44, 6, 2, 0x30, 1, 0 },
 
 	/* Core sight clocks*/
 	{ AGILEX5_CS_AT_CLK, "cs_at_clk", noc_mux, ARRAY_SIZE(noc_mux), 0,
@@ -486,35 +502,92 @@ static int agilex5_clk_register_pll(const struct agilex5_pll_clock *clks,
 	return 0;
 }
 
-static int agilex5_clkmgr_init(struct platform_device *pdev)
+static struct stratix10_clock_data *agilex5_clk_data;
+
+static struct stratix10_clock_data *agilex5_clk_data_alloc(void __iomem *base)
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
 	struct stratix10_clock_data *clk_data;
-	void __iomem *base;
-	int i, num_clks;
-
-	base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	num_clks = AGILEX5_NUM_CLKS;
+	int i;
 
-	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
-				num_clks), GFP_KERNEL);
+	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, AGILEX5_NUM_CLKS),
+			   GFP_KERNEL);
 	if (!clk_data)
-		return -ENOMEM;
+		return NULL;
 
 	clk_data->base = base;
-	clk_data->clk_data.num = num_clks;
+	clk_data->clk_data.num = AGILEX5_NUM_CLKS;
+
+	for (i = 0; i < AGILEX5_NUM_CLKS; i++)
+		clk_data->clk_data.hws[i] = ERR_PTR(-EPROBE_DEFER);
 
-	for (i = 0; i < num_clks; i++)
-		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+	return clk_data;
+}
 
+static void agilex5_clk_register_early(struct stratix10_clock_data *clk_data)
+{
 	agilex5_clk_register_pll(agilex5_pll_clks, ARRAY_SIZE(agilex5_pll_clks),
 				 clk_data);
 
-	/* mainPLL C0, C1, C2, C3 and periph PLL C0, C1, C2, C3*/
+	agilex5_clk_register_c_perip(agilex5_early_perip_c_clks,
+				     ARRAY_SIZE(agilex5_early_perip_c_clks),
+				     clk_data);
+
+	agilex5_clk_register_cnt_perip(agilex5_early_perip_cnt_clks,
+				       ARRAY_SIZE(agilex5_early_perip_cnt_clks),
+				       clk_data);
+
+	agilex5_clk_register_gate(agilex5_early_gate_clks,
+				  ARRAY_SIZE(agilex5_early_gate_clks), clk_data);
+}
+
+static void __init agilex5_clkmgr_early_init(struct device_node *np)
+{
+	struct stratix10_clock_data *clk_data;
+	void __iomem *base;
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_err("%s: failed to map clock registers\n", __func__);
+		return;
+	}
+
+	clk_data = agilex5_clk_data_alloc(base);
+	if (!clk_data) {
+		iounmap(base);
+		return;
+	}
+
+	agilex5_clk_register_early(clk_data);
+
+	if (of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data)) {
+		pr_err("%s: failed to add clock provider\n", __func__);
+		return;
+	}
+
+	agilex5_clk_data = clk_data;
+}
+
+CLK_OF_DECLARE_DRIVER(agilex5_clkmgr, "intel,agilex5-clkmgr",
+		      agilex5_clkmgr_early_init);
+
+static int agilex5_clkmgr_init(struct platform_device *pdev)
+{
+	struct stratix10_clock_data *clk_data = agilex5_clk_data;
+	void __iomem *base;
+	int i;
+
+	if (!clk_data) {
+		base = devm_platform_ioremap_resource(pdev, 0);
+		if (IS_ERR(base))
+			return PTR_ERR(base);
+
+		clk_data = agilex5_clk_data_alloc(base);
+		if (!clk_data)
+			return -ENOMEM;
+
+		agilex5_clk_register_early(clk_data);
+	}
+
 	agilex5_clk_register_c_perip(agilex5_main_perip_c_clks,
 				     ARRAY_SIZE(agilex5_main_perip_c_clks),
 				     clk_data);
@@ -526,8 +599,15 @@ static int agilex5_clkmgr_init(struct platform_device *pdev)
 	agilex5_clk_register_gate(agilex5_gate_clks,
 				  ARRAY_SIZE(agilex5_gate_clks), clk_data);
 
-	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
-	return 0;
+	for (i = 0; i < AGILEX5_NUM_CLKS; i++)
+		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
+			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+
+	if (clk_data == agilex5_clk_data)
+		return 0;
+
+	return of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get,
+				      &clk_data->clk_data);
 }
 
 static int agilex5_clkmgr_probe(struct platform_device *pdev)
-- 
2.49.GIT

[PATCH v2 3/3] clk: socfpga: stratix10: register the SP timer clocks early

[Adrian Ng Ho Yin]

The DW APB timers are probed through TIMER_OF_DECLARE() from time_init()
and cannot defer, but the clock manager is a platform driver registered
at core_initcall(). The timer's clk_get() therefore fails and the timer
is never brought up.

Register only l4_sp_clk and its parents (boot_clk, main_pll, periph_pll,
main_noc_base_clk, peri_noc_base_clk and noc_free_clk) from
of_clk_init() using CLK_OF_DECLARE_DRIVER(), with every other clock left
as -EPROBE_DEFER. The remaining clocks are still registered when the
platform driver probes, which reuses the early provider.

Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
---
 drivers/clk/socfpga/clk-s10.c | 133 +++++++++++++++++++++++++++-------
 1 file changed, 107 insertions(+), 26 deletions(-)

diff --git a/drivers/clk/socfpga/clk-s10.c b/drivers/clk/socfpga/clk-s10.c
index b4bf4e2d38e1..7eae2dfa408d 100644
--- a/drivers/clk/socfpga/clk-s10.c
+++ b/drivers/clk/socfpga/clk-s10.c
@@ -4,7 +4,9 @@
  */
 #include <linux/slab.h>
 #include <linux/clk-provider.h>
+#include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 
 #include <dt-bindings/clock/stratix10-clock.h>
@@ -188,20 +190,36 @@ static const struct stratix10_pll_clock s10_pll_clks[] = {
 	  0, 0xe4},
 };
 
+/*
+ * The SP timers are probed from time_init() and cannot defer, so their
+ * l4_sp_clk and its parents are registered from of_clk_init(). Everything
+ * else is registered when the platform driver probes.
+ */
+static const struct stratix10_perip_c_clock s10_early_perip_c_clks[] = {
+	{ STRATIX10_MAIN_NOC_BASE_CLK, "main_noc_base_clk", "main_pll", NULL, 1, 0, 0x88},
+	{ STRATIX10_PERI_NOC_BASE_CLK, "peri_noc_base_clk", "periph_pll", NULL, 1, 0,
+	  0xF8},
+};
+
+static const struct stratix10_perip_cnt_clock s10_early_perip_cnt_clks[] = {
+	{ STRATIX10_NOC_FREE_CLK, "noc_free_clk", NULL, noc_free_mux, ARRAY_SIZE(noc_free_mux),
+	  0, 0x4C, 0, 0x3C, 1},
+};
+
+static const struct stratix10_gate_clock s10_early_gate_clks[] = {
+	{ STRATIX10_L4_SP_CLK, "l4_sp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux),
+	  CLK_IS_CRITICAL, 0x30, 3, 0x70, 16, 2, 0x3C, 1, 0},
+};
+
 static const struct stratix10_perip_c_clock s10_main_perip_c_clks[] = {
 	{ STRATIX10_MAIN_MPU_BASE_CLK, "main_mpu_base_clk", "main_pll", NULL, 1, 0, 0x84},
-	{ STRATIX10_MAIN_NOC_BASE_CLK, "main_noc_base_clk", "main_pll", NULL, 1, 0, 0x88},
 	{ STRATIX10_PERI_MPU_BASE_CLK, "peri_mpu_base_clk", "periph_pll", NULL, 1, 0,
 	  0xF4},
-	{ STRATIX10_PERI_NOC_BASE_CLK, "peri_noc_base_clk", "periph_pll", NULL, 1, 0,
-	  0xF8},
 };
 
 static const struct stratix10_perip_cnt_clock s10_main_perip_cnt_clks[] = {
 	{ STRATIX10_MPU_FREE_CLK, "mpu_free_clk", NULL, mpu_free_mux, ARRAY_SIZE(mpu_free_mux),
 	   0, 0x48, 0, 0, 0},
-	{ STRATIX10_NOC_FREE_CLK, "noc_free_clk", NULL, noc_free_mux, ARRAY_SIZE(noc_free_mux),
-	  0, 0x4C, 0, 0x3C, 1},
 	{ STRATIX10_MAIN_EMACA_CLK, "main_emaca_clk", "main_noc_base_clk", NULL, 1, 0,
 	  0x50, 0, 0, 0},
 	{ STRATIX10_MAIN_EMACB_CLK, "main_emacb_clk", "main_noc_base_clk", NULL, 1, 0,
@@ -263,8 +281,6 @@ static const struct stratix10_gate_clock s10_gate_clks[] = {
 	  1, 0x70, 0, 2, 0x3C, 1, 0},
 	{ STRATIX10_L4_MP_CLK, "l4_mp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x30,
 	  2, 0x70, 8, 2, 0x3C, 1, 0},
-	{ STRATIX10_L4_SP_CLK, "l4_sp_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), CLK_IS_CRITICAL, 0x30,
-	  3, 0x70, 16, 2, 0x3C, 1, 0},
 	{ STRATIX10_CS_AT_CLK, "cs_at_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x30,
 	  4, 0x70, 24, 2, 0x3C, 1, 0},
 	{ STRATIX10_CS_TRACE_CLK, "cs_trace_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux), 0, 0x30,
@@ -382,33 +398,91 @@ static int s10_clk_register_pll(const struct stratix10_pll_clock *clks,
 	return 0;
 }
 
-static int s10_clkmgr_init(struct platform_device *pdev)
+static struct stratix10_clock_data *s10_clk_data;
+
+static struct stratix10_clock_data *s10_clk_data_alloc(void __iomem *base)
+{
+	struct stratix10_clock_data *clk_data;
+	int i;
+
+	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, STRATIX10_NUM_CLKS),
+			   GFP_KERNEL);
+	if (!clk_data)
+		return NULL;
+
+	clk_data->base = base;
+	clk_data->clk_data.num = STRATIX10_NUM_CLKS;
+
+	for (i = 0; i < STRATIX10_NUM_CLKS; i++)
+		clk_data->clk_data.hws[i] = ERR_PTR(-EPROBE_DEFER);
+
+	return clk_data;
+}
+
+static void s10_clk_register_early(struct stratix10_clock_data *clk_data)
+{
+	s10_clk_register_pll(s10_pll_clks, ARRAY_SIZE(s10_pll_clks), clk_data);
+
+	s10_clk_register_c_perip(s10_early_perip_c_clks,
+				 ARRAY_SIZE(s10_early_perip_c_clks), clk_data);
+
+	s10_clk_register_cnt_perip(s10_early_perip_cnt_clks,
+				   ARRAY_SIZE(s10_early_perip_cnt_clks),
+				   clk_data);
+
+	s10_clk_register_gate(s10_early_gate_clks,
+			      ARRAY_SIZE(s10_early_gate_clks), clk_data);
+}
+
+static void __init s10_clkmgr_early_init(struct device_node *np)
 {
-	struct device_node *np = pdev->dev.of_node;
-	struct device *dev = &pdev->dev;
 	struct stratix10_clock_data *clk_data;
 	void __iomem *base;
-	int i, num_clks;
 
-	base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(base)) {
+	base = of_iomap(np, 0);
+	if (!base) {
 		pr_err("%s: failed to map clock registers\n", __func__);
-		return PTR_ERR(base);
+		return;
 	}
 
-	num_clks = STRATIX10_NUM_CLKS;
-	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
-						 num_clks), GFP_KERNEL);
-	if (!clk_data)
-		return -ENOMEM;
+	clk_data = s10_clk_data_alloc(base);
+	if (!clk_data) {
+		iounmap(base);
+		return;
+	}
 
-	clk_data->base = base;
-	clk_data->clk_data.num = num_clks;
+	s10_clk_register_early(clk_data);
 
-	for (i = 0; i < num_clks; i++)
-		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+	if (of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data)) {
+		pr_err("%s: failed to add clock provider\n", __func__);
+		return;
+	}
 
-	s10_clk_register_pll(s10_pll_clks, ARRAY_SIZE(s10_pll_clks), clk_data);
+	s10_clk_data = clk_data;
+}
+
+CLK_OF_DECLARE_DRIVER(stratix10_clkmgr, "intel,stratix10-clkmgr",
+		      s10_clkmgr_early_init);
+
+static int s10_clkmgr_init(struct platform_device *pdev)
+{
+	struct stratix10_clock_data *clk_data = s10_clk_data;
+	void __iomem *base;
+	int i;
+
+	if (!clk_data) {
+		base = devm_platform_ioremap_resource(pdev, 0);
+		if (IS_ERR(base)) {
+			pr_err("%s: failed to map clock registers\n", __func__);
+			return PTR_ERR(base);
+		}
+
+		clk_data = s10_clk_data_alloc(base);
+		if (!clk_data)
+			return -ENOMEM;
+
+		s10_clk_register_early(clk_data);
+	}
 
 	s10_clk_register_c_perip(s10_main_perip_c_clks,
 				 ARRAY_SIZE(s10_main_perip_c_clks), clk_data);
@@ -420,8 +494,15 @@ static int s10_clkmgr_init(struct platform_device *pdev)
 	s10_clk_register_gate(s10_gate_clks, ARRAY_SIZE(s10_gate_clks),
 			      clk_data);
 
-	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
-	return 0;
+	for (i = 0; i < STRATIX10_NUM_CLKS; i++)
+		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
+			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
+
+	if (clk_data == s10_clk_data)
+		return 0;
+
+	return of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get,
+				      &clk_data->clk_data);
 }
 
 static int s10_clkmgr_probe(struct platform_device *pdev)
-- 
2.49.GIT

Re: [PATCH v2 2/3] clk: socfpga: agilex5: register the SP timer clocks early

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: socfpga: Use-After-Free of IOMEM on probe failure
- [High] clk: socfpga: loss of early clocks on early provider failure
- [Medium] clk: socfpga: memory and iomem leaks on init error paths
--

commit 41c265642a4fb3df23199b307d15dbfdc7f70409
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Subject: clk: socfpga: agilex5: register the SP timer clocks early

This commit registers the SP timer clocks early from of_clk_init() because
they are probed from time_init() and cannot defer. The remaining clocks are
registered when the platform driver probes, reusing the early provider.

> diff --git a/drivers/clk/socfpga/clk-agilex5.c b/drivers/clk/socfpga/clk-agilex5.c
> index f7f0ad884f645..e439950dd6da7 100644
> --- a/drivers/clk/socfpga/clk-agilex5.c
> +++ b/drivers/clk/socfpga/clk-agilex5.c

[ ... ]

> @@ -486,35 +502,92 @@ static int agilex5_clk_register_pll(const struct agilex5_pll_clock *clks,
>  	return 0;
>  }
>  
> -static int agilex5_clkmgr_init(struct platform_device *pdev)
> +static struct stratix10_clock_data *agilex5_clk_data;
> +
> +static struct stratix10_clock_data *agilex5_clk_data_alloc(void __iomem *base)
>  {
> -	struct device_node *np = pdev->dev.of_node;
> -	struct device *dev = &pdev->dev;
>  	struct stratix10_clock_data *clk_data;
> -	void __iomem *base;
> -	int i, num_clks;
> -
> -	base = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(base))
> -		return PTR_ERR(base);
> -
> -	num_clks = AGILEX5_NUM_CLKS;
> +	int i;
>  
> -	clk_data = devm_kzalloc(dev, struct_size(clk_data, clk_data.hws,
> -				num_clks), GFP_KERNEL);
> +	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, AGILEX5_NUM_CLKS),
> +			   GFP_KERNEL);
>  	if (!clk_data)
> -		return -ENOMEM;
> +		return NULL;
>  
>  	clk_data->base = base;
> -	clk_data->clk_data.num = num_clks;
> +	clk_data->clk_data.num = AGILEX5_NUM_CLKS;
> +
> +	for (i = 0; i < AGILEX5_NUM_CLKS; i++)
> +		clk_data->clk_data.hws[i] = ERR_PTR(-EPROBE_DEFER);
>  
> -	for (i = 0; i < num_clks; i++)
> -		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
> +	return clk_data;
> +}

[ ... ]

> +static void __init agilex5_clkmgr_early_init(struct device_node *np)
> +{
> +	struct stratix10_clock_data *clk_data;
> +	void __iomem *base;
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		pr_err("%s: failed to map clock registers\n", __func__);
> +		return;
> +	}
> +
> +	clk_data = agilex5_clk_data_alloc(base);
> +	if (!clk_data) {
> +		iounmap(base);
> +		return;
> +	}
> +
> +	agilex5_clk_register_early(clk_data);
> +
> +	if (of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data)) {
> +		pr_err("%s: failed to add clock provider\n", __func__);
> +		return;
> +	}

[Severity: Medium]
Does this code leak the mapped IOMEM base and the allocated clk_data on failure?
Returning directly here without calling iounmap() and kfree() will permanently
leak them.

[Severity: High]
If of_clk_add_hw_provider() fails here, agilex5_clk_data remains NULL but the
early clocks are already registered in the Common Clock Framework (CCF).

During late init, agilex5_clkmgr_init() sees agilex5_clk_data is NULL and
attempts to re-register the early clocks. CCF rejects the duplicate names,
causing the early clock indices in the new clk_data to remain ERR_PTR.
The late provider registration succeeds but without the early clocks, breaking
consumers that need them.

Could this code deregister the early clocks on this error path, or handle
the registration failure differently to prevent this?

> +
> +	agilex5_clk_data = clk_data;
> +}

[ ... ]

> @@ -526,8 +599,15 @@ static int agilex5_clkmgr_init(struct platform_device *pdev)
>  	agilex5_clk_register_gate(agilex5_gate_clks,
>  				  ARRAY_SIZE(agilex5_gate_clks), clk_data);
>  
> -	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
> -	return 0;
> +	for (i = 0; i < AGILEX5_NUM_CLKS; i++)
> +		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
> +			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
> +
> +	if (clk_data == agilex5_clk_data)
> +		return 0;
> +
> +	return of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get,
> +				      &clk_data->clk_data);
>  }

[Severity: High]
Does this error path result in a use-after-free of the IOMEM base?

If clk_data was allocated dynamically in this function and
of_clk_add_hw_provider() fails, the probe function returns an error, which
triggers devres to unmap the IOMEM base. However, the clocks were registered
into the CCF without devres management and remain globally accessible.
If they are accessed later, they will dereference the unmapped IOMEM base.

[Severity: Medium]
Additionally, if of_clk_add_hw_provider() fails, does this code leak the
dynamically allocated clk_data structure? Since agilex5_clk_data_alloc() was
changed from using devm_kzalloc() to kzalloc(), it requires manual cleanup
on failure.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790935401.git.adrian.ho.yin.ng@altera.com?part=2

Re: [PATCH v2 3/3] clk: socfpga: stratix10: register the SP timer clocks early

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: socfpga: use-after-free of unmapped IO memory on probe failure
- [Medium] clk: socfpga: memory leak of clk_data on probe error
- [Medium] clk: socfpga: incomplete early init rollback causes fatal registration conflicts
--

commit 68e80ef968cd718473fa02d4989427b8a0ddd324
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>

clk: socfpga: stratix10: register the SP timer clocks early

The patch registers the SP timer clocks early using CLK_OF_DECLARE_DRIVER()
to prevent probe deferral, while deferring the remaining clocks to the
platform driver probe.

> diff --git a/drivers/clk/socfpga/clk-s10.c b/drivers/clk/socfpga/clk-s10.c
> index b4bf4e2d38e16..7eae2dfa408d5 100644
> --- a/drivers/clk/socfpga/clk-s10.c
> +++ b/drivers/clk/socfpga/clk-s10.c

[ ... ]

> +static struct stratix10_clock_data *s10_clk_data_alloc(void __iomem *base)
> +{
> +	struct stratix10_clock_data *clk_data;
> +	int i;
> +
> +	clk_data = kzalloc(struct_size(clk_data, clk_data.hws, STRATIX10_NUM_CLKS),
> +			   GFP_KERNEL);
> +	if (!clk_data)
> +		return NULL;

[ ... ]

> +static void __init s10_clkmgr_early_init(struct device_node *np)
>  {

[ ... ]

> +	s10_clk_register_early(clk_data);
>  
> -	for (i = 0; i < num_clks; i++)
> -		clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
> +	if (of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data)) {
> +		pr_err("%s: failed to add clock provider\n", __func__);
> +		return;
> +	}

[Severity: Medium]
If of_clk_add_hw_provider() fails here, does it leave the globally registered
early clocks active but without a provider?

Since s10_clk_data is left as NULL, won't the fallback probe s10_clkmgr_init()
attempt to re-register the early clocks via s10_clk_register_early()? Could
this cause a fatal name collision (-EEXIST) in the clock framework and
permanently deprive downstream consumers of the early clocks?

> -	s10_clk_register_pll(s10_pll_clks, ARRAY_SIZE(s10_pll_clks), clk_data);
> +	s10_clk_data = clk_data;
> +}
> +
> +CLK_OF_DECLARE_DRIVER(stratix10_clkmgr, "intel,stratix10-clkmgr",
> +		      s10_clkmgr_early_init);
> +
> +static int s10_clkmgr_init(struct platform_device *pdev)
> +{
> +	struct stratix10_clock_data *clk_data = s10_clk_data;
> +	void __iomem *base;
> +	int i;
> +
> +	if (!clk_data) {
> +		base = devm_platform_ioremap_resource(pdev, 0);
> +		if (IS_ERR(base)) {
> +			pr_err("%s: failed to map clock registers\n", __func__);
> +			return PTR_ERR(base);
> +		}
> +
> +		clk_data = s10_clk_data_alloc(base);
> +		if (!clk_data)
> +			return -ENOMEM;
> +
> +		s10_clk_register_early(clk_data);
> +	}

[ ... ]

> -	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data);
> -	return 0;
> +	for (i = 0; i < STRATIX10_NUM_CLKS; i++)
> +		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
> +			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
> +
> +	if (clk_data == s10_clk_data)
> +		return 0;
> +
> +	return of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get,
> +				      &clk_data->clk_data);
>  }

[Severity: Medium]
If of_clk_add_hw_provider() fails here and propagates the error directly, does
it leak the clk_data allocation?

Since clk_data is now allocated with kzalloc() in s10_clk_data_alloc()
instead of devm_kzalloc(), bypassing explicit cleanup on this error path
appears to permanently leak the memory.

[Severity: High]
Does propagating the error from of_clk_add_hw_provider() also cause a
use-after-free on unmapped IO memory?

If the fallback probe mapped base using devm_platform_ioremap_resource(), a
probe failure will trigger devres cleanup and automatically unmap base.
However, since the clocks were registered without being tied to devres in
s10_clk_register_early(), they remain globally active and hold pointers to the
now unmapped IO memory.

Could any subsequent access by the clock framework to these clocks dereference
the unmapped memory and lead to a kernel panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790935401.git.adrian.ho.yin.ng@altera.com?part=3

Re: [PATCH v2 1/3] clk: socfpga: agilex: register the SP timer clocks early

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: socfpga: use-after-free of I/O memory on probe failure
- [Medium] clk: socfpga: memory and I/O mapping leaks on error paths
- [Medium] clk: socfpga: double registration of early clocks on provider failure
--

commit 1aa58afee8b5118fcfe81945cc09867307ba4bd8
Author: Adrian Ng Ho Yin <adrian.ho.yin.ng@altera.com>
Subject: clk: socfpga: agilex: register the SP timer clocks early

This commit splits the clock registration for the Agilex platform into an
early initialization phase to support timers that require clocks before
core_initcall, and defers the remaining clocks to the platform driver probe.

> diff --git a/drivers/clk/socfpga/clk-agilex.c b/drivers/clk/socfpga/clk-agilex.c
> index 2bdea1997b5ef..91a80b7b1886a 100644
> --- a/drivers/clk/socfpga/clk-agilex.c
> +++ b/drivers/clk/socfpga/clk-agilex.c

[ ... ]

> +static void __init
> +agilex_clkmgr_of_init(struct device_node *np,
> +		      void (*register_early)(struct stratix10_clock_data *))
> +{
> +	struct stratix10_clock_data *clk_data;
> +	void __iomem *base;
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		pr_err("%s: failed to map clock registers\n", __func__);
> +		return;
> +	}
> +
> +	clk_data = agilex_clk_data_alloc(base);
> +	if (!clk_data) {
> +		iounmap(base);
> +		return;
> +	}
> +
> +	register_early(clk_data);
> +
> +	if (of_clk_add_hw_provider(np, of_clk_hw_onecell_get, &clk_data->clk_data)) {

[Severity: Medium]
If of_clk_add_hw_provider() fails, register_early() will have already
registered the clocks via the CCF APIs. Because agilex_clk_data is not set,
they become permanently inaccessible to consumers. 

When the platform driver later probes, it will attempt to register these early
clocks again because agilex_clk_data is still NULL. Can this result in
duplicate CCF name rejections, leaving the new array with -ENOENT?

> +		pr_err("%s: failed to add clock provider\n", __func__);
> +		return;

[Severity: Medium]
Does this error path leak the I/O mapping and memory? It appears of_iomap()
base and agilex_clk_data_alloc() clk_data are not cleaned up with
iounmap() and kfree() before returning.

> +	}
> +
> +	agilex_clk_data = clk_data;
> +}

[ ... ]

> +static int agilex_clkmgr_add_provider(struct platform_device *pdev,
> +				      struct stratix10_clock_data *clk_data)
> +{
> +	int i;
> +
> +	for (i = 0; i < AGILEX_NUM_CLKS; i++)
> +		if (clk_data->clk_data.hws[i] == ERR_PTR(-EPROBE_DEFER))
> +			clk_data->clk_data.hws[i] = ERR_PTR(-ENOENT);
> +
> +	if (clk_data == agilex_clk_data)
> +		return 0;
> +
> +	return of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get,
> +				      &clk_data->clk_data);

[Severity: High]
If early init was skipped or failed, agilex_clkmgr_get_clk_data() maps I/O
memory using devm_platform_ioremap_resource() and registers clocks.

Unlike the original code which ignored the return value, this propagates the
error from of_clk_add_hw_provider(). Returning the error causes the driver
core's devres to unmap the memory, but the clocks remain globally registered in
CCF with raw pointers to the unmapped base.

Can this lead to a use-after-free of devm-managed I/O memory if CCF attempts
to read from the unmapped base during operations like debugfs reads?

> +}
> +
> +static int agilex_clkmgr_init(struct platform_device *pdev)
> +{
> +	struct stratix10_clock_data *clk_data;
> +
> +	clk_data = agilex_clkmgr_get_clk_data(pdev, agilex_clk_register_early);
> +	if (IS_ERR(clk_data))
> +		return PTR_ERR(clk_data);
>  
>  	agilex_clk_register_c_perip(agilex_main_perip_c_clks,
>  				 ARRAY_SIZE(agilex_main_perip_c_clks), clk_data);
> @@ -490,36 +625,17 @@ static int agilex_clkmgr_init(struct platform_device *pdev)
>  
>  	agilex_clk_register_gate(agilex_gate_clks, ARRAY_SIZE(agilex_gate_clks),
>  			      clk_data);
> +
> +	return agilex_clkmgr_add_provider(pdev, clk_data);

[Severity: Medium]
If agilex_clkmgr_add_provider() fails, probe returns an error. Does this leak
the clk_data allocated by kzalloc() in agilex_clk_data_alloc()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790935401.git.adrian.ho.yin.ng@altera.com?part=1