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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> ---
>> 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
>>