[V3,2/2] thermal: Add HiSilicon Kunpeng thermal driver

Commit Message

Yang Shen April 21, 2020, 7:44 a.m. UTC

Support HiSilicon Kunpeng tsensor. the driver will report the max
temperature for each core.

Signed-off-by: Yang Shen <[e-mail]>
Signed-off-by: Zhou Wang <[e-mail]>
Signed-off-by: Kunshan Tang <[e-mail]>
---
 drivers/thermal/Kconfig           |   8 ++
 drivers/thermal/Makefile          |   1 +
 drivers/thermal/kunpeng_thermal.c | 216 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100644 drivers/thermal/kunpeng_thermal.c

--
2.7.4

Comments

Daniel Lezcano April 27, 2020, 12:13 p.m. UTC | #1

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 ?

> Signed-off-by: Yang Shen <[e-mail]>
> Signed-off-by: Zhou Wang <[e-mail]>
> Signed-off-by: Kunshan Tang <[e-mail]>
> ---
>  drivers/thermal/Kconfig           |   8 ++
>  drivers/thermal/Makefile          |   1 +
>  drivers/thermal/kunpeng_thermal.c | 216 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 225 insertions(+)
>  create mode 100644 drivers/thermal/kunpeng_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 5a05db5..7611b5d 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -239,6 +239,14 @@ config HISI_THERMAL
>  	  thermal framework. cpufreq is used as the cooling device to throttle
>  	  CPUs when the passive trip is crossed.
> 
> +config KUNPENG_THERMAL
> +	tristate "HiSilicon kunpeng thermal driver"
> +	depends on ARM64 || COMPILE_TEST
> +	help
> +	  Enable this to plug HiSilicon kunpeng's thermal sensors driver into
> +	  the Linux thermal framework, which supports to get the highest
> +	  temperature of one Kunpeng SoC.
> +
>  config IMX_THERMAL
>  	tristate "Temperature sensor driver for Freescale i.MX SoCs"
>  	depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 9fb88e2..88ffca5 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -57,3 +57,4 @@ obj-$(CONFIG_GENERIC_ADC_THERMAL)	+= thermal-generic-adc.o
>  obj-$(CONFIG_ZX2967_THERMAL)	+= zx2967_thermal.o
>  obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
>  obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
> +obj-$(CONFIG_KUNPENG_THERMAL)     += kunpeng_thermal.o
> diff --git a/drivers/thermal/kunpeng_thermal.c b/drivers/thermal/kunpeng_thermal.c
> new file mode 100644
> index 0000000..d22e875
> --- /dev/null
> +++ b/drivers/thermal/kunpeng_thermal.c
> @@ -0,0 +1,216 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2020 HiSilicon Limited. */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +
> +#define KUNPENG_TSENSOR_CONTROL			0x20D0
> +#define KUNPENG_TSENSOR_ST			0x60D0
> +#define KUNPENG_TSENSOR_SAMPLE			0x60D4
> +#define KUNPENG_TSENSOR_CONTROL_EN		BIT(0)
> +
> +#define KUNPENG_TSENSOR_IS_READY(reg)		(((reg) >> 12) & 0x1)
> +#define KUNPENG_TSENSOR_IS_VALID(reg)		(((reg) >> 31) & 0x1)
> +#define KUNPENG_TSENSOR_TRIM_HIGH(reg)		(((reg) >> 15) & 0x7FF)
> +#define KUNPENG_TSENSOR_TRIM_LOW(reg)		((reg) & 0x7FF)
> +#define KUNPENG_TSENSOR_TEMP_VAL(reg)		((reg) & 0x3FF)
> +#define KUNPENG_TSENSOR_BASE_NUM(num)		(2 * (num))
> +#define KUNPENG_TSENSOR_TRIM_NUM(num)		(2 * (num) + 1)
> +
> +#define KUNPENG_TSENSOR_RD_INTVRL_US		5
> +#define KUNPENG_TSENSOR_RD_TMOUT_US		5000
> +#define KUNPENG_TSENSOR_BASIC_TMP		25000
> +#define KUNPENG_TSENSOR_BASIC_TRIM_RANGE	80000
> +
> +struct kunpeng_tsensor {
> +	void __iomem *base;
> +	void __iomem *trim_register;
> +};
> +
> +struct kunpeng_thermal_dev {
> +	u32 num_tsensors;
> +	struct kunpeng_tsensor tsensor[];
> +};
> +
> +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?

