Message-ID: <CANAwSgT2ROe77FVgk41s3xB-a+2SNwo5XWRZzrgxtC_SiooTXA@mail.gmail.com>
Date: Thu, 26 Jun 2025 23:51:38 +0530
From: Anand Moon <linux.amoon@...il.com>
To: Mateusz Majewski <m.majewski2@...sung.com>
Cc: alim.akhtar@...sung.com, bzolnier@...il.com, daniel.lezcano@...aro.org, 
	krzk@...nel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, 
	linux-samsung-soc@...r.kernel.org, lukasz.luba@....com, rafael@...nel.org, 
	rui.zhang@...el.com
Subject: Re: [RRC v1 2/3] thermal/drivers/exynos: Handle temperature threshold
 interrupts and clear corresponding IRQs

Hi Mateusz

On Tue, 24 Jun 2025 at 13:28, Mateusz Majewski <m.majewski2@...sung.com> wrote:
>
> > I tried to configure this, referring to the comment in the driver
> >         /*
> >          * Clear the interrupts.  Please note that the documentation for
> >          * Exynos3250, Exynos4412, Exynos5250 and Exynos5260 incorrectly
> >          * states that INTCLEAR register has a different placing of bits
> >          * responsible for FALL IRQs than INTSTAT register.  Exynos5420
> >          * and Exynos5440 documentation is correct (Exynos4210 doesn't
> >          * support FALL IRQs at all).
> >          */
> >
> > By the way, I don't see Exynos5433 and Exynos7 support
> > INTSTAT and INTCLEAR registers. We are using TMU_REG_INTPEND
> >  to read and update the same register.
> >
> >         if (data->soc == SOC_ARCH_EXYNOS5260) {
> >                 tmu_intstat = EXYNOS5260_TMU_REG_INTSTAT;
> >                 tmu_intclear = EXYNOS5260_TMU_REG_INTCLEAR;
> >         } else if (data->soc == SOC_ARCH_EXYNOS7) {
> >                 tmu_intstat = EXYNOS7_TMU_REG_INTPEND;
> >                 tmu_intclear = EXYNOS7_TMU_REG_INTPEND;
> >         } else if (data->soc == SOC_ARCH_EXYNOS5433) {
> >                 tmu_intstat = EXYNOS5433_TMU_REG_INTPEND;
> >                 tmu_intclear = EXYNOS5433_TMU_REG_INTPEND;
> >         } else {
> >                 tmu_intstat = EXYNOS_TMU_REG_INTSTAT;
> >                 tmu_intclear = EXYNOS_TMU_REG_INTCLEAR;
> >         }
>
> My understanding of this comment and the situation in general is like
> this:
>
Thanks for clarifying this in such detail.
> 1. On 5420, whenever there is edge interrupt, no matter if rise or fall,
>    a bit gets set to 1 inside INTSTAT, and we clear it by setting the
>    same bit to 1 inside INTCLEAR. The current code does not rely on the
>    concrete bit index, it will just check the temperature after the
>    interrupt.
Correct
> 2. On 4210, there is no falling edge interrupts (so
>    exynos4210_tmu_set_low_temp is empty, we enable polling in DT etc).
>    This is what the "Exynos4210 doesn't support FALL IRQs at all" means.
>    However, rising edge interrupts work exactly the same as on 5420:
>    a bit gets set to 1 inside INTSTAT, and we clear it by setting the
>    same bit to 1 inside INTCLEAR.
For Exynos4210, I am skipping this in the next patch.
> 3. On 3250, 4412, 5250, 5260, it again works the same way as 5420.
>    However, somebody had a copy of documentation that was incorrect: it
>    said that bit indices does not match somehow, which is not true.

