[PATCH v3 0/4] thermal: add driver for R-Car Gen3

[PATCH v3 0/4] thermal: add driver for R-Car Gen3

[Wolfram Sang]

Finally, here is a new series adding support for the thermal sensors on Renesas
R-Car Gen3 SoCs. The driver has been largely refactored since v2 sent by Khiem.
It is a lot simpler (no interrupts for now, will be added later), so I hope we
could still apply the 'new-driver-rule' and make it for 4.10?

Changes since last version:

Patch 1 (bindings):

* earlier versions had one node per sensor. This version has one node for all,
  but we now have one reg entry per sensor. This makes it easier to use the
  shared resources (FUSE registers, interrupts). There is no driver support
  for shared resources yet, but it will be added incrementally.

* interrupt property specified. We make clear now that all interrupts routed
  must be specified in the binding. It is up to the driver if and how it makes
  use of them. Currently, there is no interrupt support, but it will be added
  later.

* due to the well-specified reg entries, no need anymore for aliases.

* fixed register range for TSC1.

* reformatted paragraphs and fixed some typos.

Patch 2 (driver):

* removed interrupt support, needs to be refactored seperately

* adapt code to the new bindings, especially one node for all sensors.
  Similarly, we have one private structure containing n sensor structs.

* gave register bits proper namespaces, properly seperates H3 and M3-W

* refactored the way default values are determined when there are no fuses
  (which is default case currently)

* refactored the rounding routine

* removed some superfluous macros

* use pass-by-reference instead of pass-by-value where apropriate

* removed BSPisms here and there

Patches 3+4:

* DTSI updates refactored to meet the above changes

A branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/thermal-on-4.10-next

Tested on a Salvator-X with H3 and M3-W SoCs. Compared against previous and BSP versions
of the driver and the results match when turning the fan on/off.

Please test, review, comment, apply...

Thanks,

   Wolfram


Wolfram Sang (4):
  thermal: rcar_gen3_thermal: Document the R-Car Gen3
  thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver
  arm64: dts: r8a7795: Add R-Car Gen3 thermal support
  arm64: dts: r8a7796: Add R-Car Gen3 thermal support

 .../bindings/thermal/rcar-gen3-thermal.txt         |  56 ++++
 arch/arm64/boot/dts/renesas/r8a7795.dtsi           |  58 ++++
 arch/arm64/boot/dts/renesas/r8a7796.dtsi           |  58 ++++
 drivers/thermal/Kconfig                            |   9 +
 drivers/thermal/Makefile                           |   1 +
 drivers/thermal/rcar_gen3_thermal.c                | 347 +++++++++++++++++++++
 6 files changed, 529 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.txt
 create mode 100644 drivers/thermal/rcar_gen3_thermal.c

-- 
2.10.2

[PATCH v3 1/4] thermal: rcar_gen3_thermal: Document the R-Car Gen3

[Wolfram Sang]

Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since last version:

* earlier versions had one node per sensor. This version has one node for all,
  but we now have one reg entry per sensor. This makes it easier to use the
  shared resources (FUSE registers, interrupts). There is no driver support
  for shared resources yet, but it will be added incrementally.

* interrupt property specified. We make clear now that all interrupts routed
  must be specified in the binding. It is up to the driver if and how it makes
  use of them. Currently, there is no interrupt support, but it will be added
  later.

* due to the well-specified reg entries, no need anymore for aliases.

* fixed register range for TSC1.

* reformatted paragraphs and fixed some typos.

 .../bindings/thermal/rcar-gen3-thermal.txt         | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.txt

