diff mbox series

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

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

Commit Message

Conor Dooley Aug. 11, 2022, 8:33 p.m. UTC
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(+)

Comments

Krzysztof Kozlowski Aug. 12, 2022, 7:35 a.m. UTC | #1
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
Krzysztof Kozlowski Aug. 12, 2022, 8 a.m. UTC | #2
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
Conor Dooley Aug. 12, 2022, 8:09 a.m. UTC | #3
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.
Conor Dooley Aug. 14, 2022, 1:47 p.m. UTC | #4
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.
Krzysztof Kozlowski Aug. 16, 2022, 7:25 a.m. UTC | #5
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 mbox series

Patch

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: