Re: [PATCH V16 7/7] PCI: Add quirk for multifunction devices of LS7A

From: Bjorn Helgaas <helgaas@kernel.org>
To: Jianmin Lv <lvjianmin@loongson.cn>
Cc: "Huacai Chen" <chenhuacai@loongson.cn>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	linux-pci@vger.kernel.org, "Xuefeng Li" <lixuefeng@loongson.cn>,
	"Huacai Chen" <chenhuacai@gmail.com>,
	"Jiaxun Yang" <jiaxun.yang@flygoat.com>
Subject: Re: [PATCH V16 7/7] PCI: Add quirk for multifunction devices of LS7A
Date: Fri, 15 Jul 2022 22:23:34 -0500	[thread overview]
Message-ID: <20220716032334.GA1228076@bhelgaas> (raw)
In-Reply-To: <3a49465c-cd70-927f-1fd9-9454fa9c44ab@loongson.cn>

On Sat, Jul 16, 2022 at 10:27:00AM +0800, Jianmin Lv wrote:
> On 2022/7/16 上午12:37, Bjorn Helgaas wrote:
> > On Fri, Jul 15, 2022 at 04:05:12PM +0800, Jianmin Lv wrote:
> > > On 2022/7/15 上午11:44, Bjorn Helgaas wrote:
> > > > On Thu, Jul 14, 2022 at 08:42:16PM +0800, Huacai Chen wrote:
> > > > > From: Jianmin Lv <lvjianmin@loongson.cn>
> > > > > 
> > > > > In LS7A, multifunction device use same PCI PIN (because the
> > > > > PIN register report the same INTx value to each function)
> > > > > but we need different IRQ for different functions, so add a
> > > > > quirk to fix it for standard PCI PIN usage.
> > > > > 
> > > > > This patch only affect ACPI based systems (and only needed
> > > > > by ACPI based systems, too). For DT based systems, the irq
> > > > > mappings is defined in .dts files and be handled by
> > > > > of_irq_parse_pci().
> > > > 
> > > > I'm sorry, I know you've explained this before, but I don't
> > > > understand yet, so let's try again.  I *think* you're saying
> > > > that:
> > > > 
> > > >     - These devices integrated into LS7A all report 0 in their
> > > >     Interrupt Pin registers.  Per spec, this means they do not
> > > >     use INTx (PCIe r6.0, sec 7.5.1.1.13).
> > > > 
> > > >     - However, these devices actually *do* use INTx.  Function
> > > >     0 uses INTA, function 1 uses INTB, ..., function 4 uses
> > > >     INTA, ...
> > > > 
> > > >     - The quirk overrides the incorrect values read from the
> > > >     Interrupt Pin registers.
> > > 
> > > Yes, right.
> 
> Sorry, I didn't see the first item here carefully, so I have to
> correct it: all the integrated devices in 7A report 1 in PIN reg
> instead of 0.

> > > > But I'm still confused about how loongson_map_irq() gets called.  The
> > > > only likely path I see is here:
> > > > 
> > > >     pci_device_probe                            # pci_bus_type.probe
> > > >       pci_assign_irq
> > > >         pci_read_config_byte(dev, PCI_INTERRUPT_PIN, &pin)
> > > >         if (pin)
> > > > 	bridge->swizzle_irq(dev, &pin)
> > > > 	irq = bridge->map_irq(dev, slot, pin)
> > > > 
> > > > where bridge->map_irq points to loongson_map_irq().  But
> > > > pci_assign_irq() should read 0 from PCI_INTERRUPT_PIN [1], so it
> > > > wouldn't call bridge->map_irq().  Obviously I'm missing something.
> > > > 
> 
> Same thing, PCI_INTERRUPT_PIN reports 1, so bridge->map_irq() will be
> called.

OK, that makes a lot more sense, thank you!

But it does leave another question: the quirk applies to
DEV_PCIE_PORT_0 (0x7a09), DEV_PCIE_PORT_1 (0x7a19), and
DEV_PCIE_PORT_2 (0x7a29).

According to the .dtsi [1], all those root ports are at function 0,
and if they report INTA, the quirk will also compute INTA.  So why do
you need to apply the quirk for them?

The same would apply to any Device ID that only appears at function 0,
which looks like it also includes DEV_LS7A_OHCI (0x7a24), and
DEV_LS7A_GPU (0x7a15).

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/boot/dts/loongson/ls7a-pch.dtsi?id=v5.18#n231