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

[<Conor.Dooley@microchip.com>]

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