thermal/drivers/tsens: Add suspend to RAM support for tsens

  Add suspend callback support for tsens which disables tsens interrupts
in suspend to RAM callback.
Add resume callback support for tsens which reinitializes tsens hardware
and enables back tsens interrupts in resume callback.

Signed-off-by: Priyansh Jain <quic_priyjain@quicinc.com>
---
 drivers/thermal/qcom/tsens-v2.c |  2 +
 drivers/thermal/qcom/tsens.c    | 93 +++++++++++++++++++++++++++++++--
 drivers/thermal/qcom/tsens.h    |  7 +++
 3 files changed, 98 insertions(+), 4 deletions(-)
  

On Mon, 22 Jan 2024 at 12:11, Priyansh Jain <quic_priyjain@quicinc.com> wrote:
>
> Add suspend callback support for tsens which disables tsens interrupts
> in suspend to RAM callback.
> Add resume callback support for tsens which reinitializes tsens hardware
> and enables back tsens interrupts in resume callback.

This describes what the patch does. This is more or less obvious from
the patch itself. Instead it should describe why this is necessary.

>
> Signed-off-by: Priyansh Jain <quic_priyjain@quicinc.com>
> ---
>  drivers/thermal/qcom/tsens-v2.c |  2 +
>  drivers/thermal/qcom/tsens.c    | 93 +++++++++++++++++++++++++++++++--
>  drivers/thermal/qcom/tsens.h    |  7 +++
>  3 files changed, 98 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index 29a61d2d6ca3..1b74db6299c4 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -107,6 +107,8 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
>  static const struct tsens_ops ops_generic_v2 = {
>         .init           = init_common,
>         .get_temp       = get_temp_tsens_valid,
> +       .suspend        = tsens_suspend_common,
> +       .resume         = tsens_resume_common,
>  };
>
>  struct tsens_plat_data data_tsens_v2 = {
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 6d7c16ccb44d..603ccb91009d 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -17,6 +17,7 @@
>  #include <linux/pm.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
> +#include <linux/suspend.h>
>  #include <linux/thermal.h>
>  #include "../thermal_hwmon.h"
>  #include "tsens.h"
> @@ -1153,7 +1154,7 @@ static const struct thermal_zone_device_ops tsens_of_ops = {
>  };
>
>  static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
> -                             irq_handler_t thread_fn)
> +                             irq_handler_t thread_fn, int *irq_num)
>  {
>         struct platform_device *pdev;
>         int ret, irq;
> @@ -1169,6 +1170,7 @@ static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
>                 if (irq == -ENXIO)
>                         ret = 0;
>         } else {
> +               *irq_num = irq;
>                 /* VER_0 interrupt is TRIGGER_RISING, VER_0_1 and up is ONESHOT */
>                 if (tsens_version(priv) == VER_0)
>                         ret = devm_request_threaded_irq(&pdev->dev, irq,
> @@ -1193,6 +1195,85 @@ static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
>         return ret;
>  }
>
> +static int tsens_reinit(struct tsens_priv *priv)
> +{
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&priv->ul_lock, flags);
> +
> +       /* in VER_0 TSENS need to be explicitly enabled */
> +       if (tsens_version(priv) == VER_0)
> +               regmap_field_write(priv->rf[TSENS_EN], 1);
> +
> +       /*
> +        * Re-enable the watchdog, unmask the bark.
> +        * Disable cycle completion monitoring
> +        */
> +       if (priv->feat->has_watchdog) {
> +               regmap_field_write(priv->rf[WDOG_BARK_MASK], 0);
> +               regmap_field_write(priv->rf[CC_MON_MASK], 1);
> +       }
> +
> +       /* Re-enable interrupts */
> +       if (tsens_version(priv) >= VER_0_1)
> +               tsens_enable_irq(priv);
> +
> +       spin_unlock_irqrestore(&priv->ul_lock, flags);
> +
> +       return 0;
> +}
> +
> +int tsens_suspend_common(struct tsens_priv *priv)
> +{
> +       switch (pm_suspend_target_state) {
> +       case PM_SUSPEND_MEM:
> +               if (priv->combo_irq > 0) {
> +                       disable_irq_nosync(priv->combo_irq);
> +                       disable_irq_wake(priv->combo_irq);
> +               }
> +
> +               if (priv->uplow_irq > 0) {
> +                       disable_irq_nosync(priv->uplow_irq);
> +                       disable_irq_wake(priv->uplow_irq);
> +               }
> +
> +               if (priv->crit_irq > 0) {
> +                       disable_irq_nosync(priv->crit_irq);
> +                       disable_irq_wake(priv->crit_irq);
> +               }
> +               break;
> +       default:
> +               break;
> +       }
> +       return 0;
> +}
> +
> +int tsens_resume_common(struct tsens_priv *priv)
> +{
> +       switch (pm_suspend_target_state) {
> +       case PM_SUSPEND_MEM:
> +               tsens_reinit(priv);
> +               if (priv->combo_irq > 0) {
> +                       enable_irq(priv->combo_irq);
> +                       enable_irq_wake(priv->combo_irq);
> +               }
> +
> +               if (priv->uplow_irq > 0) {
> +                       enable_irq(priv->uplow_irq);
> +                       enable_irq_wake(priv->uplow_irq);
> +               }
> +
> +               if (priv->crit_irq > 0) {
> +                       enable_irq(priv->crit_irq);
> +                       enable_irq_wake(priv->crit_irq);
> +               }
> +               break;
> +       default:
> +               break;
> +       }
> +       return 0;
> +}
> +
>  static int tsens_register(struct tsens_priv *priv)
>  {
>         int i, ret;
> @@ -1227,15 +1308,19 @@ static int tsens_register(struct tsens_priv *priv)
>
>         if (priv->feat->combo_int) {
>                 ret = tsens_register_irq(priv, "combined",
> -                                        tsens_combined_irq_thread);
> +                                        tsens_combined_irq_thread,
> +                                        &priv->combo_irq);
>         } else {
> -               ret = tsens_register_irq(priv, "uplow", tsens_irq_thread);
> +               ret = tsens_register_irq(priv, "uplow",
> +                                        tsens_irq_thread,
> +                                        &priv->uplow_irq);
>                 if (ret < 0)
>                         return ret;
>
>                 if (priv->feat->crit_int)
>                         ret = tsens_register_irq(priv, "critical",
> -                                                tsens_critical_irq_thread);
> +                                                tsens_critical_irq_thread,
> +                                                &priv->crit_irq);
>         }
>
>         return ret;
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index cb637fa289ca..268bf56105be 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -582,6 +582,11 @@ struct tsens_priv {
>         const struct reg_field          *fields;
>         const struct tsens_ops          *ops;
>
> +       /* For saving irq number to re-use later */
> +       int                             uplow_irq;
> +       int                             crit_irq;
> +       int                             combo_irq;
> +
>         struct dentry                   *debug_root;
>         struct dentry                   *debug;
>
> @@ -634,6 +639,8 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *pt1, u32 *pt2, u32 mo
>  int init_common(struct tsens_priv *priv);
>  int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp);
>  int get_temp_common(const struct tsens_sensor *s, int *temp);
> +int tsens_suspend_common(struct tsens_priv *priv);
> +int tsens_resume_common(struct tsens_priv *priv);
>
>  /* TSENS target */
>  extern struct tsens_plat_data data_8960;
> --
> 2.17.1
>
>
  

