Re: [PATCH v2 1/9] dt-bindings: thermal: Define trips node in $defs

[Francesco Dolcini <francesco.dolcini@toradex.com>]

On Mon, Jun 20, 2022 at 06:44:23PM +0200, Krzysztof Kozlowski wrote:
> On 20/06/2022 17:48, Francesco Dolcini wrote:
> > On Fri, Jun 17, 2022 at 06:02:39PM -0700, Krzysztof Kozlowski wrote:
> >> but anyway this should not be done per-driver bindings, but
> >> in more general way. Either the problem - using one DTS for different
> >> temperature grades - looks generic or is wrong at the core. In the first
> >> option, the generic bindings should be fixed. In the second case - using
> >> same DTS for different HW is not correct approach and why only thermal
> >> should be specific? I can imagine that cooling devices might have
> >> different settings, regulator voltages for DVFS could be a bit different...
> > 
> > Let me try to explain the problem I am trying to solve here.
> > 
> > Currently the imx-thermal driver harcode the critical trip threshold,
> > this trip point is read-only as it is considered a system property that
> > should not be changed and it is set to a value that is less than the
> > actual SoC maximum temperature. NO thermal_of driver used.
> > 
> > Because of that there are systems that cannot work on some valid
> > temperature range.
> > 
> > We are currently looking at a solution that would be backward compatible
> > with old device tree.
> > 
> > I proposed the following:
> > 1- just increase the threshold to the actual max value allowed according
> >    to the SoC thermal grade. 
> > 
> >    As easy as 
> > 
> > -	data->temp_critical = data->temp_max - (1000 * 5);
> > +	data->temp_critical = data->temp_max;
> >    
> >    in drivers/thermal/imx_thermal.c 
> > 
> >    https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/
> > 
> >    It was not considered good enough by Lucas since this is a overall
> >    system design question, therefore should be configurable.
> > 
> > 2- make the critical trip write-able from userspace/sysfs.
> > 
> >    Daniel is against this since critical trip point is a system
> >    property, not something the user should be allowed to change.
> > 
> > 3- kernel parameter: https://lore.kernel.org/all/20220516190001.147919-1-francesco.dolcini@toradex.com/
> > 
> >    Initially proposed by Daniel, but Marco did not like the idea.
> > 
> > 4- New device tree property, fsl,tempmon-critical-offset, ditched also
> >    by Marco
> > 
> > 5- The current solution in this patch, with the existing trip points
> >    that are hardcoded in the code exposed in the device tree as trips.
> 
> Thanks for the explanation, I see the problem.
> 
> > 
> > Ideally one could just implement the imx6/7 thermal sensor reading and
> > just make use of the thermal_of driver, however that would break
> > compatibility with a lot of existing system ... to me this is just a
> > no-go.
> 
> This I did not understand...  What is not implemented in thermal sensor
> which would solve the issue? And why it cannot be implemented in
> backwards compatible way?

Currently the imx_thermal driver defines its own trip points. How would
you change the code to work with old device tree binaries using the
generic thermal_of driver? imx_thermal would need to be changed to be a
thermal sensor device, all the thermal trip point code removed.
The driver is using thermal_zone_device_register().

Maybe I am missing an obvious solution, just correct me if I am wrong.


> Your change is also not backwards friendly, which means existing boards
> (old DTS) will not receive the update.
The change proposed in this series is 100% backward compatible,
the code-defined trip point are optionally overwritten by the dts.


> > Adding only one set of thermal trip point in the dts (no thermal-grade
> > specific set) could work in some specific scenario, however it does not
> > work for me since I have the same dts files using different temperature
> > grade SoC. I would need to update this in the firmware before starting
> > Linux.
> 
> 2. If the devices are in general compatible but have discoverable
> differences, use one DTS, discover the differences and apply them
> dynamically via driver (e.g. read the temperature offset from some
> nvmem/OTP).

Yes, of course, I agree.
That would work and it would be a reasonable approach in general, but it
has one big drawback, it will force an update on the firmware on
well-established products. Anyway, would you accept a change on the
thermal_imx driver using a single set of trips from the dts, but not
using the thermal_of driver?

> > Lucas, are you really that against the simple working solution I
> > proposed initially [1]? I feel like I am running in circles ...
> 
> Yes, because it is not generic and skips other similar cases, like
> regulator voltages or battery properties. I can easily imagine that next
> week someone comes with duplicated opp tables, then duplicated voltages,
> then duplicated CPU nodes and finally we have one DTS for imx6 and imx7
> but everything is in multiple variants. :)

The patch I proposed in [1] was just making the hard-coded threshold
more reasonable, instead of setting the critical threshold to max-5 to
just max. Your concern here does not really applies.

Here the patch https://lore.kernel.org/all/20220420091300.179753-1-francesco.dolcini@toradex.com/

Francesco