On Tue, Apr 17, 2012 at 04:21:48PM +0000, Arvid Brodin wrote: > On 2012-04-17 10:56, Michael Grzeschik wrote: > > Hi Arvid, > > > > On Mon, Apr 16, 2012 at 05:11:25PM +0000, Arvid Brodin wrote: > >> On 2012-04-13 16:39, Michael Grzeschik wrote: > >>> Hi Arvid, > >>> > >>> On Fri, Apr 13, 2012 at 01:58:25PM +0000, Arvid Brodin wrote: > >>>> On 2012-04-13 12:59, Michael Grzeschik wrote: > >>>>> The errata2 workaround is described in the Datasheet as follows: > >>>>> > >>>>> An SOF INT can be used instead of an ATL INT with polling on Done > >>>>> bits. A time-out can be implemented and if a certain Done bit is never > >>>>> set, verification of the PTD completion can be done by reading PTD > >>>>> contents (valid bit). This is a proven workaround implemented in > >>>>> software. > >>>>> > >>>>> Therefore, we should do so, and ignore the check > >>>>> for the ACTIVE_BIT in DW3. Because it is possible > >>>>> that this bit is still set when the issue accures. > >>>> > >>>> I believe you trust the datasheet too much! :) I've tried this before (in fact, most of > >>>> your code seems to be from an earlier patch of mine). It doesn't work, as explained in the > >>>> comment to errata2_function() (and as tested with the usbtest/testusb suite). > >>> > >>> Yes, i know the older patches struggling with that issue. But in both > >>> cases there were some details which does not fit to the exact errata2 > >>> fix description. > >>> > >>> In the first patch (USB: isp1760: Implement solution for erratum 2) > >>> the content checking (Valid Bit: 0) was totaly left out of sight. > >>> > >>> The second patch (usb/isp1760: Use polling instead of SOF interrupts to > >>> fix Errata 2) was checking too much. As described above, the check for > >>> the ACTIVE_BIT must not be done. > >>> > >>> In the current implementation we still run into hanging unhandled urbs > >>> because they sometimes doesn't get both bits toggled. (V==0 && A==0) > >>> > >>> After solving this, it is easy to change the polling function back to > >>> always enabled interrupts. In fact the enabling of SOF Interrupts > >>> doesn't have anything else in mind than to guarantee that we never run > >>> out of interrupts. That actually is nothing else then polling in > >>> hardware. Instead of doing it with a timer function. > >> > >> Aha, I missed the removal of the ACTIVE_BIT==0 check in the patch. Nice find! > >> > >> So your patch is really doing two things: > >> > >> 1) Remove the ACTIVE_BIT==0 check - this fixes the bug you are hitting. Great! > >> 2) Switch from software timer checking at ~5 Hz to SOF interrupt checking at, was it 1 > >> kHz? Is this really neccessary/useful? > > > > Yes, because with some high rated transfers going via the host > > controller, especially in the case of having many slots queued at once, > > the issue can trigger actually a lot. Its better not to wait that long > > time every time that issue happens. This can run that host controller > > actually worthless and not worth calling "high speed" capable. ;-) > > > > Ok, I hear you. > > When you are done with the reworked "check for every urb in queue" patch I'd like to do > some testing of your patches before acking them. :) Did you find time to test this patch? This one has not yet been applied onto usb-next. Thanks, Michael -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
