| Message ID | 1545039990-19984-6-git-send-email-jorge.ramirez-ortiz@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support CPU frequency scaling on QCS404 | expand |
On 12/18/18 00:37, Stephen Boyd wrote: > Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:22) >> 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. > Why do we need to maintain backwards compatibility? Isn't is required > that the nodes have clocks properties? > > this driver -apcs clock controller- uses platform data (not DT) and therefore it uses the DT from the parent node (mailbox). And for the mailbox the clock property is optional. So the APCS clock controller requires that the parent provides at least one clock but the clock is not mandatory in the parent DT node. For instance in the case of the msm8916, the parent only provides one clock, just the pll. am I required to modify that platform instead of maintaining backwards compatibility?
On 12/18/18 15:35, Niklas Cassel wrote: > On Mon, Dec 17, 2018 at 03:37:43PM -0800, Stephen Boyd wrote: >> Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:22) >>> 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. >> >> Why do we need to maintain backwards compatibility? Isn't is required >> that the nodes have clocks properties? >> > > Hello Stephen, > > > This is the existing DT nodes for msm8916: > > a53pll: clock@b016000 { > compatible = "qcom,msm8916-a53pll"; > reg = <0xb016000 0x40>; > #clock-cells = <0>; > }; > > apcs: mailbox@b011000 { > compatible = "qcom,msm8916-apcs-kpss-global", "syscon"; > reg = <0xb011000 0x1000>; > #mbox-cells = <1>; > clocks = <&a53pll>; > #clock-cells = <0>; > }; > > > This is the (suggested) DT nodes for qcs404: > > apcs_hfpll: clock-controller@0b016000 { > compatible = "qcom,hfpll"; > reg = <0x0b016000 0x30>; > #clock-cells = <0>; > clock-output-names = "apcs_hfpll"; > clocks = <&xo_board>; > clock-names = "xo"; > }; > > apcs_glb: mailbox@b011000 { > compatible = "qcom,qcs404-apcs-apps-global", "syscon"; > reg = <0x0b011000 0x1000>; > #mbox-cells = <1>; > clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>; > clock-names = "aux", "pll"; > #clock-cells = <0>; > }; > > qcs404 specifies two clocks, with an accompanied clock-name for each clock. > > msm8916 specifies a single clock, without an accompanied clock-name. > > It is possible to append clock-names = "pll" for the existing clock, > as well as to define the aux clock in the apcs node in the msm8916 DT: > clocks = <&gcc GPLL0_VOTE>; > clock-names = "aux"; > > However, since the DT is treated as an ABI, the existing DT for msm8916 must > still work, so I don't think that it is possible to ignore having backwards > compability in the apcs clock driver. so where are we with this? do we remove backwards compatibility (see below] for v2 or is the DT really an ABI and therefore the patch under review is good as is? diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi index c5348c3..729c117 100644 --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi @@ -425,7 +425,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>; }; > > > Kind regards, > Niklas >
Quoting Niklas Cassel (2018-12-18 06:35:03) > > This is the existing DT nodes for msm8916: > > a53pll: clock@b016000 { > compatible = "qcom,msm8916-a53pll"; > reg = <0xb016000 0x40>; > #clock-cells = <0>; > }; > > apcs: mailbox@b011000 { > compatible = "qcom,msm8916-apcs-kpss-global", "syscon"; > reg = <0xb011000 0x1000>; > #mbox-cells = <1>; > clocks = <&a53pll>; > #clock-cells = <0>; > }; > > > This is the (suggested) DT nodes for qcs404: > > apcs_hfpll: clock-controller@0b016000 { > compatible = "qcom,hfpll"; > reg = <0x0b016000 0x30>; > #clock-cells = <0>; > clock-output-names = "apcs_hfpll"; > clocks = <&xo_board>; > clock-names = "xo"; > }; > > apcs_glb: mailbox@b011000 { > compatible = "qcom,qcs404-apcs-apps-global", "syscon"; > reg = <0x0b011000 0x1000>; > #mbox-cells = <1>; > clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>; > clock-names = "aux", "pll"; > #clock-cells = <0>; > }; > > qcs404 specifies two clocks, with an accompanied clock-name for each clock. > > msm8916 specifies a single clock, without an accompanied clock-name. > > It is possible to append clock-names = "pll" for the existing clock, > as well as to define the aux clock in the apcs node in the msm8916 DT: > clocks = <&gcc GPLL0_VOTE>; > clock-names = "aux"; > > However, since the DT is treated as an ABI, the existing DT for msm8916 must > still work, so I don't think that it is possible to ignore having backwards > compability in the apcs clock driver. > We typically allow one clk to match the NULL connection name, so I think things should work if you clk_get(dev, NULL) on 8916 and then try the specific "aux" and "pll" strings strings after that for updated DT. Not sure if that helps you here though. And I would push to try and make all the clk connections between clk controller nodes specified now. It will be useful to avoid global string lookups in the future, so the sooner the better.
Quoting Jorge Ramirez (2018-12-26 01:20:07) > On 12/18/18 15:35, Niklas Cassel wrote: > > On Mon, Dec 17, 2018 at 03:37:43PM -0800, Stephen Boyd wrote: > >> Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:22) > >>> 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. > >> > >> Why do we need to maintain backwards compatibility? Isn't is required > >> that the nodes have clocks properties? > >> > > > > Hello Stephen, > > > > > > This is the existing DT nodes for msm8916: > > > > a53pll: clock@b016000 { > > compatible = "qcom,msm8916-a53pll"; > > reg = <0xb016000 0x40>; > > #clock-cells = <0>; > > }; > > > > apcs: mailbox@b011000 { > > compatible = "qcom,msm8916-apcs-kpss-global", "syscon"; > > reg = <0xb011000 0x1000>; > > #mbox-cells = <1>; > > clocks = <&a53pll>; > > #clock-cells = <0>; > > }; > > > > > > This is the (suggested) DT nodes for qcs404: > > > > apcs_hfpll: clock-controller@0b016000 { > > compatible = "qcom,hfpll"; > > reg = <0x0b016000 0x30>; > > #clock-cells = <0>; > > clock-output-names = "apcs_hfpll"; > > clocks = <&xo_board>; > > clock-names = "xo"; > > }; > > > > apcs_glb: mailbox@b011000 { > > compatible = "qcom,qcs404-apcs-apps-global", "syscon"; > > reg = <0x0b011000 0x1000>; > > #mbox-cells = <1>; > > clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>; > > clock-names = "aux", "pll"; > > #clock-cells = <0>; > > }; > > > > qcs404 specifies two clocks, with an accompanied clock-name for each clock. > > > > msm8916 specifies a single clock, without an accompanied clock-name. > > > > It is possible to append clock-names = "pll" for the existing clock, > > as well as to define the aux clock in the apcs node in the msm8916 DT: > > clocks = <&gcc GPLL0_VOTE>; > > clock-names = "aux"; > > > > However, since the DT is treated as an ABI, the existing DT for msm8916 must > > still work, so I don't think that it is possible to ignore having backwards > > compability in the apcs clock driver. > > > so where are we with this? > > do we remove backwards compatibility (see below] for v2 or is the DT > really an ABI and therefore the patch under review is good as is? > Breaking compatibility is up to the platform maintainers. If anything, I would make the DTS and driver changes in parallel and then remove the driver's backwards compatibility logic later on.
On Mon 31 Dec 00:42 PST 2018, Jorge Ramirez wrote: [..] > I am not completely sure of what you mean. are you saying that the original > patch is good and we should just provide another patch (not part of the > current patch series) to remove the compatibility? > You're expected to make sure that db410c continues to function on existing DTBs, at least for a considerable amount of time. Regards, Bjorn
diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c index a6c89a3..2453242 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,17 +50,39 @@ 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) { dev_err(dev, "failed to get regmap: %d\n", ret); return ret; } - a53cc = devm_kzalloc(dev, sizeof(*a53cc), GFP_KERNEL); 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 +98,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) {