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

[Bjorn Helgaas <helgaas@kernel.org>]

On Sat, Jul 16, 2022 at 04:37:41PM +0800, Huacai Chen wrote:
> On Sat, Jul 16, 2022 at 2:12 PM Jianmin Lv <lvjianmin@loongson.cn> wrote:
> > On 2022/7/16 上午11:23, Bjorn Helgaas wrote:
> > > 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?
> >
> > Oh, yes, I don't think they are required either. The fix is only
> > required for multi-func devices of 7A.
> >
> > Huacai, we should remove PCIE ports from the patch.
>
> I agree to remove PCIE ports here. But since Bjorn has already merged
> this patch, and the redundant devices listed here have no
> side-effects, I suggest keeping it as is (but Bjorn is free to modify
> if necessary).

I'd be happy to update the branch to remove the devices mentioned
(DEV_PCIE_PORT_x, DEV_LS7A_OHCI, DEV_LS7A_GPU).

But the original patch [2] *only* listed DEV_PCIE_PORT_x, so I'm
really confused about what's going on with them.  I assume [2] fixed
*something*, but now we're suggesting that we don't need it.

[2] https://lore.kernel.org/all/20210514080025.1828197-5-chenhuacai@loongson.cn/