diff mbox series

[2/5] clk: qcom: apcs-msm8916: get parent clock names from DT

Message ID 20190826164510.6425-2-jorge.ramirez-ortiz@linaro.org
State Superseded
Headers show
Series [1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency | expand

Commit Message

Jorge Ramirez-Ortiz Aug. 26, 2019, 4:45 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>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

---
 drivers/clk/qcom/apcs-msm8916.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

-- 
2.22.0

Comments

Stephen Boyd Sept. 9, 2019, 10:21 a.m. UTC | #1 
Quoting Jorge Ramirez-Ortiz (2019-08-26 09:45:07)
> @@ -76,10 +88,11 @@ 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);

> +       a53cc->pclk = of_clk_get(parent->of_node, pll_index);


Presumably the PLL was always index 0, so why are we changing it to
index 1 sometimes? Seems unnecessary.
Jorge Ramirez-Ortiz Sept. 9, 2019, 2:17 p.m. UTC | #2 
On 09/09/19 03:21:16, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2019-08-26 09:45:07)

> > @@ -76,10 +88,11 @@ 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);

> > +       a53cc->pclk = of_clk_get(parent->of_node, pll_index);

> 

> Presumably the PLL was always index 0, so why are we changing it to

> index 1 sometimes? Seems unnecessary.

> 


it came as a personal preference. hope it is acceptable (I would
rather not change it)

apcs-msm8916.c declares the following

[..]
static const u32 gpll0_a53cc_map[] = { 4, 5 };
static const char *gpll0_a53cc[] = {
       "gpll0_vote",
	"a53pll",
	};
[..]


now will be doing this

--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -429,7 +429,8 @@
     compatible = "qcom,msm8916-apcs-kpss-global", "syscon";
     reg = <0xb011000 0x1000>;
     #mbox-cells = <1>;
-                   clocks = <&a53pll>;
+                 clocks = <&gcc GPLL0_VOTE>, <&a53pll>;
+                 clock-names = "aux", "pll";
                      #clock-cells = <0>;
               };
														

so I chose to keep the consistency between the clocks definition and
just change the index before calling of_clk_get.
Jorge Ramirez-Ortiz Sept. 9, 2019, 4:54 p.m. UTC | #3 
On 09/09/19 09:17:03, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz, Linaro (2019-09-09 07:17:40)

> > On 09/09/19 03:21:16, Stephen Boyd wrote:

> > > Quoting Jorge Ramirez-Ortiz (2019-08-26 09:45:07)

> > > > @@ -76,10 +88,11 @@ 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);

> > > > +       a53cc->pclk = of_clk_get(parent->of_node, pll_index);

> > > 

> > > Presumably the PLL was always index 0, so why are we changing it to

> > > index 1 sometimes? Seems unnecessary.

> > > 

> > 

> > it came as a personal preference. hope it is acceptable (I would

> > rather not change it)

> > 

> > apcs-msm8916.c declares the following

> > 

> > [..]

> > static const u32 gpll0_a53cc_map[] = { 4, 5 };

> > static const char *gpll0_a53cc[] = {

> >        "gpll0_vote",

> >         "a53pll",

> >         };

> > [..]

> > 

> > 

> > now will be doing this

> > 

> > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi

> > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi

> > @@ -429,7 +429,8 @@

> >      compatible = "qcom,msm8916-apcs-kpss-global", "syscon";

> >      reg = <0xb011000 0x1000>;

> >      #mbox-cells = <1>;

> > -                   clocks = <&a53pll>;

> > +                 clocks = <&gcc GPLL0_VOTE>, <&a53pll>;

> > +                 clock-names = "aux", "pll";

> >                       #clock-cells = <0>;

> >                };

> >                                                                                                                 

> > 

> > so I chose to keep the consistency between the clocks definition and

> > just change the index before calling of_clk_get.

> > 

> 

> But now the binding is different for the same compatible. I'd prefer we

> keep using devm_clk_get() and use a device pointer here and reorder the

> map and parent arrays instead. The clocks property shouldn't change in a

> way that isn't "additive" so that we maintain backwards compatibility.

> 


