PERFORCE change 113175 for review
Daan Vreeken [PA4DAN]
Danovitsch at vitsch.net
Sat Jan 20 02:14:24 UTC 2007
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 at 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
More information about the p4-projects
mailing list