diff mbox series

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

Message ID 20210914155607.14122-2-semen.protsenko@linaro.org (mailing list archive)
State New, archived
Headers show
Series clk: samsung: Introduce Exynos850 SoC clock driver | expand

Commit Message

Sam Protsenko Sept. 14, 2021, 3:56 p.m. UTC
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(+)

Comments

Krzysztof Kozlowski Sept. 15, 2021, 8:21 a.m. UTC | #1
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);
> +		}
> +	}
> +

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.

Best regards,
Krzysztof
Sylwester Nawrocki Sept. 15, 2021, 12:51 p.m. UTC | #2
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).
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.
Sam Protsenko Oct. 6, 2021, 10:46 a.m. UTC | #3
On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> 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);
> > +             }
> > +     }
> > +
>
> 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.

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.

> Best regards,
> Krzysztof
Sam Protsenko Oct. 6, 2021, 11:18 a.m. UTC | #4
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:

<<<<<<<<<<<<<<<< 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
Krzysztof Kozlowski Oct. 6, 2021, 12:38 p.m. UTC | #5
On 06/10/2021 12:46, Sam Protsenko wrote:
> On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> 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);
>>> +             }
>>> +     }
>>> +
>>
>> 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.
> 
>> Best regards,
>> Krzysztof


Best regards,
Krzysztof
Krzysztof Kozlowski Oct. 6, 2021, 12:45 p.m. UTC | #6
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
Sam Protsenko Oct. 6, 2021, 1:29 p.m. UTC | #7
On Wed, 6 Oct 2021 at 15:38, Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 06/10/2021 12:46, Sam Protsenko wrote:
> > On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski
> > <krzysztof.kozlowski@canonical.com> wrote:
> >>
> >> 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);
> >>> +             }
> >>> +     }
> >>> +
> >>
> >> 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()...
>

My patch is only touching samsung_cmu_register_one(), and
exynos5433_cmu_probe() doesn't call samsung_cmu_register_one(). So I
don't think there can be a problem there. Or I'm missing something?

samsung_cmu_register_one() is actually called from 5433 clk driver,
but only from CMUs registered with CLK_OF_DECLARE(), and those are not
setting .clk_name field, so my code is not affecting those either.

Real problem I can see is that I can't avoid using __clk_lookup() if I
implement that code in samsung_cmu_register_one(). Tried to do use
clk_get(NULL, ...) instead, but it doesn't work with 1st param (dev)
being NULL, because samsung_clk_register_*() functions don't register
clkdev (only samsung_clk_register_fixed_rate() does), hence
LIST_HEAD(clocks) is empty in clkdev.c, and clk_get() fails, when not
provided with actual 'dev' param, which in turn is not present in
samsung_cmu_register_one()...

About using platform_driver: as I can see from clk-exynos5433.c, only
CMUs which belong to Power Domains are registered as platform_driver.
Rest of CMUs are registered using CLK_OF_DECLARE(), thus they don't
get platform_device param. That makes it harder to avoid using
__clk_lookup() inside samsung_cmu_register_one().

All that said, I feel like correct way to implement this patch would be:
  1. Register all PD-capable CMUs as platform_driver in clk-exynos850
(all CMUs except CMU_TOP)
  2. Move bus clock enablement code from samsung_cmu_register_one() to
corresponding clk-exynos850 probe function

This way I would be able to use clk_get(dev, ...) instead of
__clk_lookup(), and that won't affect any existing code for sure. Code
will be more unified w.r.t. how it's done in clk-exynos5433, and
platform_device will be a foundation for implementing PM ops later.
Taking into account how much design decisions should be done for using
that in common code -- I'd say let's do that later, as a separate
refactoring activity.

Do you think that makes sense?

Thanks!

