[PATCH v2 00/10] clk: qcom: Fix parenting for dispcc/gpucc/videocc

[PATCH v2 00/10] clk: qcom: Fix parenting for dispcc/gpucc/videocc

[Douglas Anderson]

The aim of this series is to get the dispcc and gpucc in a workable
shape upstream for sc7180.  I personally wasn't focusing on (and
didn't test) videocc but pulled it along for the ride.

Most of the work in this series deals with the fact that the parenting
info for these clock controllers was in a bad shape.  It looks like it
was half transitioned from the old way of doing things (relying on
global names) to the new way of doing things (putting the linkage in
the device tree).  This should fully transition us.

As part of this transition I update the sdm845.dtsi file to specify
the info as per the new way of doing things.  Although I've now put
the linkage info in the sdm845.dtsi file, though, I haven't updated
the sdm845 clock drivers in Linux so they still work via the global
name matching.  It's left as an exercise to the reader to update the
sdm845 clock drivers in Linux.

This series passes these things for me on linux-next:

  ARCH=arm64 make dtbs_check \
    DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
  ARCH=arm64 make dtbs_check \
    DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/qcom,dispcc.yaml
  ARCH=arm64 make dtbs_check \
    DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/qcom,videocc.yaml
  ARCH=arm64 make dt_binding_check \
    DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/qcom,videocc.yaml
  ARCH=arm64 make dt_binding_check \
    DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
  ARCH=arm64 make dt_binding_check \
    DT_SCHEMA_FILES=Documentation/devicetree/bindings/clock/qcom,dispcc.yaml

I have confirmed that (with extra patches) the display/gpu come up on
sc7180 and sdm845-cheza.  You can find the top of my downstream tree at:
  https://crrev.com/c/2017976/3

I have confirmed that sdm845-cheza display / GPU come up atop
next-20200124, which is what this series is posted against.

This series is marked as 'v2' because in it I have snarfed up Taniya's
dts patch adding the clock controller nodes to sc7180.dtsi and this is
"v2" of that patch.  Everything else is brand new.

Changes in v2:
- Patch ("clk: qcom: rcg2: Don't crash...") new for v2.
- Patch ("dt-bindings: clock: Fix qcom,dispcc...") new for v2.
- Patch ("arm64: dts: qcom: sdm845: Add...dispcc") new for v2.
- Patch ("dt-bindings: clock: Fix qcom,gpucc...") new for v2.
- Patch ("clk: qcom: Fix sc7180 dispcc parent data") new for v2.
- Patch ("arm64: dts: qcom: sdm845: Add...gpucc") new for v2.
- Patch ("clk: qcom: Fix sc7180 gpucc parent data") new for v2.
- Patch ("dt-bindings: clock: Cleanup qcom,videocc") new for v2.
- Patch ("arm64: dts: qcom: sdm845: Add...videocc") new for v2.
- Added includes
- Changed various parent names to match bindings / driver

Douglas Anderson (9):
  clk: qcom: rcg2: Don't crash if our parent can't be found; return an
    error
  dt-bindings: clock: Fix qcom,dispcc bindings for sdm845/sc7180
  arm64: dts: qcom: sdm845: Add the missing clocks on the dispcc
  dt-bindings: clock: Fix qcom,gpucc bindings for sdm845/sc7180/msm8998
  clk: qcom: Fix sc7180 dispcc parent data
  arm64: dts: qcom: sdm845: Add the missing clocks on the gpucc
  clk: qcom: Fix sc7180 gpucc parent data
  dt-bindings: clock: Cleanup qcom,videocc bindings for sdm845/sc7180
  arm64: dts: qcom: sdm845: Add the missing clock on the videocc

Taniya Das (1):
  arm64: dts: sc7180: Add clock controller nodes

 .../bindings/clock/qcom,dispcc.yaml           | 87 +++++++++++++++----
 .../devicetree/bindings/clock/qcom,gpucc.yaml | 42 ++++++---
 .../bindings/clock/qcom,videocc.yaml          | 10 ++-
 arch/arm64/boot/dts/qcom/sc7180.dtsi          | 41 +++++++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi          | 20 ++++-
 drivers/clk/qcom/clk-rcg2.c                   |  3 +
 drivers/clk/qcom/dispcc-sc7180.c              | 63 +++++---------
 drivers/clk/qcom/gpucc-sc7180.c               | 11 ++-
 8 files changed, 199 insertions(+), 78 deletions(-)

-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 01/10] clk: qcom: rcg2: Don't crash if our parent can't be found; return an error

[Douglas Anderson]

When I got my clock parenting slightly wrong I ended up with a crash
that looked like this:

  Unable to handle kernel NULL pointer dereference at virtual
  address 0000000000000000
  ...
  pc : clk_hw_get_rate+0x14/0x44
  ...
  Call trace:
   clk_hw_get_rate+0x14/0x44
   _freq_tbl_determine_rate+0x94/0xfc
   clk_rcg2_determine_rate+0x2c/0x38
   clk_core_determine_round_nolock+0x4c/0x88
   clk_core_round_rate_nolock+0x6c/0xa8
   clk_core_round_rate_nolock+0x9c/0xa8
   clk_core_set_rate_nolock+0x70/0x180
   clk_set_rate+0x3c/0x6c
   of_clk_set_defaults+0x254/0x360
   platform_drv_probe+0x28/0xb0
   really_probe+0x120/0x2dc
   driver_probe_device+0x64/0xfc
   device_driver_attach+0x4c/0x6c
   __driver_attach+0xac/0xc0
   bus_for_each_dev+0x84/0xcc
   driver_attach+0x2c/0x38
   bus_add_driver+0xfc/0x1d0
   driver_register+0x64/0xf8
   __platform_driver_register+0x4c/0x58
   msm_drm_register+0x5c/0x60
   ...

It turned out that clk_hw_get_parent_by_index() was returning NULL and
we weren't checking.  Let's check it so that we don't crash.

Fixes: ac269395cdd8 ("clk: qcom: Convert to clk_hw based provider APIs")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I haven't gone back and tried to reproduce this same crash on older
kernels, but I'll put the blame on commit ac269395cdd8 ("clk: qcom:
Convert to clk_hw based provider APIs").  Before that if we got a NULL
parent back it was fine and dandy since a NULL "struct clk" is valid
to use but a NULL "struct clk_hw" is not.

Changes in v2:
- Patch ("clk: qcom: rcg2: Don't crash...") new for v2.

 drivers/clk/qcom/clk-rcg2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index da045b200def..9098001ac805 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -218,6 +218,9 @@ static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f,
 
 	clk_flags = clk_hw_get_flags(hw);
 	p = clk_hw_get_parent_by_index(hw, index);
+	if (!p)
+		return -EINVAL;
+
 	if (clk_flags & CLK_SET_RATE_PARENT) {
 		rate = f->freq;
 		if (f->pre_div) {
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 02/10] dt-bindings: clock: Fix qcom,dispcc bindings for sdm845/sc7180

[Douglas Anderson]

The qcom,dispcc bindings had a few problems with them:

1. They didn't specify all the clocks that dispcc is a client of.
   Specifically on sc7180 there are two clocks from the DSI PHY and
   two from the DP PHY.  On sdm845 there are actually two DSI PHYs
   (each of which has two clocks).  These all need to be specified.

2. The sdm845.dtsi has existed for quite some time without specifying
   the clocks.  The Linux driver was relying on global names to match
   things up.  While we should transition things, it should be noted
   in the bindings.

NOTE: It may be slightly controversial that I didn't re-order the
clocks and name the "DSI" clocks on sc7180 to "dsi0".  That would have
allowed me to have a single table and just use minItems/maxItems to
specify that sc7180 only had one DSI PHY.  I almost did that, but it
felt a little weird.  Why did the DSI clock have a 0 but not the DP
clock?  If we add a SoC that has a 2nd DP port then we can't
retroactively name old ones.  What if we have a SoC that has HDMI but
only one DSI lane?  It felt cleaner to me to just duplicate.

Also note that I updated the example.

Fixes: 5d28e44ba630 ("dt-bindings: clock: Add YAML schemas for the QCOM DISPCC clock bindings")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Patch ("dt-bindings: clock: Fix qcom,dispcc...") new for v2.

 .../bindings/clock/qcom,dispcc.yaml           | 87 +++++++++++++++----
 1 file changed, 71 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc.yaml
index 9c58e02a1de1..560c52ce3da5 100644
--- a/Documentation/devicetree/bindings/clock/qcom,dispcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,dispcc.yaml
@@ -19,18 +19,6 @@ properties:
       - qcom,sc7180-dispcc
       - qcom,sdm845-dispcc
 
-  clocks:
-    minItems: 1
-    maxItems: 2
-    items:
-      - description: Board XO source
-      - description: GPLL0 source from GCC
-
-  clock-names:
-    items:
-      - const: xo
-      - const: gpll0
-
   '#clock-cells':
     const: 1
 
@@ -52,16 +40,83 @@ required:
   - '#reset-cells'
   - '#power-domain-cells'
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: qcom,sc7180-dispcc
+then:
+  properties:
+    clocks:
+      items:
+        - description: Board XO source
+        - description: GPLL0 source from GCC
+        - description: Byte clock from DSI PHY
+        - description: Pixel clock from DSI PHY
+        - description: Link clock from DP PHY
+        - description: VCO DIV clock from DP PHY
+
+    clock-names:
+      items:
+        - const: xo
+        - const: gpll0
+        - const: dsi_phy_pll_byte
+        - const: dsi_phy_pll_pixel
+        - const: dp_phy_pll_link
+        - const: dp_phy_pll_vco_div
+
+else:
+  if:
+    # NOTE: sdm845.dtsi existed for quite some time and specified no clocks.
+    # The code had to use hardcoded mechanisms to find the input clocks.
+    # Any sdm845 device trees should be transitioned, but actual code may
+    # need to handle old dts files.
+    properties:
+      compatible:
+        contains:
+          const: qcom,sdm845-dispcc
+  then:
+    properties:
+      clocks:
+        items:
+          - description: Board XO source
+          - description: GPLL0 source from GCC
+          - description: Byte clock from DSI PHY0
+          - description: Pixel clock from DSI PHY0
+          - description: Byte clock from DSI PHY1
+          - description: Pixel clock from DSI PHY1
+          - description: Link clock from DP PHY
+          - description: VCO DIV clock from DP PHY
+
+      clock-names:
+        items:
+          - const: xo
+          - const: gpll0
+          - const: dsi0_phy_pll_byte
+          - const: dsi0_phy_pll_pixel
+          - const: dsi1_phy_pll_byte
+          - const: dsi1_phy_pll_pixel
+          - const: dp_phy_pll_link
+          - const: dp_phy_pll_vco_div
+
 examples:
   # Example of DISPCC with clock node properties for SDM845:
   - |
+    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+    #include <dt-bindings/clock/qcom,rpmh.h>
     clock-controller@af00000 {
       compatible = "qcom,sdm845-dispcc";
-      reg = <0xaf00000 0x10000>;
-      clocks = <&rpmhcc 0>, <&gcc 24>;
-      clock-names = "xo", "gpll0";
+      reg = <0 0x0af00000 0 0x10000>;
+      clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_DISP_GPLL0_CLK_SRC>,
+               <&dsi0_phy 0>, <&dsi0_phy 1>,
+               <&dsi1_phy 0>, <&dsi1_phy 1>,
+               <&dp_phy 0>, <&dp_phy 1>;
+      clock-names = "xo", "gpll0",
+                    "dsi0_phy_pll_byte", "dsi0_phy_pll_pixel",
+                    "dsi1_phy_pll_byte", "dsi1_phy_pll_pixel",
+                    "dp_phy_pll_link", "dp_phy_pll_vco_div";
       #clock-cells = <1>;
       #reset-cells = <1>;
       #power-domain-cells = <1>;
-     };
+    };
 ...
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 03/10] arm64: dts: qcom: sdm845: Add the missing clocks on the dispcc

