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

[Thierry Reding <thierry.reding@gmail.com>]

[-- 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 --]