[PATCH 0/4] Fix RISC-V/PCI dt-schema issues with dt-schema v2022.08

[PATCH 0/4] Fix RISC-V/PCI dt-schema issues with dt-schema v2022.08

[Conor Dooley]

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(-)

-- 
2.37.1

[PATCH 1/4] dt-bindings: PCI: fu740-pci: fix missing clock-names

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The commit in the fixes tag removed the clock-names property from the
SiFive FU740 PCI Controller dt-binding, but it was already in the dts
for the FU740. dtbs_check was not able to pick up on this at the time
but v2022.08 of dt-schema now can:

arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dtb: pcie@e00000000: Unevaluated properties are not allowed ('clock-names' was unexpected)
        From schema: linux/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml

The Linux driver does not use this property, but outside of the kernel
this property may have users. Re-add the property and its "clocks"
dependency.

Fixes: b92225b034c0 ("dt-bindings: PCI: designware: Fix 'unevaluatedProperties' warnings")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
I went back and forth on removing the property from the dts, but this
seems like the change that is more conservative..
---
 .../devicetree/bindings/pci/sifive,fu740-pcie.yaml          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml b/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
index 195e6afeb169..c7a9a2dc0fa6 100644
--- a/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
@@ -51,6 +51,12 @@ properties:
     description: A phandle to the PCIe power up reset line.
     maxItems: 1
 
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: pcie_aux
+
   pwren-gpios:
     description: Should specify the GPIO for controlling the PCI bus device power on.
     maxItems: 1
-- 
2.37.1

[PATCH 2/4] dt-bindings: PCI: microchip,pcie-host: fix missing clocks properties

[Conor Dooley]

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
+
   interrupts:
     minItems: 1
     items:
-- 
2.37.1

[PATCH 3/4] dt-bindings: PCI: microchip,pcie-host: fix incorrect child node name

[Conor Dooley]

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?
---
 .../devicetree/bindings/pci/microchip,pcie-host.yaml          | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
index 2a2166f09e2c..9b123bcd034c 100644
--- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
@@ -71,7 +71,7 @@ properties:
   msi-parent:
     description: MSI controller the device is capable of using.
 
-  interrupt-controller:
+  legacy-interrupt-controller:
     type: object
     properties:
       '#address-cells':
@@ -125,7 +125,7 @@ examples:
                     msi-controller;
                     bus-range = <0x00 0x7f>;
                     ranges = <0x03000000 0x0 0x78000000 0x0 0x78000000 0x0 0x04000000>;
