[PATCH RESEND v4 0/2] 96boards: add thermal senor support to hikey board

[PATCH RESEND v4 0/2] 96boards: add thermal senor support to hikey board

[Xinwei Kong]

From: kongxinwei <kong.kongxinwei@hisilicon.com>

The Linaro connect introduce 96boards series in Hong Kong,The HiKey board
is the first board to be certified 96Boards Consumer Edition compatible.
This board is based on the HiSilicon SoC. you can get more information
from https://www.96boards.org.

The hisilicon SoC contains thermal module, this thermal module has 4 sensors,

	- sensor 0: local sensor;
	- sensor 1: remote sensor for ACPU cluster 1;
	- sensor 2: remote sensor for ACPU cluster 2;
	- sensor 3: remote sensor for GPU;

It can obtain this device temperature by operating this hardware. The new
sensor driver satisfies thermal framework and to realize the ACPU ,GPU and
so on to cool function.

Changes v0->v1;
* Delete this hi6220 dtsi.
* Fix documentation and error checks.
* Modify this driver which makes use of kernel to decide how to dynamically
  bind the interrupt to hottest sensor.
* Delete "sensor-thres-temp" property and read thermal_zone trips points
  replace of it.
* Delete "sensor-reset-temp" property and define the fixed value replace
  of it.

Changes v1->v2;
* change patch's position between binding document and driver file
* clean up some regiser for enabling thermal sensor
* use mutex lock to replace the spin lock

Changes v2->v3;
* delete sensor id property in binding document 
* fix sensor driver to satisfy sensor register after deleting sensor id
  property

Changes v3->v4;
* using "usleep_range" function instead of "msleep" function
* delete code detail error

kongxinwei (2):
  dt-bindings: Document the hi6220 thermal sensor bindings
  thermal: hisilicon: add new hisilicon thermal sensor driver

 .../bindings/thermal/hisilicon-thermal.txt         |  23 ++
 drivers/thermal/Kconfig                            |   8 +
 drivers/thermal/Makefile                           |   1 +
 drivers/thermal/hisi_thermal.c                     | 437 +++++++++++++++++++++
 4 files changed, 469 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
 create mode 100644 drivers/thermal/hisi_thermal.c

-- 
1.9.1

[PATCH RESEND v4 1/2] dt-bindings: Document the hi6220 thermal sensor bindings

[Xinwei Kong]

From: kongxinwei <kong.kongxinwei@hisilicon.com>

This adds documentation of device tree bindings for the
thermal sensor controller of hi6220 SoC.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Signed-off-by: kongxinwei <kong.kongxinwei@hisilicon.com>
---
 .../bindings/thermal/hisilicon-thermal.txt         | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt

diff --git a/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt b/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
new file mode 100644
index 0000000..b2a349f
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
@@ -0,0 +1,23 @@
+* Temperature Sensor on hisilicon SoCs
+
+** Required properties :
+
+- compatible: "hisilicon,tsensor".
+- reg: physical base address of thermal sensor and length of memory mapped
+  region.
+- interrupt: The interrupt number to the cpu. Defines the interrupt used
+  by SOCTHERM.
+- clock-names: Input clock name, should be 'thermal_clk'.
+- clocks: phandles for clock specified in "clock-names" property.
+- #thermal-sensor-cells: Should be 1. See ./thermal.txt for a description.
+
+Example :
+
+	tsensor: tsensor@0,f7030700 {
+		compatible = "hisilicon,tsensor";
+		reg = <0x0 0xf7030700 0x0 0x1000>;
+		interrupts = <0 7 0x4>;
+		clocks = <&clock_sys HI6220_TSENSOR_CLK>;
+		clock-names = "thermal_clk";
+		#thermal-sensor-cells = <1>;
+	}
-- 
1.9.1

[PATCH RESEND v4 2/2] thermal: hisilicon: add new hisilicon thermal sensor driver

[Xinwei Kong]

From: kongxinwei <kong.kongxinwei@hisilicon.com>

This patch adds the support for hisilicon thermal sensor, within
hisilicon SoC. there will register sensors for thermal framework
and use device tree to bind cooling device.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
Signed-off-by: kongxinwei <kong.kongxinwei@hisilicon.com>
---
 drivers/thermal/Kconfig        |   8 +
 drivers/thermal/Makefile       |   1 +
 drivers/thermal/hisi_thermal.c | 437 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 446 insertions(+)
 create mode 100644 drivers/thermal/hisi_thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index af40db0..81aee01 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -136,6 +136,14 @@ config THERMAL_EMULATION
 	  because userland can easily disable the thermal policy by simply
 	  flooding this sysfs node with low temperature values.
 
+config HISI_THERMAL
+	tristate "Hisilicon thermal driver"
+	depends on ARCH_HISI && CPU_THERMAL && OF
+	help
+	  Enable this to plug hisilicon's thermal sensor driver into the Linux
+	  thermal framework. cpufreq is used as the cooling device to throttle
+	  CPUs when the passive trip is crossed.
+
 config IMX_THERMAL
 	tristate "Temperature sensor driver for Freescale i.MX SoCs"
 	depends on CPU_THERMAL
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index fa0dc48..08ae7ac 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -39,3 +39,4 @@ obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
 obj-$(CONFIG_INT340X_THERMAL)  += int340x_thermal/
 obj-$(CONFIG_ST_THERMAL)	+= st/
 obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
+obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
new file mode 100644
index 0000000..255c71b
--- /dev/null
+++ b/drivers/thermal/hisi_thermal.c
@@ -0,0 +1,437 @@
+/*
+ * Hisilicon thermal sensor driver
+ *
+ * Copyright (c) 2014-2015 Hisilicon Limited.
+ * Copyright (c) 2014-2015 Linaro Limited.
+ *
+ * Xinwei Kong <kong.kongxinwei@hisilicon.com>
+ * Leo Yan <leo.yan@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/cpufreq.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/thermal.h>
+#include <linux/types.h>
+
+#include "thermal_core.h"
+
+#define TEMP0_TH			(0x4)
+#define TEMP0_RST_TH			(0x8)
+#define TEMP0_CFG			(0xC)
+#define TEMP0_EN			(0x10)
+#define TEMP0_INT_EN			(0x14)
+#define TEMP0_INT_CLR			(0x18)
+#define TEMP0_RST_MSK			(0x1C)
+#define TEMP0_VALUE			(0x28)
+
+#define HISI_TEMP_BASE			(-60)
+#define HISI_TEMP_RESET			(100000)
+
+#define HISI_MAX_SENSORS		4
+
+struct hisi_thermal_sensor {
+	struct hisi_thermal_data *thermal;
+	struct thermal_zone_device *tzd;
+	const struct thermal_trip *trip;
+
+	uint32_t id;
+	uint32_t thres_temp;
+	uint32_t reset_temp;
+};
+
+struct hisi_thermal_data {
+	struct mutex thermal_lock;
+	struct platform_device *pdev;
+	struct clk *clk;
+
+	int irq, irq_bind_sensor;
+	bool irq_enabled;
+
+	unsigned int sensors_num;
+	long sensor_temp[HISI_MAX_SENSORS];
+	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
+
+	void __iomem *regs;
+};
+
+/* in millicelsius */
+static inline int _step_to_temp(int step)
+{
+	/*
+	 * Every step equals (1 * 200) / 255 celsius, and finally
+	 * need convert to millicelsius.
+	 */
+	return (HISI_TEMP_BASE + (step * 200 / 255)) * 1000;
+}
+
+static inline int _temp_to_step(int temp)
+{
+	return ((temp / 1000 - HISI_TEMP_BASE) * 255 / 200);
+}
+
+static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
+					 struct hisi_thermal_sensor *sensor)
+{
+	int val;
+
+	mutex_lock(&data->thermal_lock);
+
+	/* disable interrupt */
+	writel(0x0, data->regs + TEMP0_INT_EN);
+	writel(0x1, data->regs + TEMP0_INT_CLR);
+
+	/* disable module firstly */
+	writel(0x0, data->regs + TEMP0_EN);
+
+	/* select sensor id */
+	writel((sensor->id << 12), data->regs + TEMP0_CFG);
+
+	/* enable module */
+	writel(0x1, data->regs + TEMP0_EN);
+
+	usleep_range(3000, 5000);
+
+	val = readl(data->regs + TEMP0_VALUE);
+	val = _step_to_temp(val);
+
+	/* adjust for negative value */
+	val = (val < 0) ? 0 : val;
+
+	mutex_unlock(&data->thermal_lock);
+
+	return val;
+}
+
+static void hisi_thermal_bind_irq(struct hisi_thermal_data *data)
+{
+	struct hisi_thermal_sensor *sensor;
+
+	sensor = &data->sensors[data->irq_bind_sensor];
+
+	mutex_lock(&data->thermal_lock);
+
+	/* setting the hdak time */
+	writel(0x0, data->regs + TEMP0_CFG);
+
+	/* disable module firstly */
+	writel(0x0, data->regs + TEMP0_RST_MSK);
+	writel(0x0, data->regs + TEMP0_EN);
+
+	/* select sensor id */
+	writel((sensor->id << 12), data->regs + TEMP0_CFG);
+
+	/* enable for interrupt */
+	writel(_temp_to_step(sensor->thres_temp)
+			| 0x0FFFFFF00, data->regs + TEMP0_TH);
+
+	writel(_temp_to_step(HISI_TEMP_RESET), data->regs + TEMP0_RST_TH);
+
+	/* enable module */
+	writel(0x1, data->regs + TEMP0_RST_MSK);
+	writel(0x1, data->regs + TEMP0_EN);
+
+	writel(0x0, data->regs + TEMP0_INT_CLR);
+	writel(0x1, data->regs + TEMP0_INT_EN);
+
+	usleep_range(3000, 5000);
+
+	mutex_unlock(&data->thermal_lock);
+
+}
+
+static void hisi_thermal_enable_sensor(struct hisi_thermal_data *data)
+{
+	hisi_thermal_bind_irq(data);
+}
+
+static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
+{
+
+	mutex_lock(&data->thermal_lock);
+
+	/* disable sensor module */
+	writel(0x0, data->regs + TEMP0_INT_EN);
+	writel(0x0, data->regs + TEMP0_RST_MSK);
+	writel(0x0, data->regs + TEMP0_EN);
+
+	mutex_unlock(&data->thermal_lock);
+
+}
+
+static int hisi_thermal_get_temp(void *_sensor, long *temp)
+{
+	struct hisi_thermal_sensor *sensor = _sensor;
+	struct hisi_thermal_data *data = sensor->thermal;
+
+	int sensor_id = 0, i;
+	long max_temp = 0;
+
+	*temp = hisi_thermal_get_sensor_temp(data, sensor);
+
+	data->sensor_temp[sensor->id] = *temp;
+
+	for (i = 0; i < HISI_MAX_SENSORS; i++) {
+		if (data->sensor_temp[i] >= max_temp) {
+			max_temp = data->sensor_temp[i];
+			sensor_id = i;
+		}
+	}
+
+	data->irq_bind_sensor = sensor_id;
+
+	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%ld, thres=%d\n",
+		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
+	/*
+	 * Bind irq to sensor for two cases:
+	 *   Reenable alarm IRQ if temperature below threshold;
+	 *   if irq has been enabled, always set it;
+	 */
+	if (data->irq_enabled) {
+		hisi_thermal_bind_irq(data);
+		return 0;
+	}
+
+	if (max_temp < sensor->thres_temp) {
+		data->irq_enabled = true;
+		hisi_thermal_bind_irq(data);
+		enable_irq(data->irq);
+	}
+
+	return 0;
+}
+
+static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
+	.get_temp = hisi_thermal_get_temp,
+};
+
+static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
+{
+	struct hisi_thermal_data *data = dev;
+
+	disable_irq_nosync(irq);
+	data->irq_enabled = false;
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
+{
+	struct hisi_thermal_data *data = dev;
+	struct hisi_thermal_sensor *sensor;
+	int i;
+
+	sensor = &data->sensors[data->irq_bind_sensor];
+
+	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
+		 sensor->thres_temp / 1000);
+
+	for (i = 0; i < data->sensors_num; i++)
+		thermal_zone_device_update(data->sensors[i].tzd);
+
+	return IRQ_HANDLED;
+}
+
+static int hisi_thermal_register_sensor(struct platform_device *pdev,
+					struct hisi_thermal_data *data,
+					struct hisi_thermal_sensor *sensor,
+					int index)
+{
+	int ret, i;
+
+	sensor->id = index;
+	sensor->thermal = data;
+
+	sensor->tzd = thermal_zone_of_sensor_register(&pdev->dev, sensor->id,
+				sensor, &hisi_of_thermal_ops);
+	if (IS_ERR(sensor->tzd)) {
+		ret = PTR_ERR(sensor->tzd);
+		dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
+			sensor->id, ret);
+		return ret;
+	}
+
+	sensor->trip = of_thermal_get_trip_points(sensor->tzd);
+
+	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
+		if (sensor->trip[i].type == THERMAL_TRIP_PASSIVE) {
+			sensor->thres_temp = sensor->trip[i].temperature;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id of_hisi_thermal_match[] = {
+	{ .compatible = "hisilicon,tsensor" },
+	{ /* end */ }
+};
+MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
+
+static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
+				       bool on)
+{
+	struct thermal_zone_device *tzd = sensor->tzd;
+
+	tzd->ops->set_mode(tzd,
+		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
+}
+
+static int hisi_thermal_probe(struct platform_device *pdev)
+{
+	struct hisi_thermal_data *data;
+	struct resource *res;
+	int i;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mutex_init(&data->thermal_lock);
+	data->pdev = pdev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->regs)) {
+		dev_err(&pdev->dev, "failed to get io address\n");
+		return PTR_ERR(data->regs);
+	}
+
+	data->irq = platform_get_irq(pdev, 0);
+	if (data->irq < 0)
+		return data->irq;
+
+	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
+			hisi_thermal_alarm_irq, hisi_thermal_alarm_irq_thread,
+			0, "hisi_thermal", data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, data);
+
+	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
+	if (IS_ERR(data->clk)) {
+		ret = PTR_ERR(data->clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev,
+				"failed to get thermal clk: %d\n", ret);
+		return ret;
+	}
+
+	/* enable clock for thermal */
+	ret = clk_prepare_enable(data->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
+		ret = hisi_thermal_register_sensor(pdev, data,
+					&data->sensors[i], i);
+		if (ret) {
+			dev_err(&pdev->dev,
+			"failed to register thermal sensor: %d\n", ret);
+			goto err_get_sensor_data;
+		}
+	}
+
+	hisi_thermal_enable_sensor(data);
+	data->irq_enabled = true;
+
+	for (i = 0; i < data->sensors_num; i++)
+		hisi_thermal_toggle_sensor(&data->sensors[i], true);
+
+	return 0;
+
+err_get_sensor_data:
+	clk_disable_unprepare(data->clk);
+	return ret;
+}
+
+static int hisi_thermal_remove(struct platform_device *pdev)
+{
+	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < data->sensors_num; i++) {
+		struct hisi_thermal_sensor *sensor = &data->sensors[i];
+
+		hisi_thermal_toggle_sensor(sensor, false);
+		thermal_zone_of_sensor_unregister(&pdev->dev, sensor->tzd);
+	}
+
+	hisi_thermal_disable_sensor(data);
+	clk_disable_unprepare(data->clk);
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int hisi_thermal_suspend(struct device *dev)
+{
+	struct hisi_thermal_data *data = dev_get_drvdata(dev);
+
+	hisi_thermal_disable_sensor(data);
+	data->irq_enabled = false;
+
+	clk_disable_unprepare(data->clk);
+
+	return 0;
+}
+
+static int hisi_thermal_resume(struct device *dev)
+{
+	struct hisi_thermal_data *data = dev_get_drvdata(dev);
+
+	clk_prepare_enable(data->clk);
+
+	data->irq_enabled = true;
+	hisi_thermal_enable_sensor(data);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops,
+			 hisi_thermal_suspend, hisi_thermal_resume);
+
+static struct platform_driver hisi_thermal_driver = {
+	.driver = {
+		.name	= "hisi_thermal",
+		.owner  = THIS_MODULE,
+		.pm	= &hisi_thermal_pm_ops,
+		.of_match_table = of_hisi_thermal_match,
+	},
+	.probe		= hisi_thermal_probe,
+	.remove		= hisi_thermal_remove,
+};
+
+module_platform_driver(hisi_thermal_driver);
+
+MODULE_AUTHOR("Xinwei Kong <kong.kongxinwei@hisilicon.com>");
+MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
+MODULE_DESCRIPTION("Hisilicon thermal driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH RESEND v4 2/2] thermal: hisilicon: add new hisilicon thermal sensor driver

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 16607 bytes --]

Dear Kongxinwei,


Apologize for late answer. I really missed this one. Let's make it for
the next merge window!

Please find a couple of comments as follows.


On Mon, May 04, 2015 at 10:52:20AM +0800, Xinwei Kong wrote:
> From: kongxinwei <kong.kongxinwei@hisilicon.com>
> 
> This patch adds the support for hisilicon thermal sensor, within
> hisilicon SoC. there will register sensors for thermal framework
> and use device tree to bind cooling device.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> Signed-off-by: kongxinwei <kong.kongxinwei@hisilicon.com>
> ---
>  drivers/thermal/Kconfig        |   8 +
>  drivers/thermal/Makefile       |   1 +
>  drivers/thermal/hisi_thermal.c | 437 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 446 insertions(+)
>  create mode 100644 drivers/thermal/hisi_thermal.c


Please, make sure you have a clean ./checkpatch.pl --strict output.
Today, in this version, I see the following summary:

# ./scripts/checkpatch.pl --strict /tmp/a
total: 0 errors, 2 warnings, 7 checks, 455 lines checked

/tmp/a has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

Can you please clean them up?

> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index af40db0..81aee01 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -136,6 +136,14 @@ config THERMAL_EMULATION
>  	  because userland can easily disable the thermal policy by simply
>  	  flooding this sysfs node with low temperature values.
>  
> +config HISI_THERMAL
> +	tristate "Hisilicon thermal driver"
> +	depends on ARCH_HISI && CPU_THERMAL && OF
> +	help
> +	  Enable this to plug hisilicon's thermal sensor driver into the Linux
> +	  thermal framework. cpufreq is used as the cooling device to throttle
> +	  CPUs when the passive trip is crossed.
> +
>  config IMX_THERMAL
>  	tristate "Temperature sensor driver for Freescale i.MX SoCs"
>  	depends on CPU_THERMAL
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index fa0dc48..08ae7ac 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -39,3 +39,4 @@ obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
>  obj-$(CONFIG_INT340X_THERMAL)  += int340x_thermal/
>  obj-$(CONFIG_ST_THERMAL)	+= st/
>  obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
> +obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> new file mode 100644
> index 0000000..255c71b
> --- /dev/null
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -0,0 +1,437 @@
> +/*
> + * Hisilicon thermal sensor driver
> + *
> + * Copyright (c) 2014-2015 Hisilicon Limited.
> + * Copyright (c) 2014-2015 Linaro Limited.
> + *
> + * Xinwei Kong <kong.kongxinwei@hisilicon.com>
> + * Leo Yan <leo.yan@linaro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/cpufreq.h>

I don't see why you need this header.

> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>

neither this

> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

or this

> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +#include <linux/types.h>
> +
> +#include "thermal_core.h"


Please review the above includes. Leave only those you are using.

> +
> +#define TEMP0_TH			(0x4)
> +#define TEMP0_RST_TH			(0x8)
> +#define TEMP0_CFG			(0xC)
> +#define TEMP0_EN			(0x10)
> +#define TEMP0_INT_EN			(0x14)
> +#define TEMP0_INT_CLR			(0x18)
> +#define TEMP0_RST_MSK			(0x1C)
> +#define TEMP0_VALUE			(0x28)
> +
> +#define HISI_TEMP_BASE			(-60)
> +#define HISI_TEMP_RESET			(100000)
> +
> +#define HISI_MAX_SENSORS		4
> +
> +struct hisi_thermal_sensor {
> +	struct hisi_thermal_data *thermal;
> +	struct thermal_zone_device *tzd;
> +	const struct thermal_trip *trip;
> +
> +	uint32_t id;
> +	uint32_t thres_temp;
> +	uint32_t reset_temp;

reset_temp is never used in this driver.

> +};
> +
> +struct hisi_thermal_data {
> +	struct mutex thermal_lock;
> +	struct platform_device *pdev;
> +	struct clk *clk;
> +
> +	int irq, irq_bind_sensor;
> +	bool irq_enabled;
> +
> +	unsigned int sensors_num;
> +	long sensor_temp[HISI_MAX_SENSORS];
> +	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];

Does it make sense to make sensor_temp part of hisi_thermal_sensors?

Then you would access it through sensors.sensor_temp

> +
> +	void __iomem *regs;
> +};
> +
> +/* in millicelsius */
> +static inline int _step_to_temp(int step)
> +{
> +	/*
> +	 * Every step equals (1 * 200) / 255 celsius, and finally
> +	 * need convert to millicelsius.
> +	 */
> +	return (HISI_TEMP_BASE + (step * 200 / 255)) * 1000;
> +}
> +
> +static inline int _temp_to_step(int temp)
> +{
> +	return ((temp / 1000 - HISI_TEMP_BASE) * 255 / 200);
> +}
> +
> +static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
> +					 struct hisi_thermal_sensor *sensor)
> +{
> +	int val;
> +
> +	mutex_lock(&data->thermal_lock);
> +
> +	/* disable interrupt */
> +	writel(0x0, data->regs + TEMP0_INT_EN);
> +	writel(0x1, data->regs + TEMP0_INT_CLR);
> +
> +	/* disable module firstly */
> +	writel(0x0, data->regs + TEMP0_EN);
> +
> +	/* select sensor id */
> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
> +
> +	/* enable module */
> +	writel(0x1, data->regs + TEMP0_EN);
> +
> +	usleep_range(3000, 5000);
> +
> +	val = readl(data->regs + TEMP0_VALUE);
> +	val = _step_to_temp(val);
> +
> +	/* adjust for negative value */
> +	val = (val < 0) ? 0 : val;

Why?

What does val < 0 mean here? Does it mean negative temperature or an
error?

BTW, There is ongoing work to allow, at least representing, negative
temperature.


> +
> +	mutex_unlock(&data->thermal_lock);
> +
> +	return val;
> +}
> +
> +static void hisi_thermal_bind_irq(struct hisi_thermal_data *data)
> +{
> +	struct hisi_thermal_sensor *sensor;
> +
> +	sensor = &data->sensors[data->irq_bind_sensor];
> +
> +	mutex_lock(&data->thermal_lock);
> +
> +	/* setting the hdak time */
> +	writel(0x0, data->regs + TEMP0_CFG);
> +
> +	/* disable module firstly */
> +	writel(0x0, data->regs + TEMP0_RST_MSK);
> +	writel(0x0, data->regs + TEMP0_EN);
> +

Sorry, I get confused when you tell me you need to disable the module
every time you need to read temp / change irq configuration.

Is this really required?

> +	/* select sensor id */
> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
> +
> +	/* enable for interrupt */
> +	writel(_temp_to_step(sensor->thres_temp)
> +			| 0x0FFFFFF00, data->regs + TEMP0_TH);


nit: break after the | operator.

> +
> +	writel(_temp_to_step(HISI_TEMP_RESET), data->regs + TEMP0_RST_TH);
> +
> +	/* enable module */
> +	writel(0x1, data->regs + TEMP0_RST_MSK);
> +	writel(0x1, data->regs + TEMP0_EN);
> +
> +	writel(0x0, data->regs + TEMP0_INT_CLR);
> +	writel(0x1, data->regs + TEMP0_INT_EN);
> +
> +	usleep_range(3000, 5000);
> +
> +	mutex_unlock(&data->thermal_lock);
> +
nit: remove empty line.

> +}
> +
> +static void hisi_thermal_enable_sensor(struct hisi_thermal_data *data)
> +{
> +	hisi_thermal_bind_irq(data);
> +}
> +
> +static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
> +{
> +
> +	mutex_lock(&data->thermal_lock);
> +
> +	/* disable sensor module */
> +	writel(0x0, data->regs + TEMP0_INT_EN);
> +	writel(0x0, data->regs + TEMP0_RST_MSK);
> +	writel(0x0, data->regs + TEMP0_EN);
> +
> +	mutex_unlock(&data->thermal_lock);
> +

nit: remove empty line.

> +}
> +
> +static int hisi_thermal_get_temp(void *_sensor, long *temp)
> +{
> +	struct hisi_thermal_sensor *sensor = _sensor;
> +	struct hisi_thermal_data *data = sensor->thermal;
> +
> +	int sensor_id = 0, i;
> +	long max_temp = 0;
> +
> +	*temp = hisi_thermal_get_sensor_temp(data, sensor);
> +
> +	data->sensor_temp[sensor->id] = *temp;
> +
> +	for (i = 0; i < HISI_MAX_SENSORS; i++) {
> +		if (data->sensor_temp[i] >= max_temp) {
> +			max_temp = data->sensor_temp[i];
> +			sensor_id = i;
> +		}
> +	}
> +
> +	data->irq_bind_sensor = sensor_id;

I believe this irq_bind_sensor needs to be protected by a lock.

You read it in t_irq context, write it here while getting temp, and read
it in bind irq.

I still don't follow why you need it though, can you please explain?

In any case, seams to be racy, given that it is shared by the four
sensors, but you don't lock it to use it.

> +
> +	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%ld, thres=%d\n",
> +		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
> +	/*
> +	 * Bind irq to sensor for two cases:
> +	 *   Reenable alarm IRQ if temperature below threshold;
> +	 *   if irq has been enabled, always set it;
> +	 */
> +	if (data->irq_enabled) {
> +		hisi_thermal_bind_irq(data);
> +		return 0;
> +	}
> +
> +	if (max_temp < sensor->thres_temp) {
> +		data->irq_enabled = true;
> +		hisi_thermal_bind_irq(data);
> +		enable_irq(data->irq);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
> +	.get_temp = hisi_thermal_get_temp,
> +};
> +
> +static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
> +{
> +	struct hisi_thermal_data *data = dev;
> +
> +	disable_irq_nosync(irq);
> +	data->irq_enabled = false;
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
> +{
> +	struct hisi_thermal_data *data = dev;
> +	struct hisi_thermal_sensor *sensor;
> +	int i;
> +
> +	sensor = &data->sensors[data->irq_bind_sensor];
> +
> +	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
> +		 sensor->thres_temp / 1000);

Do you really want to be printing this every time you receive an IRQ?

> +
> +	for (i = 0; i < data->sensors_num; i++)
> +		thermal_zone_device_update(data->sensors[i].tzd);


Can you please educate me on how this chip works?

You receive a thermal alarm from the chip, does it really  mean you have
to update all thermal zones? Or do you have a way to check which sensor
generated the alarm irq?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int hisi_thermal_register_sensor(struct platform_device *pdev,
> +					struct hisi_thermal_data *data,
> +					struct hisi_thermal_sensor *sensor,
> +					int index)
> +{
> +	int ret, i;
> +
> +	sensor->id = index;
> +	sensor->thermal = data;
> +
> +	sensor->tzd = thermal_zone_of_sensor_register(&pdev->dev, sensor->id,
> +				sensor, &hisi_of_thermal_ops);
> +	if (IS_ERR(sensor->tzd)) {
> +		ret = PTR_ERR(sensor->tzd);
> +		dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
> +			sensor->id, ret);
> +		return ret;
> +	}
> +
> +	sensor->trip = of_thermal_get_trip_points(sensor->tzd);
> +
> +	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
> +		if (sensor->trip[i].type == THERMAL_TRIP_PASSIVE) {

what if you have more than one passive trip? You just care about the
first?

> +			sensor->thres_temp = sensor->trip[i].temperature;

Looks like you use sensor->trip only for filling thres_temp here. Does
it make sense to use a local variable in this function, given you won't
use sensor->trip anywhere else?

> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_hisi_thermal_match[] = {
> +	{ .compatible = "hisilicon,tsensor" },
> +	{ /* end */ }
> +};
> +MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
> +
> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
> +				       bool on)
> +{
> +	struct thermal_zone_device *tzd = sensor->tzd;
> +
> +	tzd->ops->set_mode(tzd,
> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
> +}
> +
> +static int hisi_thermal_probe(struct platform_device *pdev)
> +{
> +	struct hisi_thermal_data *data;
> +	struct resource *res;
> +	int i;
> +	int ret;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	mutex_init(&data->thermal_lock);
> +	data->pdev = pdev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->regs)) {
> +		dev_err(&pdev->dev, "failed to get io address\n");
> +		return PTR_ERR(data->regs);
> +	}
> +
> +	data->irq = platform_get_irq(pdev, 0);
> +	if (data->irq < 0)
> +		return data->irq;
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
> +			hisi_thermal_alarm_irq, hisi_thermal_alarm_irq_thread,
> +			0, "hisi_thermal", data);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
> +	if (IS_ERR(data->clk)) {
> +		ret = PTR_ERR(data->clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev,
> +				"failed to get thermal clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* enable clock for thermal */
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
> +		ret = hisi_thermal_register_sensor(pdev, data,
> +					&data->sensors[i], i);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +			"failed to register thermal sensor: %d\n", ret);
> +			goto err_get_sensor_data;
> +		}
> +	}
> +
> +	hisi_thermal_enable_sensor(data);
> +	data->irq_enabled = true;
> +
> +	for (i = 0; i < data->sensors_num; i++)
> +		hisi_thermal_toggle_sensor(&data->sensors[i], true);
> +
> +	return 0;
> +
> +err_get_sensor_data:
> +	clk_disable_unprepare(data->clk);

nit: empty line here to follow your pattern.

> +	return ret;
> +}
> +
> +static int hisi_thermal_remove(struct platform_device *pdev)
> +{
> +	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < data->sensors_num; i++) {
> +		struct hisi_thermal_sensor *sensor = &data->sensors[i];
> +
> +		hisi_thermal_toggle_sensor(sensor, false);
> +		thermal_zone_of_sensor_unregister(&pdev->dev, sensor->tzd);
> +	}
> +
> +	hisi_thermal_disable_sensor(data);
> +	clk_disable_unprepare(data->clk);

nit: empty line here to follow your pattern.

> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int hisi_thermal_suspend(struct device *dev)
> +{
> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
> +
> +	hisi_thermal_disable_sensor(data);
> +	data->irq_enabled = false;
> +
> +	clk_disable_unprepare(data->clk);
> +
> +	return 0;
> +}
> +
> +static int hisi_thermal_resume(struct device *dev)
> +{
> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
> +
> +	clk_prepare_enable(data->clk);
> +
> +	data->irq_enabled = true;
> +	hisi_thermal_enable_sensor(data);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops,
> +			 hisi_thermal_suspend, hisi_thermal_resume);
> +
> +static struct platform_driver hisi_thermal_driver = {
> +	.driver = {
> +		.name	= "hisi_thermal",
> +		.owner  = THIS_MODULE,
> +		.pm	= &hisi_thermal_pm_ops,
> +		.of_match_table = of_hisi_thermal_match,


nit: align on the '=' operator.

> +	},
> +	.probe		= hisi_thermal_probe,
> +	.remove		= hisi_thermal_remove,
> +};
> +
> +module_platform_driver(hisi_thermal_driver);
> +
> +MODULE_AUTHOR("Xinwei Kong <kong.kongxinwei@hisilicon.com>");
> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
> +MODULE_DESCRIPTION("Hisilicon thermal driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.9.1
> 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH RESEND v4 0/2] 96boards: add thermal senor support to hikey board

[Eduardo Valentin]

[-- Attachment #1: Type: text/plain, Size: 2553 bytes --]

On Mon, May 04, 2015 at 10:52:18AM +0800, Xinwei Kong wrote:
> From: kongxinwei <kong.kongxinwei@hisilicon.com>
> 
> The Linaro connect introduce 96boards series in Hong Kong,The HiKey board
> is the first board to be certified 96Boards Consumer Edition compatible.
> This board is based on the HiSilicon SoC. you can get more information
> from https://www.96boards.org.
> 
> The hisilicon SoC contains thermal module, this thermal module has 4 sensors,
> 
> 	- sensor 0: local sensor;
> 	- sensor 1: remote sensor for ACPU cluster 1;
> 	- sensor 2: remote sensor for ACPU cluster 2;
> 	- sensor 3: remote sensor for GPU;
> 
> It can obtain this device temperature by operating this hardware. The new
> sensor driver satisfies thermal framework and to realize the ACPU ,GPU and
> so on to cool function.
> 
> Changes v0->v1;
> * Delete this hi6220 dtsi.
> * Fix documentation and error checks.
> * Modify this driver which makes use of kernel to decide how to dynamically
>   bind the interrupt to hottest sensor.
> * Delete "sensor-thres-temp" property and read thermal_zone trips points
>   replace of it.
> * Delete "sensor-reset-temp" property and define the fixed value replace
>   of it.
> 
> Changes v1->v2;
> * change patch's position between binding document and driver file
> * clean up some regiser for enabling thermal sensor
> * use mutex lock to replace the spin lock
> 
> Changes v2->v3;
> * delete sensor id property in binding document 
> * fix sensor driver to satisfy sensor register after deleting sensor id
>   property
> 
> Changes v3->v4;
> * using "usleep_range" function instead of "msleep" function
> * delete code detail error
> 
> kongxinwei (2):
>   dt-bindings: Document the hi6220 thermal sensor bindings
>   thermal: hisilicon: add new hisilicon thermal sensor driver
> 
>  .../bindings/thermal/hisilicon-thermal.txt         |  23 ++
>  drivers/thermal/Kconfig                            |   8 +
>  drivers/thermal/Makefile                           |   1 +
>  drivers/thermal/hisi_thermal.c                     | 437 +++++++++++++++++++++

Can you please include a patch adding the DT bindings for a board in you
next version? Even if the patches don't go in one single tree, it is
good to review all patches (DT and drivers) in one single patch series.

>  4 files changed, 469 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
>  create mode 100644 drivers/thermal/hisi_thermal.c
> 
> -- 
> 1.9.1
> 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH RESEND v4 0/2] 96boards: add thermal senor support to hikey board

[Xinwei Kong]

On 2015/5/12 9:31, Eduardo Valentin wrote:
> On Mon, May 04, 2015 at 10:52:18AM +0800, Xinwei Kong wrote:
>> From: kongxinwei <kong.kongxinwei@hisilicon.com>
>>
>> The Linaro connect introduce 96boards series in Hong Kong,The HiKey board
>> is the first board to be certified 96Boards Consumer Edition compatible.
>> This board is based on the HiSilicon SoC. you can get more information
>> from https://www.96boards.org.
>>
>> The hisilicon SoC contains thermal module, this thermal module has 4 sensors,
>>
>> 	- sensor 0: local sensor;
>> 	- sensor 1: remote sensor for ACPU cluster 1;
>> 	- sensor 2: remote sensor for ACPU cluster 2;
>> 	- sensor 3: remote sensor for GPU;
>>
>> It can obtain this device temperature by operating this hardware. The new
>> sensor driver satisfies thermal framework and to realize the ACPU ,GPU and
>> so on to cool function.
>>
>> Changes v0->v1;
>> * Delete this hi6220 dtsi.
>> * Fix documentation and error checks.
>> * Modify this driver which makes use of kernel to decide how to dynamically
>>   bind the interrupt to hottest sensor.
>> * Delete "sensor-thres-temp" property and read thermal_zone trips points
>>   replace of it.
>> * Delete "sensor-reset-temp" property and define the fixed value replace
>>   of it.
>>
>> Changes v1->v2;
>> * change patch's position between binding document and driver file
>> * clean up some regiser for enabling thermal sensor
>> * use mutex lock to replace the spin lock
>>
>> Changes v2->v3;
>> * delete sensor id property in binding document 
>> * fix sensor driver to satisfy sensor register after deleting sensor id
>>   property
>>
>> Changes v3->v4;
>> * using "usleep_range" function instead of "msleep" function
>> * delete code detail error
>>
>> kongxinwei (2):
>>   dt-bindings: Document the hi6220 thermal sensor bindings
>>   thermal: hisilicon: add new hisilicon thermal sensor driver
>>
>>  .../bindings/thermal/hisilicon-thermal.txt         |  23 ++
>>  drivers/thermal/Kconfig                            |   8 +
>>  drivers/thermal/Makefile                           |   1 +
>>  drivers/thermal/hisi_thermal.c                     | 437 +++++++++++++++++++++
> 
> Can you please include a patch adding the DT bindings for a board in you
> next version? Even if the patches don't go in one single tree, it is
> good to review all patches (DT and drivers) in one single patch series.
> 

Ok, accept it.

>>  4 files changed, 469 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/thermal/hisilicon-thermal.txt
>>  create mode 100644 drivers/thermal/hisi_thermal.c
>>
>> -- 
>> 1.9.1
>>
>>

Re: [PATCH RESEND v4 2/2] thermal: hisilicon: add new hisilicon thermal sensor driver

[Xinwei Kong]

