Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
specifications.
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Acked-by: Stephen Boyd <[email protected]>
---
drivers/clk/qcom/clk-alpha-pll.c | 8 ++++++++
drivers/clk/qcom/clk-alpha-pll.h | 1 +
drivers/clk/qcom/gcc-qcs404.c | 2 +-
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 055318f97991..9228b7b1f56e 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -878,6 +878,14 @@ static long clk_trion_pll_round_rate(struct clk_hw *hw, unsigned long rate,
return clamp(rate, min_freq, max_freq);
}
+const struct clk_ops clk_alpha_pll_fixed_ops = {
+ .enable = clk_alpha_pll_enable,
+ .disable = clk_alpha_pll_disable,
+ .is_enabled = clk_alpha_pll_is_enabled,
+ .recalc_rate = clk_alpha_pll_recalc_rate,
+};
+EXPORT_SYMBOL_GPL(clk_alpha_pll_fixed_ops);
+
const struct clk_ops clk_alpha_pll_ops = {
.enable = clk_alpha_pll_enable,
.disable = clk_alpha_pll_disable,
diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
index 15f27f4b06df..c28eb1a08c0c 100644
--- a/drivers/clk/qcom/clk-alpha-pll.h
+++ b/drivers/clk/qcom/clk-alpha-pll.h
@@ -109,6 +109,7 @@ struct alpha_pll_config {
};
extern const struct clk_ops clk_alpha_pll_ops;
+extern const struct clk_ops clk_alpha_pll_fixed_ops;
extern const struct clk_ops clk_alpha_pll_hwfsm_ops;
extern const struct clk_ops clk_alpha_pll_postdiv_ops;
extern const struct clk_ops clk_alpha_pll_huayra_ops;
diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
index e12c04c09a6a..567140709c7d 100644
--- a/drivers/clk/qcom/gcc-qcs404.c
+++ b/drivers/clk/qcom/gcc-qcs404.c
@@ -330,7 +330,7 @@ static struct clk_alpha_pll gpll0_ao_out_main = {
.parent_names = (const char *[]){ "cxo" },
.num_parents = 1,
.flags = CLK_IS_CRITICAL,
- .ops = &clk_alpha_pll_ops,
+ .ops = &clk_alpha_pll_fixed_ops,
},
},
};
--
2.22.0
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 <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
drivers/clk/qcom/apcs-msm8916.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
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;
}
--
2.22.0
On 8/26/19 18:45, Jorge Ramirez-Ortiz wrote:
> Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
> specifications.
>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> Reviewed-by: Bjorn Andersson <[email protected]>
> Acked-by: Stephen Boyd <[email protected]>
> ---
> drivers/clk/qcom/clk-alpha-pll.c | 8 ++++++++
> drivers/clk/qcom/clk-alpha-pll.h | 1 +
> drivers/clk/qcom/gcc-qcs404.c | 2 +-
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index 055318f97991..9228b7b1f56e 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -878,6 +878,14 @@ static long clk_trion_pll_round_rate(struct clk_hw *hw, unsigned long rate,
> return clamp(rate, min_freq, max_freq);
> }
>
> +const struct clk_ops clk_alpha_pll_fixed_ops = {
> + .enable = clk_alpha_pll_enable,
> + .disable = clk_alpha_pll_disable,
> + .is_enabled = clk_alpha_pll_is_enabled,
> + .recalc_rate = clk_alpha_pll_recalc_rate,
> +};
> +EXPORT_SYMBOL_GPL(clk_alpha_pll_fixed_ops);
> +
> const struct clk_ops clk_alpha_pll_ops = {
> .enable = clk_alpha_pll_enable,
> .disable = clk_alpha_pll_disable,
> diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
> index 15f27f4b06df..c28eb1a08c0c 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.h
> +++ b/drivers/clk/qcom/clk-alpha-pll.h
> @@ -109,6 +109,7 @@ struct alpha_pll_config {
> };
>
> extern const struct clk_ops clk_alpha_pll_ops;
> +extern const struct clk_ops clk_alpha_pll_fixed_ops;
> extern const struct clk_ops clk_alpha_pll_hwfsm_ops;
> extern const struct clk_ops clk_alpha_pll_postdiv_ops;
> extern const struct clk_ops clk_alpha_pll_huayra_ops;
> diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
> index e12c04c09a6a..567140709c7d 100644
> --- a/drivers/clk/qcom/gcc-qcs404.c
> +++ b/drivers/clk/qcom/gcc-qcs404.c
> @@ -330,7 +330,7 @@ static struct clk_alpha_pll gpll0_ao_out_main = {
> .parent_names = (const char *[]){ "cxo" },
> .num_parents = 1,
> .flags = CLK_IS_CRITICAL,
> - .ops = &clk_alpha_pll_ops,
> + .ops = &clk_alpha_pll_fixed_ops,
> },
> },
> };
>
just a quick follow up, is this series being picked-up?
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.
Quoting Jorge Ramirez (2019-09-05 00:30:42)
> On 8/26/19 18:45, Jorge Ramirez-Ortiz wrote:
> > Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
> > specifications.
> >
> > Co-developed-by: Niklas Cassel <[email protected]>
> > Signed-off-by: Niklas Cassel <[email protected]>
> > Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> > Reviewed-by: Bjorn Andersson <[email protected]>
> > Acked-by: Stephen Boyd <[email protected]>
> > ---
> > drivers/clk/qcom/clk-alpha-pll.c | 8 ++++++++
> > drivers/clk/qcom/clk-alpha-pll.h | 1 +
> > drivers/clk/qcom/gcc-qcs404.c | 2 +-
> > 3 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> > index 055318f97991..9228b7b1f56e 100644
> > --- a/drivers/clk/qcom/clk-alpha-pll.c
> > +++ b/drivers/clk/qcom/clk-alpha-pll.c
> > @@ -878,6 +878,14 @@ static long clk_trion_pll_round_rate(struct clk_hw *hw, unsigned long rate,
> > return clamp(rate, min_freq, max_freq);
> > }
> >
> > +const struct clk_ops clk_alpha_pll_fixed_ops = {
> > + .enable = clk_alpha_pll_enable,
> > + .disable = clk_alpha_pll_disable,
> > + .is_enabled = clk_alpha_pll_is_enabled,
> > + .recalc_rate = clk_alpha_pll_recalc_rate,
> > +};
> > +EXPORT_SYMBOL_GPL(clk_alpha_pll_fixed_ops);
> > +
> > const struct clk_ops clk_alpha_pll_ops = {
> > .enable = clk_alpha_pll_enable,
> > .disable = clk_alpha_pll_disable,
> > diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
> > index 15f27f4b06df..c28eb1a08c0c 100644
> > --- a/drivers/clk/qcom/clk-alpha-pll.h
> > +++ b/drivers/clk/qcom/clk-alpha-pll.h
> > @@ -109,6 +109,7 @@ struct alpha_pll_config {
> > };
> >
> > extern const struct clk_ops clk_alpha_pll_ops;
> > +extern const struct clk_ops clk_alpha_pll_fixed_ops;
> > extern const struct clk_ops clk_alpha_pll_hwfsm_ops;
> > extern const struct clk_ops clk_alpha_pll_postdiv_ops;
> > extern const struct clk_ops clk_alpha_pll_huayra_ops;
> > diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
> > index e12c04c09a6a..567140709c7d 100644
> > --- a/drivers/clk/qcom/gcc-qcs404.c
> > +++ b/drivers/clk/qcom/gcc-qcs404.c
> > @@ -330,7 +330,7 @@ static struct clk_alpha_pll gpll0_ao_out_main = {
> > .parent_names = (const char *[]){ "cxo" },
> > .num_parents = 1,
> > .flags = CLK_IS_CRITICAL,
> > - .ops = &clk_alpha_pll_ops,
> > + .ops = &clk_alpha_pll_fixed_ops,
> > },
> > },
> > };
> >
>
> just a quick follow up, is this series being picked-up?
No cover letter! ;P
Anyway, I'll pick it up.
Quoting Jorge Ramirez-Ortiz (2019-08-26 09:45:07)
> @@ -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
Another nitpick: This is wrong multi-line comment style. SHould be a
bare /* on this line.
> + * name; when both of the parents are specified in the bindings, the
> + * pll is the second one (index = 1).
> + */
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.
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).
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.
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.
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.
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.
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).
[..]
>
>
>
>
>
>
>