[PATCH V4] PCI: tegra: Fix building Tegra194 PCIe driver

[PATCH V4] PCI: tegra: Fix building Tegra194 PCIe driver

[Jon Hunter]

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.
2. 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. Given that the ACPI quirk code for Tegra194 is completely
   independent of the native Tegra194 PCIe driver, move this code into
   its own file so that it can be built independently and we can remove
   the "#ifdef CONFIG_PCIE_TEGRA194" in the native driver. Note that
   given the native Tegra194 PCIe driver is only used with device-tree,
   this will not cause any conflicts.
3. The below build warnings to be seen with particular kernel
   configurations. Fix these by moving these structure definitions to
   within the necessary guards.

  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>
---
Changes since V3:
- Moved ACPI quirk code into separate source file

Changes since V2:
- Update the commit message per Bjorn's feedback
- Moved the structure definitions within the necessary guards as opposed
  to wrapping the existing defintions with the appropriate guards.

Changes since V1:
- Added fixes tag
- Fixed 'defined but not used' compiler warnings

 drivers/pci/controller/dwc/Makefile           |   3 +-
 .../pci/controller/dwc/pcie-tegra194-acpi.c   | 108 ++++++++++++++
 drivers/pci/controller/dwc/pcie-tegra194.c    | 138 +++---------------
 3 files changed, 128 insertions(+), 121 deletions(-)
 create mode 100644 drivers/pci/controller/dwc/pcie-tegra194-acpi.c

diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index eca805c1a023..9e6ce0dc2f53 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
 obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
 obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
 obj-$(CONFIG_PCI_MESON) += pci-meson.o
+obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
 obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
 obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
 
@@ -38,6 +39,6 @@ ifdef CONFIG_ACPI
 ifdef CONFIG_PCI_QUIRKS
 obj-$(CONFIG_ARM64) += pcie-al.o
 obj-$(CONFIG_ARM64) += pcie-hisi.o
-obj-$(CONFIG_ARM64) += pcie-tegra194.o
+obj-$(CONFIG_ARM64) += pcie-tegra194-acpi.o
 endif
 endif
