Possible error in Starterware USB double-buffering FIFO routines

Walter Snafu

Other Parts Discussed in Thread: OMAP-L138

I noticed a possible error in the Starterware USB double-buffering FIFO routines. At the very least it is a point of confusion, and possibly a software bug.

The USB FIFO size is set in the TXFIFOSZ and RXFIFOSZ registers, where bit 4 (or "DPB") is set for double buffering, and where the lower 4 bits is m (or "SZ", which is short for size). (For example, m = 0x0F & TXFIFOSZ)

According to the manual:

"the FIFO size is calculated as 2^(m+3) for single packet buffering, and 2^(m+4) for dual packet buffering."

In other words, the same number, m, is interpreted differently depending on whether bit 4 is set. Setting bit 4, by itself, doubles the total FIFO memory size. I think the manual is likely correct on this point. (Reference: OMAP-L138 DSP+ARM Technical Reference Manual, file SPRUH77A.pdf, sections 35.4.55 and 35.4.56)

However, that appears to be in discrepancy with the Starterware USB software.

For example, take the following two items (from usb.h)

#define USB_FIFO_SZ_512             0x00000006      // 512 byte FIFO
...
#define USB_FIFO_SZ_512_DB      0x00000017      // 512 byte double buffered FIFO
                                                                                    // (occupying 1024 bytes)

Those items are used in the function USBFIFOConfigSet() to configure the FIFO sizes.

Compare those two items. Notice that the double-buffering item adds 1, (from 6 to 7), thereby doubling the FIFO memory size, and it also sets bit 4, (resulting in 0x17), which as I showed above also doubles the FIFO memory size. It results in four times the memory usage, not just double. This will throw off the memory assignment, likely resulting in overlapping buffers. (I suspect this may explain why so many people are expressing trouble with double-buffering.)

Unfortunately, the double-buffering values (e.g., USB_FIFO_SZ_512_DB) are used in more than one place, sometimes correctly, and other times not. So the problem cannot be cured by simply redefining USB_FIFO_SZ_512_DB.

The error occurs when you use those values in the library routine for configuring the FIFOs.

USBFIFOConfigSet(USB0_BASE, USB_EP_1, 64, USB_FIFO_SZ_512_DB, USB_EP_DEV_IN)

That will result in a FIFO size of 2048 bytes, not 1024.

The fix can occur like this:

#define DOUBLE_BUF_FIFO 0x10

USBFIFOConfigSet(USB0_BASE, USB_EP_1, 64, (USB_FIFO_SZ_512 + DOUBLE_BUF_FIFO), USB_EP_DEV_IN)

-- Walter Snafu

over 10 years ago

Norman Wong over 10 years ago

Guru 26430 points

I has similar concerns a while back.
e2e.ti.com/.../214237
I think I figured out it later in response here:
e2e.ti.com/.../336594

The double buffer sizes are all shifted to allow define double buffer flag in the middle of the values. The macro USB_FIFO_SZ_TO_BYTES extracts the flag and calculates the correct size. I think that we are not suppose to use the USB_FIFO_SZ* constants as true bytes lengths. It is an excessively clever enumerated type.

#define USB_FIFO_SIZE_DB_FLAG 0x00000010
#define USB_FIFO_SZ_TO_BYTES(x) ((8 << ((x) & ~ USB_FIFO_SIZE_DB_FLAG)) * \
(((x) & USB_FIFO_SIZE_DB_FLAG) ? 2 : 1))

Maybe. StarterWare is not so good with the USB examples. The CPPI DMA uses a huge amount of memory. Most of which is not required for the example.

Walter Snafu over 10 years ago in reply to Norman Wong

Genius 3450 points

Norman Wong,

Thank you for your post. It is good to see confirmed that you too saw the error in the FIFO double-buffering setup. Thank you for your help in that.

While the solution I offered will work, I believe yours is more elegant, in that it preserves the TI intended software design, in the simplest, most direct way -- by correcting the defined values for the double-buffering parameters.

To make the solution more readily digestible by our community, I here recite the preferred solution in its entirety.

First, in the file usb.h, subtract 1 from each "_DB" value. They should now correctly read:

#define USB_FIFO_SZ_8_DB        0x00000010 // 8 byte double buffered FIFO
                                            // (occupying 16 bytes)
#define USB_FIFO_SZ_16_DB       0x00000011 // 16 byte double buffered FIFO
                                            // (occupying 32 bytes)
#define USB_FIFO_SZ_32_DB       0x00000012 // 32 byte double buffered FIFO
                                            // (occupying 64 bytes)
#define USB_FIFO_SZ_64_DB       0x00000013 // 64 byte double buffered FIFO
                                            // (occupying 128 bytes)
#define USB_FIFO_SZ_128_DB      0x00000014 // 128 byte double buffered FIFO
                                            // (occupying 256 bytes)
#define USB_FIFO_SZ_256_DB      0x00000015 // 256 byte double buffered FIFO
                                            // (occupying 512 bytes)
#define USB_FIFO_SZ_512_DB      0x00000016 // 512 byte double buffered FIFO
                                            // (occupying 1024 bytes)
#define USB_FIFO_SZ_1024_DB     0x00000017 // 1024 byte double buffered FIFO
                                            // (occupying 2048 bytes)

Second, eliminate the following four items from the list, as they require more FIFO memory than is available. (The OMAP-L138 USB FIFO has a total of 4096 bytes, of which 64 bytes are reserved for endpoint 0. Therefore, anything larger than 4032 bytes will not fit.)

#define USB_FIFO_SZ_4096
#define USB_FIFO_SZ_8192
#define USB_FIFO_SZ_2048_DB
#define USB_FIFO_SZ_4096_DB

Norman Wong deserves all the credit for discovering and solving this bug several years ago.

UPDATE EDIT:

In my earlier post, I included code for configuring the FIFO addresses and sizes. Here deleted, as that is not needed, because this is already done automatically for you in the function USBDeviceConfig(), in the file usbdconfig.c. You need only populate the following array with your values (in the file usbdenum.c). (You can even include flags for setting the DMA mode.) Something like this:

const tFIFOConfig g_sUSBDefaultFIFOConfig =
{

{

{ 1, true, USB_EP_AUTO_REQUEST | USB_EP_AUTO_CLEAR },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 }

}
{

{ 1, true, USB_EP_AUTO_SET },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 },
{ 1, false, 0 }

}

I hope an enterprising TI employee will paste something like this into the official Starterware help pages, to make it easier to find. The website search engine, though helpful, often leads people through mounds of needless diversions. If we cannot find the solutions, then they effectively don't exist, and must be re-invented each time. That is what happened here.

-- Walter Snafu

Norman Wong over 10 years ago in reply to Walter Snafu

Guru 26430 points

Thanks for the detailed notes.

Here's another way to define the sizes that avoids the magic numbers.

#define USB_FIFO_SZ_8192    0x0000000A // Biju - Was 0x10. Incorrect.

#define USB_FIFO_SZ_8_DB    (USB_FIFO_SIZE_DB_FLAG|USB_FIFO_SZ_8)
#define USB_FIFO_SZ_16_DB   (USB_FIFO_SIZE_DB_FLAG|USB_FIFO_SZ_16)
#define USB_FIFO_SZ_32_DB   (USB_FIFO_SIZE_DB_FLAG|USB_FIFO_SZ_32)
#define USB_FIFO_SZ_64_DB   (USB_FIFO_SIZE_DB_FLAG|USB_FIFO_SZ_64)
#define USB_FIFO_SZ_128_DB  (USB_FIFO_SIZE_DB_FLAG|USB_FIFO_SZ_128)
#define USB_FIFO_SZ_256_DB  (USB_FIFO_SIZE_DB_FLAG|USB_FIFO_SZ_256)
#define USB_FIFO_SZ_512_DB  (USB_FIFO_SIZE_DB_FLAG|USB_FIFO_SZ_512)
#define USB_FIFO_SZ_1024_DB (USB_FIFO_SIZE_DB_FLAG|USB_FIFO_SZ_1024)
#define USB_FIFO_SZ_2048_DB (USB_FIFO_SIZE_DB_FLAG|USB_FIFO_SZ_2048)
#define USB_FIFO_SZ_4096_DB (USB_FIFO_SIZE_DB_FLAG|USB_FIFO_SZ_4096)

I don't think we can totally get rid of 4096 and 8192, the code usbdconfig.c:GetEndpointFIFOSize() will loop through 4096 and 8192. I think the limit of 8192 comes from other platforms that have larger FIFOs. The StarterWare source is, as much as possible, common between platforms. We would need some sort of USB_MAX_FIFO platform constant. Maybe there is one already.

Walter Snafu over 10 years ago in reply to Norman Wong

Genius 3450 points

Norman Wong,

Agreed. Your latest list of #defines will function correctly. Though it does raise an issue of software philosophy. That is: How much detail ought be exposed? Versus, How much detail ought be hidden? Modular programming and object oriented programming have the goal of hiding unnecessary details, so programmers don't need to 'know' everything or 'verify' the correctness of everything. They need only know the minimum necessary to get the job done.

It is a subtle issue. And software programmers are quite understandable in having different points of view. Please forgive me for expressing a view on a rather minor point.

Goal #1, #2, and #3 of a header file is to be correct. It must be correct. Programmers rely on that. Goal #4 is use labels that are self-explanatory. Goal #5 is that the header file be easy to read. Goal #6 is that the header file be easy to debug.

Programmers lookup things in header files. Typically, they scan a list, to find a label to copy into their code. In this way, header files are intended, firstly, to be used by programmers -- not debugged. They are intended to hide information that programmers do not need. Do programmers really need to know the exact structure of the bit-fields, and how the bits are shifted and OR'd together? That information is actually a needless diversion to most programmers.

Your list of #defines is optimized toward someone debugging the header file. But we've already debugged it. Now we need to hide information the programmer doesn't need.

******

Concerning the USB FIFO, it should be emphasized in the header file itself: "It is the programmer's responsibility to ensure: (1) the FIFOs do not overlap in memory, and (2) the available USB FIFO memory can hold all the FIFOs, including the 64 bytes always required by endpoint 0. This latter point (the 64 bytes) will likely eliminate several FIFO sizes on your platform."

-- Walter Snafu

Processors

Processors forum

Possible error in Starterware USB double-buffering FIFO routines