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

STALE3661d

Revision v1 of 4 in this series.

12 messages, 4 authors, 2016-08-30 · open the first message on its own page

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

From: Marcin Wojtas <hidden>
Date: 2016-08-23 06:27:44

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

From: Marcin Wojtas <hidden>
Date: 2016-08-23 06:28:25

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 <redacted>
---
 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

From: Marcin Wojtas <hidden>
Date: 2016-08-23 06:28:29

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 <redacted>
---
 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

From: Andrew Lunn <andrew@lunn.ch>
Date: 2016-08-23 14:16:09

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

quoted hunk

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 <redacted>
---
 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

From: Marcin Wojtas <hidden>
Date: 2016-08-24 08:57:45

HI Andrew,

2016-08-23 16:16 GMT+02:00 Andrew Lunn [off-list ref]:

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

quoted

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 <redacted>
---
 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 2/2] clk: mvebu: dynamically allocate resources in Armada CP110 system controller

From: Stephen Boyd <hidden>
Date: 2016-08-25 05:05:28

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.

quoted hunk

Signed-off-by: Marcin Wojtas <redacted>
---
 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?

quoted hunk

+		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().

quoted hunk

 	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

From: Stephen Boyd <hidden>
Date: 2016-08-25 05:08:08

On 08/23, Marcin Wojtas wrote:

quoted hunk

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 <redacted>
---
 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 1/2] clk: mvebu: set flags in CP110 gate clock

From: Thomas Petazzoni <hidden>
Date: 2016-08-30 13:10:16

Hello,

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

quoted hunk

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 [off-list ref]
    Signed-off-by: Mike Turquette [off-list ref]

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

From: Marcin Wojtas <hidden>
Date: 2016-08-30 13:34:06

Hi Thomas,

2016-08-30 15:10 GMT+02:00 Thomas Petazzoni
[off-list ref]:

Hello,

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

quoted

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 [off-list ref]
    Signed-off-by: Mike Turquette [off-list ref]

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

From: Thomas Petazzoni <hidden>
Date: 2016-08-30 14:15:39

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: <redacted>

here would be useful.

Signed-off-by: Marcin Wojtas <redacted>

Other than that:

Tested-by: Thomas Petazzoni <redacted>
 (on Armada 8K hardware)
Reviewed-by: Thomas Petazzoni <redacted>

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

From: Marcin Wojtas <hidden>
Date: 2016-08-30 15:31:24

Hi Stephen,

2016-08-25 2:16 GMT+02:00 Stephen Boyd [off-list ref]:

On 08/23, Marcin Wojtas wrote:

quoted

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.

quoted

Signed-off-by: Marcin Wojtas <redacted>
---
 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)'

quoted

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

sizeof(*cp110_clk_data) please

Ok.

quoted

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

Doesn't that return NULL on error?

Same as above, will change.

quoted

+             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?

quoted

      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

From: Stephen Boyd <hidden>
Date: 2016-08-30 18:43:16

On 08/30, Marcin Wojtas wrote:

2016-08-25 2:16 GMT+02:00 Stephen Boyd [off-list ref]:

quoted

On 08/23, Marcin Wojtas wrote:

quoted

@@ -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.

quoted

      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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help