[PATCH v1] thermal: imx: Make trip point offset configurable

[PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

Currently the imx thermal driver has a hardcoded critical temperature
value offset of 5 Celsius degrees from the actual SoC maximum
temperature.

This affects applications and systems designed to be working on this close
to the limit, but yet valid, temperature range.

Given that there is no single value that will fit all the use cases make
the critical trip point offset from the max temperature configurable
using a newly added trip_offset module parameter, passive trip point is
set to 5 Celsius degrees less than the critical. By default the
system behaves exactly as before.

Link: https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 drivers/thermal/imx_thermal.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 16663373b682..42d1f8a3eccb 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -86,6 +86,10 @@ enum imx_thermal_trip {
 #define TEMPMON_IMX6SX			2
 #define TEMPMON_IMX7D			3
 
+static int trip_offset = 5;
+module_param(trip_offset, int, 0444);
+MODULE_PARM_DESC(trip_offset, "Critical trip point offset from CPU max temp in Celsius degrees (default 5)");
+
 struct thermal_soc_data {
 	u32 version;
 
@@ -504,11 +508,11 @@ static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)
 	}
 
 	/*
-	 * Set the critical trip point at 5 °C under max
-	 * Set the passive trip point at 10 °C under max (changeable via sysfs)
+	 * Set the critical trip point at 5 °C under max (changeable via module param)
+	 * Set the passive trip point at 5 °C under critical (changeable via sysfs)
 	 */
-	data->temp_critical = data->temp_max - (1000 * 5);
-	data->temp_passive = data->temp_max - (1000 * 10);
+	data->temp_critical = data->temp_max - (1000 * trip_offset);
+	data->temp_passive = data->temp_critical - (1000 * 5);
 }
 
 static int imx_init_from_tempmon_data(struct platform_device *pdev)
-- 
2.25.1

[PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

Currently the imx thermal driver has a hardcoded critical temperature
value offset of 5 Celsius degrees from the actual SoC maximum
temperature.

This affects applications and systems designed to be working on this close
to the limit, but yet valid, temperature range.

Given that there is no single value that will fit all the use cases make
the critical trip point offset from the max temperature configurable
using a newly added trip_offset module parameter, passive trip point is
set to 5 Celsius degrees less than the critical. By default the
system behaves exactly as before.

Link: https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 drivers/thermal/imx_thermal.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 16663373b682..42d1f8a3eccb 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -86,6 +86,10 @@ enum imx_thermal_trip {
 #define TEMPMON_IMX6SX			2
 #define TEMPMON_IMX7D			3
 
+static int trip_offset = 5;
+module_param(trip_offset, int, 0444);
+MODULE_PARM_DESC(trip_offset, "Critical trip point offset from CPU max temp in Celsius degrees (default 5)");
+
 struct thermal_soc_data {
 	u32 version;
 
@@ -504,11 +508,11 @@ static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)
 	}
 
 	/*
-	 * Set the critical trip point at 5 °C under max
-	 * Set the passive trip point at 10 °C under max (changeable via sysfs)
+	 * Set the critical trip point at 5 °C under max (changeable via module param)
+	 * Set the passive trip point at 5 °C under critical (changeable via sysfs)
 	 */
-	data->temp_critical = data->temp_max - (1000 * 5);
-	data->temp_passive = data->temp_max - (1000 * 10);
+	data->temp_critical = data->temp_max - (1000 * trip_offset);
+	data->temp_passive = data->temp_critical - (1000 * 5);
 }
 
 static int imx_init_from_tempmon_data(struct platform_device *pdev)
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Ahmad Fatoum]

On 16.05.22 21:00, Francesco Dolcini wrote:
> Currently the imx thermal driver has a hardcoded critical temperature
> value offset of 5 Celsius degrees from the actual SoC maximum
> temperature.
> 
> This affects applications and systems designed to be working on this close
> to the limit, but yet valid, temperature range.
> 
> Given that there is no single value that will fit all the use cases make
> the critical trip point offset from the max temperature configurable
> using a newly added trip_offset module parameter, passive trip point is
> set to 5 Celsius degrees less than the critical. By default the
> system behaves exactly as before.
> 
> Link: https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>  drivers/thermal/imx_thermal.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 16663373b682..42d1f8a3eccb 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -86,6 +86,10 @@ enum imx_thermal_trip {
>  #define TEMPMON_IMX6SX			2
>  #define TEMPMON_IMX7D			3
>  
> +static int trip_offset = 5;
> +module_param(trip_offset, int, 0444);

Is this being r--r--r-- intended?

> +MODULE_PARM_DESC(trip_offset, "Critical trip point offset from CPU max temp in Celsius degrees (default 5)");
> +
>  struct thermal_soc_data {
>  	u32 version;
>  
> @@ -504,11 +508,11 @@ static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)
>  	}
>  
>  	/*
> -	 * Set the critical trip point at 5 °C under max
> -	 * Set the passive trip point at 10 °C under max (changeable via sysfs)
> +	 * Set the critical trip point at 5 °C under max (changeable via module param)
> +	 * Set the passive trip point at 5 °C under critical (changeable via sysfs)
>  	 */
> -	data->temp_critical = data->temp_max - (1000 * 5);
> -	data->temp_passive = data->temp_max - (1000 * 10);
> +	data->temp_critical = data->temp_max - (1000 * trip_offset);
> +	data->temp_passive = data->temp_critical - (1000 * 5);
>  }
>  
>  static int imx_init_from_tempmon_data(struct platform_device *pdev)


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Ahmad Fatoum]

On 16.05.22 21:00, Francesco Dolcini wrote:
> Currently the imx thermal driver has a hardcoded critical temperature
> value offset of 5 Celsius degrees from the actual SoC maximum
> temperature.
> 
> This affects applications and systems designed to be working on this close
> to the limit, but yet valid, temperature range.
> 
> Given that there is no single value that will fit all the use cases make
> the critical trip point offset from the max temperature configurable
> using a newly added trip_offset module parameter, passive trip point is
> set to 5 Celsius degrees less than the critical. By default the
> system behaves exactly as before.
> 
> Link: https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>  drivers/thermal/imx_thermal.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 16663373b682..42d1f8a3eccb 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -86,6 +86,10 @@ enum imx_thermal_trip {
>  #define TEMPMON_IMX6SX			2
>  #define TEMPMON_IMX7D			3
>  
> +static int trip_offset = 5;
> +module_param(trip_offset, int, 0444);

Is this being r--r--r-- intended?

> +MODULE_PARM_DESC(trip_offset, "Critical trip point offset from CPU max temp in Celsius degrees (default 5)");
> +
>  struct thermal_soc_data {
>  	u32 version;
>  
> @@ -504,11 +508,11 @@ static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)
>  	}
>  
>  	/*
> -	 * Set the critical trip point at 5 °C under max
> -	 * Set the passive trip point at 10 °C under max (changeable via sysfs)
> +	 * Set the critical trip point at 5 °C under max (changeable via module param)
> +	 * Set the passive trip point at 5 °C under critical (changeable via sysfs)
>  	 */
> -	data->temp_critical = data->temp_max - (1000 * 5);
> -	data->temp_passive = data->temp_max - (1000 * 10);
> +	data->temp_critical = data->temp_max - (1000 * trip_offset);
> +	data->temp_passive = data->temp_critical - (1000 * 5);
>  }
>  
>  static int imx_init_from_tempmon_data(struct platform_device *pdev)


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

