| Message ID | 20220714124216.1489304-8-chenhuacai@loongson.cn |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Loongson pci improvements and quirks | expand |
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. That much makes sense to me. And I even see that in of_irq_parse_pci(), if there's a DT node for the device, of_irq_parse_one() gets the interrupt info from DT and returns the IRQ all the way back up to (I think) loongson_map_irq(). 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. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/setup-irq.c?id=v5.18#n37 > Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> > --- > drivers/pci/controller/pci-loongson.c | 32 +++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c > index 05997b51c86d..4043b57bcc86 100644 > --- a/drivers/pci/controller/pci-loongson.c > +++ b/drivers/pci/controller/pci-loongson.c > @@ -22,6 +22,13 @@ > #define DEV_LS2K_APB 0x7a02 > #define DEV_LS7A_CONF 0x7a10 > #define DEV_LS7A_LPC 0x7a0c > +#define DEV_LS7A_GMAC 0x7a03 > +#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 FLAG_CFG0 BIT(0) > #define FLAG_CFG1 BIT(1) > @@ -103,6 +110,31 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, > DEV_PCIE_PORT_2, loongson_bmaster_quirk); > > +static void loongson_pci_pin_quirk(struct pci_dev *pdev) > +{ > + pdev->pin = 1 + (PCI_FUNC(pdev->devfn) & 3); > +} > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, > + DEV_LS7A_DC1, loongson_pci_pin_quirk); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, > + DEV_LS7A_DC2, loongson_pci_pin_quirk); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, > + DEV_LS7A_GPU, loongson_pci_pin_quirk); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, > + DEV_LS7A_GMAC, loongson_pci_pin_quirk); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, > + DEV_LS7A_AHCI, loongson_pci_pin_quirk); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, > + DEV_LS7A_EHCI, loongson_pci_pin_quirk); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, > + DEV_LS7A_OHCI, loongson_pci_pin_quirk); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, > + DEV_PCIE_PORT_0, loongson_pci_pin_quirk); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, > + DEV_PCIE_PORT_1, loongson_pci_pin_quirk); > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, > + DEV_PCIE_PORT_2, loongson_pci_pin_quirk); > + > static struct loongson_pci *pci_bus_to_loongson_pci(struct pci_bus *bus) > { > struct pci_config_window *cfg; > -- > 2.31.1 >
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. > That much makes sense to me. > > And I even see that in of_irq_parse_pci(), if there's a DT node for > the device, of_irq_parse_one() gets the interrupt info from DT and > returns the IRQ all the way back up to (I think) loongson_map_irq(). > Agree, I think so for DT. > 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. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/setup-irq.c?id=v5.18#n37 > For ACPI, bridge->map_irq is NULL, so in above path, the pci_assign_irq will return because of !(hbrg->map_irq) as following: if (!(hbrg->map_irq)) { pci_dbg(dev, "runtime IRQ mapping not provided by arch\n"); return; } And again as I explained in previous version patch, dev->irq is set in acpi_pci_irq_enable() in the following path for ACPI: pci_device_probe ->pcibios_alloc_irq ->acpi_pci_irq_enable ->acpi_pci_irq_lookup And the reason that we fixed the pin is to get an correct entry in prt table when calling acpi_pci_irq_lookup. With out the fix, we can't find out a entry. After found an entry, we get gsi, and map irq as following: rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity); if (rc < 0) { dev_warn(&dev->dev, "PCI INT %c: failed to register GSI\n", pin_name(pin)); kfree(entry); return rc; } dev->irq = rc; Here, dev->irq is set like in pci_assign_irq for DT. >> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn> >> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> >> --- >> drivers/pci/controller/pci-loongson.c | 32 +++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c >> index 05997b51c86d..4043b57bcc86 100644 >> --- a/drivers/pci/controller/pci-loongson.c >> +++ b/drivers/pci/controller/pci-loongson.c >> @@ -22,6 +22,13 @@ >> #define DEV_LS2K_APB 0x7a02 >> #define DEV_LS7A_CONF 0x7a10 >> #define DEV_LS7A_LPC 0x7a0c >> +#define DEV_LS7A_GMAC 0x7a03 >> +#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 FLAG_CFG0 BIT(0) >> #define FLAG_CFG1 BIT(1) >> @@ -103,6 +110,31 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, >> DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, >> DEV_PCIE_PORT_2, loongson_bmaster_quirk); >> >> +static void loongson_pci_pin_quirk(struct pci_dev *pdev) >> +{ >> + pdev->pin = 1 + (PCI_FUNC(pdev->devfn) & 3); >> +} >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, >> + DEV_LS7A_DC1, loongson_pci_pin_quirk); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, >> + DEV_LS7A_DC2, loongson_pci_pin_quirk); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, >> + DEV_LS7A_GPU, loongson_pci_pin_quirk); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, >> + DEV_LS7A_GMAC, loongson_pci_pin_quirk); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, >> + DEV_LS7A_AHCI, loongson_pci_pin_quirk); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, >> + DEV_LS7A_EHCI, loongson_pci_pin_quirk); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, >> + DEV_LS7A_OHCI, loongson_pci_pin_quirk); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, >> + DEV_PCIE_PORT_0, loongson_pci_pin_quirk); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, >> + DEV_PCIE_PORT_1, loongson_pci_pin_quirk); >> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, >> + DEV_PCIE_PORT_2, loongson_pci_pin_quirk); >> + >> static struct loongson_pci *pci_bus_to_loongson_pci(struct pci_bus *bus) >> { >> struct pci_config_window *cfg; >> -- >> 2.31.1 >>
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. > > > That much makes sense to me. > > > > And I even see that in of_irq_parse_pci(), if there's a DT node for > > the device, of_irq_parse_one() gets the interrupt info from DT and > > returns the IRQ all the way back up to (I think) loongson_map_irq(). > > Agree, I think so for DT. > > > 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. > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/setup-irq.c?id=v5.18#n37 > > For ACPI, bridge->map_irq is NULL, so in above path, > the pci_assign_irq will return because of !(hbrg->map_irq) as following: > > if (!(hbrg->map_irq)) { > pci_dbg(dev, "runtime IRQ mapping not provided by arch\n"); > return; > } > > And again as I explained in previous version patch, dev->irq is set in > acpi_pci_irq_enable() in the following path for ACPI: > > pci_device_probe > ->pcibios_alloc_irq > ->acpi_pci_irq_enable > ->acpi_pci_irq_lookup > > And the reason that we fixed the pin is to get an correct entry in prt > table when calling acpi_pci_irq_lookup. With out the fix, we can't find > out a entry. > > After found an entry, we get gsi, and map irq as following: > > rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity); > if (rc < 0) { > dev_warn(&dev->dev, "PCI INT %c: failed to register GSI\n", > pin_name(pin)); > kfree(entry); > return rc; > } > dev->irq = rc; > > Here, dev->irq is set like in pci_assign_irq for DT. Yes. The above explains how things work for ACPI, but I'm not asking about that. I'm asking how this works in the *DT* case. I see that pci_assign_irq() is called for both ACPI and DT, and I see that it does nothing in the ACPI path because bridge->map_irq hasn't been set. What I *don't* see is how pci_assign_irq() works in the DT case because it reads PCI_INTERRUPT_PIN, which should return 0 for these broken devices, and if "pin == 0", it never calls ->map_irq(). Is ->map_irq() called via some other path? Bjorn
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. >>> That much makes sense to me. >>> >>> And I even see that in of_irq_parse_pci(), if there's a DT node for >>> the device, of_irq_parse_one() gets the interrupt info from DT and >>> returns the IRQ all the way back up to (I think) loongson_map_irq(). >> >> Agree, I think so for DT. >> >>> 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. >>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/setup-irq.c?id=v5.18#n37 >> >> For ACPI, bridge->map_irq is NULL, so in above path, >> the pci_assign_irq will return because of !(hbrg->map_irq) as following: >> >> if (!(hbrg->map_irq)) { >> pci_dbg(dev, "runtime IRQ mapping not provided by arch\n"); >> return; >> } >> >> And again as I explained in previous version patch, dev->irq is set in >> acpi_pci_irq_enable() in the following path for ACPI: >> >> pci_device_probe >> ->pcibios_alloc_irq >> ->acpi_pci_irq_enable >> ->acpi_pci_irq_lookup >> >> And the reason that we fixed the pin is to get an correct entry in prt >> table when calling acpi_pci_irq_lookup. With out the fix, we can't find >> out a entry. >> >> After found an entry, we get gsi, and map irq as following: >> >> rc = acpi_register_gsi(&dev->dev, gsi, triggering, polarity); >> if (rc < 0) { >> dev_warn(&dev->dev, "PCI INT %c: failed to register GSI\n", >> pin_name(pin)); >> kfree(entry); >> return rc; >> } >> dev->irq = rc; >> >> Here, dev->irq is set like in pci_assign_irq for DT. > > Yes. The above explains how things work for ACPI, but I'm not asking > about that. > > I'm asking how this works in the *DT* case. I see that > pci_assign_irq() is called for both ACPI and DT, and I see that it > does nothing in the ACPI path because bridge->map_irq hasn't been set. > > What I *don't* see is how pci_assign_irq() works in the DT case > because it reads PCI_INTERRUPT_PIN, which should return 0 for these > broken devices, and if "pin == 0", it never calls ->map_irq(). > > Is ->map_irq() called via some other path? > Same as above. > Bjorn >
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
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. > 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 >
On 2022/7/16 下午2:12, Jianmin Lv 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. > >> 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). >> Same thing, they are also not required. >> [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 >> >> > >
Hi, Jianmin, 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). Huacai > > > 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 > > > >
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/
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. The things has been explained above, only multi-func devices are required to fix. Thanks.
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. >
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@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/ > > > > 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.
Hi, Bjorn, On Tue, Jul 19, 2022 at 1:00 AM Bjorn Helgaas <helgaas@kernel.org> 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@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/ > > > > > > 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
On Thu, Jul 21, 2022 at 12:47:18PM +0800, Huacai Chen wrote: > 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 I squashed these in. Let me know if I did anything wrong: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?id=930c6074d7dd
Hi, Bjorn, On Fri, Jul 22, 2022 at 1:50 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Jul 21, 2022 at 12:47:18PM +0800, Huacai Chen wrote: > > > 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 > > I squashed these in. Let me know if I did anything wrong: > > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?id=930c6074d7dd The logic is surely correct. But what is the principle of device list order? If the order is value ascending, then LPC should be after AHCI; if not, I prefer to group them with functions as below. :) --- diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c index 05997b51c86d..a7c3d5db3be8 100644 --- a/drivers/pci/controller/pci-loongson.c +++ b/drivers/pci/controller/pci-loongson.c @@ -22,6 +22,13 @@ #define DEV_LS2K_APB 0x7a02 #define DEV_LS7A_CONF 0x7a10 #define DEV_LS7A_LPC 0x7a0c +#define DEV_LS7A_DC1 0x7a06 +#define DEV_LS7A_DC2 0x7a36 +#define DEV_LS7A_HDMI 0x7a37 +#define DEV_LS7A_AHCI 0x7a08 +#define DEV_LS7A_EHCI 0x7a14 +#define DEV_LS7A_GMAC 0x7a03 +#define DEV_LS7A_GNET 0x7a13 #define FLAG_CFG0 BIT(0) #define FLAG_CFG1 BIT(1) @@ -103,6 +110,25 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, DEV_PCIE_PORT_2, loongson_bmaster_quirk); +static void loongson_pci_pin_quirk(struct pci_dev *pdev) +{ + pdev->pin = 1 + (PCI_FUNC(pdev->devfn) & 3); +} +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_DC1, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_DC2, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_HDMI, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_AHCI, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_EHCI, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_GMAC, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_GNET, loongson_pci_pin_quirk); + static struct loongson_pci *pci_bus_to_loongson_pci(struct pci_bus *bus) { struct pci_config_window *cfg; --- Huacai
diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c index 05997b51c86d..4043b57bcc86 100644 --- a/drivers/pci/controller/pci-loongson.c +++ b/drivers/pci/controller/pci-loongson.c @@ -22,6 +22,13 @@ #define DEV_LS2K_APB 0x7a02 #define DEV_LS7A_CONF 0x7a10 #define DEV_LS7A_LPC 0x7a0c +#define DEV_LS7A_GMAC 0x7a03 +#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 FLAG_CFG0 BIT(0) #define FLAG_CFG1 BIT(1) @@ -103,6 +110,31 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, DEV_PCIE_PORT_2, loongson_bmaster_quirk); +static void loongson_pci_pin_quirk(struct pci_dev *pdev) +{ + pdev->pin = 1 + (PCI_FUNC(pdev->devfn) & 3); +} +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_DC1, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_DC2, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_GPU, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_GMAC, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_AHCI, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_EHCI, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_LS7A_OHCI, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_PCIE_PORT_0, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_PCIE_PORT_1, loongson_pci_pin_quirk); +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, + DEV_PCIE_PORT_2, loongson_pci_pin_quirk); + static struct loongson_pci *pci_bus_to_loongson_pci(struct pci_bus *bus) { struct pci_config_window *cfg;