On 2012-05-07 12:51, Michael Grzeschik wrote: > Hi Arvid, > > On Fri, May 04, 2012 at 09:40:36PM +0000, Arvid Brodin wrote: >> On 2012-05-01 09:13, Michael Grzeschik wrote: > > [...] > >>> 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) > > I cross tested this change again, with my scenario. It appears that with > both Interrupt-Sources (ATL + SOT) enabled, we run very fast into a > stalled situation, where no interrupt will trigger anymore. :( > Since I don't trust this piece of hardware very much I would really > prefer to stick to the precise description of the errata2 issue and its > workaround from the manufacturer. It mentioned to use SOT instead of ATL. I'm not sure I trust the datasheets for this chip any more than I trust the HW or the original drivers (well maybe more than the drivers, because they *really* sucked!). Anyway, the important thing to me is that we get rid of any hanged urbs. Your patch to remove the "ACTIVE_BIT == 0" check seems like a given there. Then we have the performance problem - the software timer checking that slows your high rated transfers down vs. the (perhaps academic) "no ATL" slowdown*. As you said before, using SOF interrupts should actually be nothing else than polling in hardware instead of polling in software. If we poll by SW we get to choose the frequency to poll with, and we get to use ATL interrupts simultaneously. So I suggest trying first with SLOT_CHECK_PERIOD set to 10 (or even lower if necessary). If this works, perhaps it's a better solution than the one suggested by the manufacturer? (I also noticed that you re-set the slot timestamp every time the slot has been checked; perhaps that is relevant to the speedup of your patch as well?) If this fails completely I guess we'll have to live with the huge amount of SOF interrupts and the "no ATL interrupts" slowdown... If you use this solution, then please take a look at these lines in the patch: + /* This is weird: at the first plug-in of a device there seems to be + one packet queued that never gets returned? */ + priv->active_ptds = -1; I believe that packet is the control xfer for the built-in usb hub. So there will always be one outstanding xfer. So active_ptds should probably be initialized to 0 and the check to switch to SOF interrupts should be if active_ptds > 1. * (I timed normal cp's to and between usb sticks with and without ATL ints and the slowdown without ATL ints was around 1 % (to) and 5 % (between) on my board. This is for a single cp running.) > > The tests which are affected by this change seem to be sglist (tests: > 5-8), control endpoint (tests: 9, 10) and ep halt related (13) which > are probably kind of unrelated problems. > >> 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? > > How about using dev_dbg instead then? Sounds fine to me! -- 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
