[PATCH] serial: meson: fix meson_serial_pending() tx logic

[PATCH] serial: meson: fix meson_serial_pending() tx logic

[Mattijs Korpershoek]

According to the dm_serial_ops documentation, pending() should:
> @return number of waiting characters, 0 for none, -ve on error

And:
> It is acceptable to return 1 if an indeterminant number
> of characters is waiting.

With the current implementation, we have:
* FIFO is full           -> pending() returns 0
* FIFO is partially used -> pending() returns 1
* FIFO is empty          -> pending() returns 1

This is not the same as what the documentation requires.

Moreover, since [1], arm reset now flushes all console devices
(including serial) before the cpu gets reset.
Because of the flawed logic:

  => reset # user calls reset
  flush() is called
  _serial_flush() is called
  ops->pending(dev, false) # never returns false
  # board hangs indefinitely without resetting.

Fix it by using AML_UART_TX_EMPTY instead of AML_UART_TX_FULL.

[1] commit c5f4cdb8eb60 ("console: Use flush() before panic and reset"),

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
Tested with:
=> reset # vim3 board no longer hangs
---
 drivers/serial/serial_meson.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/serial/serial_meson.c b/drivers/serial/serial_meson.c
index d026f5a7a8e5..934de2ab2358 100644
--- a/drivers/serial/serial_meson.c
+++ b/drivers/serial/serial_meson.c
@@ -201,7 +201,10 @@ static int meson_serial_pending(struct udevice *dev, bool input)
 
 		return true;
 	} else {
-		return !(status & AML_UART_TX_FULL);
+		if (status & AML_UART_TX_EMPTY)
+			return false;
+
+		return true;
 	}
 }
 

---
base-commit: a1fd9cb41fb1fda2283a089f9c97e5eed4d4940a
change-id: 20230606-fix-meson-serial-pending-890e11467215

Best regards,
-- 
Mattijs Korpershoek <mkorpershoek@baylibre.com>

Re: [PATCH] serial: meson: fix meson_serial_pending() tx logic

[Neil Armstrong]

On 06/06/2023 18:07, Mattijs Korpershoek wrote:
> According to the dm_serial_ops documentation, pending() should:
>> @return number of waiting characters, 0 for none, -ve on error
> 
> And:
>> It is acceptable to return 1 if an indeterminant number
>> of characters is waiting.
> 
> With the current implementation, we have:
> * FIFO is full           -> pending() returns 0
> * FIFO is partially used -> pending() returns 1
> * FIFO is empty          -> pending() returns 1
> 
> This is not the same as what the documentation requires.
> 
> Moreover, since [1], arm reset now flushes all console devices
> (including serial) before the cpu gets reset.
> Because of the flawed logic:
> 
>    => reset # user calls reset
>    flush() is called
>    _serial_flush() is called
>    ops->pending(dev, false) # never returns false
>    # board hangs indefinitely without resetting.
> 
> Fix it by using AML_UART_TX_EMPTY instead of AML_UART_TX_FULL.
> 
> [1] commit c5f4cdb8eb60 ("console: Use flush() before panic and reset"),
> 
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
> Tested with:
> => reset # vim3 board no longer hangs
> ---
>   drivers/serial/serial_meson.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/serial/serial_meson.c b/drivers/serial/serial_meson.c
> index d026f5a7a8e5..934de2ab2358 100644
> --- a/drivers/serial/serial_meson.c
> +++ b/drivers/serial/serial_meson.c
> @@ -201,7 +201,10 @@ static int meson_serial_pending(struct udevice *dev, bool input)
>   
>   		return true;
>   	} else {
> -		return !(status & AML_UART_TX_FULL);
> +		if (status & AML_UART_TX_EMPTY)
> +			return false;
> +
> +		return true;
>   	}
>   }
>   
> 
> ---
> base-commit: a1fd9cb41fb1fda2283a089f9c97e5eed4d4940a
> change-id: 20230606-fix-meson-serial-pending-890e11467215
> 
> Best regards,

Acked-by: Neil Armstrong <neil.armstrong@linaro.org>