[PATCH 1/4] arm64: dts: imx8mm-venice-gw700x: override thermal cfg for industrial temp

[Frieder Schrempf]

Hi Tim,

On 11.06.21 20:55, Tim Harvey wrote:
> On Mon, Jun 7, 2021 at 1:34 AM Frieder Schrempf
> <frieder.schrempf at kontron.de> wrote:
>>
>> On 07.06.21 10:00, Jacky Bai wrote:
>>>> Subject: Re: [PATCH 1/4] arm64: dts: imx8mm-venice-gw700x: override
>>>> thermal cfg for industrial temp
>>>>
>>>> On 07.06.21 09:30, Jacky Bai wrote:
>>>>>> Subject: Re: [PATCH 1/4] arm64: dts: imx8mm-venice-gw700x: override
>>>>>> thermal cfg for industrial temp
>>>>>>
>>>>>> On 04.06.21 17:42, Tim Harvey wrote:
>>>>>>> On Wed, Jun 2, 2021 at 12:11 AM Frieder Schrempf
>>>>>>> <frieder.schrempf at kontron.de> wrote:
>>>>>>>>
>>>>>>>> On 01.06.21 19:49, Tim Harvey wrote:
>>>>>>>>> Override the default temperature alert/crit for Industrial temp
>>>>>>>>> IMX8M Mini.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tim Harvey <tharvey at gateworks.com>
>>>>>>>>> ---
>>>>>>>>>  .../boot/dts/freescale/imx8mm-venice-gw700x.dtsi     | 12
>>>>>> ++++++++++++
>>>>>>>>>  1 file changed, 12 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw700x.dtsi
>>>>>>>>> b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw700x.dtsi
>>>>>>>>> index c769fadbd008..512b76cd7c3b 100644
>>>>>>>>> --- a/arch/arm64/boot/dts/freescale/imx8mm-venice-gw700x.dtsi
>>>>>>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-venice-gw700x.dtsi
>>>>>>>>> @@ -493,3 +493,15 @@
>>>>>>>>>               >;
>>>>>>>>>       };
>>>>>>>>>  };
>>>>>>>>> +
>>>>>>>>> +&cpu_alert0 {
>>>>>>>>> +     temperature = <95000>;
>>>>>>>>> +     hysteresis = <2000>;
>>>>>>>>> +     type = "passive";
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> +&cpu_crit0 {
>>>>>>>>> +     temperature = <105000>;
>>>>>>>>> +     hysteresis = <2000>;
>>>>>>>>> +     type = "critical";
>>>>>>>>> +};
>>>>>>>>
>>>>>>>> As this is not really board-specific, I think the proper way to
>>>>>>>> handle this for
>>>>>> all boards is to let the thermal driver read the temperature grading
>>>>>> from the OTP fuses and set the trip-points accordingly, similar to
>>>>>> what is done on i.MX6 [1].
>>>>>>>>
>>>>> ...
>>>>>>>
>>>>>>> Frieder,
>>>>>>>
>>>>>>> Yes, I thought about adding that kind of support to imx8mm_thermal.c
>>>>>>> but the difference is that imx8mm has alerts defined by dt and imx6
>>>>>>> does not so is it right to override dt alerts on imx8m? What if
>>>>>>> someone designs a board that they specifically want a lower alert
>>>>>>> than the cpu grade they are using based on something else on the board?
>>>>>>>
>>>>>>> My approach to this was to eventually actually adjust the imx8m dt
>>>>>>> alerts in boot firmware based on some boot firmware setting or
>>>>>>> specific board support and leave the kernel alone.
>>>>>>
>>>>>> Allowing board-specific trip points sounds like a valid request, but
>>>>>> I still think we need a way to handle the temperature grading in the
>>>>>> driver if no board-specific trip-points are given.
>>>>>>
>>>>>> What if we just set the temperature property in the trip nodes in
>>>>>> imx8mm.dtsi to zero? The thermal driver would detect this and setup
>>>>>> the correct values according to the grading. If the dt already
>>>>>> provides non-zero temperature values (through the board dts) the
>>>>>> driver will just leave the values and disregard the grading.
>>>>>>
>>>>>> I think this solution would be covering all needs.
>>>>>
>>>>> I thought to add the grading check in the imx8mm_thermal.c to
>>>>> dynamically set the trip points temp, but it seems hard to do it due
>>>>> to the fact of_thermal is used, as no helper API is exported by of_thermal,
>>>> no better way to override the trip point temp.
>>>>>
>>>>> glad to see any good suggestions.
>>>>
>>>> Right, the driver doesn't handle the trip-points directly. This is all hidden in the
>>>> framework. So this might not be so easy to implement.
>>>>
>>>> What about this other approach: Adding all the possible trip-points for the
>>>> different gradings to the SoC-devicetree and then let the thermal driver
>>>> remove the trip nodes from the dt that are not valid for the detected grading,
>>>> just before the driver registers the sensor/zone.
>>>
>>> It is more reasonable for the firmware/bootloader to handle this by checking the grading.
>>
>> If possible, I would rather like to avoid creating another dependency on bootloader/firmware. I think the kernel should be able to detect the grading by itself and adjust its behavior accordingly. We also do this for the speed grading in cpufreq.
> 
> Frieder and Jacky,
> 
> I'm ok with dropping this dt patch and instead implementing something
> in boot firmware that automatically detects and adjusts there. I'm not
> given the time to work through a more complicated or more elegant
> solution kernel-only approach for this and handling it in the boot
> firmware will not break anything or create a dependence from where we
> currently stand. We already have things in boot firmware that populate
> mac addresses, mtd partition ids, etc in dt during runtime.

>From my point of view you can also keep this patch until this is solved properly. Still, in the long run I think we need a solution that automatically handles the different SoC temperature gradings even if of_thermal is used and there is only a single devicetree to describe the SoC variants. It's similar to the case of the CPU's frequency/voltage setpoints in cpufreq.

I'm Cc-ing people from the thermal subsystem. Maybe they have some suggestion or this case has already been discussed elsewhere. 

Best regards
Frieder