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