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

[Vidya Sagar <vidyas@nvidia.com>]

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
>