clk: qcom: Drop use of pxo/cxo_board for rpm devices

qcom,rpmcc describe 2 different kind of device. Currently we have definition for rpm-smd based device but we lack Documentation for simple rpm based device.

Add the missing clk for ipq806x, apq8060, msm8660 and apq8064 and provide and additional example.

Signed-off-by Christian Marangi <[email protected]>

---
 .../devicetree/bindings/clock/qcom,rpmcc.yaml | 77 ++++++++++++++++++-
 1 file changed, 73 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml +0 -0

index 9d296b89a8d0..028eb0277495 100644

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml +12 -0

+  properties:
+    compatible:
+      contains:
+        enum:
+          - qcom,rpmcc-apq8060
+          - qcom,rpmcc-ipq806x
+          - qcom,rpmcc-msm8660
+
+then:
+  properties:
+    clocks:
+      description: pxo clock

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml +37 -0

+    properties:
+      compatible:
+        contains:
+          const: qcom,rpmcc-apq8064
+  then:
+    properties:
+      clocks:
+        items:
+          - description: pxo clock
+          - description: cxo clock
+
+      clock-names:
+        items:
+          - const: pxo
+          - const: cxo
+
+    required:
+      - clocks
+      - clock-names
+
+  else:
+    properties:
+      clocks:
+        description: xo clock
+
+      clock-names:
+        const: xo
+
 additionalProperties: false
 
 examples:

@@ -73,3 +122,23 @@ examples:
             };
         };
     };
