[PATCH v2 00/12] PCI: brcnstb: Enable STB 7712 SOC

[PATCH v2 00/12] PCI: brcnstb: Enable STB 7712 SOC

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]

V2 Changes (note: four new commits):
  o Commit "dt-bindings: PCI ..."
    -- s/Adds/Add/, fix spelling error (Bjorn)
    -- Order compatible strings alphabetically (Krzysztof)
    -- Give definitions first then rules (Krzysztof)
    -- Add reason for change in maintainer (Krzysztof)
  o Commit "Use swinit reset if available"
    -- no need for "else" clause (Philipp)
    -- fix improper use of dev_err_probe() (Philipp) 
  o Commit "Use "clk_out" error path label"
    -- Improve commit message (Bjorn)
  o Commit "PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific"
    -- Improve commit subject line (Bjorn)
  o Commit (NEW) -- Change field name from 'type' to 'model'
    -- Added as requested (Stanimir)
  o Commit (NEW) -- Check return value of all reset_control_xxx calls
    -- Added as requested (Stanimir)
  o Commit (NEW) "Get resource before we start asserting reset controllers"
    -- Added as requested (Stanimir)
  o Commit (NEW) -- "Remove two unused constants from driver"


V1:
  This submission is for the Broadcom STB 7712, sibling SOC of the RPi5 chip.
  Stanimir has already submitted a patch "Add PCIe support for bcm2712" for
  the RPi version of the SOC.  It is hoped that Stanimir will allow us to
  submit this series first and subsequently rebase his patch(es).

  The largest commit, "Refactor for chips with many regular inbound BARs"
  affects both the STB and RPi SOCs.  It allows for multiple inbound ranges
  where previously only one was effectively used.  This feature will also
  be present in future STB chips, as well as Broadcom's Cable Modem group.

Jim Quinlan (12):
  dt-bindings: PCI: Add Broadcom STB 7712 SOC, update maintainer
  PCI: brcmstb: Use "clk_out" error path label
  PCI: brcmstb: Use bridge reset if available
  PCI: brcmstb: Use swinit reset if available
  PCI: brcmstb: Get resource before we start asserting reset controllers
  PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets
    SoC-specific
  PCI: brcmstb: Remove two unused constants from driver
  PCI: brcmstb: Don't conflate the reset rescal with phy ctrl
  PCI: brcmstb: Refactor for chips with many regular inbound BARs
  PCI: brcmstb: Check return value of all reset_control_xxx calls
  PCI: brcmstb: Enable 7712 SOCs
  PCI: brcmstb: Change field name from 'type' to 'model'

 .../bindings/pci/brcm,stb-pcie.yaml           |  44 +-
 drivers/pci/controller/pcie-brcmstb.c         | 394 +++++++++++++-----
 2 files changed, 326 insertions(+), 112 deletions(-)


base-commit: 55027e689933ba2e64f3d245fb1ff185b3e7fc81
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

[PATCH v2 01/12] dt-bindings: PCI: Add Broadcom STB 7712 SOC, update maintainer

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3255 bytes --]

- Update maintainer; Nicolas hasn't been active and it
  makes more sense to have a Broadcom maintainer
- Add a driver compatible string for the new STB SOC 7712
- Add two new resets for the 7712: "bridge", for the
  the bridge between the PCIe core and the memory bus;
  "swinit", the PCIe core reset.
- Order the compatible strings alphabetically
- Restructure the reset controllers so that the definitions
  appear first before any rules that govern them.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 .../bindings/pci/brcm,stb-pcie.yaml           | 44 +++++++++++++++----
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 11f8ea33240c..a070f35d28d7 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@suse.de>
+  - Jim Quinlan <james.quinlan@broadcom.com>
 
 properties:
   compatible:
@@ -16,11 +16,12 @@ 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
+          - brcm,bcm7712-pcie # STB sibling SOC of Raspberry Pi 5
 
   reg:
     maxItems: 1
@@ -95,6 +96,20 @@ properties:
       minItems: 1
       maxItems: 3
 
+  resets:
+    items:
+      - description: reset for phy calibration
+      - description: reset for PCIe/CPU bus bridge
+      - description: reset for soft PCIe core reset
+      - description: reset for PERST# PCIe signal
+
+  reset-names:
+    items:
+      - const: rescal
+      - const: bridge
+      - const: swinit
+      - const: perst
+
 required:
   - compatible
   - reg