> >
> > 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.
> >
> >> Best regards,
> >> Krzysztof
>
>
> Best regards,
> Krzysztof
Krzysztof Kozlowski Oct. 8, 2021, 6:50 a.m. UTC | #8
On 06/10/2021 15:29, Sam Protsenko wrote:
> On Wed, 6 Oct 2021 at 15:38, Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> On 06/10/2021 12:46, Sam Protsenko wrote:
>>> On Wed, 15 Sept 2021 at 11:21, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@canonical.com> wrote:
>>>>
>>>> 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);
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>
>>>> 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()...
>>
> 
> My patch is only touching samsung_cmu_register_one(), and
> exynos5433_cmu_probe() doesn't call samsung_cmu_register_one(). So I
> don't think there can be a problem there. Or I'm missing something?
> 
> samsung_cmu_register_one() is actually called from 5433 clk driver,
> but only from CMUs registered with CLK_OF_DECLARE(), and those are not
> setting .clk_name field, so my code is not affecting those either.

You are right.

> 
> Real problem I can see is that I can't avoid using __clk_lookup() if I
> implement that code in samsung_cmu_register_one(). Tried to do use
> clk_get(NULL, ...) instead, but it doesn't work with 1st param (dev)
> being NULL, because samsung_clk_register_*() functions don't register
> clkdev (only samsung_clk_register_fixed_rate() does), hence
> LIST_HEAD(clocks) is empty in clkdev.c, and clk_get() fails, when not
> provided with actual 'dev' param, which in turn is not present in
> samsung_cmu_register_one()...
> 
> About using platform_driver: as I can see from clk-exynos5433.c, only
> CMUs which belong to Power Domains are registered as platform_driver.
> Rest of CMUs are registered using CLK_OF_DECLARE(), thus they don't
> get platform_device param. That makes it harder to avoid using
> __clk_lookup() inside samsung_cmu_register_one().
> 
> All that said, I feel like correct way to implement this patch would be:
>   1. Register all PD-capable CMUs as platform_driver in clk-exynos850
> (all CMUs except CMU_TOP)
>   2. Move bus clock enablement code from samsung_cmu_register_one() to
> corresponding clk-exynos850 probe function
> 
> This way I would be able to use clk_get(dev, ...) instead of
> __clk_lookup(), and that won't affect any existing code for sure. Code
> will be more unified w.r.t. how it's done in clk-exynos5433, and
> platform_device will be a foundation for implementing PM ops later.
> Taking into account how much design decisions should be done for using
> that in common code -- I'd say let's do that later, as a separate
> refactoring activity.
> 
> Do you think that makes sense?

Yes, makes sense. Thank you!


Best regards,
Krzysztof
Sylwester Nawrocki Oct. 9, 2021, 6:49 p.m. UTC | #9
On 06.10.2021 13:18, Sam Protsenko wrote:
> On Wed, 15 Sept 2021 at 15:51, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> 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.

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

I missed the fact that there is usually a specific SFR sequence required
for disabling the CMU root (and APB) clock. We would need to figure out what
an exact sequence is for each CMU, similarly as is done in clk-exynos5433,
then keeping the CMU source clock always enabled shouldn't be required.
I'm fine with just enabling the APB clocks in probe() until proper CMU
suspend/resume support is added.

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

I think some of the clocks supplied by a CMU need to be in use
(e.g. clk_prepare()) to get the resume op in the CMU driver invoked.

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

Enabling the clock corresponds with the pm_runtime_set_active() call you
pointed out above. Such pattern also ensures the clock will stay enabled
when CONFIG_PM_RUNTIME is disabled.

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

It is not really a separate feature, I think having the clocks permanently
enabled is not something we would like to end up with. It would need to be
revisited anyway when adding the power domains support.

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

You need device pointer to get the CMU input clocks as specified in DT.
clk_get with NULL device pointer and global clock name will now only work
on Samsung non-DT platforms, for DT-only SoCs we don't register clkdev
entries at all (see samsung_clk_register_alias()).

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

Sounds good to me, thank you for working on this.

--
Regards,
Sylwester
diff mbox series

Patch

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);