Dear Stefan Herbrechtsmeier, > Am 07.07.2012 23:58, schrieb Marek Vasut: > >> Am 04.07.2012 08:57, schrieb Schneider, Kolja: > >>>> Am 03.07.2012 20:10, schrieb Marek Vasut: > >>>>>> The commit 5dd95cf93dfffa1d19a1928990852aac9f55b9d9 'usb_storage: > >>>>>> Fix EHCI "out of buffer pointers" with CD-ROM' introduce a bug in > >>>>>> usb_storage as it wrongly assumes that every transfer can use 4k > >>>>>> per qt_buffer. This is wrong if the start address of the data > >>>>>> is not 4k aligned and leads to 'EHCI timed out on TD' messages > >>>>>> because of 'out of buffer pointers' in ehci_td_buffer function. > >>>>>> > >>>>>> Cc: Marek Vasut <ma...@denx.de> > >>>>>> Signed-off-by: Stefan Herbrechtsmeier <ste...@herbrechtsmeier.net> > >>>>> > >>>>> Ok, first I have to admit I broke my promise to look into this ASAP, > >>>>> sorry > >>>> > >>>> about > >>>> > >>>>> it :-( > >>>> > >>>> No problem, as long as we get it into the next release. ;-) > >>>> > >>>>> Just curious, but shouldn't it be ((4096 * 5) / dev_desc->blk_sz) - 1 > >>>>> ? > >>>> > >>>> No, because the first blk need to be aligned with the 4096. In worst > >>>> case the blk is at the end of the 4096 range. If we assume that the > >>>> blk is aligned to blk_sz we can change it to ((4096 * 4) / > >>>> dev_desc->blk_sz) + 1. I skip the last blk (+ 1) because with 4096 > >>>> aligned first blk we unaligned the next transfer and add extra short > >>>> packages to each ehci transfer. > >>>> > >>>> If we want to maximise the usage we need to calculate the max_xfer_blk > >>>> depending on the start address of the first blk. > >>> > >>> I admit to not totally getting it. However, there are two things that > >>> come > > > > to my mind: > >>> - Doesn't the EHCI Specification mention exactly five buffers that > >>> can/should/must > >>> > >>> be used? > >> > >> Yes, you can use up to five 4096 byte buffers. > >> > >>> - I think I once stumbled across some comment that said as much as > > > > the > > > >>> blocks > >>> > >>> always having to be aligned anyway? > >> > >> The buffers must be aligned to a 4096 byte page. This means that you > >> have to use the first and last buffer to align your data to the next or > >> previous 4096 byte page boundary. > > > > All right, I managed to replicate the issue. This (or similar) doesn't > > work for you, right: > > > > usb read 0x42000004 0x0 0x400 > > I observe the bug during > fatload usb 0 0x800000 uImage > > ... > dev=0ffb0440, pipe=c8008283, buffer=00a90000, length=20480, req=(null) > ... > dev=0ffb0440, pipe=c8008283, buffer=00a95000, length=20480, req=(null) > ... > dev=0ffb0440, pipe=c8008283, buffer=00a9a000, length=18432, req=(null) > ... > dev=0ffb0440, pipe=c8008283, buffer=00a9e800, length=20480, req=(null) > out of buffer pointers (2048 bytes left) > ... > > It looks like the file is fragmented. During load the start address > becomes unaligned and thereby the code wrongly tries to transfer more > blocks than possible. > > Before the above mentioned patch the max_xfer_blk was much smaller (20), > so that the problem never appears. > > > The proper solution would be to introduce a bounce buffer for such > > unaligned transfers. A proper, generic bounce buffer that can be > > configured to bounce on specified boundaries and warns about performance > > penalties. > > Why not limit the max_xfer_blk to the maximum save count [(4096 * 4) / > dev_desc->blksz)] or calculate the max_xfer_blk depending on the start > address of the transfer: > > max_xfer_blk = ((4096 * 5) - (start & ~4096) / dev_desc->blksz
This looks like a much better solution ;-) Can you redo the patch for that? Also, there's some macro for that rounding in include/common.h > > Regards, > Stefan Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
