[PATCH v1 0/1] clk: npcm7xx: fix base address and of_clk_get_by_name

[PATCH v1 0/1] clk: npcm7xx: fix base address and of_clk_get_by_name

[tali.perry1]

From: Tali Perry <tali.perry1@gmail.com>

Nuvoton NPCM7XX Clock Controller
fix base address and of_clk_get_by_name error handling.
    Also update error messages to be more informative.
    
    In case clk_base allocation is erronoeous the return value is null.
    Also fix handling of of_clk_get_by_name returns an error. 
    Print a better error message pointing to the dt-binding documention.

This patch is re-submitted since it was already pushed to main 
(just the diff, without the full driver which is already in 
master branch.)


Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

---

Tali Perry (1):
  clk: npcm7xx: get fixed clocks from DT

 drivers/clk/clk-npcm7xx.c | 99 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 75 insertions(+), 24 deletions(-)

-- 
2.14.1

[PATCH v1 1/1] clk: npcm7xx: get fixed clocks from DT

[tali.perry1]

From: Tali Perry <tali.perry1@gmail.com>

Nuvoton NPCM7XX Clock Controller
fix base address and of_clk_get_by_name error handling.
    Also update error messages to be more informative.
    
    In case clk_base allocation is erronoeous the return value is null.
    Also fix handling of of_clk_get_by_name returns an error. 
    Print a better error message pointing to the dt-binding documention.

This patch is re-submitted since it was already pushed to main 
(just the diff, without the full driver which is already in 
master branch.)


Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

---
 drivers/clk/clk-npcm7xx.c | 99 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 75 insertions(+), 24 deletions(-)

diff --git a/drivers/clk/clk-npcm7xx.c b/drivers/clk/clk-npcm7xx.c
index 740af90a9508..91a937720f4e 100644
--- a/drivers/clk/clk-npcm7xx.c
+++ b/drivers/clk/clk-npcm7xx.c
@@ -7,17 +7,26 @@
  * Copyright (C) 2018 Nuvoton Technologies tali.perry@nuvoton.com
  */
 
-#include <linux/module.h>
-#include <linux/clk-provider.h>
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/slab.h>
-#include <linux/err.h>
-#include <linux/bitfield.h>
-
-#include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
+
+
+
+ #include <linux/module.h>
+ #include <linux/clk.h>
+ #include <linux/clk-provider.h>
+ #include <linux/device.h>
+ #include <linux/io.h>
+ #include <linux/kernel.h>
+ #include <linux/of.h>
+ #include <linux/of_device.h>
+ #include <linux/of_platform.h>
+ #include <linux/of_address.h>
+ #include <linux/platform_device.h>
+ #include <linux/slab.h>
+ #include <linux/err.h>
+ #include <linux/rational.h>
+ #include <linux/bitfield.h>
+ #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
+
 
 struct npcm7xx_clk_pll {
 	struct clk_hw	hw;
@@ -27,6 +36,9 @@ struct npcm7xx_clk_pll {
 
 #define to_npcm7xx_clk_pll(_hw) container_of(_hw, struct npcm7xx_clk_pll, hw)
 
+static struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon,
+	const char *name, const char *parent_name, unsigned long flags);
+
 #define PLLCON_LOKI	BIT(31)
 #define PLLCON_LOKS	BIT(30)
 #define PLLCON_FBDV	GENMASK(27, 16)
@@ -44,7 +56,8 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
 	u64 ret;
 
 	if (parent_rate == 0) {
-		pr_err("%s: parent rate is zero", __func__);
+		pr_err("%s: parent rate is zero. reg=%x\n", __func__,
+			(u32)(pll->pllcon));
 		return 0;
 	}
 
@@ -61,13 +74,12 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
 	return ret;
 }
 
-static const struct clk_ops npcm7xx_clk_pll_ops = {
+const struct clk_ops npcm7xx_clk_pll_ops = {
 	.recalc_rate = npcm7xx_clk_pll_recalc_rate,
 };
 
-static struct clk_hw *
-npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
-			 const char *parent_name, unsigned long flags)
+static struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon,
+		const char *name, const char *parent_name, unsigned long flags)
 {
 	struct npcm7xx_clk_pll *pll;
 	struct clk_init_data init;
@@ -78,7 +90,8 @@ npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
 	if (!pll)
 		return ERR_PTR(-ENOMEM);
 
-	pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name);
+	pr_debug("%s reg, reg=0x%x, name=%s, p=%s\n",
+		__func__, (unsigned int)pllcon, name, parent_name);
 
 	init.name = name;
 	init.ops = &npcm7xx_clk_pll_ops;
@@ -544,9 +557,11 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 	void __iomem *clk_base;
 	struct resource res;
 	struct clk_hw *hw;
+	struct clk *clk;
 	int ret;
 	int i;
 