> 4. On 5433 and 7, it one more time works the same way as 5420, with a
>    single change: a bit gets set to 1 inside INTPEND, and we clear it
>    by setting it to 1 inside the same INTPEND.
>
> So, all we need to do to support existing SoCs is to read the 1 bit from
> one register, and set the bit with the same index in another register
> (which on some SoCs is the same register). We could interpret the index
> to see what kind of interrupt is this, but we read the temperature to
> get similar information.
>
> So in the end, is it helpful to interpret the INTSTAT bit index, only to
> reset the exact same index inside INTCLEAR? I guess it could be valuable
> if we also used the information about which interrupt it is and somehow
> used it elsewhere (which could actually help with some issues), but that
> is another thing to do.
>
correct.
> > If you have details on how INTSTAT and INTCLEAR are used
> > particularly regarding the update bits, please share them.
> > Specifically, I'm interested in how bits [7:0] correspond to rising edge
> > interrupts and bits [23:16] to falling edge interrupts
> > I feel it's the same as Exynos54222.
>
> Regarding concrete indices on 5433:
> - the 0th bit corresponds to RISE0,
> - the 1st bit corresponds to RISE1,
> - ...
> - the 7th bit corresponds to RISE7,
> - the 16th bit corresponds to FALL0,
> - the 17th bit corresponds to FALL1,
> - ...
> - the 23th bit corresponds to FALL7.
>
Thanks, I will update this in the next version.

> That is probably because this SoC supports more interrupts than others.
> Though do note that currently, we only use part of them (one RISE, one
> FALL if supported, and another RISE for critical temperature (one
> supporting hardware thermal tripping if possible)). Also note that the
> indices in INTSTAT/INTCLEAR/INTPEND match the ones in INTEN, though I
> have not checked thoroughly if that is true for all the SoCs.
>
As per the manuals, we have to enable each corresponding bit for RISE and FALL

INTEN          0x0070    Interrupt enable register
INTSTAT      0x0074    Interrupt status register
INTCLEAR   0x0078    Interrupt clear register

When current temperature exceeds a threshold rise temperature, then
it generates corresponding interrupt (INTREQ_RISE[2:0]).
When current temperature goes below a threshold fall temperature, then
it generates corresponding interrupt (INTREQ_FALL[2:0].

If we correctly configure the mapping of the RISE and FALL bits along with
INTEN, INTSTAT, and INTCLEAR, the interrupt behavior should function
as expected.

The following outlines the interrupt flow as described in the user manual

[1} https://usermanual.wiki/Document/Exynos20441220SCPUsers20ManualVer01000Preliminary0.167615881

/* Read the measured data from e-fuse */
Triminfo_25 = TRIMINFO[7:0]
/* Calibrated threshold temperature is written into THRES_TEMP_RISE
and THRES_TEMP_FALL */
/* Refer to 1.6.1 */
THRES_TEMP_RISE0 = 0x40;
THRES_TEMP_RISE1 = 0x50;
THRES_TEMP_RISE2 = 0x60;
THRES_TEMP_RISE3 = 0x70;
THRES_TEMP_FALL0 = 0x3A;
THRES_TEMP_FALL1 = 0x4A;
THRES_TEMP_FALL2 = 0x5A;
/* Parameter for sampling interval is set */
SAMPLING_INTERVAL = 0x1;
/* Interrupt enable */
INTEN[24] =0x1; // for INTEN_FALL2
INTEN[20] =0x1; // for INTEN_FALL1
INTEN[16] =0x1; // for INTEN_FALL0
INTEN[8] =0x1; // for INTEN_RISE2
INTEN[4] =0x1; // for INTEN_RISE1
INTEN[0] =0x1; // for INTEN_RISE0
/* Thermal tripping mode selection */
THERM_TRIP_MODE = 0x4;
/* Thermal tripping enable */
THERM_TRIP_EN = 0x1;
/* Check sensing operation is idle */
tmu_idle = 0;
while(tmu_idle&1) {
    tmu_idle = TMU_STATUS[0];
}
/* Start sensing operation */
TMU_CONTROL |= 1;

ISR_INTREQ_TMU () {
/* Read interrupt status register */
int_status = INTSTAT;
if(int_status[24]) {
    ISR_INT_FALL2();
}
else if(int_status[20]) {
    ISR_INT_FALL1();
}
else if(int_status[16]) {
    ISR_INT_FALL0();
}
Else if(int_status[8]) {
    ISR_INT_RISE2();
}
else if(int_status[4]) {
    ISR_INT_RISE1();
}
else if(int_status[0]) {
    ISR_INT_RISE0();
}
else {
    $display("Some error occurred..!");
}
ISR_INT0 () {
   /* Perform proper task for decrease temperature */
    INTCLEAR[0] = 0x1;
}

Hey, if you’ve got a bit of time, could you take a look at these changes?
[2]  https://lore.kernel.org/all/20250430123306.15072-2-linux.amoon@gmail.com/


> Thank you,
> Mateusz Majewski
Thanks
-Anand

Powered by blists - more mailing lists