> +}
> +
> +static int kunpeng_thermal_get_temp(struct thermal_zone_device *thermal,
> +				    int *temp)
> +{
> +	struct kunpeng_thermal_dev *tdev = thermal->devdata;
> +	int tempmax = INT_MIN;
> +	u32 i, reg, tmp, trim;
> +	int ret;
> +
> +	for (i = 0; i < tdev->num_tsensors; i++) {
> +		/* Waiting for tsensor ready */
> +		ret = readl_relaxed_poll_timeout(tdev->tsensor[i].base +
> +						 KUNPENG_TSENSOR_ST, reg,
> +						 KUNPENG_TSENSOR_IS_READY(reg),
> +						 KUNPENG_TSENSOR_RD_INTVRL_US,
> +						 KUNPENG_TSENSOR_RD_TMOUT_US);
> +		if (ret) {
> +			dev_err(&thermal->device,
> +				"Tsensor%u isn't ready!\n", i);
> +			continue;
> +		}
> +
> +		/* checking if temperatures are valid */
> +		tmp = readl_relaxed(tdev->tsensor[i].base +
> +				    KUNPENG_TSENSOR_SAMPLE);
> +		if (!KUNPENG_TSENSOR_IS_VALID(tmp)) {
> +			dev_err(&thermal->device,
> +				"Tsensor%u temperature is invalid!\n", i);
> +			continue;
> +		}
> +
> +		trim = readl_relaxed(tdev->tsensor[i].trim_register);
> +
> +		ret = kunpeng_thermal_temp_correct(tmp, trim);
> +		if (ret == INT_MIN) {
> +			dev_err(&thermal->device,
> +				"Tsensor%u trim value is invalid!\n", i);
> +			continue;
> +		}
> +
> +		tempmax = max(ret, tempmax);
> +	}
> +
> +	if (tempmax == INT_MIN)
> +		return -EINVAL;
> +
> +	*temp = tempmax;
> +
> +	return 0;
> +}
> +
> +static struct thermal_zone_device_ops ops = {
> +	.get_temp	= kunpeng_thermal_get_temp,
> +};
> +
> +static int kunpeng_thermal_get_iobase(struct platform_device *pdev,
> +				      struct kunpeng_tsensor *tsensor,
> +				      u32 resource_num)
> +{
> +	struct resource *res;
> +	void __iomem *base;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, resource_num);
> +	if (!res)
> +		return -EINVAL;
> +
> +	base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +	if (IS_ERR(base))
> +		return -EINVAL;
> +
> +	if (resource_num & 1)
> +		tsensor->trim_register = base;
> +	else
> +		tsensor->base = base;
> +
> +	return 0;
> +}
> +
> +static int kunpeng_thermal_probe(struct platform_device *pdev)
> +{
> +	u32 num_tsensors = pdev->num_resources >> 1;
> +	struct thermal_zone_device *thermal_zone;
> +	struct kunpeng_thermal_dev *tdev;
> +	u32 i, reg;
> +	int ret;
> +
> +	tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev) + sizeof(*tdev->tsensor) *
> +			    num_tsensors, GFP_KERNEL);
> +	if (!tdev)
> +		return -ENOMEM;
> +
> +	tdev->num_tsensors = num_tsensors;
> +
> +	for (i = 0; i < num_tsensors; i++) {
> +		ret = kunpeng_thermal_get_iobase(pdev, &tdev->tsensor[i],
> +						 KUNPENG_TSENSOR_BASE_NUM(i));
> +		if (ret) {
> +			dev_err(&pdev->dev, "Fail to ioremap base!\n");
> +			return ret;
> +		}
> +
> +		ret = kunpeng_thermal_get_iobase(pdev, &tdev->tsensor[i],
> +						 KUNPENG_TSENSOR_TRIM_NUM(i));
> +		if (ret) {
> +			dev_err(&pdev->dev, "Fail to ioremap trim register!\n");
> +			return ret;
> +		}

I initially thought there was a bug because the function is called with
the &tsensor[i] twice, then noticed the spin in the underlying function.

It is probably better to make the code a bit more self-explicit. May be
increment 'i' by a step of 2?


> +		reg = readl_relaxed(tdev->tsensor[i].base +
> +				    KUNPENG_TSENSOR_CONTROL);
> +		writel_relaxed(reg | KUNPENG_TSENSOR_CONTROL_EN,
> +			       tdev->tsensor[i].base +
> +			       KUNPENG_TSENSOR_CONTROL);

Please add helpers with explicit function name for understanding.

> +	}
> +
> +	thermal_zone = thermal_zone_device_register("kunpeng_thermal", 0, 0,
> +						    tdev, &ops, NULL, 0, 0);
> +	if (IS_ERR(thermal_zone)) {
> +		dev_err(&pdev->dev, "Fail to register to thermal subsystem\n");
> +		return PTR_ERR(thermal_zone);
> +	}
> +
> +	platform_set_drvdata(pdev, thermal_zone);
> +
> +	return 0;
> +}
> +
> +/**
> + * kunpeng_thermal_remove() - Unregister device from thermal.
> + *
> + * This driver and IMU share tsensor devices. This function only unregister
> + * devices from thermal but never disable tsensors.

What is the IMU ?

I don't see in this driver anything related to the sensors being shared
with something else.

> + */
> +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 ?

