[PATCH v4 0/8] Improve Exynos thermal driver

[PATCH v4 0/8] Improve Exynos thermal driver

[Mateusz Majewski]

This work improves Exynos thermal driver in various ways. This is
related to the discussion in
https://lore.kernel.org/all/97201878-3bb8-eac5-7fac-a690322ac43a@linaro.org/

The primary issue being fixed is a lockdep warning, which is fixed by
the thermal: exynos: use set_trips patch. We also simplify the code in
general.

Changelog:
 v4:
   - Resolved merge conflict when applying thermal: exynos: split
     initialization of TMU and the thermal zone
   - Reordered calls done when leaving exynos_tmu_initialize for
     symmetry
 v3:
   - Fixed regulator initialization
   - Fixed formatting of some comments
 v2:
   - Added missing field descriptions
   - Removed an unnecessary field description
   - Removed the commits that made clock management more fine-grained
     (need more discussion), and adapted the new code to manage clocks
   - Removed the devicetree changes (will be uploaded separately),
     changing the recipient list accordingly
   - Improved formatting of the devm_request_threaded_irq call

Mateusz Majewski (8):
  thermal: exynos: remove an unnecessary field description
  thermal: exynos: drop id field
  thermal: exynos: switch from workqueue-driven interrupt handling to
    threaded interrupts
  thermal: exynos: handle devm_regulator_get_optional return value
    correctly
  thermal: exynos: simplify regulator (de)initialization
  thermal: exynos: stop using the threshold mechanism on Exynos 4210
  thermal: exynos: split initialization of TMU and the thermal zone
  thermal: exynos: use set_trips

 drivers/thermal/samsung/exynos_tmu.c | 538 ++++++++++++++-------------
 1 file changed, 283 insertions(+), 255 deletions(-)

-- 
2.42.0

[PATCH v4 1/8] thermal: exynos: remove an unnecessary field description

[Mateusz Majewski]

It seems that the field has been removed in one of the previous commits,
but the description has been forgotten.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 123ec81e1943..187086658e8f 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -160,7 +160,6 @@ enum soc_type {
  *	in the positive-TC generator block
  *	0 < reference_voltage <= 31
  * @regulator: pointer to the TMU regulator structure.
- * @reg_conf: pointer to structure to register with core thermal.
  * @tzd: pointer to thermal_zone_device structure
  * @ntrip: number of supported trip points.
  * @enabled: current status of TMU device
-- 
2.42.0

[PATCH v4 2/8] thermal: exynos: drop id field

[Mateusz Majewski]

We do not use the value, and only Exynos 7 defines this alias anyway.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
v1 -> v2: minor commit message rewording.

 drivers/thermal/samsung/exynos_tmu.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 187086658e8f..4ff32245d2a9 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -138,7 +138,6 @@ enum soc_type {
 /**
  * struct exynos_tmu_data : A structure to hold the private data of the TMU
  *			    driver
- * @id: identifier of the one instance of the TMU controller.
  * @base: base address of the single instance of the TMU controller.
  * @base_second: base address of the common registers of the TMU controller.
  * @irq: irq number of the TMU controller.
@@ -172,7 +171,6 @@ enum soc_type {
  * @tmu_clear_irqs: SoC specific TMU interrupts clearing method
  */
 struct exynos_tmu_data {
-	int id;
 	void __iomem *base;
 	void __iomem *base_second;
 	int irq;
@@ -865,10 +863,6 @@ static int exynos_map_dt_data(struct platform_device *pdev)
 	if (!data || !pdev->dev.of_node)
 		return -ENODEV;
 
-	data->id = of_alias_get_id(pdev->dev.of_node, "tmuctrl");
-	if (data->id < 0)
-		data->id = 0;
-
 	data->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
 	if (data->irq <= 0) {
 		dev_err(&pdev->dev, "failed to get IRQ\n");
-- 
2.42.0

[PATCH v4 3/8] thermal: exynos: switch from workqueue-driven interrupt handling to threaded interrupts

[Mateusz Majewski]

The workqueue boilerplate is mostly one-to-one what the threaded
interrupts do.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
v1 -> v2: devm_request_threaded_irq call formatting change.

 drivers/thermal/samsung/exynos_tmu.c | 29 +++++++++-------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 4ff32245d2a9..c144592d4584 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -142,7 +142,6 @@ enum soc_type {
  * @base_second: base address of the common registers of the TMU controller.
  * @irq: irq number of the TMU controller.
  * @soc: id of the SOC type.
- * @irq_work: pointer to the irq work structure.
  * @lock: lock to implement synchronization.
  * @clk: pointer to the clock structure.
  * @clk_sec: pointer to the clock structure for accessing the base_second.
@@ -175,7 +174,6 @@ struct exynos_tmu_data {
 	void __iomem *base_second;
 	int irq;
 	enum soc_type soc;
-	struct work_struct irq_work;
 	struct mutex lock;
 	struct clk *clk, *clk_sec, *sclk;
 	u32 cal_type;
@@ -763,10 +761,9 @@ static int exynos7_tmu_read(struct exynos_tmu_data *data)
 		EXYNOS7_TMU_TEMP_MASK;
 }
 
-static void exynos_tmu_work(struct work_struct *work)
+static irqreturn_t exynos_tmu_threaded_irq(int irq, void *id)
 {
-	struct exynos_tmu_data *data = container_of(work,
-			struct exynos_tmu_data, irq_work);
+	struct exynos_tmu_data *data = id;
 
 	thermal_zone_device_update(data->tzd, THERMAL_EVENT_UNSPECIFIED);
 
@@ -778,7 +775,8 @@ static void exynos_tmu_work(struct work_struct *work)
 
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
-	enable_irq(data->irq);
+
+	return IRQ_HANDLED;
 }
 
 static void exynos4210_tmu_clear_irqs(struct exynos_tmu_data *data)
@@ -812,16 +810,6 @@ static void exynos4210_tmu_clear_irqs(struct exynos_tmu_data *data)
 	writel(val_irq, data->base + tmu_intclear);
 }
 
-static irqreturn_t exynos_tmu_irq(int irq, void *id)
-{
-	struct exynos_tmu_data *data = id;
-
-	disable_irq_nosync(irq);
-	schedule_work(&data->irq_work);
-
-	return IRQ_HANDLED;
-}
-
 static const struct of_device_id exynos_tmu_match[] = {
 	{
 		.compatible = "samsung,exynos3250-tmu",
@@ -1023,8 +1011,6 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_sensor;
 
-	INIT_WORK(&data->irq_work, exynos_tmu_work);
-
 	data->clk = devm_clk_get(&pdev->dev, "tmu_apbif");
 	if (IS_ERR(data->clk)) {
 		dev_err(&pdev->dev, "Failed to get clock\n");
@@ -1093,8 +1079,11 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 		goto err_sclk;
 	}
 
-	ret = devm_request_irq(&pdev->dev, data->irq, exynos_tmu_irq,
-		IRQF_TRIGGER_RISING | IRQF_SHARED, dev_name(&pdev->dev), data);
+	ret = devm_request_threaded_irq(&pdev->dev, data->irq, NULL,
+					exynos_tmu_threaded_irq,
+					IRQF_TRIGGER_RISING
+						| IRQF_SHARED | IRQF_ONESHOT,
+					dev_name(&pdev->dev), data);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to request irq: %d\n", data->irq);
 		goto err_sclk;
-- 
2.42.0

[PATCH v4 4/8] thermal: exynos: handle devm_regulator_get_optional return value correctly

[Mateusz Majewski]

Currently, if regulator is required in the SoC, but
devm_regulator_get_optional fails for whatever reason, the execution
will proceed without propagating the error. Meanwhile there is no reason
to output the error in case of -ENODEV.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
v3 -> v4: Moved info about not outputting error to the correct commit
  message.

 drivers/thermal/samsung/exynos_tmu.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index c144592d4584..8bcad8a70dc5 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -1002,9 +1002,17 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 			return ret;
 		}
 	} else {
-		if (PTR_ERR(data->regulator) == -EPROBE_DEFER)
+		ret = PTR_ERR(data->regulator);
+		switch (ret) {
+		case -ENODEV:
+			break;
+		case -EPROBE_DEFER:
 			return -EPROBE_DEFER;
-		dev_info(&pdev->dev, "Regulator node (vtmu) not found\n");
+		default:
+			dev_err(&pdev->dev, "Failed to get regulator: %d\n",
+				ret);
+			return ret;
+		}
 	}
 
 	ret = exynos_map_dt_data(pdev);
-- 
2.42.0

[PATCH v4 5/8] thermal: exynos: simplify regulator (de)initialization

[Mateusz Majewski]

We rewrite the initialization to enable the regulator as part of devm,
which allows us to not handle the struct instance manually.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
v3 -> v4: Resolved merge conflict and moved info about not outputting
  error to the correct commit message.
v2 -> v3: Fixed error handling of devm_regulator_get_optional to handle
  the case in which the regulator is available, but enabling it fails.
  Also removed the error message, split into two commits and reworded
  the commit message.

 drivers/thermal/samsung/exynos_tmu.c | 49 +++++++++-------------------
 1 file changed, 15 insertions(+), 34 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 8bcad8a70dc5..3bdcbab7466f 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -157,7 +157,6 @@ enum soc_type {
  * @reference_voltage: reference voltage of amplifier
  *	in the positive-TC generator block
  *	0 < reference_voltage <= 31
- * @regulator: pointer to the TMU regulator structure.
  * @tzd: pointer to thermal_zone_device structure
  * @ntrip: number of supported trip points.
  * @enabled: current status of TMU device
@@ -183,7 +182,6 @@ struct exynos_tmu_data {
 	u16 temp_error1, temp_error2;
 	u8 gain;
 	u8 reference_voltage;
-	struct regulator *regulator;
 	struct thermal_zone_device *tzd;
 	unsigned int ntrip;
 	bool enabled;
@@ -994,50 +992,40 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 	 * TODO: Add regulator as an SOC feature, so that regulator enable
 	 * is a compulsory call.
 	 */
-	data->regulator = devm_regulator_get_optional(&pdev->dev, "vtmu");
-	if (!IS_ERR(data->regulator)) {
-		ret = regulator_enable(data->regulator);
-		if (ret) {
-			dev_err(&pdev->dev, "failed to enable vtmu\n");
-			return ret;
-		}
-	} else {
-		ret = PTR_ERR(data->regulator);
-		switch (ret) {
-		case -ENODEV:
-			break;
-		case -EPROBE_DEFER:
-			return -EPROBE_DEFER;
-		default:
-			dev_err(&pdev->dev, "Failed to get regulator: %d\n",
-				ret);
-			return ret;
-		}
+	ret = devm_regulator_get_enable_optional(&pdev->dev, "vtmu");
+	switch (ret) {
+	case 0:
+	case -ENODEV:
+		break;
+	case -EPROBE_DEFER:
+		return -EPROBE_DEFER;
+	default:
+		dev_err(&pdev->dev, "Failed to get enabled regulator: %d\n",
+			ret);
+		return ret;
 	}
 
 	ret = exynos_map_dt_data(pdev);
 	if (ret)
-		goto err_sensor;
+		return ret;
 
 	data->clk = devm_clk_get(&pdev->dev, "tmu_apbif");
 	if (IS_ERR(data->clk)) {
 		dev_err(&pdev->dev, "Failed to get clock\n");
-		ret = PTR_ERR(data->clk);
-		goto err_sensor;
+		return PTR_ERR(data->clk);
 	}
 
 	data->clk_sec = devm_clk_get(&pdev->dev, "tmu_triminfo_apbif");
 	if (IS_ERR(data->clk_sec)) {
 		if (data->soc == SOC_ARCH_EXYNOS5420_TRIMINFO) {
 			dev_err(&pdev->dev, "Failed to get triminfo clock\n");
-			ret = PTR_ERR(data->clk_sec);
-			goto err_sensor;
+			return PTR_ERR(data->clk_sec);
 		}
 	} else {
 		ret = clk_prepare(data->clk_sec);
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to get clock\n");
-			goto err_sensor;
+			return ret;
 		}
 	}
 
@@ -1107,10 +1095,6 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 err_clk_sec:
 	if (!IS_ERR(data->clk_sec))
 		clk_unprepare(data->clk_sec);
-err_sensor:
-	if (!IS_ERR(data->regulator))
-		regulator_disable(data->regulator);
-
 	return ret;
 }
 
@@ -1124,9 +1108,6 @@ static void exynos_tmu_remove(struct platform_device *pdev)
 	clk_unprepare(data->clk);
 	if (!IS_ERR(data->clk_sec))
 		clk_unprepare(data->clk_sec);
-
-	if (!IS_ERR(data->regulator))
-		regulator_disable(data->regulator);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.42.0

[PATCH v4 6/8] thermal: exynos: stop using the threshold mechanism on Exynos 4210

[Mateusz Majewski]

Exynos 4210 supports setting a base threshold value, which is added to
all trip points. This might be useful, but is not really necessary in
our usecase, so we always set it to 0 to simplify the code a bit.

Additionally, this change makes it so that we convert the value to the
calibrated one in a slightly different place. This is more correct
morally, though it does not make any change when single-point
calibration is being used (which is the case currently).

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 3bdcbab7466f..d918bf6d5359 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -343,20 +343,7 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
 static void exynos4210_tmu_set_trip_temp(struct exynos_tmu_data *data,
 					 int trip_id, u8 temp)
 {
-	struct thermal_trip trip;
-	u8 ref, th_code;
-
-	if (thermal_zone_get_trip(data->tzd, 0, &trip))
-		return;
-
-	ref = trip.temperature / MCELSIUS;
-
-	if (trip_id == 0) {
-		th_code = temp_to_code(data, ref);
-		writeb(th_code, data->base + EXYNOS4210_TMU_REG_THRESHOLD_TEMP);
-	}
-
-	temp -= ref;
+	temp = temp_to_code(data, temp);
 	writeb(temp, data->base + EXYNOS4210_TMU_REG_TRIG_LEVEL0 + trip_id * 4);
 }
 
@@ -371,6 +358,8 @@ static void exynos4210_tmu_initialize(struct platform_device *pdev)
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
 
 	sanitize_temp_error(data, readl(data->base + EXYNOS_TMU_REG_TRIMINFO));
+
+	writeb(0, data->base + EXYNOS4210_TMU_REG_THRESHOLD_TEMP);
 }
 
 static void exynos4412_tmu_set_trip_temp(struct exynos_tmu_data *data,
-- 
2.42.0

[PATCH v4 7/8] thermal: exynos: split initialization of TMU and the thermal zone

[Mateusz Majewski]

This will be needed in the future, as the thermal zone subsystem might
call our callbacks right after devm_thermal_of_zone_register. Currently
we just make get_temp return EAGAIN in such case, but this will not be
possible with state-modifying callbacks, for instance set_trips.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
v3 -> v4: Reordered calls done while leaving exynos_tmu_initialize so
  that they mirror the calls done while entering this function.
v1 -> v2: We take clocks into account; exynos_tmu_initialize needs both
  clocks, as tmu_initialize might use the base_second registers. However,
  exynos_thermal_zone_configure only needs clk.

 drivers/thermal/samsung/exynos_tmu.c | 104 +++++++++++++++------------
 1 file changed, 60 insertions(+), 44 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index d918bf6d5359..6b3a7dd05c68 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -251,25 +251,8 @@ static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
 static int exynos_tmu_initialize(struct platform_device *pdev)
 {
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
-	struct thermal_zone_device *tzd = data->tzd;
-	int num_trips = thermal_zone_get_num_trips(tzd);
 	unsigned int status;
-	int ret = 0, temp;
-
-	ret = thermal_zone_get_crit_temp(tzd, &temp);
-	if (ret && data->soc != SOC_ARCH_EXYNOS5433) { /* FIXME */
-		dev_err(&pdev->dev,
-			"No CRITICAL trip point defined in device tree!\n");
-		goto out;
-	}
-
-	if (num_trips > data->ntrip) {
-		dev_info(&pdev->dev,
-			 "More trip points than supported by this TMU.\n");
-		dev_info(&pdev->dev,
-			 "%d trip points should be configured in polling mode.\n",
-			 num_trips - data->ntrip);
-	}
+	int ret = 0;
 
 	mutex_lock(&data->lock);
 	clk_enable(data->clk);
@@ -280,32 +263,63 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 	if (!status) {
 		ret = -EBUSY;
 	} else {
-		int i, ntrips =
-			min_t(int, num_trips, data->ntrip);
-
 		data->tmu_initialize(pdev);
-
-		/* Write temperature code for rising and falling threshold */
-		for (i = 0; i < ntrips; i++) {
-
-			struct thermal_trip trip;
-
-			ret = thermal_zone_get_trip(tzd, i, &trip);
-			if (ret)
-				goto err;
-
-			data->tmu_set_trip_temp(data, i, trip.temperature / MCELSIUS);
-			data->tmu_set_trip_hyst(data, i, trip.temperature / MCELSIUS,
-						trip.hysteresis / MCELSIUS);
-		}
-
 		data->tmu_clear_irqs(data);
 	}
+
+	if (!IS_ERR(data->clk_sec))
+		clk_disable(data->clk_sec);
+	clk_disable(data->clk);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int exynos_thermal_zone_configure(struct platform_device *pdev)
+{
+	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
+	struct thermal_zone_device *tzd = data->tzd;
+	int i, num_trips = thermal_zone_get_num_trips(tzd);
+	int ret = 0, temp;
+
+	ret = thermal_zone_get_crit_temp(tzd, &temp);
+
+	if (ret && data->soc != SOC_ARCH_EXYNOS5433) { /* FIXME */
+		dev_err(&pdev->dev,
+			"No CRITICAL trip point defined in device tree!\n");
+		goto out;
+	}
+
+	mutex_lock(&data->lock);
+
+	if (num_trips > data->ntrip) {
+		dev_info(&pdev->dev,
+			 "More trip points than supported by this TMU.\n");
+		dev_info(&pdev->dev,
+			 "%d trip points should be configured in polling mode.\n",
+			 num_trips - data->ntrip);
+	}
+
+	clk_enable(data->clk);
+
+	num_trips = min_t(int, num_trips, data->ntrip);
+
+	/* Write temperature code for rising and falling threshold */
+	for (i = 0; i < num_trips; i++) {
+		struct thermal_trip trip;
+
+		ret = thermal_zone_get_trip(tzd, i, &trip);
+		if (ret)
+			goto err;
+
+		data->tmu_set_trip_temp(data, i, trip.temperature / MCELSIUS);
+		data->tmu_set_trip_hyst(data, i, trip.temperature / MCELSIUS,
+					trip.hysteresis / MCELSIUS);
+	}
+
 err:
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
-	if (!IS_ERR(data->clk_sec))
-		clk_disable(data->clk_sec);
 out:
 	return ret;
 }
@@ -1044,10 +1058,12 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 		break;
 	}
 
-	/*
-	 * data->tzd must be registered before calling exynos_tmu_initialize(),
-	 * requesting irq and calling exynos_tmu_control().
-	 */
+	ret = exynos_tmu_initialize(pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to initialize TMU\n");
+		return ret;
+	}
+
 	data->tzd = devm_thermal_of_zone_register(&pdev->dev, 0, data,
 						  &exynos_sensor_ops);
 	if (IS_ERR(data->tzd)) {
@@ -1058,9 +1074,9 @@ static int exynos_tmu_probe(struct platform_device *pdev)
 		goto err_sclk;
 	}
 
-	ret = exynos_tmu_initialize(pdev);
+	ret = exynos_thermal_zone_configure(pdev);
 	if (ret) {
-		dev_err(&pdev->dev, "Failed to initialize TMU\n");
+		dev_err(&pdev->dev, "Failed to configure the thermal zone\n");
 		goto err_sclk;
 	}
 
-- 
2.42.0

[PATCH v4 8/8] thermal: exynos: use set_trips

[Mateusz Majewski]

Currently, each trip point defined in the device tree corresponds to a
single hardware interrupt. This commit instead switches to using two
hardware interrupts, whose values are set dynamically using the
set_trips callback. Additionally, the critical temperature threshold is
handled specifically.

Setting interrupts in this way also fixes a long-standing lockdep
warning, which was caused by calling thermal_zone_get_trips with our
lock being held. Do note that this requires TMU initialization to be
split into two parts, as done by the parent commit: parts of the
initialization call into the thermal_zone_device structure and so must
be done after its registration, but the initialization is also
responsible for setting up calibration, which must be done before
thermal_zone_device registration, which will call set_trips for the
first time; if the calibration is not done in time, the interrupt values
will be silently wrong!

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
v2 -> v3: Fixed formatting of some comments.
v1 -> v2: We take clocks into account; anything that sets temperature
  thresholds needs clk.

 drivers/thermal/samsung/exynos_tmu.c | 400 +++++++++++++++------------
 1 file changed, 226 insertions(+), 174 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 6b3a7dd05c68..6d19e72a490d 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -158,10 +158,12 @@ enum soc_type {
  *	in the positive-TC generator block
  *	0 < reference_voltage <= 31
  * @tzd: pointer to thermal_zone_device structure
- * @ntrip: number of supported trip points.
  * @enabled: current status of TMU device
- * @tmu_set_trip_temp: SoC specific method to set trip (rising threshold)
- * @tmu_set_trip_hyst: SoC specific to set hysteresis (falling threshold)
+ * @tmu_set_low_temp: SoC specific method to set trip (falling threshold)
+ * @tmu_set_high_temp: SoC specific method to set trip (rising threshold)
+ * @tmu_set_crit_temp: SoC specific method to set critical temperature
+ * @tmu_disable_low: SoC specific method to disable an interrupt (falling threshold)
+ * @tmu_disable_high: SoC specific method to disable an interrupt (rising threshold)
  * @tmu_initialize: SoC specific TMU initialization method
  * @tmu_control: SoC specific TMU control method
  * @tmu_read: SoC specific TMU temperature read method
@@ -183,13 +185,13 @@ struct exynos_tmu_data {
 	u8 gain;
 	u8 reference_voltage;
 	struct thermal_zone_device *tzd;
-	unsigned int ntrip;
 	bool enabled;
 
-	void (*tmu_set_trip_temp)(struct exynos_tmu_data *data, int trip,
-				 u8 temp);
-	void (*tmu_set_trip_hyst)(struct exynos_tmu_data *data, int trip,
-				 u8 temp, u8 hyst);
+	void (*tmu_set_low_temp)(struct exynos_tmu_data *data, u8 temp);
+	void (*tmu_set_high_temp)(struct exynos_tmu_data *data, u8 temp);
+	void (*tmu_set_crit_temp)(struct exynos_tmu_data *data, u8 temp);
+	void (*tmu_disable_low)(struct exynos_tmu_data *data);
+	void (*tmu_disable_high)(struct exynos_tmu_data *data);
 	void (*tmu_initialize)(struct platform_device *pdev);
 	void (*tmu_control)(struct platform_device *pdev, bool on);
 	int (*tmu_read)(struct exynos_tmu_data *data);
@@ -279,49 +281,28 @@ static int exynos_thermal_zone_configure(struct platform_device *pdev)
 {
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
 	struct thermal_zone_device *tzd = data->tzd;
-	int i, num_trips = thermal_zone_get_num_trips(tzd);
-	int ret = 0, temp;
+	int ret, temp;
 
 	ret = thermal_zone_get_crit_temp(tzd, &temp);
+	if (ret) {
+		/* FIXME: Remove this special case */
+		if (data->soc == SOC_ARCH_EXYNOS5433)
+			return 0;
 
-	if (ret && data->soc != SOC_ARCH_EXYNOS5433) { /* FIXME */
 		dev_err(&pdev->dev,
 			"No CRITICAL trip point defined in device tree!\n");
-		goto out;
+		return ret;
 	}
 
 	mutex_lock(&data->lock);
-
-	if (num_trips > data->ntrip) {
-		dev_info(&pdev->dev,
-			 "More trip points than supported by this TMU.\n");
-		dev_info(&pdev->dev,
-			 "%d trip points should be configured in polling mode.\n",
-			 num_trips - data->ntrip);
-	}
-
 	clk_enable(data->clk);
 
-	num_trips = min_t(int, num_trips, data->ntrip);
+	data->tmu_set_crit_temp(data, temp / MCELSIUS);
 
-	/* Write temperature code for rising and falling threshold */
-	for (i = 0; i < num_trips; i++) {
-		struct thermal_trip trip;
-
-		ret = thermal_zone_get_trip(tzd, i, &trip);
-		if (ret)
-			goto err;
-
-		data->tmu_set_trip_temp(data, i, trip.temperature / MCELSIUS);
-		data->tmu_set_trip_hyst(data, i, trip.temperature / MCELSIUS,
-					trip.hysteresis / MCELSIUS);
-	}
-
-err:
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
-out:
-	return ret;
+
+	return 0;
 }
 
 static u32 get_con_reg(struct exynos_tmu_data *data, u32 con)
@@ -354,17 +335,58 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
 	mutex_unlock(&data->lock);
 }
 
-static void exynos4210_tmu_set_trip_temp(struct exynos_tmu_data *data,
-					 int trip_id, u8 temp)
+static void exynos_tmu_update_bit(struct exynos_tmu_data *data, int reg_off,
+				  int bit_off, bool enable)
 {
-	temp = temp_to_code(data, temp);
-	writeb(temp, data->base + EXYNOS4210_TMU_REG_TRIG_LEVEL0 + trip_id * 4);
+	u32 interrupt_en;
+
+	interrupt_en = readl(data->base + reg_off);
+	if (enable)
+		interrupt_en |= 1 << bit_off;
+	else
+		interrupt_en &= ~(1 << bit_off);
+	writel(interrupt_en, data->base + reg_off);
 }
 
-/* failing thresholds are not supported on Exynos4210 */
-static void exynos4210_tmu_set_trip_hyst(struct exynos_tmu_data *data,
-					 int trip, u8 temp, u8 hyst)
+static void exynos4210_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
 {
+	/*
+	 * Failing thresholds are not supported on Exynos 4210.
+	 * We use polling instead.
+	 */
+}
+
+static void exynos4210_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
+{
+	temp = temp_to_code(data, temp);
+	writeb(temp, data->base + EXYNOS4210_TMU_REG_TRIG_LEVEL0 + 4);
+	exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
+			      EXYNOS_TMU_INTEN_RISE0_SHIFT + 4, true);
+}
+
+static void exynos4210_tmu_disable_low(struct exynos_tmu_data *data)
+{
+	/* Again, this is handled by polling. */
+}
+
+static void exynos4210_tmu_disable_high(struct exynos_tmu_data *data)
+{
+	exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
+			      EXYNOS_TMU_INTEN_RISE0_SHIFT + 4, false);
+}
+
+static void exynos4210_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
+{
+	/*
+	 * Hardware critical temperature handling is not supported on Exynos 4210.
+	 * We still set the critical temperature threshold, but this is only to
+	 * make sure it is handled as soon as possible. It is just a normal interrupt.
+	 */
+
+	temp = temp_to_code(data, temp);
+	writeb(temp, data->base + EXYNOS4210_TMU_REG_TRIG_LEVEL0 + 12);
+	exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
+			      EXYNOS_TMU_INTEN_RISE0_SHIFT + 12, true);
 }
 
 static void exynos4210_tmu_initialize(struct platform_device *pdev)
@@ -376,33 +398,49 @@ static void exynos4210_tmu_initialize(struct platform_device *pdev)
 	writeb(0, data->base + EXYNOS4210_TMU_REG_THRESHOLD_TEMP);
 }
 
-static void exynos4412_tmu_set_trip_temp(struct exynos_tmu_data *data,
-					 int trip, u8 temp)
-{
-	u32 th, con;
-
-	th = readl(data->base + EXYNOS_THD_TEMP_RISE);
-	th &= ~(0xff << 8 * trip);
-	th |= temp_to_code(data, temp) << 8 * trip;
-	writel(th, data->base + EXYNOS_THD_TEMP_RISE);
-
-	if (trip == 3) {
-		con = readl(data->base + EXYNOS_TMU_REG_CONTROL);
-		con |= (1 << EXYNOS_TMU_THERM_TRIP_EN_SHIFT);
-		writel(con, data->base + EXYNOS_TMU_REG_CONTROL);
-	}
-}
-
-static void exynos4412_tmu_set_trip_hyst(struct exynos_tmu_data *data,
-					 int trip, u8 temp, u8 hyst)
+static void exynos4412_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
 {
 	u32 th;
 
 	th = readl(data->base + EXYNOS_THD_TEMP_FALL);
-	th &= ~(0xff << 8 * trip);
-	if (hyst)
-		th |= temp_to_code(data, temp - hyst) << 8 * trip;
+	th &= ~(0xff << 0);
+	th |= temp_to_code(data, temp) << 0;
 	writel(th, data->base + EXYNOS_THD_TEMP_FALL);
+
+	exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
+			      EXYNOS_TMU_INTEN_FALL0_SHIFT, true);
+}
+
+static void exynos4412_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
+{
+	u32 th;
+
+	th = readl(data->base + EXYNOS_THD_TEMP_RISE);
+	th &= ~(0xff << 8);
+	th |= temp_to_code(data, temp) << 8;
+	writel(th, data->base + EXYNOS_THD_TEMP_RISE);
+
+	exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
+			      EXYNOS_TMU_INTEN_RISE0_SHIFT + 4, true);
+}
+
+static void exynos4412_tmu_disable_low(struct exynos_tmu_data *data)
+{
+	exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
+			      EXYNOS_TMU_INTEN_FALL0_SHIFT, false);
+}
+
+static void exynos4412_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
+{
+	u32 th;
+
+	th = readl(data->base + EXYNOS_THD_TEMP_RISE);
+	th &= ~(0xff << 24);
+	th |= temp_to_code(data, temp) << 24;
+	writel(th, data->base + EXYNOS_THD_TEMP_RISE);
+
+	exynos_tmu_update_bit(data, EXYNOS_TMU_REG_CONTROL,
+			      EXYNOS_TMU_THERM_TRIP_EN_SHIFT, true);
 }
 
 static void exynos4412_tmu_initialize(struct platform_device *pdev)