diff --git a/Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.txt b/Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.txt
new file mode 100644
index 00000000000000..07a9713ae6a757
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/rcar-gen3-thermal.txt
@@ -0,0 +1,56 @@
+* DT bindings for Renesas R-Car Gen3 Thermal Sensor driver
+
+On R-Car Gen3 SoCs, the thermal sensor controllers (TSC) control the thermal
+sensors (THS) which are the analog circuits for measuring temperature (Tj)
+inside the LSI.
+
+Required properties:
+- compatible		: "renesas,<soctype>-thermal",
+			  Examples with soctypes are:
+			    - "renesas,r8a7795-thermal" (R-Car H3)
+			    - "renesas,r8a7796-thermal" (R-Car M3-W)
+- reg			: Address ranges of the thermal registers. Each sensor
+			  needs one address range. Sorting must be done in
+			  increasing order according to datasheet, i.e.
+			  TSC1, TSC2, ...
+- clocks		: Must contain a reference to the functional clock.
+- #thermal-sensor-cells : must be <1>.
+
+Optional properties:
+
+- interrupts           : interrupts routed to the TSC (3 for H3 and M3-W)
+- power-domain		: Must contain a reference to the power domain. This
+			  property is mandatory if the thermal sensor instance
+			  is part of a controllable power domain.
+
+Example:
+
+	tsc: thermal@e6198000 {
+		compatible = "renesas,r8a7795-thermal";
+		reg = <0 0xe6198000 0 0x68>,
+		      <0 0xe61a0000 0 0x5c>,
+		      <0 0xe61a8000 0 0x5c>;
+		interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cpg CPG_MOD 522>;
+		power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+		#thermal-sensor-cells = <1>;
+		status = "okay";
+	};
+
+	thermal-zones {
+		sensor_thermal1: sensor-thermal1 {
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+			thermal-sensors = <&tsc 0>;
+
+			trips {
+				sensor1_crit: sensor1-crit {
+					temperature = <90000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};
+	};
-- 
2.10.2

[PATCH v3 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Wolfram Sang]

Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
Signed-off-by: Thao Nguyen <thao.nguyen.yb@rvc.renesas.com>
Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since last version:

* removed interrupt support, needs to be refactored seperately

* adapt code to the new bindings, especially one node for all sensors.
  Similarly, we have one private structure containing n sensor structs.

* gave register bits proper namespaces, properly seperates H3 and M3-W

* refactored the way default values are determined when there are no fuses
  (which is default case currently)

* refactored the rounding routine

* removed some superfluous macros

* use pass-by-reference instead of pass-by-value where apropriate

* removed BSPisms here and there

 drivers/thermal/Kconfig             |   9 +
 drivers/thermal/Makefile            |   1 +
 drivers/thermal/rcar_gen3_thermal.c | 347 ++++++++++++++++++++++++++++++++++++
 3 files changed, 357 insertions(+)
 create mode 100644 drivers/thermal/rcar_gen3_thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index c2c056cc7ea52e..3912d24a07b10f 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -245,6 +245,15 @@ config RCAR_THERMAL
 	  Enable this to plug the R-Car thermal sensor driver into the Linux
 	  thermal framework.
 
+config RCAR_GEN3_THERMAL
+	tristate "Renesas R-Car Gen3 thermal driver"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	depends on HAS_IOMEM
+	depends on OF
+	help
+	  Enable this to plug the R-Car Gen3 thermal sensor driver into the Linux
+	  thermal framework.
+
 config KIRKWOOD_THERMAL
 	tristate "Temperature sensor on Marvell Kirkwood SoCs"
 	depends on MACH_KIRKWOOD || COMPILE_TEST
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index c92eb22a41ff89..1216fb31ed4036 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
 obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
 obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
 obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
+obj-$(CONFIG_RCAR_GEN3_THERMAL)	+= rcar_gen3_thermal.o
 obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
 obj-y				+= samsung/
 obj-$(CONFIG_DOVE_THERMAL)  	+= dove_thermal.o
diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
new file mode 100644
index 00000000000000..6e162220672884
--- /dev/null
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -0,0 +1,347 @@
+/*
+ *  R-Car Gen3 THS thermal sensor driver
+ *  Based on rcar_thermal.c and work from Hien Dang and Khiem Nguyen.
+ *
+ * Copyright (C) 2016 Renesas Electronics Corporation.
+ * Copyright (C) 2016 Sang Engineering
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ */
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/spinlock.h>
+#include <linux/thermal.h>
+
+/* Register offsets */
+#define REG_GEN3_IRQSTR		0x04
+#define REG_GEN3_IRQMSK		0x08
+#define REG_GEN3_IRQCTL		0x0C
+#define REG_GEN3_IRQEN		0x10
+#define REG_GEN3_IRQTEMP1	0x14
+#define REG_GEN3_IRQTEMP2	0x18
+#define REG_GEN3_IRQTEMP3	0x1C
+#define REG_GEN3_CTSR		0x20
+#define REG_GEN3_THCTR		0x20
+#define REG_GEN3_TEMP		0x28
+#define REG_GEN3_THCODE1	0x50
+#define REG_GEN3_THCODE2	0x54
+#define REG_GEN3_THCODE3	0x58
+
+/* CTSR bits */
+#define CTSR_PONM	BIT(8)
+#define CTSR_AOUT	BIT(7)
+#define CTSR_THBGR	BIT(5)
+#define CTSR_VMEN	BIT(4)
+#define CTSR_VMST	BIT(1)
+#define CTSR_THSST	BIT(0)
+
+/* THCTR bits */
+#define THCTR_PONM	BIT(6)
+#define THCTR_THSST	BIT(0)
+
+#define CTEMP_MASK	0xFFF
+
+#define MCELSIUS(temp)	((temp) * 1000)
+#define GEN3_FUSE_MASK	0xFFF
+
+#define TSC_MAX_NUM	3
+
+/* Structure for thermal temperature calculation */
+struct equation_coefs {
+	long a1;
+	long b1;
+	long a2;
+	long b2;
+};
+
+struct rcar_gen3_thermal_tsc {
+	void __iomem *base;
+	struct thermal_zone_device *zone;
+	struct equation_coefs coef;
+	spinlock_t lock;
+	u32 ctemp;
+};
+
+struct rcar_gen3_thermal_priv {
+	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
+};
+
+struct rcar_gen3_thermal_data {
+	void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
+};
+
+/* Temperature calculation  */
+#define RCAR3_THERMAL_GRAN 500
+#define CODETSD(x) ((x) * 1000)
+#define TJ_1 96000L
+#define TJ_3 (-41000L)
+
+static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc, u32 reg)
+{
+	return ioread32(tsc->base + reg);
+}
+
+static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
+				     u32 reg, u32 data)
+{
+	iowrite32(data, tsc->base + reg);
+}
+
+static int _round_temp(int temp)
+{
+	int result, round_offs;
+
+	round_offs = temp >= 0 ? RCAR3_THERMAL_GRAN / 2 : -RCAR3_THERMAL_GRAN / 2;
+	result = (temp + round_offs) / RCAR3_THERMAL_GRAN;
+	return result * RCAR3_THERMAL_GRAN;
+}
+
+static void _linear_coefficient_calculation(struct rcar_gen3_thermal_tsc *tsc,
+					    int *ptat, int *thcode)
+{
+	int tj_2;
+	long a1, b1;
+	long a2, b2;
+	long a1_num, a1_den;
+	long a2_num, a2_den;
+
+	tj_2 = (CODETSD((ptat[1] - ptat[2]) * 137)
+		/ (ptat[0] - ptat[2])) - CODETSD(41);
+
+	/* calculate coefficients for linear equation */
+	a1_num = CODETSD(thcode[1] - thcode[2]);
+	a1_den = tj_2 - TJ_3;
+	a1 = (10000 * a1_num) / a1_den;
+	b1 = (10000 * thcode[2]) - ((a1 * TJ_3) / 1000);
+
+	a2_num = CODETSD(thcode[1] - thcode[0]);
+	a2_den = tj_2 - TJ_1;
+	a2 = (10000 * a2_num) / a2_den;
+	b2 = (10000 * thcode[0]) - ((a2 * TJ_1) / 1000);
+
+	tsc->coef.a1 = DIV_ROUND_CLOSEST(a1, 10);
+	tsc->coef.b1 = DIV_ROUND_CLOSEST(b1, 10);
+	tsc->coef.a2 = DIV_ROUND_CLOSEST(a2, 10);
+	tsc->coef.b2 = DIV_ROUND_CLOSEST(b2, 10);
+}
+
+static int _linear_temp_converter(struct equation_coefs *coef,
+					int temp_code)
+{
+	int temp, temp1, temp2;
+
+	temp1 = MCELSIUS((CODETSD(temp_code) - coef->b1)) / coef->a1;
+	temp2 = MCELSIUS((CODETSD(temp_code) - coef->b2)) / coef->a2;
+	temp = (temp1 + temp2) / 2;
+
+	return _round_temp(temp);
+}
+
+static int rcar_gen3_thermal_update_temp(struct rcar_gen3_thermal_tsc *tsc)
+{
+	u32 ctemp;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tsc->lock, flags);
+	ctemp = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
+	tsc->ctemp = ctemp;
+	spin_unlock_irqrestore(&tsc->lock, flags);
+
+	return 0;
+}
+
+static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
+{
+	struct rcar_gen3_thermal_tsc *tsc = devdata;
+	int ctemp;
+	unsigned long flags;
+
+	rcar_gen3_thermal_update_temp(tsc);
+
+	spin_lock_irqsave(&tsc->lock, flags);
+	ctemp = _linear_temp_converter(&tsc->coef, tsc->ctemp);
+	spin_unlock_irqrestore(&tsc->lock, flags);
+
+	if ((ctemp < MCELSIUS(-40)) || (ctemp > MCELSIUS(125)))
+		return -EIO;
+
+	*temp = ctemp;
+
+	return 0;
+}
+
+static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
+	.get_temp	= rcar_gen3_thermal_get_temp,
+};
+
+static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tsc->lock, flags);
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  0x0);
+
+	udelay(1000);
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR, CTSR_PONM);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
+			   CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN);
+
+	udelay(100);
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
+			  CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN |
+			  CTSR_VMST | CTSR_THSST);
+
+	spin_unlock_irqrestore(&tsc->lock, flags);
+}
+
+static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
+{
+	unsigned long flags;
+	u32 reg_val;
+
+	spin_lock_irqsave(&tsc->lock, flags);
+
+	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
+	reg_val &= ~THCTR_PONM;
+	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
+
+	udelay(1000);
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
+	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
+	reg_val |= THCTR_THSST;
+	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
+
+	spin_unlock_irqrestore(&tsc->lock, flags);
+}
+
+static const struct rcar_gen3_thermal_data r8a7795_data = {
+	.thermal_init = r8a7795_thermal_init,
+};
+
+static const struct rcar_gen3_thermal_data r8a7796_data = {
+	.thermal_init = r8a7796_thermal_init,
+};
+
+static const struct of_device_id rcar_gen3_thermal_dt_ids[] = {
+	{ .compatible = "renesas,r8a7795-thermal", .data = &r8a7795_data},
+	{ .compatible = "renesas,r8a7796-thermal", .data = &r8a7796_data},
+	{},
+};
+MODULE_DEVICE_TABLE(of, rcar_gen3_thermal_dt_ids);
+
+static int rcar_gen3_thermal_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	pm_runtime_put(dev);
+	pm_runtime_disable(dev);
+
+	return 0;
+}
+
+static int rcar_gen3_thermal_probe(struct platform_device *pdev)
+{
+	struct rcar_gen3_thermal_priv *priv;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct thermal_zone_device *zone;
+	int ret, i;
+	const struct rcar_gen3_thermal_data *match_data = of_device_get_match_data(dev);
+
+	/* default values if FUSEs are missing */
+	int ptat[3] = { 2351, 1509, 435 };
+	int thcode[TSC_MAX_NUM][3] = {
+		{ 3248, 2800, 2221 },
+		{ 3245, 2795, 2216 },
+		{ 3250, 2805, 2237 },
+	};
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+
+	for (i = 0; i < TSC_MAX_NUM; i++) {
+		struct rcar_gen3_thermal_tsc *tsc;
+
+		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
+		if (!tsc)
+			return -ENOMEM;
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		if (!res)
+			break;
+
+		tsc->base = devm_ioremap_resource(dev, res);
+		if (IS_ERR(tsc->base)) {
+			ret = PTR_ERR(tsc->base);
+			goto error_unregister;
+		}
+
+		priv->tscs[i] = tsc;
+		spin_lock_init(&tsc->lock);
+
+		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
+					&rcar_gen3_tz_of_ops);
+		if (IS_ERR(zone)) {
+			dev_err(dev, "Can't register thermal zone\n");
+			ret = PTR_ERR(zone);
+			goto error_unregister;
+		}
+		tsc->zone = zone;
+
+		match_data->thermal_init(tsc);
+
+		_linear_coefficient_calculation(tsc, ptat, thcode[i]);
+
+		ret = rcar_gen3_thermal_update_temp(tsc);
+
+		if (ret < 0)
+			goto error_unregister;
+	}
+
+	return 0;
+
+error_unregister:
+	rcar_gen3_thermal_remove(pdev);
+
+	return ret;
+}
+
+static struct platform_driver rcar_gen3_thermal_driver = {
+	.driver	= {
+		.name	= "rcar_gen3_thermal",
+		.of_match_table = rcar_gen3_thermal_dt_ids,
+	},
+	.probe		= rcar_gen3_thermal_probe,
+	.remove		= rcar_gen3_thermal_remove,
+};
+module_platform_driver(rcar_gen3_thermal_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("R-Car Gen3 THS thermal sensor driver");
+MODULE_AUTHOR("Wolfram Sang <wsa+renesas@sang-engineering.com>");
-- 
2.10.2

[PATCH v3 3/4] arm64: dts: r8a7795: Add R-Car Gen3 thermal support

[Wolfram Sang]

Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
Signed-off-by: Thao Nguyen <thao.nguyen.yb@rvc.renesas.com>
Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since last version:

* DTSI updates refactored to meet the above changes

 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 58 ++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 8c15040f2540d6..0699e04fbdf264 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -1541,5 +1541,63 @@
 				};
 			};
 		};