+
+  - |
+    rpm {
+        clock-controller {
+            compatible = "qcom,rpmcc-ipq806x", "qcom,rpmcc";
+            #clock-cells = <1>;
+            clocks = <&pxo_board>;
+            clock-names = "pxo";
+        };

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml +13 -0

+
+else:
+  if:
+    properties:
+      compatible:
+        contains:
+          const: qcom,rpmcc-apq8064
+  then:
+    properties:
+      clocks:
+        items:
+          - description: pxo clock
+          - description: cxo clock

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml

Quoted 17 lines · Christian Marangi

qcom,rpmcc describe 2 different kind of device. Currently we have definition for rpm-smd based device but we lack Documentation for simple rpm based device.

Add the missing clk for ipq806x, apq8060, msm8660 and apq8064 and provide and additional example.

Signed-off-by Christian Marangi <[email protected]>

---
 .../devicetree/bindings/clock/qcom,rpmcc.yaml | 77 ++++++++++++++++++-
 1 file changed, 73 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml +0 -0

index 9d296b89a8d0..028eb0277495 100644

[,,,,]

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml properties: · line 78

Quoted 12 lines

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml +12 -0

+
+then:
+  properties:
+    clocks:
+      description: pxo clock
+
+    clock-names:
+      const: pxo
+
+  required:
+    - clocks
+    - clock-names

I don't think you can not mark these properties as required, older schemas do not have them.

Quoted 13 lines

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml +13 -0

+
+else:
+  if:
+    properties:
+      compatible:
+        contains:
+          const: qcom,rpmcc-apq8064
+  then:
+    properties:
+      clocks:
+        items:
+          - description: pxo clock
+          - description: cxo clock

[...]

Signature · 2 lines

With best wishes
Dmitry

Quoted 20 lines · Dmitry Baryshkov

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml

Quoted 17 lines · Christian Marangi

qcom,rpmcc describe 2 different kind of device. Currently we have definition for rpm-smd based device but we lack Documentation for simple rpm based device.

Add the missing clk for ipq806x, apq8060, msm8660 and apq8064 and provide and additional example.

Signed-off-by Christian Marangi <[email protected]>

---
 .../devicetree/bindings/clock/qcom,rpmcc.yaml | 77 ++++++++++++++++++-
 1 file changed, 73 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml +0 -0

index 9d296b89a8d0..028eb0277495 100644

[,,,,]

Quoted 16 lines

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml properties: · line 78

Quoted 12 lines

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml +12 -0

+
+then:
+  properties:
+    clocks:
+      description: pxo clock
+
+    clock-names:
+      const: pxo
+
+  required:
+    - clocks
+    - clock-names

I don't think you can not mark these properties as required, older schemas do not have them.

Well considering we changed rpmcc to parent_data and rpm clock require pxo clock as parents it seems to be they should be required.

Actually no idea why this wasn't required before. Probably because this schema described only rpm-smd and not old rpm?

Quoted 14 lines

Quoted 13 lines

Documentation/devicetree/bindings/clock/qcom,rpmcc.yaml +13 -0

+
+else:
+  if:
+    properties:
+      compatible:
+        contains:
+          const: qcom,rpmcc-apq8064
+  then:
+    properties:
+      clocks:
+        items:
+          - description: pxo clock
+          - description: cxo clock

[...]

Fix dtbs_check warning for new rpmcc Documentation changes and add the required clocks.

Signed-off-by Christian Marangi <[email protected]>

---
 arch/arm/boot/dts/qcom-apq8064.dtsi | 2 ++
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 2 ++
 arch/arm/boot/dts/qcom-msm8660.dtsi | 4 +++-
 3 files changed, 7 insertions(+), 1 deletion(-)

arch/arm/boot/dts/qcom-apq8064.dtsi +2 -0

index 91adcbd54b17..6a88e616ea01 100644
@@ -853,6 +853,8 @@ rpm@108000 {
                        rpmcc: clock-controller {
                                compatible = "qcom,rpmcc-apq8064", "qcom,rpmcc";
                                #clock-cells = <1>;
+                               clocks = <&pxo_board>, <&cxo_board>;
+                               clock-names = "pxo", "cxo";
                        };
 
                        regulators {

arch/arm/boot/dts/qcom-ipq8064.dtsi +2 -0

index 4b475d98343c..1425a4e4283f 100644
@@ -773,6 +773,8 @@ rpm: rpm@108000 {
                        rpmcc: clock-controller {
                                compatible = "qcom,rpmcc-ipq806x", "qcom,rpmcc";
                                #clock-cells = <1>;
+                               clocks = <&pxo_board>;
+                               clock-names = "pxo";
                        };
                };
 

arch/arm/boot/dts/qcom-msm8660.dtsi +3 -2

index b9cded35b1cc..63a501c63cf8 100644
@@ -56,7 +56,7 @@ cxo_board {
                        clock-frequency = <19200000>;
                };
 
-               pxo_board {
+               pxo_board: pxo_board {
                        compatible = "fixed-clock";
                        #clock-cells = <0>;
                        clock-frequency = <27000000>;

@@ -411,6 +411,8 @@ rpm: rpm@104000 {
                        rpmcc: clock-controller {
                                compatible = "qcom,rpmcc-msm8660", "qcom,rpmcc";
                                #clock-cells = <1>;
+                               clocks = <&pxo_board>;
+                               clock-names = "pxo";
                        };
 
                        pm8901-regulators {
--

2.36.1

Fix dtbs_check warning for new rpmcc Documentation changes and add the required clocks.

Quoted 2 lines · Christian Marangi

Fix dtbs_check warning for new rpmcc Documentation changes and add the required clocks.

There is no warning in the kernel, right? So the commit is not correct.

Only the second part of your sentence applies, but you should extend it.

Best regards,
Krzysztof

Quoted 6 lines · Krzysztof Kozlowski

Quoted 2 lines · Christian Marangi

Fix dtbs_check warning for new rpmcc Documentation changes and add the required clocks.

There is no warning in the kernel, right? So the commit is not correct.

Oh ok, the warning is generated by the new Documentation.

Quoted 5 lines · Christian Marangi

Quoted 4 lines · Krzysztof Kozlowski

Quoted 2 lines · Christian Marangi

Fix dtbs_check warning for new rpmcc Documentation changes and add the required clocks.

Quoted 4 lines

Quoted 2 lines

There is no warning in the kernel, right? So the commit is not correct.

Oh ok, the warning is generated by the new Documentation.

Patches, especially DTS, might go via different trees, so the moment DTS is applied there might be no such warning.

Best regards,
Krzysztof

Quoted 6 lines · Krzysztof Kozlowski

Quoted 5 lines · Christian Marangi

Quoted 4 lines · Krzysztof Kozlowski

Quoted 2 lines · Christian Marangi

Fix dtbs_check warning for new rpmcc Documentation changes and add the required clocks.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

There is no warning in the kernel, right? So the commit is not correct.

Oh ok, the warning is generated by the new Documentation.

Patches, especially DTS, might go via different trees, so the moment DTS is applied there might be no such warning.

I'm still confused about this topic...
With this kind of change, I notice I sent Documentation change and then rob bot complain about dtbs_check having warning...

So the correct way is to send Documentation change and fix dtbs_check warning in the same commit OR keep what I'm doing with sending Documentation changes and fix DTS in a separate commit?

Quoted 16 lines · Christian Marangi

Quoted 6 lines · Krzysztof Kozlowski

Quoted 5 lines · Christian Marangi

Quoted 4 lines · Krzysztof Kozlowski

Quoted 2 lines · Christian Marangi

Fix dtbs_check warning for new rpmcc Documentation changes and add the required clocks.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

There is no warning in the kernel, right? So the commit is not correct.

Oh ok, the warning is generated by the new Documentation.

Patches, especially DTS, might go via different trees, so the moment DTS is applied there might be no such warning.

Quoted 7 lines

I'm still confused about this topic...
With this kind of change, I notice I sent Documentation change and then rob bot complain about dtbs_check having warning...

So the correct way is to send Documentation change and fix dtbs_check warning in the same commit OR keep what I'm doing with sending Documentation changes and fix DTS in a separate commit?

Binding is almost always separate from DTS and always separate from driver. The order depends on what you're doing. If you bring ABI break change to bindings, then the order does not matter, because each order will be non-bisectable. Because you broke ABI. That's the case in this patchset.

For other cases, usually bindings patches should be the first in patchset.

How it goes via maintainer trees is not your problem here. Patches might go together or might go separate.

Anyway it was not the topic of my comment. Comment was about not specific commit msg which does not fit the Linux kernel process and does not fit git history once applied by maintainer. It fits even less when backported to stable kernels, which you commit msg encourages to do.

Convert clk-rpm driver to parent_data API and change parent names to cxo and pxo instead of the board variant as pxo_board is an ancient name and pxo should be used instead.

Signed-off-by Christian Marangi <[email protected]>

---
 drivers/clk/qcom/clk-rpm.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

drivers/clk/qcom/clk-rpm.c +0 -0

index a18811c38018..d45a8b0fc97c 100644

drivers/clk/qcom/clk-rpm.c +2 -0

+};
+

Quoted 18 lines · Christian Marangi

Convert clk-rpm driver to parent_data API and change parent names to cxo and pxo instead of the board variant as pxo_board is an ancient name and pxo should be used instead.

Signed-off-by Christian Marangi <[email protected]>

---
 drivers/clk/qcom/clk-rpm.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

drivers/clk/qcom/clk-rpm.c +0 -0

index a18811c38018..d45a8b0fc97c 100644
@@ -23,6 +23,14 @@
 #define QCOM_RPM_SCALING_ENABLE_ID                     0x2
 #define QCOM_RPM_XO_MODE_ON                            0x2
 

drivers/clk/qcom/clk-rpm.c line 27

Quoted 2 lines

drivers/clk/qcom/clk-rpm.c +2 -0

+static const struct clk_parent_data gcc_pxo[] = {
+       { .fw_name = "pxo", .name = "pxo" },

This means that the core clock would look for the DT clock named 'pxo' with the fallback to the global clock named 'pxo'. Fallback is required to keep compatibility with older DT files, not having the clocks/clock-names properties.
For all other platforms we were using as a fallback the 'pxo_board' clock (defined in the DT) rather than the 'pxo' (hackily injected by the gcc driver).
I'd suggest following this pattern and using .fw_name = "pxo', .name = "pxo_board".

Same applies to the CXO clock.

Quoted 2 lines

drivers/clk/qcom/clk-rpm.c +2 -0

+};
+

Signature · 2 lines

With best wishes
Dmitry

Convert clk-rpm driver to parent_data API and change parent names to cxo and pxo instead of the board variant as pxo_board is an ancient name and pxo should be used instead.

Signed-off-by Christian Marangi <[email protected]>

---
 drivers/clk/qcom/clk-rpm.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

drivers/clk/qcom/clk-rpm.c +0 -0

index a18811c38018..d45a8b0fc97c 100644

drivers/clk/qcom/clk-rpm.c +14 -8

+};
+
+static const struct clk_parent_data gcc_cxo[] = {
+	{ .fw_name = "cxo", .name = "cxo" },
+};
+
 #define DEFINE_CLK_RPM(_platform, _name, _active, r_id)			      \
 	static struct clk_rpm _platform##_##_active;			      \
 	static struct clk_rpm _platform##_##_name = {			      \

@@ -32,8 +40,8 @@
 		.hw.init = &(struct clk_init_data){			      \
 			.ops = &clk_rpm_ops,				      \
 			.name = #_name,					      \
-			.parent_names = (const char *[]){ "pxo_board" },      \
-			.num_parents = 1,				      \
+			.parent_data = gcc_pxo,				      \
+			.num_parents = ARRAY_SIZE(gcc_pxo),		      \
 		},							      \
 	};								      \
 	static struct clk_rpm _platform##_##_active = {			      \

@@ -44,8 +52,8 @@
 		.hw.init = &(struct clk_init_data){			      \
 			.ops = &clk_rpm_ops,				      \
 			.name = #_active,				      \
-			.parent_names = (const char *[]){ "pxo_board" },      \
-			.num_parents = 1,				      \
+			.parent_data = gcc_pxo,				      \
+			.num_parents = ARRAY_SIZE(gcc_pxo),		      \
 		},							      \
 	}
 

@@ -56,8 +64,8 @@
 		.hw.init = &(struct clk_init_data){			      \
 			.ops = &clk_rpm_xo_ops,			      \
 			.name = #_name,					      \
-			.parent_names = (const char *[]){ "cxo_board" },      \
-			.num_parents = 1,				      \
+			.parent_data = gcc_cxo,				      \
+			.num_parents = ARRAY_SIZE(gcc_cxo),		      \
 		},							      \
 	}
 

@@ -68,8 +76,8 @@
 		.hw.init = &(struct clk_init_data){			      \
 			.ops = &clk_rpm_fixed_ops,			      \
 			.name = #_name,					      \
-			.parent_names = (const char *[]){ "pxo" },	      \
-			.num_parents = 1,				      \
+			.parent_data = gcc_pxo,				      \
+			.num_parents = ARRAY_SIZE(gcc_pxo),		      \
 		},							      \
 	}
 

Quoted 17 lines · Christian Marangi

Convert clk-rpm driver to parent_data API and change parent names to cxo and pxo instead of the board variant as pxo_board is an ancient name and pxo should be used instead.

Signed-off-by Christian Marangi <[email protected]>

---
 drivers/clk/qcom/clk-rpm.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

drivers/clk/qcom/clk-rpm.c +0 -0

index a18811c38018..d45a8b0fc97c 100644
@@ -23,6 +23,14 @@
 #define QCOM_RPM_SCALING_ENABLE_ID			0x2
 #define QCOM_RPM_XO_MODE_ON				0x2
 

drivers/clk/qcom/clk-rpm.c line 27

Quoted 2 lines

drivers/clk/qcom/clk-rpm.c +2 -0

+static const struct clk_parent_data gcc_pxo[] = {
+	{ .fw_name = "pxo", .name = "pxo" },

The .name lookup should include _board, as the old DTs expect that. If I understand correctly though, you need to preserve both pxo and pxo_board, as they were separate, not sure though..

Konrad

Quoted 53 lines

drivers/clk/qcom/clk-rpm.c +14 -8

+};
+
+static const struct clk_parent_data gcc_cxo[] = {
+	{ .fw_name = "cxo", .name = "cxo" },
+};
+
 #define DEFINE_CLK_RPM(_platform, _name, _active, r_id)			      \
 	static struct clk_rpm _platform##_##_active;			      \
 	static struct clk_rpm _platform##_##_name = {			      \

@@ -32,8 +40,8 @@
 		.hw.init = &(struct clk_init_data){			      \
 			.ops = &clk_rpm_ops,				      \
 			.name = #_name,					      \
-			.parent_names = (const char *[]){ "pxo_board" },      \
-			.num_parents = 1,				      \
+			.parent_data = gcc_pxo,				      \
+			.num_parents = ARRAY_SIZE(gcc_pxo),		      \
 		},							      \
 	};								      \
 	static struct clk_rpm _platform##_##_active = {			      \

