diff mbox series

[V6,4/4] firmware: ti_sci: Introduce system suspend resume support

Message ID 20230803064247.503036-5-d-gole@ti.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series firmware: ti_sci: Introduce system suspend support | expand

Commit Message

Dhruva Gole Aug. 3, 2023, 6:42 a.m. UTC
Introduce system suspend resume calls that will allow the ti_sci
driver to support deep sleep low power mode when the user space issues a
suspend to mem.

Also, write a ti_sci_prepare_system_suspend call to be used in the driver
suspend handler to allow the system to identify the low power mode being
entered and if necessary, send TISCI_MSG_PREPARE_SLEEP with information
about the mode is being entered and the address for allocated memory for
storing the context during Deep Sleep.

We're using "pm_suspend_target_state" to map the kernel's target suspend
state to SysFW low power mode. Make sure this is available only when
CONFIG_SUSPEND is enabled.

Co-developed-by: Dave Gerlach <d-gerlach@ti.com>
Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
Signed-off-by: Dhruva Gole <d-gole@ti.com>
---
 drivers/firmware/ti_sci.c | 63 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

Comments

Andrew Davis Aug. 3, 2023, 3:26 p.m. UTC | #1
On 8/3/23 1:42 AM, Dhruva Gole wrote:
> Introduce system suspend resume calls that will allow the ti_sci
> driver to support deep sleep low power mode when the user space issues a
> suspend to mem.
> 
> Also, write a ti_sci_prepare_system_suspend call to be used in the driver
> suspend handler to allow the system to identify the low power mode being
> entered and if necessary, send TISCI_MSG_PREPARE_SLEEP with information
> about the mode is being entered and the address for allocated memory for
> storing the context during Deep Sleep.
> 
> We're using "pm_suspend_target_state" to map the kernel's target suspend
> state to SysFW low power mode. Make sure this is available only when
> CONFIG_SUSPEND is enabled.
> 
> Co-developed-by: Dave Gerlach <d-gerlach@ti.com>
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
> Signed-off-by: Dhruva Gole <d-gole@ti.com>
> ---
>   drivers/firmware/ti_sci.c | 63 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> index 0334ade19868..172b726e00a6 100644
> --- a/drivers/firmware/ti_sci.c
> +++ b/drivers/firmware/ti_sci.c
> @@ -22,6 +22,7 @@
>   #include <linux/slab.h>
>   #include <linux/soc/ti/ti-msgmgr.h>
>   #include <linux/soc/ti/ti_sci_protocol.h>
> +#include <linux/suspend.h>
>   #include <linux/reboot.h>
>   
>   #include "ti_sci.h"
> @@ -3521,6 +3522,67 @@ static int tisci_reboot_handler(struct notifier_block *nb, unsigned long mode,
>   	return NOTIFY_BAD;
>   }
>   
> +static int ti_sci_prepare_system_suspend(struct ti_sci_info *info)
> +{
> +#if IS_ENABLED(CONFIG_SUSPEND)
> +	u8 mode;
> +
> +	/* Map and validate the target Linux suspend state to TISCI LPM. */
> +	switch (pm_suspend_target_state) {
> +	case PM_SUSPEND_MEM:
> +		/* S2MEM is not supported by the firmware. */
> +		if (!(info->fw_caps & MSG_FLAG_CAPS_LPM_DEEP_SLEEP))
> +			return 0;
> +		mode = TISCI_MSG_VALUE_SLEEP_MODE_DEEP_SLEEP;
> +		break;
> +	default:
> +		/*
> +		 * Do not fail if we don't have action to take for a
> +		 * specific suspend mode.
> +		 */
> +		return 0;
> +	}
> +
> +	return ti_sci_cmd_prepare_sleep(&info->handle, mode,
> +					(u32)(info->ctx_mem_addr & 0xffffffff),
> +					(u32)((u64)info->ctx_mem_addr >> 32), 0);
> +#else
> +	return 0;
> +#endif
> +}
> +
> +static int ti_sci_suspend(struct device *dev)
> +{
> +	struct ti_sci_info *info = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_ENABLE);

