linux-samsung-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
To: Sam Protsenko <semen.protsenko@linaro.org>,
	Sylwester Nawrocki <s.nawrocki@samsung.com>
Cc: "Paweł Chmiel" <pawel.mikolaj.chmiel@gmail.com>,
	"Chanwoo Choi" <cw00.choi@samsung.com>,
	"Tomasz Figa" <tomasz.figa@gmail.com>,
	"Ryu Euiyoul" <ryu.real@samsung.com>,
	"Tom Gall" <tom.gall@linaro.org>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	"John Stultz" <john.stultz@linaro.org>,
	"Amit Pundir" <amit.pundir@linaro.org>,
	devicetree <devicetree@vger.kernel.org>,
	"linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>,
	linux-clk <linux-clk@vger.kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Linux Samsung SOC" <linux-samsung-soc@vger.kernel.org>,
	"Michael Turquette" <mturquette@baylibre.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>
Subject: Re: [PATCH 1/6] clk: samsung: Enable bus clock on init
Date: Wed, 6 Oct 2021 14:45:24 +0200	[thread overview]
Message-ID: <eb582eb0-f281-1795-9584-fccc60147261@canonical.com> (raw)
In-Reply-To: <CAPLW+4=cL00WxQpobovE3Jo62RijOpqwYNAF8TJHXQTdGxNHHg@mail.gmail.com>

On 06/10/2021 13:18, Sam Protsenko wrote:
> On Wed, 15 Sept 2021 at 15:51, Sylwester Nawrocki
> <s.nawrocki@samsung.com> 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@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:

The device will get suspended (like you say) till the first usage, which
will resume it and thus make the clock enabled.

> 
> <<<<<<<<<<<<<<<< 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.

In general you need runtime PM to make a proper clock driver. You can
skip it, just like most of our early drivers skipped it, including
Exynos7, but it's not good in the long run. You might later hit for
example imprecise aborts when enumerating clocks (/sys/kernel/debug/clk)
or power domains.

To me it is fine with skipping runtime PM, but using platform driver now
seems good choice. When writing the code, use rather Exynos5433 as an
example, not Exynos7. The former was extensively developed and used for
mainline. The latter was only part of rather early bringup of platform
and lacks several features/drivers/DT.


Best regards,
Krzysztof

  reply	other threads:[~2021-10-06 12:45 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-14 15:56 [PATCH 0/6] clk: samsung: Introduce Exynos850 SoC clock driver Sam Protsenko
2021-09-14 15:56 ` [PATCH 1/6] clk: samsung: Enable bus clock on init Sam Protsenko
2021-09-15  8:21   ` Krzysztof Kozlowski
2021-10-06 10:46     ` Sam Protsenko
2021-10-06 12:38       ` Krzysztof Kozlowski
2021-10-06 13:29         ` Sam Protsenko
2021-10-08  6:50           ` Krzysztof Kozlowski
2021-09-15 12:51   ` Sylwester Nawrocki
2021-10-06 11:18     ` Sam Protsenko
2021-10-06 12:45       ` Krzysztof Kozlowski [this message]
2021-10-09 18:49       ` Sylwester Nawrocki
2021-09-14 15:56 ` [PATCH 2/6] clk: samsung: clk-pll: Implement pll0822x PLL type Sam Protsenko
2021-09-15  8:24   ` Krzysztof Kozlowski
2021-09-15 15:59   ` Chanwoo Choi
2021-09-14 15:56 ` [PATCH 3/6] clk: samsung: clk-pll: Implement pll0831x " Sam Protsenko
2021-09-15  8:26   ` Krzysztof Kozlowski
2021-09-15 16:11   ` Chanwoo Choi
2021-09-14 15:56 ` [PATCH 4/6] dt-bindings: clock: Add bindings definitions for Exynos850 CMU Sam Protsenko
2021-09-15  8:27   ` Krzysztof Kozlowski
2021-09-15 16:37   ` Chanwoo Choi
2021-10-05 10:28     ` Sam Protsenko
2021-10-06 10:49       ` Krzysztof Kozlowski
2021-10-06 13:31         ` Sam Protsenko
2021-09-21 21:10   ` Rob Herring
2021-09-14 15:56 ` [PATCH 5/6] dt-bindings: clock: Document Exynos850 CMU bindings Sam Protsenko
2021-09-14 21:35   ` Rob Herring
2021-09-15  8:28   ` Krzysztof Kozlowski
2021-10-05 11:48     ` Sam Protsenko
2021-09-15 16:47   ` Chanwoo Choi
2021-09-14 15:56 ` [PATCH 6/6] clk: samsung: Introduce Exynos850 clock driver Sam Protsenko
2021-09-15  8:59   ` Krzysztof Kozlowski
2021-10-05 11:29     ` Sam Protsenko
2021-10-06 12:50       ` Krzysztof Kozlowski
2021-09-15 13:07   ` Sylwester Nawrocki
2021-10-05 11:36     ` Sam Protsenko
2021-10-06 12:46       ` Krzysztof Kozlowski
2021-09-15 18:04   ` Chanwoo Choi
2021-09-15 22:00     ` Sam Protsenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=eb582eb0-f281-1795-9584-fccc60147261@canonical.com \
    --to=krzysztof.kozlowski@canonical.com \
    --cc=amit.pundir@linaro.org \
    --cc=cw00.choi@samsung.com \
    --cc=devicetree@vger.kernel.org \
    --cc=john.stultz@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=pawel.mikolaj.chmiel@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=ryu.real@samsung.com \
    --cc=s.nawrocki@samsung.com \
    --cc=sboyd@kernel.org \
    --cc=semen.protsenko@linaro.org \
    --cc=sumit.semwal@linaro.org \
    --cc=tom.gall@linaro.org \
    --cc=tomasz.figa@gmail.com \
    --subject='Re: [PATCH 1/6] clk: samsung: Enable bus clock on init' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).