On Mon, May 16, 2022 at 09:06:10PM +0200, Ahmad Fatoum wrote:
> On 16.05.22 21:00, Francesco Dolcini wrote:
> > Currently the imx thermal driver has a hardcoded critical temperature
> > value offset of 5 Celsius degrees from the actual SoC maximum
> > temperature.
> > 
> > This affects applications and systems designed to be working on this close
> > to the limit, but yet valid, temperature range.
> > 
> > Given that there is no single value that will fit all the use cases make
> > the critical trip point offset from the max temperature configurable
> > using a newly added trip_offset module parameter, passive trip point is
> > set to 5 Celsius degrees less than the critical. By default the
> > system behaves exactly as before.
> > 
> > Link: https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> >  drivers/thermal/imx_thermal.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> > index 16663373b682..42d1f8a3eccb 100644
> > --- a/drivers/thermal/imx_thermal.c
> > +++ b/drivers/thermal/imx_thermal.c
> > @@ -86,6 +86,10 @@ enum imx_thermal_trip {
> >  #define TEMPMON_IMX6SX			2
> >  #define TEMPMON_IMX7D			3
> >  
> > +static int trip_offset = 5;
> > +module_param(trip_offset, int, 0444);
> 
> Is this being r--r--r-- intended?

Yes, would you expect something more or less strict?

Daniel was pretty assertive that this is supposed to be a system
property and not something that should be possible to mess around with
[0]. Given this and that trip points are a+r in sysfs this was the most
reasonable solution to me. Said that I do not mind changing
this to 640 or 0440 or ...

[0] https://lore.kernel.org/all/4de41b5e-1fa6-ece4-9d9a-2656d399b452@linaro.org/

Francesco

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

On Mon, May 16, 2022 at 09:06:10PM +0200, Ahmad Fatoum wrote:
> On 16.05.22 21:00, Francesco Dolcini wrote:
> > Currently the imx thermal driver has a hardcoded critical temperature
> > value offset of 5 Celsius degrees from the actual SoC maximum
> > temperature.
> > 
> > This affects applications and systems designed to be working on this close
> > to the limit, but yet valid, temperature range.
> > 
> > Given that there is no single value that will fit all the use cases make
> > the critical trip point offset from the max temperature configurable
> > using a newly added trip_offset module parameter, passive trip point is
> > set to 5 Celsius degrees less than the critical. By default the
> > system behaves exactly as before.
> > 
> > Link: https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> >  drivers/thermal/imx_thermal.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> > index 16663373b682..42d1f8a3eccb 100644
> > --- a/drivers/thermal/imx_thermal.c
> > +++ b/drivers/thermal/imx_thermal.c
> > @@ -86,6 +86,10 @@ enum imx_thermal_trip {
> >  #define TEMPMON_IMX6SX			2
> >  #define TEMPMON_IMX7D			3
> >  
> > +static int trip_offset = 5;
> > +module_param(trip_offset, int, 0444);
> 
> Is this being r--r--r-- intended?

Yes, would you expect something more or less strict?

Daniel was pretty assertive that this is supposed to be a system
property and not something that should be possible to mess around with
[0]. Given this and that trip points are a+r in sysfs this was the most
reasonable solution to me. Said that I do not mind changing
this to 640 or 0440 or ...

[0] https://lore.kernel.org/all/4de41b5e-1fa6-ece4-9d9a-2656d399b452@linaro.org/

Francesco


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Ahmad Fatoum]

Hello Francesco,

On 16.05.22 21:16, Francesco Dolcini wrote:
> On Mon, May 16, 2022 at 09:06:10PM +0200, Ahmad Fatoum wrote:
>> On 16.05.22 21:00, Francesco Dolcini wrote:
>>> Currently the imx thermal driver has a hardcoded critical temperature
>>> value offset of 5 Celsius degrees from the actual SoC maximum
>>> temperature.
>>>
>>> This affects applications and systems designed to be working on this close
>>> to the limit, but yet valid, temperature range.
>>>
>>> Given that there is no single value that will fit all the use cases make
>>> the critical trip point offset from the max temperature configurable
>>> using a newly added trip_offset module parameter, passive trip point is
>>> set to 5 Celsius degrees less than the critical. By default the
>>> system behaves exactly as before.
>>>
>>> Link: https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>>> ---
>>>  drivers/thermal/imx_thermal.c | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
>>> index 16663373b682..42d1f8a3eccb 100644
>>> --- a/drivers/thermal/imx_thermal.c
>>> +++ b/drivers/thermal/imx_thermal.c
>>> @@ -86,6 +86,10 @@ enum imx_thermal_trip {
>>>  #define TEMPMON_IMX6SX			2
>>>  #define TEMPMON_IMX7D			3
>>>  
>>> +static int trip_offset = 5;
>>> +module_param(trip_offset, int, 0444);
>>
>> Is this being r--r--r-- intended?
> 
> Yes, would you expect something more or less strict?
> 
> Daniel was pretty assertive that this is supposed to be a system
> property and not something that should be possible to mess around with
> [0]. Given this and that trip points are a+r in sysfs this was the most
> reasonable solution to me. Said that I do not mind changing
> this to 640 or 0440 or ...

I just found it odd, but with your explanation, it makes sense to have
this a read-only-after-init setting.

Thanks for the clarification,
Ahmad

> 
> [0] https://lore.kernel.org/all/4de41b5e-1fa6-ece4-9d9a-2656d399b452@linaro.org/
> 
> Francesco
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Ahmad Fatoum]

Hello Francesco,

On 16.05.22 21:16, Francesco Dolcini wrote:
> On Mon, May 16, 2022 at 09:06:10PM +0200, Ahmad Fatoum wrote:
>> On 16.05.22 21:00, Francesco Dolcini wrote:
>>> Currently the imx thermal driver has a hardcoded critical temperature
>>> value offset of 5 Celsius degrees from the actual SoC maximum
>>> temperature.
>>>
>>> This affects applications and systems designed to be working on this close
>>> to the limit, but yet valid, temperature range.
>>>
>>> Given that there is no single value that will fit all the use cases make
>>> the critical trip point offset from the max temperature configurable
>>> using a newly added trip_offset module parameter, passive trip point is
>>> set to 5 Celsius degrees less than the critical. By default the
>>> system behaves exactly as before.
>>>
>>> Link: https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
>>> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>>> ---
>>>  drivers/thermal/imx_thermal.c | 12 ++++++++----
>>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
>>> index 16663373b682..42d1f8a3eccb 100644
>>> --- a/drivers/thermal/imx_thermal.c
>>> +++ b/drivers/thermal/imx_thermal.c
>>> @@ -86,6 +86,10 @@ enum imx_thermal_trip {
>>>  #define TEMPMON_IMX6SX			2
>>>  #define TEMPMON_IMX7D			3
>>>  
>>> +static int trip_offset = 5;
>>> +module_param(trip_offset, int, 0444);
>>
>> Is this being r--r--r-- intended?
> 
> Yes, would you expect something more or less strict?
> 
> Daniel was pretty assertive that this is supposed to be a system
> property and not something that should be possible to mess around with
> [0]. Given this and that trip points are a+r in sysfs this was the most
> reasonable solution to me. Said that I do not mind changing
> this to 640 or 0440 or ...

I just found it odd, but with your explanation, it makes sense to have
this a read-only-after-init setting.

Thanks for the clarification,
Ahmad

> 
> [0] https://lore.kernel.org/all/4de41b5e-1fa6-ece4-9d9a-2656d399b452@linaro.org/
> 
> Francesco
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

Hello all,

On Mon, May 16, 2022 at 09:00:01PM +0200, Francesco Dolcini wrote:
> Currently the imx thermal driver has a hardcoded critical temperature
> value offset of 5 Celsius degrees from the actual SoC maximum
> temperature.
> 
> This affects applications and systems designed to be working on this close
> to the limit, but yet valid, temperature range.
> 
> Given that there is no single value that will fit all the use cases make
> the critical trip point offset from the max temperature configurable
> using a newly added trip_offset module parameter, passive trip point is
> set to 5 Celsius degrees less than the critical. By default the
> system behaves exactly as before.

In your opinion this commit could be eligible as a fix, e.g.
Fixes: a2291badc355 ("imx: thermal: use CPU temperature grade info for thresholds")
?

From my application point of view this is a fix, but on the other hand
not sure if a premature emergency shutdown for a specific application
is considered a "bug".

Francesco

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

Hello all,

On Mon, May 16, 2022 at 09:00:01PM +0200, Francesco Dolcini wrote:
> Currently the imx thermal driver has a hardcoded critical temperature
> value offset of 5 Celsius degrees from the actual SoC maximum
> temperature.
> 
> This affects applications and systems designed to be working on this close
> to the limit, but yet valid, temperature range.
> 
> Given that there is no single value that will fit all the use cases make
> the critical trip point offset from the max temperature configurable
> using a newly added trip_offset module parameter, passive trip point is
> set to 5 Celsius degrees less than the critical. By default the
> system behaves exactly as before.

In your opinion this commit could be eligible as a fix, e.g.
Fixes: a2291badc355 ("imx: thermal: use CPU temperature grade info for thresholds")
?

From my application point of view this is a fix, but on the other hand
not sure if a premature emergency shutdown for a specific application
is considered a "bug".

Francesco


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Marco Felsch]

Hi Francesco,

On 22-05-16, Francesco Dolcini wrote:
> Currently the imx thermal driver has a hardcoded critical temperature
> value offset of 5 Celsius degrees from the actual SoC maximum
> temperature.
> 
> This affects applications and systems designed to be working on this close
> to the limit, but yet valid, temperature range.
> 
> Given that there is no single value that will fit all the use cases make
> the critical trip point offset from the max temperature configurable
> using a newly added trip_offset module parameter, passive trip point is
> set to 5 Celsius degrees less than the critical. By default the
> system behaves exactly as before.

I thought the conclusion of the discussion was to use a dt-property?
Since it is device and/or environment specific.

Regards,
  Marco

> 
> Link: https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>  drivers/thermal/imx_thermal.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 16663373b682..42d1f8a3eccb 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -86,6 +86,10 @@ enum imx_thermal_trip {
>  #define TEMPMON_IMX6SX			2
>  #define TEMPMON_IMX7D			3
>  
> +static int trip_offset = 5;
> +module_param(trip_offset, int, 0444);
> +MODULE_PARM_DESC(trip_offset, "Critical trip point offset from CPU max temp in Celsius degrees (default 5)");
> +
>  struct thermal_soc_data {
>  	u32 version;
>  
> @@ -504,11 +508,11 @@ static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)
>  	}
>  
>  	/*
> -	 * Set the critical trip point at 5 °C under max
> -	 * Set the passive trip point at 10 °C under max (changeable via sysfs)
> +	 * Set the critical trip point at 5 °C under max (changeable via module param)
> +	 * Set the passive trip point at 5 °C under critical (changeable via sysfs)
>  	 */
> -	data->temp_critical = data->temp_max - (1000 * 5);
> -	data->temp_passive = data->temp_max - (1000 * 10);
> +	data->temp_critical = data->temp_max - (1000 * trip_offset);
> +	data->temp_passive = data->temp_critical - (1000 * 5);
>  }
>  
>  static int imx_init_from_tempmon_data(struct platform_device *pdev)
> -- 
> 2.25.1
> 
> 
> 

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Marco Felsch]

Hi Francesco,

On 22-05-16, Francesco Dolcini wrote:
> Currently the imx thermal driver has a hardcoded critical temperature
> value offset of 5 Celsius degrees from the actual SoC maximum
> temperature.
> 
> This affects applications and systems designed to be working on this close
> to the limit, but yet valid, temperature range.
> 
> Given that there is no single value that will fit all the use cases make
> the critical trip point offset from the max temperature configurable
> using a newly added trip_offset module parameter, passive trip point is
> set to 5 Celsius degrees less than the critical. By default the
> system behaves exactly as before.

I thought the conclusion of the discussion was to use a dt-property?
Since it is device and/or environment specific.

Regards,
  Marco

> 
> Link: https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>  drivers/thermal/imx_thermal.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 16663373b682..42d1f8a3eccb 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -86,6 +86,10 @@ enum imx_thermal_trip {
>  #define TEMPMON_IMX6SX			2
>  #define TEMPMON_IMX7D			3
>  
> +static int trip_offset = 5;
> +module_param(trip_offset, int, 0444);
> +MODULE_PARM_DESC(trip_offset, "Critical trip point offset from CPU max temp in Celsius degrees (default 5)");
> +
>  struct thermal_soc_data {
>  	u32 version;
>  
> @@ -504,11 +508,11 @@ static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)
>  	}
>  
>  	/*
> -	 * Set the critical trip point at 5 °C under max
> -	 * Set the passive trip point at 10 °C under max (changeable via sysfs)
> +	 * Set the critical trip point at 5 °C under max (changeable via module param)
> +	 * Set the passive trip point at 5 °C under critical (changeable via sysfs)
>  	 */
> -	data->temp_critical = data->temp_max - (1000 * 5);
> -	data->temp_passive = data->temp_max - (1000 * 10);
> +	data->temp_critical = data->temp_max - (1000 * trip_offset);
> +	data->temp_passive = data->temp_critical - (1000 * 5);
>  }
>  
>  static int imx_init_from_tempmon_data(struct platform_device *pdev)
> -- 
> 2.25.1
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

Hello Marco,

On Wed, May 18, 2022 at 10:55:22AM +0200, Marco Felsch wrote:
> On 22-05-16, Francesco Dolcini wrote:
> > Currently the imx thermal driver has a hardcoded critical temperature
> > value offset of 5 Celsius degrees from the actual SoC maximum
> > temperature.
> > 
> > This affects applications and systems designed to be working on this close
> > to the limit, but yet valid, temperature range.
> > 
> > Given that there is no single value that will fit all the use cases make
> > the critical trip point offset from the max temperature configurable
> > using a newly added trip_offset module parameter, passive trip point is
> > set to 5 Celsius degrees less than the critical. By default the
> > system behaves exactly as before.
> 
> I thought the conclusion of the discussion was to use a dt-property?
> Since it is device and/or environment specific.

Daniel proposed to use a module parameter [0], but if you prefer a
dt-property I can change this. I would keep the same semantic (offset
from max temperature to compute the critical trip point).

[0] https://lore.kernel.org/all/7f5a919c-2fa1-a463-1f6b-6b531d5ee27d@linaro.org/

Francesco

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

Hello Marco,

On Wed, May 18, 2022 at 10:55:22AM +0200, Marco Felsch wrote:
> On 22-05-16, Francesco Dolcini wrote:
> > Currently the imx thermal driver has a hardcoded critical temperature
> > value offset of 5 Celsius degrees from the actual SoC maximum
> > temperature.
> > 
> > This affects applications and systems designed to be working on this close
> > to the limit, but yet valid, temperature range.
> > 
> > Given that there is no single value that will fit all the use cases make
> > the critical trip point offset from the max temperature configurable
> > using a newly added trip_offset module parameter, passive trip point is
> > set to 5 Celsius degrees less than the critical. By default the
> > system behaves exactly as before.
> 
> I thought the conclusion of the discussion was to use a dt-property?
> Since it is device and/or environment specific.

