RE: [PATCH v2 tty] tty: serial: fsl_lpuart: LS1021A has a FIFO size of 16 words, like LS1028A

From: Andy Duan
Date: Thu Oct 22 2020 - 21:49:01 EST


From: Vladimir Oltean <vladimir.oltean@xxxxxxx> Sent: Friday, October 23, 2020 9:34 AM
> Prior to the commit that this one fixes, the FIFO size was derived from the
> read-only register LPUARTx_FIFO[TXFIFOSIZE] using the following
> formula:
>
> TX FIFO size = 2 ^ (LPUARTx_FIFO[TXFIFOSIZE] - 1)
>
> The documentation for LS1021A is a mess. Under chapter 26.1.3 LS1021A
> LPUART module special consideration, it mentions TXFIFO_SZ and RXFIFO_SZ
> being equal to 4, and in the register description for LPUARTx_FIFO, it shows the
> out-of-reset value of TXFIFOSIZE and RXFIFOSIZE fields as "011", even though
> these registers read as "101" in reality.
>
> And when LPUART on LS1021A was working, the "101" value did correspond to
> "16 datawords", by applying the formula above, even though the
> documentation is wrong again (!!!!) and says that "101" means 64 datawords
> (hint: it doesn't).
>
> So the "new" formula created by commit f77ebb241ce0 has all the premises of
> being wrong for LS1021A, because it relied only on false data and no actual
> experimentation.
>
> Interestingly, in commit c2f448cff22a ("tty: serial: fsl_lpuart: add LS1028A
> support"), Michael Walle applied a workaround to this by manually setting the
> FIFO widths for LS1028A. It looks like the same values are used by LS1021A as
> well, in fact.
>
> When the driver thinks that it has a deeper FIFO than it really has, getty (user
> space) output gets truncated.
>
> Many thanks to Michael for pointing out where to look.
>
> Fixes: f77ebb241ce0 ("tty: serial: fsl_lpuart: correct the FIFO depth size")
> Suggested-by: Michael Walle <michael@xxxxxxxx>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> ---
> Changes in v2:
> Reworded commit message.

For the v2 with commit message change:
Reviewed-by:Fugang Duan <fugang.duan@xxxxxxx>
>
> drivers/tty/serial/fsl_lpuart.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c index
> ff4b88c637d0..bd047e1f9bea 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -314,9 +314,10 @@ MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
> /* Forward declare this for the dma callbacks*/ static void
> lpuart_dma_tx_complete(void *arg);
>
> -static inline bool is_ls1028a_lpuart(struct lpuart_port *sport)
> +static inline bool is_layerscape_lpuart(struct lpuart_port *sport)
> {
> - return sport->devtype == LS1028A_LPUART;
> + return (sport->devtype == LS1021A_LPUART ||
> + sport->devtype == LS1028A_LPUART);
> }
>
> static inline bool is_imx8qxp_lpuart(struct lpuart_port *sport) @@ -1701,11
> +1702,11 @@ static int lpuart32_startup(struct uart_port *port)
> UARTFIFO_FIFOSIZE_MASK);
>
> /*
> - * The LS1028A has a fixed length of 16 words. Although it supports the
> - * RX/TXSIZE fields their encoding is different. Eg the reference manual
> - * states 0b101 is 16 words.
> + * The LS1021A and LS1028A have a fixed FIFO depth of 16 words.
> + * Although they support the RX/TXSIZE fields, their encoding is
> + * different. Eg the reference manual states 0b101 is 16 words.
> */
> - if (is_ls1028a_lpuart(sport)) {
> + if (is_layerscape_lpuart(sport)) {
> sport->rxfifo_size = 16;
> sport->txfifo_size = 16;
> sport->port.fifosize = sport->txfifo_size;
> --
> 2.25.1