In message: <200906231912.20741.hsela...@c2i.net>
Hans Petter Selasky <hsela...@c2i.net> writes:
: On Tuesday 23 June 2009 11:11:29 Alexandr Rybalko wrote:
: > On Tue, 23 Jun 2009 10:35:42 +0200
: >
: > Piotr Zięcik <ko...@semihalf.com> wrote:
: > >> While bringing up EHCI (8-CURRENT, new USB stack) on ARM machine we
: > >> have found cache-related problem in the USB stack.
: > >>
: > >> The usb_pc_cpu_flush() and usb_pc_cpu_invalidate() functions are used to
: > >> flush/invalidate CPU caches in various places in USB code. Internally,
: > >> the functions are implemented using bus_dmamap_sync(). In our case, on
: > >> ARM machine, flags passed to the bus_dmamap_sync() function did not
: > >> correspond with requested operation. We have fixed the problem by
: > >> changing flags passed to the bus_dmamap_sync() function (see attached
: > >> patch).
: > >>
: > >> My question is about general idea of bus_dma usage for cache operations.
: > >> In my opinion we should not rely on bus_dmamap_sync() behaviour as this
: > >> function may do different things on different architectures. This not
: > >> always works as expected, which is clearly visible in our case. Better
: > >> solution is to use cpu-specific functions implementing cache operations.
: > >> Please comment on why CPU-specific functions are not used...
I think because busdma is supposed to abstract this out. The problem
is that the usb code chose different terms to represent these
operations than is typically used.
: > >> Patch fixing our problem:
: > >> diff --git a/sys/dev/usb/usb_busdma.c b/sys/dev/usb/usb_busdma.c
: > >> index 3d6a5be..69a6fff 100644
: > >> --- a/sys/dev/usb/usb_busdma.c
: > >> +++ b/sys/dev/usb/usb_busdma.c
: > >> @@ -658,8 +658,7 @@ usb_pc_cpu_invalidate(struct usb_page_cache *pc)
: > >> /* nothing has been loaded into this page cache! */
: > >> return;
: > >> }
: > >> - bus_dmamap_sync(pc->tag, pc->map,
: > >> - BUS_DMASYNC_POSTWRITE | BUS_DMASYNC_POSTREAD);
: > >> + bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREREAD);
: > >> }
I think this patch is currect. Invalidate should be done to a region
before you read into it.
: > >> /*----------------------------------------------------------------------
: > >>--* @@ -672,8 +671,7 @@ usb_pc_cpu_flush(struct usb_page_cache *pc) /*
: > >> nothing has been loaded into this page cache! */ return;
: > >> }
: > >> - bus_dmamap_sync(pc->tag, pc->map,
: > >> - BUS_DMASYNC_PREWRITE | BUS_DMASYNC_PREREAD);
: > >> + bus_dmamap_sync(pc->tag, pc->map, BUS_DMASYNC_PREWRITE);
: > >> }
This makes sense as well. Flushing the cache to memory is the right
logical operation before writing to the device with a DMA transfer.
: > >> /*----------------------------------------------------------------------
: > >>--*
: > >>
:
: >
: > Great thanks Piotr!
: > I work on MIPS BCM5354 and BCM5836 and after apply your patch USB work
: > correct.
:
: We are currently investigating if your patch is correct. Thanks for your
patch
: suggestion!
From the comments in the code, they look correct. I don't know if all
the usages of these functions is reflected in their comments. I've
not had a chance to audit them all...
Warner
_______________________________________________
freebsd-usb@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to "freebsd-usb-unsubscr...@freebsd.org"
