Linux PCI subsystem development
 help / color / mirror / Atom feed
* [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-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  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-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