@@ -432,44 +470,57 @@ static void exynos4412_tmu_initialize(struct platform_device *pdev)
 	sanitize_temp_error(data, trim_info);
 }
 
-static void exynos5433_tmu_set_trip_temp(struct exynos_tmu_data *data,
-					 int trip, u8 temp)
+static void exynos5433_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
 {
-	unsigned int reg_off, j;
 	u32 th;
 
-	if (trip > 3) {
-		reg_off = EXYNOS5433_THD_TEMP_RISE7_4;
-		j = trip - 4;
-	} else {
-		reg_off = EXYNOS5433_THD_TEMP_RISE3_0;
-		j = trip;
-	}
+	th = readl(data->base + EXYNOS5433_THD_TEMP_FALL3_0);
+	th &= ~(0xff << 0);
+	th |= temp_to_code(data, temp) << 0;
+	writel(th, data->base + EXYNOS5433_THD_TEMP_FALL3_0);
 
-	th = readl(data->base + reg_off);
-	th &= ~(0xff << j * 8);
-	th |= (temp_to_code(data, temp) << j * 8);
-	writel(th, data->base + reg_off);
+	exynos_tmu_update_bit(data, EXYNOS5433_TMU_REG_INTEN,
+			      EXYNOS_TMU_INTEN_FALL0_SHIFT, true);
 }
 
-static void exynos5433_tmu_set_trip_hyst(struct exynos_tmu_data *data,
-					 int trip, u8 temp, u8 hyst)
+static void exynos5433_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
 {
-	unsigned int reg_off, j;
 	u32 th;
 
-	if (trip > 3) {
-		reg_off = EXYNOS5433_THD_TEMP_FALL7_4;
-		j = trip - 4;
-	} else {
-		reg_off = EXYNOS5433_THD_TEMP_FALL3_0;
-		j = trip;
-	}
+	th = readl(data->base + EXYNOS5433_THD_TEMP_RISE3_0);
+	th &= ~(0xff << 8);
+	th |= temp_to_code(data, temp) << 8;
+	writel(th, data->base + EXYNOS5433_THD_TEMP_RISE3_0);
 
-	th = readl(data->base + reg_off);
-	th &= ~(0xff << j * 8);
-	th |= (temp_to_code(data, temp - hyst) << j * 8);
-	writel(th, data->base + reg_off);
+	exynos_tmu_update_bit(data, EXYNOS5433_TMU_REG_INTEN,
+			      EXYNOS7_TMU_INTEN_RISE0_SHIFT + 1, true);
+}
+
+static void exynos5433_tmu_disable_low(struct exynos_tmu_data *data)
+{
+	exynos_tmu_update_bit(data, EXYNOS5433_TMU_REG_INTEN,
+			      EXYNOS_TMU_INTEN_FALL0_SHIFT, false);
+}
+
+static void exynos5433_tmu_disable_high(struct exynos_tmu_data *data)
+{
+	exynos_tmu_update_bit(data, EXYNOS5433_TMU_REG_INTEN,
+			      EXYNOS7_TMU_INTEN_RISE0_SHIFT + 1, false);
+}
+
+static void exynos5433_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
+{
+	u32 th;
+
+	th = readl(data->base + EXYNOS5433_THD_TEMP_RISE7_4);
+	th &= ~(0xff << 24);
+	th |= temp_to_code(data, temp) << 24;
+	writel(th, data->base + EXYNOS5433_THD_TEMP_RISE7_4);
+
+	exynos_tmu_update_bit(data, EXYNOS_TMU_REG_CONTROL,
+			      EXYNOS_TMU_THERM_TRIP_EN_SHIFT, true);
+	exynos_tmu_update_bit(data, EXYNOS5433_TMU_REG_INTEN,
+			      EXYNOS7_TMU_INTEN_RISE0_SHIFT + 7, true);
 }
 
 static void exynos5433_tmu_initialize(struct platform_device *pdev)
