All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH/RFT] thermal: rcar_gen3_thermal: Update calculation formula due to HW evaluation
@ 2019-03-21 20:47 Yoshihiro Kaneko
  2019-04-01 13:37 ` Simon Horman
  0 siblings, 1 reply; 6+ messages in thread
From: Yoshihiro Kaneko @ 2019-03-21 20:47 UTC (permalink / raw)
  To: linux-pm
  Cc: Zhang Rui, Eduardo Valentin, Rob Herring, Simon Horman,
	Magnus Damm, linux-renesas-soc

From: Dien Pham <dien.pham.ry@renesas.com>

1/ As evaluation of hardware team, temperature calculation formula
 of M3-W is difference from all other SoCs as below:
 - M3-W: Tj_1: 116 (so Tj_1 - Tj_3 = 157)
 - Others: Tj_1: 126 (so Tj_1 - Tj_3 = 167)

2/ Update the formula to calculate CTEMP:
  Currently, the CTEMP is average of val1 (is calculated by
  formula 1) and val2 (is calculated by formula 2). But,
  as description in HWM (chapter 10A.3.1.1 Setting of Normal Mode)

  If (STEMP < Tj_T) CTEMP value should be val1.
  If (STEMP > Tj_T) CTEMP value should be val2.

3/ Update the formula to calculate temperature:
  Currently, current TEMP is calculated as
  average of val1 (is calculated by formula 1)
  and val2 (is calculated by formula 2). But,
  as description in HWM (chapter 10A.3.1.2 Normal Mode.)

  If (TEMP_CODE < THCODE2[11:0]) CTEMP value should be val1.
  If (TEMP_CODE > THCODE2[11:0]) CTEMP value should be val2.

Signed-off-by: Dien Pham <dien.pham.ry@renesas.com>
[ykaneko0929@gmail.com: use the data field of the of_device_id for Tj_1]
[ykaneko0929@gmail.com: revise a description of case 1 of the commit log]
Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
---
 drivers/thermal/rcar_gen3_thermal.c | 86 +++++++++++++++++++++++++------------
 1 file changed, 58 insertions(+), 28 deletions(-)

This patch is based on the master branch of Linus Torvalds's linux tree.

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 88fa41c..de6f244 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -63,6 +63,15 @@
 
 #define TSC_MAX_NUM	3
 
+static int tj_2;
+
+/* default THCODE values if FUSEs are missing */
+static int thcode[TSC_MAX_NUM][3] = {
+	{ 3397, 2800, 2221 },
+	{ 3393, 2795, 2216 },
+	{ 3389, 2805, 2237 },
+};
+
 /* Structure for thermal temperature calculation */
 struct equation_coefs {
 	int a1;
@@ -77,6 +86,7 @@ struct rcar_gen3_thermal_tsc {
 	struct equation_coefs coef;
 	int low;
 	int high;
+	int id; /* thermal channel id */
 };
 
 struct rcar_gen3_thermal_priv {
@@ -124,30 +134,29 @@ static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
 #define RCAR3_THERMAL_GRAN 500 /* mili Celsius */
 
 /* no idea where these constants come from */
-#define TJ_1 116
-#define TJ_3 -41
+#define TJ_2 (INT_FIXPT(tj_2))	/* Tj_T */
+#define TJ_3 -41		/* Tj_L */
 
 static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
-					 int *ptat, int *thcode)
+					 int *ptat, int *thcode,
+					 unsigned int ths_tj_1)
 {
-	int tj_2;
-
 	/* TODO: Find documentation and document constant calculation formula */
 
 	/*
 	 * Division is not scaled in BSP and if scaled it might overflow
 	 * the dividend (4095 * 4095 << 14 > INT_MAX) so keep it unscaled
 	 */
-	tj_2 = (FIXPT_INT((ptat[1] - ptat[2]) * 157)
-		/ (ptat[0] - ptat[2])) - FIXPT_INT(41);
+	tj_2 = (FIXPT_INT((ptat[1] - ptat[2]) * (ths_tj_1 - TJ_3))
+		/ (ptat[0] - ptat[2])) + FIXPT_INT(TJ_3);
 
 	coef->a1 = FIXPT_DIV(FIXPT_INT(thcode[1] - thcode[2]),
 			     tj_2 - FIXPT_INT(TJ_3));
 	coef->b1 = FIXPT_INT(thcode[2]) - coef->a1 * TJ_3;
 
 	coef->a2 = FIXPT_DIV(FIXPT_INT(thcode[1] - thcode[0]),
-			     tj_2 - FIXPT_INT(TJ_1));
-	coef->b2 = FIXPT_INT(thcode[0]) - coef->a2 * TJ_1;
+			     tj_2 - FIXPT_INT(ths_tj_1));
+	coef->b2 = FIXPT_INT(thcode[0]) - coef->a2 * ths_tj_1;
 }
 
 static int rcar_gen3_thermal_round(int temp)
@@ -163,15 +172,19 @@ static int rcar_gen3_thermal_round(int temp)
 static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
 {
 	struct rcar_gen3_thermal_tsc *tsc = devdata;
-	int mcelsius, val1, val2;
+	int mcelsius, val;
 	u32 reg;
 
 	/* Read register and convert to mili Celsius */
 	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
 
-	val1 = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b1, tsc->coef.a1);
-	val2 = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b2, tsc->coef.a2);
-	mcelsius = FIXPT_TO_MCELSIUS((val1 + val2) / 2);
+	if (reg <= thcode[tsc->id][1])
+		val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b1,
+				tsc->coef.a1);
+	else
+		val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b2,
+				tsc->coef.a2);
+	mcelsius = FIXPT_TO_MCELSIUS(val);
 
 	/* Make sure we are inside specifications */
 	if ((mcelsius < MCELSIUS(-40)) || (mcelsius > MCELSIUS(125)))
@@ -186,13 +199,15 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
 static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc,
 					      int mcelsius)
 {
-	int celsius, val1, val2;
+	int celsius, val;
 
 	celsius = DIV_ROUND_CLOSEST(mcelsius, 1000);
-	val1 = celsius * tsc->coef.a1 + tsc->coef.b1;
-	val2 = celsius * tsc->coef.a2 + tsc->coef.b2;
+	if (celsius <= TJ_2)
+		val = celsius * tsc->coef.a1 + tsc->coef.b1;
+	else
+		val = celsius * tsc->coef.a2 + tsc->coef.b2;
 
-	return INT_FIXPT((val1 + val2) / 2);
+	return INT_FIXPT(val);
 }
 
 static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
@@ -318,12 +333,29 @@ static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 	usleep_range(1000, 2000);
 }
 
+static const unsigned int rcar_gen3_ths_tj_1 = 126;
+static const unsigned int rcar_gen3_ths_tj_1_m3_w = 116;
 static const struct of_device_id rcar_gen3_thermal_dt_ids[] = {
-	{ .compatible = "renesas,r8a774a1-thermal", },
-	{ .compatible = "renesas,r8a7795-thermal", },
-	{ .compatible = "renesas,r8a7796-thermal", },
-	{ .compatible = "renesas,r8a77965-thermal", },
-	{ .compatible = "renesas,r8a77980-thermal", },
+	{
+		.compatible = "renesas,r8a774a1-thermal",
+		.data = &rcar_gen3_ths_tj_1_m3_w,
+	},
+	{
+		.compatible = "renesas,r8a7795-thermal",
+		.data = &rcar_gen3_ths_tj_1,
+	},
+	{
+		.compatible = "renesas,r8a7796-thermal",
+		.data = &rcar_gen3_ths_tj_1_m3_w,
+	},
+	{
+		.compatible = "renesas,r8a77965-thermal",
+		.data = &rcar_gen3_ths_tj_1,
+	},
+	{
+		.compatible = "renesas,r8a77980-thermal",
+		.data = &rcar_gen3_ths_tj_1,
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, rcar_gen3_thermal_dt_ids);
@@ -349,6 +381,7 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 {
 	struct rcar_gen3_thermal_priv *priv;
 	struct device *dev = &pdev->dev;
+	const unsigned int *rcar_gen3_ths_tj_1 = of_device_get_match_data(dev);
 	struct resource *res;
 	struct thermal_zone_device *zone;
 	int ret, irq, i;
@@ -357,11 +390,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	/* default values if FUSEs are missing */
 	/* TODO: Read values from hardware on supported platforms */
 	int ptat[3] = { 2631, 1509, 435 };
-	int thcode[TSC_MAX_NUM][3] = {
-		{ 3397, 2800, 2221 },
-		{ 3393, 2795, 2216 },
-		{ 3389, 2805, 2237 },
-	};
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -418,11 +446,13 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 			ret = PTR_ERR(tsc->base);
 			goto error_unregister;
 		}
+		tsc->id = i;
 
 		priv->tscs[i] = tsc;
 
 		priv->thermal_init(tsc);
-		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i]);
+		rcar_gen3_thermal_calc_coefs(&tsc->coef, ptat, thcode[i],
+					     *rcar_gen3_ths_tj_1);
 
 		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
 							    &rcar_gen3_tz_of_ops);
-- 
1.9.1


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

* Re: [PATCH/RFT] thermal: rcar_gen3_thermal: Update calculation formula due to HW evaluation
  2019-03-21 20:47 [PATCH/RFT] thermal: rcar_gen3_thermal: Update calculation formula due to HW evaluation Yoshihiro Kaneko
@ 2019-04-01 13:37 ` Simon Horman
  2019-04-03  9:22   ` Simon Horman
  2019-04-11 16:50   ` Niklas Söderlund
  0 siblings, 2 replies; 6+ messages in thread
From: Simon Horman @ 2019-04-01 13:37 UTC (permalink / raw)
  To: Yoshihiro Kaneko
  Cc: linux-pm, Zhang Rui, Eduardo Valentin, Rob Herring, Magnus Damm,
	linux-renesas-soc

On Fri, Mar 22, 2019 at 05:47:53AM +0900, Yoshihiro Kaneko wrote:
> From: Dien Pham <dien.pham.ry@renesas.com>
> 
> 1/ As evaluation of hardware team, temperature calculation formula
>  of M3-W is difference from all other SoCs as below:
>  - M3-W: Tj_1: 116 (so Tj_1 - Tj_3 = 157)
>  - Others: Tj_1: 126 (so Tj_1 - Tj_3 = 167)
> 
> 2/ Update the formula to calculate CTEMP:
>   Currently, the CTEMP is average of val1 (is calculated by
>   formula 1) and val2 (is calculated by formula 2). But,
>   as description in HWM (chapter 10A.3.1.1 Setting of Normal Mode)
> 
>   If (STEMP < Tj_T) CTEMP value should be val1.
>   If (STEMP > Tj_T) CTEMP value should be val2.
> 
> 3/ Update the formula to calculate temperature:
>   Currently, current TEMP is calculated as
>   average of val1 (is calculated by formula 1)
>   and val2 (is calculated by formula 2). But,
>   as description in HWM (chapter 10A.3.1.2 Normal Mode.)
> 
>   If (TEMP_CODE < THCODE2[11:0]) CTEMP value should be val1.
>   If (TEMP_CODE > THCODE2[11:0]) CTEMP value should be val2.
> 
> Signed-off-by: Dien Pham <dien.pham.ry@renesas.com>
> [ykaneko0929@gmail.com: use the data field of the of_device_id for Tj_1]
> [ykaneko0929@gmail.com: revise a description of case 1 of the commit log]
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>

I'm wondering if we could split this up into three patches,
one for each problem it solves? Partly because I think it is good to fix
one problem per patch. And partly because am having trouble
verifying this patch - the new if statement in rcar_gen3_thermal_get_temp()
seems to result in 0 temperature readings when the else case is used.

> ---
>  drivers/thermal/rcar_gen3_thermal.c | 86 +++++++++++++++++++++++++------------
>  1 file changed, 58 insertions(+), 28 deletions(-)
> 
> This patch is based on the master branch of Linus Torvalds's linux tree.
> 
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 88fa41c..de6f244 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -63,6 +63,15 @@
>  
>  #define TSC_MAX_NUM	3
>  
> +static int tj_2;

We need to avoid global variables. There can be multiple
instances of this driver. (Although they will all get the same
value for tj_2.

Perhaps it can be added to struct rcar_gen3_thermal_tsc?

...

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

* Re: [PATCH/RFT] thermal: rcar_gen3_thermal: Update calculation formula due to HW evaluation
  2019-04-01 13:37 ` Simon Horman
@ 2019-04-03  9:22   ` Simon Horman
  2019-04-16 17:56     ` Yoshihiro Kaneko
  2019-04-11 16:50   ` Niklas Söderlund
  1 sibling, 1 reply; 6+ messages in thread
From: Simon Horman @ 2019-04-03  9:22 UTC (permalink / raw)
  To: Yoshihiro Kaneko
  Cc: linux-pm, Zhang Rui, Eduardo Valentin, Rob Herring, Magnus Damm,
	linux-renesas-soc

On Mon, Apr 01, 2019 at 03:37:57PM +0200, Simon Horman wrote:
> On Fri, Mar 22, 2019 at 05:47:53AM +0900, Yoshihiro Kaneko wrote:
> > From: Dien Pham <dien.pham.ry@renesas.com>
> > 
> > 1/ As evaluation of hardware team, temperature calculation formula
> >  of M3-W is difference from all other SoCs as below:
> >  - M3-W: Tj_1: 116 (so Tj_1 - Tj_3 = 157)
> >  - Others: Tj_1: 126 (so Tj_1 - Tj_3 = 167)
> > 
> > 2/ Update the formula to calculate CTEMP:
> >   Currently, the CTEMP is average of val1 (is calculated by
> >   formula 1) and val2 (is calculated by formula 2). But,
> >   as description in HWM (chapter 10A.3.1.1 Setting of Normal Mode)
> > 
> >   If (STEMP < Tj_T) CTEMP value should be val1.
> >   If (STEMP > Tj_T) CTEMP value should be val2.
> > 
> > 3/ Update the formula to calculate temperature:
> >   Currently, current TEMP is calculated as
> >   average of val1 (is calculated by formula 1)
> >   and val2 (is calculated by formula 2). But,
> >   as description in HWM (chapter 10A.3.1.2 Normal Mode.)
> > 
> >   If (TEMP_CODE < THCODE2[11:0]) CTEMP value should be val1.
> >   If (TEMP_CODE > THCODE2[11:0]) CTEMP value should be val2.
> > 
> > Signed-off-by: Dien Pham <dien.pham.ry@renesas.com>
> > [ykaneko0929@gmail.com: use the data field of the of_device_id for Tj_1]
> > [ykaneko0929@gmail.com: revise a description of case 1 of the commit log]
> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> 
> I'm wondering if we could split this up into three patches,
> one for each problem it solves? Partly because I think it is good to fix
> one problem per patch. And partly because am having trouble
> verifying this patch - the new if statement in rcar_gen3_thermal_get_temp()
> seems to result in 0 temperature readings when the else case is used.
> 
> > ---
> >  drivers/thermal/rcar_gen3_thermal.c | 86 +++++++++++++++++++++++++------------
> >  1 file changed, 58 insertions(+), 28 deletions(-)
> > 
> > This patch is based on the master branch of Linus Torvalds's linux tree.
> > 
> > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > index 88fa41c..de6f244 100644
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > @@ -63,6 +63,15 @@
> >  
> >  #define TSC_MAX_NUM	3
> >  
> > +static int tj_2;
> 
> We need to avoid global variables. There can be multiple
> instances of this driver. (Although they will all get the same
> value for tj_2.
> 
> Perhaps it can be added to struct rcar_gen3_thermal_tsc?
> 
> ...

Also, from just a little further down the patch

> > +/* default THCODE values if FUSEs are missing */
> > +static int thcode[TSC_MAX_NUM][3] = {
> > +       { 3397, 2800, 2221 },
> > +       { 3393, 2795, 2216 },
> > +       { 3389, 2805, 2237 },
> > +};

I think thcode can be marked const.

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

* Re: [PATCH/RFT] thermal: rcar_gen3_thermal: Update calculation formula due to HW evaluation
  2019-04-01 13:37 ` Simon Horman
  2019-04-03  9:22   ` Simon Horman
@ 2019-04-11 16:50   ` Niklas Söderlund
  2019-04-16 17:58     ` Yoshihiro Kaneko
  1 sibling, 1 reply; 6+ messages in thread
From: Niklas Söderlund @ 2019-04-11 16:50 UTC (permalink / raw)
  To: Simon Horman
  Cc: Yoshihiro Kaneko, linux-pm, Zhang Rui, Eduardo Valentin,
	Rob Herring, Magnus Damm, linux-renesas-soc

Hi Kaneko-san,

Thanks for your work.

On 2019-04-01 15:37:57 +0200, Simon Horman wrote:
> On Fri, Mar 22, 2019 at 05:47:53AM +0900, Yoshihiro Kaneko wrote:
> > From: Dien Pham <dien.pham.ry@renesas.com>
> > 
> > 1/ As evaluation of hardware team, temperature calculation formula
> >  of M3-W is difference from all other SoCs as below:
> >  - M3-W: Tj_1: 116 (so Tj_1 - Tj_3 = 157)
> >  - Others: Tj_1: 126 (so Tj_1 - Tj_3 = 167)
> > 
> > 2/ Update the formula to calculate CTEMP:
> >   Currently, the CTEMP is average of val1 (is calculated by
> >   formula 1) and val2 (is calculated by formula 2). But,
> >   as description in HWM (chapter 10A.3.1.1 Setting of Normal Mode)
> > 
> >   If (STEMP < Tj_T) CTEMP value should be val1.
> >   If (STEMP > Tj_T) CTEMP value should be val2.
> > 
> > 3/ Update the formula to calculate temperature:
> >   Currently, current TEMP is calculated as
> >   average of val1 (is calculated by formula 1)
> >   and val2 (is calculated by formula 2). But,
> >   as description in HWM (chapter 10A.3.1.2 Normal Mode.)
> > 
> >   If (TEMP_CODE < THCODE2[11:0]) CTEMP value should be val1.
> >   If (TEMP_CODE > THCODE2[11:0]) CTEMP value should be val2.
> > 
> > Signed-off-by: Dien Pham <dien.pham.ry@renesas.com>
> > [ykaneko0929@gmail.com: use the data field of the of_device_id for Tj_1]
> > [ykaneko0929@gmail.com: revise a description of case 1 of the commit log]
> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> 
> I'm wondering if we could split this up into three patches,
> one for each problem it solves? Partly because I think it is good to fix
> one problem per patch. And partly because am having trouble
> verifying this patch - the new if statement in rcar_gen3_thermal_get_temp()
> seems to result in 0 temperature readings when the else case is used.

I think the changes in this patch are good and correct but I agree with 
Simon here, this patch should be split.

> 
> > ---
> >  drivers/thermal/rcar_gen3_thermal.c | 86 +++++++++++++++++++++++++------------
> >  1 file changed, 58 insertions(+), 28 deletions(-)
> > 
> > This patch is based on the master branch of Linus Torvalds's linux tree.
> > 
> > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > index 88fa41c..de6f244 100644
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > @@ -63,6 +63,15 @@
> >  
> >  #define TSC_MAX_NUM	3
> >  
> > +static int tj_2;
> 
> We need to avoid global variables. There can be multiple
> instances of this driver. (Although they will all get the same
> value for tj_2.
> 
> Perhaps it can be added to struct rcar_gen3_thermal_tsc?
> 
> ...

-- 
Regards,
Niklas Söderlund

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

* Re: [PATCH/RFT] thermal: rcar_gen3_thermal: Update calculation formula due to HW evaluation
  2019-04-03  9:22   ` Simon Horman
@ 2019-04-16 17:56     ` Yoshihiro Kaneko
  0 siblings, 0 replies; 6+ messages in thread
From: Yoshihiro Kaneko @ 2019-04-16 17:56 UTC (permalink / raw)
  To: Simon Horman
  Cc: Linux PM list, Zhang Rui, Eduardo Valentin, Rob Herring,
	Magnus Damm, Linux-Renesas

Hi Simon-san,

Thanks for your review.
I have split this patch and made fixes in v2.

Thanks,
Kaneko

2019年4月3日(水) 18:22 Simon Horman <horms@verge.net.au>:
>
> On Mon, Apr 01, 2019 at 03:37:57PM +0200, Simon Horman wrote:
> > On Fri, Mar 22, 2019 at 05:47:53AM +0900, Yoshihiro Kaneko wrote:
> > > From: Dien Pham <dien.pham.ry@renesas.com>
> > >
> > > 1/ As evaluation of hardware team, temperature calculation formula
> > >  of M3-W is difference from all other SoCs as below:
> > >  - M3-W: Tj_1: 116 (so Tj_1 - Tj_3 = 157)
> > >  - Others: Tj_1: 126 (so Tj_1 - Tj_3 = 167)
> > >
> > > 2/ Update the formula to calculate CTEMP:
> > >   Currently, the CTEMP is average of val1 (is calculated by
> > >   formula 1) and val2 (is calculated by formula 2). But,
> > >   as description in HWM (chapter 10A.3.1.1 Setting of Normal Mode)
> > >
> > >   If (STEMP < Tj_T) CTEMP value should be val1.
> > >   If (STEMP > Tj_T) CTEMP value should be val2.
> > >
> > > 3/ Update the formula to calculate temperature:
> > >   Currently, current TEMP is calculated as
> > >   average of val1 (is calculated by formula 1)
> > >   and val2 (is calculated by formula 2). But,
> > >   as description in HWM (chapter 10A.3.1.2 Normal Mode.)
> > >
> > >   If (TEMP_CODE < THCODE2[11:0]) CTEMP value should be val1.
> > >   If (TEMP_CODE > THCODE2[11:0]) CTEMP value should be val2.
> > >
> > > Signed-off-by: Dien Pham <dien.pham.ry@renesas.com>
> > > [ykaneko0929@gmail.com: use the data field of the of_device_id for Tj_1]
> > > [ykaneko0929@gmail.com: revise a description of case 1 of the commit log]
> > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> >
> > I'm wondering if we could split this up into three patches,
> > one for each problem it solves? Partly because I think it is good to fix
> > one problem per patch. And partly because am having trouble
> > verifying this patch - the new if statement in rcar_gen3_thermal_get_temp()
> > seems to result in 0 temperature readings when the else case is used.
> >
> > > ---
> > >  drivers/thermal/rcar_gen3_thermal.c | 86 +++++++++++++++++++++++++------------
> > >  1 file changed, 58 insertions(+), 28 deletions(-)
> > >
> > > This patch is based on the master branch of Linus Torvalds's linux tree.
> > >
> > > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > > index 88fa41c..de6f244 100644
> > > --- a/drivers/thermal/rcar_gen3_thermal.c
> > > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > > @@ -63,6 +63,15 @@
> > >
> > >  #define TSC_MAX_NUM        3
> > >
> > > +static int tj_2;
> >
> > We need to avoid global variables. There can be multiple
> > instances of this driver. (Although they will all get the same
> > value for tj_2.
> >
> > Perhaps it can be added to struct rcar_gen3_thermal_tsc?
> >
> > ...
>
> Also, from just a little further down the patch
>
> > > +/* default THCODE values if FUSEs are missing */
> > > +static int thcode[TSC_MAX_NUM][3] = {
> > > +       { 3397, 2800, 2221 },
> > > +       { 3393, 2795, 2216 },
> > > +       { 3389, 2805, 2237 },
> > > +};
>
> I think thcode can be marked const.

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

* Re: [PATCH/RFT] thermal: rcar_gen3_thermal: Update calculation formula due to HW evaluation
  2019-04-11 16:50   ` Niklas Söderlund
@ 2019-04-16 17:58     ` Yoshihiro Kaneko
  0 siblings, 0 replies; 6+ messages in thread
From: Yoshihiro Kaneko @ 2019-04-16 17:58 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Simon Horman, Yoshihiro Kaneko, Linux PM list, Zhang Rui,
	Eduardo Valentin, Rob Herring, Magnus Damm, Linux-Renesas

Hi Niklas-san,

Thanks for your review.

2019年4月12日(金) 1:50 Niklas Söderlund <niklas.soderlund@ragnatech.se>:
>
> Hi Kaneko-san,
>
> Thanks for your work.
>
> On 2019-04-01 15:37:57 +0200, Simon Horman wrote:
> > On Fri, Mar 22, 2019 at 05:47:53AM +0900, Yoshihiro Kaneko wrote:
> > > From: Dien Pham <dien.pham.ry@renesas.com>
> > >
> > > 1/ As evaluation of hardware team, temperature calculation formula
> > >  of M3-W is difference from all other SoCs as below:
> > >  - M3-W: Tj_1: 116 (so Tj_1 - Tj_3 = 157)
> > >  - Others: Tj_1: 126 (so Tj_1 - Tj_3 = 167)
> > >
> > > 2/ Update the formula to calculate CTEMP:
> > >   Currently, the CTEMP is average of val1 (is calculated by
> > >   formula 1) and val2 (is calculated by formula 2). But,
> > >   as description in HWM (chapter 10A.3.1.1 Setting of Normal Mode)
> > >
> > >   If (STEMP < Tj_T) CTEMP value should be val1.
> > >   If (STEMP > Tj_T) CTEMP value should be val2.
> > >
> > > 3/ Update the formula to calculate temperature:
> > >   Currently, current TEMP is calculated as
> > >   average of val1 (is calculated by formula 1)
> > >   and val2 (is calculated by formula 2). But,
> > >   as description in HWM (chapter 10A.3.1.2 Normal Mode.)
> > >
> > >   If (TEMP_CODE < THCODE2[11:0]) CTEMP value should be val1.
> > >   If (TEMP_CODE > THCODE2[11:0]) CTEMP value should be val2.
> > >
> > > Signed-off-by: Dien Pham <dien.pham.ry@renesas.com>
> > > [ykaneko0929@gmail.com: use the data field of the of_device_id for Tj_1]
> > > [ykaneko0929@gmail.com: revise a description of case 1 of the commit log]
> > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> >
> > I'm wondering if we could split this up into three patches,
> > one for each problem it solves? Partly because I think it is good to fix
> > one problem per patch. And partly because am having trouble
> > verifying this patch - the new if statement in rcar_gen3_thermal_get_temp()
> > seems to result in 0 temperature readings when the else case is used.
>
> I think the changes in this patch are good and correct but I agree with
> Simon here, this patch should be split.

I have split this patch and made fixes in v2.

Thanks,
Kaneko

>
> >
> > > ---
> > >  drivers/thermal/rcar_gen3_thermal.c | 86 +++++++++++++++++++++++++------------
> > >  1 file changed, 58 insertions(+), 28 deletions(-)
> > >
> > > This patch is based on the master branch of Linus Torvalds's linux tree.
> > >
> > > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > > index 88fa41c..de6f244 100644
> > > --- a/drivers/thermal/rcar_gen3_thermal.c
> > > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > > @@ -63,6 +63,15 @@
> > >
> > >  #define TSC_MAX_NUM        3
> > >
> > > +static int tj_2;
> >
> > We need to avoid global variables. There can be multiple
> > instances of this driver. (Although they will all get the same
> > value for tj_2.
> >
> > Perhaps it can be added to struct rcar_gen3_thermal_tsc?
> >
> > ...
>
> --
> Regards,
> Niklas Söderlund

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

end of thread, other threads:[~2019-04-16 17:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-21 20:47 [PATCH/RFT] thermal: rcar_gen3_thermal: Update calculation formula due to HW evaluation Yoshihiro Kaneko
2019-04-01 13:37 ` Simon Horman
2019-04-03  9:22   ` Simon Horman
2019-04-16 17:56     ` Yoshihiro Kaneko
2019-04-11 16:50   ` Niklas Söderlund
2019-04-16 17:58     ` Yoshihiro Kaneko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.