* [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02
@ 2025-01-21 11:42 Huacai Chen
2025-01-22 15:12 ` Mingcong Bai
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Huacai Chen @ 2025-01-21 11:42 UTC (permalink / raw)
To: Bjorn Helgaas, Lorenzo Pieralisi, Krzysztof Wilczyński,
Rob Herring
Cc: linux-pci, Jianmin Lv, Xuefeng Li, Huacai Chen, Jiaxun Yang,
Huacai Chen
The OHCI controller (rev 0x02) under LS7A PCI host has a hardware flaw.
MMIO register with offset 0x60/0x64 is treated as legacy PS2-compatible
keyboard/mouse interface, which confuse the OHCI controller. Since OHCI
only use a 4KB BAR resource indeed, the LS7A OHCI controller's 32KB BAR
is wrapped around (the second 4KB BAR space is the same as the first 4KB
internally). So we can add an 4KB offset (0x1000) to the BAR resource as
a workaround.
Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
drivers/pci/controller/pci-loongson.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c
index bc630ab8a283..977f7b15f3a7 100644
--- a/drivers/pci/controller/pci-loongson.c
+++ b/drivers/pci/controller/pci-loongson.c
@@ -32,6 +32,7 @@
#define DEV_LS7A_CONF 0x7a10
#define DEV_LS7A_GNET 0x7a13
#define DEV_LS7A_EHCI 0x7a14
+#define DEV_LS7A_OHCI 0x7a24
#define DEV_LS7A_DC2 0x7a36
#define DEV_LS7A_HDMI 0x7a37
@@ -144,6 +145,13 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
DEV_LS7A_PCIE_PORT6, loongson_mrrs_quirk);
+static void loongson_ohci_quirk(struct pci_dev *dev)
+{
+ if (dev->revision == 0x2)
+ dev->resource[0].start += 0x1000;
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, DEV_LS7A_OHCI, loongson_ohci_quirk);
+
static void loongson_pci_pin_quirk(struct pci_dev *pdev)
{
pdev->pin = 1 + (PCI_FUNC(pdev->devfn) & 3);
--
2.47.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 2025-01-21 11:42 [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 Huacai Chen @ 2025-01-22 15:12 ` Mingcong Bai 2025-03-14 7:53 ` Krzysztof Wilczyński 2025-03-14 19:59 ` Bjorn Helgaas 2 siblings, 0 replies; 9+ messages in thread From: Mingcong Bai @ 2025-01-22 15:12 UTC (permalink / raw) To: Huacai Chen, Bjorn Helgaas, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring Cc: linux-pci, Jianmin Lv, Xuefeng Li, Huacai Chen, Jiaxun Yang, Kexy Biscuit Hi Huacai, 在 2025/1/21 19:42, Huacai Chen 写道: > The OHCI controller (rev 0x02) under LS7A PCI host has a hardware flaw. > MMIO register with offset 0x60/0x64 is treated as legacy PS2-compatible > keyboard/mouse interface, which confuse the OHCI controller. Since OHCI > only use a 4KB BAR resource indeed, the LS7A OHCI controller's 32KB BAR > is wrapped around (the second 4KB BAR space is the same as the first 4KB > internally). So we can add an 4KB offset (0x1000) to the BAR resource as > a workaround. > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> > --- > drivers/pci/controller/pci-loongson.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c > index bc630ab8a283..977f7b15f3a7 100644 > --- a/drivers/pci/controller/pci-loongson.c > +++ b/drivers/pci/controller/pci-loongson.c > @@ -32,6 +32,7 @@ > #define DEV_LS7A_CONF 0x7a10 > #define DEV_LS7A_GNET 0x7a13 > #define DEV_LS7A_EHCI 0x7a14 > +#define DEV_LS7A_OHCI 0x7a24 > #define DEV_LS7A_DC2 0x7a36 > #define DEV_LS7A_HDMI 0x7a37 > > @@ -144,6 +145,13 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON, > DEV_LS7A_PCIE_PORT6, loongson_mrrs_quirk); > > +static void loongson_ohci_quirk(struct pci_dev *dev) > +{ > + if (dev->revision == 0x2) > + dev->resource[0].start += 0x1000; > +} > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, DEV_LS7A_OHCI, loongson_ohci_quirk); > + > static void loongson_pci_pin_quirk(struct pci_dev *pdev) > { > pdev->pin = 1 + (PCI_FUNC(pdev->devfn) & 3); I have tested this patch and can confirm that it fixes USB full-speed and low-speed devices (such as keyboards and mice) on ASUS XC-LS3A6M boards. The full-/low-speed devices only work consistently with this specific patch applied, or at least one of the two full-/low-speed devices would not work. P.S., With more than one SuperSpeed devices plugged in, the issue reproduced more reliably, where at least one of the two full-/low-speed devices would not work. With three SuperSpeed devices plugged in, filling all five downstream facing ports at the back of the board along with the two full-/low-speed devices, the issue reproduced consistently. Similar issues have also been reported by mini PC (NUC) users, but I do not own such a device. Tested-by: Mingcong Bai <jeffbai@aosc.io> Best Regards, Mingcong Bai ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 2025-01-21 11:42 [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 Huacai Chen 2025-01-22 15:12 ` Mingcong Bai @ 2025-03-14 7:53 ` Krzysztof Wilczyński 2025-03-15 3:45 ` Huacai Chen 2025-03-14 19:59 ` Bjorn Helgaas 2 siblings, 1 reply; 9+ messages in thread From: Krzysztof Wilczyński @ 2025-03-14 7:53 UTC (permalink / raw) To: Huacai Chen Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, linux-pci, Jianmin Lv, Xuefeng Li, Huacai Chen, Jiaxun Yang Hello, > The OHCI controller (rev 0x02) under LS7A PCI host has a hardware flaw. > MMIO register with offset 0x60/0x64 is treated as legacy PS2-compatible > keyboard/mouse interface, which confuse the OHCI controller. Since OHCI > only use a 4KB BAR resource indeed, the LS7A OHCI controller's 32KB BAR > is wrapped around (the second 4KB BAR space is the same as the first 4KB > internally). So we can add an 4KB offset (0x1000) to the BAR resource as > a workaround. Applied to controller/loongson, thank you! Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 2025-03-14 7:53 ` Krzysztof Wilczyński @ 2025-03-15 3:45 ` Huacai Chen 2025-03-15 4:00 ` Krzysztof Wilczyński 0 siblings, 1 reply; 9+ messages in thread From: Huacai Chen @ 2025-03-15 3:45 UTC (permalink / raw) To: Krzysztof Wilczyński Cc: Huacai Chen, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, linux-pci, Jianmin Lv, Xuefeng Li, Jiaxun Yang Hi, Krzysztof, On Fri, Mar 14, 2025 at 3:53 PM Krzysztof Wilczyński <kw@linux.com> wrote: > > Hello, > > > The OHCI controller (rev 0x02) under LS7A PCI host has a hardware flaw. > > MMIO register with offset 0x60/0x64 is treated as legacy PS2-compatible > > keyboard/mouse interface, which confuse the OHCI controller. Since OHCI > > only use a 4KB BAR resource indeed, the LS7A OHCI controller's 32KB BAR > > is wrapped around (the second 4KB BAR space is the same as the first 4KB > > internally). So we can add an 4KB offset (0x1000) to the BAR resource as > > a workaround. > > Applied to controller/loongson, thank you! I'm sorry but since Bjorn has some questions, and I need some time to investigate, please drop it at present. Thanks. Huacai > > Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 2025-03-15 3:45 ` Huacai Chen @ 2025-03-15 4:00 ` Krzysztof Wilczyński 0 siblings, 0 replies; 9+ messages in thread From: Krzysztof Wilczyński @ 2025-03-15 4:00 UTC (permalink / raw) To: Huacai Chen Cc: Huacai Chen, Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, linux-pci, Jianmin Lv, Xuefeng Li, Jiaxun Yang Hello, > > Applied to controller/loongson, thank you! > > I'm sorry but since Bjorn has some questions, and I need some time to > investigate, please drop it at present. Thanks. No problem! I saw comments from Bjorn, too. Thank you for taking the time to look into the concerns raised to refine this fix. Appreciated. Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 2025-01-21 11:42 [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 Huacai Chen 2025-01-22 15:12 ` Mingcong Bai 2025-03-14 7:53 ` Krzysztof Wilczyński @ 2025-03-14 19:59 ` Bjorn Helgaas 2025-03-18 13:07 ` Huacai Chen 2 siblings, 1 reply; 9+ messages in thread From: Bjorn Helgaas @ 2025-03-14 19:59 UTC (permalink / raw) To: Huacai Chen Cc: Bjorn Helgaas, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, linux-pci, Jianmin Lv, Xuefeng Li, Huacai Chen, Jiaxun Yang On Tue, Jan 21, 2025 at 07:42:25PM +0800, Huacai Chen wrote: > The OHCI controller (rev 0x02) under LS7A PCI host has a hardware flaw. > MMIO register with offset 0x60/0x64 is treated as legacy PS2-compatible > keyboard/mouse interface, which confuse the OHCI controller. Since OHCI > only use a 4KB BAR resource indeed, the LS7A OHCI controller's 32KB BAR > is wrapped around (the second 4KB BAR space is the same as the first 4KB > internally). So we can add an 4KB offset (0x1000) to the BAR resource as > a workaround. It looks like usb_hcd_pci_probe() only uses BAR 0 in the OHCI case, so I assume this OHCI controller has a single 32KB BAR? And you're saying that in that BAR, the 0x1000-0x1fff offsets are aliases of the 0x0000-0x0fff area? And this causes some kind of problem because the OCHI driver looks at offsets 0x60 and 0x64 into the BAR and sees something it doesn't like? And this quirk adds 0x1000 to the BAR start, so the OHCI driver looks at offsets 0x1060 and 0x1064 of the original BAR, and that somehow avoids a problem? Even though those are aliases of 0x0060 and 0x0064 of the original BAR? > +static void loongson_ohci_quirk(struct pci_dev *dev) > +{ > + if (dev->revision == 0x2) > + dev->resource[0].start += 0x1000; What does this do to the iomem_resource tree? IIUC, dev->resource[0] no longer correctly describes the PCI address space consumed by the device. If the BAR is actually programmed with [mem 0x20000000-0x20007fff], the device responds to PCI accesses in that range. Now you update resource[0] so it describes the space [mem 0x20001000-0x20008fff]. So the kernel *thinks* the space at [mem 0x20000000-0x20000fff] is free and available for something else, which is not true, and that the device responds at [mem 0x0x20008000-0x20008fff], which is also not true. I think the resource has already been put into the iomem_resource tree by the time the final fixups are run, so this also may corrupt the sorting of the tree. This just doesn't look safe to me. > +} > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, DEV_LS7A_OHCI, loongson_ohci_quirk); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 2025-03-14 19:59 ` Bjorn Helgaas @ 2025-03-18 13:07 ` Huacai Chen 2025-03-18 21:50 ` Bjorn Helgaas 0 siblings, 1 reply; 9+ messages in thread From: Huacai Chen @ 2025-03-18 13:07 UTC (permalink / raw) To: Bjorn Helgaas Cc: Huacai Chen, Bjorn Helgaas, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, linux-pci, Jianmin Lv, Xuefeng Li, Jiaxun Yang Hi, Bjorn, Sorry for the late reply. On Sat, Mar 15, 2025 at 3:59 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jan 21, 2025 at 07:42:25PM +0800, Huacai Chen wrote: > > The OHCI controller (rev 0x02) under LS7A PCI host has a hardware flaw. > > MMIO register with offset 0x60/0x64 is treated as legacy PS2-compatible > > keyboard/mouse interface, which confuse the OHCI controller. Since OHCI > > only use a 4KB BAR resource indeed, the LS7A OHCI controller's 32KB BAR > > is wrapped around (the second 4KB BAR space is the same as the first 4KB > > internally). So we can add an 4KB offset (0x1000) to the BAR resource as > > a workaround. > > It looks like usb_hcd_pci_probe() only uses BAR 0 in the OHCI case, so > I assume this OHCI controller has a single 32KB BAR? Yes. > > And you're saying that in that BAR, the 0x1000-0x1fff offsets are > aliases of the 0x0000-0x0fff area? Yes. > > And this causes some kind of problem because the OCHI driver looks at > offsets 0x60 and 0x64 into the BAR and sees something it doesn't like? Yes. > > And this quirk adds 0x1000 to the BAR start, so the OHCI driver looks > at offsets 0x1060 and 0x1064 of the original BAR, and that somehow > avoids a problem? Even though those are aliases of 0x0060 and 0x0064 > of the original BAR? Yes. > > > +static void loongson_ohci_quirk(struct pci_dev *dev) > > +{ > > + if (dev->revision == 0x2) > > + dev->resource[0].start += 0x1000; > > What does this do to the iomem_resource tree? IIUC, dev->resource[0] > no longer correctly describes the PCI address space consumed by the > device. In the iomem_resource tree the whole 32KB is requested for the OHCI controller. > > If the BAR is actually programmed with [mem 0x20000000-0x20007fff], > the device responds to PCI accesses in that range. Now you update > resource[0] so it describes the space [mem 0x20001000-0x20008fff]. So > the kernel *thinks* the space at [mem 0x20000000-0x20000fff] is free > and available for something else, which is not true, and that the > device responds at [mem 0x0x20008000-0x20008fff], which is also not > true. > > I think the resource has already been put into the iomem_resource tree > by the time the final fixups are run, so this also may corrupt the > sorting of the tree. > > This just doesn't look safe to me. OHCI only uses 4KB io resources (hardware designer told me that this is from USB specification). So if the BAR is [mem 0x20000000-0x20007fff] and without this quirk, software will only use [mem 0x20000000-0x20000fff]; and if we do this quirk, software will only use [mem 0x20001000-0x20001fff]. Since the whole 32KB belongs to OHCI, there won't be corruption. Huacai > > > +} > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, DEV_LS7A_OHCI, loongson_ohci_quirk); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 2025-03-18 13:07 ` Huacai Chen @ 2025-03-18 21:50 ` Bjorn Helgaas 2025-03-26 9:15 ` Huacai Chen 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Helgaas @ 2025-03-18 21:50 UTC (permalink / raw) To: Huacai Chen Cc: Huacai Chen, Bjorn Helgaas, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, linux-pci, Jianmin Lv, Xuefeng Li, Jiaxun Yang, Greg Kroah-Hartman [+cc Greg, USB maintainer; I'm proposing ohci_pci_quirks for this] On Tue, Mar 18, 2025 at 09:07:10PM +0800, Huacai Chen wrote: > On Sat, Mar 15, 2025 at 3:59 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jan 21, 2025 at 07:42:25PM +0800, Huacai Chen wrote: > > > The OHCI controller (rev 0x02) under LS7A PCI host has a hardware flaw. > > > MMIO register with offset 0x60/0x64 is treated as legacy PS2-compatible > > > keyboard/mouse interface, which confuse the OHCI controller. Since OHCI > > > only use a 4KB BAR resource indeed, the LS7A OHCI controller's 32KB BAR > > > is wrapped around (the second 4KB BAR space is the same as the first 4KB > > > internally). So we can add an 4KB offset (0x1000) to the BAR resource as > > > a workaround. > > > > It looks like usb_hcd_pci_probe() only uses BAR 0 in the OHCI case, so > > I assume this OHCI controller has a single 32KB BAR? > > Yes. > > > And you're saying that in that BAR, the 0x1000-0x1fff offsets are > > aliases of the 0x0000-0x0fff area? > > Yes. > > > And this causes some kind of problem because the OCHI driver looks at > > offsets 0x60 and 0x64 into the BAR and sees something it doesn't like? > > Yes. > > > And this quirk adds 0x1000 to the BAR start, so the OHCI driver looks > > at offsets 0x1060 and 0x1064 of the original BAR, and that somehow > > avoids a problem? Even though those are aliases of 0x0060 and 0x0064 > > of the original BAR? > > Yes. > > > > +static void loongson_ohci_quirk(struct pci_dev *dev) > > > +{ > > > + if (dev->revision == 0x2) > > > + dev->resource[0].start += 0x1000; > > > > What does this do to the iomem_resource tree? IIUC, dev->resource[0] > > no longer correctly describes the PCI address space consumed by the > > device. > > In the iomem_resource tree the whole 32KB is requested for the OHCI > controller. I'm skeptical. The quirk only updates the start, not the end, so I think the resource is only 28KB after the quirk. This is a final quirk, so I think the quirk runs after the BAR resource has already been put in the iomem_resource tree. We shouldn't update the resource start/end after that point. > > If the BAR is actually programmed with [mem 0x20000000-0x20007fff], > > the device responds to PCI accesses in that range. Now you update > > resource[0] so it describes the space [mem 0x20001000-0x20008fff]. So > > the kernel *thinks* the space at [mem 0x20000000-0x20000fff] is free > > and available for something else, which is not true, and that the > > device responds at [mem 0x0x20008000-0x20008fff], which is also not > > true. > > > > I think the resource has already been put into the iomem_resource tree > > by the time the final fixups are run, so this also may corrupt the > > sorting of the tree. > > > > This just doesn't look safe to me. > > OHCI only uses 4KB io resources (hardware designer told me that this > is from USB specification). So if the BAR is [mem > 0x20000000-0x20007fff] and without this quirk, software will only > use [mem 0x20000000-0x20000fff]; and if we do this quirk, software > will only use [mem 0x20001000-0x20001fff]. Since the whole 32KB > belongs to OHCI, there won't be corruption. Here's the path where I think the BAR is requested. Assume the BAR contains 0x20000000 when we boot: acpi_pci_root_add pci_acpi_scan_root # loongson acpi_pci_root_create pci_scan_child_bus pci_scan_single_device # details trimmed pci_scan_device pci_read_bases resource[0].start = 0x20000000 resource[0].end = 0x20007fff # size 32KB pci_device_add pci_fixup_device(pci_fixup_header) # <-- pci_bus_assign_resources # details trimmed pci_assign_resource pci_bus_alloc_resource allocate_resource __request_resource # inserted in iomem_resource pci_bus_add_devices pci_bus_add_device pci_fixup_device(pci_fixup_final) resource[0].start += 0x1000 # now 0x20001000 ohci_pci_probe usb_hcd_pci_probe(& ohci_pci_hc_driver) if (driver->flags & HCD_MEMORY) hcd->rsrc_start = pci_resource_start(dev, 0) # 0x20001000 hcd->rsrc_len = pci_resource_len(dev, 0) # now 28KB devm_request_mem_region hcd->regs = devm_ioremap usb_add_hcd hcd->driver->reset ohci_pci_reset # <-- So I suspect: - BAR contains 0x20000000 initially (for example) - pci_bus_assign_resources() inserts 0x20000000-0x20007fff in iomem_resource - final quirk updates res->start, so resource is now 0x20001000-0x20007fff - OHCI driver binds and sees 0x20001000-0x20007fff - OHCI driver requests 0x20001000-0x20007fff (28KB) - OHCI may only use 4KB, but AFAICS it actually reserves the entire resource[0], which is now only 28KB - BAR still contains 0x20000000 ("lspci -b" should confirm this) - /proc/iomem contains something like this: 20001000-20007fff 0000:3e:00.0 20001000-20007fff ohci-hcd Now the device still responds to accesses at 0x20000000 because that's what's in the BAR, but the kernel doesn't know that anybody is using the 0x20000000-0x20000fff region. I think the best solution would be to add an entry in ohci_pci_quirks[] that bumps hcd->regs up by 0x1000. Then we wouldn't have to touch the struct resource, it would stay 32KB, and it would accurately reflect the hardware. If that doesn't work, I guess you could also make this a *header* fixup instead of a final one. Then we would at least update the resource before inserting it in iomem_resource. But I don't like it because the device still responds to an area not mentioned in iomem_resource, and the resource is now 28KB, which is an illegal size for a PCI BAR. > > > +} > > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, DEV_LS7A_OHCI, loongson_ohci_quirk); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 2025-03-18 21:50 ` Bjorn Helgaas @ 2025-03-26 9:15 ` Huacai Chen 0 siblings, 0 replies; 9+ messages in thread From: Huacai Chen @ 2025-03-26 9:15 UTC (permalink / raw) To: Bjorn Helgaas Cc: Huacai Chen, Bjorn Helgaas, Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring, linux-pci, Jianmin Lv, Xuefeng Li, Jiaxun Yang, Greg Kroah-Hartman On Wed, Mar 19, 2025 at 5:50 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Greg, USB maintainer; I'm proposing ohci_pci_quirks for this] > > On Tue, Mar 18, 2025 at 09:07:10PM +0800, Huacai Chen wrote: > > On Sat, Mar 15, 2025 at 3:59 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Tue, Jan 21, 2025 at 07:42:25PM +0800, Huacai Chen wrote: > > > > The OHCI controller (rev 0x02) under LS7A PCI host has a hardware flaw. > > > > MMIO register with offset 0x60/0x64 is treated as legacy PS2-compatible > > > > keyboard/mouse interface, which confuse the OHCI controller. Since OHCI > > > > only use a 4KB BAR resource indeed, the LS7A OHCI controller's 32KB BAR > > > > is wrapped around (the second 4KB BAR space is the same as the first 4KB > > > > internally). So we can add an 4KB offset (0x1000) to the BAR resource as > > > > a workaround. > > > > > > It looks like usb_hcd_pci_probe() only uses BAR 0 in the OHCI case, so > > > I assume this OHCI controller has a single 32KB BAR? > > > > Yes. > > > > > And you're saying that in that BAR, the 0x1000-0x1fff offsets are > > > aliases of the 0x0000-0x0fff area? > > > > Yes. > > > > > And this causes some kind of problem because the OCHI driver looks at > > > offsets 0x60 and 0x64 into the BAR and sees something it doesn't like? > > > > Yes. > > > > > And this quirk adds 0x1000 to the BAR start, so the OHCI driver looks > > > at offsets 0x1060 and 0x1064 of the original BAR, and that somehow > > > avoids a problem? Even though those are aliases of 0x0060 and 0x0064 > > > of the original BAR? > > > > Yes. > > > > > > +static void loongson_ohci_quirk(struct pci_dev *dev) > > > > +{ > > > > + if (dev->revision == 0x2) > > > > + dev->resource[0].start += 0x1000; > > > > > > What does this do to the iomem_resource tree? IIUC, dev->resource[0] > > > no longer correctly describes the PCI address space consumed by the > > > device. > > > > In the iomem_resource tree the whole 32KB is requested for the OHCI > > controller. > > I'm skeptical. The quirk only updates the start, not the end, so I > think the resource is only 28KB after the quirk. > > This is a final quirk, so I think the quirk runs after the BAR > resource has already been put in the iomem_resource tree. We > shouldn't update the resource start/end after that point. > > > > If the BAR is actually programmed with [mem 0x20000000-0x20007fff], > > > the device responds to PCI accesses in that range. Now you update > > > resource[0] so it describes the space [mem 0x20001000-0x20008fff]. So > > > the kernel *thinks* the space at [mem 0x20000000-0x20000fff] is free > > > and available for something else, which is not true, and that the > > > device responds at [mem 0x0x20008000-0x20008fff], which is also not > > > true. > > > > > > I think the resource has already been put into the iomem_resource tree > > > by the time the final fixups are run, so this also may corrupt the > > > sorting of the tree. > > > > > > This just doesn't look safe to me. > > > > OHCI only uses 4KB io resources (hardware designer told me that this > > is from USB specification). So if the BAR is [mem > > 0x20000000-0x20007fff] and without this quirk, software will only > > use [mem 0x20000000-0x20000fff]; and if we do this quirk, software > > will only use [mem 0x20001000-0x20001fff]. Since the whole 32KB > > belongs to OHCI, there won't be corruption. > > Here's the path where I think the BAR is requested. Assume the BAR > contains 0x20000000 when we boot: > > acpi_pci_root_add > pci_acpi_scan_root # loongson > acpi_pci_root_create > pci_scan_child_bus > pci_scan_single_device # details trimmed > pci_scan_device > pci_read_bases > resource[0].start = 0x20000000 > resource[0].end = 0x20007fff # size 32KB > pci_device_add > pci_fixup_device(pci_fixup_header) # <-- > pci_bus_assign_resources # details trimmed > pci_assign_resource > pci_bus_alloc_resource > allocate_resource > __request_resource # inserted in iomem_resource > pci_bus_add_devices > pci_bus_add_device > pci_fixup_device(pci_fixup_final) > resource[0].start += 0x1000 # now 0x20001000 > > ohci_pci_probe > usb_hcd_pci_probe(& ohci_pci_hc_driver) > if (driver->flags & HCD_MEMORY) > hcd->rsrc_start = pci_resource_start(dev, 0) # 0x20001000 > hcd->rsrc_len = pci_resource_len(dev, 0) # now 28KB > devm_request_mem_region > hcd->regs = devm_ioremap > usb_add_hcd > hcd->driver->reset > ohci_pci_reset # <-- > > So I suspect: > > - BAR contains 0x20000000 initially (for example) > > - pci_bus_assign_resources() inserts 0x20000000-0x20007fff in > iomem_resource > > - final quirk updates res->start, so resource is now > 0x20001000-0x20007fff > > - OHCI driver binds and sees 0x20001000-0x20007fff > > - OHCI driver requests 0x20001000-0x20007fff (28KB) > > - OHCI may only use 4KB, but AFAICS it actually reserves the entire > resource[0], which is now only 28KB > > - BAR still contains 0x20000000 ("lspci -b" should confirm this) > > - /proc/iomem contains something like this: > > 20001000-20007fff 0000:3e:00.0 > 20001000-20007fff ohci-hcd > > Now the device still responds to accesses at 0x20000000 because that's > what's in the BAR, but the kernel doesn't know that anybody is using > the 0x20000000-0x20000fff region. > > I think the best solution would be to add an entry in > ohci_pci_quirks[] that bumps hcd->regs up by 0x1000. Then we wouldn't > have to touch the struct resource, it would stay 32KB, and it would > accurately reflect the hardware. OK, let me try this way, thanks. Huacai > > If that doesn't work, I guess you could also make this a *header* > fixup instead of a final one. Then we would at least update the > resource before inserting it in iomem_resource. But I don't like it > because the device still responds to an area not mentioned in > iomem_resource, and the resource is now 28KB, which is an illegal size > for a PCI BAR. > > > > > +} > > > > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, DEV_LS7A_OHCI, loongson_ohci_quirk); ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-03-26 9:15 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-21 11:42 [PATCH] PCI: loongson: Add quirk for OHCI device rev 0x02 Huacai Chen 2025-01-22 15:12 ` Mingcong Bai 2025-03-14 7:53 ` Krzysztof Wilczyński 2025-03-15 3:45 ` Huacai Chen 2025-03-15 4:00 ` Krzysztof Wilczyński 2025-03-14 19:59 ` Bjorn Helgaas 2025-03-18 13:07 ` Huacai Chen 2025-03-18 21:50 ` Bjorn Helgaas 2025-03-26 9:15 ` Huacai Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox