On 2012-05-01 09:13, Michael Grzeschik wrote: > 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. I didn't have time for this yesterday but today I've done some testing. I didn't find any major issues, but two minor things popped up: 1) A performance issue (easily fixed, I believe). Below are two runs of the testusb suite (the test 14 error is normal, apparently; the default params don't work with this test): a) before the "rework" patch: # ./testusb -a unknown speed /proc/bus/usb/001/003 /proc/bus/usb/001/003 test 0, 0.003234 secs /proc/bus/usb/001/003 test 1, 1.135052 secs /proc/bus/usb/001/003 test 2, 1.314615 secs /proc/bus/usb/001/003 test 3, 1.145597 secs /proc/bus/usb/001/003 test 4, 1.337246 secs /proc/bus/usb/001/003 test 5, 21.063552 secs /proc/bus/usb/001/003 test 6, 22.070504 secs /proc/bus/usb/001/003 test 7, 21.089739 secs /proc/bus/usb/001/003 test 8, 22.077262 secs /proc/bus/usb/001/003 test 9, 5.025091 secs /proc/bus/usb/001/003 test 10, 12.446566 secs /proc/bus/usb/001/003 test 11, 16.002608 secs /proc/bus/usb/001/003 test 12, 16.021846 secs /proc/bus/usb/001/003 test 13, 10.608179 secs /proc/bus/usb/001/003 test 14 --> 22 (error 22) b) after the "rework" patch: # ./testusb -a unknown speed /proc/bus/usb/001/003 /proc/bus/usb/001/003 test 0, 0.003304 secs /proc/bus/usb/001/003 test 1, 1.167813 secs /proc/bus/usb/001/003 test 2, 1.345984 secs /proc/bus/usb/001/003 test 3, 1.171061 secs /proc/bus/usb/001/003 test 4, 1.348574 secs /proc/bus/usb/001/003 test 5, 33.349864 secs /proc/bus/usb/001/003 test 6, 33.353429 secs /proc/bus/usb/001/003 test 7, 33.338316 secs /proc/bus/usb/001/003 test 8, 33.348851 secs /proc/bus/usb/001/003 test 9, 15.947056 secs /proc/bus/usb/001/003 test 10, 88.002466 secs /proc/bus/usb/001/003 test 11, 16.008327 secs /proc/bus/usb/001/003 test 12, 16.004116 secs /proc/bus/usb/001/003 test 13, 26.069834 secs /proc/bus/usb/001/003 test 14 --> 22 (error 22) I've seen this before, and the reason is that the patch disables ATL and INT interrupts when enabling SOF interrupts. This change in isp1760-hcd.h fixes it (and I've tested it quite extensively too): -#define INTERRUPT_ENABLE_MASK (HC_INTL_INT | HC_ATL_INT) -#define INTERRUPT_ENABLE_SOT_MASK (HC_SOT_INT) +#define INTERRUPT_ENABLE_MASK (HC_INTL_INT | HC_ATL_INT) +#define INTERRUPT_ENABLE_SOT_MASK (HC_INTL_INT | HC_ATL_INT | HC_SOT_INT) 2) I did manage to trigger "isp1760 isp1760: errata2 issue!" a couple of times during testing. I'm not sure if this is a problem (I didn't notice anything out of the ordinary afterward). If not, perhaps this printout should be removed? With the above change to isp1760-hcd.h, feel free to add Tested-by: Arvid Brodin <arvid.brodin@xxxxxxxx> to the patch. -- Arvid Brodin Enea Services Stockholm AB - since February 16 a part of Xdin in the Alten Group. Soon we will be working under the common brand name Xdin. Read more at www.xdin.com.-- 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
