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

From: Jan Kiszka
Date: Mon Sep 09 2024 - 08:24:00 EST


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