Re: Comment #135 for bugzilla 237666 : a USB3-handling problem with a investigatory fix for a cortex-a72 context

> On 2020-Sep-19, at 12:18, Mark Millard <[email protected]> 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)

_______________________________________________
[email protected] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arm
To unsubscribe, send any mail to "[email protected]"