[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: 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 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

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

[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

[PATCH v2 02/12] PCI: brcmstb: Use "clk_out" error path label

[Jim Quinlan]

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

Instead of invoking "clk_disable_unprepare(pcie->clk)" in
a number of error paths, we can just use a "clk_out" label
and goto the label after setting the return value.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c08683febdd4..c2eb29b886f7 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1620,24 +1620,25 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	}
 	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
 	if (IS_ERR(pcie->rescal)) {
-		clk_disable_unprepare(pcie->clk);
-		return PTR_ERR(pcie->rescal);
+		ret = PTR_ERR(pcie->rescal);
+		goto clk_out;
 	}
 	pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
 	if (IS_ERR(pcie->perst_reset)) {
-		clk_disable_unprepare(pcie->clk);
-		return PTR_ERR(pcie->perst_reset);
+		ret = PTR_ERR(pcie->perst_reset);
+		goto clk_out;
 	}
 
 	ret = reset_control_reset(pcie->rescal);
-	if (ret)
+	if (ret) {
 		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
+		goto clk_out;
+	}
 
 	ret = brcm_phy_start(pcie);
 	if (ret) {
 		reset_control_rearm(pcie->rescal);
-		clk_disable_unprepare(pcie->clk);
-		return ret;
+		goto clk_out;
 	}
 
 	ret = brcm_pcie_setup(pcie);
@@ -1676,6 +1677,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	return 0;
 
+clk_out:
+	clk_disable_unprepare(pcie->clk);
+	return ret;
 fail:
 	__brcm_pcie_remove(pcie);
 	return ret;
-- 
2.17.1


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

Re: [PATCH v2 02/12] PCI: brcmstb: Use "clk_out" error path label

[Markus Elfring]

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

Can improved adjustments be provided as regular diff data
(without an extra attachment)?


> Instead of invoking "clk_disable_unprepare(pcie->clk)" in
> a number of error paths, we can just use a "clk_out" label
> and goto the label after setting the return value.

* Please improve such a change description with imperative wordings.
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc6#n94

* How do you think about to use a summary phrase like
  “Use more common error handling code in brcm_pcie_probe()”?


…
> +++ b/drivers/pci/controller/pcie-brcmstb.c
…
>  	ret = reset_control_reset(pcie->rescal);
> -	if (ret)
> +	if (ret) {
>  		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> +		goto clk_out;
> +	}
>
>  	ret = brcm_phy_start(pcie);
…

Does this software update complete the exception handling?

Would you like to add any tags (like “Fixes” and “Cc”) accordingly?


…
> @@ -1676,6 +1677,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>
>  	return 0;
>
> +clk_out:
> +	clk_disable_unprepare(pcie->clk);
> +	return ret;
>  fail:
…

I suggest to add a blank line before the second label.

Regards,
Markus

Re: [PATCH v2 02/12] PCI: brcmstb: Use "clk_out" error path label

[Jim Quinlan]

[-- Attachment #1.1: Type: text/plain, Size: 1826 bytes --]

On Thu, Jul 4, 2024 at 7:40 AM Markus Elfring <Markus.Elfring@web.de> wrote:

> > [-- Attachment #1: Type: text/plain, Size: 1685 bytes --]
>
> Can improved adjustments be provided as regular diff data
> (without an extra attachment)?
>

I'm not sure what you are referring to... I see no attachment in the copy
of this email I received, and I am sending my patches  with "git
send-email".

I will address your other comments in v3.

Regards,
Jim Quinlan
Broadcom STB/CM

>
>
> > Instead of invoking "clk_disable_unprepare(pcie->clk)" in
> > a number of error paths, we can just use a "clk_out" label
> > and goto the label after setting the return value.
>
> * Please improve such a change description with imperative wordings.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc6#n94
>
> * How do you think about to use a summary phrase like
>   “Use more common error handling code in brcm_pcie_probe()”?
>
>
> …
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> …
> >       ret = reset_control_reset(pcie->rescal);
> > -     if (ret)
> > +     if (ret) {
> >               dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> > +             goto clk_out;
> > +     }
> >
> >       ret = brcm_phy_start(pcie);
> …
>
> Does this software update complete the exception handling?
>
> Would you like to add any tags (like “Fixes” and “Cc”) accordingly?
>
>
> …
> > @@ -1676,6 +1677,9 @@ static int brcm_pcie_probe(struct platform_device
> *pdev)
> >
> >       return 0;
> >
> > +clk_out:
> > +     clk_disable_unprepare(pcie->clk);
> > +     return ret;
> >  fail:
> …
>
> I suggest to add a blank line before the second label.
>
> Regards,
> Markus
>

[-- Attachment #1.2: Type: text/html, Size: 2806 bytes --]

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

Re: [v2 02/12] PCI: brcmstb: Use "clk_out" error path label

[Markus Elfring]

>     > [-- Attachment #1: Type: text/plain, Size: 1685 bytes --]
>
>     Can improved adjustments be provided as regular diff data
>     (without an extra attachment)?
>
>
> I'm not sure what you are referring to... I see no attachment in the copy of this email I received, …

Do I get inappropriate impressions here from published data representations?
https://lore.kernel.org/linux-kernel/20240703180300.42959-3-james.quinlan@broadcom.com/

Regards,
Markus

Re: [v2 02/12] PCI: brcmstb: Use "clk_out" error path label

[Jim Quinlan]

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

On Fri, Jul 5, 2024 at 11:45 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >     > [-- Attachment #1: Type: text/plain, Size: 1685 bytes --]
> >
> >     Can improved adjustments be provided as regular diff data
> >     (without an extra attachment)?
> >
> >
> > I'm not sure what you are referring to... I see no attachment in the copy of this email I received, …
>
> Do I get inappropriate impressions here from published data representations?
> https://lore.kernel.org/linux-kernel/20240703180300.42959-3-james.quinlan@broadcom.com/

Hi Markus,

Okay, I do see that :-).
It might be something my company is doing to the emails.  At one point
we had to stop sending submissions from our company email because it
would tag each email with
a long legal statement.  I will check with my manager (Florian
Fainaelli) to see if I can do something
about this.

Thanks for the heads-up,
Jim Quinlan
Broadcom STB/CM
>
>
>
> Regards,
> Markus

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

Re: [PATCH v2 02/12] PCI: brcmstb: Use "clk_out" error path label

[Stanimir Varbanov]

Hi Jim,

On 7/3/24 21:02, Jim Quinlan wrote:
> Instead of invoking "clk_disable_unprepare(pcie->clk)" in
> a number of error paths, we can just use a "clk_out" label
> and goto the label after setting the return value.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c08683febdd4..c2eb29b886f7 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1620,24 +1620,25 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	}
>  	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
>  	if (IS_ERR(pcie->rescal)) {
> -		clk_disable_unprepare(pcie->clk);
> -		return PTR_ERR(pcie->rescal);
> +		ret = PTR_ERR(pcie->rescal);
> +		goto clk_out;
>  	}
>  	pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
>  	if (IS_ERR(pcie->perst_reset)) {
> -		clk_disable_unprepare(pcie->clk);
> -		return PTR_ERR(pcie->perst_reset);
> +		ret = PTR_ERR(pcie->perst_reset);
> +		goto clk_out;
>  	}
>  

	ret = clk_prepare_enable(pcie->clk);

Have you considered enabling pcie->clk clock here ^^^ and avoid jumping
to clk_out label?

Basically, it'd be easier from error-path POV to get all required
resources and just after that start the initialization sequence of pcie
controller.

~Stan

>  	ret = reset_control_reset(pcie->rescal);
> -	if (ret)
> +	if (ret) {
>  		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> +		goto clk_out;
> +	}
>  
>  	ret = brcm_phy_start(pcie);
>  	if (ret) {
>  		reset_control_rearm(pcie->rescal);
> -		clk_disable_unprepare(pcie->clk);
> -		return ret;
> +		goto clk_out;
>  	}
>  
>  	ret = brcm_pcie_setup(pcie);
> @@ -1676,6 +1677,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +clk_out:
> +	clk_disable_unprepare(pcie->clk);
> +	return ret;
>  fail:
>  	__brcm_pcie_remove(pcie);
>  	return ret;

[PATCH v2 03/12] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

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

The 7712 SOC has a bridge reset which can be described in the device tree.
If it is present, use it. Otherwise, continue to use the legacy method to
reset the bridge.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c2eb29b886f7..4104c3668fdb 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -265,6 +265,7 @@ struct brcm_pcie {
 	enum pcie_type		type;
 	struct reset_control	*rescal;
 	struct reset_control	*perst_reset;
+	struct reset_control	*bridge;
 	int			num_memc;
 	u64			memc_size[PCIE_BRCM_MAX_MEMC];
 	u32			hw_rev;
@@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
 
 static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
 {
-	u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
-	u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
+	if (pcie->bridge) {
+		if (val)
+			reset_control_assert(pcie->bridge);
+		else
+			reset_control_deassert(pcie->bridge);
+	} else {
+		u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
+		u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
 
-	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
-	tmp = (tmp & ~mask) | ((val << shift) & mask);
-	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+		tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+		tmp = (tmp & ~mask) | ((val << shift) & mask);
+		writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+	}
 }
 
 static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
@@ -1635,6 +1643,12 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		goto clk_out;
 	}
 
+	pcie->bridge = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
+	if (IS_ERR(pcie->bridge)) {
+		ret = PTR_ERR(pcie->bridge);
+		goto clk_out;
+	}
+
 	ret = brcm_phy_start(pcie);
 	if (ret) {
 		reset_control_rearm(pcie->rescal);
-- 
2.17.1


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

[PATCH v2 04/12] PCI: brcmstb: Use swinit reset if available

[Jim Quinlan]

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

The 7712 SOC adds a software init reset device for the PCIe HW.
If found in the DT node, use it.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 4104c3668fdb..69926ee5c961 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -266,6 +266,7 @@ struct brcm_pcie {
 	struct reset_control	*rescal;
 	struct reset_control	*perst_reset;
 	struct reset_control	*bridge;
+	struct reset_control	*swinit;
 	int			num_memc;
 	u64			memc_size[PCIE_BRCM_MAX_MEMC];
 	u32			hw_rev;
@@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "could not enable clock\n");
 		return ret;
 	}
+
+	pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
+	if (IS_ERR(pcie->swinit)) {
+		ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
+				    "failed to get 'swinit' reset\n");
+		goto clk_out;
+	}
 	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
 	if (IS_ERR(pcie->rescal)) {
 		ret = PTR_ERR(pcie->rescal);
@@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		goto clk_out;
 	}
 
+	ret = reset_control_assert(pcie->swinit);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
+		goto clk_out;
+	}
+	ret = reset_control_deassert(pcie->swinit);
+	if (ret) {
+		dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
+		goto clk_out;
+	}
+
 	ret = reset_control_reset(pcie->rescal);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
-- 
2.17.1


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

Re: [PATCH v2 04/12] PCI: brcmstb: Use swinit reset if available

[Stanimir Varbanov]

Hi Jim,

On 7/3/24 21:02, Jim Quinlan wrote:
> The 7712 SOC adds a software init reset device for the PCIe HW.
> If found in the DT node, use it.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 4104c3668fdb..69926ee5c961 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -266,6 +266,7 @@ struct brcm_pcie {
>  	struct reset_control	*rescal;
>  	struct reset_control	*perst_reset;
>  	struct reset_control	*bridge;
> +	struct reset_control	*swinit;
>  	int			num_memc;
>  	u64			memc_size[PCIE_BRCM_MAX_MEMC];
>  	u32			hw_rev;
> @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "could not enable clock\n");
>  		return ret;
>  	}
> +
> +	pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> +	if (IS_ERR(pcie->swinit)) {
> +		ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> +				    "failed to get 'swinit' reset\n");
> +		goto clk_out;
> +	}
>  	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
>  	if (IS_ERR(pcie->rescal)) {
>  		ret = PTR_ERR(pcie->rescal);
> @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		goto clk_out;
>  	}
>  
> +	ret = reset_control_assert(pcie->swinit);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> +		goto clk_out;
> +	}
> +	ret = reset_control_deassert(pcie->swinit);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> +		goto clk_out;
> +	}

