Reporting of issues in S32K SDK

maksimsalau · ‎04-25-2018

Hello,

I'm playing with S32K144 and its SDK. Thanks for the SDK, it makes bring-up much easier!

I have found some places that can be improved. I want to report them so the development team can improve the SDK.

What is the preferred way to report such issues?

Here are issues I've found so far:

1. Incorrect type used for the timeout argument: SPI_MasterTransferBlocking accepts `uint16_t timeout` and passes the value to LPSPI_DRV_MasterTransferBlocking which accepts `uint32_t timeout`

status_t SPI_MasterTransferBlocking(
    spi_instance_t instance,
    void* txBuffer,
    void* rxBuffer,
    uint16_t numberOfFrames,
    uint16_t timeout);
status_t LPSPI_DRV_MasterTransferBlocking(
    uint32_t instance,
    const uint8_t * sendBuffer,
    uint8_t * receiveBuffer,
    uint16_t transferByteCount,
    uint32_t timeout);

Thus if you try to pass OSIF_WAIT_FOREVER, the compiler shows a warning that truncation will occur.

2. Incorrect C guards in can_pal.h:

#if defined(__cplusplus)
extern "C" {
#endif

#if defined(__cplusplus)
}
#endif

Literally. I.e. `extern "C" {}` doesn't protect function prototypes.

3. The LPUART driver allows to set custom RX and TX handlers/callbacks so one can implement his own logic, but `lpuart_hw_access.h` is not in the `drivers/inc` directory, and thus is not generally accessible. So we end up in a situation that you can define a custom handler but can't transmit a byte with `LPUART_Putchar()`, since the SDK has no header for it in `drivers/inc`. So I propose to either move `lpuart_hw_access.h` to `drivers/inc` or create a wrapper in `lpuart_driver.c|h`

The following function can act as the wrapper:

static void LPUART_DRV_PutData(uint32_t instance)

but it is defined as static. Making it public will solve my pain

4. Processor Expert shows incorrect frequency for divided clocks:

I.e PE shows 8MHz for both SIRCDIV1_CLK and SIRCDIV2_CLK. Same issue for all other clocks in the list.

5. PE doesn't generate a macro that defines SPI_PAL instance number.

I.e. for UART_PAL the following macro is defined in uart_pal1.h:

/*! @brief Device instance number */
#define INST_UART_PAL1 (1U)

But spi_pal1.h doesn't have such macro.

I hope this information will allow to make the SDK and PE even better.

Best regards,

Maksim.

danielmartynek · ‎05-04-2018

Hello Maksim,

Thank you, it has been forwarded to SDK design team.

I have just check the PE clock configuration.

I don't see any problem with that. Both dividers can be set in the Functional clocks window.

Regards,

Daniel

maksimsalau · ‎05-04-2018

Many thanks, Daniel!

I've interpreted names incorrectly. I thought that SIRCDIV2_CLK == SIRC_CLK/2 but it is just a second option, and the actual division factor is configured elsewhere.

Regards,

Maksim.

maksimsalau · ‎04-26-2018

Hello,

Few more findings:

6. SPI_PAL doesn't forward the isPcsContinuous option to LPSPI_DRV, but sets it false unconditionally, which significantly limits usage of the module.

7. LPSPI_SetBaudRate() doesn't allow to use scaler 0.

E.g.: I request 4MBit baudrate with 8MHz clock. This is possible with the following register values: SCKDIV = 0 (/2) and PRESCALE = 0 (/1). But LPSPI_SetBaudRate proceed to the next prescaler if low=0 and high=1:

        low = 0;
        high = 256;
        while((high - low) > (uint32_t)1)
        {
            scaler = (low + high) / (uint32_t)2;‍‍‍‍‍‍‍‍‍‍

So there is now way scaler can be 0 in the current implementation. The following version allows scaler to be 0:

        low = 0;
        high = 256;
        while((high - low) > (uint32_t)0)
        {
            scaler = (low + high) / (uint32_t)2;‍‍‍‍‍‍‍‍‍‍‍

Also the following lines are suspicious to me:

    /* Add default values for delay between transfers, delay between sck to pcs and between pcs to sck. */
    (void)LPSPI_SetDelay(base, LPSPI_SCK_TO_PCS, scaler >> 2U);
    (void)LPSPI_SetDelay(base, LPSPI_PCS_TO_SCK, scaler >> 2U);
    (void)LPSPI_SetDelay(base, LPSPI_BETWEEN_TRANSFER, scaler >> 2U);‍‍‍‍‍‍‍‍

I suppose bestScaler should be used instead. I.e.:

    /* Add default values for delay between transfers, delay between sck to pcs and between pcs to sck. */
    (void)LPSPI_SetDelay(base, LPSPI_SCK_TO_PCS, bestScaler >> 2U);
    (void)LPSPI_SetDelay(base, LPSPI_PCS_TO_SCK, bestScaler >> 2U);
    (void)LPSPI_SetDelay(base, LPSPI_BETWEEN_TRANSFER, bestScaler >> 2U);‍‍‍‍

Also there is a comment that we exit immediately if min_diff equals 0, but don't do this actually.

    /* In all for loops, if min_diff = 0, the exit for loop*/
    for (prescaler = (uint32_t)0; prescaler < (uint32_t)8; prescaler++)‍‍

The following patch fixes all the issues in that function:

--- ./lpspi_hw_access.c.orig     2018-04-26 00:55:11.489935500 -0700
+++ ./lpspi_hw_access.c     2018-04-26 01:13:37.033302300 -0700
@@ -496,11 +496,11 @@
     scaler = 0; /* required to avoid compilation warning */
 
     /* In all for loops, if min_diff = 0, the exit for loop*/
-    for (prescaler = (uint32_t)0; prescaler < (uint32_t)8; prescaler++)
+    for (prescaler = (uint32_t)0; (min_diff != (uint32_t)0) && (prescaler < (uint32_t)8); prescaler++)
     {
         low = 0;
         high = 256;
-        while((high - low) > (uint32_t)1)
+        while((min_diff != (uint32_t)0) && ((high - low) > (uint32_t)0))
         {
             scaler = (low + high) / (uint32_t)2;
             realBaudrate = (sourceClockInHz /
@@ -529,9 +529,9 @@
         }
     }
     /* Add default values for delay between transfers, delay between sck to pcs and between pcs to sck. */
-    (void)LPSPI_SetDelay(base, LPSPI_SCK_TO_PCS, scaler >> 2U);
-    (void)LPSPI_SetDelay(base, LPSPI_PCS_TO_SCK, scaler >> 2U);
-    (void)LPSPI_SetDelay(base, LPSPI_BETWEEN_TRANSFER, scaler >> 2U);
+    (void)LPSPI_SetDelay(base, LPSPI_SCK_TO_PCS, bestScaler >> 2U);
+    (void)LPSPI_SetDelay(base, LPSPI_PCS_TO_SCK, bestScaler >> 2U);
+    (void)LPSPI_SetDelay(base, LPSPI_BETWEEN_TRANSFER, bestScaler >> 2U);
 
     /* Write the best baud rate scalar to the CCR.
      * Note, no need to check for error since we've already checked to make sure the module is
‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍‍

I hope this will help.

Best regards,

Maksim.