@@ -505,34 +556,48 @@ static void exynos5433_tmu_initialize(struct platform_device *pdev)
 			cal_type ?  2 : 1);
 }
 
-static void exynos7_tmu_set_trip_temp(struct exynos_tmu_data *data,
-				      int trip, u8 temp)
+static void exynos7_tmu_update_temp(struct exynos_tmu_data *data, u8 temp,
+				    int idx, bool rise)
 {
 	unsigned int reg_off, bit_off;
 	u32 th;
+	void __iomem *reg;
 
-	reg_off = ((7 - trip) / 2) * 4;
-	bit_off = ((8 - trip) % 2);
+	reg_off = ((7 - idx) / 2) * 4;
+	bit_off = ((8 - idx) % 2);
 
-	th = readl(data->base + EXYNOS7_THD_TEMP_RISE7_6 + reg_off);
+	reg = data->base +
+	      (rise ? EXYNOS7_THD_TEMP_RISE7_6 : EXYNOS7_THD_TEMP_FALL7_6) +
+	      reg_off;
+	th = readl(reg);
 	th &= ~(EXYNOS7_TMU_TEMP_MASK << (16 * bit_off));
 	th |= temp_to_code(data, temp) << (16 * bit_off);
-	writel(th, data->base + EXYNOS7_THD_TEMP_RISE7_6 + reg_off);
+	writel(th, reg);
+
+	exynos_tmu_update_bit(data, EXYNOS5433_TMU_REG_INTEN,
+			      (rise ? EXYNOS7_TMU_INTEN_RISE0_SHIFT :
+				      EXYNOS_TMU_INTEN_FALL0_SHIFT) +
+				      idx,
+			      true);
 }
 
-static void exynos7_tmu_set_trip_hyst(struct exynos_tmu_data *data,
-				      int trip, u8 temp, u8 hyst)
+static void exynos7_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
 {
-	unsigned int reg_off, bit_off;
-	u32 th;
+	exynos7_tmu_update_temp(data, temp, 0, false);
+}
 
-	reg_off = ((7 - trip) / 2) * 4;
-	bit_off = ((8 - trip) % 2);
+static void exynos7_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
+{
+	exynos7_tmu_update_temp(data, temp, 1, true);
+}
 
-	th = readl(data->base + EXYNOS7_THD_TEMP_FALL7_6 + reg_off);
-	th &= ~(EXYNOS7_TMU_TEMP_MASK << (16 * bit_off));
-	th |= temp_to_code(data, temp - hyst) << (16 * bit_off);
-	writel(th, data->base + EXYNOS7_THD_TEMP_FALL7_6 + reg_off);
+static void exynos7_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
+{
+	/*
+	 * Like Exynos 4210, Exynos 7 does not seem to support critical temperature
+	 * handling in hardware. Again, we still set a separate interrupt for it.
+	 */
+	exynos7_tmu_update_temp(data, temp, 7, true);
 }
 
 static void exynos7_tmu_initialize(struct platform_device *pdev)
@@ -547,87 +612,44 @@ static void exynos7_tmu_initialize(struct platform_device *pdev)
 static void exynos4210_tmu_control(struct platform_device *pdev, bool on)
 {
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
-	struct thermal_zone_device *tz = data->tzd;
-	struct thermal_trip trip;
-	unsigned int con, interrupt_en = 0, i;
+	unsigned int con;
 
 	con = get_con_reg(data, readl(data->base + EXYNOS_TMU_REG_CONTROL));
 
-	if (on) {
-		for (i = 0; i < data->ntrip; i++) {
-			if (thermal_zone_get_trip(tz, i, &trip))
-				continue;
-
-			interrupt_en |=
-				(1 << (EXYNOS_TMU_INTEN_RISE0_SHIFT + i * 4));
-		}
-
-		if (data->soc != SOC_ARCH_EXYNOS4210)
-			interrupt_en |=
-				interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT;
-
+	if (on)
 		con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
-	} else {
+	else
 		con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT);
-	}
 
-	writel(interrupt_en, data->base + EXYNOS_TMU_REG_INTEN);
 	writel(con, data->base + EXYNOS_TMU_REG_CONTROL);
 }
 
 static void exynos5433_tmu_control(struct platform_device *pdev, bool on)
 {
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
-	struct thermal_zone_device *tz = data->tzd;
-	struct thermal_trip trip;
-	unsigned int con, interrupt_en = 0, pd_det_en, i;
+	unsigned int con, pd_det_en;
 
 	con = get_con_reg(data, readl(data->base + EXYNOS_TMU_REG_CONTROL));
 
-	if (on) {
-		for (i = 0; i < data->ntrip; i++) {
-			if (thermal_zone_get_trip(tz, i, &trip))
-				continue;
-
-			interrupt_en |=
-				(1 << (EXYNOS7_TMU_INTEN_RISE0_SHIFT + i));
-		}
-
-		interrupt_en |=
-			interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT;
-
+	if (on)
 		con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
-	} else
+	else
 		con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT);
 
 	pd_det_en = on ? EXYNOS5433_PD_DET_EN : 0;
 
 	writel(pd_det_en, data->base + EXYNOS5433_TMU_PD_DET_EN);
