linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early
@ 2021-10-25 16:12 ` Sam Protsenko
  2021-10-26 14:53   ` Krzysztof Kozlowski
  2021-11-20 12:49   ` Sylwester Nawrocki
  0 siblings, 2 replies; 8+ messages in thread
From: Sam Protsenko @ 2021-10-25 16:12 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Sumit Semwal, devicetree, linux-arm-kernel, linux-clk,
	linux-kernel, linux-samsung-soc

Some clocks must be registered before init calls. For example MCT clock
(from CMU_PERI) is needed for MCT timer driver, which is registered
with TIMER_OF_DECLARE(). By the time we get to core_initcall() used for
clk-exynos850 platform driver init, it's already too late. Inability to
get "mct" clock in MCT driver leads to kernel panic, as functions
registered with *_OF_DECLARE() can't do deferred calls. MCT timer driver
can't be fixed either, as it's acting as a clock source and it's
essential to register it in start_kernel() -> time_init().

Let's register CMU_PERI clocks early, using CLK_OF_DECLARE_DRIVER(), and
do all stuff relying on "struct dev" object (like runtime PM and
enabling bus clock) later in platform driver probe. Basically
CLK_OF_DECLARE_DRIVER() matches CMU compatible, but clears OF_POPULATED
flag, which allows the same device to be matched again later.

Similar issue was discussed at [1] and addressed in commit 1f7db7bbf031
("clk: renesas: cpg-mssr: Add early clock support"), as well as in
drivers/clk/mediatek/clk-mt2712.c.

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20180829132954.64862-2-chris.brandt@renesas.com/

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v2:
  - Register only CMU_PERI clocks early

Notes:
  - This patch should be applied on top of CMU_APM series
    (clk: samsung: exynos850: Implement CMU_APM domain)

 drivers/clk/samsung/clk-exynos850.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
index 95e373d17b42..ecffa5c7a081 100644
--- a/drivers/clk/samsung/clk-exynos850.c
+++ b/drivers/clk/samsung/clk-exynos850.c
@@ -753,6 +753,15 @@ static const struct samsung_cmu_info peri_cmu_info __initconst = {
 	.clk_name		= "dout_peri_bus",
 };
 
+static void __init exynos850_cmu_peri_init(struct device_node *np)
+{
+	exynos850_init_clocks(np, peri_clk_regs, ARRAY_SIZE(peri_clk_regs));
+	samsung_cmu_register_one(np, &peri_cmu_info);
+}
+
+CLK_OF_DECLARE_DRIVER(exynos850_cmu_peri, "samsung,exynos850-cmu-peri",
+		      exynos850_cmu_peri_init);
+
 /* ---- CMU_CORE ------------------------------------------------------------ */
 
 /* Register Offset definitions for CMU_CORE (0x12000000) */
@@ -920,8 +929,12 @@ static int __init exynos850_cmu_probe(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 
 	info = of_device_get_match_data(dev);
-	exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
-	samsung_cmu_register_one(np, info);
+
+	/* Early clocks are already registered using CLK_OF_DECLARE_DRIVER() */
+	if (info != &peri_cmu_info) {
+		exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
+		samsung_cmu_register_one(np, info);
+	}
 
 	/* Keep bus clock running, so it's possible to access CMU registers */
 	if (info->clk_name) {
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early
  2021-10-25 16:12 ` [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early Sam Protsenko
@ 2021-10-26 14:53   ` Krzysztof Kozlowski
  2021-11-20 12:49   ` Sylwester Nawrocki
  1 sibling, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-26 14:53 UTC (permalink / raw)
  To: Sam Protsenko, Sylwester Nawrocki, Paweł Chmiel,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette
  Cc: Sumit Semwal, devicetree, linux-arm-kernel, linux-clk,
	linux-kernel, linux-samsung-soc

On 25/10/2021 18:12, Sam Protsenko wrote:
> Some clocks must be registered before init calls. For example MCT clock
> (from CMU_PERI) is needed for MCT timer driver, which is registered
> with TIMER_OF_DECLARE(). By the time we get to core_initcall() used for
> clk-exynos850 platform driver init, it's already too late. Inability to
> get "mct" clock in MCT driver leads to kernel panic, as functions
> registered with *_OF_DECLARE() can't do deferred calls. MCT timer driver
> can't be fixed either, as it's acting as a clock source and it's
> essential to register it in start_kernel() -> time_init().
> 
> Let's register CMU_PERI clocks early, using CLK_OF_DECLARE_DRIVER(), and
> do all stuff relying on "struct dev" object (like runtime PM and
> enabling bus clock) later in platform driver probe. Basically
> CLK_OF_DECLARE_DRIVER() matches CMU compatible, but clears OF_POPULATED
> flag, which allows the same device to be matched again later.
> 
> Similar issue was discussed at [1] and addressed in commit 1f7db7bbf031
> ("clk: renesas: cpg-mssr: Add early clock support"), as well as in
> drivers/clk/mediatek/clk-mt2712.c.
> 
> [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20180829132954.64862-2-chris.brandt@renesas.com/
> 
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v2:
>   - Register only CMU_PERI clocks early
> 
> Notes:
>   - This patch should be applied on top of CMU_APM series
>     (clk: samsung: exynos850: Implement CMU_APM domain)
> 
>  drivers/clk/samsung/clk-exynos850.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early
  2021-10-25 16:12 ` [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early Sam Protsenko
  2021-10-26 14:53   ` Krzysztof Kozlowski
@ 2021-11-20 12:49   ` Sylwester Nawrocki
  2021-11-20 16:47     ` Sam Protsenko
  1 sibling, 1 reply; 8+ messages in thread
From: Sylwester Nawrocki @ 2021-11-20 12:49 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Sumit Semwal, devicetree, linux-arm-kernel, linux-clk,
	linux-kernel, linux-samsung-soc, Krzysztof Kozlowski,
	Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette

On 25.10.2021 18:12, Sam Protsenko wrote:
> Some clocks must be registered before init calls. For example MCT clock
> (from CMU_PERI) is needed for MCT timer driver, which is registered
> with TIMER_OF_DECLARE(). By the time we get to core_initcall() used for
> clk-exynos850 platform driver init, it's already too late. Inability to
> get "mct" clock in MCT driver leads to kernel panic, as functions
> registered with *_OF_DECLARE() can't do deferred calls. MCT timer driver
> can't be fixed either, as it's acting as a clock source and it's
> essential to register it in start_kernel() -> time_init().
> 
> Let's register CMU_PERI clocks early, using CLK_OF_DECLARE_DRIVER(), and
> do all stuff relying on "struct dev" object (like runtime PM and
> enabling bus clock) later in platform driver probe. Basically
> CLK_OF_DECLARE_DRIVER() matches CMU compatible, but clears OF_POPULATED
> flag, which allows the same device to be matched again later.

> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>

>  drivers/clk/samsung/clk-exynos850.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
> index 95e373d17b42..ecffa5c7a081 100644
> --- a/drivers/clk/samsung/clk-exynos850.c
> +++ b/drivers/clk/samsung/clk-exynos850.c
> @@ -753,6 +753,15 @@ static const struct samsung_cmu_info peri_cmu_info __initconst = {
>  	.clk_name		= "dout_peri_bus",
>  };
>  
> +static void __init exynos850_cmu_peri_init(struct device_node *np)
> +{
> +	exynos850_init_clocks(np, peri_clk_regs, ARRAY_SIZE(peri_clk_regs));
> +	samsung_cmu_register_one(np, &peri_cmu_info);
> +}
> +
> +CLK_OF_DECLARE_DRIVER(exynos850_cmu_peri, "samsung,exynos850-cmu-peri",
> +		      exynos850_cmu_peri_init);
> +
>  /* ---- CMU_CORE ------------------------------------------------------------ */
>  
>  /* Register Offset definitions for CMU_CORE (0x12000000) */
> @@ -920,8 +929,12 @@ static int __init exynos850_cmu_probe(struct platform_device *pdev)
>  	struct device_node *np = dev->of_node;
>  
>  	info = of_device_get_match_data(dev);
> -	exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
> -	samsung_cmu_register_one(np, info);
> +
> +	/* Early clocks are already registered using CLK_OF_DECLARE_DRIVER() */
> +	if (info != &peri_cmu_info) {
> +		exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
> +		samsung_cmu_register_one(np, info);
> +	}

Don't you also need to register early CMU_TOP, which provides clocks
for CMU_PERI? I'm afraid it might not work properly when you register 
CMU_PERI clocks early and only later in probe() you enable parent clock
required for the already registered clocks to be usable.
How about registering also CMU_TOP early and enabling parent clock
also in OF_CLK_DECLARE init callback, i.e. using either OF_CLK_DECLARE
or platform driver for a CMU? 

-- 
Regards,
Sylwester

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early
  2021-11-20 12:49   ` Sylwester Nawrocki
@ 2021-11-20 16:47     ` Sam Protsenko
  2021-11-20 17:38       ` Sylwester Nawrocki
  0 siblings, 1 reply; 8+ messages in thread
From: Sam Protsenko @ 2021-11-20 16:47 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Sumit Semwal, devicetree, linux-arm-kernel, linux-clk,
	linux-kernel, linux-samsung-soc, Krzysztof Kozlowski,
	Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette

On Sat, 20 Nov 2021 at 14:49, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>
> On 25.10.2021 18:12, Sam Protsenko wrote:
> > Some clocks must be registered before init calls. For example MCT clock
> > (from CMU_PERI) is needed for MCT timer driver, which is registered
> > with TIMER_OF_DECLARE(). By the time we get to core_initcall() used for
> > clk-exynos850 platform driver init, it's already too late. Inability to
> > get "mct" clock in MCT driver leads to kernel panic, as functions
> > registered with *_OF_DECLARE() can't do deferred calls. MCT timer driver
> > can't be fixed either, as it's acting as a clock source and it's
> > essential to register it in start_kernel() -> time_init().
> >
> > Let's register CMU_PERI clocks early, using CLK_OF_DECLARE_DRIVER(), and
> > do all stuff relying on "struct dev" object (like runtime PM and
> > enabling bus clock) later in platform driver probe. Basically
> > CLK_OF_DECLARE_DRIVER() matches CMU compatible, but clears OF_POPULATED
> > flag, which allows the same device to be matched again later.
>
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
>
> >  drivers/clk/samsung/clk-exynos850.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
> > index 95e373d17b42..ecffa5c7a081 100644
> > --- a/drivers/clk/samsung/clk-exynos850.c
> > +++ b/drivers/clk/samsung/clk-exynos850.c
> > @@ -753,6 +753,15 @@ static const struct samsung_cmu_info peri_cmu_info __initconst = {
> >       .clk_name               = "dout_peri_bus",
> >  };
> >
> > +static void __init exynos850_cmu_peri_init(struct device_node *np)
> > +{
> > +     exynos850_init_clocks(np, peri_clk_regs, ARRAY_SIZE(peri_clk_regs));
> > +     samsung_cmu_register_one(np, &peri_cmu_info);
> > +}
> > +
> > +CLK_OF_DECLARE_DRIVER(exynos850_cmu_peri, "samsung,exynos850-cmu-peri",
> > +                   exynos850_cmu_peri_init);
> > +
> >  /* ---- CMU_CORE ------------------------------------------------------------ */
> >
> >  /* Register Offset definitions for CMU_CORE (0x12000000) */
> > @@ -920,8 +929,12 @@ static int __init exynos850_cmu_probe(struct platform_device *pdev)
> >       struct device_node *np = dev->of_node;
> >
> >       info = of_device_get_match_data(dev);
> > -     exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
> > -     samsung_cmu_register_one(np, info);
> > +
> > +     /* Early clocks are already registered using CLK_OF_DECLARE_DRIVER() */
> > +     if (info != &peri_cmu_info) {
> > +             exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
> > +             samsung_cmu_register_one(np, info);
> > +     }
>
> Don't you also need to register early CMU_TOP, which provides clocks
> for CMU_PERI? I'm afraid it might not work properly when you register
> CMU_PERI clocks early and only later in probe() you enable parent clock
> required for the already registered clocks to be usable.

Good point, I'll do that in v2. Not sure how I missed that dependency
point, but thank you for noticing that. Guess it only works for me
because clocks are already enabled in bootloader, and I'm using
"clk_ignore_unused" param for now.

> How about registering also CMU_TOP early and enabling parent clock
> also in OF_CLK_DECLARE init callback, i.e. using either OF_CLK_DECLARE
> or platform driver for a CMU?
>

If you mean doing clk_prepare_enable() for "dout_peri_bus" clock in
exynos850_cmu_peri_init(), I don't think it's possible. clk_get()
needs "struct device *dev", and we only have that in platform driver
probe. Trying to pass dev=NULL won't work, so that's why I'm enabling
parent clocks in platform driver probe.

I'm going to submit new patch series soon. It'll include all my recent
patches, addressing all your comments. We can continue discussion
there, in case I misunderstood you and those patches are still not
correct.

Thanks!

> --
> Regards,
> Sylwester

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early
  2021-11-20 16:47     ` Sam Protsenko
@ 2021-11-20 17:38       ` Sylwester Nawrocki
  2021-11-21 22:50         ` Sam Protsenko
  0 siblings, 1 reply; 8+ messages in thread
From: Sylwester Nawrocki @ 2021-11-20 17:38 UTC (permalink / raw)
  To: Sam Protsenko
  Cc: Sumit Semwal, devicetree, linux-arm-kernel, linux-clk,
	linux-kernel, linux-samsung-soc, Krzysztof Kozlowski,
	Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette, Sylwester Nawrocki

On 20.11.2021 17:47, Sam Protsenko wrote:
>>> @@ -920,8 +929,12 @@ static int __init exynos850_cmu_probe(struct platform_device *pdev)
>>>        struct device_node *np = dev->of_node;
>>>
>>>        info = of_device_get_match_data(dev);
>>> -     exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
>>> -     samsung_cmu_register_one(np, info);
>>> +
>>> +     /* Early clocks are already registered using CLK_OF_DECLARE_DRIVER() */
>>> +     if (info != &peri_cmu_info) {
>>> +             exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
>>> +             samsung_cmu_register_one(np, info);
>>> +     }
>> Don't you also need to register early CMU_TOP, which provides clocks
>> for CMU_PERI? I'm afraid it might not work properly when you register
>> CMU_PERI clocks early and only later in probe() you enable parent clock
>> required for the already registered clocks to be usable.

> Good point, I'll do that in v2. Not sure how I missed that dependency
> point, but thank you for noticing that. Guess it only works for me
> because clocks are already enabled in bootloader, and I'm using
> "clk_ignore_unused" param for now.
> 
>> How about registering also CMU_TOP early and enabling parent clock
>> also in OF_CLK_DECLARE init callback, i.e. using either OF_CLK_DECLARE
>> or platform driver for a CMU?
>>
> If you mean doing clk_prepare_enable() for "dout_peri_bus" clock in
> exynos850_cmu_peri_init(), I don't think it's possible. clk_get()
> needs "struct device *dev", and we only have that in platform driver
> probe. Trying to pass dev=NULL won't work, so that's why I'm enabling
> parent clocks in platform driver probe.

Sorry, I didn't notice it earlier, actually CMU_TOP is already being
initialized with OF_CLK_DECLARE.

You could use of_clk_get() to get the clock, the consumer clock indexes
are fixed and defined in the DT binding. There is also
of_clk_get_by_name() which works similarly to clk_get().



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early
  2021-11-20 17:38       ` Sylwester Nawrocki
@ 2021-11-21 22:50         ` Sam Protsenko
  0 siblings, 0 replies; 8+ messages in thread
From: Sam Protsenko @ 2021-11-21 22:50 UTC (permalink / raw)
  To: Sylwester Nawrocki
  Cc: Sumit Semwal, devicetree, linux-arm-kernel, linux-clk,
	linux-kernel, linux-samsung-soc, Krzysztof Kozlowski,
	Paweł Chmiel, Chanwoo Choi, Tomasz Figa, Rob Herring,
	Stephen Boyd, Michael Turquette, Sylwester Nawrocki

On Sat, 20 Nov 2021 at 19:38, Sylwester Nawrocki <snawrocki@kernel.org> wrote:
>
> On 20.11.2021 17:47, Sam Protsenko wrote:
> >>> @@ -920,8 +929,12 @@ static int __init exynos850_cmu_probe(struct platform_device *pdev)
> >>>        struct device_node *np = dev->of_node;
> >>>
> >>>        info = of_device_get_match_data(dev);
> >>> -     exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
> >>> -     samsung_cmu_register_one(np, info);
> >>> +
> >>> +     /* Early clocks are already registered using CLK_OF_DECLARE_DRIVER() */
> >>> +     if (info != &peri_cmu_info) {
> >>> +             exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
> >>> +             samsung_cmu_register_one(np, info);
> >>> +     }
> >> Don't you also need to register early CMU_TOP, which provides clocks
> >> for CMU_PERI? I'm afraid it might not work properly when you register
> >> CMU_PERI clocks early and only later in probe() you enable parent clock
> >> required for the already registered clocks to be usable.
>
> > Good point, I'll do that in v2. Not sure how I missed that dependency
> > point, but thank you for noticing that. Guess it only works for me
> > because clocks are already enabled in bootloader, and I'm using
> > "clk_ignore_unused" param for now.
> >
> >> How about registering also CMU_TOP early and enabling parent clock
> >> also in OF_CLK_DECLARE init callback, i.e. using either OF_CLK_DECLARE
> >> or platform driver for a CMU?
> >>
> > If you mean doing clk_prepare_enable() for "dout_peri_bus" clock in
> > exynos850_cmu_peri_init(), I don't think it's possible. clk_get()
> > needs "struct device *dev", and we only have that in platform driver
> > probe. Trying to pass dev=NULL won't work, so that's why I'm enabling
> > parent clocks in platform driver probe.
>
> Sorry, I didn't notice it earlier, actually CMU_TOP is already being
> initialized with OF_CLK_DECLARE.
>

No worries, I forgot about that too, as you can see :-D But yeah, I'll
keep that as is then. So I'll only make parent clock enabled early.

> You could use of_clk_get() to get the clock, the consumer clock indexes
> are fixed and defined in the DT binding. There is also
> of_clk_get_by_name() which works similarly to clk_get().
>

Thanks for the suggestion! I'll go with of_clk_get_by_name(), it's
more natural as we already have parent clock name stored, so it can be
used in more general way.

This patch is superseded by next series (going to submit that soon):
    [PATCH 0/6] clk: samsung: exynos850: Clock driver improvements

>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early
  2021-11-22 14:42 ` Sam Protsenko
@ 2021-11-22 22:04   ` Sylwester Nawrocki
  0 siblings, 0 replies; 8+ messages in thread
From: Sylwester Nawrocki @ 2021-11-22 22:04 UTC (permalink / raw)
  To: Sam Protsenko, linux-clk
  Cc: David Virag, Paweł Chmiel, devicetree, linux-arm-kernel,
	linux-kernel, linux-samsung-soc, Krzysztof Kozlowski,
	Chanwoo Choi, Tomasz Figa, Rob Herring, Stephen Boyd,
	Michael Turquette

On 22.11.2021 15:42, Sam Protsenko wrote:
> Some clocks must be registered before init calls. For example MCT clock
> (from CMU_PERI) is needed for MCT timer driver, which is registered
> with TIMER_OF_DECLARE(). By the time we get to core_initcall() used for
> clk-exynos850 platform driver init, it's already too late. Inability to
> get "mct" clock in MCT driver leads to kernel panic, as functions
> registered with *_OF_DECLARE() can't do deferred calls. MCT timer driver
> can't be fixed either, as it's acting as a clock source and it's
> essential to register it in start_kernel() -> time_init().
> 
> Let's register CMU_PERI clocks early, using CLK_OF_DECLARE(). CMU_TOP
> generates clocks needed for CMU_PERI, but it's already registered early.
> 
> While at it, let's cleanup the code a bit, by extracting everything
> related to CMU initialization and registration to the separate function.

Applied, thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early
@ 2021-11-22 14:42 ` Sam Protsenko
  2021-11-22 22:04   ` Sylwester Nawrocki
  0 siblings, 1 reply; 8+ messages in thread
From: Sam Protsenko @ 2021-11-22 14:42 UTC (permalink / raw)
  To: Sylwester Nawrocki, Krzysztof Kozlowski, Chanwoo Choi,
	Tomasz Figa, Rob Herring, Stephen Boyd, Michael Turquette
  Cc: David Virag, Paweł Chmiel, devicetree, linux-arm-kernel,
	linux-clk, linux-kernel, linux-samsung-soc

Some clocks must be registered before init calls. For example MCT clock
(from CMU_PERI) is needed for MCT timer driver, which is registered
with TIMER_OF_DECLARE(). By the time we get to core_initcall() used for
clk-exynos850 platform driver init, it's already too late. Inability to
get "mct" clock in MCT driver leads to kernel panic, as functions
registered with *_OF_DECLARE() can't do deferred calls. MCT timer driver
can't be fixed either, as it's acting as a clock source and it's
essential to register it in start_kernel() -> time_init().

Let's register CMU_PERI clocks early, using CLK_OF_DECLARE(). CMU_TOP
generates clocks needed for CMU_PERI, but it's already registered early.

While at it, let's cleanup the code a bit, by extracting everything
related to CMU initialization and registration to the separate function.

Similar issue was discussed at [1] and addressed in commit 1f7db7bbf031
("clk: renesas: cpg-mssr: Add early clock support"), as well as in
drivers/clk/mediatek/clk-mt2712.c.

[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20180829132954.64862-2-chris.brandt@renesas.com/

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
Changes in v2:
  - Use of_clk_get_by_name() API to get parent clock when registering
    CMU early (as suggested by Sylwester Nawrocki)
  - Extracted CMU registration code to separate routine
  - Use CLK_OF_DECLARE() for CMU_PERI init, instead of
    CLK_OF_DECLARE_DRIVER(), as parent clock now can be enabled early
  - Remove "samsung,exynos850-cmu-peri" from exynos850_cmu_of_match[]
  - Add some comments for early domains

 drivers/clk/samsung/clk-exynos850.c | 70 ++++++++++++++++++++---------
 1 file changed, 49 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos850.c b/drivers/clk/samsung/clk-exynos850.c
index 04a8d202c939..568ac97c8120 100644
--- a/drivers/clk/samsung/clk-exynos850.c
+++ b/drivers/clk/samsung/clk-exynos850.c
@@ -60,6 +60,43 @@ static void __init exynos850_init_clocks(struct device_node *np,
 	iounmap(reg_base);
 }
 
+/**
+ * exynos850_register_cmu - Register specified Exynos850 CMU domain
+ * @dev:	Device object; may be NULL if this function is not being
+ *		called from platform driver probe function
+ * @np:		CMU device tree node
+ * @cmu:	CMU data
+ *
+ * Register specified CMU domain, which includes next steps:
+ *
+ * 1. Enable parent clock of @cmu CMU
+ * 2. Set initial registers configuration for @cmu CMU clocks
+ * 3. Register @cmu CMU clocks using Samsung clock framework API
+ */
+static void __init exynos850_register_cmu(struct device *dev,
+		struct device_node *np, const struct samsung_cmu_info *cmu)
+{
+	/* Keep CMU parent clock running (needed for CMU registers access) */
+	if (cmu->clk_name) {
+		struct clk *parent_clk;
+
+		if (dev)
+			parent_clk = clk_get(dev, cmu->clk_name);
+		else
+			parent_clk = of_clk_get_by_name(np, cmu->clk_name);
+
+		if (IS_ERR(parent_clk)) {
+			pr_err("%s: could not find bus clock %s; err = %ld\n",
+			       __func__, cmu->clk_name, PTR_ERR(parent_clk));
+		} else {
+			clk_prepare_enable(parent_clk);
+		}
+	}
+
+	exynos850_init_clocks(np, cmu->clk_regs, cmu->nr_clk_regs);
+	samsung_cmu_register_one(np, cmu);
+}
+
 /* ---- CMU_TOP ------------------------------------------------------------- */
 
 /* Register Offset definitions for CMU_TOP (0x120e0000) */
@@ -367,10 +404,10 @@ static const struct samsung_cmu_info top_cmu_info __initconst = {
 
 static void __init exynos850_cmu_top_init(struct device_node *np)
 {
-	exynos850_init_clocks(np, top_clk_regs, ARRAY_SIZE(top_clk_regs));
-	samsung_cmu_register_one(np, &top_cmu_info);
+	exynos850_register_cmu(NULL, np, &top_cmu_info);
 }
 
+/* Register CMU_TOP early, as it's a dependency for other early domains */
 CLK_OF_DECLARE(exynos850_cmu_top, "samsung,exynos850-cmu-top",
 	       exynos850_cmu_top_init);
 
@@ -853,6 +890,15 @@ static const struct samsung_cmu_info peri_cmu_info __initconst = {
 	.clk_name		= "dout_peri_bus",
 };
 
+static void __init exynos850_cmu_peri_init(struct device_node *np)
+{
+	exynos850_register_cmu(NULL, np, &peri_cmu_info);
+}
+
+/* Register CMU_PERI early, as it's needed for MCT timer */
+CLK_OF_DECLARE(exynos850_cmu_peri, "samsung,exynos850-cmu-peri",
+	       exynos850_cmu_peri_init);
+
 /* ---- CMU_CORE ------------------------------------------------------------ */
 
 /* Register Offset definitions for CMU_CORE (0x12000000) */
@@ -1021,24 +1067,9 @@ static int __init exynos850_cmu_probe(struct platform_device *pdev)
 {
 	const struct samsung_cmu_info *info;
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
 
 	info = of_device_get_match_data(dev);
-	exynos850_init_clocks(np, info->clk_regs, info->nr_clk_regs);
-	samsung_cmu_register_one(np, info);
-
-	/* Keep bus clock running, so it's possible to access CMU registers */
-	if (info->clk_name) {
-		struct clk *bus_clk;
-
-		bus_clk = clk_get(dev, info->clk_name);
-		if (IS_ERR(bus_clk)) {
-			pr_err("%s: could not find bus clock %s; err = %ld\n",
-			       __func__, info->clk_name, PTR_ERR(bus_clk));
-		} else {
-			clk_prepare_enable(bus_clk);
-		}
-	}
+	exynos850_register_cmu(dev, dev->of_node, info);
 
 	return 0;
 }
@@ -1053,9 +1084,6 @@ static const struct of_device_id exynos850_cmu_of_match[] = {
 	}, {
 		.compatible = "samsung,exynos850-cmu-hsi",
 		.data = &hsi_cmu_info,
-	}, {
-		.compatible = "samsung,exynos850-cmu-peri",
-		.data = &peri_cmu_info,
 	}, {
 		.compatible = "samsung,exynos850-cmu-core",
 		.data = &core_cmu_info,
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-11-22 22:06 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20211025161302eucas1p2f50ef29a0bba69c13deaf1ad31a8439c@eucas1p2.samsung.com>
2021-10-25 16:12 ` [PATCH v2 1/1] clk: samsung: exynos850: Register clocks early Sam Protsenko
2021-10-26 14:53   ` Krzysztof Kozlowski
2021-11-20 12:49   ` Sylwester Nawrocki
2021-11-20 16:47     ` Sam Protsenko
2021-11-20 17:38       ` Sylwester Nawrocki
2021-11-21 22:50         ` Sam Protsenko
     [not found] <CGME20211122144217eucas1p21c5f4930563ee051d625bb8e3a932a4a@eucas1p2.samsung.com>
2021-11-22 14:42 ` Sam Protsenko
2021-11-22 22:04   ` Sylwester Nawrocki

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