| Message ID | 20210520090123.11814-1-jonathanh@nvidia.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [V2] PCI: tegra: Fix building Tegra194 PCIe driver | expand |
Hi Jon, > Commit 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM > errata") caused a few build regressions for the Tegra194 PCIe driver > which are: > > 1. The Tegra194 PCIe driver can no longer be built as a module. This > was caused by removing the Makefile entry to build the pcie-tegra.c > based upon the CONFIG_PCIE_TEGRA194 option. Therefore, restore this > so that we can build the driver as a module if ACPI support is not > enabled in the kernel. > 2. If CONFIG_PCIE_TEGRA194 is configured to build the driver as a > module, at the same time that CONFIG_ACPI and CONFIG_PCI_QUIRKS are > selected to build the driver into the kernel, then the necessary > functions in the driver to probe and remove the device when booting > with device-tree and not compiled into to the driver. This prevents > the PCIe devices being probed when booting with device-tree. Fix this > by using the IS_ENABLED() macro. > 3. The below build warnings to be seen with particular kernel > configurations. Fix these by adding the necessary guards around these > variable definitions. > > drivers/pci/controller/dwc/pcie-tegra194.c:259:18: warning: > ‘event_cntr_data_offset’ defined but not used [-Wunused-const-variable=] > drivers/pci/controller/dwc/pcie-tegra194.c:250:18: warning: > ‘event_cntr_ctrl_offset’ defined but not used [-Wunused-const-variable=] > drivers/pci/controller/dwc/pcie-tegra194.c:243:27: warning: > ‘pcie_gen_freq’ defined but not used [-Wunused-const-variable=] > > Fixes: 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM errata") Thank you for fixing this! Much appreciated! [...] > +#if IS_ENABLED(CONFIG_PCIE_TEGRA194) > static const unsigned int pcie_gen_freq[] = { > GEN1_CORE_CLK_FREQ, > GEN2_CORE_CLK_FREQ, > GEN3_CORE_CLK_FREQ, > GEN4_CORE_CLK_FREQ > }; > +#endif > > +#if defined(CONFIG_PCIEASPM) > static const u32 event_cntr_ctrl_offset[] = { > 0x1d8, > 0x1a8, > @@ -264,6 +267,7 @@ static const u32 event_cntr_data_offset[] = { > 0x1c8, > 0x1dc > }; > +#endif A small note about the above - I believe Bjorn might prefer if we move these two into the existing blocks, rather than wrapping both as done here, so that things are grouped together. Albeit, I leave it to Bjorn to give us feedback on his preferred style. Reviewed-by: Krzysztof Wilczyński <kw@linux.com> Krzysztof
On Thu, May 20, 2021 at 10:01:23AM +0100, Jon Hunter wrote: > Commit 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM > errata") caused a few build regressions for the Tegra194 PCIe driver > which are: > > 1. The Tegra194 PCIe driver can no longer be built as a module. This > was caused by removing the Makefile entry to build the pcie-tegra.c > based upon the CONFIG_PCIE_TEGRA194 option. Therefore, restore this > so that we can build the driver as a module if ACPI support is not > enabled in the kernel. I'm not sure what "if ACPI support is not enabled in the kernel" is telling me. Does it mean that we can only build tegra194 as a module if ACPI is not enabled? I don't think so (at least, I don't think Kconfig enforces that). Should the "if ACPI support is not enabled ..." part just be dropped? I assume it should be possible to build the kernel with ACPI enabled and with pcie-tegra194 as a module? > 2. If CONFIG_PCIE_TEGRA194 is configured to build the driver as a > module, at the same time that CONFIG_ACPI and CONFIG_PCI_QUIRKS are > selected to build the driver into the kernel, then the necessary > functions in the driver to probe and remove the device when booting > with device-tree and not compiled into to the driver. This prevents > the PCIe devices being probed when booting with device-tree. Fix this > by using the IS_ENABLED() macro. The #ifdef vs IS_ENABLED() difference is kind of subtle and I have to figure it out every time. Maybe something like this? 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM errata") added "#ifdef CONFIG_PCIE_TEGRA194" around the native driver. But if we set CONFIG_PCIE_TEGRA194=m to build the driver as a module, autoconf.h contains "#define CONFIG_PCIE_TEGRA194_MODULE 1" (not "#define CONFIG_PCIE_TEGRA194 1"), so the #ifdef excludes the driver. Instead, use "IS_ENABLED(CONFIG_PCIE_TEGRA194)", which checks for either CONFIG_PCIE_TEGRA194 or CONFIG_PCIE_TEGRA194_MODULE. > 3. The below build warnings to be seen with particular kernel > configurations. Fix these by adding the necessary guards around these > variable definitions. > > drivers/pci/controller/dwc/pcie-tegra194.c:259:18: warning: > ‘event_cntr_data_offset’ defined but not used [-Wunused-const-variable=] > drivers/pci/controller/dwc/pcie-tegra194.c:250:18: warning: > ‘event_cntr_ctrl_offset’ defined but not used [-Wunused-const-variable=] > drivers/pci/controller/dwc/pcie-tegra194.c:243:27: warning: > ‘pcie_gen_freq’ defined but not used [-Wunused-const-variable=] > > Fixes: 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM errata") > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> This is a candidate for v5.13, since we merged 7f100744749e for v5.13-rc1. > --- > drivers/pci/controller/dwc/Makefile | 1 + > drivers/pci/controller/dwc/pcie-tegra194.c | 6 +++++- > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile > index eca805c1a023..f0d1e2d8c022 100644 > --- a/drivers/pci/controller/dwc/Makefile > +++ b/drivers/pci/controller/dwc/Makefile > @@ -32,6 +32,7 @@ obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o > # depending on whether ACPI, the DT driver, or both are enabled. > > obj-$(CONFIG_PCIE_AL) += pcie-al.o > +obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o > obj-$(CONFIG_PCI_HISI) += pcie-hisi.o It sounds like the interesting case is this: CONFIG_ARM64=y CONFIG_ACPI=y CONFIG_PCI_QUIRKS=y CONFIG_PCIE_TEGRA194=m I don't know how this works in this case: obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o obj-$(CONFIG_ARM64) += pcie-tegra194.o We want tegra194_acpi_init() and the rest of the ECAM quirk to be compiled into the static kernel. And we want tegra_pcie_dw_probe(), tegra_pcie_dw_remove(), etc, compiled into a module. Does kbuild really compile pcie-tegra194.c twice? And if so, it's not a problem that both the static kernel and the module contain a tegra194_pcie_ops symbol? > ifdef CONFIG_ACPI > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > index b19775ab134e..ae70e53a7826 100644 > --- a/drivers/pci/controller/dwc/pcie-tegra194.c > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > @@ -240,13 +240,16 @@ > #define EP_STATE_DISABLED 0 > #define EP_STATE_ENABLED 1 > > +#if IS_ENABLED(CONFIG_PCIE_TEGRA194) > static const unsigned int pcie_gen_freq[] = { > GEN1_CORE_CLK_FREQ, > GEN2_CORE_CLK_FREQ, > GEN3_CORE_CLK_FREQ, > GEN4_CORE_CLK_FREQ > }; > +#endif This makes the minimal patch, but as Krzysztof suggests, I would prefer to move the whole struct so it's just inside the CONFIG_PCIE_TEGRA194 #ifdef. > +#if defined(CONFIG_PCIEASPM) > static const u32 event_cntr_ctrl_offset[] = { > 0x1d8, > 0x1a8, > @@ -264,6 +267,7 @@ static const u32 event_cntr_data_offset[] = { > 0x1c8, > 0x1dc > }; > +#endif Similar for the CONFIG_PCIEASPM #ifdef. > struct tegra_pcie_dw { > struct device *dev; > @@ -409,7 +413,7 @@ const struct pci_ecam_ops tegra194_pcie_ops = { > }; > #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */ > > -#ifdef CONFIG_PCIE_TEGRA194 > +#if IS_ENABLED(CONFIG_PCIE_TEGRA194) > static inline struct tegra_pcie_dw *to_tegra_pcie(struct dw_pcie *pci) > { > -- > 2.25.1 >
On 20/05/2021 23:19, Bjorn Helgaas wrote: > On Thu, May 20, 2021 at 10:01:23AM +0100, Jon Hunter wrote: >> Commit 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM >> errata") caused a few build regressions for the Tegra194 PCIe driver >> which are: >> >> 1. The Tegra194 PCIe driver can no longer be built as a module. This >> was caused by removing the Makefile entry to build the pcie-tegra.c >> based upon the CONFIG_PCIE_TEGRA194 option. Therefore, restore this >> so that we can build the driver as a module if ACPI support is not >> enabled in the kernel. > > I'm not sure what "if ACPI support is not enabled in the kernel" is > telling me. Does it mean that we can only build tegra194 as a module > if ACPI is not enabled? I don't think so (at least, I don't think > Kconfig enforces that). If ACPI is enabled, then we will build the driver into the kernel. If we have ... CONFIG_ACPI=y CONFIG_PCIE_TEGRA194=m FWICS the pcie-tegra194.c driver is builtin to the kernel. > Should the "if ACPI support is not enabled ..." part just be dropped? > > I assume it should be possible to build the kernel with ACPI enabled > and with pcie-tegra194 as a module? Per the above that does not appear to be possible. >> 2. If CONFIG_PCIE_TEGRA194 is configured to build the driver as a >> module, at the same time that CONFIG_ACPI and CONFIG_PCI_QUIRKS are >> selected to build the driver into the kernel, then the necessary >> functions in the driver to probe and remove the device when booting >> with device-tree and not compiled into to the driver. This prevents >> the PCIe devices being probed when booting with device-tree. Fix this >> by using the IS_ENABLED() macro. > > The #ifdef vs IS_ENABLED() difference is kind of subtle and I have to > figure it out every time. Maybe something like this? > > 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM > errata") added "#ifdef CONFIG_PCIE_TEGRA194" around the native > driver. > > But if we set CONFIG_PCIE_TEGRA194=m to build the driver as a > module, autoconf.h contains "#define CONFIG_PCIE_TEGRA194_MODULE 1" > (not "#define CONFIG_PCIE_TEGRA194 1"), so the #ifdef excludes the > driver. > > Instead, use "IS_ENABLED(CONFIG_PCIE_TEGRA194)", which checks for > either CONFIG_PCIE_TEGRA194 or CONFIG_PCIE_TEGRA194_MODULE. OK sounds good. Thanks >> 3. The below build warnings to be seen with particular kernel >> configurations. Fix these by adding the necessary guards around these >> variable definitions. >> >> drivers/pci/controller/dwc/pcie-tegra194.c:259:18: warning: >> ‘event_cntr_data_offset’ defined but not used [-Wunused-const-variable=] >> drivers/pci/controller/dwc/pcie-tegra194.c:250:18: warning: >> ‘event_cntr_ctrl_offset’ defined but not used [-Wunused-const-variable=] >> drivers/pci/controller/dwc/pcie-tegra194.c:243:27: warning: >> ‘pcie_gen_freq’ defined but not used [-Wunused-const-variable=] >> >> Fixes: 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM errata") >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > > This is a candidate for v5.13, since we merged 7f100744749e for > v5.13-rc1. Yes we need to fix for v5.13. >> --- >> drivers/pci/controller/dwc/Makefile | 1 + >> drivers/pci/controller/dwc/pcie-tegra194.c | 6 +++++- >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile >> index eca805c1a023..f0d1e2d8c022 100644 >> --- a/drivers/pci/controller/dwc/Makefile >> +++ b/drivers/pci/controller/dwc/Makefile >> @@ -32,6 +32,7 @@ obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o >> # depending on whether ACPI, the DT driver, or both are enabled. >> >> obj-$(CONFIG_PCIE_AL) += pcie-al.o >> +obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o >> obj-$(CONFIG_PCI_HISI) += pcie-hisi.o > > It sounds like the interesting case is this: > > CONFIG_ARM64=y > CONFIG_ACPI=y > CONFIG_PCI_QUIRKS=y > CONFIG_PCIE_TEGRA194=m > > I don't know how this works in this case: > > obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o > obj-$(CONFIG_ARM64) += pcie-tegra194.o > > We want tegra194_acpi_init() and the rest of the ECAM quirk to be > compiled into the static kernel. And we want tegra_pcie_dw_probe(), > tegra_pcie_dw_remove(), etc, compiled into a module. > > Does kbuild really compile pcie-tegra194.c twice? And if so, it's not > a problem that both the static kernel and the module contain a > tegra194_pcie_ops symbol? FWICT it does not compile it twice and I only see it builtin. We the above I don't see any module generated. >> ifdef CONFIG_ACPI >> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c >> index b19775ab134e..ae70e53a7826 100644 >> --- a/drivers/pci/controller/dwc/pcie-tegra194.c >> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c >> @@ -240,13 +240,16 @@ >> #define EP_STATE_DISABLED 0 >> #define EP_STATE_ENABLED 1 >> >> +#if IS_ENABLED(CONFIG_PCIE_TEGRA194) >> static const unsigned int pcie_gen_freq[] = { >> GEN1_CORE_CLK_FREQ, >> GEN2_CORE_CLK_FREQ, >> GEN3_CORE_CLK_FREQ, >> GEN4_CORE_CLK_FREQ >> }; >> +#endif > > This makes the minimal patch, but as Krzysztof suggests, I would > prefer to move the whole struct so it's just inside the > CONFIG_PCIE_TEGRA194 #ifdef. OK, will do. >> +#if defined(CONFIG_PCIEASPM) >> static const u32 event_cntr_ctrl_offset[] = { >> 0x1d8, >> 0x1a8, >> @@ -264,6 +267,7 @@ static const u32 event_cntr_data_offset[] = { >> 0x1c8, >> 0x1dc >> }; >> +#endif > > Similar for the CONFIG_PCIEASPM #ifdef. OK. Thanks Jon
On Fri, May 21, 2021 at 02:11:15PM +0100, Jon Hunter wrote: > > On 20/05/2021 23:19, Bjorn Helgaas wrote: > > On Thu, May 20, 2021 at 10:01:23AM +0100, Jon Hunter wrote: > >> Commit 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM > >> errata") caused a few build regressions for the Tegra194 PCIe driver > >> which are: > >> > >> 1. The Tegra194 PCIe driver can no longer be built as a module. This > >> was caused by removing the Makefile entry to build the pcie-tegra.c > >> based upon the CONFIG_PCIE_TEGRA194 option. Therefore, restore this > >> so that we can build the driver as a module if ACPI support is not > >> enabled in the kernel. > > > > I'm not sure what "if ACPI support is not enabled in the kernel" is > > telling me. Does it mean that we can only build tegra194 as a module > > if ACPI is not enabled? I don't think so (at least, I don't think > > Kconfig enforces that). > > If ACPI is enabled, then we will build the driver into the kernel. If we > have ... > > CONFIG_ACPI=y > CONFIG_PCIE_TEGRA194=m > > FWICS the pcie-tegra194.c driver is builtin to the kernel. My understanding is that we want pcie-tegra194.c to be: - Built into the kernel when CONFIG_PCIE_TEGRA194=m or =y and CONFIG_ACPI=y and CONFIG_PCI_QUIRKS=y. If we're using the ACPI pci_root.c driver, we must have the MCFG quirk built-in, and this case worked as I expected (this is on x86): $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config CONFIG_ACPI=y CONFIG_PCI_QUIRKS=y CONFIG_PCIE_TEGRA194=y CONFIG_PCIE_TEGRA194_HOST=m CONFIG_PCIE_TEGRA194_EP=y $ rm drivers/pci/controller/dwc/pcie-tegra194.*o $ make drivers/pci/controller/dwc/ ... CC drivers/pci/controller/dwc/pcie-tegra194.o AR drivers/pci/controller/dwc/built-in.a - Built as a module when CONFIG_PCIE_TEGRA194=m and CONFIG_ACPI is not set. In this case, we're not using the ACPI pci_root.c driver, and we don't need the MCFG quirk built-in, so it should be OK to build a module, and IIUC this patch is supposed to *allow* that. But in my testing, it did *not* build a module. Am I missing something? $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config # CONFIG_ACPI is not set # CONFIG_PCI_QUIRKS is not set CONFIG_PCIE_TEGRA194=y CONFIG_PCIE_TEGRA194_HOST=m CONFIG_PCIE_TEGRA194_EP=y $ rm drivers/pci/controller/dwc/pcie-tegra194.*o $ make drivers/pci/controller/dwc/ ... CC drivers/pci/controller/dwc/pcie-tegra194.o AR drivers/pci/controller/dwc/built-in.a This was all done with V3 of the patch, but I'm responding to V2 to continue the conversation there since I don't think this part changed. > > Should the "if ACPI support is not enabled ..." part just be dropped? > > > > I assume it should be possible to build the kernel with ACPI enabled > > and with pcie-tegra194 as a module? > > Per the above that does not appear to be possible. > > >> 2. If CONFIG_PCIE_TEGRA194 is configured to build the driver as a > >> module, at the same time that CONFIG_ACPI and CONFIG_PCI_QUIRKS are > >> selected to build the driver into the kernel, then the necessary > >> functions in the driver to probe and remove the device when booting > >> with device-tree and not compiled into to the driver. This prevents > >> the PCIe devices being probed when booting with device-tree. Fix this > >> by using the IS_ENABLED() macro. > > > > The #ifdef vs IS_ENABLED() difference is kind of subtle and I have to > > figure it out every time. Maybe something like this? > > > > 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM > > errata") added "#ifdef CONFIG_PCIE_TEGRA194" around the native > > driver. > > > > But if we set CONFIG_PCIE_TEGRA194=m to build the driver as a > > module, autoconf.h contains "#define CONFIG_PCIE_TEGRA194_MODULE 1" > > (not "#define CONFIG_PCIE_TEGRA194 1"), so the #ifdef excludes the > > driver. > > > > Instead, use "IS_ENABLED(CONFIG_PCIE_TEGRA194)", which checks for > > either CONFIG_PCIE_TEGRA194 or CONFIG_PCIE_TEGRA194_MODULE. > > OK sounds good. Thanks > > >> 3. The below build warnings to be seen with particular kernel > >> configurations. Fix these by adding the necessary guards around these > >> variable definitions. > >> > >> drivers/pci/controller/dwc/pcie-tegra194.c:259:18: warning: > >> ‘event_cntr_data_offset’ defined but not used [-Wunused-const-variable=] > >> drivers/pci/controller/dwc/pcie-tegra194.c:250:18: warning: > >> ‘event_cntr_ctrl_offset’ defined but not used [-Wunused-const-variable=] > >> drivers/pci/controller/dwc/pcie-tegra194.c:243:27: warning: > >> ‘pcie_gen_freq’ defined but not used [-Wunused-const-variable=] > >> > >> Fixes: 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM errata") > >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > > > > This is a candidate for v5.13, since we merged 7f100744749e for > > v5.13-rc1. > > Yes we need to fix for v5.13. > > >> --- > >> drivers/pci/controller/dwc/Makefile | 1 + > >> drivers/pci/controller/dwc/pcie-tegra194.c | 6 +++++- > >> 2 files changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile > >> index eca805c1a023..f0d1e2d8c022 100644 > >> --- a/drivers/pci/controller/dwc/Makefile > >> +++ b/drivers/pci/controller/dwc/Makefile > >> @@ -32,6 +32,7 @@ obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o > >> # depending on whether ACPI, the DT driver, or both are enabled. > >> > >> obj-$(CONFIG_PCIE_AL) += pcie-al.o > >> +obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o > >> obj-$(CONFIG_PCI_HISI) += pcie-hisi.o > > > > It sounds like the interesting case is this: > > > > CONFIG_ARM64=y > > CONFIG_ACPI=y > > CONFIG_PCI_QUIRKS=y > > CONFIG_PCIE_TEGRA194=m > > > > I don't know how this works in this case: > > > > obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o > > obj-$(CONFIG_ARM64) += pcie-tegra194.o > > > > We want tegra194_acpi_init() and the rest of the ECAM quirk to be > > compiled into the static kernel. And we want tegra_pcie_dw_probe(), > > tegra_pcie_dw_remove(), etc, compiled into a module. > > > > Does kbuild really compile pcie-tegra194.c twice? And if so, it's not > > a problem that both the static kernel and the module contain a > > tegra194_pcie_ops symbol? > > FWICT it does not compile it twice and I only see it builtin. We the > above I don't see any module generated. > > >> ifdef CONFIG_ACPI > >> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c > >> index b19775ab134e..ae70e53a7826 100644 > >> --- a/drivers/pci/controller/dwc/pcie-tegra194.c > >> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c > >> @@ -240,13 +240,16 @@ > >> #define EP_STATE_DISABLED 0 > >> #define EP_STATE_ENABLED 1 > >> > >> +#if IS_ENABLED(CONFIG_PCIE_TEGRA194) > >> static const unsigned int pcie_gen_freq[] = { > >> GEN1_CORE_CLK_FREQ, > >> GEN2_CORE_CLK_FREQ, > >> GEN3_CORE_CLK_FREQ, > >> GEN4_CORE_CLK_FREQ > >> }; > >> +#endif > > > > This makes the minimal patch, but as Krzysztof suggests, I would > > prefer to move the whole struct so it's just inside the > > CONFIG_PCIE_TEGRA194 #ifdef. > > OK, will do. > > >> +#if defined(CONFIG_PCIEASPM) > >> static const u32 event_cntr_ctrl_offset[] = { > >> 0x1d8, > >> 0x1a8, > >> @@ -264,6 +267,7 @@ static const u32 event_cntr_data_offset[] = { > >> 0x1c8, > >> 0x1dc > >> }; > >> +#endif > > > > Similar for the CONFIG_PCIEASPM #ifdef. > > OK. > > Thanks > Jon > > -- > nvpublic
On 08/06/2021 00:50, Bjorn Helgaas wrote: ... > My understanding is that we want pcie-tegra194.c to be: > > - Built into the kernel when CONFIG_PCIE_TEGRA194=m or =y and > CONFIG_ACPI=y and CONFIG_PCI_QUIRKS=y. If we're using the ACPI > pci_root.c driver, we must have the MCFG quirk built-in, and this > case worked as I expected (this is on x86): > > $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config > CONFIG_ACPI=y > CONFIG_PCI_QUIRKS=y > CONFIG_PCIE_TEGRA194=y > CONFIG_PCIE_TEGRA194_HOST=m > CONFIG_PCIE_TEGRA194_EP=y > > $ rm drivers/pci/controller/dwc/pcie-tegra194.*o > $ make drivers/pci/controller/dwc/ > ... > CC drivers/pci/controller/dwc/pcie-tegra194.o > AR drivers/pci/controller/dwc/built-in.a > > - Built as a module when CONFIG_PCIE_TEGRA194=m and CONFIG_ACPI is > not set. In this case, we're not using the ACPI pci_root.c > driver, and we don't need the MCFG quirk built-in, so it should be > OK to build a module, and IIUC this patch is supposed to *allow* > that. But in my testing, it did *not* build a module. Am I > missing something? > > $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config > # CONFIG_ACPI is not set > # CONFIG_PCI_QUIRKS is not set > CONFIG_PCIE_TEGRA194=y > CONFIG_PCIE_TEGRA194_HOST=m > CONFIG_PCIE_TEGRA194_EP=y The problem appears to be that you still have CONFIG_PCIE_TEGRA194=y and CONFIG_PCIE_TEGRA194_EP=y above. If I have ... $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config # CONFIG_ACPI is not set CONFIG_PCI_QUIRKS=y CONFIG_PCIE_TEGRA194=m CONFIG_PCIE_TEGRA194_HOST=m # CONFIG_PCIE_TEGRA194_EP is not set > > $ rm drivers/pci/controller/dwc/pcie-tegra194.*o > $ make drivers/pci/controller/dwc/ > ... > CC drivers/pci/controller/dwc/pcie-tegra194.o > AR drivers/pci/controller/dwc/built-in.a Then I see ... $ rm drivers/pci/controller/dwc/pcie-tegra194.*o $ make drivers/pci/controller/dwc/ ... CC [M] drivers/pci/controller/dwc/pcie-tegra194.o Cheers Jon
On Tue, Jun 08, 2021 at 08:44:49AM +0100, Jon Hunter wrote: > On 08/06/2021 00:50, Bjorn Helgaas wrote: > > ... > > > My understanding is that we want pcie-tegra194.c to be: > > > > - Built into the kernel when CONFIG_PCIE_TEGRA194=m or =y and > > CONFIG_ACPI=y and CONFIG_PCI_QUIRKS=y. If we're using the ACPI > > pci_root.c driver, we must have the MCFG quirk built-in, and this > > case worked as I expected (this is on x86): > > > > $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config > > CONFIG_ACPI=y > > CONFIG_PCI_QUIRKS=y > > CONFIG_PCIE_TEGRA194=y > > CONFIG_PCIE_TEGRA194_HOST=m > > CONFIG_PCIE_TEGRA194_EP=y > > > > $ rm drivers/pci/controller/dwc/pcie-tegra194.*o > > $ make drivers/pci/controller/dwc/ > > ... > > CC drivers/pci/controller/dwc/pcie-tegra194.o > > AR drivers/pci/controller/dwc/built-in.a > > > > - Built as a module when CONFIG_PCIE_TEGRA194=m and CONFIG_ACPI is > > not set. In this case, we're not using the ACPI pci_root.c > > driver, and we don't need the MCFG quirk built-in, so it should be > > OK to build a module, and IIUC this patch is supposed to *allow* > > that. But in my testing, it did *not* build a module. Am I > > missing something? > > > > $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config > > # CONFIG_ACPI is not set > > # CONFIG_PCI_QUIRKS is not set > > CONFIG_PCIE_TEGRA194=y > > CONFIG_PCIE_TEGRA194_HOST=m > > CONFIG_PCIE_TEGRA194_EP=y > > The problem appears to be that you still have CONFIG_PCIE_TEGRA194=y and > CONFIG_PCIE_TEGRA194_EP=y above. If I have ... Huh. I can't set CONFIG_PCIE_TEGRA194 directly; it's only selectable by PCIE_TEGRA194_HOST and PCIE_TEGRA194_EP. PCIE_TEGRA194 is tristate, but apparently kconfig sets it to the most restrictive, which I guess makes sense. So I would expect the shared infrastructure to be built-in if either driver is built-in, but it's somewhat confusing that CONFIG_PCIE_TEGRA194_HOST=m results in a builtin driver. If I can set CONFIG_PCIE_TEGRA194_HOST and CONFIG_PCIE_TEGRA194_EP independently, it seems like they should *be* independent. What is the purpose of PCIE_TEGRA194_EP (added by c57247f940e8 ("PCI: tegra: Add support for PCIe endpoint mode in Tegra194") [1])? I don't see any reference to it in a makefile or a source file. It looks like one can build a single driver that works in either host or endpoint mode, depending on whether a DT node matches "nvidia,tegra194-pcie" or "nvidia,tegra194-pcie-ep". So I think PCIE_TEGRA194_EP is superfluous and should be removed and you should have a single tristate Kconfig option. Tangentially related, this cast in tegra_pcie_dw_probe() looks unnecessary: pcie->mode = (enum dw_pcie_device_mode)data->mode; Looks like it was copied from similar code in dra7xx_pcie_probe(), artpec6_pcie_probe(), and dw_plat_pcie_probe(), which are all littered with similar unnecessary casts. But that should be solved separately from the Kconfig question. [1] https://git.kernel.org/linus/c57247f940e8 > $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config > # CONFIG_ACPI is not set > CONFIG_PCI_QUIRKS=y > CONFIG_PCIE_TEGRA194=m > CONFIG_PCIE_TEGRA194_HOST=m > # CONFIG_PCIE_TEGRA194_EP is not set > > > > > > $ rm drivers/pci/controller/dwc/pcie-tegra194.*o > > $ make drivers/pci/controller/dwc/ > > ... > > CC drivers/pci/controller/dwc/pcie-tegra194.o > > AR drivers/pci/controller/dwc/built-in.a > > Then I see ... > > $ rm drivers/pci/controller/dwc/pcie-tegra194.*o > $ make drivers/pci/controller/dwc/ > ... > CC [M] drivers/pci/controller/dwc/pcie-tegra194.o > > Cheers > Jon > > -- > nvpublic
On 08/06/2021 14:02, Bjorn Helgaas wrote: > On Tue, Jun 08, 2021 at 08:44:49AM +0100, Jon Hunter wrote: >> On 08/06/2021 00:50, Bjorn Helgaas wrote: >> >> ... >> >>> My understanding is that we want pcie-tegra194.c to be: >>> >>> - Built into the kernel when CONFIG_PCIE_TEGRA194=m or =y and >>> CONFIG_ACPI=y and CONFIG_PCI_QUIRKS=y. If we're using the ACPI >>> pci_root.c driver, we must have the MCFG quirk built-in, and this >>> case worked as I expected (this is on x86): >>> >>> $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config >>> CONFIG_ACPI=y >>> CONFIG_PCI_QUIRKS=y >>> CONFIG_PCIE_TEGRA194=y >>> CONFIG_PCIE_TEGRA194_HOST=m >>> CONFIG_PCIE_TEGRA194_EP=y >>> >>> $ rm drivers/pci/controller/dwc/pcie-tegra194.*o >>> $ make drivers/pci/controller/dwc/ >>> ... >>> CC drivers/pci/controller/dwc/pcie-tegra194.o >>> AR drivers/pci/controller/dwc/built-in.a >>> >>> - Built as a module when CONFIG_PCIE_TEGRA194=m and CONFIG_ACPI is >>> not set. In this case, we're not using the ACPI pci_root.c >>> driver, and we don't need the MCFG quirk built-in, so it should be >>> OK to build a module, and IIUC this patch is supposed to *allow* >>> that. But in my testing, it did *not* build a module. Am I >>> missing something? >>> >>> $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config >>> # CONFIG_ACPI is not set >>> # CONFIG_PCI_QUIRKS is not set >>> CONFIG_PCIE_TEGRA194=y >>> CONFIG_PCIE_TEGRA194_HOST=m >>> CONFIG_PCIE_TEGRA194_EP=y >> >> The problem appears to be that you still have CONFIG_PCIE_TEGRA194=y and >> CONFIG_PCIE_TEGRA194_EP=y above. If I have ... > > Huh. I can't set CONFIG_PCIE_TEGRA194 directly; it's only selectable > by PCIE_TEGRA194_HOST and PCIE_TEGRA194_EP. PCIE_TEGRA194 is > tristate, but apparently kconfig sets it to the most restrictive, > which I guess makes sense. > > So I would expect the shared infrastructure to be built-in if either > driver is built-in, but it's somewhat confusing that > CONFIG_PCIE_TEGRA194_HOST=m results in a builtin driver. If I can set > CONFIG_PCIE_TEGRA194_HOST and CONFIG_PCIE_TEGRA194_EP independently, > it seems like they should *be* independent. > > What is the purpose of PCIE_TEGRA194_EP (added by c57247f940e8 ("PCI: > tegra: Add support for PCIe endpoint mode in Tegra194") [1])? I don't > see any reference to it in a makefile or a source file. > > It looks like one can build a single driver that works in either host > or endpoint mode, depending on whether a DT node matches > "nvidia,tegra194-pcie" or "nvidia,tegra194-pcie-ep". > > So I think PCIE_TEGRA194_EP is superfluous and should be removed and > you should have a single tristate Kconfig option. This is a good point. Sagar, any reason for this? Jon -- nvpublic
On 6/8/2021 6:50 PM, Jon Hunter wrote: > > On 08/06/2021 14:02, Bjorn Helgaas wrote: >> On Tue, Jun 08, 2021 at 08:44:49AM +0100, Jon Hunter wrote: >>> On 08/06/2021 00:50, Bjorn Helgaas wrote: >>> >>> ... >>> >>>> My understanding is that we want pcie-tegra194.c to be: >>>> >>>> - Built into the kernel when CONFIG_PCIE_TEGRA194=m or =y and >>>> CONFIG_ACPI=y and CONFIG_PCI_QUIRKS=y. If we're using the ACPI >>>> pci_root.c driver, we must have the MCFG quirk built-in, and this >>>> case worked as I expected (this is on x86): >>>> >>>> $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config >>>> CONFIG_ACPI=y >>>> CONFIG_PCI_QUIRKS=y >>>> CONFIG_PCIE_TEGRA194=y >>>> CONFIG_PCIE_TEGRA194_HOST=m >>>> CONFIG_PCIE_TEGRA194_EP=y >>>> >>>> $ rm drivers/pci/controller/dwc/pcie-tegra194.*o >>>> $ make drivers/pci/controller/dwc/ >>>> ... >>>> CC drivers/pci/controller/dwc/pcie-tegra194.o >>>> AR drivers/pci/controller/dwc/built-in.a >>>> >>>> - Built as a module when CONFIG_PCIE_TEGRA194=m and CONFIG_ACPI is >>>> not set. In this case, we're not using the ACPI pci_root.c >>>> driver, and we don't need the MCFG quirk built-in, so it should be >>>> OK to build a module, and IIUC this patch is supposed to *allow* >>>> that. But in my testing, it did *not* build a module. Am I >>>> missing something? >>>> >>>> $ grep -E "CONFIG_(ACPI\>|PCI_QUIRKS|PCIE_TEGRA194)" .config >>>> # CONFIG_ACPI is not set >>>> # CONFIG_PCI_QUIRKS is not set >>>> CONFIG_PCIE_TEGRA194=y >>>> CONFIG_PCIE_TEGRA194_HOST=m >>>> CONFIG_PCIE_TEGRA194_EP=y >>> >>> The problem appears to be that you still have CONFIG_PCIE_TEGRA194=y and >>> CONFIG_PCIE_TEGRA194_EP=y above. If I have ... >> >> Huh. I can't set CONFIG_PCIE_TEGRA194 directly; it's only selectable >> by PCIE_TEGRA194_HOST and PCIE_TEGRA194_EP. PCIE_TEGRA194 is >> tristate, but apparently kconfig sets it to the most restrictive, >> which I guess makes sense. >> >> So I would expect the shared infrastructure to be built-in if either >> driver is built-in, but it's somewhat confusing that >> CONFIG_PCIE_TEGRA194_HOST=m results in a builtin driver. If I can set >> CONFIG_PCIE_TEGRA194_HOST and CONFIG_PCIE_TEGRA194_EP independently, >> it seems like they should *be* independent. >> >> What is the purpose of PCIE_TEGRA194_EP (added by c57247f940e8 ("PCI: >> tegra: Add support for PCIe endpoint mode in Tegra194") [1])? I don't >> see any reference to it in a makefile or a source file. >> >> It looks like one can build a single driver that works in either host >> or endpoint mode, depending on whether a DT node matches >> "nvidia,tegra194-pcie" or "nvidia,tegra194-pcie-ep". >> >> So I think PCIE_TEGRA194_EP is superfluous and should be removed and >> you should have a single tristate Kconfig option. > > This is a good point. > > Sagar, any reason for this? Although it is the same driver that works for both HOST mode and EP mode, PCIE_TEGRA194_EP depends on PCI_ENDPOINT whereas the PCIE_TEGRA194_HOST mode doesn't. Similarly the PCIE_TEGRA194_HOST mode depends on PCI_MSI_IRQ_DOMAIN which PCIE_TEGRA194_EP doesn't depend on. It is possible to have end point mode support disabled (at sub-system level) in the system yet pcie-tegra194 can be compiled for the host mode vice-a-versa for the endpoint mode. Hence, appropriate config HOST/EP needs to be selected to make sure that the rest of the dependencies are enabled in the system. Hope I'm able to give the rationale correctly here. - Vidya Sagar > > Jon > -- > nvpublic >
On 08/06/2021 19:34, Vidya Sagar wrote: ... >>> What is the purpose of PCIE_TEGRA194_EP (added by c57247f940e8 ("PCI: >>> tegra: Add support for PCIe endpoint mode in Tegra194") [1])? I don't >>> see any reference to it in a makefile or a source file. >>> >>> It looks like one can build a single driver that works in either host >>> or endpoint mode, depending on whether a DT node matches >>> "nvidia,tegra194-pcie" or "nvidia,tegra194-pcie-ep". >>> >>> So I think PCIE_TEGRA194_EP is superfluous and should be removed and >>> you should have a single tristate Kconfig option. >> >> This is a good point. >> >> Sagar, any reason for this? > Although it is the same driver that works for both HOST mode and EP > mode, PCIE_TEGRA194_EP depends on PCI_ENDPOINT whereas the > PCIE_TEGRA194_HOST mode doesn't. Similarly the PCIE_TEGRA194_HOST mode > depends on PCI_MSI_IRQ_DOMAIN which PCIE_TEGRA194_EP doesn't depend on. > It is possible to have end point mode support disabled (at sub-system > level) in the system yet pcie-tegra194 can be compiled for the host mode > vice-a-versa for the endpoint mode. > Hence, appropriate config HOST/EP needs to be selected to make sure that > the rest of the dependencies are enabled in the system. > Hope I'm able to give the rationale correctly here. Yes but should we combine them like this ... diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 423d35872ce4..206455a9b70d 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -254,15 +254,12 @@ config PCI_MESON implement the driver. config PCIE_TEGRA194 - tristate - -config PCIE_TEGRA194_HOST - tristate "NVIDIA Tegra194 (and later) PCIe controller - Host Mode" + tristate "NVIDIA Tegra194 (and later) PCIe controller" depends on ARCH_TEGRA_194_SOC || COMPILE_TEST - depends on PCI_MSI_IRQ_DOMAIN - select PCIE_DW_HOST + depends on PCI_MSI_IRQ_DOMAIN || PCI_ENDPOINT + select PCIE_DW_HOST if PCI_MSI_IRQ_DOMAIN + select PCIE_DW_EP if PCI_ENDPOINT select PHY_TEGRA194_P2U - select PCIE_TEGRA194 help Enables support for the PCIe controller in the NVIDIA Tegra194 SoC to work in host mode. There are two instances of PCIe controllers in @@ -271,21 +268,6 @@ config PCIE_TEGRA194_HOST in order to enable device-specific features PCIE_TEGRA194_EP must be selected. This uses the DesignWare core. -config PCIE_TEGRA194_EP - tristate "NVIDIA Tegra194 (and later) PCIe controller - Endpoint Mode" - depends on ARCH_TEGRA_194_SOC || COMPILE_TEST - depends on PCI_ENDPOINT - select PCIE_DW_EP - select PHY_TEGRA194_P2U - select PCIE_TEGRA194 - help - Enables support for the PCIe controller in the NVIDIA Tegra194 SoC to - work in endpoint mode. There are two instances of PCIe controllers in - Tegra194. This controller can work either as EP or RC. In order to - enable host-specific features PCIE_TEGRA194_HOST must be selected and - in order to enable device-specific features PCIE_TEGRA194_EP must be - selected. This uses the DesignWare core. - Furthermore, I wonder if we should just move the code that is required for ACPI into it's own file like drivers/pci/controller/dwc/pcie-tegra194-acpi.c? Jon
On 08/06/2021 21:11, Jon Hunter wrote: ... > Furthermore, I wonder if we should just move the code > that is required for ACPI into it's own file like > drivers/pci/controller/dwc/pcie-tegra194-acpi.c? I have been doing some testing and the above works fine. IMO moving the ACPI specific code to its own file is a lot cleaner and simplifies the Makefile and Kconfig. Especially seeing as the ACPI quirk code is independent of the actual Tegra194 PCIe driver. Therefore, unless you have any objections I will send a patch to fix this by doing just that tomorrow. Also let me know if you have any concerns about the file name or location drivers/pci/controller/dwc/pcie-tegra194-acpi.c. That will at least fix the issue with v5.13. If we do that, then for v5.14 I will clean-up the Kconfig and place everything under a single CONFIG_PCIE_TEGRA194 entry (which I can send out once the initial problem is fixed). And finally I will remove the unnecessary cast in the probe function. Jon
On 6/9/2021 3:53 PM, Jon Hunter wrote: > > On 08/06/2021 21:11, Jon Hunter wrote: > > ... > >> Furthermore, I wonder if we should just move the code >> that is required for ACPI into it's own file like >> drivers/pci/controller/dwc/pcie-tegra194-acpi.c? > > I have been doing some testing and the above works fine. IMO moving the > ACPI specific code to its own file is a lot cleaner and simplifies the > Makefile and Kconfig. Especially seeing as the ACPI quirk code is > independent of the actual Tegra194 PCIe driver. Therefore, unless you > have any objections I will send a patch to fix this by doing just that > tomorrow. Also let me know if you have any concerns about the file name > or location drivers/pci/controller/dwc/pcie-tegra194-acpi.c. I'm fine with this. No concerns from my side. > > That will at least fix the issue with v5.13. If we do that, then for > v5.14 I will clean-up the Kconfig and place everything under a single > CONFIG_PCIE_TEGRA194 entry (which I can send out once the initial > problem is fixed). And finally I will remove the unnecessary cast in the > probe function. How are we going to address the dependency issues (w.r.t RP and EP) if we keep only CONFIG_PCIE_TEGRA194? > > Jon >
On Tue, Jun 08, 2021 at 09:11:27PM +0100, Jon Hunter wrote: > On 08/06/2021 19:34, Vidya Sagar wrote: > > ... > > >>> What is the purpose of PCIE_TEGRA194_EP (added by c57247f940e8 ("PCI: > >>> tegra: Add support for PCIe endpoint mode in Tegra194") [1])? I don't > >>> see any reference to it in a makefile or a source file. > >>> > >>> It looks like one can build a single driver that works in either host > >>> or endpoint mode, depending on whether a DT node matches > >>> "nvidia,tegra194-pcie" or "nvidia,tegra194-pcie-ep". > >>> > >>> So I think PCIE_TEGRA194_EP is superfluous and should be removed and > >>> you should have a single tristate Kconfig option. > >> > >> This is a good point. > >> > >> Sagar, any reason for this? > > Although it is the same driver that works for both HOST mode and EP > > mode, PCIE_TEGRA194_EP depends on PCI_ENDPOINT whereas the > > PCIE_TEGRA194_HOST mode doesn't. Similarly the PCIE_TEGRA194_HOST mode > > depends on PCI_MSI_IRQ_DOMAIN which PCIE_TEGRA194_EP doesn't depend on. > > It is possible to have end point mode support disabled (at sub-system > > level) in the system yet pcie-tegra194 can be compiled for the host mode > > vice-a-versa for the endpoint mode. > > Hence, appropriate config HOST/EP needs to be selected to make sure that > > the rest of the dependencies are enabled in the system. > > Hope I'm able to give the rationale correctly here. > > Yes but should we combine them like this ... > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > index 423d35872ce4..206455a9b70d 100644 > --- a/drivers/pci/controller/dwc/Kconfig > +++ b/drivers/pci/controller/dwc/Kconfig > @@ -254,15 +254,12 @@ config PCI_MESON > implement the driver. > > config PCIE_TEGRA194 > - tristate > - > -config PCIE_TEGRA194_HOST > - tristate "NVIDIA Tegra194 (and later) PCIe controller - Host Mode" > + tristate "NVIDIA Tegra194 (and later) PCIe controller" > depends on ARCH_TEGRA_194_SOC || COMPILE_TEST > - depends on PCI_MSI_IRQ_DOMAIN > - select PCIE_DW_HOST > + depends on PCI_MSI_IRQ_DOMAIN || PCI_ENDPOINT > + select PCIE_DW_HOST if PCI_MSI_IRQ_DOMAIN > + select PCIE_DW_EP if PCI_ENDPOINT > select PHY_TEGRA194_P2U > - select PCIE_TEGRA194 > help > Enables support for the PCIe controller in the NVIDIA Tegra194 SoC to > work in host mode. There are two instances of PCIe controllers in > @@ -271,21 +268,6 @@ config PCIE_TEGRA194_HOST > in order to enable device-specific features PCIE_TEGRA194_EP must be > selected. This uses the DesignWare core. > > -config PCIE_TEGRA194_EP > - tristate "NVIDIA Tegra194 (and later) PCIe controller - Endpoint Mode" > - depends on ARCH_TEGRA_194_SOC || COMPILE_TEST > - depends on PCI_ENDPOINT > - select PCIE_DW_EP > - select PHY_TEGRA194_P2U > - select PCIE_TEGRA194 > - help > - Enables support for the PCIe controller in the NVIDIA Tegra194 SoC to > - work in endpoint mode. There are two instances of PCIe controllers in > - Tegra194. This controller can work either as EP or RC. In order to > - enable host-specific features PCIE_TEGRA194_HOST must be selected and > - in order to enable device-specific features PCIE_TEGRA194_EP must be > - selected. This uses the DesignWare core. I'm not a Kconfig expert, but I really like that solution, as long as it addresses Vidya's concerns about RP/EP dependencies. Looks like the Kconfig help text should be updated to remove the other PCIE_TEGRA194_EP reference? Maybe it should include a clue about how the connections to host/endpoint support, e.g., "includes endpoint support if PCI_ENDPOINT is enabled"? > Furthermore, I wonder if we should just move the code > that is required for ACPI into it's own file like > drivers/pci/controller/dwc/pcie-tegra194-acpi.c? That might simplify things. I think the reason we started with things in one file is because for some drivers there's a lot of shared stuff (#defines, register accessors) between the quirk and the native host driver. Either you have to put it all in one file, or you have to add a shared .h file and make some of that stuff non-static. Bjorn
On 09/06/2021 17:18, Bjorn Helgaas wrote: > On Tue, Jun 08, 2021 at 09:11:27PM +0100, Jon Hunter wrote: >> On 08/06/2021 19:34, Vidya Sagar wrote: >> >> ... >> >>>>> What is the purpose of PCIE_TEGRA194_EP (added by c57247f940e8 ("PCI: >>>>> tegra: Add support for PCIe endpoint mode in Tegra194") [1])? I don't >>>>> see any reference to it in a makefile or a source file. >>>>> >>>>> It looks like one can build a single driver that works in either host >>>>> or endpoint mode, depending on whether a DT node matches >>>>> "nvidia,tegra194-pcie" or "nvidia,tegra194-pcie-ep". >>>>> >>>>> So I think PCIE_TEGRA194_EP is superfluous and should be removed and >>>>> you should have a single tristate Kconfig option. >>>> >>>> This is a good point. >>>> >>>> Sagar, any reason for this? >>> Although it is the same driver that works for both HOST mode and EP >>> mode, PCIE_TEGRA194_EP depends on PCI_ENDPOINT whereas the >>> PCIE_TEGRA194_HOST mode doesn't. Similarly the PCIE_TEGRA194_HOST mode >>> depends on PCI_MSI_IRQ_DOMAIN which PCIE_TEGRA194_EP doesn't depend on. >>> It is possible to have end point mode support disabled (at sub-system >>> level) in the system yet pcie-tegra194 can be compiled for the host mode >>> vice-a-versa for the endpoint mode. >>> Hence, appropriate config HOST/EP needs to be selected to make sure that >>> the rest of the dependencies are enabled in the system. >>> Hope I'm able to give the rationale correctly here. >> >> Yes but should we combine them like this ... >> >> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig >> index 423d35872ce4..206455a9b70d 100644 >> --- a/drivers/pci/controller/dwc/Kconfig >> +++ b/drivers/pci/controller/dwc/Kconfig >> @@ -254,15 +254,12 @@ config PCI_MESON >> implement the driver. >> >> config PCIE_TEGRA194 >> - tristate >> - >> -config PCIE_TEGRA194_HOST >> - tristate "NVIDIA Tegra194 (and later) PCIe controller - Host Mode" >> + tristate "NVIDIA Tegra194 (and later) PCIe controller" >> depends on ARCH_TEGRA_194_SOC || COMPILE_TEST >> - depends on PCI_MSI_IRQ_DOMAIN >> - select PCIE_DW_HOST >> + depends on PCI_MSI_IRQ_DOMAIN || PCI_ENDPOINT >> + select PCIE_DW_HOST if PCI_MSI_IRQ_DOMAIN >> + select PCIE_DW_EP if PCI_ENDPOINT >> select PHY_TEGRA194_P2U >> - select PCIE_TEGRA194 >> help >> Enables support for the PCIe controller in the NVIDIA Tegra194 SoC to >> work in host mode. There are two instances of PCIe controllers in >> @@ -271,21 +268,6 @@ config PCIE_TEGRA194_HOST >> in order to enable device-specific features PCIE_TEGRA194_EP must be >> selected. This uses the DesignWare core. >> >> -config PCIE_TEGRA194_EP >> - tristate "NVIDIA Tegra194 (and later) PCIe controller - Endpoint Mode" >> - depends on ARCH_TEGRA_194_SOC || COMPILE_TEST >> - depends on PCI_ENDPOINT >> - select PCIE_DW_EP >> - select PHY_TEGRA194_P2U >> - select PCIE_TEGRA194 >> - help >> - Enables support for the PCIe controller in the NVIDIA Tegra194 SoC to >> - work in endpoint mode. There are two instances of PCIe controllers in >> - Tegra194. This controller can work either as EP or RC. In order to >> - enable host-specific features PCIE_TEGRA194_HOST must be selected and >> - in order to enable device-specific features PCIE_TEGRA194_EP must be >> - selected. This uses the DesignWare core. > > I'm not a Kconfig expert, but I really like that solution, as long as > it addresses Vidya's concerns about RP/EP dependencies. I think that Sagar's concern is that if PCI_ENDPOINT and PCI_MSI_IRQ_DOMAIN are enabled, then we will always PCIE_DW_EP and PCIE_DW_HOST even if we only need RP or EP functionality. So there is no switch at the Tegra driver level to indicate if we want RP, EP or both. My concern with the existing implementation is that if PCIE_TEGRA194_EP is disabled and PCIE_TEGRA194_HOST is enabled, then if PCI_ENDPOINT and PCIE_DW_EP already happen to be enabled, we will still have EP functionality regardless of PCIE_TEGRA194_EP setting. I am not sure what is best/preferred in this case. > Looks like the Kconfig help text should be updated to remove the > other PCIE_TEGRA194_EP reference? Maybe it should include a clue > about how the connections to host/endpoint support, e.g., "includes > endpoint support if PCI_ENDPOINT is enabled"? > >> Furthermore, I wonder if we should just move the code >> that is required for ACPI into it's own file like >> drivers/pci/controller/dwc/pcie-tegra194-acpi.c? > > That might simplify things. I think the reason we started with things > in one file is because for some drivers there's a lot of shared stuff > (#defines, register accessors) between the quirk and the native host > driver. Either you have to put it all in one file, or you have to add > a shared .h file and make some of that stuff non-static. I did test this and there is nothing that needs to be shared via a local header. We only need to include the appropriate pci headers and so the code is well isolated. I send the patch if that is easier to see. Jon
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index eca805c1a023..f0d1e2d8c022 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -32,6 +32,7 @@ obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o # depending on whether ACPI, the DT driver, or both are enabled. obj-$(CONFIG_PCIE_AL) += pcie-al.o +obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o obj-$(CONFIG_PCI_HISI) += pcie-hisi.o ifdef CONFIG_ACPI diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c index b19775ab134e..ae70e53a7826 100644 --- a/drivers/pci/controller/dwc/pcie-tegra194.c +++ b/drivers/pci/controller/dwc/pcie-tegra194.c @@ -240,13 +240,16 @@ #define EP_STATE_DISABLED 0 #define EP_STATE_ENABLED 1 +#if IS_ENABLED(CONFIG_PCIE_TEGRA194) static const unsigned int pcie_gen_freq[] = { GEN1_CORE_CLK_FREQ, GEN2_CORE_CLK_FREQ, GEN3_CORE_CLK_FREQ, GEN4_CORE_CLK_FREQ }; +#endif +#if defined(CONFIG_PCIEASPM) static const u32 event_cntr_ctrl_offset[] = { 0x1d8, 0x1a8, @@ -264,6 +267,7 @@ static const u32 event_cntr_data_offset[] = { 0x1c8, 0x1dc }; +#endif struct tegra_pcie_dw { struct device *dev; @@ -409,7 +413,7 @@ const struct pci_ecam_ops tegra194_pcie_ops = { }; #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */ -#ifdef CONFIG_PCIE_TEGRA194 +#if IS_ENABLED(CONFIG_PCIE_TEGRA194) static inline struct tegra_pcie_dw *to_tegra_pcie(struct dw_pcie *pci) {
Commit 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM errata") caused a few build regressions for the Tegra194 PCIe driver which are: 1. The Tegra194 PCIe driver can no longer be built as a module. This was caused by removing the Makefile entry to build the pcie-tegra.c based upon the CONFIG_PCIE_TEGRA194 option. Therefore, restore this so that we can build the driver as a module if ACPI support is not enabled in the kernel. 2. If CONFIG_PCIE_TEGRA194 is configured to build the driver as a module, at the same time that CONFIG_ACPI and CONFIG_PCI_QUIRKS are selected to build the driver into the kernel, then the necessary functions in the driver to probe and remove the device when booting with device-tree and not compiled into to the driver. This prevents the PCIe devices being probed when booting with device-tree. Fix this by using the IS_ENABLED() macro. 3. The below build warnings to be seen with particular kernel configurations. Fix these by adding the necessary guards around these variable definitions. drivers/pci/controller/dwc/pcie-tegra194.c:259:18: warning: ‘event_cntr_data_offset’ defined but not used [-Wunused-const-variable=] drivers/pci/controller/dwc/pcie-tegra194.c:250:18: warning: ‘event_cntr_ctrl_offset’ defined but not used [-Wunused-const-variable=] drivers/pci/controller/dwc/pcie-tegra194.c:243:27: warning: ‘pcie_gen_freq’ defined but not used [-Wunused-const-variable=] Fixes: 7f100744749e ("PCI: tegra: Add Tegra194 MCFG quirks for ECAM errata") Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-tegra194.c | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-)