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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 20/06/2022 17:48, Francesco Dolcini wrote:
> Hello Krzysztof,
> thanks for your comment, let me try to provide you some additional
> background to better understand this change.
> 
> On Fri, Jun 17, 2022 at 06:02:39PM -0700, Krzysztof Kozlowski wrote:
>> On 17/06/2022 00:08, Francesco Dolcini wrote:
>>> Move `trips` definition to `#/$defs/trips-base` and just reference it
>>> from the trips node. This allows to easily re-use this binding from
>>> another binding file.
>>>
>>> No functional changes expected.
>>
>> If you want to re-use trips, they should be rather moved to separate
>> YAML file...
> 
> Fine, this should not be a big deal to achieve. Let's agree on the rest
> first, however.
> 
>> 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.
> 
> 
> 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.
> 
> 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.
> 
> Krzysztof, what do you think? I would not mind to get back to one of
> the more simpler approach I proposed.
> 
> Lucas, are you really that against the simple working solution I
> proposed initially [1]? I feel like I am running in circles ...

BTW, the link [1] was missing in your email, so I understood that you
meant this patchset. If [1] refers to something else, then we need to
discuss that something else.

Best regards,
Krzysztof