| Message ID | 20220811203306.179744-1-mail@conchuod.ie |
|---|---|
| Headers | show |
| Series | Fix RISC-V/PCI dt-schema issues with dt-schema v2022.08 | expand |
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 10:55, Conor.Dooley@microchip.com wrote: > On 12/08/2022 08:42, 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> >>> >>> v2022.08 of dt-schema improved checking of unevaluatedProperties, and >>> exposed a previously unseen warning for the PCIe controller's interrupt >>> controller node name: >>> >>> 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 >>> >>> Make the property in the binding match the node name actually used in >>> the dts. >>> >>> Fixes: dcd49679fb3a ("dt-bindings: PCI: Fix 'unevaluatedProperties' warnings") >>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >>> --- >>> This is another one Rob where I feel like I'm doing the wrong thing. >>> The Linux driver gets the child node without using the name, but >>> another OS etc could in theory (or reality), right? >> >> Yes and we had such cases when renaming device nodes caused regression. >> My interpretation is that node name is not part of ABI, so anyone >> depending on it made a mistake and they need to fix their stuff. I think >> actually that is really poor coding and poor solution to parse device >> node names and expect specific name. >> >> Other folks interpretation is that we never break the users of kernel, >> regardless what is documented in the ABI... so it depends. :) >> >> Here however it is not a device node name, but a property name (although >> still a node). Bindings require these to be specific, thus such name is >> a part of ABI. > > Yup, pretty much aligned to my thoughts on this. > >> For your case, I wonder why it was called "legacy-interrupt-controller" >> in the first place? Node names - also for properties - should be >> generic, so generic name is just "interrupt-controller". > > I don't know. It's what we had in our internal tree prior to upstreaming. > "We" don't rely on the name for the Linux driver, so I am not really that > bothered if we change the binding or the dts. Then I propose to change the name in DTS. Best regards, Krzysztof
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.
From: Conor Dooley <conor.dooley@microchip.com> Hey all, Got a few fixes for PCI dt-bindings that I noticed after upgrading my dt-schema to v2022.08. I am unsure if some of these patches are the right fixes, which I noted in the patches themselves, especially the address translation property. Thanks, Conor. Conor Dooley (4): dt-bindings: PCI: fu740-pci: fix missing clock-names dt-bindings: PCI: microchip,pcie-host: fix missing clocks properties dt-bindings: PCI: microchip,pcie-host: fix incorrect child node name dt-bindings: PCI: microchip,pcie-host: fix missing address translation property .../bindings/pci/microchip,pcie-host.yaml | 40 ++++++++++++++++++- .../bindings/pci/sifive,fu740-pcie.yaml | 6 +++ 2 files changed, 44 insertions(+), 2 deletions(-)