+	clk_base = NULL;
 	ret = of_address_to_resource(clk_np, 0, &res);
 	if (ret) {
 		pr_err("%s: failed to get resource, ret %d\n", clk_np->name,
@@ -560,14 +575,44 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 
 	npcm7xx_clk_data = kzalloc(sizeof(*npcm7xx_clk_data->hws) *
 		NPCM7XX_NUM_CLOCKS + sizeof(npcm7xx_clk_data), GFP_KERNEL);
-	if (!npcm7xx_clk_data)
+
+	npcm7xx_clk_data->num = 0;
+
+	if (!npcm7xx_clk_data->hws) {
+		pr_err("Can't alloc npcm7xx_clk_data\n");
 		goto npcm7xx_init_np_err;
+	}
 
 	npcm7xx_clk_data->num = NPCM7XX_NUM_CLOCKS;
 
 	for (i = 0; i < NPCM7XX_NUM_CLOCKS; i++)
 		npcm7xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
 
+	/* Read fixed clocks. These 3 clocks must be defined in DT */
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_REFCLK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external REFCLK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_SYSBYPCK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external SYSBYPCK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_MCBYPCK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external MCBYPCK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
 	/* Register plls */
 	for (i = 0; i < ARRAY_SIZE(npcm7xx_plls); i++) {
 		const struct npcm7xx_clk_pll_data *pll_data = &npcm7xx_plls[i];
@@ -584,16 +629,16 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 	}
 
 	/* Register fixed dividers */
-	hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,
+	clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,
 			NPCM7XX_CLK_S_PLL1, 0, 1, 2);
-	if (IS_ERR(hw)) {
+	if (IS_ERR(clk)) {
 		pr_err("npcm7xx_clk: Can't register fixed div\n");
 		goto npcm7xx_init_fail;
 	}
 
-	hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
+	clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
 			NPCM7XX_CLK_S_PLL2, 0, 1, 2);
-	if (IS_ERR(hw)) {
+	if (IS_ERR(clk)) {
 		pr_err("npcm7xx_clk: Can't register div2\n");
 		goto npcm7xx_init_fail;
 	}
@@ -646,11 +691,17 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 
 	return;
 
+npcm7xx_init_fail_no_clk_on_dt:
+	pr_err("see Documentation/devicetree/bindings/clock/");
+	pr_err("nuvoton,npcm750-clk.txt  for details\n");
 npcm7xx_init_fail:
-	kfree(npcm7xx_clk_data->hws);
+	if (npcm7xx_clk_data->num)
+		kfree(npcm7xx_clk_data->hws);
 npcm7xx_init_np_err:
-	iounmap(clk_base);
+	if (clk_base != NULL)
+		iounmap(clk_base);
 npcm7xx_init_error:
 	of_node_put(clk_np);
+	pr_err("clk setup fail\n");
 }
 CLK_OF_DECLARE(npcm7xx_clk_init, "nuvoton,npcm750-clk", npcm7xx_clk_init);
-- 
2.14.1

Re: [PATCH v1 1/1] clk: npcm7xx: get fixed clocks from DT

[Stephen Boyd]

Quoting tali.perry1@gmail.com (2018-11-11 05:47:12)
> From: Tali Perry <tali.perry1@gmail.com>
> 
> Nuvoton NPCM7XX Clock Controller
> fix base address and of_clk_get_by_name error handling.
>     Also update error messages to be more informative.
>     
>     In case clk_base allocation is erronoeous the return value is null.
>     Also fix handling of of_clk_get_by_name returns an error. 
>     Print a better error message pointing to the dt-binding documention.
> 
> This patch is re-submitted since it was already pushed to main 
> (just the diff, without the full driver which is already in 
> master branch.)
> 
> 
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> 

I don't know what's going on with this patch. Is it something new? Can
you break it up into logical changes and submit them with proper commit
text? The signoff chain is also odd. Can you follow
Documentation/process/submitting-patches.rst?

> ---
>  drivers/clk/clk-npcm7xx.c | 99 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 75 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/clk/clk-npcm7xx.c b/drivers/clk/clk-npcm7xx.c
> index 740af90a9508..91a937720f4e 100644
> --- a/drivers/clk/clk-npcm7xx.c
> +++ b/drivers/clk/clk-npcm7xx.c
> @@ -7,17 +7,26 @@
>   * Copyright (C) 2018 Nuvoton Technologies tali.perry@nuvoton.com
>   */
>  
> -#include <linux/module.h>
> -#include <linux/clk-provider.h>
> -#include <linux/io.h>
> -#include <linux/kernel.h>
> -#include <linux/of.h>
> -#include <linux/of_address.h>
> -#include <linux/slab.h>
> -#include <linux/err.h>
> -#include <linux/bitfield.h>
> -
> -#include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
> +
> +
> +
> + #include <linux/module.h>
> + #include <linux/clk.h>
> + #include <linux/clk-provider.h>
> + #include <linux/device.h>
> + #include <linux/io.h>
> + #include <linux/kernel.h>
> + #include <linux/of.h>
> + #include <linux/of_device.h>
> + #include <linux/of_platform.h>
> + #include <linux/of_address.h>
> + #include <linux/platform_device.h>
> + #include <linux/slab.h>
> + #include <linux/err.h>
> + #include <linux/rational.h>
> + #include <linux/bitfield.h>
> + #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>

Is any of this hunk necessary or relevant to the subject of this patch?

> +
>  
>  struct npcm7xx_clk_pll {
> @@ -44,7 +56,8 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
>         u64 ret;
>  
>         if (parent_rate == 0) {
> -               pr_err("%s: parent rate is zero", __func__);
> +               pr_err("%s: parent rate is zero. reg=%x\n", __func__,
> +                       (u32)(pll->pllcon));

Maybe you should print clk name instead of register?

>                 return 0;
>         }
>  
> @@ -61,13 +74,12 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
>         return ret;
>  }
>  
> -static const struct clk_ops npcm7xx_clk_pll_ops = {
> +const struct clk_ops npcm7xx_clk_pll_ops = {

Why?

>         .recalc_rate = npcm7xx_clk_pll_recalc_rate,
>  };
>  
> -static struct clk_hw *
> -npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
> -                        const char *parent_name, unsigned long flags)
> +static struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon,
> +               const char *name, const char *parent_name, unsigned long flags)
>  {
>         struct npcm7xx_clk_pll *pll;
>         struct clk_init_data init;
> @@ -78,7 +90,8 @@ npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
>         if (!pll)
>                 return ERR_PTR(-ENOMEM);
>  
> -       pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name);
> +       pr_debug("%s reg, reg=0x%x, name=%s, p=%s\n",
> +               __func__, (unsigned int)pllcon, name, parent_name);

What's going on here? Why cast?

>  
>         init.name = name;
>         init.ops = &npcm7xx_clk_pll_ops;
> @@ -544,9 +557,11 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
>         void __iomem *clk_base;
>         struct resource res;
>         struct clk_hw *hw;
> +       struct clk *clk;
>         int ret;
>         int i;
>  
> +       clk_base = NULL;

Why?

>         ret = of_address_to_resource(clk_np, 0, &res);
>         if (ret) {
>                 pr_err("%s: failed to get resource, ret %d\n", clk_np->name,
> @@ -560,14 +575,44 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
>  
>         npcm7xx_clk_data = kzalloc(sizeof(*npcm7xx_clk_data->hws) *
>                 NPCM7XX_NUM_CLOCKS + sizeof(npcm7xx_clk_data), GFP_KERNEL);
> -       if (!npcm7xx_clk_data)
> +
> +       npcm7xx_clk_data->num = 0;

kzalloc already does that initialization to 0 for you.

> +
> +       if (!npcm7xx_clk_data->hws) {
> +               pr_err("Can't alloc npcm7xx_clk_data\n");

We don't need allocation error messages.

>                 goto npcm7xx_init_np_err;
> +       }
>  
>         npcm7xx_clk_data->num = NPCM7XX_NUM_CLOCKS;
>  
>         for (i = 0; i < NPCM7XX_NUM_CLOCKS; i++)
>                 npcm7xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
>  
> +       /* Read fixed clocks. These 3 clocks must be defined in DT */
> +       clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_REFCLK);
> +       if (IS_ERR(clk)) {
> +               pr_err("failed to find external REFCLK on device tree, err=%ld\n",
> +                       PTR_ERR(clk));
> +               clk_put(clk);
> +               goto npcm7xx_init_fail_no_clk_on_dt;
> +       }
> +
> +       clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_SYSBYPCK);
> +       if (IS_ERR(clk)) {
> +               pr_err("failed to find external SYSBYPCK on device tree, err=%ld\n",
> +                       PTR_ERR(clk));
> +               clk_put(clk);
> +               goto npcm7xx_init_fail_no_clk_on_dt;
> +       }
> +
> +       clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_MCBYPCK);
> +       if (IS_ERR(clk)) {
> +               pr_err("failed to find external MCBYPCK on device tree, err=%ld\n",
> +                       PTR_ERR(clk));
> +               clk_put(clk);
> +               goto npcm7xx_init_fail_no_clk_on_dt;
> +       }

