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

[Zhou Wang <wangzhou1@hisilicon.com>]

On 2020/4/28 22:02, Daniel Lezcano wrote:
> 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 ?

Hi Daniel,

In our SoC, we use IMU(Intelligent Management Unit) which is an out of band
management processor to control cooling device. We use fans to cool CPU, one
fan is for one SoC. So getting one temperature for one SoC is enough here.

We also want to report temperature of the SoC from kernel thermal subsystem,
so users get get SoC temperature from sysfs or user space tool, like Im-sensor.
The goal of this driver is just to do this.

Best,
Zhou

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