[PATCH] serial: msm_geni: validate RX data with IRQ status

[Balaji Selvanathan]

Hi Casey,

On 6/30/2026 1:27 PM, Casey Connolly wrote:
> Hi Balaji,
>
> On 6/29/26 13:31, Balaji Selvanathan wrote:
>> The GENI UART hardware can retain stale data in the RX FIFO.
>> The msm_serial_pending() function was reporting this stale data as
>> pending input, causing U-Boot to incorrectly assume a key press and
>> stop at the CLI prompt instead of continuing the boot process.
>>
>> Add IRQ status validation to distinguish between legitimate received
>> data and stale FIFO contents. Check the Secondary IRQ status register
>> for RX watermark or last flags before reporting pending data. If the
>> FIFO contains data but no valid IRQ flags are set, ignore it as stale.
>
> Taking a look here compared to the Linux polling mode driver I don't 
> think this is the right way to solve this.
>
> In qcom_geni_serial_stop_rx_fifo() in Linux after clearing the RX IRQs 
> the cancel command is sent (and polled on) and then if 
> S_RX_FIFO_LAST_EN is set the FIFO is drained.
>
> So the right fix here for U-Boot would be to do something similar in 
> msm_geni_serial_setup_rx(), send a cancel rather than an abort, wait 
> for it to complete (or timeout), drain the fifo, and then send an 
> abort if S_GENI_CMD_ACTIVE is still set to fully reset it.
>
> so NAK, please solve this properly by fixing the init sequence.

Fixed the init sequence in v2: 
https://lore.kernel.org/u-boot/20260707-uart-v2-0-d38277b6c4ce@oss.qualcomm.com/

But even after fixing init sequence, still the issue is observed, so 
seems SE_GENI_S_IRQ_STATUS register has to be checked to make sure data 
is valid 
(https://lore.kernel.org/u-boot/20260707-uart-v2-2-d38277b6c4ce@oss.qualcomm.com/).

Thanks,

Balaji

>
>>
>> Signed-off-by: Balaji Selvanathan <balaji.selvanathan at oss.qualcomm.com>
>> ---
>>   drivers/serial/serial_msm_geni.c | 27 ++++++++++++++++++++++-----
>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/serial/serial_msm_geni.c 
>> b/drivers/serial/serial_msm_geni.c
>> index 3dca581f68f..6d30f150966 100644
>> --- a/drivers/serial/serial_msm_geni.c
>> +++ b/drivers/serial/serial_msm_geni.c
>> @@ -424,13 +424,30 @@ static int msm_serial_getc(struct udevice *dev)
>>   static int msm_serial_pending(struct udevice *dev, bool input)
>>   {
>>       struct msm_serial_data *priv = dev_get_priv(dev);
>> -
>> -    if (input)
>> -        return readl(priv->base + SE_GENI_RX_FIFO_STATUS) &
>> -               RX_FIFO_WC_MSK;
>> -    else
>> +    u32 rx_fifo_status, s_irq_status;
>> +    int result;
>> +
>> +    if (input) {
>> +        rx_fifo_status = readl(priv->base + SE_GENI_RX_FIFO_STATUS);
>> +        result = rx_fifo_status & RX_FIFO_WC_MSK;
>> +
>> +        /*
>> +         * Validate RX data with IRQ status
>> +         * Only report pending data if valid RX IRQ indicators are set.
>
> What does this mean "valid RX IRQ indicators"??? This comment is 
> fluff, use better variable names (result -> word_count)
>> +         */
>> +        if (result) {
>
> if (word_count > 0)> +            s_irq_status = readl(priv->base + 
> SE_GENI_S_IRQ_STATUS);
>> +
>> +            if (!(s_irq_status & (S_RX_FIFO_WATERMARK_EN | 
>> S_RX_FIFO_LAST_EN))) {
>> +                /* No valid RX IRQ, ignore stale data */
>> +                return 0;
>> +            }
>> +        }
>> +        return result;
>> +    } else {
>>           return readl(priv->base + SE_GENI_TX_FIFO_STATUS) &
>>                  TX_FIFO_WC_MSK;
>> +    }
>>         return 0;
>>   }
>>
>> ---
>> base-commit: 6902fb4c17faa375003124c451c2550deab5463d
>> change-id: 20260629-uart-e07d893e4508
>>
>> Best regards,
>