[Douglas Anderson]

We're transitioning over to requiring the Qualcomm Display Clock
Controller to specify all the input clocks.  Let's add them for
sdm845.

NOTES:
- Until the Linux driver for sdm845's dispcc is updated, these clocks
  will not actually be used in Linux.  It will continue to use global
  clock names to match things up.
- Although the clocks from the DP PHY are required, the DP PHY isn't
  represented in the dts yet.  Apparently the magic for this is just
  to use <0>.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Patch ("arm64: dts: qcom: sdm845: Add...dispcc") new for v2.

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index d42302b8889b..01354533a61b 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2933,6 +2933,18 @@ opp-200000000 {
 		dispcc: clock-controller@af00000 {
 			compatible = "qcom,sdm845-dispcc";
 			reg = <0 0x0af00000 0 0x10000>;
+			clocks = <&rpmhcc RPMH_CXO_CLK>,
+				 <&gcc GCC_DISP_GPLL0_CLK_SRC>,
+				 <&dsi0_phy 0>,
+				 <&dsi0_phy 1>,
+				 <&dsi1_phy 0>,
+				 <&dsi1_phy 1>,
+				 <0>,
+				 <0>;
+			clock-names = "xo", "gpll0",
+				      "dsi0_phy_pll_byte", "dsi0_phy_pll_pixel",
+				      "dsi1_phy_pll_byte", "dsi1_phy_pll_pixel",
+				      "dp_phy_pll_link", "dp_phy_pll_vco_div";
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 04/10] dt-bindings: clock: Fix qcom,gpucc bindings for sdm845/sc7180/msm8998

[Douglas Anderson]

The qcom,dispcc bindings had a few problems with them:

1. When things were converted to yaml the name of the "gpll0 main"
   clock got changed from "gpll0" to "gpll0_main".  Change it back.

2. The bindings are written so that new boards don't have to specify
   all the clocks.  That doesn't really make sense.  Make it so that
   on new boards all 3 clocks are required.

This also updates the example to be sc7180 and use symbolic names for
clock indicies.

NOTE: It seems that we can only make things _more_ restrictive in the
per-SoC overrides for minItems/maxItems.  ...so by default we start
out with a loose min=2, max=3 (implicit).  Then we restrict msm8998 to
exactly 2 and everything else to exactly 3.

Fixes: 5c6f3a36b913 ("dt-bindings: clock: Add YAML schemas for the QCOM GPUCC clock bindings")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Patch ("dt-bindings: clock: Fix qcom,gpucc...") new for v2.

 .../devicetree/bindings/clock/qcom,gpucc.yaml | 42 ++++++++++++++-----
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
index 622845aa643f..64cf3c450325 100644
--- a/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,gpucc.yaml
@@ -21,19 +21,17 @@ properties:
       - qcom,sdm845-gpucc
 
   clocks:
-    minItems: 1
-    maxItems: 3
+    minItems: 2
     items:
       - description: Board XO source
-      - description: GPLL0 main branch source from GCC(gcc_gpu_gpll0_clk_src)
-      - description: GPLL0 div branch source from GCC(gcc_gpu_gpll0_div_clk_src)
+      - description: GPLL0 main branch source (gcc_gpu_gpll0_clk_src)
+      - description: GPLL0 div branch source (gcc_gpu_gpll0_div_clk_src)
 
   clock-names:
-    minItems: 1
-    maxItems: 3
+    minItems: 2
     items:
       - const: xo
-      - const: gpll0_main
+      - const: gpll0
       - const: gpll0_div
 
   '#clock-cells':
@@ -57,16 +55,38 @@ required:
   - '#reset-cells'
   - '#power-domain-cells'
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: qcom,msm8998-gpucc
+then:
+  properties:
+    clocks:
+      maxItems: 2
+    clock-names:
+      maxItems: 2
+else:
+  properties:
+    clocks:
+      minItems: 3
+    clock-names:
+      minItems: 3
+
 examples:
   # Example of GPUCC with clock node properties for SDM845:
   - |
+    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+    #include <dt-bindings/clock/qcom,rpmh.h>
     clock-controller@5090000 {
       compatible = "qcom,sdm845-gpucc";
-      reg = <0x5090000 0x9000>;
-      clocks = <&rpmhcc 0>, <&gcc 31>, <&gcc 32>;
-      clock-names = "xo", "gpll0_main", "gpll0_div";
+      reg = <0 0x05090000 0 0x9000>;
+      clocks = <&rpmhcc RPMH_CXO_CLK>,
+         <&gcc GCC_GPU_GPLL0_CLK_SRC>,
+         <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
+      clock-names = "xo", "gpll0", "gpll0_div";
       #clock-cells = <1>;
       #reset-cells = <1>;
       #power-domain-cells = <1>;
-     };
+    };
 ...
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 05/10] clk: qcom: Fix sc7180 dispcc parent data

[Douglas Anderson]

The bindings file (qcom,dispcc.yaml) says that the two clocks that
dispcc is a client of are named "xo" and "gpll0".  That means we have
to refer to them by those names.  We weren't referring to "xo"
properly in the driver.

Then, in the patch ("dt-bindings: clock: Fix qcom,dispcc bindings for
sdm845/sc7180") we clarify the names for all of the clocks that we are
a client of.  Fix all those too, also getting rid of the "fallback"
names for them.  Since sc7180 is still in infancy there is no reason
to specify a fallback name.  People should just get the device tree
right.

Since we didn't add the "test" clock to the bindings (apparently it's
never used), kill it from the driver.  If someone has a use for it we
should add it to the bindings and bring it back.

Instead of updating all of the sizes of the arrays now that the test
clock is gone, switch to using the less error-prone ARRAY_SIZE.  Not
sure why it didn't always use that.

Fixes: dd3d06622138 ("clk: qcom: Add display clock controller driver for SC7180")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Patch ("clk: qcom: Fix sc7180 dispcc parent data") new for v2.

 drivers/clk/qcom/dispcc-sc7180.c | 63 ++++++++++++--------------------
 1 file changed, 24 insertions(+), 39 deletions(-)

