Re: [PATCH 1/3 v4] thermal: samsung: correct the fall interrupt en, status bit fields

From: Eduardo Valentin <eduardo.valentin@ti.com>
To: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: Eduardo Valentin <eduardo.valentin@ti.com>,
	Naveen Krishna Chatradhi <ch.naveen@samsung.com>,
	<linux-pm@vger.kernel.org>, <naveenkrishna.ch@gmail.com>,
	<rui.zhang@intel.com>, <linux-samsung-soc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <amit.daniel@samsung.com>,
	<kgene.kim@samsung.com>, <devicetree@vger.kernel.org>,
	Lukasz Majewski <l.majewski@samsung.com>
Subject: Re: [PATCH 1/3 v4] thermal: samsung: correct the fall interrupt en, status bit fields
Date: Mon, 14 Oct 2013 10:18:03 -0400	[thread overview]
Message-ID: <525BFD1B.5080709@ti.com> (raw)
In-Reply-To: <1615083.zyGXhBfPcS@amdc1032>

[-- Attachment #1: Type: text/plain, Size: 6645 bytes --]

On 11-10-2013 11:57, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Friday, October 11, 2013 11:10:38 AM Eduardo Valentin wrote:
>> Hi Naveen,
>>
>> On 09-10-2013 10:03, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> Hi,
>>>
>>> All patches (#1-#3) look good to me, FWIW you can add:
>>>
>>> 	Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>>
>>> Please note that (at least) patch #3 conflicts with Lukasz's EXYNOS4412
>>> fixup patchset:
>>>
>>> 	https://lkml.org/lkml/2013/10/9/35
>>>
>>> It is up to Eduardo to resolve this but it probably would be better to
>>> merge EXYNOS4412 fixes first and then add EXYNOS5420 support. This would
>>> require you to port patch #3 over Lukasz's patchset though.
>>
>> My question is if this fix applies also to EXYNOS4412, as it  is not
>> mentioned in the patch description and the change affects all supported
> 
> This patch doesn't affect EXYNOS4210 as exynos4210_tmu_registers struct

I was, at least for now, worried about 4412, as I mentioned above.

> uses the default zero value for inten_fall_mask, inten_fall_shift and
> intclr_fall_shift.
> 
>> chip version deliberately. Has this change been validated on all
>> supported chip versions?
>>
>> Amit, I saw you ack, but still, it is not clear how this change behaves
>> across supported hardware.
> 
> For EXYNOS4412 and EXYNOS5250 this patch doesn't cause any functionality
> changes because while the patch changes inten_fall_shift usage to
> intclr_fall_shift one in exynos_tmu_initialize() it defines
> EXYNOS_TMU_CLEAR_FALL_INT_SHIFT to 12 (old EXYNOS_TMU_FALL_INT_SHIFT
> value).
> 

OK. Then the patch is about a symbol rename, right?

> This patch only changes driver behavior for EXYNOS5440 on which the
> used shift value changes from 4 to 12.

I see.

> 
> PS I've only noticed it now but after this patch inten_fall_shift becomes
> unused and can be removed.
> 

Then we should remove it.

> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
>>>
>>> Best regards,
>>> --
>>> Bartlomiej Zolnierkiewicz
>>> Samsung R&D Institute Poland
>>> Samsung Electronics
>>>
>>> On Wednesday, October 09, 2013 05:38:27 PM Naveen Krishna Chatradhi wrote:
>>>> The FALL interrupt related en, status bits are available at an offset of
>>>> 16 on INTEN, INTSTAT registers and at an offset of
>>>> 12 on INTCLEAR register.
>>>>
>>>> This patch corrects the same for exyns5250 and exynos5440
>>>>
>>>> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
>>>> ---
>>>> Changes since v1:
>>>> Changes since v2:
>>>> Changes since v3:
>>>>   None
>>>>
>>>>  drivers/thermal/samsung/exynos_tmu.c      |    2 +-
>>>>  drivers/thermal/samsung/exynos_tmu.h      |    2 ++
>>>>  drivers/thermal/samsung/exynos_tmu_data.c |    2 ++
>>>>  drivers/thermal/samsung/exynos_tmu_data.h |    3 ++-
>>>>  4 files changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>>> index b43afda..af69209 100644
>>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>>> @@ -265,7 +265,7 @@ skip_calib_data:
>>>>  				data->base + reg->threshold_th1);
>>>>  
>>>>  		writel((reg->inten_rise_mask << reg->inten_rise_shift) |
>>>> -			(reg->inten_fall_mask << reg->inten_fall_shift),
>>>> +			(reg->inten_fall_mask << reg->intclr_fall_shift),
>>>>  				data->base + reg->tmu_intclear);
>>>>  
>>>>  		/* if last threshold limit is also present */
>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
>>>> index b364c9e..7c6c34a 100644
>>>> --- a/drivers/thermal/samsung/exynos_tmu.h
>>>> +++ b/drivers/thermal/samsung/exynos_tmu.h
>>>> @@ -134,6 +134,7 @@ enum soc_type {
>>>>   * @inten_fall3_shift: shift bits of falling 3 interrupt bits.
>>>>   * @tmu_intstat: Register containing the interrupt status values.
>>>>   * @tmu_intclear: Register for clearing the raised interrupt status.
>>>> + * @intclr_fall_shift: shift bits for interrupt clear fall 0
>>>>   * @emul_con: TMU emulation controller register.
>>>>   * @emul_temp_shift: shift bits of emulation temperature.
>>>>   * @emul_time_shift: shift bits of emulation time.
>>>> @@ -204,6 +205,7 @@ struct exynos_tmu_registers {
>>>>  	u32	tmu_intstat;
>>>>  
>>>>  	u32	tmu_intclear;
>>>> +	u32	intclr_fall_shift;
>>>>  
>>>>  	u32	emul_con;
>>>>  	u32	emul_temp_shift;
>>>> diff --git a/drivers/thermal/samsung/exynos_tmu_data.c b/drivers/thermal/samsung/exynos_tmu_data.c
>>>> index 9002499..23fea23 100644
>>>> --- a/drivers/thermal/samsung/exynos_tmu_data.c
>>>> +++ b/drivers/thermal/samsung/exynos_tmu_data.c
>>>> @@ -122,6 +122,7 @@ static const struct exynos_tmu_registers exynos5250_tmu_registers = {
>>>>  	.inten_fall0_shift = EXYNOS_TMU_INTEN_FALL0_SHIFT,
>>>>  	.tmu_intstat = EXYNOS_TMU_REG_INTSTAT,
>>>>  	.tmu_intclear = EXYNOS_TMU_REG_INTCLEAR,
>>>> +	.intclr_fall_shift = EXYNOS_TMU_CLEAR_FALL_INT_SHIFT,
>>>>  	.emul_con = EXYNOS_EMUL_CON,
>>>>  	.emul_temp_shift = EXYNOS_EMUL_DATA_SHIFT,
>>>>  	.emul_time_shift = EXYNOS_EMUL_TIME_SHIFT,
>>>> @@ -210,6 +211,7 @@ static const struct exynos_tmu_registers exynos5440_tmu_registers = {
>>>>  	.inten_fall0_shift = EXYNOS5440_TMU_INTEN_FALL0_SHIFT,
>>>>  	.tmu_intstat = EXYNOS5440_TMU_S0_7_IRQ,
>>>>  	.tmu_intclear = EXYNOS5440_TMU_S0_7_IRQ,
>>>> +	.intclr_fall_shift = EXYNOS_TMU_CLEAR_FALL_INT_SHIFT,
>>>>  	.tmu_irqstatus = EXYNOS5440_TMU_IRQ_STATUS,
>>>>  	.emul_con = EXYNOS5440_TMU_S0_7_DEBUG,
>>>>  	.emul_temp_shift = EXYNOS_EMUL_DATA_SHIFT,
>>>> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
>>>> index dc7feb5..8788a87 100644
>>>> --- a/drivers/thermal/samsung/exynos_tmu_data.h
>>>> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
>>>> @@ -69,9 +69,10 @@
>>>>  #define EXYNOS_TMU_RISE_INT_MASK	0x111
>>>>  #define EXYNOS_TMU_RISE_INT_SHIFT	0
>>>>  #define EXYNOS_TMU_FALL_INT_MASK	0x111
>>>> -#define EXYNOS_TMU_FALL_INT_SHIFT	12
>>>> +#define EXYNOS_TMU_FALL_INT_SHIFT	16
>>>>  #define EXYNOS_TMU_CLEAR_RISE_INT	0x111
>>>>  #define EXYNOS_TMU_CLEAR_FALL_INT	(0x111 << 12)
>>>> +#define EXYNOS_TMU_CLEAR_FALL_INT_SHIFT	12
>>>>  #define EXYNOS_TMU_TRIP_MODE_SHIFT	13
>>>>  #define EXYNOS_TMU_TRIP_MODE_MASK	0x7
>>>>  #define EXYNOS_TMU_THERM_TRIP_EN_SHIFT	12
> 
> 
> 

-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]