Message-ID: <f9527614-01ba-4954-88de-8a17ae1a84ba@linaro.org>
Date: Tue, 30 Jul 2024 14:58:58 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Alexandre Bailon <abailon@...libre.com>, rafael@...nel.org,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org
Cc: rui.zhang@...el.com, lukasz.luba@....com, linux-pm@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 0/4] thermal: Add support of multiple sensors


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 |
<http://www.linaro.org/linaro-blog/> Blog

Powered by blists - more mailing lists