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

[Huacai Chen <chenhuacai@gmail.com>]

Hi, Bjorn,

On Sun, Jul 17, 2022 at 9:41 AM Jianmin Lv <lvjianmin@loongson.cn> wrote:
>
>
>
> On 2022/7/17 上午7:32, Bjorn Helgaas wrote:
> > 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/
> >
>
> Hi, Bjorn
>
> My original patch(obviously just for simple, unwilling to list so much
> devices in the patch) is following;
>
> +static void loongson_pci_pin_quirk(struct pci_dev *dev)
> +{
> +    u8 fun = dev->devfn & 7;
> +
> +    dev->pin = 1 + (fun & 3);
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, PCI_ANY_ID,
> loongson_pci_pin_quirk);
> +
>
> Mybe Huacai think only PCIE ports need the fix, so he adds the PCIE
> ports in the patch when submitting it.
Yes, what Jianmin said is correct, the first version of my patch is
wrong and the current version is correct.

Huacai
>
> The things has been explained above, only multi-func devices are
> required to fix.
>
> Thanks.
>