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@xxxxxxxxxxx/ > > > > 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@xxxxxxxxxxx/ > > > > 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@xxxxxxxxxxx/ Francesco