Daniel proposed to use a module parameter [0], but if you prefer a
dt-property I can change this. I would keep the same semantic (offset
from max temperature to compute the critical trip point).

[0] https://lore.kernel.org/all/7f5a919c-2fa1-a463-1f6b-6b531d5ee27d@linaro.org/

Francesco


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Daniel Lezcano]

On 18/05/2022 11:49, Francesco Dolcini wrote:
> Hello Marco,
> 
> On Wed, May 18, 2022 at 10:55:22AM +0200, Marco Felsch wrote:
>> On 22-05-16, Francesco Dolcini wrote:
>>> Currently the imx thermal driver has a hardcoded critical temperature
>>> value offset of 5 Celsius degrees from the actual SoC maximum
>>> temperature.
>>>
>>> This affects applications and systems designed to be working on this close
>>> to the limit, but yet valid, temperature range.
>>>
>>> Given that there is no single value that will fit all the use cases make
>>> the critical trip point offset from the max temperature configurable
>>> using a newly added trip_offset module parameter, passive trip point is
>>> set to 5 Celsius degrees less than the critical. By default the
>>> system behaves exactly as before.
>>
>> I thought the conclusion of the discussion was to use a dt-property?
>> Since it is device and/or environment specific.
> 
> Daniel proposed to use a module parameter [0], but if you prefer a
> dt-property I can change this. I would keep the same semantic (offset
> from max temperature to compute the critical trip point).
> 
> [0] https://lore.kernel.org/all/7f5a919c-2fa1-a463-1f6b-6b531d5ee27d@linaro.org/


It would have been easier if the imx6/7 platforms were defining the 
thermal zones in their DT ...


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Daniel Lezcano]

On 18/05/2022 11:49, Francesco Dolcini wrote:
> Hello Marco,
> 
> On Wed, May 18, 2022 at 10:55:22AM +0200, Marco Felsch wrote:
>> On 22-05-16, Francesco Dolcini wrote:
>>> Currently the imx thermal driver has a hardcoded critical temperature
>>> value offset of 5 Celsius degrees from the actual SoC maximum
>>> temperature.
>>>
>>> This affects applications and systems designed to be working on this close
>>> to the limit, but yet valid, temperature range.
>>>
>>> Given that there is no single value that will fit all the use cases make
>>> the critical trip point offset from the max temperature configurable
>>> using a newly added trip_offset module parameter, passive trip point is
>>> set to 5 Celsius degrees less than the critical. By default the
>>> system behaves exactly as before.
>>
>> I thought the conclusion of the discussion was to use a dt-property?
>> Since it is device and/or environment specific.
> 
> Daniel proposed to use a module parameter [0], but if you prefer a
> dt-property I can change this. I would keep the same semantic (offset
> from max temperature to compute the critical trip point).
> 
> [0] https://lore.kernel.org/all/7f5a919c-2fa1-a463-1f6b-6b531d5ee27d@linaro.org/


It would have been easier if the imx6/7 platforms were defining the 
thermal zones in their DT ...


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

On Wed, May 18, 2022 at 12:10:34PM +0200, Daniel Lezcano wrote:
> On 18/05/2022 11:49, Francesco Dolcini wrote:
> > On Wed, May 18, 2022 at 10:55:22AM +0200, Marco Felsch wrote:
> > > On 22-05-16, Francesco Dolcini wrote:
> > > > Currently the imx thermal driver has a hardcoded critical temperature
> > > > value offset of 5 Celsius degrees from the actual SoC maximum
> > > > temperature.
> > > > 
> > > > This affects applications and systems designed to be working on this close
> > > > to the limit, but yet valid, temperature range.
> > > > 
> > > > Given that there is no single value that will fit all the use cases make
> > > > the critical trip point offset from the max temperature configurable
> > > > using a newly added trip_offset module parameter, passive trip point is
> > > > set to 5 Celsius degrees less than the critical. By default the
> > > > system behaves exactly as before.
> > > 
> > > I thought the conclusion of the discussion was to use a dt-property?
> > > Since it is device and/or environment specific.
> > 
> > Daniel proposed to use a module parameter [0], but if you prefer a
> > dt-property I can change this. I would keep the same semantic (offset
> > from max temperature to compute the critical trip point).
> > 
> > [0] https://lore.kernel.org/all/7f5a919c-2fa1-a463-1f6b-6b531d5ee27d@linaro.org/
> 
> 
> It would have been easier if the imx6/7 platforms were defining the thermal
> zones in their DT ...

Marco, Lucas,
What about a &tempon { fsl,tempmon-critical-offset=<0>; } dt property?
From my point of view this is even a better solution.

Daniel, would something like that also acceptable from your point of
view?

Francesco

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

On Wed, May 18, 2022 at 12:10:34PM +0200, Daniel Lezcano wrote:
> On 18/05/2022 11:49, Francesco Dolcini wrote:
> > On Wed, May 18, 2022 at 10:55:22AM +0200, Marco Felsch wrote:
> > > On 22-05-16, Francesco Dolcini wrote:
> > > > Currently the imx thermal driver has a hardcoded critical temperature
> > > > value offset of 5 Celsius degrees from the actual SoC maximum
> > > > temperature.
> > > > 
> > > > This affects applications and systems designed to be working on this close
> > > > to the limit, but yet valid, temperature range.
> > > > 
> > > > Given that there is no single value that will fit all the use cases make
> > > > the critical trip point offset from the max temperature configurable
> > > > using a newly added trip_offset module parameter, passive trip point is
> > > > set to 5 Celsius degrees less than the critical. By default the
> > > > system behaves exactly as before.
> > > 
> > > I thought the conclusion of the discussion was to use a dt-property?
> > > Since it is device and/or environment specific.
> > 
> > Daniel proposed to use a module parameter [0], but if you prefer a
> > dt-property I can change this. I would keep the same semantic (offset
> > from max temperature to compute the critical trip point).
> > 
> > [0] https://lore.kernel.org/all/7f5a919c-2fa1-a463-1f6b-6b531d5ee27d@linaro.org/
> 
> 
> It would have been easier if the imx6/7 platforms were defining the thermal
> zones in their DT ...