I assume the DTS is not shipped?

> +
>         /* Register plls */
>         for (i = 0; i < ARRAY_SIZE(npcm7xx_plls); i++) {
>                 const struct npcm7xx_clk_pll_data *pll_data = &npcm7xx_plls[i];
> @@ -584,16 +629,16 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
>         }
>  
>         /* Register fixed dividers */
> -       hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,
> +       clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,

This is going backwards. Please use clk_hw APIs.

>                         NPCM7XX_CLK_S_PLL1, 0, 1, 2);
> -       if (IS_ERR(hw)) {
> +       if (IS_ERR(clk)) {
>                 pr_err("npcm7xx_clk: Can't register fixed div\n");
>                 goto npcm7xx_init_fail;
>         }
>  
> -       hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
> +       clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
>                         NPCM7XX_CLK_S_PLL2, 0, 1, 2);
> -       if (IS_ERR(hw)) {
> +       if (IS_ERR(clk)) {
>                 pr_err("npcm7xx_clk: Can't register div2\n");
>                 goto npcm7xx_init_fail;
>         }
> @@ -646,11 +691,17 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
>  
>         return;
>  
> +npcm7xx_init_fail_no_clk_on_dt:
> +       pr_err("see Documentation/devicetree/bindings/clock/");
> +       pr_err("nuvoton,npcm750-clk.txt  for details\n");
>  npcm7xx_init_fail:
> -       kfree(npcm7xx_clk_data->hws);
> +       if (npcm7xx_clk_data->num)
> +               kfree(npcm7xx_clk_data->hws);
>  npcm7xx_init_np_err:
> -       iounmap(clk_base);
> +       if (clk_base != NULL)
> +               iounmap(clk_base);
>  npcm7xx_init_error:
>         of_node_put(clk_np);
> +       pr_err("clk setup fail\n");

Is this hunk of changes related to the subject of this patch? I don't
think it is, so it just leads to more confusion.

[PATCH v1 1/1] clk: npcm7xx: get fixed clocks from DT

[Tali Perry]

Nuvoton NPCM7XX Clock Controller
fix base address and of_clk_get_by_name error handling.
    Also update error messages to be more informative.
    
    In case clk_base allocation is erronoeous the return value is null.
    Also fix handling of of_clk_get_by_name returns an error. 
    Print a better error message pointing to the dt-binding documention.

This patch is re-submitted since it was already pushed to main 
(just the diff, without the full driver which is already in 
master branch.)


Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

---
 drivers/clk/clk-npcm7xx.c | 99 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 75 insertions(+), 24 deletions(-)

diff --git a/drivers/clk/clk-npcm7xx.c b/drivers/clk/clk-npcm7xx.c
index 740af90a9508..91a937720f4e 100644
--- a/drivers/clk/clk-npcm7xx.c
+++ b/drivers/clk/clk-npcm7xx.c
@@ -7,17 +7,26 @@
  * Copyright (C) 2018 Nuvoton Technologies tali.perry@nuvoton.com
  */
 
-#include <linux/module.h>
-#include <linux/clk-provider.h>
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/slab.h>
-#include <linux/err.h>
-#include <linux/bitfield.h>
-
-#include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
+
+
+
+ #include <linux/module.h>
+ #include <linux/clk.h>
+ #include <linux/clk-provider.h>
+ #include <linux/device.h>
+ #include <linux/io.h>
+ #include <linux/kernel.h>
+ #include <linux/of.h>
+ #include <linux/of_device.h>
+ #include <linux/of_platform.h>
+ #include <linux/of_address.h>
+ #include <linux/platform_device.h>
+ #include <linux/slab.h>
+ #include <linux/err.h>
+ #include <linux/rational.h>
+ #include <linux/bitfield.h>
+ #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
+
 
 struct npcm7xx_clk_pll {
 	struct clk_hw	hw;
@@ -27,6 +36,9 @@ struct npcm7xx_clk_pll {
 
 #define to_npcm7xx_clk_pll(_hw) container_of(_hw, struct npcm7xx_clk_pll, hw)
 
+static struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon,
+	const char *name, const char *parent_name, unsigned long flags);
+
 #define PLLCON_LOKI	BIT(31)
 #define PLLCON_LOKS	BIT(30)
 #define PLLCON_FBDV	GENMASK(27, 16)
@@ -44,7 +56,8 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
 	u64 ret;
 
 	if (parent_rate == 0) {
-		pr_err("%s: parent rate is zero", __func__);
+		pr_err("%s: parent rate is zero. reg=%x\n", __func__,
+			(u32)(pll->pllcon));
 		return 0;
 	}
 
@@ -61,13 +74,12 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
 	return ret;
 }
 
-static const struct clk_ops npcm7xx_clk_pll_ops = {
+const struct clk_ops npcm7xx_clk_pll_ops = {
 	.recalc_rate = npcm7xx_clk_pll_recalc_rate,
 };
 
-static struct clk_hw *
-npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
-			 const char *parent_name, unsigned long flags)
+static struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon,
+		const char *name, const char *parent_name, unsigned long flags)
 {
 	struct npcm7xx_clk_pll *pll;
 	struct clk_init_data init;
@@ -78,7 +90,8 @@ npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
 	if (!pll)
 		return ERR_PTR(-ENOMEM);
 
-	pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name);
+	pr_debug("%s reg, reg=0x%x, name=%s, p=%s\n",
+		__func__, (unsigned int)pllcon, name, parent_name);
 
 	init.name = name;
 	init.ops = &npcm7xx_clk_pll_ops;
@@ -544,9 +557,11 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 	void __iomem *clk_base;
 	struct resource res;
 	struct clk_hw *hw;
+	struct clk *clk;
 	int ret;
 	int i;
 