> +	return 0;
> +}
> +
> +static const struct acpi_device_id kunpeng_thermal_acpi_match[] = {
> +	{ "HISI0371", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, kunpeng_thermal_acpi_match);
> +
> +static struct platform_driver kunpeng_thermal_driver = {
> +	.probe		= kunpeng_thermal_probe,
> +	.remove		= kunpeng_thermal_remove,
> +	.driver		= {
> +		.name	= "kunpeng_thermal",
> +		.acpi_match_table = ACPI_PTR(kunpeng_thermal_acpi_match),
> +	},
> +};
> +
> +module_platform_driver(kunpeng_thermal_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Yang Shen <[e-mail]>");
> +MODULE_DESCRIPTION("HiSilicon Kunpeng thermal driver");
> --
> 2.7.4
>

Yang Shen April 28, 2020, 11:58 a.m. UTC | #2

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.

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

>> Signed-off-by: Yang Shen <[e-mail]>
>> Signed-off-by: Zhou Wang <[e-mail]>
>> Signed-off-by: Kunshan Tang <[e-mail]>
>> ---
>>  drivers/thermal/Kconfig           |   8 ++
>>  drivers/thermal/Makefile          |   1 +
>>  drivers/thermal/kunpeng_thermal.c | 216 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 225 insertions(+)
>>  create mode 100644 drivers/thermal/kunpeng_thermal.c
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 5a05db5..7611b5d 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -239,6 +239,14 @@ config HISI_THERMAL
>>  	  thermal framework. cpufreq is used as the cooling device to throttle
>>  	  CPUs when the passive trip is crossed.
>>
>> +config KUNPENG_THERMAL
>> +	tristate "HiSilicon kunpeng thermal driver"
>> +	depends on ARM64 || COMPILE_TEST
>> +	help
>> +	  Enable this to plug HiSilicon kunpeng's thermal sensors driver into
>> +	  the Linux thermal framework, which supports to get the highest
>> +	  temperature of one Kunpeng SoC.
>> +
>>  config IMX_THERMAL
>>  	tristate "Temperature sensor driver for Freescale i.MX SoCs"
>>  	depends on ARCH_MXC || COMPILE_TEST
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index 9fb88e2..88ffca5 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -57,3 +57,4 @@ obj-$(CONFIG_GENERIC_ADC_THERMAL)	+= thermal-generic-adc.o
>>  obj-$(CONFIG_ZX2967_THERMAL)	+= zx2967_thermal.o
>>  obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
>>  obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
>> +obj-$(CONFIG_KUNPENG_THERMAL)     += kunpeng_thermal.o
>> diff --git a/drivers/thermal/kunpeng_thermal.c b/drivers/thermal/kunpeng_thermal.c
>> new file mode 100644
>> index 0000000..d22e875
>> --- /dev/null
>> +++ b/drivers/thermal/kunpeng_thermal.c
>> @@ -0,0 +1,216 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2020 HiSilicon Limited. */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/thermal.h>
>> +
>> +#define KUNPENG_TSENSOR_CONTROL			0x20D0
>> +#define KUNPENG_TSENSOR_ST			0x60D0
>> +#define KUNPENG_TSENSOR_SAMPLE			0x60D4
>> +#define KUNPENG_TSENSOR_CONTROL_EN		BIT(0)
>> +
>> +#define KUNPENG_TSENSOR_IS_READY(reg)		(((reg) >> 12) & 0x1)
>> +#define KUNPENG_TSENSOR_IS_VALID(reg)		(((reg) >> 31) & 0x1)
>> +#define KUNPENG_TSENSOR_TRIM_HIGH(reg)		(((reg) >> 15) & 0x7FF)
>> +#define KUNPENG_TSENSOR_TRIM_LOW(reg)		((reg) & 0x7FF)
>> +#define KUNPENG_TSENSOR_TEMP_VAL(reg)		((reg) & 0x3FF)
>> +#define KUNPENG_TSENSOR_BASE_NUM(num)		(2 * (num))
>> +#define KUNPENG_TSENSOR_TRIM_NUM(num)		(2 * (num) + 1)
>> +
>> +#define KUNPENG_TSENSOR_RD_INTVRL_US		5
>> +#define KUNPENG_TSENSOR_RD_TMOUT_US		5000
>> +#define KUNPENG_TSENSOR_BASIC_TMP		25000
>> +#define KUNPENG_TSENSOR_BASIC_TRIM_RANGE	80000
>> +
>> +struct kunpeng_tsensor {
>> +	void __iomem *base;
>> +	void __iomem *trim_register;
>> +};
>> +
>> +struct kunpeng_thermal_dev {
>> +	u32 num_tsensors;
>> +	struct kunpeng_tsensor tsensor[];
>> +};
>> +
>> +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.

>> +}
>> +
>> +static int kunpeng_thermal_get_temp(struct thermal_zone_device *thermal,
>> +				    int *temp)
>> +{
>> +	struct kunpeng_thermal_dev *tdev = thermal->devdata;
>> +	int tempmax = INT_MIN;
>> +	u32 i, reg, tmp, trim;
>> +	int ret;
>> +
>> +	for (i = 0; i < tdev->num_tsensors; i++) {
>> +		/* Waiting for tsensor ready */
>> +		ret = readl_relaxed_poll_timeout(tdev->tsensor[i].base +
>> +						 KUNPENG_TSENSOR_ST, reg,
>> +						 KUNPENG_TSENSOR_IS_READY(reg),
>> +						 KUNPENG_TSENSOR_RD_INTVRL_US,
>> +						 KUNPENG_TSENSOR_RD_TMOUT_US);
>> +		if (ret) {
>> +			dev_err(&thermal->device,
>> +				"Tsensor%u isn't ready!\n", i);
>> +			continue;
>> +		}
>> +
>> +		/* checking if temperatures are valid */
>> +		tmp = readl_relaxed(tdev->tsensor[i].base +
>> +				    KUNPENG_TSENSOR_SAMPLE);
>> +		if (!KUNPENG_TSENSOR_IS_VALID(tmp)) {
>> +			dev_err(&thermal->device,
>> +				"Tsensor%u temperature is invalid!\n", i);
>> +			continue;
>> +		}
>> +
>> +		trim = readl_relaxed(tdev->tsensor[i].trim_register);
>> +
>> +		ret = kunpeng_thermal_temp_correct(tmp, trim);
>> +		if (ret == INT_MIN) {
>> +			dev_err(&thermal->device,
>> +				"Tsensor%u trim value is invalid!\n", i);
>> +			continue;
>> +		}
>> +
>> +		tempmax = max(ret, tempmax);
>> +	}
>> +
>> +	if (tempmax == INT_MIN)
>> +		return -EINVAL;
>> +
>> +	*temp = tempmax;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct thermal_zone_device_ops ops = {
>> +	.get_temp	= kunpeng_thermal_get_temp,
>> +};
>> +
>> +static int kunpeng_thermal_get_iobase(struct platform_device *pdev,
>> +				      struct kunpeng_tsensor *tsensor,
>> +				      u32 resource_num)
>> +{
>> +	struct resource *res;
>> +	void __iomem *base;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, resource_num);
>> +	if (!res)
>> +		return -EINVAL;
>> +
>> +	base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>> +	if (IS_ERR(base))
>> +		return -EINVAL;
>> +
>> +	if (resource_num & 1)
>> +		tsensor->trim_register = base;
>> +	else
>> +		tsensor->base = base;
>> +
>> +	return 0;
>> +}
>> +
>> +static int kunpeng_thermal_probe(struct platform_device *pdev)
>> +{
>> +	u32 num_tsensors = pdev->num_resources >> 1;
>> +	struct thermal_zone_device *thermal_zone;
>> +	struct kunpeng_thermal_dev *tdev;
>> +	u32 i, reg;
>> +	int ret;
>> +
>> +	tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev) + sizeof(*tdev->tsensor) *
>> +			    num_tsensors, GFP_KERNEL);
>> +	if (!tdev)
>> +		return -ENOMEM;
>> +
>> +	tdev->num_tsensors = num_tsensors;
>> +
>> +	for (i = 0; i < num_tsensors; i++) {
>> +		ret = kunpeng_thermal_get_iobase(pdev, &tdev->tsensor[i],
>> +						 KUNPENG_TSENSOR_BASE_NUM(i));
>> +		if (ret) {
>> +			dev_err(&pdev->dev, "Fail to ioremap base!\n");
>> +			return ret;
>> +		}
>> +
>> +		ret = kunpeng_thermal_get_iobase(pdev, &tdev->tsensor[i],
>> +						 KUNPENG_TSENSOR_TRIM_NUM(i));
>> +		if (ret) {
>> +			dev_err(&pdev->dev, "Fail to ioremap trim register!\n");
>> +			return ret;
>> +		}
>
> I initially thought there was a bug because the function is called with
> the &tsensor[i] twice, then noticed the spin in the underlying function.
>
> It is probably better to make the code a bit more self-explicit. May be
> increment 'i' by a step of 2?
>
>

My original idea was to extract the iobase function as a separate
function, so I named it kunpeng_thermal_get_iobase.
I can modify it to be a function for initializing the iobase of a
single sensor, which might be easier to understand.

>> +		reg = readl_relaxed(tdev->tsensor[i].base +
>> +				    KUNPENG_TSENSOR_CONTROL);
>> +		writel_relaxed(reg | KUNPENG_TSENSOR_CONTROL_EN,
>> +			       tdev->tsensor[i].base +
>> +			       KUNPENG_TSENSOR_CONTROL);
>
> Please add helpers with explicit function name for understanding.
>

OK, I will fix in next version.

>> +	}
>> +
>> +	thermal_zone = thermal_zone_device_register("kunpeng_thermal", 0, 0,
>> +						    tdev, &ops, NULL, 0, 0);
>> +	if (IS_ERR(thermal_zone)) {
>> +		dev_err(&pdev->dev, "Fail to register to thermal subsystem\n");
>> +		return PTR_ERR(thermal_zone);
>> +	}
>> +
>> +	platform_set_drvdata(pdev, thermal_zone);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * kunpeng_thermal_remove() - Unregister device from thermal.
>> + *
>> + * This driver and IMU share tsensor devices. This function only unregister
>> + * devices from thermal but never disable tsensors.
>
> 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.

