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@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxx>
>> ---
>>  .../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.