@@ -118,13 +133,10 @@ allOf:
     then:
       properties:
         resets:
-          items:
-            - description: reset controller handling the PERST# signal
-
+          minItems: 1
         reset-names:
           items:
             - const: perst
-
       required:
         - resets
         - reset-names
@@ -136,12 +148,28 @@ allOf:
     then:
       properties:
         resets:
+          minItems: 1
+        reset-names:
           items:
-            - description: phandle pointing to the RESCAL reset controller
+            - const: rescal
+      required:
+        - resets
+        - reset-names
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: brcm,bcm7712-pcie
+    then:
+      properties:
+        resets:
+          minItems: 3
 
         reset-names:
           items:
             - const: rescal
+            - const: bridge
+            - const: swinit
 
       required:
         - resets
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

Re: [PATCH v2 01/12] dt-bindings: PCI: Add Broadcom STB 7712 SOC, update maintainer

[Krzysztof Kozlowski]

On 03/07/2024 20:02, Jim Quinlan wrote:
> - Update maintainer; Nicolas hasn't been active and it
>   makes more sense to have a Broadcom maintainer
> - Add a driver compatible string for the new STB SOC 7712

You meant device? Bindings are for hardware.

> - Add two new resets for the 7712: "bridge", for the
>   the bridge between the PCIe core and the memory bus;
>   "swinit", the PCIe core reset.
> - Order the compatible strings alphabetically
> - Restructure the reset controllers so that the definitions
>   appear first before any rules that govern them.

Please split cleanups from new device support.

> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  .../bindings/pci/brcm,stb-pcie.yaml           | 44 +++++++++++++++----
>  1 file changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 11f8ea33240c..a070f35d28d7 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@suse.de>
> +  - Jim Quinlan <james.quinlan@broadcom.com>
>  
>  properties:
>    compatible:
> @@ -16,11 +16,12 @@ 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
> +          - brcm,bcm7712-pcie # STB sibling SOC of Raspberry Pi 5
>  
>    reg:
>      maxItems: 1
> @@ -95,6 +96,20 @@ properties:
>        minItems: 1
>        maxItems: 3
>  
> +  resets:
> +    items:
> +      - description: reset for phy calibration
> +      - description: reset for PCIe/CPU bus bridge
> +      - description: reset for soft PCIe core reset
> +      - description: reset for PERST# PCIe signal

This won't work and I doubt you tested your code. You miss minItems.

> +
> +  reset-names:
> +    items:
> +      - const: rescal
> +      - const: bridge
> +      - const: swinit
> +      - const: perst

This does not match what you have in conditional, so just keep min and
max Items here.


> +
>  required:
>    - compatible
>    - reg
> @@ -118,13 +133,10 @@ allOf:
>      then:
>        properties:
>          resets:
> -          items:
> -            - description: reset controller handling the PERST# signal
> -
> +          minItems: 1

maxItems instead. Why three resets should be valid?


>          reset-names:
>            items:
>              - const: perst
> -
>        required:
>          - resets
>          - reset-names
> @@ -136,12 +148,28 @@ allOf:
>      then:
>        properties:
>          resets:
> +          minItems: 1
> +        reset-names:
>            items:
> -            - description: phandle pointing to the RESCAL reset controller
> +            - const: rescal
> +      required:
> +        - resets
> +        - reset-names

Why?

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: brcm,bcm7712-pcie
> +    then:
> +      properties:
> +        resets:
> +          minItems: 3

Again, you do not have 4 items here.

>  
>          reset-names:
>            items:
>              - const: rescal
> +            - const: bridge
> +            - const: swinit
>  
>        required:
>          - resets

Best regards,
Krzysztof

Re: [PATCH v2 01/12] dt-bindings: PCI: Add Broadcom STB 7712 SOC, update maintainer

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 5207 bytes --]