+	clk_base = NULL;
 	ret = of_address_to_resource(clk_np, 0, &res);
 	if (ret) {
 		pr_err("%s: failed to get resource, ret %d\n", clk_np->name,
@@ -560,14 +575,44 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 
 	npcm7xx_clk_data = kzalloc(sizeof(*npcm7xx_clk_data->hws) *
 		NPCM7XX_NUM_CLOCKS + sizeof(npcm7xx_clk_data), GFP_KERNEL);
-	if (!npcm7xx_clk_data)
+
+	npcm7xx_clk_data->num = 0;
+
+	if (!npcm7xx_clk_data->hws) {
+		pr_err("Can't alloc npcm7xx_clk_data\n");
 		goto npcm7xx_init_np_err;
+	}
 
 	npcm7xx_clk_data->num = NPCM7XX_NUM_CLOCKS;
 
 	for (i = 0; i < NPCM7XX_NUM_CLOCKS; i++)
 		npcm7xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
 
+	/* Read fixed clocks. These 3 clocks must be defined in DT */
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_REFCLK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external REFCLK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_SYSBYPCK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external SYSBYPCK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_MCBYPCK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external MCBYPCK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
 	/* Register plls */
 	for (i = 0; i < ARRAY_SIZE(npcm7xx_plls); i++) {
 		const struct npcm7xx_clk_pll_data *pll_data = &npcm7xx_plls[i];
@@ -584,16 +629,16 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 	}
 
 	/* Register fixed dividers */
-	hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,
+	clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,
 			NPCM7XX_CLK_S_PLL1, 0, 1, 2);
-	if (IS_ERR(hw)) {
+	if (IS_ERR(clk)) {
 		pr_err("npcm7xx_clk: Can't register fixed div\n");
 		goto npcm7xx_init_fail;
 	}
 
-	hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
+	clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
 			NPCM7XX_CLK_S_PLL2, 0, 1, 2);
-	if (IS_ERR(hw)) {
+	if (IS_ERR(clk)) {
 		pr_err("npcm7xx_clk: Can't register div2\n");
 		goto npcm7xx_init_fail;
 	}
@@ -646,11 +691,17 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 
 	return;
 
+npcm7xx_init_fail_no_clk_on_dt:
+	pr_err("see Documentation/devicetree/bindings/clock/");
+	pr_err("nuvoton,npcm750-clk.txt  for details\n");
 npcm7xx_init_fail:
-	kfree(npcm7xx_clk_data->hws);
+	if (npcm7xx_clk_data->num)
+		kfree(npcm7xx_clk_data->hws);
 npcm7xx_init_np_err:
-	iounmap(clk_base);
+	if (clk_base != NULL)
+		iounmap(clk_base);
 npcm7xx_init_error:
 	of_node_put(clk_np);
+	pr_err("clk setup fail\n");
 }
 CLK_OF_DECLARE(npcm7xx_clk_init, "nuvoton,npcm750-clk", npcm7xx_clk_init);
-- 
2.14.1

[PATCH v1 1/1] clk: npcm7xx: get fixed clocks from DT

[Tali Perry]

Nuvoton NPCM7XX Clock Controller
fix base address and of_clk_get_by_name error handling.
    Also update error messages to be more informative.
    
    In case clk_base allocation is erronoeous the return value is null.
    Also fix handling of of_clk_get_by_name returns an error. 
    Print a better error message pointing to the dt-binding documention.

This patch is re-submitted since it was already pushed to main 
(just the diff, without the full driver which is already in 
master branch.)


Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

---
 drivers/clk/clk-npcm7xx.c | 101 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 77 insertions(+), 24 deletions(-)

diff --git a/drivers/clk/clk-npcm7xx.c b/drivers/clk/clk-npcm7xx.c
index 740af90a9508..a5206ee88f4c 100644
--- a/drivers/clk/clk-npcm7xx.c
+++ b/drivers/clk/clk-npcm7xx.c
@@ -7,17 +7,28 @@
  * Copyright (C) 2018 Nuvoton Technologies tali.perry@nuvoton.com
  */
 
-#include <linux/module.h>
-#include <linux/clk-provider.h>
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/slab.h>
-#include <linux/err.h>
-#include <linux/bitfield.h>
-
-#include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
+
+
+
+ #include <linux/module.h>
+ #include <linux/clk.h>
+ #include <linux/clk-provider.h>
+ #include <linux/device.h>
+ #include <linux/io.h>
+ #include <linux/kernel.h>
+ #include <linux/of.h>
+ #include <linux/of_device.h>
+ #include <linux/of_platform.h>
+ #include <linux/of_address.h>
+ #include <linux/platform_device.h>
+ #include <linux/slab.h>
+ #include <linux/err.h>
+ #include <linux/rational.h>
+ #include <linux/bitfield.h>
+ #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
+
+ #include <asm/cp15.h>
+
 
 struct npcm7xx_clk_pll {
 	struct clk_hw	hw;
@@ -27,6 +38,9 @@ struct npcm7xx_clk_pll {
 
 #define to_npcm7xx_clk_pll(_hw) container_of(_hw, struct npcm7xx_clk_pll, hw)
 
+static struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon,
+	const char *name, const char *parent_name, unsigned long flags);
+
 #define PLLCON_LOKI	BIT(31)
 #define PLLCON_LOKS	BIT(30)
 #define PLLCON_FBDV	GENMASK(27, 16)
@@ -44,7 +58,8 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
 	u64 ret;
 
 	if (parent_rate == 0) {
-		pr_err("%s: parent rate is zero", __func__);
+		pr_err("%s: parent rate is zero. reg=%x\n", __func__,
+			(u32)(pll->pllcon));
 		return 0;
 	}
 
@@ -61,13 +76,12 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
 	return ret;
 }
 
-static const struct clk_ops npcm7xx_clk_pll_ops = {
+const struct clk_ops npcm7xx_clk_pll_ops = {
 	.recalc_rate = npcm7xx_clk_pll_recalc_rate,
 };
 
-static struct clk_hw *
-npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
-			 const char *parent_name, unsigned long flags)
+static struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon,
+		const char *name, const char *parent_name, unsigned long flags)
 {
 	struct npcm7xx_clk_pll *pll;
 	struct clk_init_data init;
@@ -78,7 +92,8 @@ npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
 	if (!pll)
 		return ERR_PTR(-ENOMEM);
 
-	pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name);
+	pr_debug("%s reg, reg=0x%x, name=%s, p=%s\n",
+		__func__, (unsigned int)pllcon, name, parent_name);
 
 	init.name = name;
 	init.ops = &npcm7xx_clk_pll_ops;
@@ -544,9 +559,11 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 	void __iomem *clk_base;
 	struct resource res;
 	struct clk_hw *hw;
