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

[Bjorn Helgaas]

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?

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()?

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@xxxxxxxxxx>
> ---
>  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
>