Re: [PATCH v5 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Jan Kiszka]

On 09.09.24 09:49, Krzysztof Kozlowski wrote:
> On 09/09/2024 08:48, Jan Kiszka wrote:
>> On 09.09.24 08:22, Krzysztof Kozlowski wrote:
>>> On Sun, Sep 08, 2024 at 07:32:28PM +0200, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
>>>>
>>>> The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
>>>> to specific regions of host memory. Add the optional property
>>>> "memory-regions" to point to such regions of memory when PVU is used.
>>>>
>>>> Since the PVU deals with system physical addresses, utilizing the PVU
>>>> with PCIe devices also requires setting up the VMAP registers to map the
>>>> Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
>>>> mapped to the system physical address. Hence, describe the VMAP
>>>> registers which are optional unless the PVU shall be used for PCIe.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
>>>> ---
>>>> CC: Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>
>>>> CC: "Krzysztof Wilczyński" <kw@xxxxxxxxx>
>>>> CC: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>>>> CC: linux-pci@xxxxxxxxxxxxxxx
>>>> ---
>>>>  .../bindings/pci/ti,am65-pci-host.yaml        | 29 +++++++++++++++++--
>>>>  1 file changed, 26 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>> index 0a9d10532cc8..0c297d12173c 100644
>>>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>> @@ -20,14 +20,18 @@ properties:
>>>>        - ti,keystone-pcie
>>>>  
>>>>    reg:
>>>> -    maxItems: 4
>>>> +    minItems: 4
>>>> +    maxItems: 6
>>>>  
>>>>    reg-names:
>>>> +    minItems: 4
>>>>      items:
>>>>        - const: app
>>>>        - const: dbics
>>>>        - const: config
>>>>        - const: atu
>>>> +      - const: vmap_lp
>>>> +      - const: vmap_hp
>>>>  
>>>>    interrupts:
>>>>      maxItems: 1
>>>> @@ -83,13 +87,30 @@ if:
>>>>      compatible:
>>>>        enum:
>>>>          - ti,am654-pcie-rc
>>>> +
>>>>  then:
>>>> +  properties:
>>>> +    memory-region:
>>>
>>> I think I said it two times already. You must define properties in
>>> top-level. That's how we expect, that's how dtschema works (even if it
>>> works fine otherwise, it's not always that case), that's how almost all
>>> bindings are written.
>>
>> Look, if you have such rules, also enhance the checker, or people like
>> me will continue to work intuitively. Add reasoning along that as well,
> 
> That would be ideal, but I also asked to do this twice. It does not
> matter if dtschema  or me tells you this, if you do not implement it.
> 
>> would help further to reduce your review effort. The current situation
>> with rather fuzzy results from the checker and strange mechanisms inside
>> (see my maxItems finding) is not very helpful IMHO.
>>
>> I this concrete case, I would add this item top-level, just to set
>> maxItems to 0 for ti,keystone-pcie? Not a pattern I'm finding anywhere.
>> Or do we have to allow memory-regions for all compatibles now?
> 
> Is it really not suitable for all the compatibles? Maybe these are quite
> different devices in such case?
> 
> But if it is not really suitable, then you can disallow it for other
> variants with :false. This is also explicitly documented in example-schema:
> https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L212
> 

I will address this via documentation: The property is not hardware
related but permits swiotlb. It can be applied to devices as well that
have no hardware enforcement, unlike the am654.

Jan

-- 
Siemens AG, Technology
Linux Expert Center