Date: Sat, 20 Jan 2007 01:11:40 +0100 From: "Daan Vreeken [PA4DAN]" <Danovitsch@vitsch.net> To: Hans Petter Selasky <hselasky@freebsd.org> Cc: Perforce@freebsd.org Subject: Re: PERFORCE change 113175 for review Message-ID: <200701200111.40339.Danovitsch@vitsch.net> In-Reply-To: <200701192115.l0JLFwFI088824@repoman.freebsd.org> References: <200701192115.l0JLFwFI088824@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Hans, On Friday 19 January 2007 22:15, Hans Petter Selasky wrote: > http://perforce.freebsd.org/chv.cgi?CH=113175 > > Change 113175 by hselasky@hselasky_mini_itx on 2007/01/19 21:15:07 > > Add missing "bus_dmamap_sync()" calls to the USB host controller > drivers. Optimize the USB code with regard to "bus_dmamap_sync()". > calls. Please correct me if I'm wrong, but I think you've swapped the meaning of the pre & post read & write sync calls in this commit. For example : > +static uint8_t > ehci_dump_sqtd(ehci_qtd_t *sqtd) > { > + uint8_t temp; > + usbd_page_sync(sqtd->page, BUS_DMASYNC_PREREAD); > printf("QTD(%p) at 0x%08x:\n", sqtd, le32toh(sqtd->qtd_self)); > ehci_dump_qtd(sqtd); > - return; > + temp = (sqtd->qtd_next & htole32(EHCI_LINK_TERMINATE)) ? 1 : 0; > + usbd_page_sync(sqtd->page, BUS_DMASYNC_POSTREAD); > + return temp; > } Here you do a sync_preread, then you read the sqtd page and then you do a sync_postread. I think this is wrong. As far as I understand it, bus_dmamap_sync() should be used in either of the following two ways : 1) Processor wants to "read" from a device. (device is going to alter our memory (eg. read from a harddisk) ) : o The page of memory is owned by the CPU. o CPU does sync(BUS_DMASYNC_PREREAD). o Page is handed over to the device (CPU shouldn't touch the page after this moment) o Device writes data into page. o Device signals CPU it's done writing... o Page is handed over back to the CPU. o CPU does sync(BUS_DMASYNC_POSTREAD) o CPU may now use data in the page. 2) Processor wants to "write" to a device. (device is going to use the data in our memory (eg. write it to a harddisk) ) o The page of memory is owned by the CPU. o CPU writes the data it wants to transfer into the page o CPU does sync(BUS_DMASYNC_PREWRITE). o Page is handed over to the device (CPU shouldn't touch the page after this moment) o Device uses data in the page. o Device signals CPU it's done with the data... o Page is handed over back to the CPU. o CPU does sync(BUS_DMASYNC_POSTREAD) See "man bus_dmamap_sync". I think the sync calls should only be placed at places where pages (transfer descriptors or data pages) are handed to or from the USB domain. ehci_dump_sqtd() shouldn't have sync call's in it and it should only ever be used on descriptors that are "owned" by the processor (eg: descriptors that re not on an active queue). ehci_dump_sqtd() can never be used on active descriptors as they might be changing while we read them (between the sync call and the printf). By the way, I think what you're doing is great. Adding these sync call's to your USB stack would make the stack useable on the ARM platform. Before christmas came along I have spent quite some time trying to get USB to function on the ARM platform, but I haven't had time to look at it since. I think it's time for me to boot up my ARM board again... ;-) I am willing to volunteering to help out with getting this code to work there, although I'm limited in available time. Grtz, -- Daan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200701200111.40339.Danovitsch>