but the backwards compatibility is fully maintained - that is the main reason
behind the change. the new stuff is that  instead of hardcoding the
names in the source - like it is being done on the msm8916- we provide
the clocks in the dts node (a cleaner approach with the obvious
benefit of allowing new users to be added without having to modify the
sources).
Jorge Ramirez-Ortiz Sept. 10, 2019, 9:34 a.m. UTC | #4 
On 9/10/19 11:14, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz, Linaro (2019-09-09 09:54:08)

>> On 09/09/19 09:17:03, Stephen Boyd wrote:

>>> But now the binding is different for the same compatible. I'd prefer we

>>> keep using devm_clk_get() and use a device pointer here and reorder the

>>> map and parent arrays instead. The clocks property shouldn't change in a

>>> way that isn't "additive" so that we maintain backwards compatibility.

>>>

>>

>> but the backwards compatibility is fully maintained - that is the main reason

>> behind the change. the new stuff is that  instead of hardcoding the

>> names in the source - like it is being done on the msm8916- we provide

>> the clocks in the dts node (a cleaner approach with the obvious

>> benefit of allowing new users to be added without having to modify the

>> sources).

>>

> 

> This is not a backwards compatible change.

> 

>>>> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi

>>>> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi

>>>> @@ -429,7 +429,8 @@

>>>>      compatible = "qcom,msm8916-apcs-kpss-global", "syscon";

>>>>      reg = <0xb011000 0x1000>;

>>>>      #mbox-cells = <1>;

>>>> -                   clocks = <&a53pll>;

>>>> +                 clocks = <&gcc GPLL0_VOTE>, <&a53pll>;

>>>> +                 clock-names = "aux", "pll";

>>>>                       #clock-cells = <0>;

>>>>                };

>>>>                                                                                                                 

> 

> Because the "clocks" property changed from

> 

> 	<&a53pll>

> 

> to

> 

> 	<&gcc GPLL0_VOTE>, <&a53pll>

> 

> and that moves pll to cell 1 instead of cell 0.

> 

> 


what do you mean by backwards compatible? because this change does not
break previous clients.
Jorge Ramirez-Ortiz Sept. 10, 2019, 9:40 a.m. UTC | #5 
On 9/10/19 11:34, Jorge Ramirez wrote:
> On 9/10/19 11:14, Stephen Boyd wrote:

>> Quoting Jorge Ramirez-Ortiz, Linaro (2019-09-09 09:54:08)

>>> On 09/09/19 09:17:03, Stephen Boyd wrote:

>>>> But now the binding is different for the same compatible. I'd prefer we

>>>> keep using devm_clk_get() and use a device pointer here and reorder the

>>>> map and parent arrays instead. The clocks property shouldn't change in a

>>>> way that isn't "additive" so that we maintain backwards compatibility.

>>>>

>>>

>>> but the backwards compatibility is fully maintained - that is the main reason

>>> behind the change. the new stuff is that  instead of hardcoding the

>>> names in the source - like it is being done on the msm8916- we provide

>>> the clocks in the dts node (a cleaner approach with the obvious

>>> benefit of allowing new users to be added without having to modify the

>>> sources).

>>>

>>

>> This is not a backwards compatible change.

>>

>>>>> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi

>>>>> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi

>>>>> @@ -429,7 +429,8 @@

>>>>>      compatible = "qcom,msm8916-apcs-kpss-global", "syscon";

>>>>>      reg = <0xb011000 0x1000>;

>>>>>      #mbox-cells = <1>;

>>>>> -                   clocks = <&a53pll>;

>>>>> +                 clocks = <&gcc GPLL0_VOTE>, <&a53pll>;

>>>>> +                 clock-names = "aux", "pll";

>>>>>                       #clock-cells = <0>;

>>>>>                };

>>>>>                                                                                                                 

>>

>> Because the "clocks" property changed from

>>

>> 	<&a53pll>

>>

>> to

>>

>> 	<&gcc GPLL0_VOTE>, <&a53pll>

>>

>> and that moves pll to cell 1 instead of cell 0.

>>

>>

> 

> what do you mean by backwards compatible? because this change does not

> break previous clients.


as per the comments I added to the code (in case this helps framing the
discussion)

