Re: [PATCH v4 0/4] thermal: Add support of multiple sensors

Daniel Lezcano
2024-07-30T12:59:01Z

Hi Alexandre,

thanks for your series and my apologizes for taking a so long time to
review.

I went through the series and at the first glance I'm not sure we want
to add all the multi specific code in a separate file.

IMO, there is a preparatory work by changing the functions:

thermal_zone_device_register_with_trips() and
thermal_zone_device_register_tripless()

where we group and move the functions parameters to the
thermal_zone_device_param.

Then we can add a num_ops field which is will default to zero.

With that we should have put a foundation for multiple ops, so multiple
sensors.

On 13/06/2024 15:24, Alexandre Bailon wrote:
> Following this comment [1], this updates thermal_of to support multiple
> sensors.
>
> This series intends to add support of thermal aggregation.
> One use case for it is using the IPA in the case we have
> multiple sensors for one performance domain.
>
> This has been tested on the mt8195 using s-tui.
> To test and validate, we heat up the CPU and the heat sink.
> At some point, we run benchmark tests with different configurations:
> - Mediatek kernel (IPA + their own thermal aggregation)
> - Mainline kernel
> - Mainline kernel with IPA and aggregation enabled
> With the IPA and the aggregation enabled, we get the best performances
> with the most stable CPU temperature.
>
> The aggregation is configured and enabled using device tree.
> One thermal zone has to be created with a list of sensors.
> It will take care of registering a thermal zone for each sensors.
> The cooling device will only be registered with the aggregating thermal
> zone.
>
> There are still something important missing: a way to check that all
> aggregated sensors are part of the same performance domain.
> So far, I don't see how this should be done. Some recommendations would be
> appreciated.
>
> Changes in v2:
> - Rebased on 6.7
> - Separated generic multi sensor and dt specific code
> - Simplified the code
> - Drop min / max and only do weighted average (seems more adequate for IPA)
>
> Changes in v3:
> - Rebased on 6.9
> - Reworked the way to register a multi sensor thermal zone
> - Only one thermal zone to define in device tree
> - Max has been re-added
> - Enabled it on mt8195
>
> Changes in v4:
> - Rebased on lastest master (fixed the build issue)
> - Dropped the average since I don't have any usecase for it
>
> [1]: https://patchwork.kernel.org/comment/24723927/
>
> Alexandre Bailon (4):
> dt-bindings: thermal: Restore the thermal-sensors property
> thermal: Add support of multi sensors to thermal_core
> thermal: Add support of multi sensors to thermal_of
> ARM64: mt8195: Use thermal aggregation for big and little cpu
>
> .../bindings/thermal/thermal-zones.yaml | 5 +-
> arch/arm64/boot/dts/mediatek/mt8195.dtsi | 212 ++-----------
> drivers/thermal/Makefile | 1 +
> drivers/thermal/thermal_core.h | 15 +
> drivers/thermal/thermal_multi.c | 288 ++++++++++++++++++
> drivers/thermal/thermal_of.c | 250 ++++++++++++++-
> include/uapi/linux/thermal.h | 5 +
> 7 files changed, 579 insertions(+), 197 deletions(-)
> create mode 100644 drivers/thermal/thermal_multi.c
>

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |