On 06/10/2021 12:46, Sam Protsenko wrote:

On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski
[off-list ref] wrote:

quoted

On 14/09/2021 17:56, Sam Protsenko wrote:

quoted

By default if bus clock has no users its "enable count" value is 0. It
might be actually running if it's already enabled in bootloader, but
then in some cases it can be disabled by mistake. For example, such case
was observed when dw_mci_probe() enabled bus clock, then failed to do
something and disabled that bus clock on error path. After that even
attempt to read the 'clk_summary' file in DebugFS freezed forever, as
CMU bus clock ended up being disabled and it wasn't possible to access
CMU registers anymore.

To avoid such cases, CMU driver must increment the ref count for that
bus clock by running clk_prepare_enable(). There is already existing
'.clk_name' field in struct samsung_cmu_info, exactly for that reason.
It was added in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
support for runtime PM"). But the clock is actually enabled only in
Exynos5433 clock driver. Let's mimic what is done there in generic
samsung_cmu_register_one() function, so other drivers can benefit from
that `.clk_name' field. As was described above, it might be helpful not
only for PM reasons, but also to prevent possible erroneous clock gating
on error paths.

Another way to workaround that issue would be to use CLOCK_IS_CRITICAL
flag for corresponding gate clocks. But that might be not very good
design decision, as we might still want to disable that bus clock, e.g.
on PM suspend.

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 drivers/clk/samsung/clk.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 1949ae7851b2..da65149fa502 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c

@@ -357,6 +357,19 @@ struct samsung_clk_provider * __init samsung_cmu_register_one(

      ctx = samsung_clk_init(np, reg_base, cmu->nr_clk_ids);

+     /* Keep bus clock running, so it's possible to access CMU registers */
+     if (cmu->clk_name) {
+             struct clk *bus_clk;
+
+             bus_clk = __clk_lookup(cmu->clk_name);
+             if (bus_clk) {
+                     clk_prepare_enable(bus_clk);
+             } else {
+                     pr_err("%s: could not find bus clock %s\n", __func__,
+                            cmu->clk_name);
+             }
+     }
+

Solving this problem in generic way makes sense but your solution is
insufficient. You skipped suspend/resume paths and in such case you
should remove the Exynos5433-specific code.

Keeping core bus clocks always running seems like a separate
independent feature to me (not related to suspend/resume). It's
mentioned in commit 523d3de41f02 ("clk: samsung: exynos5433: Add
support for runtime PM") this way:

    "Also for each CMU there is one special parent clock, which has to
be enabled all the time when any access to CMU registers is being
done."

Why do you think suspend/resume paths have to be implemented along
with it? Btw, I didn't add PM ops in clk-exynos850, as PM is not
implemented on my board yet and I can't test it.

You can skip the runtime PM, so keep your patch almost like it is now
(in respect to Sylwester's comment about __clk_lookup). However now the
Exynos5433 will enable the clk_name twice: here and in
exynos5433_cmu_probe().

If you keep this approach, you need to remove duplicated part in
exynos5433_cmu_probe()...

If you are suggesting moving all stuff from exynos5433_cmu_probe()
into samsung_cmu_register_one(), it would take passing platform_device
there, and implementing all PM related operations. I guess it's not a
super easy task, as it would require converting clk-exynos7 to
platform_driver for instance, and re-testing everything on exynos5433
and exynos7 boards (which I don't have).

What do you say if I pull that code to clk-exynos850.c instead for v2?
Refactoring (merging stuff from exynos5433_cmu_probe() into
samsung_cmu_register_one() ) can be done later, when I add PM ops into
clk-exynos850.

quoted

Best regards,
Krzysztof

Best regards,
Krzysztof