On Sat, 20 Nov 2021 at 14:49, Sylwester Nawrocki [off-list ref] wrote:

On 25.10.2021 18:12, Sam Protsenko wrote:

quoted

Some clocks must be registered before init calls. For example MCT clock
(from CMU_PERI) is needed for MCT timer driver, which is registered
with TIMER_OF_DECLARE(). By the time we get to core_initcall() used for
clk-exynos850 platform driver init, it's already too late. Inability to
get "mct" clock in MCT driver leads to kernel panic, as functions
registered with *_OF_DECLARE() can't do deferred calls. MCT timer driver
can't be fixed either, as it's acting as a clock source and it's
essential to register it in start_kernel() -> time_init().

Let's register CMU_PERI clocks early, using CLK_OF_DECLARE_DRIVER(), and
do all stuff relying on "struct dev" object (like runtime PM and
enabling bus clock) later in platform driver probe. Basically
CLK_OF_DECLARE_DRIVER() matches CMU compatible, but clears OF_POPULATED
flag, which allows the same device to be matched again later.

quoted

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

quoted

 drivers/clk/samsung/clk-exynos850.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
index 95e373d17b42..ecffa5c7a081 100644
--- a/drivers/clk/samsung/clk-exynos850.c
+++ b/drivers/clk/samsung/clk-exynos850.c

@@ -753,6 +753,15 @@ static const struct samsung_cmu_info peri_cmu_info __initconst = {
      .clk_name               = "dout_peri_bus",
 };

+static void __init exynos850_cmu_peri_init(struct device_node *np)
+{
+     exynos850_init_clocks(np, peri_clk_regs, ARRAY_SIZE(peri_clk_regs));
+     samsung_cmu_register_one(np, &peri_cmu_info);
+}
+
+CLK_OF_DECLARE_DRIVER(exynos850_cmu_peri, "samsung,exynos850-cmu-peri",
+                   exynos850_cmu_peri_init);
+
 /* ---- CMU_CORE ------------------------------------------------------------ */

 /* Register Offset definitions for CMU_CORE (0x12000000) */

@@ -920,8 +929,12 @@ static int __init exynos850_cmu_probe(struct platform_device *pdev)
      struct device_node *np = dev->of_node;

      info = of_device_get_match_data(dev);
-     exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
-     samsung_cmu_register_one(np, info);
+
+     /* Early clocks are already registered using CLK_OF_DECLARE_DRIVER() */
+     if (info != &peri_cmu_info) {
+             exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
+             samsung_cmu_register_one(np, info);
+     }

Don't you also need to register early CMU_TOP, which provides clocks
for CMU_PERI? I'm afraid it might not work properly when you register
CMU_PERI clocks early and only later in probe() you enable parent clock
required for the already registered clocks to be usable.

Good point, I'll do that in v2. Not sure how I missed that dependency
point, but thank you for noticing that. Guess it only works for me
because clocks are already enabled in bootloader, and I'm using
"clk_ignore_unused" param for now.

How about registering also CMU_TOP early and enabling parent clock
also in OF_CLK_DECLARE init callback, i.e. using either OF_CLK_DECLARE
or platform driver for a CMU?

If you mean doing clk_prepare_enable() for "dout_peri_bus" clock in
exynos850_cmu_peri_init(), I don't think it's possible. clk_get()
needs "struct device *dev", and we only have that in platform driver
probe. Trying to pass dev=NULL won't work, so that's why I'm enabling
parent clocks in platform driver probe.

I'm going to submit new patch series soon. It'll include all my recent
patches, addressing all your comments. We can continue discussion
there, in case I misunderstood you and those patches are still not
correct.

Thanks!

--
Regards,
Sylwester