linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support
@ 2021-05-04  5:28 Shawn Guo
  2021-05-04  5:28 ` [PATCH 1/5] clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical Shawn Guo
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Shawn Guo @ 2021-05-04  5:28 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk, Shawn Guo

This series adds MSM8939 APCS/A53PLL clock support.  Most outstanding
thing about MSM8939 is that it integrates 3 APCS instances, for Cluster0
(little cores), Cluster1 (big cores) and CCI (Cache Coherent Interconnect)
respectively.

Note: the first one is a small improvement which is not specific to
MSM8939 support.


Shawn Guo (5):
  clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical
  dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support
  clk: qcom: apcs-msm8916: Retrieve clock name from DT
  clk: qcom: a53-pll: Pass freq_tbl via match data
  clk: qcom: a53-pll: Add MSM8939 a53pll clocks

 .../bindings/clock/qcom,a53pll.yaml           | 34 +++++++++++
 drivers/clk/qcom/a53-pll.c                    | 59 +++++++++++++++++--
 drivers/clk/qcom/apcs-msm8916.c               |  7 ++-
 3 files changed, 93 insertions(+), 7 deletions(-)

-- 
2.17.1


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

* [PATCH 1/5] clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical
  2021-05-04  5:28 [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support Shawn Guo
@ 2021-05-04  5:28 ` Shawn Guo
  2021-06-28  0:27   ` Stephen Boyd
  2021-05-04  5:28 ` [PATCH 2/5] dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support Shawn Guo
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2021-05-04  5:28 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk, Shawn Guo

The clock source for MSM8916 cpu cores is like below.

                        |\
         a53pll --------| \ a53mux     +------+
                        | |------------| cpus |
     gpll0_vote --------| /            +------+
                        |/

So clock a53mux rather than a53pll is actually the clock source of cpu
cores.  It makes more sense to flag a53mux rather than a53pll as
critical, since a53pll could be irrelevant if a53mux switches its parent
clock to be gpll0_vote.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/qcom/a53-pll.c      | 1 -
 drivers/clk/qcom/apcs-msm8916.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
index 45cfc57bff92..8614b0b0e82c 100644
--- a/drivers/clk/qcom/a53-pll.c
+++ b/drivers/clk/qcom/a53-pll.c
@@ -70,7 +70,6 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
 	init.parent_names = (const char *[]){ "xo" };
 	init.num_parents = 1;
 	init.ops = &clk_pll_sr2_ops;
-	init.flags = CLK_IS_CRITICAL;
 	pll->clkr.hw.init = &init;
 
 	ret = devm_clk_register_regmap(dev, &pll->clkr);
diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
index cf69a97d0439..d7ac6d6b15b6 100644
--- a/drivers/clk/qcom/apcs-msm8916.c
+++ b/drivers/clk/qcom/apcs-msm8916.c
@@ -65,7 +65,7 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	init.parent_data = pdata;
 	init.num_parents = ARRAY_SIZE(pdata);
 	init.ops = &clk_regmap_mux_div_ops;
-	init.flags = CLK_SET_RATE_PARENT;
+	init.flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT;
 
 	a53cc->clkr.hw.init = &init;
 	a53cc->clkr.regmap = regmap;
-- 
2.17.1


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