Marco, Lucas,
What about a &tempon { fsl,tempmon-critical-offset=<0>; } dt property?
From my point of view this is even a better solution.

Daniel, would something like that also acceptable from your point of
view?

Francesco


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Marco Felsch]

Hi Francesco,

On 22-05-23, Francesco Dolcini wrote:
> On Wed, May 18, 2022 at 12:10:34PM +0200, Daniel Lezcano wrote:
> > On 18/05/2022 11:49, Francesco Dolcini wrote:
> > > On Wed, May 18, 2022 at 10:55:22AM +0200, Marco Felsch wrote:
> > > > On 22-05-16, Francesco Dolcini wrote:
> > > > > Currently the imx thermal driver has a hardcoded critical temperature
> > > > > value offset of 5 Celsius degrees from the actual SoC maximum
> > > > > temperature.
> > > > > 
> > > > > This affects applications and systems designed to be working on this close
> > > > > to the limit, but yet valid, temperature range.
> > > > > 
> > > > > Given that there is no single value that will fit all the use cases make
> > > > > the critical trip point offset from the max temperature configurable
> > > > > using a newly added trip_offset module parameter, passive trip point is
> > > > > set to 5 Celsius degrees less than the critical. By default the
> > > > > system behaves exactly as before.
> > > > 
> > > > I thought the conclusion of the discussion was to use a dt-property?
> > > > Since it is device and/or environment specific.
> > > 
> > > Daniel proposed to use a module parameter [0], but if you prefer a
> > > dt-property I can change this. I would keep the same semantic (offset
> > > from max temperature to compute the critical trip point).
> > > 
> > > [0] https://lore.kernel.org/all/7f5a919c-2fa1-a463-1f6b-6b531d5ee27d@linaro.org/
> > 
> > 
> > It would have been easier if the imx6/7 platforms were defining the thermal
> > zones in their DT ...

After checking the driver I completely agree with you... But this would
need much more rework.

> Marco, Lucas,
> What about a &tempon { fsl,tempmon-critical-offset=<0>; } dt property?

I would not introduce a special binding for it. Instead I would add the
support to parse the already exisiting bindings so the new-tempmon node
would look like:

tempmon: tempmon {
        compatible = "fsl,imx6q-tempmon";
        interrupt-parent = <&gpc>;
        interrupts = <0 49 IRQ_TYPE_LEVEL_HIGH>;
        fsl,tempmon = <&anatop>;
        nvmem-cells = <&tempmon_calib>, <&tempmon_temp_grade>;
        nvmem-cell-names = "calib", "temp_grade";
        clocks = <&clks IMX6QDL_CLK_PLL3_USB_OTG>;
        #thermal-sensor-cells = <0>;

	commercial-thermal {
		trips {
			commercial_trip_passive: trip-point0 {
				temperature = <85000>;
				type = "passive";
			};
			commercial_trip_crit: trip-point1 {
				temperature = <90000>;
				type = "critical";
			};
		};
	};

	extended-commercial-thermal {
		trips {
			ecommercial_trip_passive: trip-point0 {
				temperature = <95000>;
				type = "passive";
			};
			ecommercial_trip_crit: trip-point1 {
				temperature = <100000>;
				type = "critical";
			};
		};
	};

	industrial-thermal { };
	automotive-thermal { };
};

That way we can reuse the existing dt definitions and if no *-thermal
child device nodes exist we can keep the original logic.

You could than adapt the trip point in your device-tree by:

&ecommercial_trip_crit {
	temperature = <105000>;
};

Regards,
  Marco

> From my point of view this is even a better solution.
> 
> Daniel, would something like that also acceptable from your point of
> view?
> 
> Francesco
> 
> 

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Marco Felsch]

Hi Francesco,

On 22-05-23, Francesco Dolcini wrote:
> On Wed, May 18, 2022 at 12:10:34PM +0200, Daniel Lezcano wrote:
> > On 18/05/2022 11:49, Francesco Dolcini wrote:
> > > On Wed, May 18, 2022 at 10:55:22AM +0200, Marco Felsch wrote:
> > > > On 22-05-16, Francesco Dolcini wrote:
> > > > > Currently the imx thermal driver has a hardcoded critical temperature
> > > > > value offset of 5 Celsius degrees from the actual SoC maximum
> > > > > temperature.
> > > > > 
> > > > > This affects applications and systems designed to be working on this close
> > > > > to the limit, but yet valid, temperature range.
> > > > > 
> > > > > Given that there is no single value that will fit all the use cases make
> > > > > the critical trip point offset from the max temperature configurable
> > > > > using a newly added trip_offset module parameter, passive trip point is
> > > > > set to 5 Celsius degrees less than the critical. By default the
> > > > > system behaves exactly as before.
> > > > 
> > > > I thought the conclusion of the discussion was to use a dt-property?
> > > > Since it is device and/or environment specific.
> > > 
> > > Daniel proposed to use a module parameter [0], but if you prefer a
> > > dt-property I can change this. I would keep the same semantic (offset
> > > from max temperature to compute the critical trip point).
> > > 
> > > [0] https://lore.kernel.org/all/7f5a919c-2fa1-a463-1f6b-6b531d5ee27d@linaro.org/
> > 
> > 
> > It would have been easier if the imx6/7 platforms were defining the thermal
> > zones in their DT ...

After checking the driver I completely agree with you... But this would
need much more rework.

