diff mbox series

[v2,05/14] clk: qcom: apcs-msm8916: get parent clock names from DT

Message ID 1548700381-22376-6-git-send-email-jorge.ramirez-ortiz@linaro.org
State New
Headers show
Series Support CPU frequency scaling on QCS404 | expand

Commit Message

Jorge Ramirez-Ortiz Jan. 28, 2019, 6:32 p.m. UTC 
Allow accessing the parent clock names required for the driver
operation by using the device tree node.

This permits extending the driver to other platforms without having to
modify its source code.

For backwards compatibility leave previous values as default.

Co-developed-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

---
 drivers/clk/qcom/apcs-msm8916.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

-- 
2.7.4

Comments

Stephen Boyd Feb. 22, 2019, 6:11 p.m. UTC | #1 
Quoting Jorge Ramirez-Ortiz (2019-01-28 10:32:52)
> @@ -61,6 +65,25 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)

>         if (!a53cc)

>                 return -ENOMEM;

>  

> +       /* check if the parent names are present in the device tree */


This looks odd.

> +       ret = devm_clk_bulk_get(parent, ARRAY_SIZE(pclks), pclks);

> +       if (ret == -EPROBE_DEFER)

> +               return ret;


Why can't we use of_clk_parent_fill() if we know this is always a DT
platform?  The parent clks may not be registered at the time of probe?
Maybe this series should wait for the parent registration stuff I'm
working on so that this can be made simpler.

> +

> +       if (!ret) {

> +               gpll0_a53cc[0] = __clk_get_name(pclks[0].clk);

> +               gpll0_a53cc[1] = __clk_get_name(pclks[1].clk);

> +               a53cc->pclk = pclks[1].clk;

> +       } else {

> +               /* support old binding where only pll was explicitily defined */

> +               a53cc->pclk = devm_clk_get(parent, NULL);

> +               if (IS_ERR(a53cc->pclk)) {

> +                       ret = PTR_ERR(a53cc->pclk);

> +                       dev_err(dev, "failed to get clk: %d\n", ret);

> +                       return ret;

> +               }

> +       }

> +

>         init.name = "a53mux";

>         init.parent_names = gpll0_a53cc;

>         init.num_parents = ARRAY_SIZE(gpll0_a53cc);
Jorge Ramirez-Ortiz April 22, 2019, 11:44 a.m. UTC | #2 
On 2/22/19 19:11, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2019-01-28 10:32:52)

>> @@ -61,6 +65,25 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)

>>         if (!a53cc)

>>                 return -ENOMEM;

>>  

>> +       /* check if the parent names are present in the device tree */

> 

> This looks odd.

> 

>> +       ret = devm_clk_bulk_get(parent, ARRAY_SIZE(pclks), pclks);

>> +       if (ret == -EPROBE_DEFER)

>> +               return ret;

> 

> Why can't we use of_clk_parent_fill() if we know this is always a DT

> platform?  The parent clks may not be registered at the time of probe?


yes, and AFAICS the important thing at this point is that the clock is
registered hence the handling of defer.

I could use of_clk_parent_fill and then - if needed - call
devm_clk_bulk_get but I am not sure of the gains of doing it (wouldnt
this just make the code more confusing?)


> Maybe this series should wait for the parent registration stuff I'm

> working on so that this can be made simpler.


the need for the clock name is not intrinsic to this driver (the driver
itself doesnt use these names) but it just feeds these to the framework.

I was looking into your parent registration code and I am not sure how
can I use it in this particular driver other than simply removing the
names and hoping that things are handled properly at the lower
levels.... could you clarify please?


> 

>> +

>> +       if (!ret) {

>> +               gpll0_a53cc[0] = __clk_get_name(pclks[0].clk);

>> +               gpll0_a53cc[1] = __clk_get_name(pclks[1].clk);

>> +               a53cc->pclk = pclks[1].clk;

>> +       } else {

>> +               /* support old binding where only pll was explicitily defined */

>> +               a53cc->pclk = devm_clk_get(parent, NULL);

>> +               if (IS_ERR(a53cc->pclk)) {

>> +                       ret = PTR_ERR(a53cc->pclk);

>> +                       dev_err(dev, "failed to get clk: %d\n", ret);

>> +                       return ret;

>> +               }

>> +       }

>> +

>>         init.name = "a53mux";

>>         init.parent_names = gpll0_a53cc;

>>         init.num_parents = ARRAY_SIZE(gpll0_a53cc);

>
diff mbox series

Patch

diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
index a6c89a3..987ee00 100644
--- a/drivers/clk/qcom/apcs-msm8916.c
+++ b/drivers/clk/qcom/apcs-msm8916.c
@@ -19,7 +19,7 @@ 
 
 static const u32 gpll0_a53cc_map[] = { 4, 5 };
 
-static const char * const gpll0_a53cc[] = {
+static const char *gpll0_a53cc[] = {
 	"gpll0_vote",
 	"a53pll",
 };
@@ -50,6 +50,10 @@  static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	struct clk_init_data init = { };
 	int ret = -ENODEV;
+	struct clk_bulk_data pclks[] = {
+		[0] = { .id = "aux", .clk = NULL },
+		[1] = { .id = "pll", .clk = NULL },
+	};
 
 	regmap = dev_get_regmap(parent, NULL);
 	if (!regmap) {
@@ -61,6 +65,25 @@  static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	if (!a53cc)
 		return -ENOMEM;
 
+	/* check if the parent names are present in the device tree */
+	ret = devm_clk_bulk_get(parent, ARRAY_SIZE(pclks), pclks);
+	if (ret == -EPROBE_DEFER)
+		return ret;
+
+	if (!ret) {
+		gpll0_a53cc[0] = __clk_get_name(pclks[0].clk);
+		gpll0_a53cc[1] = __clk_get_name(pclks[1].clk);
+		a53cc->pclk = pclks[1].clk;
+	} else {
+		/* support old binding where only pll was explicitily defined */
+		a53cc->pclk = devm_clk_get(parent, NULL);
+		if (IS_ERR(a53cc->pclk)) {
+			ret = PTR_ERR(a53cc->pclk);
+			dev_err(dev, "failed to get clk: %d\n", ret);
+			return ret;
+		}
+	}
+
 	init.name = "a53mux";
 	init.parent_names = gpll0_a53cc;
 	init.num_parents = ARRAY_SIZE(gpll0_a53cc);
@@ -76,13 +99,6 @@  static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	a53cc->src_shift = 8;
 	a53cc->parent_map = gpll0_a53cc_map;
 
-	a53cc->pclk = devm_clk_get(parent, NULL);
-	if (IS_ERR(a53cc->pclk)) {
-		ret = PTR_ERR(a53cc->pclk);
-		dev_err(dev, "failed to get clk: %d\n", ret);
-		return ret;
-	}
-
 	a53cc->clk_nb.notifier_call = a53cc_notifier_cb;
 	ret = clk_notifier_register(a53cc->pclk, &a53cc->clk_nb);
 	if (ret) {