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

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

[Jim Quinlan]

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

V4 Changes:
  o Commit "Check return value of all reset_control_xxx calls"
    -- Blank line before "return" (Stan)
  o Commit "Use common error handling code in brcmstb_probe()"
    -- Drop the "Fixes" tag (Stan)
  o Commit "dt-bindings: PCI ..."
    -- Separate the main commit into two: cleanup and adding the
       7712 SoC (Krzysztof)
    -- Fold maintainer change commit into cleanup change (Krzysztof)
    -- Use minItems/maxItems where appropriate (Krzysztof)
    -- Consistent order of resets/reset-names in decl and usage
       (Krzysztof)

V3 Changes:
  o Commit "Enable 7712 SOCs"
    -- Move "model" check from outside to inside func (Stan)
  o Commit "Check return value of all reset_control_xxx calls"
    -- Propagate errors up the chain instead of ignoring them (Stan)
  o Commit "Refactor for chips with many regular inbound BARs"
    -- Nine suggestions given, nine implemented (Stan)
  o Commit "Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific"
    -- Drop tab, add parens around macro params in expression (Stan)
  o Commit "Use swinit reset if available"
    -- Treat swinit the same as other reset controllers (Stan)
       Stan suggested to use dev_err_probe() for getting resources
       but I will defer that to future series (if that's okay).
  o Commit "Get resource before we start asserting resets"
    -- Squash this with previous commit (Stan)
  o Commit "Use "clk_out" error path label"
    -- Move clk_prepare_enable() after getting resouurces (Stan)
    -- Change subject to "Use more common error handling code in
       brcm_pcie_probe()" (Markus)
    -- Use imperative commit description (Markus)
    -- "Fixes:" tag added for missing error return. (Markus)
  o Commit "dt-bindings: PCI ..."
    -- Split off maintainer change in separate commit.
    -- Tried to accomodate Krzysztof's requests, I'm not sure I
       have succeeded.  Krzysztof, please see [1] below.
  
  [1] Wrt the YAML of brcmstb PCIe resets, here is what I am trying
      to describe:

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


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: Cleanup of brcmstb YAML and add 7712 SoC
  dt-bindings: PCI: brcmstb: Add 7712 SoC description
  PCI: brcmstb: Use common error handling code in brcm_pcie_probe()
  PCI: brcmstb: Use bridge reset if available
  PCI: brcmstb: Use swinit reset if available
  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: Change field name from 'type' to 'model'
  PCI: brcmstb: Enable 7712 SOCs

 .../bindings/pci/brcm,stb-pcie.yaml           |  50 +-
 drivers/pci/controller/pcie-brcmstb.c         | 485 +++++++++++++-----
 2 files changed, 400 insertions(+), 135 deletions(-)


base-commit: 55027e689933ba2e64f3d245fb1ff185b3e7fc81
-- 
2.17.1


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

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

[Jim Quinlan]

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

o Change order of the compatible strings to be alphabetical

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

o Add minItems/maxItems where needed.

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

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

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


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

[PATCH v4 02/12] dt-bindings: PCI: brcmstb: Add 7712 SoC description

[Jim Quinlan]

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

This adds the description for the 7712 SoC, a Broadcom
STB sibling chip of the RPi 5.  Two new reset controllers
are described.

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

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 692f7ed7c98e..90683a0df2c5 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -21,6 +21,7 @@ properties:
           - brcm,bcm7425-pcie # Broadcom 7425 MIPs
           - brcm,bcm7435-pcie # Broadcom 7435 MIPs
           - brcm,bcm7445-pcie # Broadcom 7445 Arm
+          - brcm,bcm7712-pcie # Broadcom STB sibling of Rpi 5
 
   reg:
     maxItems: 1
@@ -100,12 +101,16 @@ properties:
     items:
       - description: reset for external PCIe PERST# signal # perst
       - description: reset for phy reset calibration       # rescal
+      - description: reset for PCIe/CPU bus bridge         # bridge
+      - description: reset for soft PCIe core reset        # swinit
 
   reset-names:
     minItems: 1
     items:
       - const: perst
       - const: rescal
+      - const: bridge
+      - const: swinit
 
 required:
   - compatible
@@ -159,6 +164,27 @@ allOf:
         - resets
         - reset-names
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: brcm,bcm7712-pcie
+    then:
+      properties:
+        resets:
+          minItems: 3
+          maxItems: 3
+
+        reset-names:
+          items:
+            - const: rescal
+            - const: bridge
+            - const: swinit
+
+      required:
+        - resets
+        - reset-names
+
 unevaluatedProperties: false
 
 examples:
-- 
2.17.1


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

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

[Krzysztof Kozlowski]

On 16/07/2024 23:31, Jim Quinlan wrote:
> o Change order of the compatible strings to be alphabetical
> 
> o Describe resets/reset-names before using them in rules
> 

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

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

Thank you.
</form letter>

> o Add minItems/maxItems where needed.
> 
> o Change maintainer: Nicolas has not been active for a while.  It also
>   makes sense for a Broadcom employee to be the maintainer as many of the
>   details are privy to Broadcom.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 11f8ea33240c..692f7ed7c98e 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>  title: Brcmstb PCIe Host Controller
>  
>  maintainers:
> -  - Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> +  - Jim Quinlan <james.quinlan@broadcom.com>
>  
>  properties:
>    compatible:
> @@ -16,11 +16,11 @@ properties:
>            - brcm,bcm2711-pcie # The Raspberry Pi 4
>            - brcm,bcm4908-pcie
>            - brcm,bcm7211-pcie # Broadcom STB version of RPi4
> -          - brcm,bcm7278-pcie # Broadcom 7278 Arm
>            - brcm,bcm7216-pcie # Broadcom 7216 Arm
> -          - brcm,bcm7445-pcie # Broadcom 7445 Arm
> +          - brcm,bcm7278-pcie # Broadcom 7278 Arm
>            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
>            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
> +          - brcm,bcm7445-pcie # Broadcom 7445 Arm
>  
>    reg:
>      maxItems: 1
> @@ -95,6 +95,18 @@ properties:
>        minItems: 1
>        maxItems: 3
>  
> +  resets:
> +    minItems: 1
> +    items:
> +      - description: reset for external PCIe PERST# signal # perst
> +      - description: reset for phy reset calibration       # rescal
> +
> +  reset-names:
> +    minItems: 1
> +    items:
> +      - const: perst
> +      - const: rescal

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

Best regards,
Krzysztof

Re: [PATCH v4 02/12] dt-bindings: PCI: brcmstb: Add 7712 SoC description

[Krzysztof Kozlowski]

On 16/07/2024 23:31, Jim Quinlan wrote:
> This adds the description for the 7712 SoC, a Broadcom
> STB sibling chip of the RPi 5.  Two new reset controllers
> are described.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 692f7ed7c98e..90683a0df2c5 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -21,6 +21,7 @@ properties:
>            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
>            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
>            - brcm,bcm7445-pcie # Broadcom 7445 Arm
> +          - brcm,bcm7712-pcie # Broadcom STB sibling of Rpi 5
>  
>    reg:
>      maxItems: 1
> @@ -100,12 +101,16 @@ properties:
>      items:
>        - description: reset for external PCIe PERST# signal # perst
>        - description: reset for phy reset calibration       # rescal
> +      - description: reset for PCIe/CPU bus bridge         # bridge
> +      - description: reset for soft PCIe core reset        # swinit
>  
>    reset-names:
>      minItems: 1
>      items:
>        - const: perst
>        - const: rescal
> +      - const: bridge
> +      - const: swinit

This does not match at all what you have in allOf:if:then section.

>  
>  required:
>    - compatible
> @@ -159,6 +164,27 @@ allOf:
>          - resets
>          - reset-names
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: brcm,bcm7712-pcie
> +    then:
> +      properties:
> +        resets:
> +          minItems: 3
> +          maxItems: 3
> +
> +        reset-names:
> +          items:
> +            - const: rescal

Look - here it is rescal. Before you said it must be perst.

https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132

Best regards,
Krzysztof

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

[Jim Quinlan]

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

On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/07/2024 23:31, Jim Quinlan wrote:
> > o Change order of the compatible strings to be alphabetical
> >
> > o Describe resets/reset-names before using them in rules
> >
>
> <form letter>
> This is a friendly reminder during the review process.
>
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
>
> Thank you.
> </form letter>

I'm sorry Krzysztof, but AFAICT I paid attention to all of your comments and
tried to answer them as best as I could.  Please do not resort to a
form letter; if
you think I missed something(s) please oblige me and say what it is rather than
having  me search for something that you know and I do not.

>
> > o Add minItems/maxItems where needed.
> >
> > o Change maintainer: Nicolas has not been active for a while.  It also
> >   makes sense for a Broadcom employee to be the maintainer as many of the
> >   details are privy to Broadcom.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 11f8ea33240c..692f7ed7c98e 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
> >  title: Brcmstb PCIe Host Controller
> >
> >  maintainers:
> > -  - Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > +  - Jim Quinlan <james.quinlan@broadcom.com>
> >
> >  properties:
> >    compatible:
> > @@ -16,11 +16,11 @@ properties:
> >            - brcm,bcm2711-pcie # The Raspberry Pi 4
> >            - brcm,bcm4908-pcie
> >            - brcm,bcm7211-pcie # Broadcom STB version of RPi4
> > -          - brcm,bcm7278-pcie # Broadcom 7278 Arm
> >            - brcm,bcm7216-pcie # Broadcom 7216 Arm
> > -          - brcm,bcm7445-pcie # Broadcom 7445 Arm
> > +          - brcm,bcm7278-pcie # Broadcom 7278 Arm
> >            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
> >            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
> > +          - brcm,bcm7445-pcie # Broadcom 7445 Arm
> >
> >    reg:
> >      maxItems: 1
> > @@ -95,6 +95,18 @@ properties:
> >        minItems: 1
> >        maxItems: 3
> >
> > +  resets:
> > +    minItems: 1
> > +    items:
> > +      - description: reset for external PCIe PERST# signal # perst
> > +      - description: reset for phy reset calibration       # rescal
> > +
> > +  reset-names:
> > +    minItems: 1
> > +    items:
> > +      - const: perst
> > +      - const: rescal
>
> There are no devices with two resets. Anyway, this does not match one of
> your variants which have first element as rescal.

The 4908 chip exclusively uses the "perst" reset, and the  7216 chip
exclusively uses the "rescal" reset.   The rest of the chips use zero
resets.   All together, there are two resets.

You are the one that wanted me to first list all resets at the top,
and refer to them by the conditional rules.

What am I missing here?

Regards,
Jim Quinlan
Broadcom STB/CM
>
> Best regards,
> Krzysztof
>

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

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

[Krzysztof Kozlowski]

On 17/07/2024 15:20, Jim Quinlan wrote:
> On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 16/07/2024 23:31, Jim Quinlan wrote:
>>> o Change order of the compatible strings to be alphabetical
>>>
>>> o Describe resets/reset-names before using them in rules
>>>
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>> </form letter>
> 
> I'm sorry Krzysztof, but AFAICT I paid attention to all of your comments and
> tried to answer them as best as I could.  Please do not resort to a
> form letter; if
> you think I missed something(s) please oblige me and say what it is rather than
> having  me search for something that you know and I do not.

I do not see your response at all to my comments on patch #2.

> 
>>
>>> o Add minItems/maxItems where needed.
>>>
>>> o Change maintainer: Nicolas has not been active for a while.  It also
>>>   makes sense for a Broadcom employee to be the maintainer as many of the
>>>   details are privy to Broadcom.
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
>>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 11f8ea33240c..692f7ed7c98e 100644
>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> @@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>>>  title: Brcmstb PCIe Host Controller
>>>
>>>  maintainers:
>>> -  - Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>> +  - Jim Quinlan <james.quinlan@broadcom.com>
>>>
>>>  properties:
>>>    compatible:
>>> @@ -16,11 +16,11 @@ properties:
>>>            - brcm,bcm2711-pcie # The Raspberry Pi 4
>>>            - brcm,bcm4908-pcie
>>>            - brcm,bcm7211-pcie # Broadcom STB version of RPi4
>>> -          - brcm,bcm7278-pcie # Broadcom 7278 Arm
>>>            - brcm,bcm7216-pcie # Broadcom 7216 Arm
>>> -          - brcm,bcm7445-pcie # Broadcom 7445 Arm
>>> +          - brcm,bcm7278-pcie # Broadcom 7278 Arm
>>>            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
>>>            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
>>> +          - brcm,bcm7445-pcie # Broadcom 7445 Arm
>>>
>>>    reg:
>>>      maxItems: 1
>>> @@ -95,6 +95,18 @@ properties:
>>>        minItems: 1
>>>        maxItems: 3
>>>
>>> +  resets:
>>> +    minItems: 1
>>> +    items:
>>> +      - description: reset for external PCIe PERST# signal # perst
>>> +      - description: reset for phy reset calibration       # rescal
>>> +
>>> +  reset-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: perst
>>> +      - const: rescal
>>
>> There are no devices with two resets. Anyway, this does not match one of
>> your variants which have first element as rescal.
> 
> The 4908 chip exclusively uses the "perst" reset, and the  7216 chip
> exclusively uses the "rescal" reset.   The rest of the chips use zero
> resets.   All together, there are two resets.

This is not enum, but a list. What you do mean overall two resets? You
have a chip which is both 4908 and 7216 at the same time? How is this
possible?

> 
> You are the one that wanted me to first list all resets at the top,
> and refer to them by the conditional rules.

No, I wanted widest constraints at the top.

My comment at v2 was already saying this:

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

and you have numerous examples in the code for this.

Best regards,
Krzysztof

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

[Florian Fainelli]

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

On 7/16/24 23:51, Krzysztof Kozlowski wrote:
> On 16/07/2024 23:31, Jim Quinlan wrote:
>> o Change order of the compatible strings to be alphabetical
>>
>> o Describe resets/reset-names before using them in rules
>>
> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.

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

> 
> Thank you.
> </form letter>
> 
>> o Add minItems/maxItems where needed.
>>
>> o Change maintainer: Nicolas has not been active for a while.  It also
>>    makes sense for a Broadcom employee to be the maintainer as many of the
>>    details are privy to Broadcom.
>>
>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>> ---
>>   .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
>>   1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>> index 11f8ea33240c..692f7ed7c98e 100644
>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>> @@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>>   title: Brcmstb PCIe Host Controller
>>   
>>   maintainers:
>> -  - Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>> +  - Jim Quinlan <james.quinlan@broadcom.com>
>>   
>>   properties:
>>     compatible:
>> @@ -16,11 +16,11 @@ properties:
>>             - brcm,bcm2711-pcie # The Raspberry Pi 4
>>             - brcm,bcm4908-pcie
>>             - brcm,bcm7211-pcie # Broadcom STB version of RPi4
>> -          - brcm,bcm7278-pcie # Broadcom 7278 Arm
>>             - brcm,bcm7216-pcie # Broadcom 7216 Arm
>> -          - brcm,bcm7445-pcie # Broadcom 7445 Arm
>> +          - brcm,bcm7278-pcie # Broadcom 7278 Arm
>>             - brcm,bcm7425-pcie # Broadcom 7425 MIPs
>>             - brcm,bcm7435-pcie # Broadcom 7435 MIPs
>> +          - brcm,bcm7445-pcie # Broadcom 7445 Arm
>>   
>>     reg:
>>       maxItems: 1
>> @@ -95,6 +95,18 @@ properties:
>>         minItems: 1
>>         maxItems: 3
>>   
>> +  resets:
>> +    minItems: 1
>> +    items:
>> +      - description: reset for external PCIe PERST# signal # perst
>> +      - description: reset for phy reset calibration       # rescal
>> +
>> +  reset-names:
>> +    minItems: 1
>> +    items:
>> +      - const: perst
>> +      - const: rescal
> 
> There are no devices with two resets. Anyway, this does not match one of
> your variants which have first element as rescal.

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

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

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

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

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

  unevaluatedProperties: false

Thanks!
--
Florian


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

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

[Krzysztof Kozlowski]

On 17/07/2024 23:06, Florian Fainelli wrote:
>>> +  resets:
>>> +    minItems: 1
>>> +    items:
>>> +      - description: reset for external PCIe PERST# signal # perst
>>> +      - description: reset for phy reset calibration       # rescal
>>> +
>>> +  reset-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: perst
>>> +      - const: rescal
>>
>> There are no devices with two resets. Anyway, this does not match one of
>> your variants which have first element as rescal.
> 
> Just to be clear, is the diff below what you would expect to see when 
> applying both patch 1 and 4 in succession, that is the reset properties 
> are described "generically" and the conditional section only describes 
> the order and values:

Yeah, this would work. I still propose the format I provided link to
(here in this thread or in my talks).

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 17/07/2024 23:06, Florian Fainelli wrote:
> On 7/16/24 23:51, Krzysztof Kozlowski wrote:
>> On 16/07/2024 23:31, Jim Quinlan wrote:
>>> o Change order of the compatible strings to be alphabetical
>>>
>>> o Describe resets/reset-names before using them in rules
>>>
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
> 
> Maybe the feedback was not clear, the fault cannot always be in the 
> submitter, right?

That why it is nice to ack each reviewers comment. If reviewers comments
are somehow trickier, then such acks or further clarifications are even
more useful.

Did it happen for v3? No.

Best regards,
Krzysztof

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

[Jim Quinlan]

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

On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/07/2024 23:31, Jim Quinlan wrote:
> > o Change order of the compatible strings to be alphabetical
> >
> > o Describe resets/reset-names before using them in rules
> >
>
> <form letter>
> This is a friendly reminder during the review process.
>
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
>
> Thank you.
> </form letter>
>
> > o Add minItems/maxItems where needed.
> >
> > o Change maintainer: Nicolas has not been active for a while.  It also
> >   makes sense for a Broadcom employee to be the maintainer as many of the
> >   details are privy to Broadcom.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 11f8ea33240c..692f7ed7c98e 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
> >  title: Brcmstb PCIe Host Controller
> >
> >  maintainers:
> > -  - Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > +  - Jim Quinlan <james.quinlan@broadcom.com>
> >
> >  properties:
> >    compatible:
> > @@ -16,11 +16,11 @@ properties:
> >            - brcm,bcm2711-pcie # The Raspberry Pi 4
> >            - brcm,bcm4908-pcie
> >            - brcm,bcm7211-pcie # Broadcom STB version of RPi4
> > -          - brcm,bcm7278-pcie # Broadcom 7278 Arm
> >            - brcm,bcm7216-pcie # Broadcom 7216 Arm
> > -          - brcm,bcm7445-pcie # Broadcom 7445 Arm
> > +          - brcm,bcm7278-pcie # Broadcom 7278 Arm
> >            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
> >            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
> > +          - brcm,bcm7445-pcie # Broadcom 7445 Arm
> >
> >    reg:
> >      maxItems: 1
> > @@ -95,6 +95,18 @@ properties:
> >        minItems: 1
> >        maxItems: 3
> >
> > +  resets:
> > +    minItems: 1
> > +    items:
> > +      - description: reset for external PCIe PERST# signal # perst
> > +      - description: reset for phy reset calibration       # rescal
> > +
> > +  reset-names:
> > +    minItems: 1
> > +    items:
> > +      - const: perst
> > +      - const: rescal
>
> There are no devices with two resets. Anyway, this does not match one of
> your variants which have first element as rescal.


Hello Krzysztof,

At this commit there are two resets; the 4908 requires "perst" and the
7216 requires "rescal".   I now think what you are looking for is the
top-level
description of something like

resets:
  maxItems: 1
    oneOf:
      - description: reset controller handling the PERST# signal
      - description: phandle pointing to the RESCAL reset controller

reset-names:
  maxItems: 1
    oneOf:
      - const: perst
      - const: rescal

I left out minItems because imItems==maxItems=1

Before I was giving both of them as the "potential candidates list"
that will be used further on, but this is not how Yaml should be used.

Is the above in the right direction?

Regards,
Jim Quinlan
Broadcom STB/CM

>
> Best regards,
> Krzysztof
>

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

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

[Jim Quinlan]

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

On Wed, Jul 17, 2024 at 9:30 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 17/07/2024 15:20, Jim Quinlan wrote:
> > On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 16/07/2024 23:31, Jim Quinlan wrote:
> >>> o Change order of the compatible strings to be alphabetical
> >>>
> >>> o Describe resets/reset-names before using them in rules
> >>>
> >>
> >> <form letter>
> >> This is a friendly reminder during the review process.
> >>
> >> It seems my or other reviewer's previous comments were not fully
> >> addressed. Maybe the feedback got lost between the quotes, maybe you
> >> just forgot to apply it. Please go back to the previous discussion and
> >> either implement all requested changes or keep discussing them.
> >>
> >> Thank you.
> >> </form letter>
> >
> > I'm sorry Krzysztof, but AFAICT I paid attention to all of your comments and
> > tried to answer them as best as I could.  Please do not resort to a
> > form letter; if
> > you think I missed something(s) please oblige me and say what it is rather than
> > having  me search for something that you know and I do not.
>
> I do not see your response at all to my comments on patch #2.
>
> >
> >>
> >>> o Add minItems/maxItems where needed.
> >>>
> >>> o Change maintainer: Nicolas has not been active for a while.  It also
> >>>   makes sense for a Broadcom employee to be the maintainer as many of the
> >>>   details are privy to Broadcom.
> >>>
> >>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> >>> ---
> >>>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
> >>>  1 file changed, 19 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>> index 11f8ea33240c..692f7ed7c98e 100644
> >>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>> @@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>  title: Brcmstb PCIe Host Controller
> >>>
> >>>  maintainers:
> >>> -  - Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> >>> +  - Jim Quinlan <james.quinlan@broadcom.com>
> >>>
> >>>  properties:
> >>>    compatible:
> >>> @@ -16,11 +16,11 @@ properties:
> >>>            - brcm,bcm2711-pcie # The Raspberry Pi 4
> >>>            - brcm,bcm4908-pcie
> >>>            - brcm,bcm7211-pcie # Broadcom STB version of RPi4
> >>> -          - brcm,bcm7278-pcie # Broadcom 7278 Arm
> >>>            - brcm,bcm7216-pcie # Broadcom 7216 Arm
> >>> -          - brcm,bcm7445-pcie # Broadcom 7445 Arm
> >>> +          - brcm,bcm7278-pcie # Broadcom 7278 Arm
> >>>            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
> >>>            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
> >>> +          - brcm,bcm7445-pcie # Broadcom 7445 Arm
> >>>
> >>>    reg:
> >>>      maxItems: 1
> >>> @@ -95,6 +95,18 @@ properties:
> >>>        minItems: 1
> >>>        maxItems: 3
> >>>
> >>> +  resets:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - description: reset for external PCIe PERST# signal # perst
> >>> +      - description: reset for phy reset calibration       # rescal
> >>> +
> >>> +  reset-names:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - const: perst
> >>> +      - const: rescal
> >>
> >> There are no devices with two resets. Anyway, this does not match one of
> >> your variants which have first element as rescal.
> >
> > The 4908 chip exclusively uses the "perst" reset, and the  7216 chip
> > exclusively uses the "rescal" reset.   The rest of the chips use zero
> > resets.   All together, there are two resets.
>
> This is not enum, but a list. What you do mean overall two resets? You
> have a chip which is both 4908 and 7216 at the same time? How is this
> possible?
>
> >
> > You are the one that wanted me to first list all resets at the top,
> > and refer to them by the conditional rules.
>
> No, I wanted widest constraints at the top.
Can you explain what you mean by "widest constraint"?  I did not see
the word "wide" in any of the Linux YAML documentation.

>
> My comment at v2 was already saying this:
>
> "This does not match what you have in conditional, so just keep min and
> max Items here."
>
Okay, what I was referring to was your V1 response:

"Fix the binding first - properties should be defined in top level
"properties:" and then customized."

I think this is correct but I have not been describing the
reset/reset-names properties properly at the top level; i.e.  I have
been giving a full list instead of using "oneOf".

Regards,
Jim Quinlan
Broadcom STB/CM

> and you have numerous examples in the code for this.
>
> Best regards,
> Krzysztof
>

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

Re: [PATCH v4 02/12] dt-bindings: PCI: brcmstb: Add 7712 SoC description

[Jim Quinlan]

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

On Wed, Jul 17, 2024 at 2:53 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/07/2024 23:31, Jim Quinlan wrote:
> > This adds the description for the 7712 SoC, a Broadcom
> > STB sibling chip of the RPi 5.  Two new reset controllers
> > are described.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  .../bindings/pci/brcm,stb-pcie.yaml           | 26 +++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 692f7ed7c98e..90683a0df2c5 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -21,6 +21,7 @@ properties:
> >            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
> >            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
> >            - brcm,bcm7445-pcie # Broadcom 7445 Arm
> > +          - brcm,bcm7712-pcie # Broadcom STB sibling of Rpi 5
> >
> >    reg:
> >      maxItems: 1
> > @@ -100,12 +101,16 @@ properties:
> >      items:
> >        - description: reset for external PCIe PERST# signal # perst
> >        - description: reset for phy reset calibration       # rescal
> > +      - description: reset for PCIe/CPU bus bridge         # bridge
> > +      - description: reset for soft PCIe core reset        # swinit
> >
> >    reset-names:
> >      minItems: 1
> >      items:
> >        - const: perst
> >        - const: rescal
> > +      - const: bridge
> > +      - const: swinit
>
> This does not match at all what you have in allOf:if:then section.
>
> >
> >  required:
> >    - compatible
> > @@ -159,6 +164,27 @@ allOf:
> >          - resets
> >          - reset-names
> >
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: brcm,bcm7712-pcie
> > +    then:
> > +      properties:
> > +        resets:
> > +          minItems: 3
> > +          maxItems: 3
> > +
> > +        reset-names:
> > +          items:
> > +            - const: rescal
>
> Look - here it is rescal. Before you said it must be perst.
>
> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132

Hello Krzysztof,

The difference between my commits and the above example is that the
above example has no "desc" line(s) to describe the clocks  -- How
would you add this?  Or are you okay with (a) no description or (b)
using a "#comment..." next to the clock's name?

Regards,
Jim Quinlan
Broadcom STB/CM
>
> Best regards,
> Krzysztof
>

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

Re: [PATCH v4 02/12] dt-bindings: PCI: brcmstb: Add 7712 SoC description

[Krzysztof Kozlowski]

On 23/07/2024 23:03, Jim Quinlan wrote:
> On Wed, Jul 17, 2024 at 2:53 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 16/07/2024 23:31, Jim Quinlan wrote:
>>> This adds the description for the 7712 SoC, a Broadcom
>>> STB sibling chip of the RPi 5.  Two new reset controllers
>>> are described.
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 +++++++++++++++++++
>>>  1 file changed, 26 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 692f7ed7c98e..90683a0df2c5 100644
>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> @@ -21,6 +21,7 @@ properties:
>>>            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
>>>            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
>>>            - brcm,bcm7445-pcie # Broadcom 7445 Arm
>>> +          - brcm,bcm7712-pcie # Broadcom STB sibling of Rpi 5
>>>
>>>    reg:
>>>      maxItems: 1
>>> @@ -100,12 +101,16 @@ properties:
>>>      items:
>>>        - description: reset for external PCIe PERST# signal # perst
>>>        - description: reset for phy reset calibration       # rescal
>>> +      - description: reset for PCIe/CPU bus bridge         # bridge
>>> +      - description: reset for soft PCIe core reset        # swinit
>>>
>>>    reset-names:
>>>      minItems: 1
>>>      items:
>>>        - const: perst
>>>        - const: rescal
>>> +      - const: bridge
>>> +      - const: swinit
>>
>> This does not match at all what you have in allOf:if:then section.
>>
>>>
>>>  required:
>>>    - compatible
>>> @@ -159,6 +164,27 @@ allOf:
>>>          - resets
>>>          - reset-names
>>>
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: brcm,bcm7712-pcie
>>> +    then:
>>> +      properties:
>>> +        resets:
>>> +          minItems: 3
>>> +          maxItems: 3
>>> +
>>> +        reset-names:
>>> +          items:
>>> +            - const: rescal
>>
>> Look - here it is rescal. Before you said it must be perst.
>>
>> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132
> 
> Hello Krzysztof,
> 
> The difference between my commits and the above example is that the
> above example has no "desc" line(s) to describe the clocks  -- How

Which does not really matter to illustrate the concept where you define
the widest constraints and where you narrow them.

> would you add this?  Or are you okay with (a) no description or (b)
> using a "#comment..." next to the clock's name?



Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 23/07/2024 20:44, Jim Quinlan wrote:
> On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 16/07/2024 23:31, Jim Quinlan wrote:
>>> o Change order of the compatible strings to be alphabetical
>>>
>>> o Describe resets/reset-names before using them in rules
>>>
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>> </form letter>
>>
>>> o Add minItems/maxItems where needed.
>>>
>>> o Change maintainer: Nicolas has not been active for a while.  It also
>>>   makes sense for a Broadcom employee to be the maintainer as many of the
>>>   details are privy to Broadcom.
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
>>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 11f8ea33240c..692f7ed7c98e 100644
>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> @@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>>>  title: Brcmstb PCIe Host Controller
>>>
>>>  maintainers:
>>> -  - Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>> +  - Jim Quinlan <james.quinlan@broadcom.com>
>>>
>>>  properties:
>>>    compatible:
>>> @@ -16,11 +16,11 @@ properties:
>>>            - brcm,bcm2711-pcie # The Raspberry Pi 4
>>>            - brcm,bcm4908-pcie
>>>            - brcm,bcm7211-pcie # Broadcom STB version of RPi4
>>> -          - brcm,bcm7278-pcie # Broadcom 7278 Arm
>>>            - brcm,bcm7216-pcie # Broadcom 7216 Arm
>>> -          - brcm,bcm7445-pcie # Broadcom 7445 Arm
>>> +          - brcm,bcm7278-pcie # Broadcom 7278 Arm
>>>            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
>>>            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
>>> +          - brcm,bcm7445-pcie # Broadcom 7445 Arm
>>>
>>>    reg:
>>>      maxItems: 1
>>> @@ -95,6 +95,18 @@ properties:
>>>        minItems: 1
>>>        maxItems: 3
>>>
>>> +  resets:
>>> +    minItems: 1
>>> +    items:
>>> +      - description: reset for external PCIe PERST# signal # perst
>>> +      - description: reset for phy reset calibration       # rescal
>>> +
>>> +  reset-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: perst
>>> +      - const: rescal
>>
>> There are no devices with two resets. Anyway, this does not match one of
>> your variants which have first element as rescal.
> 
> 
> Hello Krzysztof,
> 
> At this commit there are two resets; the 4908 requires "perst" and the
> 7216 requires "rescal".   I now think what you are looking for is the
> top-level
> description of something like
> 
> resets:
>   maxItems: 1
>     oneOf:
>       - description: reset controller handling the PERST# signal
>       - description: phandle pointing to the RESCAL reset controller

Now tell me, what sort of new information comes with this description?
"Phandle"? It cannot be anything else. Redundant. "Pointing to"?
Redundant. "reset-controller"? Well, resets always point to reset
controller.

So what is the point of this description? Any point?

> 
> reset-names:
>   maxItems: 1
>     oneOf:
>       - const: perst
>       - const: rescal
> 
> I left out minItems because imItems==maxItems=1
> 
> Before I was giving both of them as the "potential candidates list"
> that will be used further on, but this is not how Yaml should be used.
> 
> Is the above in the right direction?

It's over complicated. First maxItems are redundant, because you define
number of items in items. Second, you have EXACTLY the same case as the
hardware for which I gave you bindings to use. I don't understand why
you insist on doing things differently, but you can. Take a look at many
other bindings how they achieve this - there are many, many examples.
But do not invent third or fourth method...

Best regards,
Krzysztof

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

[Jim Quinlan]

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

On Wed, Jul 24, 2024 at 4:05 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 23/07/2024 20:44, Jim Quinlan wrote:
> > On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 16/07/2024 23:31, Jim Quinlan wrote:
> >>> o Change order of the compatible strings to be alphabetical
> >>>
> >>> o Describe resets/reset-names before using them in rules
> >>>
> >>
> >> <form letter>
> >> This is a friendly reminder during the review process.
> >>
> >> It seems my or other reviewer's previous comments were not fully
> >> addressed. Maybe the feedback got lost between the quotes, maybe you
> >> just forgot to apply it. Please go back to the previous discussion and
> >> either implement all requested changes or keep discussing them.
> >>
> >> Thank you.
> >> </form letter>
> >>
> >>> o Add minItems/maxItems where needed.
> >>>
> >>> o Change maintainer: Nicolas has not been active for a while.  It also
> >>>   makes sense for a Broadcom employee to be the maintainer as many of the
> >>>   details are privy to Broadcom.
> >>>
> >>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> >>> ---
> >>>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
> >>>  1 file changed, 19 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>> index 11f8ea33240c..692f7ed7c98e 100644
> >>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>> @@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>  title: Brcmstb PCIe Host Controller
> >>>
> >>>  maintainers:
> >>> -  - Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> >>> +  - Jim Quinlan <james.quinlan@broadcom.com>
> >>>
> >>>  properties:
> >>>    compatible:
> >>> @@ -16,11 +16,11 @@ properties:
> >>>            - brcm,bcm2711-pcie # The Raspberry Pi 4
> >>>            - brcm,bcm4908-pcie
> >>>            - brcm,bcm7211-pcie # Broadcom STB version of RPi4
> >>> -          - brcm,bcm7278-pcie # Broadcom 7278 Arm
> >>>            - brcm,bcm7216-pcie # Broadcom 7216 Arm
> >>> -          - brcm,bcm7445-pcie # Broadcom 7445 Arm
> >>> +          - brcm,bcm7278-pcie # Broadcom 7278 Arm
> >>>            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
> >>>            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
> >>> +          - brcm,bcm7445-pcie # Broadcom 7445 Arm
> >>>
> >>>    reg:
> >>>      maxItems: 1
> >>> @@ -95,6 +95,18 @@ properties:
> >>>        minItems: 1
> >>>        maxItems: 3
> >>>
> >>> +  resets:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - description: reset for external PCIe PERST# signal # perst
> >>> +      - description: reset for phy reset calibration       # rescal
> >>> +
> >>> +  reset-names:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - const: perst
> >>> +      - const: rescal
> >>
> >> There are no devices with two resets. Anyway, this does not match one of
> >> your variants which have first element as rescal.
> >
> >
> > Hello Krzysztof,
> >
> > At this commit there are two resets; the 4908 requires "perst" and the
> > 7216 requires "rescal".   I now think what you are looking for is the
> > top-level
> > description of something like
> >
> > resets:
> >   maxItems: 1
> >     oneOf:
> >       - description: reset controller handling the PERST# signal
> >       - description: phandle pointing to the RESCAL reset controller
>
> Now tell me, what sort of new information comes with this description?
> "Phandle"? It cannot be anything else. Redundant. "Pointing to"?
> Redundant. "reset-controller"? Well, resets always point to reset
> controller.
>
> So what is the point of this description? Any point?
>
> >
> > reset-names:
> >   maxItems: 1
> >     oneOf:
> >       - const: perst
> >       - const: rescal
> >
> > I left out minItems because imItems==maxItems=1
> >
> > Before I was giving both of them as the "potential candidates list"
> > that will be used further on, but this is not how Yaml should be used.
> >
> > Is the above in the right direction?
>
> It's over complicated. First maxItems are redundant, because you define
> number of items in items. Second, you have EXACTLY the same case as the
> hardware for which I gave you bindings to use. I don't understand why
> you insist on doing things differently, but you can. Take a look at many
> other bindings how they achieve this - there are many, many examples.
> But do not invent third or fourth method...

ack, I will follow the qcom,ufs.yaml file you referenced.

>
> Best regards,
> Krzysztof
>

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

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

[Manivannan Sadhasivam]

On Tue, Jul 16, 2024 at 05:31:15PM -0400, Jim Quinlan wrote:
> V4 Changes:
>   o Commit "Check return value of all reset_control_xxx calls"
>     -- Blank line before "return" (Stan)
>   o Commit "Use common error handling code in brcmstb_probe()"
>     -- Drop the "Fixes" tag (Stan)
>   o Commit "dt-bindings: PCI ..."
>     -- Separate the main commit into two: cleanup and adding the
>        7712 SoC (Krzysztof)
>     -- Fold maintainer change commit into cleanup change (Krzysztof)
>     -- Use minItems/maxItems where appropriate (Krzysztof)
>     -- Consistent order of resets/reset-names in decl and usage
>        (Krzysztof)
> 

Rpi mailing list owners: Could you please make the list 'unmoderated'? I haven't
subscribe to this list, so I just keep getting the 'Your message to
linux-rpi-kernel awaits moderator approval' for every review comment I post,
which is a spam to me.

Also I won't subscribe to this list unless I work on Rpi. So using moderated
list in LKML is just spamming the reviewers.

- Mani

> V3 Changes:
>   o Commit "Enable 7712 SOCs"
>     -- Move "model" check from outside to inside func (Stan)
>   o Commit "Check return value of all reset_control_xxx calls"
>     -- Propagate errors up the chain instead of ignoring them (Stan)
>   o Commit "Refactor for chips with many regular inbound BARs"
>     -- Nine suggestions given, nine implemented (Stan)
>   o Commit "Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific"
>     -- Drop tab, add parens around macro params in expression (Stan)
>   o Commit "Use swinit reset if available"
>     -- Treat swinit the same as other reset controllers (Stan)
>        Stan suggested to use dev_err_probe() for getting resources
>        but I will defer that to future series (if that's okay).
>   o Commit "Get resource before we start asserting resets"
>     -- Squash this with previous commit (Stan)
>   o Commit "Use "clk_out" error path label"
>     -- Move clk_prepare_enable() after getting resouurces (Stan)
>     -- Change subject to "Use more common error handling code in
>        brcm_pcie_probe()" (Markus)
>     -- Use imperative commit description (Markus)
>     -- "Fixes:" tag added for missing error return. (Markus)
>   o Commit "dt-bindings: PCI ..."
>     -- Split off maintainer change in separate commit.
>     -- Tried to accomodate Krzysztof's requests, I'm not sure I
>        have succeeded.  Krzysztof, please see [1] below.
>   
>   [1] Wrt the YAML of brcmstb PCIe resets, here is what I am trying
>       to describe:
> 
>       CHIP       NUM_RESETS    NAMES
>       ====       ==========    =====
>       4908       1             perst
>       7216       1             rescal
>       7712       3             rescal, bridge, swinit
>       Others     0             -
> 
> 
> 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: Cleanup of brcmstb YAML and add 7712 SoC
>   dt-bindings: PCI: brcmstb: Add 7712 SoC description
>   PCI: brcmstb: Use common error handling code in brcm_pcie_probe()
>   PCI: brcmstb: Use bridge reset if available
>   PCI: brcmstb: Use swinit reset if available
>   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: Change field name from 'type' to 'model'
>   PCI: brcmstb: Enable 7712 SOCs
> 
>  .../bindings/pci/brcm,stb-pcie.yaml           |  50 +-
>  drivers/pci/controller/pcie-brcmstb.c         | 485 +++++++++++++-----
>  2 files changed, 400 insertions(+), 135 deletions(-)
> 
> 
> base-commit: 55027e689933ba2e64f3d245fb1ff185b3e7fc81
> -- 
> 2.17.1
> 



-- 
மணிவண்ணன் சதாசிவம்