[PATCH 0/3] Convert {a,i}pq8064 SATA AHCI controller bindings to dtschema

[PATCH 0/3] Convert {a,i}pq8064 SATA AHCI controller bindings to dtschema

[Rayyan Ansari]

Hello,
The following patches convert the old text bindings to dtschema by using
ahci-platform.yaml, whilst also fixing the device trees to actually
adhere to this schema.

Thanks,
Rayyan

Rayyan Ansari (3):
  ARM: dts: qcom: {a,i}pq8064: correct clock-names in sata node
  dt-bindings: ata: qcom,ipq806x-ahci: use dtschema
  dt-bindings: ata: qcom,apq8064-ahci: add to dtschema

 .../bindings/ata/ahci-platform.yaml           | 42 +++++++++++++++-
 .../devicetree/bindings/ata/qcom-sata.txt     | 48 -------------------
 arch/arm/boot/dts/qcom/qcom-apq8064.dtsi      |  4 +-
 arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi      |  2 +-
 4 files changed, 43 insertions(+), 53 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/ata/qcom-sata.txt

-- 
2.45.2

[PATCH 1/3] ARM: dts: qcom: {a,i}pq8064: correct clock-names in sata node

[Rayyan Ansari]

Correct the clock-names in the AHCI SATA controller node to adhere to
the bindings.

Signed-off-by: Rayyan Ansari <rayyan.ansari@linaro.org>
---
 arch/arm/boot/dts/qcom/qcom-apq8064.dtsi | 4 ++--
 arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
