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

[Praveen Talari]

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@xxxxxxxxxxxxxxxx>
> > ---
> [...]
> > @@ -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

> >   }
> [...]