Re: [PATCH] serial: qcom-geni: add force suspend/resume to system sleep callbacks

[Praveen Talari <praveen.talari@oss.qualcomm.com>]

HI Mukesh

On 01-07-2026 20:47, Mukesh Savaliya wrote:
>
>
> On 7/1/2026 11:27 AM, Praveen Talari wrote:
>> During system sleep the hardware resources (clocks, interconnect) are
>> not gated because the runtime-suspend callback is never invoked from
>> the system sleep path.  This prevents the platform from reaching its
>> lowest idle state.
>>
>> The system sleep callbacks qcom_geni_serial_suspend() and
>> qcom_geni_serial_resume() rely solely on uart_suspend_port() /
>> uart_resume_port() to manage power.  uart_suspend_port() drives the
>> UART PM state machine to UART_PM_STATE_OFF, which in turn calls
>> pm_runtime_put_sync() and eventually the runtime-suspend callback.
>> However, if the runtime-PM usage count is still elevated at the time
>> of system sleep (e.g. the port is held active by an open file
>> descriptor), the runtime-suspend callback is never invoked and the
>> hardware resources (clocks, interconnect) remain enabled across
>> suspend, preventing the platform from reaching its lowest idle state.
>>
>> Fix this by calling pm_runtime_force_suspend() at the end of
>> qcom_geni_serial_suspend() so that the runtime-suspend callback is
>> always executed regardless of the usage count, and by calling
>> pm_runtime_force_resume() at the start of qcom_geni_serial_resume()
>> to restore those resources before uart_resume_port() re-opens the
>> port.
>>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
> [...]
>> @@ -1963,7 +1964,19 @@ static int qcom_geni_serial_suspend(struct 
>> device *dev)
>>           geni_icc_set_tag(&port->se, QCOM_ICC_TAG_ACTIVE_ONLY);
>>           geni_icc_set_bw(&port->se);
>>       }
>> -    return uart_suspend_port(private_data->drv, uport);
>> +
>> +    ret = uart_suspend_port(private_data->drv, uport);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /*
>> +     * When no_console_suspend is set the console must remain active
>> +     * across system sleep, so skip the force suspend path.
>> +     */
>> +    if (uart_console(uport) && !uport->suspended)
>> +        return 0;
> Rather use console_suspend_enabled and take action to go force suspend.
In uart_suspend_port(), uport->suspended is updated only after the 
console_suspend_enabled check. Therefore, its value directly reflects 
whether the console suspend path was taken:

uport->suspended == 0 → the console was not suspended.
uport->suspended == 1 → the console was suspended.

Looking at the code below, when console_suspend_enabled is disabled for 
a console port, the function returns before setting uport->suspended = 
1. As a result, uport->suspended remains 0, which accurately indicates 
that the console was not suspended.
Therefore, I believe using uport->suspended is the more appropriate 
check here. Please let me know your thoughts.

Code snippet from core layer

int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
{
[...]

     /*
      * Nothing to do if the console is not suspending
      * except stop_rx to prevent any asynchronous data
      * over RX line. However ensure that we will be
      * able to Re-start_rx later.
      */
     if (!console_suspend_enabled && uart_console(uport)) {
         if (uport->ops->start_rx) {
             guard(uart_port_lock_irq)(uport);
             uport->ops->stop_rx(uport);
         }
         device_set_awake_path(uport->dev);
         return 0;
     }

     uport->suspended = 1;

     if (tty_port_initialized(port)) {
[...]
}

> Here, it sounds opposite, if port is resumed, you don't go to suspend 
> within suspend function.
It is straightforward: uport->suspended remains 0 even after 
uart_suspend_port() is called, which indicates that the console has not 
been suspended.
>> +
>> +    return pm_runtime_force_suspend(dev);
> Is this really required ? if  uart_suspend_port() successful, what 
> will happen with this ?

Yes, this is covered in the commit message. The key point is that 
uart_suspend_port() may not trigger the runtime suspend callback if the 
runtime-PM usage count remains non-zero. In such cases, 
pm_runtime_force_suspend() is needed to ensure that the hardware 
resources are properly suspended during system sleep like our i2c/spi 
supported.

Thanks,

Praveen Talari

>
>>   }
> [...]
>
>