• State Resolved
  • Locked Locked
  • Replies 4 replies
  • Subscribers 98 subscribers
  • Views 546 views
  • Users 0 members are here
Support feedback
Options
Options
Related

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.

Compiler: Starterware UART.C mistake should be looked in to, I fixed it in my company's copy

Fredric Rice
Prodigy 130 points
Part Number: PROCESSOR-SDK-AM335X

Tool/software: TI C/C++ Compiler

In what looks to be the latest version f Starterware 2.1.1.0 dated 2013, there is a mistake in UART.C:

#define UART_IIR_FCR_MIRROR_MASK (0x000000c0U)
#define UART_IIR_FCR_MIRROR (0x000000C0u)
#define UART_IIR_FCR_MIRROR_SHIFT (0x00000006u)


uint32_t UARTIsFifoEnabled(uint32_t baseAddr)
{
    uint16_t lcrRegVal = 0U;
    uint32_t status = FALSE;

    /* Switch to Register Operational Mode of operation. */
    lcrRegVal = UARTSetRegAccessMode(baseAddr, UART_REG_ACCESS_MODE_OPER);

    if(UART_IIR_FCR_MIRROR_MASK == HW_RD_FIELD16((baseAddr + UART_IIR), UART_IIR_FCR_MIRROR))
    {
        status = TRUE;
    }

    /* Restoring the value of LCR. */
    HW_WR_REG16(baseAddr + UART_LCR, lcrRegVal);

    return status;
}

The MACRO will retrieve the FCR Mirror bits from the IIR register and then mask them with 0xC0, then sift the result right by 6 bits to place the two bits in to the least significant bits, then the code checks to see if the result is 0xC0 which is incorrect. The code should look more like this:

    if(0 != HW_RD_FIELD16((baseAddr + UART_IIR), UART_IIR_FCR_MIRROR))

The desire is to test the two FCI Mirror bits to see if they are either 00 or 11. The code as provided by TI always returns FALSE when someone calls the API to determine if their UART's FIFO is enabled or not.

I've fixed it in my company's copy of Starterware and because this is the second issue we have had (the first one was MACRO expansion of arguments not wrapped in parenthesis)

#define EDMA_TPCC_DMAQNUM(n) ((uint32_t)0x240U + (n * 0x4U))  <-- n not wrapped (n)

HW_WR_FIELD32(baseAddr + EDMA_TPCC_DMAQNUM(chNum >> 3U),
    EDMA_TPCC_DMAQNUM_E1, queueNum);

We are having one of our software engineers go through the Starterware modules looking for any other issues. This isn't a request for assistance, it's just a request that TI correct the function in future versions of Starterware.

Thanks!

 

  • 0
    TI__Guru**** 249380 points

    Which TI device do you use?

    Thanks and regards,

    -George

  • 0 in reply to George Mock
    Prodigy 130 points
    Woops! I'm sorry, it's the AM335x CPU. uart.c contains:

    "Copyright (C) 2013 Texas Instruments Incorporated"
  • 0 in reply to Fredric Rice
    TI__Guru** 116170 points
    Fredric,

    Thanks for bringing this to our attention. The starterware baseline that you are using is from 2013. this is legacy piece of software that is currently in maintenance mode. We currently only support PRocessor SDK RTOS baseline for all AM335x development and have integrated a small subset of starterware functionality into the baseline to allow customers to migrate. I will check to confirm that this issue is not present in the baseline that is currently actively being developed and maintained.

    Processor SDK RTOS provides a high level driver for UART and EDMA that supports a wider range of features for these modules on the device that baseline starterware. I will check to see if the issue that you have highlighted don`t exist in the existing baseline and file a bug ID to follow up. Thanks again for bringing this to our attention.

    Regards,
    Rahul

    PS: If this issue was found during as part of new development then I would consider switching to Processor SDK RTOS for longer term support and bug fixes in software for this device.
  • 0
    Prodigy 130 points

    Part Number: PROCESSOR-SDK-AM335X

    Tool/software: TI C/C++ Compiler

    Greetings, TI (and anyone else) :)

    This was a coding mistake that I fixed a couple of months ago, and since I posted a note here about two other mistakes in the Starterware (one in UART.C and the other with EDMA.C) I thought I would drop a note here and see if TI wants to check their latest versions of Starterware to see if this is an issue in their current versions. It is not a problem for my company any longer so this E2E is merely to ask TI to look at their existing code and correct it if it's still a problem.

    In the AM335x Starterware version 2.1.1.0 in function UARTSetBaudRate() the existing code looks like this:

    #define UART_DLL_CLOCK_LSB_MASK (0x000000ffU)
    #define UART_DLH_CLOCK_MSB_MASK (0x0000003fU)
    #define UART_DLH_CLOCK_MSB_SHIFT (0U)

    lsbDivisorVal = (divisorVal & UART_DLL_CLOCK_LSB_MASK);
    msbDivisorVal = ((divisorVal & UART_DLH_CLOCK_MSB_MASK) >> UART_DLH_MSB_SHIFT);

    So to acquire a baud rate of 9600 with a base clock of 48,000,000 MHZ, the calculated divisorVal is 48000000 /(16 * 9600) or 312 decimal which is 0x138 Hex. Using the UART.C module provided by TI in the AM335x Starterware version 2.1.1.0 the code for lsbDivisorVal is correct (the value computed is 0x38 hex) however the value for msbDivisorVal is incorrect because

    msbDivisorVal = ((0x138 & 0x3F) >> 0);
    msbDivisorVal = ((0x38) >> 0)
    msbDivisorVal = 0x38

    When the correct answer would actually be 0x01

    To correct the coding mistake we did:

    msbDivisorVal = ((divisorVal >> 8u) & UART_DLH_CLOCK_MSB_MASK);

    So that now

    msbDivisorVal = ((0x138 >> 8u) & 0x3F)
    msbDivisorVal = (0x01 & 0x3f)
    msbDivisorVal = 0x01

    Our products have had the broken code in it for over 10 years, but because we typically operate at 115,200 baud, the divisor never exceeded 8 bits of occupation, however testing at 19,200 and 9,600 failed so I tracked it down to the Starterware function.

    Thanks!