On Wed, 15 Sept 2021 at 15:51, Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx> wrote: > > Hi, > > On 14.09.2021 17:56, Sam Protsenko wrote: > > 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@xxxxxxxxxx> > > --- > > 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