> Marco, Lucas,
> What about a &tempon { fsl,tempmon-critical-offset=<0>; } dt property?

I would not introduce a special binding for it. Instead I would add the
support to parse the already exisiting bindings so the new-tempmon node
would look like:

tempmon: tempmon {
        compatible = "fsl,imx6q-tempmon";
        interrupt-parent = <&gpc>;
        interrupts = <0 49 IRQ_TYPE_LEVEL_HIGH>;
        fsl,tempmon = <&anatop>;
        nvmem-cells = <&tempmon_calib>, <&tempmon_temp_grade>;
        nvmem-cell-names = "calib", "temp_grade";
        clocks = <&clks IMX6QDL_CLK_PLL3_USB_OTG>;
        #thermal-sensor-cells = <0>;

	commercial-thermal {
		trips {
			commercial_trip_passive: trip-point0 {
				temperature = <85000>;
				type = "passive";
			};
			commercial_trip_crit: trip-point1 {
				temperature = <90000>;
				type = "critical";
			};
		};
	};

	extended-commercial-thermal {
		trips {
			ecommercial_trip_passive: trip-point0 {
				temperature = <95000>;
				type = "passive";
			};
			ecommercial_trip_crit: trip-point1 {
				temperature = <100000>;
				type = "critical";
			};
		};
	};

	industrial-thermal { };
	automotive-thermal { };
};

That way we can reuse the existing dt definitions and if no *-thermal
child device nodes exist we can keep the original logic.

You could than adapt the trip point in your device-tree by:

&ecommercial_trip_crit {
	temperature = <105000>;
};

Regards,
  Marco

> From my point of view this is even a better solution.
> 
> Daniel, would something like that also acceptable from your point of
> view?
> 
> Francesco
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

On Tue, May 24, 2022 at 10:35:14AM +0200, Marco Felsch wrote:
> On 22-05-23, Francesco Dolcini wrote:
> > On Wed, May 18, 2022 at 12:10:34PM +0200, Daniel Lezcano wrote:
> > > It would have been easier if the imx6/7 platforms were defining the thermal
> > > zones in their DT ...
> 
> After checking the driver I completely agree with you... But this would
> need much more rework.
> 
> > Marco, Lucas,
> > What about a &tempon { fsl,tempmon-critical-offset=<0>; } dt property?
> 
> I would not introduce a special binding for it. Instead I would add the
> support to parse the already exisiting bindings so the new-tempmon node
> would look like:
> 
> tempmon: tempmon {
>         compatible = "fsl,imx6q-tempmon";
>         interrupt-parent = <&gpc>;
>         interrupts = <0 49 IRQ_TYPE_LEVEL_HIGH>;
>         fsl,tempmon = <&anatop>;
>         nvmem-cells = <&tempmon_calib>, <&tempmon_temp_grade>;
>         nvmem-cell-names = "calib", "temp_grade";
>         clocks = <&clks IMX6QDL_CLK_PLL3_USB_OTG>;
>         #thermal-sensor-cells = <0>;
> 
> 	commercial-thermal {
> 		trips {
> 			commercial_trip_passive: trip-point0 {
> 				temperature = <85000>;
> 				type = "passive";
> 			};
> 			commercial_trip_crit: trip-point1 {
> 				temperature = <90000>;
> 				type = "critical";
> 			};
> 		};
> 	};
> 
> 	extended-commercial-thermal {
> 		trips {
> 			ecommercial_trip_passive: trip-point0 {
> 				temperature = <95000>;
> 				type = "passive";
> 			};
> 			ecommercial_trip_crit: trip-point1 {
> 				temperature = <100000>;
> 				type = "critical";
> 			};
> 		};
> 	};
> 
> 	industrial-thermal { };
> 	automotive-thermal { };
> };
> 
> That way we can reuse the existing dt definitions and if no *-thermal
> child device nodes exist we can keep the original logic.

Good for me, it's more work, of course, but if this is the way to do it
let it be.
Daniel: I'd like to have an ack from you before implementing this.

Francesco

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Francesco Dolcini]

On Tue, May 24, 2022 at 10:35:14AM +0200, Marco Felsch wrote:
> On 22-05-23, Francesco Dolcini wrote:
> > On Wed, May 18, 2022 at 12:10:34PM +0200, Daniel Lezcano wrote:
> > > It would have been easier if the imx6/7 platforms were defining the thermal
> > > zones in their DT ...
> 
> After checking the driver I completely agree with you... But this would
> need much more rework.
> 
> > Marco, Lucas,
> > What about a &tempon { fsl,tempmon-critical-offset=<0>; } dt property?
> 
> I would not introduce a special binding for it. Instead I would add the
> support to parse the already exisiting bindings so the new-tempmon node
> would look like:
> 
> tempmon: tempmon {
>         compatible = "fsl,imx6q-tempmon";
>         interrupt-parent = <&gpc>;
>         interrupts = <0 49 IRQ_TYPE_LEVEL_HIGH>;
>         fsl,tempmon = <&anatop>;
>         nvmem-cells = <&tempmon_calib>, <&tempmon_temp_grade>;
>         nvmem-cell-names = "calib", "temp_grade";
>         clocks = <&clks IMX6QDL_CLK_PLL3_USB_OTG>;
>         #thermal-sensor-cells = <0>;
> 
> 	commercial-thermal {
> 		trips {
> 			commercial_trip_passive: trip-point0 {
> 				temperature = <85000>;
> 				type = "passive";
> 			};
> 			commercial_trip_crit: trip-point1 {
> 				temperature = <90000>;
> 				type = "critical";
> 			};
> 		};
> 	};
> 
> 	extended-commercial-thermal {
> 		trips {
> 			ecommercial_trip_passive: trip-point0 {
> 				temperature = <95000>;
> 				type = "passive";
> 			};
> 			ecommercial_trip_crit: trip-point1 {
> 				temperature = <100000>;
> 				type = "critical";
> 			};
> 		};
> 	};
> 
> 	industrial-thermal { };
> 	automotive-thermal { };
> };
> 
> That way we can reuse the existing dt definitions and if no *-thermal
> child device nodes exist we can keep the original logic.