After this the will the IOs be in isolation? Or does the firmware wait
until power down begins later?

Andrew

> +	if (ret)
> +		return ret;
> +	dev_dbg(dev, "%s: set I/O isolation: %d\n", __func__, ret);
> +
> +	ret = ti_sci_prepare_system_suspend(info);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ti_sci_resume(struct device *dev)
> +{
> +	struct ti_sci_info *info = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_DISABLE);
> +	if (ret)
> +		return ret;
> +	dev_dbg(dev, "%s: disable I/O isolation: %d\n", __func__, ret);
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(ti_sci_pm_ops, ti_sci_suspend, ti_sci_resume);
> +
>   static int ti_sci_init_suspend(struct platform_device *pdev,
>   			       struct ti_sci_info *info)
>   {
> @@ -3759,6 +3821,7 @@ static struct platform_driver ti_sci_driver = {
>   	.driver = {
>   		   .name = "ti-sci",
>   		   .of_match_table = of_match_ptr(ti_sci_of_match),
> +		   .pm = &ti_sci_pm_ops,
>   	},
>   };
>   module_platform_driver(ti_sci_driver);
Dhruva Gole Aug. 3, 2023, 3:55 p.m. UTC | #2
On Aug 03, 2023 at 10:26:32 -0500, Andrew Davis wrote:
> On 8/3/23 1:42 AM, Dhruva Gole wrote:
> > Introduce system suspend resume calls that will allow the ti_sci
> > driver to support deep sleep low power mode when the user space issues a
> > suspend to mem.
> > 
> > Also, write a ti_sci_prepare_system_suspend call to be used in the driver
> > suspend handler to allow the system to identify the low power mode being
> > entered and if necessary, send TISCI_MSG_PREPARE_SLEEP with information
> > about the mode is being entered and the address for allocated memory for
> > storing the context during Deep Sleep.
> > 
> > We're using "pm_suspend_target_state" to map the kernel's target suspend
> > state to SysFW low power mode. Make sure this is available only when
> > CONFIG_SUSPEND is enabled.
> > 
> > Co-developed-by: Dave Gerlach <d-gerlach@ti.com>
> > Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> > Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
> > Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
> > Signed-off-by: Dhruva Gole <d-gole@ti.com>
> > ---
> >   drivers/firmware/ti_sci.c | 63 +++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 63 insertions(+)
> > 
[..snip..]
> > +static int ti_sci_suspend(struct device *dev)
> > +{
> > +	struct ti_sci_info *info = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_ENABLE);
> 
> After this the will the IOs be in isolation? Or does the firmware wait
> until power down begins later?

From what I understand,
IOs will be in isolation immediately

> 
> Andrew
> 
> > +	if (ret)
> > +		return ret;
> > +	dev_dbg(dev, "%s: set I/O isolation: %d\n", __func__, ret);
> > +
> > +	ret = ti_sci_prepare_system_suspend(info);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ti_sci_resume(struct device *dev)
> > +{
> > +	struct ti_sci_info *info = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_DISABLE);
> > +	if (ret)
> > +		return ret;
> > +	dev_dbg(dev, "%s: disable I/O isolation: %d\n", __func__, ret);
> > +
> > +	return 0;
> > +}
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(ti_sci_pm_ops, ti_sci_suspend, ti_sci_resume);
> > +
> >   static int ti_sci_init_suspend(struct platform_device *pdev,
> >   			       struct ti_sci_info *info)
> >   {
> > @@ -3759,6 +3821,7 @@ static struct platform_driver ti_sci_driver = {
> >   	.driver = {
> >   		   .name = "ti-sci",
> >   		   .of_match_table = of_match_ptr(ti_sci_of_match),
> > +		   .pm = &ti_sci_pm_ops,
> >   	},
> >   };
> >   module_platform_driver(ti_sci_driver);
Andrew Davis Aug. 3, 2023, 4 p.m. UTC | #3
On 8/3/23 10:55 AM, Dhruva Gole wrote:
> On Aug 03, 2023 at 10:26:32 -0500, Andrew Davis wrote:
>> On 8/3/23 1:42 AM, Dhruva Gole wrote:
>>> Introduce system suspend resume calls that will allow the ti_sci
>>> driver to support deep sleep low power mode when the user space issues a
>>> suspend to mem.
>>>
>>> Also, write a ti_sci_prepare_system_suspend call to be used in the driver
>>> suspend handler to allow the system to identify the low power mode being
>>> entered and if necessary, send TISCI_MSG_PREPARE_SLEEP with information
>>> about the mode is being entered and the address for allocated memory for
>>> storing the context during Deep Sleep.
>>>
>>> We're using "pm_suspend_target_state" to map the kernel's target suspend
>>> state to SysFW low power mode. Make sure this is available only when
>>> CONFIG_SUSPEND is enabled.
>>>
>>> Co-developed-by: Dave Gerlach <d-gerlach@ti.com>
>>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
>>> Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
>>> Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
>>> Signed-off-by: Dhruva Gole <d-gole@ti.com>
>>> ---
>>>    drivers/firmware/ti_sci.c | 63 +++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 63 insertions(+)
>>>
> [..snip..]
>>> +static int ti_sci_suspend(struct device *dev)
>>> +{
>>> +	struct ti_sci_info *info = dev_get_drvdata(dev);
>>> +	int ret;
>>> +
>>> +	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_ENABLE);
>>
>> After this the will the IOs be in isolation? Or does the firmware wait
>> until power down begins later?
> 
>  From what I understand,
> IOs will be in isolation immediately
> 

That is what I understand too, so then any device that may need to do some
external communication for its suspend will not function, this must be the
last driver _suspend() the system calls, how do you enforce that?

Andrew

>>
>> Andrew
>>
>>> +	if (ret)
>>> +		return ret;
>>> +	dev_dbg(dev, "%s: set I/O isolation: %d\n", __func__, ret);
>>> +
>>> +	ret = ti_sci_prepare_system_suspend(info);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int ti_sci_resume(struct device *dev)
>>> +{
>>> +	struct ti_sci_info *info = dev_get_drvdata(dev);
>>> +	int ret;
>>> +
>>> +	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_DISABLE);
>>> +	if (ret)
>>> +		return ret;
>>> +	dev_dbg(dev, "%s: disable I/O isolation: %d\n", __func__, ret);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static DEFINE_SIMPLE_DEV_PM_OPS(ti_sci_pm_ops, ti_sci_suspend, ti_sci_resume);
>>> +
>>>    static int ti_sci_init_suspend(struct platform_device *pdev,
>>>    			       struct ti_sci_info *info)
>>>    {
>>> @@ -3759,6 +3821,7 @@ static struct platform_driver ti_sci_driver = {
>>>    	.driver = {
>>>    		   .name = "ti-sci",
>>>    		   .of_match_table = of_match_ptr(ti_sci_of_match),
>>> +		   .pm = &ti_sci_pm_ops,
>>>    	},
>>>    };
>>>    module_platform_driver(ti_sci_driver);
>
Dhruva Gole Aug. 3, 2023, 4:08 p.m. UTC | #4
On Aug 03, 2023 at 11:00:11 -0500, Andrew Davis wrote:
> On 8/3/23 10:55 AM, Dhruva Gole wrote:
> > On Aug 03, 2023 at 10:26:32 -0500, Andrew Davis wrote:
> > > On 8/3/23 1:42 AM, Dhruva Gole wrote:
> > > > Introduce system suspend resume calls that will allow the ti_sci
> > > > driver to support deep sleep low power mode when the user space issues a
> > > > suspend to mem.
> > > > 
> > > > Also, write a ti_sci_prepare_system_suspend call to be used in the driver
> > > > suspend handler to allow the system to identify the low power mode being
> > > > entered and if necessary, send TISCI_MSG_PREPARE_SLEEP with information
> > > > about the mode is being entered and the address for allocated memory for
> > > > storing the context during Deep Sleep.
> > > > 
> > > > We're using "pm_suspend_target_state" to map the kernel's target suspend
> > > > state to SysFW low power mode. Make sure this is available only when
> > > > CONFIG_SUSPEND is enabled.
> > > > 
> > > > Co-developed-by: Dave Gerlach <d-gerlach@ti.com>
> > > > Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> > > > Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
> > > > Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
> > > > Signed-off-by: Dhruva Gole <d-gole@ti.com>
> > > > ---
> > > >    drivers/firmware/ti_sci.c | 63 +++++++++++++++++++++++++++++++++++++++
> > > >    1 file changed, 63 insertions(+)
> > > > 
> > [..snip..]
> > > > +static int ti_sci_suspend(struct device *dev)
> > > > +{
> > > > +	struct ti_sci_info *info = dev_get_drvdata(dev);
> > > > +	int ret;
> > > > +
> > > > +	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_ENABLE);
> > > 
> > > After this the will the IOs be in isolation? Or does the firmware wait
> > > until power down begins later?
> > 
> >  From what I understand,
> > IOs will be in isolation immediately
> > 
> 
> That is what I understand too, so then any device that may need to do some
> external communication for its suspend will not function, this must be the
> last driver _suspend() the system calls, how do you enforce that?

I will make use of .suspend_noirq callbacks in that case. Does that
sound better, or is there anything else I may not be aware of?

> 
> Andrew
> 
> > > 
> > > Andrew
> > > 
[..snip..]
> >
Kevin Hilman Aug. 7, 2023, 9:57 p.m. UTC | #5
Dhruva Gole <d-gole@ti.com> writes:

> On Aug 03, 2023 at 11:00:11 -0500, Andrew Davis wrote:
>> On 8/3/23 10:55 AM, Dhruva Gole wrote:
>> > On Aug 03, 2023 at 10:26:32 -0500, Andrew Davis wrote:
>> > > On 8/3/23 1:42 AM, Dhruva Gole wrote:
>> > > > Introduce system suspend resume calls that will allow the ti_sci
>> > > > driver to support deep sleep low power mode when the user space issues a
>> > > > suspend to mem.
>> > > > 
>> > > > Also, write a ti_sci_prepare_system_suspend call to be used in the driver
>> > > > suspend handler to allow the system to identify the low power mode being
>> > > > entered and if necessary, send TISCI_MSG_PREPARE_SLEEP with information
>> > > > about the mode is being entered and the address for allocated memory for
>> > > > storing the context during Deep Sleep.
>> > > > 
>> > > > We're using "pm_suspend_target_state" to map the kernel's target suspend
>> > > > state to SysFW low power mode. Make sure this is available only when
>> > > > CONFIG_SUSPEND is enabled.
>> > > > 
>> > > > Co-developed-by: Dave Gerlach <d-gerlach@ti.com>
>> > > > Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
>> > > > Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
>> > > > Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
>> > > > Signed-off-by: Dhruva Gole <d-gole@ti.com>
>> > > > ---
>> > > >    drivers/firmware/ti_sci.c | 63 +++++++++++++++++++++++++++++++++++++++
>> > > >    1 file changed, 63 insertions(+)
>> > > > 
>> > [..snip..]
>> > > > +static int ti_sci_suspend(struct device *dev)
>> > > > +{
>> > > > +	struct ti_sci_info *info = dev_get_drvdata(dev);
>> > > > +	int ret;
>> > > > +
>> > > > +	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_ENABLE);
>> > > 
>> > > After this the will the IOs be in isolation? Or does the firmware wait
>> > > until power down begins later?
>> > 
>> >  From what I understand,
>> > IOs will be in isolation immediately
>> > 
>> 
>> That is what I understand too, so then any device that may need to do some
>> external communication for its suspend will not function, this must be the
>> last driver _suspend() the system calls, how do you enforce that?
>
> I will make use of .suspend_noirq callbacks in that case. Does that
> sound better, or is there anything else I may not be aware of?

Using _noirq just moves the problem.  What if other drivers are also
using _noirq callbacks and run after the SCI driver?  You still cannot
guarantee ordering.

It seems to me that the IO isolation stuff is a system-wide operation,
and should probably be handled at the platform suspend_ops level
(e.g. suspend_ops->prepare_late()).   This would ensure that it runs
*after* all the driver hooks (even driver _noirq hooks.) and right
before the full suspend (or s2idle.)

Now, all that being said, I noticed that in v7, you didn't move this to
_noirq, but instead suggested that this be handled by TF-A.  I suppose
that's an option also, but my suggestion above should work also.

Kevin
Dhruva Gole Aug. 8, 2023, 11:54 a.m. UTC | #6
Kevin,

Thanks for the suggestion. I have a rough proposal inline, please can
you take a look? I will test those changes and respin this series
accordingly

On Aug 07, 2023 at 14:57:05 -0700, Kevin Hilman wrote:
> Dhruva Gole <d-gole@ti.com> writes:
> 
> > On Aug 03, 2023 at 11:00:11 -0500, Andrew Davis wrote:
> >> On 8/3/23 10:55 AM, Dhruva Gole wrote:
> >> > On Aug 03, 2023 at 10:26:32 -0500, Andrew Davis wrote:
> >> > > On 8/3/23 1:42 AM, Dhruva Gole wrote:
> >> > > > Introduce system suspend resume calls that will allow the ti_sci
> >> > > > driver to support deep sleep low power mode when the user space issues a
> >> > > > suspend to mem.
> >> > > > 
> >> > > > Also, write a ti_sci_prepare_system_suspend call to be used in the driver
> >> > > > suspend handler to allow the system to identify the low power mode being
> >> > > > entered and if necessary, send TISCI_MSG_PREPARE_SLEEP with information
> >> > > > about the mode is being entered and the address for allocated memory for
> >> > > > storing the context during Deep Sleep.
> >> > > > 
> >> > > > We're using "pm_suspend_target_state" to map the kernel's target suspend
> >> > > > state to SysFW low power mode. Make sure this is available only when
> >> > > > CONFIG_SUSPEND is enabled.
> >> > > > 
> >> > > > Co-developed-by: Dave Gerlach <d-gerlach@ti.com>
> >> > > > Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> >> > > > Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
> >> > > > Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
> >> > > > Signed-off-by: Dhruva Gole <d-gole@ti.com>
> >> > > > ---
> >> > > >    drivers/firmware/ti_sci.c | 63 +++++++++++++++++++++++++++++++++++++++
> >> > > >    1 file changed, 63 insertions(+)
> >> > > > 
> >> > [..snip..]
> >> > > > +static int ti_sci_suspend(struct device *dev)
> >> > > > +{
> >> > > > +	struct ti_sci_info *info = dev_get_drvdata(dev);
> >> > > > +	int ret;
> >> > > > +
> >> > > > +	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_ENABLE);
> >> > > 
> >> > > After this the will the IOs be in isolation? Or does the firmware wait
> >> > > until power down begins later?
> >> > 
> >> >  From what I understand,
> >> > IOs will be in isolation immediately
> >> > 
> >> 
> >> That is what I understand too, so then any device that may need to do some
> >> external communication for its suspend will not function, this must be the
> >> last driver _suspend() the system calls, how do you enforce that?
> >
> > I will make use of .suspend_noirq callbacks in that case. Does that
> > sound better, or is there anything else I may not be aware of?
> 
> Using _noirq just moves the problem.  What if other drivers are also
> using _noirq callbacks and run after the SCI driver?  You still cannot

True, this thought occurred to me as well which is why I was thinking
that moving it to ATF might be a better choice.

> guarantee ordering.
> 
> It seems to me that the IO isolation stuff is a system-wide operation,
> and should probably be handled at the platform suspend_ops level
> (e.g. suspend_ops->prepare_late()).   This would ensure that it runs

I must have missed this approach! Are you suggesting something like what
was done for am335?

static const struct platform_suspend_ops am33xx_pm_ops

have a similar code for tisci..?

static const struct platform_suspend_ops tisci_pm_ops = {
	.prepare_late = tisci_set_io_isolation
	};

And then while resuming we may want the pinctrl driver to scan for the
wk_evt bit[0] before the isolation is disabled, so we want the
tisci_resume/ remove isolation to be called later than that.

So I a wondering if the code below makes sense?

static const struct platform_suspend_ops tisci_pm_ops = {
	.prepare_late = tisci_suspend // also includes set isolation
	.end = tisci_resume 	// Disables isolation
	};

However a minor drawback here maybe that the serial logs on the resume
path may not appear when using a serial console for example. However
they should be able to easily access using dmesg.

> *after* all the driver hooks (even driver _noirq hooks.) and right
> before the full suspend (or s2idle.)
> 
> Now, all that being said, I noticed that in v7, you didn't move this to
> _noirq, but instead suggested that this be handled by TF-A.  I suppose
> that's an option also, but my suggestion above should work also.

Thanks for the pointer! I do believe it will make more sense to do it
from linux itself unless we have no way to do it in linux.

> 
> Kevin


[0] Table 5-517. Description Of The Pad Configuration Register Bits
https://www.ti.com/lit/pdf/spruid7

NOTE: The hardware works in such a way that as soon as the IO isolation
is disabled the wake_evt information is lost so the pinctrl-single
driver won't be able to know what pin woke it up if we disable io
isolation before it has the chance to look at the padconf registers
Kevin Hilman Aug. 9, 2023, 12:20 a.m. UTC | #7
Hi Dhruva,

Dhruva Gole <d-gole@ti.com> writes:

> Kevin,
>
> Thanks for the suggestion. I have a rough proposal inline, please can
> you take a look? I will test those changes and respin this series
> accordingly
>
> On Aug 07, 2023 at 14:57:05 -0700, Kevin Hilman wrote:
>> Dhruva Gole <d-gole@ti.com> writes:
>> 
>> > On Aug 03, 2023 at 11:00:11 -0500, Andrew Davis wrote:
>> >> On 8/3/23 10:55 AM, Dhruva Gole wrote:
>> >> > On Aug 03, 2023 at 10:26:32 -0500, Andrew Davis wrote:
>> >> > > On 8/3/23 1:42 AM, Dhruva Gole wrote:
>> >> > > > Introduce system suspend resume calls that will allow the ti_sci
>> >> > > > driver to support deep sleep low power mode when the user space issues a
>> >> > > > suspend to mem.
>> >> > > > 
>> >> > > > Also, write a ti_sci_prepare_system_suspend call to be used in the driver
>> >> > > > suspend handler to allow the system to identify the low power mode being
>> >> > > > entered and if necessary, send TISCI_MSG_PREPARE_SLEEP with information
>> >> > > > about the mode is being entered and the address for allocated memory for
>> >> > > > storing the context during Deep Sleep.
>> >> > > > 
>> >> > > > We're using "pm_suspend_target_state" to map the kernel's target suspend
>> >> > > > state to SysFW low power mode. Make sure this is available only when
>> >> > > > CONFIG_SUSPEND is enabled.
>> >> > > > 
>> >> > > > Co-developed-by: Dave Gerlach <d-gerlach@ti.com>
>> >> > > > Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
>> >> > > > Signed-off-by: Vibhore Vardhan <vibhore@ti.com>
>> >> > > > Signed-off-by: Georgi Vlaev <g-vlaev@ti.com>
>> >> > > > Signed-off-by: Dhruva Gole <d-gole@ti.com>
>> >> > > > ---
>> >> > > >    drivers/firmware/ti_sci.c | 63 +++++++++++++++++++++++++++++++++++++++
>> >> > > >    1 file changed, 63 insertions(+)
>> >> > > > 
>> >> > [..snip..]
>> >> > > > +static int ti_sci_suspend(struct device *dev)
>> >> > > > +{
>> >> > > > +	struct ti_sci_info *info = dev_get_drvdata(dev);
>> >> > > > +	int ret;
>> >> > > > +
>> >> > > > +	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_ENABLE);
>> >> > > 
>> >> > > After this the will the IOs be in isolation? Or does the firmware wait
>> >> > > until power down begins later?
>> >> > 
>> >> >  From what I understand,
>> >> > IOs will be in isolation immediately
>> >> > 
>> >> 
>> >> That is what I understand too, so then any device that may need to do some
>> >> external communication for its suspend will not function, this must be the
>> >> last driver _suspend() the system calls, how do you enforce that?
>> >
>> > I will make use of .suspend_noirq callbacks in that case. Does that
>> > sound better, or is there anything else I may not be aware of?
>> 
>> Using _noirq just moves the problem.  What if other drivers are also
>> using _noirq callbacks and run after the SCI driver?  You still cannot
>
> True, this thought occurred to me as well which is why I was thinking
> that moving it to ATF might be a better choice.
>
>> guarantee ordering.
>> 
>> It seems to me that the IO isolation stuff is a system-wide operation,
>> and should probably be handled at the platform suspend_ops level
>> (e.g. suspend_ops->prepare_late()).   This would ensure that it runs
>
> I must have missed this approach! Are you suggesting something like what
> was done for am335?
>
> static const struct platform_suspend_ops am33xx_pm_ops
>
> have a similar code for tisci..?
>
> static const struct platform_suspend_ops tisci_pm_ops = {
> 	.prepare_late = tisci_set_io_isolation
> 	};

Yes, with the minor nit that I would make a tisci_prepare_late()
function, which then calls _set_io_isolation(), since there may be some
other things we want to do in the "late" prepare for other LPM modes.

Also, for the additional LPM modes (more than DeepSleep), we're looking
at using suspend-to-idle (s2idle) to manage those.  So in addition to
platform_suspend_ops->prepare_late(), you should add this function to
s2idle_ops->prepare_late() also.

> And then while resuming we may want the pinctrl driver to scan for the
> wk_evt bit[0] before the isolation is disabled, so we want the
> tisci_resume/ remove isolation to be called later than that.
>
> So I a wondering if the code below makes sense?
>
> static const struct platform_suspend_ops tisci_pm_ops = {
> 	.prepare_late = tisci_suspend // also includes set isolation
> 	.end = tisci_resume 	// Disables isolation
> 	};
>
> However a minor drawback here maybe that the serial logs on the resume
> path may not appear when using a serial console for example. However
> they should be able to easily access using dmesg.

I'm not sure I fully understand this usecase, but using ->end() seems
drastic.  If IO isolation is disabled that long, won't that cause
problems for driver's ->resume callbacks that want to touch hardware?

To me, it sounds like you might want to use ->resume_early() or maybe
->resume_noirq() in the pinctrl driver for this so that IO isolation can
be disabled sooner?

>> *after* all the driver hooks (even driver _noirq hooks.) and right
>> before the full suspend (or s2idle.)
>> 
>> Now, all that being said, I noticed that in v7, you didn't move this to
>> _noirq, but instead suggested that this be handled by TF-A.  I suppose
>> that's an option also, but my suggestion above should work also.
>
> Thanks for the pointer! I do believe it will make more sense to do it
> from linux itself unless we have no way to do it in linux.
>
>> 
>> Kevin
>
>
> [0] Table 5-517. Description Of The Pad Configuration Register Bits
> https://www.ti.com/lit/pdf/spruid7
>
> NOTE: The hardware works in such a way that as soon as the IO isolation
> is disabled the wake_evt information is lost so the pinctrl-single
> driver won't be able to know what pin woke it up if we disable io
> isolation before it has the chance to look at the padconf registers

Ah, OK.  So yeah, as I hinted at above, what about using
->resume_noirq() in the pinctrl driver to get the wake_evt information,
and then use the s2idle_ops->restore_early() to disable IO isolation,
since this happens after all the driver's noirq hooks have run.

Kevin
Tony Lindgren Aug. 9, 2023, 7:23 a.m. UTC | #8
* Kevin Hilman <khilman@kernel.org> [230809 00:20]:
> To me, it sounds like you might want to use ->resume_early() or maybe
> ->resume_noirq() in the pinctrl driver for this so that IO isolation can
> be disabled sooner?

For calls that need to happen just before the SoC is disabled or first
thing on resume path, cpu_cluster_pm_enter() and cpu_cluster_pm_exit()
notifiers work nice and allow distributing the code across the related
SoC specific code and device drivers. See for example the usage in
drivers/irqchip/irq-gic.c for CPU_CLUSTER_PM_ENTER.

Regards,

Tony
Kevin Hilman Aug. 9, 2023, 5:37 p.m. UTC | #9
Tony Lindgren <tony@atomide.com> writes:

> * Kevin Hilman <khilman@kernel.org> [230809 00:20]:
>> To me, it sounds like you might want to use ->resume_early() or maybe
>> ->resume_noirq() in the pinctrl driver for this so that IO isolation can
>> be disabled sooner?
>
> For calls that need to happen just before the SoC is disabled or first
> thing on resume path, cpu_cluster_pm_enter() and cpu_cluster_pm_exit()
> notifiers work nice and allow distributing the code across the related
> SoC specific code and device drivers. See for example the usage in
> drivers/irqchip/irq-gic.c for CPU_CLUSTER_PM_ENTER.

Indeed, this is an option too, but for things that already have "full"
drivers (e.g. not an irqchip), they already have a full range of PM
callbacks, and adding another set of callbacks/notifiers for cpu_pm_* is
a bit clunky IMO.

That being said, for things like this IO isolation stuff that is
system-wide, and needs to happen very late in suspend (and/or very early
in suspend), cpu_pm_ is worth considering if the same cannot be done
with the normal PM callbacks.

Kevin
diff mbox series

Patch

diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
index 0334ade19868..172b726e00a6 100644
--- a/drivers/firmware/ti_sci.c
+++ b/drivers/firmware/ti_sci.c
@@ -22,6 +22,7 @@ 
 #include <linux/slab.h>
 #include <linux/soc/ti/ti-msgmgr.h>
 #include <linux/soc/ti/ti_sci_protocol.h>
+#include <linux/suspend.h>
 #include <linux/reboot.h>
 
 #include "ti_sci.h"
@@ -3521,6 +3522,67 @@  static int tisci_reboot_handler(struct notifier_block *nb, unsigned long mode,
 	return NOTIFY_BAD;
 }
 
+static int ti_sci_prepare_system_suspend(struct ti_sci_info *info)
+{
+#if IS_ENABLED(CONFIG_SUSPEND)
+	u8 mode;
+
+	/* Map and validate the target Linux suspend state to TISCI LPM. */
+	switch (pm_suspend_target_state) {
+	case PM_SUSPEND_MEM:
+		/* S2MEM is not supported by the firmware. */
+		if (!(info->fw_caps & MSG_FLAG_CAPS_LPM_DEEP_SLEEP))
+			return 0;
+		mode = TISCI_MSG_VALUE_SLEEP_MODE_DEEP_SLEEP;
+		break;
+	default:
+		/*
+		 * Do not fail if we don't have action to take for a
+		 * specific suspend mode.
+		 */
+		return 0;
+	}
+
+	return ti_sci_cmd_prepare_sleep(&info->handle, mode,
+					(u32)(info->ctx_mem_addr & 0xffffffff),
+					(u32)((u64)info->ctx_mem_addr >> 32), 0);
+#else
+	return 0;
+#endif
+}
+
+static int ti_sci_suspend(struct device *dev)
+{
+	struct ti_sci_info *info = dev_get_drvdata(dev);
+	int ret;
+
+	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_ENABLE);
+	if (ret)
+		return ret;
+	dev_dbg(dev, "%s: set I/O isolation: %d\n", __func__, ret);
+
+	ret = ti_sci_prepare_system_suspend(info);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ti_sci_resume(struct device *dev)
+{
+	struct ti_sci_info *info = dev_get_drvdata(dev);
+	int ret;
+
+	ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_DISABLE);
+	if (ret)
+		return ret;
+	dev_dbg(dev, "%s: disable I/O isolation: %d\n", __func__, ret);
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(ti_sci_pm_ops, ti_sci_suspend, ti_sci_resume);
+
 static int ti_sci_init_suspend(struct platform_device *pdev,
 			       struct ti_sci_info *info)
 {
@@ -3759,6 +3821,7 @@  static struct platform_driver ti_sci_driver = {
 	.driver = {
 		   .name = "ti-sci",
 		   .of_match_table = of_match_ptr(ti_sci_of_match),
+		   .pm = &ti_sci_pm_ops,
 	},
 };
 module_platform_driver(ti_sci_driver);