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