* [PATCH 2/5] dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support
  2021-05-04  5:28 [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support Shawn Guo
  2021-05-04  5:28 ` [PATCH 1/5] clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical Shawn Guo
@ 2021-05-04  5:28 ` Shawn Guo
  2021-05-06 20:27   ` Rob Herring
  2021-05-04  5:28 ` [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT Shawn Guo
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2021-05-04  5:28 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk, Shawn Guo

Update qcom,a53pll bindings for MSM8939 support:

 - Add optional clock-output-names property.
 - Add MSM8939 specific compatibles.
 - Add MSM8939 examples.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 .../bindings/clock/qcom,a53pll.yaml           | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml b/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
index db3d0ea6bc7a..7a410a76be2f 100644
--- a/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
@@ -18,6 +18,9 @@ properties:
     enum:
       - qcom,ipq6018-a53pll
       - qcom,msm8916-a53pll
+      - qcom,msm8939-a53pll-c0
+      - qcom,msm8939-a53pll-c1
+      - qcom,msm8939-a53pll-cci
 
   reg:
     maxItems: 1
@@ -33,6 +36,9 @@ properties:
     items:
       - const: xo
 
+  clock-output-names:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -57,3 +63,31 @@ examples:
         clocks = <&xo>;
         clock-names = "xo";
     };
+  #Example 3 - A53 PLLs found on MSM8939 devices
+  - |
+    a53pll_c1: clock-controller@b016000 {
+        compatible = "qcom,msm8939-a53pll-c1";
+        reg = <0xb016000 0x40>;
+        #clock-cells = <0>;
+        clocks = <&xo_board>;
+        clock-names = "xo";
+        clock-output-names = "a53pll_c1";
+    };
+
+    a53pll_c0: clock-controller@b116000 {
+        compatible = "qcom,msm8939-a53pll-c0";
+        reg = <0xb116000 0x40>;
+        #clock-cells = <0>;
+        clocks = <&xo_board>;
+        clock-names = "xo";
+        clock-output-names = "a53pll_c0";
+    };
+
+    a53pll_cci: clock-controller@b1d0000 {
+        compatible = "qcom,msm8939-a53pll-cci";
+        reg = <0xb1d0000 0x40>;
+        #clock-cells = <0>;
+        clocks = <&xo_board>;
+        clock-names = "xo";
+        clock-output-names = "a53pll_cci";
+    };
-- 
2.17.1


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

* [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT
  2021-05-04  5:28 [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support Shawn Guo
  2021-05-04  5:28 ` [PATCH 1/5] clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical Shawn Guo
  2021-05-04  5:28 ` [PATCH 2/5] dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support Shawn Guo
@ 2021-05-04  5:28 ` Shawn Guo
  2021-06-28  0:28   ` Stephen Boyd
  2021-05-04  5:28 ` [PATCH 4/5] clk: qcom: a53-pll: Pass freq_tbl via match data Shawn Guo
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2021-05-04  5:28 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk, Shawn Guo

Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for
Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent
Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,
retrieve the name from DT, so that multiple APCS clocks can be
registered.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/qcom/a53-pll.c      | 5 ++++-
 drivers/clk/qcom/apcs-msm8916.c | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
index 8614b0b0e82c..964f5ab7d02f 100644
--- a/drivers/clk/qcom/a53-pll.c
+++ b/drivers/clk/qcom/a53-pll.c
@@ -42,6 +42,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
 	struct clk_pll *pll;
 	void __iomem *base;
 	struct clk_init_data init = { };
+	const char *clk_name = NULL;
 	int ret;
 
 	pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
@@ -66,7 +67,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
 	pll->status_bit = 16;
 	pll->freq_tbl = a53pll_freq;
 
-	init.name = "a53pll";
+	of_property_read_string(pdev->dev.of_node, "clock-output-names",
+				&clk_name);
+	init.name = clk_name ? clk_name : "a53pll";
 	init.parent_names = (const char *[]){ "xo" };
 	init.num_parents = 1;
 	init.ops = &clk_pll_sr2_ops;
diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
index d7ac6d6b15b6..b8bbfe9622e1 100644
--- a/drivers/clk/qcom/apcs-msm8916.c
+++ b/drivers/clk/qcom/apcs-msm8916.c
@@ -49,6 +49,7 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	struct clk_regmap_mux_div *a53cc;
 	struct regmap *regmap;
 	struct clk_init_data init = { };
+	const char *clk_name = NULL;
 	int ret = -ENODEV;
 
 	regmap = dev_get_regmap(parent, NULL);
@@ -61,7 +62,9 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 	if (!a53cc)
 		return -ENOMEM;
 
-	init.name = "a53mux";
+	of_property_read_string(parent->of_node, "clock-output-names",
+				&clk_name);
+	init.name = clk_name ? clk_name : "a53mux";
 	init.parent_data = pdata;
 	init.num_parents = ARRAY_SIZE(pdata);
 	init.ops = &clk_regmap_mux_div_ops;
-- 
2.17.1


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

* [PATCH 4/5] clk: qcom: a53-pll: Pass freq_tbl via match data
  2021-05-04  5:28 [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support Shawn Guo
                   ` (2 preceding siblings ...)
  2021-05-04  5:28 ` [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT Shawn Guo
@ 2021-05-04  5:28 ` Shawn Guo
  2021-05-04  5:28 ` [PATCH 5/5] clk: qcom: a53-pll: Add MSM8939 a53pll clocks Shawn Guo
  2021-06-21  6:37 ` [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support Shawn Guo
  5 siblings, 0 replies; 17+ messages in thread
From: Shawn Guo @ 2021-05-04  5:28 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk, Shawn Guo

The frequency table is SoC specific.  Instead of hard coding, pass it
via match data, so that the driver can work for more than just MSM8916.
This is a preparation change for adding MSM8939 A53PLL support.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/qcom/a53-pll.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
index 964f5ab7d02f..bfa048dc01ec 100644
--- a/drivers/clk/qcom/a53-pll.c
+++ b/drivers/clk/qcom/a53-pll.c
@@ -15,7 +15,7 @@
 #include "clk-pll.h"
 #include "clk-regmap.h"
 
-static const struct pll_freq_tbl a53pll_freq[] = {
+static const struct pll_freq_tbl msm8916_freq[] = {
 	{  998400000, 52, 0x0, 0x1, 0 },
 	{ 1094400000, 57, 0x0, 0x1, 0 },
 	{ 1152000000, 62, 0x0, 0x1, 0 },
@@ -43,8 +43,13 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
 	void __iomem *base;
 	struct clk_init_data init = { };
 	const char *clk_name = NULL;
+	const struct pll_freq_tbl *freq_tbl;
 	int ret;
 
+	freq_tbl = device_get_match_data(&pdev->dev);
+	if (!freq_tbl)
+		return -ENODEV;
+
 	pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
 	if (!pll)
 		return -ENOMEM;
@@ -65,7 +70,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
 	pll->mode_reg = 0x00;
 	pll->status_reg = 0x1c;
 	pll->status_bit = 16;
-	pll->freq_tbl = a53pll_freq;
+	pll->freq_tbl = freq_tbl;
 
 	of_property_read_string(pdev->dev.of_node, "clock-output-names",
 				&clk_name);
@@ -92,7 +97,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_a53pll_match_table[] = {
-	{ .compatible = "qcom,msm8916-a53pll" },
+	{ .compatible = "qcom,msm8916-a53pll", .data = msm8916_freq },
 	{ }
 };
 
-- 
2.17.1


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

* [PATCH 5/5] clk: qcom: a53-pll: Add MSM8939 a53pll clocks
  2021-05-04  5:28 [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support Shawn Guo
                   ` (3 preceding siblings ...)
  2021-05-04  5:28 ` [PATCH 4/5] clk: qcom: a53-pll: Pass freq_tbl via match data Shawn Guo
@ 2021-05-04  5:28 ` Shawn Guo
  2021-06-28  0:24   ` Stephen Boyd
  2021-06-21  6:37 ` [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support Shawn Guo
  5 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2021-05-04  5:28 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk, Shawn Guo

It adds support for MSM8939 a53pll clock of Cluster0, Cluster1 and CCI
(Cache Coherent Interconnect).  The frequency data comes from vendor
kernel.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/qcom/a53-pll.c | 42 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
index bfa048dc01ec..8c6f8bcc6128 100644
--- a/drivers/clk/qcom/a53-pll.c
+++ b/drivers/clk/qcom/a53-pll.c
@@ -26,6 +26,45 @@ static const struct pll_freq_tbl msm8916_freq[] = {
 	{ }
 };
 
+static const struct pll_freq_tbl msm8939_c0_freq[] = {
+	{  998400000,  52, 0x0, 0x1, 0 },
+	{ 1113600000,  58, 0x0, 0x1, 0 },
+	{ 1209600000,  63, 0x0, 0x1, 0 },
+};
+
+static const struct pll_freq_tbl msm8939_c1_freq[] = {
+	{  652800000, 34, 0x0, 0x1, 0 },
+	{  691200000, 36, 0x0, 0x1, 0 },
+	{  729600000, 38, 0x0, 0x1, 0 },
+	{  806400000, 42, 0x0, 0x1, 0 },
+	{  844800000, 44, 0x0, 0x1, 0 },
+	{  883200000, 46, 0x0, 0x1, 0 },
+	{  960000000, 50, 0x0, 0x1, 0 },
+	{  998400000, 52, 0x0, 0x1, 0 },
+	{ 1036800000, 54, 0x0, 0x1, 0 },
+	{ 1113600000, 58, 0x0, 0x1, 0 },
+	{ 1209600000, 63, 0x0, 0x1, 0 },
+	{ 1190400000, 62, 0x0, 0x1, 0 },
+	{ 1267200000, 66, 0x0, 0x1, 0 },
+	{ 1344000000, 70, 0x0, 0x1, 0 },
+	{ 1363200000, 71, 0x0, 0x1, 0 },
+	{ 1420800000, 74, 0x0, 0x1, 0 },
+	{ 1459200000, 76, 0x0, 0x1, 0 },
+	{ 1497600000, 78, 0x0, 0x1, 0 },
+	{ 1536000000, 80, 0x0, 0x1, 0 },
+	{ 1574400000, 82, 0x0, 0x1, 0 },
+	{ 1612800000, 84, 0x0, 0x1, 0 },
+	{ 1632000000, 85, 0x0, 0x1, 0 },
+	{ 1651200000, 86, 0x0, 0x1, 0 },
+	{ 1689600000, 88, 0x0, 0x1, 0 },
+	{ 1708800000, 89, 0x0, 0x1, 0 },
+};
+
+static const struct pll_freq_tbl msm8939_cci_freq[] = {
+	{ 403200000, 21, 0x0, 0x1, 0 },
+	{ 595200000, 31, 0x0, 0x1, 0 },
+};
+
 static const struct regmap_config a53pll_regmap_config = {
 	.reg_bits		= 32,
 	.reg_stride		= 4,
@@ -98,6 +137,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
 
 static const struct of_device_id qcom_a53pll_match_table[] = {
 	{ .compatible = "qcom,msm8916-a53pll", .data = msm8916_freq },
+	{ .compatible = "qcom,msm8939-a53pll-c0", .data = &msm8939_c0_freq },
+	{ .compatible = "qcom,msm8939-a53pll-c1", .data = &msm8939_c1_freq },
+	{ .compatible = "qcom,msm8939-a53pll-cci", .data = &msm8939_cci_freq },
 	{ }
 };
 
-- 
2.17.1


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

* Re: [PATCH 2/5] dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support
  2021-05-04  5:28 ` [PATCH 2/5] dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support Shawn Guo
@ 2021-05-06 20:27   ` Rob Herring
  2021-05-07  0:18     ` Shawn Guo
  0 siblings, 1 reply; 17+ messages in thread
From: Rob Herring @ 2021-05-06 20:27 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Stephen Boyd, Bjorn Andersson, Sivaprakash Murugesan,
	Benjamin Li, devicetree, linux-arm-msm, linux-clk

On Tue, May 04, 2021 at 01:28:41PM +0800, Shawn Guo wrote:
> Update qcom,a53pll bindings for MSM8939 support:
> 
>  - Add optional clock-output-names property.
>  - Add MSM8939 specific compatibles.
>  - Add MSM8939 examples.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  .../bindings/clock/qcom,a53pll.yaml           | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml b/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
> index db3d0ea6bc7a..7a410a76be2f 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
> @@ -18,6 +18,9 @@ properties:
>      enum:
>        - qcom,ipq6018-a53pll
>        - qcom,msm8916-a53pll
> +      - qcom,msm8939-a53pll-c0
> +      - qcom,msm8939-a53pll-c1
> +      - qcom,msm8939-a53pll-cci

These 3 have differences?

>  
>    reg:
>      maxItems: 1
> @@ -33,6 +36,9 @@ properties:
>      items:
>        - const: xo
>  
> +  clock-output-names:
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
> @@ -57,3 +63,31 @@ examples:
>          clocks = <&xo>;
>          clock-names = "xo";
>      };
> +  #Example 3 - A53 PLLs found on MSM8939 devices
> +  - |
> +    a53pll_c1: clock-controller@b016000 {
> +        compatible = "qcom,msm8939-a53pll-c1";
> +        reg = <0xb016000 0x40>;
> +        #clock-cells = <0>;
> +        clocks = <&xo_board>;
> +        clock-names = "xo";
> +        clock-output-names = "a53pll_c1";
> +    };
> +
> +    a53pll_c0: clock-controller@b116000 {
> +        compatible = "qcom,msm8939-a53pll-c0";
> +        reg = <0xb116000 0x40>;
> +        #clock-cells = <0>;
> +        clocks = <&xo_board>;
> +        clock-names = "xo";
> +        clock-output-names = "a53pll_c0";
> +    };
> +
> +    a53pll_cci: clock-controller@b1d0000 {
> +        compatible = "qcom,msm8939-a53pll-cci";
> +        reg = <0xb1d0000 0x40>;
> +        #clock-cells = <0>;
> +        clocks = <&xo_board>;
> +        clock-names = "xo";
> +        clock-output-names = "a53pll_cci";
> +    };

Do these examples really add anything?

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

* Re: [PATCH 2/5] dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support
  2021-05-06 20:27   ` Rob Herring
@ 2021-05-07  0:18     ` Shawn Guo
  0 siblings, 0 replies; 17+ messages in thread
From: Shawn Guo @ 2021-05-07  0:18 UTC (permalink / raw)
  To: Rob Herring
  Cc: Stephen Boyd, Bjorn Andersson, Sivaprakash Murugesan,
	Benjamin Li, devicetree, linux-arm-msm, linux-clk

On Thu, May 06, 2021 at 03:27:26PM -0500, Rob Herring wrote:
> On Tue, May 04, 2021 at 01:28:41PM +0800, Shawn Guo wrote:
> > Update qcom,a53pll bindings for MSM8939 support:
> > 
> >  - Add optional clock-output-names property.
> >  - Add MSM8939 specific compatibles.
> >  - Add MSM8939 examples.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  .../bindings/clock/qcom,a53pll.yaml           | 34 +++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml b/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
> > index db3d0ea6bc7a..7a410a76be2f 100644
> > --- a/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
> > +++ b/Documentation/devicetree/bindings/clock/qcom,a53pll.yaml
> > @@ -18,6 +18,9 @@ properties:
> >      enum:
> >        - qcom,ipq6018-a53pll
> >        - qcom,msm8916-a53pll
> > +      - qcom,msm8939-a53pll-c0
> > +      - qcom,msm8939-a53pll-c1
> > +      - qcom,msm8939-a53pll-cci
> 
> These 3 have differences?

Yes.  They need to hook up with different frequency table as shown in
patch #5.

> 
> >  
> >    reg:
> >      maxItems: 1
> > @@ -33,6 +36,9 @@ properties:
> >      items:
> >        - const: xo
> >  
> > +  clock-output-names:
> > +    maxItems: 1
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -57,3 +63,31 @@ examples:
> >          clocks = <&xo>;
> >          clock-names = "xo";
> >      };
> > +  #Example 3 - A53 PLLs found on MSM8939 devices
> > +  - |
> > +    a53pll_c1: clock-controller@b016000 {
> > +        compatible = "qcom,msm8939-a53pll-c1";
> > +        reg = <0xb016000 0x40>;
> > +        #clock-cells = <0>;
> > +        clocks = <&xo_board>;
> > +        clock-names = "xo";
> > +        clock-output-names = "a53pll_c1";
> > +    };
> > +
> > +    a53pll_c0: clock-controller@b116000 {
> > +        compatible = "qcom,msm8939-a53pll-c0";
> > +        reg = <0xb116000 0x40>;
> > +        #clock-cells = <0>;
> > +        clocks = <&xo_board>;
> > +        clock-names = "xo";
> > +        clock-output-names = "a53pll_c0";
> > +    };
> > +
> > +    a53pll_cci: clock-controller@b1d0000 {
> > +        compatible = "qcom,msm8939-a53pll-cci";
> > +        reg = <0xb1d0000 0x40>;
> > +        #clock-cells = <0>;
> > +        clocks = <&xo_board>;
> > +        clock-names = "xo";
> > +        clock-output-names = "a53pll_cci";
> > +    };
> 
> Do these examples really add anything?

The example was added to demonstrate that multiple A53PLL clock
controllers could be found on a single SoC.  But I'm happy to drop it
if you think it's not so useful.

Shawn

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

* Re: [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support
  2021-05-04  5:28 [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support Shawn Guo
                   ` (4 preceding siblings ...)
  2021-05-04  5:28 ` [PATCH 5/5] clk: qcom: a53-pll: Add MSM8939 a53pll clocks Shawn Guo
@ 2021-06-21  6:37 ` Shawn Guo
  5 siblings, 0 replies; 17+ messages in thread
From: Shawn Guo @ 2021-06-21  6:37 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk

On Tue, May 04, 2021 at 01:28:39PM +0800, Shawn Guo wrote:
> This series adds MSM8939 APCS/A53PLL clock support.  Most outstanding
> thing about MSM8939 is that it integrates 3 APCS instances, for Cluster0
> (little cores), Cluster1 (big cores) and CCI (Cache Coherent Interconnect)
> respectively.
> 
> Note: the first one is a small improvement which is not specific to
> MSM8939 support.
> 
> 
> Shawn Guo (5):
>   clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical
>   dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support
>   clk: qcom: apcs-msm8916: Retrieve clock name from DT
>   clk: qcom: a53-pll: Pass freq_tbl via match data
>   clk: qcom: a53-pll: Add MSM8939 a53pll clocks
> 
>  .../bindings/clock/qcom,a53pll.yaml           | 34 +++++++++++
>  drivers/clk/qcom/a53-pll.c                    | 59 +++++++++++++++++--
>  drivers/clk/qcom/apcs-msm8916.c               |  7 ++-
>  3 files changed, 93 insertions(+), 7 deletions(-)

Hi Stephen,

Did you get a chance to look at the series?

Shawn

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

* Re: [PATCH 5/5] clk: qcom: a53-pll: Add MSM8939 a53pll clocks
  2021-05-04  5:28 ` [PATCH 5/5] clk: qcom: a53-pll: Add MSM8939 a53pll clocks Shawn Guo
@ 2021-06-28  0:24   ` Stephen Boyd
  0 siblings, 0 replies; 17+ messages in thread
From: Stephen Boyd @ 2021-06-28  0:24 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk, Shawn Guo

Quoting Shawn Guo (2021-05-03 22:28:44)
> diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> index bfa048dc01ec..8c6f8bcc6128 100644
> --- a/drivers/clk/qcom/a53-pll.c
> +++ b/drivers/clk/qcom/a53-pll.c
[..]
> +       { 1651200000, 86, 0x0, 0x1, 0 },
> +       { 1689600000, 88, 0x0, 0x1, 0 },
> +       { 1708800000, 89, 0x0, 0x1, 0 },
> +};
> +
> +static const struct pll_freq_tbl msm8939_cci_freq[] = {
> +       { 403200000, 21, 0x0, 0x1, 0 },
> +       { 595200000, 31, 0x0, 0x1, 0 },
> +};
> +
>  static const struct regmap_config a53pll_regmap_config = {
>         .reg_bits               = 32,
>         .reg_stride             = 4,
> @@ -98,6 +137,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
>  
>  static const struct of_device_id qcom_a53pll_match_table[] = {
>         { .compatible = "qcom,msm8916-a53pll", .data = msm8916_freq },
> +       { .compatible = "qcom,msm8939-a53pll-c0", .data = &msm8939_c0_freq },
> +       { .compatible = "qcom,msm8939-a53pll-c1", .data = &msm8939_c1_freq },
> +       { .compatible = "qcom,msm8939-a53pll-cci", .data = &msm8939_cci_freq },

Can we push these compatibles and tables into an OPP table? Then the
frequency plan would be an opp-table binding and the driver can drive
the multiplier on XO (probably l_val) without having to hardcode it here
in the driver. It does mean we spend a bit more time at probe detecting
the frequency plan, but it would at least avoid the concern that Rob has
with multiple compatibles and probably make it easier to handle CPU
frequencies anyway.

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

* Re: [PATCH 1/5] clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical
  2021-05-04  5:28 ` [PATCH 1/5] clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical Shawn Guo
@ 2021-06-28  0:27   ` Stephen Boyd
  2021-06-29  2:37     ` Shawn Guo
  0 siblings, 1 reply; 17+ messages in thread
From: Stephen Boyd @ 2021-06-28  0:27 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk, Shawn Guo

Quoting Shawn Guo (2021-05-03 22:28:40)
> The clock source for MSM8916 cpu cores is like below.
> 
>                         |\
>          a53pll --------| \ a53mux     +------+
>                         | |------------| cpus |
>      gpll0_vote --------| /            +------+
>                         |/
> 
> So clock a53mux rather than a53pll is actually the clock source of cpu
> cores.  It makes more sense to flag a53mux rather than a53pll as
> critical, since a53pll could be irrelevant if a53mux switches its parent
> clock to be gpll0_vote.

Can you add some more detail here? I think the idea is to mark the mux
as critical so that either a53pll or gpll0_vote is kept enabled, but
only if they're used by the CPU. That isn't very clear from the commit
text. Otherwise it seems OK.

>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/clk/qcom/a53-pll.c      | 1 -
>  drivers/clk/qcom/apcs-msm8916.c | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> index 45cfc57bff92..8614b0b0e82c 100644
> --- a/drivers/clk/qcom/a53-pll.c
> +++ b/drivers/clk/qcom/a53-pll.c
> @@ -70,7 +70,6 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
>         init.parent_names = (const char *[]){ "xo" };
>         init.num_parents = 1;
>         init.ops = &clk_pll_sr2_ops;
> -       init.flags = CLK_IS_CRITICAL;
>         pll->clkr.hw.init = &init;
>  
>         ret = devm_clk_register_regmap(dev, &pll->clkr);
> diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
> index cf69a97d0439..d7ac6d6b15b6 100644
> --- a/drivers/clk/qcom/apcs-msm8916.c
> +++ b/drivers/clk/qcom/apcs-msm8916.c
> @@ -65,7 +65,7 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
>         init.parent_data = pdata;
>         init.num_parents = ARRAY_SIZE(pdata);
>         init.ops = &clk_regmap_mux_div_ops;
> -       init.flags = CLK_SET_RATE_PARENT;
> +       init.flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT;
>  
>         a53cc->clkr.hw.init = &init;
>         a53cc->clkr.regmap = regmap;
> -- 
> 2.17.1
>

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

* Re: [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT
  2021-05-04  5:28 ` [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT Shawn Guo
@ 2021-06-28  0:28   ` Stephen Boyd
  2021-06-29 13:36     ` Shawn Guo
  0 siblings, 1 reply; 17+ messages in thread
From: Stephen Boyd @ 2021-06-28  0:28 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk, Shawn Guo

Quoting Shawn Guo (2021-05-03 22:28:42)
> Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for
> Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent
> Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,
> retrieve the name from DT, so that multiple APCS clocks can be
> registered.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/clk/qcom/a53-pll.c      | 5 ++++-
>  drivers/clk/qcom/apcs-msm8916.c | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> index 8614b0b0e82c..964f5ab7d02f 100644
> --- a/drivers/clk/qcom/a53-pll.c
> +++ b/drivers/clk/qcom/a53-pll.c
> @@ -42,6 +42,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
>         struct clk_pll *pll;
>         void __iomem *base;
>         struct clk_init_data init = { };
> +       const char *clk_name = NULL;
>         int ret;
>  
>         pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> @@ -66,7 +67,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
>         pll->status_bit = 16;
>         pll->freq_tbl = a53pll_freq;
>  
> -       init.name = "a53pll";
> +       of_property_read_string(pdev->dev.of_node, "clock-output-names",
> +                               &clk_name);

Please no? Is there any use for this? Why not just generate the name as
a53pll@<MMIO ADDRESS>?

> +       init.name = clk_name ? clk_name : "a53pll";
>         init.parent_names = (const char *[]){ "xo" };
>         init.num_parents = 1;
>         init.ops = &clk_pll_sr2_ops;

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

* Re: [PATCH 1/5] clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical
  2021-06-28  0:27   ` Stephen Boyd
@ 2021-06-29  2:37     ` Shawn Guo
  0 siblings, 0 replies; 17+ messages in thread
From: Shawn Guo @ 2021-06-29  2:37 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk

On Sun, Jun 27, 2021 at 05:27:41PM -0700, Stephen Boyd wrote:
> Quoting Shawn Guo (2021-05-03 22:28:40)
> > The clock source for MSM8916 cpu cores is like below.
> > 
> >                         |\
> >          a53pll --------| \ a53mux     +------+
> >                         | |------------| cpus |
> >      gpll0_vote --------| /            +------+
> >                         |/
> > 
> > So clock a53mux rather than a53pll is actually the clock source of cpu
> > cores.  It makes more sense to flag a53mux rather than a53pll as
> > critical, since a53pll could be irrelevant if a53mux switches its parent
> > clock to be gpll0_vote.
> 
> Can you add some more detail here? I think the idea is to mark the mux
> as critical so that either a53pll or gpll0_vote is kept enabled, but
> only if they're used by the CPU. That isn't very clear from the commit
> text. Otherwise it seems OK.

Sure.  I will rewrite the commit log.

Shawn

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

* Re: [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT
  2021-06-28  0:28   ` Stephen Boyd
@ 2021-06-29 13:36     ` Shawn Guo
  2021-06-29 15:57       ` Bjorn Andersson
  0 siblings, 1 reply; 17+ messages in thread
From: Shawn Guo @ 2021-06-29 13:36 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk

On Sun, Jun 27, 2021 at 05:28:34PM -0700, Stephen Boyd wrote:
> Quoting Shawn Guo (2021-05-03 22:28:42)
> > Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for
> > Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent
> > Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,
> > retrieve the name from DT, so that multiple APCS clocks can be
> > registered.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  drivers/clk/qcom/a53-pll.c      | 5 ++++-
> >  drivers/clk/qcom/apcs-msm8916.c | 5 ++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> > index 8614b0b0e82c..964f5ab7d02f 100644
> > --- a/drivers/clk/qcom/a53-pll.c
> > +++ b/drivers/clk/qcom/a53-pll.c
> > @@ -42,6 +42,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> >         struct clk_pll *pll;
> >         void __iomem *base;
> >         struct clk_init_data init = { };
> > +       const char *clk_name = NULL;
> >         int ret;
> >  
> >         pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> > @@ -66,7 +67,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> >         pll->status_bit = 16;
> >         pll->freq_tbl = a53pll_freq;
> >  
> > -       init.name = "a53pll";
> > +       of_property_read_string(pdev->dev.of_node, "clock-output-names",
> > +                               &clk_name);
> 
> Please no? Is there any use for this? Why not just generate the name as
> a53pll@<MMIO ADDRESS>?

There is no other use for this than getting different names.  I will do
what you suggest here.  Thanks!

Shawn

> 
> > +       init.name = clk_name ? clk_name : "a53pll";
> >         init.parent_names = (const char *[]){ "xo" };
> >         init.num_parents = 1;
> >         init.ops = &clk_pll_sr2_ops;

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

* Re: [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT
  2021-06-29 13:36     ` Shawn Guo
@ 2021-06-29 15:57       ` Bjorn Andersson
  2021-06-29 20:23         ` Stephen Boyd
  0 siblings, 1 reply; 17+ messages in thread
From: Bjorn Andersson @ 2021-06-29 15:57 UTC (permalink / raw)
  To: Shawn Guo
  Cc: Stephen Boyd, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk

On Tue 29 Jun 08:36 CDT 2021, Shawn Guo wrote:

> On Sun, Jun 27, 2021 at 05:28:34PM -0700, Stephen Boyd wrote:
> > Quoting Shawn Guo (2021-05-03 22:28:42)
> > > Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for
> > > Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent
> > > Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,
> > > retrieve the name from DT, so that multiple APCS clocks can be
> > > registered.
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > ---
> > >  drivers/clk/qcom/a53-pll.c      | 5 ++++-
> > >  drivers/clk/qcom/apcs-msm8916.c | 5 ++++-
> > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> > > index 8614b0b0e82c..964f5ab7d02f 100644
> > > --- a/drivers/clk/qcom/a53-pll.c
> > > +++ b/drivers/clk/qcom/a53-pll.c
> > > @@ -42,6 +42,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> > >         struct clk_pll *pll;
> > >         void __iomem *base;
> > >         struct clk_init_data init = { };
> > > +       const char *clk_name = NULL;
> > >         int ret;
> > >  
> > >         pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> > > @@ -66,7 +67,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> > >         pll->status_bit = 16;
> > >         pll->freq_tbl = a53pll_freq;
> > >  
> > > -       init.name = "a53pll";
> > > +       of_property_read_string(pdev->dev.of_node, "clock-output-names",
> > > +                               &clk_name);
> > 
> > Please no? Is there any use for this? Why not just generate the name as
> > a53pll@<MMIO ADDRESS>?
> 
> There is no other use for this than getting different names.  I will do
> what you suggest here.  Thanks!
> 

I have exactly the same problem with my two DP PHYs (in
phy_dp_clks_register()), so I'm in favor of us setting some sort of
standard for this (not for anyone to rely on, but to avoid everyone
coming up with their own scheme).

But unfortunately I don't have easy access to the phy block's base
address in phy_dp_clks_register().

Regards,
Bjorn

> Shawn
> 
> > 
> > > +       init.name = clk_name ? clk_name : "a53pll";
> > >         init.parent_names = (const char *[]){ "xo" };
> > >         init.num_parents = 1;
> > >         init.ops = &clk_pll_sr2_ops;

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

* Re: [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT
  2021-06-29 15:57       ` Bjorn Andersson
@ 2021-06-29 20:23         ` Stephen Boyd
  2021-06-29 20:39           ` Bjorn Andersson
  0 siblings, 1 reply; 17+ messages in thread
From: Stephen Boyd @ 2021-06-29 20:23 UTC (permalink / raw)
  To: Bjorn Andersson, Shawn Guo
  Cc: Rob Herring, Sivaprakash Murugesan, Benjamin Li, devicetree,
	linux-arm-msm, linux-clk

Quoting Bjorn Andersson (2021-06-29 08:57:29)
> On Tue 29 Jun 08:36 CDT 2021, Shawn Guo wrote:
> 
> > On Sun, Jun 27, 2021 at 05:28:34PM -0700, Stephen Boyd wrote:
> > > Quoting Shawn Guo (2021-05-03 22:28:42)
> > > > Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for
> > > > Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent
> > > > Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,
> > > > retrieve the name from DT, so that multiple APCS clocks can be
> > > > registered.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > ---
> > > >  drivers/clk/qcom/a53-pll.c      | 5 ++++-
> > > >  drivers/clk/qcom/apcs-msm8916.c | 5 ++++-
> > > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> > > > index 8614b0b0e82c..964f5ab7d02f 100644
> > > > --- a/drivers/clk/qcom/a53-pll.c
> > > > +++ b/drivers/clk/qcom/a53-pll.c
> > > > @@ -42,6 +42,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> > > >         struct clk_pll *pll;
> > > >         void __iomem *base;
> > > >         struct clk_init_data init = { };
> > > > +       const char *clk_name = NULL;
> > > >         int ret;
> > > >  
> > > >         pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> > > > @@ -66,7 +67,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> > > >         pll->status_bit = 16;
> > > >         pll->freq_tbl = a53pll_freq;
> > > >  
> > > > -       init.name = "a53pll";
> > > > +       of_property_read_string(pdev->dev.of_node, "clock-output-names",
> > > > +                               &clk_name);
> > > 
> > > Please no? Is there any use for this? Why not just generate the name as
> > > a53pll@<MMIO ADDRESS>?
> > 
> > There is no other use for this than getting different names.  I will do
> > what you suggest here.  Thanks!
> > 
> 
> I have exactly the same problem with my two DP PHYs (in
> phy_dp_clks_register()), so I'm in favor of us setting some sort of
> standard for this (not for anyone to rely on, but to avoid everyone
> coming up with their own scheme).
> 
> But unfortunately I don't have easy access to the phy block's base
> address in phy_dp_clks_register().

It really doesn't matter what name you use as it's basically only for
debugging. The problem is uniqueness. I've wondered if leaving the name
as NULL and then passing in a dev would be sufficient to generate a clk
name at runtime. Basically dev_name() plus an incrementing global
numberspace would probably work fine. Debugging would be annoying in
that case, but maybe it wouldn't matter.

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

* Re: [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT
  2021-06-29 20:23         ` Stephen Boyd
@ 2021-06-29 20:39           ` Bjorn Andersson
  0 siblings, 0 replies; 17+ messages in thread
From: Bjorn Andersson @ 2021-06-29 20:39 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Shawn Guo, Rob Herring, Sivaprakash Murugesan, Benjamin Li,
	devicetree, linux-arm-msm, linux-clk

On Tue 29 Jun 15:23 CDT 2021, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2021-06-29 08:57:29)
> > On Tue 29 Jun 08:36 CDT 2021, Shawn Guo wrote:
> > 
> > > On Sun, Jun 27, 2021 at 05:28:34PM -0700, Stephen Boyd wrote:
> > > > Quoting Shawn Guo (2021-05-03 22:28:42)
> > > > > Unlike MSM8916 which has only one APCS clock, MSM8939 gets three for
> > > > > Cluster0 (little cores), Cluster1 (big cores) and CCI (Cache Coherent
> > > > > Interconnect).  Instead of hard coding APCS (and A53PLL) clock name,
> > > > > retrieve the name from DT, so that multiple APCS clocks can be
> > > > > registered.
> > > > > 
> > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > > > > ---
> > > > >  drivers/clk/qcom/a53-pll.c      | 5 ++++-
> > > > >  drivers/clk/qcom/apcs-msm8916.c | 5 ++++-
> > > > >  2 files changed, 8 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> > > > > index 8614b0b0e82c..964f5ab7d02f 100644
> > > > > --- a/drivers/clk/qcom/a53-pll.c
> > > > > +++ b/drivers/clk/qcom/a53-pll.c
> > > > > @@ -42,6 +42,7 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> > > > >         struct clk_pll *pll;
> > > > >         void __iomem *base;
> > > > >         struct clk_init_data init = { };
> > > > > +       const char *clk_name = NULL;
> > > > >         int ret;
> > > > >  
> > > > >         pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> > > > > @@ -66,7 +67,9 @@ static int qcom_a53pll_probe(struct platform_device *pdev)
> > > > >         pll->status_bit = 16;
> > > > >         pll->freq_tbl = a53pll_freq;
> > > > >  
> > > > > -       init.name = "a53pll";
> > > > > +       of_property_read_string(pdev->dev.of_node, "clock-output-names",
> > > > > +                               &clk_name);
> > > > 
> > > > Please no? Is there any use for this? Why not just generate the name as
> > > > a53pll@<MMIO ADDRESS>?
> > > 
> > > There is no other use for this than getting different names.  I will do
> > > what you suggest here.  Thanks!
> > > 
> > 
> > I have exactly the same problem with my two DP PHYs (in
> > phy_dp_clks_register()), so I'm in favor of us setting some sort of
> > standard for this (not for anyone to rely on, but to avoid everyone
> > coming up with their own scheme).
> > 
> > But unfortunately I don't have easy access to the phy block's base
> > address in phy_dp_clks_register().
> 
> It really doesn't matter what name you use as it's basically only for
> debugging. The problem is uniqueness. I've wondered if leaving the name
> as NULL and then passing in a dev would be sufficient to generate a clk
> name at runtime. Basically dev_name() plus an incrementing global
> numberspace would probably work fine. Debugging would be annoying in
> that case, but maybe it wouldn't matter.

Something like "%s:link" and "%s:vco_div" based on dev_name() would be
quite nice in the case of DP. Probably more enjoyable to read than
dev_name():N and dev_name():N+1.

It comes with a cost of a few extra lines of code in each driver, but if
it's only a few drivers I don't think it warrants the extra logic in the
core.

Regards,
Bjorn

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

end of thread, other threads:[~2021-06-29 20:39 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-04  5:28 [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support Shawn Guo
2021-05-04  5:28 ` [PATCH 1/5] clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical Shawn Guo
2021-06-28  0:27   ` Stephen Boyd
2021-06-29  2:37     ` Shawn Guo
2021-05-04  5:28 ` [PATCH 2/5] dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support Shawn Guo
2021-05-06 20:27   ` Rob Herring
2021-05-07  0:18     ` Shawn Guo
2021-05-04  5:28 ` [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT Shawn Guo
2021-06-28  0:28   ` Stephen Boyd
2021-06-29 13:36     ` Shawn Guo
2021-06-29 15:57       ` Bjorn Andersson
2021-06-29 20:23         ` Stephen Boyd
2021-06-29 20:39           ` Bjorn Andersson
2021-05-04  5:28 ` [PATCH 4/5] clk: qcom: a53-pll: Pass freq_tbl via match data Shawn Guo
2021-05-04  5:28 ` [PATCH 5/5] clk: qcom: a53-pll: Add MSM8939 a53pll clocks Shawn Guo
2021-06-28  0:24   ` Stephen Boyd
2021-06-21  6:37 ` [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support Shawn Guo

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