Hi Mateusz,
Thanks for your review comments..
On Tue, 19 Aug 2025 at 18:47, Mateusz Majewski <m.majewski2@xxxxxxxxxxx> wrote:
>
> Hello :)
>
> > +/* Map Rise and Falling edges for IRQ Clean */
> > +struct tmu_irq_map {
> > + u32 fall[3];
> > + u32 rise[3];
> > +};
>
Ops, this is an overkill approach that could be simplified further.
static void exynos4412_tmu_clear_irqs(struct exynos_tmu_data *data)
{
unsigned int val_irq, clear_irq = 0;
u32 tmu_intstat = data->tmu_intstat;
u32 tmu_intclear = data->tmu_intclear;
static const u32 irq_bits[] = {
BIT(0), BIT(4), BIT(8), /* Rising edge */
BIT(12), BIT(16), BIT(20) /* Falling edge */
};
val_irq = readl(data->base + tmu_intstat);
/* Set SoC-specific interrupt bit mappings */
for (int i = 0; i < ARRAY_SIZE(irq_bits); i++) {
if (val_irq & irq_bits[i])
clear_irq |= irq_bits[i];
}
if (clear_irq)
writel(clear_irq, data->base + tmu_intclear);
}
> Hmm, we can probably get away with less interrupts. We actually only
> enable one fall interrupt in tmu_set_low_temp and one rise interrupt in
> tmu_set_high_temp.
Got it — we need to enable INTEN for both rising and falling edges,
specific to each SoC.
So each SoC has a one-to-one mapping of INTEN with INTSTAT and INTCLEAR bits
for rising and falling edge detection:.
Exynos4412
Falling edge: bits 20, 16, 12
Rising edge: bits 8, 4, 0
Exynos5422
Falling edge: bits 24, 20, 16
Rising edge: bits 8, 4, 0
Exynos5433
Falling edge: bits 23, 17, 16
Rising edge: bits 7, 1, 0
So we need to enable INTEN like below.
static void exynos4412_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
{
exynos_tmu_update_temp(data, EXYNOS_THD_TEMP_FALL, 0, temp);
- exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
- EXYNOS_TMU_INTEN_FALL0_SHIFT, true);
+ for (int i = 0; i < 3; i++)
+ exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
+ EXYNOS_TMU_INTEN_FALL0_SHIFT + i
* 4, true);
}
static void exynos4412_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
{
exynos_tmu_update_temp(data, EXYNOS_THD_TEMP_RISE, 8, temp);
- exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
- EXYNOS_TMU_INTEN_RISE0_SHIFT + 4, true);
+ for (int i = 0; i < 3; i++)
+ exynos_tmu_update_bit(data, EXYNOS_TMU_REG_INTEN,
+ EXYNOS_TMU_INTEN_RISE0_SHIFT + 1 * 4, true);
}
>
> Regarding tmu_set_crit_temp, on SoCs that have hardware thermal tripping
> there is nothing to clear. On others we will reboot immediately anyway,
> though maybe there is nothing wrong with clearing the interrupt
> beforehand? Regardless of this, there is only a rise critical
> temperature interrupt, we never set a matching fall interrupt.
>
As per my understanding, it depends on the calibration consists of reading the
measured data from e-fuse and wrote the calibrated threshold temperature to
generate interrupts into trigger level 0-3
THRES_TEMP_RISE and THRES_TEMP_FALL.
When the current temperature exceeds a threshold rise temperature,
then it generates the corresponding interrupt (INTREQ_RISE[2:0]).
When the current temperature goes below a threshold fall temperature, t
hen it generates a corresponding interrupt (INTREQ_FALL[2:0].
It also depends on the sample interval
> Maybe it would also be good to add a bool to this struct containing
> information about whether a fall interrupt is in use, and reuse
> the same logic for 4210?
>
> (Nitpick: I am not a native speaker of English, but I think "clean" and
> "clear" have slightly different meanings, and the rest of the code
> consistently uses "clear", so it would be worthwhile to also use "clear"
> here.)
>
> > + break;
>
> Maybe put irq_map inside exynos_tmu_data? exynos_map_dt_data has a
> switch block that is quite similar, in that it also matches on the SoC
> type. This way also there is no need to have a fallback.
>
I want to avoid this If you have any design feedback, plz let me know.
I had attempted to refactor exynos_tmu_data by splitting it per-SoC,
assigning dedicated callback functions for each variant. The goal was
to eliminate
the need for data->soc checks and make the code more modular.
However, the approach didn’t work as expected—devm_thermal_of_zone_register()
wouldn’t bind the sensor-specific callbacks correctly.
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,
};
> Kind regards,
> Mateusz Majewski
Thanks
-Anand
