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

[Krzysztof Kozlowski]

On 20/06/2022 19:43, Francesco Dolcini wrote:
> 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.

Probably you would need to support both solutions in the same driver,
based on presence of thermal-sensor-cells property. It won't be
particularly easy code, but maybe it is worth anyway... I am quite
surprised to see that IMX thermal driver does not use generic framework
and does not support generic bindings.

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

More comments were not concering the Linux IMX thermal implementation
but rather bindings. From my point of view, you can use the same generic
thermal bindings even if your implementation does not use thermal_of.

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


Ah, that comment was not to me but to Lucas.

Best regards,
Krzysztof