Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

Florian Fainelli <florian.fainelli@xxxxxxxxxxxx> · Wed, 17 Jul 2024 14:06:17 -0700

On 7/16/24 23:51, Krzysztof Kozlowski wrote:
On 16/07/2024 23:31, Jim Quinlan wrote:
o Change order of the compatible strings to be alphabetical

o Describe resets/reset-names before using them in rules


<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Maybe the feedback was not clear, the fault cannot always be in the 
submitter, right?


Thank you.
</form letter>

o Add minItems/maxItems where needed.

o Change maintainer: Nicolas has not been active for a while.  It also
   makes sense for a Broadcom employee to be the maintainer as many of the
   details are privy to Broadcom.

Signed-off-by: Jim Quinlan <james.quinlan@xxxxxxxxxxxx>
---
  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
  1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 11f8ea33240c..692f7ed7c98e 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
  title: Brcmstb PCIe Host Controller
  
  maintainers:
-  - Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
+  - Jim Quinlan <james.quinlan@xxxxxxxxxxxx>
  
  properties:
    compatible:
@@ -16,11 +16,11 @@ properties:
            - brcm,bcm2711-pcie # The Raspberry Pi 4
            - brcm,bcm4908-pcie
            - brcm,bcm7211-pcie # Broadcom STB version of RPi4
-          - brcm,bcm7278-pcie # Broadcom 7278 Arm
            - brcm,bcm7216-pcie # Broadcom 7216 Arm
-          - brcm,bcm7445-pcie # Broadcom 7445 Arm
+          - brcm,bcm7278-pcie # Broadcom 7278 Arm
            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
+          - brcm,bcm7445-pcie # Broadcom 7445 Arm
  
    reg:
      maxItems: 1
@@ -95,6 +95,18 @@ properties:
        minItems: 1
        maxItems: 3
  
+  resets:
+    minItems: 1
+    items:
+      - description: reset for external PCIe PERST# signal # perst
+      - description: reset for phy reset calibration       # rescal
+
+  reset-names:
+    minItems: 1
+    items:
+      - const: perst
+      - const: rescal

There are no devices with two resets. Anyway, this does not match one of
your variants which have first element as rescal.

Just to be clear, is the diff below what you would expect to see when 
applying both patch 1 and 4 in succession, that is the reset properties 
are described "generically" and the conditional section only describes 
the order and values:

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml 
b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 11f8ea33240c..faab7291d722 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -95,6 +95,28 @@ properties:
       minItems: 1
       maxItems: 3

+  resets:
+    minItems: 1
+    maxItems: 3
+    oneOf:
+      - description: reset controller handling the PERST# signal
+      - description: phandle pointing to the RESCAL reset controller
+      - items:
+          - description: phandle pointing to the RESCAL reset controller
+          - description: reset for PCIe/CPU bus bridge
+          - description: reset for soft PCIe core reset
+
+  reset-names:
+    minItems: 1
+    maxItems: 3
+    oneOf:
+      - const: perst
+      - const: rescal
+      - items:
+          - const: rescal
+          - const: bridge
+          - const: swinit
+
 required:
   - compatible
   - reg
@@ -118,8 +140,7 @@ allOf:
     then:
       properties:
         resets:
-          items:
-            - description: reset controller handling the PERST# signal
+          maxItems: 1

         reset-names:
           items:
@@ -136,8 +157,7 @@ allOf:
     then:
       properties:
         resets:
-          items:
-            - description: phandle pointing to the RESCAL reset controller
+          maxItems: 1

         reset-names:
           items:
@@ -146,6 +166,22 @@ allOf:
       required:
         - resets
         - reset-names
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: brcm,bcm7712-pcie
+    then:
+      properties:
+        resets:
+          minItems: 3
+
+        reset-names:
+          minItems: 3
+
+      required:
+        - resets
+        - reset-names

 unevaluatedProperties: false

Thanks!
--
Florian

Attachment:
smime.p7s

Description: S/MIME Cryptographic Signature




