linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency
@ 2019-08-26 16:45 Jorge Ramirez-Ortiz
  2019-08-26 16:45 ` [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT Jorge Ramirez-Ortiz
                   ` (4 more replies)
  0 siblings, 5 replies; 19+ messages in thread
From: Jorge Ramirez-Ortiz @ 2019-08-26 16:45 UTC (permalink / raw)
  To: jorge.ramirez-ortiz, sboyd, agross, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
specifications.

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>
Acked-by: Stephen Boyd <sboyd@kernel.org>
---
 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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
  2019-08-26 16:45 [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency Jorge Ramirez-Ortiz
@ 2019-08-26 16:45 ` Jorge Ramirez-Ortiz
  2019-09-09 10:21   ` Stephen Boyd
  2019-09-09 10:23   ` Stephen Boyd
  2019-08-26 16:45 ` [PATCH 3/5] clk: qcom: hfpll: " Jorge Ramirez-Ortiz
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 19+ messages in thread
From: Jorge Ramirez-Ortiz @ 2019-08-26 16:45 UTC (permalink / raw)
  To: jorge.ramirez-ortiz, sboyd, agross, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

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(-)

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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/5] clk: qcom: hfpll: get parent clock names from DT
  2019-08-26 16:45 [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency Jorge Ramirez-Ortiz
  2019-08-26 16:45 ` [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT Jorge Ramirez-Ortiz
@ 2019-08-26 16:45 ` Jorge Ramirez-Ortiz
  2019-09-09 10:22   ` Stephen Boyd
  2019-08-26 16:45 ` [PATCH 4/5] clk: qcom: hfpll: register as clock provider Jorge Ramirez-Ortiz
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 19+ messages in thread
From: Jorge Ramirez-Ortiz @ 2019-08-26 16:45 UTC (permalink / raw)
  To: jorge.ramirez-ortiz, sboyd, agross, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

Allow accessing the parent clock name required for the driver
operation using the device tree node.

This permits extending the driver to other platforms without having to
modify its source code.

For backwards compatibility leave the previous value 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/hfpll.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c
index a6de7101430c..87b7f46d27e0 100644
--- a/drivers/clk/qcom/hfpll.c
+++ b/drivers/clk/qcom/hfpll.c
@@ -52,6 +52,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
 	void __iomem *base;
 	struct regmap *regmap;
 	struct clk_hfpll *h;
+	struct clk *pclk;
 	struct clk_init_data init = {
 		.parent_names = (const char *[]){ "xo" },
 		.num_parents = 1,
@@ -75,6 +76,13 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
 					  0, &init.name))
 		return -ENODEV;
 