-	writel(interrupt_en, data->base + EXYNOS5433_TMU_REG_INTEN);
 	writel(con, data->base + EXYNOS_TMU_REG_CONTROL);
 }
 
 static void exynos7_tmu_control(struct platform_device *pdev, bool on)
 {
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
-	struct thermal_zone_device *tz = data->tzd;
-	struct thermal_trip trip;
-	unsigned int con, interrupt_en = 0, i;
+	unsigned int con;
 
 	con = get_con_reg(data, readl(data->base + EXYNOS_TMU_REG_CONTROL));
 
 	if (on) {
-		for (i = 0; i < data->ntrip; i++) {
-			if (thermal_zone_get_trip(tz, i, &trip))
-				continue;
-
-			interrupt_en |=
-				(1 << (EXYNOS7_TMU_INTEN_RISE0_SHIFT + i));
-		}
-
-		interrupt_en |=
-			interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT;
-
 		con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
 		con |= (1 << EXYNOS7_PD_DET_EN_SHIFT);
 	} else {
@@ -635,7 +657,6 @@ static void exynos7_tmu_control(struct platform_device *pdev, bool on)
 		con &= ~(1 << EXYNOS7_PD_DET_EN_SHIFT);
 	}
 
-	writel(interrupt_en, data->base + EXYNOS7_TMU_REG_INTEN);
 	writel(con, data->base + EXYNOS_TMU_REG_CONTROL);
 }
 
@@ -873,13 +894,15 @@ static int exynos_map_dt_data(struct platform_device *pdev)
 
 	switch (data->soc) {
 	case SOC_ARCH_EXYNOS4210:
-		data->tmu_set_trip_temp = exynos4210_tmu_set_trip_temp;
-		data->tmu_set_trip_hyst = exynos4210_tmu_set_trip_hyst;
+		data->tmu_set_low_temp = exynos4210_tmu_set_low_temp;
+		data->tmu_set_high_temp = exynos4210_tmu_set_high_temp;
+		data->tmu_disable_low = exynos4210_tmu_disable_low;
+		data->tmu_disable_high = exynos4210_tmu_disable_high;
+		data->tmu_set_crit_temp = exynos4210_tmu_set_crit_temp;
 		data->tmu_initialize = exynos4210_tmu_initialize;
 		data->tmu_control = exynos4210_tmu_control;
 		data->tmu_read = exynos4210_tmu_read;
 		data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
-		data->ntrip = 4;
 		data->gain = 15;
 		data->reference_voltage = 7;
 		data->efuse_value = 55;
@@ -892,14 +915,16 @@ static int exynos_map_dt_data(struct platform_device *pdev)
 	case SOC_ARCH_EXYNOS5260:
 	case SOC_ARCH_EXYNOS5420:
 	case SOC_ARCH_EXYNOS5420_TRIMINFO:
-		data->tmu_set_trip_temp = exynos4412_tmu_set_trip_temp;
-		data->tmu_set_trip_hyst = exynos4412_tmu_set_trip_hyst;
+		data->tmu_set_low_temp = exynos4412_tmu_set_low_temp;
+		data->tmu_set_high_temp = exynos4412_tmu_set_high_temp;
+		data->tmu_disable_low = exynos4412_tmu_disable_low;
+		data->tmu_disable_high = exynos4210_tmu_disable_high;
+		data->tmu_set_crit_temp = exynos4412_tmu_set_crit_temp;
 		data->tmu_initialize = exynos4412_tmu_initialize;
 		data->tmu_control = exynos4210_tmu_control;
 		data->tmu_read = exynos4412_tmu_read;
 		data->tmu_set_emulation = exynos4412_tmu_set_emulation;
 		data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
-		data->ntrip = 4;
 		data->gain = 8;
 		data->reference_voltage = 16;
 		data->efuse_value = 55;
@@ -911,14 +936,16 @@ static int exynos_map_dt_data(struct platform_device *pdev)
 		data->max_efuse_value = 100;
 		break;
 	case SOC_ARCH_EXYNOS5433:
-		data->tmu_set_trip_temp = exynos5433_tmu_set_trip_temp;
-		data->tmu_set_trip_hyst = exynos5433_tmu_set_trip_hyst;
+		data->tmu_set_low_temp = exynos5433_tmu_set_low_temp;
+		data->tmu_set_high_temp = exynos5433_tmu_set_high_temp;
+		data->tmu_disable_low = exynos5433_tmu_disable_low;
+		data->tmu_disable_high = exynos5433_tmu_disable_high;
+		data->tmu_set_crit_temp = exynos5433_tmu_set_crit_temp;
 		data->tmu_initialize = exynos5433_tmu_initialize;
 		data->tmu_control = exynos5433_tmu_control;
 		data->tmu_read = exynos4412_tmu_read;
 		data->tmu_set_emulation = exynos4412_tmu_set_emulation;
 		data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
-		data->ntrip = 8;
 		data->gain = 8;
 		if (res.start == EXYNOS5433_G3D_BASE)
 			data->reference_voltage = 23;
@@ -929,14 +956,16 @@ static int exynos_map_dt_data(struct platform_device *pdev)
 		data->max_efuse_value = 150;
 		break;
 	case SOC_ARCH_EXYNOS7:
-		data->tmu_set_trip_temp = exynos7_tmu_set_trip_temp;
-		data->tmu_set_trip_hyst = exynos7_tmu_set_trip_hyst;
+		data->tmu_set_low_temp = exynos7_tmu_set_low_temp;
+		data->tmu_set_high_temp = exynos7_tmu_set_high_temp;
+		data->tmu_disable_low = exynos5433_tmu_disable_low;
+		data->tmu_disable_high = exynos5433_tmu_disable_high;
+		data->tmu_set_crit_temp = exynos7_tmu_set_crit_temp;
 		data->tmu_initialize = exynos7_tmu_initialize;
 		data->tmu_control = exynos7_tmu_control;
 		data->tmu_read = exynos7_tmu_read;
 		data->tmu_set_emulation = exynos4412_tmu_set_emulation;
 		data->tmu_clear_irqs = exynos4210_tmu_clear_irqs;
-		data->ntrip = 8;
 		data->gain = 9;
 		data->reference_voltage = 17;
 		data->efuse_value = 75;
@@ -972,9 +1001,32 @@ static int exynos_map_dt_data(struct platform_device *pdev)
 	return 0;
 }
 
+static int exynos_set_trips(struct thermal_zone_device *tz, int low, int high)
+{
+	struct exynos_tmu_data *data = thermal_zone_device_priv(tz);
+
+	mutex_lock(&data->lock);
+	clk_enable(data->clk);
+
+	if (low > INT_MIN)
+		data->tmu_set_low_temp(data, low / MCELSIUS);
+	else
+		data->tmu_disable_low(data);
+	if (high < INT_MAX)
+		data->tmu_set_high_temp(data, high / MCELSIUS);
+	else
+		data->tmu_disable_high(data);
+
+	clk_disable(data->clk);
+	mutex_unlock(&data->lock);
+
+	return 0;
+}
+
 static const struct thermal_zone_device_ops exynos_sensor_ops = {
 	.get_temp = exynos_get_temp,
 	.set_emul_temp = exynos_tmu_set_emulation,
+	.set_trips = exynos_set_trips,
 };
 
 static int exynos_tmu_probe(struct platform_device *pdev)
-- 
2.42.0

Re: [PATCH v4 8/8] thermal: exynos: use set_trips

[Lukasz Luba]

On 10/25/23 14:30, Mateusz Majewski wrote:
> Currently, each trip point defined in the device tree corresponds to a
> single hardware interrupt. This commit instead switches to using two
> hardware interrupts, whose values are set dynamically using the
> set_trips callback. Additionally, the critical temperature threshold is
> handled specifically.
> 

[snip]

