Re: [PATCH V3 2/2] thermal: Add HiSilicon Kunpeng thermal driver

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 28/04/2020 13:58, shenyang (M) wrote:
> On 2020/4/27 20:13, Daniel Lezcano wrote:
>> On 21/04/2020 09:44, Yang Shen wrote:
>>> Support HiSilicon Kunpeng tsensor. the driver will report the max
>>> temperature for each core.
>>
>> As this is a new driver, can you give a bit more details of the hardware
>> in this description.
>>
>> A subsidiary question, why do you want to aggregate the temperatures in
>> this driver ?
>>
> 
> OK. In fact, there are five temperature sensors distributed in the SOC.
> And our strategy is to collect all temperatures and return the max to
> the interface.

The aggregation should be done in the thermal framework not in the driver.

Why not create one sensor per thermal zone, so giving the opportunity to
create different configurations with different cooling device ?

The function thermal_zone_of_get_sensor_id() will return the sensor id
associated with the thermal zone. So you can get inspiration from
imx_sc_thermal_probe() to instantiate multiple sensors from the thermal
zone.

The platform will end up with one thermal zone per core (assuming that
is what you have).

Each thermal zone will be cool down by the same cooling device, the
thermal framework will aggregate the decision and it will take the max
cooling level requested by each governor of each thermal zone, that ends
up to the same result as aggregating the temperature in the driver.

By doing this, you keep the possibility to do different combinations at
the DT level for better thermal control with different type of cooling
device.


> I will add a description of the hardware in the next version.
> 

[ ... ]

>>> +static int kunpeng_thermal_temp_correct(u32 tmp, u32 trim)
>>> +{
>>> +    int trim_high = KUNPENG_TSENSOR_TRIM_HIGH(trim);
>>> +    int trim_low = KUNPENG_TSENSOR_TRIM_LOW(trim);
>>> +    int val = KUNPENG_TSENSOR_TEMP_VAL(tmp);
>>> +
>>> +    if (trim_high == trim_low)
>>> +        return INT_MIN;
>>> +
>>> +    /* temperature of tsensor needs to be calibrated */
>>> +    return KUNPENG_TSENSOR_BASIC_TRIM_RANGE * (val - trim_low) /
>>> +           (trim_high - trim_low) + KUNPENG_TSENSOR_BASIC_TMP;
>>
>> Is it possible to give some details about why this is done?
>>
> 
> The hardware will measure the two standard temperature readings and
> write them into the registers, and the driver will calibrate the
> current readings according to the two standard temperature readings.
> 
> I will add this comment.
> 

[ ... ]

>> What is the IMU ?
>>
> 
> IMU is stand for Intelligent Management Unit. It functions as a
> supervisor and a manager of the chip. It has complete SoC
> components and is totally independent from the application processor
> system.
> So the IMU will read the tsensors temperature registers too.
> 
>> I don't see in this driver anything related to the sensors being shared
>> with something else.
>>
> 
> Yes. This driver use the device independently. But the driver cannot
> disable devices when it is removed.
> I add comments here to avoid user confusion.

I see, the kernel has no knowledge of it.

Thanks for the clarification.

>>> + */
>>> +static int kunpeng_thermal_remove(struct platform_device *pdev)
>>> +{
>>> +    struct thermal_zone_device *thermal_zone =
>>> platform_get_drvdata(pdev);
>>> +
>>> +    thermal_zone_device_unregister(thermal_zone);
>> Why not add a devm_thermal_zone_device_register() ? and get rid of this
>> function ?
>>
> 
> Do  you ask me to add this function in thermal_core.c?

Yes


-- 
<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