@@ -44,8 +52,8 @@
 		.hw.init = &(struct clk_init_data){			      \
 			.ops = &clk_rpm_ops,				      \
 			.name = #_active,				      \
-			.parent_names = (const char *[]){ "pxo_board" },      \
-			.num_parents = 1,				      \
+			.parent_data = gcc_pxo,				      \
+			.num_parents = ARRAY_SIZE(gcc_pxo),		      \
 		},							      \
 	}
 

@@ -56,8 +64,8 @@
 		.hw.init = &(struct clk_init_data){			      \
 			.ops = &clk_rpm_xo_ops,			      \
 			.name = #_name,					      \
-			.parent_names = (const char *[]){ "cxo_board" },      \
-			.num_parents = 1,				      \
+			.parent_data = gcc_cxo,				      \
+			.num_parents = ARRAY_SIZE(gcc_cxo),		      \
 		},							      \
 	}
 

@@ -68,8 +76,8 @@
 		.hw.init = &(struct clk_init_data){			      \
 			.ops = &clk_rpm_fixed_ops,			      \
 			.name = #_name,					      \
-			.parent_names = (const char *[]){ "pxo" },	      \
-			.num_parents = 1,				      \
+			.parent_data = gcc_pxo,				      \
+			.num_parents = ARRAY_SIZE(gcc_pxo),		      \
 		},							      \
 	}
 