On Thu, Jul 4, 2024 at 2:40 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 03/07/2024 20:02, Jim Quinlan wrote:
> > - Update maintainer; Nicolas hasn't been active and it
> >   makes more sense to have a Broadcom maintainer
> > - Add a driver compatible string for the new STB SOC 7712
>
> You meant device? Bindings are for hardware.
>
> > - Add two new resets for the 7712: "bridge", for the
> >   the bridge between the PCIe core and the memory bus;
> >   "swinit", the PCIe core reset.
> > - Order the compatible strings alphabetically
> > - Restructure the reset controllers so that the definitions
> >   appear first before any rules that govern them.
>
> Please split cleanups from new device support.
>
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  .../bindings/pci/brcm,stb-pcie.yaml           | 44 +++++++++++++++----
> >  1 file changed, 36 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 11f8ea33240c..a070f35d28d7 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@suse.de>
> > +  - Jim Quinlan <james.quinlan@broadcom.com>
> >
> >  properties:
> >    compatible:
> > @@ -16,11 +16,12 @@ 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
> > +          - brcm,bcm7712-pcie # STB sibling SOC of Raspberry Pi 5
> >
> >    reg:
> >      maxItems: 1
> > @@ -95,6 +96,20 @@ properties:
> >        minItems: 1
> >        maxItems: 3
> >
> > +  resets:
> > +    items:
> > +      - description: reset for phy calibration
> > +      - description: reset for PCIe/CPU bus bridge
> > +      - description: reset for soft PCIe core reset
> > +      - description: reset for PERST# PCIe signal
>
> This won't work and I doubt you tested your code. You miss minItems.

I did test my code and there were no errors.  I perform the following test:

make ARCH=arm64 dt_binding_check DT_CHECKER_FLAGS=-m
DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml

Is this incorrect?

>
> > +
> > +  reset-names:
> > +    items:
> > +      - const: rescal
> > +      - const: bridge
> > +      - const: swinit
> > +      - const: perst
>
> This does not match what you have in conditional, so just keep min and
> max Items here.

I'm not sure what you mean.  One chips uses a single reset, another
chip uses a different single reset,
and the third (7712) uses three of the four resets.

I was instructed to separate the descriptions from the rules, or at
least that's what I thought I was asked.
>
>
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -118,13 +133,10 @@ allOf:
> >      then:
> >        properties:
> >          resets:
> > -          items:
> > -            - description: reset controller handling the PERST# signal
> > -
> > +          minItems: 1
>
> maxItems instead. Why three resets should be valid?

See above.  Note that I was just instructed to separate the rules from
the descriptions.
In doing so I placed all of the reset descripts on the top and then
the rules below.
There are four possible resets but no single chip uses all of them and
three chips
use one or three of them.

Please advise.

Thanks,
Jim Quinlan
Broadcom STB/CM
>
>
> >          reset-names:
> >            items:
> >              - const: perst
> > -
> >        required:
> >          - resets
> >          - reset-names
> > @@ -136,12 +148,28 @@ allOf:
> >      then:
> >        properties:
> >          resets:
> > +          minItems: 1
> > +        reset-names:
> >            items:
> > -            - description: phandle pointing to the RESCAL reset controller
> > +            - const: rescal
> > +      required:
> > +        - resets
> > +        - reset-names
>
> Why?
>
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: brcm,bcm7712-pcie
> > +    then:
> > +      properties:
> > +        resets:
> > +          minItems: 3
>
> Again, you do not have 4 items here.
>
> >
> >          reset-names:
> >            items:
> >              - const: rescal
> > +            - const: bridge
> > +            - const: swinit
> >
> >        required:
> >          - resets
>
> Best regards,
> Krzysztof
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

Re: [PATCH v2 01/12] dt-bindings: PCI: Add Broadcom STB 7712 SOC, update maintainer

[Krzysztof Kozlowski]

On 05/07/2024 22:02, Jim Quinlan wrote:
> On Thu, Jul 4, 2024 at 2:40 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 03/07/2024 20:02, Jim Quinlan wrote:
>>> - Update maintainer; Nicolas hasn't been active and it
>>>   makes more sense to have a Broadcom maintainer
>>> - Add a driver compatible string for the new STB SOC 7712
>>
>> You meant device? Bindings are for hardware.
>>
>>> - Add two new resets for the 7712: "bridge", for the
>>>   the bridge between the PCIe core and the memory bus;
>>>   "swinit", the PCIe core reset.
>>> - Order the compatible strings alphabetically
>>> - Restructure the reset controllers so that the definitions
>>>   appear first before any rules that govern them.
>>
>> Please split cleanups from new device support.
>>
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>  .../bindings/pci/brcm,stb-pcie.yaml           | 44 +++++++++++++++----
>>>  1 file changed, 36 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 11f8ea33240c..a070f35d28d7 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@suse.de>
>>> +  - Jim Quinlan <james.quinlan@broadcom.com>
>>>
>>>  properties:
>>>    compatible:
>>> @@ -16,11 +16,12 @@ 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
>>> +          - brcm,bcm7712-pcie # STB sibling SOC of Raspberry Pi 5
>>>
>>>    reg:
>>>      maxItems: 1
>>> @@ -95,6 +96,20 @@ properties:
>>>        minItems: 1
>>>        maxItems: 3
>>>
>>> +  resets:
>>> +    items:
>>> +      - description: reset for phy calibration
>>> +      - description: reset for PCIe/CPU bus bridge
>>> +      - description: reset for soft PCIe core reset
>>> +      - description: reset for PERST# PCIe signal
>>
>> This won't work and I doubt you tested your code. You miss minItems.
> 
> I did test my code and there were no errors.  I perform the following test:
> 
> make ARCH=arm64 dt_binding_check DT_CHECKER_FLAGS=-m
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> 
> Is this incorrect?