>> + */
>> +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?

>> +	return 0;
>> +}
>> +
>> +static const struct acpi_device_id kunpeng_thermal_acpi_match[] = {
>> +	{ "HISI0371", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, kunpeng_thermal_acpi_match);
>> +
>> +static struct platform_driver kunpeng_thermal_driver = {
>> +	.probe		= kunpeng_thermal_probe,
>> +	.remove		= kunpeng_thermal_remove,
>> +	.driver		= {
>> +		.name	= "kunpeng_thermal",
>> +		.acpi_match_table = ACPI_PTR(kunpeng_thermal_acpi_match),
>> +	},
>> +};
>> +
>> +module_platform_driver(kunpeng_thermal_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Yang Shen <[e-mail]>");
>> +MODULE_DESCRIPTION("HiSilicon Kunpeng thermal driver");
>> --
>> 2.7.4
>>
>
>

Daniel Lezcano April 28, 2020, 2:02 p.m. UTC | #3

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

Zhou Wang May 9, 2020, 7:35 a.m. UTC | #4

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

Daniel Lezcano May 10, 2020, 5:04 a.m. UTC | #5

On 09/05/2020 09:35, Zhou Wang wrote:
> 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.

Are you saying you don't care of any cooling devices from the kernel as
the IMU is taking care of it ?

Do you have a pointer to a DT where the thermal zones are defined?

[ ... ]

Zhou Wang May 11, 2020, 1:26 a.m. UTC | #6

On 2020/5/10 13:04, Daniel Lezcano wrote:
> On 09/05/2020 09:35, Zhou Wang wrote:
>> 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.
> 
> Are you saying you don't care of any cooling devices from the kernel as
> the IMU is taking care of it ?

Yes.

> 
> Do you have a pointer to a DT where the thermal zones are defined?

These sensors are in an aarch64 server system(Kunpeng920), which is based on ACPI.

Best,
Zhou

> 
> [ ... ]
> 
>

Daniel Lezcano May 11, 2020, 8:14 a.m. UTC | #7

On 11/05/2020 03:26, Zhou Wang wrote:
> On 2020/5/10 13:04, Daniel Lezcano wrote:
>> On 09/05/2020 09:35, Zhou Wang wrote:
>>> 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.
>>
>> Are you saying you don't care of any cooling devices from the kernel as
>> the IMU is taking care of it ?
> 
> Yes.
> 
>>
>> Do you have a pointer to a DT where the thermal zones are defined?
> 
> These sensors are in an aarch64 server system(Kunpeng920), which is based on ACPI.

Ah right.

What is the benefit of hiding the real hardware by taking the max value
of all the sensors and providing to the userspace a truncated view of
what is happening on the system?

Why not simply register a thermal_zone_device per sensor and get rid of
the 'max' value? So the userspace can have a correct view of the thermal
behavior of its system.

Zhou Wang May 13, 2020, 8:12 a.m. UTC | #8

On 2020/5/11 16:14, Daniel Lezcano wrote:
> On 11/05/2020 03:26, Zhou Wang wrote:
>> On 2020/5/10 13:04, Daniel Lezcano wrote:
>>> On 09/05/2020 09:35, Zhou Wang wrote:
>>>> 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.
>>>
>>> Are you saying you don't care of any cooling devices from the kernel as
>>> the IMU is taking care of it ?
>>
>> Yes.
>>
>>>
>>> Do you have a pointer to a DT where the thermal zones are defined?
>>
>> These sensors are in an aarch64 server system(Kunpeng920), which is based on ACPI.
> 
> Ah right.
> 
> What is the benefit of hiding the real hardware by taking the max value
> of all the sensors and providing to the userspace a truncated view of
> what is happening on the system?

As above discussion, we only want to show the temperature of SoCs, it
is simple and can satisfy our requirement.

As the cooling devices are fans in SoC level, exporting lower level sensors
seems not much useful in the future too.

And user can get the temperature of SoCs by the server's BMC Web interface
according to IMU. We also make temperatures to be the same value in both scenarios

Best,
Zhou

> 
> Why not simply register a thermal_zone_device per sensor and get rid of
> the 'max' value? So the userspace can have a correct view of the thermal
> behavior of its system.
> 
> 
> 
>

Amit Kucheria May 13, 2020, 12:45 p.m. UTC | #9

On Mon, May 11, 2020 at 1:44 PM Daniel Lezcano
<[e-mail]> wrote:
>
> On 11/05/2020 03:26, Zhou Wang wrote:
> > On 2020/5/10 13:04, Daniel Lezcano wrote:
> >> On 09/05/2020 09:35, Zhou Wang wrote:
> >>> 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.
> >>
> >> Are you saying you don't care of any cooling devices from the kernel as
> >> the IMU is taking care of it ?
> >
> > Yes.
> >
> >>
> >> Do you have a pointer to a DT where the thermal zones are defined?
> >
> > These sensors are in an aarch64 server system(Kunpeng920), which is based on ACPI.
>
> Ah right.
>
> What is the benefit of hiding the real hardware by taking the max value
> of all the sensors and providing to the userspace a truncated view of
> what is happening on the system?
>
> Why not simply register a thermal_zone_device per sensor and get rid of
> the 'max' value? So the userspace can have a correct view of the thermal
> behavior of its system.
>

They're simply using the thermal framework to show some temperatures
in sysfs. It seems they have no use for any of the other features of
the thermal framework, all that is handled in ACPI firmware.

I do wonder if such drivers would be better off registering a hwmon
driver instead.

Regards,
Amit

Zhou Wang May 14, 2020, 1:08 p.m. UTC | #10

On 2020/5/13 20:45, Amit Kucheria wrote:
> On Mon, May 11, 2020 at 1:44 PM Daniel Lezcano
> <[e-mail]> wrote:
>>
>> On 11/05/2020 03:26, Zhou Wang wrote:
>>> On 2020/5/10 13:04, Daniel Lezcano wrote:
>>>> On 09/05/2020 09:35, Zhou Wang wrote:
>>>>> 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.
>>>>
>>>> Are you saying you don't care of any cooling devices from the kernel as
>>>> the IMU is taking care of it ?
>>>
>>> Yes.
>>>
>>>>
>>>> Do you have a pointer to a DT where the thermal zones are defined?
>>>
>>> These sensors are in an aarch64 server system(Kunpeng920), which is based on ACPI.
>>
>> Ah right.
>>
>> What is the benefit of hiding the real hardware by taking the max value
>> of all the sensors and providing to the userspace a truncated view of
>> what is happening on the system?
>>
>> Why not simply register a thermal_zone_device per sensor and get rid of
>> the 'max' value? So the userspace can have a correct view of the thermal
>> behavior of its system.
>>
> 
> They're simply using the thermal framework to show some temperatures
> in sysfs. It seems they have no use for any of the other features of
> the thermal framework, all that is handled in ACPI firmware.

This driver is ACPI based.

> 
> I do wonder if such drivers would be better off registering a hwmon
> driver instead.

If this driver is registering as a hwmon driver, it will block possible
usages of other thermal features in the future.

So as a new driver, we would like to register to thermal subsystem :)

Best,
Zhou

> 
> Regards,
> Amit
> .
>

Daniel Lezcano May 15, 2020, 6:29 p.m. UTC | #11

On Thu, May 14, 2020 at 09:08:10PM +0800, Zhou Wang wrote:
> On 2020/5/13 20:45, Amit Kucheria wrote:
> > On Mon, May 11, 2020 at 1:44 PM Daniel Lezcano
> > <[e-mail]> wrote:

[ ... ]

> > They're simply using the thermal framework to show some temperatures
> > in sysfs. It seems they have no use for any of the other features of
> > the thermal framework, all that is handled in ACPI firmware.
> 
> This driver is ACPI based.
> 
> > 
> > I do wonder if such drivers would be better off registering a hwmon
> > driver instead.
> 
> If this driver is registering as a hwmon driver, it will block possible
> usages of other thermal features in the future.
> 
> So as a new driver, we would like to register to thermal subsystem :)

