Comment #135 for bugzilla 237666 : a USB3-handling problem with a investigatory fix for a cortex-a72 context
Mark Millard
marklmi at yahoo.com
Sat Sep 19 19:36:18 UTC 2020
> On 2020-Sep-19, at 12:18, Mark Millard <marklmi at yahoo.com> wrote:
>
> In the below I've cut out text without always indicating
> where so that code looks complete that is complete. I've
> also included some text from another reply of yours that
> fills in some specifics of what you were after.
>
> On 2020-Sep-19, at 04:46, Hans Petter Selasky <hps at selasky.org> wrote:
>>
>>>>> . . .
>>>>>
>>>>> Your finding indicate a problem in usb_pc_cpu_flush() and
>>>>>
>>>>> bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREWRITE);
>>>>>
>>>>> Try to put the dsb only after dmamap_sync.
>>>>>
>>>>> void
>>>>> usb_pc_cpu_flush(struct usb_page_cache *pc)
>>>>> {
>>>>> if (pc->page_offset_end == pc->page_offset_buf) {
>>>>> /* nothing has been loaded into this page cache! */
>>>>> return;
>>>>> }
>>>>> bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREWRITE);
>>>>> }
>>>>> . . .
>>>> After adding a couple of more DSB ST instructions and rebuilding
>>>> (cross build), installing, and booting, I started a self-hosted,
>>>> from-scratch, buildworld buildlkernel and intend to installkernel
>>>> installworld and reboot if it appears to go well. (Better than
>>>> just a boot test for if things seem at least sufficient, even if
>>>> overkill.) But it will be hours before buildworld build kernel is
>>>> done (-j3). (I'll sleep during much of that time.)
>
> The buildworld buildkernel, installkernel installworld, reboot
> sequence seems to have gone fine.
>
>>>> . . .
>>>> phwr = buf_res.buffer;
>>>> addr = buf_res.physaddr;
>>>> addr += (uintptr_t)&((struct xhci_hw_root *)0)->hwr_events[0];
>>>> /* reset hardware root structure */
>>>> memset(phwr, 0, sizeof(*phwr));
>>>> phwr->hwr_ring_seg[0].qwEvrsTablePtr = htole64(addr);
>>>> phwr->hwr_ring_seg[0].dwEvrsTableSize = htole32(XHCI_MAX_EVENTS);
>>>> DPRINTF("ERDP(0)=0x%016llx\n", (unsigned long long)addr);
>>>> #ifdef __aarch64__
>>>> __asm __volatile("dsb st" : : : "memory");
>>>> #endif
>>>> XWRITE4(sc, runt, XHCI_ERDP_LO(0), (uint32_t)addr);
>>>> XWRITE4(sc, runt, XHCI_ERDP_HI(0), (uint32_t)(addr >> 32));
>>>> addr = buf_res.physaddr;
>>>> DPRINTF("ERSTBA(0)=0x%016llx\n", (unsigned long long)addr);
>>>> XWRITE4(sc, runt, XHCI_ERSTBA_LO(0), (uint32_t)addr);
>>>> XWRITE4(sc, runt, XHCI_ERSTBA_HI(0), (uint32_t)(addr >> 32));
>>> . . .
>>> Probably same bug in the USB invalidate function.
>
>>> On 2020-09-19 11:31, Mark Millard wrote:
>>>> #ifdef __aarch64__
>>>> __asm __volatile("dsb ld" : : : "memory");
>>>> #endif
>>>> temp = le32toh(phwr->hwr_events[i].dwTrb3);
>>>>
>>>
>>>
>>> If you look a few lines up you'll find the:
>>>
>>> usb_pc_cpu_invalidate(&sc->sc_hw.root_pc);
>>>
>>> That's where this instruction belongs.
>
>>> Try to patch those generic flush/invalidate functions instead. I've been very careful about those.
>>
>> Sure.
>>
>>>> temp = le32toh(phwr->hwr_events[i].dwTrb3);
>>>> k = (temp & XHCI_TRB_3_CYCLE_BIT) ? 1 : 0;
>>>> if (j != k)
>>>> break;
>>>> always took the break because temp was always zero, k was always zero,
>>>> and j was 1. I expect this is because the wrong qwEvrsTablePtr and
>>>> dwEvrsTableSize content was in use by the xhci (fixed by the DSB ST).
>>>> . . .
>>>
>>> It smells like a generic problem in the busdma synchronize code.
>>>
>>>> (Pe means "executing processor".)
>>>> I expect the removal of that specific DSB ST to result in
>>>> xhci_interrupt_poll's code again taking the break every time,
>>>> just based on event ring behavior alone.
>>>
>>> Again, try to patch both
>>> usb_pc_cpu_flush()
>>> and
>>> usb_pc_cpu_invalidate()
>>
>> Sure.
>>
>
> Unfortunately, it fails the same old way as far as
> what is seen on the console.
>
> So the steps used were:
>
> I reverted /usr/src/sys/dev/usb/controller/xhci.c .
>
> Then changed things so that:
>
> # svnlite diff /usr/src/sys/dev/usb/usb_busdma.c
> Index: /usr/src/sys/dev/usb/usb_busdma.c
> ===================================================================
> --- /usr/src/sys/dev/usb/usb_busdma.c (revision 363590)
> +++ /usr/src/sys/dev/usb/usb_busdma.c (working copy)
> @@ -737,6 +737,9 @@
> */
> bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_POSTREAD);
> bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREREAD);
> +#ifdef __aarch64__
> +__asm __volatile("dsb ld" : : : "memory");
> +#endif
> }
>
> /*------------------------------------------------------------------------*
> @@ -750,6 +753,9 @@
> return;
> }
> bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREWRITE);
> +#ifdef __aarch64__
> +__asm __volatile("dsb st" : : : "memory");
> +#endif
> }
>
> /*------------------------------------------------------------------------*
>
>
> As I expected, the resulting build fails to boot the same old
> way: a sequence messages that repeats indefinitely and
> involves "xhci0: Resetting controller" and no root file system
> mount happens.
>
> So I put back the just last change that had previously lead to
> the successful booting (given the prior DSB ST / DSB LD
> additions). I did so exactly as I originally had it:
>
> # svnlite diff /usr/src/sys/dev/usb/controller/xhci.c
> Index: /usr/src/sys/dev/usb/controller/xhci.c
> ===================================================================
> --- /usr/src/sys/dev/usb/controller/xhci.c (revision 363590)
> +++ /usr/src/sys/dev/usb/controller/xhci.c (working copy)
> @@ -406,6 +406,9 @@
>
> addr = buf_res.physaddr;
>
> +#ifdef __aarch64__
> +__asm __volatile("dsb st" : : : "memory");
> +#endif
> XWRITE4(sc, oper, XHCI_DCBAAP_LO, (uint32_t)addr);
> XWRITE4(sc, oper, XHCI_DCBAAP_HI, (uint32_t)(addr >> 32));
> XWRITE4(sc, oper, XHCI_DCBAAP_LO, (uint32_t)addr);
>
> The result still fails. So more is required.
The controller/xhci.c change above was the wrong one: I grabbed
the wrong text. Retrying with:
# svnlite diff /usr/src/sys/dev/usb/controller/xhci.c
Index: /usr/src/sys/dev/usb/controller/xhci.c
===================================================================
--- /usr/src/sys/dev/usb/controller/xhci.c (revision 363590)
+++ /usr/src/sys/dev/usb/controller/xhci.c (working copy)
@@ -441,6 +441,9 @@
DPRINTF("ERSTBA(0)=0x%016llx\n", (unsigned long long)addr);
+#ifdef __aarch64__
+__asm __volatile("dsb st" : : : "memory");
+#endif
XWRITE4(sc, runt, XHCI_ERSTBA_LO(0), (uint32_t)addr);
XWRITE4(sc, runt, XHCI_ERSTBA_HI(0), (uint32_t)(addr >> 32));
booted just fine. This is what I expected relative to
initializing for the event ring.
===
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)
More information about the freebsd-arm
mailing list