why not call reset_control_reset(pcie->swinit) directly?

~Stan
> +
>  	ret = reset_control_reset(pcie->rescal);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");

Re: [PATCH v2 04/12] PCI: brcmstb: Use swinit reset if available

[Jim Quinlan]

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

On Thu, Jul 4, 2024 at 8:56 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Jim,
>
> On 7/3/24 21:02, Jim Quinlan wrote:
> > The 7712 SOC adds a software init reset device for the PCIe HW.
> > If found in the DT node, use it.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 4104c3668fdb..69926ee5c961 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -266,6 +266,7 @@ struct brcm_pcie {
> >       struct reset_control    *rescal;
> >       struct reset_control    *perst_reset;
> >       struct reset_control    *bridge;
> > +     struct reset_control    *swinit;
> >       int                     num_memc;
> >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> >       u32                     hw_rev;
> > @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >               dev_err(&pdev->dev, "could not enable clock\n");
> >               return ret;
> >       }
> > +
> > +     pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > +     if (IS_ERR(pcie->swinit)) {
> > +             ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> > +                                 "failed to get 'swinit' reset\n");
> > +             goto clk_out;
> > +     }
> >       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> >       if (IS_ERR(pcie->rescal)) {
> >               ret = PTR_ERR(pcie->rescal);
> > @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >               goto clk_out;
> >       }
> >
> > +     ret = reset_control_assert(pcie->swinit);
> > +     if (ret) {
> > +             dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> > +             goto clk_out;
> > +     }
> > +     ret = reset_control_deassert(pcie->swinit);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> > +             goto clk_out;
> > +     }
>
> why not call reset_control_reset(pcie->swinit) directly?
Hi Stan,

There is no reset_control_reset() method defined for reset-brcmstb.c.
The only reason I can
think of for this is that it allows the callers of assert/deassert to
insert a delay if desired.

Regards,
Jim Quinlan
Broadcom STB/CM
>
> ~Stan
> > +
> >       ret = reset_control_reset(pcie->rescal);
> >       if (ret) {
> >               dev_err(&pdev->dev, "failed to deassert 'rescal'\n");

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

Re: [PATCH v2 04/12] PCI: brcmstb: Use swinit reset if available

[Philipp Zabel]

On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote:
> On Thu, Jul 4, 2024 at 8:56 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
> > 
> > Hi Jim,
> > 
> > On 7/3/24 21:02, Jim Quinlan wrote:
> > > The 7712 SOC adds a software init reset device for the PCIe HW.
> > > If found in the DT node, use it.
> > > 
> > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > ---
> > >  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > index 4104c3668fdb..69926ee5c961 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -266,6 +266,7 @@ struct brcm_pcie {
> > >       struct reset_control    *rescal;
> > >       struct reset_control    *perst_reset;
> > >       struct reset_control    *bridge;
> > > +     struct reset_control    *swinit;
> > >       int                     num_memc;
> > >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> > >       u32                     hw_rev;
> > > @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > >               dev_err(&pdev->dev, "could not enable clock\n");
> > >               return ret;
> > >       }
> > > +
> > > +     pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > > +     if (IS_ERR(pcie->swinit)) {
> > > +             ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> > > +                                 "failed to get 'swinit' reset\n");
> > > +             goto clk_out;
> > > +     }
> > >       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> > >       if (IS_ERR(pcie->rescal)) {
> > >               ret = PTR_ERR(pcie->rescal);
> > > @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > >               goto clk_out;
> > >       }
> > > 
> > > +     ret = reset_control_assert(pcie->swinit);
> > > +     if (ret) {
> > > +             dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> > > +             goto clk_out;
> > > +     }
> > > +     ret = reset_control_deassert(pcie->swinit);
> > > +     if (ret) {
> > > +             dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> > > +             goto clk_out;
> > > +     }
> > 
> > why not call reset_control_reset(pcie->swinit) directly?
> Hi Stan,
> 
> There is no reset_control_reset() method defined for reset-brcmstb.c.
> The only reason I can
> think of for this is that it allows the callers of assert/deassert to
> insert a delay if desired.

The main reason for the existence of reset_control_reset() is that
there are reset controllers that can only be triggered (e.g. by writing
a bit to a self-clearing register) to produce a complete reset pulse,
with assertion, delay, and deassertion all handled by the reset
controller.

regards
Philipp

Re: [PATCH v2 04/12] PCI: brcmstb: Use swinit reset if available

[Stanimir Varbanov]

Hi Philipp,

On 7/8/24 12:37, Philipp Zabel wrote:
> On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote:
>> On Thu, Jul 4, 2024 at 8:56 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>>>
>>> Hi Jim,
>>>
>>> On 7/3/24 21:02, Jim Quinlan wrote:
>>>> The 7712 SOC adds a software init reset device for the PCIe HW.
>>>> If found in the DT node, use it.
>>>>
>>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>>> ---
>>>>  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
>>>>  1 file changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>>>> index 4104c3668fdb..69926ee5c961 100644
>>>> --- a/drivers/pci/controller/pcie-brcmstb.c
>>>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>>>> @@ -266,6 +266,7 @@ struct brcm_pcie {
>>>>       struct reset_control    *rescal;
>>>>       struct reset_control    *perst_reset;
>>>>       struct reset_control    *bridge;
>>>> +     struct reset_control    *swinit;
>>>>       int                     num_memc;
>>>>       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
>>>>       u32                     hw_rev;
>>>> @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>>>               dev_err(&pdev->dev, "could not enable clock\n");
>>>>               return ret;
>>>>       }
>>>> +
>>>> +     pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
>>>> +     if (IS_ERR(pcie->swinit)) {
>>>> +             ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
>>>> +                                 "failed to get 'swinit' reset\n");
>>>> +             goto clk_out;
>>>> +     }
>>>>       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
>>>>       if (IS_ERR(pcie->rescal)) {
>>>>               ret = PTR_ERR(pcie->rescal);
>>>> @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>>>               goto clk_out;
>>>>       }
>>>>
>>>> +     ret = reset_control_assert(pcie->swinit);
>>>> +     if (ret) {
>>>> +             dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
>>>> +             goto clk_out;
>>>> +     }
>>>> +     ret = reset_control_deassert(pcie->swinit);
>>>> +     if (ret) {
>>>> +             dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
>>>> +             goto clk_out;
>>>> +     }
>>>
>>> why not call reset_control_reset(pcie->swinit) directly?
>> Hi Stan,
>>
>> There is no reset_control_reset() method defined for reset-brcmstb.c.
>> The only reason I can
>> think of for this is that it allows the callers of assert/deassert to
>> insert a delay if desired.
> 
> The main reason for the existence of reset_control_reset() is that
> there are reset controllers that can only be triggered (e.g. by writing
> a bit to a self-clearing register) to produce a complete reset pulse,
> with assertion, delay, and deassertion all handled by the reset
> controller.


Got it. Thank you for explanation.

But, IMO that means that the consumer driver should have knowledge of
low-level reset implementation, which is not generic API?

Otherwise, I don't see a problem to implement asset/deassert sequence in
.reset op in this particular reset-brcmstb.c low-level driver.


~Stan

Re: [PATCH v2 04/12] PCI: brcmstb: Use swinit reset if available

[Philipp Zabel]

Hi Stanimir,

On Mo, 2024-07-08 at 14:14 +0300, Stanimir Varbanov wrote:
> Hi Philipp,
> 
> On 7/8/24 12:37, Philipp Zabel wrote:
> > On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote:
> > > On Thu, Jul 4, 2024 at 8:56 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
> > > > 
> > > > Hi Jim,
> > > > 
> > > > On 7/3/24 21:02, Jim Quinlan wrote:
> > > > > The 7712 SOC adds a software init reset device for the PCIe HW.
> > > > > If found in the DT node, use it.
> > > > > 
> > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > > > ---
> > > > >  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
> > > > >  1 file changed, 19 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > > > index 4104c3668fdb..69926ee5c961 100644
> > > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > > @@ -266,6 +266,7 @@ struct brcm_pcie {
> > > > >       struct reset_control    *rescal;
> > > > >       struct reset_control    *perst_reset;
> > > > >       struct reset_control    *bridge;
> > > > > +     struct reset_control    *swinit;
> > > > >       int                     num_memc;
> > > > >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> > > > >       u32                     hw_rev;
> > > > > @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > > >               dev_err(&pdev->dev, "could not enable clock\n");
> > > > >               return ret;
> > > > >       }
> > > > > +
> > > > > +     pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > > > > +     if (IS_ERR(pcie->swinit)) {
> > > > > +             ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> > > > > +                                 "failed to get 'swinit' reset\n");
> > > > > +             goto clk_out;
> > > > > +     }
> > > > >       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> > > > >       if (IS_ERR(pcie->rescal)) {
> > > > >               ret = PTR_ERR(pcie->rescal);
> > > > > @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > > >               goto clk_out;
> > > > >       }
> > > > > 
> > > > > +     ret = reset_control_assert(pcie->swinit);
> > > > > +     if (ret) {
> > > > > +             dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> > > > > +             goto clk_out;
> > > > > +     }
> > > > > +     ret = reset_control_deassert(pcie->swinit);
> > > > > +     if (ret) {
> > > > > +             dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> > > > > +             goto clk_out;
> > > > > +     }
> > > > 
> > > > why not call reset_control_reset(pcie->swinit) directly?
> > > Hi Stan,
> > > 
> > > There is no reset_control_reset() method defined for reset-brcmstb.c.
> > > The only reason I can
> > > think of for this is that it allows the callers of assert/deassert to
> > > insert a delay if desired.
> > 
> > The main reason for the existence of reset_control_reset() is that
> > there are reset controllers that can only be triggered (e.g. by writing
> > a bit to a self-clearing register) to produce a complete reset pulse,
> > with assertion, delay, and deassertion all handled by the reset
> > controller.
> 
> Got it. Thank you for explanation.
> 
> But, IMO that means that the consumer driver should have knowledge of
> low-level reset implementation, which is not generic API?

Kind of. If the reset controller hardware has self-clearing resets, it
is impossible to implement manual reset_control_assert/deassert() on
top. So if a reset consumer requires that level of control, it just
can't work with a self-deasserting controller. The other way around, a
reset controller driver can emulate self-deasserting resets, iff it
knows the timing requirements of all consumers.

If the reset consumer only needs to see a pulse on the reset line, and
there are no ordering requirements with other resets or clocks, and the
device either doesn't care about timing or the reset controller knows
how to produce the required delay, then using reset_control_reset()
would be semantically correct.

> Otherwise, I don't see a problem to implement asset/deassert sequence in
> .reset op in this particular reset-brcmstb.c low-level driver.

When reset_control_reset() is used, every reset controller that can be
paired with this consumer needs to implement the .reset method,
requiring to know the delay requirements for all of their consumers.
The reset-simple driver implements this with a configurable worst-case
delay, for example. As far as I can see, that has never been used.

So yes, in this particular case, pcie-brcmstb only ever being paired
with reset-brcmstb, it might be no problem to implement .reset in
reset-brcmstb correctly, if all its consumers and their required delays
are known.

regards
Philipp

Re: [PATCH v2 04/12] PCI: brcmstb: Use swinit reset if available

[Jim Quinlan]

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

