The following patchset enables CPU frequency scaling support on the
QCS404.
Patch 8 "clk: qcom: hfpll: CLK_IGNORE_UNUSED" is a bit controversial;
in this platform, this PLL provides the clock signal to a CPU
core. But in others it might not.
I opted for the minimal ammount of changes without affecting the
default functionality: simply bypassing the COMMON_CLK_DISABLE_UNUSED
framework and letting the firwmare chose whether to enable or disable
the clock at boot. However maybe a DT property and marking the clock
as critical would be more appropriate for this PLL. I'd appreciate the
maintainer's input on this topic.
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
Jorge Ramirez-Ortiz (13):
clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency
mbox: qcom: add APCS child device for QCS404
mbox: qcom: replace integer with valid macro
dt-bindings: mailbox: qcom: Add clock-name optional property
clk: qcom: apcs-msm8916: get parent clock names from DT
clk: qcom: hfpll: get parent clock names from DT
clk: qcom: hfpll: register as clock provider
clk: qcom: hfpll: CLK_IGNORE_UNUSED
arm64: dts: qcom: qcs404: Add OPP table
arm64: dts: qcom: qcs404: Add HFPLL node
arm64: dts: qcom: qcs404: Add the clocks for APCS mux/divider
arm64: dts: qcom: qcs404: Add cpufreq support
arm64: defconfig: Enable HFPLL
.../bindings/mailbox/qcom,apcs-kpss-global.txt | 21 +++++++++++++
arch/arm64/boot/dts/qcom/qcs404.dtsi | 35 ++++++++++++++++++++++
arch/arm64/configs/defconfig | 1 +
drivers/clk/qcom/apcs-msm8916.c | 33 ++++++++++++++------
drivers/clk/qcom/gcc-qcs404.c | 6 ++++
drivers/clk/qcom/hfpll.c | 19 +++++++++++-
drivers/mailbox/qcom-apcs-ipc-mailbox.c | 21 ++++++++-----
7 files changed, 118 insertions(+), 18 deletions(-)
--
2.7.4
There is clock controller functionality in the APCS hardware block of
qcs404 devices similar to msm8916.
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
drivers/mailbox/qcom-apcs-ipc-mailbox.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index aed23ac..dc8fdab1 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -97,16 +97,21 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
return ret;
}
- if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global")) {
- apcs->clk = platform_device_register_data(&pdev->dev,
- "qcom-apcs-msm8916-clk",
- -1, NULL, 0);
- if (IS_ERR(apcs->clk))
- dev_err(&pdev->dev, "failed to register APCS clk\n");
- }
-
platform_set_drvdata(pdev, apcs);
+ if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global") ||
+ of_device_is_compatible(np, "qcom,qcs404-apcs-apps-global"))
+ goto register_clk;
+
+ return 0;
+
+register_clk:
+ apcs->clk = platform_device_register_data(&pdev->dev,
+ "qcom-apcs-msm8916-clk",
+ -1, NULL, 0);
+ if (IS_ERR(apcs->clk))
+ dev_err(&pdev->dev, "failed to register APCS clk\n");
+
return 0;
}
--
2.7.4
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]>
---
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 87b7f46..0ffed0d 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.7.4
Add a CPU OPP table to qcs404
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index 9b5c165..4594fea7 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -62,6 +62,21 @@
};
};
+ cpu_opp_table: cpu_opp_table {
+ compatible = "operating-points-v2";
+ opp-shared;
+
+ opp-1094400000 {
+ opp-hz = /bits/ 64 <1094400000>;
+ };
+ opp-1248000000 {
+ opp-hz = /bits/ 64 <1248000000>;
+ };
+ opp-1401600000 {
+ opp-hz = /bits/ 64 <1401600000>;
+ };
+ };
+
firmware {
scm: scm {
compatible = "qcom,scm-qcs404", "qcom,scm";
--
2.7.4
The high frequency pll functionality is required to enable CPU
frequency scaling operation.
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
arch/arm64/boot/dts/qcom/qcs404.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index 4594fea7..ec3f6c7 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -375,6 +375,15 @@
#mbox-cells = <1>;
};
+ apcs_hfpll: clock-controller@0b016000 {
+ compatible = "qcom,hfpll";
+ reg = <0x0b016000 0x30>;
+ #clock-cells = <0>;
+ clock-output-names = "apcs_hfpll";
+ clocks = <&xo_board>;
+ clock-names = "xo";
+ };
+
timer@b120000 {
#address-cells = <1>;
#size-cells = <1>;
--
2.7.4
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 <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
drivers/clk/qcom/apcs-msm8916.c | 33 ++++++++++++++++++++++++---------
1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
index a6c89a3..2453242 100644
--- a/drivers/clk/qcom/apcs-msm8916.c
+++ b/drivers/clk/qcom/apcs-msm8916.c
@@ -19,7 +19,7 @@
static const u32 gpll0_a53cc_map[] = { 4, 5 };
-static const char * const gpll0_a53cc[] = {
+static const char *gpll0_a53cc[] = {
"gpll0_vote",
"a53pll",
};
@@ -50,17 +50,39 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
struct regmap *regmap;
struct clk_init_data init = { };
int ret = -ENODEV;
+ struct clk_bulk_data pclks[] = {
+ [0] = { .id = "aux", .clk = NULL },
+ [1] = { .id = "pll", .clk = NULL },
+ };
regmap = dev_get_regmap(parent, NULL);
if (!regmap) {
dev_err(dev, "failed to get regmap: %d\n", ret);
return ret;
}
-
a53cc = devm_kzalloc(dev, sizeof(*a53cc), GFP_KERNEL);
if (!a53cc)
return -ENOMEM;
+ /* check if the parent names are present in the device tree */
+ ret = devm_clk_bulk_get(parent, ARRAY_SIZE(pclks), pclks);
+ if (ret == -EPROBE_DEFER)
+ return ret;
+
+ if (!ret) {
+ gpll0_a53cc[0] = __clk_get_name(pclks[0].clk);
+ gpll0_a53cc[1] = __clk_get_name(pclks[1].clk);
+ a53cc->pclk = pclks[1].clk;
+ } else {
+ /* support old binding where only pll was explicitily defined */
+ a53cc->pclk = devm_clk_get(parent, NULL);
+ if (IS_ERR(a53cc->pclk)) {
+ ret = PTR_ERR(a53cc->pclk);
+ dev_err(dev, "failed to get clk: %d\n", ret);
+ return ret;
+ }
+ }
+
init.name = "a53mux";
init.parent_names = gpll0_a53cc;
init.num_parents = ARRAY_SIZE(gpll0_a53cc);
@@ -76,13 +98,6 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
a53cc->src_shift = 8;
a53cc->parent_map = gpll0_a53cc_map;
- a53cc->pclk = devm_clk_get(parent, NULL);
- if (IS_ERR(a53cc->pclk)) {
- ret = PTR_ERR(a53cc->pclk);
- dev_err(dev, "failed to get clk: %d\n", ret);
- return ret;
- }
-
a53cc->clk_nb.notifier_call = a53cc_notifier_cb;
ret = clk_notifier_register(a53cc->pclk, &a53cc->clk_nb);
if (ret) {
--
2.7.4
When the APCS clock is registered (platform dependent), it retrieves
its parent names from hardcoded values in the driver.
The following commit allows the DT node to provide such clock names to
the platform data based clock driver therefore avoiding having to
explicitly embed those names in the clock driver source code.
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
.../bindings/mailbox/qcom,apcs-kpss-global.txt | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
index 1232fc9..f252439 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
@@ -23,6 +23,10 @@ platforms.
Value type: <phandle>
Definition: phandle to the input PLL, which feeds the APCS mux/divider
+ Usage: required if #clock-names property is present
+ Value type: <phandle array>
+ Definition: phandles to the two parent clocks of the clock driver.
+
- #mbox-cells:
Usage: required
Value type: <u32>
@@ -33,6 +37,12 @@ platforms.
Value type: <u32>
Definition: as described in clock.txt, must be 0
+- clock-names:
+ Usage: required if the platform data based clock driver needs to
+ retrieve the parent clock names from device tree.
+ This will requires two mandatory clocks to be defined.
+ Value type: <string-array>
+ Definition: must be "aux" and "pll"
= EXAMPLE
The following example describes the APCS HMSS found in MSM8996 and part of the
@@ -65,3 +75,14 @@ Below is another example of the APCS binding on MSM8916 platforms:
clocks = <&a53pll>;
#clock-cells = <0>;
};
+
+Below is another example of the APCS binding on QCS404 platforms:
+
+ apcs_glb: mailbox@b011000 {
+ compatible = "qcom,qcs404-apcs-apps-global", "syscon";
+ reg = <0x0b011000 0x1000>;
+ #mbox-cells = <1>;
+ clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
+ clock-names = "aux", "pll";
+ #clock-cells = <0>;
+ };
--
2.7.4
The high frequency pll is required on compatible Qualcomm SoCs to
support the CPU frequency scaling feature.
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 5c2b1f6..da390aa 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -615,6 +615,7 @@ CONFIG_MSM_MMCC_8996=y
CONFIG_MSM_GCC_8998=y
CONFIG_QCS_GCC_404=y
CONFIG_SDM_GCC_845=y
+CONFIG_QCOM_HFPLL=y
CONFIG_HWSPINLOCK=y
CONFIG_HWSPINLOCK_QCOM=y
CONFIG_ARM_MHU=y
--
2.7.4
Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
specifications.
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
index 64da032..833436a 100644
--- a/drivers/clk/qcom/gcc-qcs404.c
+++ b/drivers/clk/qcom/gcc-qcs404.c
@@ -304,10 +304,16 @@ static struct clk_alpha_pll gpll0_out_main = {
},
};
+static const struct pll_vco gpll0_ao_out_vco[] = {
+ { 800000000, 800000000, 0 },
+};
+
static struct clk_alpha_pll gpll0_ao_out_main = {
.offset = 0x21000,
.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
.flags = SUPPORTS_FSM_MODE,
+ .vco_table = gpll0_ao_out_vco,
+ .num_vco = ARRAY_SIZE(gpll0_ao_out_vco),
.clkr = {
.enable_reg = 0x45000,
.enable_mask = BIT(0),
--
2.7.4
Use the correct macro when registering the platform device.
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
drivers/mailbox/qcom-apcs-ipc-mailbox.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index dc8fdab1..b782173 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -108,7 +108,7 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
register_clk:
apcs->clk = platform_device_register_data(&pdev->dev,
"qcom-apcs-msm8916-clk",
- -1, NULL, 0);
+ PLATFORM_DEVID_NONE, NULL, 0);
if (IS_ERR(apcs->clk))
dev_err(&pdev->dev, "failed to register APCS clk\n");
--
2.7.4
Specify the clocks that feed the APCS mux/divider instead of using
default hardcoded values in the source code.
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
arch/arm64/boot/dts/qcom/qcs404.dtsi | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index ec3f6c7..2d9e70e 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -373,6 +373,9 @@
compatible = "qcom,qcs404-apcs-apps-global", "syscon";
reg = <0x0b011000 0x1000>;
#mbox-cells = <1>;
+ clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
+ clock-names = "aux", "pll";
+ #clock-cells = <0>;
};
apcs_hfpll: clock-controller@0b016000 {
--
2.7.4
Support CPU frequency scaling on qcs404.
Co-developed-by: Niklas Cassel <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
arch/arm64/boot/dts/qcom/qcs404.dtsi | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index 2d9e70e..5a14887 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -30,6 +30,8 @@
reg = <0x100>;
enable-method = "psci";
next-level-cache = <&L2_0>;
+ clocks = <&apcs_glb>;
+ operating-points-v2 = <&cpu_opp_table>;
};
CPU1: cpu@101 {
@@ -38,6 +40,8 @@
reg = <0x101>;
enable-method = "psci";
next-level-cache = <&L2_0>;
+ clocks = <&apcs_glb>;
+ operating-points-v2 = <&cpu_opp_table>;
};
CPU2: cpu@102 {
@@ -46,6 +50,8 @@
reg = <0x102>;
enable-method = "psci";
next-level-cache = <&L2_0>;
+ clocks = <&apcs_glb>;
+ operating-points-v2 = <&cpu_opp_table>;
};
CPU3: cpu@103 {
@@ -54,6 +60,8 @@
reg = <0x103>;
enable-method = "psci";
next-level-cache = <&L2_0>;
+ clocks = <&apcs_glb>;
+ operating-points-v2 = <&cpu_opp_table>;
};
L2_0: l2-cache {
--
2.7.4
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]>
---
drivers/clk/qcom/hfpll.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c
index 0ffed0d..9d92f5d 100644
--- a/drivers/clk/qcom/hfpll.c
+++ b/drivers/clk/qcom/hfpll.c
@@ -58,6 +58,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
.parent_names = (const char *[]){ "xo" },
.num_parents = 1,
.ops = &clk_ops_hfpll,
+ .flags = CLK_IGNORE_UNUSED,
};
h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL);
--
2.7.4
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 <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>
Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
---
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 a6de7101..87b7f46 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.7.4
Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:27)
> The high frequency pll functionality is required to enable CPU
> frequency scaling operation.
>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/qcs404.dtsi | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> index 4594fea7..ec3f6c7 100644
> --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> @@ -375,6 +375,15 @@
> #mbox-cells = <1>;
> };
>
> + apcs_hfpll: clock-controller@0b016000 {
Drop leading 0 on unit address please.
> + compatible = "qcom,hfpll";
> + reg = <0x0b016000 0x30>;
Wow that is small!
> + #clock-cells = <0>;
> + clock-output-names = "apcs_hfpll";
> + clocks = <&xo_board>;
> + clock-names = "xo";
> + };
> +
Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:22)
> Allow accessing the parent clock names required for the driver
> operation by using the device tree node.
>
> This permits extending the driver to other platforms without having to
> modify its source code.
>
> For backwards compatibility leave previous values as default.
Why do we need to maintain backwards compatibility? Isn't is required
that the nodes have clocks properties?
On 12/18/18 00:37, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:22)
>> Allow accessing the parent clock names required for the driver
>> operation by using the device tree node.
>>
>> This permits extending the driver to other platforms without having to
>> modify its source code.
>>
>> For backwards compatibility leave previous values as default.
> Why do we need to maintain backwards compatibility? Isn't is required
> that the nodes have clocks properties?
>
>
this driver -apcs clock controller- uses platform data (not DT) and
therefore it uses the DT from the parent node (mailbox).
And for the mailbox the clock property is optional.
So the APCS clock controller requires that the parent provides at least
one clock but the clock is not mandatory in the parent DT node.
For instance in the case of the msm8916, the parent only provides one
clock, just the pll.
am I required to modify that platform instead of maintaining backwards
compatibility?
On Mon, Dec 17, 2018 at 03:37:43PM -0800, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:22)
> > Allow accessing the parent clock names required for the driver
> > operation by using the device tree node.
> >
> > This permits extending the driver to other platforms without having to
> > modify its source code.
> >
> > For backwards compatibility leave previous values as default.
>
> Why do we need to maintain backwards compatibility? Isn't is required
> that the nodes have clocks properties?
>
Hello Stephen,
This is the existing DT nodes for msm8916:
a53pll: clock@b016000 {
compatible = "qcom,msm8916-a53pll";
reg = <0xb016000 0x40>;
#clock-cells = <0>;
};
apcs: mailbox@b011000 {
compatible = "qcom,msm8916-apcs-kpss-global", "syscon";
reg = <0xb011000 0x1000>;
#mbox-cells = <1>;
clocks = <&a53pll>;
#clock-cells = <0>;
};
This is the (suggested) DT nodes for qcs404:
apcs_hfpll: clock-controller@0b016000 {
compatible = "qcom,hfpll";
reg = <0x0b016000 0x30>;
#clock-cells = <0>;
clock-output-names = "apcs_hfpll";
clocks = <&xo_board>;
clock-names = "xo";
};
apcs_glb: mailbox@b011000 {
compatible = "qcom,qcs404-apcs-apps-global", "syscon";
reg = <0x0b011000 0x1000>;
#mbox-cells = <1>;
clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
clock-names = "aux", "pll";
#clock-cells = <0>;
};
qcs404 specifies two clocks, with an accompanied clock-name for each clock.
msm8916 specifies a single clock, without an accompanied clock-name.
It is possible to append clock-names = "pll" for the existing clock,
as well as to define the aux clock in the apcs node in the msm8916 DT:
clocks = <&gcc GPLL0_VOTE>;
clock-names = "aux";
However, since the DT is treated as an ABI, the existing DT for msm8916 must
still work, so I don't think that it is possible to ignore having backwards
compability in the apcs clock driver.
Kind regards,
Niklas
On Mon, 17 Dec 2018 10:46:21 +0100, Jorge Ramirez-Ortiz wrote:
> When the APCS clock is registered (platform dependent), it retrieves
> its parent names from hardcoded values in the driver.
>
> The following commit allows the DT node to provide such clock names to
> the platform data based clock driver therefore avoiding having to
> explicitly embed those names in the clock driver source code.
>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> .../bindings/mailbox/qcom,apcs-kpss-global.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
On 12/17/2018 3:16 PM, Jorge Ramirez-Ortiz wrote:
> Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
> specifications.
>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
> index 64da032..833436a 100644
> --- a/drivers/clk/qcom/gcc-qcs404.c
> +++ b/drivers/clk/qcom/gcc-qcs404.c
> @@ -304,10 +304,16 @@ static struct clk_alpha_pll gpll0_out_main = {
> },
> };
>
> +static const struct pll_vco gpll0_ao_out_vco[] = {
> + { 800000000, 800000000, 0 },
> +};
> +
> static struct clk_alpha_pll gpll0_ao_out_main = {
> .offset = 0x21000,
> .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
> .flags = SUPPORTS_FSM_MODE,
> + .vco_table = gpll0_ao_out_vco,
> + .num_vco = ARRAY_SIZE(gpll0_ao_out_vco),
Could you please help as to why this is required? This is a fixed PLL
and we do not require a VCO table for it.
> .clkr = {
> .enable_reg = 0x45000,
> .enable_mask = BIT(0),
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--
On 12/21/18 12:19, Taniya Das wrote:
>
>
> On 12/17/2018 3:16 PM, Jorge Ramirez-Ortiz wrote:
>> Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
>> specifications.
>>
>> Co-developed-by: Niklas Cassel <[email protected]>
>> Signed-off-by: Niklas Cassel <[email protected]>
>> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
>> ---
>> Â drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
>> Â 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/gcc-qcs404.c
>> b/drivers/clk/qcom/gcc-qcs404.c
>> index 64da032..833436a 100644
>> --- a/drivers/clk/qcom/gcc-qcs404.c
>> +++ b/drivers/clk/qcom/gcc-qcs404.c
>> @@ -304,10 +304,16 @@ static struct clk_alpha_pll gpll0_out_main = {
>> Â Â Â Â Â },
>> Â };
>> Â +static const struct pll_vco gpll0_ao_out_vco[] = {
>> +Â Â Â { 800000000, 800000000, 0 },
>> +};
>> +
>> Â static struct clk_alpha_pll gpll0_ao_out_main = {
>> Â Â Â Â Â .offset = 0x21000,
>> Â Â Â Â Â .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
>> Â Â Â Â Â .flags = SUPPORTS_FSM_MODE,
>> +Â Â Â .vco_table = gpll0_ao_out_vco,
>> +Â Â Â .num_vco = ARRAY_SIZE(gpll0_ao_out_vco),
>
> Could you please help as to why this is required? This is a fixed PLL
> and we do not require a VCO table for it.
Hi Taniya,
this patch - the additional information that it provides about the
hardware - helps to select the right parent clock for a given frequency.
On the qcs404 this clock is one of the two parent clocks of the apcs
clock controller (the other one being the high frequency pll)
When cpufreq sets a target frequency, there is an iteration through the
list of parents to select the one that delivers the best match.
When attempting to set the clock for an alpha_pll, the operation does a
sanity check to validate that the requested frequency is in fact
reachable using the vco range: trying to set a value that is not in
range will fail.
This patch makes sure that its range is explicitly defined.
It also helps making sure there are no rounding issues when setting its
value: without it the clock was being read at 799MHz
>
>> Â Â Â Â Â .clkr = {
>> Â Â Â Â Â Â Â Â Â .enable_reg = 0x45000,
>> Â Â Â Â Â Â Â Â Â .enable_mask = BIT(0),
>>
>
On 12/21/18 20:28, Bjorn Andersson wrote:
> On Fri 21 Dec 09:58 PST 2018, Taniya Das wrote:
>
>> Hello,
>>
>> On 12/21/2018 6:06 PM, Jorge Ramirez wrote:
>>> On 12/21/18 12:19, Taniya Das wrote:
>>>>
>>>> On 12/17/2018 3:16 PM, Jorge Ramirez-Ortiz wrote:
>>>>> Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
>>>>> specifications.
>>>>>
>>>>> Co-developed-by: Niklas Cassel <[email protected]>
>>>>> Signed-off-by: Niklas Cassel <[email protected]>
>>>>> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
>>>>> ---
>>>>> Â drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
>>>>> Â 1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/qcom/gcc-qcs404.c
>>>>> b/drivers/clk/qcom/gcc-qcs404.c
>>>>> index 64da032..833436a 100644
>>>>> --- a/drivers/clk/qcom/gcc-qcs404.c
>>>>> +++ b/drivers/clk/qcom/gcc-qcs404.c
>>>>> @@ -304,10 +304,16 @@ static struct clk_alpha_pll gpll0_out_main = {
>>>>> Â Â Â Â Â },
>>>>> Â };
>>>>> Â +static const struct pll_vco gpll0_ao_out_vco[] = {
>>>>> +Â Â Â { 800000000, 800000000, 0 },
>>>>> +};
>>>>> +
>>>>> Â static struct clk_alpha_pll gpll0_ao_out_main = {
>>>>> Â Â Â Â Â .offset = 0x21000,
>>>>> Â Â Â Â Â .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
>>>>> Â Â Â Â Â .flags = SUPPORTS_FSM_MODE,
>>>>> +Â Â Â .vco_table = gpll0_ao_out_vco,
>>>>> +Â Â Â .num_vco = ARRAY_SIZE(gpll0_ao_out_vco),
>>>> Could you please help as to why this is required? This is a fixed
>>>> PLL and we do not require a VCO table for it.
>>> Hi Taniya,
>>>
>>> this patch - the additional information that it provides about the
>>> hardware - helps to select the right parent clock for a given frequency.
>>>
>>> On the qcs404 this clock is one of the two parent clocks of the apcs
>>> clock controller (the other one being the high frequency pll)
>>> When cpufreq sets a target frequency, there is an iteration through the
>>> list of parents to select the one that delivers the best match.
>>>
>>> When attempting to set the clock for an alpha_pll, the operation does a
>>> sanity check to validate that the requested frequency is in fact
>>> reachable using the vco range: trying to set a value that is not in
>>> range will fail.
>>>
>>> This patch makes sure that its range is explicitly defined.
>>>
>>> It also helps making sure there are no rounding issues when setting its
>>> value: without it the clock was being read at 799MHz
>>>
>>>
>> If the PLL is being read as 799MHz it would because not all the 40 bits of
>> the ALPHA_VAL being programmed by the bootloaders(which are the original
>> owners of this PLL). So we should go with the way they are being set by
>> bootloaders and read by HLOS driver.
>>
>> And a VCO range you have considered is wrong from a PLL perspective. As
>> these are fixed PLLs and VCO range really does not matter here, so please
>> drop this change.
>>
> The problem here is that the PLL should be fixed at 800MHz, but the
> alpha PLL is defined such that it can change. So when the mux-div is
> looking for a suitable parent and divider for our CPU clock it concludes
> that the best way to reach certain frequencies is to change the rate of
> GPLL0.
>
> Adding the vco_table limits the available frequencies for GPLL0 in
> QCS404, without modifying the implementation of the alpha PLL.
>
> Perhaps there's a better way to define that this particular clock
> hardware can change rate, but in this implementation it must not?
the initialization for this particular PLL on this particular platform
is wrong
as the interface does not apply to the platform needs even though it is an
alpha_pll
if the VCO is not an option -even though it reflects the platform
constrains-
I would suggest nullifying the alpha_pll_ops that do not apply to this
platform:
ie: set_rate, round_rate set to null in the probe.
allowing the interface calls (ops) to go through to later on make them fail
based on some setting would be fundamentally wrong IMO
>
> Regards,
> Bjorn
>
On 12/21/18 22:40, Stephen Boyd wrote:
> Quoting Jorge Ramirez (2018-12-21 11:45:28)
>> On 12/21/18 20:28, Bjorn Andersson wrote:
>>> Perhaps there's a better way to define that this particular clock
>>> hardware can change rate, but in this implementation it must not?
>> the initialization for this particular PLL on this particular platform
>> is wrong
>> as the interface does not apply to the platform needs even though it is an
>> alpha_pll
>>
>> if the VCO is not an option -even though it reflects the platform
>> constrains-
>> I would suggest nullifying the alpha_pll_ops that do not apply to this
>> platform:
>> ie: set_rate, round_rate set to null in the probe.
>>
>> allowing the interface calls (ops) to go through to later on make them fail
>> based on some setting would be fundamentally wrong IMO
>>
> We have clk_alpha_pll_postdiv_ro_ops so maybe just add another set of
> those for the alpha_pll itself?
>
>
something like 
clk_alpha_pll_fixed_ops?
Quoting Jorge Ramirez (2018-12-21 13:45:41)
> On 12/21/18 22:40, Stephen Boyd wrote:
> > Quoting Jorge Ramirez (2018-12-21 11:45:28)
> >> On 12/21/18 20:28, Bjorn Andersson wrote:
> >>> Perhaps there's a better way to define that this particular clock
> >>> hardware can change rate, but in this implementation it must not?
> >> the initialization for this particular PLL on this particular platform
> >> is wrong
> >> as the interface does not apply to the platform needs even though it is an
> >> alpha_pll
> >>
> >> if the VCO is not an option -even though it reflects the platform
> >> constrains-
> >> I would suggest nullifying the alpha_pll_ops that do not apply to this
> >> platform:
> >> ie: set_rate, round_rate set to null in the probe.
> >>
> >> allowing the interface calls (ops) to go through to later on make them fail
> >> based on some setting would be fundamentally wrong IMO
> >>
> > We have clk_alpha_pll_postdiv_ro_ops so maybe just add another set of
> > those for the alpha_pll itself?
> >
> >
>
> something like
> clk_alpha_pll_fixed_ops?
Either way works. We're not consistent in naming now that we have
clk_alpha_pll_fixed_fabia_ops.
Hello,
On 12/21/2018 6:06 PM, Jorge Ramirez wrote:
> On 12/21/18 12:19, Taniya Das wrote:
>>
>>
>> On 12/17/2018 3:16 PM, Jorge Ramirez-Ortiz wrote:
>>> Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
>>> specifications.
>>>
>>> Co-developed-by: Niklas Cassel <[email protected]>
>>> Signed-off-by: Niklas Cassel <[email protected]>
>>> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
>>> ---
>>> Â drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
>>> Â 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/gcc-qcs404.c
>>> b/drivers/clk/qcom/gcc-qcs404.c
>>> index 64da032..833436a 100644
>>> --- a/drivers/clk/qcom/gcc-qcs404.c
>>> +++ b/drivers/clk/qcom/gcc-qcs404.c
>>> @@ -304,10 +304,16 @@ static struct clk_alpha_pll gpll0_out_main = {
>>> Â Â Â Â Â },
>>> Â };
>>> Â +static const struct pll_vco gpll0_ao_out_vco[] = {
>>> +Â Â Â { 800000000, 800000000, 0 },
>>> +};
>>> +
>>> Â static struct clk_alpha_pll gpll0_ao_out_main = {
>>> Â Â Â Â Â .offset = 0x21000,
>>> Â Â Â Â Â .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
>>> Â Â Â Â Â .flags = SUPPORTS_FSM_MODE,
>>> +Â Â Â .vco_table = gpll0_ao_out_vco,
>>> +Â Â Â .num_vco = ARRAY_SIZE(gpll0_ao_out_vco),
>>
>> Could you please help as to why this is required? This is a fixed PLL
>> and we do not require a VCO table for it.
>
> Hi Taniya,
>
> this patch - the additional information that it provides about the
> hardware - helps to select the right parent clock for a given frequency.
>
> On the qcs404 this clock is one of the two parent clocks of the apcs
> clock controller (the other one being the high frequency pll)
> When cpufreq sets a target frequency, there is an iteration through the
> list of parents to select the one that delivers the best match.
>
> When attempting to set the clock for an alpha_pll, the operation does a
> sanity check to validate that the requested frequency is in fact
> reachable using the vco range: trying to set a value that is not in
> range will fail.
>
> This patch makes sure that its range is explicitly defined.
>
> It also helps making sure there are no rounding issues when setting its
> value: without it the clock was being read at 799MHz
>
>
If the PLL is being read as 799MHz it would because not all the 40 bits
of the ALPHA_VAL being programmed by the bootloaders(which are the
original owners of this PLL). So we should go with the way they are
being set by bootloaders and read by HLOS driver.
And a VCO range you have considered is wrong from a PLL perspective. As
these are fixed PLLs and VCO range really does not matter here, so
please drop this change.
>>
>>> Â Â Â Â Â .clkr = {
>>> Â Â Â Â Â Â Â Â Â .enable_reg = 0x45000,
>>> Â Â Â Â Â Â Â Â Â .enable_mask = BIT(0),
>>>
>>
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--
On Fri 21 Dec 09:58 PST 2018, Taniya Das wrote:
> Hello,
>
> On 12/21/2018 6:06 PM, Jorge Ramirez wrote:
> > On 12/21/18 12:19, Taniya Das wrote:
> > >
> > >
> > > On 12/17/2018 3:16 PM, Jorge Ramirez-Ortiz wrote:
> > > > Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
> > > > specifications.
> > > >
> > > > Co-developed-by: Niklas Cassel <[email protected]>
> > > > Signed-off-by: Niklas Cassel <[email protected]>
> > > > Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> > > > ---
> > > > ? drivers/clk/qcom/gcc-qcs404.c | 6 ++++++
> > > > ? 1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/drivers/clk/qcom/gcc-qcs404.c
> > > > b/drivers/clk/qcom/gcc-qcs404.c
> > > > index 64da032..833436a 100644
> > > > --- a/drivers/clk/qcom/gcc-qcs404.c
> > > > +++ b/drivers/clk/qcom/gcc-qcs404.c
> > > > @@ -304,10 +304,16 @@ static struct clk_alpha_pll gpll0_out_main = {
> > > > ????? },
> > > > ? };
> > > > ? +static const struct pll_vco gpll0_ao_out_vco[] = {
> > > > +??? { 800000000, 800000000, 0 },
> > > > +};
> > > > +
> > > > ? static struct clk_alpha_pll gpll0_ao_out_main = {
> > > > ????? .offset = 0x21000,
> > > > ????? .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_DEFAULT],
> > > > ????? .flags = SUPPORTS_FSM_MODE,
> > > > +??? .vco_table = gpll0_ao_out_vco,
> > > > +??? .num_vco = ARRAY_SIZE(gpll0_ao_out_vco),
> > >
> > > Could you please help as to why this is required? This is a fixed
> > > PLL and we do not require a VCO table for it.
> >
> > Hi Taniya,
> >
> > this patch - the additional information that it provides about the
> > hardware - helps to select the right parent clock for a given frequency.
> >
> > On the qcs404 this clock is one of the two parent clocks of the apcs
> > clock controller (the other one being the high frequency pll)
> > When cpufreq sets a target frequency, there is an iteration through the
> > list of parents to select the one that delivers the best match.
> >
> > When attempting to set the clock for an alpha_pll, the operation does a
> > sanity check to validate that the requested frequency is in fact
> > reachable using the vco range: trying to set a value that is not in
> > range will fail.
> >
> > This patch makes sure that its range is explicitly defined.
> >
> > It also helps making sure there are no rounding issues when setting its
> > value: without it the clock was being read at 799MHz
> >
> >
>
> If the PLL is being read as 799MHz it would because not all the 40 bits of
> the ALPHA_VAL being programmed by the bootloaders(which are the original
> owners of this PLL). So we should go with the way they are being set by
> bootloaders and read by HLOS driver.
>
> And a VCO range you have considered is wrong from a PLL perspective. As
> these are fixed PLLs and VCO range really does not matter here, so please
> drop this change.
>
The problem here is that the PLL should be fixed at 800MHz, but the
alpha PLL is defined such that it can change. So when the mux-div is
looking for a suitable parent and divider for our CPU clock it concludes
that the best way to reach certain frequencies is to change the rate of
GPLL0.
Adding the vco_table limits the available frequencies for GPLL0 in
QCS404, without modifying the implementation of the alpha PLL.
Perhaps there's a better way to define that this particular clock
hardware can change rate, but in this implementation it must not?
Regards,
Bjorn
Quoting Jorge Ramirez (2018-12-21 11:45:28)
> On 12/21/18 20:28, Bjorn Andersson wrote:
> >
> > Perhaps there's a better way to define that this particular clock
> > hardware can change rate, but in this implementation it must not?
>
> the initialization for this particular PLL on this particular platform
> is wrong
> as the interface does not apply to the platform needs even though it is an
> alpha_pll
>
> if the VCO is not an option -even though it reflects the platform
> constrains-
> I would suggest nullifying the alpha_pll_ops that do not apply to this
> platform:
> ie: set_rate, round_rate set to null in the probe.
>
> allowing the interface calls (ops) to go through to later on make them fail
> based on some setting would be fundamentally wrong IMO
>
We have clk_alpha_pll_postdiv_ro_ops so maybe just add another set of
those for the alpha_pll itself?
On 12/18/18 15:35, Niklas Cassel wrote:
> On Mon, Dec 17, 2018 at 03:37:43PM -0800, Stephen Boyd wrote:
>> Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:22)
>>> Allow accessing the parent clock names required for the driver
>>> operation by using the device tree node.
>>>
>>> This permits extending the driver to other platforms without having to
>>> modify its source code.
>>>
>>> For backwards compatibility leave previous values as default.
>>
>> Why do we need to maintain backwards compatibility? Isn't is required
>> that the nodes have clocks properties?
>>
>
> Hello Stephen,
>
>
> This is the existing DT nodes for msm8916:
>
> a53pll: clock@b016000 {
> compatible = "qcom,msm8916-a53pll";
> reg = <0xb016000 0x40>;
> #clock-cells = <0>;
> };
>
> apcs: mailbox@b011000 {
> compatible = "qcom,msm8916-apcs-kpss-global", "syscon";
> reg = <0xb011000 0x1000>;
> #mbox-cells = <1>;
> clocks = <&a53pll>;
> #clock-cells = <0>;
> };
>
>
> This is the (suggested) DT nodes for qcs404:
>
> apcs_hfpll: clock-controller@0b016000 {
> compatible = "qcom,hfpll";
> reg = <0x0b016000 0x30>;
> #clock-cells = <0>;
> clock-output-names = "apcs_hfpll";
> clocks = <&xo_board>;
> clock-names = "xo";
> };
>
> apcs_glb: mailbox@b011000 {
> compatible = "qcom,qcs404-apcs-apps-global", "syscon";
> reg = <0x0b011000 0x1000>;
> #mbox-cells = <1>;
> clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
> clock-names = "aux", "pll";
> #clock-cells = <0>;
> };
>
> qcs404 specifies two clocks, with an accompanied clock-name for each clock.
>
> msm8916 specifies a single clock, without an accompanied clock-name.
>
> It is possible to append clock-names = "pll" for the existing clock,
> as well as to define the aux clock in the apcs node in the msm8916 DT:
> clocks = <&gcc GPLL0_VOTE>;
> clock-names = "aux";
>
> However, since the DT is treated as an ABI, the existing DT for msm8916 must
> still work, so I don't think that it is possible to ignore having backwards
> compability in the apcs clock driver.
so where are we with this?
do we remove backwards compatibility (see below] for v2 or is the DT
really an ABI and therefore the patch under review is good as is?
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi
b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index c5348c3..729c117 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -425,7 +425,8 @@
compatible = "qcom,msm8916-apcs-kpss-global",
"syscon";
reg = <0xb011000 0x1000>;
#mbox-cells = <1>;
- clocks = <&a53pll>;
+ clocks = <&gcc GPLL0_VOTE>, <&a53pll>;
+ clock-names = "aux", "pll";
#clock-cells = <0>;
};
>
>
> Kind regards,
> Niklas
>
Quoting Niklas Cassel (2018-12-18 06:35:03)
>
> This is the existing DT nodes for msm8916:
>
> a53pll: clock@b016000 {
> compatible = "qcom,msm8916-a53pll";
> reg = <0xb016000 0x40>;
> #clock-cells = <0>;
> };
>
> apcs: mailbox@b011000 {
> compatible = "qcom,msm8916-apcs-kpss-global", "syscon";
> reg = <0xb011000 0x1000>;
> #mbox-cells = <1>;
> clocks = <&a53pll>;
> #clock-cells = <0>;
> };
>
>
> This is the (suggested) DT nodes for qcs404:
>
> apcs_hfpll: clock-controller@0b016000 {
> compatible = "qcom,hfpll";
> reg = <0x0b016000 0x30>;
> #clock-cells = <0>;
> clock-output-names = "apcs_hfpll";
> clocks = <&xo_board>;
> clock-names = "xo";
> };
>
> apcs_glb: mailbox@b011000 {
> compatible = "qcom,qcs404-apcs-apps-global", "syscon";
> reg = <0x0b011000 0x1000>;
> #mbox-cells = <1>;
> clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
> clock-names = "aux", "pll";
> #clock-cells = <0>;
> };
>
> qcs404 specifies two clocks, with an accompanied clock-name for each clock.
>
> msm8916 specifies a single clock, without an accompanied clock-name.
>
> It is possible to append clock-names = "pll" for the existing clock,
> as well as to define the aux clock in the apcs node in the msm8916 DT:
> clocks = <&gcc GPLL0_VOTE>;
> clock-names = "aux";
>
> However, since the DT is treated as an ABI, the existing DT for msm8916 must
> still work, so I don't think that it is possible to ignore having backwards
> compability in the apcs clock driver.
>
We typically allow one clk to match the NULL connection name, so I think
things should work if you clk_get(dev, NULL) on 8916 and then try the
specific "aux" and "pll" strings strings after that for updated DT. Not
sure if that helps you here though.
And I would push to try and make all the clk connections between clk
controller nodes specified now. It will be useful to avoid global string
lookups in the future, so the sooner the better.
Quoting Jorge Ramirez (2018-12-26 01:20:07)
> On 12/18/18 15:35, Niklas Cassel wrote:
> > On Mon, Dec 17, 2018 at 03:37:43PM -0800, Stephen Boyd wrote:
> >> Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:22)
> >>> Allow accessing the parent clock names required for the driver
> >>> operation by using the device tree node.
> >>>
> >>> This permits extending the driver to other platforms without having to
> >>> modify its source code.
> >>>
> >>> For backwards compatibility leave previous values as default.
> >>
> >> Why do we need to maintain backwards compatibility? Isn't is required
> >> that the nodes have clocks properties?
> >>
> >
> > Hello Stephen,
> >
> >
> > This is the existing DT nodes for msm8916:
> >
> > a53pll: clock@b016000 {
> > compatible = "qcom,msm8916-a53pll";
> > reg = <0xb016000 0x40>;
> > #clock-cells = <0>;
> > };
> >
> > apcs: mailbox@b011000 {
> > compatible = "qcom,msm8916-apcs-kpss-global", "syscon";
> > reg = <0xb011000 0x1000>;
> > #mbox-cells = <1>;
> > clocks = <&a53pll>;
> > #clock-cells = <0>;
> > };
> >
> >
> > This is the (suggested) DT nodes for qcs404:
> >
> > apcs_hfpll: clock-controller@0b016000 {
> > compatible = "qcom,hfpll";
> > reg = <0x0b016000 0x30>;
> > #clock-cells = <0>;
> > clock-output-names = "apcs_hfpll";
> > clocks = <&xo_board>;
> > clock-names = "xo";
> > };
> >
> > apcs_glb: mailbox@b011000 {
> > compatible = "qcom,qcs404-apcs-apps-global", "syscon";
> > reg = <0x0b011000 0x1000>;
> > #mbox-cells = <1>;
> > clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
> > clock-names = "aux", "pll";
> > #clock-cells = <0>;
> > };
> >
> > qcs404 specifies two clocks, with an accompanied clock-name for each clock.
> >
> > msm8916 specifies a single clock, without an accompanied clock-name.
> >
> > It is possible to append clock-names = "pll" for the existing clock,
> > as well as to define the aux clock in the apcs node in the msm8916 DT:
> > clocks = <&gcc GPLL0_VOTE>;
> > clock-names = "aux";
> >
> > However, since the DT is treated as an ABI, the existing DT for msm8916 must
> > still work, so I don't think that it is possible to ignore having backwards
> > compability in the apcs clock driver.
>
>
> so where are we with this?
>
> do we remove backwards compatibility (see below] for v2 or is the DT
> really an ABI and therefore the patch under review is good as is?
>
Breaking compatibility is up to the platform maintainers. If anything, I
would make the DTS and driver changes in parallel and then remove the
driver's backwards compatibility logic later on.
On 12/28/18 23:28, Stephen Boyd wrote:
> Quoting Jorge Ramirez (2018-12-26 01:20:07)
>> On 12/18/18 15:35, Niklas Cassel wrote:
>>> On Mon, Dec 17, 2018 at 03:37:43PM -0800, Stephen Boyd wrote:
>>>> Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:22)
>>>>> Allow accessing the parent clock names required for the driver
>>>>> operation by using the device tree node.
>>>>>
>>>>> This permits extending the driver to other platforms without having to
>>>>> modify its source code.
>>>>>
>>>>> For backwards compatibility leave previous values as default.
>>>>
>>>> Why do we need to maintain backwards compatibility? Isn't is required
>>>> that the nodes have clocks properties?
>>>>
>>>
>>> Hello Stephen,
>>>
>>>
>>> This is the existing DT nodes for msm8916:
>>>
>>> a53pll: clock@b016000 {
>>> compatible = "qcom,msm8916-a53pll";
>>> reg = <0xb016000 0x40>;
>>> #clock-cells = <0>;
>>> };
>>>
>>> apcs: mailbox@b011000 {
>>> compatible = "qcom,msm8916-apcs-kpss-global", "syscon";
>>> reg = <0xb011000 0x1000>;
>>> #mbox-cells = <1>;
>>> clocks = <&a53pll>;
>>> #clock-cells = <0>;
>>> };
>>>
>>>
>>> This is the (suggested) DT nodes for qcs404:
>>>
>>> apcs_hfpll: clock-controller@0b016000 {
>>> compatible = "qcom,hfpll";
>>> reg = <0x0b016000 0x30>;
>>> #clock-cells = <0>;
>>> clock-output-names = "apcs_hfpll";
>>> clocks = <&xo_board>;
>>> clock-names = "xo";
>>> };
>>>
>>> apcs_glb: mailbox@b011000 {
>>> compatible = "qcom,qcs404-apcs-apps-global", "syscon";
>>> reg = <0x0b011000 0x1000>;
>>> #mbox-cells = <1>;
>>> clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
>>> clock-names = "aux", "pll";
>>> #clock-cells = <0>;
>>> };
>>>
>>> qcs404 specifies two clocks, with an accompanied clock-name for each clock.
>>>
>>> msm8916 specifies a single clock, without an accompanied clock-name.
>>>
>>> It is possible to append clock-names = "pll" for the existing clock,
>>> as well as to define the aux clock in the apcs node in the msm8916 DT:
>>> clocks = <&gcc GPLL0_VOTE>;
>>> clock-names = "aux";
>>>
>>> However, since the DT is treated as an ABI, the existing DT for msm8916 must
>>> still work, so I don't think that it is possible to ignore having backwards
>>> compability in the apcs clock driver.
>>
>>
>> so where are we with this?
>>
>> do we remove backwards compatibility (see below] for v2 or is the DT
>> really an ABI and therefore the patch under review is good as is?
>>
>
> Breaking compatibility is up to the platform maintainers. If anything, I
> would make the DTS and driver changes in parallel and then remove the
> driver's backwards compatibility logic later on.
>
>
I am not completely sure of what you mean. are you saying that the
original patch is good and we should just provide another patch (not
part of the current patch series) to remove the compatibility?
TIA
On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
> Specify the clocks that feed the APCS mux/divider instead of using
> default hardcoded values in the source code.
>
Reviewed-by: Bjorn Andersson <[email protected]>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/qcs404.dtsi | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> index ec3f6c7..2d9e70e 100644
> --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> @@ -373,6 +373,9 @@
> compatible = "qcom,qcs404-apcs-apps-global", "syscon";
> reg = <0x0b011000 0x1000>;
> #mbox-cells = <1>;
> + clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
> + clock-names = "aux", "pll";
> + #clock-cells = <0>;
> };
>
> apcs_hfpll: clock-controller@0b016000 {
> --
> 2.7.4
>
On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
> Make the output of the high frequency pll a clock provider.
> On the QCS404 this PLL controls cpu frequency scaling.
>
Reviewed-by: Bjorn Andersson <[email protected]>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> 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 87b7f46..0ffed0d 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.7.4
>
On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
> Support CPU frequency scaling on qcs404.
>
Reviewed-by: Bjorn Andersson <[email protected]>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/qcs404.dtsi | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> index 2d9e70e..5a14887 100644
> --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> @@ -30,6 +30,8 @@
> reg = <0x100>;
> enable-method = "psci";
> next-level-cache = <&L2_0>;
> + clocks = <&apcs_glb>;
> + operating-points-v2 = <&cpu_opp_table>;
> };
>
> CPU1: cpu@101 {
> @@ -38,6 +40,8 @@
> reg = <0x101>;
> enable-method = "psci";
> next-level-cache = <&L2_0>;
> + clocks = <&apcs_glb>;
> + operating-points-v2 = <&cpu_opp_table>;
> };
>
> CPU2: cpu@102 {
> @@ -46,6 +50,8 @@
> reg = <0x102>;
> enable-method = "psci";
> next-level-cache = <&L2_0>;
> + clocks = <&apcs_glb>;
> + operating-points-v2 = <&cpu_opp_table>;
> };
>
> CPU3: cpu@103 {
> @@ -54,6 +60,8 @@
> reg = <0x103>;
> enable-method = "psci";
> next-level-cache = <&L2_0>;
> + clocks = <&apcs_glb>;
> + operating-points-v2 = <&cpu_opp_table>;
> };
>
> L2_0: l2-cache {
> --
> 2.7.4
>
On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
> Add a CPU OPP table to qcs404
>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> index 9b5c165..4594fea7 100644
> --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> @@ -62,6 +62,21 @@
> };
> };
>
> + cpu_opp_table: cpu_opp_table {
Please don't use _ in the node name. (cpu_opp_table: cpu-opp-table)
Apart from that, this looks good
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> + compatible = "operating-points-v2";
> + opp-shared;
> +
> + opp-1094400000 {
> + opp-hz = /bits/ 64 <1094400000>;
> + };
> + opp-1248000000 {
> + opp-hz = /bits/ 64 <1248000000>;
> + };
> + opp-1401600000 {
> + opp-hz = /bits/ 64 <1401600000>;
> + };
> + };
> +
> firmware {
> scm: scm {
> compatible = "qcom,scm-qcs404", "qcom,scm";
> --
> 2.7.4
>
On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
> 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.
>
With the CPUFREQ referencing the CPU clock driver, that has decided to
run off this clock, why is it not refcounted?
Regards,
Bjorn
> 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]>
> ---
> drivers/clk/qcom/hfpll.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c
> index 0ffed0d..9d92f5d 100644
> --- a/drivers/clk/qcom/hfpll.c
> +++ b/drivers/clk/qcom/hfpll.c
> @@ -58,6 +58,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
> .parent_names = (const char *[]){ "xo" },
> .num_parents = 1,
> .ops = &clk_ops_hfpll,
> + .flags = CLK_IGNORE_UNUSED,
> };
>
> h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL);
> --
> 2.7.4
>
On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
> The high frequency pll is required on compatible Qualcomm SoCs to
> support the CPU frequency scaling feature.
>
Reviewed-by: Bjorn Andersson <[email protected]>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> arch/arm64/configs/defconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 5c2b1f6..da390aa 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -615,6 +615,7 @@ CONFIG_MSM_MMCC_8996=y
> CONFIG_MSM_GCC_8998=y
> CONFIG_QCS_GCC_404=y
> CONFIG_SDM_GCC_845=y
> +CONFIG_QCOM_HFPLL=y
> CONFIG_HWSPINLOCK=y
> CONFIG_HWSPINLOCK_QCOM=y
> CONFIG_ARM_MHU=y
> --
> 2.7.4
>
On Mon 17 Dec 11:39 PST 2018, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:27)
> > The high frequency pll functionality is required to enable CPU
> > frequency scaling operation.
> >
> > Co-developed-by: Niklas Cassel <[email protected]>
> > Signed-off-by: Niklas Cassel <[email protected]>
> > Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> > ---
> > arch/arm64/boot/dts/qcom/qcs404.dtsi | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > index 4594fea7..ec3f6c7 100644
> > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > @@ -375,6 +375,15 @@
> > #mbox-cells = <1>;
> > };
> >
> > + apcs_hfpll: clock-controller@0b016000 {
>
> Drop leading 0 on unit address please.
>
> > + compatible = "qcom,hfpll";
> > + reg = <0x0b016000 0x30>;
>
> Wow that is small!
>
I double checked and it's actually 0x34, but the last register is
protected.
Regards,
Bjorn
> > + #clock-cells = <0>;
> > + clock-output-names = "apcs_hfpll";
> > + clocks = <&xo_board>;
> > + clock-names = "xo";
> > + };
> > +
On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
> When the APCS clock is registered (platform dependent), it retrieves
> its parent names from hardcoded values in the driver.
>
> The following commit allows the DT node to provide such clock names to
> the platform data based clock driver therefore avoiding having to
> explicitly embed those names in the clock driver source code.
>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> .../bindings/mailbox/qcom,apcs-kpss-global.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> index 1232fc9..f252439 100644
> --- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> +++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> @@ -23,6 +23,10 @@ platforms.
> Value type: <phandle>
> Definition: phandle to the input PLL, which feeds the APCS mux/divider
>
> + Usage: required if #clock-names property is present
> + Value type: <phandle array>
> + Definition: phandles to the two parent clocks of the clock driver.
I presume you meant to replace the existing definition of "clocks"?
I think the purpose of "required if #clock-cells" comes from that
meaning that it represents a clock-controller if #clock-cells is
specified, in which case it requires the upstream clock(s).
Regards,
Bjorn
> +
> - #mbox-cells:
> Usage: required
> Value type: <u32>
> @@ -33,6 +37,12 @@ platforms.
> Value type: <u32>
> Definition: as described in clock.txt, must be 0
>
> +- clock-names:
> + Usage: required if the platform data based clock driver needs to
> + retrieve the parent clock names from device tree.
> + This will requires two mandatory clocks to be defined.
> + Value type: <string-array>
> + Definition: must be "aux" and "pll"
>
> = EXAMPLE
> The following example describes the APCS HMSS found in MSM8996 and part of the
> @@ -65,3 +75,14 @@ Below is another example of the APCS binding on MSM8916 platforms:
> clocks = <&a53pll>;
> #clock-cells = <0>;
> };
> +
> +Below is another example of the APCS binding on QCS404 platforms:
> +
> + apcs_glb: mailbox@b011000 {
> + compatible = "qcom,qcs404-apcs-apps-global", "syscon";
> + reg = <0x0b011000 0x1000>;
> + #mbox-cells = <1>;
> + clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
> + clock-names = "aux", "pll";
> + #clock-cells = <0>;
> + };
> --
> 2.7.4
>
On Mon 31 Dec 00:42 PST 2018, Jorge Ramirez wrote:
[..]
> I am not completely sure of what you mean. are you saying that the original
> patch is good and we should just provide another patch (not part of the
> current patch series) to remove the compatibility?
>
You're expected to make sure that db410c continues to function on
existing DTBs, at least for a considerable amount of time.
Regards,
Bjorn
On 1/17/19 11:08, Viresh Kumar wrote:
> On 17-01-19, 09:38, Jorge Ramirez wrote:
>> COMMON_CLK_DISABLED_UNUSED relies on the enable_count reference counter
>> to disable the clocks that were enabled by the firwmare and not by the
>> drivers.
>>
>> the cpufreq driver does not enable the cpu clock.
>>
>> so when clk_change_rate is called, the enable_count counter is not
>> incremented and therefore it just remains null since this was enabled by
>> the firmware.
>>
>> I tried doing:
>>
>> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
>> index e58bfcb..5a9f83e 100644
>> --- a/drivers/cpufreq/cpufreq-dt.c
>> +++ b/drivers/cpufreq/cpufreq-dt.c
>> @@ -124,6 +124,10 @@ static int resources_available(void)
>> return ret;
>> }
>>
>> + ret = clk_prepare_enable(cpu_clk);
>> + if (ret)
>> + return ret;
>> +
>> clk_put(cpu_clk);
>>
>> name = find_supply_name(cpu_dev);
>>
>>
>> and that removed the need for CLK_IGNORE_UNUSED. But I am not sure of
>> the system wide consequences of that change to cpufreq.
>
> If the cpufreq driver enables it then it should disable it on exit as
> well, right ? And in that case if you unload your driver's module, you
> will hang the system as the clock will get disabled :)
ah, of course, sorry was over-thinking this thing :)
>
> Every other platform must either be marking it with CLK_IGNORE_UNUSED
> or they must be doing clk_enable from somewhere, maybe the CPU online
> path, not sure though.
>
since this clock is enabled by the firmware, it seems to me that using
CLK_IGNORE_UNUSED remains the best option.
On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
> 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.
>
Reviewed-by: Bjorn Andersson <[email protected]>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> 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 a6de7101..87b7f46 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.7.4
>
On 1/17/19 07:33, Bjorn Andersson wrote:
> On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
>
>> 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.
>>
>
> With the CPUFREQ referencing the CPU clock driver, that has decided to
> run off this clock, why is it not refcounted?
COMMON_CLK_DISABLED_UNUSED relies on the enable_count reference counter
to disable the clocks that were enabled by the firwmare and not by the
drivers.
the cpufreq driver does not enable the cpu clock.
so when clk_change_rate is called, the enable_count counter is not
incremented and therefore it just remains null since this was enabled by
the firmware.
I tried doing:
diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index e58bfcb..5a9f83e 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -124,6 +124,10 @@ static int resources_available(void)
return ret;
}
+ ret = clk_prepare_enable(cpu_clk);
+ if (ret)
+ return ret;
+
clk_put(cpu_clk);
name = find_supply_name(cpu_dev);
and that removed the need for CLK_IGNORE_UNUSED. But I am not sure of
the system wide consequences of that change to cpufreq.
maybe Viresh can comment?
>
> Regards,
> Bjorn
>
>> 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]>
>> ---
>> drivers/clk/qcom/hfpll.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c
>> index 0ffed0d..9d92f5d 100644
>> --- a/drivers/clk/qcom/hfpll.c
>> +++ b/drivers/clk/qcom/hfpll.c
>> @@ -58,6 +58,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
>> .parent_names = (const char *[]){ "xo" },
>> .num_parents = 1,
>> .ops = &clk_ops_hfpll,
>> + .flags = CLK_IGNORE_UNUSED,
>> };
>>
>> h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL);
>> --
>> 2.7.4
>>
>
On 17-01-19, 09:38, Jorge Ramirez wrote:
> COMMON_CLK_DISABLED_UNUSED relies on the enable_count reference counter
> to disable the clocks that were enabled by the firwmare and not by the
> drivers.
>
> the cpufreq driver does not enable the cpu clock.
>
> so when clk_change_rate is called, the enable_count counter is not
> incremented and therefore it just remains null since this was enabled by
> the firmware.
>
> I tried doing:
>
> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> index e58bfcb..5a9f83e 100644
> --- a/drivers/cpufreq/cpufreq-dt.c
> +++ b/drivers/cpufreq/cpufreq-dt.c
> @@ -124,6 +124,10 @@ static int resources_available(void)
> return ret;
> }
>
> + ret = clk_prepare_enable(cpu_clk);
> + if (ret)
> + return ret;
> +
> clk_put(cpu_clk);
>
> name = find_supply_name(cpu_dev);
>
>
> and that removed the need for CLK_IGNORE_UNUSED. But I am not sure of
> the system wide consequences of that change to cpufreq.
If the cpufreq driver enables it then it should disable it on exit as
well, right ? And in that case if you unload your driver's module, you
will hang the system as the clock will get disabled :)
Every other platform must either be marking it with CLK_IGNORE_UNUSED
or they must be doing clk_enable from somewhere, maybe the CPU online
path, not sure though.
--
viresh
On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
> There is clock controller functionality in the APCS hardware block of
> qcs404 devices similar to msm8916.
>
Reviewed-by: Bjorn Andersson <[email protected]>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> drivers/mailbox/qcom-apcs-ipc-mailbox.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> index aed23ac..dc8fdab1 100644
> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> @@ -97,16 +97,21 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
> return ret;
> }
>
> - if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global")) {
> - apcs->clk = platform_device_register_data(&pdev->dev,
> - "qcom-apcs-msm8916-clk",
> - -1, NULL, 0);
> - if (IS_ERR(apcs->clk))
> - dev_err(&pdev->dev, "failed to register APCS clk\n");
> - }
> -
> platform_set_drvdata(pdev, apcs);
>
> + if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global") ||
> + of_device_is_compatible(np, "qcom,qcs404-apcs-apps-global"))
> + goto register_clk;
> +
> + return 0;
> +
> +register_clk:
> + apcs->clk = platform_device_register_data(&pdev->dev,
> + "qcom-apcs-msm8916-clk",
> + -1, NULL, 0);
> + if (IS_ERR(apcs->clk))
> + dev_err(&pdev->dev, "failed to register APCS clk\n");
> +
> return 0;
> }
>
> --
> 2.7.4
>
On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
> Use the correct macro when registering the platform device.
>
Reviewed-by: Bjorn Andersson <[email protected]>
> Co-developed-by: Niklas Cassel <[email protected]>
> Signed-off-by: Niklas Cassel <[email protected]>
> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> ---
> drivers/mailbox/qcom-apcs-ipc-mailbox.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> index dc8fdab1..b782173 100644
> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> @@ -108,7 +108,7 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
> register_clk:
> apcs->clk = platform_device_register_data(&pdev->dev,
> "qcom-apcs-msm8916-clk",
> - -1, NULL, 0);
> + PLATFORM_DEVID_NONE, NULL, 0);
> if (IS_ERR(apcs->clk))
> dev_err(&pdev->dev, "failed to register APCS clk\n");
>
> --
> 2.7.4
>
Quoting Jorge Ramirez (2019-01-17 02:46:21)
> On 1/17/19 11:08, Viresh Kumar wrote:
> > On 17-01-19, 09:38, Jorge Ramirez wrote:
> >> COMMON_CLK_DISABLED_UNUSED relies on the enable_count reference counter
> >> to disable the clocks that were enabled by the firwmare and not by the
> >> drivers.
> >>
> >> the cpufreq driver does not enable the cpu clock.
> >>
> >> so when clk_change_rate is called, the enable_count counter is not
> >> incremented and therefore it just remains null since this was enabled by
> >> the firmware.
> >>
> >> I tried doing:
> >>
> >> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
> >> index e58bfcb..5a9f83e 100644
> >> --- a/drivers/cpufreq/cpufreq-dt.c
> >> +++ b/drivers/cpufreq/cpufreq-dt.c
> >> @@ -124,6 +124,10 @@ static int resources_available(void)
> >> return ret;
> >> }
> >>
> >> + ret = clk_prepare_enable(cpu_clk);
> >> + if (ret)
> >> + return ret;
> >> +
> >> clk_put(cpu_clk);
> >>
> >> name = find_supply_name(cpu_dev);
> >>
> >>
> >> and that removed the need for CLK_IGNORE_UNUSED. But I am not sure of
> >> the system wide consequences of that change to cpufreq.
> >
> > If the cpufreq driver enables it then it should disable it on exit as
> > well, right ? And in that case if you unload your driver's module, you
> > will hang the system as the clock will get disabled :)
>
> ah, of course, sorry was over-thinking this thing :)
>
> >
> > Every other platform must either be marking it with CLK_IGNORE_UNUSED
> > or they must be doing clk_enable from somewhere, maybe the CPU online
> > path, not sure though.
> >
>
> since this clock is enabled by the firmware, it seems to me that using
> CLK_IGNORE_UNUSED remains the best option.
>
What do you do about CPUs being offlined? Presumably when the CPU is
gone the system doesn't need to keep the clk enabled anymore.
Quoting Bjorn Andersson (2019-01-16 22:38:04)
> On Mon 17 Dec 11:39 PST 2018, Stephen Boyd wrote:
>
> > Quoting Jorge Ramirez-Ortiz (2018-12-17 01:46:27)
> > > The high frequency pll functionality is required to enable CPU
> > > frequency scaling operation.
> > >
> > > Co-developed-by: Niklas Cassel <[email protected]>
> > > Signed-off-by: Niklas Cassel <[email protected]>
> > > Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
> > > ---
> > > arch/arm64/boot/dts/qcom/qcs404.dtsi | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > index 4594fea7..ec3f6c7 100644
> > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > @@ -375,6 +375,15 @@
> > > #mbox-cells = <1>;
> > > };
> > >
> > > + apcs_hfpll: clock-controller@0b016000 {
> >
> > Drop leading 0 on unit address please.
> >
> > > + compatible = "qcom,hfpll";
> > > + reg = <0x0b016000 0x30>;
> >
> > Wow that is small!
> >
>
> I double checked and it's actually 0x34, but the last register is
> protected.
>
Ok, so then it should be 0x34? I don't think we've left out protected
registers from the size before.
On 1/22/19 19:47, Stephen Boyd wrote:
> Quoting Jorge Ramirez (2019-01-17 02:46:21)
>> On 1/17/19 11:08, Viresh Kumar wrote:
>>> On 17-01-19, 09:38, Jorge Ramirez wrote:
>>>> COMMON_CLK_DISABLED_UNUSED relies on the enable_count reference counter
>>>> to disable the clocks that were enabled by the firwmare and not by the
>>>> drivers.
>>>>
>>>> the cpufreq driver does not enable the cpu clock.
>>>>
>>>> so when clk_change_rate is called, the enable_count counter is not
>>>> incremented and therefore it just remains null since this was enabled by
>>>> the firmware.
>>>>
>>>> I tried doing:
>>>>
>>>> diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
>>>> index e58bfcb..5a9f83e 100644
>>>> --- a/drivers/cpufreq/cpufreq-dt.c
>>>> +++ b/drivers/cpufreq/cpufreq-dt.c
>>>> @@ -124,6 +124,10 @@ static int resources_available(void)
>>>> return ret;
>>>> }
>>>>
>>>> + ret = clk_prepare_enable(cpu_clk);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> clk_put(cpu_clk);
>>>>
>>>> name = find_supply_name(cpu_dev);
>>>>
>>>>
>>>> and that removed the need for CLK_IGNORE_UNUSED. But I am not sure of
>>>> the system wide consequences of that change to cpufreq.
>>>
>>> If the cpufreq driver enables it then it should disable it on exit as
>>> well, right ? And in that case if you unload your driver's module, you
>>> will hang the system as the clock will get disabled :)
>>
>> ah, of course, sorry was over-thinking this thing :)
>>
>>>
>>> Every other platform must either be marking it with CLK_IGNORE_UNUSED
>>> or they must be doing clk_enable from somewhere, maybe the CPU online
>>> path, not sure though.
>>>
>>
>> since this clock is enabled by the firmware, it seems to me that using
>> CLK_IGNORE_UNUSED remains the best option.
>>
>
> What do you do about CPUs being offlined? Presumably when the CPU is
> gone the system doesn't need to keep the clk enabled anymore.
>
>
the kernel does not take any action - it is under firmware control; and
since the clock is shared between all cores I there will have to be some
involvement not only at TF-A/firmware level but also at the TrustedOS
level for when the last core is offlined.
I dont have visibility on either of those projects though.
On 1/17/19 07:44, Bjorn Andersson wrote:
> On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
>
>> When the APCS clock is registered (platform dependent), it retrieves
>> its parent names from hardcoded values in the driver.
>>
>> The following commit allows the DT node to provide such clock names to
>> the platform data based clock driver therefore avoiding having to
>> explicitly embed those names in the clock driver source code.
>>
>> Co-developed-by: Niklas Cassel <[email protected]>
>> Signed-off-by: Niklas Cassel <[email protected]>
>> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
>> ---
>> .../bindings/mailbox/qcom,apcs-kpss-global.txt | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
>> index 1232fc9..f252439 100644
>> --- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
>> +++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
>> @@ -23,6 +23,10 @@ platforms.
>> Value type: <phandle>
>> Definition: phandle to the input PLL, which feeds the APCS mux/divider
>>
>> + Usage: required if #clock-names property is present
>> + Value type: <phandle array>
>> + Definition: phandles to the two parent clocks of the clock driver.
>
> I presume you meant to replace the existing definition of "clocks"?
I am sorry didn't reply to this earlier. Yes this is not very clear.
This is required as an extension to the apcs-msm8916.c driver also used
on the qcs404; since it is an extension, the previous definition should
still be applicable.
In the case of the msm8916 the #clock-names property is NOT necessary
(it would be ignored by the driver), so having it should not mean that
we need to have #clocks.
In the case of the qcs404, having clock-names means that we do need to
have #clocks (hence the additional if)
and not quite sure how to word this condition in the bindings..
I am going to post v2 with some updates and if required will post a v3
with more clarifications.
>
> I think the purpose of "required if #clock-cells" comes from that
> meaning that it represents a clock-controller if #clock-cells is
> specified, in which case it requires the upstream clock(s).
>
> Regards,
> Bjorn
>
>> +
>> - #mbox-cells:
>> Usage: required
>> Value type: <u32>
>> @@ -33,6 +37,12 @@ platforms.
>> Value type: <u32>
>> Definition: as described in clock.txt, must be 0
>>
>> +- clock-names:
>> + Usage: required if the platform data based clock driver needs to
>> + retrieve the parent clock names from device tree.
>> + This will requires two mandatory clocks to be defined.
>> + Value type: <string-array>
>> + Definition: must be "aux" and "pll"
>>
>> = EXAMPLE
>> The following example describes the APCS HMSS found in MSM8996 and part of the
>> @@ -65,3 +75,14 @@ Below is another example of the APCS binding on MSM8916 platforms:
>> clocks = <&a53pll>;
>> #clock-cells = <0>;
>> };
>> +
>> +Below is another example of the APCS binding on QCS404 platforms:
>> +
>> + apcs_glb: mailbox@b011000 {
>> + compatible = "qcom,qcs404-apcs-apps-global", "syscon";
>> + reg = <0x0b011000 0x1000>;
>> + #mbox-cells = <1>;
>> + clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
>> + clock-names = "aux", "pll";
>> + #clock-cells = <0>;
>> + };
>> --
>> 2.7.4
>>
>
On 1/28/19 17:57, Jorge Ramirez wrote:
> On 1/17/19 07:44, Bjorn Andersson wrote:
>> On Mon 17 Dec 01:46 PST 2018, Jorge Ramirez-Ortiz wrote:
>>
>>> When the APCS clock is registered (platform dependent), it retrieves
>>> its parent names from hardcoded values in the driver.
>>>
>>> The following commit allows the DT node to provide such clock names to
>>> the platform data based clock driver therefore avoiding having to
>>> explicitly embed those names in the clock driver source code.
>>>
>>> Co-developed-by: Niklas Cassel <[email protected]>
>>> Signed-off-by: Niklas Cassel <[email protected]>
>>> Signed-off-by: Jorge Ramirez-Ortiz <[email protected]>
>>> ---
>>> .../bindings/mailbox/qcom,apcs-kpss-global.txt | 21 +++++++++++++++++++++
>>> 1 file changed, 21 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
>>> index 1232fc9..f252439 100644
>>> --- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
>>> +++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
>>> @@ -23,6 +23,10 @@ platforms.
>>> Value type: <phandle>
>>> Definition: phandle to the input PLL, which feeds the APCS mux/divider
>>>
>>> + Usage: required if #clock-names property is present
>>> + Value type: <phandle array>
>>> + Definition: phandles to the two parent clocks of the clock driver.
>>
>> I presume you meant to replace the existing definition of "clocks"?
>
> I am sorry didn't reply to this earlier. Yes this is not very clear.
>
> This is required as an extension to the apcs-msm8916.c driver also used
> on the qcs404; since it is an extension, the previous definition should
> still be applicable.
>
> In the case of the msm8916 the #clock-names property is NOT necessary
> (it would be ignored by the driver), so having it should not mean that
> we need to have #clocks.
>
> In the case of the qcs404, having clock-names means that we do need to
> have #clocks (hence the additional if)
>
> and not quite sure how to word this condition in the bindings..
>
> I am going to post v2 with some updates and if required will post a v3
> with more clarifications.
In the version that I am about to post I ended up following your
suggestion: replaced the existing definition (and the apcs mailbox node
msm8916.dts) but kept bacwards compatibility in the driver (so old
bindings will still work).
That should enable migration to the new bindings -as per the
documentation - for new platforms (something that IIRC sboyd also asked for)
>
>>
>> I think the purpose of "required if #clock-cells" comes from that
>> meaning that it represents a clock-controller if #clock-cells is
>> specified, in which case it requires the upstream clock(s).
>>
>> Regards,
>> Bjorn
>>
>>> +
>>> - #mbox-cells:
>>> Usage: required
>>> Value type: <u32>
>>> @@ -33,6 +37,12 @@ platforms.
>>> Value type: <u32>
>>> Definition: as described in clock.txt, must be 0
>>>
>>> +- clock-names:
>>> + Usage: required if the platform data based clock driver needs to
>>> + retrieve the parent clock names from device tree.
>>> + This will requires two mandatory clocks to be defined.
>>> + Value type: <string-array>
>>> + Definition: must be "aux" and "pll"
>>>
>>> = EXAMPLE
>>> The following example describes the APCS HMSS found in MSM8996 and part of the
>>> @@ -65,3 +75,14 @@ Below is another example of the APCS binding on MSM8916 platforms:
>>> clocks = <&a53pll>;
>>> #clock-cells = <0>;
>>> };
>>> +
>>> +Below is another example of the APCS binding on QCS404 platforms:
>>> +
>>> + apcs_glb: mailbox@b011000 {
>>> + compatible = "qcom,qcs404-apcs-apps-global", "syscon";
>>> + reg = <0x0b011000 0x1000>;
>>> + #mbox-cells = <1>;
>>> + clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
>>> + clock-names = "aux", "pll";
>>> + #clock-cells = <0>;
>>> + };
>>> --
>>> 2.7.4
>>>
>>
>