[PATCH 0/2] Armada 7k/8k CP110 system controller fixes

[PATCH 0/2] Armada 7k/8k CP110 system controller fixes

[Marcin Wojtas]

Hi,

Newly added clock driver for Marvell Armada 7k/8k CP110 HW block occurred
not to be working properly, especially when using two instances of CP110
in Armada 8k. Below tiny patchset comprise fixes for that (prevent from
using uninitialized 'flag' field and static global resources).

Any feedback would be very welcome.

Best regards,
Marcin

Marcin Wojtas (2):
  clk: mvebu: set flags in CP110 gate clock
  clk: mvebu: dynamically allocate resources in Armada CP110 system
    controller

 drivers/clk/mvebu/cp110-system-controller.c | 30 ++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] clk: mvebu: set flags in CP110 gate clock

[Marcin Wojtas]

Armada CP110 system controller comprise its own routine responsble
for registering gate clocks. Among others 'flags' field in
struct clk_init_data was not set, using a random values, which
may cause an unpredicted behavior.

This patch fixes the problem by setting CLK_IS_BASIC flag for
all gated clocks of Armada 7k/8k SoCs family.

Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...")

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 drivers/clk/mvebu/cp110-system-controller.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
index 7fa42d6..0835e1d 100644
--- a/drivers/clk/mvebu/cp110-system-controller.c
+++ b/drivers/clk/mvebu/cp110-system-controller.c
@@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name,
 
 	init.name = name;
 	init.ops = &cp110_gate_ops;
+	init.flags = CLK_IS_BASIC;
 	init.parent_names = &parent_name;
 	init.num_parents = 1;
 
-- 
1.8.3.1

[PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada CP110 system controller

[Marcin Wojtas]

Original commit, which added support for Armada CP110 system controller
used global variables for storing all clock information. It worked
fine for Armada 7k SoC, with single CP110 block. After dual-CP110 Armada 8k
was introduced, the data got overwritten and corrupted.

This patch fixes the issue by allocating resources dynamically in the
driver probe and storing it as platform drvdata.

Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...")

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 drivers/clk/mvebu/cp110-system-controller.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
index 0835e1d..2bd87d2 100644
--- a/drivers/clk/mvebu/cp110-system-controller.c
+++ b/drivers/clk/mvebu/cp110-system-controller.c
@@ -81,13 +81,6 @@ enum {
 #define CP110_GATE_EIP150		25
 #define CP110_GATE_EIP197		26
 
-static struct clk *cp110_clks[CP110_CLK_NUM];
-
-static struct clk_onecell_data cp110_clk_data = {
-	.clks = cp110_clks,
-	.clk_num = CP110_CLK_NUM,
-};
-
 struct cp110_gate_clk {
 	struct clk_hw hw;
 	struct regmap *regmap;
@@ -195,7 +188,8 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	struct device_node *np = pdev->dev.of_node;
 	const char *ppv2_name, *apll_name, *core_name, *eip_name, *nand_name;
-	struct clk *clk;
+	struct clk_onecell_data *cp110_clk_data;
+	struct clk *clk, **cp110_clks;
 	u32 nand_clk_ctrl;
 	int i, ret;
 
@@ -208,6 +202,20 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	cp110_clks = devm_kcalloc(&pdev->dev, sizeof(struct clk *),
+				  CP110_CLK_NUM, GFP_KERNEL);
+	if (IS_ERR(cp110_clks))
+		return PTR_ERR(cp110_clks);
+
+	cp110_clk_data = devm_kzalloc(&pdev->dev,
+				      sizeof(struct clk_onecell_data),
+				      GFP_KERNEL);
+	if (IS_ERR(cp110_clk_data))
+		return PTR_ERR(cp110_clk_data);
+
+	cp110_clk_data->clks = cp110_clks;
+	cp110_clk_data->clk_num = CP110_CLK_NUM;
+
 	/* Register the APLL which is the root of the clk tree */
 	of_property_read_string_index(np, "core-clock-output-names",
 				      CP110_CORE_APLL, &apll_name);
@@ -335,10 +343,12 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
 		cp110_clks[CP110_MAX_CORE_CLOCKS + i] = clk;
 	}
 
-	ret = of_clk_add_provider(np, cp110_of_clk_get, &cp110_clk_data);
+	ret = of_clk_add_provider(np, cp110_of_clk_get, cp110_clk_data);
 	if (ret)
 		goto fail_clk_add;
 
+	platform_set_drvdata(pdev, cp110_clks);
+
 	return 0;
 
 fail_clk_add:
@@ -365,6 +375,7 @@ fail0:
 
 static int cp110_syscon_clk_remove(struct platform_device *pdev)
 {
+	struct clk **cp110_clks = platform_get_drvdata(pdev);
 	int i;
 
 	of_clk_del_provider(pdev->dev.of_node);
-- 
1.8.3.1

Re: [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock

[Andrew Lunn]

On Tue, Aug 23, 2016 at 08:26:48AM +0200, Marcin Wojtas wrote:
> Armada CP110 system controller comprise its own routine responsble
> for registering gate clocks. Among others 'flags' field in
> struct clk_init_data was not set, using a random values, which
> may cause an unpredicted behavior.
> 
> This patch fixes the problem by setting CLK_IS_BASIC flag for
> all gated clocks of Armada 7k/8k SoCs family.
> 
> Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...")
> 
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  drivers/clk/mvebu/cp110-system-controller.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
> index 7fa42d6..0835e1d 100644
> --- a/drivers/clk/mvebu/cp110-system-controller.c
> +++ b/drivers/clk/mvebu/cp110-system-controller.c
> @@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name,
>  
>  	init.name = name;
>  	init.ops = &cp110_gate_ops;
> +	init.flags = CLK_IS_BASIC;
>  	init.parent_names = &parent_name;
>  	init.num_parents = 1;

Hi Marcin

How about adding a memset for init? That would also help if new fields
every get added to clk_init_data.

      Andrew

Re: [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock

[Marcin Wojtas]

HI Andrew,

2016-08-23 16:16 GMT+02:00 Andrew Lunn <andrew@lunn.ch>:
> On Tue, Aug 23, 2016 at 08:26:48AM +0200, Marcin Wojtas wrote:
>> Armada CP110 system controller comprise its own routine responsble
>> for registering gate clocks. Among others 'flags' field in
>> struct clk_init_data was not set, using a random values, which
>> may cause an unpredicted behavior.
>>
>> This patch fixes the problem by setting CLK_IS_BASIC flag for
>> all gated clocks of Armada 7k/8k SoCs family.
>>
>> Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...")
>>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  drivers/clk/mvebu/cp110-system-controller.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
>> index 7fa42d6..0835e1d 100644
>> --- a/drivers/clk/mvebu/cp110-system-controller.c
>> +++ b/drivers/clk/mvebu/cp110-system-controller.c
>> @@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name,
>>
>>       init.name = name;
>>       init.ops = &cp110_gate_ops;
>> +     init.flags = CLK_IS_BASIC;
>>       init.parent_names = &parent_name;
>>       init.num_parents = 1;
>
> Hi Marcin
>
> How about adding a memset for init? That would also help if new fields
> every get added to clk_init_data.
>

Sure, it can be added.

Best regards,
Marcin

Re: [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock

[Stephen Boyd]

On 08/23, Marcin Wojtas wrote:
> Armada CP110 system controller comprise its own routine responsble
> for registering gate clocks. Among others 'flags' field in
> struct clk_init_data was not set, using a random values, which
> may cause an unpredicted behavior.
> 
> This patch fixes the problem by setting CLK_IS_BASIC flag for
> all gated clocks of Armada 7k/8k SoCs family.
> 
> Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...")
> 
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  drivers/clk/mvebu/cp110-system-controller.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
> index 7fa42d6..0835e1d 100644
> --- a/drivers/clk/mvebu/cp110-system-controller.c
> +++ b/drivers/clk/mvebu/cp110-system-controller.c
> @@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name,
>  
>  	init.name = name;
>  	init.ops = &cp110_gate_ops;
> +	init.flags = CLK_IS_BASIC;

Please don't use CLK_IS_BASIC unless you need it (so far only TI
clks seem to want it?). Just set it to 0 if possible.

>  	init.parent_names = &parent_name;
>  	init.num_parents = 1;
>  

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada CP110 system controller

[Stephen Boyd]

On 08/23, Marcin Wojtas wrote:
> Original commit, which added support for Armada CP110 system controller
> used global variables for storing all clock information. It worked
> fine for Armada 7k SoC, with single CP110 block. After dual-CP110 Armada 8k
> was introduced, the data got overwritten and corrupted.
> 
> This patch fixes the issue by allocating resources dynamically in the
> driver probe and storing it as platform drvdata.
> 
> Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...")
> 

Please drop the space between fixes tag and the signoff.

> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  drivers/clk/mvebu/cp110-system-controller.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
> index 0835e1d..2bd87d2 100644
> --- a/drivers/clk/mvebu/cp110-system-controller.c
> +++ b/drivers/clk/mvebu/cp110-system-controller.c
> @@ -81,13 +81,6 @@ enum {
>  #define CP110_GATE_EIP150		25
>  #define CP110_GATE_EIP197		26
>  
> -static struct clk *cp110_clks[CP110_CLK_NUM];
> -
> -static struct clk_onecell_data cp110_clk_data = {
> -	.clks = cp110_clks,
> -	.clk_num = CP110_CLK_NUM,
> -};
> -
>  struct cp110_gate_clk {
>  	struct clk_hw hw;
>  	struct regmap *regmap;
> @@ -195,7 +188,8 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>  	struct regmap *regmap;
>  	struct device_node *np = pdev->dev.of_node;
>  	const char *ppv2_name, *apll_name, *core_name, *eip_name, *nand_name;
> -	struct clk *clk;
> +	struct clk_onecell_data *cp110_clk_data;
> +	struct clk *clk, **cp110_clks;
>  	u32 nand_clk_ctrl;
>  	int i, ret;
>  
> @@ -208,6 +202,20 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	cp110_clks = devm_kcalloc(&pdev->dev, sizeof(struct clk *),
> +				  CP110_CLK_NUM, GFP_KERNEL);
> +	if (IS_ERR(cp110_clks))

Doesn't that return NULL on error?

> +		return PTR_ERR(cp110_clks);
> +
> +	cp110_clk_data = devm_kzalloc(&pdev->dev,
> +				      sizeof(struct clk_onecell_data),

sizeof(*cp110_clk_data) please

> +				      GFP_KERNEL);
> +	if (IS_ERR(cp110_clk_data))

Doesn't that return NULL on error?

> +		return PTR_ERR(cp110_clk_data);
> +
> +	cp110_clk_data->clks = cp110_clks;
> +	cp110_clk_data->clk_num = CP110_CLK_NUM;
> +
>  	/* Register the APLL which is the root of the clk tree */
>  	of_property_read_string_index(np, "core-clock-output-names",
>  				      CP110_CORE_APLL, &apll_name);
> @@ -335,10 +343,12 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>  		cp110_clks[CP110_MAX_CORE_CLOCKS + i] = clk;
>  	}
>  
> -	ret = of_clk_add_provider(np, cp110_of_clk_get, &cp110_clk_data);
> +	ret = of_clk_add_provider(np, cp110_of_clk_get, cp110_clk_data);

It would be nice if this could be converted to
of_clk_add_hw_provider().

>  	if (ret)
>  		goto fail_clk_add;
>  
> +	platform_set_drvdata(pdev, cp110_clks);
> +
>  	return 0;
>  
>  fail_clk_add:
> @@ -365,6 +375,7 @@ fail0:
>  
>  static int cp110_syscon_clk_remove(struct platform_device *pdev)
>  {
> +	struct clk **cp110_clks = platform_get_drvdata(pdev);

Is this variable unused now?

>  	int i;
>  
>  	of_clk_del_provider(pdev->dev.of_node);
> -- 
> 1.8.3.1
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock

[Thomas Petazzoni]

Hello,

On Tue, 23 Aug 2016 08:26:48 +0200, Marcin Wojtas wrote:

> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
> index 7fa42d6..0835e1d 100644
> --- a/drivers/clk/mvebu/cp110-system-controller.c
> +++ b/drivers/clk/mvebu/cp110-system-controller.c
> @@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name,
>  
>  	init.name = name;
>  	init.ops = &cp110_gate_ops;
> +	init.flags = CLK_IS_BASIC;

Is this really correct?

The documentation for CLK_IS_BASIC is pretty slim, but it says:

   #define CLK_IS_BASIC            BIT(5) /* Basic clk, can't do a to_clk_foo() */

However, we *do* have a to_clk_*() macro in this driver:

struct cp110_gate_clk {
        struct clk_hw hw;
        struct regmap *regmap;
        u8 bit_idx;
};

#define to_cp110_gate_clk(clk) container_of(clk, struct cp110_gate_clk, hw)

If you read the commit log of commit
f7d8caadfd2813cbada82ce9041b13c38e8e5282, which introduced the flag, it
says:

    clk: Add CLK_IS_BASIC flag to identify basic clocks
    
    Most platforms end up using a mix of basic clock types and
    some which use clk_hw_foo struct for filling in custom platform
    information when the clocks don't fit into basic types supported.
    
    In platform code, its useful to know if a clock is using a basic
    type or clk_hw_foo, which helps platforms know if they can
    safely use to_clk_hw_foo to derive the clk_hw_foo pointer from
    clk_hw.
    
    Mark all basic clocks with a CLK_IS_BASIC flag.
    
    Signed-off-by: Rajendra Nayak <rnayak@ti.com>
    Signed-off-by: Mike Turquette <mturquette@linaro.org>

We are in the case where we have our own clk_hw_foo structure, and a
to_clk_hw_foo macro to derive the clk_hw_foo from clk_hw.

According to this, the CP110 clocks are *not* basic clocks, and
therefore we shouldn't have this flag. Perhaps just the memset() is
missing.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock

[Marcin Wojtas]

Hi Thomas,

2016-08-30 15:10 GMT+02:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Hello,
>
> On Tue, 23 Aug 2016 08:26:48 +0200, Marcin Wojtas wrote:
>
>> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
>> index 7fa42d6..0835e1d 100644
>> --- a/drivers/clk/mvebu/cp110-system-controller.c
>> +++ b/drivers/clk/mvebu/cp110-system-controller.c
>> @@ -144,6 +144,7 @@ static struct clk *cp110_register_gate(const char *name,
>>
>>       init.name = name;
>>       init.ops = &cp110_gate_ops;
>> +     init.flags = CLK_IS_BASIC;
>
> Is this really correct?
>
> The documentation for CLK_IS_BASIC is pretty slim, but it says:
>
>    #define CLK_IS_BASIC            BIT(5) /* Basic clk, can't do a to_clk_foo() */
>
> However, we *do* have a to_clk_*() macro in this driver:
>
> struct cp110_gate_clk {
>         struct clk_hw hw;
>         struct regmap *regmap;
>         u8 bit_idx;
> };
>
> #define to_cp110_gate_clk(clk) container_of(clk, struct cp110_gate_clk, hw)
>
> If you read the commit log of commit
> f7d8caadfd2813cbada82ce9041b13c38e8e5282, which introduced the flag, it
> says:
>
>     clk: Add CLK_IS_BASIC flag to identify basic clocks
>
>     Most platforms end up using a mix of basic clock types and
>     some which use clk_hw_foo struct for filling in custom platform
>     information when the clocks don't fit into basic types supported.
>
>     In platform code, its useful to know if a clock is using a basic
>     type or clk_hw_foo, which helps platforms know if they can
>     safely use to_clk_hw_foo to derive the clk_hw_foo pointer from
>     clk_hw.
>
>     Mark all basic clocks with a CLK_IS_BASIC flag.
>
>     Signed-off-by: Rajendra Nayak <rnayak@ti.com>
>     Signed-off-by: Mike Turquette <mturquette@linaro.org>
>
> We are in the case where we have our own clk_hw_foo structure, and a
> to_clk_hw_foo macro to derive the clk_hw_foo from clk_hw.
>
> According to this, the CP110 clocks are *not* basic clocks, and
> therefore we shouldn't have this flag. Perhaps just the memset() is
> missing.
>

I agree, from functional point of view and considering also not exact
fit to CLK_IS_BASIC definition, memset should be sufficient.

Best regards,
Marcin

Re: [PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada CP110 system controller

[Thomas Petazzoni]

Hello,

On Tue, 23 Aug 2016 08:26:49 +0200, Marcin Wojtas wrote:
> Original commit, which added support for Armada CP110 system controller
> used global variables for storing all clock information. It worked
> fine for Armada 7k SoC, with single CP110 block. After dual-CP110 Armada 8k
> was introduced, the data got overwritten and corrupted.
> 
> This patch fixes the issue by allocating resources dynamically in the
> driver probe and storing it as platform drvdata.
> 
> Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...")

Adding:

CC: <stable@vger.kernel.org>

here would be useful.

> 
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Other than that:

Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
 (on Armada 8K hardware)
Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Thanks a lot for fixing the crap that I initially wrote :-/

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada CP110 system controller

[Marcin Wojtas]

Hi Stephen,

2016-08-25 2:16 GMT+02:00 Stephen Boyd <sboyd@codeaurora.org>:
> On 08/23, Marcin Wojtas wrote:
>> Original commit, which added support for Armada CP110 system controller
>> used global variables for storing all clock information. It worked
>> fine for Armada 7k SoC, with single CP110 block. After dual-CP110 Armada 8k
>> was introduced, the data got overwritten and corrupted.
>>
>> This patch fixes the issue by allocating resources dynamically in the
>> driver probe and storing it as platform drvdata.
>>
>> Fixes: d3da3eaef7f4 ("clk: mvebu: new driver for Armada CP110 system ...")
>>
>
> Please drop the space between fixes tag and the signoff.

Ok.

>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  drivers/clk/mvebu/cp110-system-controller.c | 29 ++++++++++++++++++++---------
>>  1 file changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
>> index 0835e1d..2bd87d2 100644
>> --- a/drivers/clk/mvebu/cp110-system-controller.c
>> +++ b/drivers/clk/mvebu/cp110-system-controller.c
>> @@ -81,13 +81,6 @@ enum {
>>  #define CP110_GATE_EIP150            25
>>  #define CP110_GATE_EIP197            26
>>
>> -static struct clk *cp110_clks[CP110_CLK_NUM];
>> -
>> -static struct clk_onecell_data cp110_clk_data = {
>> -     .clks = cp110_clks,
>> -     .clk_num = CP110_CLK_NUM,
>> -};
>> -
>>  struct cp110_gate_clk {
>>       struct clk_hw hw;
>>       struct regmap *regmap;
>> @@ -195,7 +188,8 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>>       struct regmap *regmap;
>>       struct device_node *np = pdev->dev.of_node;
>>       const char *ppv2_name, *apll_name, *core_name, *eip_name, *nand_name;
>> -     struct clk *clk;
>> +     struct clk_onecell_data *cp110_clk_data;
>> +     struct clk *clk, **cp110_clks;
>>       u32 nand_clk_ctrl;
>>       int i, ret;
>>
>> @@ -208,6 +202,20 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>>       if (ret)
>>               return ret;
>>
>> +     cp110_clks = devm_kcalloc(&pdev->dev, sizeof(struct clk *),
>> +                               CP110_CLK_NUM, GFP_KERNEL);
>> +     if (IS_ERR(cp110_clks))
>
> Doesn't that return NULL on error?

Will change to 'if (!cp110clks)'

>
>> +             return PTR_ERR(cp110_clks);
>> +
>> +     cp110_clk_data = devm_kzalloc(&pdev->dev,
>> +                                   sizeof(struct clk_onecell_data),
>
> sizeof(*cp110_clk_data) please

Ok.

>
>> +                                   GFP_KERNEL);
>> +     if (IS_ERR(cp110_clk_data))
>
> Doesn't that return NULL on error?

Same as above, will change.

>
>> +             return PTR_ERR(cp110_clk_data);
>> +
>> +     cp110_clk_data->clks = cp110_clks;
>> +     cp110_clk_data->clk_num = CP110_CLK_NUM;
>> +
>>       /* Register the APLL which is the root of the clk tree */
>>       of_property_read_string_index(np, "core-clock-output-names",
>>                                     CP110_CORE_APLL, &apll_name);
>> @@ -335,10 +343,12 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
>>               cp110_clks[CP110_MAX_CORE_CLOCKS + i] = clk;
>>       }
>>
>> -     ret = of_clk_add_provider(np, cp110_of_clk_get, &cp110_clk_data);
>> +     ret = of_clk_add_provider(np, cp110_of_clk_get, cp110_clk_data);
>
> It would be nice if this could be converted to
> of_clk_add_hw_provider().

Will try it. Shouldn't such change be placed in separate commit?

>
>>       if (ret)
>>               goto fail_clk_add;
>>
>> +     platform_set_drvdata(pdev, cp110_clks);
>> +
>>       return 0;
>>
>>  fail_clk_add:
>> @@ -365,6 +375,7 @@ fail0:
>>
>>  static int cp110_syscon_clk_remove(struct platform_device *pdev)
>>  {
>> +     struct clk **cp110_clks = platform_get_drvdata(pdev);
>
> Is this variable unused now?

No, why? Just below there is a loop using it. Before it was taken from
global variable, which I got rid of.

Best regards,
Marcin

Re: [PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada CP110 system controller

[Stephen Boyd]

On 08/30, Marcin Wojtas wrote:
> 2016-08-25 2:16 GMT+02:00 Stephen Boyd <sboyd@codeaurora.org>:
> > On 08/23, Marcin Wojtas wrote:
> >> @@ -335,10 +343,12 @@ static int cp110_syscon_clk_probe(struct platform_device *pdev)
> >>               cp110_clks[CP110_MAX_CORE_CLOCKS + i] = clk;
> >>       }
> >>
> >> -     ret = of_clk_add_provider(np, cp110_of_clk_get, &cp110_clk_data);
> >> +     ret = of_clk_add_provider(np, cp110_of_clk_get, cp110_clk_data);
> >
> > It would be nice if this could be converted to
> > of_clk_add_hw_provider().
> 
> Will try it. Shouldn't such change be placed in separate commit?

Yes, of course.

> 
> >
> >>       if (ret)
> >>               goto fail_clk_add;
> >>
> >> +     platform_set_drvdata(pdev, cp110_clks);
> >> +
> >>       return 0;
> >>
> >>  fail_clk_add:
> >> @@ -365,6 +375,7 @@ fail0:
> >>
> >>  static int cp110_syscon_clk_remove(struct platform_device *pdev)
> >>  {
> >> +     struct clk **cp110_clks = platform_get_drvdata(pdev);
> >
> > Is this variable unused now?
> 
> No, why? Just below there is a loop using it. Before it was taken from
> global variable, which I got rid of.
> 

Ok. I was just looking at the patch context.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project