On 22.01.2024 11:07, Priyansh Jain wrote:
> Add suspend callback support for tsens which disables tsens interrupts
> in suspend to RAM callback.

Would it not be preferrable to have the "critical overheat", wakeup-
capable interrupts be enabled, even if the system is suspended?

> Add resume callback support for tsens which reinitializes tsens hardware
> and enables back tsens interrupts in resume callback.
> 
> Signed-off-by: Priyansh Jain <quic_priyjain@quicinc.com>
> ---

[...]


> +
> +int tsens_suspend_common(struct tsens_priv *priv)
> +{
> +	switch (pm_suspend_target_state) {
> +	case PM_SUSPEND_MEM:
> +		if (priv->combo_irq > 0) {
> +			disable_irq_nosync(priv->combo_irq);
> +			disable_irq_wake(priv->combo_irq);
> +		}
> +
> +		if (priv->uplow_irq > 0) {
> +			disable_irq_nosync(priv->uplow_irq);
> +			disable_irq_wake(priv->uplow_irq);
> +		}
> +
> +		if (priv->crit_irq > 0) {
> +			disable_irq_nosync(priv->crit_irq);
> +			disable_irq_wake(priv->crit_irq);
> +		}
> +		break;
> +	default:
> +		break;
> +	}

if (pm_suspend_target_state != PM_SUSPEND_MEM)
	return 0;

<rest of the code>

[...]

>  
> +	/* For saving irq number to re-use later */

This is rather self-explanatory

Konrad
  

On 1/22/2024 7:24 PM, Dmitry Baryshkov wrote:
> On Mon, 22 Jan 2024 at 12:11, Priyansh Jain <quic_priyjain@quicinc.com> wrote:
>>
>> Add suspend callback support for tsens which disables tsens interrupts
>> in suspend to RAM callback.
>> Add resume callback support for tsens which reinitializes tsens hardware
>> and enables back tsens interrupts in resume callback.
> 
> This describes what the patch does. This is more or less obvious from
> the patch itself. Instead it should describe why this is necessary.
> 
sure will update in next version.
Regards,
Priyansh
>>
>> Signed-off-by: Priyansh Jain <quic_priyjain@quicinc.com>
>> ---
>>   drivers/thermal/qcom/tsens-v2.c |  2 +
>>   drivers/thermal/qcom/tsens.c    | 93 +++++++++++++++++++++++++++++++--
>>   drivers/thermal/qcom/tsens.h    |  7 +++
>>   3 files changed, 98 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
>> index 29a61d2d6ca3..1b74db6299c4 100644
>> --- a/drivers/thermal/qcom/tsens-v2.c
>> +++ b/drivers/thermal/qcom/tsens-v2.c
>> @@ -107,6 +107,8 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
>>   static const struct tsens_ops ops_generic_v2 = {
>>          .init           = init_common,
>>          .get_temp       = get_temp_tsens_valid,
>> +       .suspend        = tsens_suspend_common,
>> +       .resume         = tsens_resume_common,
>>   };
>>
>>   struct tsens_plat_data data_tsens_v2 = {
>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>> index 6d7c16ccb44d..603ccb91009d 100644
>> --- a/drivers/thermal/qcom/tsens.c
>> +++ b/drivers/thermal/qcom/tsens.c
>> @@ -17,6 +17,7 @@
>>   #include <linux/pm.h>
>>   #include <linux/regmap.h>
>>   #include <linux/slab.h>
>> +#include <linux/suspend.h>
>>   #include <linux/thermal.h>
>>   #include "../thermal_hwmon.h"
>>   #include "tsens.h"
>> @@ -1153,7 +1154,7 @@ static const struct thermal_zone_device_ops tsens_of_ops = {
>>   };
>>
>>   static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
>> -                             irq_handler_t thread_fn)
>> +                             irq_handler_t thread_fn, int *irq_num)
>>   {
>>          struct platform_device *pdev;
>>          int ret, irq;
>> @@ -1169,6 +1170,7 @@ static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
>>                  if (irq == -ENXIO)
>>                          ret = 0;
>>          } else {
>> +               *irq_num = irq;
>>                  /* VER_0 interrupt is TRIGGER_RISING, VER_0_1 and up is ONESHOT */
>>                  if (tsens_version(priv) == VER_0)
>>                          ret = devm_request_threaded_irq(&pdev->dev, irq,
>> @@ -1193,6 +1195,85 @@ static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
>>          return ret;
>>   }
>>
>> +static int tsens_reinit(struct tsens_priv *priv)
>> +{
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&priv->ul_lock, flags);
>> +
>> +       /* in VER_0 TSENS need to be explicitly enabled */
>> +       if (tsens_version(priv) == VER_0)
>> +               regmap_field_write(priv->rf[TSENS_EN], 1);
>> +
>> +       /*
>> +        * Re-enable the watchdog, unmask the bark.
>> +        * Disable cycle completion monitoring
>> +        */
>> +       if (priv->feat->has_watchdog) {
>> +               regmap_field_write(priv->rf[WDOG_BARK_MASK], 0);
>> +               regmap_field_write(priv->rf[CC_MON_MASK], 1);
>> +       }
>> +
>> +       /* Re-enable interrupts */
>> +       if (tsens_version(priv) >= VER_0_1)
>> +               tsens_enable_irq(priv);
>> +
>> +       spin_unlock_irqrestore(&priv->ul_lock, flags);
>> +
>> +       return 0;
>> +}
>> +
>> +int tsens_suspend_common(struct tsens_priv *priv)
>> +{
>> +       switch (pm_suspend_target_state) {
>> +       case PM_SUSPEND_MEM:
>> +               if (priv->combo_irq > 0) {
>> +                       disable_irq_nosync(priv->combo_irq);
>> +                       disable_irq_wake(priv->combo_irq);
>> +               }
>> +
>> +               if (priv->uplow_irq > 0) {
>> +                       disable_irq_nosync(priv->uplow_irq);
>> +                       disable_irq_wake(priv->uplow_irq);
>> +               }
>> +
>> +               if (priv->crit_irq > 0) {
>> +                       disable_irq_nosync(priv->crit_irq);
>> +                       disable_irq_wake(priv->crit_irq);
>> +               }
>> +               break;
>> +       default:
>> +               break;
>> +       }
>> +       return 0;
>> +}
>> +
>> +int tsens_resume_common(struct tsens_priv *priv)
>> +{
>> +       switch (pm_suspend_target_state) {
>> +       case PM_SUSPEND_MEM:
>> +               tsens_reinit(priv);
>> +               if (priv->combo_irq > 0) {
>> +                       enable_irq(priv->combo_irq);
>> +                       enable_irq_wake(priv->combo_irq);
>> +               }
>> +
>> +               if (priv->uplow_irq > 0) {
>> +                       enable_irq(priv->uplow_irq);
>> +                       enable_irq_wake(priv->uplow_irq);
>> +               }
>> +
>> +               if (priv->crit_irq > 0) {
>> +                       enable_irq(priv->crit_irq);
>> +                       enable_irq_wake(priv->crit_irq);
>> +               }
>> +               break;
>> +       default:
>> +               break;
>> +       }
>> +       return 0;
>> +}
>> +
>>   static int tsens_register(struct tsens_priv *priv)
>>   {
>>          int i, ret;
>> @@ -1227,15 +1308,19 @@ static int tsens_register(struct tsens_priv *priv)
>>
>>          if (priv->feat->combo_int) {
>>                  ret = tsens_register_irq(priv, "combined",
>> -                                        tsens_combined_irq_thread);
>> +                                        tsens_combined_irq_thread,
>> +                                        &priv->combo_irq);
>>          } else {
>> -               ret = tsens_register_irq(priv, "uplow", tsens_irq_thread);
>> +               ret = tsens_register_irq(priv, "uplow",
>> +                                        tsens_irq_thread,
>> +                                        &priv->uplow_irq);
>>                  if (ret < 0)
>>                          return ret;
>>
>>                  if (priv->feat->crit_int)
>>                          ret = tsens_register_irq(priv, "critical",
>> -                                                tsens_critical_irq_thread);
>> +                                                tsens_critical_irq_thread,
>> +                                                &priv->crit_irq);
>>          }
>>
>>          return ret;
>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>> index cb637fa289ca..268bf56105be 100644
>> --- a/drivers/thermal/qcom/tsens.h
>> +++ b/drivers/thermal/qcom/tsens.h
>> @@ -582,6 +582,11 @@ struct tsens_priv {
>>          const struct reg_field          *fields;
>>          const struct tsens_ops          *ops;
>>
>> +       /* For saving irq number to re-use later */
>> +       int                             uplow_irq;
>> +       int                             crit_irq;
>> +       int                             combo_irq;
>> +
>>          struct dentry                   *debug_root;
>>          struct dentry                   *debug;
>>
>> @@ -634,6 +639,8 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *pt1, u32 *pt2, u32 mo
>>   int init_common(struct tsens_priv *priv);
>>   int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp);
>>   int get_temp_common(const struct tsens_sensor *s, int *temp);
>> +int tsens_suspend_common(struct tsens_priv *priv);
>> +int tsens_resume_common(struct tsens_priv *priv);
>>
>>   /* TSENS target */
>>   extern struct tsens_plat_data data_8960;
>> --
>> 2.17.1
>>
>>
> 
>
  