[..]
legacy bindings only defined the pll parent clock (index = 0) with no
name; when both of the parents are specified in the bindings, the
pll is the second one (index = 1).
[..]



> 

> 

> 

> 

> 

> 

>
Stephen Boyd Sept. 10, 2019, 10:20 a.m. UTC | #6 
Quoting Jorge Ramirez (2019-09-10 02:40:34)
> On 9/10/19 11:34, Jorge Ramirez wrote:

> > On 9/10/19 11:14, Stephen Boyd wrote:

> >>

> >> This is not a backwards compatible change.

> >>

> >>>>> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi

> >>>>> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi

> >>>>> @@ -429,7 +429,8 @@

> >>>>>      compatible = "qcom,msm8916-apcs-kpss-global", "syscon";

> >>>>>      reg = <0xb011000 0x1000>;

> >>>>>      #mbox-cells = <1>;

> >>>>> -                   clocks = <&a53pll>;

> >>>>> +                 clocks = <&gcc GPLL0_VOTE>, <&a53pll>;

> >>>>> +                 clock-names = "aux", "pll";

> >>>>>                       #clock-cells = <0>;

> >>>>>                };

> >>>>>                                                                                                                 

> >>

> >> Because the "clocks" property changed from

> >>

> >>      <&a53pll>

> >>

> >> to

> >>

> >>      <&gcc GPLL0_VOTE>, <&a53pll>

> >>

> >> and that moves pll to cell 1 instead of cell 0.

> >>

> >>

> > 

> > what do you mean by backwards compatible? because this change does not

> > break previous clients.

> 

> as per the comments I added to the code (in case this helps framing the

> discussion)

> 

> [..]

> legacy bindings only defined the pll parent clock (index = 0) with no

> name; when both of the parents are specified in the bindings, the

> pll is the second one (index = 1).


The 'clock-names' property is entirely irrelevant to this discussion.
The PLL _must_ be index 0 forever so that the binding is left in a
backwards compatible state. Moving the PLL to index 1 and then using
clock-names to find it is a backwards incompatible change. The order of
clks in the 'clocks' property is an ABI.
diff mbox series

Patch

diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
index a6c89a310b18..dd82eb1e5202 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,8 @@  static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	struct clk_init_data init = { };
 	int ret = -ENODEV;
+	const char *parents[2];
+	int pll_index = 0;
 
 	regmap = dev_get_regmap(parent, NULL);
 	if (!regmap) {
@@ -61,6 +63,16 @@  static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	if (!a53cc)
 		return -ENOMEM;
 
+	/* legacy bindings only defined the pll parent clock (index = 0) with no
+	 * name; when both of the parents are specified in the bindings, the
+	 * pll is the second one (index = 1).
+	 */
+	if (of_clk_parent_fill(parent->of_node, parents, 2) == 2) {
+		gpll0_a53cc[0] = parents[0];
+		gpll0_a53cc[1] = parents[1];
+		pll_index = 1;
+	}
+
 	init.name = "a53mux";
 	init.parent_names = gpll0_a53cc;
 	init.num_parents = ARRAY_SIZE(gpll0_a53cc);
@@ -76,10 +88,11 @@  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);
+	a53cc->pclk = of_clk_get(parent->of_node, pll_index);
 	if (IS_ERR(a53cc->pclk)) {
 		ret = PTR_ERR(a53cc->pclk);
-		dev_err(dev, "failed to get clk: %d\n", ret);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get clk: %d\n", ret);
 		return ret;
 	}
 
@@ -87,6 +100,7 @@  static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	ret = clk_notifier_register(a53cc->pclk, &a53cc->clk_nb);
 	if (ret) {
 		dev_err(dev, "failed to register clock notifier: %d\n", ret);
+		clk_put(a53cc->pclk);
 		return ret;
 	}
 
@@ -109,6 +123,8 @@  static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 
 err:
 	clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb);
+	clk_put(a53cc->pclk);
+
 	return ret;
 }
 
@@ -117,6 +133,7 @@  static int qcom_apcs_msm8916_clk_remove(struct platform_device *pdev)
 	struct clk_regmap_mux_div *a53cc = platform_get_drvdata(pdev);
 
 	clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb);
+	clk_put(a53cc->pclk);
 
 	return 0;
 }