+
+		tsc: thermal@e6198000 {
+			compatible = "renesas,r8a7795-thermal";
+			reg = <0 0xe6198000 0 0x68>,
+			      <0 0xe61a0000 0 0x5c>,
+			      <0 0xe61a8000 0 0x5c>;
+			interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 522>;
+			power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+			#thermal-sensor-cells = <1>;
+			status = "okay";
+		};
+
+		thermal-zones {
+			sensor_thermal1: sensor-thermal1 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 0>;
+
+				trips {
+					sensor1_crit: sensor1-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+
+			sensor_thermal2: sensor-thermal2 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 1>;
+
+				trips {
+					sensor2_crit: sensor2-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+
+			sensor_thermal3: sensor-thermal3 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 2>;
+
+				trips {
+					sensor3_crit: sensor3-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+		};
 	};
 };
-- 
2.10.2

[PATCH v3 4/4] arm64: dts: r8a7796: Add R-Car Gen3 thermal support

[Wolfram Sang]

Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
Signed-off-by: Thao Nguyen <thao.nguyen.yb@rvc.renesas.com>
Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Changes since last version:

* DTSI updates refactored to meet the above changes

 arch/arm64/boot/dts/renesas/r8a7796.dtsi | 58 ++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
index 9217da98352565..5261f0f77ce2e9 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
@@ -251,5 +251,63 @@
 			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
 			status = "disabled";
 		};