diff --git a/drivers/clk/qcom/dispcc-sc7180.c b/drivers/clk/qcom/dispcc-sc7180.c
index 30c1e25d3edb..380eca3f847d 100644
--- a/drivers/clk/qcom/dispcc-sc7180.c
+++ b/drivers/clk/qcom/dispcc-sc7180.c
@@ -43,7 +43,7 @@ static struct clk_alpha_pll disp_cc_pll0 = {
 		.hw.init = &(struct clk_init_data){
 			.name = "disp_cc_pll0",
 			.parent_data = &(const struct clk_parent_data){
-				.fw_name = "bi_tcxo",
+				.fw_name = "xo",
 			},
 			.num_parents = 1,
 			.ops = &clk_alpha_pll_fabia_ops,
@@ -76,40 +76,32 @@ static struct clk_alpha_pll_postdiv disp_cc_pll0_out_even = {
 
 static const struct parent_map disp_cc_parent_map_0[] = {
 	{ P_BI_TCXO, 0 },
-	{ P_CORE_BI_PLL_TEST_SE, 7 },
 };
 
 static const struct clk_parent_data disp_cc_parent_data_0[] = {
-	{ .fw_name = "bi_tcxo" },
-	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
+	{ .fw_name = "xo" },
 };
 
 static const struct parent_map disp_cc_parent_map_1[] = {
 	{ P_BI_TCXO, 0 },
 	{ P_DP_PHY_PLL_LINK_CLK, 1 },
 	{ P_DP_PHY_PLL_VCO_DIV_CLK, 2 },
-	{ P_CORE_BI_PLL_TEST_SE, 7 },
 };
 
 static const struct clk_parent_data disp_cc_parent_data_1[] = {
-	{ .fw_name = "bi_tcxo" },
-	{ .fw_name = "dp_phy_pll_link_clk", .name = "dp_phy_pll_link_clk" },
-	{ .fw_name = "dp_phy_pll_vco_div_clk",
-				.name = "dp_phy_pll_vco_div_clk"},
-	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
+	{ .fw_name = "xo" },
+	{ .fw_name = "dp_phy_pll_link" },
+	{ .fw_name = "dp_phy_pll_vco_div" },
 };
 
 static const struct parent_map disp_cc_parent_map_2[] = {
 	{ P_BI_TCXO, 0 },
 	{ P_DSI0_PHY_PLL_OUT_BYTECLK, 1 },
-	{ P_CORE_BI_PLL_TEST_SE, 7 },
 };
 
 static const struct clk_parent_data disp_cc_parent_data_2[] = {
-	{ .fw_name = "bi_tcxo" },
-	{ .fw_name = "dsi0_phy_pll_out_byteclk",
-				.name = "dsi0_phy_pll_out_byteclk" },
-	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
+	{ .fw_name = "xo" },
+	{ .fw_name = "dsi_phy_pll_byte" },
 };
 
 static const struct parent_map disp_cc_parent_map_3[] = {
@@ -117,40 +109,33 @@ static const struct parent_map disp_cc_parent_map_3[] = {
 	{ P_DISP_CC_PLL0_OUT_MAIN, 1 },
 	{ P_GPLL0_OUT_MAIN, 4 },
 	{ P_DISP_CC_PLL0_OUT_EVEN, 5 },
-	{ P_CORE_BI_PLL_TEST_SE, 7 },
 };
 
 static const struct clk_parent_data disp_cc_parent_data_3[] = {
-	{ .fw_name = "bi_tcxo" },
+	{ .fw_name = "xo" },
 	{ .hw = &disp_cc_pll0.clkr.hw },
-	{ .fw_name = "gcc_disp_gpll0_clk_src" },
+	{ .fw_name = "gpll0" },
 	{ .hw = &disp_cc_pll0_out_even.clkr.hw },
-	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
 };
 
 static const struct parent_map disp_cc_parent_map_4[] = {
 	{ P_BI_TCXO, 0 },
 	{ P_GPLL0_OUT_MAIN, 4 },
-	{ P_CORE_BI_PLL_TEST_SE, 7 },
 };
 
 static const struct clk_parent_data disp_cc_parent_data_4[] = {
-	{ .fw_name = "bi_tcxo" },
-	{ .fw_name = "gcc_disp_gpll0_clk_src" },
-	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
+	{ .fw_name = "xo" },
+	{ .fw_name = "gpll0" },
 };
 
 static const struct parent_map disp_cc_parent_map_5[] = {
 	{ P_BI_TCXO, 0 },
 	{ P_DSI0_PHY_PLL_OUT_DSICLK, 1 },
-	{ P_CORE_BI_PLL_TEST_SE, 7 },
 };
 
 static const struct clk_parent_data disp_cc_parent_data_5[] = {
-	{ .fw_name = "bi_tcxo" },
-	{ .fw_name = "dsi0_phy_pll_out_dsiclk",
-				.name = "dsi0_phy_pll_out_dsiclk" },
-	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
+	{ .fw_name = "xo" },
+	{ .fw_name = "dsi_phy_pll_pixel" },
 };
 
 static const struct freq_tbl ftbl_disp_cc_mdss_ahb_clk_src[] = {
@@ -169,7 +154,7 @@ static struct clk_rcg2 disp_cc_mdss_ahb_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "disp_cc_mdss_ahb_clk_src",
 		.parent_data = disp_cc_parent_data_4,
-		.num_parents = 3,
+		.num_parents = ARRAY_SIZE(disp_cc_parent_data_4),
 		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_rcg2_shared_ops,
 	},
@@ -183,7 +168,7 @@ static struct clk_rcg2 disp_cc_mdss_byte0_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "disp_cc_mdss_byte0_clk_src",
 		.parent_data = disp_cc_parent_data_2,
-		.num_parents = 3,
+		.num_parents = ARRAY_SIZE(disp_cc_parent_data_2),
 		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_byte2_ops,
 	},
@@ -203,7 +188,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_aux_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "disp_cc_mdss_dp_aux_clk_src",
 		.parent_data = disp_cc_parent_data_0,
-		.num_parents = 2,
+		.num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
 		.ops = &clk_rcg2_ops,
 	},
 };
@@ -216,7 +201,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_crypto_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "disp_cc_mdss_dp_crypto_clk_src",
 		.parent_data = disp_cc_parent_data_1,
-		.num_parents = 4,
+		.num_parents = ARRAY_SIZE(disp_cc_parent_data_1),
 		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_byte2_ops,
 	},
@@ -230,7 +215,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_link_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "disp_cc_mdss_dp_link_clk_src",
 		.parent_data = disp_cc_parent_data_1,
-		.num_parents = 4,
+		.num_parents = ARRAY_SIZE(disp_cc_parent_data_1),
 		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_byte2_ops,
 	},
@@ -244,7 +229,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_pixel_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "disp_cc_mdss_dp_pixel_clk_src",
 		.parent_data = disp_cc_parent_data_1,
-		.num_parents = 4,
+		.num_parents = ARRAY_SIZE(disp_cc_parent_data_1),
 		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_dp_ops,
 	},
@@ -259,7 +244,7 @@ static struct clk_rcg2 disp_cc_mdss_esc0_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "disp_cc_mdss_esc0_clk_src",
 		.parent_data = disp_cc_parent_data_2,
-		.num_parents = 3,
+		.num_parents = ARRAY_SIZE(disp_cc_parent_data_2),
 		.ops = &clk_rcg2_ops,
 	},
 };
@@ -282,7 +267,7 @@ static struct clk_rcg2 disp_cc_mdss_mdp_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "disp_cc_mdss_mdp_clk_src",
 		.parent_data = disp_cc_parent_data_3,
-		.num_parents = 5,
+		.num_parents = ARRAY_SIZE(disp_cc_parent_data_3),
 		.ops = &clk_rcg2_shared_ops,
 	},
 };
@@ -295,7 +280,7 @@ static struct clk_rcg2 disp_cc_mdss_pclk0_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "disp_cc_mdss_pclk0_clk_src",
 		.parent_data = disp_cc_parent_data_5,
-		.num_parents = 3,
+		.num_parents = ARRAY_SIZE(disp_cc_parent_data_5),
 		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_pixel_ops,
 	},
@@ -310,7 +295,7 @@ static struct clk_rcg2 disp_cc_mdss_rot_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "disp_cc_mdss_rot_clk_src",
 		.parent_data = disp_cc_parent_data_3,
-		.num_parents = 5,
+		.num_parents = ARRAY_SIZE(disp_cc_parent_data_3),
 		.ops = &clk_rcg2_shared_ops,
 	},
 };
@@ -324,7 +309,7 @@ static struct clk_rcg2 disp_cc_mdss_vsync_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "disp_cc_mdss_vsync_clk_src",
 		.parent_data = disp_cc_parent_data_0,
-		.num_parents = 2,
+		.num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
 		.ops = &clk_rcg2_shared_ops,
 	},
 };
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 06/10] arm64: dts: qcom: sdm845: Add the missing clocks on the gpucc

[Douglas Anderson]

We're transitioning over to requiring the Qualcomm GPU Clock
Controller to specify all the input clocks.  Let's add them for
sdm845.

NOTE: Until the Linux driver for sdm845's gpucc is updated, these
clocks will not actually be used in Linux.  It will continue to use
global clock names to match things up.  Of course, Linux didn't use
the old "xo" clock anyway.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Patch ("arm64: dts: qcom: sdm845: Add...gpucc") new for v2.

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 01354533a61b..e624c91dbd6d 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1903,8 +1903,10 @@ gpucc: clock-controller@5090000 {
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;
-			clocks = <&rpmhcc RPMH_CXO_CLK>;
-			clock-names = "xo";
+			clocks = <&rpmhcc RPMH_CXO_CLK>,
+				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
+				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
+			clock-names = "xo", "gpll0", "gpll0_div";
 		};
 
 		stm@6002000 {
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 07/10] clk: qcom: Fix sc7180 gpucc parent data

[Douglas Anderson]

The bindings file (qcom,gpucc.yaml) does not agree with the names we
use for input clocks.  Fix us.  This takes into account the changes in
the recent patch ("dt-bindings: clock: Fix qcom,gpucc bindings for
sdm845/sc7180/msm8998"), but even without that patch the names in the
driver were still not right.

Since we didn't add the "test" clock to the bindings (apparently it's
never used), kill it from the driver.  If someone has a use for it we
should add it to the bindings and bring it back.

Instead of updating the size of the array now that the test clock is
gone, switch to using the less error-prone ARRAY_SIZE.  Not sure why
it didn't always use that.

Fixes: 745ff069a49c ("clk: qcom: Add graphics clock controller driver for SC7180")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Patch ("clk: qcom: Fix sc7180 gpucc parent data") new for v2.

 drivers/clk/qcom/gpucc-sc7180.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c
