On 12/07/2024 21:54, Jim Quinlan wrote: > On Thu, Jul 4, 2024 at 2:40 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: >> >> On 03/07/2024 20:02, Jim Quinlan wrote: >>> - Update maintainer; Nicolas hasn't been active and it >>> makes more sense to have a Broadcom maintainer >>> - Add a driver compatible string for the new STB SOC 7712 >> >> You meant device? Bindings are for hardware. > > Hello Krzysztof, > > I should have replied to this before sending out V3. Since your form > letter says I did not address previous comments, I will address them > here and now (your v2 review of the bindings commit). > >> >>> - Add two new resets for the 7712: "bridge", for the >>> the bridge between the PCIe core and the memory bus; >>> "swinit", the PCIe core reset. >>> - Order the compatible strings alphabetically >>> - Restructure the reset controllers so that the definitions >>> appear first before any rules that govern them. >> >> Please split cleanups from new device support. > Okay. >> >>> >>> Signed-off-by: Jim Quinlan <james.quinlan@xxxxxxxxxxxx> >>> --- >>> .../bindings/pci/brcm,stb-pcie.yaml | 44 +++++++++++++++---- >>> 1 file changed, 36 insertions(+), 8 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml >>> index 11f8ea33240c..a070f35d28d7 100644 >>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml >>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml >>> @@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml# >>> title: Brcmstb PCIe Host Controller >>> >>> maintainers: >>> - - Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx> >>> + - Jim Quinlan <james.quinlan@xxxxxxxxxxxx> >>> >>> properties: >>> compatible: >>> @@ -16,11 +16,12 @@ properties: >>> - brcm,bcm2711-pcie # The Raspberry Pi 4 >>> - brcm,bcm4908-pcie >>> - brcm,bcm7211-pcie # Broadcom STB version of RPi4 >>> - - brcm,bcm7278-pcie # Broadcom 7278 Arm >>> - brcm,bcm7216-pcie # Broadcom 7216 Arm >>> - - brcm,bcm7445-pcie # Broadcom 7445 Arm >>> + - brcm,bcm7278-pcie # Broadcom 7278 Arm >>> - brcm,bcm7425-pcie # Broadcom 7425 MIPs >>> - brcm,bcm7435-pcie # Broadcom 7435 MIPs >>> + - brcm,bcm7445-pcie # Broadcom 7445 Arm >>> + - brcm,bcm7712-pcie # STB sibling SOC of Raspberry Pi 5 >>> >>> reg: >>> maxItems: 1 >>> @@ -95,6 +96,20 @@ properties: >>> minItems: 1 >>> maxItems: 3 >>> >>> + resets: >>> + items: >>> + - description: reset for phy calibration >>> + - description: reset for PCIe/CPU bus bridge >>> + - description: reset for soft PCIe core reset >>> + - description: reset for PERST# PCIe signal >> >> This won't work and I doubt you tested your code. You miss minItems. >> >>> + >>> + reset-names: >>> + items: >>> + - const: rescal >>> + - const: bridge >>> + - const: swinit >>> + - const: perst >> >> This does not match what you have in conditional, so just keep min and >> max Items here. > > I do not understand. There are four possible resets, but any one chip > uses only 0, 1, or 3 of them: > > CHIP NUM_RESETS NAMES > ==== ========== ===== > 4908 1 perst > 7216 1 rescal > 7712 3 rescal, bridge, swinit > Other_Chips 0 - > > Although I list four "reset-names", I have, in the rule for 7712, > maxItems=3 because it only uses rescal, bridge, and swinit. So I > don't know what you mean when you say "this does not match what you > have in your conditional". AFAICT, they are not supposed to match. One place says they have order A+B+C, other place says they have order C+B+A or whatever other combination. Look at first element: A ! = C. So they do not match. > > >> >> >>> + >>> required: >>> - compatible >>> - reg >>> @@ -118,13 +133,10 @@ allOf: >>> then: >>> properties: >>> resets: >>> - items: >>> - - description: reset controller handling the PERST# signal >>> - >>> + minItems: 1 >> >> maxItems instead. Why three resets should be valid? > For the "4908" conditional, minItems==maxItems==1. I do not > understand your question "Why three resets should be valid" -- can you > please elaborate? Where do you have maxItems? I see only minItems. > >> >> >>> reset-names: >>> items: >>> - const: perst >>> - >>> required: >>> - resets >>> - reset-names >>> @@ -136,12 +148,28 @@ allOf: >>> then: >>> properties: >>> resets: >>> + minItems: 1 >>> + reset-names: >>> items: >>> - - description: phandle pointing to the RESCAL reset controller >>> + - const: rescal >>> + required: >>> + - resets >>> + - reset-names >> >> Why? > > I do not know what you are questioning. The 7216 device uses one > reset: the "rescal". Again, maxItems==minItems==1. Please see the > summary note below. You are breaking the ABI, so I am questioning. I don't see ABI break explained in the commit msg. > >> >>> + - if: >>> + properties: >>> + compatible: >>> + contains: >>> + const: brcm,bcm7712-pcie >>> + then: >>> + properties: >>> + resets: >>> + minItems: 3 >> >> Again, you do not have 4 items here. > > I do not want to have 4 items here; I want to have 3 for "rescal", > "bridge," and "swinit". In this case, maxItems==minItems==3. Your schema does not define that. > > Now , for V1 you requested that I define all resets at the top; I've > done that and there are 4 of them. But no chip uses all 4; each > individual chip only uses 0, 1, or 3 resets. I assumed they follow same order. If you have different order, the top defines only widest constraints. > > So there is no way that each chip's conditional rule can define > minItems and maxItems to match the description list of 4 resets, > unless you want me to undo your V1 request of describing the resets at > the top level instead of describing them in the rules. > Best regards, Krzysztof
