| Message ID | 20220811203306.179744-3-mail@conchuod.ie (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | Fix RISC-V/PCI dt-schema issues with dt-schema v2022.08 | expand |
On 11/08/2022 23:33, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > Upgrading dt-schema to v2022.08 reveals unevaluatedProperties issues > that were not previously visible, such as the missing clocks and > clock-names properties for PolarFire SoC's PCI controller: > arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dtb: pcie@2000000000: Unevaluated properties are not allowed ('clock-names', 'clocks', 'legacy-interrupt-controller', 'microchip,axi-m-atr0' were unexpected) > From schema: Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml > > The clocks are required to enable interfaces between the FPGA fabric > and the core complex, so add them to the binding. > > Fixes: 6ee6c89aac35 ("dt-bindings: PCI: microchip: Add Microchip PolarFire host binding") > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > .../bindings/pci/microchip,pcie-host.yaml | 25 +++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml > index edb4f81253c8..2a2166f09e2c 100644 > --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml > +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml > @@ -25,6 +25,31 @@ properties: > - const: cfg > - const: apb > > + clocks: > + description: > + Fabric Interface Controllers, FICs, are the interface between the FPGA > + fabric and the core complex on PolarFire SoC. The FICs require two clocks, > + one from each side of the interface. The "FIC clocks" described by this > + property are on the core complex side & communication through a FIC is not > + possible unless it's corresponding clock is enabled. A clock must be > + enabled for each of the interfaces the root port is connected through. > + minItems: 1 > + items: > + - description: FIC0's clock > + - description: FIC1's clock > + - description: FIC2's clock > + - description: FIC3's clock > + > + clock-names: > + items: > + enum: > + - fic0 > + - fic1 > + - fic2 > + - fic3 > + minItems: 1 > + maxItems: 4 No need for maxItems. Best regards, Krzysztof
On 12/08/2022 10:35, Krzysztof Kozlowski wrote: > On 11/08/2022 23:33, Conor Dooley wrote: >> From: Conor Dooley <conor.dooley@microchip.com> >> >> Upgrading dt-schema to v2022.08 reveals unevaluatedProperties issues >> that were not previously visible, such as the missing clocks and >> clock-names properties for PolarFire SoC's PCI controller: I don't think this part of sentence is worth staying in Git. The schema is released so obviously everyone should upgrade. In two years will it matter which version brought unevaluatedProperties to a enforced state? Best regards, Krzysztof
On 12/08/2022 09:00, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 12/08/2022 10:35, Krzysztof Kozlowski wrote: >> On 11/08/2022 23:33, Conor Dooley wrote: >>> From: Conor Dooley <conor.dooley@microchip.com> >>> >>> Upgrading dt-schema to v2022.08 reveals unevaluatedProperties issues >>> that were not previously visible, such as the missing clocks and >>> clock-names properties for PolarFire SoC's PCI controller: > > I don't think this part of sentence is worth staying in Git. The schema > is released so obviously everyone should upgrade. In two years will it > matter which version brought unevaluatedProperties to a enforced state? I have no strong feelings :) I'll put it under the --- in the next version as I think it has value for people reading the patches since it's fairly likely they won't see the errors.
On 12/08/2022 08:35, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 11/08/2022 23:33, Conor Dooley wrote: >> From: Conor Dooley <conor.dooley@microchip.com> >> >> Upgrading dt-schema to v2022.08 reveals unevaluatedProperties issues >> that were not previously visible, such as the missing clocks and >> clock-names properties for PolarFire SoC's PCI controller: >> arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dtb: pcie@2000000000: Unevaluated properties are not allowed ('clock-names', 'clocks', 'legacy-interrupt-controller', 'microchip,axi-m-atr0' were unexpected) >> From schema: Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml >> >> The clocks are required to enable interfaces between the FPGA fabric >> and the core complex, so add them to the binding. >> >> Fixes: 6ee6c89aac35 ("dt-bindings: PCI: microchip: Add Microchip PolarFire host binding") >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >> --- >> .../bindings/pci/microchip,pcie-host.yaml | 25 +++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml >> index edb4f81253c8..2a2166f09e2c 100644 >> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml >> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml >> @@ -25,6 +25,31 @@ properties: >> - const: cfg >> - const: apb >> >> + clocks: >> + description: >> + Fabric Interface Controllers, FICs, are the interface between the FPGA >> + fabric and the core complex on PolarFire SoC. The FICs require two clocks, >> + one from each side of the interface. The "FIC clocks" described by this >> + property are on the core complex side & communication through a FIC is not >> + possible unless it's corresponding clock is enabled. A clock must be >> + enabled for each of the interfaces the root port is connected through. >> + minItems: 1 >> + items: >> + - description: FIC0's clock >> + - description: FIC1's clock >> + - description: FIC2's clock >> + - description: FIC3's clock >> + >> + clock-names: >> + items: >> + enum: >> + - fic0 >> + - fic1 >> + - fic2 >> + - fic3 >> + minItems: 1 >> + maxItems: 4 > > No need for maxItems. I brought this up on IRC, but transferring it here since it's been an (understandable!!) few days & just didn't want things to get lost if my net died. Cutting out the back & forth, in summary: " I'm trying to remove the maxItems from the clock-names array you didn't like - but I can't figure out what to do instead that doesn't trigger errors. All 4 clocks are optional, the only requirement is that any one of them is present. Either I seem to get complaints that my property is not an array (simply removing the maxItems) or complaints that because I have clock0,1,3 and not 2 that clock3 is unexpected. The root port is physically on the opposite side of the FPGA to the cpus & the AXI connection is through the FPGA fabric. There are 4 AXI interconnects to the fabric which the PCI controller could in theory be connected to all 4, but it only needs to be connected to one.. I had done done minItems and maxItems a la: devicetree/bindings/watchdog/st,stm32-iwdg.yaml b/c that seems to have two clocks that it doesnt care about the order of " Rob then suggested: " I would remove the 'items' list in 'clocks' and make the description clear that any of clocks is possible. It's not ideal, but it's a case of that's what is already there. " I'd then have something along the lines of: clocks: description: Fabric Interface Controllers, FICs, are the interface between the FPGA fabric and the core complex on PolarFire SoC. The FICs require two clocks, one from each side of the interface. The "FIC clocks" described by this property are on the core complex side & communication through a FIC is not possible unless it's corresponding clock is enabled. A clock must be enabled for each of the interfaces the root port is connected through. This could in theory be all 4 interfaces, one interface or any combination in between. minItems: 1 maxItems: 4 clock-names: items: enum: - fic0 - fic1 - fic2 - fic3 minItems: 1 maxItems: 4 Does that seem reasonable to you? Thanks, Conor.
On 14/08/2022 16:47, Conor.Dooley@microchip.com wrote: > On 12/08/2022 08:35, Krzysztof Kozlowski wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 11/08/2022 23:33, Conor Dooley wrote: >>> From: Conor Dooley <conor.dooley@microchip.com> >>> >>> Upgrading dt-schema to v2022.08 reveals unevaluatedProperties issues >>> that were not previously visible, such as the missing clocks and >>> clock-names properties for PolarFire SoC's PCI controller: >>> arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dtb: pcie@2000000000: Unevaluated properties are not allowed ('clock-names', 'clocks', 'legacy-interrupt-controller', 'microchip,axi-m-atr0' were unexpected) >>> From schema: Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml >>> >>> The clocks are required to enable interfaces between the FPGA fabric >>> and the core complex, so add them to the binding. >>> >>> Fixes: 6ee6c89aac35 ("dt-bindings: PCI: microchip: Add Microchip PolarFire host binding") >>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >>> --- >>> .../bindings/pci/microchip,pcie-host.yaml | 25 +++++++++++++++++++ >>> 1 file changed, 25 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml >>> index edb4f81253c8..2a2166f09e2c 100644 >>> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml >>> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml >>> @@ -25,6 +25,31 @@ properties: >>> - const: cfg >>> - const: apb >>> >>> + clocks: >>> + description: >>> + Fabric Interface Controllers, FICs, are the interface between the FPGA >>> + fabric and the core complex on PolarFire SoC. The FICs require two clocks, >>> + one from each side of the interface. The "FIC clocks" described by this >>> + property are on the core complex side & communication through a FIC is not >>> + possible unless it's corresponding clock is enabled. A clock must be >>> + enabled for each of the interfaces the root port is connected through. >>> + minItems: 1 >>> + items: >>> + - description: FIC0's clock >>> + - description: FIC1's clock >>> + - description: FIC2's clock >>> + - description: FIC3's clock >>> + >>> + clock-names: >>> + items: >>> + enum: >>> + - fic0 >>> + - fic1 >>> + - fic2 >>> + - fic3 >>> + minItems: 1 >>> + maxItems: 4 >> >> No need for maxItems. > > I brought this up on IRC, but transferring it here since it's been > an (understandable!!) few days & just didn't want things to get lost > if my net died. Cutting out the back & forth, in summary: > " > I'm trying to remove the maxItems from the clock-names array you > didn't like - but I can't figure out what to do instead that doesn't > trigger errors. All 4 clocks are optional, the only requirement is > that any one of them is present. Either I seem to get complaints that > my property is not an array (simply removing the maxItems) or complaints > that because I have clock0,1,3 and not 2 that clock3 is unexpected. Eh, I misread the code and thought that you list the items, but you just enumerate the schema for each item. My advice was wrong, you need maxItems. > The root port is physically on the opposite side of the FPGA to the cpus > & the AXI connection is through the FPGA fabric. There are 4 AXI > interconnects to the fabric which the PCI controller could in theory be > connected to all 4, but it only needs to be connected to one.. I had > done done minItems and maxItems a la: > devicetree/bindings/watchdog/st,stm32-iwdg.yaml > b/c that seems to have two clocks that it doesnt care about the order of > " > > Rob then suggested: > " > I would remove the 'items' list in 'clocks' and make the description > clear that any of clocks is possible. It's not ideal, but it's a case of > that's what is already there. > " > > I'd then have something along the lines of: > clocks: > description: > Fabric Interface Controllers, FICs, are the interface between the FPGA > fabric and the core complex on PolarFire SoC. The FICs require two clocks, > one from each side of the interface. The "FIC clocks" described by this > property are on the core complex side & communication through a FIC is not > possible unless it's corresponding clock is enabled. A clock must be > enabled for each of the interfaces the root port is connected through. > This could in theory be all 4 interfaces, one interface or any combination > in between. > minItems: 1 > maxItems: 4 > > clock-names: > items: > enum: > - fic0 > - fic1 > - fic2 > - fic3 > minItems: 1 > maxItems: 4 > > Does that seem reasonable to you? Yes. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml index edb4f81253c8..2a2166f09e2c 100644 --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml @@ -25,6 +25,31 @@ properties: - const: cfg - const: apb + clocks: + description: + Fabric Interface Controllers, FICs, are the interface between the FPGA + fabric and the core complex on PolarFire SoC. The FICs require two clocks, + one from each side of the interface. The "FIC clocks" described by this + property are on the core complex side & communication through a FIC is not + possible unless it's corresponding clock is enabled. A clock must be + enabled for each of the interfaces the root port is connected through. + minItems: 1 + items: + - description: FIC0's clock + - description: FIC1's clock + - description: FIC2's clock + - description: FIC3's clock + + clock-names: + items: + enum: + - fic0 + - fic1 + - fic2 + - fic3 + minItems: 1 + maxItems: 4 + interrupts: minItems: 1 items: