linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv4 0/4] thermal: exynos: Add support for TRIMINFO feature of Exynos3250
@ 2014-08-25  7:30 Chanwoo Choi
  2014-08-25  7:30 ` [PATCHv4 1/4] thermal: exynos: Fix wrong value of TRIMINFO_RELOAD_SHIFT Chanwoo Choi
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Chanwoo Choi @ 2014-08-25  7:30 UTC (permalink / raw)
  To: eduardo.valentin, amit.daniel, rui.zhang
  Cc: kgene.kim, robh+dt, pawel.moll, mark.rutland, galak, ch.naveen,
	kyungmin.park, linux-pm, devicetree, linux-arm-kernel,
	linux-samsung-soc, linux-kernel, Chanwoo Choi

This patchset add the support of TRIMINFO_RELOAD feature for Exynos3250.
But Exynos3250 has two TRIMINFO_CTRL register instead other Exynos has only one
TRIMINFO_CTRL register. So, this patchset support the some Exynos SoC which
has more than one TRIMINF_CTRL.

Also, this patchset fix wrong value of TRIMINFO_RELOAD_SHIFT and remove
duplicate code when reading triminfo register of Exynos5440.

Changes from v3:
- Add reviewed message of Amit Daniel Kachhap <amit.daniel@samsung.com>
- Split RELOAD patch as two patch
- Fix 'TRIMINFO_RELOAD_SHIFT' value
- This series includes separate patch[1]
  [1] https://lkml.org/lkml/2014/8/20/5
- Drop ACTIME bit setting because TRM includes not enough information of ACTIME bit.

Changes from v2:
- Fix build break because of missing 'or' operation.

Changes from v1:
- Add missing 'TMU_SUPPORT_TRIM_RELOAD' features

Chanwoo Choi (4):
  thermal: exynos: Fix wrong value of TRIMINFO_RELOAD_SHIFT
  thermal: exynos: Add support for many TRIMINFO_CTRL registers
  thermal: exynos: Add support for TRIM_RELOAD feature at Exynos3250
  thermal: exynos: Remove duplicate code when reading triminfo register
    of Exynos5440

 drivers/thermal/samsung/exynos_thermal_common.h |  1 +
 drivers/thermal/samsung/exynos_tmu.c            | 27 +++++++++++++++++++------
 drivers/thermal/samsung/exynos_tmu.h            |  9 +++++++--
 drivers/thermal/samsung/exynos_tmu_data.c       | 14 +++++++++++--
 drivers/thermal/samsung/exynos_tmu_data.h       | 12 ++++++++---
 5 files changed, 50 insertions(+), 13 deletions(-)

-- 
1.8.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCHv4 1/4] thermal: exynos: Fix wrong value of TRIMINFO_RELOAD_SHIFT
  2014-08-25  7:30 [PATCHv4 0/4] thermal: exynos: Add support for TRIMINFO feature of Exynos3250 Chanwoo Choi
@ 2014-08-25  7:30 ` Chanwoo Choi
  2014-08-25  7:30 ` [PATCHv4 2/4] thermal: exynos: Add support for many TRIMINFO_CTRL registers Chanwoo Choi
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2014-08-25  7:30 UTC (permalink / raw)
  To: eduardo.valentin, amit.daniel, rui.zhang
  Cc: kgene.kim, robh+dt, pawel.moll, mark.rutland, galak, ch.naveen,
	kyungmin.park, linux-pm, devicetree, linux-arm-kernel,
	linux-samsung-soc, linux-kernel, Chanwoo Choi, Eduardo Valentin

This patch fix wrong value of TRIMINFO_RELOAD_SHIFT. TRIMINFO_CONTROL register
has 'RELOAD' field at [0] bit.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <cw00.choi@samsung.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu_data.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
index f0979e5..401bab7 100644
--- a/drivers/thermal/samsung/exynos_tmu_data.h
+++ b/drivers/thermal/samsung/exynos_tmu_data.h
@@ -63,7 +63,7 @@
 #define EXYNOS_THD_TEMP_FALL		0x54
 #define EXYNOS_EMUL_CON		0x80
 
-#define EXYNOS_TRIMINFO_RELOAD_SHIFT	1
+#define EXYNOS_TRIMINFO_RELOAD_SHIFT	0
 #define EXYNOS_TRIMINFO_25_SHIFT	0
 #define EXYNOS_TRIMINFO_85_SHIFT	8
 #define EXYNOS_TMU_RISE_INT_MASK	0x111
-- 
1.8.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv4 2/4] thermal: exynos: Add support for many TRIMINFO_CTRL registers
  2014-08-25  7:30 [PATCHv4 0/4] thermal: exynos: Add support for TRIMINFO feature of Exynos3250 Chanwoo Choi
  2014-08-25  7:30 ` [PATCHv4 1/4] thermal: exynos: Fix wrong value of TRIMINFO_RELOAD_SHIFT Chanwoo Choi
@ 2014-08-25  7:30 ` Chanwoo Choi
  2014-08-25 10:15   ` Bartlomiej Zolnierkiewicz
  2014-08-25  7:30 ` [PATCHv4 3/4] thermal: exynos: Add support for TRIM_RELOAD feature at Exynos3250 Chanwoo Choi
  2014-08-25  7:30 ` [PATCHv4 4/4] thermal: exynos: Remove duplicate code when reading triminfo register of Exynos5440 Chanwoo Choi
  3 siblings, 1 reply; 10+ messages in thread
From: Chanwoo Choi @ 2014-08-25  7:30 UTC (permalink / raw)
  To: eduardo.valentin, amit.daniel, rui.zhang
  Cc: kgene.kim, robh+dt, pawel.moll, mark.rutland, galak, ch.naveen,
	kyungmin.park, linux-pm, devicetree, linux-arm-kernel,
	linux-samsung-soc, linux-kernel, Chanwoo Choi, Eduardo Valentin

This patch support many TRIMINFO_CTRL registers if specific Exynos SoC
has one more TRIMINFO_CTRL registers. Also this patch uses proper 'RELOAD'
shift/mask bit operation to set RELOAD feature instead of static value.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
---
 drivers/thermal/samsung/exynos_thermal_common.h |  1 +
 drivers/thermal/samsung/exynos_tmu.c            | 23 ++++++++++++++++++++---
 drivers/thermal/samsung/exynos_tmu.h            |  9 +++++++--
 drivers/thermal/samsung/exynos_tmu_data.c       |  5 ++++-
 drivers/thermal/samsung/exynos_tmu_data.h       |  3 +++
 5 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_thermal_common.h b/drivers/thermal/samsung/exynos_thermal_common.h
index 3eb2ed9..b211976 100644
--- a/drivers/thermal/samsung/exynos_thermal_common.h
+++ b/drivers/thermal/samsung/exynos_thermal_common.h
@@ -28,6 +28,7 @@
 #define MAX_TRIP_COUNT	8
 #define MAX_COOLING_DEVICE 4
 #define MAX_THRESHOLD_LEVS 5
+#define MAX_TRIMINFO_CTRL_REG	2
 
 #define ACTIVE_INTERVAL 500
 #define IDLE_INTERVAL 10000
diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index acbff14..7234f38 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -147,7 +147,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
 	struct exynos_tmu_platform_data *pdata = data->pdata;
 	const struct exynos_tmu_registers *reg = pdata->registers;
-	unsigned int status, trim_info = 0, con;
+	unsigned int status, trim_info = 0, con, ctrl;
 	unsigned int rising_threshold = 0, falling_threshold = 0;
 	int ret = 0, threshold_code, i, trigger_levs = 0;
 
@@ -164,8 +164,25 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 		}
 	}
 
-	if (TMU_SUPPORTS(pdata, TRIM_RELOAD))
-		__raw_writel(1, data->base + reg->triminfo_ctrl);
+	if (TMU_SUPPORTS(pdata, TRIM_RELOAD)) {
+		if (reg->triminfo_ctrl_count > MAX_TRIMINFO_CTRL_REG) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		for (i = 0; i < reg->triminfo_ctrl_count; i++) {
+			if (pdata->triminfo_reload[i]) {
+				ctrl = readl(data->base +
+						reg->triminfo_ctrl[i]);
+				ctrl &= ~(reg->triminfo_reload_mask <<
+						reg->triminfo_reload_shift);
+				ctrl |= pdata->triminfo_reload[i] <<
+						reg->triminfo_reload_shift;
+				__raw_writel(ctrl, data->base +
+						reg->triminfo_ctrl[i]);
+			}
+		}
+	}
 
 	if (pdata->cal_mode == HW_MODE)
 		goto skip_calib_data;
diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
index 1b4a644..d7674ad 100644
--- a/drivers/thermal/samsung/exynos_tmu.h
+++ b/drivers/thermal/samsung/exynos_tmu.h
@@ -85,8 +85,11 @@ enum soc_type {
  * @triminfo_25_shift: shift bit of the 25 C trim value in triminfo_data reg.
  * @triminfo_85_shift: shift bit of the 85 C trim value in triminfo_data reg.
  * @triminfo_ctrl: trim info controller register.
+ * @triminfo_ctrl_count: the number of trim info controller register.
  * @triminfo_reload_shift: shift of triminfo reload enable bit in triminfo_ctrl
 	reg.
+ * @triminfo_reload_mask: mask of triminfo reload enable bit in triminfo_ctrl
+	reg.
  * @tmu_ctrl: TMU main controller register.
  * @test_mux_addr_shift: shift bits of test mux address.
  * @buf_vref_sel_shift: shift bits of reference voltage in tmu_ctrl register.
@@ -151,9 +154,10 @@ struct exynos_tmu_registers {
 	u32	triminfo_25_shift;
 	u32	triminfo_85_shift;
 
-	u32	triminfo_ctrl;
-	u32	triminfo_ctrl1;
+	u32	triminfo_ctrl[MAX_TRIMINFO_CTRL_REG];
+	u32	triminfo_ctrl_count;
 	u32	triminfo_reload_shift;
+	u32	triminfo_reload_mask;
 
 	u32	tmu_ctrl;
 	u32     test_mux_addr_shift;
@@ -295,6 +299,7 @@ struct exynos_tmu_platform_data {
 	u8 second_point_trim;
 	u8 default_temp_offset;
 	u8 test_mux;
+	u8 triminfo_reload[MAX_TRIMINFO_CTRL_REG];
 
 	enum calibration_type cal_type;
 	enum calibration_mode cal_mode;
diff --git a/drivers/thermal/samsung/exynos_tmu_data.c b/drivers/thermal/samsung/exynos_tmu_data.c
index aa8e0de..754c638 100644
--- a/drivers/thermal/samsung/exynos_tmu_data.c
+++ b/drivers/thermal/samsung/exynos_tmu_data.c
@@ -184,8 +184,10 @@ static const struct exynos_tmu_registers exynos4412_tmu_registers = {
 	.triminfo_data = EXYNOS_TMU_REG_TRIMINFO,
 	.triminfo_25_shift = EXYNOS_TRIMINFO_25_SHIFT,
 	.triminfo_85_shift = EXYNOS_TRIMINFO_85_SHIFT,
-	.triminfo_ctrl = EXYNOS_TMU_TRIMINFO_CON,
+	.triminfo_ctrl[0] = EXYNOS_TMU_TRIMINFO_CON,
+	.triminfo_ctrl_count = 1,
 	.triminfo_reload_shift = EXYNOS_TRIMINFO_RELOAD_SHIFT,
+	.triminfo_reload_mask = EXYNOS_TRIMINFO_RELOAD_MASK,
 	.tmu_ctrl = EXYNOS_TMU_REG_CONTROL,
 	.test_mux_addr_shift = EXYNOS4412_MUX_ADDR_SHIFT,
 	.buf_vref_sel_shift = EXYNOS_TMU_REF_VOLTAGE_SHIFT,
@@ -252,6 +254,7 @@ static const struct exynos_tmu_registers exynos4412_tmu_registers = {
 		.temp_level = 95, \
 	}, \
 	.freq_tab_count = 2, \
+	.triminfo_reload[0] = EXYNOS_TRIMINFO_RELOAD_ENABLE, \
 	.registers = &exynos4412_tmu_registers, \
 	.features = (TMU_SUPPORT_EMULATION | TMU_SUPPORT_TRIM_RELOAD | \
 			TMU_SUPPORT_FALLING_TRIP | TMU_SUPPORT_READY_STATUS | \
diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
index 401bab7..87454f63 100644
--- a/drivers/thermal/samsung/exynos_tmu_data.h
+++ b/drivers/thermal/samsung/exynos_tmu_data.h
@@ -64,6 +64,9 @@
 #define EXYNOS_EMUL_CON		0x80
 
 #define EXYNOS_TRIMINFO_RELOAD_SHIFT	0
+#define EXYNOS_TRIMINFO_RELOAD_MASK	0x1
+#define EXYNOS_TRIMINFO_RELOAD_ENABLE	1
+#define EXYNOS_TRIMINFO_RELOAD_DISABLE	0
 #define EXYNOS_TRIMINFO_25_SHIFT	0
 #define EXYNOS_TRIMINFO_85_SHIFT	8
 #define EXYNOS_TMU_RISE_INT_MASK	0x111
-- 
1.8.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv4 3/4] thermal: exynos: Add support for TRIM_RELOAD feature at Exynos3250
  2014-08-25  7:30 [PATCHv4 0/4] thermal: exynos: Add support for TRIMINFO feature of Exynos3250 Chanwoo Choi
  2014-08-25  7:30 ` [PATCHv4 1/4] thermal: exynos: Fix wrong value of TRIMINFO_RELOAD_SHIFT Chanwoo Choi
  2014-08-25  7:30 ` [PATCHv4 2/4] thermal: exynos: Add support for many TRIMINFO_CTRL registers Chanwoo Choi
@ 2014-08-25  7:30 ` Chanwoo Choi
  2014-08-25  7:30 ` [PATCHv4 4/4] thermal: exynos: Remove duplicate code when reading triminfo register of Exynos5440 Chanwoo Choi
  3 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2014-08-25  7:30 UTC (permalink / raw)
  To: eduardo.valentin, amit.daniel, rui.zhang
  Cc: kgene.kim, robh+dt, pawel.moll, mark.rutland, galak, ch.naveen,
	kyungmin.park, linux-pm, devicetree, linux-arm-kernel,
	linux-samsung-soc, linux-kernel, Chanwoo Choi, Eduardo Valentin

This patch add support for TRIM_RELOAD feature at Exynos3250. The TMu of
Exynos3250 has two TRIMINFO_CON register and must need to set RELOAD bit
before reading TRIMINFO register.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu_data.c | 11 +++++++++--
 drivers/thermal/samsung/exynos_tmu_data.h |  7 +++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu_data.c b/drivers/thermal/samsung/exynos_tmu_data.c
index 754c638..4006eb4 100644
--- a/drivers/thermal/samsung/exynos_tmu_data.c
+++ b/drivers/thermal/samsung/exynos_tmu_data.c
@@ -95,6 +95,11 @@ static const struct exynos_tmu_registers exynos3250_tmu_registers = {
 	.triminfo_data = EXYNOS_TMU_REG_TRIMINFO,
 	.triminfo_25_shift = EXYNOS_TRIMINFO_25_SHIFT,
 	.triminfo_85_shift = EXYNOS_TRIMINFO_85_SHIFT,
+	.triminfo_ctrl[0] = EXYNOS_TMU_TRIMINFO_CON1,
+	.triminfo_ctrl[1] = EXYNOS_TMU_TRIMINFO_CON2,
+	.triminfo_ctrl_count = 2,
+	.triminfo_reload_shift = EXYNOS_TRIMINFO_RELOAD_SHIFT,
+	.triminfo_reload_mask = EXYNOS_TRIMINFO_RELOAD_MASK,
 	.tmu_ctrl = EXYNOS_TMU_REG_CONTROL,
 	.test_mux_addr_shift = EXYNOS4412_MUX_ADDR_SHIFT,
 	.buf_vref_sel_shift = EXYNOS_TMU_REF_VOLTAGE_SHIFT,
@@ -161,7 +166,9 @@ static const struct exynos_tmu_registers exynos3250_tmu_registers = {
 	}, \
 	.freq_tab_count = 2, \
 	.registers = &exynos3250_tmu_registers, \
-	.features = (TMU_SUPPORT_EMULATION | \
+	.triminfo_reload[0] = EXYNOS_TRIMINFO_RELOAD_ENABLE, \
+	.triminfo_reload[1] = EXYNOS_TRIMINFO_RELOAD_ENABLE, \
+	.features = (TMU_SUPPORT_EMULATION | TMU_SUPPORT_TRIM_RELOAD | \
 			TMU_SUPPORT_FALLING_TRIP | TMU_SUPPORT_READY_STATUS | \
 			TMU_SUPPORT_EMUL_TIME)
 #endif
@@ -184,7 +191,7 @@ static const struct exynos_tmu_registers exynos4412_tmu_registers = {
 	.triminfo_data = EXYNOS_TMU_REG_TRIMINFO,
 	.triminfo_25_shift = EXYNOS_TRIMINFO_25_SHIFT,
 	.triminfo_85_shift = EXYNOS_TRIMINFO_85_SHIFT,
-	.triminfo_ctrl[0] = EXYNOS_TMU_TRIMINFO_CON,
+	.triminfo_ctrl[0] = EXYNOS_TMU_TRIMINFO_CON2,
 	.triminfo_ctrl_count = 1,
 	.triminfo_reload_shift = EXYNOS_TRIMINFO_RELOAD_SHIFT,
 	.triminfo_reload_mask = EXYNOS_TRIMINFO_RELOAD_MASK,
diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
index 87454f63..f62fdde 100644
--- a/drivers/thermal/samsung/exynos_tmu_data.h
+++ b/drivers/thermal/samsung/exynos_tmu_data.h
@@ -57,8 +57,11 @@
 #define EXYNOS4210_TMU_TRIG_LEVEL_MASK	0x1111
 #define EXYNOS4210_TMU_INTCLEAR_VAL	0x1111
 
-/* Exynos5250 and Exynos4412 specific registers */
-#define EXYNOS_TMU_TRIMINFO_CON	0x14
+/*  Exynos3250 specific registers */
+#define EXYNOS_TMU_TRIMINFO_CON1	0x10
+
+/* Exynos5250 ,Exynos4412 and Exynos3250 specific registers */
+#define EXYNOS_TMU_TRIMINFO_CON2	0x14
 #define EXYNOS_THD_TEMP_RISE		0x50
 #define EXYNOS_THD_TEMP_FALL		0x54
 #define EXYNOS_EMUL_CON		0x80
-- 
1.8.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv4 4/4] thermal: exynos: Remove duplicate code when reading triminfo register of Exynos5440
  2014-08-25  7:30 [PATCHv4 0/4] thermal: exynos: Add support for TRIMINFO feature of Exynos3250 Chanwoo Choi
                   ` (2 preceding siblings ...)
  2014-08-25  7:30 ` [PATCHv4 3/4] thermal: exynos: Add support for TRIM_RELOAD feature at Exynos3250 Chanwoo Choi
@ 2014-08-25  7:30 ` Chanwoo Choi
  3 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2014-08-25  7:30 UTC (permalink / raw)
  To: eduardo.valentin, amit.daniel, rui.zhang
  Cc: kgene.kim, robh+dt, pawel.moll, mark.rutland, galak, ch.naveen,
	kyungmin.park, linux-pm, devicetree, linux-arm-kernel,
	linux-samsung-soc, linux-kernel, Chanwoo Choi, Eduardo Valentin

This patch remove simply duplicate code when reading triminfo register of Exynos5440.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
---
 drivers/thermal/samsung/exynos_tmu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 7234f38..caeb445 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -195,15 +195,13 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
 		 */
 		switch (data->id) {
 		case 0:
+		case 2:
 			trim_info = readl(data->base +
 			EXYNOS5440_EFUSE_SWAP_OFFSET + reg->triminfo_data);
 			break;
 		case 1:
 			trim_info = readl(data->base + reg->triminfo_data);
 			break;
-		case 2:
-			trim_info = readl(data->base -
-			EXYNOS5440_EFUSE_SWAP_OFFSET + reg->triminfo_data);
 		}
 	} else {
 		/* On exynos5420 the triminfo register is in the shared space */
-- 
1.8.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCHv4 2/4] thermal: exynos: Add support for many TRIMINFO_CTRL registers
  2014-08-25  7:30 ` [PATCHv4 2/4] thermal: exynos: Add support for many TRIMINFO_CTRL registers Chanwoo Choi
@ 2014-08-25 10:15   ` Bartlomiej Zolnierkiewicz
  2014-08-25 10:37     ` Chanwoo Choi
  0 siblings, 1 reply; 10+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2014-08-25 10:15 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Chanwoo Choi, eduardo.valentin, amit.daniel, rui.zhang,
	mark.rutland, devicetree, kgene.kim, pawel.moll, linux-pm,
	linux-kernel, Eduardo Valentin, linux-samsung-soc, kyungmin.park,
	robh+dt, galak, ch.naveen


Hi,

On Monday, August 25, 2014 04:30:23 PM Chanwoo Choi wrote:
> This patch support many TRIMINFO_CTRL registers if specific Exynos SoC
> has one more TRIMINFO_CTRL registers. Also this patch uses proper 'RELOAD'
> shift/mask bit operation to set RELOAD feature instead of static value.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> ---
>  drivers/thermal/samsung/exynos_thermal_common.h |  1 +
>  drivers/thermal/samsung/exynos_tmu.c            | 23 ++++++++++++++++++++---
>  drivers/thermal/samsung/exynos_tmu.h            |  9 +++++++--
>  drivers/thermal/samsung/exynos_tmu_data.c       |  5 ++++-
>  drivers/thermal/samsung/exynos_tmu_data.h       |  3 +++
>  5 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_thermal_common.h b/drivers/thermal/samsung/exynos_thermal_common.h
> index 3eb2ed9..b211976 100644
> --- a/drivers/thermal/samsung/exynos_thermal_common.h
> +++ b/drivers/thermal/samsung/exynos_thermal_common.h
> @@ -28,6 +28,7 @@
>  #define MAX_TRIP_COUNT	8
>  #define MAX_COOLING_DEVICE 4
>  #define MAX_THRESHOLD_LEVS 5
> +#define MAX_TRIMINFO_CTRL_REG	2
>  
>  #define ACTIVE_INTERVAL 500
>  #define IDLE_INTERVAL 10000
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index acbff14..7234f38 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -147,7 +147,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>  	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>  	struct exynos_tmu_platform_data *pdata = data->pdata;
>  	const struct exynos_tmu_registers *reg = pdata->registers;
> -	unsigned int status, trim_info = 0, con;
> +	unsigned int status, trim_info = 0, con, ctrl;
>  	unsigned int rising_threshold = 0, falling_threshold = 0;
>  	int ret = 0, threshold_code, i, trigger_levs = 0;
>  
> @@ -164,8 +164,25 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>  		}
>  	}
>  
> -	if (TMU_SUPPORTS(pdata, TRIM_RELOAD))
> -		__raw_writel(1, data->base + reg->triminfo_ctrl);
> +	if (TMU_SUPPORTS(pdata, TRIM_RELOAD)) {
> +		if (reg->triminfo_ctrl_count > MAX_TRIMINFO_CTRL_REG) {

Please remove this check and MAX_TRIMINFO_CTRL_REG define.

We do not want such runtime checks for development time errors.

> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		for (i = 0; i < reg->triminfo_ctrl_count; i++) {
> +			if (pdata->triminfo_reload[i]) {
> +				ctrl = readl(data->base +
> +						reg->triminfo_ctrl[i]);
> +				ctrl &= ~(reg->triminfo_reload_mask <<
> +						reg->triminfo_reload_shift);
> +				ctrl |= pdata->triminfo_reload[i] <<
> +						reg->triminfo_reload_shift;

triminfo_reload_shift and triminfo_reload_mask variables have always
the same values when this code is run so there is no need for them.

They should be added only when needed instead of doing it now.

> +				__raw_writel(ctrl, data->base +
> +						reg->triminfo_ctrl[i]);
> +			}
> +		}
> +	}
>  
>  	if (pdata->cal_mode == HW_MODE)
>  		goto skip_calib_data;
> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
> index 1b4a644..d7674ad 100644
> --- a/drivers/thermal/samsung/exynos_tmu.h
> +++ b/drivers/thermal/samsung/exynos_tmu.h
> @@ -85,8 +85,11 @@ enum soc_type {
>   * @triminfo_25_shift: shift bit of the 25 C trim value in triminfo_data reg.
>   * @triminfo_85_shift: shift bit of the 85 C trim value in triminfo_data reg.
>   * @triminfo_ctrl: trim info controller register.
> + * @triminfo_ctrl_count: the number of trim info controller register.
>   * @triminfo_reload_shift: shift of triminfo reload enable bit in triminfo_ctrl
>  	reg.
> + * @triminfo_reload_mask: mask of triminfo reload enable bit in triminfo_ctrl
> +	reg.
>   * @tmu_ctrl: TMU main controller register.
>   * @test_mux_addr_shift: shift bits of test mux address.
>   * @buf_vref_sel_shift: shift bits of reference voltage in tmu_ctrl register.
> @@ -151,9 +154,10 @@ struct exynos_tmu_registers {
>  	u32	triminfo_25_shift;
>  	u32	triminfo_85_shift;
>  
> -	u32	triminfo_ctrl;
> -	u32	triminfo_ctrl1;
> +	u32	triminfo_ctrl[MAX_TRIMINFO_CTRL_REG];
> +	u32	triminfo_ctrl_count;
>  	u32	triminfo_reload_shift;
> +	u32	triminfo_reload_mask;
>  
>  	u32	tmu_ctrl;
>  	u32     test_mux_addr_shift;
> @@ -295,6 +299,7 @@ struct exynos_tmu_platform_data {
>  	u8 second_point_trim;
>  	u8 default_temp_offset;
>  	u8 test_mux;
> +	u8 triminfo_reload[MAX_TRIMINFO_CTRL_REG];
>  
>  	enum calibration_type cal_type;
>  	enum calibration_mode cal_mode;
> diff --git a/drivers/thermal/samsung/exynos_tmu_data.c b/drivers/thermal/samsung/exynos_tmu_data.c
> index aa8e0de..754c638 100644
> --- a/drivers/thermal/samsung/exynos_tmu_data.c
> +++ b/drivers/thermal/samsung/exynos_tmu_data.c
> @@ -184,8 +184,10 @@ static const struct exynos_tmu_registers exynos4412_tmu_registers = {
>  	.triminfo_data = EXYNOS_TMU_REG_TRIMINFO,
>  	.triminfo_25_shift = EXYNOS_TRIMINFO_25_SHIFT,
>  	.triminfo_85_shift = EXYNOS_TRIMINFO_85_SHIFT,
> -	.triminfo_ctrl = EXYNOS_TMU_TRIMINFO_CON,
> +	.triminfo_ctrl[0] = EXYNOS_TMU_TRIMINFO_CON,
> +	.triminfo_ctrl_count = 1,
>  	.triminfo_reload_shift = EXYNOS_TRIMINFO_RELOAD_SHIFT,
> +	.triminfo_reload_mask = EXYNOS_TRIMINFO_RELOAD_MASK,
>  	.tmu_ctrl = EXYNOS_TMU_REG_CONTROL,
>  	.test_mux_addr_shift = EXYNOS4412_MUX_ADDR_SHIFT,
>  	.buf_vref_sel_shift = EXYNOS_TMU_REF_VOLTAGE_SHIFT,
> @@ -252,6 +254,7 @@ static const struct exynos_tmu_registers exynos4412_tmu_registers = {
>  		.temp_level = 95, \
>  	}, \
>  	.freq_tab_count = 2, \
> +	.triminfo_reload[0] = EXYNOS_TRIMINFO_RELOAD_ENABLE, \
>  	.registers = &exynos4412_tmu_registers, \
>  	.features = (TMU_SUPPORT_EMULATION | TMU_SUPPORT_TRIM_RELOAD | \
>  			TMU_SUPPORT_FALLING_TRIP | TMU_SUPPORT_READY_STATUS | \
> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
> index 401bab7..87454f63 100644
> --- a/drivers/thermal/samsung/exynos_tmu_data.h
> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
> @@ -64,6 +64,9 @@
>  #define EXYNOS_EMUL_CON		0x80
>  
>  #define EXYNOS_TRIMINFO_RELOAD_SHIFT	0
> +#define EXYNOS_TRIMINFO_RELOAD_MASK	0x1
> +#define EXYNOS_TRIMINFO_RELOAD_ENABLE	1
> +#define EXYNOS_TRIMINFO_RELOAD_DISABLE	0

This define is never used please remove it.

>  #define EXYNOS_TRIMINFO_25_SHIFT	0
>  #define EXYNOS_TRIMINFO_85_SHIFT	8
>  #define EXYNOS_TMU_RISE_INT_MASK	0x111

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv4 2/4] thermal: exynos: Add support for many TRIMINFO_CTRL registers
  2014-08-25 10:15   ` Bartlomiej Zolnierkiewicz
@ 2014-08-25 10:37     ` Chanwoo Choi
  2014-08-25 11:19       ` Bartlomiej Zolnierkiewicz
  0 siblings, 1 reply; 10+ messages in thread
From: Chanwoo Choi @ 2014-08-25 10:37 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: linux-arm-kernel, eduardo.valentin, amit.daniel, rui.zhang,
	mark.rutland, devicetree, kgene.kim, pawel.moll, linux-pm,
	linux-kernel, Eduardo Valentin, linux-samsung-soc, kyungmin.park,
	robh+dt, galak, ch.naveen

Hi Bartlomiej,

On 08/25/2014 07:15 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Monday, August 25, 2014 04:30:23 PM Chanwoo Choi wrote:
>> This patch support many TRIMINFO_CTRL registers if specific Exynos SoC
>> has one more TRIMINFO_CTRL registers. Also this patch uses proper 'RELOAD'
>> shift/mask bit operation to set RELOAD feature instead of static value.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: Eduardo Valentin <edubezval@gmail.com>
>> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
>> Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>> ---
>>  drivers/thermal/samsung/exynos_thermal_common.h |  1 +
>>  drivers/thermal/samsung/exynos_tmu.c            | 23 ++++++++++++++++++++---
>>  drivers/thermal/samsung/exynos_tmu.h            |  9 +++++++--
>>  drivers/thermal/samsung/exynos_tmu_data.c       |  5 ++++-
>>  drivers/thermal/samsung/exynos_tmu_data.h       |  3 +++
>>  5 files changed, 35 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/thermal/samsung/exynos_thermal_common.h b/drivers/thermal/samsung/exynos_thermal_common.h
>> index 3eb2ed9..b211976 100644
>> --- a/drivers/thermal/samsung/exynos_thermal_common.h
>> +++ b/drivers/thermal/samsung/exynos_thermal_common.h
>> @@ -28,6 +28,7 @@
>>  #define MAX_TRIP_COUNT	8
>>  #define MAX_COOLING_DEVICE 4
>>  #define MAX_THRESHOLD_LEVS 5
>> +#define MAX_TRIMINFO_CTRL_REG	2
>>  
>>  #define ACTIVE_INTERVAL 500
>>  #define IDLE_INTERVAL 10000
>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>> index acbff14..7234f38 100644
>> --- a/drivers/thermal/samsung/exynos_tmu.c
>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>> @@ -147,7 +147,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>  	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>>  	struct exynos_tmu_platform_data *pdata = data->pdata;
>>  	const struct exynos_tmu_registers *reg = pdata->registers;
>> -	unsigned int status, trim_info = 0, con;
>> +	unsigned int status, trim_info = 0, con, ctrl;
>>  	unsigned int rising_threshold = 0, falling_threshold = 0;
>>  	int ret = 0, threshold_code, i, trigger_levs = 0;
>>  
>> @@ -164,8 +164,25 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>  		}
>>  	}
>>  
>> -	if (TMU_SUPPORTS(pdata, TRIM_RELOAD))
>> -		__raw_writel(1, data->base + reg->triminfo_ctrl);
>> +	if (TMU_SUPPORTS(pdata, TRIM_RELOAD)) {
>> +		if (reg->triminfo_ctrl_count > MAX_TRIMINFO_CTRL_REG) {
> 
> Please remove this check and MAX_TRIMINFO_CTRL_REG define.
> 
> We do not want such runtime checks for development time errors.

OK, I'll remove it.

> 
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		for (i = 0; i < reg->triminfo_ctrl_count; i++) {
>> +			if (pdata->triminfo_reload[i]) {
>> +				ctrl = readl(data->base +
>> +						reg->triminfo_ctrl[i]);
>> +				ctrl &= ~(reg->triminfo_reload_mask <<
>> +						reg->triminfo_reload_shift);
>> +				ctrl |= pdata->triminfo_reload[i] <<
>> +						reg->triminfo_reload_shift;
> 
> triminfo_reload_shift and triminfo_reload_mask variables have always
> the same values when this code is run so there is no need for them.

I don't understand. Do you mean that timinfo_reload_{shift/mask} variable is un-needed?
If you possible, I need more detailed comment.

> 
> They should be added only when needed instead of doing it now.
> 
>> +				__raw_writel(ctrl, data->base +
>> +						reg->triminfo_ctrl[i]);
>> +			}
>> +		}
>> +	}
>>  
>>  	if (pdata->cal_mode == HW_MODE)
>>  		goto skip_calib_data;
>> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
>> index 1b4a644..d7674ad 100644
>> --- a/drivers/thermal/samsung/exynos_tmu.h
>> +++ b/drivers/thermal/samsung/exynos_tmu.h
>> @@ -85,8 +85,11 @@ enum soc_type {
>>   * @triminfo_25_shift: shift bit of the 25 C trim value in triminfo_data reg.
>>   * @triminfo_85_shift: shift bit of the 85 C trim value in triminfo_data reg.
>>   * @triminfo_ctrl: trim info controller register.
>> + * @triminfo_ctrl_count: the number of trim info controller register.
>>   * @triminfo_reload_shift: shift of triminfo reload enable bit in triminfo_ctrl
>>  	reg.
>> + * @triminfo_reload_mask: mask of triminfo reload enable bit in triminfo_ctrl
>> +	reg.
>>   * @tmu_ctrl: TMU main controller register.
>>   * @test_mux_addr_shift: shift bits of test mux address.
>>   * @buf_vref_sel_shift: shift bits of reference voltage in tmu_ctrl register.
>> @@ -151,9 +154,10 @@ struct exynos_tmu_registers {
>>  	u32	triminfo_25_shift;
>>  	u32	triminfo_85_shift;
>>  
>> -	u32	triminfo_ctrl;
>> -	u32	triminfo_ctrl1;
>> +	u32	triminfo_ctrl[MAX_TRIMINFO_CTRL_REG];
>> +	u32	triminfo_ctrl_count;
>>  	u32	triminfo_reload_shift;
>> +	u32	triminfo_reload_mask;
>>  
>>  	u32	tmu_ctrl;
>>  	u32     test_mux_addr_shift;
>> @@ -295,6 +299,7 @@ struct exynos_tmu_platform_data {
>>  	u8 second_point_trim;
>>  	u8 default_temp_offset;
>>  	u8 test_mux;
>> +	u8 triminfo_reload[MAX_TRIMINFO_CTRL_REG];
>>  
>>  	enum calibration_type cal_type;
>>  	enum calibration_mode cal_mode;
>> diff --git a/drivers/thermal/samsung/exynos_tmu_data.c b/drivers/thermal/samsung/exynos_tmu_data.c
>> index aa8e0de..754c638 100644
>> --- a/drivers/thermal/samsung/exynos_tmu_data.c
>> +++ b/drivers/thermal/samsung/exynos_tmu_data.c
>> @@ -184,8 +184,10 @@ static const struct exynos_tmu_registers exynos4412_tmu_registers = {
>>  	.triminfo_data = EXYNOS_TMU_REG_TRIMINFO,
>>  	.triminfo_25_shift = EXYNOS_TRIMINFO_25_SHIFT,
>>  	.triminfo_85_shift = EXYNOS_TRIMINFO_85_SHIFT,
>> -	.triminfo_ctrl = EXYNOS_TMU_TRIMINFO_CON,
>> +	.triminfo_ctrl[0] = EXYNOS_TMU_TRIMINFO_CON,
>> +	.triminfo_ctrl_count = 1,
>>  	.triminfo_reload_shift = EXYNOS_TRIMINFO_RELOAD_SHIFT,
>> +	.triminfo_reload_mask = EXYNOS_TRIMINFO_RELOAD_MASK,
>>  	.tmu_ctrl = EXYNOS_TMU_REG_CONTROL,
>>  	.test_mux_addr_shift = EXYNOS4412_MUX_ADDR_SHIFT,
>>  	.buf_vref_sel_shift = EXYNOS_TMU_REF_VOLTAGE_SHIFT,
>> @@ -252,6 +254,7 @@ static const struct exynos_tmu_registers exynos4412_tmu_registers = {
>>  		.temp_level = 95, \
>>  	}, \
>>  	.freq_tab_count = 2, \
>> +	.triminfo_reload[0] = EXYNOS_TRIMINFO_RELOAD_ENABLE, \
>>  	.registers = &exynos4412_tmu_registers, \
>>  	.features = (TMU_SUPPORT_EMULATION | TMU_SUPPORT_TRIM_RELOAD | \
>>  			TMU_SUPPORT_FALLING_TRIP | TMU_SUPPORT_READY_STATUS | \
>> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
>> index 401bab7..87454f63 100644
>> --- a/drivers/thermal/samsung/exynos_tmu_data.h
>> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
>> @@ -64,6 +64,9 @@
>>  #define EXYNOS_EMUL_CON		0x80
>>  
>>  #define EXYNOS_TRIMINFO_RELOAD_SHIFT	0
>> +#define EXYNOS_TRIMINFO_RELOAD_MASK	0x1
>> +#define EXYNOS_TRIMINFO_RELOAD_ENABLE	1
>> +#define EXYNOS_TRIMINFO_RELOAD_DISABLE	0
> 
> This define is never used please remove it.

OK, I'll remove it. Thanks.

Beset Regards,
Chanwoo CHoi

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv4 2/4] thermal: exynos: Add support for many TRIMINFO_CTRL registers
  2014-08-25 10:37     ` Chanwoo Choi
@ 2014-08-25 11:19       ` Bartlomiej Zolnierkiewicz
  2014-08-25 11:28         ` Bartlomiej Zolnierkiewicz
  2014-08-25 11:49         ` Chanwoo Choi
  0 siblings, 2 replies; 10+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2014-08-25 11:19 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: linux-arm-kernel, eduardo.valentin, amit.daniel, rui.zhang,
	mark.rutland, devicetree, kgene.kim, pawel.moll, linux-pm,
	linux-kernel, Eduardo Valentin, linux-samsung-soc, kyungmin.park,
	robh+dt, galak, ch.naveen

On Monday, August 25, 2014 07:37:25 PM Chanwoo Choi wrote:
> Hi Bartlomiej,
> 
> On 08/25/2014 07:15 PM, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Monday, August 25, 2014 04:30:23 PM Chanwoo Choi wrote:
> >> This patch support many TRIMINFO_CTRL registers if specific Exynos SoC
> >> has one more TRIMINFO_CTRL registers. Also this patch uses proper 'RELOAD'
> >> shift/mask bit operation to set RELOAD feature instead of static value.
> >>
> >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> >> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> Cc: Zhang Rui <rui.zhang@intel.com>
> >> Cc: Eduardo Valentin <edubezval@gmail.com>
> >> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> >> Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> >> ---
> >>  drivers/thermal/samsung/exynos_thermal_common.h |  1 +
> >>  drivers/thermal/samsung/exynos_tmu.c            | 23 ++++++++++++++++++++---
> >>  drivers/thermal/samsung/exynos_tmu.h            |  9 +++++++--
> >>  drivers/thermal/samsung/exynos_tmu_data.c       |  5 ++++-
> >>  drivers/thermal/samsung/exynos_tmu_data.h       |  3 +++
> >>  5 files changed, 35 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/thermal/samsung/exynos_thermal_common.h b/drivers/thermal/samsung/exynos_thermal_common.h
> >> index 3eb2ed9..b211976 100644
> >> --- a/drivers/thermal/samsung/exynos_thermal_common.h
> >> +++ b/drivers/thermal/samsung/exynos_thermal_common.h
> >> @@ -28,6 +28,7 @@
> >>  #define MAX_TRIP_COUNT	8
> >>  #define MAX_COOLING_DEVICE 4
> >>  #define MAX_THRESHOLD_LEVS 5
> >> +#define MAX_TRIMINFO_CTRL_REG	2
> >>  
> >>  #define ACTIVE_INTERVAL 500
> >>  #define IDLE_INTERVAL 10000
> >> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> >> index acbff14..7234f38 100644
> >> --- a/drivers/thermal/samsung/exynos_tmu.c
> >> +++ b/drivers/thermal/samsung/exynos_tmu.c
> >> @@ -147,7 +147,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> >>  	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> >>  	struct exynos_tmu_platform_data *pdata = data->pdata;
> >>  	const struct exynos_tmu_registers *reg = pdata->registers;
> >> -	unsigned int status, trim_info = 0, con;
> >> +	unsigned int status, trim_info = 0, con, ctrl;
> >>  	unsigned int rising_threshold = 0, falling_threshold = 0;
> >>  	int ret = 0, threshold_code, i, trigger_levs = 0;
> >>  
> >> @@ -164,8 +164,25 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> >>  		}
> >>  	}
> >>  
> >> -	if (TMU_SUPPORTS(pdata, TRIM_RELOAD))
> >> -		__raw_writel(1, data->base + reg->triminfo_ctrl);
> >> +	if (TMU_SUPPORTS(pdata, TRIM_RELOAD)) {
> >> +		if (reg->triminfo_ctrl_count > MAX_TRIMINFO_CTRL_REG) {
> > 
> > Please remove this check and MAX_TRIMINFO_CTRL_REG define.
> > 
> > We do not want such runtime checks for development time errors.
> 
> OK, I'll remove it.
> 
> > 
> >> +			ret = -EINVAL;
> >> +			goto out;
> >> +		}
> >> +
> >> +		for (i = 0; i < reg->triminfo_ctrl_count; i++) {
> >> +			if (pdata->triminfo_reload[i]) {
> >> +				ctrl = readl(data->base +
> >> +						reg->triminfo_ctrl[i]);
> >> +				ctrl &= ~(reg->triminfo_reload_mask <<
> >> +						reg->triminfo_reload_shift);
> >> +				ctrl |= pdata->triminfo_reload[i] <<
> >> +						reg->triminfo_reload_shift;
> > 
> > triminfo_reload_shift and triminfo_reload_mask variables have always
> > the same values when this code is run so there is no need for them.
> 
> I don't understand. Do you mean that timinfo_reload_{shift/mask} variable is un-needed?

Yes.

> If you possible, I need more detailed comment.

Currently triminfo_reload_shift is always "0" and triminfo_reload_mask
is "1" so there is no need to add an abstraction for different SoCs
(it should be added only when there is a real need for it).

Please just rewrite this code as:

			if (pdata->triminfo_reload[i]) {
				ctrl = readl(data->base +
						reg->triminfo_ctrl[i]);
				ctrl |= pdata->triminfo_reload[i];
				__raw_writel(ctrl, data->base +
						reg->triminfo_ctrl[i]);
			}

Then you can remove unused triminfo_reload_shift and
EXYNOS_TRIMINFO_RELOAD_SHIFT.

Please also include my patch (https://lkml.org/lkml/2014/8/20/481) in
your patch series (or at least mark it in the cover letter that my
patch should be merged before your patch #2/4).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> > They should be added only when needed instead of doing it now.
> > 
> >> +				__raw_writel(ctrl, data->base +
> >> +						reg->triminfo_ctrl[i]);
> >> +			}
> >> +		}
> >> +	}
> >>  
> >>  	if (pdata->cal_mode == HW_MODE)
> >>  		goto skip_calib_data;
> >> diff --git a/drivers/thermal/samsung/exynos_tmu.h b/drivers/thermal/samsung/exynos_tmu.h
> >> index 1b4a644..d7674ad 100644
> >> --- a/drivers/thermal/samsung/exynos_tmu.h
> >> +++ b/drivers/thermal/samsung/exynos_tmu.h
> >> @@ -85,8 +85,11 @@ enum soc_type {
> >>   * @triminfo_25_shift: shift bit of the 25 C trim value in triminfo_data reg.
> >>   * @triminfo_85_shift: shift bit of the 85 C trim value in triminfo_data reg.
> >>   * @triminfo_ctrl: trim info controller register.
> >> + * @triminfo_ctrl_count: the number of trim info controller register.
> >>   * @triminfo_reload_shift: shift of triminfo reload enable bit in triminfo_ctrl
> >>  	reg.
> >> + * @triminfo_reload_mask: mask of triminfo reload enable bit in triminfo_ctrl
> >> +	reg.
> >>   * @tmu_ctrl: TMU main controller register.
> >>   * @test_mux_addr_shift: shift bits of test mux address.
> >>   * @buf_vref_sel_shift: shift bits of reference voltage in tmu_ctrl register.
> >> @@ -151,9 +154,10 @@ struct exynos_tmu_registers {
> >>  	u32	triminfo_25_shift;
> >>  	u32	triminfo_85_shift;
> >>  
> >> -	u32	triminfo_ctrl;
> >> -	u32	triminfo_ctrl1;
> >> +	u32	triminfo_ctrl[MAX_TRIMINFO_CTRL_REG];
> >> +	u32	triminfo_ctrl_count;
> >>  	u32	triminfo_reload_shift;
> >> +	u32	triminfo_reload_mask;
> >>  
> >>  	u32	tmu_ctrl;
> >>  	u32     test_mux_addr_shift;
> >> @@ -295,6 +299,7 @@ struct exynos_tmu_platform_data {
> >>  	u8 second_point_trim;
> >>  	u8 default_temp_offset;
> >>  	u8 test_mux;
> >> +	u8 triminfo_reload[MAX_TRIMINFO_CTRL_REG];
> >>  
> >>  	enum calibration_type cal_type;
> >>  	enum calibration_mode cal_mode;
> >> diff --git a/drivers/thermal/samsung/exynos_tmu_data.c b/drivers/thermal/samsung/exynos_tmu_data.c
> >> index aa8e0de..754c638 100644
> >> --- a/drivers/thermal/samsung/exynos_tmu_data.c
> >> +++ b/drivers/thermal/samsung/exynos_tmu_data.c
> >> @@ -184,8 +184,10 @@ static const struct exynos_tmu_registers exynos4412_tmu_registers = {
> >>  	.triminfo_data = EXYNOS_TMU_REG_TRIMINFO,
> >>  	.triminfo_25_shift = EXYNOS_TRIMINFO_25_SHIFT,
> >>  	.triminfo_85_shift = EXYNOS_TRIMINFO_85_SHIFT,
> >> -	.triminfo_ctrl = EXYNOS_TMU_TRIMINFO_CON,
> >> +	.triminfo_ctrl[0] = EXYNOS_TMU_TRIMINFO_CON,
> >> +	.triminfo_ctrl_count = 1,
> >>  	.triminfo_reload_shift = EXYNOS_TRIMINFO_RELOAD_SHIFT,
> >> +	.triminfo_reload_mask = EXYNOS_TRIMINFO_RELOAD_MASK,
> >>  	.tmu_ctrl = EXYNOS_TMU_REG_CONTROL,
> >>  	.test_mux_addr_shift = EXYNOS4412_MUX_ADDR_SHIFT,
> >>  	.buf_vref_sel_shift = EXYNOS_TMU_REF_VOLTAGE_SHIFT,
> >> @@ -252,6 +254,7 @@ static const struct exynos_tmu_registers exynos4412_tmu_registers = {
> >>  		.temp_level = 95, \
> >>  	}, \
> >>  	.freq_tab_count = 2, \
> >> +	.triminfo_reload[0] = EXYNOS_TRIMINFO_RELOAD_ENABLE, \
> >>  	.registers = &exynos4412_tmu_registers, \
> >>  	.features = (TMU_SUPPORT_EMULATION | TMU_SUPPORT_TRIM_RELOAD | \
> >>  			TMU_SUPPORT_FALLING_TRIP | TMU_SUPPORT_READY_STATUS | \
> >> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h b/drivers/thermal/samsung/exynos_tmu_data.h
> >> index 401bab7..87454f63 100644
> >> --- a/drivers/thermal/samsung/exynos_tmu_data.h
> >> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
> >> @@ -64,6 +64,9 @@
> >>  #define EXYNOS_EMUL_CON		0x80
> >>  
> >>  #define EXYNOS_TRIMINFO_RELOAD_SHIFT	0
> >> +#define EXYNOS_TRIMINFO_RELOAD_MASK	0x1
> >> +#define EXYNOS_TRIMINFO_RELOAD_ENABLE	1
> >> +#define EXYNOS_TRIMINFO_RELOAD_DISABLE	0
> > 
> > This define is never used please remove it.
> 
> OK, I'll remove it. Thanks.
> 
> Beset Regards,
> Chanwoo CHoi


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv4 2/4] thermal: exynos: Add support for many TRIMINFO_CTRL registers
  2014-08-25 11:19       ` Bartlomiej Zolnierkiewicz
@ 2014-08-25 11:28         ` Bartlomiej Zolnierkiewicz
  2014-08-25 11:49         ` Chanwoo Choi
  1 sibling, 0 replies; 10+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2014-08-25 11:28 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: linux-arm-kernel, eduardo.valentin, amit.daniel, rui.zhang,
	mark.rutland, devicetree, kgene.kim, pawel.moll, linux-pm,
	linux-kernel, Eduardo Valentin, linux-samsung-soc, kyungmin.park,
	robh+dt, galak, ch.naveen

On Monday, August 25, 2014 01:19:04 PM Bartlomiej Zolnierkiewicz wrote:
> On Monday, August 25, 2014 07:37:25 PM Chanwoo Choi wrote:
> > Hi Bartlomiej,
> > 
> > On 08/25/2014 07:15 PM, Bartlomiej Zolnierkiewicz wrote:
> > > 
> > > Hi,
> > > 
> > > On Monday, August 25, 2014 04:30:23 PM Chanwoo Choi wrote:
> > >> This patch support many TRIMINFO_CTRL registers if specific Exynos SoC
> > >> has one more TRIMINFO_CTRL registers. Also this patch uses proper 'RELOAD'
> > >> shift/mask bit operation to set RELOAD feature instead of static value.
> > >>
> > >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> > >> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> > >> Cc: Zhang Rui <rui.zhang@intel.com>
> > >> Cc: Eduardo Valentin <edubezval@gmail.com>
> > >> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
> > >> Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> > >> ---
> > >>  drivers/thermal/samsung/exynos_thermal_common.h |  1 +
> > >>  drivers/thermal/samsung/exynos_tmu.c            | 23 ++++++++++++++++++++---
> > >>  drivers/thermal/samsung/exynos_tmu.h            |  9 +++++++--
> > >>  drivers/thermal/samsung/exynos_tmu_data.c       |  5 ++++-
> > >>  drivers/thermal/samsung/exynos_tmu_data.h       |  3 +++
> > >>  5 files changed, 35 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/drivers/thermal/samsung/exynos_thermal_common.h b/drivers/thermal/samsung/exynos_thermal_common.h
> > >> index 3eb2ed9..b211976 100644
> > >> --- a/drivers/thermal/samsung/exynos_thermal_common.h
> > >> +++ b/drivers/thermal/samsung/exynos_thermal_common.h
> > >> @@ -28,6 +28,7 @@
> > >>  #define MAX_TRIP_COUNT	8
> > >>  #define MAX_COOLING_DEVICE 4
> > >>  #define MAX_THRESHOLD_LEVS 5
> > >> +#define MAX_TRIMINFO_CTRL_REG	2
> > >>  
> > >>  #define ACTIVE_INTERVAL 500
> > >>  #define IDLE_INTERVAL 10000
> > >> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> > >> index acbff14..7234f38 100644
> > >> --- a/drivers/thermal/samsung/exynos_tmu.c
> > >> +++ b/drivers/thermal/samsung/exynos_tmu.c
> > >> @@ -147,7 +147,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> > >>  	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> > >>  	struct exynos_tmu_platform_data *pdata = data->pdata;
> > >>  	const struct exynos_tmu_registers *reg = pdata->registers;
> > >> -	unsigned int status, trim_info = 0, con;
> > >> +	unsigned int status, trim_info = 0, con, ctrl;
> > >>  	unsigned int rising_threshold = 0, falling_threshold = 0;
> > >>  	int ret = 0, threshold_code, i, trigger_levs = 0;
> > >>  
> > >> @@ -164,8 +164,25 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
> > >>  		}
> > >>  	}
> > >>  
> > >> -	if (TMU_SUPPORTS(pdata, TRIM_RELOAD))
> > >> -		__raw_writel(1, data->base + reg->triminfo_ctrl);
> > >> +	if (TMU_SUPPORTS(pdata, TRIM_RELOAD)) {
> > >> +		if (reg->triminfo_ctrl_count > MAX_TRIMINFO_CTRL_REG) {
> > > 
> > > Please remove this check and MAX_TRIMINFO_CTRL_REG define.
> > > 
> > > We do not want such runtime checks for development time errors.
> > 
> > OK, I'll remove it.
> > 
> > > 
> > >> +			ret = -EINVAL;
> > >> +			goto out;
> > >> +		}
> > >> +
> > >> +		for (i = 0; i < reg->triminfo_ctrl_count; i++) {
> > >> +			if (pdata->triminfo_reload[i]) {
> > >> +				ctrl = readl(data->base +
> > >> +						reg->triminfo_ctrl[i]);
> > >> +				ctrl &= ~(reg->triminfo_reload_mask <<
> > >> +						reg->triminfo_reload_shift);
> > >> +				ctrl |= pdata->triminfo_reload[i] <<
> > >> +						reg->triminfo_reload_shift;
> > > 
> > > triminfo_reload_shift and triminfo_reload_mask variables have always
> > > the same values when this code is run so there is no need for them.
> > 
> > I don't understand. Do you mean that timinfo_reload_{shift/mask} variable is un-needed?
> 
> Yes.
> 
> > If you possible, I need more detailed comment.
> 
> Currently triminfo_reload_shift is always "0" and triminfo_reload_mask
> is "1" so there is no need to add an abstraction for different SoCs
> (it should be added only when there is a real need for it).
> 
> Please just rewrite this code as:
> 
> 			if (pdata->triminfo_reload[i]) {
> 				ctrl = readl(data->base +
> 						reg->triminfo_ctrl[i]);
> 				ctrl |= pdata->triminfo_reload[i];
> 				__raw_writel(ctrl, data->base +
> 						reg->triminfo_ctrl[i]);
> 			}

or even better lets get rid of the only __raw_writel() call in Exynos
thermal driver while at it:

			if (pdata->triminfo_reload[i]) {
				ctrl = readl(data->base +
						reg->triminfo_ctrl[i]);
				ctrl |= pdata->triminfo_reload[i];
				writel(ctrl, data->base +
						reg->triminfo_ctrl[i]);
			}

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv4 2/4] thermal: exynos: Add support for many TRIMINFO_CTRL registers
  2014-08-25 11:19       ` Bartlomiej Zolnierkiewicz
  2014-08-25 11:28         ` Bartlomiej Zolnierkiewicz
@ 2014-08-25 11:49         ` Chanwoo Choi
  1 sibling, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2014-08-25 11:49 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: linux-arm-kernel, eduardo.valentin, amit.daniel, rui.zhang,
	mark.rutland, devicetree, kgene.kim, pawel.moll, linux-pm,
	linux-kernel, Eduardo Valentin, linux-samsung-soc, kyungmin.park,
	robh+dt, galak, ch.naveen

On 08/25/2014 08:19 PM, Bartlomiej Zolnierkiewicz wrote:
> On Monday, August 25, 2014 07:37:25 PM Chanwoo Choi wrote:
>> Hi Bartlomiej,
>>
>> On 08/25/2014 07:15 PM, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> Hi,
>>>
>>> On Monday, August 25, 2014 04:30:23 PM Chanwoo Choi wrote:
>>>> This patch support many TRIMINFO_CTRL registers if specific Exynos SoC
>>>> has one more TRIMINFO_CTRL registers. Also this patch uses proper 'RELOAD'
>>>> shift/mask bit operation to set RELOAD feature instead of static value.
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> Cc: Zhang Rui <rui.zhang@intel.com>
>>>> Cc: Eduardo Valentin <edubezval@gmail.com>
>>>> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>> Reviewed-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>> ---
>>>>  drivers/thermal/samsung/exynos_thermal_common.h |  1 +
>>>>  drivers/thermal/samsung/exynos_tmu.c            | 23 ++++++++++++++++++++---
>>>>  drivers/thermal/samsung/exynos_tmu.h            |  9 +++++++--
>>>>  drivers/thermal/samsung/exynos_tmu_data.c       |  5 ++++-
>>>>  drivers/thermal/samsung/exynos_tmu_data.h       |  3 +++
>>>>  5 files changed, 35 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/samsung/exynos_thermal_common.h b/drivers/thermal/samsung/exynos_thermal_common.h
>>>> index 3eb2ed9..b211976 100644
>>>> --- a/drivers/thermal/samsung/exynos_thermal_common.h
>>>> +++ b/drivers/thermal/samsung/exynos_thermal_common.h
>>>> @@ -28,6 +28,7 @@
>>>>  #define MAX_TRIP_COUNT	8
>>>>  #define MAX_COOLING_DEVICE 4
>>>>  #define MAX_THRESHOLD_LEVS 5
>>>> +#define MAX_TRIMINFO_CTRL_REG	2
>>>>  
>>>>  #define ACTIVE_INTERVAL 500
>>>>  #define IDLE_INTERVAL 10000
>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>>> index acbff14..7234f38 100644
>>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>>> @@ -147,7 +147,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>  	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>>>>  	struct exynos_tmu_platform_data *pdata = data->pdata;
>>>>  	const struct exynos_tmu_registers *reg = pdata->registers;
>>>> -	unsigned int status, trim_info = 0, con;
>>>> +	unsigned int status, trim_info = 0, con, ctrl;
>>>>  	unsigned int rising_threshold = 0, falling_threshold = 0;
>>>>  	int ret = 0, threshold_code, i, trigger_levs = 0;
>>>>  
>>>> @@ -164,8 +164,25 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>>  		}
>>>>  	}
>>>>  
>>>> -	if (TMU_SUPPORTS(pdata, TRIM_RELOAD))
>>>> -		__raw_writel(1, data->base + reg->triminfo_ctrl);
>>>> +	if (TMU_SUPPORTS(pdata, TRIM_RELOAD)) {
>>>> +		if (reg->triminfo_ctrl_count > MAX_TRIMINFO_CTRL_REG) {
>>>
>>> Please remove this check and MAX_TRIMINFO_CTRL_REG define.
>>>
>>> We do not want such runtime checks for development time errors.
>>
>> OK, I'll remove it.
>>
>>>
>>>> +			ret = -EINVAL;
>>>> +			goto out;
>>>> +		}
>>>> +
>>>> +		for (i = 0; i < reg->triminfo_ctrl_count; i++) {
>>>> +			if (pdata->triminfo_reload[i]) {
>>>> +				ctrl = readl(data->base +
>>>> +						reg->triminfo_ctrl[i]);
>>>> +				ctrl &= ~(reg->triminfo_reload_mask <<
>>>> +						reg->triminfo_reload_shift);
>>>> +				ctrl |= pdata->triminfo_reload[i] <<
>>>> +						reg->triminfo_reload_shift;
>>>
>>> triminfo_reload_shift and triminfo_reload_mask variables have always
>>> the same values when this code is run so there is no need for them.
>>
>> I don't understand. Do you mean that timinfo_reload_{shift/mask} variable is un-needed?
> 
> Yes.
> 
>> If you possible, I need more detailed comment.
> 
> Currently triminfo_reload_shift is always "0" and triminfo_reload_mask
> is "1" so there is no need to add an abstraction for different SoCs
> (it should be added only when there is a real need for it).
> 
> Please just rewrite this code as:
> 
> 			if (pdata->triminfo_reload[i]) {
> 				ctrl = readl(data->base +
> 						reg->triminfo_ctrl[i]);
> 				ctrl |= pdata->triminfo_reload[i];
> 				__raw_writel(ctrl, data->base +
> 						reg->triminfo_ctrl[i]);
> 			}
> 
> Then you can remove unused triminfo_reload_shift and
> EXYNOS_TRIMINFO_RELOAD_SHIFT.
> 
> Please also include my patch (https://lkml.org/lkml/2014/8/20/481) in
> your patch series (or at least mark it in the cover letter that my
> patch should be merged before your patch #2/4).

OK. thanks for your comment.

Best Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2014-08-25 11:49 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-25  7:30 [PATCHv4 0/4] thermal: exynos: Add support for TRIMINFO feature of Exynos3250 Chanwoo Choi
2014-08-25  7:30 ` [PATCHv4 1/4] thermal: exynos: Fix wrong value of TRIMINFO_RELOAD_SHIFT Chanwoo Choi
2014-08-25  7:30 ` [PATCHv4 2/4] thermal: exynos: Add support for many TRIMINFO_CTRL registers Chanwoo Choi
2014-08-25 10:15   ` Bartlomiej Zolnierkiewicz
2014-08-25 10:37     ` Chanwoo Choi
2014-08-25 11:19       ` Bartlomiej Zolnierkiewicz
2014-08-25 11:28         ` Bartlomiej Zolnierkiewicz
2014-08-25 11:49         ` Chanwoo Choi
2014-08-25  7:30 ` [PATCHv4 3/4] thermal: exynos: Add support for TRIM_RELOAD feature at Exynos3250 Chanwoo Choi
2014-08-25  7:30 ` [PATCHv4 4/4] thermal: exynos: Remove duplicate code when reading triminfo register of Exynos5440 Chanwoo Choi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).