On Mon, Jul 8, 2024 at 9:26 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Stanimir,
>
> On Mo, 2024-07-08 at 14:14 +0300, Stanimir Varbanov wrote:
> > Hi Philipp,
> >
> > On 7/8/24 12:37, Philipp Zabel wrote:
> > > On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote:
> > > > On Thu, Jul 4, 2024 at 8:56 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
> > > > >
> > > > > Hi Jim,
> > > > >
> > > > > On 7/3/24 21:02, Jim Quinlan wrote:
> > > > > > The 7712 SOC adds a software init reset device for the PCIe HW.
> > > > > > If found in the DT node, use it.
> > > > > >
> > > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > > > > ---
> > > > > >  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
> > > > > >  1 file changed, 19 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > > > > index 4104c3668fdb..69926ee5c961 100644
> > > > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > > > @@ -266,6 +266,7 @@ struct brcm_pcie {
> > > > > >       struct reset_control    *rescal;
> > > > > >       struct reset_control    *perst_reset;
> > > > > >       struct reset_control    *bridge;
> > > > > > +     struct reset_control    *swinit;
> > > > > >       int                     num_memc;
> > > > > >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> > > > > >       u32                     hw_rev;
> > > > > > @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > > > >               dev_err(&pdev->dev, "could not enable clock\n");
> > > > > >               return ret;
> > > > > >       }
> > > > > > +
> > > > > > +     pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > > > > > +     if (IS_ERR(pcie->swinit)) {
> > > > > > +             ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> > > > > > +                                 "failed to get 'swinit' reset\n");
> > > > > > +             goto clk_out;
> > > > > > +     }
> > > > > >       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> > > > > >       if (IS_ERR(pcie->rescal)) {
> > > > > >               ret = PTR_ERR(pcie->rescal);
> > > > > > @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > > > >               goto clk_out;
> > > > > >       }
> > > > > >
> > > > > > +     ret = reset_control_assert(pcie->swinit);
> > > > > > +     if (ret) {
> > > > > > +             dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> > > > > > +             goto clk_out;
> > > > > > +     }
> > > > > > +     ret = reset_control_deassert(pcie->swinit);
> > > > > > +     if (ret) {
> > > > > > +             dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> > > > > > +             goto clk_out;
> > > > > > +     }
> > > > >
> > > > > why not call reset_control_reset(pcie->swinit) directly?
> > > > Hi Stan,
> > > >
> > > > There is no reset_control_reset() method defined for reset-brcmstb.c.
> > > > The only reason I can
> > > > think of for this is that it allows the callers of assert/deassert to
> > > > insert a delay if desired.
> > >
> > > The main reason for the existence of reset_control_reset() is that
> > > there are reset controllers that can only be triggered (e.g. by writing
> > > a bit to a self-clearing register) to produce a complete reset pulse,
> > > with assertion, delay, and deassertion all handled by the reset
> > > controller.
> >
> > Got it. Thank you for explanation.
> >
> > But, IMO that means that the consumer driver should have knowledge of
> > low-level reset implementation, which is not generic API?
>
> Kind of. If the reset controller hardware has self-clearing resets, it
> is impossible to implement manual reset_control_assert/deassert() on
> top. So if a reset consumer requires that level of control, it just
> can't work with a self-deasserting controller. The other way around, a
> reset controller driver can emulate self-deasserting resets, iff it
> knows the timing requirements of all consumers.
>
> If the reset consumer only needs to see a pulse on the reset line, and
> there are no ordering requirements with other resets or clocks, and the
> device either doesn't care about timing or the reset controller knows
> how to produce the required delay, then using reset_control_reset()
> would be semantically correct.
>
> > Otherwise, I don't see a problem to implement asset/deassert sequence in
> > .reset op in this particular reset-brcmstb.c low-level driver.
>
> When reset_control_reset() is used, every reset controller that can be
> paired with this consumer needs to implement the .reset method,
> requiring to know the delay requirements for all of their consumers.
> The reset-simple driver implements this with a configurable worst-case
> delay, for example. As far as I can see, that has never been used.
>
> So yes, in this particular case, pcie-brcmstb only ever being paired
> with reset-brcmstb, it might be no problem to implement .reset in
> reset-brcmstb correctly, if all its consumers and their required delays
> are known.

This will not work.  reset-brcmstb.c is a reset provider; it provides
resets for dozens of different HW blocks.  Forcing  dozens of the
resets  to operate at the worst-case delay of the slowest one of them
is unacceptable, especially if a particular HW block uses its reset
often  and/or requires timely execution.

Regards,
Jim Quinlan
Broadcom STB/CM


>
> regards
> Philipp

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

Re: [PATCH v2 04/12] PCI: brcmstb: Use swinit reset if available

[Stanimir Varbanov]

Hi,

On 7/3/24 21:02, Jim Quinlan wrote:
> The 7712 SOC adds a software init reset device for the PCIe HW.
> If found in the DT node, use it.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 4104c3668fdb..69926ee5c961 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -266,6 +266,7 @@ struct brcm_pcie {
>  	struct reset_control	*rescal;
>  	struct reset_control	*perst_reset;
>  	struct reset_control	*bridge;
> +	struct reset_control	*swinit;
>  	int			num_memc;
>  	u64			memc_size[PCIE_BRCM_MAX_MEMC];
>  	u32			hw_rev;
> @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "could not enable clock\n");
>  		return ret;
>  	}
> +
> +	pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> +	if (IS_ERR(pcie->swinit)) {
> +		ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit),
> +				    "failed to get 'swinit' reset\n");

Why "swinit" reset is special in this series? For example previous 3/12
patch which add "bridge" reset does not use dev_err_probe.

IMO you should use dev_err() here and below, and make a new follow-up
patch which change dev_err -> dev_err_probe on all places where getting
resources like resets and clocks.

> +		goto clk_out;
> +	}
>  	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
>  	if (IS_ERR(pcie->rescal)) {
>  		ret = PTR_ERR(pcie->rescal);
> @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		goto clk_out;
>  	}
>  
> +	ret = reset_control_assert(pcie->swinit);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> +		goto clk_out;

ditto

> +	}
> +	ret = reset_control_deassert(pcie->swinit);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
> +		goto clk_out;

this is fine

> +	}
> +
>  	ret = reset_control_reset(pcie->rescal);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");

~Stan

[PATCH v2 05/12] PCI: brcmstb: Get resource before we start asserting reset controllers

[Jim Quinlan]

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

Place all of the devm_reset_contol_get*() calls above the calls that
assert the reset controllers.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 69926ee5c961..59daa4b2e6c5 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1644,6 +1644,11 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		ret = PTR_ERR(pcie->perst_reset);
 		goto clk_out;
 	}
+	pcie->bridge = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
+	if (IS_ERR(pcie->bridge)) {
+		ret = PTR_ERR(pcie->bridge);
+		goto clk_out;
+	}
 
 	ret = reset_control_assert(pcie->swinit);
 	if (ret) {
@@ -1662,12 +1667,6 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		goto clk_out;
 	}
 
-	pcie->bridge = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
-	if (IS_ERR(pcie->bridge)) {
-		ret = PTR_ERR(pcie->bridge);
-		goto clk_out;
-	}
-
 	ret = brcm_phy_start(pcie);
 	if (ret) {
 		reset_control_rearm(pcie->rescal);
-- 
2.17.1


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

Re: [PATCH v2 05/12] PCI: brcmstb: Get resource before we start asserting reset controllers

[Stanimir Varbanov]

Hi Jim,

On 7/3/24 21:02, Jim Quinlan wrote:
> Place all of the devm_reset_contol_get*() calls above the calls that
> assert the reset controllers.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 69926ee5c961..59daa4b2e6c5 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1644,6 +1644,11 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		ret = PTR_ERR(pcie->perst_reset);
>  		goto clk_out;
>  	}
> +	pcie->bridge = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
> +	if (IS_ERR(pcie->bridge)) {
> +		ret = PTR_ERR(pcie->bridge);
> +		goto clk_out;
> +	}
>  

This change must be merged in 3/12 - "PCI: brcmstb: Use bridge reset if
available"