When I did the first comments, I missed the point of the ACPI aspect but the
main concern remains, especially in regard with the "features in the future".

Please, do not aggregate the sensors in the driver. Make them separate to give
the real view of the system.

The aggregation aspect is still under discussion (virtual sensor or virtual
thermal zone).

Patch

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 5a05db5..7611b5d 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -239,6 +239,14 @@  config HISI_THERMAL
 	  thermal framework. cpufreq is used as the cooling device to throttle
 	  CPUs when the passive trip is crossed.

+config KUNPENG_THERMAL
+	tristate "HiSilicon kunpeng thermal driver"
+	depends on ARM64 || COMPILE_TEST
+	help
+	  Enable this to plug HiSilicon kunpeng's thermal sensors driver into
+	  the Linux thermal framework, which supports to get the highest
+	  temperature of one Kunpeng SoC.
+
 config IMX_THERMAL
 	tristate "Temperature sensor driver for Freescale i.MX SoCs"
 	depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 9fb88e2..88ffca5 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -57,3 +57,4 @@  obj-$(CONFIG_GENERIC_ADC_THERMAL)	+= thermal-generic-adc.o
 obj-$(CONFIG_ZX2967_THERMAL)	+= zx2967_thermal.o
 obj-$(CONFIG_UNIPHIER_THERMAL)	+= uniphier_thermal.o
 obj-$(CONFIG_AMLOGIC_THERMAL)     += amlogic_thermal.o
