Issue with mss uart driver on FreeRTOS for tag 2023.6

[aScurtu86]

In uart_isr()

https://github.com/polarfire-soc/platform/blob/main/drivers/mss/mss_mmuart/mss_uart.c

for IIRF_THRE the code has changed and the lines (1603-1606):
if (this_uart->tx_idx == this_uart->tx_buff_size)
{
MSS_UART_disable_irq(this_uart, MSS_UART_TBE_IRQ);
this_uart->tx_buff_size = TX_COMPLETE;
}
were moved from the end of the tx_handler routine to the the isr body right after the handler has been called.

This is an issue in case the tx_handler calls portEND_SWITCHING_ISR(xHigherPriorityTaskWoken) because a task sending data over uart will restart before returning to the ISR and in case the task wants to send someting else on the using interrupts the call will stop in MSS_UART_irq_tx() at line 357:

ASSERT(TX_COMPLETE == this_uart->tx_buff_size);

[maxbourges]

Hello Andrei,

Could you send me the tx handler you have defined ? The issue might come from it.
The latest update of the driver was motivated by a "bug" in the way of using the user-defined tx handler. One of the consequence of the fix is the way to implement the tx handler has changed: we provided an example of user-defined tx handler here: https://github.com/polarfire-soc/platform/blob/main/drivers/mss/mss_mmuart/mss_uart.h#L2246 We recommend to use the lines 2248 to 2258 i your user-defined handler.

[aScurtu86]

Hello Max,

This is my tx handler:

void uart_tx_handler(mss_uart_instance_t * base)
{
usart_handle_t *handle;

size_t size_in_fifo;
const uint8_t * sub_buffer = &base->tx_buffer[base->tx_idx];
uint32_t sub_buffer_size = base->tx_buff_size - base->tx_idx;

/* Check arguments */
handle = getUsartHandle(base);
assert((NULL != base) && (NULL != handle));


ASSERT(( (uint8_t*)0 ) != base->tx_buffer);
ASSERT(0u < base->tx_buff_size);



size_in_fifo = MSS_UART_fill_tx_fifo(base,
                                     sub_buffer,
                                     sub_buffer_size);
base->tx_idx += size_in_fifo;

/* Flag Tx as complete if all data has been pushed into the Tx FIFO. */
if (base->tx_idx == base->tx_buff_size)
{
  /* Set txState to idle only when all data has been sent out to bus. */
  handle->txState = (uint8_t)kUSART_TxIdle;

  //code moved from the isr; otherwise when the callback yields from isr a task will run
  //and in case it sends data to the uart it will stop in the  ASSERT(TX_COMPLETE == this_uart->tx_buff_size);
  //of  MSS_UART_irq_tx()
  base->tx_buff_size = 0;//TX_COMPLETE;
  MSS_UART_disable_irq(base, MSS_UART_TBE_IRQ);


  /* Trigger callback. */
  if (handle->callback != NULL)
  {
      handle->callback(base, handle, kStatus_USART_TxIdle, handle->userData);
  }
}

}

It is based on the provided example and also includes the fix for the problem I reported.
This is the callback:

static void USART_RTOS_Callback(mss_uart_instance_t *base, usart_handle_t *state, status_t status, void *param)
{
usart_rtos_handle_t *handle = (usart_rtos_handle_t *)param;
BaseType_t xHigherPriorityTaskWoken, xResult;

xHigherPriorityTaskWoken = pdFALSE;
xResult                  = pdFAIL;

if (status == kStatus_USART_RxIdle)
{
    xResult = xEventGroupSetBitsFromISR(handle->rxEvent, RTOS_USART_COMPLETE, &xHigherPriorityTaskWoken);
}
else if (status == kStatus_USART_TxIdle)
{
    xResult = xEventGroupSetBitsFromISR(handle->txEvent, RTOS_USART_COMPLETE, &xHigherPriorityTaskWoken);
}
else if (status == kStatus_USART_RxRingBufferOverrun)
{
    xResult = xEventGroupSetBitsFromISR(handle->rxEvent, RTOS_USART_RING_BUFFER_OVERRUN, &xHigherPriorityTaskWoken);
}
else
{
    xResult = pdFAIL;
}

if (xResult != pdFAIL)
{
  portEND_SWITCHING_ISR(xHigherPriorityTaskWoken);// can be portYIELD_FROM_ISR or portEND_SWITCHING_ISR depending on the port
}

}

My assumption is that the new driver was only tested with the bare metal examples, as in that usage case there is no issue. The issue I'm having only affects some of the ports of FreeRTOS because it is generated by the way the context switching is handled by the task scheduler.
The Polarfire port of FreeRTOS seems to switch the context from the task scheduler directly to the woken task and not back to the ISR that was beeing serviced. I did not find any way to configure this behavior and it seems to be embedded in the port of FreeRTOS. Some of the literature I found on the subject made a note that for ports that behave like this, the portEND_SWITCHING_ISR(xHigherPriorityTaskWoken); call needs to be the last line of code of the ISR because controll will be directly switched to the newly woken up task before returning to the ISR and execute other parts of code.
This is what I'm seeing in this case: the control returns to the task before the ISR has a chance to run completely and to set TX_COMPLETE, so when the task tries to send the next chunk of data using MSS_UART_irq_tx(). it hangs in the ASSERT(TX_COMPLETE == this_uart->tx_buff_size); because tx_buff_size stil has the non-zero value from the last call of the transmit function.

This was not the case before the driver was updated, as the TX_COMPLETE was set before the user tx handler was called. So this situation was created by the latest changes that moved driver code after the call to the tx handler.

I'm new to FreeRTOS so I may be missing something here, so please correct me if I'm wrong.

[maxbourges]

Oh ok I understand better the situation. I think you are right about what the issue is. We did not anticipate the possibility of a UART tx isr being triggered inside a UART tx isr (in a bare-metal context, this use case would not really happen).

Anyway, your fix makes total sense for this particular use case. The fix at application level should not conflict with the driver implementation.
We made the change you mentioned in your first message so users would not have to worry about adding these lines in their handler, and so make it more user friendly. We will try to think of a way to implement the UART isr take this use case into account. Thank you for reporting it !