clk: samsung: Use clk_hw API for calling clk framework from clk notifiers

Message ID 20181002115210.15541-1-m.szyprowski@samsung.com
State New
Headers show
Series
  • clk: samsung: Use clk_hw API for calling clk framework from clk notifiers
Related show

Commit Message

Marek Szyprowski Oct. 2, 2018, 11:52 a.m.
clk_notifier_register() documentation states, that the provided notifier
callbacks associated with the notifier must not re-enter into the clk
framework by calling any top-level clk APIs. Fix this by replacing
clk_get_rate() calls with clk_hw_get_rate(), which is safe in this
context.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
 drivers/clk/samsung/clk-cpu.c | 6 +++---
 drivers/clk/samsung/clk-cpu.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.17.1

Comments

Sylwester Nawrocki Oct. 3, 2018, 9:03 p.m. | #1
On 10/02/2018 01:52 PM, Marek Szyprowski wrote:
> clk_notifier_register() documentation states, that the provided notifier

> callbacks associated with the notifier must not re-enter into the clk

> framework by calling any top-level clk APIs. Fix this by replacing

> clk_get_rate() calls with clk_hw_get_rate(), which is safe in this

> context.

> 

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>


Thanks for fixing this, I have applied the patch to samsung-clk tree.
Stephen Boyd Oct. 8, 2018, 2:47 a.m. | #2
Quoting Marek Szyprowski (2018-10-02 04:52:10)
> @@ -432,7 +432,7 @@ int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,

>         else

>                 cpuclk->clk_nb.notifier_call = exynos_cpuclk_notifier_cb;

>  

> -       cpuclk->alt_parent = __clk_lookup(alt_parent);

> +       cpuclk->alt_parent = __clk_get_hw(__clk_lookup(alt_parent));


It would be nice to get rid of __clk_lookup() from here too. Please try
to do that if at all possible.

>         if (!cpuclk->alt_parent) {

>                 pr_err("%s: could not lookup alternate parent %s\n",

>                                 __func__, alt_parent);
Marek Szyprowski Oct. 8, 2018, 2:51 p.m. | #3
Hi Stephen,

On 2018-10-08 04:47, Stephen Boyd wrote:
> Quoting Marek Szyprowski (2018-10-02 04:52:10)

>> @@ -432,7 +432,7 @@ int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,

>>         else

>>                 cpuclk->clk_nb.notifier_call = exynos_cpuclk_notifier_cb;

>>  

>> -       cpuclk->alt_parent = __clk_lookup(alt_parent);

>> +       cpuclk->alt_parent = __clk_get_hw(__clk_lookup(alt_parent));

> It would be nice to get rid of __clk_lookup() from here too. Please try

> to do that if at all possible.


So far it won't be possible to get rid of __clk_lookup() completely from
samsung/clk-cpu.c, because there is a need to register clk notifier for
CPU clock, but there is no clk_hw_notifier_register() api. I'm not sure if
this is the right way to add it.

> ...


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Stephen Boyd Oct. 9, 2018, 10:52 p.m. | #4
Quoting Marek Szyprowski (2018-10-08 07:51:30)
> Hi Stephen,

> 

> On 2018-10-08 04:47, Stephen Boyd wrote:

> > Quoting Marek Szyprowski (2018-10-02 04:52:10)

> >> @@ -432,7 +432,7 @@ int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,

> >>         else

> >>                 cpuclk->clk_nb.notifier_call = exynos_cpuclk_notifier_cb;

> >>  

> >> -       cpuclk->alt_parent = __clk_lookup(alt_parent);

> >> +       cpuclk->alt_parent = __clk_get_hw(__clk_lookup(alt_parent));

> > It would be nice to get rid of __clk_lookup() from here too. Please try

> > to do that if at all possible.

> 

> So far it won't be possible to get rid of __clk_lookup() completely from

> samsung/clk-cpu.c, because there is a need to register clk notifier for

> CPU clock, but there is no clk_hw_notifier_register() api. I'm not sure if

> this is the right way to add it.

> 


Probably there needs to be some way to indicate that the alt_parent is
going here and then grab the clk_hw pointer to it. Instead of looking up
a clk by random string name.

I don't really care about the clk_notifier_register() API, that's fine.
Eventually I'd like to see that API not be used by clk provider drivers.

Patch

diff --git a/drivers/clk/samsung/clk-cpu.c b/drivers/clk/samsung/clk-cpu.c
index d2c99d8916b8..a5fddebbe530 100644
--- a/drivers/clk/samsung/clk-cpu.c
+++ b/drivers/clk/samsung/clk-cpu.c
@@ -152,7 +152,7 @@  static int exynos_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
 			struct exynos_cpuclk *cpuclk, void __iomem *base)
 {
 	const struct exynos_cpuclk_cfg_data *cfg_data = cpuclk->cfg;
-	unsigned long alt_prate = clk_get_rate(cpuclk->alt_parent);
+	unsigned long alt_prate = clk_hw_get_rate(cpuclk->alt_parent);
 	unsigned long alt_div = 0, alt_div_mask = DIV_MASK;
 	unsigned long div0, div1 = 0, mux_reg;
 	unsigned long flags;
@@ -280,7 +280,7 @@  static int exynos5433_cpuclk_pre_rate_change(struct clk_notifier_data *ndata,
 			struct exynos_cpuclk *cpuclk, void __iomem *base)
 {
 	const struct exynos_cpuclk_cfg_data *cfg_data = cpuclk->cfg;
-	unsigned long alt_prate = clk_get_rate(cpuclk->alt_parent);
+	unsigned long alt_prate = clk_hw_get_rate(cpuclk->alt_parent);
 	unsigned long alt_div = 0, alt_div_mask = DIV_MASK;
 	unsigned long div0, div1 = 0, mux_reg;
 	unsigned long flags;
@@ -432,7 +432,7 @@  int __init exynos_register_cpu_clock(struct samsung_clk_provider *ctx,
 	else
 		cpuclk->clk_nb.notifier_call = exynos_cpuclk_notifier_cb;
 
-	cpuclk->alt_parent = __clk_lookup(alt_parent);
+	cpuclk->alt_parent = __clk_get_hw(__clk_lookup(alt_parent));
 	if (!cpuclk->alt_parent) {
 		pr_err("%s: could not lookup alternate parent %s\n",
 				__func__, alt_parent);
diff --git a/drivers/clk/samsung/clk-cpu.h b/drivers/clk/samsung/clk-cpu.h
index d4b6b517fe1b..bd38c6aa3897 100644
--- a/drivers/clk/samsung/clk-cpu.h
+++ b/drivers/clk/samsung/clk-cpu.h
@@ -49,7 +49,7 @@  struct exynos_cpuclk_cfg_data {
  */
 struct exynos_cpuclk {
 	struct clk_hw				hw;
-	struct clk				*alt_parent;
+	struct clk_hw				*alt_parent;
 	void __iomem				*ctrl_base;
 	spinlock_t				*lock;
 	const struct exynos_cpuclk_cfg_data	*cfg;