Re: [PATCH 1/6] clk: samsung: Enable bus clock on init

On Wed, 15 Sept 2021 at 15:51, Sylwester Nawrocki
[off-list ref] wrote:

Hi,

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);
+             }
+     }
+
      if (cmu->pll_clks)
              samsung_clk_register_pll(ctx, cmu->pll_clks, cmu->nr_pll_clks,
                      reg_base);

I would suggest to implement runtime PM ops in your driver instead, even though
those would initially only contain single clk enable/disable. Things like
the clk_summary will work then thanks to runtime PM support in the clk core
(see clk_pm_runtime_* calls).

Can you please elaborate more? I don't see how adding PM ops would
solve the problem I'm trying to address, which is keeping core bus
clocks always running. For example, I'm looking at clk-exynos5433.c
implementation, which enables bus clock on resume path:

<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
static int __maybe_unused exynos5433_cmu_resume(struct device *dev)
{
    ...
    clk_prepare_enable(data->clk);
    ...
}
<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>

But that resume operation won't be called on driver init, because it
configures runtime PM like this:

<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
static int __init exynos5433_cmu_probe(struct platform_device *pdev)
{
    ...
    /*
     * Enable runtime PM here to allow the clock core using runtime PM
     * for the registered clocks. Additionally, we increase the runtime
     * PM usage count before registering the clocks, to prevent the
     * clock core from runtime suspending the device.
     */
    pm_runtime_get_noresume(dev);
    pm_runtime_set_active(dev);
    pm_runtime_enable(dev);
    ...
    pm_runtime_put_sync(dev);
    ...
}
<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>

When I tried to implement the same in my driver, only suspend function
is called during kernel startup.

Anyway, even clk-exynos5433.c driver (which also implements PM ops)
does the same for core bus clocks:

<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
static int __init exynos5433_cmu_probe(struct platform_device *pdev)
{
    ...
    if (info->clk_name)
        data->clk = clk_get(dev, info->clk_name);
    clk_prepare_enable(data->clk);
    ...
}
<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>

So it looks like separate feature to me. Not sure how that can be
implemented only by adding PM ops. Also, my board lacks PM support in
upstream kernel right now, so I probably won't be able to test PM ops
if I implement those, that's why I decided to skip it for now.

We could also make common runtime PM suspend/resume helpers but I wouldn't focus
on that too much now, it could well be done later.
And please avoid introducing new __clk_lookup() calls.

The reason I used __clk_lookup() is that it's the only API that works
in that case. I tried to use clk_get(), but we lack 'struct dev'
pointer in samsung_cmu_register_one(), so when providing dev=NULL into
clk_get() it fails to get the clock. That's happening because
LIST_HEAD(clocks) is probably empty in clkdev.c. So this chain fails:

<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>
clk_get()    // dev = NULL
  v
__clk_get_sys()
  v
clk_find_hw()
  v
clk_find()   // returns 0, because LIST_HEAD(clocks) is empty
<<<<<<<<<<<<<<<< cut here >>>>>>>>>>>>>>>>

I saw your patches which get rid of __clk_lookup() usage by accessing
ctx->clk_data.hws[], but that requires using clock index, not name.
'struct samsung_cmu_info' only stores bus clock name (.clk_name),
which seems logical to me, so we can't get away from using
__clk_lookup() in that case without refactoring 'struct
samsung_cmu_info' first.

All that said, I suggest next: I'll pull the code from this patch into
clk-exynos850.c, adding platform_driver registration there, so I can
actually use clk_get() for getting bus clocks. As for PM ops, I'd like
to skip it for now, if you don't mind, as I can't fully test those.
Otherwise please elaborate more on how PM ops can solve this problem.

Thanks!

--
Regards,
Sylwester