That's correct and you are right - it passes the checks. Recent dtschema
changed the logic behind this. I am not sure if the new approach will
stay, I would find explicit minItems here more obvious and readable, so:
resets:
  minItems: 1
  items:
    - .........
    - .........
    - .........
    - .........


> 
>>
>>> +
>>> +  reset-names:
>>> +    items:
>>> +      - const: rescal
>>> +      - const: bridge
>>> +      - const: swinit
>>> +      - const: perst
>>
>> This does not match what you have in conditional, so just keep min and
>> max Items here.
> 
> I'm not sure what you mean.  One chips uses a single reset, another
> chip uses a different single reset,
> and the third (7712) uses three of the four resets.

Your conditional in allOf:if:then has different order.

> 
> I was instructed to separate the descriptions from the rules, or at
> least that's what I thought I was asked.
>>
>>
>>> +
>>>  required:
>>>    - compatible
>>>    - reg
>>> @@ -118,13 +133,10 @@ allOf:
>>>      then:
>>>        properties:
>>>          resets:
>>> -          items:
>>> -            - description: reset controller handling the PERST# signal
>>> -
>>> +          minItems: 1
>>
>> maxItems instead. Why three resets should be valid?
> 
> See above.  Note that I was just instructed to separate the rules from
> the descriptions.
> In doing so I placed all of the reset descripts on the top and then
> the rules below.
> There are four possible resets but no single chip uses all of them and
> three chips
> use one or three of them.
> 
> Please advise.

I don't understand that explanation. Why this particular variant works
with 1, 2, 3 or 4 resets in the same time?

Constraints are supposed to be precise / exact.

Best regards,
Krzysztof

Re: [PATCH v2 01/12] dt-bindings: PCI: Add Broadcom STB 7712 SOC, update maintainer

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 6183 bytes --]

On Thu, Jul 4, 2024 at 2:40 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 03/07/2024 20:02, Jim Quinlan wrote:
> > - Update maintainer; Nicolas hasn't been active and it
> >   makes more sense to have a Broadcom maintainer
> > - Add a driver compatible string for the new STB SOC 7712
>
> You meant device? Bindings are for hardware.

Hello Krzysztof,

I should have replied to this before sending out V3.  Since your form
letter says I did not address previous comments, I will address them
here and now (your v2 review of the bindings commit).

>
> > - Add two new resets for the 7712: "bridge", for the
> >   the bridge between the PCIe core and the memory bus;
> >   "swinit", the PCIe core reset.
> > - Order the compatible strings alphabetically
> > - Restructure the reset controllers so that the definitions
> >   appear first before any rules that govern them.
>
> Please split cleanups from new device support.
Okay.
>
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  .../bindings/pci/brcm,stb-pcie.yaml           | 44 +++++++++++++++----
> >  1 file changed, 36 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 11f8ea33240c..a070f35d28d7 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@suse.de>
> > +  - Jim Quinlan <james.quinlan@broadcom.com>
> >
> >  properties:
> >    compatible:
> > @@ -16,11 +16,12 @@ 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
> > +          - brcm,bcm7712-pcie # STB sibling SOC of Raspberry Pi 5
> >
> >    reg:
> >      maxItems: 1
> > @@ -95,6 +96,20 @@ properties:
> >        minItems: 1
> >        maxItems: 3
> >
> > +  resets:
> > +    items:
> > +      - description: reset for phy calibration
> > +      - description: reset for PCIe/CPU bus bridge
> > +      - description: reset for soft PCIe core reset
> > +      - description: reset for PERST# PCIe signal
>
> This won't work and I doubt you tested your code. You miss minItems.
>
> > +
> > +  reset-names:
> > +    items:
> > +      - const: rescal
> > +      - const: bridge
> > +      - const: swinit
> > +      - const: perst
>
> This does not match what you have in conditional, so just keep min and
> max Items here.

