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 (mailing list archive)
State New, archived
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(-)

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);
>
Stephen Boyd April 25, 2019, 9:29 p.m. UTC | #3 
Quoting Jorge Ramirez (2019-04-22 04:44:50)
> 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?)

Yeah of_clk_parent_fill() isn't the best approach. But it at least keeps
this driver from using clk consumer APIs?

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

I think so. I've forgotten the context of this patch, but the general
idea would be to specify the parents with clock-names or DT index in the
DT node for the clks registered here and not use of_clk_parent_fill() or
do any sort of devm_clk_bulk_get() calls. Then the framework will take
care of finding the parents for the clks and hooking things up properly
for the parent-child relationship.
Jorge Ramirez-Ortiz April 25, 2019, 9:42 p.m. UTC | #4 
On 4/25/19 23:29, Stephen Boyd wrote:
> Quoting Jorge Ramirez (2019-04-22 04:44:50)
>> 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?)
> 
> Yeah of_clk_parent_fill() isn't the best approach. But it at least keeps
> this driver from using clk consumer APIs?

ok will do that then.

> 
>>
>>
>>> 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?
>>
> 
> I think so. I've forgotten the context of this patch, but the general
> idea would be to specify the parents with clock-names or DT index in the
> DT node for the clks registered here and not use of_clk_parent_fill() or
> do any sort of devm_clk_bulk_get() calls. Then the framework will take
> care of finding the parents for the clks and hooking things up properly
> for the parent-child relationship.
> 
> 

ok will try that. thanks for confirming!
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) {