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

[Jim Quinlan <james.quinlan@broadcom.com>]

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