Good for me, it's more work, of course, but if this is the way to do it
let it be.
Daniel: I'd like to have an ack from you before implementing this.

Francesco


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Daniel Lezcano]

Hi Francesco,


On 24/05/2022 12:07, Francesco Dolcini wrote:
> On Tue, May 24, 2022 at 10:35:14AM +0200, Marco Felsch wrote:
>> On 22-05-23, Francesco Dolcini wrote:
>>> On Wed, May 18, 2022 at 12:10:34PM +0200, Daniel Lezcano wrote:
>>>> It would have been easier if the imx6/7 platforms were defining the thermal
>>>> zones in their DT ...
>>
>> After checking the driver I completely agree with you... But this would
>> need much more rework.
>>
>>> Marco, Lucas,
>>> What about a &tempon { fsl,tempmon-critical-offset=<0>; } dt property?
>>
>> I would not introduce a special binding for it. Instead I would add the
>> support to parse the already exisiting bindings so the new-tempmon node
>> would look like:
>>
>> tempmon: tempmon {
>>          compatible = "fsl,imx6q-tempmon";
>>          interrupt-parent = <&gpc>;
>>          interrupts = <0 49 IRQ_TYPE_LEVEL_HIGH>;
>>          fsl,tempmon = <&anatop>;
>>          nvmem-cells = <&tempmon_calib>, <&tempmon_temp_grade>;
>>          nvmem-cell-names = "calib", "temp_grade";
>>          clocks = <&clks IMX6QDL_CLK_PLL3_USB_OTG>;
>>          #thermal-sensor-cells = <0>;
>>
>> 	commercial-thermal {
>> 		trips {
>> 			commercial_trip_passive: trip-point0 {
>> 				temperature = <85000>;
>> 				type = "passive";
>> 			};
>> 			commercial_trip_crit: trip-point1 {
>> 				temperature = <90000>;
>> 				type = "critical";
>> 			};
>> 		};
>> 	};
>>
>> 	extended-commercial-thermal {
>> 		trips {
>> 			ecommercial_trip_passive: trip-point0 {
>> 				temperature = <95000>;
>> 				type = "passive";
>> 			};
>> 			ecommercial_trip_crit: trip-point1 {
>> 				temperature = <100000>;
>> 				type = "critical";
>> 			};
>> 		};
>> 	};
>>
>> 	industrial-thermal { };
>> 	automotive-thermal { };
>> };
>>
>> That way we can reuse the existing dt definitions and if no *-thermal
>> child device nodes exist we can keep the original logic.
> 
> Good for me, it's more work, of course, but if this is the way to do it
> let it be.
> Daniel: I'd like to have an ack from you before implementing this.

It sounds like a correct approach, especially that is back compatible 
with old DTs.




-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v1] thermal: imx: Make trip point offset configurable

[Daniel Lezcano]

Hi Francesco,


On 24/05/2022 12:07, Francesco Dolcini wrote:
> On Tue, May 24, 2022 at 10:35:14AM +0200, Marco Felsch wrote:
>> On 22-05-23, Francesco Dolcini wrote:
>>> On Wed, May 18, 2022 at 12:10:34PM +0200, Daniel Lezcano wrote:
>>>> It would have been easier if the imx6/7 platforms were defining the thermal
>>>> zones in their DT ...
>>
>> After checking the driver I completely agree with you... But this would
>> need much more rework.
>>
>>> Marco, Lucas,
>>> What about a &tempon { fsl,tempmon-critical-offset=<0>; } dt property?
>>
>> I would not introduce a special binding for it. Instead I would add the
>> support to parse the already exisiting bindings so the new-tempmon node
>> would look like:
>>
>> tempmon: tempmon {
>>          compatible = "fsl,imx6q-tempmon";
>>          interrupt-parent = <&gpc>;
>>          interrupts = <0 49 IRQ_TYPE_LEVEL_HIGH>;
>>          fsl,tempmon = <&anatop>;
>>          nvmem-cells = <&tempmon_calib>, <&tempmon_temp_grade>;
>>          nvmem-cell-names = "calib", "temp_grade";
>>          clocks = <&clks IMX6QDL_CLK_PLL3_USB_OTG>;
>>          #thermal-sensor-cells = <0>;
>>
>> 	commercial-thermal {
>> 		trips {
>> 			commercial_trip_passive: trip-point0 {
>> 				temperature = <85000>;
>> 				type = "passive";
>> 			};
>> 			commercial_trip_crit: trip-point1 {
>> 				temperature = <90000>;
>> 				type = "critical";
>> 			};
>> 		};
>> 	};
>>
>> 	extended-commercial-thermal {
>> 		trips {
>> 			ecommercial_trip_passive: trip-point0 {
>> 				temperature = <95000>;
>> 				type = "passive";
>> 			};
>> 			ecommercial_trip_crit: trip-point1 {
>> 				temperature = <100000>;
>> 				type = "critical";
>> 			};
>> 		};
>> 	};
>>
>> 	industrial-thermal { };
>> 	automotive-thermal { };
>> };
>>
>> That way we can reuse the existing dt definitions and if no *-thermal
>> child device nodes exist we can keep the original logic.
> 
> Good for me, it's more work, of course, but if this is the way to do it
> let it be.
> Daniel: I'd like to have an ack from you before implementing this.

It sounds like a correct approach, especially that is back compatible 
with old DTs.




-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel