| Message ID | 28d31a14fe9cc1867f023ebaddd6074459d15e40.1725444016.git.jan.kiszka@siemens.com |
|---|---|
| State | New |
| Headers | show |
| Series | soc: ti: Add and use PVU on K3-AM65 for DMA isolation | expand |
On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > 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 optionally unless the PVU shall used for PCIe. The last line above seems to be accidentally modified when compared to the one at: https://lore.kernel.org/r/ada462d5-157a-4e11-ba25-d412a2bb678f@ti.com/ "Hence, describe the VMAP registers which are optionally configured whenever PVU is used for PCIe." If you intended to modify it, then the sentence appears distorted. Regards, Siddharth.
On 04.09.24 12:16, Siddharth Vadapalli wrote: > On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> 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 optionally unless the PVU shall used for PCIe. > > The last line above seems to be accidentally modified when compared to > the one at: > https://lore.kernel.org/r/ada462d5-157a-4e11-ba25-d412a2bb678f@ti.com/ > "Hence, describe the VMAP registers which are optionally > configured whenever PVU is used for PCIe." > > If you intended to modify it, then the sentence appears distorted. > Yeah, I wanted to make the original sentence clearer but failed: "...which are optional unless the PVU shall be used for PCIe." Jan
On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > 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 optionally unless the PVU shall used for PCIe. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > CC: Lorenzo Pieralisi <lpieralisi@kernel.org> > CC: "Krzysztof Wilczyński" <kw@linux.com> > CC: Bjorn Helgaas <bhelgaas@google.com> > CC: linux-pci@vger.kernel.org > --- > .../bindings/pci/ti,am65-pci-host.yaml | 52 ++++++++++++++----- > 1 file changed, 40 insertions(+), 12 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..d8182bad92de 100644 > --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml > +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml > @@ -19,16 +19,6 @@ properties: > - ti,am654-pcie-rc > - ti,keystone-pcie > > - reg: > - maxItems: 4 > - > - reg-names: > - items: > - - const: app > - - const: dbics > - - const: config > - - const: atu Nothing improved here. > - > interrupts: > maxItems: 1 > > @@ -84,12 +74,48 @@ if: > enum: > - ti,am654-pcie-rc > then: > + properties: > + reg: > + minItems: 4 > + maxItems: 6 > + > + reg-names: > + minItems: 4 > + items: > + - const: app > + - const: dbics > + - const: config > + - const: atu > + - const: vmap_lp > + - const: vmap_hp > + > + memory-region: > + minItems: 1 Missing maxItems > + description: | > + phandle to one or more restricted DMA pools to be used for all devices > + behind this controller. The regions should be defined according to > + reserved-memory/shared-dma-pool.yaml. > + items: > + maxItems: 1 And this feels redundant. Best regards, Krzysztof
On 05.09.24 08:32, Krzysztof Kozlowski wrote: > On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> 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 optionally unless the PVU shall used for PCIe. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> --- >> CC: Lorenzo Pieralisi <lpieralisi@kernel.org> >> CC: "Krzysztof Wilczyński" <kw@linux.com> >> CC: Bjorn Helgaas <bhelgaas@google.com> >> CC: linux-pci@vger.kernel.org >> --- >> .../bindings/pci/ti,am65-pci-host.yaml | 52 ++++++++++++++----- >> 1 file changed, 40 insertions(+), 12 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..d8182bad92de 100644 >> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >> @@ -19,16 +19,6 @@ properties: >> - ti,am654-pcie-rc >> - ti,keystone-pcie >> >> - reg: >> - maxItems: 4 >> - >> - reg-names: >> - items: >> - - const: app >> - - const: dbics >> - - const: config >> - - const: atu > > > Nothing improved here. Yes, explained the background to you. Sorry, if you do not address my replies, I'm lost with your feedback. > >> - >> interrupts: >> maxItems: 1 >> >> @@ -84,12 +74,48 @@ if: >> enum: >> - ti,am654-pcie-rc >> then: >> + properties: >> + reg: >> + minItems: 4 >> + maxItems: 6 >> + >> + reg-names: >> + minItems: 4 >> + items: >> + - const: app >> + - const: dbics >> + - const: config >> + - const: atu >> + - const: vmap_lp >> + - const: vmap_hp >> + >> + memory-region: >> + minItems: 1 > > Missing maxItems Same as above. Please address my answers. > >> + description: | >> + phandle to one or more restricted DMA pools to be used for all devices >> + behind this controller. The regions should be defined according to >> + reserved-memory/shared-dma-pool.yaml. >> + items: >> + maxItems: 1 > > And this feels redundant. I can drop that if it's not needed. Unfortunately, schemas are far from being intuitive to me. Jan
On 05/09/2024 08:40, Jan Kiszka wrote: > On 05.09.24 08:32, Krzysztof Kozlowski wrote: >> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote: >>> From: Jan Kiszka <jan.kiszka@siemens.com> >>> >>> 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 optionally unless the PVU shall used for PCIe. >>> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>> --- >>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org> >>> CC: "Krzysztof Wilczyński" <kw@linux.com> >>> CC: Bjorn Helgaas <bhelgaas@google.com> >>> CC: linux-pci@vger.kernel.org >>> --- >>> .../bindings/pci/ti,am65-pci-host.yaml | 52 ++++++++++++++----- >>> 1 file changed, 40 insertions(+), 12 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..d8182bad92de 100644 >>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>> @@ -19,16 +19,6 @@ properties: >>> - ti,am654-pcie-rc >>> - ti,keystone-pcie >>> >>> - reg: >>> - maxItems: 4 >>> - >>> - reg-names: >>> - items: >>> - - const: app >>> - - const: dbics >>> - - const: config >>> - - const: atu >> >> >> Nothing improved here. > > Yes, explained the background to you. Sorry, if you do not address my > replies, I'm lost with your feedback. My magic ball could not figure out the problem, so did not provide the answer. I gave you the exact code which illustrates how to do it. If you do it that way: it works. If you do it other way: it might not work. However without seeing anything, magic ball was silent, so I am not participating in game: would you be so kind to give more information so I won't waste my day in asking what is wrong. Best regards, Krzysztof
On 04/09/2024 12:00, Jan Kiszka wrote: > + > + reg-names: > + minItems: 4 > + items: > + - const: app > + - const: dbics > + - const: config > + - const: atu > + - const: vmap_lp > + - const: vmap_hp > + > + memory-region: This also did not improve. You did not address any feedback from v3. Missed feedback: This *must* be defined in top-level. I still think this must have some sort of maxItems. I accept your explanation that you could have multiple memory pools, but I don't think 2147000 pools is possible. Make it 4, 8 or 32. > + minItems: 1 > + description: | > + phandle to one or more restricted DMA pools to be used for all devices > + behind this controller. The regions should be defined according to > + reserved-memory/shared-dma-pool.yaml. > + items: > + maxItems: 1 Best regards, Krzysztof
On 05.09.24 08:53, Krzysztof Kozlowski wrote: > On 05/09/2024 08:40, Jan Kiszka wrote: >> On 05.09.24 08:32, Krzysztof Kozlowski wrote: >>> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote: >>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>> >>>> 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 optionally unless the PVU shall used for PCIe. >>>> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>> --- >>>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org> >>>> CC: "Krzysztof Wilczyński" <kw@linux.com> >>>> CC: Bjorn Helgaas <bhelgaas@google.com> >>>> CC: linux-pci@vger.kernel.org >>>> --- >>>> .../bindings/pci/ti,am65-pci-host.yaml | 52 ++++++++++++++----- >>>> 1 file changed, 40 insertions(+), 12 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..d8182bad92de 100644 >>>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>>> @@ -19,16 +19,6 @@ properties: >>>> - ti,am654-pcie-rc >>>> - ti,keystone-pcie >>>> >>>> - reg: >>>> - maxItems: 4 >>>> - >>>> - reg-names: >>>> - items: >>>> - - const: app >>>> - - const: dbics >>>> - - const: config >>>> - - const: atu >>> >>> >>> Nothing improved here. >> >> Yes, explained the background to you. Sorry, if you do not address my >> replies, I'm lost with your feedback. > > My magic ball could not figure out the problem, so did not provide the > answer. > > I gave you the exact code which illustrates how to do it. If you do it > that way: it works. If you do it other way: it might not work. However The link you provided was unfortunately not self-explanatory because if I - apparently - do it like that example, I'm getting the errors below. > without seeing anything, magic ball was silent, so I am not > participating in game: would you be so kind to give more information so > I won't waste my day in asking what is wrong. With my patch: # make ... dtbs_check DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dtb DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dtb OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie.dtb OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-usb3.dtb DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dtb DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dtb DTC [C] arch/arm64/boot/dts/ti/k3-am654-base-board.dtb With this revert on top: diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml index d8182bad92de..dd753dae24c6 100644 --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml @@ -19,6 +19,16 @@ properties: - ti,am654-pcie-rc - ti,keystone-pcie + reg: + maxItems: 4 + + reg-names: + items: + - const: app + - const: dbics + - const: config + - const: atu + interrupts: maxItems: 1 @@ -104,18 +114,6 @@ then: - msi-map - num-viewport -else: - properties: - reg: - maxItems: 4 - - reg-names: - items: - - const: app - - const: dbics - - const: config - - const: atu - unevaluatedProperties: false examples: # make ... dtbs_check DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5500000: reg: [[0, 89128960, 0, 4096], [0, 89133056, 0, 4096], [0, 268435456, 0, 8192], [0, 89153536, 0, 4096], [0, 42991616, 0, 4096], [0, 43024384, 0, 4096]] is too long from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml# .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5500000: reg-names: ['app', 'dbics', 'config', 'atu', 'vmap_lp', 'vmap_hp'] is too long from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml# .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5600000: reg: [[0, 90177536, 0, 4096], [0, 90181632, 0, 4096], [0, 402653184, 0, 8192], [0, 90202112, 0, 4096], [0, 43057152, 0, 4096], [0, 43089920, 0, 4096]] is too long from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml# .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5600000: reg-names: ['app', 'dbics', 'config', 'atu', 'vmap_lp', 'vmap_hp'] is too long from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml# DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb ... Which magic dtschema spell am I missing to make this work like you suggest? Jan
On 05.09.24 08:57, Krzysztof Kozlowski wrote: > On 04/09/2024 12:00, Jan Kiszka wrote: >> + >> + reg-names: >> + minItems: 4 >> + items: >> + - const: app >> + - const: dbics >> + - const: config >> + - const: atu >> + - const: vmap_lp >> + - const: vmap_hp >> + >> + memory-region: > > This also did not improve. You did not address any feedback from v3. > > Missed feedback: > > This *must* be defined in top-level. Answered in the other reply. > I still think this must have some sort of maxItems. I accept your > explanation that you could have multiple memory pools, but I don't think > 2147000 pools is possible. Make it 4, 8 or 32. Can do - if you can explain the benefit of that. Jan
On 05/09/2024 09:15, Jan Kiszka wrote: > On 05.09.24 08:53, Krzysztof Kozlowski wrote: >> On 05/09/2024 08:40, Jan Kiszka wrote: >>> On 05.09.24 08:32, Krzysztof Kozlowski wrote: >>>> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote: >>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>> >>>>> 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 optionally unless the PVU shall used for PCIe. >>>>> >>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>> --- >>>>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org> >>>>> CC: "Krzysztof Wilczyński" <kw@linux.com> >>>>> CC: Bjorn Helgaas <bhelgaas@google.com> >>>>> CC: linux-pci@vger.kernel.org >>>>> --- >>>>> .../bindings/pci/ti,am65-pci-host.yaml | 52 ++++++++++++++----- >>>>> 1 file changed, 40 insertions(+), 12 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..d8182bad92de 100644 >>>>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>>>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>>>> @@ -19,16 +19,6 @@ properties: >>>>> - ti,am654-pcie-rc >>>>> - ti,keystone-pcie >>>>> >>>>> - reg: >>>>> - maxItems: 4 >>>>> - >>>>> - reg-names: >>>>> - items: >>>>> - - const: app >>>>> - - const: dbics >>>>> - - const: config >>>>> - - const: atu >>>> >>>> >>>> Nothing improved here. >>> >>> Yes, explained the background to you. Sorry, if you do not address my >>> replies, I'm lost with your feedback. >> >> My magic ball could not figure out the problem, so did not provide the >> answer. >> >> I gave you the exact code which illustrates how to do it. If you do it >> that way: it works. If you do it other way: it might not work. However > > The link you provided was unfortunately not self-explanatory because if > I - apparently - do it like that example, I'm getting the errors below. > >> without seeing anything, magic ball was silent, so I am not >> participating in game: would you be so kind to give more information so >> I won't waste my day in asking what is wrong. > > With my patch: > > # make ... dtbs_check > DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb > DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb > DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dtb > DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dtb > OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie.dtb > OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-usb3.dtb > DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dtb > DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dtb > DTC [C] arch/arm64/boot/dts/ti/k3-am654-base-board.dtb > > With this revert on top: > > diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml > index d8182bad92de..dd753dae24c6 100644 > --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml > +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml > @@ -19,6 +19,16 @@ properties: > - ti,am654-pcie-rc > - ti,keystone-pcie > > + reg: > + maxItems: 4 > + > + reg-names: > + items: > + - const: app > + - const: dbics > + - const: config > + - const: atu There is nothing like that in that example. https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44 > + > interrupts: > maxItems: 1 > > @@ -104,18 +114,6 @@ then: > - msi-map > - num-viewport > > -else: > - properties: > - reg: > - maxItems: 4 > - > - reg-names: > - items: > - - const: app > - - const: dbics > - - const: config > - - const: atu Neither this. Each case MUST be covered, look: https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L191 > - > unevaluatedProperties: false > > examples: > > # make ... dtbs_check > DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb > .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5500000: reg: [[0, 89128960, 0, 4096], [0, 89133056, 0, 4096], [0, 268435456, 0, 8192], [0, 89153536, 0, 4096], [0, 42991616, 0, 4096], [0, 43024384, 0, 4096]] is too long > from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml# > .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5500000: reg-names: ['app', 'dbics', 'config', 'atu', 'vmap_lp', 'vmap_hp'] is too long > from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml# > .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5600000: reg: [[0, 90177536, 0, 4096], [0, 90181632, 0, 4096], [0, 402653184, 0, 8192], [0, 90202112, 0, 4096], [0, 43057152, 0, 4096], [0, 43089920, 0, 4096]] is too long > from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml# > .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5600000: reg-names: ['app', 'dbics', 'config', 'atu', 'vmap_lp', 'vmap_hp'] is too long > from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml# > DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb > ... > > > Which magic dtschema spell am I missing to make this work like you > suggest? follow the example. You do it entirely different so you have different result. Code works deterministically. Best regards, Krzysztof
On 05/09/2024 09:16, Jan Kiszka wrote: > On 05.09.24 08:57, Krzysztof Kozlowski wrote: >> On 04/09/2024 12:00, Jan Kiszka wrote: >>> + >>> + reg-names: >>> + minItems: 4 >>> + items: >>> + - const: app >>> + - const: dbics >>> + - const: config >>> + - const: atu >>> + - const: vmap_lp >>> + - const: vmap_hp >>> + >>> + memory-region: >> >> This also did not improve. You did not address any feedback from v3. >> >> Missed feedback: >> >> This *must* be defined in top-level. > > Answered in the other reply. I don't see anywhere answer, but regardless your binding is not an exception. Please follow rules for every binding, not expect something special here. Best regards, Krzysztof
On 05/09/2024 09:50, Krzysztof Kozlowski wrote: > On 05/09/2024 09:15, Jan Kiszka wrote: >> On 05.09.24 08:53, Krzysztof Kozlowski wrote: >>> On 05/09/2024 08:40, Jan Kiszka wrote: >>>> On 05.09.24 08:32, Krzysztof Kozlowski wrote: >>>>> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote: >>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>> >>>>>> 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 optionally unless the PVU shall used for PCIe. >>>>>> >>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>> --- >>>>>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org> >>>>>> CC: "Krzysztof Wilczyński" <kw@linux.com> >>>>>> CC: Bjorn Helgaas <bhelgaas@google.com> >>>>>> CC: linux-pci@vger.kernel.org >>>>>> --- >>>>>> .../bindings/pci/ti,am65-pci-host.yaml | 52 ++++++++++++++----- >>>>>> 1 file changed, 40 insertions(+), 12 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..d8182bad92de 100644 >>>>>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>>>>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>>>>> @@ -19,16 +19,6 @@ properties: >>>>>> - ti,am654-pcie-rc >>>>>> - ti,keystone-pcie >>>>>> >>>>>> - reg: >>>>>> - maxItems: 4 >>>>>> - >>>>>> - reg-names: >>>>>> - items: >>>>>> - - const: app >>>>>> - - const: dbics >>>>>> - - const: config >>>>>> - - const: atu >>>>> >>>>> >>>>> Nothing improved here. >>>> >>>> Yes, explained the background to you. Sorry, if you do not address my >>>> replies, I'm lost with your feedback. >>> >>> My magic ball could not figure out the problem, so did not provide the >>> answer. >>> >>> I gave you the exact code which illustrates how to do it. If you do it >>> that way: it works. If you do it other way: it might not work. However >> >> The link you provided was unfortunately not self-explanatory because if >> I - apparently - do it like that example, I'm getting the errors below. >> >>> without seeing anything, magic ball was silent, so I am not >>> participating in game: would you be so kind to give more information so >>> I won't waste my day in asking what is wrong. >> >> With my patch: >> >> # make ... dtbs_check >> DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb >> DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb >> DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dtb >> DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dtb >> OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie.dtb >> OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-usb3.dtb >> DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dtb >> DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dtb >> DTC [C] arch/arm64/boot/dts/ti/k3-am654-base-board.dtb >> >> With this revert on top: >> >> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >> index d8182bad92de..dd753dae24c6 100644 >> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >> @@ -19,6 +19,16 @@ properties: >> - ti,am654-pcie-rc >> - ti,keystone-pcie >> >> + reg: >> + maxItems: 4 >> + >> + reg-names: >> + items: >> + - const: app >> + - const: dbics >> + - const: config >> + - const: atu > > There is nothing like that in that example. > https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44 > >> + >> interrupts: >> maxItems: 1 >> >> @@ -104,18 +114,6 @@ then: >> - msi-map >> - num-viewport >> >> -else: >> - properties: >> - reg: >> - maxItems: 4 >> - >> - reg-names: >> - items: >> - - const: app >> - - const: dbics >> - - const: config >> - - const: atu > > Neither this. > > Each case MUST be covered, look: > https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L191 Actually your case fits better another example from UFS, so take that one: https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/samsung,exynos-ufs.yaml#L39 Best regards, Krzysztof
On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > 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 optionally unless the PVU shall used for PCIe. s/optionally/optional/ s/shall used/should be/ ? (Not sure that's the sense you intend)
On 05.09.24 09:56, Krzysztof Kozlowski wrote: > On 05/09/2024 09:50, Krzysztof Kozlowski wrote: >> On 05/09/2024 09:15, Jan Kiszka wrote: >>> On 05.09.24 08:53, Krzysztof Kozlowski wrote: >>>> On 05/09/2024 08:40, Jan Kiszka wrote: >>>>> On 05.09.24 08:32, Krzysztof Kozlowski wrote: >>>>>> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote: >>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>> >>>>>>> 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 optionally unless the PVU shall used for PCIe. >>>>>>> >>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>> --- >>>>>>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org> >>>>>>> CC: "Krzysztof Wilczyński" <kw@linux.com> >>>>>>> CC: Bjorn Helgaas <bhelgaas@google.com> >>>>>>> CC: linux-pci@vger.kernel.org >>>>>>> --- >>>>>>> .../bindings/pci/ti,am65-pci-host.yaml | 52 ++++++++++++++----- >>>>>>> 1 file changed, 40 insertions(+), 12 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..d8182bad92de 100644 >>>>>>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>>>>>> @@ -19,16 +19,6 @@ properties: >>>>>>> - ti,am654-pcie-rc >>>>>>> - ti,keystone-pcie >>>>>>> >>>>>>> - reg: >>>>>>> - maxItems: 4 >>>>>>> - >>>>>>> - reg-names: >>>>>>> - items: >>>>>>> - - const: app >>>>>>> - - const: dbics >>>>>>> - - const: config >>>>>>> - - const: atu >>>>>> >>>>>> >>>>>> Nothing improved here. >>>>> >>>>> Yes, explained the background to you. Sorry, if you do not address my >>>>> replies, I'm lost with your feedback. >>>> >>>> My magic ball could not figure out the problem, so did not provide the >>>> answer. >>>> >>>> I gave you the exact code which illustrates how to do it. If you do it >>>> that way: it works. If you do it other way: it might not work. However >>> >>> The link you provided was unfortunately not self-explanatory because if >>> I - apparently - do it like that example, I'm getting the errors below. >>> >>>> without seeing anything, magic ball was silent, so I am not >>>> participating in game: would you be so kind to give more information so >>>> I won't waste my day in asking what is wrong. >>> >>> With my patch: >>> >>> # make ... dtbs_check >>> DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb >>> DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb >>> DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dtb >>> DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dtb >>> OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie.dtb >>> OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-usb3.dtb >>> DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dtb >>> DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dtb >>> DTC [C] arch/arm64/boot/dts/ti/k3-am654-base-board.dtb >>> >>> With this revert on top: >>> >>> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>> index d8182bad92de..dd753dae24c6 100644 >>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml >>> @@ -19,6 +19,16 @@ properties: >>> - ti,am654-pcie-rc >>> - ti,keystone-pcie >>> >>> + reg: >>> + maxItems: 4 >>> + >>> + reg-names: >>> + items: >>> + - const: app >>> + - const: dbics >>> + - const: config >>> + - const: atu >> >> There is nothing like that in that example. >> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44 >> >>> + >>> interrupts: >>> maxItems: 1 >>> >>> @@ -104,18 +114,6 @@ then: >>> - msi-map >>> - num-viewport >>> >>> -else: >>> - properties: >>> - reg: >>> - maxItems: 4 >>> - >>> - reg-names: >>> - items: >>> - - const: app >>> - - const: dbics >>> - - const: config >>> - - const: atu >> >> Neither this. >> >> Each case MUST be covered, look: >> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L191 > > Actually your case fits better another example from UFS, so take that one: > > https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/samsung,exynos-ufs.yaml#L39 > Via lots of trial-and-error, I think I now understood the magic behind this: You may reduce maxItems in a condition, but you cannot increase it. That's why all my intuition-based attempts failed before. Not sure if this is a specified property of the meta schema or just an oddity of the validator. Jan
On 05.09.24 09:16, Jan Kiszka wrote: > On 05.09.24 08:57, Krzysztof Kozlowski wrote: >> On 04/09/2024 12:00, Jan Kiszka wrote: >>> + >>> + reg-names: >>> + minItems: 4 >>> + items: >>> + - const: app >>> + - const: dbics >>> + - const: config >>> + - const: atu >>> + - const: vmap_lp >>> + - const: vmap_hp >>> + >>> + memory-region: >> >> This also did not improve. You did not address any feedback from v3. >> >> Missed feedback: >> >> This *must* be defined in top-level. > > Answered in the other reply. > >> I still think this must have some sort of maxItems. I accept your >> explanation that you could have multiple memory pools, but I don't think >> 2147000 pools is possible. Make it 4, 8 or 32. > > Can do - if you can explain the benefit of that. > It turned out during further investigations that swiotlb actually only allows a single restricted DMA pool per device. So this discussion auto-resolves, bindings and code will be adjusted in v5. Jan
diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml index 0a9d10532cc8..d8182bad92de 100644 --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml @@ -19,16 +19,6 @@ properties: - ti,am654-pcie-rc - ti,keystone-pcie - reg: - maxItems: 4 - - reg-names: - items: - - const: app - - const: dbics - - const: config - - const: atu - interrupts: maxItems: 1 @@ -84,12 +74,48 @@ if: enum: - ti,am654-pcie-rc then: + properties: + reg: + minItems: 4 + maxItems: 6 + + reg-names: + minItems: 4 + items: + - const: app + - const: dbics + - const: config + - const: atu + - const: vmap_lp + - const: vmap_hp + + memory-region: + minItems: 1 + description: | + phandle to one or more restricted DMA pools to be used for all devices + behind this controller. The regions should be defined according to + reserved-memory/shared-dma-pool.yaml. + items: + maxItems: 1 + required: - dma-coherent - power-domains - msi-map - num-viewport +else: + properties: + reg: + maxItems: 4 + + reg-names: + items: + - const: app + - const: dbics + - const: config + - const: atu + unevaluatedProperties: false examples: @@ -104,8 +130,10 @@ examples: reg = <0x5500000 0x1000>, <0x5501000 0x1000>, <0x10000000 0x2000>, - <0x5506000 0x1000>; - reg-names = "app", "dbics", "config", "atu"; + <0x5506000 0x1000>, + <0x2900000 0x1000>, + <0x2908000 0x1000>; + reg-names = "app", "dbics", "config", "atu", "vmap_lp", "vmap_hp"; power-domains = <&k3_pds 120 TI_SCI_PD_EXCLUSIVE>; #address-cells = <3>; #size-cells = <2>;