index 81cf387e1817..277bde958d0e 100644
--- a/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
@@ -889,9 +889,9 @@ sata0: sata@29000000 {
 				 <&gcc SATA_PMALIVE_CLK>;
 			clock-names = "slave_iface",
 				      "iface",
-				      "bus",
+				      "core",
 				      "rxoob",
-				      "core_pmalive";
+				      "pmalive";
 
 			assigned-clocks = <&gcc SATA_RXOOB_CLK>,
 					  <&gcc SATA_PMALIVE_CLK>;
diff --git a/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi
index da0fd75f4711..dd974eb4065f 100644
--- a/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi
@@ -1292,7 +1292,7 @@ sata: sata@29000000 {
 				 <&gcc SATA_A_CLK>,
 				 <&gcc SATA_RXOOB_CLK>,
 				 <&gcc SATA_PMALIVE_CLK>;
-			clock-names = "slave_face", "iface", "core",
+			clock-names = "slave_iface", "iface", "core",
 					"rxoob", "pmalive";
 
 			assigned-clocks = <&gcc SATA_RXOOB_CLK>, <&gcc SATA_PMALIVE_CLK>;
-- 
2.45.2

Re: [PATCH 1/3] ARM: dts: qcom: {a,i}pq8064: correct clock-names in sata node

[Niklas Cassel]

On Tue, Jul 16, 2024 at 11:45:59AM +0100, Rayyan Ansari wrote:
> Correct the clock-names in the AHCI SATA controller node to adhere to
> the bindings.
> 
> Signed-off-by: Rayyan Ansari <rayyan.ansari@linaro.org>

Hello Rayyan,

This patch is 1/3, so first in the series.
A patch that is first in the series usually has no other dependencies.
(Unless referencing another series in the cover-letter.)

So is this a fix that can be sent out separately and picked up the
QCOM maintainers / ARM DT maintainers directly, or does this patch
actually depend on patches 2-3 ?

If the former, I suggest that you send out patch 1/3 as a standalone
fix, since it does not need to be blocked by unrelated DT binding
conversion.

If the latter, perhaps reorder the patches and improve the commit log
for this patch.


Kind regards,
Niklas


> ---
>  arch/arm/boot/dts/qcom/qcom-apq8064.dtsi | 4 ++--
>  arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
> index 81cf387e1817..277bde958d0e 100644
> --- a/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
> +++ b/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
> @@ -889,9 +889,9 @@ sata0: sata@29000000 {
>  				 <&gcc SATA_PMALIVE_CLK>;
>  			clock-names = "slave_iface",
>  				      "iface",
> -				      "bus",
> +				      "core",
>  				      "rxoob",
> -				      "core_pmalive";
> +				      "pmalive";
>  
>  			assigned-clocks = <&gcc SATA_RXOOB_CLK>,
>  					  <&gcc SATA_PMALIVE_CLK>;
> diff --git a/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi
> index da0fd75f4711..dd974eb4065f 100644
> --- a/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi
> +++ b/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi
> @@ -1292,7 +1292,7 @@ sata: sata@29000000 {
>  				 <&gcc SATA_A_CLK>,
>  				 <&gcc SATA_RXOOB_CLK>,
>  				 <&gcc SATA_PMALIVE_CLK>;
> -			clock-names = "slave_face", "iface", "core",
> +			clock-names = "slave_iface", "iface", "core",
>  					"rxoob", "pmalive";
>  
>  			assigned-clocks = <&gcc SATA_RXOOB_CLK>, <&gcc SATA_PMALIVE_CLK>;
> -- 
> 2.45.2
> 

Re: [PATCH 1/3] ARM: dts: qcom: {a,i}pq8064: correct clock-names in sata node

[Rayyan Ansari]

On Wed Jul 17, 2024 at 9:45 AM BST, Niklas Cassel wrote:
> On Tue, Jul 16, 2024 at 11:45:59AM +0100, Rayyan Ansari wrote:
> > Correct the clock-names in the AHCI SATA controller node to adhere to
> > the bindings.
> > 
> > Signed-off-by: Rayyan Ansari <rayyan.ansari@linaro.org>
>
> Hello Rayyan,
>
> This patch is 1/3, so first in the series.
> A patch that is first in the series usually has no other dependencies.
> (Unless referencing another series in the cover-letter.)
>
> So is this a fix that can be sent out separately and picked up the
> QCOM maintainers / ARM DT maintainers directly, or does this patch
> actually depend on patches 2-3 ?

Hi Niklas,

Yes, this patch does not depend on the following two patches, I just
thought that sending this as a series would make sense given that
patches 2-3 would surface this error (as we can run dtbs_check against
yaml bindings but not text bindings).

> If the former, I suggest that you send out patch 1/3 as a standalone
> fix, since it does not need to be blocked by unrelated DT binding
> conversion.

Ah okay - for v2 I'll send patch 1 on its own, and then patch 2 & 3 as a
series.

> If the latter, perhaps reorder the patches and improve the commit log
> for this patch.
>
>
> Kind regards,
> Niklas

Thanks,
Rayyan

> > ---
> >  arch/arm/boot/dts/qcom/qcom-apq8064.dtsi | 4 ++--
> >  arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi | 2 +-
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
> > index 81cf387e1817..277bde958d0e 100644
> > --- a/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
> > +++ b/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
> > @@ -889,9 +889,9 @@ sata0: sata@29000000 {
> >  				 <&gcc SATA_PMALIVE_CLK>;
> >  			clock-names = "slave_iface",
> >  				      "iface",
> > -				      "bus",
> > +				      "core",
> >  				      "rxoob",
> > -				      "core_pmalive";
> > +				      "pmalive";
> >  
> >  			assigned-clocks = <&gcc SATA_RXOOB_CLK>,
> >  					  <&gcc SATA_PMALIVE_CLK>;
> > diff --git a/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi
> > index da0fd75f4711..dd974eb4065f 100644
> > --- a/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi
> > +++ b/arch/arm/boot/dts/qcom/qcom-ipq8064.dtsi
> > @@ -1292,7 +1292,7 @@ sata: sata@29000000 {
> >  				 <&gcc SATA_A_CLK>,
> >  				 <&gcc SATA_RXOOB_CLK>,
> >  				 <&gcc SATA_PMALIVE_CLK>;
> > -			clock-names = "slave_face", "iface", "core",
> > +			clock-names = "slave_iface", "iface", "core",
> >  					"rxoob", "pmalive";
> >  
> >  			assigned-clocks = <&gcc SATA_RXOOB_CLK>, <&gcc SATA_PMALIVE_CLK>;
> > -- 
> > 2.45.2
> > 

Re: [PATCH 1/3] ARM: dts: qcom: {a,i}pq8064: correct clock-names in sata node

[Niklas Cassel]

On Wed, Jul 17, 2024 at 10:05:09AM +0100, Rayyan Ansari wrote:
> 
> Hi Niklas,
> 
> Yes, this patch does not depend on the following two patches, I just
> thought that sending this as a series would make sense given that
> patches 2-3 would surface this error (as we can run dtbs_check against
> yaml bindings but not text bindings).

Usually, DT maintainers prefer for DT bindings to go via subsystem trees
(in this case libata).

I guess DT maintainers could have picked the whole series, as they do
occasionally, but they seem to want to avoid this as much as possible.


In this case, considering that the DTS change (patch 1/3) is a strict fix,
I think that it should be merged ASAP (target 6.11 instead of 6.12).

We will queue the DT binding changes for 6.12.

When also taking into consideration that the DT bindings and DTS changes
have different trees, splitting the series was probably the right move.


Kind regards,
Niklas

[PATCH 2/3] dt-bindings: ata: qcom,ipq806x-ahci: use dtschema

[Rayyan Ansari]

Remove old text bindings and add ipq806x AHCI compatible to
ahci-common.yaml, as well as its required properties.

Signed-off-by: Rayyan Ansari <rayyan.ansari@linaro.org>
---
 .../bindings/ata/ahci-platform.yaml           | 39 ++++++++++++++-
 .../devicetree/bindings/ata/qcom-sata.txt     | 48 -------------------
 2 files changed, 37 insertions(+), 50 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/ata/qcom-sata.txt

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
index 358617115bb8..b103d2c44421 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
@@ -30,6 +30,7 @@ select:
           - marvell,armada-3700-ahci
           - marvell,armada-8k-ahci
           - marvell,berlin2q-ahci
+          - qcom,ipq806x-ahci
           - socionext,uniphier-pro4-ahci
           - socionext,uniphier-pxs2-ahci
           - socionext,uniphier-pxs3-ahci
@@ -45,6 +46,7 @@ properties:
               - marvell,armada-8k-ahci
               - marvell,berlin2-ahci
               - marvell,berlin2q-ahci
+              - qcom,ipq806x-ahci
               - socionext,uniphier-pro4-ahci
               - socionext,uniphier-pxs2-ahci
               - socionext,uniphier-pxs3-ahci
@@ -64,11 +66,11 @@ properties:
 
   clocks:
     minItems: 1
-    maxItems: 3
+    maxItems: 5
 
   clock-names:
     minItems: 1
-    maxItems: 3
+    maxItems: 5
 
   interrupts:
     maxItems: 1
@@ -97,6 +99,39 @@ required:
 
 allOf:
   - $ref: ahci-common.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq806x-ahci
+    then:
+      properties:
+        clocks:
+          minItems: 5
+        clock-names:
+          items:
+            - const: slave_iface
+            - const: iface
+            - const: core
+            - const: rxoob
+            - const: pmalive
+        assigned-clocks:
+          minItems: 2
+          maxItems: 2
+        assigned-clock-rates:
+          items:
+            - const: 100000000
+            - const: 100000000
+      required:
+        - phys
+        - phy-names
+        - clocks
+        - clock-names
+        - assigned-clocks
+        - assigned-clock-rates
+
   - if:
       properties:
         compatible:
diff --git a/Documentation/devicetree/bindings/ata/qcom-sata.txt b/Documentation/devicetree/bindings/ata/qcom-sata.txt
deleted file mode 100644
index 094de91cd9fd..000000000000
--- a/Documentation/devicetree/bindings/ata/qcom-sata.txt
+++ /dev/null
@@ -1,48 +0,0 @@
-* Qualcomm AHCI SATA Controller
-
-SATA nodes are defined to describe on-chip Serial ATA controllers.
-Each SATA controller should have its own node.
-
-Required properties:
-- compatible		: compatible list, must contain "generic-ahci"
-- interrupts		: <interrupt mapping for SATA IRQ>
-- reg			: <registers mapping>
-- phys			: Must contain exactly one entry as specified
-			  in phy-bindings.txt
-- phy-names		: Must be "sata-phy"
-
-Required properties for "qcom,ipq806x-ahci" compatible:
-- clocks		: Must contain an entry for each entry in clock-names.
-- clock-names		: Shall be:
-				"slave_iface" - Fabric port AHB clock for SATA
-				"iface" - AHB clock
-				"core" - core clock
-				"rxoob" - RX out-of-band clock
-				"pmalive" - Power Module Alive clock
-- assigned-clocks	: Shall be:
-				SATA_RXOOB_CLK
-				SATA_PMALIVE_CLK
-- assigned-clock-rates	: Shall be:
-				100Mhz (100000000) for SATA_RXOOB_CLK
-				100Mhz (100000000) for SATA_PMALIVE_CLK
-
-Example:
-	sata@29000000 {
-		compatible = "qcom,ipq806x-ahci", "generic-ahci";
-		reg = <0x29000000 0x180>;
-
-		interrupts = <0 209 0x0>;
-
-		clocks = <&gcc SFAB_SATA_S_H_CLK>,
-			 <&gcc SATA_H_CLK>,
-			 <&gcc SATA_A_CLK>,
-			 <&gcc SATA_RXOOB_CLK>,
-			 <&gcc SATA_PMALIVE_CLK>;
-		clock-names = "slave_iface", "iface", "core",
-				"rxoob", "pmalive";
-		assigned-clocks = <&gcc SATA_RXOOB_CLK>, <&gcc SATA_PMALIVE_CLK>;
-		assigned-clock-rates = <100000000>, <100000000>;
-
-		phys = <&sata_phy>;
-		phy-names = "sata-phy";
-	};
-- 
2.45.2

Re: [PATCH 2/3] dt-bindings: ata: qcom,ipq806x-ahci: use dtschema

[Krzysztof Kozlowski]

On 16/07/2024 12:46, Rayyan Ansari wrote:
> Remove old text bindings and add ipq806x AHCI compatible to
> ahci-common.yaml, as well as its required properties.
> 

>  
>  allOf:
>    - $ref: ahci-common.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq806x-ahci
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 5
> +        clock-names:
> +          items:
> +            - const: slave_iface
> +            - const: iface
> +            - const: core
> +            - const: rxoob
> +            - const: pmalive
> +        assigned-clocks:
> +          minItems: 2
> +          maxItems: 2
> +        assigned-clock-rates:


assigned* should not be needed in the binding. You can drop these.

> +          items:
> +            - const: 100000000
> +            - const: 100000000
> +      required:
> +        - phys
> +        - phy-names
> +        - clocks
> +        - clock-names
> +        - assigned-clocks
> +        - assigned-clock-rates

Also from here.


Best regards,
Krzysztof

[PATCH 3/3] dt-bindings: ata: qcom,apq8064-ahci: add to dtschema

[Rayyan Ansari]

The APQ8064 SATA AHCI controller is used in apq8064.dtsi, although it
was not documented in the old text bindings.
Add its compatible to ahci-platform.yaml.

Signed-off-by: Rayyan Ansari <rayyan.ansari@linaro.org>
---
 Documentation/devicetree/bindings/ata/ahci-platform.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.yaml b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
index b103d2c44421..51624406392a 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.yaml
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.yaml
@@ -30,6 +30,7 @@ select:
           - marvell,armada-3700-ahci
           - marvell,armada-8k-ahci
           - marvell,berlin2q-ahci
+          - qcom,apq8064-ahci
           - qcom,ipq806x-ahci
           - socionext,uniphier-pro4-ahci
           - socionext,uniphier-pxs2-ahci
@@ -46,6 +47,7 @@ properties:
               - marvell,armada-8k-ahci
               - marvell,berlin2-ahci
               - marvell,berlin2q-ahci
+              - qcom,apq8064-ahci
               - qcom,ipq806x-ahci
               - socionext,uniphier-pro4-ahci
               - socionext,uniphier-pxs2-ahci
@@ -105,6 +107,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - qcom,apq8064-ahci
               - qcom,ipq806x-ahci
     then:
       properties:
-- 
2.45.2

Re: [PATCH 3/3] dt-bindings: ata: qcom,apq8064-ahci: add to dtschema

[Krzysztof Kozlowski]

On 16/07/2024 12:46, Rayyan Ansari wrote:
> The APQ8064 SATA AHCI controller is used in apq8064.dtsi, although it
> was not documented in the old text bindings.
> Add its compatible to ahci-platform.yaml.
> 
> Signed-off-by: Rayyan Ansari <rayyan.ansari@linaro.org>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof