Message-ID: <20250710054401.5hmhsdtyulcskwug@zodiac>
Date: Thu, 10 Jul 2025 00:44:01 -0500
From: Nishanth Menon <nm@...com>
To: Kendall Willis <k-willis@...com>
CC: <kristo@...nel.org>, <ssantosh@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <ulf.hansson@...aro.org>, <vigneshr@...com>, <d-gole@...com>,
        <vishalm@...com>, <sebin.francis@...com>, <msp@...libre.com>,
        <khilman@...libre.com>
Subject: Re: [PATCH v3] firmware: ti_sci: Enable abort handling of entry to
 LPM

On 17:16-20250709, Kendall Willis wrote:
> The PM co-processor (device manager or DM) adds the ability to abort
> entry to a low power mode by clearing the mode selection in the
> latest version of its firmware (11.x). The following power management
> operation defined in the TISCI Low Power Mode API [1] is implemented to
> enable aborting entry to LPM:
> 
> TISCI_MSG_LPM_ABORT
> Abort the current low power mode entry by clearing the current mode
> selection.
> 
> Introduce LPM abort call that enables the ti_sci driver to support abort
> by clearing the low power mode selection of the DM. This fixes behavior
> from the DM where if system suspend failed, the next time system suspend
> is entered, it will fail because DM did not have the low power mode
> selection cleared. Instead of this behavior, the low power mode selection
> will be cleared after Linux resume which will allow subsequent system
> suspends to work correctly.
> 
> When Linux suspends, the TI SCI ->suspend() call will send a prepare_sleep
> message to the DM. The DM will choose what low power mode to enter once
> Linux is suspended based on constraints given by devices in the TI SCI PM
> domain. After system suspend completes, regardless of if system suspend
> succeeds or fails, the ->complete() hook in TI SCI will be called. In the
> ->complete() hook, a message will be sent to the DM to clear the current
> low power mode selection. This is necessary because if suspend fails, the
> low power mode selection in the DM is not cleared and the next system
> suspend will fail due to the low power mode not having been cleared from
> the previous failed system suspend.
> 
> Clearing the mode selection unconditionally acts as a cleanup from sending
> the prepare_sleep message in ->suspend(). The DM already clears the low
> power selection automatically when resuming from system suspend. If
> suspend/resume executed without failure, clearing the low power mode
> selection will not cause an error in the DM.
> 
> The flow for the abort sequence is the following:
>    1. User sends a command to enter sleep
>    2. Linux starts to suspend drivers
>    3. TI SCI suspends and sends prepare_sleep message to DM
>    4. A driver fails to suspend
>    5. Linux resumes the drivers that have already suspended
>    6. Linux sends DM to clear the current low power mode selection from
>       TI SCI ->complete() hook
>    7. DM aborts LPM entry by clearing the current mode selection
>    8. Linux works as normal

Could we trim the message a bit down? it is informative, thanks.. but I
think a bit repetitive.

> 
> [1] https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/pm/lpm.html
> 
> Signed-off-by: Kendall Willis <k-willis@...com>
> ---
> Series has been tested on an SK-AM62B-P1 board. Normal suspend/resume
> has been verified. Abort was tested by adding an error into the TI SCI
> suspend hook.

btw, does this handle the noirq case as well? I have'nt looked closely
at the sequence to be sure.

> 
> Link to v2:
> https://lore.kernel.org/all/20250709205332.2235072-1-k-willis@ti.com/
> Link to v1:
> https://lore.kernel.org/all/20250627204821.1150459-1-k-willis@ti.com/
> 
> Changes from v2 to v3:
>   - added links to previous series and the changes between them

Thanks, but in the future, I'd rather not want a v3, but just reply
with the missing info and better still, add to your pre-send checklist
to ensure you don't miss it in the future ;).


> 
> Changes from v1 to v2:
>    - rebase on linux-next
>    - drop the following patch:
>      pmdomain: ti_sci: Add LPM abort sequence to suspend path
>    - remove lpm_abort from ti_sci_pm_ops
>    - add ->complete() hook with ti_sci_cmd_lpm_abort to be called
>      unconditionally within it
>    - remove ti_sci_cmd_lpm_abort from the ->suspend() and
>      ->suspend_noirq() hooks
>    - reword commit message
> ---
>  drivers/firmware/ti_sci.c | 61 +++++++++++++++++++++++++++++++++++++++
>  drivers/firmware/ti_sci.h |  3 +-
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> index ae5fd1936ad32..63c405f7037f0 100644
> --- a/drivers/firmware/ti_sci.c
> +++ b/drivers/firmware/ti_sci.c
> @@ -2015,6 +2015,58 @@ static int ti_sci_cmd_set_latency_constraint(const struct ti_sci_handle *handle,
>  	return ret;
>  }
>  
> +/**
> + * ti_sci_cmd_lpm_abort() - Abort entry to LPM by clearing selection of LPM to enter
> + * @handle:     pointer to TI SCI handle
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_cmd_lpm_abort(const struct ti_sci_handle *handle)
> +{
> +	struct ti_sci_info *info;
> +	struct ti_sci_msg_hdr *req;
> +	struct ti_sci_msg_hdr *resp;
> +	struct ti_sci_xfer *xfer;
> +	struct device *dev;
> +	int ret = 0;
> +
> +	if (IS_ERR(handle))
> +		return PTR_ERR(handle);
> +	if (!handle)
> +		return -EINVAL;
> +
> +	info = handle_to_ti_sci_info(handle);
> +	dev = info->dev;

-ECONFUSED. ti_sci_complete already gets dev and info and this API is
not exposed to other users. So why do we need to flip back and forth
with info->handle and then get info from handle and dev again??
> +
> +	xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_LPM_ABORT,
> +				   TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> +				   sizeof(*req), sizeof(*resp));
> +	if (IS_ERR(xfer)) {
> +		ret = PTR_ERR(xfer);
> +		dev_err(dev, "Message alloc failed(%d)\n", ret);
> +		return ret;
> +	}
> +	req = (struct ti_sci_msg_hdr *)xfer->xfer_buf;
> +
> +	ret = ti_sci_do_xfer(info, xfer);
> +	if (ret) {
> +		dev_err(dev, "Mbox send fail %d\n", ret);
> +		goto fail;
> +	}
> +
> +	resp = (struct ti_sci_msg_hdr *)xfer->xfer_buf;
> +
> +	if (!ti_sci_is_response_ack(resp))
> +		ret = -ENODEV;
> +	else
> +		ret = 0;
is'nt ret already 0?

OR you could go with ? like rest of code.. ;)

> +
> +fail:
> +	ti_sci_put_one_xfer(&info->minfo, xfer);
> +
> +	return ret;
> +}
> +
>  static int ti_sci_cmd_core_reboot(const struct ti_sci_handle *handle)
>  {
>  	struct ti_sci_info *info;
> @@ -3739,11 +3791,20 @@ static int __maybe_unused ti_sci_resume_noirq(struct device *dev)
>  	return 0;
>  }
>  
> +static void __maybe_unused ti_sci_complete(struct device *dev)

ti_sci_pm_complete or something like that?

> +{
> +	struct ti_sci_info *info = dev_get_drvdata(dev);
> +
> +	if (ti_sci_cmd_lpm_abort(&info->handle))

I see from the documentation of .complete that it is invoked in
multitude of scenarios, including resume as well. While I think it is
probably fine to clear the state, have you had a chance to look at
possible side effects in other flows (thaw etc..?)

Additionally, do we want to check info->fw_caps &
MSG_FLAG_CAPS_LPM_DM_MANAGED before sending it over to DM?

> +		dev_err(dev, "LPM clear selection failed.\n");
> +}
> +
>  static const struct dev_pm_ops ti_sci_pm_ops = {
>  #ifdef CONFIG_PM_SLEEP
>  	.suspend = ti_sci_suspend,
>  	.suspend_noirq = ti_sci_suspend_noirq,
>  	.resume_noirq = ti_sci_resume_noirq,
> +	.complete = ti_sci_complete,

Another question - when is .complete called as part of rewind? does DM
behave sane while other drivers are resuming back up before .complete is
invoked?

>  #endif
>  };
>  
> diff --git a/drivers/firmware/ti_sci.h b/drivers/firmware/ti_sci.h
> index 053387d7baa06..51d77f90a32cc 100644
> --- a/drivers/firmware/ti_sci.h
> +++ b/drivers/firmware/ti_sci.h
> @@ -6,7 +6,7 @@
>   * The system works in a message response protocol
>   * See: https://software-dl.ti.com/tisci/esd/latest/index.html for details
>   *
> - * Copyright (C)  2015-2024 Texas Instruments Incorporated - https://www.ti.com/
> + * Copyright (C)  2015-2025 Texas Instruments Incorporated - https://www.ti.com/

please dont keep shifting license year for trivial changes :)
>   */
>  
>  #ifndef __TI_SCI_H
> @@ -42,6 +42,7 @@
>  #define TI_SCI_MSG_SET_IO_ISOLATION	0x0307
>  #define TI_SCI_MSG_LPM_SET_DEVICE_CONSTRAINT	0x0309
>  #define TI_SCI_MSG_LPM_SET_LATENCY_CONSTRAINT	0x030A
> +#define TI_SCI_MSG_LPM_ABORT	0x0311

NOTE: all the LPM stuff is enabled with MSG_FLAG_CAPS_LPM_DM_MANAGED.
Is this supported from the very beginning version of firmware that
has this? else will we see issues in the field with a mix of firmware
versions.. some just crashing out when the message is not supported?

>  
>  /* Resource Management Requests */
>  #define TI_SCI_MSG_GET_RESOURCE_RANGE	0x1500
> 
> base-commit: 835244aba90de290b4b0b1fa92b6734f3ee7b3d9
> -- 
> 2.34.1
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D
https://ti.com/opensource

Powered by blists - more mailing lists