+obj-$(CONFIG_KUNPENG_THERMAL)     += kunpeng_thermal.o
diff --git a/drivers/thermal/kunpeng_thermal.c b/drivers/thermal/kunpeng_thermal.c
new file mode 100644
index 0000000..d22e875
--- /dev/null
+++ b/drivers/thermal/kunpeng_thermal.c
@@ -0,0 +1,216 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2020 HiSilicon Limited. */
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/thermal.h>
+
+#define KUNPENG_TSENSOR_CONTROL			0x20D0
+#define KUNPENG_TSENSOR_ST			0x60D0
+#define KUNPENG_TSENSOR_SAMPLE			0x60D4
+#define KUNPENG_TSENSOR_CONTROL_EN		BIT(0)
+
+#define KUNPENG_TSENSOR_IS_READY(reg)		(((reg) >> 12) & 0x1)
+#define KUNPENG_TSENSOR_IS_VALID(reg)		(((reg) >> 31) & 0x1)
+#define KUNPENG_TSENSOR_TRIM_HIGH(reg)		(((reg) >> 15) & 0x7FF)
+#define KUNPENG_TSENSOR_TRIM_LOW(reg)		((reg) & 0x7FF)
+#define KUNPENG_TSENSOR_TEMP_VAL(reg)		((reg) & 0x3FF)
+#define KUNPENG_TSENSOR_BASE_NUM(num)		(2 * (num))
+#define KUNPENG_TSENSOR_TRIM_NUM(num)		(2 * (num) + 1)
+
+#define KUNPENG_TSENSOR_RD_INTVRL_US		5
+#define KUNPENG_TSENSOR_RD_TMOUT_US		5000
+#define KUNPENG_TSENSOR_BASIC_TMP		25000
+#define KUNPENG_TSENSOR_BASIC_TRIM_RANGE	80000
+
+struct kunpeng_tsensor {
+	void __iomem *base;
+	void __iomem *trim_register;
+};
+
+struct kunpeng_thermal_dev {
+	u32 num_tsensors;
+	struct kunpeng_tsensor tsensor[];
+};
+
+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;
+}
+
+static int kunpeng_thermal_get_temp(struct thermal_zone_device *thermal,
+				    int *temp)
+{
+	struct kunpeng_thermal_dev *tdev = thermal->devdata;
+	int tempmax = INT_MIN;
+	u32 i, reg, tmp, trim;
+	int ret;
+
+	for (i = 0; i < tdev->num_tsensors; i++) {
+		/* Waiting for tsensor ready */
+		ret = readl_relaxed_poll_timeout(tdev->tsensor[i].base +
+						 KUNPENG_TSENSOR_ST, reg,
+						 KUNPENG_TSENSOR_IS_READY(reg),
+						 KUNPENG_TSENSOR_RD_INTVRL_US,
+						 KUNPENG_TSENSOR_RD_TMOUT_US);
+		if (ret) {
+			dev_err(&thermal->device,
+				"Tsensor%u isn't ready!\n", i);
+			continue;
+		}
+
+		/* checking if temperatures are valid */
+		tmp = readl_relaxed(tdev->tsensor[i].base +
+				    KUNPENG_TSENSOR_SAMPLE);
+		if (!KUNPENG_TSENSOR_IS_VALID(tmp)) {
+			dev_err(&thermal->device,
+				"Tsensor%u temperature is invalid!\n", i);
+			continue;
+		}
+
+		trim = readl_relaxed(tdev->tsensor[i].trim_register);
+
+		ret = kunpeng_thermal_temp_correct(tmp, trim);
+		if (ret == INT_MIN) {
+			dev_err(&thermal->device,
+				"Tsensor%u trim value is invalid!\n", i);
+			continue;
+		}
+
+		tempmax = max(ret, tempmax);
+	}
+
+	if (tempmax == INT_MIN)
+		return -EINVAL;
+
+	*temp = tempmax;
+
+	return 0;
+}
+
+static struct thermal_zone_device_ops ops = {
+	.get_temp	= kunpeng_thermal_get_temp,
+};
+
+static int kunpeng_thermal_get_iobase(struct platform_device *pdev,
+				      struct kunpeng_tsensor *tsensor,
+				      u32 resource_num)
+{
+	struct resource *res;
+	void __iomem *base;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, resource_num);
+	if (!res)
+		return -EINVAL;
+
+	base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (IS_ERR(base))
+		return -EINVAL;
+
+	if (resource_num & 1)
+		tsensor->trim_register = base;
+	else
+		tsensor->base = base;
+
+	return 0;
+}
+
+static int kunpeng_thermal_probe(struct platform_device *pdev)
+{
+	u32 num_tsensors = pdev->num_resources >> 1;
+	struct thermal_zone_device *thermal_zone;
+	struct kunpeng_thermal_dev *tdev;
+	u32 i, reg;
+	int ret;
+
+	tdev = devm_kzalloc(&pdev->dev, sizeof(*tdev) + sizeof(*tdev->tsensor) *
+			    num_tsensors, GFP_KERNEL);
+	if (!tdev)
+		return -ENOMEM;
+
+	tdev->num_tsensors = num_tsensors;
+
+	for (i = 0; i < num_tsensors; i++) {
+		ret = kunpeng_thermal_get_iobase(pdev, &tdev->tsensor[i],
+						 KUNPENG_TSENSOR_BASE_NUM(i));
+		if (ret) {
+			dev_err(&pdev->dev, "Fail to ioremap base!\n");
+			return ret;
+		}
+
+		ret = kunpeng_thermal_get_iobase(pdev, &tdev->tsensor[i],
+						 KUNPENG_TSENSOR_TRIM_NUM(i));
+		if (ret) {
+			dev_err(&pdev->dev, "Fail to ioremap trim register!\n");
+			return ret;
+		}
+
+		reg = readl_relaxed(tdev->tsensor[i].base +
+				    KUNPENG_TSENSOR_CONTROL);
+		writel_relaxed(reg | KUNPENG_TSENSOR_CONTROL_EN,
+			       tdev->tsensor[i].base +
+			       KUNPENG_TSENSOR_CONTROL);
+	}
+
+	thermal_zone = thermal_zone_device_register("kunpeng_thermal", 0, 0,
+						    tdev, &ops, NULL, 0, 0);
+	if (IS_ERR(thermal_zone)) {
+		dev_err(&pdev->dev, "Fail to register to thermal subsystem\n");
+		return PTR_ERR(thermal_zone);
+	}
+
+	platform_set_drvdata(pdev, thermal_zone);
+
+	return 0;
+}
+
+/**
+ * kunpeng_thermal_remove() - Unregister device from thermal.
+ *
+ * This driver and IMU share tsensor devices. This function only unregister
+ * devices from thermal but never disable tsensors.
+ */
+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);
+
+	return 0;
+}
+
+static const struct acpi_device_id kunpeng_thermal_acpi_match[] = {
+	{ "HISI0371", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, kunpeng_thermal_acpi_match);
+
+static struct platform_driver kunpeng_thermal_driver = {
+	.probe		= kunpeng_thermal_probe,
+	.remove		= kunpeng_thermal_remove,
+	.driver		= {
+		.name	= "kunpeng_thermal",
+		.acpi_match_table = ACPI_PTR(kunpeng_thermal_acpi_match),
+	},
+};
+
+module_platform_driver(kunpeng_thermal_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Yang Shen <[e-mail]>");
+MODULE_DESCRIPTION("HiSilicon Kunpeng thermal driver");