index ec61194cceaf..da56506036e2 100644
--- a/drivers/clk/qcom/gpucc-sc7180.c
+++ b/drivers/clk/qcom/gpucc-sc7180.c
@@ -47,7 +47,7 @@ static struct clk_alpha_pll gpu_cc_pll1 = {
 		.hw.init = &(struct clk_init_data){
 			.name = "gpu_cc_pll1",
 			.parent_data =  &(const struct clk_parent_data){
-				.fw_name = "bi_tcxo",
+				.fw_name = "xo",
 			},
 			.num_parents = 1,
 			.ops = &clk_alpha_pll_fabia_ops,
@@ -64,11 +64,10 @@ static const struct parent_map gpu_cc_parent_map_0[] = {
 };
 
 static const struct clk_parent_data gpu_cc_parent_data_0[] = {
-	{ .fw_name = "bi_tcxo" },
+	{ .fw_name = "xo" },
 	{ .hw = &gpu_cc_pll1.clkr.hw },
-	{ .fw_name = "gcc_gpu_gpll0_clk_src" },
-	{ .fw_name = "gcc_gpu_gpll0_div_clk_src" },
-	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
+	{ .fw_name = "gpll0" },
+	{ .fw_name = "gpll0_div" },
 };
 
 static const struct freq_tbl ftbl_gpu_cc_gmu_clk_src[] = {
@@ -86,7 +85,7 @@ static struct clk_rcg2 gpu_cc_gmu_clk_src = {
 	.clkr.hw.init = &(struct clk_init_data){
 		.name = "gpu_cc_gmu_clk_src",
 		.parent_data = gpu_cc_parent_data_0,
-		.num_parents = 5,
+		.num_parents = ARRAY_SIZE(gpu_cc_parent_data_0),
 		.flags = CLK_SET_RATE_PARENT,
 		.ops = &clk_rcg2_shared_ops,
 	},
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 08/10] dt-bindings: clock: Cleanup qcom,videocc bindings for sdm845/sc7180

[Douglas Anderson]

This makes the qcom,videocc bindings match the recent changes to the
dispcc and gpucc.  Unlike the recent changes to those files, this one
doesn't really have any functional change.  It:

1. Adds a description for the XO clock.  Not terribly important but
   nice if it cleanly matches its cousins.

2. Updates the example to use the symbolic name for the RPMH clock and
   also show that the real devices are currently using 2 address cells
   / size cells and fixes the spacing on the closing brace.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Patch ("dt-bindings: clock: Cleanup qcom,videocc") new for v2.

 .../devicetree/bindings/clock/qcom,videocc.yaml        | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
index 43cfc893a8d1..745928dc0fcb 100644
--- a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
@@ -20,7 +20,8 @@ properties:
       - qcom,sdm845-videocc
 
   clocks:
-    maxItems: 1
+    items:
+      - description: Board XO source
 
   clock-names:
     items:
@@ -50,13 +51,14 @@ required:
 examples:
   # Example of VIDEOCC with clock node properties for SDM845:
   - |
+    #include <dt-bindings/clock/qcom,rpmh.h>
     clock-controller@ab00000 {
       compatible = "qcom,sdm845-videocc";
-      reg = <0xab00000 0x10000>;
-      clocks = <&rpmhcc 0>;
+      reg = <0 0x0ab00000 0 0x10000>;
+      clocks = <&rpmhcc RPMH_CXO_CLK>;
       clock-names = "xo";
       #clock-cells = <1>;
       #reset-cells = <1>;
       #power-domain-cells = <1>;
-     };
+    };
 ...
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 09/10] arm64: dts: qcom: sdm845: Add the missing clock on the videocc

[Douglas Anderson]

We're transitioning over to requiring the Qualcomm Video Clock
Controller to specify all the input clocks.  Let's add the one input
clock for the videocc for sdm845.

NOTE: Until the Linux driver for sdm845's video is updated, this clock
will not actually be used in Linux.  It will continue to use global
clock names to match things up.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Patch ("arm64: dts: qcom: sdm845: Add...videocc") new for v2.

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index e624c91dbd6d..8c41e25bd4a8 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2605,6 +2605,8 @@ video-core1 {
 		videocc: clock-controller@ab00000 {
 			compatible = "qcom,sdm845-videocc";
 			reg = <0 0x0ab00000 0 0x10000>;
+			clocks = <&rpmhcc RPMH_CXO_CLK>;
+			clock-names = "xo";
 			#clock-cells = <1>;
 			#power-domain-cells = <1>;
 			#reset-cells = <1>;
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 10/10] arm64: dts: sc7180: Add clock controller nodes

[Douglas Anderson]

From: Taniya Das <tdas@codeaurora.org>

Add the display, video & graphics clock controller nodes supported on
SC7180.

NOTE: the dispcc needs input clocks from various PHYs that aren't in
the device tree yet.  For now we'll leave these stubbed out with <0>,
which is apparently the magic way to do this.  These clocks aren't
really "optional" and this stubbing out method is apparently the best
way to handle it.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Added includes
- Changed various parent names to match bindings / driver

 arch/arm64/boot/dts/qcom/sc7180.dtsi | 41 ++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 8011c5fe2a31..ee3b4bade66b 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -5,7 +5,9 @@
  * Copyright (c) 2019, The Linux Foundation. All rights reserved.
  */
 
+#include <dt-bindings/clock/qcom,dispcc-sc7180.h>
 #include <dt-bindings/clock/qcom,gcc-sc7180.h>
+#include <dt-bindings/clock/qcom,gpucc-sc7180.h>
 #include <dt-bindings/clock/qcom,rpmh.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/phy/phy-qcom-qusb2.h>
@@ -1039,6 +1041,18 @@ pinmux {
 			};
 		};
 
+		gpucc: clock-controller@5090000 {
+			compatible = "qcom,sc7180-gpucc";
+			reg = <0 0x05090000 0 0x9000>;
+			clocks = <&rpmhcc RPMH_CXO_CLK>,
+				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
+				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
+			clock-names = "xo", "gpll0", "gpll0_div";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			#power-domain-cells = <1>;
+		};
+
 		qspi: spi@88dc000 {
 			compatible = "qcom,qspi-v1";
 			reg = <0 0x088dc000 0 0x600>;
@@ -1151,6 +1165,33 @@ usb_1_dwc3: dwc3@a600000 {
 			};
 		};
 
+		videocc: clock-controller@ab00000 {
+			compatible = "qcom,sc7180-videocc";
+			reg = <0 0x0ab00000 0 0x10000>;
+			clocks = <&rpmhcc RPMH_CXO_CLK>;
+			clock-names = "xo";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			#power-domain-cells = <1>;
+		};
+
+		dispcc: clock-controller@af00000 {
+			compatible = "qcom,sc7180-dispcc";
+			reg = <0 0x0af00000 0 0x200000>;
+			clocks = <&rpmhcc RPMH_CXO_CLK>,
+				 <&gcc GCC_DISP_GPLL0_CLK_SRC>,
+				 <0>,
+				 <0>,
+				 <0>,
+				 <0>;
+			clock-names = "xo", "gpll0",
+				      "dsi_phy_pll_byte", "dsi_phy_pll_pixel",
+				      "dp_phy_pll_link", "dp_phy_pll_vco_div";
+			#clock-cells = <1>;
+			#reset-cells = <1>;
+			#power-domain-cells = <1>;
+		};
+
 		pdc: interrupt-controller@b220000 {
 			compatible = "qcom,sc7180-pdc", "qcom,pdc";
 			reg = <0 0x0b220000 0 0x30000>;
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH v2 05/10] clk: qcom: Fix sc7180 dispcc parent data

[Taniya Das]

Hi Doug,

Thanks for the patch.

On 1/25/2020 4:12 AM, Douglas Anderson wrote:
> The bindings file (qcom,dispcc.yaml) says that the two clocks that
> dispcc is a client of are named "xo" and "gpll0".  That means we have
> to refer to them by those names.  We weren't referring to "xo"
> properly in the driver.
> 
> Then, in the patch ("dt-bindings: clock: Fix qcom,dispcc bindings for
> sdm845/sc7180") we clarify the names for all of the clocks that we are
> a client of.  Fix all those too, also getting rid of the "fallback"
> names for them.  Since sc7180 is still in infancy there is no reason
> to specify a fallback name.  People should just get the device tree
> right.
> 
> Since we didn't add the "test" clock to the bindings (apparently it's
> never used), kill it from the driver.  If someone has a use for it we
> should add it to the bindings and bring it back.
> 
> Instead of updating all of the sizes of the arrays now that the test
> clock is gone, switch to using the less error-prone ARRAY_SIZE.  Not
> sure why it didn't always use that.
> 
> Fixes: dd3d06622138 ("clk: qcom: Add display clock controller driver for SC7180")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v2:
> - Patch ("clk: qcom: Fix sc7180 dispcc parent data") new for v2.
> 
>   drivers/clk/qcom/dispcc-sc7180.c | 63 ++++++++++++--------------------
>   1 file changed, 24 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/clk/qcom/dispcc-sc7180.c b/drivers/clk/qcom/dispcc-sc7180.c
> index 30c1e25d3edb..380eca3f847d 100644
> --- a/drivers/clk/qcom/dispcc-sc7180.c
> +++ b/drivers/clk/qcom/dispcc-sc7180.c
> @@ -43,7 +43,7 @@ static struct clk_alpha_pll disp_cc_pll0 = {
>   		.hw.init = &(struct clk_init_data){
>   			.name = "disp_cc_pll0",
>   			.parent_data = &(const struct clk_parent_data){
> -				.fw_name = "bi_tcxo",
> +				.fw_name = "xo",

These clock names are as per our HW design and we would not like to 
update them as they require lot of hand-coding. These codes are all 
auto-generated.

>   			},
>   			.num_parents = 1,
>   			.ops = &clk_alpha_pll_fabia_ops,
> @@ -76,40 +76,32 @@ static struct clk_alpha_pll_postdiv disp_cc_pll0_out_even = {
>   
>   static const struct parent_map disp_cc_parent_map_0[] = {
>   	{ P_BI_TCXO, 0 },
> -	{ P_CORE_BI_PLL_TEST_SE, 7 },
>   };
>   
>   static const struct clk_parent_data disp_cc_parent_data_0[] = {
> -	{ .fw_name = "bi_tcxo" },
> -	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +	{ .fw_name = "xo" },
>   };
>   
>   static const struct parent_map disp_cc_parent_map_1[] = {
>   	{ P_BI_TCXO, 0 },
>   	{ P_DP_PHY_PLL_LINK_CLK, 1 },
>   	{ P_DP_PHY_PLL_VCO_DIV_CLK, 2 },
> -	{ P_CORE_BI_PLL_TEST_SE, 7 },
>   };
>   
>   static const struct clk_parent_data disp_cc_parent_data_1[] = {
> -	{ .fw_name = "bi_tcxo" },
> -	{ .fw_name = "dp_phy_pll_link_clk", .name = "dp_phy_pll_link_clk" },
> -	{ .fw_name = "dp_phy_pll_vco_div_clk",
> -				.name = "dp_phy_pll_vco_div_clk"},
> -	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +	{ .fw_name = "xo" },
> +	{ .fw_name = "dp_phy_pll_link" },
> +	{ .fw_name = "dp_phy_pll_vco_div" },

similar comments for these too. They would conflict with our HW design 
clock names.
>   };
>   
>   static const struct parent_map disp_cc_parent_map_2[] = {
>   	{ P_BI_TCXO, 0 },
>   	{ P_DSI0_PHY_PLL_OUT_BYTECLK, 1 },
> -	{ P_CORE_BI_PLL_TEST_SE, 7 },
>   };
>   
>   static const struct clk_parent_data disp_cc_parent_data_2[] = {
> -	{ .fw_name = "bi_tcxo" },
> -	{ .fw_name = "dsi0_phy_pll_out_byteclk",
> -				.name = "dsi0_phy_pll_out_byteclk" },
> -	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +	{ .fw_name = "xo" },
> +	{ .fw_name = "dsi_phy_pll_byte" },
>   };
>   
>   static const struct parent_map disp_cc_parent_map_3[] = {
> @@ -117,40 +109,33 @@ static const struct parent_map disp_cc_parent_map_3[] = {
>   	{ P_DISP_CC_PLL0_OUT_MAIN, 1 },
>   	{ P_GPLL0_OUT_MAIN, 4 },
>   	{ P_DISP_CC_PLL0_OUT_EVEN, 5 },
> -	{ P_CORE_BI_PLL_TEST_SE, 7 },
>   };
>   
>   static const struct clk_parent_data disp_cc_parent_data_3[] = {
> -	{ .fw_name = "bi_tcxo" },
> +	{ .fw_name = "xo" },
>   	{ .hw = &disp_cc_pll0.clkr.hw },
> -	{ .fw_name = "gcc_disp_gpll0_clk_src" },
> +	{ .fw_name = "gpll0" },

This is not the correct clock, we have a child/branch clock which 
requires to be turned ON "gcc_disp_gpll0_clk_src" when we switch to this 
source.

>   	{ .hw = &disp_cc_pll0_out_even.clkr.hw },
> -	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
>   };
>   
>   static const struct parent_map disp_cc_parent_map_4[] = {
>   	{ P_BI_TCXO, 0 },
>   	{ P_GPLL0_OUT_MAIN, 4 },
> -	{ P_CORE_BI_PLL_TEST_SE, 7 },
>   };
>   
>   static const struct clk_parent_data disp_cc_parent_data_4[] = {
> -	{ .fw_name = "bi_tcxo" },
> -	{ .fw_name = "gcc_disp_gpll0_clk_src" },
> -	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +	{ .fw_name = "xo" },
> +	{ .fw_name = "gpll0" },

same comment as above.

>   };
>   
>   static const struct parent_map disp_cc_parent_map_5[] = {
>   	{ P_BI_TCXO, 0 },
>   	{ P_DSI0_PHY_PLL_OUT_DSICLK, 1 },
> -	{ P_CORE_BI_PLL_TEST_SE, 7 },
>   };
>   
>   static const struct clk_parent_data disp_cc_parent_data_5[] = {
> -	{ .fw_name = "bi_tcxo" },
> -	{ .fw_name = "dsi0_phy_pll_out_dsiclk",
> -				.name = "dsi0_phy_pll_out_dsiclk" },
> -	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +	{ .fw_name = "xo" },
> +	{ .fw_name = "dsi_phy_pll_pixel" },
>   };
>   
>   static const struct freq_tbl ftbl_disp_cc_mdss_ahb_clk_src[] = {
> @@ -169,7 +154,7 @@ static struct clk_rcg2 disp_cc_mdss_ahb_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "disp_cc_mdss_ahb_clk_src",
>   		.parent_data = disp_cc_parent_data_4,
> -		.num_parents = 3,
> +		.num_parents = ARRAY_SIZE(disp_cc_parent_data_4),
>   		.flags = CLK_SET_RATE_PARENT,
>   		.ops = &clk_rcg2_shared_ops,
>   	},
> @@ -183,7 +168,7 @@ static struct clk_rcg2 disp_cc_mdss_byte0_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "disp_cc_mdss_byte0_clk_src",
>   		.parent_data = disp_cc_parent_data_2,
> -		.num_parents = 3,
> +		.num_parents = ARRAY_SIZE(disp_cc_parent_data_2),
>   		.flags = CLK_SET_RATE_PARENT,
>   		.ops = &clk_byte2_ops,
>   	},
> @@ -203,7 +188,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_aux_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "disp_cc_mdss_dp_aux_clk_src",
>   		.parent_data = disp_cc_parent_data_0,
> -		.num_parents = 2,
> +		.num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
>   		.ops = &clk_rcg2_ops,
>   	},
>   };
> @@ -216,7 +201,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_crypto_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "disp_cc_mdss_dp_crypto_clk_src",
>   		.parent_data = disp_cc_parent_data_1,
> -		.num_parents = 4,
> +		.num_parents = ARRAY_SIZE(disp_cc_parent_data_1),
>   		.flags = CLK_SET_RATE_PARENT,
>   		.ops = &clk_byte2_ops,
>   	},
> @@ -230,7 +215,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_link_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "disp_cc_mdss_dp_link_clk_src",
>   		.parent_data = disp_cc_parent_data_1,
> -		.num_parents = 4,
> +		.num_parents = ARRAY_SIZE(disp_cc_parent_data_1),
>   		.flags = CLK_SET_RATE_PARENT,
>   		.ops = &clk_byte2_ops,
>   	},
> @@ -244,7 +229,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_pixel_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "disp_cc_mdss_dp_pixel_clk_src",
>   		.parent_data = disp_cc_parent_data_1,
> -		.num_parents = 4,
> +		.num_parents = ARRAY_SIZE(disp_cc_parent_data_1),
>   		.flags = CLK_SET_RATE_PARENT,
>   		.ops = &clk_dp_ops,
>   	},
> @@ -259,7 +244,7 @@ static struct clk_rcg2 disp_cc_mdss_esc0_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "disp_cc_mdss_esc0_clk_src",
>   		.parent_data = disp_cc_parent_data_2,
> -		.num_parents = 3,
> +		.num_parents = ARRAY_SIZE(disp_cc_parent_data_2),
>   		.ops = &clk_rcg2_ops,
>   	},
>   };
> @@ -282,7 +267,7 @@ static struct clk_rcg2 disp_cc_mdss_mdp_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "disp_cc_mdss_mdp_clk_src",
>   		.parent_data = disp_cc_parent_data_3,
> -		.num_parents = 5,
> +		.num_parents = ARRAY_SIZE(disp_cc_parent_data_3),
>   		.ops = &clk_rcg2_shared_ops,
>   	},
>   };
> @@ -295,7 +280,7 @@ static struct clk_rcg2 disp_cc_mdss_pclk0_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "disp_cc_mdss_pclk0_clk_src",
>   		.parent_data = disp_cc_parent_data_5,
> -		.num_parents = 3,
> +		.num_parents = ARRAY_SIZE(disp_cc_parent_data_5),
>   		.flags = CLK_SET_RATE_PARENT,
>   		.ops = &clk_pixel_ops,
>   	},
> @@ -310,7 +295,7 @@ static struct clk_rcg2 disp_cc_mdss_rot_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "disp_cc_mdss_rot_clk_src",
>   		.parent_data = disp_cc_parent_data_3,
> -		.num_parents = 5,
> +		.num_parents = ARRAY_SIZE(disp_cc_parent_data_3),
>   		.ops = &clk_rcg2_shared_ops,
>   	},
>   };
> @@ -324,7 +309,7 @@ static struct clk_rcg2 disp_cc_mdss_vsync_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "disp_cc_mdss_vsync_clk_src",
>   		.parent_data = disp_cc_parent_data_0,
> -		.num_parents = 2,
> +		.num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
>   		.ops = &clk_rcg2_shared_ops,
>   	},
>   };
> 

