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

On 2020-09-19 05:41, Mark Millard via freebsd-arm wrote:
> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=237666 is about USB3
> problems that folks have been having for over a year. Bjoern A. Zeeb
> comment #125 that looked like something I'd seen on a cortex-a72 when
> I tried a kernel that had been built with -mcpu=cortex-a72 : an
> indefinite looping that involved "Resetting controller" for xhci0.
> (This prevented booting from the USB3 device.)
> 
> Well, I got the A72 to boot and comment #135 indicates how but I'll
> paste the information below. All that was involved was adding
> examples of "dsb st" and one "dsb ld".
> 
> 
> Comment 135's content:
> 
> A cortex-a72 success! (In the form of investigatory code, not
> code to check-in as is.)
> 
> Just by adding some "dsb st" commands and a "dsb ld" command in
> places in the xhci code (just for __arach64__), I've gotten the
> cortext-A72 to boot from the USB3 SSD via a -mcpu=cortex-a72
> based kernel build. No more indefinitely repeating "Resetting
> controller" problem.
> 
> I do not claim the additions are the minimal ones. I know one place
> is required for sure: the last one added enabled the boot (given
> the others were already present). Prior to that I still had the
> indefinitely repeating "Resetting controller" problem.
> 
> There could be more places that should have dsb st or dsb ld for
> overall correctness.
> 
> This evidence may suggest somewhat analogous problems for other
> platforms than aarch64 that are seeing problems.
> 
> For the aarch64 context, the evidence is (the changes are)
> as follows. The first "dsb st" textually is the last one I
> added.
> 
> # 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)
> @@ -431,6 +431,9 @@
>   
>   	phwr->hwr_ring_seg[0].qwEvrsTablePtr = htole64(addr);
>   	phwr->hwr_ring_seg[0].dwEvrsTableSize = htole32(XHCI_MAX_EVENTS);
> +#ifdef __aarch64__
> +__asm __volatile("dsb st" : : : "memory");
> +#endif
>   
>   	DPRINTF("ERDP(0)=0x%016llx\n", (unsigned long long)addr);
>   
> @@ -1098,6 +1101,9 @@
>   
>   	while (1) {
>   
> +#ifdef __aarch64__
> +__asm __volatile("dsb ld" : : : "memory");
> +#endif
>   		temp = le32toh(phwr->hwr_events[i].dwTrb3);
>   
>   		k = (temp & XHCI_TRB_3_CYCLE_BIT) ? 1 : 0;
> @@ -1107,6 +1113,9 @@
>   
>   		event = XHCI_TRB_3_TYPE_GET(temp);
>   
> +#ifdef __aarch64__
> +__asm __volatile("dsb ld" : : : "memory");
> +#endif
>   		DPRINTFN(10, "event[%u] = %u (0x%016llx 0x%08lx 0x%08lx)\n",
>   		    i, event, (long long)le64toh(phwr->hwr_events[i].qwTrb0),
>   		    (long)le32toh(phwr->hwr_events[i].dwTrb2),
> @@ -1239,6 +1248,9 @@
>   	sc->sc_command_idx = i;
>   	sc->sc_command_ccs = j;
>   
> +#ifdef __aarch64__
> +__asm __volatile("dsb st" : : : "memory");
> +#endif
>   	XWRITE4(sc, door, XHCI_DOORBELL(0), 0);
>   
>   	err = cv_timedwait(&sc->sc_cmd_cv, &sc->sc_bus.bus_mtx,
> @@ -2855,6 +2867,9 @@
>   	index = xfer->xroot->udev->controller_slot_id;
>   
>   	if (xfer->xroot->udev->flags.self_suspended == 0) {
> +#ifdef __aarch64__
> +__asm __volatile("dsb st" : : : "memory");
> +#endif
>   		XWRITE4(sc, door, XHCI_DOORBELL(index),
>   		    epno | XHCI_DB_SID_SET(xfer->stream_id));
>   	}
> @@ -4180,6 +4195,9 @@
>   
>   	for (n = 1; n != XHCI_MAX_ENDPOINTS; n++) {
>   		for (p = 0; p != XHCI_MAX_STREAMS; p++) {
> +#ifdef __aarch64__
> +__asm __volatile("dsb st" : : : "memory");
> +#endif
>   			XWRITE4(sc, door, XHCI_DOORBELL(index),
>   			    n | XHCI_DB_SID_SET(p));
>   		}
> 
> 
> I'm clearly just "evidence hacking" here. I've no clue what a
> good design for allowing aarch64 build to supply having any of
> those "dsb st"s or the "dsb ld".
> 
> Hopefully someone that knows what they are doing can figure out
> if any of the other examples on other platforms are analogous
> --and ,if they are, provide some hook for platform to contribute
> such code to the xhci code.
> 

Hi Mark,

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);
}

The PCI I/O memory should be coherent and should not need any DSB's.

--HPS

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