I do not understand.  There are four possible resets, but any one chip
uses only 0, 1, or 3 of them:

    CHIP            NUM_RESETS    NAMES
    ====            ==========    =====
    4908            1             perst
    7216            1             rescal
    7712            3             rescal, bridge, swinit
    Other_Chips     0             -

Although I list four "reset-names", I have, in the rule for 7712,
maxItems=3 because it only uses rescal, bridge, and swinit.  So I
don't know what you mean when you say "this does not match what you
have in your conditional".  AFAICT, they are not supposed to match.


>
>
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -118,13 +133,10 @@ allOf:
> >      then:
> >        properties:
> >          resets:
> > -          items:
> > -            - description: reset controller handling the PERST# signal
> > -
> > +          minItems: 1
>
> maxItems instead. Why three resets should be valid?
For the "4908" conditional, minItems==maxItems==1.  I do not
understand your question "Why three resets should be valid" -- can you
please elaborate?

>
>
> >          reset-names:
> >            items:
> >              - const: perst
> > -
> >        required:
> >          - resets
> >          - reset-names
> > @@ -136,12 +148,28 @@ allOf:
> >      then:
> >        properties:
> >          resets:
> > +          minItems: 1
> > +        reset-names:
> >            items:
> > -            - description: phandle pointing to the RESCAL reset controller
> > +            - const: rescal
> > +      required:
> > +        - resets
> > +        - reset-names
>
> Why?

I do not know what you are questioning.  The 7216 device uses one
reset: the "rescal".  Again, maxItems==minItems==1.  Please see the
summary note below.

>
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: brcm,bcm7712-pcie
> > +    then:
> > +      properties:
> > +        resets:
> > +          minItems: 3
>
> Again, you do not have 4 items here.

I do not want to have 4 items here; I want to have 3 for "rescal",
"bridge," and "swinit".  In this case, maxItems==minItems==3.

Now , for V1 you requested that I define all resets at the top; I've
done that and there are 4 of them.  But no chip uses all 4; each
individual chip only uses 0, 1, or 3 resets.

So there is no way that each chip's conditional rule can define
minItems and maxItems to match the description list of 4 resets,
unless you want me to undo your V1 request of describing the resets at
the top level instead of describing them in the rules.

Please advise,

Regards,
Jim Quinlan
Broadcom STB/CM




>
> >
> >          reset-names:
> >            items:
> >              - const: rescal
> > +            - const: bridge
> > +            - const: swinit
> >
> >        required:
> >          - resets
>
> Best regards,
> Krzysztof
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

Re: [PATCH v2 01/12] dt-bindings: PCI: Add Broadcom STB 7712 SOC, update maintainer

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 5596 bytes --]

On Sun, Jul 7, 2024 at 7:58 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 05/07/2024 22:02, Jim Quinlan wrote:
> > On Thu, Jul 4, 2024 at 2:40 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 03/07/2024 20:02, Jim Quinlan wrote:
> >>> - Update maintainer; Nicolas hasn't been active and it
> >>>   makes more sense to have a Broadcom maintainer
> >>> - Add a driver compatible string for the new STB SOC 7712
> >>
> >> You meant device? Bindings are for hardware.
> >>
> >>> - Add two new resets for the 7712: "bridge", for the
> >>>   the bridge between the PCIe core and the memory bus;
> >>>   "swinit", the PCIe core reset.
> >>> - Order the compatible strings alphabetically
> >>> - Restructure the reset controllers so that the definitions
> >>>   appear first before any rules that govern them.
> >>
> >> Please split cleanups from new device support.
> >>
> >>>
> >>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> >>> ---
> >>>  .../bindings/pci/brcm,stb-pcie.yaml           | 44 +++++++++++++++----
> >>>  1 file changed, 36 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>> index 11f8ea33240c..a070f35d28d7 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@suse.de>
> >>> +  - Jim Quinlan <james.quinlan@broadcom.com>
> >>>
> >>>  properties:
> >>>    compatible:
> >>> @@ -16,11 +16,12 @@ 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
> >>> +          - brcm,bcm7712-pcie # STB sibling SOC of Raspberry Pi 5
> >>>
> >>>    reg:
> >>>      maxItems: 1
> >>> @@ -95,6 +96,20 @@ properties:
> >>>        minItems: 1
> >>>        maxItems: 3
> >>>
> >>> +  resets:
> >>> +    items:
> >>> +      - description: reset for phy calibration
> >>> +      - description: reset for PCIe/CPU bus bridge
> >>> +      - description: reset for soft PCIe core reset
> >>> +      - description: reset for PERST# PCIe signal
> >>
> >> This won't work and I doubt you tested your code. You miss minItems.
> >
> > I did test my code and there were no errors.  I perform the following test:
> >
> > make ARCH=arm64 dt_binding_check DT_CHECKER_FLAGS=-m
> > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >
> > Is this incorrect?
>
> That's correct and you are right - it passes the checks. Recent dtschema
> changed the logic behind this. I am not sure if the new approach will
> stay, I would find explicit minItems here more obvious and readable, so:
> resets:
>   minItems: 1
>   items:
>     - .........
>     - .........
>     - .........
>     - .........
>
>
> >
> >>
> >>> +
> >>> +  reset-names:
> >>> +    items:
> >>> +      - const: rescal
> >>> +      - const: bridge
> >>> +      - const: swinit
> >>> +      - const: perst
> >>
> >> This does not match what you have in conditional, so just keep min and
> >> max Items here.
> >
> > I'm not sure what you mean.  One chips uses a single reset, another
> > chip uses a different single reset,
> > and the third (7712) uses three of the four resets.
>
> Your conditional in allOf:if:then has different order.
Different order then what, and ordering by chip or by reset name?

