PERFORCE change 113175 for review

[Hans Petter Selasky]

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 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).

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