>   
> -static void exynos4210_tmu_set_trip_temp(struct exynos_tmu_data *data,
> -					 int trip_id, u8 temp)
> +static void exynos_tmu_update_bit(struct exynos_tmu_data *data, int reg_off,
> +				  int bit_off, bool enable)
>   {
> -	temp = temp_to_code(data, temp);
> -	writeb(temp, data->base + EXYNOS4210_TMU_REG_TRIG_LEVEL0 + trip_id * 4);
> +	u32 interrupt_en;
> +
> +	interrupt_en = readl(data->base + reg_off);
> +	if (enable)
> +		interrupt_en |= 1 << bit_off;
> +	else
> +		interrupt_en &= ~(1 << bit_off);

Why not to use dedicated stuff for this?
val |= BIT(x)
val &= ~BIT(x)
You can find plenty of example in the kernel

> +	writel(interrupt_en, data->base + reg_off);
>   }
>   

[snip]

> -static void exynos4412_tmu_set_trip_temp(struct exynos_tmu_data *data,
> -					 int trip, u8 temp)
> -{
> -	u32 th, con;
> -
> -	th = readl(data->base + EXYNOS_THD_TEMP_RISE);
> -	th &= ~(0xff << 8 * trip);
> -	th |= temp_to_code(data, temp) << 8 * trip;
> -	writel(th, data->base + EXYNOS_THD_TEMP_RISE);
> -
> -	if (trip == 3) {
> -		con = readl(data->base + EXYNOS_TMU_REG_CONTROL);
> -		con |= (1 << EXYNOS_TMU_THERM_TRIP_EN_SHIFT);
> -		writel(con, data->base + EXYNOS_TMU_REG_CONTROL);
> -	}
> -}
> -
> -static void exynos4412_tmu_set_trip_hyst(struct exynos_tmu_data *data,
> -					 int trip, u8 temp, u8 hyst)
> +static void exynos4412_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
>   {
>   	u32 th;
>   
>   	th = readl(data->base + EXYNOS_THD_TEMP_FALL);
> -	th &= ~(0xff << 8 * trip);
> -	if (hyst)
> -		th |= temp_to_code(data, temp - hyst) << 8 * trip;
> +	th &= ~(0xff << 0);
> +	th |= temp_to_code(data, temp) << 0;

This 2-line pattern repeats a few times. It looks like a nice cadidate
for an inline function which can abstract that. Something like:

val = update_temp_value(data, temp, threshold, LOW_TEMP_SHIFT)

Assisted with the macros {LOW|HIGH|CRIT}_TEMP_SHIFT, the code
would look less convoluted IMO.
(The old code with the multiplication for the shift value wasn't
cleaner nor faster).

>   	writel(th, data->base + EXYNOS_THD_TEMP_FALL);
> +
> +	exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
> +			      EXYNOS_TMU_INTEN_FALL0_SHIFT, true);
> +}

[snip]

> -static void exynos7_tmu_set_trip_temp(struct exynos_tmu_data *data,
> -				      int trip, u8 temp)
> +static void exynos7_tmu_update_temp(struct exynos_tmu_data *data, u8 temp,
> +				    int idx, bool rise)
>   {
>   	unsigned int reg_off, bit_off;
>   	u32 th;
> +	void __iomem *reg;
>   
> -	reg_off = ((7 - trip) / 2) * 4;
> -	bit_off = ((8 - trip) % 2);
> +	reg_off = ((7 - idx) / 2) * 4;

Why can't we just have a set of defined register macros and pick one
in some small function?
A lot of operations here, also some assumption.

> +	bit_off = ((8 - idx) % 2);

So this can only be 0 or 1 and than it's used for the shift
multiplication. Also I don't know the history of older code and
if it was missed after some cleaning, but 'idx % 2' gives
equal values but w/o subtraction.

BTW, the code assumes the 'idx' values are under control somewhere else.
Is that because the DT make sure in the schema that the range cannot be
too big?
What are the possible values for 'idx'?

>   
> -	th = readl(data->base + EXYNOS7_THD_TEMP_RISE7_6 + reg_off);
> +	reg = data->base +
> +	      (rise ? EXYNOS7_THD_TEMP_RISE7_6 : EXYNOS7_THD_TEMP_FALL7_6) +
> +	      reg_off;
> +	th = readl(reg);
>   	th &= ~(EXYNOS7_TMU_TEMP_MASK << (16 * bit_off));
>   	th |= temp_to_code(data, temp) << (16 * bit_off);

Can you simplify and abstract those bit_off usage and use some
macros and less math operations?

> -	writel(th, data->base + EXYNOS7_THD_TEMP_RISE7_6 + reg_off);
> +	writel(th, reg);
> +
> +	exynos_tmu_update_bit(data, EXYNOS5433_TMU_REG_INTEN,
> +			      (rise ? EXYNOS7_TMU_INTEN_RISE0_SHIFT :
> +				      EXYNOS_TMU_INTEN_FALL0_SHIFT) +
> +				      idx,
> +			      true);
>   }

[snip]

>   
> -	if (on) {
> -		for (i = 0; i < data->ntrip; i++) {
> -			if (thermal_zone_get_trip(tz, i, &trip))
> -				continue;
> -
> -			interrupt_en |=
> -				(1 << (EXYNOS_TMU_INTEN_RISE0_SHIFT + i * 4));
> -		}
> -
> -		if (data->soc != SOC_ARCH_EXYNOS4210)
> -			interrupt_en |=
> -				interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT;
> -
> +	if (on)
>   		con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
> -	} else {
> +	else
>   		con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT);

Please also consider the BIT() helper here and above...

> -	}
>   
> -	writel(interrupt_en, data->base + EXYNOS_TMU_REG_INTEN);
>   	writel(con, data->base + EXYNOS_TMU_REG_CONTROL);
>   }
>   
>   static void exynos5433_tmu_control(struct platform_device *pdev, bool on)
>   {
>   	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> -	struct thermal_zone_device *tz = data->tzd;
> -	struct thermal_trip trip;
> -	unsigned int con, interrupt_en = 0, pd_det_en, i;
> +	unsigned int con, pd_det_en;
>   
>   	con = get_con_reg(data, readl(data->base + EXYNOS_TMU_REG_CONTROL));
>   
> -	if (on) {
> -		for (i = 0; i < data->ntrip; i++) {
> -			if (thermal_zone_get_trip(tz, i, &trip))
> -				continue;
> -
> -			interrupt_en |=
> -				(1 << (EXYNOS7_TMU_INTEN_RISE0_SHIFT + i));
> -		}
> -
> -		interrupt_en |=
> -			interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT;
> -
> +	if (on)
>   		con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
> -	} else
> +	else
>   		con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT);

... and here. Basically in all places where it's possible.

Regards,
Lukasz

RE: Re: [PATCH v4 8/8] thermal: exynos: use set_trips

[Mateusz Majewski]

Hi,

> > +        th &= ~(0xff << 0);
> > +        th |= temp_to_code(data, temp) << 0;
> 
> This 2-line pattern repeats a few times. It looks like a nice cadidate
> for an inline function which can abstract that. Something like:
> 
> val = update_temp_value(data, temp, threshold, LOW_TEMP_SHIFT)
> 
> Assisted with the macros {LOW|HIGH|CRIT}_TEMP_SHIFT, the code
> would look less convoluted IMO.
> (The old code with the multiplication for the shift value wasn't
> cleaner nor faster).

What would you think about something like this?

static void exynos_tmu_update_temp(struct exynos_tmu_data *data, int reg_off,
                                   int bit_off, u8 temp)
{
        u32 th;

        th = readl(data->base + reg_off);
        th &= ~(0xff << bit_off);
        th |= temp_to_code(data, temp) << bit_off;
        writel(th, data->base + reg_off);
}

And then, it would be used like this:

static void exynos4412_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
{
        exynos_tmu_update_temp(data, EXYNOS_THD_TEMP_RISE, 24, temp);
        exynos_tmu_update_bit(data, EXYNOS_TMU_REG_CONTROL,
                              EXYNOS_TMU_THERM_TRIP_EN_SHIFT, true);
}

