| Message ID | 20220516190001.147919-1-francesco.dolcini@toradex.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] thermal: imx: Make trip point offset configurable | expand |
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?
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
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(-)