[PATCH v2 0/7] thermal: rcar_gen3_thermal: add support for interrupt triggerd trip points

[PATCH v2 0/7] thermal: rcar_gen3_thermal: add support for interrupt triggerd trip points

[Niklas Söderlund]

Hi,

This series adds support for hardware backed trip point windows. It is
based on top of v4.11-rc1 and is tested on R-Car H3 and M3-W.

The series starts out by fixing three issues (1/7, 2/7, 3/7) that should
have been fixed by me before the initial driver where submitted to
upstream. Sorry for not spotting the issues sooner.

The series then extends the rcar_gen3_thermal driver with hardware
interrupts for trip point windows by implementing the .set_trips()
callback of struct thermal_zone_of_device_ops (4/7, 5/7). It then adds
suspend and resume handlers so that the hardware interrupts are
preserved across suspend/resume cycles (6/7, 7/7).

* Changes since v1
- Added check that at least one TSC is found
- Record number of TSC in private data structure
- Request interrupts separately from TSCs
- Have only one function which controls both IRQ on and off
- Check return value from of_thermal_get_ntrips()
- Fixed spelling in commit messages
- Add Reviewed-by tags

Niklas Söderlund (7):
  thermal: rcar_gen3_thermal: add delay in .thermal_init on r8a7796
  thermal: rcar_gen3_thermal: remove unneeded mutex
  thermal: rcar_gen3_thermal: check that TSC exists before memory
    allocation
  thermal: rcar_gen3_thermal: record and check number of TSCs found
  thermal: rcar_gen3_thermal: enable hardware interrupts for trip points
  thermal: rcar_gen3_thermal: store device match data in private
    structure
  thermal: rcar_gen3_thermal: add suspend and resume support

 drivers/thermal/rcar_gen3_thermal.c | 195 +++++++++++++++++++++++++++++++++---
 1 file changed, 180 insertions(+), 15 deletions(-)

-- 
2.12.0

[PATCH v2 1/7] thermal: rcar_gen3_thermal: add delay in .thermal_init on r8a7796

[Niklas Söderlund]

The .thermal_init needs to be delayed a short amount of time to allow
for the TEMP register to contain something useful. If it's not delayed
these warnings are common during boot:

thermal thermal_zone0: failed to read out thermal zone (-5)
thermal thermal_zone1: failed to read out thermal zone (-5)
thermal thermal_zone2: failed to read out thermal zone (-5)

The warnings are triggered by the first call to .get_temp() while the
TEMP register contains 0 and rcar_gen3_thermal_get_temp() returns -EIO
since a TEMP value of 0 will result in a temperature reading which is
out of specifications.

This should have been done in the initial commit which adds the driver
as the same issue was found and corrected for r8a7795.

Fixes: 564e73d283af9d4c ("thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver")
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/thermal/rcar_gen3_thermal.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index d33c845244b1d819..ec477d47d0bae8e5 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -222,6 +222,8 @@ static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
 	reg_val |= THCTR_THSST;
 	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
+
+	usleep_range(1000, 2000);
 }
 
 static const struct rcar_gen3_thermal_data r8a7795_data = {
-- 
2.12.0

[PATCH v2 2/7] thermal: rcar_gen3_thermal: remove unneeded mutex

[Niklas Söderlund]

There is no point in protecting a register read with a lock. This is
most likely a leftover from when the driver was reworked before submitted
for upstream.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/thermal/rcar_gen3_thermal.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index ec477d47d0bae8e5..cb5c362c00009921 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -20,7 +20,6 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -72,7 +71,6 @@ struct rcar_gen3_thermal_tsc {
 	void __iomem *base;
 	struct thermal_zone_device *zone;
 	struct equation_coefs coef;
-	struct mutex lock;
 };
 
 struct rcar_gen3_thermal_priv {
@@ -163,16 +161,12 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
 	u32 reg;
 
 	/* Read register and convert to mili Celsius */
-	mutex_lock(&tsc->lock);
-
 	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
 
 	val1 = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b1, tsc->coef.a1);
 	val2 = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b2, tsc->coef.a2);
 	mcelsius = FIXPT_TO_MCELSIUS((val1 + val2) / 2);
 
-	mutex_unlock(&tsc->lock);
-
 	/* Make sure we are inside specifications */
 	if ((mcelsius < MCELSIUS(-40)) || (mcelsius > MCELSIUS(125)))
 		return -EIO;
@@ -299,7 +293,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		}
 
 		priv->tscs[i] = tsc;
-		mutex_init(&tsc->lock);
 
 		match_data->thermal_init(tsc);
 		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);
-- 
2.12.0

[PATCH v2 3/7] thermal: rcar_gen3_thermal: check that TSC exists before memory allocation

[Niklas Söderlund]

Move the check for a TSC resource before allocating memory for a new
TSC. If no TSC is found there is little point in allocating memory for
it.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_gen3_thermal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index cb5c362c00009921..9b6bc03dd142a8dd 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -276,16 +276,16 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	for (i = 0; i < TSC_MAX_NUM; i++) {
 		struct rcar_gen3_thermal_tsc *tsc;
 
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		if (!res)
+			break;
+
 		tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
 		if (!tsc) {
 			ret = -ENOMEM;
 			goto error_unregister;
 		}
 
-		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);
-- 
2.12.0

[PATCH v2 4/7] thermal: rcar_gen3_thermal: record and check number of TSCs found

[Niklas Söderlund]

Record how many TSCs are found in struct rcar_gen3_thermal_priv, this is
needed to be able to add hardware interrupts for trip points later. Also
add a check to make sure at least one TSC is found.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_gen3_thermal.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 9b6bc03dd142a8dd..3b6e85abfadc61a3 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -74,6 +74,7 @@ struct rcar_gen3_thermal_tsc {
 };
 
 struct rcar_gen3_thermal_priv {
+	unsigned int num_tscs;
 	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
 };
 
@@ -305,6 +306,12 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 			goto error_unregister;
 		}
 		tsc->zone = zone;
+		priv->num_tscs++;
+	}
+
+	if (!priv->num_tscs) {
+		ret = -ENODEV;
+		goto error_unregister;
 	}
 
 	return 0;
-- 
2.12.0

[PATCH v2 5/7] thermal: rcar_gen3_thermal: enable hardware interrupts for trip points

[Niklas Söderlund]

Enable hardware trip points by implementing the set_trips callback. The
thermal core will take care of setting the initial trip point window and
to update it once the driver reports a TSC has moved outside it.

The interrupt structure for this device is a bit odd. There is not a
dedicated IRQ for each TSC, instead the interrupts are shared between
all TSCs. IRQn is fired if the temp monitored in IRQTEMPn is reached in
any of the TSCs, example IRQ3 is fired if temperature in IRQTEMP3 is
reached in either TSC0, TSC1 or TSC2.

For this reason the usage of interrupts in this driver is an all-on or
all-off design. When an interrupt happens all TSCs are checked and all
thermal zones are updated. This could be refined to be more fine grained
but the thermal core takes care of only updating the thermal zones that
have left their trip point window.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_gen3_thermal.c | 129 +++++++++++++++++++++++++++++++++++-
 1 file changed, 128 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 3b6e85abfadc61a3..dc019b7873273cf2 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -23,8 +23,11 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/spinlock.h>
 #include <linux/thermal.h>
 
+#include "thermal_core.h"
+
 /* Register offsets */
 #define REG_GEN3_IRQSTR		0x04
 #define REG_GEN3_IRQMSK		0x08
@@ -40,6 +43,14 @@
 #define REG_GEN3_THCODE2	0x54
 #define REG_GEN3_THCODE3	0x58
 
+/* IRQ{STR,MSK,EN} bits */
+#define IRQ_TEMP1		BIT(0)
+#define IRQ_TEMP2		BIT(1)
+#define IRQ_TEMP3		BIT(2)
+#define IRQ_TEMPD1		BIT(3)
+#define IRQ_TEMPD2		BIT(4)
+#define IRQ_TEMPD3		BIT(5)
+
 /* CTSR bits */
 #define CTSR_PONM	BIT(8)
 #define CTSR_AOUT	BIT(7)
@@ -74,6 +85,7 @@ struct rcar_gen3_thermal_tsc {
 };
 
 struct rcar_gen3_thermal_priv {
+	spinlock_t lock; /* Protect interrupts on and off */
 	unsigned int num_tscs;
 	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
 };
@@ -113,6 +125,7 @@ static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
 
 #define FIXPT_SHIFT 7
 #define FIXPT_INT(_x) ((_x) << FIXPT_SHIFT)
+#define INT_FIXPT(_x) ((_x) >> FIXPT_SHIFT)
 #define FIXPT_DIV(_a, _b) DIV_ROUND_CLOSEST(((_a) << FIXPT_SHIFT), (_b))
 #define FIXPT_TO_MCELSIUS(_x) ((_x) * 1000 >> FIXPT_SHIFT)
 
@@ -178,10 +191,89 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
 	return 0;
 }
 
+static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc,
+					      int mcelsius)
+{
+	int celsius, val1, val2;
+
+	celsius = DIV_ROUND_CLOSEST(mcelsius, 1000);
+	val1 = celsius * tsc->coef.a1 + tsc->coef.b1;
+	val2 = celsius * tsc->coef.a2 + tsc->coef.b2;
+
+	return INT_FIXPT((val1 + val2) / 2);
+}
+
+static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
+{
+	struct rcar_gen3_thermal_tsc *tsc = devdata;
+
+	low = clamp_val(low, -40000, 125000);
+	high = clamp_val(high, -40000, 125000);
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1,
+				rcar_gen3_thermal_mcelsius_to_temp(tsc, low));
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2,
+				rcar_gen3_thermal_mcelsius_to_temp(tsc, high));
+
+	return 0;
+}
+
 static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
 	.get_temp	= rcar_gen3_thermal_get_temp,
+	.set_trips	= rcar_gen3_thermal_set_trips,
 };
 
+static void rcar_thermal_irq_set(struct rcar_gen3_thermal_priv *priv, int on)
+{
+	unsigned int i;
+	u32 val;
+
+	val = on ? IRQ_TEMPD1 | IRQ_TEMP2 : 0;
+
+	for (i = 0; i < priv->num_tscs; i++)
+		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK, val);
+}
+
+static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data)
+{
+	struct rcar_gen3_thermal_priv *priv = data;
+	unsigned long flags;
+	u32 status;
+	int i, ret = IRQ_HANDLED;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	for (i = 0; i < priv->num_tscs; i++) {
+		status = rcar_gen3_thermal_read(priv->tscs[i], REG_GEN3_IRQSTR);
+		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQSTR, 0);
+		if (status)
+			ret = IRQ_WAKE_THREAD;
+	}
+
+	if (ret == IRQ_WAKE_THREAD)
+		rcar_thermal_irq_set(priv, 0);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return ret;
+}
+
+static irqreturn_t rcar_gen3_thermal_irq_thread(int irq, void *data)
+{
+	struct rcar_gen3_thermal_priv *priv = data;
+	unsigned long flags;
+	int i;
+
+	for (i = 0; i < priv->num_tscs; i++)
+		thermal_zone_device_update(priv->tscs[i]->zone,
+					   THERMAL_EVENT_UNSPECIFIED);
+
+	spin_lock_irqsave(&priv->lock, flags);
+	rcar_thermal_irq_set(priv, 1);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
 static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 {
 	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
@@ -190,7 +282,11 @@ static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 	usleep_range(1000, 2000);
 
 	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_IRQMSK, 0);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQEN, IRQ_TEMPD1 | IRQ_TEMP2);
+
 	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
 				CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN);
 
@@ -214,6 +310,9 @@ static void r8a7796_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 	usleep_range(1000, 2000);
 
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, 0);
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQEN, IRQ_TEMPD1 | IRQ_TEMP2);
+
 	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
 	reg_val |= THCTR_THSST;
 	rcar_gen3_thermal_write(tsc, REG_GEN3_THCTR, reg_val);
@@ -252,7 +351,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	struct thermal_zone_device *zone;
-	int ret, i;
+	int ret, irq, i;
+	char *irqname;
 	const struct rcar_gen3_thermal_data *match_data =
 		of_device_get_match_data(dev);
 
@@ -269,8 +369,27 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	spin_lock_init(&priv->lock);
+
 	platform_set_drvdata(pdev, priv);
 
+	for (i = 0; i < 2; i++) {
+		irq = platform_get_irq(pdev, i);
+		if (irq < 0)
+			return irq;
+
+		irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
+					 dev_name(dev), i);
+		if (!irqname)
+			return -ENOMEM;
+
+		ret = devm_request_threaded_irq(dev, irq, rcar_gen3_thermal_irq,
+						rcar_gen3_thermal_irq_thread,
+						IRQF_SHARED, irqname, priv);
+		if (ret)
+			return ret;
+	}
+
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
@@ -307,6 +426,12 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		}
 		tsc->zone = zone;
 		priv->num_tscs++;
+
+		ret = of_thermal_get_ntrips(tsc->zone);
+		if (ret < 0)
+			goto error_unregister;
+
+		dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret);
 	}
 
 	if (!priv->num_tscs) {
@@ -314,6 +439,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		goto error_unregister;
 	}
 
+	rcar_thermal_irq_set(priv, 1);
+
 	return 0;
 
 error_unregister:
-- 
2.12.0

[PATCH v2 6/7] thermal: rcar_gen3_thermal: store device match data in private structure

[Niklas Söderlund]

The device match data needs to be accessible outside the probe function,
store it in the private data structure.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/thermal/rcar_gen3_thermal.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index dc019b7873273cf2..438fb9988eabb91b 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -85,6 +85,7 @@ struct rcar_gen3_thermal_tsc {
 };
 
 struct rcar_gen3_thermal_priv {
+	const struct rcar_gen3_thermal_data *data;
 	spinlock_t lock; /* Protect interrupts on and off */
 	unsigned int num_tscs;
 	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
@@ -353,8 +354,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	struct thermal_zone_device *zone;
 	int ret, irq, i;
 	char *irqname;
-	const struct rcar_gen3_thermal_data *match_data =
-		of_device_get_match_data(dev);
 
 	/* default values if FUSEs are missing */
 	/* TODO: Read values from hardware on supported platforms */
@@ -369,6 +368,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	priv->data = of_device_get_match_data(dev);
+
 	spin_lock_init(&priv->lock);
 
 	platform_set_drvdata(pdev, priv);
@@ -414,7 +415,7 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 
 		priv->tscs[i] = tsc;
 
-		match_data->thermal_init(tsc);
+		priv->data->thermal_init(tsc);
 		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);
 
 		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
-- 
2.12.0

[PATCH v2 7/7] thermal: rcar_gen3_thermal: add suspend and resume support

[Niklas Söderlund]

To restore operation it's easiest to reinitialise all TSCs. In order to
do this the current trip window needs to be stored in the TSC structure
so that it can be restored upon resume.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_gen3_thermal.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 438fb9988eabb91b..b5e3d7b41629803c 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -82,6 +82,8 @@ struct rcar_gen3_thermal_tsc {
 	void __iomem *base;
 	struct thermal_zone_device *zone;
 	struct equation_coefs coef;
+	int low;
+	int high;
 };
 
 struct rcar_gen3_thermal_priv {
@@ -217,6 +219,9 @@ static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2,
 				rcar_gen3_thermal_mcelsius_to_temp(tsc, high));
 
+	tsc->low = low;
+	tsc->high = high;
+
 	return 0;
 }
 
@@ -450,9 +455,39 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int __maybe_unused rcar_gen3_thermal_suspend(struct device *dev)
+{
+	struct rcar_gen3_thermal_priv *priv = dev_get_drvdata(dev);
+
+	rcar_thermal_irq_set(priv, 0);
+
+	return 0;
+}
+
+static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
+{
+	struct rcar_gen3_thermal_priv *priv = dev_get_drvdata(dev);
+	unsigned int i;
+
+	for (i = 0; i < priv->num_tscs; i++) {
+		struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
+
+		priv->data->thermal_init(tsc);
+		rcar_gen3_thermal_set_trips(tsc, tsc->low, tsc->high);
+	}
+
+	rcar_thermal_irq_set(priv, 1);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(rcar_gen3_thermal_pm_ops, rcar_gen3_thermal_suspend,
+			 rcar_gen3_thermal_resume);
+
 static struct platform_driver rcar_gen3_thermal_driver = {
 	.driver	= {
 		.name	= "rcar_gen3_thermal",
+		.pm = &rcar_gen3_thermal_pm_ops,
 		.of_match_table = rcar_gen3_thermal_dt_ids,
 	},
 	.probe		= rcar_gen3_thermal_probe,
-- 
2.12.0

Re: [PATCH v2 2/7] thermal: rcar_gen3_thermal: remove unneeded mutex

[Sergei Shtylyov]

On 3/17/2017 6:52 PM, Niklas Söderlund wrote:

> There is no point in protecting a register read with a lock. This is

    Register I/O is usually protected with spinlock...

> most likely a leftover from when the driver was reworked before submitted
> for upstream.

    Before being submitted?

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
[...]

MBR, Sergei

Re: [PATCH v2 3/7] thermal: rcar_gen3_thermal: check that TSC exists before memory allocation

[Geert Uytterhoeven]

On Fri, Mar 17, 2017 at 4:52 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Move the check for a TSC resource before allocating memory for a new
> TSC. If no TSC is found there is little point in allocating memory for
> it.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 v2 4/7] thermal: rcar_gen3_thermal: record and check number of TSCs found

[Geert Uytterhoeven]

On Fri, Mar 17, 2017 at 4:52 PM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Record how many TSCs are found in struct rcar_gen3_thermal_priv, this is
> needed to be able to add hardware interrupts for trip points later. Also
> add a check to make sure at least one TSC is found.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> ---
>  drivers/thermal/rcar_gen3_thermal.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 9b6bc03dd142a8dd..3b6e85abfadc61a3 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -74,6 +74,7 @@ struct rcar_gen3_thermal_tsc {
>  };
>
>  struct rcar_gen3_thermal_priv {
> +       unsigned int num_tscs;

I would insert this 32-bit number after the pointers below (which are 64-bit),
because of alignment rules and implicit gaps.

>         struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
>  };

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 v2 3/7] thermal: rcar_gen3_thermal: check that TSC exists before memory allocation

[Wolfram Sang]

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

On Fri, Mar 17, 2017 at 04:52:56PM +0100, Niklas Söderlund wrote:
> Move the check for a TSC resource before allocating memory for a new
> TSC. If no TSC is found there is little point in allocating memory for
> it.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

Re: [PATCH v2 4/7] thermal: rcar_gen3_thermal: record and check number of TSCs found

[Wolfram Sang]

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

On Fri, Mar 17, 2017 at 04:52:57PM +0100, Niklas Söderlund wrote:
> Record how many TSCs are found in struct rcar_gen3_thermal_priv, this is
> needed to be able to add hardware interrupts for trip points later. Also
> add a check to make sure at least one TSC is found.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/thermal/rcar_gen3_thermal.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 9b6bc03dd142a8dd..3b6e85abfadc61a3 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -74,6 +74,7 @@ struct rcar_gen3_thermal_tsc {
>  };
>  
>  struct rcar_gen3_thermal_priv {
> +	unsigned int num_tscs;
>  	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
>  };
>  
> @@ -305,6 +306,12 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  			goto error_unregister;
>  		}
>  		tsc->zone = zone;
> +		priv->num_tscs++;
> +	}
> +
> +	if (!priv->num_tscs) {
> +		ret = -ENODEV;
> +		goto error_unregister;

I would have probably done 'priv->num_tscs = i;' after the loop instead
of 'priv->num_tscs++;' in the loop, but that is super-minor.

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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

Re: [PATCH v2 5/7] thermal: rcar_gen3_thermal: enable hardware interrupts for trip points

[Wolfram Sang]

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

Hi Niklas,

On Fri, Mar 17, 2017 at 04:52:58PM +0100, Niklas Söderlund wrote:
> Enable hardware trip points by implementing the set_trips callback. The
> thermal core will take care of setting the initial trip point window and
> to update it once the driver reports a TSC has moved outside it.
> 
> The interrupt structure for this device is a bit odd. There is not a
> dedicated IRQ for each TSC, instead the interrupts are shared between
> all TSCs. IRQn is fired if the temp monitored in IRQTEMPn is reached in
> any of the TSCs, example IRQ3 is fired if temperature in IRQTEMP3 is
> reached in either TSC0, TSC1 or TSC2.
> 
> For this reason the usage of interrupts in this driver is an all-on or
> all-off design. When an interrupt happens all TSCs are checked and all
> thermal zones are updated. This could be refined to be more fine grained
> but the thermal core takes care of only updating the thermal zones that
> have left their trip point window.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Looks mostly good, thanks!

...

>  static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
>  	.get_temp	= rcar_gen3_thermal_get_temp,
> +	.set_trips	= rcar_gen3_thermal_set_trips,
>  };
>  
> +static void rcar_thermal_irq_set(struct rcar_gen3_thermal_priv *priv, int on)

