[PATCH v3 0/7] Clock changes to support cpufreq on QCS404

[PATCH v3 0/7] Clock changes to support cpufreq on QCS404

[Niklas Cassel]

The following clock changes are required to enable cpufreq support on
the QCS404.

Changes since v2:
-Addressed Stephen Boyd's comment regarding apcs-msm8916
should use new way of specifying clock parents.
-DT binding now has "pll" as first clock, in order to
not break DT backwards compatibility (in case no clock-names
are given).
-Moved EPROBE_DEFER error handling to its own patch.

Jorge Ramirez-Ortiz (6):
  dt-bindings: mailbox: qcom: Add clock-name optional property
  clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency
  clk: qcom: hfpll: register as clock provider
  clk: qcom: hfpll: CLK_IGNORE_UNUSED
  clk: qcom: hfpll: use clk_parent_data to specify the parent
  clk: qcom: apcs-msm8916: silently error out on EPROBE_DEFER

Niklas Cassel (1):
  clk: qcom: apcs-msm8916: use clk_parent_data to specify the parent

 .../mailbox/qcom,apcs-kpss-global.txt         | 24 ++++++++++++++---
 drivers/clk/qcom/apcs-msm8916.c               | 26 ++++++++++++++-----
 drivers/clk/qcom/clk-alpha-pll.c              |  8 ++++++
 drivers/clk/qcom/clk-alpha-pll.h              |  1 +
 drivers/clk/qcom/gcc-qcs404.c                 |  2 +-
 drivers/clk/qcom/hfpll.c                      | 21 +++++++++++++--
 6 files changed, 70 insertions(+), 12 deletions(-)

-- 
2.23.0

[PATCH v3 2/7] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency

[Niklas Cassel]

From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

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>
---
Changes since v2:
-None

 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 9b0c4ce2ef4e..46d314d69250 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.23.0

[PATCH v3 3/7] clk: qcom: hfpll: register as clock provider

[Niklas Cassel]

From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

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>
---
Changes since v2:
-None

 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 a6de7101430c..e64c0fd82fe4 100644
--- a/drivers/clk/qcom/hfpll.c
+++ b/drivers/clk/qcom/hfpll.c
@@ -57,6 +57,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
 		.num_parents = 1,
 		.ops = &clk_ops_hfpll,
 	};
+	int ret;
 
 	h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL);
 	if (!h)
@@ -79,7 +80,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.23.0

[PATCH v3 4/7] clk: qcom: hfpll: CLK_IGNORE_UNUSED

[Niklas Cassel]

From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

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>
---
Changes since v2:
-None

 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 e64c0fd82fe4..225c675f6779 100644
--- a/drivers/clk/qcom/hfpll.c
+++ b/drivers/clk/qcom/hfpll.c
@@ -56,6 +56,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,
 	};
 	int ret;
 
-- 
2.23.0

[PATCH v3 5/7] clk: qcom: hfpll: use clk_parent_data to specify the parent

[Niklas Cassel]

From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

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

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>
---
Changes since v2:
-None

 drivers/clk/qcom/hfpll.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c