-                    pcie_intc0: interrupt-controller {
+                    pcie_intc0: legacy-interrupt-controller {
                         #address-cells = <0>;
                         #interrupt-cells = <1>;
                         interrupt-controller;
-- 
2.37.1

[PATCH 4/4] dt-bindings: PCI: microchip,pcie-host: fix missing address translation property

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

When the PCI controller node was added to the PolarFire SoC dtsi,
dt-schema was not able to detect the presence of some undocumented
properties due to how it handled unevaluatedProperties. v2022.08
introduces better validation, producing the following error:

arch/riscv/boot/dts/microchip/mpfs-polarberry.dtb: pcie@2000000000: Unevaluated properties are not allowed ('clock-names', 'microchip,axi-m-atr0' were unexpected)
        From schema: Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml

Fixes: 528a5b1f2556 ("riscv: dts: microchip: add new peripherals to icicle kit device tree")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
I feel like there's a pretty good chance that this is not the way this
should have been done and the property should be marked as deprecated
but I don't know enough about PCI to answer that.
---
 .../devicetree/bindings/pci/microchip,pcie-host.yaml  | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
index 9b123bcd034c..9ac34b33c4b2 100644
--- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
@@ -71,6 +71,17 @@ properties:
   msi-parent:
     description: MSI controller the device is capable of using.
 
+  microchip,axi-m-atr0:
+    description: |
+      Depending on the FPGA bitstream, the AXIM address translation table in the
+      PCIe controllers bridge layer may need to be configured. Use this property
+      to set the address offset. For more information, see Section 1.3.3,
+      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe User Guide:
+      https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    minItems: 2
+    maxItems: 2
+
   legacy-interrupt-controller:
     type: object
     properties:
-- 
2.37.1

Re: [PATCH 1/4] dt-bindings: PCI: fu740-pci: fix missing clock-names

[Krzysztof Kozlowski]

On 11/08/2022 23:33, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The commit in the fixes tag removed the clock-names property from the

Instead:
The commit b92225b034c0 ("dt-bindings: PCI: designware: Fix
'unevaluatedProperties' warnings")....

> SiFive FU740 PCI Controller dt-binding,

No, it did not do it... At least I cannot see it. Where is the removal
exactly in that patch? The commit removed clock-names from required, not
from properties.

 but it was already in the dts
> for the FU740. dtbs_check was not able to pick up on this at the time
> but v2022.08 of dt-schema now can:
> 
> arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dtb: pcie@e00000000: Unevaluated properties are not allowed ('clock-names' was unexpected)
>         From schema: linux/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
> 
> The Linux driver does not use this property, but outside of the kernel
> this property may have users. Re-add the property and its "clocks"
> dependency.
> 


Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: PCI: microchip,pcie-host: fix missing clocks properties

[Krzysztof Kozlowski]

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

Re: [PATCH 3/4] dt-bindings: PCI: microchip,pcie-host: fix incorrect child node name

[Krzysztof Kozlowski]

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.

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".

> ---
>  .../devicetree/bindings/pci/microchip,pcie-host.yaml          | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> index 2a2166f09e2c..9b123bcd034c 100644
> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> @@ -71,7 +71,7 @@ properties:
>    msi-parent:
>      description: MSI controller the device is capable of using.
>  
> -  interrupt-controller:
> +  legacy-interrupt-controller:


Best regards,
Krzysztof

Re: [PATCH 4/4] dt-bindings: PCI: microchip,pcie-host: fix missing address translation property

[Krzysztof Kozlowski]

On 11/08/2022 23:33, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> When the PCI controller node was added to the PolarFire SoC dtsi,
> dt-schema was not able to detect the presence of some undocumented
> properties due to how it handled unevaluatedProperties. v2022.08
> introduces better validation, producing the following error:
> 
> arch/riscv/boot/dts/microchip/mpfs-polarberry.dtb: pcie@2000000000: Unevaluated properties are not allowed ('clock-names', 'microchip,axi-m-atr0' were unexpected)
>         From schema: Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> 
> Fixes: 528a5b1f2556 ("riscv: dts: microchip: add new peripherals to icicle kit device tree")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> I feel like there's a pretty good chance that this is not the way this
> should have been done and the property should be marked as deprecated
> but I don't know enough about PCI to answer that.

It seems bindings were added incomplete and now based on DTS (which did
not match bindings), we keep adding "missing" properties. I don't think
it is good. It creates a precedence where someone might intentionally
sneak limited bindings (without controversial property) and later claim
"I forgot to include foo,bar".

Therefore the property should pass review just like it is newly added
property.

> ---
>  .../devicetree/bindings/pci/microchip,pcie-host.yaml  | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> index 9b123bcd034c..9ac34b33c4b2 100644
> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> @@ -71,6 +71,17 @@ properties:
>    msi-parent:
>      description: MSI controller the device is capable of using.
>  
> +  microchip,axi-m-atr0:

Name is not helping. If it is offset, add such suffix (see
brcm,iproc-pcie.yaml).

Unfortunately I don't know PCIe good enough to judge whether the
property makes any sense or some other ranges-style should be used.

> +    description: |
> +      Depending on the FPGA bitstream, the AXIM address translation table in the
> +      PCIe controllers bridge layer may need to be configured. Use this property
> +      to set the address offset. For more information, see Section 1.3.3,
> +      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe User Guide:
> +      https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    minItems: 2

minItems should not be needed, but you should instead describe the items
in the matrix.

> +    maxItems: 2


Best regards,
Krzysztof

Re: [PATCH 3/4] dt-bindings: PCI: microchip,pcie-host: fix incorrect child node name

[Conor.Dooley]

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.

> 
>> ---
>>   .../devicetree/bindings/pci/microchip,pcie-host.yaml          | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> index 2a2166f09e2c..9b123bcd034c 100644
>> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> @@ -71,7 +71,7 @@ properties:
>>     msi-parent:
>>       description: MSI controller the device is capable of using.
>>
>> -  interrupt-controller:
>> +  legacy-interrupt-controller:
> 
> 
> Best regards,
> Krzysztof
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/4] dt-bindings: PCI: fu740-pci: fix missing clock-names

[Conor.Dooley]

On 12/08/2022 08:34, 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>
>>
>> The commit in the fixes tag removed the clock-names property from the
> 
> Instead:
> The commit b92225b034c0 ("dt-bindings: PCI: designware: Fix
> 'unevaluatedProperties' warnings")....
> 
>> SiFive FU740 PCI Controller dt-binding,
> 
> No, it did not do it... At least I cannot see it. Where is the removal
> exactly in that patch? The commit removed clock-names from required, not
> from properties.

I rewrote the commmit message 3 times as I went back and forth on the
"right" change to make, prob just added the wrong fixes tag to a copy
paste of the original message.
Thanks,
Conor.

> 
>   but it was already in the dts
>> for the FU740. dtbs_check was not able to pick up on this at the time
>> but v2022.08 of dt-schema now can:
>>
>> arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dtb: pcie@e00000000: Unevaluated properties are not allowed ('clock-names' was unexpected)
>>          From schema: linux/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
>>
>> The Linux driver does not use this property, but outside of the kernel
>> this property may have users. Re-add the property and its "clocks"
>> dependency.
>>
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 2/4] dt-bindings: PCI: microchip,pcie-host: fix missing clocks properties

[Krzysztof Kozlowski]

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

Re: [PATCH 2/4] dt-bindings: PCI: microchip,pcie-host: fix missing clocks properties

[Conor.Dooley]

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.

Re: [PATCH 4/4] dt-bindings: PCI: microchip,pcie-host: fix missing address translation property

[Conor.Dooley]

On 12/08/2022 08:52, 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>
>>
>> When the PCI controller node was added to the PolarFire SoC dtsi,
>> dt-schema was not able to detect the presence of some undocumented
>> properties due to how it handled unevaluatedProperties. v2022.08
>> introduces better validation, producing the following error:
>>
>> arch/riscv/boot/dts/microchip/mpfs-polarberry.dtb: pcie@2000000000: Unevaluated properties are not allowed ('clock-names', 'microchip,axi-m-atr0' were unexpected)
>>          From schema: Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>>
>> Fixes: 528a5b1f2556 ("riscv: dts: microchip: add new peripherals to icicle kit device tree")
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>> I feel like there's a pretty good chance that this is not the way this
>> should have been done and the property should be marked as deprecated
>> but I don't know enough about PCI to answer that.
> 
> It seems bindings were added incomplete and now based on DTS (which did
> not match bindings), we keep adding "missing" properties. I don't think
> it is good. It creates a precedence where someone might intentionally
> sneak limited bindings (without controversial property) and later claim
> "I forgot to include foo,bar".

Yup, again pretty much the same thoughts as me. I don't think that, even
if the property is valid, should be either named as it is or only work
for translation table 0.

> 
> Therefore the property should pass review just like it is newly added
> property.

SGTM.

> 
>> ---
>>   .../devicetree/bindings/pci/microchip,pcie-host.yaml  | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> index 9b123bcd034c..9ac34b33c4b2 100644
>> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>> @@ -71,6 +71,17 @@ properties:
>>     msi-parent:
>>       description: MSI controller the device is capable of using.
>>
>> +  microchip,axi-m-atr0:
> 
> Name is not helping. If it is offset, add such suffix (see
> brcm,iproc-pcie.yaml).
> 
> Unfortunately I don't know PCIe good enough to judge whether the
> property makes any sense or some other ranges-style should be used.

Yup, I think it is similar to that. Except we have 4 tables rather
than one.

> 
>> +    description: |
>> +      Depending on the FPGA bitstream, the AXIM address translation table in the
>> +      PCIe controllers bridge layer may need to be configured. Use this property
>> +      to set the address offset. For more information, see Section 1.3.3,
>> +      "PCIe/AXI4 Address Translation" of the PolarFire SoC PCIe User Guide:
>> +      https://www.microsemi.com/document-portal/doc_download/1245812-polarfire-fpga-and-polarfire-soc-fpga-pci-express-user-guide
>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>> +    minItems: 2
> 
> minItems should not be needed, but you should instead describe the items
> in the matrix.

SGTM. Thanks Krzysztof.

> 
>> +    maxItems: 2
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 3/4] dt-bindings: PCI: microchip,pcie-host: fix incorrect child node name

[Krzysztof Kozlowski]

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

Re: [PATCH 2/4] dt-bindings: PCI: microchip,pcie-host: fix missing clocks properties

[Conor.Dooley]

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.

Re: [PATCH 2/4] dt-bindings: PCI: microchip,pcie-host: fix missing clocks properties

[Krzysztof Kozlowski]

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

Re: [PATCH 4/4] dt-bindings: PCI: microchip,pcie-host: fix missing address translation property

[Rob Herring]

On Fri, Aug 12, 2022 at 08:20:45AM +0000, Conor.Dooley@microchip.com wrote:
> On 12/08/2022 08:52, 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>
> >>
> >> When the PCI controller node was added to the PolarFire SoC dtsi,
> >> dt-schema was not able to detect the presence of some undocumented
> >> properties due to how it handled unevaluatedProperties. v2022.08
> >> introduces better validation, producing the following error:
> >>
> >> arch/riscv/boot/dts/microchip/mpfs-polarberry.dtb: pcie@2000000000: Unevaluated properties are not allowed ('clock-names', 'microchip,axi-m-atr0' were unexpected)
> >>          From schema: Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> >>
> >> Fixes: 528a5b1f2556 ("riscv: dts: microchip: add new peripherals to icicle kit device tree")
> >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> >> ---
> >> I feel like there's a pretty good chance that this is not the way this
> >> should have been done and the property should be marked as deprecated
> >> but I don't know enough about PCI to answer that.
> > 
> > It seems bindings were added incomplete and now based on DTS (which did
> > not match bindings), we keep adding "missing" properties. I don't think
> > it is good. It creates a precedence where someone might intentionally
> > sneak limited bindings (without controversial property) and later claim
> > "I forgot to include foo,bar".
> 
> Yup, again pretty much the same thoughts as me. I don't think that, even
> if the property is valid, should be either named as it is or only work
> for translation table 0.
> 
> > 
> > Therefore the property should pass review just like it is newly added
> > property.
> 
> SGTM.
> 
> > 
> >> ---
> >>   .../devicetree/bindings/pci/microchip,pcie-host.yaml  | 11 +++++++++++
> >>   1 file changed, 11 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> >> index 9b123bcd034c..9ac34b33c4b2 100644
> >> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> >> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> >> @@ -71,6 +71,17 @@ properties:
> >>     msi-parent:
> >>       description: MSI controller the device is capable of using.
> >>
> >> +  microchip,axi-m-atr0:
> > 
> > Name is not helping. If it is offset, add such suffix (see
> > brcm,iproc-pcie.yaml).
> > 
> > Unfortunately I don't know PCIe good enough to judge whether the
> > property makes any sense or some other ranges-style should be used.
> 
> Yup, I think it is similar to that. Except we have 4 tables rather
> than one.

Looks to me like dma-ranges is the answer.

Rob

Re: [PATCH 4/4] dt-bindings: PCI: microchip,pcie-host: fix missing address translation property

[Conor.Dooley]

On 16/08/2022 18:16, Rob Herring wrote:
>>>>   .../devicetree/bindings/pci/microchip,pcie-host.yaml  | 11 +++++++++++
>>>>   1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>>>> index 9b123bcd034c..9ac34b33c4b2 100644
>>>> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>>>> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
>>>> @@ -71,6 +71,17 @@ properties:
>>>>     msi-parent:
>>>>       description: MSI controller the device is capable of using.
>>>>
>>>> +  microchip,axi-m-atr0:
>>>
>>> Name is not helping. If it is offset, add such suffix (see
>>> brcm,iproc-pcie.yaml).
>>>
>>> Unfortunately I don't know PCIe good enough to judge whether the
>>> property makes any sense or some other ranges-style should be used.
>>
>> Yup, I think it is similar to that. Except we have 4 tables rather
>> than one.
> 
> Looks to me like dma-ranges is the answer.

Hey Rob,
Thanks for chiming in. I think what I'll do is:
- delete the property from the dts, as it should never have been
  there in the first place.
- resubmit the series without this patch to clear the dtbs_check
  warnings
- add dma-ranges separately

Thanks,
Conor.