On 2015/5/12 9:29, Eduardo Valentin wrote:
> Dear Kongxinwei,
> 
> 
> Apologize for late answer. I really missed this one. Let's make it for
> the next merge window!
> 
> Please find a couple of comments as follows.
> 
> 
> On Mon, May 04, 2015 at 10:52:20AM +0800, Xinwei Kong wrote:
>> From: kongxinwei <kong.kongxinwei@hisilicon.com>
>>
>> This patch adds the support for hisilicon thermal sensor, within
>> hisilicon SoC. there will register sensors for thermal framework
>> and use device tree to bind cooling device.
>>
>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> Signed-off-by: kongxinwei <kong.kongxinwei@hisilicon.com>
>> ---
>>  drivers/thermal/Kconfig        |   8 +
>>  drivers/thermal/Makefile       |   1 +
>>  drivers/thermal/hisi_thermal.c | 437 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 446 insertions(+)
>>  create mode 100644 drivers/thermal/hisi_thermal.c
> 
> 
> Please, make sure you have a clean ./checkpatch.pl --strict output.
> Today, in this version, I see the following summary:
> 
> # ./scripts/checkpatch.pl --strict /tmp/a
> total: 0 errors, 2 warnings, 7 checks, 455 lines checked
> 
> /tmp/a has style problems, please review.
> 
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
> 
> Can you please clean them up?
> 
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index af40db0..81aee01 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -136,6 +136,14 @@ config THERMAL_EMULATION
>>  	  because userland can easily disable the thermal policy by simply
>>  	  flooding this sysfs node with low temperature values.
>>  
>> +config HISI_THERMAL
>> +	tristate "Hisilicon thermal driver"
>> +	depends on ARCH_HISI && CPU_THERMAL && OF
>> +	help
>> +	  Enable this to plug hisilicon's thermal sensor driver into the Linux
>> +	  thermal framework. cpufreq is used as the cooling device to throttle
>> +	  CPUs when the passive trip is crossed.
>> +
>>  config IMX_THERMAL
>>  	tristate "Temperature sensor driver for Freescale i.MX SoCs"
>>  	depends on CPU_THERMAL
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index fa0dc48..08ae7ac 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -39,3 +39,4 @@ obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
>>  obj-$(CONFIG_INT340X_THERMAL)  += int340x_thermal/
>>  obj-$(CONFIG_ST_THERMAL)	+= st/
>>  obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
>> +obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>> new file mode 100644
>> index 0000000..255c71b
>> --- /dev/null
>> +++ b/drivers/thermal/hisi_thermal.c
>> @@ -0,0 +1,437 @@
>> +/*
>> + * Hisilicon thermal sensor driver
>> + *
>> + * Copyright (c) 2014-2015 Hisilicon Limited.
>> + * Copyright (c) 2014-2015 Linaro Limited.
>> + *
>> + * Xinwei Kong <kong.kongxinwei@hisilicon.com>
>> + * Leo Yan <leo.yan@linaro.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/cpufreq.h>
> 
> I don't see why you need this header.
> 
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
> 
> neither this
> 
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
> 
> or this
> 
>> +#include <linux/slab.h>
>> +#include <linux/thermal.h>
>> +#include <linux/types.h>
>> +
>> +#include "thermal_core.h"
> 
> 
> Please review the above includes. Leave only those you are using.
> 
>> +
>> +#define TEMP0_TH			(0x4)
>> +#define TEMP0_RST_TH			(0x8)
>> +#define TEMP0_CFG			(0xC)
>> +#define TEMP0_EN			(0x10)
>> +#define TEMP0_INT_EN			(0x14)
>> +#define TEMP0_INT_CLR			(0x18)
>> +#define TEMP0_RST_MSK			(0x1C)
>> +#define TEMP0_VALUE			(0x28)
>> +
>> +#define HISI_TEMP_BASE			(-60)
>> +#define HISI_TEMP_RESET			(100000)
>> +
>> +#define HISI_MAX_SENSORS		4
>> +
>> +struct hisi_thermal_sensor {
>> +	struct hisi_thermal_data *thermal;
>> +	struct thermal_zone_device *tzd;
>> +	const struct thermal_trip *trip;
>> +
>> +	uint32_t id;
>> +	uint32_t thres_temp;
>> +	uint32_t reset_temp;
> 
> reset_temp is never used in this driver.
> 
>> +};
>> +


Ok, I will fix it and pay attention to the detail format. thx.


>> +struct hisi_thermal_data {
>> +	struct mutex thermal_lock;
>> +	struct platform_device *pdev;
>> +	struct clk *clk;
>> +
>> +	int irq, irq_bind_sensor;
>> +	bool irq_enabled;
>> +
>> +	unsigned int sensors_num;
>> +	long sensor_temp[HISI_MAX_SENSORS];
>> +	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
> 
> Does it make sense to make sensor_temp part of hisi_thermal_sensors?
> 
> Then you would access it through sensors.sensor_temp
> 

I understand this way, use and test it

>> +
>> +	void __iomem *regs;
>> +};
>> +
>> +/* in millicelsius */
>> +static inline int _step_to_temp(int step)
>> +{
>> +	/*
>> +	 * Every step equals (1 * 200) / 255 celsius, and finally
>> +	 * need convert to millicelsius.
>> +	 */
>> +	return (HISI_TEMP_BASE + (step * 200 / 255)) * 1000;
>> +}
>> +
>> +static inline int _temp_to_step(int temp)
>> +{
>> +	return ((temp / 1000 - HISI_TEMP_BASE) * 255 / 200);
>> +}
>> +
>> +static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
>> +					 struct hisi_thermal_sensor *sensor)
>> +{
>> +	int val;
>> +
>> +	mutex_lock(&data->thermal_lock);
>> +
>> +	/* disable interrupt */
>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>> +	writel(0x1, data->regs + TEMP0_INT_CLR);
>> +
>> +	/* disable module firstly */
>> +	writel(0x0, data->regs + TEMP0_EN);
>> +
>> +	/* select sensor id */
>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>> +
>> +	/* enable module */
>> +	writel(0x1, data->regs + TEMP0_EN);
>> +
>> +	usleep_range(3000, 5000);
>> +
>> +	val = readl(data->regs + TEMP0_VALUE);
>> +	val = _step_to_temp(val);
>> +
>> +	/* adjust for negative value */
>> +	val = (val < 0) ? 0 : val;
> 
> Why?
> 
> What does val < 0 mean here? Does it mean negative temperature or an
> error?
> 
> BTW, There is ongoing work to allow, at least representing, negative
> temperature.
> 
 consult hardware engineer to get some information which temperature range
is -40 ~ +160.  I will deal it in the next version.

> 
>> +
>> +	mutex_unlock(&data->thermal_lock);
>> +
>> +	return val;
>> +}
>> +
>> +static void hisi_thermal_bind_irq(struct hisi_thermal_data *data)
>> +{
>> +	struct hisi_thermal_sensor *sensor;
>> +
>> +	sensor = &data->sensors[data->irq_bind_sensor];
>> +
>> +	mutex_lock(&data->thermal_lock);
>> +
>> +	/* setting the hdak time */
>> +	writel(0x0, data->regs + TEMP0_CFG);
>> +
>> +	/* disable module firstly */
>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>> +	writel(0x0, data->regs + TEMP0_EN);
>> +
> 
> Sorry, I get confused when you tell me you need to disable the module
> every time you need to read temp / change irq configuration.
> 
> Is this really required?
> 

yes. Soc have four sensor. They share a series of register. If not disabling
this module, the systerm will possible produce some problem.

For example, when setting the CPU0 sensor thres_temp is 50 degree. I will plan
to set and check the GPU sensor temp, (but the gpu sensor thres_temp require
70 degree). However, GPU sensor temp is 60 degree in fact. Before change
data->regs + TEMP0_TH register, the GPU sensor will produce interrupt under
50 degree. Because GPU and CPU0 share the TEMP0_TH register, so i will disable
this module.

>> +	/* select sensor id */
>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>> +
>> +	/* enable for interrupt */
>> +	writel(_temp_to_step(sensor->thres_temp)
>> +			| 0x0FFFFFF00, data->regs + TEMP0_TH);
> 
> 
> nit: break after the | operator.
> 
>> +
>> +	writel(_temp_to_step(HISI_TEMP_RESET), data->regs + TEMP0_RST_TH);
>> +
>> +	/* enable module */
>> +	writel(0x1, data->regs + TEMP0_RST_MSK);
>> +	writel(0x1, data->regs + TEMP0_EN);
>> +
>> +	writel(0x0, data->regs + TEMP0_INT_CLR);
>> +	writel(0x1, data->regs + TEMP0_INT_EN);
>> +
>> +	usleep_range(3000, 5000);
>> +
>> +	mutex_unlock(&data->thermal_lock);
>> +
> nit: remove empty line.
> 
>> +}
>> +
>> +static void hisi_thermal_enable_sensor(struct hisi_thermal_data *data)
>> +{
>> +	hisi_thermal_bind_irq(data);
>> +}
>> +
>> +static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
>> +{
>> +
>> +	mutex_lock(&data->thermal_lock);
>> +
>> +	/* disable sensor module */
>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>> +	writel(0x0, data->regs + TEMP0_EN);
>> +
>> +	mutex_unlock(&data->thermal_lock);
>> +
> 
> nit: remove empty line.

ok. good comment

> 
>> +}
>> +
>> +static int hisi_thermal_get_temp(void *_sensor, long *temp)
>> +{
>> +	struct hisi_thermal_sensor *sensor = _sensor;
>> +	struct hisi_thermal_data *data = sensor->thermal;
>> +
>> +	int sensor_id = 0, i;
>> +	long max_temp = 0;
>> +
>> +	*temp = hisi_thermal_get_sensor_temp(data, sensor);
>> +
>> +	data->sensor_temp[sensor->id] = *temp;
>> +
>> +	for (i = 0; i < HISI_MAX_SENSORS; i++) {
>> +		if (data->sensor_temp[i] >= max_temp) {
>> +			max_temp = data->sensor_temp[i];
>> +			sensor_id = i;
>> +		}
>> +	}
>> +
>> +	data->irq_bind_sensor = sensor_id;
> 
> I believe this irq_bind_sensor needs to be protected by a lock.
> 
> You read it in t_irq context, write it here while getting temp, and read
> it in bind irq.
> 
> I still don't follow why you need it though, can you please explain?
> 
> In any case, seams to be racy, given that it is shared by the four
> sensors, but you don't lock it to use it.
> 
>> +
>> +	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%ld, thres=%d\n",
>> +		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
>> +	/*
>> +	 * Bind irq to sensor for two cases:
>> +	 *   Reenable alarm IRQ if temperature below threshold;
>> +	 *   if irq has been enabled, always set it;
>> +	 */
>> +	if (data->irq_enabled) {
>> +		hisi_thermal_bind_irq(data);
>> +		return 0;
>> +	}
>> +
>> +	if (max_temp < sensor->thres_temp) {
>> +		data->irq_enabled = true;
>> +		hisi_thermal_bind_irq(data);
>> +		enable_irq(data->irq);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
>> +	.get_temp = hisi_thermal_get_temp,
>> +};
>> +
>> +static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev;
>> +
>> +	disable_irq_nosync(irq);
>> +	data->irq_enabled = false;
>> +
>> +	return IRQ_WAKE_THREAD;
>> +}
>> +
>> +static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev;
>> +	struct hisi_thermal_sensor *sensor;
>> +	int i;
>> +
>> +	sensor = &data->sensors[data->irq_bind_sensor];
>> +
>> +	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
>> +		 sensor->thres_temp / 1000);
> 
> Do you really want to be printing this every time you receive an IRQ?
> 
>> +
>> +	for (i = 0; i < data->sensors_num; i++)
>> +		thermal_zone_device_update(data->sensors[i].tzd);
> 
> 
> Can you please educate me on how this chip works?
> 
> You receive a thermal alarm from the chip, does it really  mean you have
> to update all thermal zones? Or do you have a way to check which sensor
> generated the alarm irq?
> 
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int hisi_thermal_register_sensor(struct platform_device *pdev,
>> +					struct hisi_thermal_data *data,
>> +					struct hisi_thermal_sensor *sensor,
>> +					int index)
>> +{
>> +	int ret, i;
>> +
>> +	sensor->id = index;
>> +	sensor->thermal = data;
>> +
>> +	sensor->tzd = thermal_zone_of_sensor_register(&pdev->dev, sensor->id,
>> +				sensor, &hisi_of_thermal_ops);
>> +	if (IS_ERR(sensor->tzd)) {
>> +		ret = PTR_ERR(sensor->tzd);
>> +		dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
>> +			sensor->id, ret);
>> +		return ret;
>> +	}
>> +
>> +	sensor->trip = of_thermal_get_trip_points(sensor->tzd);
>> +
>> +	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
>> +		if (sensor->trip[i].type == THERMAL_TRIP_PASSIVE) {
> 
> what if you have more than one passive trip? You just care about the
> first?
> 
>> +			sensor->thres_temp = sensor->trip[i].temperature;
> 
> Looks like you use sensor->trip only for filling thres_temp here. Does
> it make sense to use a local variable in this function, given you won't
> use sensor->trip anywhere else?
> 
>> +			break;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id of_hisi_thermal_match[] = {
>> +	{ .compatible = "hisilicon,tsensor" },
>> +	{ /* end */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>> +
>> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>> +				       bool on)
>> +{
>> +	struct thermal_zone_device *tzd = sensor->tzd;
>> +
>> +	tzd->ops->set_mode(tzd,
>> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>> +}
>> +
>> +static int hisi_thermal_probe(struct platform_device *pdev)
>> +{
>> +	struct hisi_thermal_data *data;
>> +	struct resource *res;
>> +	int i;
>> +	int ret;
>> +
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&data->thermal_lock);
>> +	data->pdev = pdev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(data->regs)) {
>> +		dev_err(&pdev->dev, "failed to get io address\n");
>> +		return PTR_ERR(data->regs);
>> +	}
>> +
>> +	data->irq = platform_get_irq(pdev, 0);
>> +	if (data->irq < 0)
>> +		return data->irq;
>> +
>> +	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>> +			hisi_thermal_alarm_irq, hisi_thermal_alarm_irq_thread,
>> +			0, "hisi_thermal", data);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, data);
>> +
>> +	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
>> +	if (IS_ERR(data->clk)) {
>> +		ret = PTR_ERR(data->clk);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(&pdev->dev,
>> +				"failed to get thermal clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* enable clock for thermal */
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
>> +		ret = hisi_thermal_register_sensor(pdev, data,
>> +					&data->sensors[i], i);
>> +		if (ret) {
>> +			dev_err(&pdev->dev,
>> +			"failed to register thermal sensor: %d\n", ret);
>> +			goto err_get_sensor_data;
>> +		}
>> +	}
>> +
>> +	hisi_thermal_enable_sensor(data);
>> +	data->irq_enabled = true;
>> +
>> +	for (i = 0; i < data->sensors_num; i++)
>> +		hisi_thermal_toggle_sensor(&data->sensors[i], true);
>> +
>> +	return 0;
>> +
>> +err_get_sensor_data:
>> +	clk_disable_unprepare(data->clk);
> 
> nit: empty line here to follow your pattern.
> 
>> +	return ret;
>> +}
>> +
>> +static int hisi_thermal_remove(struct platform_device *pdev)
>> +{
>> +	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
>> +	int i;
>> +
>> +	for (i = 0; i < data->sensors_num; i++) {
>> +		struct hisi_thermal_sensor *sensor = &data->sensors[i];
>> +
>> +		hisi_thermal_toggle_sensor(sensor, false);
>> +		thermal_zone_of_sensor_unregister(&pdev->dev, sensor->tzd);
>> +	}
>> +
>> +	hisi_thermal_disable_sensor(data);
>> +	clk_disable_unprepare(data->clk);
> 
> nit: empty line here to follow your pattern.
> 
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int hisi_thermal_suspend(struct device *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>> +
>> +	hisi_thermal_disable_sensor(data);
>> +	data->irq_enabled = false;
>> +
>> +	clk_disable_unprepare(data->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hisi_thermal_resume(struct device *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>> +
>> +	clk_prepare_enable(data->clk);
>> +
>> +	data->irq_enabled = true;
>> +	hisi_thermal_enable_sensor(data);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops,
>> +			 hisi_thermal_suspend, hisi_thermal_resume);
>> +
>> +static struct platform_driver hisi_thermal_driver = {
>> +	.driver = {
>> +		.name	= "hisi_thermal",
>> +		.owner  = THIS_MODULE,
>> +		.pm	= &hisi_thermal_pm_ops,
>> +		.of_match_table = of_hisi_thermal_match,
> 
> 
> nit: align on the '=' operator.
> 
>> +	},
>> +	.probe		= hisi_thermal_probe,
>> +	.remove		= hisi_thermal_remove,
>> +};
>> +
>> +module_platform_driver(hisi_thermal_driver);
>> +
>> +MODULE_AUTHOR("Xinwei Kong <kong.kongxinwei@hisilicon.com>");
>> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
>> +MODULE_DESCRIPTION("Hisilicon thermal driver");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 1.9.1
>>
>>

Re: [PATCH RESEND v4 2/2] thermal: hisilicon: add new hisilicon thermal sensor driver

[Xinwei Kong]

On 2015/5/12 9:29, Eduardo Valentin wrote:
> Dear Kongxinwei,
> 
> 
> Apologize for late answer. I really missed this one. Let's make it for
> the next merge window!
> 
> Please find a couple of comments as follows.
> 
> 
> On Mon, May 04, 2015 at 10:52:20AM +0800, Xinwei Kong wrote:
>> From: kongxinwei <kong.kongxinwei-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
>>
>> This patch adds the support for hisilicon thermal sensor, within
>> hisilicon SoC. there will register sensors for thermal framework
>> and use device tree to bind cooling device.
>>
>> +
>> +static void hisi_thermal_enable_sensor(struct hisi_thermal_data *data)
>> +{
>> +	hisi_thermal_bind_irq(data);
>> +}
>> +
>> +static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
>> +{
>> +
>> +	mutex_lock(&data->thermal_lock);
>> +
>> +	/* disable sensor module */
>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>> +	writel(0x0, data->regs + TEMP0_EN);
>> +
>> +	mutex_unlock(&data->thermal_lock);
>> +
> 
> nit: remove empty line.
> 
>> +}
>> +
>> +static int hisi_thermal_get_temp(void *_sensor, long *temp)
>> +{
>> +	struct hisi_thermal_sensor *sensor = _sensor;
>> +	struct hisi_thermal_data *data = sensor->thermal;
>> +
>> +	int sensor_id = 0, i;
>> +	long max_temp = 0;
>> +
>> +	*temp = hisi_thermal_get_sensor_temp(data, sensor);
>> +
>> +	data->sensor_temp[sensor->id] = *temp;
>> +
>> +	for (i = 0; i < HISI_MAX_SENSORS; i++) {
>> +		if (data->sensor_temp[i] >= max_temp) {
>> +			max_temp = data->sensor_temp[i];
>> +			sensor_id = i;
>> +		}
>> +	}
>> +
>> +	data->irq_bind_sensor = sensor_id;
> 
> I believe this irq_bind_sensor needs to be protected by a lock.
> 
> You read it in t_irq context, write it here while getting temp, and read
> it in bind irq.

OK, add mutex lock to protected this variable.

> 
> I still don't follow why you need it though, can you please explain?

The thermal sensor module has four sensors (local、acpu0、acpu1、gpu),
Four sensor share or use the same register. let diff sensor ip enable
by setting diff bit for the same register. For the interrupt it has
only one interrupt signal; This interrupt signal can only be used by
one sensor;

For example, Four sensor can generate interrupt signal by selecting sensor
which use  "writel((sensor->id << 12), data->regs + TEMP0_CFG)" , when
using sensor->id value to enable diff sensor, the enabled sensor maybe
generate the interrupt signal while other three sensor don't generate the
interrupt signal.

Four sensor ip distribute diff situation in the Soc. We will use kernel to
decide the interrupt signal binding one of four sensor. However, the binding
sensor will be close to interrupt status. the variable "irq_bind_sensor" will
record this binding sensor id.

> 
> In any case, seams to be racy, given that it is shared by the four
> sensors, but you don't lock it to use it.
> 
>> +
>> +	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%ld, thres=%d\n",
>> +		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
>> +	/*
>> +	 * Bind irq to sensor for two cases:
>> +	 *   Reenable alarm IRQ if temperature below threshold;
>> +	 *   if irq has been enabled, always set it;
>> +	 */
>> +	if (data->irq_enabled) {
>> +		hisi_thermal_bind_irq(data);
>> +		return 0;
>> +	}
>> +
>> +	if (max_temp < sensor->thres_temp) {
>> +		data->irq_enabled = true;
>> +		hisi_thermal_bind_irq(data);
>> +		enable_irq(data->irq);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
>> +	.get_temp = hisi_thermal_get_temp,
>> +};
>> +
>> +static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev;
>> +
>> +	disable_irq_nosync(irq);
>> +	data->irq_enabled = false;
>> +
>> +	return IRQ_WAKE_THREAD;
>> +}
>> +
>> +static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev;
>> +	struct hisi_thermal_sensor *sensor;
>> +	int i;
>> +
>> +	sensor = &data->sensors[data->irq_bind_sensor];
>> +
>> +	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
>> +		 sensor->thres_temp / 1000);
> 
> Do you really want to be printing this every time you receive an IRQ?

This print information will display some sensor which generate the interrupt
signal. If deleting it,We don't know which sensor generate interrupt signal.

Generating interruption frequency is low. When the temperature of binding
sensor is higher than "thres_temp" value, this will print it. But in the thermal
framework it will get temperature by polling sensor, then cooling this device.
During polling time the sensor temperature is higher than "thres_temp" value,it
can generate interrupt signal. The interrupt dealing is helpful to the polling
machine.

> 
>> +
>> +	for (i = 0; i < data->sensors_num; i++)
>> +		thermal_zone_device_update(data->sensors[i].tzd);
> 
> 
> Can you please educate me on how this chip works?

This chip have four sensors, every time you can only use one sensor by selecting
sensor register. At the same time one sensor is regular work, you require to
set the same register for the diff bit.

> 
> You receive a thermal alarm from the chip, does it really  mean you have
> to update all thermal zones? Or do you have a way to check which sensor
> generated the alarm irq?
> 

If one sensor generate interrupt signal, it will update all thermal zones.

>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int hisi_thermal_register_sensor(struct platform_device *pdev,
>> +					struct hisi_thermal_data *data,
>> +					struct hisi_thermal_sensor *sensor,
>> +					int index)
>> +{
>> +	int ret, i;
>> +
>> +	sensor->id = index;
>> +	sensor->thermal = data;
>> +
>> +	sensor->tzd = thermal_zone_of_sensor_register(&pdev->dev, sensor->id,
>> +				sensor, &hisi_of_thermal_ops);
>> +	if (IS_ERR(sensor->tzd)) {
>> +		ret = PTR_ERR(sensor->tzd);
>> +		dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
>> +			sensor->id, ret);
>> +		return ret;
>> +	}
>> +
>> +	sensor->trip = of_thermal_get_trip_points(sensor->tzd);
>> +
>> +	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
>> +		if (sensor->trip[i].type == THERMAL_TRIP_PASSIVE) {
> 
> what if you have more than one passive trip? You just care about the
> first?
> 

Yes, this "passive" type have satisfied the demands for our Soc.

>> +			sensor->thres_temp = sensor->trip[i].temperature;
> 
> Looks like you use sensor->trip only for filling thres_temp here. Does
> it make sense to use a local variable in this function, given you won't
> use sensor->trip anywhere else?
> 
good idea. I understand it. I remove this "const struct thermal_trip *trip;"
from " struct hisi_thermal_sensor". Then use it in this function.

>> +			break;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id of_hisi_thermal_match[] = {
>> +	{ .compatible = "hisilicon,tsensor" },
>> +	{ /* end */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>> +
>> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>> +				       bool on)
>> +{
>> +	struct thermal_zone_device *tzd = sensor->tzd;
>> +
>> +	tzd->ops->set_mode(tzd,
>> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>> +}
>> +
>> +static int hisi_thermal_probe(struct platform_device *pdev)
>> +{
>> +	struct hisi_thermal_data *data;
>> +	struct resource *res;
>> +	int i;
>> +	int ret;
>> +
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&data->thermal_lock);
>> +	data->pdev = pdev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(data->regs)) {
>> +		dev_err(&pdev->dev, "failed to get io address\n");
>> +		return PTR_ERR(data->regs);
>> +	}
>> +
>> +	data->irq = platform_get_irq(pdev, 0);
>> +	if (data->irq < 0)
>> +		return data->irq;
>> +
>> +	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>> +			hisi_thermal_alarm_irq, hisi_thermal_alarm_irq_thread,
>> +			0, "hisi_thermal", data);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, data);
>> +
>> +	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
>> +	if (IS_ERR(data->clk)) {
>> +		ret = PTR_ERR(data->clk);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(&pdev->dev,
>> +				"failed to get thermal clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* enable clock for thermal */
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
>> +		ret = hisi_thermal_register_sensor(pdev, data,
>> +					&data->sensors[i], i);
>> +		if (ret) {
>> +			dev_err(&pdev->dev,
>> +			"failed to register thermal sensor: %d\n", ret);
>> +			goto err_get_sensor_data;
>> +		}
>> +	}
>> +
>> +	hisi_thermal_enable_sensor(data);
>> +	data->irq_enabled = true;
>> +
>> +	for (i = 0; i < data->sensors_num; i++)
>> +		hisi_thermal_toggle_sensor(&data->sensors[i], true);
>> +
>> +	return 0;
>> +
>> +err_get_sensor_data:
>> +	clk_disable_unprepare(data->clk);
> 
> nit: empty line here to follow your pattern.
> 
>> +	return ret;
>> +}
>> +
>> +static int hisi_thermal_remove(struct platform_device *pdev)
>> +{
>> +	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
>> +	int i;
>> +
>> +	for (i = 0; i < data->sensors_num; i++) {
>> +		struct hisi_thermal_sensor *sensor = &data->sensors[i];
>> +
>> +		hisi_thermal_toggle_sensor(sensor, false);
>> +		thermal_zone_of_sensor_unregister(&pdev->dev, sensor->tzd);
>> +	}
>> +
>> +	hisi_thermal_disable_sensor(data);
>> +	clk_disable_unprepare(data->clk);
> 
> nit: empty line here to follow your pattern.
> 
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int hisi_thermal_suspend(struct device *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>> +
>> +	hisi_thermal_disable_sensor(data);
>> +	data->irq_enabled = false;
>> +
>> +	clk_disable_unprepare(data->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hisi_thermal_resume(struct device *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>> +
>> +	clk_prepare_enable(data->clk);
>> +
>> +	data->irq_enabled = true;
>> +	hisi_thermal_enable_sensor(data);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops,
>> +			 hisi_thermal_suspend, hisi_thermal_resume);
>> +
>> +static struct platform_driver hisi_thermal_driver = {
>> +	.driver = {
>> +		.name	= "hisi_thermal",
>> +		.owner  = THIS_MODULE,
>> +		.pm	= &hisi_thermal_pm_ops,
>> +		.of_match_table = of_hisi_thermal_match,
> 
> 
> nit: align on the '=' operator.
> 
>> +	},
>> +	.probe		= hisi_thermal_probe,
>> +	.remove		= hisi_thermal_remove,
>> +};
>> +
>> +module_platform_driver(hisi_thermal_driver);
>> +
>> +MODULE_AUTHOR("Xinwei Kong <kong.kongxinwei-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>");
>> +MODULE_AUTHOR("Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>");
>> +MODULE_DESCRIPTION("Hisilicon thermal driver");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 1.9.1
>>
>>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND v4 2/2] thermal: hisilicon: add new hisilicon thermal sensor driver

[Xinwei Kong]

hi Eduardo,

this following review log is from our min systerm patchset. this thermal
sensor driver is important part for 96boards. If our thermal sensor can't
be merged into kernel and the min systerm be merged. the board is not safe
for using man (causing fire). So i wish you can accept this patchset before
the min systerm patchset is accepted.

thanks
Xinwei


On 2015/5/7 17:02, Will Deacon wrote:> Hi Bintian,
>
> On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
>> PSCI is enabled in device tree and there is no problem to boot all the
>> octal cores, and the CPU hotplug is also working now, you can download
>> and compile the latest firmware based on the following link to run this
>> patch set:
>> https://github.com/96boards/documentation/wiki/UEFI
>>
>> Changes v4:
>> * Rebase to kernel 4.1-rc1
>> * Delete "arm,cortex-a15-gic" from the gic node in dts
>
> I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
> above.
>
> The good news is that the thing booted and all the cores entered at EL2.
> Thanks!
>
> The bad news is that running hackbench quickly got the *heatsink*
> temperature to 73 degress C and rising (measured with an infrared
> thermometer).
>
> So my question is, does this SoC have an automatic thermal cut out? Whilst
> I'm all for merging enabling code into the kernel, if it really relies on
> the kernel to stop it from catching fire, maybe it's not a great idea
> putting these patches into people's hands just yet.
>
> Will
>
> .
>

On 2015/5/12 9:29, Eduardo Valentin wrote:
> Dear Kongxinwei,
> 
> 
> Apologize for late answer. I really missed this one. Let's make it for
> the next merge window!
> 
> Please find a couple of comments as follows.
> 
> 
> On Mon, May 04, 2015 at 10:52:20AM +0800, Xinwei Kong wrote:
>> From: kongxinwei <kong.kongxinwei@hisilicon.com>
>>
>> This patch adds the support for hisilicon thermal sensor, within
>> hisilicon SoC. there will register sensors for thermal framework
>> and use device tree to bind cooling device.
>>
>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> Signed-off-by: kongxinwei <kong.kongxinwei@hisilicon.com>
>> ---
>>  drivers/thermal/Kconfig        |   8 +
>>  drivers/thermal/Makefile       |   1 +
>>  drivers/thermal/hisi_thermal.c | 437 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 446 insertions(+)
>>  create mode 100644 drivers/thermal/hisi_thermal.c
> 
> 
> Please, make sure you have a clean ./checkpatch.pl --strict output.
> Today, in this version, I see the following summary:
> 
> # ./scripts/checkpatch.pl --strict /tmp/a
> total: 0 errors, 2 warnings, 7 checks, 455 lines checked
> 
> /tmp/a has style problems, please review.
> 
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
> 
> Can you please clean them up?
> 
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index af40db0..81aee01 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -136,6 +136,14 @@ config THERMAL_EMULATION
>>  	  because userland can easily disable the thermal policy by simply
>>  	  flooding this sysfs node with low temperature values.
>>  
>> +config HISI_THERMAL
>> +	tristate "Hisilicon thermal driver"
>> +	depends on ARCH_HISI && CPU_THERMAL && OF
>> +	help
>> +	  Enable this to plug hisilicon's thermal sensor driver into the Linux
>> +	  thermal framework. cpufreq is used as the cooling device to throttle
>> +	  CPUs when the passive trip is crossed.
>> +
>>  config IMX_THERMAL
>>  	tristate "Temperature sensor driver for Freescale i.MX SoCs"
>>  	depends on CPU_THERMAL
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index fa0dc48..08ae7ac 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -39,3 +39,4 @@ obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
>>  obj-$(CONFIG_INT340X_THERMAL)  += int340x_thermal/
>>  obj-$(CONFIG_ST_THERMAL)	+= st/
>>  obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
>> +obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>> new file mode 100644
>> index 0000000..255c71b
>> --- /dev/null
>> +++ b/drivers/thermal/hisi_thermal.c
>> @@ -0,0 +1,437 @@
>> +/*
>> + * Hisilicon thermal sensor driver
>> + *
>> + * Copyright (c) 2014-2015 Hisilicon Limited.
>> + * Copyright (c) 2014-2015 Linaro Limited.
>> + *
>> + * Xinwei Kong <kong.kongxinwei@hisilicon.com>
>> + * Leo Yan <leo.yan@linaro.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/cpufreq.h>
> 
> I don't see why you need this header.
> 
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
> 
> neither this
> 
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
> 
> or this
> 
>> +#include <linux/slab.h>
>> +#include <linux/thermal.h>
>> +#include <linux/types.h>
>> +
>> +#include "thermal_core.h"
> 
> 
> Please review the above includes. Leave only those you are using.
> 
>> +
>> +#define TEMP0_TH			(0x4)
>> +#define TEMP0_RST_TH			(0x8)
>> +#define TEMP0_CFG			(0xC)
>> +#define TEMP0_EN			(0x10)
>> +#define TEMP0_INT_EN			(0x14)
>> +#define TEMP0_INT_CLR			(0x18)
>> +#define TEMP0_RST_MSK			(0x1C)
>> +#define TEMP0_VALUE			(0x28)
>> +
>> +#define HISI_TEMP_BASE			(-60)
>> +#define HISI_TEMP_RESET			(100000)
>> +
>> +#define HISI_MAX_SENSORS		4
>> +
>> +struct hisi_thermal_sensor {
>> +	struct hisi_thermal_data *thermal;
>> +	struct thermal_zone_device *tzd;
>> +	const struct thermal_trip *trip;
>> +
>> +	uint32_t id;
>> +	uint32_t thres_temp;
>> +	uint32_t reset_temp;
> 
> reset_temp is never used in this driver.
> 
>> +};
>> +
>> +struct hisi_thermal_data {
>> +	struct mutex thermal_lock;
>> +	struct platform_device *pdev;
>> +	struct clk *clk;
>> +
>> +	int irq, irq_bind_sensor;
>> +	bool irq_enabled;
>> +
>> +	unsigned int sensors_num;
>> +	long sensor_temp[HISI_MAX_SENSORS];
>> +	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
> 
> Does it make sense to make sensor_temp part of hisi_thermal_sensors?
> 
> Then you would access it through sensors.sensor_temp
> 
>> +
>> +	void __iomem *regs;
>> +};
>> +
>> +/* in millicelsius */
>> +static inline int _step_to_temp(int step)
>> +{
>> +	/*
>> +	 * Every step equals (1 * 200) / 255 celsius, and finally
>> +	 * need convert to millicelsius.
>> +	 */
>> +	return (HISI_TEMP_BASE + (step * 200 / 255)) * 1000;
>> +}
>> +
>> +static inline int _temp_to_step(int temp)
>> +{
>> +	return ((temp / 1000 - HISI_TEMP_BASE) * 255 / 200);
>> +}
>> +
>> +static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
>> +					 struct hisi_thermal_sensor *sensor)
>> +{
>> +	int val;
>> +
>> +	mutex_lock(&data->thermal_lock);
>> +
>> +	/* disable interrupt */
>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>> +	writel(0x1, data->regs + TEMP0_INT_CLR);
>> +
>> +	/* disable module firstly */
>> +	writel(0x0, data->regs + TEMP0_EN);
>> +
>> +	/* select sensor id */
>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>> +
>> +	/* enable module */
>> +	writel(0x1, data->regs + TEMP0_EN);
>> +
>> +	usleep_range(3000, 5000);
>> +
>> +	val = readl(data->regs + TEMP0_VALUE);
>> +	val = _step_to_temp(val);
>> +
>> +	/* adjust for negative value */
>> +	val = (val < 0) ? 0 : val;
> 
> Why?
> 
> What does val < 0 mean here? Does it mean negative temperature or an
> error?
> 
> BTW, There is ongoing work to allow, at least representing, negative
> temperature.
> 
> 
>> +
>> +	mutex_unlock(&data->thermal_lock);
>> +
>> +	return val;
>> +}
>> +
>> +static void hisi_thermal_bind_irq(struct hisi_thermal_data *data)
>> +{
>> +	struct hisi_thermal_sensor *sensor;
>> +
>> +	sensor = &data->sensors[data->irq_bind_sensor];
>> +
>> +	mutex_lock(&data->thermal_lock);
>> +
>> +	/* setting the hdak time */
>> +	writel(0x0, data->regs + TEMP0_CFG);
>> +
>> +	/* disable module firstly */
>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>> +	writel(0x0, data->regs + TEMP0_EN);
>> +
> 
> Sorry, I get confused when you tell me you need to disable the module
> every time you need to read temp / change irq configuration.
> 
> Is this really required?
> 
>> +	/* select sensor id */
>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>> +
>> +	/* enable for interrupt */
>> +	writel(_temp_to_step(sensor->thres_temp)
>> +			| 0x0FFFFFF00, data->regs + TEMP0_TH);
> 
> 
> nit: break after the | operator.
> 
>> +
>> +	writel(_temp_to_step(HISI_TEMP_RESET), data->regs + TEMP0_RST_TH);
>> +
>> +	/* enable module */
>> +	writel(0x1, data->regs + TEMP0_RST_MSK);
>> +	writel(0x1, data->regs + TEMP0_EN);
>> +
>> +	writel(0x0, data->regs + TEMP0_INT_CLR);
>> +	writel(0x1, data->regs + TEMP0_INT_EN);
>> +
>> +	usleep_range(3000, 5000);
>> +
>> +	mutex_unlock(&data->thermal_lock);
>> +
> nit: remove empty line.
> 
>> +}
>> +
>> +static void hisi_thermal_enable_sensor(struct hisi_thermal_data *data)
>> +{
>> +	hisi_thermal_bind_irq(data);
>> +}
>> +
>> +static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
>> +{
>> +
>> +	mutex_lock(&data->thermal_lock);
>> +
>> +	/* disable sensor module */
>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>> +	writel(0x0, data->regs + TEMP0_EN);
>> +
>> +	mutex_unlock(&data->thermal_lock);
>> +
> 
> nit: remove empty line.
> 
>> +}
>> +
>> +static int hisi_thermal_get_temp(void *_sensor, long *temp)
>> +{
>> +	struct hisi_thermal_sensor *sensor = _sensor;
>> +	struct hisi_thermal_data *data = sensor->thermal;
>> +
>> +	int sensor_id = 0, i;
>> +	long max_temp = 0;
>> +
>> +	*temp = hisi_thermal_get_sensor_temp(data, sensor);
>> +
>> +	data->sensor_temp[sensor->id] = *temp;
>> +
>> +	for (i = 0; i < HISI_MAX_SENSORS; i++) {
>> +		if (data->sensor_temp[i] >= max_temp) {
>> +			max_temp = data->sensor_temp[i];
>> +			sensor_id = i;
>> +		}
>> +	}
>> +
>> +	data->irq_bind_sensor = sensor_id;
> 
> I believe this irq_bind_sensor needs to be protected by a lock.
> 
> You read it in t_irq context, write it here while getting temp, and read
> it in bind irq.
> 
> I still don't follow why you need it though, can you please explain?
> 
> In any case, seams to be racy, given that it is shared by the four
> sensors, but you don't lock it to use it.
> 
>> +
>> +	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%ld, thres=%d\n",
>> +		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
>> +	/*
>> +	 * Bind irq to sensor for two cases:
>> +	 *   Reenable alarm IRQ if temperature below threshold;
>> +	 *   if irq has been enabled, always set it;
>> +	 */
>> +	if (data->irq_enabled) {
>> +		hisi_thermal_bind_irq(data);
>> +		return 0;
>> +	}
>> +
>> +	if (max_temp < sensor->thres_temp) {
>> +		data->irq_enabled = true;
>> +		hisi_thermal_bind_irq(data);
>> +		enable_irq(data->irq);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
>> +	.get_temp = hisi_thermal_get_temp,
>> +};
>> +
>> +static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev;
>> +
>> +	disable_irq_nosync(irq);
>> +	data->irq_enabled = false;
>> +
>> +	return IRQ_WAKE_THREAD;
>> +}
>> +
>> +static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev;
>> +	struct hisi_thermal_sensor *sensor;
>> +	int i;
>> +
>> +	sensor = &data->sensors[data->irq_bind_sensor];
>> +
>> +	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
>> +		 sensor->thres_temp / 1000);
> 
> Do you really want to be printing this every time you receive an IRQ?
> 
>> +
>> +	for (i = 0; i < data->sensors_num; i++)
>> +		thermal_zone_device_update(data->sensors[i].tzd);
> 
> 
> Can you please educate me on how this chip works?
> 
> You receive a thermal alarm from the chip, does it really  mean you have
> to update all thermal zones? Or do you have a way to check which sensor
> generated the alarm irq?
> 
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int hisi_thermal_register_sensor(struct platform_device *pdev,
>> +					struct hisi_thermal_data *data,
>> +					struct hisi_thermal_sensor *sensor,
>> +					int index)
>> +{
>> +	int ret, i;
>> +
>> +	sensor->id = index;
>> +	sensor->thermal = data;
>> +
>> +	sensor->tzd = thermal_zone_of_sensor_register(&pdev->dev, sensor->id,
>> +				sensor, &hisi_of_thermal_ops);
>> +	if (IS_ERR(sensor->tzd)) {
>> +		ret = PTR_ERR(sensor->tzd);
>> +		dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
>> +			sensor->id, ret);
>> +		return ret;
>> +	}
>> +
>> +	sensor->trip = of_thermal_get_trip_points(sensor->tzd);
>> +
>> +	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
>> +		if (sensor->trip[i].type == THERMAL_TRIP_PASSIVE) {
> 
> what if you have more than one passive trip? You just care about the
> first?
> 
>> +			sensor->thres_temp = sensor->trip[i].temperature;
> 
> Looks like you use sensor->trip only for filling thres_temp here. Does
> it make sense to use a local variable in this function, given you won't
> use sensor->trip anywhere else?
> 
>> +			break;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id of_hisi_thermal_match[] = {
>> +	{ .compatible = "hisilicon,tsensor" },
>> +	{ /* end */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>> +
>> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>> +				       bool on)
>> +{
>> +	struct thermal_zone_device *tzd = sensor->tzd;
>> +
>> +	tzd->ops->set_mode(tzd,
>> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>> +}
>> +
>> +static int hisi_thermal_probe(struct platform_device *pdev)
>> +{
>> +	struct hisi_thermal_data *data;
>> +	struct resource *res;
>> +	int i;
>> +	int ret;
>> +
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&data->thermal_lock);
>> +	data->pdev = pdev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(data->regs)) {
>> +		dev_err(&pdev->dev, "failed to get io address\n");
>> +		return PTR_ERR(data->regs);
>> +	}
>> +
>> +	data->irq = platform_get_irq(pdev, 0);
>> +	if (data->irq < 0)
>> +		return data->irq;
>> +
>> +	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>> +			hisi_thermal_alarm_irq, hisi_thermal_alarm_irq_thread,
>> +			0, "hisi_thermal", data);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, data);
>> +
>> +	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
>> +	if (IS_ERR(data->clk)) {
>> +		ret = PTR_ERR(data->clk);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(&pdev->dev,
>> +				"failed to get thermal clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* enable clock for thermal */
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
>> +		ret = hisi_thermal_register_sensor(pdev, data,
>> +					&data->sensors[i], i);
>> +		if (ret) {
>> +			dev_err(&pdev->dev,
>> +			"failed to register thermal sensor: %d\n", ret);
>> +			goto err_get_sensor_data;
>> +		}
>> +	}
>> +
>> +	hisi_thermal_enable_sensor(data);
>> +	data->irq_enabled = true;
>> +
>> +	for (i = 0; i < data->sensors_num; i++)
>> +		hisi_thermal_toggle_sensor(&data->sensors[i], true);
>> +
>> +	return 0;
>> +
>> +err_get_sensor_data:
>> +	clk_disable_unprepare(data->clk);
> 
> nit: empty line here to follow your pattern.
> 
>> +	return ret;
>> +}
>> +
>> +static int hisi_thermal_remove(struct platform_device *pdev)
>> +{
>> +	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
>> +	int i;
>> +
>> +	for (i = 0; i < data->sensors_num; i++) {
>> +		struct hisi_thermal_sensor *sensor = &data->sensors[i];
>> +
>> +		hisi_thermal_toggle_sensor(sensor, false);
>> +		thermal_zone_of_sensor_unregister(&pdev->dev, sensor->tzd);
>> +	}
>> +
>> +	hisi_thermal_disable_sensor(data);
>> +	clk_disable_unprepare(data->clk);
> 
> nit: empty line here to follow your pattern.
> 
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int hisi_thermal_suspend(struct device *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>> +
>> +	hisi_thermal_disable_sensor(data);
>> +	data->irq_enabled = false;
>> +
>> +	clk_disable_unprepare(data->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hisi_thermal_resume(struct device *dev)
>> +{
>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>> +
>> +	clk_prepare_enable(data->clk);
>> +
>> +	data->irq_enabled = true;
>> +	hisi_thermal_enable_sensor(data);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops,
>> +			 hisi_thermal_suspend, hisi_thermal_resume);
>> +
>> +static struct platform_driver hisi_thermal_driver = {
>> +	.driver = {
>> +		.name	= "hisi_thermal",
>> +		.owner  = THIS_MODULE,
>> +		.pm	= &hisi_thermal_pm_ops,
>> +		.of_match_table = of_hisi_thermal_match,
> 
> 
> nit: align on the '=' operator.
> 
>> +	},
>> +	.probe		= hisi_thermal_probe,
>> +	.remove		= hisi_thermal_remove,
>> +};
>> +
>> +module_platform_driver(hisi_thermal_driver);
>> +
>> +MODULE_AUTHOR("Xinwei Kong <kong.kongxinwei@hisilicon.com>");
>> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
>> +MODULE_DESCRIPTION("Hisilicon thermal driver");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 1.9.1
>>
>>

Re: [PATCH RESEND v4 2/2] thermal: hisilicon: add new hisilicon thermal sensor driver

[kxw]

hi Eduardo,

I am sorry to understand this following log and hardware principle. being
afraid of using man  is unreasonable.

Sorry you to bother me.

Thanks
Xinwei

On 2015年05月18日 12:03, Xinwei Kong wrote:
> hi Eduardo,
>
> this following review log is from our min systerm patchset. this thermal
> sensor driver is important part for 96boards. If our thermal sensor can't
> be merged into kernel and the min systerm be merged. the board is not safe
> for using man (causing fire). So i wish you can accept this patchset before
> the min systerm patchset is accepted.
>
> thanks
> Xinwei
>
>
> On 2015/5/7 17:02, Will Deacon wrote:> Hi Bintian,
>> On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
>>> PSCI is enabled in device tree and there is no problem to boot all the
>>> octal cores, and the CPU hotplug is also working now, you can download
>>> and compile the latest firmware based on the following link to run this
>>> patch set:
>>> https://github.com/96boards/documentation/wiki/UEFI
>>>
>>> Changes v4:
>>> * Rebase to kernel 4.1-rc1
>>> * Delete "arm,cortex-a15-gic" from the gic node in dts
>> I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
>> above.
>>
>> The good news is that the thing booted and all the cores entered at EL2.
>> Thanks!
>>
>> The bad news is that running hackbench quickly got the *heatsink*
>> temperature to 73 degress C and rising (measured with an infrared
>> thermometer).
>>
>> So my question is, does this SoC have an automatic thermal cut out? Whilst
>> I'm all for merging enabling code into the kernel, if it really relies on
>> the kernel to stop it from catching fire, maybe it's not a great idea
>> putting these patches into people's hands just yet.
>>
>> Will
>>
>> .
>>
> On 2015/5/12 9:29, Eduardo Valentin wrote:
>> Dear Kongxinwei,
>>
>>
>> Apologize for late answer. I really missed this one. Let's make it for
>> the next merge window!
>>
>> Please find a couple of comments as follows.
>>
>>
>> On Mon, May 04, 2015 at 10:52:20AM +0800, Xinwei Kong wrote:
>>> From: kongxinwei <kong.kongxinwei@hisilicon.com>
>>>
>>> This patch adds the support for hisilicon thermal sensor, within
>>> hisilicon SoC. there will register sensors for thermal framework
>>> and use device tree to bind cooling device.
>>>
>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>> Signed-off-by: kongxinwei <kong.kongxinwei@hisilicon.com>
>>> ---
>>>   drivers/thermal/Kconfig        |   8 +
>>>   drivers/thermal/Makefile       |   1 +
>>>   drivers/thermal/hisi_thermal.c | 437 +++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 446 insertions(+)
>>>   create mode 100644 drivers/thermal/hisi_thermal.c
>>
>> Please, make sure you have a clean ./checkpatch.pl --strict output.
>> Today, in this version, I see the following summary:
>>
>> # ./scripts/checkpatch.pl --strict /tmp/a
>> total: 0 errors, 2 warnings, 7 checks, 455 lines checked
>>
>> /tmp/a has style problems, please review.
>>
>> If any of these errors are false positives, please report
>> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>>
>> Can you please clean them up?
>>
>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>>> index af40db0..81aee01 100644
>>> --- a/drivers/thermal/Kconfig
>>> +++ b/drivers/thermal/Kconfig
>>> @@ -136,6 +136,14 @@ config THERMAL_EMULATION
>>>   	  because userland can easily disable the thermal policy by simply
>>>   	  flooding this sysfs node with low temperature values.
>>>   
>>> +config HISI_THERMAL
>>> +	tristate "Hisilicon thermal driver"
>>> +	depends on ARCH_HISI && CPU_THERMAL && OF
>>> +	help
>>> +	  Enable this to plug hisilicon's thermal sensor driver into the Linux
>>> +	  thermal framework. cpufreq is used as the cooling device to throttle
>>> +	  CPUs when the passive trip is crossed.
>>> +
>>>   config IMX_THERMAL
>>>   	tristate "Temperature sensor driver for Freescale i.MX SoCs"
>>>   	depends on CPU_THERMAL
>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>>> index fa0dc48..08ae7ac 100644
>>> --- a/drivers/thermal/Makefile
>>> +++ b/drivers/thermal/Makefile
>>> @@ -39,3 +39,4 @@ obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
>>>   obj-$(CONFIG_INT340X_THERMAL)  += int340x_thermal/
>>>   obj-$(CONFIG_ST_THERMAL)	+= st/
>>>   obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
>>> +obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
>>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>>> new file mode 100644
>>> index 0000000..255c71b
>>> --- /dev/null
>>> +++ b/drivers/thermal/hisi_thermal.c
>>> @@ -0,0 +1,437 @@
>>> +/*
>>> + * Hisilicon thermal sensor driver
>>> + *
>>> + * Copyright (c) 2014-2015 Hisilicon Limited.
>>> + * Copyright (c) 2014-2015 Linaro Limited.
>>> + *
>>> + * Xinwei Kong <kong.kongxinwei@hisilicon.com>
>>> + * Leo Yan <leo.yan@linaro.org>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>>> + * kind, whether express or implied; without even the implied warranty
>>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/cpufreq.h>
>> I don't see why you need this header.
>>
>>> +#include <linux/delay.h>
>>> +#include <linux/device.h>
>>> +#include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/mfd/syscon.h>
>> neither this
>>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>> or this
>>
>>> +#include <linux/slab.h>
>>> +#include <linux/thermal.h>
>>> +#include <linux/types.h>
>>> +
>>> +#include "thermal_core.h"
>>
>> Please review the above includes. Leave only those you are using.
>>
>>> +
>>> +#define TEMP0_TH			(0x4)
>>> +#define TEMP0_RST_TH			(0x8)
>>> +#define TEMP0_CFG			(0xC)
>>> +#define TEMP0_EN			(0x10)
>>> +#define TEMP0_INT_EN			(0x14)
>>> +#define TEMP0_INT_CLR			(0x18)
>>> +#define TEMP0_RST_MSK			(0x1C)
>>> +#define TEMP0_VALUE			(0x28)
>>> +
>>> +#define HISI_TEMP_BASE			(-60)
>>> +#define HISI_TEMP_RESET			(100000)
>>> +
>>> +#define HISI_MAX_SENSORS		4
>>> +
>>> +struct hisi_thermal_sensor {
>>> +	struct hisi_thermal_data *thermal;
>>> +	struct thermal_zone_device *tzd;
>>> +	const struct thermal_trip *trip;
>>> +
>>> +	uint32_t id;
>>> +	uint32_t thres_temp;
>>> +	uint32_t reset_temp;
>> reset_temp is never used in this driver.
>>
>>> +};
>>> +
>>> +struct hisi_thermal_data {
>>> +	struct mutex thermal_lock;
>>> +	struct platform_device *pdev;
>>> +	struct clk *clk;
>>> +
>>> +	int irq, irq_bind_sensor;
>>> +	bool irq_enabled;
>>> +
>>> +	unsigned int sensors_num;
>>> +	long sensor_temp[HISI_MAX_SENSORS];
>>> +	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
>> Does it make sense to make sensor_temp part of hisi_thermal_sensors?
>>
>> Then you would access it through sensors.sensor_temp
>>
>>> +
>>> +	void __iomem *regs;
>>> +};
>>> +
>>> +/* in millicelsius */
>>> +static inline int _step_to_temp(int step)
>>> +{
>>> +	/*
>>> +	 * Every step equals (1 * 200) / 255 celsius, and finally
>>> +	 * need convert to millicelsius.
>>> +	 */
>>> +	return (HISI_TEMP_BASE + (step * 200 / 255)) * 1000;
>>> +}
>>> +
>>> +static inline int _temp_to_step(int temp)
>>> +{
>>> +	return ((temp / 1000 - HISI_TEMP_BASE) * 255 / 200);
>>> +}
>>> +
>>> +static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
>>> +					 struct hisi_thermal_sensor *sensor)
>>> +{
>>> +	int val;
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* disable interrupt */
>>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>>> +	writel(0x1, data->regs + TEMP0_INT_CLR);
>>> +
>>> +	/* disable module firstly */
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>>> +	/* select sensor id */
>>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>>> +
>>> +	/* enable module */
>>> +	writel(0x1, data->regs + TEMP0_EN);
>>> +
>>> +	usleep_range(3000, 5000);
>>> +
>>> +	val = readl(data->regs + TEMP0_VALUE);
>>> +	val = _step_to_temp(val);
>>> +
>>> +	/* adjust for negative value */
>>> +	val = (val < 0) ? 0 : val;
>> Why?
>>
>> What does val < 0 mean here? Does it mean negative temperature or an
>> error?
>>
>> BTW, There is ongoing work to allow, at least representing, negative
>> temperature.
>>
>>
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>>> +	return val;
>>> +}
>>> +
>>> +static void hisi_thermal_bind_irq(struct hisi_thermal_data *data)
>>> +{
>>> +	struct hisi_thermal_sensor *sensor;
>>> +
>>> +	sensor = &data->sensors[data->irq_bind_sensor];
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* setting the hdak time */
>>> +	writel(0x0, data->regs + TEMP0_CFG);
>>> +
>>> +	/* disable module firstly */
>>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>> Sorry, I get confused when you tell me you need to disable the module
>> every time you need to read temp / change irq configuration.
>>
>> Is this really required?
>>
>>> +	/* select sensor id */
>>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>>> +
>>> +	/* enable for interrupt */
>>> +	writel(_temp_to_step(sensor->thres_temp)
>>> +			| 0x0FFFFFF00, data->regs + TEMP0_TH);
>>
>> nit: break after the | operator.
>>
>>> +
>>> +	writel(_temp_to_step(HISI_TEMP_RESET), data->regs + TEMP0_RST_TH);
>>> +
>>> +	/* enable module */
>>> +	writel(0x1, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x1, data->regs + TEMP0_EN);
>>> +
>>> +	writel(0x0, data->regs + TEMP0_INT_CLR);
>>> +	writel(0x1, data->regs + TEMP0_INT_EN);
>>> +
>>> +	usleep_range(3000, 5000);
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>> nit: remove empty line.
>>
>>> +}
>>> +
>>> +static void hisi_thermal_enable_sensor(struct hisi_thermal_data *data)
>>> +{
>>> +	hisi_thermal_bind_irq(data);
>>> +}
>>> +
>>> +static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
>>> +{
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* disable sensor module */
>>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>> nit: remove empty line.
>>
>>> +}
>>> +
>>> +static int hisi_thermal_get_temp(void *_sensor, long *temp)
>>> +{
>>> +	struct hisi_thermal_sensor *sensor = _sensor;
>>> +	struct hisi_thermal_data *data = sensor->thermal;
>>> +
>>> +	int sensor_id = 0, i;
>>> +	long max_temp = 0;
>>> +
>>> +	*temp = hisi_thermal_get_sensor_temp(data, sensor);
>>> +
>>> +	data->sensor_temp[sensor->id] = *temp;
>>> +
>>> +	for (i = 0; i < HISI_MAX_SENSORS; i++) {
>>> +		if (data->sensor_temp[i] >= max_temp) {
>>> +			max_temp = data->sensor_temp[i];
>>> +			sensor_id = i;
>>> +		}
>>> +	}
>>> +
>>> +	data->irq_bind_sensor = sensor_id;
>> I believe this irq_bind_sensor needs to be protected by a lock.
>>
>> You read it in t_irq context, write it here while getting temp, and read
>> it in bind irq.
>>
>> I still don't follow why you need it though, can you please explain?
>>
>> In any case, seams to be racy, given that it is shared by the four
>> sensors, but you don't lock it to use it.
>>
>>> +
>>> +	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%ld, thres=%d\n",
>>> +		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
>>> +	/*
>>> +	 * Bind irq to sensor for two cases:
>>> +	 *   Reenable alarm IRQ if temperature below threshold;
>>> +	 *   if irq has been enabled, always set it;
>>> +	 */
>>> +	if (data->irq_enabled) {
>>> +		hisi_thermal_bind_irq(data);
>>> +		return 0;
>>> +	}
>>> +
>>> +	if (max_temp < sensor->thres_temp) {
>>> +		data->irq_enabled = true;
>>> +		hisi_thermal_bind_irq(data);
>>> +		enable_irq(data->irq);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
>>> +	.get_temp = hisi_thermal_get_temp,
>>> +};
>>> +
>>> +static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev;
>>> +
>>> +	disable_irq_nosync(irq);
>>> +	data->irq_enabled = false;
>>> +
>>> +	return IRQ_WAKE_THREAD;
>>> +}
>>> +
>>> +static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev;
>>> +	struct hisi_thermal_sensor *sensor;
>>> +	int i;
>>> +
>>> +	sensor = &data->sensors[data->irq_bind_sensor];
>>> +
>>> +	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
>>> +		 sensor->thres_temp / 1000);
>> Do you really want to be printing this every time you receive an IRQ?
>>
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++)
>>> +		thermal_zone_device_update(data->sensors[i].tzd);
>>
>> Can you please educate me on how this chip works?
>>
>> You receive a thermal alarm from the chip, does it really  mean you have
>> to update all thermal zones? Or do you have a way to check which sensor
>> generated the alarm irq?
>>
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int hisi_thermal_register_sensor(struct platform_device *pdev,
>>> +					struct hisi_thermal_data *data,
>>> +					struct hisi_thermal_sensor *sensor,
>>> +					int index)
>>> +{
>>> +	int ret, i;
>>> +
>>> +	sensor->id = index;
>>> +	sensor->thermal = data;
>>> +
>>> +	sensor->tzd = thermal_zone_of_sensor_register(&pdev->dev, sensor->id,
>>> +				sensor, &hisi_of_thermal_ops);
>>> +	if (IS_ERR(sensor->tzd)) {
>>> +		ret = PTR_ERR(sensor->tzd);
>>> +		dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
>>> +			sensor->id, ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	sensor->trip = of_thermal_get_trip_points(sensor->tzd);
>>> +
>>> +	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
>>> +		if (sensor->trip[i].type == THERMAL_TRIP_PASSIVE) {
>> what if you have more than one passive trip? You just care about the
>> first?
>>
>>> +			sensor->thres_temp = sensor->trip[i].temperature;
>> Looks like you use sensor->trip only for filling thres_temp here. Does
>> it make sense to use a local variable in this function, given you won't
>> use sensor->trip anywhere else?
>>
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id of_hisi_thermal_match[] = {
>>> +	{ .compatible = "hisilicon,tsensor" },
>>> +	{ /* end */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>>> +
>>> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>>> +				       bool on)
>>> +{
>>> +	struct thermal_zone_device *tzd = sensor->tzd;
>>> +
>>> +	tzd->ops->set_mode(tzd,
>>> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>>> +}
>>> +
>>> +static int hisi_thermal_probe(struct platform_device *pdev)
>>> +{
>>> +	struct hisi_thermal_data *data;
>>> +	struct resource *res;
>>> +	int i;
>>> +	int ret;
>>> +
>>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>>> +	if (!data)
>>> +		return -ENOMEM;
>>> +
>>> +	mutex_init(&data->thermal_lock);
>>> +	data->pdev = pdev;
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(data->regs)) {
>>> +		dev_err(&pdev->dev, "failed to get io address\n");
>>> +		return PTR_ERR(data->regs);
>>> +	}
>>> +
>>> +	data->irq = platform_get_irq(pdev, 0);
>>> +	if (data->irq < 0)
>>> +		return data->irq;
>>> +
>>> +	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>>> +			hisi_thermal_alarm_irq, hisi_thermal_alarm_irq_thread,
>>> +			0, "hisi_thermal", data);
>>> +	if (ret < 0) {
>>> +		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, data);
>>> +
>>> +	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
>>> +	if (IS_ERR(data->clk)) {
>>> +		ret = PTR_ERR(data->clk);
>>> +		if (ret != -EPROBE_DEFER)
>>> +			dev_err(&pdev->dev,
>>> +				"failed to get thermal clk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* enable clock for thermal */
>>> +	ret = clk_prepare_enable(data->clk);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
>>> +		ret = hisi_thermal_register_sensor(pdev, data,
>>> +					&data->sensors[i], i);
>>> +		if (ret) {
>>> +			dev_err(&pdev->dev,
>>> +			"failed to register thermal sensor: %d\n", ret);
>>> +			goto err_get_sensor_data;
>>> +		}
>>> +	}
>>> +
>>> +	hisi_thermal_enable_sensor(data);
>>> +	data->irq_enabled = true;
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++)
>>> +		hisi_thermal_toggle_sensor(&data->sensors[i], true);
>>> +
>>> +	return 0;
>>> +
>>> +err_get_sensor_data:
>>> +	clk_disable_unprepare(data->clk);
>> nit: empty line here to follow your pattern.
>>
>>> +	return ret;
>>> +}
>>> +
>>> +static int hisi_thermal_remove(struct platform_device *pdev)
>>> +{
>>> +	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
>>> +	int i;
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++) {
>>> +		struct hisi_thermal_sensor *sensor = &data->sensors[i];
>>> +
>>> +		hisi_thermal_toggle_sensor(sensor, false);
>>> +		thermal_zone_of_sensor_unregister(&pdev->dev, sensor->tzd);
>>> +	}
>>> +
>>> +	hisi_thermal_disable_sensor(data);
>>> +	clk_disable_unprepare(data->clk);
>> nit: empty line here to follow your pattern.
>>
>>> +	return 0;
>>> +}
>>> +
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int hisi_thermal_suspend(struct device *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>>> +
>>> +	hisi_thermal_disable_sensor(data);
>>> +	data->irq_enabled = false;
>>> +
>>> +	clk_disable_unprepare(data->clk);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int hisi_thermal_resume(struct device *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>>> +
>>> +	clk_prepare_enable(data->clk);
>>> +
>>> +	data->irq_enabled = true;
>>> +	hisi_thermal_enable_sensor(data);
>>> +
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
>>> +static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops,
>>> +			 hisi_thermal_suspend, hisi_thermal_resume);
>>> +
>>> +static struct platform_driver hisi_thermal_driver = {
>>> +	.driver = {
>>> +		.name	= "hisi_thermal",
>>> +		.owner  = THIS_MODULE,
>>> +		.pm	= &hisi_thermal_pm_ops,
>>> +		.of_match_table = of_hisi_thermal_match,
>>
>> nit: align on the '=' operator.
>>
>>> +	},
>>> +	.probe		= hisi_thermal_probe,
>>> +	.remove		= hisi_thermal_remove,
>>> +};
>>> +
>>> +module_platform_driver(hisi_thermal_driver);
>>> +
>>> +MODULE_AUTHOR("Xinwei Kong <kong.kongxinwei@hisilicon.com>");
>>> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
>>> +MODULE_DESCRIPTION("Hisilicon thermal driver");
>>> +MODULE_LICENSE("GPL v2");
>>> -- 
>>> 1.9.1
>>>
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND v4 2/2] thermal: hisilicon: add new hisilicon thermal sensor driver

[Bintian]

Hello Xinwei, Eduardo,

On 2015/5/18 12:03, Xinwei Kong wrote:
> hi Eduardo,
>
> this following review log is from our min systerm patchset. this thermal
> sensor driver is important part for 96boards. If our thermal sensor can't
> be merged into kernel and the min systerm be merged. the board is not safe
> for using man (causing fire).
Not so serious,  I explained this problem in another email.

> So i wish you can accept this patchset before
> the min systerm patchset is accepted.
Xinwei, please let maintainer judge the quality of your patch and decide
to merge or not.

Thanks,

Bintian
>
> thanks
> Xinwei
>
>
> On 2015/5/7 17:02, Will Deacon wrote:> Hi Bintian,
>>
>> On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
>>> PSCI is enabled in device tree and there is no problem to boot all the
>>> octal cores, and the CPU hotplug is also working now, you can download
>>> and compile the latest firmware based on the following link to run this
>>> patch set:
>>> https://github.com/96boards/documentation/wiki/UEFI
>>>
>>> Changes v4:
>>> * Rebase to kernel 4.1-rc1
>>> * Delete "arm,cortex-a15-gic" from the gic node in dts
>>
>> I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
>> above.
>>
>> The good news is that the thing booted and all the cores entered at EL2.
>> Thanks!
>>
>> The bad news is that running hackbench quickly got the *heatsink*
>> temperature to 73 degress C and rising (measured with an infrared
>> thermometer).
>>
>> So my question is, does this SoC have an automatic thermal cut out? Whilst
>> I'm all for merging enabling code into the kernel, if it really relies on
>> the kernel to stop it from catching fire, maybe it's not a great idea
>> putting these patches into people's hands just yet.
>>
>> Will
>>
>> .
>>
>
> On 2015/5/12 9:29, Eduardo Valentin wrote:
>> Dear Kongxinwei,
>>
>>
>> Apologize for late answer. I really missed this one. Let's make it for
>> the next merge window!
>>
>> Please find a couple of comments as follows.
>>
>>
>> On Mon, May 04, 2015 at 10:52:20AM +0800, Xinwei Kong wrote:
>>> From: kongxinwei <kong.kongxinwei@hisilicon.com>
>>>
>>> This patch adds the support for hisilicon thermal sensor, within
>>> hisilicon SoC. there will register sensors for thermal framework
>>> and use device tree to bind cooling device.
>>>
>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>> Signed-off-by: kongxinwei <kong.kongxinwei@hisilicon.com>
>>> ---
>>>   drivers/thermal/Kconfig        |   8 +
>>>   drivers/thermal/Makefile       |   1 +
>>>   drivers/thermal/hisi_thermal.c | 437 +++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 446 insertions(+)
>>>   create mode 100644 drivers/thermal/hisi_thermal.c
>>
>>
>> Please, make sure you have a clean ./checkpatch.pl --strict output.
>> Today, in this version, I see the following summary:
>>
>> # ./scripts/checkpatch.pl --strict /tmp/a
>> total: 0 errors, 2 warnings, 7 checks, 455 lines checked
>>
>> /tmp/a has style problems, please review.
>>
>> If any of these errors are false positives, please report
>> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>>
>> Can you please clean them up?
>>
>>>
>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>>> index af40db0..81aee01 100644
>>> --- a/drivers/thermal/Kconfig
>>> +++ b/drivers/thermal/Kconfig
>>> @@ -136,6 +136,14 @@ config THERMAL_EMULATION
>>>   	  because userland can easily disable the thermal policy by simply
>>>   	  flooding this sysfs node with low temperature values.
>>>
>>> +config HISI_THERMAL
>>> +	tristate "Hisilicon thermal driver"
>>> +	depends on ARCH_HISI && CPU_THERMAL && OF
>>> +	help
>>> +	  Enable this to plug hisilicon's thermal sensor driver into the Linux
>>> +	  thermal framework. cpufreq is used as the cooling device to throttle
>>> +	  CPUs when the passive trip is crossed.
>>> +
>>>   config IMX_THERMAL
>>>   	tristate "Temperature sensor driver for Freescale i.MX SoCs"
>>>   	depends on CPU_THERMAL
>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>>> index fa0dc48..08ae7ac 100644
>>> --- a/drivers/thermal/Makefile
>>> +++ b/drivers/thermal/Makefile
>>> @@ -39,3 +39,4 @@ obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
>>>   obj-$(CONFIG_INT340X_THERMAL)  += int340x_thermal/
>>>   obj-$(CONFIG_ST_THERMAL)	+= st/
>>>   obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
>>> +obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
>>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>>> new file mode 100644
>>> index 0000000..255c71b
>>> --- /dev/null
>>> +++ b/drivers/thermal/hisi_thermal.c
>>> @@ -0,0 +1,437 @@
>>> +/*
>>> + * Hisilicon thermal sensor driver
>>> + *
>>> + * Copyright (c) 2014-2015 Hisilicon Limited.
>>> + * Copyright (c) 2014-2015 Linaro Limited.
>>> + *
>>> + * Xinwei Kong <kong.kongxinwei@hisilicon.com>
>>> + * Leo Yan <leo.yan@linaro.org>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>>> + * kind, whether express or implied; without even the implied warranty
>>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/cpufreq.h>
>>
>> I don't see why you need this header.
>>
>>> +#include <linux/delay.h>
>>> +#include <linux/device.h>
>>> +#include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/mfd/syscon.h>
>>
>> neither this
>>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>
>> or this
>>
>>> +#include <linux/slab.h>
>>> +#include <linux/thermal.h>
>>> +#include <linux/types.h>
>>> +
>>> +#include "thermal_core.h"
>>
>>
>> Please review the above includes. Leave only those you are using.
>>
>>> +
>>> +#define TEMP0_TH			(0x4)
>>> +#define TEMP0_RST_TH			(0x8)
>>> +#define TEMP0_CFG			(0xC)
>>> +#define TEMP0_EN			(0x10)
>>> +#define TEMP0_INT_EN			(0x14)
>>> +#define TEMP0_INT_CLR			(0x18)
>>> +#define TEMP0_RST_MSK			(0x1C)
>>> +#define TEMP0_VALUE			(0x28)
>>> +
>>> +#define HISI_TEMP_BASE			(-60)
>>> +#define HISI_TEMP_RESET			(100000)
>>> +
>>> +#define HISI_MAX_SENSORS		4
>>> +
>>> +struct hisi_thermal_sensor {
>>> +	struct hisi_thermal_data *thermal;
>>> +	struct thermal_zone_device *tzd;
>>> +	const struct thermal_trip *trip;
>>> +
>>> +	uint32_t id;
>>> +	uint32_t thres_temp;
>>> +	uint32_t reset_temp;
>>
>> reset_temp is never used in this driver.
>>
>>> +};
>>> +
>>> +struct hisi_thermal_data {
>>> +	struct mutex thermal_lock;
>>> +	struct platform_device *pdev;
>>> +	struct clk *clk;
>>> +
>>> +	int irq, irq_bind_sensor;
>>> +	bool irq_enabled;
>>> +
>>> +	unsigned int sensors_num;
>>> +	long sensor_temp[HISI_MAX_SENSORS];
>>> +	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
>>
>> Does it make sense to make sensor_temp part of hisi_thermal_sensors?
>>
>> Then you would access it through sensors.sensor_temp
>>
>>> +
>>> +	void __iomem *regs;
>>> +};
>>> +
>>> +/* in millicelsius */
>>> +static inline int _step_to_temp(int step)
>>> +{
>>> +	/*
>>> +	 * Every step equals (1 * 200) / 255 celsius, and finally
>>> +	 * need convert to millicelsius.
>>> +	 */
>>> +	return (HISI_TEMP_BASE + (step * 200 / 255)) * 1000;
>>> +}
>>> +
>>> +static inline int _temp_to_step(int temp)
>>> +{
>>> +	return ((temp / 1000 - HISI_TEMP_BASE) * 255 / 200);
>>> +}
>>> +
>>> +static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
>>> +					 struct hisi_thermal_sensor *sensor)
>>> +{
>>> +	int val;
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* disable interrupt */
>>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>>> +	writel(0x1, data->regs + TEMP0_INT_CLR);
>>> +
>>> +	/* disable module firstly */
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>>> +	/* select sensor id */
>>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>>> +
>>> +	/* enable module */
>>> +	writel(0x1, data->regs + TEMP0_EN);
>>> +
>>> +	usleep_range(3000, 5000);
>>> +
>>> +	val = readl(data->regs + TEMP0_VALUE);
>>> +	val = _step_to_temp(val);
>>> +
>>> +	/* adjust for negative value */
>>> +	val = (val < 0) ? 0 : val;
>>
>> Why?
>>
>> What does val < 0 mean here? Does it mean negative temperature or an
>> error?
>>
>> BTW, There is ongoing work to allow, at least representing, negative
>> temperature.
>>
>>
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>>> +	return val;
>>> +}
>>> +
>>> +static void hisi_thermal_bind_irq(struct hisi_thermal_data *data)
>>> +{
>>> +	struct hisi_thermal_sensor *sensor;
>>> +
>>> +	sensor = &data->sensors[data->irq_bind_sensor];
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* setting the hdak time */
>>> +	writel(0x0, data->regs + TEMP0_CFG);
>>> +
>>> +	/* disable module firstly */
>>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>>
>> Sorry, I get confused when you tell me you need to disable the module
>> every time you need to read temp / change irq configuration.
>>
>> Is this really required?
>>
>>> +	/* select sensor id */
>>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>>> +
>>> +	/* enable for interrupt */
>>> +	writel(_temp_to_step(sensor->thres_temp)
>>> +			| 0x0FFFFFF00, data->regs + TEMP0_TH);
>>
>>
>> nit: break after the | operator.
>>
>>> +
>>> +	writel(_temp_to_step(HISI_TEMP_RESET), data->regs + TEMP0_RST_TH);
>>> +
>>> +	/* enable module */
>>> +	writel(0x1, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x1, data->regs + TEMP0_EN);
>>> +
>>> +	writel(0x0, data->regs + TEMP0_INT_CLR);
>>> +	writel(0x1, data->regs + TEMP0_INT_EN);
>>> +
>>> +	usleep_range(3000, 5000);
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>> nit: remove empty line.
>>
>>> +}
>>> +
>>> +static void hisi_thermal_enable_sensor(struct hisi_thermal_data *data)
>>> +{
>>> +	hisi_thermal_bind_irq(data);
>>> +}
>>> +
>>> +static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
>>> +{
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* disable sensor module */
>>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>>
>> nit: remove empty line.
>>
>>> +}
>>> +
>>> +static int hisi_thermal_get_temp(void *_sensor, long *temp)
>>> +{
>>> +	struct hisi_thermal_sensor *sensor = _sensor;
>>> +	struct hisi_thermal_data *data = sensor->thermal;
>>> +
>>> +	int sensor_id = 0, i;
>>> +	long max_temp = 0;
>>> +
>>> +	*temp = hisi_thermal_get_sensor_temp(data, sensor);
>>> +
>>> +	data->sensor_temp[sensor->id] = *temp;
>>> +
>>> +	for (i = 0; i < HISI_MAX_SENSORS; i++) {
>>> +		if (data->sensor_temp[i] >= max_temp) {
>>> +			max_temp = data->sensor_temp[i];
>>> +			sensor_id = i;
>>> +		}
>>> +	}
>>> +
>>> +	data->irq_bind_sensor = sensor_id;
>>
>> I believe this irq_bind_sensor needs to be protected by a lock.
>>
>> You read it in t_irq context, write it here while getting temp, and read
>> it in bind irq.
>>
>> I still don't follow why you need it though, can you please explain?
>>
>> In any case, seams to be racy, given that it is shared by the four
>> sensors, but you don't lock it to use it.
>>
>>> +
>>> +	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%ld, thres=%d\n",
>>> +		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
>>> +	/*
>>> +	 * Bind irq to sensor for two cases:
>>> +	 *   Reenable alarm IRQ if temperature below threshold;
>>> +	 *   if irq has been enabled, always set it;
>>> +	 */
>>> +	if (data->irq_enabled) {
>>> +		hisi_thermal_bind_irq(data);
>>> +		return 0;
>>> +	}
>>> +
>>> +	if (max_temp < sensor->thres_temp) {
>>> +		data->irq_enabled = true;
>>> +		hisi_thermal_bind_irq(data);
>>> +		enable_irq(data->irq);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
>>> +	.get_temp = hisi_thermal_get_temp,
>>> +};
>>> +
>>> +static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev;
>>> +
>>> +	disable_irq_nosync(irq);
>>> +	data->irq_enabled = false;
>>> +
>>> +	return IRQ_WAKE_THREAD;
>>> +}
>>> +
>>> +static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev;
>>> +	struct hisi_thermal_sensor *sensor;
>>> +	int i;
>>> +
>>> +	sensor = &data->sensors[data->irq_bind_sensor];
>>> +
>>> +	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
>>> +		 sensor->thres_temp / 1000);
>>
>> Do you really want to be printing this every time you receive an IRQ?
>>
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++)
>>> +		thermal_zone_device_update(data->sensors[i].tzd);
>>
>>
>> Can you please educate me on how this chip works?
>>
>> You receive a thermal alarm from the chip, does it really  mean you have
>> to update all thermal zones? Or do you have a way to check which sensor
>> generated the alarm irq?
>>
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int hisi_thermal_register_sensor(struct platform_device *pdev,
>>> +					struct hisi_thermal_data *data,
>>> +					struct hisi_thermal_sensor *sensor,
>>> +					int index)
>>> +{
>>> +	int ret, i;
>>> +
>>> +	sensor->id = index;
>>> +	sensor->thermal = data;
>>> +
>>> +	sensor->tzd = thermal_zone_of_sensor_register(&pdev->dev, sensor->id,
>>> +				sensor, &hisi_of_thermal_ops);
>>> +	if (IS_ERR(sensor->tzd)) {
>>> +		ret = PTR_ERR(sensor->tzd);
>>> +		dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
>>> +			sensor->id, ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	sensor->trip = of_thermal_get_trip_points(sensor->tzd);
>>> +
>>> +	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
>>> +		if (sensor->trip[i].type == THERMAL_TRIP_PASSIVE) {
>>
>> what if you have more than one passive trip? You just care about the
>> first?
>>
>>> +			sensor->thres_temp = sensor->trip[i].temperature;
>>
>> Looks like you use sensor->trip only for filling thres_temp here. Does
>> it make sense to use a local variable in this function, given you won't
>> use sensor->trip anywhere else?
>>
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id of_hisi_thermal_match[] = {
>>> +	{ .compatible = "hisilicon,tsensor" },
>>> +	{ /* end */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>>> +
>>> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>>> +				       bool on)
>>> +{
>>> +	struct thermal_zone_device *tzd = sensor->tzd;
>>> +
>>> +	tzd->ops->set_mode(tzd,
>>> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>>> +}
>>> +
>>> +static int hisi_thermal_probe(struct platform_device *pdev)
>>> +{
>>> +	struct hisi_thermal_data *data;
>>> +	struct resource *res;
>>> +	int i;
>>> +	int ret;
>>> +
>>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>>> +	if (!data)
>>> +		return -ENOMEM;
>>> +
>>> +	mutex_init(&data->thermal_lock);
>>> +	data->pdev = pdev;
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(data->regs)) {
>>> +		dev_err(&pdev->dev, "failed to get io address\n");
>>> +		return PTR_ERR(data->regs);
>>> +	}
>>> +
>>> +	data->irq = platform_get_irq(pdev, 0);
>>> +	if (data->irq < 0)
>>> +		return data->irq;
>>> +
>>> +	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>>> +			hisi_thermal_alarm_irq, hisi_thermal_alarm_irq_thread,
>>> +			0, "hisi_thermal", data);
>>> +	if (ret < 0) {
>>> +		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, data);
>>> +
>>> +	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
>>> +	if (IS_ERR(data->clk)) {
>>> +		ret = PTR_ERR(data->clk);
>>> +		if (ret != -EPROBE_DEFER)
>>> +			dev_err(&pdev->dev,
>>> +				"failed to get thermal clk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* enable clock for thermal */
>>> +	ret = clk_prepare_enable(data->clk);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
>>> +		ret = hisi_thermal_register_sensor(pdev, data,
>>> +					&data->sensors[i], i);
>>> +		if (ret) {
>>> +			dev_err(&pdev->dev,
>>> +			"failed to register thermal sensor: %d\n", ret);
>>> +			goto err_get_sensor_data;
>>> +		}
>>> +	}
>>> +
>>> +	hisi_thermal_enable_sensor(data);
>>> +	data->irq_enabled = true;
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++)
>>> +		hisi_thermal_toggle_sensor(&data->sensors[i], true);
>>> +
>>> +	return 0;
>>> +
>>> +err_get_sensor_data:
>>> +	clk_disable_unprepare(data->clk);
>>
>> nit: empty line here to follow your pattern.
>>
>>> +	return ret;
>>> +}
>>> +
>>> +static int hisi_thermal_remove(struct platform_device *pdev)
>>> +{
>>> +	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
>>> +	int i;
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++) {
>>> +		struct hisi_thermal_sensor *sensor = &data->sensors[i];
>>> +
>>> +		hisi_thermal_toggle_sensor(sensor, false);
>>> +		thermal_zone_of_sensor_unregister(&pdev->dev, sensor->tzd);
>>> +	}
>>> +
>>> +	hisi_thermal_disable_sensor(data);
>>> +	clk_disable_unprepare(data->clk);
>>
>> nit: empty line here to follow your pattern.
>>
>>> +	return 0;
>>> +}
>>> +
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int hisi_thermal_suspend(struct device *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>>> +
>>> +	hisi_thermal_disable_sensor(data);
>>> +	data->irq_enabled = false;
>>> +
>>> +	clk_disable_unprepare(data->clk);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int hisi_thermal_resume(struct device *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>>> +
>>> +	clk_prepare_enable(data->clk);
>>> +
>>> +	data->irq_enabled = true;
>>> +	hisi_thermal_enable_sensor(data);
>>> +
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
>>> +static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops,
>>> +			 hisi_thermal_suspend, hisi_thermal_resume);
>>> +
>>> +static struct platform_driver hisi_thermal_driver = {
>>> +	.driver = {
>>> +		.name	= "hisi_thermal",
>>> +		.owner  = THIS_MODULE,
>>> +		.pm	= &hisi_thermal_pm_ops,
>>> +		.of_match_table = of_hisi_thermal_match,
>>
>>
>> nit: align on the '=' operator.
>>
>>> +	},
>>> +	.probe		= hisi_thermal_probe,
>>> +	.remove		= hisi_thermal_remove,
>>> +};
>>> +
>>> +module_platform_driver(hisi_thermal_driver);
>>> +
>>> +MODULE_AUTHOR("Xinwei Kong <kong.kongxinwei@hisilicon.com>");
>>> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
>>> +MODULE_DESCRIPTION("Hisilicon thermal driver");
>>> +MODULE_LICENSE("GPL v2");
>>> --
>>> 1.9.1
>>>
>>>
>
>
> .
>

Re: [PATCH RESEND v4 2/2] thermal: hisilicon: add new hisilicon thermal sensor driver

[kxw]

hi Eduardo

I am sorry the i don't understand this following log and hardware principle, being afraid
of using is unreasonale.

sorry to bother you

thanks
Xinwei

On 2015年05月18日 12:03, Xinwei Kong wrote:
> hi Eduardo,
>
> this following review log is from our min systerm patchset. this thermal
> sensor driver is important part for 96boards. If our thermal sensor can't
> be merged into kernel and the min systerm be merged. the board is not safe
> for using man (causing fire). So i wish you can accept this patchset before
> the min systerm patchset is accepted.
>
> thanks
> Xinwei
>
>
> On 2015/5/7 17:02, Will Deacon wrote:> Hi Bintian,
>> On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
>>> PSCI is enabled in device tree and there is no problem to boot all the
>>> octal cores, and the CPU hotplug is also working now, you can download
>>> and compile the latest firmware based on the following link to run this
>>> patch set:
>>> https://github.com/96boards/documentation/wiki/UEFI
>>>
>>> Changes v4:
>>> * Rebase to kernel 4.1-rc1
>>> * Delete "arm,cortex-a15-gic" from the gic node in dts
>> I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
>> above.
>>
>> The good news is that the thing booted and all the cores entered at EL2.
>> Thanks!
>>
>> The bad news is that running hackbench quickly got the *heatsink*
>> temperature to 73 degress C and rising (measured with an infrared
>> thermometer).
>>
>> So my question is, does this SoC have an automatic thermal cut out? Whilst
>> I'm all for merging enabling code into the kernel, if it really relies on
>> the kernel to stop it from catching fire, maybe it's not a great idea
>> putting these patches into people's hands just yet.
>>
>> Will
>>
>> .
>>
> On 2015/5/12 9:29, Eduardo Valentin wrote:
>> Dear Kongxinwei,
>>
>>
>> Apologize for late answer. I really missed this one. Let's make it for
>> the next merge window!
>>
>> Please find a couple of comments as follows.
>>
>>
>> On Mon, May 04, 2015 at 10:52:20AM +0800, Xinwei Kong wrote:
>>> From: kongxinwei <kong.kongxinwei@hisilicon.com>
>>>
>>> This patch adds the support for hisilicon thermal sensor, within
>>> hisilicon SoC. there will register sensors for thermal framework
>>> and use device tree to bind cooling device.
>>>
>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>> Signed-off-by: kongxinwei <kong.kongxinwei@hisilicon.com>
>>> ---
>>>   drivers/thermal/Kconfig        |   8 +
>>>   drivers/thermal/Makefile       |   1 +
>>>   drivers/thermal/hisi_thermal.c | 437 +++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 446 insertions(+)
>>>   create mode 100644 drivers/thermal/hisi_thermal.c
>>
>> Please, make sure you have a clean ./checkpatch.pl --strict output.
>> Today, in this version, I see the following summary:
>>
>> # ./scripts/checkpatch.pl --strict /tmp/a
>> total: 0 errors, 2 warnings, 7 checks, 455 lines checked
>>
>> /tmp/a has style problems, please review.
>>
>> If any of these errors are false positives, please report
>> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>>
>> Can you please clean them up?
>>
>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>>> index af40db0..81aee01 100644
>>> --- a/drivers/thermal/Kconfig
>>> +++ b/drivers/thermal/Kconfig
>>> @@ -136,6 +136,14 @@ config THERMAL_EMULATION
>>>   	  because userland can easily disable the thermal policy by simply
>>>   	  flooding this sysfs node with low temperature values.
>>>   
>>> +config HISI_THERMAL
>>> +	tristate "Hisilicon thermal driver"
>>> +	depends on ARCH_HISI && CPU_THERMAL && OF
>>> +	help
>>> +	  Enable this to plug hisilicon's thermal sensor driver into the Linux
>>> +	  thermal framework. cpufreq is used as the cooling device to throttle
>>> +	  CPUs when the passive trip is crossed.
>>> +
>>>   config IMX_THERMAL
>>>   	tristate "Temperature sensor driver for Freescale i.MX SoCs"
>>>   	depends on CPU_THERMAL
>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>>> index fa0dc48..08ae7ac 100644
>>> --- a/drivers/thermal/Makefile
>>> +++ b/drivers/thermal/Makefile
>>> @@ -39,3 +39,4 @@ obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
>>>   obj-$(CONFIG_INT340X_THERMAL)  += int340x_thermal/
>>>   obj-$(CONFIG_ST_THERMAL)	+= st/
>>>   obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
>>> +obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
>>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>>> new file mode 100644
>>> index 0000000..255c71b
>>> --- /dev/null
>>> +++ b/drivers/thermal/hisi_thermal.c
>>> @@ -0,0 +1,437 @@
>>> +/*
>>> + * Hisilicon thermal sensor driver
>>> + *
>>> + * Copyright (c) 2014-2015 Hisilicon Limited.
>>> + * Copyright (c) 2014-2015 Linaro Limited.
>>> + *
>>> + * Xinwei Kong <kong.kongxinwei@hisilicon.com>
>>> + * Leo Yan <leo.yan@linaro.org>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>>> + * kind, whether express or implied; without even the implied warranty
>>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/cpufreq.h>
>> I don't see why you need this header.
>>
>>> +#include <linux/delay.h>
>>> +#include <linux/device.h>
>>> +#include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/mfd/syscon.h>
>> neither this
>>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>> or this
>>
>>> +#include <linux/slab.h>
>>> +#include <linux/thermal.h>
>>> +#include <linux/types.h>
>>> +
>>> +#include "thermal_core.h"
>>
>> Please review the above includes. Leave only those you are using.
>>
>>> +
>>> +#define TEMP0_TH			(0x4)
>>> +#define TEMP0_RST_TH			(0x8)
>>> +#define TEMP0_CFG			(0xC)
>>> +#define TEMP0_EN			(0x10)
>>> +#define TEMP0_INT_EN			(0x14)
>>> +#define TEMP0_INT_CLR			(0x18)
>>> +#define TEMP0_RST_MSK			(0x1C)
>>> +#define TEMP0_VALUE			(0x28)
>>> +
>>> +#define HISI_TEMP_BASE			(-60)
>>> +#define HISI_TEMP_RESET			(100000)
>>> +
>>> +#define HISI_MAX_SENSORS		4
>>> +
>>> +struct hisi_thermal_sensor {
>>> +	struct hisi_thermal_data *thermal;
>>> +	struct thermal_zone_device *tzd;
>>> +	const struct thermal_trip *trip;
>>> +
>>> +	uint32_t id;
>>> +	uint32_t thres_temp;
>>> +	uint32_t reset_temp;
>> reset_temp is never used in this driver.
>>
>>> +};
>>> +
>>> +struct hisi_thermal_data {
>>> +	struct mutex thermal_lock;
>>> +	struct platform_device *pdev;
>>> +	struct clk *clk;
>>> +
>>> +	int irq, irq_bind_sensor;
>>> +	bool irq_enabled;
>>> +
>>> +	unsigned int sensors_num;
>>> +	long sensor_temp[HISI_MAX_SENSORS];
>>> +	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
>> Does it make sense to make sensor_temp part of hisi_thermal_sensors?
>>
>> Then you would access it through sensors.sensor_temp
>>
>>> +
>>> +	void __iomem *regs;
>>> +};
>>> +
>>> +/* in millicelsius */
>>> +static inline int _step_to_temp(int step)
>>> +{
>>> +	/*
>>> +	 * Every step equals (1 * 200) / 255 celsius, and finally
>>> +	 * need convert to millicelsius.
>>> +	 */
>>> +	return (HISI_TEMP_BASE + (step * 200 / 255)) * 1000;
>>> +}
>>> +
>>> +static inline int _temp_to_step(int temp)
>>> +{
>>> +	return ((temp / 1000 - HISI_TEMP_BASE) * 255 / 200);
>>> +}
>>> +
>>> +static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
>>> +					 struct hisi_thermal_sensor *sensor)
>>> +{
>>> +	int val;
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* disable interrupt */
>>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>>> +	writel(0x1, data->regs + TEMP0_INT_CLR);
>>> +
>>> +	/* disable module firstly */
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>>> +	/* select sensor id */
>>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>>> +
>>> +	/* enable module */
>>> +	writel(0x1, data->regs + TEMP0_EN);
>>> +
>>> +	usleep_range(3000, 5000);
>>> +
>>> +	val = readl(data->regs + TEMP0_VALUE);
>>> +	val = _step_to_temp(val);
>>> +
>>> +	/* adjust for negative value */
>>> +	val = (val < 0) ? 0 : val;
>> Why?
>>
>> What does val < 0 mean here? Does it mean negative temperature or an
>> error?
>>
>> BTW, There is ongoing work to allow, at least representing, negative
>> temperature.
>>
>>
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>>> +	return val;
>>> +}
>>> +
>>> +static void hisi_thermal_bind_irq(struct hisi_thermal_data *data)
>>> +{
>>> +	struct hisi_thermal_sensor *sensor;
>>> +
>>> +	sensor = &data->sensors[data->irq_bind_sensor];
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* setting the hdak time */
>>> +	writel(0x0, data->regs + TEMP0_CFG);
>>> +
>>> +	/* disable module firstly */
>>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>> Sorry, I get confused when you tell me you need to disable the module
>> every time you need to read temp / change irq configuration.
>>
>> Is this really required?
>>
>>> +	/* select sensor id */
>>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>>> +
>>> +	/* enable for interrupt */
>>> +	writel(_temp_to_step(sensor->thres_temp)
>>> +			| 0x0FFFFFF00, data->regs + TEMP0_TH);
>>
>> nit: break after the | operator.
>>
>>> +
>>> +	writel(_temp_to_step(HISI_TEMP_RESET), data->regs + TEMP0_RST_TH);
>>> +
>>> +	/* enable module */
>>> +	writel(0x1, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x1, data->regs + TEMP0_EN);
>>> +
>>> +	writel(0x0, data->regs + TEMP0_INT_CLR);
>>> +	writel(0x1, data->regs + TEMP0_INT_EN);
>>> +
>>> +	usleep_range(3000, 5000);
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>> nit: remove empty line.
>>
>>> +}
>>> +
>>> +static void hisi_thermal_enable_sensor(struct hisi_thermal_data *data)
>>> +{
>>> +	hisi_thermal_bind_irq(data);
>>> +}
>>> +
>>> +static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
>>> +{
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* disable sensor module */
>>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>> nit: remove empty line.
>>
>>> +}
>>> +
>>> +static int hisi_thermal_get_temp(void *_sensor, long *temp)
>>> +{
>>> +	struct hisi_thermal_sensor *sensor = _sensor;
>>> +	struct hisi_thermal_data *data = sensor->thermal;
>>> +
>>> +	int sensor_id = 0, i;
>>> +	long max_temp = 0;
>>> +
>>> +	*temp = hisi_thermal_get_sensor_temp(data, sensor);
>>> +
>>> +	data->sensor_temp[sensor->id] = *temp;
>>> +
>>> +	for (i = 0; i < HISI_MAX_SENSORS; i++) {
>>> +		if (data->sensor_temp[i] >= max_temp) {
>>> +			max_temp = data->sensor_temp[i];
>>> +			sensor_id = i;
>>> +		}
>>> +	}
>>> +
>>> +	data->irq_bind_sensor = sensor_id;
>> I believe this irq_bind_sensor needs to be protected by a lock.
>>
>> You read it in t_irq context, write it here while getting temp, and read
>> it in bind irq.
>>
>> I still don't follow why you need it though, can you please explain?
>>
>> In any case, seams to be racy, given that it is shared by the four
>> sensors, but you don't lock it to use it.
>>
>>> +
>>> +	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%ld, thres=%d\n",
>>> +		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
>>> +	/*
>>> +	 * Bind irq to sensor for two cases:
>>> +	 *   Reenable alarm IRQ if temperature below threshold;
>>> +	 *   if irq has been enabled, always set it;
>>> +	 */
>>> +	if (data->irq_enabled) {
>>> +		hisi_thermal_bind_irq(data);
>>> +		return 0;
>>> +	}
>>> +
>>> +	if (max_temp < sensor->thres_temp) {
>>> +		data->irq_enabled = true;
>>> +		hisi_thermal_bind_irq(data);
>>> +		enable_irq(data->irq);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
>>> +	.get_temp = hisi_thermal_get_temp,
>>> +};
>>> +
>>> +static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev;
>>> +
>>> +	disable_irq_nosync(irq);
>>> +	data->irq_enabled = false;
>>> +
>>> +	return IRQ_WAKE_THREAD;
>>> +}
>>> +
>>> +static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev;
>>> +	struct hisi_thermal_sensor *sensor;
>>> +	int i;
>>> +
>>> +	sensor = &data->sensors[data->irq_bind_sensor];
>>> +
>>> +	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
>>> +		 sensor->thres_temp / 1000);
>> Do you really want to be printing this every time you receive an IRQ?
>>
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++)
>>> +		thermal_zone_device_update(data->sensors[i].tzd);
>>
>> Can you please educate me on how this chip works?
>>
>> You receive a thermal alarm from the chip, does it really  mean you have
>> to update all thermal zones? Or do you have a way to check which sensor
>> generated the alarm irq?
>>
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int hisi_thermal_register_sensor(struct platform_device *pdev,
>>> +					struct hisi_thermal_data *data,
>>> +					struct hisi_thermal_sensor *sensor,
>>> +					int index)
>>> +{
>>> +	int ret, i;
>>> +
>>> +	sensor->id = index;
>>> +	sensor->thermal = data;
>>> +
>>> +	sensor->tzd = thermal_zone_of_sensor_register(&pdev->dev, sensor->id,
>>> +				sensor, &hisi_of_thermal_ops);
>>> +	if (IS_ERR(sensor->tzd)) {
>>> +		ret = PTR_ERR(sensor->tzd);
>>> +		dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
>>> +			sensor->id, ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	sensor->trip = of_thermal_get_trip_points(sensor->tzd);
>>> +
>>> +	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
>>> +		if (sensor->trip[i].type == THERMAL_TRIP_PASSIVE) {
>> what if you have more than one passive trip? You just care about the
>> first?
>>
>>> +			sensor->thres_temp = sensor->trip[i].temperature;
>> Looks like you use sensor->trip only for filling thres_temp here. Does
>> it make sense to use a local variable in this function, given you won't
>> use sensor->trip anywhere else?
>>
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id of_hisi_thermal_match[] = {
>>> +	{ .compatible = "hisilicon,tsensor" },
>>> +	{ /* end */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>>> +
>>> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>>> +				       bool on)
>>> +{
>>> +	struct thermal_zone_device *tzd = sensor->tzd;
>>> +
>>> +	tzd->ops->set_mode(tzd,
>>> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>>> +}
>>> +
>>> +static int hisi_thermal_probe(struct platform_device *pdev)
>>> +{
>>> +	struct hisi_thermal_data *data;
>>> +	struct resource *res;
>>> +	int i;
>>> +	int ret;
>>> +
>>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>>> +	if (!data)
>>> +		return -ENOMEM;
>>> +
>>> +	mutex_init(&data->thermal_lock);
>>> +	data->pdev = pdev;
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(data->regs)) {
>>> +		dev_err(&pdev->dev, "failed to get io address\n");
>>> +		return PTR_ERR(data->regs);
>>> +	}
>>> +
>>> +	data->irq = platform_get_irq(pdev, 0);
>>> +	if (data->irq < 0)
>>> +		return data->irq;
>>> +
>>> +	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>>> +			hisi_thermal_alarm_irq, hisi_thermal_alarm_irq_thread,
>>> +			0, "hisi_thermal", data);
>>> +	if (ret < 0) {
>>> +		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, data);
>>> +
>>> +	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
>>> +	if (IS_ERR(data->clk)) {
>>> +		ret = PTR_ERR(data->clk);
>>> +		if (ret != -EPROBE_DEFER)
>>> +			dev_err(&pdev->dev,
>>> +				"failed to get thermal clk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* enable clock for thermal */
>>> +	ret = clk_prepare_enable(data->clk);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
>>> +		ret = hisi_thermal_register_sensor(pdev, data,
>>> +					&data->sensors[i], i);
>>> +		if (ret) {
>>> +			dev_err(&pdev->dev,
>>> +			"failed to register thermal sensor: %d\n", ret);
>>> +			goto err_get_sensor_data;
>>> +		}
>>> +	}
>>> +
>>> +	hisi_thermal_enable_sensor(data);
>>> +	data->irq_enabled = true;
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++)
>>> +		hisi_thermal_toggle_sensor(&data->sensors[i], true);
>>> +
>>> +	return 0;
>>> +
>>> +err_get_sensor_data:
>>> +	clk_disable_unprepare(data->clk);
>> nit: empty line here to follow your pattern.
>>
>>> +	return ret;
>>> +}
>>> +
>>> +static int hisi_thermal_remove(struct platform_device *pdev)
>>> +{
>>> +	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
>>> +	int i;
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++) {
>>> +		struct hisi_thermal_sensor *sensor = &data->sensors[i];
>>> +
>>> +		hisi_thermal_toggle_sensor(sensor, false);
>>> +		thermal_zone_of_sensor_unregister(&pdev->dev, sensor->tzd);
>>> +	}
>>> +
>>> +	hisi_thermal_disable_sensor(data);
>>> +	clk_disable_unprepare(data->clk);
>> nit: empty line here to follow your pattern.
>>
>>> +	return 0;
>>> +}
>>> +
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int hisi_thermal_suspend(struct device *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>>> +
>>> +	hisi_thermal_disable_sensor(data);
>>> +	data->irq_enabled = false;
>>> +
>>> +	clk_disable_unprepare(data->clk);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int hisi_thermal_resume(struct device *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>>> +
>>> +	clk_prepare_enable(data->clk);
>>> +
>>> +	data->irq_enabled = true;
>>> +	hisi_thermal_enable_sensor(data);
>>> +
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
>>> +static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops,
>>> +			 hisi_thermal_suspend, hisi_thermal_resume);
>>> +
>>> +static struct platform_driver hisi_thermal_driver = {
>>> +	.driver = {
>>> +		.name	= "hisi_thermal",
>>> +		.owner  = THIS_MODULE,
>>> +		.pm	= &hisi_thermal_pm_ops,
>>> +		.of_match_table = of_hisi_thermal_match,
>>
>> nit: align on the '=' operator.
>>
>>> +	},
>>> +	.probe		= hisi_thermal_probe,
>>> +	.remove		= hisi_thermal_remove,
>>> +};
>>> +
>>> +module_platform_driver(hisi_thermal_driver);
>>> +
>>> +MODULE_AUTHOR("Xinwei Kong <kong.kongxinwei@hisilicon.com>");
>>> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
>>> +MODULE_DESCRIPTION("Hisilicon thermal driver");
>>> +MODULE_LICENSE("GPL v2");
>>> -- 
>>> 1.9.1
>>>
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND v4 2/2] thermal: hisilicon: add new hisilicon thermal sensor driver

[kxw]

hi Eduardo

I am sorry the i don't understand this following log and hardware principle, being afraid
of using is unreasonale.

sorry to bother you

thanks
Xinwei


On 2015年05月18日 12:03, Xinwei Kong wrote:
> hi Eduardo,
>
> this following review log is from our min systerm patchset. this thermal
> sensor driver is important part for 96boards. If our thermal sensor can't
> be merged into kernel and the min systerm be merged. the board is not safe
> for using man (causing fire). So i wish you can accept this patchset before
> the min systerm patchset is accepted.
>
> thanks
> Xinwei
>
>
> On 2015/5/7 17:02, Will Deacon wrote:> Hi Bintian,
>> On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
>>> PSCI is enabled in device tree and there is no problem to boot all the
>>> octal cores, and the CPU hotplug is also working now, you can download
>>> and compile the latest firmware based on the following link to run this
>>> patch set:
>>> https://github.com/96boards/documentation/wiki/UEFI
>>>
>>> Changes v4:
>>> * Rebase to kernel 4.1-rc1
>>> * Delete "arm,cortex-a15-gic" from the gic node in dts
>> I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
>> above.
>>
>> The good news is that the thing booted and all the cores entered at EL2.
>> Thanks!
>>
>> The bad news is that running hackbench quickly got the *heatsink*
>> temperature to 73 degress C and rising (measured with an infrared
>> thermometer).
>>
>> So my question is, does this SoC have an automatic thermal cut out? Whilst
>> I'm all for merging enabling code into the kernel, if it really relies on
>> the kernel to stop it from catching fire, maybe it's not a great idea
>> putting these patches into people's hands just yet.
>>
>> Will
>>
>> .
>>
> On 2015/5/12 9:29, Eduardo Valentin wrote:
>> Dear Kongxinwei,
>>
>>
>> Apologize for late answer. I really missed this one. Let's make it for
>> the next merge window!
>>
>> Please find a couple of comments as follows.
>>
>>
>> On Mon, May 04, 2015 at 10:52:20AM +0800, Xinwei Kong wrote:
>>> From: kongxinwei <kong.kongxinwei@hisilicon.com>
>>>
>>> This patch adds the support for hisilicon thermal sensor, within
>>> hisilicon SoC. there will register sensors for thermal framework
>>> and use device tree to bind cooling device.
>>>
>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>> Signed-off-by: kongxinwei <kong.kongxinwei@hisilicon.com>
>>> ---
>>>   drivers/thermal/Kconfig        |   8 +
>>>   drivers/thermal/Makefile       |   1 +
>>>   drivers/thermal/hisi_thermal.c | 437 +++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 446 insertions(+)
>>>   create mode 100644 drivers/thermal/hisi_thermal.c
>>
>> Please, make sure you have a clean ./checkpatch.pl --strict output.
>> Today, in this version, I see the following summary:
>>
>> # ./scripts/checkpatch.pl --strict /tmp/a
>> total: 0 errors, 2 warnings, 7 checks, 455 lines checked
>>
>> /tmp/a has style problems, please review.
>>
>> If any of these errors are false positives, please report
>> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>>
>> Can you please clean them up?
>>
>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>>> index af40db0..81aee01 100644
>>> --- a/drivers/thermal/Kconfig
>>> +++ b/drivers/thermal/Kconfig
>>> @@ -136,6 +136,14 @@ config THERMAL_EMULATION
>>>   	  because userland can easily disable the thermal policy by simply
>>>   	  flooding this sysfs node with low temperature values.
>>>   
>>> +config HISI_THERMAL
>>> +	tristate "Hisilicon thermal driver"
>>> +	depends on ARCH_HISI && CPU_THERMAL && OF
>>> +	help
>>> +	  Enable this to plug hisilicon's thermal sensor driver into the Linux
>>> +	  thermal framework. cpufreq is used as the cooling device to throttle
>>> +	  CPUs when the passive trip is crossed.
>>> +
>>>   config IMX_THERMAL
>>>   	tristate "Temperature sensor driver for Freescale i.MX SoCs"
>>>   	depends on CPU_THERMAL
>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>>> index fa0dc48..08ae7ac 100644
>>> --- a/drivers/thermal/Makefile
>>> +++ b/drivers/thermal/Makefile
>>> @@ -39,3 +39,4 @@ obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
>>>   obj-$(CONFIG_INT340X_THERMAL)  += int340x_thermal/
>>>   obj-$(CONFIG_ST_THERMAL)	+= st/
>>>   obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
>>> +obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
>>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>>> new file mode 100644
>>> index 0000000..255c71b
>>> --- /dev/null
>>> +++ b/drivers/thermal/hisi_thermal.c
>>> @@ -0,0 +1,437 @@
>>> +/*
>>> + * Hisilicon thermal sensor driver
>>> + *
>>> + * Copyright (c) 2014-2015 Hisilicon Limited.
>>> + * Copyright (c) 2014-2015 Linaro Limited.
>>> + *
>>> + * Xinwei Kong <kong.kongxinwei@hisilicon.com>
>>> + * Leo Yan <leo.yan@linaro.org>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>>> + * kind, whether express or implied; without even the implied warranty
>>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/cpufreq.h>
>> I don't see why you need this header.
>>
>>> +#include <linux/delay.h>
>>> +#include <linux/device.h>
>>> +#include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/mfd/syscon.h>
>> neither this
>>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>> or this
>>
>>> +#include <linux/slab.h>
>>> +#include <linux/thermal.h>
>>> +#include <linux/types.h>
>>> +
>>> +#include "thermal_core.h"
>>
>> Please review the above includes. Leave only those you are using.
>>
>>> +
>>> +#define TEMP0_TH			(0x4)
>>> +#define TEMP0_RST_TH			(0x8)
>>> +#define TEMP0_CFG			(0xC)
>>> +#define TEMP0_EN			(0x10)
>>> +#define TEMP0_INT_EN			(0x14)
>>> +#define TEMP0_INT_CLR			(0x18)
>>> +#define TEMP0_RST_MSK			(0x1C)
>>> +#define TEMP0_VALUE			(0x28)
>>> +
>>> +#define HISI_TEMP_BASE			(-60)
>>> +#define HISI_TEMP_RESET			(100000)
>>> +
>>> +#define HISI_MAX_SENSORS		4
>>> +
>>> +struct hisi_thermal_sensor {
>>> +	struct hisi_thermal_data *thermal;
>>> +	struct thermal_zone_device *tzd;
>>> +	const struct thermal_trip *trip;
>>> +
>>> +	uint32_t id;
>>> +	uint32_t thres_temp;
>>> +	uint32_t reset_temp;
>> reset_temp is never used in this driver.
>>
>>> +};
>>> +
>>> +struct hisi_thermal_data {
>>> +	struct mutex thermal_lock;
>>> +	struct platform_device *pdev;
>>> +	struct clk *clk;
>>> +
>>> +	int irq, irq_bind_sensor;
>>> +	bool irq_enabled;
>>> +
>>> +	unsigned int sensors_num;
>>> +	long sensor_temp[HISI_MAX_SENSORS];
>>> +	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
>> Does it make sense to make sensor_temp part of hisi_thermal_sensors?
>>
>> Then you would access it through sensors.sensor_temp
>>
>>> +
>>> +	void __iomem *regs;
>>> +};
>>> +
>>> +/* in millicelsius */
>>> +static inline int _step_to_temp(int step)
>>> +{
>>> +	/*
>>> +	 * Every step equals (1 * 200) / 255 celsius, and finally
>>> +	 * need convert to millicelsius.
>>> +	 */
>>> +	return (HISI_TEMP_BASE + (step * 200 / 255)) * 1000;
>>> +}
>>> +
>>> +static inline int _temp_to_step(int temp)
>>> +{
>>> +	return ((temp / 1000 - HISI_TEMP_BASE) * 255 / 200);
>>> +}
>>> +
>>> +static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
>>> +					 struct hisi_thermal_sensor *sensor)
>>> +{
>>> +	int val;
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* disable interrupt */
>>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>>> +	writel(0x1, data->regs + TEMP0_INT_CLR);
>>> +
>>> +	/* disable module firstly */
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>>> +	/* select sensor id */
>>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>>> +
>>> +	/* enable module */
>>> +	writel(0x1, data->regs + TEMP0_EN);
>>> +
>>> +	usleep_range(3000, 5000);
>>> +
>>> +	val = readl(data->regs + TEMP0_VALUE);
>>> +	val = _step_to_temp(val);
>>> +
>>> +	/* adjust for negative value */
>>> +	val = (val < 0) ? 0 : val;
>> Why?
>>
>> What does val < 0 mean here? Does it mean negative temperature or an
>> error?
>>
>> BTW, There is ongoing work to allow, at least representing, negative
>> temperature.
>>
>>
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>>> +	return val;
>>> +}
>>> +
>>> +static void hisi_thermal_bind_irq(struct hisi_thermal_data *data)
>>> +{
>>> +	struct hisi_thermal_sensor *sensor;
>>> +
>>> +	sensor = &data->sensors[data->irq_bind_sensor];
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* setting the hdak time */
>>> +	writel(0x0, data->regs + TEMP0_CFG);
>>> +
>>> +	/* disable module firstly */
>>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>> Sorry, I get confused when you tell me you need to disable the module
>> every time you need to read temp / change irq configuration.
>>
>> Is this really required?
>>
>>> +	/* select sensor id */
>>> +	writel((sensor->id << 12), data->regs + TEMP0_CFG);
>>> +
>>> +	/* enable for interrupt */
>>> +	writel(_temp_to_step(sensor->thres_temp)
>>> +			| 0x0FFFFFF00, data->regs + TEMP0_TH);
>>
>> nit: break after the | operator.
>>
>>> +
>>> +	writel(_temp_to_step(HISI_TEMP_RESET), data->regs + TEMP0_RST_TH);
>>> +
>>> +	/* enable module */
>>> +	writel(0x1, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x1, data->regs + TEMP0_EN);
>>> +
>>> +	writel(0x0, data->regs + TEMP0_INT_CLR);
>>> +	writel(0x1, data->regs + TEMP0_INT_EN);
>>> +
>>> +	usleep_range(3000, 5000);
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>> nit: remove empty line.
>>
>>> +}
>>> +
>>> +static void hisi_thermal_enable_sensor(struct hisi_thermal_data *data)
>>> +{
>>> +	hisi_thermal_bind_irq(data);
>>> +}
>>> +
>>> +static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
>>> +{
>>> +
>>> +	mutex_lock(&data->thermal_lock);
>>> +
>>> +	/* disable sensor module */
>>> +	writel(0x0, data->regs + TEMP0_INT_EN);
>>> +	writel(0x0, data->regs + TEMP0_RST_MSK);
>>> +	writel(0x0, data->regs + TEMP0_EN);
>>> +
>>> +	mutex_unlock(&data->thermal_lock);
>>> +
>> nit: remove empty line.
>>
>>> +}
>>> +
>>> +static int hisi_thermal_get_temp(void *_sensor, long *temp)
>>> +{
>>> +	struct hisi_thermal_sensor *sensor = _sensor;
>>> +	struct hisi_thermal_data *data = sensor->thermal;
>>> +
>>> +	int sensor_id = 0, i;
>>> +	long max_temp = 0;
>>> +
>>> +	*temp = hisi_thermal_get_sensor_temp(data, sensor);
>>> +
>>> +	data->sensor_temp[sensor->id] = *temp;
>>> +
>>> +	for (i = 0; i < HISI_MAX_SENSORS; i++) {
>>> +		if (data->sensor_temp[i] >= max_temp) {
>>> +			max_temp = data->sensor_temp[i];
>>> +			sensor_id = i;
>>> +		}
>>> +	}
>>> +
>>> +	data->irq_bind_sensor = sensor_id;
>> I believe this irq_bind_sensor needs to be protected by a lock.
>>
>> You read it in t_irq context, write it here while getting temp, and read
>> it in bind irq.
>>
>> I still don't follow why you need it though, can you please explain?
>>
>> In any case, seams to be racy, given that it is shared by the four
>> sensors, but you don't lock it to use it.
>>
>>> +
>>> +	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%ld, thres=%d\n",
>>> +		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
>>> +	/*
>>> +	 * Bind irq to sensor for two cases:
>>> +	 *   Reenable alarm IRQ if temperature below threshold;
>>> +	 *   if irq has been enabled, always set it;
>>> +	 */
>>> +	if (data->irq_enabled) {
>>> +		hisi_thermal_bind_irq(data);
>>> +		return 0;
>>> +	}
>>> +
>>> +	if (max_temp < sensor->thres_temp) {
>>> +		data->irq_enabled = true;
>>> +		hisi_thermal_bind_irq(data);
>>> +		enable_irq(data->irq);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
>>> +	.get_temp = hisi_thermal_get_temp,
>>> +};
>>> +
>>> +static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev;
>>> +
>>> +	disable_irq_nosync(irq);
>>> +	data->irq_enabled = false;
>>> +
>>> +	return IRQ_WAKE_THREAD;
>>> +}
>>> +
>>> +static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev;
>>> +	struct hisi_thermal_sensor *sensor;
>>> +	int i;
>>> +
>>> +	sensor = &data->sensors[data->irq_bind_sensor];
>>> +
>>> +	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
>>> +		 sensor->thres_temp / 1000);
>> Do you really want to be printing this every time you receive an IRQ?
>>
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++)
>>> +		thermal_zone_device_update(data->sensors[i].tzd);
>>
>> Can you please educate me on how this chip works?
>>
>> You receive a thermal alarm from the chip, does it really  mean you have
>> to update all thermal zones? Or do you have a way to check which sensor
>> generated the alarm irq?
>>
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int hisi_thermal_register_sensor(struct platform_device *pdev,
>>> +					struct hisi_thermal_data *data,
>>> +					struct hisi_thermal_sensor *sensor,
>>> +					int index)
>>> +{
>>> +	int ret, i;
>>> +
>>> +	sensor->id = index;
>>> +	sensor->thermal = data;
>>> +
>>> +	sensor->tzd = thermal_zone_of_sensor_register(&pdev->dev, sensor->id,
>>> +				sensor, &hisi_of_thermal_ops);
>>> +	if (IS_ERR(sensor->tzd)) {
>>> +		ret = PTR_ERR(sensor->tzd);
>>> +		dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
>>> +			sensor->id, ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	sensor->trip = of_thermal_get_trip_points(sensor->tzd);
>>> +
>>> +	for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
>>> +		if (sensor->trip[i].type == THERMAL_TRIP_PASSIVE) {
>> what if you have more than one passive trip? You just care about the
>> first?
>>
>>> +			sensor->thres_temp = sensor->trip[i].temperature;
>> Looks like you use sensor->trip only for filling thres_temp here. Does
>> it make sense to use a local variable in this function, given you won't
>> use sensor->trip anywhere else?
>>
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id of_hisi_thermal_match[] = {
>>> +	{ .compatible = "hisilicon,tsensor" },
>>> +	{ /* end */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>>> +
>>> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>>> +				       bool on)
>>> +{
>>> +	struct thermal_zone_device *tzd = sensor->tzd;
>>> +
>>> +	tzd->ops->set_mode(tzd,
>>> +		on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>>> +}
>>> +
>>> +static int hisi_thermal_probe(struct platform_device *pdev)
>>> +{
>>> +	struct hisi_thermal_data *data;
>>> +	struct resource *res;
>>> +	int i;
>>> +	int ret;
>>> +
>>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>>> +	if (!data)
>>> +		return -ENOMEM;
>>> +
>>> +	mutex_init(&data->thermal_lock);
>>> +	data->pdev = pdev;
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(data->regs)) {
>>> +		dev_err(&pdev->dev, "failed to get io address\n");
>>> +		return PTR_ERR(data->regs);
>>> +	}
>>> +
>>> +	data->irq = platform_get_irq(pdev, 0);
>>> +	if (data->irq < 0)
>>> +		return data->irq;
>>> +
>>> +	ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>>> +			hisi_thermal_alarm_irq, hisi_thermal_alarm_irq_thread,
>>> +			0, "hisi_thermal", data);
>>> +	if (ret < 0) {
>>> +		dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, data);
>>> +
>>> +	data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
>>> +	if (IS_ERR(data->clk)) {
>>> +		ret = PTR_ERR(data->clk);
>>> +		if (ret != -EPROBE_DEFER)
>>> +			dev_err(&pdev->dev,
>>> +				"failed to get thermal clk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* enable clock for thermal */
>>> +	ret = clk_prepare_enable(data->clk);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
>>> +		ret = hisi_thermal_register_sensor(pdev, data,
>>> +					&data->sensors[i], i);
>>> +		if (ret) {
>>> +			dev_err(&pdev->dev,
>>> +			"failed to register thermal sensor: %d\n", ret);
>>> +			goto err_get_sensor_data;
>>> +		}
>>> +	}
>>> +
>>> +	hisi_thermal_enable_sensor(data);
>>> +	data->irq_enabled = true;
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++)
>>> +		hisi_thermal_toggle_sensor(&data->sensors[i], true);
>>> +
>>> +	return 0;
>>> +
>>> +err_get_sensor_data:
>>> +	clk_disable_unprepare(data->clk);
>> nit: empty line here to follow your pattern.
>>
>>> +	return ret;
>>> +}
>>> +
>>> +static int hisi_thermal_remove(struct platform_device *pdev)
>>> +{
>>> +	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
>>> +	int i;
>>> +
>>> +	for (i = 0; i < data->sensors_num; i++) {
>>> +		struct hisi_thermal_sensor *sensor = &data->sensors[i];
>>> +
>>> +		hisi_thermal_toggle_sensor(sensor, false);
>>> +		thermal_zone_of_sensor_unregister(&pdev->dev, sensor->tzd);
>>> +	}
>>> +
>>> +	hisi_thermal_disable_sensor(data);
>>> +	clk_disable_unprepare(data->clk);
>> nit: empty line here to follow your pattern.
>>
>>> +	return 0;
>>> +}
>>> +
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int hisi_thermal_suspend(struct device *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>>> +
>>> +	hisi_thermal_disable_sensor(data);
>>> +	data->irq_enabled = false;
>>> +
>>> +	clk_disable_unprepare(data->clk);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int hisi_thermal_resume(struct device *dev)
>>> +{
>>> +	struct hisi_thermal_data *data = dev_get_drvdata(dev);
>>> +
>>> +	clk_prepare_enable(data->clk);
>>> +
>>> +	data->irq_enabled = true;
>>> +	hisi_thermal_enable_sensor(data);
>>> +
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
>>> +static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops,
>>> +			 hisi_thermal_suspend, hisi_thermal_resume);
>>> +
>>> +static struct platform_driver hisi_thermal_driver = {
>>> +	.driver = {
>>> +		.name	= "hisi_thermal",
>>> +		.owner  = THIS_MODULE,
>>> +		.pm	= &hisi_thermal_pm_ops,
>>> +		.of_match_table = of_hisi_thermal_match,
>>
>> nit: align on the '=' operator.
>>
>>> +	},
>>> +	.probe		= hisi_thermal_probe,
>>> +	.remove		= hisi_thermal_remove,
>>> +};
>>> +
>>> +module_platform_driver(hisi_thermal_driver);
>>> +
>>> +MODULE_AUTHOR("Xinwei Kong <kong.kongxinwei@hisilicon.com>");
>>> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
>>> +MODULE_DESCRIPTION("Hisilicon thermal driver");
>>> +MODULE_LICENSE("GPL v2");
>>> -- 
>>> 1.9.1
>>>
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND v4 2/2] thermal: hisilicon: add new hisilicon thermal sensor driver

[Xinwei Kong]

On 2015/5/18 12:43, Bintian wrote:
> Hello Xinwei, Eduardo,
> 
> On 2015/5/18 12:03, Xinwei Kong wrote:
>> hi Eduardo,
>>
>> this following review log is from our min systerm patchset. this thermal
>> sensor driver is important part for 96boards. If our thermal sensor can't
>> be merged into kernel and the min systerm be merged. the board is not safe
>> for using man (causing fire).
> Not so serious,  I explained this problem in another email.
> 
Pls forgive my newbie.

>> So i wish you can accept this patchset before
>> the min systerm patchset is accepted.
> Xinwei, please let maintainer judge the quality of your patch and decide
> to merge or not.
> 
Ok, thanks brother tian

> Thanks,
> 
> Bintian
>>
>> thanks
>> Xinwei
>>
>>
>> On 2015/5/7 17:02, Will Deacon wrote:> Hi Bintian,
>>>
>>> On Tue, May 05, 2015 at 01:06:34PM +0100, Bintian Wang wrote:
>>>> PSCI is enabled in device tree and there is no problem to boot all the
>>>> octal cores, and the CPU hotplug is also working now, you can download
>>>> and compile the latest firmware based on the following link to run this
>>>> patch set:
>>>> https://github.com/96boards/documentation/wiki/UEFI
>>>>
>>>> Changes v4:
>>>> * Rebase to kernel 4.1-rc1
>>>> * Delete "arm,cortex-a15-gic" from the gic node in dts
>>>
>>> I gave these patches a go on top of -rc2 using the ATF and UEFI you link to
>>> above.
>>>
>>> The good news is that the thing booted and all the cores entered at EL2.
>>> Thanks!
>>>
>>> The bad news is that running hackbench quickly got the *heatsink*
>>> temperature to 73 degress C and rising (measured with an infrared
>>> thermometer).
>>>
>>> So my question is, does this SoC have an automatic thermal cut out? Whilst
>>> I'm all for merging enabling code into the kernel, if it really relies on
>>> the kernel to stop it from catching fire, maybe it's not a great idea
>>> putting these patches into people's hands just yet.
>>>
>>> Will
>>>
>>> .
>>>
>>
>> On 2015/5/12 9:29, Eduardo Valentin wrote:
>>> Dear Kongxinwei,
>>>
>>>
>>> Apologize for late answer. I really missed this one. Let's make it for
>>> the next merge window!
>>>
>>> Please find a couple of comments as follows.
>>>
>>>
>>> On Mon, May 04, 2015 at 10:52:20AM +0800, Xinwei Kong wrote:
>>>> From: kongxinwei <kong.kongxinwei@hisilicon.com>
>>>>
>>>> This patch adds the support for hisilicon thermal sensor, within
>>>> hisilicon SoC. there will register sensors for thermal framework
>>>> and use device tree to bind cooling device.
>>>>
>>>> Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>>> Signed-off-by: kongxinwei <kong.kongxinwei@hisilicon.com>
>>>> ---
>>>>   drivers/thermal/Kconfig        |   8 +
>>>>   drivers/thermal/Makefile       |   1 +
>>>>   drivers/thermal/hisi_thermal.c | 437 +++++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 446 insertions(+)
>>>>   create mode 100644 drivers/thermal/hisi_thermal.c
>>>
>>>
>>> Please, make sure you have a clean ./checkpatch.pl --strict output.
>>> Today, in this version, I see the following summary:
>>>
>>> # ./scripts/checkpatch.pl --strict /tmp/a
>>> total: 0 errors, 2 warnings, 7 checks, 455 lines checked
>>>
>>> /tmp/a has style problems, please review.
>>>
>>> If any of these errors are false positives, please report
>>> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>>>
>>> Can you please clean them up?
>>>
>>>>
>>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>>>> index af40db0..81aee01 100644
>>>> --- a/drivers/thermal/Kconfig
>>>> +++ b/drivers/thermal/Kconfig
>>>> @@ -136,6 +136,14 @@ config THERMAL_EMULATION
>>>>         because userland can easily disable the thermal policy by simply
>>>>         flooding this sysfs node with low temperature values.
>>>>
>>>> +config HISI_THERMAL
>>>> +    tristate "Hisilicon thermal driver"
>>>> +    depends on ARCH_HISI && CPU_THERMAL && OF
>>>> +    help
>>>> +      Enable this to plug hisilicon's thermal sensor driver into the Linux
>>>> +      thermal framework. cpufreq is used as the cooling device to throttle
>>>> +      CPUs when the passive trip is crossed.
>>>> +
>>>>   config IMX_THERMAL
>>>>       tristate "Temperature sensor driver for Freescale i.MX SoCs"
>>>>       depends on CPU_THERMAL
>>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>>>> index fa0dc48..08ae7ac 100644
>>>> --- a/drivers/thermal/Makefile
>>>> +++ b/drivers/thermal/Makefile
>>>> @@ -39,3 +39,4 @@ obj-$(CONFIG_TI_SOC_THERMAL)    += ti-soc-thermal/
>>>>   obj-$(CONFIG_INT340X_THERMAL)  += int340x_thermal/
>>>>   obj-$(CONFIG_ST_THERMAL)    += st/
>>>>   obj-$(CONFIG_TEGRA_SOCTHERM)    += tegra_soctherm.o
>>>> +obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
>>>> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
>>>> new file mode 100644
>>>> index 0000000..255c71b
>>>> --- /dev/null
>>>> +++ b/drivers/thermal/hisi_thermal.c
>>>> @@ -0,0 +1,437 @@
>>>> +/*
>>>> + * Hisilicon thermal sensor driver
>>>> + *
>>>> + * Copyright (c) 2014-2015 Hisilicon Limited.
>>>> + * Copyright (c) 2014-2015 Linaro Limited.
>>>> + *
>>>> + * Xinwei Kong <kong.kongxinwei@hisilicon.com>
>>>> + * Leo Yan <leo.yan@linaro.org>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>>>> + * kind, whether express or implied; without even the implied warranty
>>>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <linux/cpufreq.h>
>>>
>>> I don't see why you need this header.
>>>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/mfd/syscon.h>
>>>
>>> neither this
>>>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regmap.h>
>>>
>>> or this
>>>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/thermal.h>
>>>> +#include <linux/types.h>
>>>> +
>>>> +#include "thermal_core.h"
>>>
>>>
>>> Please review the above includes. Leave only those you are using.
>>>
>>>> +
>>>> +#define TEMP0_TH            (0x4)
>>>> +#define TEMP0_RST_TH            (0x8)
>>>> +#define TEMP0_CFG            (0xC)
>>>> +#define TEMP0_EN            (0x10)
>>>> +#define TEMP0_INT_EN            (0x14)
>>>> +#define TEMP0_INT_CLR            (0x18)
>>>> +#define TEMP0_RST_MSK            (0x1C)
>>>> +#define TEMP0_VALUE            (0x28)
>>>> +
>>>> +#define HISI_TEMP_BASE            (-60)
>>>> +#define HISI_TEMP_RESET            (100000)
>>>> +
>>>> +#define HISI_MAX_SENSORS        4
>>>> +
>>>> +struct hisi_thermal_sensor {
>>>> +    struct hisi_thermal_data *thermal;
>>>> +    struct thermal_zone_device *tzd;
>>>> +    const struct thermal_trip *trip;
>>>> +
>>>> +    uint32_t id;
>>>> +    uint32_t thres_temp;
>>>> +    uint32_t reset_temp;
>>>
>>> reset_temp is never used in this driver.
>>>
>>>> +};
>>>> +
>>>> +struct hisi_thermal_data {
>>>> +    struct mutex thermal_lock;
>>>> +    struct platform_device *pdev;
>>>> +    struct clk *clk;
>>>> +
>>>> +    int irq, irq_bind_sensor;
>>>> +    bool irq_enabled;
>>>> +
>>>> +    unsigned int sensors_num;
>>>> +    long sensor_temp[HISI_MAX_SENSORS];
>>>> +    struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
>>>
>>> Does it make sense to make sensor_temp part of hisi_thermal_sensors?
>>>
>>> Then you would access it through sensors.sensor_temp
>>>
>>>> +
>>>> +    void __iomem *regs;
>>>> +};
>>>> +
>>>> +/* in millicelsius */
>>>> +static inline int _step_to_temp(int step)
>>>> +{
>>>> +    /*
>>>> +     * Every step equals (1 * 200) / 255 celsius, and finally
>>>> +     * need convert to millicelsius.
>>>> +     */
>>>> +    return (HISI_TEMP_BASE + (step * 200 / 255)) * 1000;
>>>> +}
>>>> +
>>>> +static inline int _temp_to_step(int temp)
>>>> +{
>>>> +    return ((temp / 1000 - HISI_TEMP_BASE) * 255 / 200);
>>>> +}
>>>> +
>>>> +static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data,
>>>> +                     struct hisi_thermal_sensor *sensor)
>>>> +{
>>>> +    int val;
>>>> +
>>>> +    mutex_lock(&data->thermal_lock);
>>>> +
>>>> +    /* disable interrupt */
>>>> +    writel(0x0, data->regs + TEMP0_INT_EN);
>>>> +    writel(0x1, data->regs + TEMP0_INT_CLR);
>>>> +
>>>> +    /* disable module firstly */
>>>> +    writel(0x0, data->regs + TEMP0_EN);
>>>> +
>>>> +    /* select sensor id */
>>>> +    writel((sensor->id << 12), data->regs + TEMP0_CFG);
>>>> +
>>>> +    /* enable module */
>>>> +    writel(0x1, data->regs + TEMP0_EN);
>>>> +
>>>> +    usleep_range(3000, 5000);
>>>> +
>>>> +    val = readl(data->regs + TEMP0_VALUE);
>>>> +    val = _step_to_temp(val);
>>>> +
>>>> +    /* adjust for negative value */
>>>> +    val = (val < 0) ? 0 : val;
>>>
>>> Why?
>>>
>>> What does val < 0 mean here? Does it mean negative temperature or an
>>> error?
>>>
>>> BTW, There is ongoing work to allow, at least representing, negative
>>> temperature.
>>>
>>>
>>>> +
>>>> +    mutex_unlock(&data->thermal_lock);
>>>> +
>>>> +    return val;
>>>> +}
>>>> +
>>>> +static void hisi_thermal_bind_irq(struct hisi_thermal_data *data)
>>>> +{
>>>> +    struct hisi_thermal_sensor *sensor;
>>>> +
>>>> +    sensor = &data->sensors[data->irq_bind_sensor];
>>>> +
>>>> +    mutex_lock(&data->thermal_lock);
>>>> +
>>>> +    /* setting the hdak time */
>>>> +    writel(0x0, data->regs + TEMP0_CFG);
>>>> +
>>>> +    /* disable module firstly */
>>>> +    writel(0x0, data->regs + TEMP0_RST_MSK);
>>>> +    writel(0x0, data->regs + TEMP0_EN);
>>>> +
>>>
>>> Sorry, I get confused when you tell me you need to disable the module
>>> every time you need to read temp / change irq configuration.
>>>
>>> Is this really required?
>>>
>>>> +    /* select sensor id */
>>>> +    writel((sensor->id << 12), data->regs + TEMP0_CFG);
>>>> +
>>>> +    /* enable for interrupt */
>>>> +    writel(_temp_to_step(sensor->thres_temp)
>>>> +            | 0x0FFFFFF00, data->regs + TEMP0_TH);
>>>
>>>
>>> nit: break after the | operator.
>>>
>>>> +
>>>> +    writel(_temp_to_step(HISI_TEMP_RESET), data->regs + TEMP0_RST_TH);
>>>> +
>>>> +    /* enable module */
>>>> +    writel(0x1, data->regs + TEMP0_RST_MSK);
>>>> +    writel(0x1, data->regs + TEMP0_EN);
>>>> +
>>>> +    writel(0x0, data->regs + TEMP0_INT_CLR);
>>>> +    writel(0x1, data->regs + TEMP0_INT_EN);
>>>> +
>>>> +    usleep_range(3000, 5000);
>>>> +
>>>> +    mutex_unlock(&data->thermal_lock);
>>>> +
>>> nit: remove empty line.
>>>
>>>> +}
>>>> +
>>>> +static void hisi_thermal_enable_sensor(struct hisi_thermal_data *data)
>>>> +{
>>>> +    hisi_thermal_bind_irq(data);
>>>> +}
>>>> +
>>>> +static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data)
>>>> +{
>>>> +
>>>> +    mutex_lock(&data->thermal_lock);
>>>> +
>>>> +    /* disable sensor module */
>>>> +    writel(0x0, data->regs + TEMP0_INT_EN);
>>>> +    writel(0x0, data->regs + TEMP0_RST_MSK);
>>>> +    writel(0x0, data->regs + TEMP0_EN);
>>>> +
>>>> +    mutex_unlock(&data->thermal_lock);
>>>> +
>>>
>>> nit: remove empty line.
>>>
>>>> +}
>>>> +
>>>> +static int hisi_thermal_get_temp(void *_sensor, long *temp)
>>>> +{
>>>> +    struct hisi_thermal_sensor *sensor = _sensor;
>>>> +    struct hisi_thermal_data *data = sensor->thermal;
>>>> +
>>>> +    int sensor_id = 0, i;
>>>> +    long max_temp = 0;
>>>> +
>>>> +    *temp = hisi_thermal_get_sensor_temp(data, sensor);
>>>> +
>>>> +    data->sensor_temp[sensor->id] = *temp;
>>>> +
>>>> +    for (i = 0; i < HISI_MAX_SENSORS; i++) {
>>>> +        if (data->sensor_temp[i] >= max_temp) {
>>>> +            max_temp = data->sensor_temp[i];
>>>> +            sensor_id = i;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    data->irq_bind_sensor = sensor_id;
>>>
>>> I believe this irq_bind_sensor needs to be protected by a lock.
>>>
>>> You read it in t_irq context, write it here while getting temp, and read
>>> it in bind irq.
>>>
>>> I still don't follow why you need it though, can you please explain?
>>>
>>> In any case, seams to be racy, given that it is shared by the four
>>> sensors, but you don't lock it to use it.
>>>
>>>> +
>>>> +    dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%ld, thres=%d\n",
>>>> +        sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
>>>> +    /*
>>>> +     * Bind irq to sensor for two cases:
>>>> +     *   Reenable alarm IRQ if temperature below threshold;
>>>> +     *   if irq has been enabled, always set it;
>>>> +     */
>>>> +    if (data->irq_enabled) {
>>>> +        hisi_thermal_bind_irq(data);
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    if (max_temp < sensor->thres_temp) {
>>>> +        data->irq_enabled = true;
>>>> +        hisi_thermal_bind_irq(data);
>>>> +        enable_irq(data->irq);
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static struct thermal_zone_of_device_ops hisi_of_thermal_ops = {
>>>> +    .get_temp = hisi_thermal_get_temp,
>>>> +};
>>>> +
>>>> +static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev)
>>>> +{
>>>> +    struct hisi_thermal_data *data = dev;
>>>> +
>>>> +    disable_irq_nosync(irq);
>>>> +    data->irq_enabled = false;
>>>> +
>>>> +    return IRQ_WAKE_THREAD;
>>>> +}
>>>> +
>>>> +static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>>>> +{
>>>> +    struct hisi_thermal_data *data = dev;
>>>> +    struct hisi_thermal_sensor *sensor;
>>>> +    int i;
>>>> +
>>>> +    sensor = &data->sensors[data->irq_bind_sensor];
>>>> +
>>>> +    dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
>>>> +         sensor->thres_temp / 1000);
>>>
>>> Do you really want to be printing this every time you receive an IRQ?
>>>
>>>> +
>>>> +    for (i = 0; i < data->sensors_num; i++)
>>>> +        thermal_zone_device_update(data->sensors[i].tzd);
>>>
>>>
>>> Can you please educate me on how this chip works?
>>>
>>> You receive a thermal alarm from the chip, does it really  mean you have
>>> to update all thermal zones? Or do you have a way to check which sensor
>>> generated the alarm irq?
>>>
>>>> +
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int hisi_thermal_register_sensor(struct platform_device *pdev,
>>>> +                    struct hisi_thermal_data *data,
>>>> +                    struct hisi_thermal_sensor *sensor,
>>>> +                    int index)
>>>> +{
>>>> +    int ret, i;
>>>> +
>>>> +    sensor->id = index;
>>>> +    sensor->thermal = data;
>>>> +
>>>> +    sensor->tzd = thermal_zone_of_sensor_register(&pdev->dev, sensor->id,
>>>> +                sensor, &hisi_of_thermal_ops);
>>>> +    if (IS_ERR(sensor->tzd)) {
>>>> +        ret = PTR_ERR(sensor->tzd);
>>>> +        dev_err(&pdev->dev, "failed to register sensor id %d: %d\n",
>>>> +            sensor->id, ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    sensor->trip = of_thermal_get_trip_points(sensor->tzd);
>>>> +
>>>> +    for (i = 0; i < of_thermal_get_ntrips(sensor->tzd); i++) {
>>>> +        if (sensor->trip[i].type == THERMAL_TRIP_PASSIVE) {
>>>
>>> what if you have more than one passive trip? You just care about the
>>> first?
>>>
>>>> +            sensor->thres_temp = sensor->trip[i].temperature;
>>>
>>> Looks like you use sensor->trip only for filling thres_temp here. Does
>>> it make sense to use a local variable in this function, given you won't
>>> use sensor->trip anywhere else?
>>>
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id of_hisi_thermal_match[] = {
>>>> +    { .compatible = "hisilicon,tsensor" },
>>>> +    { /* end */ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, of_hisi_thermal_match);
>>>> +
>>>> +static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor,
>>>> +                       bool on)
>>>> +{
>>>> +    struct thermal_zone_device *tzd = sensor->tzd;
>>>> +
>>>> +    tzd->ops->set_mode(tzd,
>>>> +        on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED);
>>>> +}
>>>> +
>>>> +static int hisi_thermal_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct hisi_thermal_data *data;
>>>> +    struct resource *res;
>>>> +    int i;
>>>> +    int ret;
>>>> +
>>>> +    data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>>>> +    if (!data)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    mutex_init(&data->thermal_lock);
>>>> +    data->pdev = pdev;
>>>> +
>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +    data->regs = devm_ioremap_resource(&pdev->dev, res);
>>>> +    if (IS_ERR(data->regs)) {
>>>> +        dev_err(&pdev->dev, "failed to get io address\n");
>>>> +        return PTR_ERR(data->regs);
>>>> +    }
>>>> +
>>>> +    data->irq = platform_get_irq(pdev, 0);
>>>> +    if (data->irq < 0)
>>>> +        return data->irq;
>>>> +
>>>> +    ret = devm_request_threaded_irq(&pdev->dev, data->irq,
>>>> +            hisi_thermal_alarm_irq, hisi_thermal_alarm_irq_thread,
>>>> +            0, "hisi_thermal", data);
>>>> +    if (ret < 0) {
>>>> +        dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    platform_set_drvdata(pdev, data);
>>>> +
>>>> +    data->clk = devm_clk_get(&pdev->dev, "thermal_clk");
>>>> +    if (IS_ERR(data->clk)) {
>>>> +        ret = PTR_ERR(data->clk);
>>>> +        if (ret != -EPROBE_DEFER)
>>>> +            dev_err(&pdev->dev,
>>>> +                "failed to get thermal clk: %d\n", ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    /* enable clock for thermal */
>>>> +    ret = clk_prepare_enable(data->clk);
>>>> +    if (ret) {
>>>> +        dev_err(&pdev->dev, "failed to enable thermal clk: %d\n", ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < HISI_MAX_SENSORS; ++i) {
>>>> +        ret = hisi_thermal_register_sensor(pdev, data,
>>>> +                    &data->sensors[i], i);
>>>> +        if (ret) {
>>>> +            dev_err(&pdev->dev,
>>>> +            "failed to register thermal sensor: %d\n", ret);
>>>> +            goto err_get_sensor_data;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    hisi_thermal_enable_sensor(data);
>>>> +    data->irq_enabled = true;
>>>> +
>>>> +    for (i = 0; i < data->sensors_num; i++)
>>>> +        hisi_thermal_toggle_sensor(&data->sensors[i], true);
>>>> +
>>>> +    return 0;
>>>> +
>>>> +err_get_sensor_data:
>>>> +    clk_disable_unprepare(data->clk);
>>>
>>> nit: empty line here to follow your pattern.
>>>
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int hisi_thermal_remove(struct platform_device *pdev)
>>>> +{
>>>> +    struct hisi_thermal_data *data = platform_get_drvdata(pdev);
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < data->sensors_num; i++) {
>>>> +        struct hisi_thermal_sensor *sensor = &data->sensors[i];
>>>> +
>>>> +        hisi_thermal_toggle_sensor(sensor, false);
>>>> +        thermal_zone_of_sensor_unregister(&pdev->dev, sensor->tzd);
>>>> +    }
>>>> +
>>>> +    hisi_thermal_disable_sensor(data);
>>>> +    clk_disable_unprepare(data->clk);
>>>
>>> nit: empty line here to follow your pattern.
>>>
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +#ifdef CONFIG_PM_SLEEP
>>>> +static int hisi_thermal_suspend(struct device *dev)
>>>> +{
>>>> +    struct hisi_thermal_data *data = dev_get_drvdata(dev);
>>>> +
>>>> +    hisi_thermal_disable_sensor(data);
>>>> +    data->irq_enabled = false;
>>>> +
>>>> +    clk_disable_unprepare(data->clk);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int hisi_thermal_resume(struct device *dev)
>>>> +{
>>>> +    struct hisi_thermal_data *data = dev_get_drvdata(dev);
>>>> +
>>>> +    clk_prepare_enable(data->clk);
>>>> +
>>>> +    data->irq_enabled = true;
>>>> +    hisi_thermal_enable_sensor(data);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>> +static SIMPLE_DEV_PM_OPS(hisi_thermal_pm_ops,
>>>> +             hisi_thermal_suspend, hisi_thermal_resume);
>>>> +
>>>> +static struct platform_driver hisi_thermal_driver = {
>>>> +    .driver = {
>>>> +        .name    = "hisi_thermal",
>>>> +        .owner  = THIS_MODULE,
>>>> +        .pm    = &hisi_thermal_pm_ops,
>>>> +        .of_match_table = of_hisi_thermal_match,
>>>
>>>
>>> nit: align on the '=' operator.
>>>
>>>> +    },
>>>> +    .probe        = hisi_thermal_probe,
>>>> +    .remove        = hisi_thermal_remove,
>>>> +};
>>>> +
>>>> +module_platform_driver(hisi_thermal_driver);
>>>> +
>>>> +MODULE_AUTHOR("Xinwei Kong <kong.kongxinwei@hisilicon.com>");
>>>> +MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
>>>> +MODULE_DESCRIPTION("Hisilicon thermal driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> -- 
>>>> 1.9.1
>>>>
>>>>
>>
>>
>> .
>>
> 
> 
> .
>