+
+		tsc: thermal@e6198000 {
+			compatible = "renesas,r8a7796-thermal";
+			reg = <0 0xe6198000 0 0x68>,
+			      <0 0xe61a0000 0 0x5c>,
+			      <0 0xe61a8000 0 0x5c>;
+			interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 522>;
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			#thermal-sensor-cells = <1>;
+			status = "okay";
+		};
+
+		thermal-zones {
+			sensor_thermal1: sensor-thermal1 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 0>;
+
+				trips {
+					sensor1_crit: sensor1-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+
+			sensor_thermal2: sensor-thermal2 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 1>;
+
+				trips {
+					sensor2_crit: sensor2-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+
+			sensor_thermal3: sensor-thermal3 {
+				polling-delay-passive = <250>;
+				polling-delay = <1000>;
+				thermal-sensors = <&tsc 2>;
+
+				trips {
+					sensor3_crit: sensor3-crit {
+						temperature = <90000>;
+						hysteresis = <2000>;
+						type = "critical";
+					};
+				};
+			};
+		};
 	};
 };
-- 
2.10.2

Re: [PATCH v3 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Eduardo Valentin]

Wolfram,

Thanks for sending the driver. Questions on the driver locking strategy as follows.

On Mon, Nov 28, 2016 at 10:09:22PM +0100, Wolfram Sang wrote:

