[PATCH/RFT,v2,1/3] thermal: rcar_gen3_thermal: Update value of Tj_1
diff mbox series

Message ID 1555436655-5262-2-git-send-email-ykaneko0929@gmail.com
State Changes Requested
Delegated to: Eduardo Valentin
Headers show
Series
  • thermal: rcar_gen3_thermal: Update calculation formula due to HW evaluation
Related show

Commit Message

Yoshihiro Kaneko April 16, 2019, 5:44 p.m. UTC
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)

Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
---
 drivers/thermal/rcar_gen3_thermal.c | 41 +++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 11 deletions(-)

Comments

Niklas Soderlund April 18, 2019, 8:40 a.m. UTC | #1
Hi Kaneko-san,

Thanks for your work.

On 2019-04-17 02:44:13 +0900, Yoshihiro Kaneko wrote:
> 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)
> 
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> ---
>  drivers/thermal/rcar_gen3_thermal.c | 41 +++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 88fa41c..a2fd0fd 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -124,11 +124,11 @@ 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
>  
>  static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
> -					 int *ptat, int *thcode)
> +					 int *ptat, int *thcode,
> +					 unsigned int ths_tj_1)

I would move tj_1 inside struce rcar_gen3_thermal_tsc as you in 2/3 move 
tj_2 there. You could still keep the value in .data but init the tj_1 in 
the struct at probe instead of passing it as an argument.

>  {
>  	int tj_2;
>  
> @@ -139,15 +139,15 @@ static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
>  	 * 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);
> +		/ (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)
> @@ -318,12 +318,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 +366,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;
> @@ -422,7 +440,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  		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
>
Simon Horman April 24, 2019, 2:54 p.m. UTC | #2
Hi Kaneko-san,

On Wed, Apr 17, 2019 at 02:44:13AM +0900, Yoshihiro Kaneko wrote:
> 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)
> 
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> ---
>  drivers/thermal/rcar_gen3_thermal.c | 41 +++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 88fa41c..a2fd0fd 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -124,11 +124,11 @@ 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
>  
>  static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
> -					 int *ptat, int *thcode)
> +					 int *ptat, int *thcode,
> +					 unsigned int ths_tj_1)

While testing I found that the type of ths_tj_1 needs to be int
rather than unsigned int, in order for the FIXPT logic to work correctly.

And with that change in place the entire series appears to work correctly.

My suggestion is to change the types of ths_tj_1 here, rcar_gen3_ths_tj_1
in rcar_gen3_thermal_probe(), and rcar_gen3_ths_tj_1 and
rcar_gen3_ths_tj_1_m3_w, which are gloabl to this file accordingly.

>  {
>  	int tj_2;
>  
> @@ -139,15 +139,15 @@ static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
>  	 * 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);
> +		/ (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)
> @@ -318,12 +318,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 +366,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;
> @@ -422,7 +440,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  		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
>
Yoshihiro Kaneko May 8, 2019, 6:23 a.m. UTC | #3
Hi Niklas-san,

Thanks for your review!

2019年4月18日(木) 17:40 Niklas Söderlund <niklas.soderlund@ragnatech.se>:
>
> Hi Kaneko-san,
>
> Thanks for your work.
>
> On 2019-04-17 02:44:13 +0900, Yoshihiro Kaneko wrote:
> > 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)
> >
> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > ---
> >  drivers/thermal/rcar_gen3_thermal.c | 41 +++++++++++++++++++++++++++----------
> >  1 file changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > index 88fa41c..a2fd0fd 100644
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > @@ -124,11 +124,11 @@ 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
> >
> >  static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
> > -                                      int *ptat, int *thcode)
> > +                                      int *ptat, int *thcode,
> > +                                      unsigned int ths_tj_1)
>
> I would move tj_1 inside struce rcar_gen3_thermal_tsc as you in 2/3 move
> tj_2 there. You could still keep the value in .data but init the tj_1 in
> the struct at probe instead of passing it as an argument.

This function is a simple subroutine, is not called back.
Therefore, I think that it is not necessary to move tj_1 into
rcar_gen3_thermal_tsc.

Thanks,
Kaneko

