| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 4 Sep 2017 13:06:39 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: "Wangtao (Kevin, Kirin)" <kevin.wangtao@...ilicon.com>,
rui.zhang@...el.com, edubezval@...il.com, robh+dt@...nel.org,
mark.rutland@....com, xuwei5@...ilicon.com,
catalin.marinas@....com, will.deacon@....com
Cc: leo.yan@...aro.org, kevin.wangtao@...aro.org,
linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
sunzhaosheng@...ilicon.com, gengyanping@...ilicon.com
Subject: Re: [PATCH v4 2/3] thermal: hisilicon: add thermal sensor driver for
Hi3660
Hi Kevin,
On 04/09/2017 09:56, Wangtao (Kevin, Kirin) wrote:
>
>
> 在 2017/9/1 5:17, Daniel Lezcano 写道:
>>
>> Hi Kevin,
>>
>>
>> On 29/08/2017 10:17, Tao Wang wrote:
>>> From: Tao Wang <kevin.wangtao@...aro.org>
>>>
>>> This patch adds the support for thermal sensor of Hi3660 SoC.
>>
>> for the Hi3660 SoC thermal sensor.
>>
>>> this will register sensors for thermal framework and use device
>>> tree to bind cooling device.
>>
>> Is it possible to give a pointer to some documentation or to describe
>> the hardware?
> Yes, there used to be on patch V3, I removed it on V4.
>>
>> An explanation of the adc min max coef[] range[] conversion would be
>> nice.
> OK
>>
>> In addition, having the rational behind the average and the max would be
>> nice. Do we really need both avg and max as virtual sensor?
> We only need max currently.
>>
>>> Signed-off-by: Tao Wang <kevin.wangtao@...aro.org>
>>> Signed-off-by: Leo Yan <leo.yan@...aro.org>
>>> ---
>>> drivers/thermal/Kconfig | 13 +++
>>> drivers/thermal/Makefile | 1 +
>>> drivers/thermal/hisi_tsensor.c | 209
>>> +++++++++++++++++++++++++++++++++++++++++
>>
>>
>> IMO, we don't need a new file, but merge this code with the current
>> hisi_thermal.c driver. BTW, the hi6220 has also a tsensor which is
>> different from this one.
>>
>> I suggest to base the hi3660 thermal driver on top of the cleanup I sent
>> for the hi6220.
> The tsensor of hi3660 is a different one, merging the code with hi6220
> will need a lot of change.
Have a look at the hisi_thermal.c at:
https://git.linaro.org/people/daniel.lezcano/linux.git/tree/drivers/thermal/hisi_thermal.c?h=thermal/hikey-4.14
after the cleanup I recently sent.
I'm pretty sure, with a little effort, we can merge both.
Especially if the virtual things is separated.
At the end, what do we do ? Read a register.
>>> 3 files changed, 223 insertions(+)
>>> create mode 100644 drivers/thermal/hisi_tsensor.c
>>>
>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>>> index b5b5fac..32f582d 100644
>>> --- a/drivers/thermal/Kconfig
>>> +++ b/drivers/thermal/Kconfig
>>> @@ -203,6 +203,19 @@ config HISI_THERMAL
>>> thermal framework. cpufreq is used as the cooling device to
>>> throttle
>>> CPUs when the passive trip is crossed.
>>> +config HISI_TSENSOR
>>> + tristate "Hisilicon tsensor driver"
>>> + depends on ARCH_HISI || COMPILE_TEST
>>> + depends on HAS_IOMEM
>>> + depends on OF
>>> + default y
>>> + help
>>> + Enable this to plug Hisilicon's tsensor driver into the Linux
>>> thermal
>>> + framework. Besides all the hardware sensors, this also support
>>> two
>>> + virtual sensors, one for maximum of all the hardware sensor, and
>>> + one for average of all the hardware sensor.
>>> + Compitable with Hi3660 or higher.
>>
>> s/Compitable/Compatible/
> OK
>>
>>> +
>>> config IMX_THERMAL
>>> tristate "Temperature sensor driver for Freescale i.MX SoCs"
>>> depends on (ARCH_MXC && CPU_THERMAL) || COMPILE_TEST
>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>>> index 094d703..8a16bd4 100644
>>> --- a/drivers/thermal/Makefile
>>> +++ b/drivers/thermal/Makefile
>>> @@ -56,6 +56,7 @@ obj-$(CONFIG_ST_THERMAL) += st/
>>> obj-$(CONFIG_QCOM_TSENS) += qcom/
>>> obj-$(CONFIG_TEGRA_SOCTHERM) += tegra/
>>> obj-$(CONFIG_HISI_THERMAL) += hisi_thermal.o
>>> +obj-$(CONFIG_HISI_TSENSOR) += hisi_tsensor.o
>>> obj-$(CONFIG_MTK_THERMAL) += mtk_thermal.o
>>> obj-$(CONFIG_GENERIC_ADC_THERMAL) += thermal-generic-adc.o
>>> obj-$(CONFIG_ZX2967_THERMAL) += zx2967_thermal.o
>>> diff --git a/drivers/thermal/hisi_tsensor.c
>>> b/drivers/thermal/hisi_tsensor.c
>>> new file mode 100644
>>> index 0000000..34cf2ba
>>> --- /dev/null
>>> +++ b/drivers/thermal/hisi_tsensor.c
>>> @@ -0,0 +1,209 @@
>>> +/*
>>> + * linux/drivers/thermal/hisi_tsensor.c
>>> + *
>>> + * Copyright (c) 2017 Hisilicon Limited.
>>> + * Copyright (c) 2017 Linaro Limited.
>>> + *
>>> + * Author: Tao Wang <kevin.wangtao@...aro.org>
>>> + * Author: Leo Yan <leo.yan@...aro.org>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> modify
>>> + * it under the terms of the GNU General Public License as
>>> published by
>>> + * the Free Software Foundation; version 2 of the License.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program. If not, see
>>> <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/device.h>
>>> +#include <linux/err.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/of.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/thermal.h>
>>> +
>>> +#include "thermal_core.h"
>>> +
>>> +#define VIRTUAL_SENSORS 2
>>> +
>>> +/* hisi Thermal Sensor Dev Structure */
>>> +struct hisi_thermal_sensor {
>>> + struct hisi_thermal_data *thermal;
>>> + struct thermal_zone_device *tzd;
>>> + void __iomem *sensor_reg;
>>> + unsigned int id;
>>> +};
>>> +
>>> +struct hisi_thermal_data {
>>> + struct platform_device *pdev;
>>> + struct hisi_thermal_sensor *sensors;
>>> + unsigned int range[2];
>>> + unsigned int coef[2];
>>> + unsigned int max_hw_sensor;
>>> +};
>>> +
>>> +static int hisi_thermal_get_temp(void *_sensor, int *temp)
>>> +{
>>> + struct hisi_thermal_sensor *sensor = _sensor;
>>> + struct hisi_thermal_data *data = sensor->thermal;
>>> + unsigned int idx, adc_min, adc_max, max_sensor;
>>> + int val, average = 0, max = 0;
>>> +
>>> + adc_min = data->range[0];
>>> + adc_max = data->range[1];
>>> + max_sensor = data->max_hw_sensor;
>>> +
>>> + if (sensor->id < max_sensor) {
>>> + val = readl(sensor->sensor_reg);
>>> + val = clamp_val(val, adc_min, adc_max);
>>
>> That looks a bit fuzzy. Why not create a get_temp for physical sensor
>> and another one for the virtual? So there will be a clear distinction
>> between both.
> make sense
After thinking about it. I think it virtual sensor should be a separate
driver.
[ ... ]
>> Do we really need a max sensor definition in the DT? Isn't it something
>> we can deduce by looping with platform_get_resource below ?
>>
>> eg.
>>
>> while ((res = platform_get_resource(..., num_sensor++)) {
>> ...
>> }
>>
>> That said, I think we can assume there are 3 sensors always, no?
> If we have three CPU cluster or two cluster share the same sensor in
> future, that number is certain on hi3660
Do you mean, it is always 3 ?
>>> + data->sensors = devm_kzalloc(dev,
>>> + sizeof(*data->sensors) * (max_sensor + VIRTUAL_SENSORS),
>>> + GFP_KERNEL);
>>> + if (IS_ERR(data->sensors)) {
[ ... ]
>>> + }
>>> + }
>>> +
>>> + sensor->id = i;
>>
>> How can we deal with holes in the DT?Do you mean the holes of sensor id?
Yes.
[ ... ]
-- Daniel
--
<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