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

[Robert Crowston]

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?

— RHC.

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"