No commit message ? :-( generally speaking here, it is a good practice
to describe what you are doing, what you have considered, and what you
have tested, just for the sake of code history/documentation.

> Signed-off-by: Hien Dang <hien.dang.eb@renesas.com>
> Signed-off-by: Thao Nguyen <thao.nguyen.yb@rvc.renesas.com>
> Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> Changes since last version:
> 
> * removed interrupt support, needs to be refactored seperately
> 
> * adapt code to the new bindings, especially one node for all sensors.
>   Similarly, we have one private structure containing n sensor structs.
> 
> * gave register bits proper namespaces, properly seperates H3 and M3-W
> 
> * refactored the way default values are determined when there are no fuses
>   (which is default case currently)
> 
> * refactored the rounding routine
> 
> * removed some superfluous macros
> 
> * use pass-by-reference instead of pass-by-value where apropriate
> 
> * removed BSPisms here and there
> 
>  drivers/thermal/Kconfig             |   9 +
>  drivers/thermal/Makefile            |   1 +
>  drivers/thermal/rcar_gen3_thermal.c | 347 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 357 insertions(+)
>  create mode 100644 drivers/thermal/rcar_gen3_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index c2c056cc7ea52e..3912d24a07b10f 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -245,6 +245,15 @@ config RCAR_THERMAL
>  	  Enable this to plug the R-Car thermal sensor driver into the Linux
>  	  thermal framework.
>  
> +config RCAR_GEN3_THERMAL
> +	tristate "Renesas R-Car Gen3 thermal driver"
> +	depends on ARCH_RENESAS || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	depends on OF
> +	help
> +	  Enable this to plug the R-Car Gen3 thermal sensor driver into the Linux
> +	  thermal framework.
> +
>  config KIRKWOOD_THERMAL
>  	tristate "Temperature sensor on Marvell Kirkwood SoCs"
>  	depends on MACH_KIRKWOOD || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index c92eb22a41ff89..1216fb31ed4036 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM)	+= qcom-spmi-temp-alarm.o
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
>  obj-$(CONFIG_ROCKCHIP_THERMAL)	+= rockchip_thermal.o
>  obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
> +obj-$(CONFIG_RCAR_GEN3_THERMAL)	+= rcar_gen3_thermal.o
>  obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
>  obj-y				+= samsung/
>  obj-$(CONFIG_DOVE_THERMAL)  	+= dove_thermal.o
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> new file mode 100644
> index 00000000000000..6e162220672884
> --- /dev/null
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -0,0 +1,347 @@
> +/*
> + *  R-Car Gen3 THS thermal sensor driver
> + *  Based on rcar_thermal.c and work from Hien Dang and Khiem Nguyen.
> + *
> + * Copyright (C) 2016 Renesas Electronics Corporation.
> + * Copyright (C) 2016 Sang Engineering
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + */
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/spinlock.h>
> +#include <linux/thermal.h>
> +
> +/* Register offsets */
> +#define REG_GEN3_IRQSTR		0x04
> +#define REG_GEN3_IRQMSK		0x08
> +#define REG_GEN3_IRQCTL		0x0C
> +#define REG_GEN3_IRQEN		0x10
> +#define REG_GEN3_IRQTEMP1	0x14
> +#define REG_GEN3_IRQTEMP2	0x18
> +#define REG_GEN3_IRQTEMP3	0x1C
> +#define REG_GEN3_CTSR		0x20
> +#define REG_GEN3_THCTR		0x20
> +#define REG_GEN3_TEMP		0x28
> +#define REG_GEN3_THCODE1	0x50
> +#define REG_GEN3_THCODE2	0x54
> +#define REG_GEN3_THCODE3	0x58
> +
> +/* CTSR bits */
> +#define CTSR_PONM	BIT(8)
> +#define CTSR_AOUT	BIT(7)
> +#define CTSR_THBGR	BIT(5)
> +#define CTSR_VMEN	BIT(4)
> +#define CTSR_VMST	BIT(1)
> +#define CTSR_THSST	BIT(0)
> +
> +/* THCTR bits */
> +#define THCTR_PONM	BIT(6)
> +#define THCTR_THSST	BIT(0)
> +
> +#define CTEMP_MASK	0xFFF
> +
> +#define MCELSIUS(temp)	((temp) * 1000)
> +#define GEN3_FUSE_MASK	0xFFF
> +
> +#define TSC_MAX_NUM	3
> +
> +/* Structure for thermal temperature calculation */
> +struct equation_coefs {
> +	long a1;
> +	long b1;
> +	long a2;
> +	long b2;
> +};
> +
> +struct rcar_gen3_thermal_tsc {
> +	void __iomem *base;
> +	struct thermal_zone_device *zone;
> +	struct equation_coefs coef;
> +	spinlock_t lock;
> +	u32 ctemp;
> +};
> +
> +struct rcar_gen3_thermal_priv {
> +	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
> +};
> +
> +struct rcar_gen3_thermal_data {
> +	void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc);
> +};
> +
> +/* Temperature calculation  */
> +#define RCAR3_THERMAL_GRAN 500
> +#define CODETSD(x) ((x) * 1000)
> +#define TJ_1 96000L
> +#define TJ_3 (-41000L)
> +
> +static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc, u32 reg)
> +{
> +	return ioread32(tsc->base + reg);
> +}
> +
> +static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
> +				     u32 reg, u32 data)
> +{
> +	iowrite32(data, tsc->base + reg);
> +}
> +
> +static int _round_temp(int temp)
> +{
> +	int result, round_offs;
> +
> +	round_offs = temp >= 0 ? RCAR3_THERMAL_GRAN / 2 : -RCAR3_THERMAL_GRAN / 2;
> +	result = (temp + round_offs) / RCAR3_THERMAL_GRAN;
> +	return result * RCAR3_THERMAL_GRAN;
> +}
> +
> +static void _linear_coefficient_calculation(struct rcar_gen3_thermal_tsc *tsc,
> +					    int *ptat, int *thcode)
> +{
> +	int tj_2;
> +	long a1, b1;
> +	long a2, b2;
> +	long a1_num, a1_den;
> +	long a2_num, a2_den;
> +
> +	tj_2 = (CODETSD((ptat[1] - ptat[2]) * 137)
> +		/ (ptat[0] - ptat[2])) - CODETSD(41);
> +
> +	/* calculate coefficients for linear equation */
> +	a1_num = CODETSD(thcode[1] - thcode[2]);
> +	a1_den = tj_2 - TJ_3;
> +	a1 = (10000 * a1_num) / a1_den;
> +	b1 = (10000 * thcode[2]) - ((a1 * TJ_3) / 1000);
> +
> +	a2_num = CODETSD(thcode[1] - thcode[0]);
> +	a2_den = tj_2 - TJ_1;
> +	a2 = (10000 * a2_num) / a2_den;
> +	b2 = (10000 * thcode[0]) - ((a2 * TJ_1) / 1000);
> +
> +	tsc->coef.a1 = DIV_ROUND_CLOSEST(a1, 10);
> +	tsc->coef.b1 = DIV_ROUND_CLOSEST(b1, 10);
> +	tsc->coef.a2 = DIV_ROUND_CLOSEST(a2, 10);
> +	tsc->coef.b2 = DIV_ROUND_CLOSEST(b2, 10);
> +}
> +
> +static int _linear_temp_converter(struct equation_coefs *coef,
> +					int temp_code)
> +{
> +	int temp, temp1, temp2;
> +
> +	temp1 = MCELSIUS((CODETSD(temp_code) - coef->b1)) / coef->a1;
> +	temp2 = MCELSIUS((CODETSD(temp_code) - coef->b2)) / coef->a2;
> +	temp = (temp1 + temp2) / 2;
> +
> +	return _round_temp(temp);
> +}
> +
> +static int rcar_gen3_thermal_update_temp(struct rcar_gen3_thermal_tsc *tsc)
> +{
> +	u32 ctemp;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tsc->lock, flags);

Why do you need a full blown spin lock irqsave? Can this locking be
solved by a simple mutex?

> +	ctemp = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> +	tsc->ctemp = ctemp;
> +	spin_unlock_irqrestore(&tsc->lock, flags);

I see it is a very short critical section, but still, do you really need
to disable irqs to read this sensor?

> +
> +	return 0;
> +}
> +
> +static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
> +{
> +	struct rcar_gen3_thermal_tsc *tsc = devdata;
> +	int ctemp;
> +	unsigned long flags;
> +
> +	rcar_gen3_thermal_update_temp(tsc);
> +
> +	spin_lock_irqsave(&tsc->lock, flags);
> +	ctemp = _linear_temp_converter(&tsc->coef, tsc->ctemp);
> +	spin_unlock_irqrestore(&tsc->lock, flags);
> +
> +	if ((ctemp < MCELSIUS(-40)) || (ctemp > MCELSIUS(125)))
> +		return -EIO;
> +
> +	*temp = ctemp;
> +
> +	return 0;
> +}
> +
> +static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
> +	.get_temp	= rcar_gen3_thermal_get_temp,
> +};
> +
> +static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tsc->lock, flags);
> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  0x0);
> +
> +	udelay(1000);

Why do you disable irqs, then busyloop for 1ms?

> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR, CTSR_PONM);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
> +			   CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN);
> +
> +	udelay(100);

1.1ms?

> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
> +			  CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN |
> +			  CTSR_VMST | CTSR_THSST);
> +
> +	spin_unlock_irqrestore(&tsc->lock, flags);

again, why not a simpler way, by using a mutex?

> +}
> +
> +static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> +{
> +	unsigned long flags;
> +	u32 reg_val;
> +
> +	spin_lock_irqsave(&tsc->lock, flags);
> +
> +	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
> +	reg_val &= ~THCTR_PONM;
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
> +
> +	udelay(1000);
> +
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
> +	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
> +	reg_val |= THCTR_THSST;
> +	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
> +
> +	spin_unlock_irqrestore(&tsc->lock, flags);
>

 ditto

+}
> +
> +static const struct rcar_gen3_thermal_data r8a7795_data = {
> +	.thermal_init = r8a7795_thermal_init,
> +};
> +
> +static const struct rcar_gen3_thermal_data r8a7796_data = {
> +	.thermal_init = r8a7796_thermal_init,
> +};
> +
> +static const struct of_device_id rcar_gen3_thermal_dt_ids[] = {
> +	{ .compatible = "renesas,r8a7795-thermal", .data = &r8a7795_data},
> +	{ .compatible = "renesas,r8a7796-thermal", .data = &r8a7796_data},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rcar_gen3_thermal_dt_ids);
> +
> +static int rcar_gen3_thermal_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	pm_runtime_put(dev);
> +	pm_runtime_disable(dev);
> +
> +	return 0;
> +}
> +
> +static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> +{
> +	struct rcar_gen3_thermal_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct thermal_zone_device *zone;
> +	int ret, i;
> +	const struct rcar_gen3_thermal_data *match_data = of_device_get_match_data(dev);
> +
> +	/* default values if FUSEs are missing */
> +	int ptat[3] = { 2351, 1509, 435 };
> +	int thcode[TSC_MAX_NUM][3] = {
> +		{ 3248, 2800, 2221 },
> +		{ 3245, 2795, 2216 },
> +		{ 3250, 2805, 2237 },
> +	};

are these fuses valid for both? 7796 and 7797? or would you need a
different array for each version?

> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +
> +	for (i = 0; i < TSC_MAX_NUM; i++) {
> +		struct rcar_gen3_thermal_tsc *tsc;
> +
> +		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> +		if (!tsc)
> +			return -ENOMEM;
> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		if (!res)
> +			break;
> +
> +		tsc->base = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(tsc->base)) {
> +			ret = PTR_ERR(tsc->base);
> +			goto error_unregister;
> +		}
> +
> +		priv->tscs[i] = tsc;
> +		spin_lock_init(&tsc->lock);
> +
> +		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
> +					&rcar_gen3_tz_of_ops);
> +		if (IS_ERR(zone)) {
> +			dev_err(dev, "Can't register thermal zone\n");
> +			ret = PTR_ERR(zone);
> +			goto error_unregister;
> +		}
> +		tsc->zone = zone;
> +
> +		match_data->thermal_init(tsc);
> +
> +		_linear_coefficient_calculation(tsc, ptat, thcode[i]);
> +
> +		ret = rcar_gen3_thermal_update_temp(tsc);
> +
> +		if (ret < 0)
> +			goto error_unregister;
> +	}
> +
> +	return 0;
> +
> +error_unregister:
> +	rcar_gen3_thermal_remove(pdev);
> +
> +	return ret;
> +}
> +
> +static struct platform_driver rcar_gen3_thermal_driver = {
> +	.driver	= {
> +		.name	= "rcar_gen3_thermal",
> +		.of_match_table = rcar_gen3_thermal_dt_ids,
> +	},
> +	.probe		= rcar_gen3_thermal_probe,
> +	.remove		= rcar_gen3_thermal_remove,
> +};
> +module_platform_driver(rcar_gen3_thermal_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("R-Car Gen3 THS thermal sensor driver");
> +MODULE_AUTHOR("Wolfram Sang <wsa+renesas@sang-engineering.com>");
> -- 
> 2.10.2
> 

Re: [PATCH v3 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Wolfram Sang]

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

Hi Eduardo,

> No commit message ? :-( generally speaking here, it is a good practice
> to describe what you are doing, what you have considered, and what you
> have tested, just for the sake of code history/documentation.

OK, can do.

> > +	spin_lock_irqsave(&tsc->lock, flags);
> 
> Why do you need a full blown spin lock irqsave? Can this locking be
> solved by a simple mutex?

Currently, it can be a mutex, yes. This is a "left-over" from the
version which had interrupt support. I am not fully done with
refactoring the irqs, but I thought it is likely we need this lock then
again, so I chose to leave it. I can use a mutex for now if you prefer.

> 
> > +static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&tsc->lock, flags);
> > +
> > +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
> > +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  0x0);
> > +
> > +	udelay(1000);
> 
> Why do you disable irqs, then busyloop for 1ms?

