* Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC
2024-07-17 6:51 ` Krzysztof Kozlowski
@ 2024-07-17 13:20 ` Jim Quinlan
2024-07-17 13:30 ` Krzysztof Kozlowski
2024-07-17 21:06 ` Florian Fainelli
2024-07-23 18:44 ` Jim Quinlan
2 siblings, 1 reply; 17+ messages in thread
From: Jim Quinlan @ 2024-07-17 13:20 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: linux-pci, Nicolas Saenz Julienne, Bjorn Helgaas,
Lorenzo Pieralisi, Cyril Brulebois, Stanimir Varbanov,
bcm-kernel-feedback-list, jim2101024, Florian Fainelli,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC
2024-07-17 13:20 ` Jim Quinlan
@ 2024-07-17 13:30 ` Krzysztof Kozlowski
2024-07-23 18:49 ` Jim Quinlan
0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2024-07-17 13:30 UTC (permalink / raw)
To: Jim Quinlan
Cc: linux-pci, Nicolas Saenz Julienne, Bjorn Helgaas,
Lorenzo Pieralisi, Cyril Brulebois, Stanimir Varbanov,
bcm-kernel-feedback-list, jim2101024, Florian Fainelli,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC
2024-07-17 13:30 ` Krzysztof Kozlowski
@ 2024-07-23 18:49 ` Jim Quinlan
0 siblings, 0 replies; 17+ messages in thread
From: Jim Quinlan @ 2024-07-23 18:49 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: linux-pci, Nicolas Saenz Julienne, Bjorn Helgaas,
Lorenzo Pieralisi, Cyril Brulebois, Stanimir Varbanov,
bcm-kernel-feedback-list, jim2101024, Florian Fainelli,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC
2024-07-17 6:51 ` Krzysztof Kozlowski
2024-07-17 13:20 ` Jim Quinlan
@ 2024-07-17 21:06 ` Florian Fainelli
2024-07-18 6:02 ` Krzysztof Kozlowski
2024-07-18 6:07 ` Krzysztof Kozlowski
2024-07-23 18:44 ` Jim Quinlan
2 siblings, 2 replies; 17+ messages in thread
From: Florian Fainelli @ 2024-07-17 21:06 UTC (permalink / raw)
To: Krzysztof Kozlowski, Jim Quinlan, linux-pci,
Nicolas Saenz Julienne, Bjorn Helgaas, Lorenzo Pieralisi,
Cyril Brulebois, Stanimir Varbanov, bcm-kernel-feedback-list,
jim2101024
Cc: Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
[-- 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 --]
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC
2024-07-17 21:06 ` Florian Fainelli
@ 2024-07-18 6:02 ` Krzysztof Kozlowski
2024-07-18 6:07 ` Krzysztof Kozlowski
1 sibling, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2024-07-18 6:02 UTC (permalink / raw)
To: Florian Fainelli, Jim Quinlan, linux-pci, Nicolas Saenz Julienne,
Bjorn Helgaas, Lorenzo Pieralisi, Cyril Brulebois,
Stanimir Varbanov, bcm-kernel-feedback-list, jim2101024
Cc: Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC
2024-07-17 21:06 ` Florian Fainelli
2024-07-18 6:02 ` Krzysztof Kozlowski
@ 2024-07-18 6:07 ` Krzysztof Kozlowski
1 sibling, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2024-07-18 6:07 UTC (permalink / raw)
To: Florian Fainelli, Jim Quinlan, linux-pci, Nicolas Saenz Julienne,
Bjorn Helgaas, Lorenzo Pieralisi, Cyril Brulebois,
Stanimir Varbanov, bcm-kernel-feedback-list, jim2101024
Cc: Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC
2024-07-17 6:51 ` Krzysztof Kozlowski
2024-07-17 13:20 ` Jim Quinlan
2024-07-17 21:06 ` Florian Fainelli
@ 2024-07-23 18:44 ` Jim Quinlan
2024-07-24 8:05 ` Krzysztof Kozlowski
2 siblings, 1 reply; 17+ messages in thread
From: Jim Quinlan @ 2024-07-23 18:44 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: linux-pci, Nicolas Saenz Julienne, Bjorn Helgaas,
Lorenzo Pieralisi, Cyril Brulebois, Stanimir Varbanov,
bcm-kernel-feedback-list, jim2101024, Florian Fainelli,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC
2024-07-23 18:44 ` Jim Quinlan
@ 2024-07-24 8:05 ` Krzysztof Kozlowski
2024-07-24 18:57 ` Jim Quinlan
0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2024-07-24 8:05 UTC (permalink / raw)
To: Jim Quinlan
Cc: linux-pci, Nicolas Saenz Julienne, Bjorn Helgaas,
Lorenzo Pieralisi, Cyril Brulebois, Stanimir Varbanov,
bcm-kernel-feedback-list, jim2101024, Florian Fainelli,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC
2024-07-24 8:05 ` Krzysztof Kozlowski
@ 2024-07-24 18:57 ` Jim Quinlan
0 siblings, 0 replies; 17+ messages in thread
From: Jim Quinlan @ 2024-07-24 18:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: linux-pci, Nicolas Saenz Julienne, Bjorn Helgaas,
Lorenzo Pieralisi, Cyril Brulebois, Stanimir Varbanov,
bcm-kernel-feedback-list, jim2101024, Florian Fainelli,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread