diff mbox series

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

Message ID 20191125135910.679310-8-niklas.cassel@linaro.org (mailing list archive)
State New, archived
Headers show
Series Clock changes to support cpufreq on QCS404 | expand

Commit Message

Niklas Cassel Nov. 25, 2019, 1:59 p.m. UTC 
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(-)

Comments

Stephen Boyd Dec. 19, 2019, 6:23 a.m. UTC | #1 
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;
Bjorn Andersson Dec. 20, 2019, 5:22 p.m. UTC | #2 
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;
Niklas Cassel Dec. 20, 2019, 5:56 p.m. UTC | #3 
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;
Stephen Boyd Dec. 24, 2019, 2:16 a.m. UTC | #4 
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.
Bjorn Andersson Dec. 27, 2019, 2:26 a.m. UTC | #5 
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)
Stephen Boyd Dec. 30, 2019, 6:04 p.m. UTC | #6 
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!
Bjorn Andersson Jan. 3, 2020, 7:47 a.m. UTC | #7 
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
>
diff mbox series

Patch

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;