index 225c675f6779..5ff7f5a60620 100644
--- a/drivers/clk/qcom/hfpll.c
+++ b/drivers/clk/qcom/hfpll.c
@@ -53,7 +53,6 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	struct clk_hfpll *h;
 	struct clk_init_data init = {
-		.parent_names = (const char *[]){ "xo" },
 		.num_parents = 1,
 		.ops = &clk_ops_hfpll,
 		/*
@@ -65,6 +64,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
 		.flags = CLK_IGNORE_UNUSED,
 	};
 	int ret;
+	struct clk_parent_data pdata = { .index = 0 };
 
 	h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL);
 	if (!h)
@@ -83,6 +83,8 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
 					  0, &init.name))
 		return -ENODEV;
 
+	init.parent_data = &pdata;
+
 	h->d = &hdata;
 	h->clkr.hw.init = &init;
 	spin_lock_init(&h->lock);
-- 
2.23.0

[PATCH v3 6/7] clk: qcom: apcs-msm8916: silently error out on EPROBE_DEFER

[Niklas Cassel]

From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

If devm_clk_get() fails due to probe deferral, we shouldn't print an
error message. Just be silent in this case.

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>
---
Changes since v2:
-New patch. (This change was previously part of another
patch in this series.)

 drivers/clk/qcom/apcs-msm8916.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
index a6c89a310b18..46061b3d230e 100644
--- a/drivers/clk/qcom/apcs-msm8916.c
+++ b/drivers/clk/qcom/apcs-msm8916.c
@@ -79,7 +79,8 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	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);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get clk: %d\n", ret);
 		return ret;
 	}
 
-- 
2.23.0

[PATCH v3 7/7] clk: qcom: apcs-msm8916: use clk_parent_data to specify the parent

[Niklas Cassel]

Allow accessing the parent clock names required for the driver
operation by using the device tree node, while falling back to
the previous method of using names in the global name space.

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

Co-developed-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
---
Changes since v2:
-Use clk_parent_data when specifying clock parents.

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

diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
index 46061b3d230e..bb91644edc00 100644
--- a/drivers/clk/qcom/apcs-msm8916.c
+++ b/drivers/clk/qcom/apcs-msm8916.c
@@ -19,9 +19,9 @@
 
 static const u32 gpll0_a53cc_map[] = { 4, 5 };
 
-static const char * const gpll0_a53cc[] = {
-	"gpll0_vote",
-	"a53pll",
+static const struct clk_parent_data pdata[] = {
+	{ .fw_name = "aux", .name = "gpll0_vote", },
+	{ .fw_name = "pll", .name = "a53pll", },
 };
 
 /*
@@ -51,6 +51,19 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	struct clk_init_data init = { };
 	int ret = -ENODEV;
 
+	/*
+	 * This driver is defined by the devicetree binding
+	 * Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt,
+	 * however, this driver is registered as a platform device by
+	 * qcom-apcs-ipc-mailbox.c. Because of this, when this driver
+	 * uses dev_get_regmap() and devm_clk_get(), it has to send the parent
+	 * device as argument.
+	 * When registering with the clock framework, we cannot use this trick,
+	 * since the clock framework always looks at dev->of_node when it tries
+	 * to find parent clock names using clk_parent_data.
+	 */
+	dev->of_node = parent->of_node;
+
 	regmap = dev_get_regmap(parent, NULL);
 	if (!regmap) {
 		dev_err(dev, "failed to get regmap: %d\n", ret);
@@ -62,8 +75,8 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	init.name = "a53mux";
-	init.parent_names = gpll0_a53cc;
-	init.num_parents = ARRAY_SIZE(gpll0_a53cc);
+	init.parent_data = pdata;
+	init.num_parents = ARRAY_SIZE(pdata);
 	init.ops = &clk_regmap_mux_div_ops;
 	init.flags = CLK_SET_RATE_PARENT;
 
-- 
2.23.0

Re: [PATCH v3 0/7] Clock changes to support cpufreq on QCS404

[Niklas Cassel]

On Mon, Nov 25, 2019 at 02:59:02PM +0100, Niklas Cassel wrote:
> The following clock changes are required to enable cpufreq support on
> the QCS404.
> 
> Changes since v2:
> -Addressed Stephen Boyd's comment regarding apcs-msm8916
> should use new way of specifying clock parents.
> -DT binding now has "pll" as first clock, in order to
> not break DT backwards compatibility (in case no clock-names
> are given).
> -Moved EPROBE_DEFER error handling to its own patch.
> 
> Jorge Ramirez-Ortiz (6):
>   dt-bindings: mailbox: qcom: Add clock-name optional property
>   clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency
>   clk: qcom: hfpll: register as clock provider
>   clk: qcom: hfpll: CLK_IGNORE_UNUSED
>   clk: qcom: hfpll: use clk_parent_data to specify the parent
>   clk: qcom: apcs-msm8916: silently error out on EPROBE_DEFER
> 
> Niklas Cassel (1):
>   clk: qcom: apcs-msm8916: use clk_parent_data to specify the parent
> 
>  .../mailbox/qcom,apcs-kpss-global.txt         | 24 ++++++++++++++---
>  drivers/clk/qcom/apcs-msm8916.c               | 26 ++++++++++++++-----
>  drivers/clk/qcom/clk-alpha-pll.c              |  8 ++++++
>  drivers/clk/qcom/clk-alpha-pll.h              |  1 +
>  drivers/clk/qcom/gcc-qcs404.c                 |  2 +-
>  drivers/clk/qcom/hfpll.c                      | 21 +++++++++++++--
>  6 files changed, 70 insertions(+), 12 deletions(-)
> 
> -- 
> 2.23.0
> 

Hello Stephen,

I have adressed your review comments
on the previous patch series version.

Could you please have a look?

If it looks good, could you please
consider taking them via your tree?

Kind regards,
Niklas

Re: [PATCH v3 7/7] clk: qcom: apcs-msm8916: use clk_parent_data to specify the parent

[Stephen Boyd]

Quoting Niklas Cassel (2019-11-25 05:59:09)
> diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
> index 46061b3d230e..bb91644edc00 100644
> --- a/drivers/clk/qcom/apcs-msm8916.c
> +++ b/drivers/clk/qcom/apcs-msm8916.c
> @@ -51,6 +51,19 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
>         struct clk_init_data init = { };
>         int ret = -ENODEV;
>  
> +       /*
> +        * This driver is defined by the devicetree binding
> +        * Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt,
> +        * however, this driver is registered as a platform device by
> +        * qcom-apcs-ipc-mailbox.c. Because of this, when this driver
> +        * uses dev_get_regmap() and devm_clk_get(), it has to send the parent
> +        * device as argument.
> +        * When registering with the clock framework, we cannot use this trick,
> +        * since the clock framework always looks at dev->of_node when it tries
> +        * to find parent clock names using clk_parent_data.
> +        */
> +       dev->of_node = parent->of_node;

This is odd. The clks could be registered with of_clk_hw_register() but
then we lose the device provider information. Maybe we should search up
one level to the parent node and if that has a DT node but the
clk controller device doesn't we should use that instead?

----8<-----
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index b68e200829f2..c8745c415c04 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3669,7 +3669,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 	if (dev && pm_runtime_enabled(dev))
 		core->rpm_enabled = true;
 	core->dev = dev;
-	core->of_node = np;
+	core->of_node = np ? : dev_of_node(dev->parent);
 	if (dev && dev->driver)
 		core->owner = dev->driver->owner;
 	core->hw = hw;

Re: [PATCH v3 2/7] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency

[Stephen Boyd]

Quoting Niklas Cassel (2019-11-25 05:59:04)
> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> 
> 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>
> ---

Applied to clk-next

Re: [PATCH v3 3/7] clk: qcom: hfpll: register as clock provider

[Stephen Boyd]

Quoting Niklas Cassel (2019-11-25 05:59:05)
> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> 
> 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>
> ---

Applied to clk-next

Re: [PATCH v3 4/7] clk: qcom: hfpll: CLK_IGNORE_UNUSED

[Stephen Boyd]

Quoting Niklas Cassel (2019-11-25 05:59:06)
> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> 
> 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>
> ---

Applied to clk-next

Re: [PATCH v3 5/7] clk: qcom: hfpll: use clk_parent_data to specify the parent

[Stephen Boyd]

Quoting Niklas Cassel (2019-11-25 05:59:07)
> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> 
> This permits extending the driver to other platforms without having to
> modify its source code.
> 
> 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>
> ---

Applied to clk-next

Re: [PATCH v3 6/7] clk: qcom: apcs-msm8916: silently error out on EPROBE_DEFER

[Stephen Boyd]

Quoting Niklas Cassel (2019-11-25 05:59:08)
> From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> 
> If devm_clk_get() fails due to probe deferral, we shouldn't print an
> error message. Just be silent in this case.
> 
> 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>
> ---

Applied to clk-next

Re: [PATCH v3 7/7] clk: qcom: apcs-msm8916: use clk_parent_data to specify the parent

[Bjorn Andersson]

On Wed 18 Dec 22:23 PST 2019, Stephen Boyd wrote:

> Quoting Niklas Cassel (2019-11-25 05:59:09)
> > diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
> > index 46061b3d230e..bb91644edc00 100644
> > --- a/drivers/clk/qcom/apcs-msm8916.c
> > +++ b/drivers/clk/qcom/apcs-msm8916.c
> > @@ -51,6 +51,19 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
> >         struct clk_init_data init = { };
> >         int ret = -ENODEV;
> >  
> > +       /*
> > +        * This driver is defined by the devicetree binding
> > +        * Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt,
> > +        * however, this driver is registered as a platform device by
> > +        * qcom-apcs-ipc-mailbox.c. Because of this, when this driver
> > +        * uses dev_get_regmap() and devm_clk_get(), it has to send the parent
> > +        * device as argument.
> > +        * When registering with the clock framework, we cannot use this trick,
> > +        * since the clock framework always looks at dev->of_node when it tries
> > +        * to find parent clock names using clk_parent_data.
> > +        */
> > +       dev->of_node = parent->of_node;
> 
> This is odd. The clks could be registered with of_clk_hw_register() but
> then we lose the device provider information. Maybe we should search up
> one level to the parent node and if that has a DT node but the
> clk controller device doesn't we should use that instead?
> 

Yeah, we shouldn't have two struct device with the same of_node in the
system, and your suggestion looks quite reasonable. Do you mind spinning
a patch out of it and we can drop above chunk from Niklas' patch - and
afaict merge all the remaining patches to enable CPR on our first
target!

Thanks,
Bjorn

> ----8<-----
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b68e200829f2..c8745c415c04 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3669,7 +3669,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>  	if (dev && pm_runtime_enabled(dev))
>  		core->rpm_enabled = true;
>  	core->dev = dev;
> -	core->of_node = np;
> +	core->of_node = np ? : dev_of_node(dev->parent);
>  	if (dev && dev->driver)
>  		core->owner = dev->driver->owner;
>  	core->hw = hw;

Re: [PATCH v3 7/7] clk: qcom: apcs-msm8916: use clk_parent_data to specify the parent

[Niklas Cassel]

On Wed, Dec 18, 2019 at 10:23:39PM -0800, Stephen Boyd wrote:
> Quoting Niklas Cassel (2019-11-25 05:59:09)
> > diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
> > index 46061b3d230e..bb91644edc00 100644
> > --- a/drivers/clk/qcom/apcs-msm8916.c
> > +++ b/drivers/clk/qcom/apcs-msm8916.c
> > @@ -51,6 +51,19 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
> >         struct clk_init_data init = { };
> >         int ret = -ENODEV;
> >  
> > +       /*
> > +        * This driver is defined by the devicetree binding
> > +        * Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt,
> > +        * however, this driver is registered as a platform device by
> > +        * qcom-apcs-ipc-mailbox.c. Because of this, when this driver
> > +        * uses dev_get_regmap() and devm_clk_get(), it has to send the parent
> > +        * device as argument.
> > +        * When registering with the clock framework, we cannot use this trick,
> > +        * since the clock framework always looks at dev->of_node when it tries
> > +        * to find parent clock names using clk_parent_data.
> > +        */
> > +       dev->of_node = parent->of_node;
> 
> This is odd. The clks could be registered with of_clk_hw_register() but
> then we lose the device provider information. Maybe we should search up
> one level to the parent node and if that has a DT node but the
> clk controller device doesn't we should use that instead?

Hello Stephen,

Having this in the clk core is totally fine with me,
since it solves my problem.

Will you cook up a patch, or do you want me to do it?

Kind regards,
Niklas

> 
> ----8<-----
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b68e200829f2..c8745c415c04 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3669,7 +3669,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>  	if (dev && pm_runtime_enabled(dev))
>  		core->rpm_enabled = true;
>  	core->dev = dev;
> -	core->of_node = np;
> +	core->of_node = np ? : dev_of_node(dev->parent);
>  	if (dev && dev->driver)
>  		core->owner = dev->driver->owner;
>  	core->hw = hw;

Re: [PATCH v3 7/7] clk: qcom: apcs-msm8916: use clk_parent_data to specify the parent

[Stephen Boyd]

Quoting Niklas Cassel (2019-12-20 09:56:16)
> On Wed, Dec 18, 2019 at 10:23:39PM -0800, Stephen Boyd wrote:
> > This is odd. The clks could be registered with of_clk_hw_register() but
> > then we lose the device provider information. Maybe we should search up
> > one level to the parent node and if that has a DT node but the
> > clk controller device doesn't we should use that instead?
> 
> Hello Stephen,
> 
> Having this in the clk core is totally fine with me,
> since it solves my problem.
> 
> Will you cook up a patch, or do you want me to do it?
> 

Can you try the patch I appended to my previous mail? I can write
something up more proper later this week.

Re: [PATCH v3 7/7] clk: qcom: apcs-msm8916: use clk_parent_data to specify the parent

[Bjorn Andersson]

On Mon 23 Dec 18:16 PST 2019, Stephen Boyd wrote:

> Quoting Niklas Cassel (2019-12-20 09:56:16)
> > On Wed, Dec 18, 2019 at 10:23:39PM -0800, Stephen Boyd wrote:
> > > This is odd. The clks could be registered with of_clk_hw_register() but
> > > then we lose the device provider information. Maybe we should search up
> > > one level to the parent node and if that has a DT node but the
> > > clk controller device doesn't we should use that instead?
> > 
> > Hello Stephen,
> > 
> > Having this in the clk core is totally fine with me,
> > since it solves my problem.
> > 
> > Will you cook up a patch, or do you want me to do it?
> > 
> 
> Can you try the patch I appended to my previous mail? I can write
> something up more proper later this week.
> 

Unfortunately we have clocks with no dev, so this fail as below. Adding
a second check for dev != NULL to your oneliner works fine though.

I.e. this ugly hack works fine:
  core->of_node = np ? : (dev ? dev_of_node(dev->parent) : NULL);

Regards,
Bjorn

[    0.000000] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000040
[    0.000000] Mem abort info:
[    0.000000]   ESR = 0x96000004
[    0.000000]   EC = 0x25: DABT (current EL), IL = 32 bits
[    0.000000]   SET = 0, FnV = 0
[    0.000000]   EA = 0, S1PTW = 0
[    0.000000] Data abort info:
[    0.000000]   ISV = 0, ISS = 0x00000004
[    0.000000]   CM = 0, WnR = 0
[    0.000000] [0000000000000040] user address but active_mm is swapper
[    0.000000] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.5.0-rc2-next-20191220-00017-g359fd8f91acb-dirty #107
[    0.000000] Hardware name: Qualcomm Technologies, Inc. QCS404 EVB 4000 (DT)
[    0.000000] pstate: 80000085 (Nzcv daIf -PAN -UAO)
[    0.000000] pc : __clk_register (drivers/clk/clk.c:3679)
[    0.000000] lr : __clk_register (drivers/clk/clk.c:3664)
[    0.000000] sp : ffffdb6871043d70
[    0.000000] x29: ffffdb6871043d70 x28: ffff00003ddf4518
[    0.000000] x27: 0000000000000001 x26: 0000000000000008
[    0.000000] x25: 0000000000000000 x24: 0000000000000000
[    0.000000] x23: 0000000000000000 x22: 0000000000000000
[    0.000000] x21: ffff00003d821080 x20: ffffdb6871043e60
[    0.000000] x19: ffff00003d822000 x18: 0000000000000014
[    0.000000] x17: 000000006f7295ba x16: 0000000043d45a86
[    0.000000] x15: 000000005f0037cd x14: 00000000b22e3fc4
[    0.000000] x13: 0000000000000001 x12: 0000000000000000
[    0.000000] x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
[    0.000000] x9 : fefefefefefefeff x8 : 7f7f7f7f7f7f7f7f
[    0.000000] x7 : 6371606e612c6e77 x6 : ffff00003d821109
[    0.000000] x5 : 0000000000000000 x4 : ffff00003dd9d060
[    0.000000] x3 : 0000000000000000 x2 : 0000000000000009
[    0.000000] x1 : ffff00003ddf47b9 x0 : ffffdb68705b0ee0
[    0.000000] Call trace:
[    0.000000] __clk_register (drivers/clk/clk.c:3679)
[    0.000000] clk_hw_register (./include/linux/err.h:60 drivers/clk/clk.c:3760)
[    0.000000] clk_hw_register_fixed_rate_with_accuracy (drivers/clk/clk-fixed-rate.c:82)
[    0.000000] _of_fixed_clk_setup (drivers/clk/clk-fixed-rate.c:98 drivers/clk/clk-fixed-rate.c:173)
[    0.000000] of_fixed_clk_setup (drivers/clk/clk-fixed-rate.c:193)
[    0.000000] of_clk_init (drivers/clk/clk.c:4856)
[    0.000000] time_init (arch/arm64/kernel/time.c:59)
[    0.000000] start_kernel (init/main.c:697)

Re: [PATCH v3 7/7] clk: qcom: apcs-msm8916: use clk_parent_data to specify the parent

[Stephen Boyd]

Quoting Bjorn Andersson (2019-12-26 18:26:52)
> On Mon 23 Dec 18:16 PST 2019, Stephen Boyd wrote:
> 
> > Quoting Niklas Cassel (2019-12-20 09:56:16)
> > > On Wed, Dec 18, 2019 at 10:23:39PM -0800, Stephen Boyd wrote:
> > > > This is odd. The clks could be registered with of_clk_hw_register() but
> > > > then we lose the device provider information. Maybe we should search up
> > > > one level to the parent node and if that has a DT node but the
> > > > clk controller device doesn't we should use that instead?
> > > 
> > > Hello Stephen,
> > > 
> > > Having this in the clk core is totally fine with me,
> > > since it solves my problem.
> > > 
> > > Will you cook up a patch, or do you want me to do it?
> > > 
> > 
> > Can you try the patch I appended to my previous mail? I can write
> > something up more proper later this week.
> > 
> 
> Unfortunately we have clocks with no dev, so this fail as below. Adding
> a second check for dev != NULL to your oneliner works fine though.
> 
> I.e. this ugly hack works fine:
>   core->of_node = np ? : (dev ? dev_of_node(dev->parent) : NULL);
> 

Ok, thanks for testing!

Re: [PATCH v3 7/7] clk: qcom: apcs-msm8916: use clk_parent_data to specify the parent

[Bjorn Andersson]

On Mon 25 Nov 05:59 PST 2019, Niklas Cassel wrote:

> Allow accessing the parent clock names required for the driver
> operation by using the device tree node, while falling back to
> the previous method of using names in the global name space.
> 
> This permits extending the driver to other platforms without having to
> modify its source code.
> 
> Co-developed-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> ---
> Changes since v2:
> -Use clk_parent_data when specifying clock parents.
> 
>  drivers/clk/qcom/apcs-msm8916.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
> index 46061b3d230e..bb91644edc00 100644
> --- a/drivers/clk/qcom/apcs-msm8916.c
> +++ b/drivers/clk/qcom/apcs-msm8916.c
> @@ -19,9 +19,9 @@
>  
>  static const u32 gpll0_a53cc_map[] = { 4, 5 };
>  
> -static const char * const gpll0_a53cc[] = {
> -	"gpll0_vote",
> -	"a53pll",
> +static const struct clk_parent_data pdata[] = {
> +	{ .fw_name = "aux", .name = "gpll0_vote", },
> +	{ .fw_name = "pll", .name = "a53pll", },
>  };
>  
>  /*
> @@ -51,6 +51,19 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
>  	struct clk_init_data init = { };
>  	int ret = -ENODEV;
>  
> +	/*
> +	 * This driver is defined by the devicetree binding
> +	 * Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt,
> +	 * however, this driver is registered as a platform device by
> +	 * qcom-apcs-ipc-mailbox.c. Because of this, when this driver
> +	 * uses dev_get_regmap() and devm_clk_get(), it has to send the parent
> +	 * device as argument.
> +	 * When registering with the clock framework, we cannot use this trick,
> +	 * since the clock framework always looks at dev->of_node when it tries
> +	 * to find parent clock names using clk_parent_data.
> +	 */
> +	dev->of_node = parent->of_node;
> +

With this hunk replaced by Stephen's patch for handling this in the
clock core I did some basic tests and things seems to work as expected.

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

Regards,
Bjorn

>  	regmap = dev_get_regmap(parent, NULL);
>  	if (!regmap) {
>  		dev_err(dev, "failed to get regmap: %d\n", ret);
> @@ -62,8 +75,8 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	init.name = "a53mux";
> -	init.parent_names = gpll0_a53cc;
> -	init.num_parents = ARRAY_SIZE(gpll0_a53cc);
> +	init.parent_data = pdata;
> +	init.num_parents = ARRAY_SIZE(pdata);
>  	init.ops = &clk_regmap_mux_div_ops;
>  	init.flags = CLK_SET_RATE_PARENT;
>  
> -- 
> 2.23.0
>