| Message ID | 20221114155333.234496-2-jonathanh@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add ECAM aperture for Tegra234 | expand |
On 14/11/2022 16:53, Jon Hunter wrote: > From: Vidya Sagar <vidyas@nvidia.com> > > Add support for ECAM aperture that is only supported for Tegra234 > devices. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Mon, Nov 14, 2022 at 03:53:32PM +0000, Jon Hunter wrote: > From: Vidya Sagar <vidyas@nvidia.com> > > Add support for ECAM aperture that is only supported for Tegra234 > devices. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > Changes since V2: > - Avoid duplication of reg items and reg-names > Changes since V1: > - Restricted the ECAM aperture to only Tegra234 devices that support it. > > .../bindings/pci/nvidia,tegra194-pcie.yaml | 34 +++++++++++++++++-- > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +- > 2 files changed, 33 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml > index 75da3e8eecb9..fe81d52c7277 100644 > --- a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml > +++ b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml > @@ -27,6 +27,7 @@ properties: > - nvidia,tegra234-pcie > > reg: > + minItems: 4 > items: > - description: controller's application logic registers > - description: configuration registers > @@ -35,13 +36,16 @@ properties: > available for software access. > - description: aperture where the Root Port's own configuration > registers are available. > + - description: aperture to access the configuration space through ECAM. > > reg-names: > + minItems: 4 > items: > - const: appl > - const: config > - const: atu_dma > - const: dbi > + - const: ecam Wouldn't this be mutually exclusive with 'config'? 'config' is not really h/w, but an just an iATU window typically. Where's the driver change to use this? Rob
On 15/11/2022 02:01, Rob Herring wrote: > On Mon, Nov 14, 2022 at 03:53:32PM +0000, Jon Hunter wrote: >> From: Vidya Sagar <vidyas@nvidia.com> >> >> Add support for ECAM aperture that is only supported for Tegra234 >> devices. >> >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> >> Co-developed-by: Jon Hunter <jonathanh@nvidia.com> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> Changes since V2: >> - Avoid duplication of reg items and reg-names >> Changes since V1: >> - Restricted the ECAM aperture to only Tegra234 devices that support it. >> >> .../bindings/pci/nvidia,tegra194-pcie.yaml | 34 +++++++++++++++++-- >> .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +- >> 2 files changed, 33 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml >> index 75da3e8eecb9..fe81d52c7277 100644 >> --- a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml >> +++ b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml >> @@ -27,6 +27,7 @@ properties: >> - nvidia,tegra234-pcie >> >> reg: >> + minItems: 4 >> items: >> - description: controller's application logic registers >> - description: configuration registers >> @@ -35,13 +36,16 @@ properties: >> available for software access. >> - description: aperture where the Root Port's own configuration >> registers are available. >> + - description: aperture to access the configuration space through ECAM. >> >> reg-names: >> + minItems: 4 >> items: >> - const: appl >> - const: config >> - const: atu_dma >> - const: dbi >> + - const: ecam > > Wouldn't this be mutually exclusive with 'config'? 'config' is not > really h/w, but an just an iATU window typically. Yes that is true, however, I was chatting with Sagar and we really need both ranges to be defined. > Where's the driver change to use this? For Linux there is not one. However, we need this for our port of the EDK2 bootloader [0] that parses device-tree and can support booting Linux with either device-tree or ACPI. We wanted to have a common device-tree we can use for EDK2 and Linux. Jon [0] https://github.com/NVIDIA/edk2-nvidia/wiki
On Tue, Nov 15, 2022 at 10:03 AM Jon Hunter <jonathanh@nvidia.com> wrote: > > > On 15/11/2022 02:01, Rob Herring wrote: > > On Mon, Nov 14, 2022 at 03:53:32PM +0000, Jon Hunter wrote: > >> From: Vidya Sagar <vidyas@nvidia.com> > >> > >> Add support for ECAM aperture that is only supported for Tegra234 > >> devices. > >> > >> Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > >> Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > >> --- > >> Changes since V2: > >> - Avoid duplication of reg items and reg-names > >> Changes since V1: > >> - Restricted the ECAM aperture to only Tegra234 devices that support it. > >> > >> .../bindings/pci/nvidia,tegra194-pcie.yaml | 34 +++++++++++++++++-- > >> .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +- > >> 2 files changed, 33 insertions(+), 3 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml > >> index 75da3e8eecb9..fe81d52c7277 100644 > >> --- a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml > >> +++ b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml > >> @@ -27,6 +27,7 @@ properties: > >> - nvidia,tegra234-pcie > >> > >> reg: > >> + minItems: 4 > >> items: > >> - description: controller's application logic registers > >> - description: configuration registers > >> @@ -35,13 +36,16 @@ properties: > >> available for software access. > >> - description: aperture where the Root Port's own configuration > >> registers are available. > >> + - description: aperture to access the configuration space through ECAM. > >> > >> reg-names: > >> + minItems: 4 > >> items: > >> - const: appl > >> - const: config > >> - const: atu_dma > >> - const: dbi > >> + - const: ecam > > > > Wouldn't this be mutually exclusive with 'config'? 'config' is not > > really h/w, but an just an iATU window typically. > > Yes that is true, however, I was chatting with Sagar and we really need > both ranges to be defined. > > > Where's the driver change to use this? > > For Linux there is not one. However, we need this for our port of the > EDK2 bootloader [0] that parses device-tree and can support booting > Linux with either device-tree or ACPI. We wanted to have a common > device-tree we can use for EDK2 and Linux. Ok, makes sense. Acked-by: Rob Herring <robh@kernel.org>
On Mon, Nov 14, 2022 at 03:53:32PM +0000, Jon Hunter wrote: > From: Vidya Sagar <vidyas@nvidia.com> > > Add support for ECAM aperture that is only supported for Tegra234 > devices. > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > Changes since V2: > - Avoid duplication of reg items and reg-names > Changes since V1: > - Restricted the ECAM aperture to only Tegra234 devices that support it. > > .../bindings/pci/nvidia,tegra194-pcie.yaml | 34 +++++++++++++++++-- > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +- > 2 files changed, 33 insertions(+), 3 deletions(-) Both patches applied now. Thanks, Thierry
On Thu, Nov 17, 2022 at 3:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Mon, Nov 14, 2022 at 03:53:32PM +0000, Jon Hunter wrote: > > From: Vidya Sagar <vidyas@nvidia.com> > > > > Add support for ECAM aperture that is only supported for Tegra234 > > devices. > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > > --- > > Changes since V2: > > - Avoid duplication of reg items and reg-names > > Changes since V1: > > - Restricted the ECAM aperture to only Tegra234 devices that support it. > > > > .../bindings/pci/nvidia,tegra194-pcie.yaml | 34 +++++++++++++++++-- > > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +- > > 2 files changed, 33 insertions(+), 3 deletions(-) > > Both patches applied now. linux-next now fails with this. I suspect it is due to Sergey's changes to the DWC schema. /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be fixed: 'dbi' was expected 'dbi2' was expected 'ecam' is not one of ['elbi', 'app'] 'atu' was expected 'dma' was expected 'phy' was expected 'config' was expected /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be fixed: 'ecam' is not one of ['apb', 'mgmt', 'link', 'ulreg', 'appl'] 'ecam' is not one of ['atu_dma'] 'ecam' is not one of ['smu', 'mpu'] From schema: /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml
On Mon, Dec 05, 2022 at 05:41:55PM -0600, Rob Herring wrote: > On Thu, Nov 17, 2022 at 3:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Mon, Nov 14, 2022 at 03:53:32PM +0000, Jon Hunter wrote: > > > From: Vidya Sagar <vidyas@nvidia.com> > > > > > > Add support for ECAM aperture that is only supported for Tegra234 > > > devices. > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > > > --- > > > Changes since V2: > > > - Avoid duplication of reg items and reg-names > > > Changes since V1: > > > - Restricted the ECAM aperture to only Tegra234 devices that support it. > > > > > > .../bindings/pci/nvidia,tegra194-pcie.yaml | 34 +++++++++++++++++-- > > > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +- > > > 2 files changed, 33 insertions(+), 3 deletions(-) > > > > Both patches applied now. > > linux-next now fails with this. I suspect it is due to Sergey's > changes to the DWC schema. > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > fixed: > 'dbi' was expected > 'dbi2' was expected > 'ecam' is not one of ['elbi', 'app'] > 'atu' was expected > 'dma' was expected > 'phy' was expected > 'config' was expected > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > fixed: > 'ecam' is not one of ['apb', 'mgmt', 'link', 'ulreg', 'appl'] > 'ecam' is not one of ['atu_dma'] > 'ecam' is not one of ['smu', 'mpu'] > From schema: > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml Stephen reported the other day that he wasn't able to resolve this conflict in linux-next, so he dropped the ECAM bits. The ECAM patch has now propagated to ARM SoC so it can't be easily backed out, but I guess we could revert on that tree and instead apply the patch to the DT tree and resolve the conflict there. I guess the better alternative would be to try and resolve the merge properly and let Stephen (and Linus) know. Thierry
On Tue, Dec 6, 2022 at 10:44 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Mon, Dec 05, 2022 at 05:41:55PM -0600, Rob Herring wrote: > > On Thu, Nov 17, 2022 at 3:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > On Mon, Nov 14, 2022 at 03:53:32PM +0000, Jon Hunter wrote: > > > > From: Vidya Sagar <vidyas@nvidia.com> > > > > > > > > Add support for ECAM aperture that is only supported for Tegra234 > > > > devices. > > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > > > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > > > > --- > > > > Changes since V2: > > > > - Avoid duplication of reg items and reg-names > > > > Changes since V1: > > > > - Restricted the ECAM aperture to only Tegra234 devices that support it. > > > > > > > > .../bindings/pci/nvidia,tegra194-pcie.yaml | 34 +++++++++++++++++-- > > > > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +- > > > > 2 files changed, 33 insertions(+), 3 deletions(-) > > > > > > Both patches applied now. > > > > linux-next now fails with this. I suspect it is due to Sergey's > > changes to the DWC schema. > > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > > fixed: > > 'dbi' was expected > > 'dbi2' was expected > > 'ecam' is not one of ['elbi', 'app'] > > 'atu' was expected > > 'dma' was expected > > 'phy' was expected > > 'config' was expected > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > > fixed: > > 'ecam' is not one of ['apb', 'mgmt', 'link', 'ulreg', 'appl'] > > 'ecam' is not one of ['atu_dma'] > > 'ecam' is not one of ['smu', 'mpu'] > > From schema: > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml > > Stephen reported the other day that he wasn't able to resolve this > conflict in linux-next, so he dropped the ECAM bits. The ECAM patch has > now propagated to ARM SoC so it can't be easily backed out, but I guess > we could revert on that tree and instead apply the patch to the DT tree > and resolve the conflict there. > > I guess the better alternative would be to try and resolve the merge > properly and let Stephen (and Linus) know. Instead, can you prepare a patch on top of Sergey's adding a 'oneOf' entry with 'ecam'. As this is a new thing, it should have its own entry. Then when merging, we just throw out the change from your side. I'd really prefer that bindings don't go thru the soc tree unless there's some strong reason. The default is to go via the subsystem trees. Beyond 'we are running the dtschema checks on all our dts files and can't have the warnings', I don't know what that would be. I wish everyone was doing that, but I'm pretty sure most are not. Rob
On Tue, Dec 06, 2022 at 03:14:58PM -0600, Rob Herring wrote: > On Tue, Dec 6, 2022 at 10:44 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Mon, Dec 05, 2022 at 05:41:55PM -0600, Rob Herring wrote: > > > On Thu, Nov 17, 2022 at 3:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > On Mon, Nov 14, 2022 at 03:53:32PM +0000, Jon Hunter wrote: > > > > > From: Vidya Sagar <vidyas@nvidia.com> > > > > > > > > > > Add support for ECAM aperture that is only supported for Tegra234 > > > > > devices. > > > > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > > > > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > > > > > --- > > > > > Changes since V2: > > > > > - Avoid duplication of reg items and reg-names > > > > > Changes since V1: > > > > > - Restricted the ECAM aperture to only Tegra234 devices that support it. > > > > > > > > > > .../bindings/pci/nvidia,tegra194-pcie.yaml | 34 +++++++++++++++++-- > > > > > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +- > > > > > 2 files changed, 33 insertions(+), 3 deletions(-) > > > > > > > > Both patches applied now. > > > > > > linux-next now fails with this. I suspect it is due to Sergey's > > > changes to the DWC schema. > > > > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > > > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > > > fixed: > > > 'dbi' was expected > > > 'dbi2' was expected > > > 'ecam' is not one of ['elbi', 'app'] > > > 'atu' was expected > > > 'dma' was expected > > > 'phy' was expected > > > 'config' was expected > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > > > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > > > fixed: > > > 'ecam' is not one of ['apb', 'mgmt', 'link', 'ulreg', 'appl'] > > > 'ecam' is not one of ['atu_dma'] > > > 'ecam' is not one of ['smu', 'mpu'] > > > From schema: > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml > > > > Stephen reported the other day that he wasn't able to resolve this > > conflict in linux-next, so he dropped the ECAM bits. The ECAM patch has > > now propagated to ARM SoC so it can't be easily backed out, but I guess > > we could revert on that tree and instead apply the patch to the DT tree > > and resolve the conflict there. > > > > I guess the better alternative would be to try and resolve the merge > > properly and let Stephen (and Linus) know. > > Instead, can you prepare a patch on top of Sergey's adding a 'oneOf' > entry with 'ecam'. As this is a new thing, it should have its own > entry. Then when merging, we just throw out the change from your side. > > I'd really prefer that bindings don't go thru the soc tree unless > there's some strong reason. The default is to go via the subsystem > trees. Beyond 'we are running the dtschema checks on all our dts files > and can't have the warnings', I don't know what that would be. I wish > everyone was doing that, but I'm pretty sure most are not. That's actually the reason why I wanted this to go through ARM SoC, so that the validation tools wouldn't accumulate even more errors. For some time now I've been working on getting at least Tegra to validate properly and on 64-bit ARM I have a local tree on top of linux-next that has no warnings left, though I do need to flush out quite a few bindings conversions. Until those are all upstream, that argument is a little moot and don't feel strongly about where the DT bindings ultimately end up. Thierry
On Tue, Dec 06, 2022 at 03:14:58PM -0600, Rob Herring wrote: > On Tue, Dec 6, 2022 at 10:44 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Mon, Dec 05, 2022 at 05:41:55PM -0600, Rob Herring wrote: > > > On Thu, Nov 17, 2022 at 3:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > On Mon, Nov 14, 2022 at 03:53:32PM +0000, Jon Hunter wrote: > > > > > From: Vidya Sagar <vidyas@nvidia.com> > > > > > > > > > > Add support for ECAM aperture that is only supported for Tegra234 > > > > > devices. > > > > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > > > > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > > > > > --- > > > > > Changes since V2: > > > > > - Avoid duplication of reg items and reg-names > > > > > Changes since V1: > > > > > - Restricted the ECAM aperture to only Tegra234 devices that support it. > > > > > > > > > > .../bindings/pci/nvidia,tegra194-pcie.yaml | 34 +++++++++++++++++-- > > > > > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +- > > > > > 2 files changed, 33 insertions(+), 3 deletions(-) > > > > > > > > Both patches applied now. > > > > > > linux-next now fails with this. I suspect it is due to Sergey's > > > changes to the DWC schema. > > > > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > > > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > > > fixed: > > > 'dbi' was expected > > > 'dbi2' was expected > > > 'ecam' is not one of ['elbi', 'app'] > > > 'atu' was expected > > > 'dma' was expected > > > 'phy' was expected > > > 'config' was expected > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > > > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > > > fixed: > > > 'ecam' is not one of ['apb', 'mgmt', 'link', 'ulreg', 'appl'] > > > 'ecam' is not one of ['atu_dma'] > > > 'ecam' is not one of ['smu', 'mpu'] > > > From schema: > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml > > > > Stephen reported the other day that he wasn't able to resolve this > > conflict in linux-next, so he dropped the ECAM bits. The ECAM patch has > > now propagated to ARM SoC so it can't be easily backed out, but I guess > > we could revert on that tree and instead apply the patch to the DT tree > > and resolve the conflict there. > > > > I guess the better alternative would be to try and resolve the merge > > properly and let Stephen (and Linus) know. > > Instead, can you prepare a patch on top of Sergey's adding a 'oneOf' > entry with 'ecam'. As this is a new thing, it should have its own > entry. Then when merging, we just throw out the change from your side. Right, the only change that is required here is to extend the reg-names oneOf list of the DT-bindings: < Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml with the 'ecam' entry. If it's a vendor-specific part then add to the last the last entry defines the vendor-specific duplicates of the generic CSR spaces. On the other hand I don't really see a reason in adding the ECAM CSRs space to the generic DW PCIe device since basically the ECAM memory is just a pre-configured outbound iATU window. So if it's a ECAM-based device then it should have been already configured by the system bootloader upon the kernel boot up. Thus there is no point in having the generic DW PCIe resources and it should be just a generic ECAM-based device with a single ECAM CSR space as the "snps,dw-pcie-ecam"/"pci-host-ecam-generic" DT-bindings require especially seeing the Nvidia low-level driver doesn't use the ECAM registers at all. Moreover the DW PCIe core driver doesn't differentiate between the already configured iATU windows and the one available for the ranges-based mapping. Instead the DW PCIe core just disables all the detected in- and outbound iATUs by means of the dw_pcie_iatu_setup() method. So the pre-configured ECAM space will be reset by the driver core anyway. What I would suggest here is to split up the devices: 1. If it's a ECAM-based device, then it should be identified as "snps,dw-pcie-ecam"/"pci-host-ecam-generic", then it will have a single ECAM CSR space with no need in other resources. 2. If it's a DW PCIe device with Nvidia Tegra194-specific settings, then it should be identified as "nvidia,tegra194-pcie" with no ECAM registers. Thus we wouldn't need to have any modifications applied to the bindings and to the DT-files. What do you think? -Serge(y) > > I'd really prefer that bindings don't go thru the soc tree unless > there's some strong reason. The default is to go via the subsystem > trees. Beyond 'we are running the dtschema checks on all our dts files > and can't have the warnings', I don't know what that would be. I wish > everyone was doing that, but I'm pretty sure most are not. > > Rob >
On Fri, Dec 9, 2022 at 4:17 AM Serge Semin <Sergey.Semin@baikalelectronics.ru> wrote: > > On Tue, Dec 06, 2022 at 03:14:58PM -0600, Rob Herring wrote: > > On Tue, Dec 6, 2022 at 10:44 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > On Mon, Dec 05, 2022 at 05:41:55PM -0600, Rob Herring wrote: > > > > On Thu, Nov 17, 2022 at 3:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > > > On Mon, Nov 14, 2022 at 03:53:32PM +0000, Jon Hunter wrote: > > > > > > From: Vidya Sagar <vidyas@nvidia.com> > > > > > > > > > > > > Add support for ECAM aperture that is only supported for Tegra234 > > > > > > devices. > > > > > > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > > > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > > > > > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > > > > > > --- > > > > > > Changes since V2: > > > > > > - Avoid duplication of reg items and reg-names > > > > > > Changes since V1: > > > > > > - Restricted the ECAM aperture to only Tegra234 devices that support it. > > > > > > > > > > > > .../bindings/pci/nvidia,tegra194-pcie.yaml | 34 +++++++++++++++++-- > > > > > > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +- > > > > > > 2 files changed, 33 insertions(+), 3 deletions(-) > > > > > > > > > > Both patches applied now. > > > > > > > > linux-next now fails with this. I suspect it is due to Sergey's > > > > changes to the DWC schema. > > > > > > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > > > > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > > > > fixed: > > > > 'dbi' was expected > > > > 'dbi2' was expected > > > > 'ecam' is not one of ['elbi', 'app'] > > > > 'atu' was expected > > > > 'dma' was expected > > > > 'phy' was expected > > > > 'config' was expected > > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > > > > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > > > > fixed: > > > > 'ecam' is not one of ['apb', 'mgmt', 'link', 'ulreg', 'appl'] > > > > 'ecam' is not one of ['atu_dma'] > > > > 'ecam' is not one of ['smu', 'mpu'] > > > > From schema: > > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml > > > > > > Stephen reported the other day that he wasn't able to resolve this > > > conflict in linux-next, so he dropped the ECAM bits. The ECAM patch has > > > now propagated to ARM SoC so it can't be easily backed out, but I guess > > > we could revert on that tree and instead apply the patch to the DT tree > > > and resolve the conflict there. > > > > > > I guess the better alternative would be to try and resolve the merge > > > properly and let Stephen (and Linus) know. > > > > > Instead, can you prepare a patch on top of Sergey's adding a 'oneOf' > > entry with 'ecam'. As this is a new thing, it should have its own > > entry. Then when merging, we just throw out the change from your side. > > Right, the only change that is required here is to extend the > reg-names oneOf list of the DT-bindings: > < Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > with the 'ecam' entry. If it's a vendor-specific part then add to the > last the last entry defines the vendor-specific duplicates of the generic CSR > spaces. > > On the other hand I don't really see a reason in adding the ECAM CSRs > space to the generic DW PCIe device since basically the ECAM memory is > just a pre-configured outbound iATU window. So if it's a ECAM-based > device then it should have been already configured by the system > bootloader upon the kernel boot up. Thus there is no point in having > the generic DW PCIe resources and it should be just a generic > ECAM-based device with a single ECAM CSR space as the > "snps,dw-pcie-ecam"/"pci-host-ecam-generic" DT-bindings require > especially seeing the Nvidia low-level driver doesn't use the ECAM > registers at all. Moreover the DW PCIe core driver doesn't > differentiate between the already configured iATU windows and the one > available for the ranges-based mapping. Instead the DW PCIe core just > disables all the detected in- and outbound iATUs by means of the > dw_pcie_iatu_setup() method. So the pre-configured ECAM space will be > reset by the driver core anyway. This was discussed some before. This is for the firmware/bootloader to setup ECAM mode. Then the kernel will see generic (ACPI) ECAM. Yes, it is iATU config, but so is 'config'. If we were starting over, I'd say 'reg' should just have the entire address space for iATU and the driver could figure out how to configure it (beyond what ranges says). But that ship has sailed. Also, note that the address range here is disjoint from 'config', so it looks like we'd need 2 entries anyways. Rob
On Fri, Dec 09, 2022 at 08:29:52AM -0600, Rob Herring wrote: > On Fri, Dec 9, 2022 at 4:17 AM Serge Semin > <Sergey.Semin@baikalelectronics.ru> wrote: > > > > On Tue, Dec 06, 2022 at 03:14:58PM -0600, Rob Herring wrote: > > > On Tue, Dec 6, 2022 at 10:44 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > On Mon, Dec 05, 2022 at 05:41:55PM -0600, Rob Herring wrote: > > > > > On Thu, Nov 17, 2022 at 3:38 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > > > > > On Mon, Nov 14, 2022 at 03:53:32PM +0000, Jon Hunter wrote: > > > > > > > From: Vidya Sagar <vidyas@nvidia.com> > > > > > > > > > > > > > > Add support for ECAM aperture that is only supported for Tegra234 > > > > > > > devices. > > > > > > > > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com> > > > > > > > Co-developed-by: Jon Hunter <jonathanh@nvidia.com> > > > > > > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > > > > > > > --- > > > > > > > Changes since V2: > > > > > > > - Avoid duplication of reg items and reg-names > > > > > > > Changes since V1: > > > > > > > - Restricted the ECAM aperture to only Tegra234 devices that support it. > > > > > > > > > > > > > > .../bindings/pci/nvidia,tegra194-pcie.yaml | 34 +++++++++++++++++-- > > > > > > > .../devicetree/bindings/pci/snps,dw-pcie.yaml | 2 +- > > > > > > > 2 files changed, 33 insertions(+), 3 deletions(-) > > > > > > > > > > > > Both patches applied now. > > > > > > > > > > linux-next now fails with this. I suspect it is due to Sergey's > > > > > changes to the DWC schema. > > > > > > > > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > > > > > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > > > > > fixed: > > > > > 'dbi' was expected > > > > > 'dbi2' was expected > > > > > 'ecam' is not one of ['elbi', 'app'] > > > > > 'atu' was expected > > > > > 'dma' was expected > > > > > 'phy' was expected > > > > > 'config' was expected > > > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.example.dtb: > > > > > pcie@14160000: reg-names:4: 'oneOf' conditional failed, one must be > > > > > fixed: > > > > > 'ecam' is not one of ['apb', 'mgmt', 'link', 'ulreg', 'appl'] > > > > > 'ecam' is not one of ['atu_dma'] > > > > > 'ecam' is not one of ['smu', 'mpu'] > > > > > From schema: > > > > > /builds/robherring/linux-dt/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml > > > > > > > > Stephen reported the other day that he wasn't able to resolve this > > > > conflict in linux-next, so he dropped the ECAM bits. The ECAM patch has > > > > now propagated to ARM SoC so it can't be easily backed out, but I guess > > > > we could revert on that tree and instead apply the patch to the DT tree > > > > and resolve the conflict there. > > > > > > > > I guess the better alternative would be to try and resolve the merge > > > > properly and let Stephen (and Linus) know. > > > > > > > > Instead, can you prepare a patch on top of Sergey's adding a 'oneOf' > > > entry with 'ecam'. As this is a new thing, it should have its own > > > entry. Then when merging, we just throw out the change from your side. > > > > Right, the only change that is required here is to extend the > > reg-names oneOf list of the DT-bindings: > > < Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml > > with the 'ecam' entry. If it's a vendor-specific part then add to the > > last the last entry defines the vendor-specific duplicates of the generic CSR > > spaces. > > > > On the other hand I don't really see a reason in adding the ECAM CSRs > > space to the generic DW PCIe device since basically the ECAM memory is > > just a pre-configured outbound iATU window. So if it's a ECAM-based > > device then it should have been already configured by the system > > bootloader upon the kernel boot up. Thus there is no point in having > > the generic DW PCIe resources and it should be just a generic > > ECAM-based device with a single ECAM CSR space as the > > "snps,dw-pcie-ecam"/"pci-host-ecam-generic" DT-bindings require > > especially seeing the Nvidia low-level driver doesn't use the ECAM > > registers at all. Moreover the DW PCIe core driver doesn't > > differentiate between the already configured iATU windows and the one > > available for the ranges-based mapping. Instead the DW PCIe core just > > disables all the detected in- and outbound iATUs by means of the > > dw_pcie_iatu_setup() method. So the pre-configured ECAM space will be > > reset by the driver core anyway. > > This was discussed some before. This is for the firmware/bootloader to > setup ECAM mode. Then the kernel will see generic (ACPI) ECAM. > > Yes, it is iATU config, but so is 'config'. If we were starting over, > I'd say 'reg' should just have the entire address space for iATU and > the driver could figure out how to configure it (beyond what ranges > says). But that ship has sailed. Also, note that the address range > here is disjoint from 'config', so it looks like we'd need 2 entries > anyways. Thanks for the explanation. I've got another suggestion them. The semantics of the 'config' reg-space and the ECAM reg-space is very similar. The only difference is the way the corresponding outbound iATU window is configured. The DW PCIe driver just maps it in accordance with the requested peripheral device BDF value so the very first 4KB is translated to the CFG TLPs sent out to that device. The same space could be mapped with the IATU_REGION_CTRL_2_OFF_OUTBOUND_i.CFG_SHIFT_MODE flag set. So the entire PCIe BDFs space will be available via the same MMIO region. Thus the corresponding peripheral device will be available at the BDF-based offset with respect to the 'config' reg-space base address. Of course in the later case the 'config' MMIO range must be much greater (256MB) than in the former case (4KB), but still the difference in just the way the window is configured. Moreover the DW PCIe driver could be easier fixed to using the later approach if the 'config' reg-space is specified wide enough to map the entire PCIe bus CFG-space (256MB). Based on that and on the fact you said that the ECAM reg-space will be used by the bootloader/firmware only anyway, I'd suggest to just extend the 'config' reg-space semantics. So the bootloader/firmware will be able to use it to create a generic ECAM device. The kernel DW PCIe driver will by default use it to remap each peripheral device CFG-space on request, but at some point the driver could be converted to map the entire PCIe BDFs similarly to the ECAM space. Thus we won't need to add a redundant ECAM reg-space. What do you think? * hopefully it isn't too late for the suggested approach. -Serge(y) > > Rob
diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml index 75da3e8eecb9..fe81d52c7277 100644 --- a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml +++ b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml @@ -27,6 +27,7 @@ properties: - nvidia,tegra234-pcie reg: + minItems: 4 items: - description: controller's application logic registers - description: configuration registers @@ -35,13 +36,16 @@ properties: available for software access. - description: aperture where the Root Port's own configuration registers are available. + - description: aperture to access the configuration space through ECAM. reg-names: + minItems: 4 items: - const: appl - const: config - const: atu_dma - const: dbi + - const: ecam interrupts: items: @@ -202,6 +206,31 @@ properties: allOf: - $ref: /schemas/pci/snps,dw-pcie.yaml# + - if: + properties: + compatible: + contains: + enum: + - nvidia,tegra194-pcie + then: + properties: + reg: + maxItems: 4 + reg-names: + maxItems: 4 + + - if: + properties: + compatible: + contains: + enum: + - nvidia,tegra234-pcie + then: + properties: + reg: + minItems: 5 + reg-names: + minItems: 5 unevaluatedProperties: false @@ -305,8 +334,9 @@ examples: reg = <0x00 0x14160000 0x0 0x00020000>, /* appl registers (128K) */ <0x00 0x36000000 0x0 0x00040000>, /* configuration space (256K) */ <0x00 0x36040000 0x0 0x00040000>, /* iATU_DMA reg space (256K) */ - <0x00 0x36080000 0x0 0x00040000>; /* DBI reg space (256K) */ - reg-names = "appl", "config", "atu_dma", "dbi"; + <0x00 0x36080000 0x0 0x00040000>, /* DBI reg space (256K) */ + <0x24 0x30000000 0x0 0x10000000>; /* ECAM (256MB) */ + reg-names = "appl", "config", "atu_dma", "dbi", "ecam"; #address-cells = <3>; #size-cells = <2>; diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml index 7287d395e1b6..7e0b015f1414 100644 --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml @@ -35,7 +35,7 @@ properties: maxItems: 5 items: enum: [ dbi, dbi2, config, atu, atu_dma, app, appl, elbi, mgmt, ctrl, - parf, cfg, link, ulreg, smu, mpu, apb, phy ] + parf, cfg, link, ulreg, smu, mpu, apb, phy, ecam ] num-lanes: description: |