All the above code are auto-generated and we really do not want to 
hand-code.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

Re: [PATCH v2 07/10] clk: qcom: Fix sc7180 gpucc parent data

[Taniya Das]

Hi Doug,

Thanks for your patch. But as mentioned earlier we really want to avoid 
updating the auto generated code.

On 1/25/2020 4:12 AM, Douglas Anderson wrote:
> The bindings file (qcom,gpucc.yaml) does not agree with the names we
> use for input clocks.  Fix us.  This takes into account the changes in
> the recent patch ("dt-bindings: clock: Fix qcom,gpucc bindings for
> sdm845/sc7180/msm8998"), but even without that patch the names in the
> driver were still not right.
> 
> Since we didn't add the "test" clock to the bindings (apparently it's
> never used), kill it from the driver.  If someone has a use for it we
> should add it to the bindings and bring it back.
> 
> Instead of updating the size of the array now that the test clock is
> gone, switch to using the less error-prone ARRAY_SIZE.  Not sure why
> it didn't always use that.
> 
> Fixes: 745ff069a49c ("clk: qcom: Add graphics clock controller driver for SC7180")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v2:
> - Patch ("clk: qcom: Fix sc7180 gpucc parent data") new for v2.
> 
>   drivers/clk/qcom/gpucc-sc7180.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c
> index ec61194cceaf..da56506036e2 100644
> --- a/drivers/clk/qcom/gpucc-sc7180.c
> +++ b/drivers/clk/qcom/gpucc-sc7180.c
> @@ -47,7 +47,7 @@ static struct clk_alpha_pll gpu_cc_pll1 = {
>   		.hw.init = &(struct clk_init_data){
>   			.name = "gpu_cc_pll1",
>   			.parent_data =  &(const struct clk_parent_data){
> -				.fw_name = "bi_tcxo",
> +				.fw_name = "xo",
>   			},
>   			.num_parents = 1,
>   			.ops = &clk_alpha_pll_fabia_ops,
> @@ -64,11 +64,10 @@ static const struct parent_map gpu_cc_parent_map_0[] = {
>   };
>   
>   static const struct clk_parent_data gpu_cc_parent_data_0[] = {
> -	{ .fw_name = "bi_tcxo" },
> +	{ .fw_name = "xo" },
>   	{ .hw = &gpu_cc_pll1.clkr.hw },
> -	{ .fw_name = "gcc_gpu_gpll0_clk_src" },
> -	{ .fw_name = "gcc_gpu_gpll0_div_clk_src" },
> -	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +	{ .fw_name = "gpll0" },
> +	{ .fw_name = "gpll0_div" },
>   };
>   
>   static const struct freq_tbl ftbl_gpu_cc_gmu_clk_src[] = {
> @@ -86,7 +85,7 @@ static struct clk_rcg2 gpu_cc_gmu_clk_src = {
>   	.clkr.hw.init = &(struct clk_init_data){
>   		.name = "gpu_cc_gmu_clk_src",
>   		.parent_data = gpu_cc_parent_data_0,
> -		.num_parents = 5,
> +		.num_parents = ARRAY_SIZE(gpu_cc_parent_data_0),
>   		.flags = CLK_SET_RATE_PARENT,
>   		.ops = &clk_rcg2_shared_ops,
>   	},
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

Re: [PATCH v2 10/10] arm64: dts: sc7180: Add clock controller nodes

[Taniya Das]

Hi Doug,

Thanks for the patch.

On 1/25/2020 4:12 AM, Douglas Anderson wrote:
> From: Taniya Das <tdas@codeaurora.org>
> 
> Add the display, video & graphics clock controller nodes supported on
> SC7180.
> 
> NOTE: the dispcc needs input clocks from various PHYs that aren't in
> the device tree yet.  For now we'll leave these stubbed out with <0>,
> which is apparently the magic way to do this.  These clocks aren't
> really "optional" and this stubbing out method is apparently the best
> way to handle it.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v2:
> - Added includes
> - Changed various parent names to match bindings / driver
> 
>   arch/arm64/boot/dts/qcom/sc7180.dtsi | 41 ++++++++++++++++++++++++++++
>   1 file changed, 41 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 8011c5fe2a31..ee3b4bade66b 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -5,7 +5,9 @@
>    * Copyright (c) 2019, The Linux Foundation. All rights reserved.
>    */
>   
> +#include <dt-bindings/clock/qcom,dispcc-sc7180.h>
>   #include <dt-bindings/clock/qcom,gcc-sc7180.h>
> +#include <dt-bindings/clock/qcom,gpucc-sc7180.h>

My bad, but we are still missing the videocc header. I could send across 
the new patch.
>   #include <dt-bindings/clock/qcom,rpmh.h>
>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>   #include <dt-bindings/phy/phy-qcom-qusb2.h>
> @@ -1039,6 +1041,18 @@ pinmux {
>   			};
>   		};
>   
> +		gpucc: clock-controller@5090000 {
> +			compatible = "qcom,sc7180-gpucc";
> +			reg = <0 0x05090000 0 0x9000>;
> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
> +				 <&gcc GCC_GPU_GPLL0_CLK_SRC>,
> +				 <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>;
> +			clock-names = "xo", "gpll0", "gpll0_div";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <1>;
> +		};
> +
>   		qspi: spi@88dc000 {
>   			compatible = "qcom,qspi-v1";
>   			reg = <0 0x088dc000 0 0x600>;
> @@ -1151,6 +1165,33 @@ usb_1_dwc3: dwc3@a600000 {
>   			};
>   		};
>   
> +		videocc: clock-controller@ab00000 {
> +			compatible = "qcom,sc7180-videocc";
> +			reg = <0 0x0ab00000 0 0x10000>;
> +			clocks = <&rpmhcc RPMH_CXO_CLK>;
> +			clock-names = "xo";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <1>;
> +		};
> +
> +		dispcc: clock-controller@af00000 {
> +			compatible = "qcom,sc7180-dispcc";
> +			reg = <0 0x0af00000 0 0x200000>;
> +			clocks = <&rpmhcc RPMH_CXO_CLK>,
> +				 <&gcc GCC_DISP_GPLL0_CLK_SRC>,
> +				 <0>,
> +				 <0>,
> +				 <0>,
> +				 <0>;
> +			clock-names = "xo", "gpll0",
> +				      "dsi_phy_pll_byte", "dsi_phy_pll_pixel",
> +				      "dp_phy_pll_link", "dp_phy_pll_vco_div";
> +			#clock-cells = <1>;
> +			#reset-cells = <1>;
> +			#power-domain-cells = <1>;
> +		};
> +
>   		pdc: interrupt-controller@b220000 {
>   			compatible = "qcom,sc7180-pdc", "qcom,pdc";
>   			reg = <0 0x0b220000 0 0x30000>;
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

Re: [PATCH v2 05/10] clk: qcom: Fix sc7180 dispcc parent data

[Doug Anderson]

Hi,

On Mon, Jan 27, 2020 at 9:53 PM Taniya Das <tdas@codeaurora.org> wrote:
>
> Hi Doug,
>
> Thanks for the patch.
>
> On 1/25/2020 4:12 AM, Douglas Anderson wrote:
> > The bindings file (qcom,dispcc.yaml) says that the two clocks that
> > dispcc is a client of are named "xo" and "gpll0".  That means we have
> > to refer to them by those names.  We weren't referring to "xo"
> > properly in the driver.
> >
> > Then, in the patch ("dt-bindings: clock: Fix qcom,dispcc bindings for
> > sdm845/sc7180") we clarify the names for all of the clocks that we are
> > a client of.  Fix all those too, also getting rid of the "fallback"
> > names for them.  Since sc7180 is still in infancy there is no reason
> > to specify a fallback name.  People should just get the device tree
> > right.
> >
> > Since we didn't add the "test" clock to the bindings (apparently it's
> > never used), kill it from the driver.  If someone has a use for it we
> > should add it to the bindings and bring it back.
> >
> > Instead of updating all of the sizes of the arrays now that the test
> > clock is gone, switch to using the less error-prone ARRAY_SIZE.  Not
> > sure why it didn't always use that.
> >
> > Fixes: dd3d06622138 ("clk: qcom: Add display clock controller driver for SC7180")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Patch ("clk: qcom: Fix sc7180 dispcc parent data") new for v2.
> >
> >   drivers/clk/qcom/dispcc-sc7180.c | 63 ++++++++++++--------------------
> >   1 file changed, 24 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/clk/qcom/dispcc-sc7180.c b/drivers/clk/qcom/dispcc-sc7180.c
> > index 30c1e25d3edb..380eca3f847d 100644
> > --- a/drivers/clk/qcom/dispcc-sc7180.c
> > +++ b/drivers/clk/qcom/dispcc-sc7180.c
> > @@ -43,7 +43,7 @@ static struct clk_alpha_pll disp_cc_pll0 = {
> >               .hw.init = &(struct clk_init_data){
> >                       .name = "disp_cc_pll0",
> >                       .parent_data = &(const struct clk_parent_data){
> > -                             .fw_name = "bi_tcxo",
> > +                             .fw_name = "xo",
>
> These clock names are as per our HW design and we would not like to
> update them as they require lot of hand-coding. These codes are all
> auto-generated.

The names in your HW design are global names.  These are local names.
That means that these names are only used in the context of this one
clock driver.  As I understand it the way moving forward is that all
clocks that are inputs to this clock driver should be specified via
local names and that these local names should be somewhat stable
between different SoCs.  They should also, ideally, be more human
readable.

The mapping between local names and global names happens in the device
tree.  Specifically in the 10th patch in this series [1].

You can see that the clock "xo" (which is a local name) maps to
<&rpmhcc RPMH_CXO_CLK>.

It is OK that the global name for this clock in Linux is "bi_tcxo".
The string "xo" is _only_ used to look in the device tree to find the
clock this refers to.


A) If you are saying that the local clock name should have been
referred to as "bi_tcxo", then we need to go and change the bindings.
The bindings already say that the input clock is called "xo" and this
is true even without my series.  If we wanted to change this we'd also
need to go change some existing device tree files.  I don't think this
is the right way to go.

B) If you are saying that you don't like the idea of local names and
you'd rather use the old way of matching (relying on a global lookup
of a clock named "bi_tcxo"), I don't personally think that's right.
...but if the common clock maintainers say that's the way to jump then
I will.


Summary: I'm pretty sure it should be "xo" and you will have to update
your auto-generation code to handle the concept of local names.


> >                       },
> >                       .num_parents = 1,
> >                       .ops = &clk_alpha_pll_fabia_ops,
> > @@ -76,40 +76,32 @@ static struct clk_alpha_pll_postdiv disp_cc_pll0_out_even = {
> >
> >   static const struct parent_map disp_cc_parent_map_0[] = {
> >       { P_BI_TCXO, 0 },
> > -     { P_CORE_BI_PLL_TEST_SE, 7 },
> >   };
> >
> >   static const struct clk_parent_data disp_cc_parent_data_0[] = {
> > -     { .fw_name = "bi_tcxo" },
> > -     { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> > +     { .fw_name = "xo" },
> >   };
> >
> >   static const struct parent_map disp_cc_parent_map_1[] = {
> >       { P_BI_TCXO, 0 },
> >       { P_DP_PHY_PLL_LINK_CLK, 1 },
> >       { P_DP_PHY_PLL_VCO_DIV_CLK, 2 },
> > -     { P_CORE_BI_PLL_TEST_SE, 7 },
> >   };
> >
> >   static const struct clk_parent_data disp_cc_parent_data_1[] = {
> > -     { .fw_name = "bi_tcxo" },
> > -     { .fw_name = "dp_phy_pll_link_clk", .name = "dp_phy_pll_link_clk" },
> > -     { .fw_name = "dp_phy_pll_vco_div_clk",
> > -                             .name = "dp_phy_pll_vco_div_clk"},
> > -     { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> > +     { .fw_name = "xo" },
> > +     { .fw_name = "dp_phy_pll_link" },
> > +     { .fw_name = "dp_phy_pll_vco_div" },
>
> similar comments for these too. They would conflict with our HW design
> clock names.

By using ".fw_name" you are asserting that these are _local names_ for
the clocks.  Yet, they are missing from the binding.  As per above
argument, the right answer is _not_ to move back to the old global
name matching, so we definitely need to addthis to the binding.  When
thinking about adding the clocks to the binding, I think can hear
Rob's voice whispering into my ear that if I'm adding the name of a
clock to the binding I don't need the name to end with "_clk".  Again,
I think you need to update your auto-generation tools.


Summary: I think my change is correct here.


> >   };
> >
> >   static const struct parent_map disp_cc_parent_map_2[] = {
> >       { P_BI_TCXO, 0 },
> >       { P_DSI0_PHY_PLL_OUT_BYTECLK, 1 },
> > -     { P_CORE_BI_PLL_TEST_SE, 7 },
> >   };
> >
> >   static const struct clk_parent_data disp_cc_parent_data_2[] = {
> > -     { .fw_name = "bi_tcxo" },
> > -     { .fw_name = "dsi0_phy_pll_out_byteclk",
> > -                             .name = "dsi0_phy_pll_out_byteclk" },
> > -     { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> > +     { .fw_name = "xo" },
> > +     { .fw_name = "dsi_phy_pll_byte" },
> >   };
> >
> >   static const struct parent_map disp_cc_parent_map_3[] = {
> > @@ -117,40 +109,33 @@ static const struct parent_map disp_cc_parent_map_3[] = {
> >       { P_DISP_CC_PLL0_OUT_MAIN, 1 },
> >       { P_GPLL0_OUT_MAIN, 4 },
> >       { P_DISP_CC_PLL0_OUT_EVEN, 5 },
> > -     { P_CORE_BI_PLL_TEST_SE, 7 },
> >   };
> >
> >   static const struct clk_parent_data disp_cc_parent_data_3[] = {
> > -     { .fw_name = "bi_tcxo" },
> > +     { .fw_name = "xo" },
> >       { .hw = &disp_cc_pll0.clkr.hw },
> > -     { .fw_name = "gcc_disp_gpll0_clk_src" },
> > +     { .fw_name = "gpll0" },
>
> This is not the correct clock, we have a child/branch clock which
> requires to be turned ON "gcc_disp_gpll0_clk_src" when we switch to this
> source.

Whether or not it is the right clock depends on patch #10.  In patch
10 you can see that I specify that "gpll0" is:

<&gcc GCC_DISP_GPLL0_CLK_SRC>,

...which means that we end up with the same clock as before.  So I
think it is the correct clock.

Then we can have a debate about whether the binding should have called
this local clock something different.  I will say that the bindings
already describe an input clock that is called "gpll0".  Should this
have been called something else in the bindings?


Summary: I think my change is correct unless we want to change the
existing bindings to call this something other than "gpll0".


> >       { .hw = &disp_cc_pll0_out_even.clkr.hw },
> > -     { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> >   };
> >
> >   static const struct parent_map disp_cc_parent_map_4[] = {
> >       { P_BI_TCXO, 0 },
> >       { P_GPLL0_OUT_MAIN, 4 },
> > -     { P_CORE_BI_PLL_TEST_SE, 7 },
> >   };
> >
> >   static const struct clk_parent_data disp_cc_parent_data_4[] = {
> > -     { .fw_name = "bi_tcxo" },
> > -     { .fw_name = "gcc_disp_gpll0_clk_src" },
> > -     { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> > +     { .fw_name = "xo" },
> > +     { .fw_name = "gpll0" },
>
> same comment as above.
>
> >   };
> >
> >   static const struct parent_map disp_cc_parent_map_5[] = {
> >       { P_BI_TCXO, 0 },
> >       { P_DSI0_PHY_PLL_OUT_DSICLK, 1 },
> > -     { P_CORE_BI_PLL_TEST_SE, 7 },
> >   };
> >
> >   static const struct clk_parent_data disp_cc_parent_data_5[] = {
> > -     { .fw_name = "bi_tcxo" },
> > -     { .fw_name = "dsi0_phy_pll_out_dsiclk",
> > -                             .name = "dsi0_phy_pll_out_dsiclk" },
> > -     { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> > +     { .fw_name = "xo" },
> > +     { .fw_name = "dsi_phy_pll_pixel" },
> >   };
> >
> >   static const struct freq_tbl ftbl_disp_cc_mdss_ahb_clk_src[] = {
> > @@ -169,7 +154,7 @@ static struct clk_rcg2 disp_cc_mdss_ahb_clk_src = {
> >       .clkr.hw.init = &(struct clk_init_data){
> >               .name = "disp_cc_mdss_ahb_clk_src",
> >               .parent_data = disp_cc_parent_data_4,
> > -             .num_parents = 3,
> > +             .num_parents = ARRAY_SIZE(disp_cc_parent_data_4),
> >               .flags = CLK_SET_RATE_PARENT,
> >               .ops = &clk_rcg2_shared_ops,
> >       },
> > @@ -183,7 +168,7 @@ static struct clk_rcg2 disp_cc_mdss_byte0_clk_src = {
> >       .clkr.hw.init = &(struct clk_init_data){
> >               .name = "disp_cc_mdss_byte0_clk_src",
> >               .parent_data = disp_cc_parent_data_2,
> > -             .num_parents = 3,
> > +             .num_parents = ARRAY_SIZE(disp_cc_parent_data_2),
> >               .flags = CLK_SET_RATE_PARENT,
> >               .ops = &clk_byte2_ops,
> >       },
> > @@ -203,7 +188,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_aux_clk_src = {
> >       .clkr.hw.init = &(struct clk_init_data){
> >               .name = "disp_cc_mdss_dp_aux_clk_src",
> >               .parent_data = disp_cc_parent_data_0,
> > -             .num_parents = 2,
> > +             .num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
> >               .ops = &clk_rcg2_ops,
> >       },
> >   };
> > @@ -216,7 +201,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_crypto_clk_src = {
> >       .clkr.hw.init = &(struct clk_init_data){
> >               .name = "disp_cc_mdss_dp_crypto_clk_src",
> >               .parent_data = disp_cc_parent_data_1,
> > -             .num_parents = 4,
> > +             .num_parents = ARRAY_SIZE(disp_cc_parent_data_1),
> >               .flags = CLK_SET_RATE_PARENT,
> >               .ops = &clk_byte2_ops,
> >       },
> > @@ -230,7 +215,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_link_clk_src = {
> >       .clkr.hw.init = &(struct clk_init_data){
> >               .name = "disp_cc_mdss_dp_link_clk_src",
> >               .parent_data = disp_cc_parent_data_1,
> > -             .num_parents = 4,
> > +             .num_parents = ARRAY_SIZE(disp_cc_parent_data_1),
> >               .flags = CLK_SET_RATE_PARENT,
> >               .ops = &clk_byte2_ops,
> >       },
> > @@ -244,7 +229,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_pixel_clk_src = {
> >       .clkr.hw.init = &(struct clk_init_data){
> >               .name = "disp_cc_mdss_dp_pixel_clk_src",
> >               .parent_data = disp_cc_parent_data_1,
> > -             .num_parents = 4,
> > +             .num_parents = ARRAY_SIZE(disp_cc_parent_data_1),
> >               .flags = CLK_SET_RATE_PARENT,
> >               .ops = &clk_dp_ops,
> >       },
> > @@ -259,7 +244,7 @@ static struct clk_rcg2 disp_cc_mdss_esc0_clk_src = {
> >       .clkr.hw.init = &(struct clk_init_data){
> >               .name = "disp_cc_mdss_esc0_clk_src",
> >               .parent_data = disp_cc_parent_data_2,
> > -             .num_parents = 3,
> > +             .num_parents = ARRAY_SIZE(disp_cc_parent_data_2),
> >               .ops = &clk_rcg2_ops,
> >       },
> >   };
> > @@ -282,7 +267,7 @@ static struct clk_rcg2 disp_cc_mdss_mdp_clk_src = {
> >       .clkr.hw.init = &(struct clk_init_data){
> >               .name = "disp_cc_mdss_mdp_clk_src",
> >               .parent_data = disp_cc_parent_data_3,
> > -             .num_parents = 5,
> > +             .num_parents = ARRAY_SIZE(disp_cc_parent_data_3),
> >               .ops = &clk_rcg2_shared_ops,
> >       },
> >   };
> > @@ -295,7 +280,7 @@ static struct clk_rcg2 disp_cc_mdss_pclk0_clk_src = {
> >       .clkr.hw.init = &(struct clk_init_data){
> >               .name = "disp_cc_mdss_pclk0_clk_src",
> >               .parent_data = disp_cc_parent_data_5,
> > -             .num_parents = 3,
> > +             .num_parents = ARRAY_SIZE(disp_cc_parent_data_5),
> >               .flags = CLK_SET_RATE_PARENT,
> >               .ops = &clk_pixel_ops,
> >       },
> > @@ -310,7 +295,7 @@ static struct clk_rcg2 disp_cc_mdss_rot_clk_src = {
> >       .clkr.hw.init = &(struct clk_init_data){
> >               .name = "disp_cc_mdss_rot_clk_src",
> >               .parent_data = disp_cc_parent_data_3,
> > -             .num_parents = 5,
> > +             .num_parents = ARRAY_SIZE(disp_cc_parent_data_3),
> >               .ops = &clk_rcg2_shared_ops,
> >       },
> >   };
> > @@ -324,7 +309,7 @@ static struct clk_rcg2 disp_cc_mdss_vsync_clk_src = {
> >       .clkr.hw.init = &(struct clk_init_data){
> >               .name = "disp_cc_mdss_vsync_clk_src",
> >               .parent_data = disp_cc_parent_data_0,
> > -             .num_parents = 2,
> > +             .num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
> >               .ops = &clk_rcg2_shared_ops,
> >       },
> >   };
> >
>
> All the above code are auto-generated and we really do not want to
> hand-code.

This is about ARRAY_SIZE()?  Maybe you can update your auto-generation
script.  I think it's cleaner / more readable and it would have
prevented a previous problem I would have had to debug.  See commit
74c31ff9c84a ("clk: qcom: gpu_cc_gmu_clk_src has 5 parents, not 6").

...or is this about the removal of the test clock?  I removed the test
clock at Stephen's request.  Once I have done that then I will not
match your auto-generated code anyway, so you probably need to update
them.  If you can convince Stephen that we should add the test clock
back in then I have no objections, though we'd need to add it as an
optional clock to the bindings (or accept that fact that it uses a
global name lookup to match).


Summary: I think my change is correct, but if you and Stephen come to
some different agreement about the test clock I can change.


[1] https://lore.kernel.org/r/20200124144154.v2.10.I1a4b93fb005791e29a9dcf288fc8bd459a555a59@changeid

-Doug

Re: [PATCH v2 07/10] clk: qcom: Fix sc7180 gpucc parent data

[Doug Anderson]

Hi,

On Mon, Jan 27, 2020 at 9:55 PM Taniya Das <tdas@codeaurora.org> wrote:
>
> Hi Doug,
>
> Thanks for your patch. But as mentioned earlier we really want to avoid
> updating the auto generated code.

As per my reply [1], I think you need to update your auto-generation
tools to generate the code that results from my patch.  The existing
code either requires using global clock names to match or requires to
pass clocks in the dts that aren't documented in the bindings.  That
needs to be fixed and my patch fixes it.

[1] https://lore.kernel.org/r/CAD=FV=XFFCPj8S7-WPjPLFe=iygpkYiyMqbneY0DMXsMz+j73w@mail.gmail.com

-Doug

Re: [PATCH v2 10/10] arm64: dts: sc7180: Add clock controller nodes

[Doug Anderson]

Hi,

On Mon, Jan 27, 2020 at 9:58 PM Taniya Das <tdas@codeaurora.org> wrote:
>
> Hi Doug,
>
> Thanks for the patch.
>
> On 1/25/2020 4:12 AM, Douglas Anderson wrote:
> > From: Taniya Das <tdas@codeaurora.org>
> >
> > Add the display, video & graphics clock controller nodes supported on
> > SC7180.
> >
> > NOTE: the dispcc needs input clocks from various PHYs that aren't in
> > the device tree yet.  For now we'll leave these stubbed out with <0>,
> > which is apparently the magic way to do this.  These clocks aren't
> > really "optional" and this stubbing out method is apparently the best
> > way to handle it.
> >
> > Signed-off-by: Taniya Das <tdas@codeaurora.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Added includes
> > - Changed various parent names to match bindings / driver
> >
> >   arch/arm64/boot/dts/qcom/sc7180.dtsi | 41 ++++++++++++++++++++++++++++
> >   1 file changed, 41 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > index 8011c5fe2a31..ee3b4bade66b 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> > @@ -5,7 +5,9 @@
> >    * Copyright (c) 2019, The Linux Foundation. All rights reserved.
> >    */
> >
> > +#include <dt-bindings/clock/qcom,dispcc-sc7180.h>
> >   #include <dt-bindings/clock/qcom,gcc-sc7180.h>
> > +#include <dt-bindings/clock/qcom,gpucc-sc7180.h>
>
> My bad, but we are still missing the videocc header. I could send across
> the new patch.

Good point, thanks for noticing!  I won't spin with this right away as
we continue to discuss the driver / bindings patches.  If it turns out
that the rest of the series doesn't need to be spun I will be content
if Bjorn / Andy wants to make that fix when applying the patch, or I'm
happy to send a new patch.

-Doug

Re: [PATCH v2 01/10] clk: qcom: rcg2: Don't crash if our parent can't be found; return an error

[Matthias Kaehlcke]

On Fri, Jan 24, 2020 at 02:42:16PM -0800, Douglas Anderson wrote:
> When I got my clock parenting slightly wrong I ended up with a crash
> that looked like this:
> 
>   Unable to handle kernel NULL pointer dereference at virtual
>   address 0000000000000000
>   ...
>   pc : clk_hw_get_rate+0x14/0x44
>   ...
>   Call trace:
>    clk_hw_get_rate+0x14/0x44
>    _freq_tbl_determine_rate+0x94/0xfc
>    clk_rcg2_determine_rate+0x2c/0x38
>    clk_core_determine_round_nolock+0x4c/0x88
>    clk_core_round_rate_nolock+0x6c/0xa8
>    clk_core_round_rate_nolock+0x9c/0xa8
>    clk_core_set_rate_nolock+0x70/0x180
>    clk_set_rate+0x3c/0x6c
>    of_clk_set_defaults+0x254/0x360
>    platform_drv_probe+0x28/0xb0
>    really_probe+0x120/0x2dc
>    driver_probe_device+0x64/0xfc
>    device_driver_attach+0x4c/0x6c
>    __driver_attach+0xac/0xc0
>    bus_for_each_dev+0x84/0xcc
>    driver_attach+0x2c/0x38
>    bus_add_driver+0xfc/0x1d0
>    driver_register+0x64/0xf8
>    __platform_driver_register+0x4c/0x58
>    msm_drm_register+0x5c/0x60
>    ...
> 
> It turned out that clk_hw_get_parent_by_index() was returning NULL and
> we weren't checking.  Let's check it so that we don't crash.
> 
> Fixes: ac269395cdd8 ("clk: qcom: Convert to clk_hw based provider APIs")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> I haven't gone back and tried to reproduce this same crash on older
> kernels, but I'll put the blame on commit ac269395cdd8 ("clk: qcom:
> Convert to clk_hw based provider APIs").  Before that if we got a NULL
> parent back it was fine and dandy since a NULL "struct clk" is valid
> to use but a NULL "struct clk_hw" is not.
> 
> Changes in v2:
> - Patch ("clk: qcom: rcg2: Don't crash...") new for v2.
> 
>  drivers/clk/qcom/clk-rcg2.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index da045b200def..9098001ac805 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -218,6 +218,9 @@ static int _freq_tbl_determine_rate(struct clk_hw *hw, const struct freq_tbl *f,
>  
>  	clk_flags = clk_hw_get_flags(hw);
>  	p = clk_hw_get_parent_by_index(hw, index);
> +	if (!p)
> +		return -EINVAL;
> +
>  	if (clk_flags & CLK_SET_RATE_PARENT) {
>  		rate = f->freq;
>  		if (f->pre_div) {

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v2 05/10] clk: qcom: Fix sc7180 dispcc parent data

[Stephen Boyd]

Quoting Douglas Anderson (2020-01-24 14:42:20)
> 
> diff --git a/drivers/clk/qcom/dispcc-sc7180.c b/drivers/clk/qcom/dispcc-sc7180.c
> index 30c1e25d3edb..380eca3f847d 100644
> --- a/drivers/clk/qcom/dispcc-sc7180.c
> +++ b/drivers/clk/qcom/dispcc-sc7180.c
> @@ -76,40 +76,32 @@ static struct clk_alpha_pll_postdiv disp_cc_pll0_out_even = {
>  
>  static const struct parent_map disp_cc_parent_map_0[] = {
>         { P_BI_TCXO, 0 },
> -       { P_CORE_BI_PLL_TEST_SE, 7 },
>  };
>  
>  static const struct clk_parent_data disp_cc_parent_data_0[] = {
> -       { .fw_name = "bi_tcxo" },
> -       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> +       { .fw_name = "xo" },

If we can make the binding match the code here and keep saying "bi_tcxo"
then that is preferred. That way we don't have to see bi_tcxo changing
and qcom folks are happy to keep the weird name. The name in the binding
is really up to the binding writer.

>  };
>  
>  static const struct parent_map disp_cc_parent_map_1[] = {
>         { P_BI_TCXO, 0 },
>         { P_DP_PHY_PLL_LINK_CLK, 1 },
>         { P_DP_PHY_PLL_VCO_DIV_CLK, 2 },
> -       { P_CORE_BI_PLL_TEST_SE, 7 },
>  };
[...]
> @@ -203,7 +188,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_aux_clk_src = {
>         .clkr.hw.init = &(struct clk_init_data){
>                 .name = "disp_cc_mdss_dp_aux_clk_src",
>                 .parent_data = disp_cc_parent_data_0,
> -               .num_parents = 2,
> +               .num_parents = ARRAY_SIZE(disp_cc_parent_data_0),

Can you split this ARRAY_SIZE() stuff to another patch? That will keep
focus on what's relevant here without distracting from the patch
contents. I know that parent array size is changing, but I don't want it
to be changing this line too.

>                 .ops = &clk_rcg2_ops,
>         },
>  };

Re: [PATCH v2 05/10] clk: qcom: Fix sc7180 dispcc parent data

[Doug Anderson]

Hi,

On Tue, Jan 28, 2020 at 4:51 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Douglas Anderson (2020-01-24 14:42:20)
> >
> > diff --git a/drivers/clk/qcom/dispcc-sc7180.c b/drivers/clk/qcom/dispcc-sc7180.c
> > index 30c1e25d3edb..380eca3f847d 100644
> > --- a/drivers/clk/qcom/dispcc-sc7180.c
> > +++ b/drivers/clk/qcom/dispcc-sc7180.c
> > @@ -76,40 +76,32 @@ static struct clk_alpha_pll_postdiv disp_cc_pll0_out_even = {
> >
> >  static const struct parent_map disp_cc_parent_map_0[] = {
> >         { P_BI_TCXO, 0 },
> > -       { P_CORE_BI_PLL_TEST_SE, 7 },
> >  };
> >
> >  static const struct clk_parent_data disp_cc_parent_data_0[] = {
> > -       { .fw_name = "bi_tcxo" },
> > -       { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> > +       { .fw_name = "xo" },
>
> If we can make the binding match the code here and keep saying "bi_tcxo"
> then that is preferred. That way we don't have to see bi_tcxo changing
> and qcom folks are happy to keep the weird name. The name in the binding
> is really up to the binding writer.

v3 is now out and it still says "bi_tcxo" and generally uses the
"internal" name.  The big exception is msm8998's gpucc.  It seems like
a whole bunch of work has been done to move that over to more "purist"
(AKA logical) names and I didn't want to undo that work.  If we should
move msm8998 to match everyone else then hopefully someone can do it
as a followup patch?


> >  };
> >
> >  static const struct parent_map disp_cc_parent_map_1[] = {
> >         { P_BI_TCXO, 0 },
> >         { P_DP_PHY_PLL_LINK_CLK, 1 },
> >         { P_DP_PHY_PLL_VCO_DIV_CLK, 2 },
> > -       { P_CORE_BI_PLL_TEST_SE, 7 },
> >  };
> [...]
> > @@ -203,7 +188,7 @@ static struct clk_rcg2 disp_cc_mdss_dp_aux_clk_src = {
> >         .clkr.hw.init = &(struct clk_init_data){
> >                 .name = "disp_cc_mdss_dp_aux_clk_src",
> >                 .parent_data = disp_cc_parent_data_0,
> > -               .num_parents = 2,
> > +               .num_parents = ARRAY_SIZE(disp_cc_parent_data_0),
>
> Can you split this ARRAY_SIZE() stuff to another patch? That will keep
> focus on what's relevant here without distracting from the patch
> contents. I know that parent array size is changing, but I don't want it
> to be changing this line too.

It has been done.

-Doug