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

[PATCH V2] 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 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(-)

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)
 {
-- 
2.25.1

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

[Krzysztof Wilczyński]

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

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

[Bjorn Helgaas]

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
> 

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

[Jon Hunter]

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

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

[Bjorn Helgaas]

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

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

[Jon Hunter]

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

-- 
nvpublic

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

[Bjorn Helgaas]

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

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

[Jon Hunter]

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

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

[Vidya Sagar]

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
> 

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

[Jon Hunter]

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

-- 
nvpublic

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

[Jon Hunter]

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

-- 
nvpublic

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

[Vidya Sagar]

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
> 

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

[Bjorn Helgaas]

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

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

[Jon Hunter]

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

-- 
nvpublic