Rockbox

This crashes on gigabeat S for some reason. Taking out some error handling (that I’ve never seen triggered) helps. The problem is that this also takes size of transfered data, which doesn’t matter for storage, but it does for e.g. serial. This means that this patch, while good for testing, isn’t ready for commit.

   chained-tds-2.diff (3.6 KiB)

It seems like each transfer descriptor can handle 20K. Why are they being limited to 16K?

They can handle 20K, if the data is 4K-aligned. If not, you can only get 16K (or actually, you can get whatever fits. Just assuming 16K is safe and simple)

I have tested the patch on H10-20 with 60GB HDD using UDMA1 mode.

USB wite (PC→H10) speed is 9.4MB/s (about 1.5MB/s faster then using OF) without the patch and about 11.2MB/s with the patch, and USB read speed is 8MB/s (OF attains 14-18MB/s) no matter the patch is applied or not.

Tested on my e250v1.
3.1mb/s write, 6.1mb/s read without patch.
3.3mb/s write, 6.1mb/s read with patch.

I tried chained-tds-2.diff on my PC that had disconnects with Rockbox’s USB mode and I saw a pretty big improvement. With this patch I didn’t get a disconnect until over 5GB had been transfered. Without it I typically got one within a couple hundred MB, with 500MB being relatively rare.

Tested chained-tds-2.diff with r20323 and  FS#9708  v0.6 (UDMA1 and UDMA4) on my 5.5G.

Keeping the CPU at 80MHz I get:
PIO4: 8100 KB/s write, 4700 KB/s read
UDMA1: 7100 KB/s write, 9400 KB/s read
UDMA4: 7100 KB/s write, 11300 KB/s read

When setting the CPU to 100MHz I get:
UDMA1: 8100 KB/s write, 9400 KB/s read
UDMA4: 8100 KB/s write, 11300 KB/s read

Edit: Oops, corrected read/write.

This one adds proper length handling, but it gets data aborts on gigabeat S at 0x59a18 (see the attached elf file for context)

Edit: I forgot the buffer size change to usb_storage in this one. It needs that too of course to get proper speed benefits

   chained-tds-3.diff (3.8 KiB)
   rockbox.elf (937.7 KiB)

The data abort seems to happen while accessing td→reserved in:

while(td!=QH_NEXT_TERMINATE)
{
length += 1);

td=td->next_td_ptr;

}

By the way, do ‘transfer_descriptor’ and ‘queue_head’ need to be byte packed? If the related struct members are accessed often, there might be a performance loss.

I don’t really see how this can happen, as td→reserved should be nicely aligned. I’ll check if it never jumps to an invalid location.

transfer_descriptor and queue_head are used directly by the hardware, so they need to be precisely packed and aligned. They only consist of 4-byte int fields though, so I don’t think it should make any difference

It was a bad pointer. Fixed now (and committed)