Message ID | 1553201273-16757-1-git-send-email-ykaneko0929@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | [PATCH/RFT] thermal: rcar_gen3_thermal: Update calculation formula due to HW evaluation | expand |
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? ...
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.
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? > > ...
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.
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
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);