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
From: Jorge Ramirez-Ortiz <[email protected]>
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 <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Acked-by: Stephen Boyd <[email protected]>
---
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
From: Jorge Ramirez-Ortiz <[email protected]>
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 <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
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
From: Jorge Ramirez-Ortiz <[email protected]>
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 <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
---
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
From: Jorge Ramirez-Ortiz <[email protected]>
This permits extending the driver to other platforms without having to
modify its source code.
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
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
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;
Quoting Niklas Cassel (2019-11-25 05:59:05)
> From: Jorge Ramirez-Ortiz <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> Reviewed-by: Bjorn Andersson <[email protected]>
> Acked-by: Stephen Boyd <[email protected]>
> ---
Applied to clk-next
Quoting Niklas Cassel (2019-11-25 05:59:06)
> From: Jorge Ramirez-Ortiz <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> Reviewed-by: Bjorn Andersson <[email protected]>
> ---
Applied to clk-next
Quoting Niklas Cassel (2019-11-25 05:59:07)
> From: Jorge Ramirez-Ortiz <[email protected]>
>
> This permits extending the driver to other platforms without having to
> modify its source code.
>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
Applied to clk-next
Quoting Niklas Cassel (2019-11-25 05:59:08)
> From: Jorge Ramirez-Ortiz <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
Applied to clk-next
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;
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;
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.
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)
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!
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 <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> 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 <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
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
>