diff --git a/drivers/pci/controller/dwc/pcie-tegra194-acpi.c b/drivers/pci/controller/dwc/pcie-tegra194-acpi.c
new file mode 100644
index 000000000000..c2de6ed4d86f
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-tegra194-acpi.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ACPI quirks for Tegra194 PCIe host controller
+ *
+ * Copyright (C) 2021 NVIDIA Corporation.
+ *
+ * Author: Vidya Sagar <vidyas@nvidia.com>
+ */
+
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+#include <linux/pci-ecam.h>
+
+#include "pcie-designware.h"
+
+struct tegra194_pcie_ecam  {
+	void __iomem *config_base;
+	void __iomem *iatu_base;
+	void __iomem *dbi_base;
+};
+
+static int tegra194_acpi_init(struct pci_config_window *cfg)
+{
+	struct device *dev = cfg->parent;
+	struct tegra194_pcie_ecam *pcie_ecam;
+
+	pcie_ecam = devm_kzalloc(dev, sizeof(*pcie_ecam), GFP_KERNEL);
+	if (!pcie_ecam)
+		return -ENOMEM;
+
+	pcie_ecam->config_base = cfg->win;
+	pcie_ecam->iatu_base = cfg->win + SZ_256K;
+	pcie_ecam->dbi_base = cfg->win + SZ_512K;
+	cfg->priv = pcie_ecam;
+
+	return 0;
+}
+
+static void atu_reg_write(struct tegra194_pcie_ecam *pcie_ecam, int index,
+			  u32 val, u32 reg)
+{
+	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
+
+	writel(val, pcie_ecam->iatu_base + offset + reg);
+}
+
+static void program_outbound_atu(struct tegra194_pcie_ecam *pcie_ecam,
+				 int index, int type, u64 cpu_addr,
+				 u64 pci_addr, u64 size)
+{
+	atu_reg_write(pcie_ecam, index, lower_32_bits(cpu_addr),
+		      PCIE_ATU_LOWER_BASE);
+	atu_reg_write(pcie_ecam, index, upper_32_bits(cpu_addr),
+		      PCIE_ATU_UPPER_BASE);
+	atu_reg_write(pcie_ecam, index, lower_32_bits(pci_addr),
+		      PCIE_ATU_LOWER_TARGET);
+	atu_reg_write(pcie_ecam, index, lower_32_bits(cpu_addr + size - 1),
+		      PCIE_ATU_LIMIT);
+	atu_reg_write(pcie_ecam, index, upper_32_bits(pci_addr),
+		      PCIE_ATU_UPPER_TARGET);
+	atu_reg_write(pcie_ecam, index, type, PCIE_ATU_CR1);
+	atu_reg_write(pcie_ecam, index, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
+}
+
+static void __iomem *tegra194_map_bus(struct pci_bus *bus,
+				      unsigned int devfn, int where)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+	struct tegra194_pcie_ecam *pcie_ecam = cfg->priv;
+	u32 busdev;
+	int type;
+
+	if (bus->number < cfg->busr.start || bus->number > cfg->busr.end)
+		return NULL;
+
+	if (bus->number == cfg->busr.start) {
+		if (PCI_SLOT(devfn) == 0)
+			return pcie_ecam->dbi_base + where;
+		else
+			return NULL;
+	}
+
+	busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
+		 PCIE_ATU_FUNC(PCI_FUNC(devfn));
+
+	if (bus->parent->number == cfg->busr.start) {
+		if (PCI_SLOT(devfn) == 0)
+			type = PCIE_ATU_TYPE_CFG0;
+		else
+			return NULL;
+	} else {
+		type = PCIE_ATU_TYPE_CFG1;
+	}
+
+	program_outbound_atu(pcie_ecam, 0, type, cfg->res.start, busdev,
+			     SZ_256K);
+
+	return pcie_ecam->config_base + where;
+}
+
+const struct pci_ecam_ops tegra194_pcie_ops = {
+	.init		= tegra194_acpi_init,
+	.pci_ops	= {
+		.map_bus	= tegra194_map_bus,
+		.read		= pci_generic_config_read,
+		.write		= pci_generic_config_write,
+	}
+};
diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index b19775ab134e..fa6b12cfc043 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -22,8 +22,6 @@
 #include <linux/of_irq.h>
 #include <linux/of_pci.h>
 #include <linux/pci.h>
-#include <linux/pci-acpi.h>
-#include <linux/pci-ecam.h>
 #include <linux/phy/phy.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
@@ -247,24 +245,6 @@ static const unsigned int pcie_gen_freq[] = {
 	GEN4_CORE_CLK_FREQ
 };
 
