Re: [PATCH] PCI: loongson: Add LS7A MSI enablement quirk

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jun 12, 2024 at 02:53:15PM +0800, Huacai Chen wrote:
> LS7A chipset can be used as a downstream bridge which connected to a
> high-level host bridge. In this case DEV_LS7A_PCIE_PORT5 is used as the
> upward port. We should always enable MSI caps of this port, otherwise
> downstream devices cannot use MSI.

Can you clarify this topology a bit?  Since DEV_LS7A_PCIE_PORT5
apparently has a class of PCI_CLASS_BRIDGE_HOST, I guess that in PCIe
terms, it is basically a PCI host bridge (Root Complex, if you prefer)
that is materialized as a PCI Endpoint?

I'm curious about what's going on here because the normal PCI MSI
support should set PCI_MSI_FLAGS_ENABLE since it's completely
specified by the spec, which says it controls whether *this function*
can use MSI.

But in this case PCI_MSI_FLAGS_ENABLE seems to have non-architected
behavior of controlling MSI from *other* devices below this host
bridge?  That's a little bit weird too because MSI looks like DMA to
any bridges upstream from the device that is using MSI, and the Bus
Master Enable bit in those bridges controls whether they forward those
MSI DMA accesses upstream.  And of course the PCI core should already
make sure those bridges have Bus Master Enable set when downstream
devices use MSI.

> Cc: <stable@vger.kernel.org>
> Signed-off-by: Sheng Wu <wusheng@loongson.cn>
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
>  drivers/pci/controller/pci-loongson.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c
> index 8b34ccff073a..ffc581605834 100644
> --- a/drivers/pci/controller/pci-loongson.c
> +++ b/drivers/pci/controller/pci-loongson.c
> @@ -163,6 +163,18 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON,
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON,
>  			DEV_LS7A_HDMI, loongson_pci_pin_quirk);
>  
> +static void loongson_pci_msi_quirk(struct pci_dev *dev)
> +{
> +	u16 val, class = dev->class >> 8;
> +
> +	if (class == PCI_CLASS_BRIDGE_HOST) {
> +		pci_read_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, &val);
> +		val |= PCI_MSI_FLAGS_ENABLE;
> +		pci_write_config_word(dev, dev->msi_cap + PCI_MSI_FLAGS, val);
> +	}
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, DEV_LS7A_PCIE_PORT5, loongson_pci_msi_quirk);
> +
>  static struct loongson_pci *pci_bus_to_loongson_pci(struct pci_bus *bus)
>  {
>  	struct pci_config_window *cfg;
> -- 
> 2.43.0
>