From owner-p4-projects@FreeBSD.ORG Sat Jan 20 14:03:53 2007 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 3B65D16A406; Sat, 20 Jan 2007 14:03:53 +0000 (UTC) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id E9D8C16A400 for ; Sat, 20 Jan 2007 14:03:52 +0000 (UTC) (envelope-from hselasky@freebsd.org) Received: from swip.net (mailfe04.swip.net [212.247.154.97]) by mx1.freebsd.org (Postfix) with ESMTP id 617DD13C457 for ; Sat, 20 Jan 2007 14:03:52 +0000 (UTC) (envelope-from hselasky@freebsd.org) X-Cloudmark-Score: 0.000000 [] Received: from [193.217.102.48] (account mc467741@c2i.net HELO [10.0.0.249]) by mailfe04.swip.net (CommuniGate Pro SMTP 5.0.12) with ESMTPA id 388047650; Sat, 20 Jan 2007 14:03:47 +0100 From: Hans Petter Selasky To: "Daan Vreeken [PA4DAN]" Date: Sat, 20 Jan 2007 14:03:20 +0100 User-Agent: KMail/1.7 References: <200701192115.l0JLFwFI088824@repoman.freebsd.org> <200701200111.40339.Danovitsch@vitsch.net> In-Reply-To: <200701200111.40339.Danovitsch@vitsch.net> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-6" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200701201403.21642.hselasky@freebsd.org> Cc: perforce@freebsd.org Subject: Re: PERFORCE change 113175 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 20 Jan 2007 14:03:53 -0000 On Saturday 20 January 2007 01:11, Daan Vreeken [PA4DAN] wrote: > 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). Yes, I see that I have mixed around the meaning. I think I looked at a bad example. But can anyone explain if the following will work. When I want to read USB descriptor status, then I need to make sure that all PCI writes are flushed. If I do like this: bus_dmamap_sync(BUS_DMASYNC_POSTREAD|BUS_DMASYNC_POSTWRITE); read status from dma-able memory bus_dmamap_sync(BUS_DMASYNC_PREREAD|BUS_DMASYNC_PREWRITE); Will writes that happen between these sync calls be written afterwards to the DMA-able memory. What happens on the PCI bus if the USB host controller tries to write something when I am reading the status? > > 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. Thanks for pointing this out in detail. --HPS