Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1468318 > unrolled thread

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

Started byMarcin Wojtas <mw@semihalf.com>
First post2016-08-23 08:30 +0200
Last post2016-08-25 07:10 +0200
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] Armada 7k/8k CP110 system controller fixes Marcin Wojtas <mw@semihalf.com> - 2016-08-23 08:30 +0200
    [PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada CP110 system controller Marcin Wojtas <mw@semihalf.com> - 2016-08-23 08:30 +0200
      Re: [PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada  CP110 system controller Stephen Boyd <sboyd@codeaurora.org> - 2016-08-25 07:10 +0200
    [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock Marcin Wojtas <mw@semihalf.com> - 2016-08-23 08:30 +0200
      Re: [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock Andrew Lunn <andrew@lunn.ch> - 2016-08-23 16:20 +0200
        Re: [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock Marcin Wojtas <mw@semihalf.com> - 2016-08-24 11:00 +0200
      Re: [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock Stephen Boyd <sboyd@codeaurora.org> - 2016-08-25 07:10 +0200

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

FromMarcin Wojtas <mw@semihalf.com>
Date2016-08-23 08:30 +0200
Subject[PATCH 0/2] Armada 7k/8k CP110 system controller fixes
Message-ID<s9dnI-eE-9@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

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

FromMarcin Wojtas <mw@semihalf.com>
Date2016-08-23 08:30 +0200
Subject[PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada CP110 system controller
Message-ID<s9dnI-eE-15@gated-at.bofh.it>
In reply to#1468318
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

[toc] | [prev] | [next] | [standalone]

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

FromStephen Boyd <sboyd@codeaurora.org>
Date2016-08-25 07:10 +0200
SubjectRe: [PATCH 2/2] clk: mvebu: dynamically allocate resources in Armada CP110 system controller
Message-ID<s9V5o-50Y-25@gated-at.bofh.it>
In reply to#1468319
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

[toc] | [prev] | [next] | [standalone]

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

FromMarcin Wojtas <mw@semihalf.com>
Date2016-08-23 08:30 +0200
Subject[PATCH 1/2] clk: mvebu: set flags in CP110 gate clock
Message-ID<s9dnI-eE-11@gated-at.bofh.it>
In reply to#1468318
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

[toc] | [prev] | [next] | [standalone]

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

FromAndrew Lunn <andrew@lunn.ch>
Date2016-08-23 16:20 +0200
SubjectRe: [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock
Message-ID<s9kIy-563-27@gated-at.bofh.it>
In reply to#1468321
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

[toc] | [prev] | [next] | [standalone]

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

FromMarcin Wojtas <mw@semihalf.com>
Date2016-08-24 11:00 +0200
SubjectRe: [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock
Message-ID<s9Ccp-8hd-19@gated-at.bofh.it>
In reply to#1468584
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

[toc] | [prev] | [next] | [standalone]

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

FromStephen Boyd <sboyd@codeaurora.org>
Date2016-08-25 07:10 +0200
SubjectRe: [PATCH 1/2] clk: mvebu: set flags in CP110 gate clock
Message-ID<s9V5o-50Y-3@gated-at.bofh.it>
In reply to#1468321
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web