[PATCH 0/5] Add MSM8939 APCS/A53PLL clock support

[PATCH 0/5] Add MSM8939 APCS/A53PLL clock support

[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

[PATCH 1/5] clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical

[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

Re: [PATCH 1/5] clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical

[Stephen Boyd]

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
>

Re: [PATCH 1/5] clk: qcom: apcs-msm8916: Flag a53mux instead of a53pll as critical

[Shawn Guo]

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

[PATCH 2/5] dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support

[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

Re: [PATCH 2/5] dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support

[Rob Herring]

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?

Re: [PATCH 2/5] dt-bindings: clock: update qcom,a53pll bindings for MSM8939 support

[Shawn Guo]

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

[PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT

[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

Re: [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT

[Stephen Boyd]

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;

Re: [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT

[Shawn Guo]

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;

Re: [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT

[Bjorn Andersson]

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;

Re: [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT

[Stephen Boyd]

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.

Re: [PATCH 3/5] clk: qcom: apcs-msm8916: Retrieve clock name from DT

[Bjorn Andersson]

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

[PATCH 4/5] clk: qcom: a53-pll: Pass freq_tbl via match data

[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

[PATCH 5/5] clk: qcom: a53-pll: Add MSM8939 a53pll clocks

[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

Re: [PATCH 5/5] clk: qcom: a53-pll: Add MSM8939 a53pll clocks

[Stephen Boyd]

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.

Re: [PATCH 0/5] Add MSM8939 APCS/A53PLL clock support

[Shawn Guo]

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