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

[Marco Felsch <m.felsch@pengutronix.de>]

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
> 
>