+	struct clk *clk;
 	int ret;
 	int i;
 
+	clk_base = NULL;
 	ret = of_address_to_resource(clk_np, 0, &res);
 	if (ret) {
 		pr_err("%s: failed to get resource, ret %d\n", clk_np->name,
@@ -560,14 +577,44 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 
 	npcm7xx_clk_data = kzalloc(sizeof(*npcm7xx_clk_data->hws) *
 		NPCM7XX_NUM_CLOCKS + sizeof(npcm7xx_clk_data), GFP_KERNEL);
-	if (!npcm7xx_clk_data)
+
+	npcm7xx_clk_data->num = 0;
+
+	if (!npcm7xx_clk_data->hws) {
+		pr_err("Can't alloc npcm7xx_clk_data\n");
 		goto npcm7xx_init_np_err;
+	}
 
 	npcm7xx_clk_data->num = NPCM7XX_NUM_CLOCKS;
 
 	for (i = 0; i < NPCM7XX_NUM_CLOCKS; i++)
 		npcm7xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
 
+	/* Read fixed clocks. These 3 clocks must be defined in DT */
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_REFCLK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external REFCLK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_SYSBYPCK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external SYSBYPCK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_MCBYPCK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external MCBYPCK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
 	/* Register plls */
 	for (i = 0; i < ARRAY_SIZE(npcm7xx_plls); i++) {
 		const struct npcm7xx_clk_pll_data *pll_data = &npcm7xx_plls[i];
@@ -584,16 +631,16 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 	}
 
 	/* Register fixed dividers */
-	hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,
+	clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,
 			NPCM7XX_CLK_S_PLL1, 0, 1, 2);
-	if (IS_ERR(hw)) {
+	if (IS_ERR(clk)) {
 		pr_err("npcm7xx_clk: Can't register fixed div\n");
 		goto npcm7xx_init_fail;
 	}
 
-	hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
+	clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
 			NPCM7XX_CLK_S_PLL2, 0, 1, 2);
-	if (IS_ERR(hw)) {
+	if (IS_ERR(clk)) {
 		pr_err("npcm7xx_clk: Can't register div2\n");
 		goto npcm7xx_init_fail;
 	}
@@ -646,11 +693,17 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 
 	return;
 
+npcm7xx_init_fail_no_clk_on_dt:
+	pr_err("see Documentation/devicetree/bindings/clock/");
+	pr_err("nuvoton,npcm750-clk.txt  for details\n");
 npcm7xx_init_fail:
-	kfree(npcm7xx_clk_data->hws);
+	if (npcm7xx_clk_data->num)
+		kfree(npcm7xx_clk_data->hws);
 npcm7xx_init_np_err:
-	iounmap(clk_base);
+	if (clk_base != NULL)
+		iounmap(clk_base);
 npcm7xx_init_error:
 	of_node_put(clk_np);
+	pr_err("clk setup fail\n");
 }
 CLK_OF_DECLARE(npcm7xx_clk_init, "nuvoton,npcm750-clk", npcm7xx_clk_init);
-- 
2.14.1

Re: [PATCH v1 1/1] clk: npcm7xx: get fixed clocks from DT

[Russell King - ARM Linux]

On Mon, Jul 16, 2018 at 01:10:07PM +0300, Tali Perry wrote:
> Nuvoton NPCM7XX Clock Controller
> fix base address and of_clk_get_by_name error handling.
>     Also update error messages to be more informative.
>     
>     In case clk_base allocation is erronoeous the return value is null.
>     Also fix handling of of_clk_get_by_name returns an error. 
>     Print a better error message pointing to the dt-binding documention.
> 
> This patch is re-submitted since it was already pushed to main 
> (just the diff, without the full driver which is already in 
> master branch.)
> 
> 
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> 
> ---
>  drivers/clk/clk-npcm7xx.c | 102 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 87 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/clk/clk-npcm7xx.c b/drivers/clk/clk-npcm7xx.c
> index 740af90a9508..6ff97f79fcd7 100644
> --- a/drivers/clk/clk-npcm7xx.c
> +++ b/drivers/clk/clk-npcm7xx.c
> @@ -8,17 +8,25 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/clk.h>
>  #include <linux/clk-provider.h>
> +#include <linux/device.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
>  #include <linux/of_address.h>
> +#include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/err.h>
> +#include <linux/rational.h>
>  #include <linux/bitfield.h>
> -
>  #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
>  
> +#include <asm/cp15.h>

Why do you need this include?

> +
> +

Why do you need all these additional blank lines in this file, it makes
this diff harder to review.  If you wish to reformat the file, please
split the reformatting out as a separate patch.

Thanks.

>  struct npcm7xx_clk_pll {
>  	struct clk_hw	hw;
>  	void __iomem	*pllcon;
> @@ -27,6 +35,9 @@ struct npcm7xx_clk_pll {
>  
>  #define to_npcm7xx_clk_pll(_hw) container_of(_hw, struct npcm7xx_clk_pll, hw)
>  
> +struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
> +	const char *parent_name, unsigned long flags);
> +
>  #define PLLCON_LOKI	BIT(31)
>  #define PLLCON_LOKS	BIT(30)
>  #define PLLCON_FBDV	GENMASK(27, 16)
> @@ -44,7 +55,8 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
>  	u64 ret;
>  
>  	if (parent_rate == 0) {
> -		pr_err("%s: parent rate is zero", __func__);
> +		pr_err("%s: parent rate is zero. reg=%x\n", __func__,
> +			(u32)(pll->pllcon));
>  		return 0;
>  	}
>  
> @@ -61,13 +73,13 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
>  	return ret;
>  }
>  
> -static const struct clk_ops npcm7xx_clk_pll_ops = {
> +const struct clk_ops npcm7xx_clk_pll_ops = {
>  	.recalc_rate = npcm7xx_clk_pll_recalc_rate,
>  };
>  
> -static struct clk_hw *
> -npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
> -			 const char *parent_name, unsigned long flags)
> +
> +struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
> +	const char *parent_name, unsigned long flags)
>  {
>  	struct npcm7xx_clk_pll *pll;
>  	struct clk_init_data init;
> @@ -78,7 +90,8 @@ npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
>  	if (!pll)
>  		return ERR_PTR(-ENOMEM);
>  
> -	pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name);
> +	pr_debug("%s reg, reg=0x%x, name=%s, p=%s\n",
> +		__func__, (unsigned int)pllcon, name, parent_name);
>  
>  	init.name = name;
>  	init.ops = &npcm7xx_clk_pll_ops;
> @@ -100,6 +113,7 @@ npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
>  	return hw;
>  }
>  
> +
>  #define NPCM7XX_CLKEN1          (0x00)
>  #define NPCM7XX_CLKEN2          (0x28)
>  #define NPCM7XX_CLKEN3          (0x30)
> @@ -129,6 +143,7 @@ npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
>  #define NPCM7XX_SECCNT          (0x68)
>  #define NPCM7XX_CNTR25M         (0x6C)
>  
> +
>  struct npcm7xx_clk_gate_data {
>  	u32 reg;
>  	u8 bit_idx;
> @@ -204,6 +219,7 @@ struct npcm7xx_clk_pll_data {
>  	int onecell_idx;
>  };
>  
> +
>  /*
>   * Single copy of strings used to refer to clocks within this driver indexed by
>   * above enum.
> @@ -255,6 +271,7 @@ struct npcm7xx_clk_pll_data {
>  #define NPCM7XX_CLK_S_USB_BRIDGE  "usb_bridge"
>  #define NPCM7XX_CLK_S_PCI         "pci"
>  
> +
>  static u32 pll_mux_table[] = {0, 1, 2, 3};
>  static const char * const pll_mux_parents[] __initconst = {
>  	NPCM7XX_CLK_S_PLL0,
> @@ -311,6 +328,7 @@ static const char * const dvcssel_mux_parents[] __initconst = {
>  	NPCM7XX_CLK_S_PLL2,
>  };
>  
> +
>  static const struct npcm7xx_clk_pll_data npcm7xx_plls[] __initconst = {
>  	{NPCM7XX_PLLCON0, NPCM7XX_CLK_S_PLL0, NPCM7XX_CLK_S_REFCLK, 0, -1},
>  
> @@ -324,6 +342,7 @@ static const struct npcm7xx_clk_pll_data npcm7xx_plls[] __initconst = {
>  	NPCM7XX_CLK_S_REFCLK, 0, -1},
>  };
>  
> +
>  static const struct npcm7xx_clk_mux_data npcm7xx_muxes[] __initconst = {
>  	{0, GENMASK(1, 0), cpuck_mux_table, NPCM7XX_CLK_S_CPU_MUX,
>  	cpuck_mux_parents, ARRAY_SIZE(cpuck_mux_parents), CLK_IS_CRITICAL,
> @@ -368,6 +387,7 @@ static const struct npcm7xx_clk_div_fixed_data npcm7xx_divs_fx[] __initconst = {
>  	{ 1, 2, NPCM7XX_CLK_S_PLL2_DIV2, NPCM7XX_CLK_S_PLL2, 0, -1},
>  };
>  
> +
>  /* configurable dividers: */
>  static const struct npcm7xx_clk_div_data npcm7xx_divs[] __initconst = {
>  	{NPCM7XX_CLKDIV1, 28, 3, NPCM7XX_CLK_S_ADC,
> @@ -435,6 +455,7 @@ static const struct npcm7xx_clk_div_data npcm7xx_divs[] __initconst = {
>  
>  };
>  
> +
>  static const struct npcm7xx_clk_gate_data npcm7xx_gates[] __initconst = {
>  	{NPCM7XX_CLKEN1, 31, "smb1-gate", NPCM7XX_CLK_S_APB2, 0},
>  	{NPCM7XX_CLKEN1, 30, "smb0-gate", NPCM7XX_CLK_S_APB2, 0},
> @@ -536,17 +557,23 @@ static const struct npcm7xx_clk_gate_data npcm7xx_gates[] __initconst = {
>  	{NPCM7XX_CLKEN3, 0, "pwmm1-gate", NPCM7XX_CLK_S_APB3, 0},
>  };
>  
> +
> +
>  static DEFINE_SPINLOCK(npcm7xx_clk_lock);
>  
> +
>  static void __init npcm7xx_clk_init(struct device_node *clk_np)
>  {
>  	struct clk_hw_onecell_data *npcm7xx_clk_data;
>  	void __iomem *clk_base;
>  	struct resource res;
>  	struct clk_hw *hw;
> +	struct clk *clk;
>  	int ret;
>  	int i;
>  
> +	clk_base = NULL;
> +
>  	ret = of_address_to_resource(clk_np, 0, &res);
>  	if (ret) {
>  		pr_err("%s: failed to get resource, ret %d\n", clk_np->name,
> @@ -554,20 +581,52 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
>  		return;
>  	}
>  
> +
>  	clk_base = ioremap(res.start, resource_size(&res));
>  	if (!clk_base)
>  		goto npcm7xx_init_error;
>  
> +
>  	npcm7xx_clk_data = kzalloc(sizeof(*npcm7xx_clk_data->hws) *
>  		NPCM7XX_NUM_CLOCKS + sizeof(npcm7xx_clk_data), GFP_KERNEL);
> -	if (!npcm7xx_clk_data)
> +
> +	npcm7xx_clk_data->num = 0;
> +
> +	if (!npcm7xx_clk_data->hws) {
> +		pr_err("Can't alloc npcm7xx_clk_data\n");
>  		goto npcm7xx_init_np_err;
> +	}
>  
>  	npcm7xx_clk_data->num = NPCM7XX_NUM_CLOCKS;
>  
>  	for (i = 0; i < NPCM7XX_NUM_CLOCKS; i++)
>  		npcm7xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
>  
> +	/* Read fixed clocks. These 3 clocks must be defined in DT */
> +	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_REFCLK);
> +	if (IS_ERR(clk)) {
> +		pr_err("failed to find external REFCLK on device tree, err=%ld\n",
> +			PTR_ERR(clk));
> +		clk_put(clk);
> +		goto npcm7xx_init_fail_no_clk_on_dt;
> +	}
> +
> +	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_SYSBYPCK);
> +	if (IS_ERR(clk)) {
> +		pr_err("failed to find external SYSBYPCK on device tree, err=%ld\n",
> +			PTR_ERR(clk));
> +		clk_put(clk);
> +		goto npcm7xx_init_fail_no_clk_on_dt;
> +	}
> +
> +	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_MCBYPCK);
> +	if (IS_ERR(clk)) {
> +		pr_err("failed to find external MCBYPCK on device tree, err=%ld\n",
> +			PTR_ERR(clk));
> +		clk_put(clk);
> +		goto npcm7xx_init_fail_no_clk_on_dt;
> +	}
> +
>  	/* Register plls */
>  	for (i = 0; i < ARRAY_SIZE(npcm7xx_plls); i++) {
>  		const struct npcm7xx_clk_pll_data *pll_data = &npcm7xx_plls[i];
> @@ -584,16 +643,18 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
>  	}
>  
>  	/* Register fixed dividers */
> -	hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,
> +	clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,
>  			NPCM7XX_CLK_S_PLL1, 0, 1, 2);
> -	if (IS_ERR(hw)) {
> +	if (IS_ERR(clk)) {
>  		pr_err("npcm7xx_clk: Can't register fixed div\n");
>  		goto npcm7xx_init_fail;
>  	}
>  
> -	hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
> +
> +	clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
>  			NPCM7XX_CLK_S_PLL2, 0, 1, 2);
> -	if (IS_ERR(hw)) {
> +
> +	if (IS_ERR(clk)) {
>  		pr_err("npcm7xx_clk: Can't register div2\n");
>  		goto npcm7xx_init_fail;
>  	}
> @@ -618,7 +679,7 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
>  			npcm7xx_clk_data->hws[mux_data->onecell_idx] = hw;
>  	}
>  
> -	/* Register clock dividers specified in npcm7xx_divs */
> +	/* Register clock dividers specified in npcm7xx_divs. */
>  	for (i = 0; i < ARRAY_SIZE(npcm7xx_divs); i++) {
>  		const struct npcm7xx_clk_div_data *div_data = &npcm7xx_divs[i];
>  
> @@ -642,15 +703,26 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
>  	if (ret)
>  		pr_err("failed to add DT provider: %d\n", ret);
>  
> +
>  	of_node_put(clk_np);
>  
>  	return;
>  
> +npcm7xx_init_fail_no_clk_on_dt:
> +	pr_err("see Documentation/devicetree/bindings/clock/");
> +	pr_err("nuvoton,npcm750-clk.txt  for details\n");
>  npcm7xx_init_fail:
> -	kfree(npcm7xx_clk_data->hws);
> +	if (npcm7xx_clk_data->num)
> +		kfree(npcm7xx_clk_data->hws);
>  npcm7xx_init_np_err:
> -	iounmap(clk_base);
> +	if (clk_base != NULL)
> +		iounmap(clk_base);
>  npcm7xx_init_error:
>  	of_node_put(clk_np);
> +	pr_err("clk setup fail\n");
>  }
> +
>  CLK_OF_DECLARE(npcm7xx_clk_init, "nuvoton,npcm750-clk", npcm7xx_clk_init);
> +
> +
> +
> -- 
> 2.14.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

[PATCH v1 1/1] clk: npcm7xx: get fixed clocks from DT

[Tali Perry]

Nuvoton NPCM7XX Clock Controller
fix base address and of_clk_get_by_name error handling.
    Also update error messages to be more informative.
    
    In case clk_base allocation is erronoeous the return value is null.
    Also fix handling of of_clk_get_by_name returns an error. 
    Print a better error message pointing to the dt-binding documention.

This patch is re-submitted since it was already pushed to main 
(just the diff, without the full driver which is already in 
master branch.)


Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

---
 drivers/clk/clk-npcm7xx.c | 102 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 87 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/clk-npcm7xx.c b/drivers/clk/clk-npcm7xx.c
index 740af90a9508..6ff97f79fcd7 100644
--- a/drivers/clk/clk-npcm7xx.c
+++ b/drivers/clk/clk-npcm7xx.c
@@ -8,17 +8,25 @@
  */
 
 #include <linux/module.h>
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
+#include <linux/device.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
 #include <linux/of_address.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/err.h>
+#include <linux/rational.h>
 #include <linux/bitfield.h>
-
 #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
 
+#include <asm/cp15.h>
+
+
 struct npcm7xx_clk_pll {
 	struct clk_hw	hw;
 	void __iomem	*pllcon;
@@ -27,6 +35,9 @@ struct npcm7xx_clk_pll {
 
 #define to_npcm7xx_clk_pll(_hw) container_of(_hw, struct npcm7xx_clk_pll, hw)
 
+struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
+	const char *parent_name, unsigned long flags);
+
 #define PLLCON_LOKI	BIT(31)
 #define PLLCON_LOKS	BIT(30)
 #define PLLCON_FBDV	GENMASK(27, 16)
@@ -44,7 +55,8 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
 	u64 ret;
 
 	if (parent_rate == 0) {
-		pr_err("%s: parent rate is zero", __func__);
+		pr_err("%s: parent rate is zero. reg=%x\n", __func__,
+			(u32)(pll->pllcon));
 		return 0;
 	}
 
@@ -61,13 +73,13 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,
 	return ret;
 }
 
-static const struct clk_ops npcm7xx_clk_pll_ops = {
+const struct clk_ops npcm7xx_clk_pll_ops = {
 	.recalc_rate = npcm7xx_clk_pll_recalc_rate,
 };
 
-static struct clk_hw *
-npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
-			 const char *parent_name, unsigned long flags)
+
+struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
+	const char *parent_name, unsigned long flags)
 {
 	struct npcm7xx_clk_pll *pll;
 	struct clk_init_data init;
@@ -78,7 +90,8 @@ npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
 	if (!pll)
 		return ERR_PTR(-ENOMEM);
 
-	pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name);
+	pr_debug("%s reg, reg=0x%x, name=%s, p=%s\n",
+		__func__, (unsigned int)pllcon, name, parent_name);
 
 	init.name = name;
 	init.ops = &npcm7xx_clk_pll_ops;
@@ -100,6 +113,7 @@ npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
 	return hw;
 }
 