Uh yes, this is ugly. I don't think we need to lock during init, but
I'll double check later.

> > +	/* default values if FUSEs are missing */
> > +	int ptat[3] = { 2351, 1509, 435 };
> > +	int thcode[TSC_MAX_NUM][3] = {
> > +		{ 3248, 2800, 2221 },
> > +		{ 3245, 2795, 2216 },
> > +		{ 3250, 2805, 2237 },
> > +	};
> 
> are these fuses valid for both? 7796 and 7797? or would you need a
> different array for each version?

According to the information I have today, it is the same array. And all
later versions are promised to have fuse registers. This is already
documented, but such HW is not available to me currently.

Thanks for the quick review, very much appreciated!

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Nov 28, 2016 at 10:09 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> --- /dev/null
> +++ b/drivers/thermal/rcar_gen3_thermal.c

> +/* Structure for thermal temperature calculation */
> +struct equation_coefs {
> +       long a1;
> +       long b1;
> +       long a2;
> +       long b2;

Why long? Long has a different size for 32-bit and 64-bit platforms.
I know this is a driver for arm64, but if you need 64-bits, you want to make
this clear using s64, or long long.

> +static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc, u32 reg)

What a long function name...

> +{
> +       return ioread32(tsc->base + reg);
> +}
> +
> +static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
> +                                    u32 reg, u32 data)
> +{
> +       iowrite32(data, tsc->base + reg);
> +}

... so using the "convenience" wrappers is more (typing) work than using
io{read,write}32() directly?