~Stan
>  	ret = reset_control_assert(pcie->swinit);
>  	if (ret) {
> @@ -1662,12 +1667,6 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		goto clk_out;
>  	}
>  
> -	pcie->bridge = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
> -	if (IS_ERR(pcie->bridge)) {
> -		ret = PTR_ERR(pcie->bridge);
> -		goto clk_out;
> -	}
> -
>  	ret = brcm_phy_start(pcie);
>  	if (ret) {
>  		reset_control_rearm(pcie->rescal);

[PATCH v2 06/12] PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific

[Jim Quinlan]

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

Our HW design has again changed a register offset which used to be standard
for all Broadcom SOCs with PCIe cores.  This difference is now reconciled
for the registers HARD_DEBUG and INTR2_CPU_BASE.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 33 +++++++++++++++++----------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 59daa4b2e6c5..d8c0f1474369 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -122,7 +122,6 @@
 #define PCIE_MEM_WIN0_LIMIT_HI(win)	\
 		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI + ((win) * 8)
 
-#define PCIE_MISC_HARD_PCIE_HARD_DEBUG					0x4204
 #define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK	0x2
 #define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK		0x200000
 #define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK		0x08000000
@@ -131,9 +130,9 @@
 	  (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
 	   PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
 
-#define PCIE_INTR2_CPU_BASE		0x4300
 #define PCIE_MSI_INTR2_BASE		0x4500
-/* Offsets from PCIE_INTR2_CPU_BASE and PCIE_MSI_INTR2_BASE */
+
+/* Offsets from INTR2_CPU and MSI_INTR2 BASE offsets */
 #define  MSI_INT_STATUS			0x0
 #define  MSI_INT_CLR			0x8
 #define  MSI_INT_MASK_SET		0x10
@@ -187,6 +186,8 @@
 #define IDX_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_INDEX])
 #define DATA_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_DATA])
 #define PCIE_RGR1_SW_INIT_1(pcie)	(pcie->reg_offsets[RGR1_SW_INIT_1])
+#define	HARD_DEBUG(pcie)		(pcie->reg_offsets[PCIE_HARD_DEBUG])
+#define	INTR2_CPU_BASE(pcie)		(pcie->reg_offsets[PCIE_INTR2_CPU_BASE])
 
 /* Rescal registers */
 #define PCIE_DVT_PMU_PCIE_PHY_CTRL				0xc700
@@ -205,6 +206,8 @@ enum {
 	RGR1_SW_INIT_1,
 	EXT_CFG_INDEX,
 	EXT_CFG_DATA,
+	PCIE_HARD_DEBUG,
+	PCIE_INTR2_CPU_BASE,
 };
 
 enum {
@@ -651,7 +654,7 @@ static int brcm_pcie_enable_msi(struct brcm_pcie *pcie)
 	BUILD_BUG_ON(BRCM_INT_PCI_MSI_LEGACY_NR > BRCM_INT_PCI_MSI_NR);
 
 	if (msi->legacy) {
-		msi->intr_base = msi->base + PCIE_INTR2_CPU_BASE;
+		msi->intr_base = msi->base + INTR2_CPU_BASE(pcie);
 		msi->nr = BRCM_INT_PCI_MSI_LEGACY_NR;
 		msi->legacy_shift = 24;
 	} else {
@@ -898,12 +901,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	/* Take the bridge out of reset */
 	pcie->bridge_sw_init_set(pcie, 0);
 
-	tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	tmp = readl(base + HARD_DEBUG(pcie));
 	if (is_bmips(pcie))
 		tmp &= ~PCIE_BMIPS_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK;
 	else
 		tmp &= ~PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK;
-	writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	writel(tmp, base + HARD_DEBUG(pcie));
 	/* Wait for SerDes to be stable */
 	usleep_range(100, 200);
 
@@ -1072,7 +1075,7 @@ static void brcm_config_clkreq(struct brcm_pcie *pcie)
 	}
 
 	/* Start out assuming safe mode (both mode bits cleared) */
-	clkreq_cntl = readl(pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	clkreq_cntl = readl(pcie->base + HARD_DEBUG(pcie));
 	clkreq_cntl &= ~PCIE_CLKREQ_MASK;
 
 	if (strcmp(mode, "no-l1ss") == 0) {
@@ -1115,7 +1118,7 @@ static void brcm_config_clkreq(struct brcm_pcie *pcie)
 			dev_err(pcie->dev, err_msg);
 		mode = "safe";
 	}
-	writel(clkreq_cntl, pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	writel(clkreq_cntl, pcie->base + HARD_DEBUG(pcie));
 
 	dev_info(pcie->dev, "clkreq-mode set to %s\n", mode);
 }
@@ -1337,9 +1340,9 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	writel(tmp, base + PCIE_MISC_PCIE_CTRL);
 
 	/* Turn off SerDes */
-	tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	tmp = readl(base + HARD_DEBUG(pcie));
 	u32p_replace_bits(&tmp, 1, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK);
-	writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	writel(tmp, base + HARD_DEBUG(pcie));
 
 	/* Shutdown PCIe bridge */
 	pcie->bridge_sw_init_set(pcie, 1);
@@ -1425,9 +1428,9 @@ static int brcm_pcie_resume_noirq(struct device *dev)
 	pcie->bridge_sw_init_set(pcie, 0);
 
 	/* SERDES_IDDQ = 0 */
-	tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	tmp = readl(base + HARD_DEBUG(pcie));
 	u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK);
-	writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	writel(tmp, base + HARD_DEBUG(pcie));
 
 	/* wait for serdes to be stable */
 	udelay(100);
@@ -1499,12 +1502,16 @@ static const int pcie_offsets[] = {
 	[RGR1_SW_INIT_1] = 0x9210,
 	[EXT_CFG_INDEX]  = 0x9000,
 	[EXT_CFG_DATA]   = 0x9004,
+	[PCIE_HARD_DEBUG] = 0x4204,
+	[PCIE_INTR2_CPU_BASE] = 0x4300,
 };
 
 static const int pcie_offsets_bmips_7425[] = {
 	[RGR1_SW_INIT_1] = 0x8010,
 	[EXT_CFG_INDEX]  = 0x8300,
 	[EXT_CFG_DATA]   = 0x8304,
+	[PCIE_HARD_DEBUG] = 0x4204,
+	[PCIE_INTR2_CPU_BASE] = 0x4300,
 };
 
 static const struct pcie_cfg_data generic_cfg = {
@@ -1539,6 +1546,8 @@ static const int pcie_offset_bcm7278[] = {
 	[RGR1_SW_INIT_1] = 0xc010,
 	[EXT_CFG_INDEX] = 0x9000,
 	[EXT_CFG_DATA] = 0x9004,
+	[PCIE_HARD_DEBUG] = 0x4204,
+	[PCIE_INTR2_CPU_BASE] = 0x4300,
 };
 
 static const struct pcie_cfg_data bcm7278_cfg = {
-- 
2.17.1


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

Re: [PATCH v2 06/12] PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific

[Stanimir Varbanov]

Hi Jim,

On 7/3/24 21:02, Jim Quinlan wrote:
> Our HW design has again changed a register offset which used to be standard
> for all Broadcom SOCs with PCIe cores.  This difference is now reconciled
> for the registers HARD_DEBUG and INTR2_CPU_BASE.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 33 +++++++++++++++++----------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 59daa4b2e6c5..d8c0f1474369 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -122,7 +122,6 @@
>  #define PCIE_MEM_WIN0_LIMIT_HI(win)	\
>  		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI + ((win) * 8)
>  
> -#define PCIE_MISC_HARD_PCIE_HARD_DEBUG					0x4204
>  #define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK	0x2
>  #define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK		0x200000
>  #define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK		0x08000000
> @@ -131,9 +130,9 @@
>  	  (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
>  	   PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
>  
> -#define PCIE_INTR2_CPU_BASE		0x4300
>  #define PCIE_MSI_INTR2_BASE		0x4500
> -/* Offsets from PCIE_INTR2_CPU_BASE and PCIE_MSI_INTR2_BASE */
> +
> +/* Offsets from INTR2_CPU and MSI_INTR2 BASE offsets */
>  #define  MSI_INT_STATUS			0x0
>  #define  MSI_INT_CLR			0x8
>  #define  MSI_INT_MASK_SET		0x10
> @@ -187,6 +186,8 @@
>  #define IDX_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_INDEX])
>  #define DATA_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_DATA])
>  #define PCIE_RGR1_SW_INIT_1(pcie)	(pcie->reg_offsets[RGR1_SW_INIT_1])
> +#define	HARD_DEBUG(pcie)		(pcie->reg_offsets[PCIE_HARD_DEBUG])
> +#define	INTR2_CPU_BASE(pcie)		(pcie->reg_offsets[PCIE_INTR2_CPU_BASE])
>  

Drop the tab before HARD_DEBUG & INTR2_CPU_BASE. Also checkpatch should
complain about missing brackets around pcie:

#define	HARD_DEBUG(pcie) ((pcie)->reg_offsets[PCIE_HARD_DEBUG])

~Stan

[PATCH v2 07/12] PCI: brcmstb: Remove two unused constants from driver

[Jim Quinlan]

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

Two constants in the driver, RGR1_SW_INIT_1_INIT_MASK and
RGR1_SW_INIT_1_INIT_SHIFT are no longer used and are removed.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index d8c0f1474369..3aa82871e9b3 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -210,11 +210,6 @@ enum {
 	PCIE_INTR2_CPU_BASE,
 };
 
-enum {
-	RGR1_SW_INIT_1_INIT_MASK,
-	RGR1_SW_INIT_1_INIT_SHIFT,
-};
-
 enum pcie_type {
 	GENERIC,
 	BCM7425,
-- 
2.17.1


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

Re: [PATCH v2 07/12] PCI: brcmstb: Remove two unused constants from driver

[Stanimir Varbanov]

On 7/3/24 21:02, Jim Quinlan wrote:
> Two constants in the driver, RGR1_SW_INIT_1_INIT_MASK and
> RGR1_SW_INIT_1_INIT_SHIFT are no longer used and are removed.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 5 -----
>  1 file changed, 5 deletions(-)
> 

Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>

~Stan
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index d8c0f1474369..3aa82871e9b3 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -210,11 +210,6 @@ enum {
>  	PCIE_INTR2_CPU_BASE,
>  };
>  
> -enum {
> -	RGR1_SW_INIT_1_INIT_MASK,
> -	RGR1_SW_INIT_1_INIT_SHIFT,
> -};
> -
>  enum pcie_type {
>  	GENERIC,
>  	BCM7425,

[PATCH v2 08/12] PCI: brcmstb: Don't conflate the reset rescal with phy ctrl

[Jim Quinlan]

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

We've been assuming that if an SOC has a "rescal" reset controller that we
should automatically invoke brcm_phy_cntl(...).  This will not be true in
future SOCs, so we create a bool "has_phy" and adjust the cfg_data
appropriately (we need to give 7216 its own cfg_data structure instead of
sharing one).

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 3aa82871e9b3..ffb3e8d8fb2a 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -222,6 +222,7 @@ enum pcie_type {
 struct pcie_cfg_data {
 	const int *offsets;
 	const enum pcie_type type;
+	const bool has_phy;
 	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 };
@@ -272,6 +273,7 @@ struct brcm_pcie {
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct subdev_regulators *sr;
 	bool			ep_wakeup_capable;
+	bool			has_phy;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -1311,12 +1313,12 @@ static int brcm_phy_cntl(struct brcm_pcie *pcie, const int start)
 
 static inline int brcm_phy_start(struct brcm_pcie *pcie)
 {
-	return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
+	return pcie->has_phy ? brcm_phy_cntl(pcie, 1) : 0;
 }
 
 static inline int brcm_phy_stop(struct brcm_pcie *pcie)
 {
-	return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
+	return pcie->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
 }
 
 static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
@@ -1559,12 +1561,20 @@ static const struct pcie_cfg_data bcm2711_cfg = {
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
+static const struct pcie_cfg_data bcm7216_cfg = {
+	.offsets	= pcie_offset_bcm7278,
+	.type		= BCM7278,
+	.perst_set	= brcm_pcie_perst_set_7278,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+	.has_phy	= true,
+};
+
 static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
 	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
 	{ .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
 	{ .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
-	{ .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
+	{ .compatible = "brcm,bcm7216-pcie", .data = &bcm7216_cfg },
 	{ .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
 	{ .compatible = "brcm,bcm7435-pcie", .data = &bcm7435_cfg },
 	{ .compatible = "brcm,bcm7425-pcie", .data = &bcm7425_cfg },
@@ -1612,6 +1622,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->type = data->type;
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
+	pcie->has_phy = data->has_phy;
 
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
-- 
2.17.1


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

Re: [PATCH v2 08/12] PCI: brcmstb: Don't conflate the reset rescal with phy ctrl

[Stanimir Varbanov]

Hi Jim,


On 7/3/24 21:02, Jim Quinlan wrote:
> We've been assuming that if an SOC has a "rescal" reset controller that we
> should automatically invoke brcm_phy_cntl(...).  This will not be true in
> future SOCs, so we create a bool "has_phy" and adjust the cfg_data
> appropriately (we need to give 7216 its own cfg_data structure instead of
> sharing one).
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)

Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>

~Stan
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 3aa82871e9b3..ffb3e8d8fb2a 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -222,6 +222,7 @@ enum pcie_type {
>  struct pcie_cfg_data {
>  	const int *offsets;
>  	const enum pcie_type type;
> +	const bool has_phy;
>  	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
>  	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
>  };
> @@ -272,6 +273,7 @@ struct brcm_pcie {
>  	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
>  	struct subdev_regulators *sr;
>  	bool			ep_wakeup_capable;
> +	bool			has_phy;
>  };
>  
>  static inline bool is_bmips(const struct brcm_pcie *pcie)
> @@ -1311,12 +1313,12 @@ static int brcm_phy_cntl(struct brcm_pcie *pcie, const int start)
>  
>  static inline int brcm_phy_start(struct brcm_pcie *pcie)
>  {
> -	return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
> +	return pcie->has_phy ? brcm_phy_cntl(pcie, 1) : 0;
>  }
>  
>  static inline int brcm_phy_stop(struct brcm_pcie *pcie)
>  {
> -	return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
> +	return pcie->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
>  }
>  
>  static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
> @@ -1559,12 +1561,20 @@ static const struct pcie_cfg_data bcm2711_cfg = {
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  };
>  
> +static const struct pcie_cfg_data bcm7216_cfg = {
> +	.offsets	= pcie_offset_bcm7278,
> +	.type		= BCM7278,
> +	.perst_set	= brcm_pcie_perst_set_7278,
> +	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
> +	.has_phy	= true,
> +};
> +
>  static const struct of_device_id brcm_pcie_match[] = {
>  	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
>  	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
>  	{ .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
>  	{ .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
> -	{ .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
> +	{ .compatible = "brcm,bcm7216-pcie", .data = &bcm7216_cfg },
>  	{ .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
>  	{ .compatible = "brcm,bcm7435-pcie", .data = &bcm7435_cfg },
>  	{ .compatible = "brcm,bcm7425-pcie", .data = &bcm7425_cfg },
> @@ -1612,6 +1622,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	pcie->type = data->type;
>  	pcie->perst_set = data->perst_set;
>  	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
> +	pcie->has_phy = data->has_phy;
>  
>  	pcie->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(pcie->base))

[PATCH v2 09/12] PCI: brcmstb: Refactor for chips with many regular inbound BARs

[Jim Quinlan]

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

Previously, our chips provided three inbound "BARS" with fixed purposes:
the first was for mapping SOC registers, the second was for memory, and the
third was for memory but with the endian swapped.  We typically only used
one of these BARs.

Complicating that BARs usage was the fact that the PCIe HW would do a
baroque internal mapping of system memory, and concatenate the regions of
multiple memory controllers.

Newer chips such as the 7712 and Cable Modem SOCs have taken a step forward
and now provide multiple inbound BARs.  This works in concert with the
dma-ranges property, where each provided range becomes an inbound BAR.

This commit provides support for these new chips and their multiple
inbound BARs but also keeps the legacy support for the older system.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 199 +++++++++++++++++++-------
 1 file changed, 150 insertions(+), 49 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index ffb3e8d8fb2a..5f632fdc0052 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -75,15 +75,12 @@
 #define PCIE_MEM_WIN0_HI(win)	\
 		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
 
+#define PCIE_BRCM_MAX_RC_BARS				16
 #define PCIE_MISC_RC_BAR1_CONFIG_LO			0x402c
 #define  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK		0x1f
 
-#define PCIE_MISC_RC_BAR2_CONFIG_LO			0x4034
-#define  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK		0x1f
-#define PCIE_MISC_RC_BAR2_CONFIG_HI			0x4038
+#define PCIE_MISC_RC_BAR4_CONFIG_LO			0x40d4
 
-#define PCIE_MISC_RC_BAR3_CONFIG_LO			0x403c
-#define  PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK		0x1f
 
 #define PCIE_MISC_MSI_BAR_CONFIG_LO			0x4044
 #define PCIE_MISC_MSI_BAR_CONFIG_HI			0x4048
@@ -130,6 +127,10 @@
 	  (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
 	   PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
 
+#define PCIE_MISC_UBUS_BAR1_CONFIG_REMAP			0x40ac
+#define  PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK	0x1
+#define PCIE_MISC_UBUS_BAR4_CONFIG_REMAP			0x410c
+
 #define PCIE_MSI_INTR2_BASE		0x4500
 
 /* Offsets from INTR2_CPU and MSI_INTR2 BASE offsets */
@@ -217,6 +218,13 @@ enum pcie_type {
 	BCM4908,
 	BCM7278,
 	BCM2711,
+	BCM7712,
+};
+
+struct rc_bar {
+	u64 size;
+	u64 pci_offset;
+	u64 cpu_addr;
 };
 
 struct pcie_cfg_data {
@@ -274,6 +282,7 @@ struct brcm_pcie {
 	struct subdev_regulators *sr;
 	bool			ep_wakeup_capable;
 	bool			has_phy;
+	int			num_inbound;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -789,23 +798,60 @@ static void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
 	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 }
 
-static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
-							u64 *rc_bar2_size,
-							u64 *rc_bar2_offset)
+static inline void set_bar(struct rc_bar *b, int *count, u64 size,
+			   u64 cpu_addr, u64 pci_offset)
+{
+	b->size = size;
+	b->cpu_addr = cpu_addr;
+	b->pci_offset = pci_offset;
+	(*count)++;
+}
+
+static int brcm_pcie_get_rc_bar_sizes_and_offsets(struct brcm_pcie *pcie,
+						  struct rc_bar rc_bars[])
 {
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
+	u64 pci_offset, cpu_addr, size = 0, tot_size = 0;
 	struct resource_entry *entry;
 	struct device *dev = pcie->dev;
 	u64 lowest_pcie_addr = ~(u64)0;
-	int ret, i = 0;
-	u64 size = 0;
+
+	int ret, i = 0, n = 0;
+
+	/*
+	 * The HW registers (and PCIe) use order-1 numbering for BARs. As
+	 * such, we have rc_bars[0] unused and BAR1 starts at rc_bars[1].
+	 */
+	struct rc_bar *b_begin = &rc_bars[1];
+	struct rc_bar *b = b_begin;
+
+	/*
+	 * STB chips beside 7712 disable BAR1 by default.  It is mapped not
+	 * to system memory but to a regiion all of the SOC registers.  No
+	 * one uses this anymore.
+	 */
+	if (pcie->type != BCM7712)
+		set_bar(b++, &n, 0, 0, 0);
 
 	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
 		u64 pcie_beg = entry->res->start - entry->offset;
+		u64 cpu_beg = entry->res->start;
 
-		size += entry->res->end - entry->res->start + 1;
+		size = entry->res->end - entry->res->start + 1;
+		tot_size += size;
 		if (pcie_beg < lowest_pcie_addr)
 			lowest_pcie_addr = pcie_beg;
+		/*
+		 * 7712 and newer chips may have many BARs, with each
+		 * offering a non-overlapping viewport to system memory.
+		 * That being said, each BARs size must still be a power of
+		 * two.
+		 */
+		if (pcie->type == BCM7712)
+			set_bar(b++, &n, size, cpu_beg, pcie_beg);
+
+		if (n > pcie->num_inbound)
+			break;
 	}
 
 	if (lowest_pcie_addr == ~(u64)0) {
@@ -813,13 +859,20 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 		return -EINVAL;
 	}
 
+	/*
+	 * 7712 and newer chips do not have an internal memory mapping system
+	 * that enables multiple memory controllers.  As such, it can return
+	 * now w/o doing special configuration.
+	 */
+	if (pcie->type == BCM7712)
+		return n;
+
 	ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
 						  PCIE_BRCM_MAX_MEMC);
-
 	if (ret <= 0) {
 		/* Make an educated guess */
 		pcie->num_memc = 1;
-		pcie->memc_size[0] = 1ULL << fls64(size - 1);
+		pcie->memc_size[0] = 1ULL << fls64(tot_size - 1);
 	} else {
 		pcie->num_memc = ret;
 	}
@@ -828,10 +881,15 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 	for (i = 0, size = 0; i < pcie->num_memc; i++)
 		size += pcie->memc_size[i];
 
-	/* System memory starts at this address in PCIe-space */
-	*rc_bar2_offset = lowest_pcie_addr;
-	/* The sum of all memc views must also be a power of 2 */
-	*rc_bar2_size = 1ULL << fls64(size - 1);
+	/* Our HW mandates that the window size must be a power of 2 */
+	size = 1ULL << fls64(size - 1);
+
+	/*
+	 * For STB chips, the BAR2 cpu_addr is hardwired to the start
+	 * of system memory, so we set it to 0.
+	 */
+	cpu_addr = 0;
+	pci_offset = lowest_pcie_addr;
 
 	/*
 	 * We validate the inbound memory view even though we should trust
@@ -866,25 +924,50 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 	 *   outbound memory @ 3GB). So instead it will  start at the 1x
 	 *   multiple of its size
 	 */
-	if (!*rc_bar2_size || (*rc_bar2_offset & (*rc_bar2_size - 1)) ||
-	    (*rc_bar2_offset < SZ_4G && *rc_bar2_offset > SZ_2G)) {
+	if (!size || (pci_offset & (size - 1)) ||
+	    (pci_offset < SZ_4G && pci_offset > SZ_2G)) {
 		dev_err(dev, "Invalid rc_bar2_offset/size: size 0x%llx, off 0x%llx\n",
-			*rc_bar2_size, *rc_bar2_offset);
+			size, pci_offset);
 		return -EINVAL;
 	}
 
-	return 0;
+	/* Enable BAR2, the inbound window for STB chips */
+	set_bar(b++, &n, size, cpu_addr, pci_offset);
+
+	/*
+	 * Disable BAR3.  On some chips presents the same window as BAR2
+	 * but the data appears in a settable endianness.
+	 */
+	set_bar(b++, &n, 0, 0, 0);
+
+	return n;
+}
+
+static unsigned int brcm_calc_bar_reg_offset(int bar)
+{
+	if (bar <= 3)
+		return PCIE_MISC_RC_BAR1_CONFIG_LO + 8 * (bar - 1);
+	else
+		return PCIE_MISC_RC_BAR4_CONFIG_LO + 8 * (bar - 4);
+}
+
+static unsigned int brcm_calc_ubus_reg_offset(int bar)
+{
+	if (bar <= 3)
+		return PCIE_MISC_UBUS_BAR1_CONFIG_REMAP + 8 * (bar - 1);
+	else
+		return PCIE_MISC_UBUS_BAR4_CONFIG_REMAP + 8 * (bar - 4);
 }
 
 static int brcm_pcie_setup(struct brcm_pcie *pcie)
 {
-	u64 rc_bar2_offset, rc_bar2_size;
+	struct rc_bar rc_bars[PCIE_BRCM_MAX_RC_BARS];
 	void __iomem *base = pcie->base;
 	struct pci_host_bridge *bridge;
 	struct resource_entry *entry;
 	u32 tmp, burst, aspm_support;
-	int num_out_wins = 0;
-	int ret, memc;
+	int num_out_wins = 0, num_rc_bars = 0;
+	int i, memc;
 
 	/* Reset the bridge */
 	pcie->bridge_sw_init_set(pcie, 1);
@@ -933,17 +1016,47 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK);
 	writel(tmp, base + PCIE_MISC_MISC_CTRL);
 
-	ret = brcm_pcie_get_rc_bar2_size_and_offset(pcie, &rc_bar2_size,
-						    &rc_bar2_offset);
-	if (ret)
-		return ret;
+	num_rc_bars = brcm_pcie_get_rc_bar_sizes_and_offsets(pcie, rc_bars);
+	if (num_rc_bars < 0)
+		return num_rc_bars;
+
+	for (i = 1; i <= num_rc_bars; i++) {
+		u64 pci_offset = rc_bars[i].pci_offset;
+		u64 cpu_addr = rc_bars[i].cpu_addr;
+		u64 size = rc_bars[i].size;
+		u32 reg_offset = brcm_calc_bar_reg_offset(i);
+
+		tmp = lower_32_bits(pci_offset);
+		u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(size),
+				  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK);
+
+		/* Write low */
+		writel(tmp, base + reg_offset);
+		/* Write high */
+		writel(upper_32_bits(pci_offset),
+		       base + reg_offset + 4);
+
+		/*
+		 * Most STB chips:
+		 *     Do nothing.
+		 * 7712:
+		 *     All of their BARs need to be set.
+		 */
+		if (pcie->type == BCM7712) {
+			/* BUS remap register settings */
+			reg_offset = brcm_calc_ubus_reg_offset(i);
+			tmp = lower_32_bits(cpu_addr) & ~0xfff;
+			tmp |= PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK;
+			writel(tmp, base + reg_offset);
+			tmp = upper_32_bits(cpu_addr);
+			writel(tmp, base + reg_offset + 4);
+		}
+	}
 
-	tmp = lower_32_bits(rc_bar2_offset);
-	u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(rc_bar2_size),
-			  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK);
-	writel(tmp, base + PCIE_MISC_RC_BAR2_CONFIG_LO);
-	writel(upper_32_bits(rc_bar2_offset),
-	       base + PCIE_MISC_RC_BAR2_CONFIG_HI);
+	if (!brcm_pcie_rc_mode(pcie)) {
+		dev_err(pcie->dev, "PCIe RC controller misconfigured as Endpoint\n");
+		return -EINVAL;
+	}
 
 	tmp = readl(base + PCIE_MISC_MISC_CTRL);
 	for (memc = 0; memc < pcie->num_memc; memc++) {
@@ -965,25 +1078,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	 * 4GB or when the inbound area is smaller than 4GB (taking into
 	 * account the rounding-up we're forced to perform).
 	 */
-	if (rc_bar2_offset >= SZ_4G || (rc_bar2_size + rc_bar2_offset) < SZ_4G)
+	if (rc_bars[2].pci_offset >= SZ_4G ||
+	    (rc_bars[2].size + rc_bars[2].pci_offset) < SZ_4G)
 		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_LT_4GB;
 	else
 		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_GT_4GB;
 
-	if (!brcm_pcie_rc_mode(pcie)) {
-		dev_err(pcie->dev, "PCIe RC controller misconfigured as Endpoint\n");
-		return -EINVAL;
-	}
-
-	/* disable the PCIe->GISB memory window (RC_BAR1) */
-	tmp = readl(base + PCIE_MISC_RC_BAR1_CONFIG_LO);
-	tmp &= ~PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK;
-	writel(tmp, base + PCIE_MISC_RC_BAR1_CONFIG_LO);
-
-	/* disable the PCIe->SCB memory window (RC_BAR3) */
-	tmp = readl(base + PCIE_MISC_RC_BAR3_CONFIG_LO);
-	tmp &= ~PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK;
-	writel(tmp, base + PCIE_MISC_RC_BAR3_CONFIG_LO);
 
 	/* Don't advertise L0s capability if 'aspm-no-l0s' */
 	aspm_support = PCIE_LINK_STATE_L1;
@@ -1623,6 +1723,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 	pcie->has_phy = data->has_phy;
+	pcie->num_inbound = (pcie->type == BCM7712) ? 10 : 3;
 
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
-- 
2.17.1


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

Re: [PATCH v2 09/12] PCI: brcmstb: Refactor for chips with many regular inbound BARs

[Stanimir Varbanov]

Hi Jim,

On 7/3/24 21:02, Jim Quinlan wrote:
> Previously, our chips provided three inbound "BARS" with fixed purposes:
> the first was for mapping SOC registers, the second was for memory, and the
> third was for memory but with the endian swapped.  We typically only used
> one of these BARs.
> 
> Complicating that BARs usage was the fact that the PCIe HW would do a
> baroque internal mapping of system memory, and concatenate the regions of
> multiple memory controllers.
> 
> Newer chips such as the 7712 and Cable Modem SOCs have taken a step forward
> and now provide multiple inbound BARs.  This works in concert with the
> dma-ranges property, where each provided range becomes an inbound BAR.
> 
> This commit provides support for these new chips and their multiple
> inbound BARs but also keeps the legacy support for the older system.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 199 +++++++++++++++++++-------
>  1 file changed, 150 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index ffb3e8d8fb2a..5f632fdc0052 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -75,15 +75,12 @@
>  #define PCIE_MEM_WIN0_HI(win)	\
>  		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
>  
> +#define PCIE_BRCM_MAX_RC_BARS				16
>  #define PCIE_MISC_RC_BAR1_CONFIG_LO			0x402c
>  #define  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK		0x1f
>  
> -#define PCIE_MISC_RC_BAR2_CONFIG_LO			0x4034
> -#define  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK		0x1f
> -#define PCIE_MISC_RC_BAR2_CONFIG_HI			0x4038
> +#define PCIE_MISC_RC_BAR4_CONFIG_LO			0x40d4
>  
> -#define PCIE_MISC_RC_BAR3_CONFIG_LO			0x403c
> -#define  PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK		0x1f
>  
>  #define PCIE_MISC_MSI_BAR_CONFIG_LO			0x4044
>  #define PCIE_MISC_MSI_BAR_CONFIG_HI			0x4048
> @@ -130,6 +127,10 @@
>  	  (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
>  	   PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
>  
> +#define PCIE_MISC_UBUS_BAR1_CONFIG_REMAP			0x40ac
> +#define  PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK	0x1
> +#define PCIE_MISC_UBUS_BAR4_CONFIG_REMAP			0x410c
> +
>  #define PCIE_MSI_INTR2_BASE		0x4500
>  
>  /* Offsets from INTR2_CPU and MSI_INTR2 BASE offsets */
> @@ -217,6 +218,13 @@ enum pcie_type {
>  	BCM4908,
>  	BCM7278,
>  	BCM2711,
> +	BCM7712,
> +};
> +
> +struct rc_bar {
> +	u64 size;
> +	u64 pci_offset;
> +	u64 cpu_addr;
>  };
>  
>  struct pcie_cfg_data {
> @@ -274,6 +282,7 @@ struct brcm_pcie {
>  	struct subdev_regulators *sr;
>  	bool			ep_wakeup_capable;
>  	bool			has_phy;
> +	int			num_inbound;
>  };
>  
>  static inline bool is_bmips(const struct brcm_pcie *pcie)
> @@ -789,23 +798,60 @@ static void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
>  	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
>  }
>  
> -static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
> -							u64 *rc_bar2_size,
> -							u64 *rc_bar2_offset)
> +static inline void set_bar(struct rc_bar *b, int *count, u64 size,
> +			   u64 cpu_addr, u64 pci_offset)
> +{
> +	b->size = size;
> +	b->cpu_addr = cpu_addr;
> +	b->pci_offset = pci_offset;
> +	(*count)++;
> +}
> +
> +static int brcm_pcie_get_rc_bar_sizes_and_offsets(struct brcm_pcie *pcie,
> +						  struct rc_bar rc_bars[])
>  {
>  	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> +	u64 pci_offset, cpu_addr, size = 0, tot_size = 0;
>  	struct resource_entry *entry;
>  	struct device *dev = pcie->dev;
>  	u64 lowest_pcie_addr = ~(u64)0;
> -	int ret, i = 0;
> -	u64 size = 0;
> +

Drop this ^^^ blank line.

> +	int ret, i = 0, n = 0;
> +
> +	/*
> +	 * The HW registers (and PCIe) use order-1 numbering for BARs. As
> +	 * such, we have rc_bars[0] unused and BAR1 starts at rc_bars[1].
> +	 */
> +	struct rc_bar *b_begin = &rc_bars[1];
> +	struct rc_bar *b = b_begin;
> +
> +	/*
> +	 * STB chips beside 7712 disable BAR1 by default.  It is mapped not
> +	 * to system memory but to a regiion all of the SOC registers.  No

typo.

Also I really do not understand the sentence "but to a regiion all of
the SOC registers" can you improve it, please?

> +	 * one uses this anymore.
> +	 */
> +	if (pcie->type != BCM7712)
> +		set_bar(b++, &n, 0, 0, 0);

It'd be nice if we can avoid such type (model) checks in the common
functions, but I don't have better idea.

>  
>  	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
>  		u64 pcie_beg = entry->res->start - entry->offset;
> +		u64 cpu_beg = entry->res->start;
>  
> -		size += entry->res->end - entry->res->start + 1;
> +		size = entry->res->end - entry->res->start + 1;

resource_size(entry->res) ?

> +		tot_size += size;
>  		if (pcie_beg < lowest_pcie_addr)
>  			lowest_pcie_addr = pcie_beg;
> +		/*
> +		 * 7712 and newer chips may have many BARs, with each
> +		 * offering a non-overlapping viewport to system memory.
> +		 * That being said, each BARs size must still be a power of
> +		 * two.
> +		 */
> +		if (pcie->type == BCM7712)
> +			set_bar(b++, &n, size, cpu_beg, pcie_beg);
> +
> +		if (n > pcie->num_inbound)
> +			break;
>  	}
>  
>  	if (lowest_pcie_addr == ~(u64)0) {
> @@ -813,13 +859,20 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * 7712 and newer chips do not have an internal memory mapping system
> +	 * that enables multiple memory controllers.  As such, it can return
> +	 * now w/o doing special configuration.
> +	 */
> +	if (pcie->type == BCM7712)
> +		return n;
> +
>  	ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
>  						  PCIE_BRCM_MAX_MEMC);
> -
>  	if (ret <= 0) {
>  		/* Make an educated guess */
>  		pcie->num_memc = 1;
> -		pcie->memc_size[0] = 1ULL << fls64(size - 1);
> +		pcie->memc_size[0] = 1ULL << fls64(tot_size - 1);
>  	} else {
>  		pcie->num_memc = ret;
>  	}
> @@ -828,10 +881,15 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
>  	for (i = 0, size = 0; i < pcie->num_memc; i++)
>  		size += pcie->memc_size[i];
>  
> -	/* System memory starts at this address in PCIe-space */
> -	*rc_bar2_offset = lowest_pcie_addr;
> -	/* The sum of all memc views must also be a power of 2 */
> -	*rc_bar2_size = 1ULL << fls64(size - 1);
> +	/* Our HW mandates that the window size must be a power of 2 */
> +	size = 1ULL << fls64(size - 1);
> +
> +	/*
> +	 * For STB chips, the BAR2 cpu_addr is hardwired to the start
> +	 * of system memory, so we set it to 0.
> +	 */
> +	cpu_addr = 0;
> +	pci_offset = lowest_pcie_addr;
>  
>  	/*
>  	 * We validate the inbound memory view even though we should trust
> @@ -866,25 +924,50 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
>  	 *   outbound memory @ 3GB). So instead it will  start at the 1x
>  	 *   multiple of its size
>  	 */
> -	if (!*rc_bar2_size || (*rc_bar2_offset & (*rc_bar2_size - 1)) ||
> -	    (*rc_bar2_offset < SZ_4G && *rc_bar2_offset > SZ_2G)) {
> +	if (!size || (pci_offset & (size - 1)) ||
> +	    (pci_offset < SZ_4G && pci_offset > SZ_2G)) {
>  		dev_err(dev, "Invalid rc_bar2_offset/size: size 0x%llx, off 0x%llx\n",
> -			*rc_bar2_size, *rc_bar2_offset);
> +			size, pci_offset);
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	/* Enable BAR2, the inbound window for STB chips */
> +	set_bar(b++, &n, size, cpu_addr, pci_offset);
> +
> +	/*
> +	 * Disable BAR3.  On some chips presents the same window as BAR2
> +	 * but the data appears in a settable endianness.
> +	 */
> +	set_bar(b++, &n, 0, 0, 0);
> +
> +	return n;
> +}
> +
> +static unsigned int brcm_calc_bar_reg_offset(int bar)

IMO brcm_bar_reg_offset name is better, i.e. drop "calc"

> +{
> +	if (bar <= 3)
> +		return PCIE_MISC_RC_BAR1_CONFIG_LO + 8 * (bar - 1);
> +	else
> +		return PCIE_MISC_RC_BAR4_CONFIG_LO + 8 * (bar - 4);
> +}
> +
> +static unsigned int brcm_calc_ubus_reg_offset(int bar)

ditto

> +{
> +	if (bar <= 3)
> +		return PCIE_MISC_UBUS_BAR1_CONFIG_REMAP + 8 * (bar - 1);
> +	else
> +		return PCIE_MISC_UBUS_BAR4_CONFIG_REMAP + 8 * (bar - 4);
>  }
>  
>  static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  {
> -	u64 rc_bar2_offset, rc_bar2_size;
> +	struct rc_bar rc_bars[PCIE_BRCM_MAX_RC_BARS];
>  	void __iomem *base = pcie->base;
>  	struct pci_host_bridge *bridge;
>  	struct resource_entry *entry;
>  	u32 tmp, burst, aspm_support;
> -	int num_out_wins = 0;
> -	int ret, memc;
> +	int num_out_wins = 0, num_rc_bars = 0;
> +	int i, memc;
>  
>  	/* Reset the bridge */
>  	pcie->bridge_sw_init_set(pcie, 1);
> @@ -933,17 +1016,47 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  	u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK);
>  	writel(tmp, base + PCIE_MISC_MISC_CTRL);
>  
> -	ret = brcm_pcie_get_rc_bar2_size_and_offset(pcie, &rc_bar2_size,
> -						    &rc_bar2_offset);
> -	if (ret)
> -		return ret;
> +	num_rc_bars = brcm_pcie_get_rc_bar_sizes_and_offsets(pcie, rc_bars);

Can we change the function name to brcm_pcie_get_inbound_regions? ...

> +	if (num_rc_bars < 0)
> +		return num_rc_bars;
> +
> +	for (i = 1; i <= num_rc_bars; i++) {
> +		u64 pci_offset = rc_bars[i].pci_offset;
> +		u64 cpu_addr = rc_bars[i].cpu_addr;
> +		u64 size = rc_bars[i].size;
> +		u32 reg_offset = brcm_calc_bar_reg_offset(i);

u32 is good type for reg offset but brcm_calc_bar_reg_offset() is
returning "unsigned int", could you unify the types to u32?

> +
> +		tmp = lower_32_bits(pci_offset);
> +		u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(size),
> +				  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK);
> +
> +		/* Write low */
> +		writel(tmp, base + reg_offset);
> +		/* Write high */
> +		writel(upper_32_bits(pci_offset),
> +		       base + reg_offset + 4);

no need of a new line for the second function argument.

> +
> +		/*
> +		 * Most STB chips:
> +		 *     Do nothing.
> +		 * 7712:
> +		 *     All of their BARs need to be set.
> +		 */
> +		if (pcie->type == BCM7712) {
> +			/* BUS remap register settings */
> +			reg_offset = brcm_calc_ubus_reg_offset(i);
> +			tmp = lower_32_bits(cpu_addr) & ~0xfff;
> +			tmp |= PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK;
> +			writel(tmp, base + reg_offset);
> +			tmp = upper_32_bits(cpu_addr);
> +			writel(tmp, base + reg_offset + 4);
> +		}
> +	}
>  

... and move this for loop in separate brcm_pcie_set_inbound_regions()
function?

> -	tmp = lower_32_bits(rc_bar2_offset);
> -	u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(rc_bar2_size),
> -			  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK);
> -	writel(tmp, base + PCIE_MISC_RC_BAR2_CONFIG_LO);
> -	writel(upper_32_bits(rc_bar2_offset),
> -	       base + PCIE_MISC_RC_BAR2_CONFIG_HI);
> +	if (!brcm_pcie_rc_mode(pcie)) {
> +		dev_err(pcie->dev, "PCIe RC controller misconfigured as Endpoint\n");
> +		return -EINVAL;
> +	}
>  
>  	tmp = readl(base + PCIE_MISC_MISC_CTRL);
>  	for (memc = 0; memc < pcie->num_memc; memc++) {
> @@ -965,25 +1078,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  	 * 4GB or when the inbound area is smaller than 4GB (taking into
>  	 * account the rounding-up we're forced to perform).
>  	 */
> -	if (rc_bar2_offset >= SZ_4G || (rc_bar2_size + rc_bar2_offset) < SZ_4G)
> +	if (rc_bars[2].pci_offset >= SZ_4G ||
> +	    (rc_bars[2].size + rc_bars[2].pci_offset) < SZ_4G)
>  		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_LT_4GB;
>  	else
>  		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_GT_4GB;
>  
> -	if (!brcm_pcie_rc_mode(pcie)) {
> -		dev_err(pcie->dev, "PCIe RC controller misconfigured as Endpoint\n");
> -		return -EINVAL;
> -	}
> -
> -	/* disable the PCIe->GISB memory window (RC_BAR1) */
> -	tmp = readl(base + PCIE_MISC_RC_BAR1_CONFIG_LO);
> -	tmp &= ~PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK;
> -	writel(tmp, base + PCIE_MISC_RC_BAR1_CONFIG_LO);
> -
> -	/* disable the PCIe->SCB memory window (RC_BAR3) */
> -	tmp = readl(base + PCIE_MISC_RC_BAR3_CONFIG_LO);
> -	tmp &= ~PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK;
> -	writel(tmp, base + PCIE_MISC_RC_BAR3_CONFIG_LO);
>  
>  	/* Don't advertise L0s capability if 'aspm-no-l0s' */
>  	aspm_support = PCIE_LINK_STATE_L1;
> @@ -1623,6 +1723,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	pcie->perst_set = data->perst_set;
>  	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
>  	pcie->has_phy = data->has_phy;
> +	pcie->num_inbound = (pcie->type == BCM7712) ? 10 : 3;

num_inbound could be part of struct pcie_cfg_data ?

>  
>  	pcie->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(pcie->base))

regards,
Stan

Re: [PATCH v2 09/12] PCI: brcmstb: Refactor for chips with many regular inbound BARs

[Stanimir Varbanov]

Hi,

On 7/4/24 16:30, Stanimir Varbanov wrote:
> Hi Jim,
> 
> On 7/3/24 21:02, Jim Quinlan wrote:
>> Previously, our chips provided three inbound "BARS" with fixed purposes:
>> the first was for mapping SOC registers, the second was for memory, and the
>> third was for memory but with the endian swapped.  We typically only used
>> one of these BARs.
>>
>> Complicating that BARs usage was the fact that the PCIe HW would do a
>> baroque internal mapping of system memory, and concatenate the regions of
>> multiple memory controllers.
>>
>> Newer chips such as the 7712 and Cable Modem SOCs have taken a step forward
>> and now provide multiple inbound BARs.  This works in concert with the
>> dma-ranges property, where each provided range becomes an inbound BAR.
>>
>> This commit provides support for these new chips and their multiple
>> inbound BARs but also keeps the legacy support for the older system.
>>
>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>> ---
>>  drivers/pci/controller/pcie-brcmstb.c | 199 +++++++++++++++++++-------
>>  1 file changed, 150 insertions(+), 49 deletions(-)

<cut>

>>  static int brcm_pcie_setup(struct brcm_pcie *pcie)
>>  {
>> -	u64 rc_bar2_offset, rc_bar2_size;
>> +	struct rc_bar rc_bars[PCIE_BRCM_MAX_RC_BARS];
>>  	void __iomem *base = pcie->base;
>>  	struct pci_host_bridge *bridge;
>>  	struct resource_entry *entry;
>>  	u32 tmp, burst, aspm_support;
>> -	int num_out_wins = 0;
>> -	int ret, memc;
>> +	int num_out_wins = 0, num_rc_bars = 0;
>> +	int i, memc;
>>  
>>  	/* Reset the bridge */
>>  	pcie->bridge_sw_init_set(pcie, 1);
>> @@ -933,17 +1016,47 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>>  	u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK);
>>  	writel(tmp, base + PCIE_MISC_MISC_CTRL);
>>  
>> -	ret = brcm_pcie_get_rc_bar2_size_and_offset(pcie, &rc_bar2_size,
>> -						    &rc_bar2_offset);
>> -	if (ret)
>> -		return ret;
>> +	num_rc_bars = brcm_pcie_get_rc_bar_sizes_and_offsets(pcie, rc_bars);
> 
> Can we change the function name to brcm_pcie_get_inbound_regions? ...

... or better brcm_pcie_get_inbound_wins() ?

~Stan

Re: [PATCH v2 09/12] PCI: brcmstb: Refactor for chips with many regular inbound BARs

[Stanimir Varbanov]

Hi Jim,

On 7/3/24 21:02, Jim Quinlan wrote:
> Previously, our chips provided three inbound "BARS" with fixed purposes:
> the first was for mapping SOC registers, the second was for memory, and the
> third was for memory but with the endian swapped.  We typically only used
> one of these BARs.
> 
> Complicating that BARs usage was the fact that the PCIe HW would do a
> baroque internal mapping of system memory, and concatenate the regions of
> multiple memory controllers.
> 
> Newer chips such as the 7712 and Cable Modem SOCs have taken a step forward
> and now provide multiple inbound BARs.  This works in concert with the
> dma-ranges property, where each provided range becomes an inbound BAR.
> 
> This commit provides support for these new chips and their multiple
> inbound BARs but also keeps the legacy support for the older system.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 199 +++++++++++++++++++-------
>  1 file changed, 150 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index ffb3e8d8fb2a..5f632fdc0052 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -75,15 +75,12 @@
>  #define PCIE_MEM_WIN0_HI(win)	\
>  		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
>  
> +#define PCIE_BRCM_MAX_RC_BARS				16
>  #define PCIE_MISC_RC_BAR1_CONFIG_LO			0x402c
>  #define  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK		0x1f
>  
> -#define PCIE_MISC_RC_BAR2_CONFIG_LO			0x4034
> -#define  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK		0x1f
> -#define PCIE_MISC_RC_BAR2_CONFIG_HI			0x4038
> +#define PCIE_MISC_RC_BAR4_CONFIG_LO			0x40d4
>  
> -#define PCIE_MISC_RC_BAR3_CONFIG_LO			0x403c
> -#define  PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK		0x1f
>  
>  #define PCIE_MISC_MSI_BAR_CONFIG_LO			0x4044
>  #define PCIE_MISC_MSI_BAR_CONFIG_HI			0x4048
> @@ -130,6 +127,10 @@
>  	  (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
>  	   PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
>  
> +#define PCIE_MISC_UBUS_BAR1_CONFIG_REMAP			0x40ac
> +#define  PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK	0x1

could you use BIT(0)

~Stan

[PATCH v2 10/12] PCI: brcmstb: Check return value of all reset_control_xxx calls

[Jim Quinlan]

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

In some cases the result of a reset_control_xxx() call have been ignored.
Now we check all return values of such functions and at the least issue a
dev_err(...) message if the return value is not zero.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 33 ++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 5f632fdc0052..1c3ce0c182d1 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -743,11 +743,16 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
 
 static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
 {
+	int ret;
+
 	if (pcie->bridge) {
 		if (val)
-			reset_control_assert(pcie->bridge);
+			ret = reset_control_assert(pcie->bridge);
 		else
-			reset_control_deassert(pcie->bridge);
+			ret = reset_control_deassert(pcie->bridge);
+		if (ret)
+			dev_err(pcie->dev, "failed to %s 'bridge' reset, err=%d\n",
+				val ? "assert" : "deassert", ret);
 	} else {
 		u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
 		u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
@@ -770,13 +775,20 @@ static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
 
 static void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val)
 {
+	int ret;
+
 	if (WARN_ONCE(!pcie->perst_reset, "missing PERST# reset controller\n"))
 		return;
 
 	if (val)
-		reset_control_assert(pcie->perst_reset);
+		ret = reset_control_assert(pcie->perst_reset);
 	else
-		reset_control_deassert(pcie->perst_reset);
+		ret = reset_control_deassert(pcie->perst_reset);
+
+	if (ret)
+		dev_err(pcie->dev, "failed to %s 'perst' reset, err=%d\n",
+			val ? "assert" : "deassert", ret);
+
 }
 
 static void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val)
@@ -1460,7 +1472,7 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
-	int ret;
+	int ret, rret;
 
 	brcm_pcie_turn_off(pcie);
 	/*
@@ -1491,7 +1503,10 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
 						     pcie->sr->supplies);
 			if (ret) {
 				dev_err(dev, "Could not turn off regulators\n");
-				reset_control_reset(pcie->rescal);
+				rret = reset_control_reset(pcie->rescal);
+				if (rret)
+					dev_err(dev, "failed to reset 'rascal' controller ret=%d\n",
+						rret);
 				return ret;
 			}
 		}
@@ -1506,7 +1521,7 @@ static int brcm_pcie_resume_noirq(struct device *dev)
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
 	void __iomem *base;
 	u32 tmp;
-	int ret;
+	int ret, rret;
 
 	base = pcie->base;
 	ret = clk_prepare_enable(pcie->clk);
@@ -1568,7 +1583,9 @@ static int brcm_pcie_resume_noirq(struct device *dev)
 	if (pcie->sr)
 		regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
 err_reset:
-	reset_control_rearm(pcie->rescal);
+	rret = reset_control_rearm(pcie->rescal);
+	if (rret)
+		dev_err(pcie->dev, "failed to rearm 'rescal' reset, err=%d\n", rret);
 err_disable_clk:
 	clk_disable_unprepare(pcie->clk);
 	return ret;
-- 
2.17.1


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

Re: [PATCH v2 10/12] PCI: brcmstb: Check return value of all reset_control_xxx calls

[Stanimir Varbanov]

Hi Jim,

On 7/3/24 21:02, Jim Quinlan wrote:
> In some cases the result of a reset_control_xxx() call have been ignored.
> Now we check all return values of such functions and at the least issue a
> dev_err(...) message if the return value is not zero.
> 

When I made the comment for the return value of reset_control_xxx API I
was thinking for propagating the error to upper PCI layer and not just
print it.

Printing the error is a step forward but I don't think it is enough.
Please drop the patch from the series, we can fix that problem in the
driver with follow up patches.

~Stan

> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 33 ++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 

Re: [PATCH v2 10/12] PCI: brcmstb: Check return value of all reset_control_xxx calls

[Jim Quinlan]

[-- Attachment #1.1: Type: text/plain, Size: 1066 bytes --]

On Thu, Jul 4, 2024 at 9:49 AM Stanimir Varbanov <svarbanov@suse.de> wrote:

> Hi Jim,
>
> On 7/3/24 21:02, Jim Quinlan wrote:
> > In some cases the result of a reset_control_xxx() call have been ignored.
> > Now we check all return values of such functions and at the least issue a
> > dev_err(...) message if the return value is not zero.
> >
>
> When I made the comment for the return value of reset_control_xxx API I
> was thinking for propagating the error to upper PCI layer and not just
> print it.
>
> Printing the error is a step forward but I don't think it is enough.
> Please drop the patch from the series, we can fix that problem in the
> driver with follow up patches.
>

I'll return the value up the chain as you want as there is a list of things
anyway for v3.

Regards
Jim Quinlan
Broadcom STB

>
> ~Stan
>
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 33 ++++++++++++++++++++-------
> >  1 file changed, 25 insertions(+), 8 deletions(-)
> >
>

[-- Attachment #1.2: Type: text/html, Size: 1717 bytes --]

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

[PATCH v2 11/12] PCI: brcmstb: Enable 7712 SOCs

[Jim Quinlan]

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

The Broadcom STB 7712 is the sibling chip of the RPi 5 (2712).

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 1c3ce0c182d1..39d7dea282ff 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1216,7 +1216,9 @@ static void brcm_config_clkreq(struct brcm_pcie *pcie)
 		 * atypical and should happen only with older devices.
 		 */
 		clkreq_cntl |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK;
-		brcm_extend_rbus_timeout(pcie);
+		/* 7712 does not have this (RGR1) timer */
+		if (pcie->type != BCM7712)
+			brcm_extend_rbus_timeout(pcie);
 
 	} else {
 		/*
@@ -1628,6 +1630,13 @@ static const int pcie_offsets_bmips_7425[] = {
 	[PCIE_INTR2_CPU_BASE] = 0x4300,
 };
 
+static const int pcie_offset_bcm7712[] = {
+	[EXT_CFG_INDEX]  = 0x9000,
+	[EXT_CFG_DATA]   = 0x9004,
+	[PCIE_HARD_DEBUG] = 0x4304,
+	[PCIE_INTR2_CPU_BASE] = 0x4400,
+};
+
 static const struct pcie_cfg_data generic_cfg = {
 	.offsets	= pcie_offsets,
 	.type		= GENERIC,
@@ -1686,6 +1695,13 @@ static const struct pcie_cfg_data bcm7216_cfg = {
 	.has_phy	= true,
 };
 
+static const struct pcie_cfg_data bcm7712_cfg = {
+	.offsets	= pcie_offset_bcm7712,
+	.perst_set	= brcm_pcie_perst_set_7278,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.type		= BCM7712,
+};
+
 static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
 	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
@@ -1695,6 +1711,7 @@ static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
 	{ .compatible = "brcm,bcm7435-pcie", .data = &bcm7435_cfg },
 	{ .compatible = "brcm,bcm7425-pcie", .data = &bcm7425_cfg },
+	{ .compatible = "brcm,bcm7712-pcie", .data = &bcm7712_cfg },
 	{},
 };
 
-- 
2.17.1


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

Re: [PATCH v2 11/12] PCI: brcmstb: Enable 7712 SOCs

[Stanimir Varbanov]

Hi Jim,

On 7/3/24 21:02, Jim Quinlan wrote:
> The Broadcom STB 7712 is the sibling chip of the RPi 5 (2712).
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 1c3ce0c182d1..39d7dea282ff 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1216,7 +1216,9 @@ static void brcm_config_clkreq(struct brcm_pcie *pcie)
>  		 * atypical and should happen only with older devices.
>  		 */
>  		clkreq_cntl |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK;
> -		brcm_extend_rbus_timeout(pcie);
> +		/* 7712 does not have this (RGR1) timer */
> +		if (pcie->type != BCM7712)
> +			brcm_extend_rbus_timeout(pcie);

I'd move the check in brcm_extend_rbus_timeout() function.

Otherwise:

Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>

>  
>  	} else {
>  		/*
> @@ -1628,6 +1630,13 @@ static const int pcie_offsets_bmips_7425[] = {
>  	[PCIE_INTR2_CPU_BASE] = 0x4300,
>  };
>  
> +static const int pcie_offset_bcm7712[] = {
> +	[EXT_CFG_INDEX]  = 0x9000,
> +	[EXT_CFG_DATA]   = 0x9004,
> +	[PCIE_HARD_DEBUG] = 0x4304,
> +	[PCIE_INTR2_CPU_BASE] = 0x4400,
> +};
> +
>  static const struct pcie_cfg_data generic_cfg = {
>  	.offsets	= pcie_offsets,
>  	.type		= GENERIC,
> @@ -1686,6 +1695,13 @@ static const struct pcie_cfg_data bcm7216_cfg = {
>  	.has_phy	= true,
>  };
>  
> +static const struct pcie_cfg_data bcm7712_cfg = {
> +	.offsets	= pcie_offset_bcm7712,
> +	.perst_set	= brcm_pcie_perst_set_7278,
> +	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> +	.type		= BCM7712,
> +};
> +
>  static const struct of_device_id brcm_pcie_match[] = {
>  	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
>  	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
> @@ -1695,6 +1711,7 @@ static const struct of_device_id brcm_pcie_match[] = {
>  	{ .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
>  	{ .compatible = "brcm,bcm7435-pcie", .data = &bcm7435_cfg },
>  	{ .compatible = "brcm,bcm7425-pcie", .data = &bcm7425_cfg },
> +	{ .compatible = "brcm,bcm7712-pcie", .data = &bcm7712_cfg },
>  	{},
>  };
>  

[PATCH v2 12/12] PCI: brcmstb: Change field name from 'type' to 'model'

[Jim Quinlan]

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

The 'type' field used in the driver to discern SoC differences is confusing
so change it to the more apt 'model'.  We considered using 'family' but
this conflicts with Broadcom's conception of a family; for example, 7216a0
and 7216b0 chips are both considered separate families as each has multiple
derivative product chips based on the original design.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 48 +++++++++++++--------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 39d7dea282ff..3ab35d3dbe36 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -211,7 +211,7 @@ enum {
 	PCIE_INTR2_CPU_BASE,
 };
 
-enum pcie_type {
+enum pcie_model {
 	GENERIC,
 	BCM7425,
 	BCM7435,
@@ -229,7 +229,7 @@ struct rc_bar {
 
 struct pcie_cfg_data {
 	const int *offsets;
-	const enum pcie_type type;
+	const enum pcie_model model;
 	const bool has_phy;
 	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
@@ -269,7 +269,7 @@ struct brcm_pcie {
 	u64			msi_target_addr;
 	struct brcm_msi		*msi;
 	const int		*reg_offsets;
-	enum pcie_type		type;
+	enum pcie_model		model;
 	struct reset_control	*rescal;
 	struct reset_control	*perst_reset;
 	struct reset_control	*bridge;
@@ -287,7 +287,7 @@ struct brcm_pcie {
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
 {
-	return pcie->type == BCM7435 || pcie->type == BCM7425;
+	return pcie->model == BCM7435 || pcie->model == BCM7425;
 }
 
 /*
@@ -842,7 +842,7 @@ static int brcm_pcie_get_rc_bar_sizes_and_offsets(struct brcm_pcie *pcie,
 	 * to system memory but to a regiion all of the SOC registers.  No
 	 * one uses this anymore.
 	 */
-	if (pcie->type != BCM7712)
+	if (pcie->model != BCM7712)
 		set_bar(b++, &n, 0, 0, 0);
 
 	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
@@ -859,7 +859,7 @@ static int brcm_pcie_get_rc_bar_sizes_and_offsets(struct brcm_pcie *pcie,
 		 * That being said, each BARs size must still be a power of
 		 * two.
 		 */
-		if (pcie->type == BCM7712)
+		if (pcie->model == BCM7712)
 			set_bar(b++, &n, size, cpu_beg, pcie_beg);
 
 		if (n > pcie->num_inbound)
@@ -876,7 +876,7 @@ static int brcm_pcie_get_rc_bar_sizes_and_offsets(struct brcm_pcie *pcie,
 	 * that enables multiple memory controllers.  As such, it can return
 	 * now w/o doing special configuration.
 	 */
-	if (pcie->type == BCM7712)
+	if (pcie->model == BCM7712)
 		return n;
 
 	ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
@@ -985,7 +985,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	pcie->bridge_sw_init_set(pcie, 1);
 
 	/* Ensure that PERST# is asserted; some bootloaders may deassert it. */
-	if (pcie->type == BCM2711)
+	if (pcie->model == BCM2711)
 		pcie->perst_set(pcie, 1);
 
 	usleep_range(100, 200);
@@ -1009,9 +1009,9 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	 */
 	if (is_bmips(pcie))
 		burst = 0x1; /* 256 bytes */
-	else if (pcie->type == BCM2711)
+	else if (pcie->model == BCM2711)
 		burst = 0x0; /* 128 bytes */
-	else if (pcie->type == BCM7278)
+	else if (pcie->model == BCM7278)
 		burst = 0x3; /* 512 bytes */
 	else
 		burst = 0x2; /* 512 bytes */
@@ -1054,7 +1054,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 		 * 7712:
 		 *     All of their BARs need to be set.
 		 */
-		if (pcie->type == BCM7712) {
+		if (pcie->model == BCM7712) {
 			/* BUS remap register settings */
 			reg_offset = brcm_calc_ubus_reg_offset(i);
 			tmp = lower_32_bits(cpu_addr) & ~0xfff;
@@ -1217,7 +1217,7 @@ static void brcm_config_clkreq(struct brcm_pcie *pcie)
 		 */
 		clkreq_cntl |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK;
 		/* 7712 does not have this (RGR1) timer */
-		if (pcie->type != BCM7712)
+		if (pcie->model != BCM7712)
 			brcm_extend_rbus_timeout(pcie);
 
 	} else {
@@ -1639,28 +1639,28 @@ static const int pcie_offset_bcm7712[] = {
 
 static const struct pcie_cfg_data generic_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= GENERIC,
+	.model		= GENERIC,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
 static const struct pcie_cfg_data bcm7425_cfg = {
 	.offsets	= pcie_offsets_bmips_7425,
-	.type		= BCM7425,
+	.model		= BCM7425,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
 static const struct pcie_cfg_data bcm7435_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= BCM7435,
+	.model		= BCM7435,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
 static const struct pcie_cfg_data bcm4908_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= BCM4908,
+	.model		= BCM4908,
 	.perst_set	= brcm_pcie_perst_set_4908,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
@@ -1675,21 +1675,21 @@ static const int pcie_offset_bcm7278[] = {
 
 static const struct pcie_cfg_data bcm7278_cfg = {
 	.offsets	= pcie_offset_bcm7278,
-	.type		= BCM7278,
+	.model		= BCM7278,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
 };
 
 static const struct pcie_cfg_data bcm2711_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= BCM2711,
+	.model		= BCM2711,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
 static const struct pcie_cfg_data bcm7216_cfg = {
 	.offsets	= pcie_offset_bcm7278,
-	.type		= BCM7278,
+	.model		= BCM7278,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
 	.has_phy	= true,
@@ -1699,7 +1699,7 @@ static const struct pcie_cfg_data bcm7712_cfg = {
 	.offsets	= pcie_offset_bcm7712,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
-	.type		= BCM7712,
+	.model		= BCM7712,
 };
 
 static const struct of_device_id brcm_pcie_match[] = {
@@ -1753,11 +1753,11 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->dev = &pdev->dev;
 	pcie->np = np;
 	pcie->reg_offsets = data->offsets;
-	pcie->type = data->type;
+	pcie->model = data->model;
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 	pcie->has_phy = data->has_phy;
-	pcie->num_inbound = (pcie->type == BCM7712) ? 10 : 3;
+	pcie->num_inbound = (pcie->model == BCM7712) ? 10 : 3;
 
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
@@ -1828,7 +1828,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		goto fail;
 
 	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
-	if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
+	if (pcie->model == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
 		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
 		ret = -ENODEV;
 		goto fail;
@@ -1843,7 +1843,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		}
 	}
 
-	bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
+	bridge->ops = pcie->model == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
 	bridge->sysdata = pcie;
 
 	platform_set_drvdata(pdev, pcie);
-- 
2.17.1


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