This work improves Exynos thermal driver in various ways. This is
related to the discussion in
https://lore.kernel.org/all/[email protected]/
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:
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 | 536 ++++++++++++++-------------
1 file changed, 282 insertions(+), 254 deletions(-)
--
2.42.0
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.
Signed-off-by: Mateusz Majewski <[email protected]>
---
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 0e970638193d..6070b03cff9d 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
We rewrite the initialization to enable the regulator as part of devm,
which allows us to not handle the struct instance manually. We also
remove the error message in case the regulator is unavailable, as this
is expected behaviour.
Signed-off-by: Mateusz Majewski <[email protected]>
---
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 6070b03cff9d..a0a1f7e1e63f 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;
}
@@ -1125,9 +1109,6 @@ static int exynos_tmu_remove(struct platform_device *pdev)
if (!IS_ERR(data->clk_sec))
clk_unprepare(data->clk_sec);
- if (!IS_ERR(data->regulator))
- regulator_disable(data->regulator);
-
return 0;
}
--
2.42.0
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.
Signed-off-by: Mateusz Majewski <[email protected]>
---
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 7138e001fa5a..343e27c61528 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);
}
+
+ mutex_unlock(&data->lock);
+ clk_disable(data->clk);
+ if (!IS_ERR(data->clk_sec))
+ clk_disable(data->clk_sec);
+
+ 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
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 <[email protected]>
---
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 343e27c61528..667bb18205fc 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
It seems that the field has been removed in one of the previous commits,
but the description has been forgotten.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Mateusz Majewski <[email protected]>
---
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 e5bc2c82010f..2c1cfb8c4b33 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
The workqueue boilerplate is mostly one-to-one what the threaded
interrupts do.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Mateusz Majewski <[email protected]>
---
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 90c33e8017af..0e970638193d 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
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 <[email protected]>
Signed-off-by: Mateusz Majewski <[email protected]>
---
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 a0a1f7e1e63f..7138e001fa5a 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
On 03/10/2023 13:16, Mateusz Majewski wrote:
> 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.
>
> Signed-off-by: Mateusz Majewski <[email protected]>
> ---
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 03/10/2023 13:16, Mateusz Majewski wrote:
> We rewrite the initialization to enable the regulator as part of devm,
> which allows us to not handle the struct instance manually. We also
> remove the error message in case the regulator is unavailable, as this
> is expected behaviour.
>
> Signed-off-by: Mateusz Majewski <[email protected]>
> ---
> 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.
>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 03/10/2023 13:16, Mateusz Majewski wrote:
> 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.
>
> Signed-off-by: Mateusz Majewski <[email protected]>
> ---
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Hi Mateusz,
On 10/3/23 12:16, Mateusz Majewski wrote:
> 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.
>
> Signed-off-by: Mateusz Majewski <[email protected]>
> ---
> 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 7138e001fa5a..343e27c61528 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);
> }
> +
> + mutex_unlock(&data->lock);
> + clk_disable(data->clk);
> + if (!IS_ERR(data->clk_sec))
> + clk_disable(data->clk_sec);
In all other places the clock is strictly protected inside the critical
section, but not here. In this code in theory on SMP (especially with
big.LITTLE system with different speeds of CPUs) this could lead to
disabling the clocks just after other CPU acquired the mutex and enabled
them (in order to use the HW regs).
Please put those two clocks before the mutex_unlock() and in the
reverse order.
Regards,
Lukasz
On 23.10.2023 14:59, Lukasz Luba wrote:
> On 10/3/23 12:16, Mateusz Majewski wrote:
>> 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.
>>
>> Signed-off-by: Mateusz Majewski <[email protected]>
>> ---
>> 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 7138e001fa5a..343e27c61528 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);
>> }
>> +
>> + mutex_unlock(&data->lock);
>> + clk_disable(data->clk);
>> + if (!IS_ERR(data->clk_sec))
>> + clk_disable(data->clk_sec);
>
> In all other places the clock is strictly protected inside the critical
> section, but not here. In this code in theory on SMP (especially with
> big.LITTLE system with different speeds of CPUs) this could lead to
> disabling the clocks just after other CPU acquired the mutex and enabled
> them (in order to use the HW regs).
Clocks have internal atomic counters, so it is safe to disable them
after leaving critical section. The clock would still be enabled in the
mentioned case.
> Please put those two clocks before the mutex_unlock() and in the
> reverse order.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On 10/23/23 14:33, Marek Szyprowski wrote:
> On 23.10.2023 14:59, Lukasz Luba wrote:
>> On 10/3/23 12:16, Mateusz Majewski wrote:
>>> 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.
>>>
>>> Signed-off-by: Mateusz Majewski <[email protected]>
>>> ---
>>> 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 7138e001fa5a..343e27c61528 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);
>>> }
>>> +
>>> + mutex_unlock(&data->lock);
>>> + clk_disable(data->clk);
>>> + if (!IS_ERR(data->clk_sec))
>>> + clk_disable(data->clk_sec);
>>
>> In all other places the clock is strictly protected inside the critical
>> section, but not here. In this code in theory on SMP (especially with
>> big.LITTLE system with different speeds of CPUs) this could lead to
>> disabling the clocks just after other CPU acquired the mutex and enabled
>> them (in order to use the HW regs).
>
>
> Clocks have internal atomic counters, so it is safe to disable them
> after leaving critical section. The clock would still be enabled in the
> mentioned case.
Fair enough. So I would just put them there for code cleanup and
aliment with all other places (otherwise it looks odd).