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@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? Your change is also not backwards friendly, which means existing boards (old DTS) will not receive the update. > 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. Usually the bootloader loads the overlay and this is recommended approach to runtime tweaking DTB for some variant. > > Krzysztof, what do you think? I would not mind to get back to one of > the more simpler approach I proposed. As I said, I see the problem, but I am not sure that solution is correct. I can also rephrase the solution to a such one: "I want to support iMX6 and iMX7 with one DTS, so I will embed all properties from both DTS into one DTS and then during boot I will read soc-id register and choose some subset of the properties" No way... I also brought in previous reply trouble with regulator voltages or some other electric-properties. I don't want several duplicated properties per different variants of the same SoC. 1. If the devices are fully compatible, use one DTS. If you can squeeze different variants into the same DTS without any duplication so that entire DTS is used 100% by both variants - sure no, problem, less code. 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). 3. If the devices are partially the same but have differences, you can use overlays for that differences. This is quite flexible and clean solution as it also clearly documents the hardware in DTS and its overlays. 4. In all other options devices are different, so I expect different DTS. We had similar cases already in the past - some SoC versions could work on higher frequencies with higher voltages (arch/arm/boot/dts/exynos4412-prime.dtsi). These variants identified as exactly the same SoC as the earlier/slower one. You cannot use one DTS for them. We have chosen different DTS. For Samsung other case, same SoC comes in different bins with slightly different voltages for CPU/memory. Same frequencies but different voltages. This might be the closest to your problem. This was detectable runtime, so we had one DTS and we adjusted the voltages based on static tables in the driver (exynos5422-asv.c). > 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. :) Also I am against because DTS describes one hardware, not multiple different variants. Best regards, Krzysztof