> +static void _linear_coefficient_calculation(struct rcar_gen3_thermal_tsc *tsc,
> +                                           int *ptat, int *thcode)
> +{
> +       int tj_2;
> +       long a1, b1;
> +       long a2, b2;
> +       long a1_num, a1_den;
> +       long a2_num, a2_den;

s64?

> +       tj_2 = (CODETSD((ptat[1] - ptat[2]) * 137)
> +               / (ptat[0] - ptat[2])) - CODETSD(41);

Isn't "* 1000" easier to read then CODETSD()?

> +       /* calculate coefficients for linear equation */
> +       a1_num = CODETSD(thcode[1] - thcode[2]);
> +       a1_den = tj_2 - TJ_3;
> +       a1 = (10000 * a1_num) / a1_den;
> +       b1 = (10000 * thcode[2]) - ((a1 * TJ_3) / 1000);

Rounding needed for / 1000?

> +       a2_num = CODETSD(thcode[1] - thcode[0]);
> +       a2_den = tj_2 - TJ_1;
> +       a2 = (10000 * a2_num) / a2_den;
> +       b2 = (10000 * thcode[0]) - ((a2 * TJ_1) / 1000);

Idem.

> +static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> +{
> +       struct rcar_gen3_thermal_priv *priv;
> +       struct device *dev = &pdev->dev;
> +       struct resource *res;
> +       struct thermal_zone_device *zone;
> +       int ret, i;

unsigned int i;

> +       const struct rcar_gen3_thermal_data *match_data = of_device_get_match_data(dev);
> +
> +       /* default values if FUSEs are missing */
> +       int ptat[3] = { 2351, 1509, 435 };

unsigned?

> +       int thcode[TSC_MAX_NUM][3] = {

unsigned?

> +               { 3248, 2800, 2221 },
> +               { 3245, 2795, 2216 },
> +               { 3250, 2805, 2237 },
> +       };
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, priv);
> +
> +       pm_runtime_enable(dev);
> +       pm_runtime_get_sync(dev);
> +
> +       for (i = 0; i < TSC_MAX_NUM; i++) {
> +               struct rcar_gen3_thermal_tsc *tsc;
> +
> +               tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> +               if (!tsc)
> +                       return -ENOMEM;

Missing pm_runtime_put() etc.?

ret = -ENOMEM;
goto error_unregister;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Wolfram Sang]

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

Hi Geert,

thanks for the prompt review!

> > +/* Structure for thermal temperature calculation */
> > +struct equation_coefs {
> > +       long a1;
> > +       long b1;
> > +       long a2;
> > +       long b2;
> 
> Why long? Long has a different size for 32-bit and 64-bit platforms.
> I know this is a driver for arm64, but if you need 64-bits, you want to make
> this clear using s64, or long long.

I'll check if int will do.

> > +static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
> > +                                    u32 reg, u32 data)
> > +{
> > +       iowrite32(data, tsc->base + reg);
> > +}
> 
> ... so using the "convenience" wrappers is more (typing) work than using
> io{read,write}32() directly?

TBH I don't favor such macros, but since they are in quite some Renesas
drivers, I kept them in the first take for consistency reasons.

> > +       tj_2 = (CODETSD((ptat[1] - ptat[2]) * 137)
> > +               / (ptat[0] - ptat[2])) - CODETSD(41);
> 
> Isn't "* 1000" easier to read then CODETSD()?

Probably. I'd think there can be done even more to make this code a tad
more readable. For the first take, I'd like to keep these formulas,
though. They come from the BSP and are the only documentation about how
to calculate the temperature which we currently have. Changing them
will obsolete all the testing done so far.


> > +       /* calculate coefficients for linear equation */
> > +       a1_num = CODETSD(thcode[1] - thcode[2]);
> > +       a1_den = tj_2 - TJ_3;
> > +       a1 = (10000 * a1_num) / a1_den;
> > +       b1 = (10000 * thcode[2]) - ((a1 * TJ_3) / 1000);
> 
> Rounding needed for / 1000?

Ditto.

> > +static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> > +{
> > +       struct rcar_gen3_thermal_priv *priv;
> > +       struct device *dev = &pdev->dev;
> > +       struct resource *res;
> > +       struct thermal_zone_device *zone;
> > +       int ret, i;
> 
> unsigned int i;

I'll likely produce more bugs if 'i' is not an int ;)

> 
> > +       const struct rcar_gen3_thermal_data *match_data = of_device_get_match_data(dev);
> > +
> > +       /* default values if FUSEs are missing */
> > +       int ptat[3] = { 2351, 1509, 435 };
> 
> unsigned?
> 
> > +       int thcode[TSC_MAX_NUM][3] = {
> 
> unsigned?

Had that before, didn't work. Since the calculation involves
substraction with other ints, I prefer to have them all the same type as
the fix.

> > +               tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> > +               if (!tsc)
> > +                       return -ENOMEM;
> 
> Missing pm_runtime_put() etc.?
> 
> ret = -ENOMEM;
> goto error_unregister;

Yes!

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Eduardo Valentin]

Hey,

On Tue, Nov 29, 2016 at 09:32:42AM +0100, Wolfram Sang wrote:
> Hi Eduardo,
> 
> > No commit message ? :-( generally speaking here, it is a good practice
> > to describe what you are doing, what you have considered, and what you
> > have tested, just for the sake of code history/documentation.
> 
> OK, can do.

cool!

> 
> > > +	spin_lock_irqsave(&tsc->lock, flags);
> > 
> > Why do you need a full blown spin lock irqsave? Can this locking be
> > solved by a simple mutex?
> 
> Currently, it can be a mutex, yes. This is a "left-over" from the
> version which had interrupt support. I am not fully done with
> refactoring the irqs, but I thought it is likely we need this lock then
> again, so I chose to leave it. I can use a mutex for now if you prefer.

yes, yield the processor when possible, please. So, if your locking
don't really need to disable IRQs, then avoid doing it. If a mutex is
enough, use it.

> 
> > 
> > > +static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&tsc->lock, flags);
> > > +
> > > +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
> > > +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  0x0);
> > > +
> > > +	udelay(1000);
> > 
> > Why do you disable irqs, then busyloop for 1ms?
> 
> Uh yes, this is ugly. I don't think we need to lock during init, but
> I'll double check later.
> 

OK. You could probably leave the (mutex) lock, if you think you need it.
But also consider using usleep_range(1000) instead. Have a look at:
Documentation/timers/timers-howto.txt

for a better explanation.


> > > +	/* default values if FUSEs are missing */
> > > +	int ptat[3] = { 2351, 1509, 435 };
> > > +	int thcode[TSC_MAX_NUM][3] = {
> > > +		{ 3248, 2800, 2221 },
> > > +		{ 3245, 2795, 2216 },
> > > +		{ 3250, 2805, 2237 },
> > > +	};
> > 
> > are these fuses valid for both? 7796 and 7797? or would you need a
> > different array for each version?
> 
> According to the information I have today, it is the same array. And all
> later versions are promised to have fuse registers. This is already
> documented, but such HW is not available to me currently.
> 

Ok. If you have the confirmation for that, then it is fine. I just asked
because cases I saw (different manufacturer of course) would have different values
to be programmed, depending on the chip version.

Anyways, just checking.

> Thanks for the quick review, very much appreciated!
> 
>    Wolfram
>