+	/* get parent clock from device tree (optional) */
+	pclk = devm_clk_get(dev, "xo");
+	if (!IS_ERR(pclk))
+		init.parent_names = (const char *[]){ __clk_get_name(pclk) };
+	else if (PTR_ERR(pclk) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
 	h->d = &hdata;
 	h->clkr.hw.init = &init;
 	spin_lock_init(&h->lock);
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/5] clk: qcom: hfpll: register as clock provider
  2019-08-26 16:45 [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency Jorge Ramirez-Ortiz
  2019-08-26 16:45 ` [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT Jorge Ramirez-Ortiz
  2019-08-26 16:45 ` [PATCH 3/5] clk: qcom: hfpll: " Jorge Ramirez-Ortiz
@ 2019-08-26 16:45 ` Jorge Ramirez-Ortiz
  2019-09-09 10:22   ` Stephen Boyd
  2019-08-26 16:45 ` [PATCH 5/5] clk: qcom: hfpll: CLK_IGNORE_UNUSED Jorge Ramirez-Ortiz
  2019-09-05  7:30 ` [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency Jorge Ramirez
  4 siblings, 1 reply; 19+ messages in thread
From: Jorge Ramirez-Ortiz @ 2019-08-26 16:45 UTC (permalink / raw)
  To: jorge.ramirez-ortiz, sboyd, agross, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

Make the output of the high frequency pll a clock provider.
On the QCS404 this PLL controls cpu frequency scaling.

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>
Acked-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/qcom/hfpll.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c
index 87b7f46d27e0..0ffed0d41c50 100644
--- a/drivers/clk/qcom/hfpll.c
+++ b/drivers/clk/qcom/hfpll.c
@@ -53,6 +53,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	struct clk_hfpll *h;
 	struct clk *pclk;
+	int ret;
 	struct clk_init_data init = {
 		.parent_names = (const char *[]){ "xo" },
 		.num_parents = 1,
@@ -87,7 +88,14 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
 	h->clkr.hw.init = &init;
 	spin_lock_init(&h->lock);
 
-	return devm_clk_register_regmap(&pdev->dev, &h->clkr);
+	ret = devm_clk_register_regmap(dev, &h->clkr);
+	if (ret) {
+		dev_err(dev, "failed to register regmap clock: %d\n", ret);
+		return ret;
+	}
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+					   &h->clkr.hw);
 }
 
 static struct platform_driver qcom_hfpll_driver = {
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 5/5] clk: qcom: hfpll: CLK_IGNORE_UNUSED
  2019-08-26 16:45 [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency Jorge Ramirez-Ortiz
                   ` (2 preceding siblings ...)
  2019-08-26 16:45 ` [PATCH 4/5] clk: qcom: hfpll: register as clock provider Jorge Ramirez-Ortiz
@ 2019-08-26 16:45 ` Jorge Ramirez-Ortiz
  2019-09-09 10:23   ` Stephen Boyd
  2019-09-05  7:30 ` [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency Jorge Ramirez
  4 siblings, 1 reply; 19+ messages in thread
From: Jorge Ramirez-Ortiz @ 2019-08-26 16:45 UTC (permalink / raw)
  To: jorge.ramirez-ortiz, sboyd, agross, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

When COMMON_CLK_DISABLED_UNUSED is set, in an effort to save power and
to keep the software model of the clock in line with reality, the
framework transverses the clock tree and disables those clocks that
were enabled by the firmware but have not been enabled by any device
driver.

If CPUFREQ is enabled, early during the system boot, it might attempt
to change the CPU frequency ("set_rate"). If the HFPLL is selected as
a provider, it will then change the rate for this clock.

As boot continues, clk_disable_unused_subtree will run. Since it wont
find a valid counter (enable_count) for a clock that is actually
enabled it will attempt to disable it which will cause the CPU to
stop. Notice that in this driver, calls to check whether the clock is
enabled are routed via the is_enabled callback which queries the
hardware.

The following commit, rather than marking the clock critical and
forcing the clock to be always enabled, addresses the above scenario
making sure the clock is not disabled but it continues to rely on the
firmware to enable the clock.

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/hfpll.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c
index 0ffed0d41c50..d5fd27938e7b 100644
--- a/drivers/clk/qcom/hfpll.c
+++ b/drivers/clk/qcom/hfpll.c
@@ -58,6 +58,13 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
 		.parent_names = (const char *[]){ "xo" },
 		.num_parents = 1,
 		.ops = &clk_ops_hfpll,
+		/*
+		 * rather than marking the clock critical and forcing the clock
+		 * to be always enabled, we make sure that the clock is not
+		 * disabled: the firmware remains responsible of enabling this
+		 * clock (for more info check the commit log)
+		 */
+		.flags = CLK_IGNORE_UNUSED,
 	};
 
 	h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL);
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency
  2019-08-26 16:45 [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency Jorge Ramirez-Ortiz
                   ` (3 preceding siblings ...)
  2019-08-26 16:45 ` [PATCH 5/5] clk: qcom: hfpll: CLK_IGNORE_UNUSED Jorge Ramirez-Ortiz
@ 2019-09-05  7:30 ` Jorge Ramirez
  2019-09-09 10:05   ` Stephen Boyd
  4 siblings, 1 reply; 19+ messages in thread
From: Jorge Ramirez @ 2019-09-05  7:30 UTC (permalink / raw)
  To: sboyd, agross, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

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 <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>
> Acked-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  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?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency
  2019-09-05  7:30 ` [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency Jorge Ramirez
@ 2019-09-09 10:05   ` Stephen Boyd
  0 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2019-09-09 10:05 UTC (permalink / raw)
  To: Jorge Ramirez, agross, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

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 <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>
> > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > ---
> >  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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
  2019-08-26 16:45 ` [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT Jorge Ramirez-Ortiz
@ 2019-09-09 10:21   ` Stephen Boyd
  2019-09-09 14:17     ` Jorge Ramirez-Ortiz, Linaro
  2019-09-09 10:23   ` Stephen Boyd
  1 sibling, 1 reply; 19+ messages in thread
From: Stephen Boyd @ 2019-09-09 10:21 UTC (permalink / raw)
  To: agross, jorge.ramirez-ortiz, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 3/5] clk: qcom: hfpll: get parent clock names from DT
  2019-08-26 16:45 ` [PATCH 3/5] clk: qcom: hfpll: " Jorge Ramirez-Ortiz
@ 2019-09-09 10:22   ` Stephen Boyd
  0 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2019-09-09 10:22 UTC (permalink / raw)
  To: agross, jorge.ramirez-ortiz, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

Quoting Jorge Ramirez-Ortiz (2019-08-26 09:45:08)
> diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c
> index a6de7101430c..87b7f46d27e0 100644
> --- a/drivers/clk/qcom/hfpll.c
> +++ b/drivers/clk/qcom/hfpll.c
> @@ -52,6 +52,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
>         void __iomem *base;
>         struct regmap *regmap;
>         struct clk_hfpll *h;
> +       struct clk *pclk;
>         struct clk_init_data init = {
>                 .parent_names = (const char *[]){ "xo" },
>                 .num_parents = 1,
> @@ -75,6 +76,13 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
>                                           0, &init.name))
>                 return -ENODEV;
>  
> +       /* get parent clock from device tree (optional) */
> +       pclk = devm_clk_get(dev, "xo");
> +       if (!IS_ERR(pclk))
> +               init.parent_names = (const char *[]){ __clk_get_name(pclk) };
> +       else if (PTR_ERR(pclk) == -EPROBE_DEFER)
> +               return -EPROBE_DEFER;
> +

Can this use the "new" way of specifying parents of clks? That would be
better than calling clk_get() on the XO clk to handle this.

>         h->d = &hdata;
>         h->clkr.hw.init = &init;
>         spin_lock_init(&h->lock);

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/5] clk: qcom: hfpll: register as clock provider
  2019-08-26 16:45 ` [PATCH 4/5] clk: qcom: hfpll: register as clock provider Jorge Ramirez-Ortiz
@ 2019-09-09 10:22   ` Stephen Boyd
  0 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2019-09-09 10:22 UTC (permalink / raw)
  To: agross, jorge.ramirez-ortiz, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

Quoting Jorge Ramirez-Ortiz (2019-08-26 09:45:09)
> Make the output of the high frequency pll a clock provider.
> On the QCS404 this PLL controls cpu frequency scaling.
> 
> 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>
> Acked-by: Stephen Boyd <sboyd@kernel.org>

Please move this earlier in the series.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 5/5] clk: qcom: hfpll: CLK_IGNORE_UNUSED
  2019-08-26 16:45 ` [PATCH 5/5] clk: qcom: hfpll: CLK_IGNORE_UNUSED Jorge Ramirez-Ortiz
@ 2019-09-09 10:23   ` Stephen Boyd
  0 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2019-09-09 10:23 UTC (permalink / raw)
  To: agross, jorge.ramirez-ortiz, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

Quoting Jorge Ramirez-Ortiz (2019-08-26 09:45:10)
> When COMMON_CLK_DISABLED_UNUSED is set, in an effort to save power and
> to keep the software model of the clock in line with reality, the
> framework transverses the clock tree and disables those clocks that
> were enabled by the firmware but have not been enabled by any device
> driver.
> 
> If CPUFREQ is enabled, early during the system boot, it might attempt
> to change the CPU frequency ("set_rate"). If the HFPLL is selected as
> a provider, it will then change the rate for this clock.
> 
> As boot continues, clk_disable_unused_subtree will run. Since it wont
> find a valid counter (enable_count) for a clock that is actually
> enabled it will attempt to disable it which will cause the CPU to
> stop. Notice that in this driver, calls to check whether the clock is
> enabled are routed via the is_enabled callback which queries the
> hardware.
> 
> The following commit, rather than marking the clock critical and
> forcing the clock to be always enabled, addresses the above scenario
> making sure the clock is not disabled but it continues to rely on the
> firmware to enable the clock.
> 
> 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>
> ---

Also this one. Seems fine to merge immediately vs the ones that do some
DT trickery.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
  2019-08-26 16:45 ` [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT Jorge Ramirez-Ortiz
  2019-09-09 10:21   ` Stephen Boyd
@ 2019-09-09 10:23   ` Stephen Boyd
  1 sibling, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2019-09-09 10:23 UTC (permalink / raw)
  To: agross, jorge.ramirez-ortiz, mturquette
  Cc: bjorn.andersson, niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

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).
> +        */

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
  2019-09-09 10:21   ` Stephen Boyd
@ 2019-09-09 14:17     ` Jorge Ramirez-Ortiz, Linaro
  2019-09-09 16:17       ` Stephen Boyd
  0 siblings, 1 reply; 19+ messages in thread
From: Jorge Ramirez-Ortiz, Linaro @ 2019-09-09 14:17 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: agross, jorge.ramirez-ortiz, mturquette, bjorn.andersson,
	niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

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.





^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
  2019-09-09 14:17     ` Jorge Ramirez-Ortiz, Linaro
@ 2019-09-09 16:17       ` Stephen Boyd
  2019-09-09 16:54         ` Jorge Ramirez-Ortiz, Linaro
  0 siblings, 1 reply; 19+ messages in thread
From: Stephen Boyd @ 2019-09-09 16:17 UTC (permalink / raw)
  To: Jorge Ramirez-Ortiz, Linaro
  Cc: agross, jorge.ramirez-ortiz, mturquette, bjorn.andersson,
	niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
  2019-09-09 16:17       ` Stephen Boyd
@ 2019-09-09 16:54         ` Jorge Ramirez-Ortiz, Linaro
  2019-09-10  9:14           ` Stephen Boyd
  0 siblings, 1 reply; 19+ messages in thread
From: Jorge Ramirez-Ortiz, Linaro @ 2019-09-09 16:54 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Jorge Ramirez-Ortiz, Linaro, agross, mturquette, bjorn.andersson,
	niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

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).




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
  2019-09-09 16:54         ` Jorge Ramirez-Ortiz, Linaro
@ 2019-09-10  9:14           ` Stephen Boyd
  2019-09-10  9:34             ` Jorge Ramirez
  0 siblings, 1 reply; 19+ messages in thread
From: Stephen Boyd @ 2019-09-10  9:14 UTC (permalink / raw)
  To: Jorge Ramirez-Ortiz, Linaro
  Cc: Jorge Ramirez-Ortiz, Linaro, agross, mturquette, bjorn.andersson,
	niklas.cassel, linux-arm-msm, linux-clk, linux-kernel

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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
  2019-09-10  9:14           ` Stephen Boyd
@ 2019-09-10  9:34             ` Jorge Ramirez
  2019-09-10  9:40               ` Jorge Ramirez
  0 siblings, 1 reply; 19+ messages in thread
From: Jorge Ramirez @ 2019-09-10  9:34 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: agross, mturquette, bjorn.andersson, niklas.cassel,
	linux-arm-msm, linux-clk, linux-kernel

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.








^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
  2019-09-10  9:34             ` Jorge Ramirez
@ 2019-09-10  9:40               ` Jorge Ramirez
  2019-09-10 10:20                 ` Stephen Boyd
  0 siblings, 1 reply; 19+ messages in thread
From: Jorge Ramirez @ 2019-09-10  9:40 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: agross, mturquette, bjorn.andersson, niklas.cassel,
	linux-arm-msm, linux-clk, linux-kernel

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).
[..]



> 
> 
> 
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
  2019-09-10  9:40               ` Jorge Ramirez
@ 2019-09-10 10:20                 ` Stephen Boyd
  0 siblings, 0 replies; 19+ messages in thread
From: Stephen Boyd @ 2019-09-10 10:20 UTC (permalink / raw)
  To: Jorge Ramirez
  Cc: agross, mturquette, bjorn.andersson, niklas.cassel,
	linux-arm-msm, linux-clk, linux-kernel

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.


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-09-10 10:20 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-26 16:45 [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency Jorge Ramirez-Ortiz
2019-08-26 16:45 ` [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT Jorge Ramirez-Ortiz
2019-09-09 10:21   ` Stephen Boyd
2019-09-09 14:17     ` Jorge Ramirez-Ortiz, Linaro
2019-09-09 16:17       ` Stephen Boyd
2019-09-09 16:54         ` Jorge Ramirez-Ortiz, Linaro
2019-09-10  9:14           ` Stephen Boyd
2019-09-10  9:34             ` Jorge Ramirez
2019-09-10  9:40               ` Jorge Ramirez
2019-09-10 10:20                 ` Stephen Boyd
2019-09-09 10:23   ` Stephen Boyd
2019-08-26 16:45 ` [PATCH 3/5] clk: qcom: hfpll: " Jorge Ramirez-Ortiz
2019-09-09 10:22   ` Stephen Boyd
2019-08-26 16:45 ` [PATCH 4/5] clk: qcom: hfpll: register as clock provider Jorge Ramirez-Ortiz
2019-09-09 10:22   ` Stephen Boyd
2019-08-26 16:45 ` [PATCH 5/5] clk: qcom: hfpll: CLK_IGNORE_UNUSED Jorge Ramirez-Ortiz
2019-09-09 10:23   ` Stephen Boyd
2019-09-05  7:30 ` [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency Jorge Ramirez
2019-09-09 10:05   ` Stephen Boyd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).