| Message ID | 20220516190001.147919-1-francesco.dolcini@toradex.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | [v1] thermal: imx: Make trip point offset configurable | expand |
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)
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
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 > >
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
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 > > >
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
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 ...
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
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 > >
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
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.
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)
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(-)