This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

Two Bugs in AM335x PDK 3.0 McSPI Driver Prevents Using Word Widths Greater than 8 Bits

Other Parts Discussed in Thread: AM3358

Greetings.

What I'm working with:  Win7-64-bit,

Dev Env:  CCS 6.1.2

Platform:  Custom board with MYIR brand MCC-AM335X-Y board with AM3358, 250MB RAM and other electronics that seem to be working perfectly.

Packages:  SYS/BIOS 6.45.1.29, UIA 2.0.5.50, AM335x PDK 3.0 (installs C:\ti\pdk_am335x_1_0_3\...)


I see that the <pdk>\packages\ti\drv\spi\...  and <pdk>\packages\ti\csl\src\ip\mcspi\v0\priv\mcspi.c  appear to be fairly evolved and cleverly implemented.

However, I REALLLLLLLLY wish that the designer would write down somewhere what his design intentions are because it has taken me HOURS under the debugger to figure what he was trying to do, only to find a bug in the design of how the McSPI driver works with the CSL library.  This seems to be a common situation in the PDK source code I see:  nobody wrote down the design or the designer's intentions on how to use it -- this should be in the top of the .H or .C files involved.

So this is in part a bug report to fix the following bug, and in part, a plea for better documentation.  The author should be writing to an audience that has never seen the driver before, but can be expected to understand SPI [or whatever] protocol -- but what I have seen more often than not is that the person writing the documentation seems to be writing to himself or another technician with a high level of familiarity with the driver's design.  If it is written with the appropriate introductory material that describes the overall flow of control, overall flow of data, and what the designer's intentions are on how to use it correctly, then it doesn't take so long to figure out how to correctly use a driver.  It's fine to write a driver with constraints, but AT LEAST THE AUTHOR SHOULD WRITE DOWN WHAT THE CONSTRAINTS ARE so others aren't tripped up by making wrong assumptions.  An example of such constraints in this driver:  the MCSPI_CHxCONF.TRM  bits are always going to be set to "Transmit and receive mode", even though the silicon supports other combinations.  This then causes the FIFO to be split across RX and TX data paths.  There is code in the SPI_v1.c (in the OPEN function) for the other FIFO configurations, but the other parts of the IF/ELSE IF/ELSE block will never execute because of the first constraint.

What causes this is this code that determines how the MCSPI_CHxCONF.TRM bits will be set:

        /* Extract actual mode */
        if(SPI_MASTER == params->mode)
        {
          object->spiMode = MCSPI_TX_RX_MODE;
        }
        else
        {
          object->spiMode = MCSPI_TX_RX_MODE;
        }

As you can see, there are no other choices.

But I digress.  On to the bugs:

How I discovered these bugs is that I have a remote SPI Slave device that uses exclusively 32-bit words SPI transfers.  According to the TRM, the silicon appears to be up for the challenge.  So I set up my own SPI_Params  object to reflect my needs (notably, 32-bit word width). I expected (hoped) it would work on the first try, so I ran it with the remote chip connected, and didn't get what I expected, so I disconnected the remote chip and simply hooked up a multi-channel oscilloscope to the 4 SPI pins to see (chip-select included) so I could see exactly what was happening.  But under a scope, the SPI clock was activating (32 bits worth of SCK signal), but only the first byte (MSB) traveling on the SPI data lines, and in the LSB position from the viewpoint of the remote SPI Slave device!

###   Bug #1:  TX data width:   C:\ti\pdk_am335x_1_0_3\packages\ti\drv\spi\src\v1\SPI_v1.c

Function:  SPI_v1_hwiFxn()

The (undocumented) design that this driver uses to transmit is that it sets the TX EMPTY interrupt (among others) and then handles populating the TX FIFO (or TX register) from within the interrupt that gets generated as soon as the channel (0) is enabled and the Chip-Select line is forced active.  The problem is that the code that populates the TX FIFO (or TX register) is coded in a way so as to ignore the user-configurable word width -- instead it is hard-coded to only write an 8-bit byte.  Specifically, this line of code -- the 2nd argument uses a pointer to grab and submit only the 8 bits at  'object->writeBufIdx':

            McSPITransmitData(hwAttrs->baseAddr, *(object->writeBufIdx), hwAttrs->chNum);

whereas I believe it should be coded to submit the correct word width based on  object->spiParams.dataSize  which is correctly carrying the value 32.

Old code:

        while ((((McSPIChannelStatusGet(hwAttrs->baseAddr, hwAttrs->chNum) &
               CSL_MCSPI_CH0STAT_TXFFF_MASK) >> CSL_MCSPI_CH0STAT_TXFFF_SHIFT) == 0U) &&
               (object->writeCountIdx != 0U)) {

            /* Fill the FIFO with remaining data */
            McSPITransmitData(hwAttrs->baseAddr, *(object->writeBufIdx), hwAttrs->chNum);
            (object->writeBufIdx)++;
            object->writeCountIdx--;
        }

Replaced with:

    	luiDataSize = object->spiParams.dataSize;

		if (luiDataSize <= 8) {
			while ((((McSPIChannelStatusGet(hwAttrs->baseAddr, hwAttrs->chNum) &
					CSL_MCSPI_CH0STAT_TXFFF_MASK) >> CSL_MCSPI_CH0STAT_TXFFF_SHIFT) == 0U) &&
					(object->writeCountIdx != 0U)) {

				McSPITransmitData(hwAttrs->baseAddr, *(object->writeBufIdx), hwAttrs->chNum);
				object->writeBufIdx++;
				object->writeCountIdx--;
			}
		} else if (luiDataSize <= 16) {
			while ((((McSPIChannelStatusGet(hwAttrs->baseAddr, hwAttrs->chNum) &
					CSL_MCSPI_CH0STAT_TXFFF_MASK) >> CSL_MCSPI_CH0STAT_TXFFF_SHIFT) == 0U) &&
					(object->writeCountIdx != 0U)) {

				McSPITransmitData(hwAttrs->baseAddr, *(uint16_t *)(object->writeBufIdx), hwAttrs->chNum);
				object->writeBufIdx += sizeof(uint16_t);
				object->writeCountIdx--;
			}
		} else if (luiDataSize <= 32) {
			while ((((McSPIChannelStatusGet(hwAttrs->baseAddr, hwAttrs->chNum) &
					CSL_MCSPI_CH0STAT_TXFFF_MASK) >> CSL_MCSPI_CH0STAT_TXFFF_SHIFT) == 0U) &&
					(object->writeCountIdx != 0U)) {

				McSPITransmitData(hwAttrs->baseAddr, *(uint32_t *)(object->writeBufIdx), hwAttrs->chNum);
				object->writeBufIdx += sizeof(uint32_t);
				object->writeCountIdx--;
			}
		}

is working for 32-bit data, but this bears proving if this is the correct logic for the silicon or not for other word sizes (e.g. the silicon appears capable of a word size of, say 19 bits, and I haven't tested this).

###   Bug #2:  RX data width:   C:\ti\pdk_am335x_1_0_3\packages\ti\drv\spi\src\v1\SPI_v1.c

Also in Function:  SPI_v1_hwiFxn()

A similar assumption (hard-coded 8 bit word width) is in the portion of the ISR function that handles  receiving data from the RX register or FIFO.  Because 'object->readBufIdx' is a  uint8_t  object, the following code from the ISR will only ever retrieve 8 bits at a time even when the actual RX register (or FIFO) read (when the word width is 32 bits) actually retrieves 32 bits at a time, thus losing 3/4 of the data:

         while ((((McSPIChannelStatusGet(hwAttrs->baseAddr, hwAttrs->chNum) & CSL_MCSPI_CH0STAT_RXFFE_MASK) == 0U)
               || ((McSPIChannelStatusGet(hwAttrs->baseAddr, hwAttrs->chNum) & CSL_MCSPI_CH0STAT_RXS_MASK) != 0U)) && (object->readCountIdx != 0U)) {
            /* Read from the FIFO until no data left */
            *(object->readBufIdx) = McSPIReceiveData(hwAttrs->baseAddr, hwAttrs->chNum);
            (object->readBufIdx)++;
            object->readCountIdx--;
        }

The solution to this would (I believe) be similar to the solution for the TX word width.

=-=-=-=

There is (in my opinion) also some missing capabilities that the silicon has, that the SPI driver is not set up to utilize.  For example, the this same application with the 32-bit-word external SPI Slave device, the most appropriate utilization of FIFO would be NONE on the TX side, and a full FIFO (64 bytes) on the RX.  But there is nothing in the SPI_Params or  SPI_v1_HWAttrs object in the SPI_soc.c file that allows this.

=-=-=-=

Another design bug in my opinion:  the SPI_v1_HWAttrs object is supposed to be SPI hardware attributes (such as interrupt numbers, register addresses, clock speeds, which it has), but it ADDITIONALLY contains some application configuration information that should be in the SPI_Params object -- namely, 3- or 4-pin mode, data line configuration, channel mode, chip-select polarity, and whether interrupts are enabled are all APPLICATION configuration choices -- not attributes of the AM335x).  Sugg:  move these fields to the SPI_Params struct.

=-=-=-=

Also, the SPI_soc.c::SPI_config  object lists a QSPI peripheral that doesn't exist in the AM335x -- at least the AM335x TRM and memory maps do not mention the AM335x having any QSPI peripherals.  Sugg:  remove QSPI device from SPI_config object.

=-=-=-=

Finally, <pdk>\packages\ti\csl\src\ip\mcspi\v0\hw_mcspi.h  contains a host of very useful bit-field definitions for the McSPI peripherals, but the following are worded such as to promote a misunderstanding.  Namely, the MCSPI_CHxCONF.DPE0 and DPE1 bits when placed next to the definition of the 'IS' (Input Select) bit could be misunderstood (the DPEx bits) to be fore ENABLING the pins (pads) rather than what they really are:  enabling TX on each DPEx pin that has a '0' bit in its corresponding DPEx field in this register.  These "setting values" in the  hw_mcspi.h  file tend to promote this misunderstanding.

#define MCSPI_CH0CONF_DPE0_DISABLED           (1U)
#define MCSPI_CH0CONF_DPE0_ENABLED            (0U)

// and

#define MCSPI_CH0CONF_DPE1_ENABLED            (0U)
#define MCSPI_CH0CONF_DPE1_DISABLED           (1U)

If I had been naming these symbols, I would have called them:

#define MCSPI_CH0CONF_DPE0_TX_DISABLED           (1U)
#define MCSPI_CH0CONF_DPE0_TX_ENABLED            (0U)

// and

#define MCSPI_CH0CONF_DPE1_TX_ENABLED            (0U)
#define MCSPI_CH0CONF_DPE1_TX_DISABLED           (1U)

Sugg:  rename them to clarify as above.

As a side effect, this would cause the #define'd symbols that begin with "MCSPI_DATA_LINE_COMM_MODE_..." in  mcspi.h  to be a GREAT DEAL easier to understand what they mean!

Hope this helps.

Kind regards,
Vic

  • Note: I don't think the above is a complete solution because -- at least on the reading side -- because, at least according to my understanding of the McSPI section of the AM335x TRM, the McSPIReceiveData() function (or a set of functions) would have to do a pointer-read from the RX register using a specified bit width (8, 16 or 32) in order to fully dovetail with the silicon's capabilities. This statement would need to be proven by testing.  As it is, the function only does 32-bit reads.