>
> >
> > I was instructed to separate the descriptions from the rules, or at
> > least that's what I thought I was asked.
> >>
> >>
> >>> +
> >>>  required:
> >>>    - compatible
> >>>    - reg
> >>> @@ -118,13 +133,10 @@ allOf:
> >>>      then:
> >>>        properties:
> >>>          resets:
> >>> -          items:
> >>> -            - description: reset controller handling the PERST# signal
> >>> -
> >>> +          minItems: 1
> >>
> >> maxItems instead. Why three resets should be valid?
> >
> > See above.  Note that I was just instructed to separate the rules from
> > the descriptions.
> > In doing so I placed all of the reset descripts on the top and then
> > the rules below.
> > There are four possible resets but no single chip uses all of them and
> > three chips
> > use one or three of them.
> >
> > Please advise.
>
> I don't understand that explanation. Why this particular variant works
> with 1, 2, 3 or 4 resets in the same time?

What do you mean in the "same time"?  The resets are just not present
in most of our PCIe HW.  In two chips there is only 1 reset in the HW,
and in the 7712 there are 3 resets in the HW.   You asked me to
describe all of the resets first at the top level and I have done
that.  But none of our chips ever use all four.

Regards,
Jim Quinlan
Broadcom STB/CM


>
> Constraints are supposed to be precise / exact.
>
> Best regards,
> Krzysztof
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

Re: [PATCH v2 01/12] dt-bindings: PCI: Add Broadcom STB 7712 SOC, update maintainer

[Krzysztof Kozlowski]

