Re: [PATCH V1 09/10] PCI: Disable MSI for Tegra234 root ports

[Vidya Sagar <vidyas@nvidia.com>]

On 2/7/2022 11:06 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Sat, Feb 05, 2022 at 09:51:43PM +0530, Vidya Sagar wrote:
>> Tegra234 PCIe rootports don't generate MSI interrupts for PME and AER
>> events. Since PCIe spec (Ref: r4.0 sec 7.7.1.2 and 7.7.2.2) doesn't support
>> using a mix of INTx and MSI/MSI-X, MSI needs to be disabled to avoid root
>> ports service drivers registering their respective ISRs with MSI interrupt
>> and to let only INTx be used for all events.
> 
> s/rootports/root ports/ to match other usage here.
> 
> This argument matches that in 8c7e96d3fe75 ("PCI: Disable MSI for
> Tegra root ports") [1], but that's not quite what sec 7.7.1.2 and
> 7.7.2.2 say.  Those sections talk about what happens when both MSI and
> MSI-X are disabled:
> 
>    If MSI and MSI-X are both disabled, the Function requests servicing
>    using INTx interrupts (if supported).
> 
> but they don't say anything about what happens when MSI or MSI-X is
> *enabled*.
> 
> I think a better citation is PCIe r6.0, sec 6.1.4.3, which says:
> 
>    While enabled for MSI or MSI-X operation, a Function is prohibited
>    from using INTx interrupts (if implemented) to request service (MSI,
>    MSI-X, and INTx are mutually exclusive).
> 
> Can you please update the comment in the code and this commit log to
> cite PCIe r6.0, sec 6.1.4.3 instead, and to clarify that these Tegra
> devices always use INTx for PME and AER, even when MSI/MSI-X is
> enabled?

Sure. I would fix this in the next patch set.

> 
> Why do these Tegra quirks use DECLARE_PCI_FIXUP_CLASS_EARLY() instead
> of just DECLARE_PCI_FIXUP_EARLY()?  quirk_al_msi_disable() uses the
> _CLASS version because the same Device ID is used for non-Root Port
> devices.  Is the same true here, or could these use
> DECLARE_PCI_FIXUP_EARLY()?

Tegra's PCIe controllers are also dual mode controllers and MSI works 
just fine when they operate in the endpoint mode configuration.

> 
> There are many quirks that disable MSI, and they're a mixture of EARLY
> and FINAL.  They should probably all be the same.
> 
> [1] https://git.kernel.org/linus/8c7e96d3fe75
> 
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>>   drivers/pci/quirks.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index d2dd6a6cda60..3ac5c45e61a1 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -2747,6 +2747,15 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_NVIDIA, 0x10e5,
>>   DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_NVIDIA, 0x10e6,
>>                              PCI_CLASS_BRIDGE_PCI, 8,
>>                              pci_quirk_nvidia_tegra_disable_rp_msi);
>> +DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_NVIDIA, 0x229a,
>> +                           PCI_CLASS_BRIDGE_PCI, 8,
>> +                           pci_quirk_nvidia_tegra_disable_rp_msi);
>> +DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_NVIDIA, 0x229c,
>> +                           PCI_CLASS_BRIDGE_PCI, 8,
>> +                           pci_quirk_nvidia_tegra_disable_rp_msi);
>> +DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_NVIDIA, 0x229e,
>> +                           PCI_CLASS_BRIDGE_PCI, 8,
>> +                           pci_quirk_nvidia_tegra_disable_rp_msi);
>>
>>   /*
>>    * Some versions of the MCP55 bridge from Nvidia have a legacy IRQ routing
>> --
>> 2.17.1
>>