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

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jul 10, 2024 at 11:04:24AM +0800, Huacai Chen wrote:
> Hi, Bjorn,
> 
> On Wed, Jul 10, 2024 at 5:24 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > 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?
>
> Now most of the existing LoongArch CPUs don't have an integrated PCIe
> RC, instead they have HyperTransport controllers. But the latest CPU
> (Loongson-3C6000) has an integrated PCIe RC and removed
> HyperTransport.
> 
> LS7A bridge can work together with both old (HT) CPUs and new (PCIe)
> CPUs. If it is connected to an old CPU, its upstream port is a HT
> port, and DEV_LS7A_PCIE_PORT5 works as a normal downstream PCIe port.
> If it is connected to a new CPU, DEV_LS7A_PCIE_PORT5 works as an
> upstream port (the class code becomes PCI_CLASS_BRIDGE_HOST) and the
> HT port is idle.

What does lspci look like for both the old HT and the new PCIe CPUs?

With the old HT CPU, I imagine this:

  [LS7A includes a HT port that doesn't appear as a PCI device and
  basically implements a PCIe Root Complex]
  00:00.0 Root Port to [bus 01-1f] (DEV_LS7A_PCIE_PORT5)

With a new PCIe CPU, if DEV_LS7A_PCIE_PORT5 is a PCIe Upstream Port,
it would be part of a switch, so I'm imagining something like this:

  00:00.0 Root Port to [bus 01-1f] (integrated into Loongson-3C6000)
  01:00.0 Upstream Port to [bus 02-1f] (DEV_LS7A_PCIE_PORT5)
  02:00.0 Downstream Port to [bus 03-1f] (part of the LS7A switch)

In both cases, 00:00.0 and 01:00.0 (DEV_LS7A_PCIE_PORT5) would be a
Type 1 device that is enumerated as a PCI-to-PCI bridge, which would
normally have a Class Code of 0x0604 (PCI_CLASS_BRIDGE_PCI).

But you're saying DEV_LS7A_PCIE_PORT5 has a Class Code of
PCI_CLASS_BRIDGE_HOST, which is 0x0600.  That would normally be a Type
0 device and would not have a secondary bus.

> > 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.
>
> In my opinion this is a hardware bug, when DEV_LS7A_PCIE_PORT5 works
> as a host bridge, it should enable MSI automatically. But
> unfortunately hardware doesn't behave like this, so we need a quirk
> here.

I'm fine with the quirk to work around this issue.  But the commit log
is confusing.

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