On 12/07/2024 21:54, Jim Quinlan wrote:
> On Thu, Jul 4, 2024 at 2:40 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 03/07/2024 20:02, Jim Quinlan wrote:
>>> - Update maintainer; Nicolas hasn't been active and it
>>>   makes more sense to have a Broadcom maintainer
>>> - Add a driver compatible string for the new STB SOC 7712
>>
>> You meant device? Bindings are for hardware.
> 
> Hello Krzysztof,
> 
> I should have replied to this before sending out V3.  Since your form
> letter says I did not address previous comments, I will address them
> here and now (your v2 review of the bindings commit).
> 
>>
>>> - Add two new resets for the 7712: "bridge", for the
>>>   the bridge between the PCIe core and the memory bus;
>>>   "swinit", the PCIe core reset.
>>> - Order the compatible strings alphabetically
>>> - Restructure the reset controllers so that the definitions
>>>   appear first before any rules that govern them.
>>
>> Please split cleanups from new device support.
> Okay.
>>
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>  .../bindings/pci/brcm,stb-pcie.yaml           | 44 +++++++++++++++----
>>>  1 file changed, 36 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 11f8ea33240c..a070f35d28d7 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@suse.de>
>>> +  - Jim Quinlan <james.quinlan@broadcom.com>
>>>
>>>  properties:
>>>    compatible:
>>> @@ -16,11 +16,12 @@ 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
>>> +          - brcm,bcm7712-pcie # STB sibling SOC of Raspberry Pi 5
>>>
>>>    reg:
>>>      maxItems: 1
>>> @@ -95,6 +96,20 @@ properties:
>>>        minItems: 1
>>>        maxItems: 3
>>>
>>> +  resets:
>>> +    items:
>>> +      - description: reset for phy calibration
>>> +      - description: reset for PCIe/CPU bus bridge
>>> +      - description: reset for soft PCIe core reset
>>> +      - description: reset for PERST# PCIe signal
>>
>> This won't work and I doubt you tested your code. You miss minItems.
>>
>>> +
>>> +  reset-names:
>>> +    items:
>>> +      - const: rescal
>>> +      - const: bridge
>>> +      - const: swinit
>>> +      - const: perst
>>
>> This does not match what you have in conditional, so just keep min and
>> max Items here.
> 
> I do not understand.  There are four possible resets, but any one chip
> uses only 0, 1, or 3 of them:
> 
>     CHIP            NUM_RESETS    NAMES
>     ====            ==========    =====
>     4908            1             perst
>     7216            1             rescal
>     7712            3             rescal, bridge, swinit
>     Other_Chips     0             -
> 
> Although I list four "reset-names", I have, in the rule for 7712,
> maxItems=3 because it only uses rescal, bridge, and swinit.  So I
> don't know what you mean when you say "this does not match what you
> have in your conditional".  AFAICT, they are not supposed to match.

One place says they have order A+B+C, other place says they have order
C+B+A or whatever other combination. Look at first element: A ! = C. So
they do not match.


> 
> 
>>
>>
>>> +
>>>  required:
>>>    - compatible
>>>    - reg
>>> @@ -118,13 +133,10 @@ allOf:
>>>      then:
>>>        properties:
>>>          resets:
>>> -          items:
>>> -            - description: reset controller handling the PERST# signal
>>> -
>>> +          minItems: 1
>>
>> maxItems instead. Why three resets should be valid?
> For the "4908" conditional, minItems==maxItems==1.  I do not
> understand your question "Why three resets should be valid" -- can you
> please elaborate?

Where do you have maxItems? I see only minItems.

> 
>>
>>
>>>          reset-names:
>>>            items:
>>>              - const: perst
>>> -
>>>        required:
>>>          - resets
>>>          - reset-names
>>> @@ -136,12 +148,28 @@ allOf:
>>>      then:
>>>        properties:
>>>          resets:
>>> +          minItems: 1
>>> +        reset-names:
>>>            items:
>>> -            - description: phandle pointing to the RESCAL reset controller
>>> +            - const: rescal
>>> +      required:
>>> +        - resets
>>> +        - reset-names
>>
>> Why?
> 
> I do not know what you are questioning.  The 7216 device uses one
> reset: the "rescal".  Again, maxItems==minItems==1.  Please see the
> summary note below.

You are breaking the ABI, so I am questioning. I don't see ABI break
explained in the commit msg.

> 
>>
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: brcm,bcm7712-pcie
>>> +    then:
>>> +      properties:
>>> +        resets:
>>> +          minItems: 3
>>
>> Again, you do not have 4 items here.
> 
> I do not want to have 4 items here; I want to have 3 for "rescal",
> "bridge," and "swinit".  In this case, maxItems==minItems==3.

Your schema does not define that.

> 
> Now , for V1 you requested that I define all resets at the top; I've
> done that and there are 4 of them.  But no chip uses all 4; each
> individual chip only uses 0, 1, or 3 resets.

I assumed they follow same order. If you have different order, the top
defines only widest constraints.

> 
> So there is no way that each chip's conditional rule can define
> minItems and maxItems to match the description list of 4 resets,
> unless you want me to undo your V1 request of describing the resets at
> the top level instead of describing them in the rules.
> 


Best regards,
Krzysztof

Re: [PATCH v2 01/12] dt-bindings: PCI: Add Broadcom STB 7712 SOC, update maintainer

[Krzysztof Kozlowski]

