diff mbox

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

Message ID 1471933609-8456-3-git-send-email-mw@semihalf.com (mailing list archive)
State Changes Requested, archived
Delegated to: Stephen Boyd
Headers show

Commit Message

Marcin Wojtas Aug. 23, 2016, 6:26 a.m. UTC
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(-)

Comments

Stephen Boyd Aug. 25, 2016, 12:16 a.m. UTC | #1
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
>
Thomas Petazzoni Aug. 30, 2016, 2:15 p.m. UTC | #2
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
Marcin Wojtas Aug. 30, 2016, 3:31 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd Aug. 30, 2016, 6:43 p.m. UTC | #4
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.
diff mbox

Patch

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