+
 #define NPCM7XX_CLKEN1          (0x00)
 #define NPCM7XX_CLKEN2          (0x28)
 #define NPCM7XX_CLKEN3          (0x30)
@@ -129,6 +143,7 @@ npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,
 #define NPCM7XX_SECCNT          (0x68)
 #define NPCM7XX_CNTR25M         (0x6C)
 
+
 struct npcm7xx_clk_gate_data {
 	u32 reg;
 	u8 bit_idx;
@@ -204,6 +219,7 @@ struct npcm7xx_clk_pll_data {
 	int onecell_idx;
 };
 
+
 /*
  * Single copy of strings used to refer to clocks within this driver indexed by
  * above enum.
@@ -255,6 +271,7 @@ struct npcm7xx_clk_pll_data {
 #define NPCM7XX_CLK_S_USB_BRIDGE  "usb_bridge"
 #define NPCM7XX_CLK_S_PCI         "pci"
 
+
 static u32 pll_mux_table[] = {0, 1, 2, 3};
 static const char * const pll_mux_parents[] __initconst = {
 	NPCM7XX_CLK_S_PLL0,
@@ -311,6 +328,7 @@ static const char * const dvcssel_mux_parents[] __initconst = {
 	NPCM7XX_CLK_S_PLL2,
 };
 
+
 static const struct npcm7xx_clk_pll_data npcm7xx_plls[] __initconst = {
 	{NPCM7XX_PLLCON0, NPCM7XX_CLK_S_PLL0, NPCM7XX_CLK_S_REFCLK, 0, -1},
 
@@ -324,6 +342,7 @@ static const struct npcm7xx_clk_pll_data npcm7xx_plls[] __initconst = {
 	NPCM7XX_CLK_S_REFCLK, 0, -1},
 };
 
+
 static const struct npcm7xx_clk_mux_data npcm7xx_muxes[] __initconst = {
 	{0, GENMASK(1, 0), cpuck_mux_table, NPCM7XX_CLK_S_CPU_MUX,
 	cpuck_mux_parents, ARRAY_SIZE(cpuck_mux_parents), CLK_IS_CRITICAL,
@@ -368,6 +387,7 @@ static const struct npcm7xx_clk_div_fixed_data npcm7xx_divs_fx[] __initconst = {
 	{ 1, 2, NPCM7XX_CLK_S_PLL2_DIV2, NPCM7XX_CLK_S_PLL2, 0, -1},
 };
 
+
 /* configurable dividers: */
 static const struct npcm7xx_clk_div_data npcm7xx_divs[] __initconst = {
 	{NPCM7XX_CLKDIV1, 28, 3, NPCM7XX_CLK_S_ADC,
@@ -435,6 +455,7 @@ static const struct npcm7xx_clk_div_data npcm7xx_divs[] __initconst = {
 
 };
 
+
 static const struct npcm7xx_clk_gate_data npcm7xx_gates[] __initconst = {
 	{NPCM7XX_CLKEN1, 31, "smb1-gate", NPCM7XX_CLK_S_APB2, 0},
 	{NPCM7XX_CLKEN1, 30, "smb0-gate", NPCM7XX_CLK_S_APB2, 0},
@@ -536,17 +557,23 @@ static const struct npcm7xx_clk_gate_data npcm7xx_gates[] __initconst = {
 	{NPCM7XX_CLKEN3, 0, "pwmm1-gate", NPCM7XX_CLK_S_APB3, 0},
 };
 
+
+
 static DEFINE_SPINLOCK(npcm7xx_clk_lock);
 
+
 static void __init npcm7xx_clk_init(struct device_node *clk_np)
 {
 	struct clk_hw_onecell_data *npcm7xx_clk_data;
 	void __iomem *clk_base;
 	struct resource res;
 	struct clk_hw *hw;
+	struct clk *clk;
 	int ret;
 	int i;
 
+	clk_base = NULL;
+
 	ret = of_address_to_resource(clk_np, 0, &res);
 	if (ret) {
 		pr_err("%s: failed to get resource, ret %d\n", clk_np->name,
@@ -554,20 +581,52 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 		return;
 	}
 
+
 	clk_base = ioremap(res.start, resource_size(&res));
 	if (!clk_base)
 		goto npcm7xx_init_error;
 
+
 	npcm7xx_clk_data = kzalloc(sizeof(*npcm7xx_clk_data->hws) *
 		NPCM7XX_NUM_CLOCKS + sizeof(npcm7xx_clk_data), GFP_KERNEL);
-	if (!npcm7xx_clk_data)
+
+	npcm7xx_clk_data->num = 0;
+
+	if (!npcm7xx_clk_data->hws) {
+		pr_err("Can't alloc npcm7xx_clk_data\n");
 		goto npcm7xx_init_np_err;
+	}
 
 	npcm7xx_clk_data->num = NPCM7XX_NUM_CLOCKS;
 
 	for (i = 0; i < NPCM7XX_NUM_CLOCKS; i++)
 		npcm7xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
 
+	/* Read fixed clocks. These 3 clocks must be defined in DT */
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_REFCLK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external REFCLK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_SYSBYPCK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external SYSBYPCK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
+	clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_MCBYPCK);
+	if (IS_ERR(clk)) {
+		pr_err("failed to find external MCBYPCK on device tree, err=%ld\n",
+			PTR_ERR(clk));
+		clk_put(clk);
+		goto npcm7xx_init_fail_no_clk_on_dt;
+	}
+
 	/* Register plls */
 	for (i = 0; i < ARRAY_SIZE(npcm7xx_plls); i++) {
 		const struct npcm7xx_clk_pll_data *pll_data = &npcm7xx_plls[i];
@@ -584,16 +643,18 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 	}
 
 	/* Register fixed dividers */
-	hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,
+	clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,
 			NPCM7XX_CLK_S_PLL1, 0, 1, 2);
-	if (IS_ERR(hw)) {
+	if (IS_ERR(clk)) {
 		pr_err("npcm7xx_clk: Can't register fixed div\n");
 		goto npcm7xx_init_fail;
 	}
 
-	hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
+
+	clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,
 			NPCM7XX_CLK_S_PLL2, 0, 1, 2);
-	if (IS_ERR(hw)) {
+
+	if (IS_ERR(clk)) {
 		pr_err("npcm7xx_clk: Can't register div2\n");
 		goto npcm7xx_init_fail;
 	}
@@ -618,7 +679,7 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 			npcm7xx_clk_data->hws[mux_data->onecell_idx] = hw;
 	}
 
-	/* Register clock dividers specified in npcm7xx_divs */
+	/* Register clock dividers specified in npcm7xx_divs. */
 	for (i = 0; i < ARRAY_SIZE(npcm7xx_divs); i++) {
 		const struct npcm7xx_clk_div_data *div_data = &npcm7xx_divs[i];
 
@@ -642,15 +703,26 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)
 	if (ret)
 		pr_err("failed to add DT provider: %d\n", ret);
 
+
 	of_node_put(clk_np);
 
 	return;
 
+npcm7xx_init_fail_no_clk_on_dt:
+	pr_err("see Documentation/devicetree/bindings/clock/");
+	pr_err("nuvoton,npcm750-clk.txt  for details\n");
 npcm7xx_init_fail:
-	kfree(npcm7xx_clk_data->hws);
+	if (npcm7xx_clk_data->num)
+		kfree(npcm7xx_clk_data->hws);
 npcm7xx_init_np_err:
-	iounmap(clk_base);
+	if (clk_base != NULL)
+		iounmap(clk_base);
 npcm7xx_init_error:
 	of_node_put(clk_np);
+	pr_err("clk setup fail\n");
 }
+
 CLK_OF_DECLARE(npcm7xx_clk_init, "nuvoton,npcm750-clk", npcm7xx_clk_init);
+
+
+
-- 
2.14.1