>
> >  {
> >       int tj_2;
> >
> > @@ -139,15 +139,15 @@ static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
> >        * 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);
> > +             / (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)
> > @@ -318,12 +318,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 +366,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;
> > @@ -422,7 +440,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> >               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
> >
>
> --
> Regards,
> Niklas Söderlund
Yoshihiro Kaneko May 8, 2019, 6:36 a.m. UTC | #4
Hi Simon-san,

Thanks for testing this patch!

2019年4月24日(水) 23:54 Simon Horman <horms@verge.net.au>:
>
> Hi Kaneko-san,
>
> On Wed, Apr 17, 2019 at 02:44:13AM +0900, Yoshihiro Kaneko wrote:
> > 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)
> >
> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > ---
> >  drivers/thermal/rcar_gen3_thermal.c | 41 +++++++++++++++++++++++++++----------
> >  1 file changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > index 88fa41c..a2fd0fd 100644
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > @@ -124,11 +124,11 @@ 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
> >
> >  static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
> > -                                      int *ptat, int *thcode)
> > +                                      int *ptat, int *thcode,
> > +                                      unsigned int ths_tj_1)
>
> While testing I found that the type of ths_tj_1 needs to be int
> rather than unsigned int, in order for the FIXPT logic to work correctly.
>
> And with that change in place the entire series appears to work correctly.
>
> My suggestion is to change the types of ths_tj_1 here, rcar_gen3_ths_tj_1
> in rcar_gen3_thermal_probe(), and rcar_gen3_ths_tj_1 and
> rcar_gen3_ths_tj_1_m3_w, which are gloabl to this file accordingly.

I understood. Why did I decide to use unsigned?
I will fix it in v3.

Thanks,
Kaneko

>
> >  {
> >       int tj_2;
> >
> > @@ -139,15 +139,15 @@ static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
> >        * 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);
> > +             / (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)
> > @@ -318,12 +318,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 +366,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;
> > @@ -422,7 +440,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> >               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
> >
Simon Horman May 8, 2019, 10:14 a.m. UTC | #5
On Wed, May 08, 2019 at 03:36:36PM +0900, Yoshihiro Kaneko wrote:
> Hi Simon-san,
> 
> Thanks for testing this patch!
> 
> 2019年4月24日(水) 23:54 Simon Horman <horms@verge.net.au>:
> >
> > Hi Kaneko-san,
> >
> > On Wed, Apr 17, 2019 at 02:44:13AM +0900, Yoshihiro Kaneko wrote:
> > > 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)
> > >
> > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > > ---
> > >  drivers/thermal/rcar_gen3_thermal.c | 41 +++++++++++++++++++++++++++----------
> > >  1 file changed, 30 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> > > index 88fa41c..a2fd0fd 100644
> > > --- a/drivers/thermal/rcar_gen3_thermal.c
> > > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > > @@ -124,11 +124,11 @@ 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
> > >
> > >  static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
> > > -                                      int *ptat, int *thcode)
> > > +                                      int *ptat, int *thcode,
> > > +                                      unsigned int ths_tj_1)
> >
> > While testing I found that the type of ths_tj_1 needs to be int
> > rather than unsigned int, in order for the FIXPT logic to work correctly.
> >
> > And with that change in place the entire series appears to work correctly.
> >
> > My suggestion is to change the types of ths_tj_1 here, rcar_gen3_ths_tj_1
> > in rcar_gen3_thermal_probe(), and rcar_gen3_ths_tj_1 and
> > rcar_gen3_ths_tj_1_m3_w, which are gloabl to this file accordingly.
> 
> I understood. Why did I decide to use unsigned?
> I will fix it in v3.

Probably due to an earlier suggestion by me.
Sorry about that.

...

Could you post v3 with this fix and the changes suggested by Niklas
for patch 2/3?

Patch
diff mbox series

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 88fa41c..a2fd0fd 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -124,11 +124,11 @@  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
 
 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;
 
@@ -139,15 +139,15 @@  static void rcar_gen3_thermal_calc_coefs(struct equation_coefs *coef,
 	 * 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);
+		/ (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)
@@ -318,12 +318,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 +366,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;
@@ -422,7 +440,8 @@  static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		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);