| Message ID | 20190826164510.6425-2-jorge.ramirez-ortiz@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency | expand |
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-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). > + */
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 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).
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: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.
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). [..] > > > > > > >
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 --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; }