Message-ID: <CA+-6iNy5U6XG-sSjoBH4pAhFhP4=KDcKRVbD3dXZdsM2WCkd-w@mail.gmail.com>
Date: Tue, 23 Jul 2024 14:49:43 -0400
From: Jim Quinlan <james.quinlan@...adcom.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: linux-pci@...r.kernel.org, Nicolas Saenz Julienne <nsaenz@...nel.org>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Lorenzo Pieralisi <lorenzo.pieralisi@....com>, 
	Cyril Brulebois <kibi@...ian.org>, Stanimir Varbanov <svarbanov@...e.de>, 
	bcm-kernel-feedback-list@...adcom.com, jim2101024@...il.com, 
	Florian Fainelli <florian.fainelli@...adcom.com>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Krzysztof Wilczyński <kw@...ux.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@...ts.infradead.org>, 
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-arm-kernel@...ts.infradead.org>, 
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and
 add 7712 SoC

On Wed, Jul 17, 2024 at 9:30 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 17/07/2024 15:20, Jim Quinlan wrote:
> > On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@...nel.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@...adcom.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@...e.de>
> >>> +  - Jim Quinlan <james.quinlan@...adcom.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
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4210 bytes)

Powered by blists - more mailing lists