On 12/07/2024 22:13, Jim Quinlan wrote:
> On Sun, Jul 7, 2024 at 7:58 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 05/07/2024 22:02, Jim Quinlan wrote:
>>> On Thu, Jul 4, 2024 at 2:40 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>
>>>> On 03/07/2024 20:02, Jim Quinlan wrote:
>>>>> - Update maintainer; Nicolas hasn't been active and it
>>>>>   makes more sense to have a Broadcom maintainer
>>>>> - Add a driver compatible string for the new STB SOC 7712
>>>>
>>>> You meant device? Bindings are for hardware.
>>>>
>>>>> - Add two new resets for the 7712: "bridge", for the
>>>>>   the bridge between the PCIe core and the memory bus;
>>>>>   "swinit", the PCIe core reset.
>>>>> - Order the compatible strings alphabetically
>>>>> - Restructure the reset controllers so that the definitions
>>>>>   appear first before any rules that govern them.
>>>>
>>>> Please split cleanups from new device support.
>>>>
>>>>>
>>>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>>>> ---
>>>>>  .../bindings/pci/brcm,stb-pcie.yaml           | 44 +++++++++++++++----
>>>>>  1 file changed, 36 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>>> index 11f8ea33240c..a070f35d28d7 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@suse.de>
>>>>> +  - Jim Quinlan <james.quinlan@broadcom.com>
>>>>>
>>>>>  properties:
>>>>>    compatible:
>>>>> @@ -16,11 +16,12 @@ 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
>>>>> +          - brcm,bcm7712-pcie # STB sibling SOC of Raspberry Pi 5
>>>>>
>>>>>    reg:
>>>>>      maxItems: 1
>>>>> @@ -95,6 +96,20 @@ properties:
>>>>>        minItems: 1
>>>>>        maxItems: 3
>>>>>
>>>>> +  resets:
>>>>> +    items:
>>>>> +      - description: reset for phy calibration
>>>>> +      - description: reset for PCIe/CPU bus bridge
>>>>> +      - description: reset for soft PCIe core reset
>>>>> +      - description: reset for PERST# PCIe signal
>>>>
>>>> This won't work and I doubt you tested your code. You miss minItems.
>>>
>>> I did test my code and there were no errors.  I perform the following test:
>>>
>>> make ARCH=arm64 dt_binding_check DT_CHECKER_FLAGS=-m
>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>
>>> Is this incorrect?
>>
>> That's correct and you are right - it passes the checks. Recent dtschema
>> changed the logic behind this. I am not sure if the new approach will
>> stay, I would find explicit minItems here more obvious and readable, so:
>> resets:
>>   minItems: 1
>>   items:
>>     - .........
>>     - .........
>>     - .........
>>     - .........
>>
>>
>>>
>>>>
>>>>> +
>>>>> +  reset-names:
>>>>> +    items:
>>>>> +      - const: rescal
>>>>> +      - const: bridge
>>>>> +      - const: swinit
>>>>> +      - const: perst
>>>>
>>>> This does not match what you have in conditional, so just keep min and
>>>> max Items here.
>>>
>>> I'm not sure what you mean.  One chips uses a single reset, another
>>> chip uses a different single reset,
>>> and the third (7712) uses three of the four resets.
>>
>> Your conditional in allOf:if:then has different order.
> Different order then what, and ordering by chip or by reset name?

Where is my comment? Comment is under reset-names.

> 
>>
>>>
>>> I was instructed to separate the descriptions from the rules, or at
>>> least that's what I thought I was asked.
>>>>
>>>>
>>>>> +
>>>>>  required:
>>>>>    - compatible
>>>>>    - reg
>>>>> @@ -118,13 +133,10 @@ allOf:
>>>>>      then:
>>>>>        properties:
>>>>>          resets:
>>>>> -          items:
>>>>> -            - description: reset controller handling the PERST# signal
>>>>> -
>>>>> +          minItems: 1
>>>>
>>>> maxItems instead. Why three resets should be valid?
>>>
>>> See above.  Note that I was just instructed to separate the rules from
>>> the descriptions.
>>> In doing so I placed all of the reset descripts on the top and then
>>> the rules below.
>>> There are four possible resets but no single chip uses all of them and
>>> three chips
>>> use one or three of them.
>>>
>>> Please advise.
>>
>> I don't understand that explanation. Why this particular variant works
>> with 1, 2, 3 or 4 resets in the same time?
> 
> What do you mean in the "same time"?  The resets are just not present

Your schema says that you can have 1, 2, 3 or 4 resets.

> in most of our PCIe HW.  In two chips there is only 1 reset in the HW,
> and in the 7712 there are 3 resets in the HW.   You asked me to
> describe all of the resets first at the top level and I have done
> that.  But none of our chips ever use all four.
> 

Then express specific constraints in schema.

Best regards,
Krzysztof