From: Jon Hunter <jonathanh@nvidia.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Thierry Reding" <thierry.reding@gmail.com>,
"Vidya Sagar" <vidyas@nvidia.com>,
"Krzysztof Wilczyński" <kw@linux.com>,
linux-pci@vger.kernel.org, linux-tegra@vger.kernel.org
Subject: Re: [PATCH V2] PCI: tegra: Fix building Tegra194 PCIe driver
Date: Fri, 21 May 2021 14:11:15 +0100 [thread overview]
Message-ID: <a3f39a47-8dff-a4b0-a529-0f17f42114f3@nvidia.com> (raw)
In-Reply-To: <20210520221948.GA352305@bjorn-Precision-5520>
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
--
nvpublic
next prev parent reply other threads:[~2021-05-21 13:11 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-20 9:01 [PATCH V2] PCI: tegra: Fix building Tegra194 PCIe driver Jon Hunter
2021-05-20 9:57 ` Krzysztof Wilczyński
2021-05-20 22:19 ` Bjorn Helgaas
2021-05-21 13:11 ` Jon Hunter [this message]
2021-06-07 23:50 ` Bjorn Helgaas
2021-06-08 7:44 ` Jon Hunter
2021-06-08 13:02 ` Bjorn Helgaas
2021-06-08 13:20 ` Jon Hunter
2021-06-08 18:34 ` Vidya Sagar
2021-06-08 20:11 ` Jon Hunter
2021-06-09 10:23 ` Jon Hunter
2021-06-09 14:00 ` Vidya Sagar
2021-06-09 16:18 ` Bjorn Helgaas
2021-06-09 17:07 ` Jon Hunter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a3f39a47-8dff-a4b0-a529-0f17f42114f3@nvidia.com \
--to=jonathanh@nvidia.com \
--cc=bhelgaas@google.com \
--cc=helgaas@kernel.org \
--cc=kw@linux.com \
--cc=linux-pci@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=thierry.reding@gmail.com \
--cc=vidyas@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).