Granted it's not as clear as if we had some macro like CRIT_TEMP_SHIFT, but
we would need more than one variant anyway, as Exynos 5433 uses different
values of reg_off, and the new function looks short and inviting IMHO.

> > -static void exynos7_tmu_set_trip_temp(struct exynos_tmu_data *data,
> > -                                      int trip, u8 temp)
> > +static void exynos7_tmu_update_temp(struct exynos_tmu_data *data, u8 temp,
> > +                                    int idx, bool rise)
> >   {
> >           unsigned int reg_off, bit_off;
> >           u32 th;
> > +        void __iomem *reg;
> >   
> > -        reg_off = ((7 - trip) / 2) * 4;
> > -        bit_off = ((8 - trip) % 2);
> > +        reg_off = ((7 - idx) / 2) * 4;
> 
> Why can't we just have a set of defined register macros and pick one
> in some small function?
> A lot of operations here, also some assumption.
> 
> > +        bit_off = ((8 - idx) % 2);
> 
> So this can only be 0 or 1 and than it's used for the shift
> multiplication. Also I don't know the history of older code and
> if it was missed after some cleaning, but 'idx % 2' gives
> equal values but w/o subtraction.
> 
> BTW, the code assumes the 'idx' values are under control somewhere else.
> Is that because the DT make sure in the schema that the range cannot be
> too big?
> What are the possible values for 'idx'?

In the old code, the values of trip (which is the same thing, I will
change the name back from idx) were limited by the value of data->ntrip,
which was always 8 (value is per SoC). In the new code, there are only three
variants:

static void exynos7_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
{
        exynos7_tmu_update_temp(data, temp, 0, false);
}

static void exynos7_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
{
        exynos7_tmu_update_temp(data, temp, 1, true);
}

static void exynos7_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
{
        /*
         * Like Exynos 4210, Exynos 7 does not seem to support critical temperature
         * handling in hardware. Again, we still set a separate interrupt for it.
         */
        exynos7_tmu_update_temp(data, temp, 7, true);
}

To be fair, considering the values are constant like this, I should probably
just do the calculations myself and then in code just call exynos_tmu_update_temp
(from above) and exynos_tmu_update_bit, like on all other SoCs. I guess I were
a bit too scared to touch Exynos 7 code...

> > -        if (on) {
> > -                for (i = 0; i < data->ntrip; i++) {
> > -                        if (thermal_zone_get_trip(tz, i, &trip))
> > -                                continue;
> > -
> > -                        interrupt_en |=
> > -                                (1 << (EXYNOS_TMU_INTEN_RISE0_SHIFT + i * 4));
> > -                }
> > -
> > -                if (data->soc != SOC_ARCH_EXYNOS4210)
> > -                        interrupt_en |=
> > -                                interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT;
> > -
> > +        if (on)
> >                   con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
> > -        } else {
> > +        else
> >                   con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT);
> 
> Please also consider the BIT() helper here and above...

Will do, but should I do this in a separate patch in these cases? I don't touch
the con lines otherwise, and this patch is already humongous.

Thank you :)
Mateusz

Re: [PATCH v4 8/8] thermal: exynos: use set_trips

[Lukasz Luba]

On 11/2/23 10:35, Mateusz Majewski wrote:
> Hi,
> 
>>> +        th &= ~(0xff << 0);
>>> +        th |= temp_to_code(data, temp) << 0;
>>   
>> This 2-line pattern repeats a few times. It looks like a nice cadidate
>> for an inline function which can abstract that. Something like:
>>   
>> val = update_temp_value(data, temp, threshold, LOW_TEMP_SHIFT)
>>   
>> Assisted with the macros {LOW|HIGH|CRIT}_TEMP_SHIFT, the code
>> would look less convoluted IMO.
>> (The old code with the multiplication for the shift value wasn't
>> cleaner nor faster).
> 
> What would you think about something like this?
> 
> static void exynos_tmu_update_temp(struct exynos_tmu_data *data, int reg_off,
>                                     int bit_off, u8 temp)
> {
>          u32 th;
> 
>          th = readl(data->base + reg_off);
>          th &= ~(0xff << bit_off);
>          th |= temp_to_code(data, temp) << bit_off;
>          writel(th, data->base + reg_off);
> }
> 
> And then, it would be used like this:
> 
> static void exynos4412_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          exynos_tmu_update_temp(data, EXYNOS_THD_TEMP_RISE, 24, temp);
>          exynos_tmu_update_bit(data, EXYNOS_TMU_REG_CONTROL,
>                                EXYNOS_TMU_THERM_TRIP_EN_SHIFT, true);
> }

Yes, this looks good.

> 
> Granted it's not as clear as if we had some macro like CRIT_TEMP_SHIFT, but
> we would need more than one variant anyway, as Exynos 5433 uses different
> values of reg_off, and the new function looks short and inviting IMHO.

Fair enough.

> 
>>> -static void exynos7_tmu_set_trip_temp(struct exynos_tmu_data *data,
>>> -                                      int trip, u8 temp)
>>> +static void exynos7_tmu_update_temp(struct exynos_tmu_data *data, u8 temp,
>>> +                                    int idx, bool rise)
>>>     {
>>>             unsigned int reg_off, bit_off;
>>>             u32 th;
>>> +        void __iomem *reg;
>>>     
>>> -        reg_off = ((7 - trip) / 2) * 4;
>>> -        bit_off = ((8 - trip) % 2);
>>> +        reg_off = ((7 - idx) / 2) * 4;
>>   
>> Why can't we just have a set of defined register macros and pick one
>> in some small function?
>> A lot of operations here, also some assumption.
>>   
>>> +        bit_off = ((8 - idx) % 2);
>>   
>> So this can only be 0 or 1 and than it's used for the shift
>> multiplication. Also I don't know the history of older code and
>> if it was missed after some cleaning, but 'idx % 2' gives
>> equal values but w/o subtraction.
>>   
>> BTW, the code assumes the 'idx' values are under control somewhere else.
>> Is that because the DT make sure in the schema that the range cannot be
>> too big?
>> What are the possible values for 'idx'?
> 
> In the old code, the values of trip (which is the same thing, I will
> change the name back from idx) were limited by the value of data->ntrip,
> which was always 8 (value is per SoC). In the new code, there are only three
> variants:
> 
> static void exynos7_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          exynos7_tmu_update_temp(data, temp, 0, false);
> }
> 
> static void exynos7_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          exynos7_tmu_update_temp(data, temp, 1, true);
> }
> 
> static void exynos7_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          /*
>           * Like Exynos 4210, Exynos 7 does not seem to support critical temperature
>           * handling in hardware. Again, we still set a separate interrupt for it.
>           */
>          exynos7_tmu_update_temp(data, temp, 7, true);
> }
> 
> To be fair, considering the values are constant like this, I should probably
> just do the calculations myself and then in code just call exynos_tmu_update_temp
> (from above) and exynos_tmu_update_bit, like on all other SoCs. I guess I were
> a bit too scared to touch Exynos 7 code...

Yes, anything that can be pre-calculated with nice comment, would be
more desired. I would suggest to not be afraid about touching that
Exynos 7 code.

> 
>>> -        if (on) {
>>> -                for (i = 0; i < data->ntrip; i++) {
>>> -                        if (thermal_zone_get_trip(tz, i, &trip))
>>> -                                continue;
>>> -
>>> -                        interrupt_en |=
>>> -                                (1 << (EXYNOS_TMU_INTEN_RISE0_SHIFT + i * 4));
>>> -                }
>>> -
>>> -                if (data->soc != SOC_ARCH_EXYNOS4210)
>>> -                        interrupt_en |=
>>> -                                interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT;
>>> -
>>> +        if (on)
>>>                     con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
>>> -        } else {
>>> +        else
>>>                     con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT);
>>   
>> Please also consider the BIT() helper here and above...
> 
> Will do, but should I do this in a separate patch in these cases? I don't touch
> the con lines otherwise, and this patch is already humongous.

That would definitely deserve an extra patch to address it. Please add
to the patch set.

Regards,
Lukasz