'bool' instead of 'int'?

> +{
> +	unsigned int i;
> +	u32 val;
> +
> +	val = on ? IRQ_TEMPD1 | IRQ_TEMP2 : 0;

Very minor, merge the 2 lines?

	u32 val = on ? IRQ_TEMPD1 | IRQ_TEMP2 : 0;


> +
> +	for (i = 0; i < priv->num_tscs; i++)
> +		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQMSK, val);
> +}
> +
> +static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data)
> +{
> +	struct rcar_gen3_thermal_priv *priv = data;
> +	unsigned long flags;
> +	u32 status;
> +	int i, ret = IRQ_HANDLED;
> +
> +	spin_lock_irqsave(&priv->lock, flags);

Why _irqsave? You are in interrupt context already.

> +	for (i = 0; i < priv->num_tscs; i++) {
> +		status = rcar_gen3_thermal_read(priv->tscs[i], REG_GEN3_IRQSTR);
> +		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQSTR, 0);
> +		if (status)
> +			ret = IRQ_WAKE_THREAD;
> +	}
> +
> +	if (ret == IRQ_WAKE_THREAD)
> +		rcar_thermal_irq_set(priv, 0);

'false' instead of '0'? I think this is more readable.

> +	spin_unlock_irqrestore(&priv->lock, flags);

Add a blank line before unlock?

> +
> +	return ret;
> +}
> +
> +static irqreturn_t rcar_gen3_thermal_irq_thread(int irq, void *data)
> +{
> +	struct rcar_gen3_thermal_priv *priv = data;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < priv->num_tscs; i++)
> +		thermal_zone_device_update(priv->tscs[i]->zone,
> +					   THERMAL_EVENT_UNSPECIFIED);
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	rcar_thermal_irq_set(priv, 1);

'true' instead of '1'?

> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return IRQ_HANDLED;
> +}
> +

...

> @@ -252,7 +351,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct resource *res;
>  	struct thermal_zone_device *zone;
> -	int ret, i;
> +	int ret, irq, i;
> +	char *irqname;
>  	const struct rcar_gen3_thermal_data *match_data =
>  		of_device_get_match_data(dev);
>  
> @@ -269,8 +369,27 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	spin_lock_init(&priv->lock);
> +
>  	platform_set_drvdata(pdev, priv);
>  
> +	for (i = 0; i < 2; i++) {

Maybe a comment saying that the driver works with two interrupts
currently, so the '2' gets explained?

> +		irq = platform_get_irq(pdev, i);
> +		if (irq < 0)
> +			return irq;
> +
> +		irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
> +					 dev_name(dev), i);
> +		if (!irqname)
> +			return -ENOMEM;
> +
> +		ret = devm_request_threaded_irq(dev, irq, rcar_gen3_thermal_irq,
> +						rcar_gen3_thermal_irq_thread,
> +						IRQF_SHARED, irqname, priv);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	pm_runtime_enable(dev);
>  	pm_runtime_get_sync(dev);
>  
> @@ -307,6 +426,12 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  		}
>  		tsc->zone = zone;
>  		priv->num_tscs++;
> +
> +		ret = of_thermal_get_ntrips(tsc->zone);
> +		if (ret < 0)
> +			goto error_unregister;
> +
> +		dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret);
>  	}
>  
>  	if (!priv->num_tscs) {
> @@ -314,6 +439,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  		goto error_unregister;
>  	}
>  
> +	rcar_thermal_irq_set(priv, 1)

'true' instead of '1' again?

> +

Kind regards,

   Wolfram


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

Re: [PATCH v2 7/7] thermal: rcar_gen3_thermal: add suspend and resume support

[Wolfram Sang]

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

On Fri, Mar 17, 2017 at 04:53:00PM +0100, Niklas Söderlund wrote:
> To restore operation it's easiest to reinitialise all TSCs. In order to
> do this the current trip window needs to be stored in the TSC structure
> so that it can be restored upon resume.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

If you refactor rcar_thermal_irq_set(), then you'd need adaptions here
as well, but besides this minor thing:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


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