-static const u32 event_cntr_ctrl_offset[] = {
-	0x1d8,
-	0x1a8,
-	0x1a8,
-	0x1a8,
-	0x1c4,
-	0x1d8
-};
-
-static const u32 event_cntr_data_offset[] = {
-	0x1dc,
-	0x1ac,
-	0x1ac,
-	0x1ac,
-	0x1c8,
-	0x1dc
-};
-
 struct tegra_pcie_dw {
 	struct device *dev;
 	struct resource *appl_res;
@@ -313,104 +293,6 @@ struct tegra_pcie_dw_of_data {
 	enum dw_pcie_device_mode mode;
 };
 
-#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
-struct tegra194_pcie_ecam  {
-	void __iomem *config_base;
-	void __iomem *iatu_base;
-	void __iomem *dbi_base;
-};
-
-static int tegra194_acpi_init(struct pci_config_window *cfg)
-{
-	struct device *dev = cfg->parent;
-	struct tegra194_pcie_ecam *pcie_ecam;
-
-	pcie_ecam = devm_kzalloc(dev, sizeof(*pcie_ecam), GFP_KERNEL);
-	if (!pcie_ecam)
-		return -ENOMEM;
-
-	pcie_ecam->config_base = cfg->win;
-	pcie_ecam->iatu_base = cfg->win + SZ_256K;
-	pcie_ecam->dbi_base = cfg->win + SZ_512K;
-	cfg->priv = pcie_ecam;
-
-	return 0;
-}
-
-static void atu_reg_write(struct tegra194_pcie_ecam *pcie_ecam, int index,
-			  u32 val, u32 reg)
-{
-	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
-
-	writel(val, pcie_ecam->iatu_base + offset + reg);
-}
-
-static void program_outbound_atu(struct tegra194_pcie_ecam *pcie_ecam,
-				 int index, int type, u64 cpu_addr,
-				 u64 pci_addr, u64 size)
-{
-	atu_reg_write(pcie_ecam, index, lower_32_bits(cpu_addr),
-		      PCIE_ATU_LOWER_BASE);
-	atu_reg_write(pcie_ecam, index, upper_32_bits(cpu_addr),
-		      PCIE_ATU_UPPER_BASE);
-	atu_reg_write(pcie_ecam, index, lower_32_bits(pci_addr),
-		      PCIE_ATU_LOWER_TARGET);
-	atu_reg_write(pcie_ecam, index, lower_32_bits(cpu_addr + size - 1),
-		      PCIE_ATU_LIMIT);
-	atu_reg_write(pcie_ecam, index, upper_32_bits(pci_addr),
-		      PCIE_ATU_UPPER_TARGET);
-	atu_reg_write(pcie_ecam, index, type, PCIE_ATU_CR1);
-	atu_reg_write(pcie_ecam, index, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
-}
-
-static void __iomem *tegra194_map_bus(struct pci_bus *bus,
-				      unsigned int devfn, int where)
-{
-	struct pci_config_window *cfg = bus->sysdata;
-	struct tegra194_pcie_ecam *pcie_ecam = cfg->priv;
-	u32 busdev;
-	int type;
-
-	if (bus->number < cfg->busr.start || bus->number > cfg->busr.end)
-		return NULL;
-
-	if (bus->number == cfg->busr.start) {
-		if (PCI_SLOT(devfn) == 0)
-			return pcie_ecam->dbi_base + where;
-		else
-			return NULL;
-	}
-
-	busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
-		 PCIE_ATU_FUNC(PCI_FUNC(devfn));
-
-	if (bus->parent->number == cfg->busr.start) {
-		if (PCI_SLOT(devfn) == 0)
-			type = PCIE_ATU_TYPE_CFG0;
-		else
-			return NULL;
-	} else {
-		type = PCIE_ATU_TYPE_CFG1;
-	}
-
-	program_outbound_atu(pcie_ecam, 0, type, cfg->res.start, busdev,
-			     SZ_256K);
-
-	return pcie_ecam->config_base + where;
-}
-
-const struct pci_ecam_ops tegra194_pcie_ops = {
-	.init		= tegra194_acpi_init,
-	.pci_ops	= {
-		.map_bus	= tegra194_map_bus,
-		.read		= pci_generic_config_read,
-		.write		= pci_generic_config_write,
-	}
-};
-#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
-
-#ifdef CONFIG_PCIE_TEGRA194
-
 static inline struct tegra_pcie_dw *to_tegra_pcie(struct dw_pcie *pci)
 {
 	return container_of(pci, struct tegra_pcie_dw, pci);
@@ -694,6 +576,24 @@ static struct pci_ops tegra_pci_ops = {
 };
 
 #if defined(CONFIG_PCIEASPM)
+static const u32 event_cntr_ctrl_offset[] = {
+	0x1d8,
+	0x1a8,
+	0x1a8,
+	0x1a8,
+	0x1c4,
+	0x1d8
+};
+
+static const u32 event_cntr_data_offset[] = {
+	0x1dc,
+	0x1ac,
+	0x1ac,
+	0x1ac,
+	0x1c8,
+	0x1dc
+};
+
 static void disable_aspm_l11(struct tegra_pcie_dw *pcie)
 {
 	u32 val;
@@ -2413,5 +2313,3 @@ MODULE_DEVICE_TABLE(of, tegra_pcie_dw_of_match);
 MODULE_AUTHOR("Vidya Sagar <vidyas@nvidia.com>");
 MODULE_DESCRIPTION("NVIDIA PCIe host controller driver");
 MODULE_LICENSE("GPL v2");
-
-#endif /* CONFIG_PCIE_TEGRA194 */
-- 
2.25.1

Re: [PATCH V4] PCI: tegra: Fix building Tegra194 PCIe driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3439 bytes --]

On Thu, Jun 10, 2021 at 07:41:34AM +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.
> 2. 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

I think the correct way to write conditionals that are true on =y and =m
is to use:

	#ifdef IS_ENABLED(CONFIG_PCIE_TEGRA194)

but that doesn't fix the issue of not getting the quirks included when
CONFIG_PCIE_TEGRA194=m.

>    driver. Given that the ACPI quirk code for Tegra194 is completely
>    independent of the native Tegra194 PCIe driver, move this code into
>    its own file so that it can be built independently and we can remove
>    the "#ifdef CONFIG_PCIE_TEGRA194" in the native driver. Note that
>    given the native Tegra194 PCIe driver is only used with device-tree,
>    this will not cause any conflicts.
> 3. The below build warnings to be seen with particular kernel
>    configurations. Fix these by moving these structure definitions to
>    within the necessary guards.
> 
>   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>
> ---
> Changes since V3:
> - Moved ACPI quirk code into separate source file
> 
> Changes since V2:
> - Update the commit message per Bjorn's feedback
> - Moved the structure definitions within the necessary guards as opposed
>   to wrapping the existing defintions with the appropriate guards.
> 
> Changes since V1:
> - Added fixes tag
> - Fixed 'defined but not used' compiler warnings
> 
>  drivers/pci/controller/dwc/Makefile           |   3 +-
>  .../pci/controller/dwc/pcie-tegra194-acpi.c   | 108 ++++++++++++++
>  drivers/pci/controller/dwc/pcie-tegra194.c    | 138 +++---------------
>  3 files changed, 128 insertions(+), 121 deletions(-)
>  create mode 100644 drivers/pci/controller/dwc/pcie-tegra194-acpi.c

We've had a similar problem with Tegra210 EMC and the EMC frequency
table parsing code. The EMC frequency table parsing code needs to be
always built-in because it is stashed into a linker section. However
we still want the driver to be able to be a loadable module. The only
way I know how to achieve that is by splitting up the sources files
like you do here, so I this looks like a correct fix:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH V4] PCI: tegra: Fix building Tegra194 PCIe driver

[Jon Hunter]

Hi Bjorn, Lorenzo,

On 10/06/2021 10:07, Thierry Reding wrote:
> On Thu, Jun 10, 2021 at 07:41:34AM +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.
>> 2. 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
> 
> I think the correct way to write conditionals that are true on =y and =m
> is to use:
> 
> 	#ifdef IS_ENABLED(CONFIG_PCIE_TEGRA194)
> 
> but that doesn't fix the issue of not getting the quirks included when
> CONFIG_PCIE_TEGRA194=m.
> 
>>    driver. Given that the ACPI quirk code for Tegra194 is completely
>>    independent of the native Tegra194 PCIe driver, move this code into
>>    its own file so that it can be built independently and we can remove
>>    the "#ifdef CONFIG_PCIE_TEGRA194" in the native driver. Note that
>>    given the native Tegra194 PCIe driver is only used with device-tree,
>>    this will not cause any conflicts.
>> 3. The below build warnings to be seen with particular kernel
>>    configurations. Fix these by moving these structure definitions to
>>    within the necessary guards.
>>
>>   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>
>> ---
>> Changes since V3:
>> - Moved ACPI quirk code into separate source file
>>
>> Changes since V2:
>> - Update the commit message per Bjorn's feedback
>> - Moved the structure definitions within the necessary guards as opposed
>>   to wrapping the existing defintions with the appropriate guards.
>>
>> Changes since V1:
>> - Added fixes tag
>> - Fixed 'defined but not used' compiler warnings
>>
>>  drivers/pci/controller/dwc/Makefile           |   3 +-
>>  .../pci/controller/dwc/pcie-tegra194-acpi.c   | 108 ++++++++++++++
>>  drivers/pci/controller/dwc/pcie-tegra194.c    | 138 +++---------------
>>  3 files changed, 128 insertions(+), 121 deletions(-)
>>  create mode 100644 drivers/pci/controller/dwc/pcie-tegra194-acpi.c
> 
> We've had a similar problem with Tegra210 EMC and the EMC frequency
> table parsing code. The EMC frequency table parsing code needs to be
> always built-in because it is stashed into a linker section. However
> we still want the driver to be able to be a loadable module. The only
> way I know how to achieve that is by splitting up the sources files
> like you do here, so I this looks like a correct fix:
> 
> Reviewed-by: Thierry Reding <treding@nvidia.com>


Just a reminder that we need to get this fixed for v5.13. Please let me
know if you have any more comments on this fix.

Thanks!
Jon

-- 
nvpublic

Re: [PATCH V4] PCI: tegra: Fix building Tegra194 PCIe driver

[Jon Hunter]

Adding Bjorn to the To ...

On 14/06/2021 21:37, Jon Hunter wrote:
> Hi Bjorn, Lorenzo,
> 
> On 10/06/2021 10:07, Thierry Reding wrote:
>> On Thu, Jun 10, 2021 at 07:41:34AM +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.
>>> 2. 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
>>
>> I think the correct way to write conditionals that are true on =y and =m
>> is to use:
>>
>> 	#ifdef IS_ENABLED(CONFIG_PCIE_TEGRA194)
>>
>> but that doesn't fix the issue of not getting the quirks included when
>> CONFIG_PCIE_TEGRA194=m.
>>
>>>    driver. Given that the ACPI quirk code for Tegra194 is completely
>>>    independent of the native Tegra194 PCIe driver, move this code into
>>>    its own file so that it can be built independently and we can remove
>>>    the "#ifdef CONFIG_PCIE_TEGRA194" in the native driver. Note that
>>>    given the native Tegra194 PCIe driver is only used with device-tree,
>>>    this will not cause any conflicts.
>>> 3. The below build warnings to be seen with particular kernel
>>>    configurations. Fix these by moving these structure definitions to
>>>    within the necessary guards.
>>>
>>>   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>
>>> ---
>>> Changes since V3:
>>> - Moved ACPI quirk code into separate source file
>>>
>>> Changes since V2:
>>> - Update the commit message per Bjorn's feedback
>>> - Moved the structure definitions within the necessary guards as opposed
>>>   to wrapping the existing defintions with the appropriate guards.
>>>
>>> Changes since V1:
>>> - Added fixes tag
>>> - Fixed 'defined but not used' compiler warnings
>>>
>>>  drivers/pci/controller/dwc/Makefile           |   3 +-
>>>  .../pci/controller/dwc/pcie-tegra194-acpi.c   | 108 ++++++++++++++
>>>  drivers/pci/controller/dwc/pcie-tegra194.c    | 138 +++---------------
>>>  3 files changed, 128 insertions(+), 121 deletions(-)
>>>  create mode 100644 drivers/pci/controller/dwc/pcie-tegra194-acpi.c
>>
>> We've had a similar problem with Tegra210 EMC and the EMC frequency
>> table parsing code. The EMC frequency table parsing code needs to be
>> always built-in because it is stashed into a linker section. However
>> we still want the driver to be able to be a loadable module. The only
>> way I know how to achieve that is by splitting up the sources files
>> like you do here, so I this looks like a correct fix:
>>
>> Reviewed-by: Thierry Reding <treding@nvidia.com>
> 
> 
> Just a reminder that we need to get this fixed for v5.13. Please let me
> know if you have any more comments on this fix.
> 
> Thanks!
> Jon
> 

-- 
nvpublic

Re: [PATCH V4] PCI: tegra: Fix building Tegra194 PCIe driver

[Bjorn Helgaas]

On Thu, Jun 10, 2021 at 07:41:34AM +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.
> 2. 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. Given that the ACPI quirk code for Tegra194 is completely
>    independent of the native Tegra194 PCIe driver, move this code into
>    its own file so that it can be built independently and we can remove
>    the "#ifdef CONFIG_PCIE_TEGRA194" in the native driver. Note that
>    given the native Tegra194 PCIe driver is only used with device-tree,
>    this will not cause any conflicts.
> 3. The below build warnings to be seen with particular kernel
>    configurations. Fix these by moving these structure definitions to
>    within the necessary guards.
> 
>   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>

Applied with Thierry's reviewed-by and the following subject to
for-linus for v5.13, thanks!

  PCI: tegra194: Fix MCFG quirk build regressions

> ---
> Changes since V3:
> - Moved ACPI quirk code into separate source file
> 
> Changes since V2:
> - Update the commit message per Bjorn's feedback
> - Moved the structure definitions within the necessary guards as opposed
>   to wrapping the existing defintions with the appropriate guards.
> 
> Changes since V1:
> - Added fixes tag
> - Fixed 'defined but not used' compiler warnings
> 
>  drivers/pci/controller/dwc/Makefile           |   3 +-
>  .../pci/controller/dwc/pcie-tegra194-acpi.c   | 108 ++++++++++++++
>  drivers/pci/controller/dwc/pcie-tegra194.c    | 138 +++---------------
>  3 files changed, 128 insertions(+), 121 deletions(-)
>  create mode 100644 drivers/pci/controller/dwc/pcie-tegra194-acpi.c
> 
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index eca805c1a023..9e6ce0dc2f53 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
>  obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
>  obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
>  obj-$(CONFIG_PCI_MESON) += pci-meson.o
> +obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
>  obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>  obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
>  
> @@ -38,6 +39,6 @@ ifdef CONFIG_ACPI
>  ifdef CONFIG_PCI_QUIRKS
>  obj-$(CONFIG_ARM64) += pcie-al.o
>  obj-$(CONFIG_ARM64) += pcie-hisi.o
> -obj-$(CONFIG_ARM64) += pcie-tegra194.o
> +obj-$(CONFIG_ARM64) += pcie-tegra194-acpi.o
>  endif
>  endif
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194-acpi.c b/drivers/pci/controller/dwc/pcie-tegra194-acpi.c
> new file mode 100644
> index 000000000000..c2de6ed4d86f
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-tegra194-acpi.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * ACPI quirks for Tegra194 PCIe host controller
> + *
> + * Copyright (C) 2021 NVIDIA Corporation.
> + *
> + * Author: Vidya Sagar <vidyas@nvidia.com>
> + */
> +
> +#include <linux/pci.h>
> +#include <linux/pci-acpi.h>
> +#include <linux/pci-ecam.h>
> +
> +#include "pcie-designware.h"
> +
> +struct tegra194_pcie_ecam  {
> +	void __iomem *config_base;
> +	void __iomem *iatu_base;
> +	void __iomem *dbi_base;
> +};
> +
> +static int tegra194_acpi_init(struct pci_config_window *cfg)
> +{
> +	struct device *dev = cfg->parent;
> +	struct tegra194_pcie_ecam *pcie_ecam;
> +
> +	pcie_ecam = devm_kzalloc(dev, sizeof(*pcie_ecam), GFP_KERNEL);
> +	if (!pcie_ecam)
> +		return -ENOMEM;
> +
> +	pcie_ecam->config_base = cfg->win;
> +	pcie_ecam->iatu_base = cfg->win + SZ_256K;
> +	pcie_ecam->dbi_base = cfg->win + SZ_512K;
> +	cfg->priv = pcie_ecam;
> +
> +	return 0;
> +}
> +
> +static void atu_reg_write(struct tegra194_pcie_ecam *pcie_ecam, int index,
> +			  u32 val, u32 reg)
> +{
> +	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
> +
> +	writel(val, pcie_ecam->iatu_base + offset + reg);
> +}
> +
> +static void program_outbound_atu(struct tegra194_pcie_ecam *pcie_ecam,
> +				 int index, int type, u64 cpu_addr,
> +				 u64 pci_addr, u64 size)
> +{
> +	atu_reg_write(pcie_ecam, index, lower_32_bits(cpu_addr),
> +		      PCIE_ATU_LOWER_BASE);
> +	atu_reg_write(pcie_ecam, index, upper_32_bits(cpu_addr),
> +		      PCIE_ATU_UPPER_BASE);
> +	atu_reg_write(pcie_ecam, index, lower_32_bits(pci_addr),
> +		      PCIE_ATU_LOWER_TARGET);
> +	atu_reg_write(pcie_ecam, index, lower_32_bits(cpu_addr + size - 1),
> +		      PCIE_ATU_LIMIT);
> +	atu_reg_write(pcie_ecam, index, upper_32_bits(pci_addr),
> +		      PCIE_ATU_UPPER_TARGET);
> +	atu_reg_write(pcie_ecam, index, type, PCIE_ATU_CR1);
> +	atu_reg_write(pcie_ecam, index, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> +}
> +
> +static void __iomem *tegra194_map_bus(struct pci_bus *bus,
> +				      unsigned int devfn, int where)
> +{
> +	struct pci_config_window *cfg = bus->sysdata;
> +	struct tegra194_pcie_ecam *pcie_ecam = cfg->priv;
> +	u32 busdev;
> +	int type;
> +
> +	if (bus->number < cfg->busr.start || bus->number > cfg->busr.end)
> +		return NULL;
> +
> +	if (bus->number == cfg->busr.start) {
> +		if (PCI_SLOT(devfn) == 0)
> +			return pcie_ecam->dbi_base + where;
> +		else
> +			return NULL;
> +	}
> +
> +	busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> +		 PCIE_ATU_FUNC(PCI_FUNC(devfn));
> +
> +	if (bus->parent->number == cfg->busr.start) {
> +		if (PCI_SLOT(devfn) == 0)
> +			type = PCIE_ATU_TYPE_CFG0;
> +		else
> +			return NULL;
> +	} else {
> +		type = PCIE_ATU_TYPE_CFG1;
> +	}
> +
> +	program_outbound_atu(pcie_ecam, 0, type, cfg->res.start, busdev,
> +			     SZ_256K);
> +
> +	return pcie_ecam->config_base + where;
> +}
> +
> +const struct pci_ecam_ops tegra194_pcie_ops = {
> +	.init		= tegra194_acpi_init,
> +	.pci_ops	= {
> +		.map_bus	= tegra194_map_bus,
> +		.read		= pci_generic_config_read,
> +		.write		= pci_generic_config_write,
> +	}
> +};
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index b19775ab134e..fa6b12cfc043 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -22,8 +22,6 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_pci.h>
>  #include <linux/pci.h>
> -#include <linux/pci-acpi.h>
> -#include <linux/pci-ecam.h>
>  #include <linux/phy/phy.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
> @@ -247,24 +245,6 @@ static const unsigned int pcie_gen_freq[] = {
>  	GEN4_CORE_CLK_FREQ
>  };
>  
> -static const u32 event_cntr_ctrl_offset[] = {
> -	0x1d8,
> -	0x1a8,
> -	0x1a8,
> -	0x1a8,
> -	0x1c4,
> -	0x1d8
> -};
> -
> -static const u32 event_cntr_data_offset[] = {
> -	0x1dc,
> -	0x1ac,
> -	0x1ac,
> -	0x1ac,
> -	0x1c8,
> -	0x1dc
> -};
> -
>  struct tegra_pcie_dw {
>  	struct device *dev;
>  	struct resource *appl_res;
> @@ -313,104 +293,6 @@ struct tegra_pcie_dw_of_data {
>  	enum dw_pcie_device_mode mode;
>  };
>  
> -#if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS)
> -struct tegra194_pcie_ecam  {
> -	void __iomem *config_base;
> -	void __iomem *iatu_base;
> -	void __iomem *dbi_base;
> -};
> -
> -static int tegra194_acpi_init(struct pci_config_window *cfg)
> -{
> -	struct device *dev = cfg->parent;
> -	struct tegra194_pcie_ecam *pcie_ecam;
> -
> -	pcie_ecam = devm_kzalloc(dev, sizeof(*pcie_ecam), GFP_KERNEL);
> -	if (!pcie_ecam)
> -		return -ENOMEM;
> -
> -	pcie_ecam->config_base = cfg->win;
> -	pcie_ecam->iatu_base = cfg->win + SZ_256K;
> -	pcie_ecam->dbi_base = cfg->win + SZ_512K;
> -	cfg->priv = pcie_ecam;
> -
> -	return 0;
> -}
> -
> -static void atu_reg_write(struct tegra194_pcie_ecam *pcie_ecam, int index,
> -			  u32 val, u32 reg)
> -{
> -	u32 offset = PCIE_GET_ATU_OUTB_UNR_REG_OFFSET(index);
> -
> -	writel(val, pcie_ecam->iatu_base + offset + reg);
> -}
> -
> -static void program_outbound_atu(struct tegra194_pcie_ecam *pcie_ecam,
> -				 int index, int type, u64 cpu_addr,
> -				 u64 pci_addr, u64 size)
> -{
> -	atu_reg_write(pcie_ecam, index, lower_32_bits(cpu_addr),
> -		      PCIE_ATU_LOWER_BASE);
> -	atu_reg_write(pcie_ecam, index, upper_32_bits(cpu_addr),
> -		      PCIE_ATU_UPPER_BASE);
> -	atu_reg_write(pcie_ecam, index, lower_32_bits(pci_addr),
> -		      PCIE_ATU_LOWER_TARGET);
> -	atu_reg_write(pcie_ecam, index, lower_32_bits(cpu_addr + size - 1),
> -		      PCIE_ATU_LIMIT);
> -	atu_reg_write(pcie_ecam, index, upper_32_bits(pci_addr),
> -		      PCIE_ATU_UPPER_TARGET);
> -	atu_reg_write(pcie_ecam, index, type, PCIE_ATU_CR1);
> -	atu_reg_write(pcie_ecam, index, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> -}
> -
> -static void __iomem *tegra194_map_bus(struct pci_bus *bus,
> -				      unsigned int devfn, int where)
> -{
> -	struct pci_config_window *cfg = bus->sysdata;
> -	struct tegra194_pcie_ecam *pcie_ecam = cfg->priv;
> -	u32 busdev;
> -	int type;
> -
> -	if (bus->number < cfg->busr.start || bus->number > cfg->busr.end)
> -		return NULL;
> -
> -	if (bus->number == cfg->busr.start) {
> -		if (PCI_SLOT(devfn) == 0)
> -			return pcie_ecam->dbi_base + where;
> -		else
> -			return NULL;
> -	}
> -
> -	busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> -		 PCIE_ATU_FUNC(PCI_FUNC(devfn));
> -
> -	if (bus->parent->number == cfg->busr.start) {
> -		if (PCI_SLOT(devfn) == 0)
> -			type = PCIE_ATU_TYPE_CFG0;
> -		else
> -			return NULL;
> -	} else {
> -		type = PCIE_ATU_TYPE_CFG1;
> -	}
> -
> -	program_outbound_atu(pcie_ecam, 0, type, cfg->res.start, busdev,
> -			     SZ_256K);
> -
> -	return pcie_ecam->config_base + where;
> -}
> -
> -const struct pci_ecam_ops tegra194_pcie_ops = {
> -	.init		= tegra194_acpi_init,
> -	.pci_ops	= {
> -		.map_bus	= tegra194_map_bus,
> -		.read		= pci_generic_config_read,
> -		.write		= pci_generic_config_write,
> -	}
> -};
> -#endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> -
> -#ifdef CONFIG_PCIE_TEGRA194
> -
>  static inline struct tegra_pcie_dw *to_tegra_pcie(struct dw_pcie *pci)
>  {
>  	return container_of(pci, struct tegra_pcie_dw, pci);
> @@ -694,6 +576,24 @@ static struct pci_ops tegra_pci_ops = {
>  };
>  
>  #if defined(CONFIG_PCIEASPM)
> +static const u32 event_cntr_ctrl_offset[] = {
> +	0x1d8,
> +	0x1a8,
> +	0x1a8,
> +	0x1a8,
> +	0x1c4,
> +	0x1d8
> +};
> +
> +static const u32 event_cntr_data_offset[] = {
> +	0x1dc,
> +	0x1ac,
> +	0x1ac,
> +	0x1ac,
> +	0x1c8,
> +	0x1dc
> +};
> +
>  static void disable_aspm_l11(struct tegra_pcie_dw *pcie)
>  {
>  	u32 val;
> @@ -2413,5 +2313,3 @@ MODULE_DEVICE_TABLE(of, tegra_pcie_dw_of_match);
>  MODULE_AUTHOR("Vidya Sagar <vidyas@nvidia.com>");
>  MODULE_DESCRIPTION("NVIDIA PCIe host controller driver");
>  MODULE_LICENSE("GPL v2");
> -
> -#endif /* CONFIG_PCIE_TEGRA194 */
> -- 
> 2.25.1
>