Subject: Re: [PATCH v12 1/2] thermal: loongson-2: add thermal management
 support
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     "Rafael J . Wysocki" <rafael@kernel.org>,
        Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        zhanghongchen <zhanghongchen@loongson.cn>,
        Liu Peibao <liupeibao@loongson.cn>, lvjianmin@loongson.cn,
        wanghongliang@loongson.cn, zhuyinbo@loongson.cn
References: <20221114024709.7975-1-zhuyinbo@loongson.cn>
 <20230206135921.GA15176@linaro.org>
 <64d9782c-cafd-cdc3-3602-719c386d98cc@loongson.cn>
 <20230208104919.GA120053@linaro.org>
From:   zhuyinbo <zhuyinbo@loongson.cn>
Message-ID: <5abe22ee-77d4-4349-1d88-43e4324177f0@loongson.cn>
Date:   Mon, 13 Feb 2023 20:45:42 +0800
User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20230208104919.GA120053@linaro.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


在 2023/2/8 下午6:49, Daniel Lezcano 写道:
> On Wed, Feb 08, 2023 at 11:13:33AM +0800, zhuyinbo wrote:
>
> [ ... ]
>
>>>> +struct loongson2_thermal_data {
>>>> +	struct thermal_zone_device *tzd;
>>> 'tzd' won't be needed after taking into account the comments
>> The 'tzd' element is needed,  because the thermal_zone_device_update need
>> pass a data->tzd element.
>>
>> static irqreturn_t loongson2_thermal_irq_thread(int irq, void *dev)
>> {
>>          struct loongson2_thermal_data *data = dev;
>>
>>          thermal_zone_device_update(data->tzd,
>>                                     THERMAL_EVENT_UNSPECIFIED);
>>          enable_irq(data->irq);
>>
>>          return IRQ_HANDLED;
>> }
> After taking into account all the comments, enabled_irq() won't be
> called, so 'data' won't be needed. 'tzd' will be passed to
> devm_request_threaded_irq() instead of 'data'.
>
> As loongson2_thermal_irq_thread() is the only place where 'tzd' is
> needed and 'tzd' being local to the call site of
> thermal_zone_device_register() and devm_request_threaded_irq(), there
> is no need to store the pointer in the 'data' structure.

okay, I got it. I will remove tzd element in data struct.

>
>>>> +	int irq;
>>> 'irq' won't be needed after taking into account the comments
>> I will drop it.
>>>> +	int id;
>>>> +	void __iomem *regs;
>>>> +	struct platform_device *pdev;
>>> 'pdev' is not needed
>> I will drop it.
>>>> +	u16 ctrl_low_val;
>>>> +	u16 ctrl_hi_val;
>>> Those fields won't be needed after taking into account the comments
>> I will drop it.
>>>> +};
>>>> +
>>>> +static int loongson2_thermal_set(struct loongson2_thermal_data *data,
>>>> +					int low, int high, bool enable)
>>>> +{
>>>> +	u64 reg_ctrl = 0;
>>>> +	int reg_off = data->id * 2;
>>>> +
>>>> +	if (low > high)
>>>> +		return -EINVAL;
>>>> +
>>>> +	low = max(low, -100);
>>>> +	high = min(high, 155);
> Documentation says -40, 125
My previous calculation is to consider that the range of 8bit 
representation is 0 to 255,
and node (cpu) temp=Thens0_ out -100, So the temperature range is 0-100 
~ 255-100, and this
range includes -40~125.  In fact, the range described in the manual is - 
40~125, I will
adop it.
>
>>>> +	low += 100;
>>>> +	high += 100;
>>> Why are those values added to the low and high ? Did you mean low += 0x100 ?
>>>
>>> Mind to describe a bit the register layout?
>> node(cpu) temp = Thens0_out -100,
>>
>> low and high is record node temp, so low and high need add '100' as
>> Thens0_out.
> If I refer to the documentation it is a raw value converted from
> centigrade. The function has degree.
>
> So it should be:
>
> temp_deci = temp_milli / 10
>
> raw = temp_to_raw(temp_deci);
>
> -> temp_to_raw to be determined from temp = (raw * 731) / 0x4000 - 273
>
> [ ... ]
I have review the 3a5000 datasheet,  what you said is right about 
3a5000, but in 2k1000 datasheet,

the calculation of temperature is follows:

Temperature = Thens0_out - 100
  
get_temp return value is (Temperature * 1000) and set_trips is (value /1000) in 2k1000.
I don't find a caculate about "temp_deci = temp_milli / 10" . Are you talking about "
temp_deci = temp_milli / 1000" in set_trips?

>
>>>> +static int loongson2_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
>>>> +{
>>>> +	u32 reg_val;
>>>> +	struct loongson2_thermal_data *data = tz->devdata;
>>>> +
>>>> +	reg_val = readl(data->regs + LOONGSON2_TSENSOR_OUT);
>>>> +	*temp = ((reg_val & 0xff) - 100) * 1000;
>>> Why '-100' ?
>>>
>>> Is the unit returned 'degrees' ?
>> node(cpu) temp = Thens0_out -100,
>>
>> Here we need to get a node temp.
> If I refer to the Loongson-3A5000 manual and assuming it is the right
> one, the documentation says:
>
> Temperature = Thens0_out * 731 / 0x4000 - 273
>
> The unit is centigrade.
>
> [ ... ]

Yes, 3a5000 is what you said, but 2k1000 is calculated as follows:

Temperature = Thens0_out - 100
>
>>>> +	writeb(0xff, data->regs + LOONGSON2_TSENSOR_STATUS);
>>>> +
>>>> +	loongson2_thermal_set(data, 0, 0, false);
>>> It would be nicer to use a reset line if it is available
>> sorry, I don't get your meaning. Please describe more details about 'reset
>> line'.
> After a reset, the thermal controller should be in default state and the interrupt
> flag cleared.
>
> One example:
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/arch/arm64/boot/dts/nvidia/tegra210.dtsi#n1560
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/tegra/soctherm.c#n2169
>
> Then search in the driver for:
>       reset_control_assert(reset);
>       reset_control_deassert(reset);
>
>
> [ ... ]
>
> Thanks
>
>    -- Daniel
thanks your explicate!   but our platform doesn't support it.
>