Hi, Bjorn, On Tue, Jul 19, 2022 at 1:00 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Sun, Jul 17, 2022 at 10:11:17PM +0800, Huacai Chen wrote: > > On Sun, Jul 17, 2022 at 9:41 AM Jianmin Lv <lvjianmin@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > > >>>>>>>>> > > > >>>>>>>>> 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@xxxxxxxxxxx/ > > > > > > 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. > > Thanks for clearing that up. I dropped DEV_PCIE_PORT_x, > DEV_LS7A_OHCI, DEV_LS7A_GPU from the quirk. Unfortunately, this patch only lists devices in LS7A1000, but some of LS7A2000 (GNET and HDMI) also need to quirk, can they be squashed in this patch? If not, we will add them in a new patch. #define DEV_LS7A_CONF 0x7a10 #define DEV_LS7A_LPC 0x7a0c #define DEV_LS7A_GMAC 0x7a03 +#define DEV_LS7A_GNET 0x7a13 #define DEV_LS7A_DC1 0x7a06 #define DEV_LS7A_DC2 0x7a36 #define DEV_LS7A_GPU 0x7a15 #define DEV_LS7A_AHCI 0x7a08 #define DEV_LS7A_EHCI 0x7a14 #define DEV_LS7A_OHCI 0x7a24 +#define DEV_LS7A_HDMI 0x7a37 Huacai