On 1/22/2024 8:02 PM, Konrad Dybcio wrote:
> On 22.01.2024 11:07, Priyansh Jain wrote:
>> Add suspend callback support for tsens which disables tsens interrupts
>> in suspend to RAM callback.
> 
> Would it not be preferrable to have the "critical overheat", wakeup-
> capable interrupts be enabled, even if the system is suspended?
> 


As part of suspend to RAM, tsens hardware will be turned off and it 
cannot generate any interrupt.Also system doesn't want to abort suspend 
to RAM due to tsens interrupts since system is already going into lowest
power state. Hence disabling tsens interrupt during suspend to RAM callback.

Regards,
Priyansh

>> Add resume callback support for tsens which reinitializes tsens hardware
>> and enables back tsens interrupts in resume callback.
>>
>> Signed-off-by: Priyansh Jain <quic_priyjain@quicinc.com>
>> ---
> 
> [...]
> 
> 
>> +
>> +int tsens_suspend_common(struct tsens_priv *priv)
>> +{
>> +	switch (pm_suspend_target_state) {
>> +	case PM_SUSPEND_MEM:
>> +		if (priv->combo_irq > 0) {
>> +			disable_irq_nosync(priv->combo_irq);
>> +			disable_irq_wake(priv->combo_irq);
>> +		}
>> +
>> +		if (priv->uplow_irq > 0) {
>> +			disable_irq_nosync(priv->uplow_irq);
>> +			disable_irq_wake(priv->uplow_irq);
>> +		}
>> +
>> +		if (priv->crit_irq > 0) {
>> +			disable_irq_nosync(priv->crit_irq);
>> +			disable_irq_wake(priv->crit_irq);
>> +		}
>> +		break;
>> +	default:
>> +		break;
>> +	}
> 
> if (pm_suspend_target_state != PM_SUSPEND_MEM)
> 	return 0;
> 
> <rest of the code>
> 
> [...]
> 
>>   
>> +	/* For saving irq number to re-use later */
> 
> This is rather self-explanatory
> 
> Konrad
  

On 1/24/24 11:42, Priyansh Jain wrote:
> 
> 
> On 1/22/2024 8:02 PM, Konrad Dybcio wrote:
>> On 22.01.2024 11:07, Priyansh Jain wrote:
>>> Add suspend callback support for tsens which disables tsens interrupts
>>> in suspend to RAM callback.
>>
>> Would it not be preferrable to have the "critical overheat", wakeup-
>> capable interrupts be enabled, even if the system is suspended?
>>
> 
> 
> As part of suspend to RAM, tsens hardware will be turned off and it cannot generate any interrupt.Also system doesn't want to abort suspend to RAM due to tsens interrupts since system is already going into lowest
> power state. Hence disabling tsens interrupt during suspend to RAM callback.

Is that a hardware limitation, or a software design choice? I'm not
sure I want my phone to have thermal notifications disabled when
it's suspended.

Konrad
  

On 1/24/2024 6:04 PM, Konrad Dybcio wrote:
> 
> 
> On 1/24/24 11:42, Priyansh Jain wrote:
>>
>>
>> On 1/22/2024 8:02 PM, Konrad Dybcio wrote:
>>> On 22.01.2024 11:07, Priyansh Jain wrote:
>>>> Add suspend callback support for tsens which disables tsens interrupts
>>>> in suspend to RAM callback.
>>>
>>> Would it not be preferrable to have the "critical overheat", wakeup-
>>> capable interrupts be enabled, even if the system is suspended?
>>>
>>
>>
>> As part of suspend to RAM, tsens hardware will be turned off and it 
>> cannot generate any interrupt.Also system doesn't want to abort 
>> suspend to RAM due to tsens interrupts since system is already going 
>> into lowest
>> power state. Hence disabling tsens interrupt during suspend to RAM 
>> callback.
> 
> Is that a hardware limitation, or a software design choice? I'm not
> sure I want my phone to have thermal notifications disabled when
> it's suspended.

> Konrad

As part of suspend to RAM , entire SOC will be off, this mode (suspend 
to RAM) is not intended for Mobile product. Tsens interrupts are not
disabled as part of suspend to idle(suspend mode for mobile).

Regards,
Priyansh
  

On 1/24/24 16:25, Priyansh Jain wrote:
> 
> 
> On 1/24/2024 6:04 PM, Konrad Dybcio wrote:
>>
>>
>> On 1/24/24 11:42, Priyansh Jain wrote:
>>>
>>>
>>> On 1/22/2024 8:02 PM, Konrad Dybcio wrote:
>>>> On 22.01.2024 11:07, Priyansh Jain wrote:
>>>>> Add suspend callback support for tsens which disables tsens interrupts
>>>>> in suspend to RAM callback.
>>>>
>>>> Would it not be preferrable to have the "critical overheat", wakeup-
>>>> capable interrupts be enabled, even if the system is suspended?
>>>>
>>>
>>>
>>> As part of suspend to RAM, tsens hardware will be turned off and it cannot generate any interrupt.Also system doesn't want to abort suspend to RAM due to tsens interrupts since system is already going into lowest
>>> power state. Hence disabling tsens interrupt during suspend to RAM callback.
>>
>> Is that a hardware limitation, or a software design choice? I'm not
>> sure I want my phone to have thermal notifications disabled when
>> it's suspended.
> 
>> Konrad
> 
> As part of suspend to RAM , entire SOC will be off,

What do you mean by "entire SOC[sic] will be off"? Surely the memory
controller must be on to keep refreshing the memory? Are you thinking
of suspend-to-disk (hibernation), by chance?

> this mode (suspend to RAM) is not intended for Mobile product. Tsens interrupts are not
> disabled as part of suspend to idle(suspend mode for mobile).

That's clearly untrue, e.g. the PSCI firmware on SM8550 implements
PSCI_SYSTEM_SUSPEND, which does S2R.

Konrad
  

+Maulik and Raghavendra

Hi Konrad,

On 1/25/2024 4:38 PM, Konrad Dybcio wrote:
>
>
> On 1/24/24 16:25, Priyansh Jain wrote:
>>
>>
>> On 1/24/2024 6:04 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 1/24/24 11:42, Priyansh Jain wrote:
>>>>
>>>>
>>>> On 1/22/2024 8:02 PM, Konrad Dybcio wrote:
>>>>> On 22.01.2024 11:07, Priyansh Jain wrote:
>>>>>> Add suspend callback support for tsens which disables tsens 
>>>>>> interrupts
>>>>>> in suspend to RAM callback.
>>>>>
>>>>> Would it not be preferrable to have the "critical overheat", wakeup-
>>>>> capable interrupts be enabled, even if the system is suspended?
>>>>>
>>>>
>>>>
>>>> As part of suspend to RAM, tsens hardware will be turned off and it 
>>>> cannot generate any interrupt.Also system doesn't want to abort 
>>>> suspend to RAM due to tsens interrupts since system is already 
>>>> going into lowest
>>>> power state. Hence disabling tsens interrupt during suspend to RAM 
>>>> callback.
>>>
>>> Is that a hardware limitation, or a software design choice? I'm not
>>> sure I want my phone to have thermal notifications disabled when
>>> it's suspended.
>>
>>> Konrad
>>
>> As part of suspend to RAM , entire SOC will be off,
>
> What do you mean by "entire SOC[sic] will be off"? Surely the memory
> controller must be on to keep refreshing the memory? Are you thinking
> of suspend-to-disk (hibernation), by chance?

Yes, Memory will be in self refreshing  mode(Retained). But SOC will be off

and will do cold boot to come out of S2R.

>
>> this mode (suspend to RAM) is not intended for Mobile product. Tsens 
>> interrupts are not
>> disabled as part of suspend to idle(suspend mode for mobile).
>
> That's clearly untrue, e.g. the PSCI firmware on SM8550 implements
> PSCI_SYSTEM_SUSPEND, which does S2R.

IIUC, PSCI_SYSTEM_SUSPEND will be enabled only for S2R supported 
products and will be removed it for others.

Maulik/Raghavendra can comment more

>
> Konrad
  

On Wed, Jan 24, 2024 at 8:55 PM Priyansh Jain <quic_priyjain@quicinc.com> wrote:
>
>
>
> On 1/24/2024 6:04 PM, Konrad Dybcio wrote:
> >
> >
> > On 1/24/24 11:42, Priyansh Jain wrote:

> >>
> >> As part of suspend to RAM, tsens hardware will be turned off and it
> >> cannot generate any interrupt.Also system doesn't want to abort
> >> suspend to RAM due to tsens interrupts since system is already going
> >> into lowest
> >> power state. Hence disabling tsens interrupt during suspend to RAM
> >> callback.
> >
> > Is that a hardware limitation, or a software design choice? I'm not
> > sure I want my phone to have thermal notifications disabled when
> > it's suspended.
>
> > Konrad
>
> As part of suspend to RAM , entire SOC will be off, this mode (suspend
> to RAM) is not intended for Mobile product. Tsens interrupts are not
> disabled as part of suspend to idle(suspend mode for mobile).

You should hide the callbacks behind the CONFIG_SUSPEND Kconfig option
so that it only gets enabled when S2R is enabled.
  

On 1/27/2024 9:07 PM, Manaf Meethalavalappu Pallikunhi wrote:

> +Maulik and Raghavendra
> 
> Hi Konrad,
> 
> On 1/25/2024 4:38 PM, Konrad Dybcio wrote:
>>
>>
>> On 1/24/24 16:25, Priyansh Jain wrote:
>>>
>>>
>>> On 1/24/2024 6:04 PM, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 1/24/24 11:42, Priyansh Jain wrote:
>>>>>
>>>>>
>>>>> On 1/22/2024 8:02 PM, Konrad Dybcio wrote:
>>>>>> On 22.01.2024 11:07, Priyansh Jain wrote:
>>>>>>> Add suspend callback support for tsens which disables tsens 
>>>>>>> interrupts
>>>>>>> in suspend to RAM callback.
>>>>>>
>>>>>> Would it not be preferrable to have the "critical overheat", wakeup-
>>>>>> capable interrupts be enabled, even if the system is suspended?
>>>>>>
>>>>>
>>>>>
>>>>> As part of suspend to RAM, tsens hardware will be turned off and it 
>>>>> cannot generate any interrupt.Also system doesn't want to abort 
>>>>> suspend to RAM due to tsens interrupts since system is already 
>>>>> going into lowest
>>>>> power state. Hence disabling tsens interrupt during suspend to RAM 
>>>>> callback.
>>>>
>>>> Is that a hardware limitation, or a software design choice? I'm not
>>>> sure I want my phone to have thermal notifications disabled when
>>>> it's suspended.
>>>
>>>> Konrad
>>>
>>> As part of suspend to RAM , entire SOC will be off,
>>
>> What do you mean by "entire SOC[sic] will be off"? Surely the memory
>> controller must be on to keep refreshing the memory? Are you thinking
>> of suspend-to-disk (hibernation), by chance?
> 
> Yes, Memory will be in self refreshing  mode(Retained). But SOC will be off
> 
> and will do cold boot to come out of S2R.
> 
>>
>>> this mode (suspend to RAM) is not intended for Mobile product. Tsens 
>>> interrupts are not
>>> disabled as part of suspend to idle(suspend mode for mobile).
>>
>> That's clearly untrue, e.g. the PSCI firmware on SM8550 implements
>> PSCI_SYSTEM_SUSPEND, which does S2R.
> 
> IIUC, PSCI_SYSTEM_SUSPEND will be enabled only for S2R supported 
> products and will be removed it for others.
> 
> Maulik/Raghavendra can comment more
> 
Sorry for delayed response, we have discussed internally on this and
came to the conclusion that disabling tsens interrupt in S2R path is not 
correct approach as S2R is being exercised on mobile kind of products as 
well. I will update the required changes in the next patch.

Priyansh
>>
>> Konrad
  

On 1/28/2024 1:41 AM, Amit Kucheria wrote:
> On Wed, Jan 24, 2024 at 8:55 PM Priyansh Jain <quic_priyjain@quicinc.com> wrote:
>>
>>
>>
>> On 1/24/2024 6:04 PM, Konrad Dybcio wrote:
>>>
>>>
>>> On 1/24/24 11:42, Priyansh Jain wrote:
> 
>>>>
>>>> As part of suspend to RAM, tsens hardware will be turned off and it
>>>> cannot generate any interrupt.Also system doesn't want to abort
>>>> suspend to RAM due to tsens interrupts since system is already going
>>>> into lowest
>>>> power state. Hence disabling tsens interrupt during suspend to RAM
>>>> callback.
>>>
>>> Is that a hardware limitation, or a software design choice? I'm not
>>> sure I want my phone to have thermal notifications disabled when
>>> it's suspended.
>>
>>> Konrad
>>
>> As part of suspend to RAM , entire SOC will be off, this mode (suspend
>> to RAM) is not intended for Mobile product. Tsens interrupts are not
>> disabled as part of suspend to idle(suspend mode for mobile).
> 
> You should hide the callbacks behind the CONFIG_SUSPEND Kconfig option
> so that it only gets enabled when S2R is enabled.

Yes i will take care of this in next version.

Priyansh