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

[Mark Millard]

On 2020-Sep-19, at 14:49, Robert Crowston <crowston at protonmail.com> wrote:
> 
> I’ve been working with aarch64 with mcpu=cortex72 on the rpi4 with u-boot. I did not need this extra barrier or flush. This suggests the problem is slightly more nuanced. (I do not object to the extra barrier if it fixes your problem, I’m just adding extra information.)
> 
> My question, which may be irrelevant or misguided: The flags field in the dma tag has an option for specifying if the hardware is cache coherent (BUS_DMA_COHERENT). Does the UEFI-derived tag passed through to the xhci driver have this bit set?
> 
> I was slightly confused that explicitly generating a barrier after calling bus_dmamap_sync(9) had any effect. I understood that the purpose of the sync function was to generate whatever barrier or fence the underlying bus required, if any, to ensure consistency. But, if those barriers were not required in the end, perhaps that is a red herring?

You are just somewhat behind on the investigation/testing.
All my explicit DSB ST / DSB LD use has been eliminated. (I
warned originally that what started working need not be
minimal. Only one turned out to be contributing to allowing
correct operation in my A72 context.)

The one DSB that needed to be added (for aarch64 contexts)
is now covered by a call to: usb_bus_mem_flush_all so that
the change is machine independent at this level and picks
up machine dependent code as needed:

# 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,17 @@

	phwr->hwr_ring_seg[0].qwEvrsTablePtr = htole64(addr);
	phwr->hwr_ring_seg[0].dwEvrsTableSize = htole32(XHCI_MAX_EVENTS);
+	/*
+	 * For bugzilla 237666:
+	 * According to extensible-host-controler-interface-usb-xhci.pdf ,
+	 * the later XWRITE4's to XHCI_ERSTBA_LO and _HI lead to the xhci
+	 * needing to copy the qwEvrsTablePtr and dwEvrsTableSize
+	 * values above at that time (as the xhci initializes its event
+	 * ring support). This is before the event ring starts to pay
+	 * attention to Run/Stop. Thus, make sure the values are
+	 * observable to the xhci before that point.
+	 */
+	usb_bus_mem_flush_all(&sc->sc_bus, &xhci_iterate_hw_softc);

	DPRINTF("ERDP(0)=0x%016llx\n", (unsigned long long)addr);

(I'm not the one that identified usb_bus_mem_flush_all as the
appropriate FreeBSD way to express the criteria in a machine
independent way. I just did the testing and earlier evidence
gathering in a A72 context that is an example of the more
general bugzilla 237666 issue where non-arm platforms also
show odd behavior. I'm hopeful that the above will fix them
all.)

On Sat, Sep 19, 2020 at 22:18, Mark Millard via freebsd-arm <freebsd-arm at freebsd.org> wrote:
> On 2020-Sep-19, at 13:54, Mark Millard <marklmi at yahoo.com> wrote:
> 
> . . .
> >
> > As stands, in my head -r363590 based context, the patch set for this
> > overall currently looks like (up to E-mail variability in spaces):
> >
> > # svnlite diff /usr/src/sys/dev/usb/usb_busdma.c /usr/src/sys/dev/usb/controller/xhci.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
> > }
> >
> 
> Looking at the lower level code it looked like DSB SY is used for
> the above two automatically and it should be a valid sustitute
> for DSB ST and DSB LD, even if overkill. So I'm testing with
> dev/usb/usb_busdma.c reverted. (See later below.)
> 
> > /*------------------------------------------------------------------------*
> > 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,7 @@
> >
> > phwr->hwr_ring_seg[0].qwEvrsTablePtr = htole64(addr);
> > phwr->hwr_ring_seg[0].dwEvrsTableSize = htole32(XHCI_MAX_EVENTS);
> > + usb_bus_mem_flush_all(&sc->sc_bus, &xhci_iterate_hw_softc);
> >
> > DPRINTF("ERDP(0)=0x%016llx\n", (unsigned long long)addr);
> >
> >
> 
> The test booted just fine, no "Resetting controller" notices or the
> like. So now I have just:
> 
> # 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,7 @@
> 
> phwr->hwr_ring_seg[0].qwEvrsTablePtr = htole64(addr);
> phwr->hwr_ring_seg[0].dwEvrsTableSize = htole32(XHCI_MAX_EVENTS);
> + usb_bus_mem_flush_all(&sc->sc_bus, &xhci_iterate_hw_softc);
> 
> DPRINTF("ERDP(0)=0x%016llx\n", (unsigned long long)addr);
> 
> 
> for the issue. (So my earlier powerpc* SYNC comments are likely junk.)
> 
> ===
> Mark Millard
> marklmi at yahoo.com
> ( dsl-only.net went
> away in early 2018-Mar)
> 
> _______________________________________________
> freebsd-arm at freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-arm
> To unsubscribe, send any mail to "freebsd-arm-unsubscribe at freebsd.org"



===
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)