Drivers expect to call pxo/cxo_board clock as pxo/cxo.
Use clock-output-names to set the correct naming for these fixed clock to save drivers that are migrating to parent_data and still use parent_names.

Signed-off-by Christian Marangi <[email protected]>

---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 2 ++
 1 file changed, 2 insertions(+)

arch/arm/boot/dts/qcom-ipq8064.dtsi +2 -1

index 1425a4e4283f..720e27e29627 100644
@@ -302,12 +302,14 @@ cxo_board: cxo_board {
                        compatible = "fixed-clock";
                        #clock-cells = <0>;
                        clock-frequency = <25000000>;
+                       clock-output-names = "cxo";
                };
 
                pxo_board: pxo_board {
                        compatible = "fixed-clock";
                        #clock-cells = <0>;
                        clock-frequency = <25000000>;
+                       clock-output-names = "pxo";
                };
 
                sleep_clk: sleep_clk {
--

2.36.1

Quoted 5 lines · Christian Marangi

Drivers expect to call pxo/cxo_board clock as pxo/cxo.
Use clock-output-names to set the correct naming for these fixed clock to save drivers that are migrating to parent_data and still use parent_names.

Please no. There is no reason to add these names. Let's follow the example of other platforms.

Quoted 28 lines

Signed-off-by Christian Marangi <[email protected]>

---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 2 ++
 1 file changed, 2 insertions(+)

arch/arm/boot/dts/qcom-ipq8064.dtsi +2 -1

index 1425a4e4283f..720e27e29627 100644
@@ -302,12 +302,14 @@ cxo_board: cxo_board {
                        compatible = "fixed-clock";
                        #clock-cells = <0>;
                        clock-frequency = <25000000>;
+                       clock-output-names = "cxo";
                };
 
                pxo_board: pxo_board {
                        compatible = "fixed-clock";
                        #clock-cells = <0>;
                        clock-frequency = <25000000>;
+                       clock-output-names = "pxo";
                };
 
                sleep_clk: sleep_clk {
--

2.36.1

Signature · 2 lines

With best wishes
Dmitry

Drivers expect to call pxo/cxo_board clock as pxo/cxo.
Use clock-output-names to set the correct naming for these fixed clock to save drivers that are migrating to parent_data and still use parent_names.

Signed-off-by Christian Marangi <[email protected]>

---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 2 ++
 1 file changed, 2 insertions(+)

arch/arm/boot/dts/qcom-ipq8064.dtsi +2 -0

index 1425a4e4283f..720e27e29627 100644
@@ -302,12 +302,14 @@ cxo_board: cxo_board {
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
 			clock-frequency = <25000000>;
+			clock-output-names = "cxo";
 		};
 
 		pxo_board: pxo_board {
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
 			clock-frequency = <25000000>;
+			clock-output-names = "pxo";
 		};
 
 		sleep_clk: sleep_clk {