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:
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
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 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
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 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
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 <[email protected]>
Signed-off-by: Mateusz Majewski <[email protected]>
---
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
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 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
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 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
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 <[email protected]>
Signed-off-by: Mateusz Majewski <[email protected]>
---
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
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
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
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