[PATCH 0/3] thermal: rcar_gen3: Use temperature approximation from datasheet

[PATCH 0/3] thermal: rcar_gen3: Use temperature approximation from datasheet

[Niklas Söderlund]

Hello,

When the driver was first added the temperature approximation was 
reversed engineered from an out-of-tree driver as the datasheets of the 
time did not contain this information. Recent datasheets, both Gen3 and 
Gen4, now contains this information.

This series changes the temperature approximation formula to match 
what's described in the datasheets. It has been tested on both Gen3 and 
Gen4 with minimal changes in temperatures reported.

Patch 1 is a cleanup making the scope of a constant more clear. Patch 3 
increases the granularity of the readout to 1 decimal to match recent 
datasheets. While Patch 2 is the real work changing the approximation 
formula.

Niklas Söderlund (3):
  thermal: rcar_gen3: Move Tj_T storage to shared private data
  thermal: rcar_gen3: Update temperature approximation calculation
  thermal: rcar_gen3: Increase granularity of readings

 drivers/thermal/rcar_gen3_thermal.c | 144 ++++++++++++++++------------
 1 file changed, 81 insertions(+), 63 deletions(-)

-- 
2.44.0

[PATCH 1/3] thermal: rcar_gen3: Move Tj_T storage to shared private data

[Niklas Söderlund]

The calculated Tj_T constant is calculated from the PTAT data either
read from the first TSC zone on the device if calibration data is fused,
or from fallback values in the driver itself. The value calculated is
shared among all TSC zones.

Move the Tj_T constant to the shared private data structure instead of
duplicating it in each TSC private data. This requires adding a pointer
to the shared data to the TSC private data structure. This back pointer
make it easier to curter rework the temperature conversion logic.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_gen3_thermal.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index cafcb6d6e235..da1b971b287d 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -81,10 +81,10 @@ struct rcar_thermal_info {
 };
 
 struct rcar_gen3_thermal_tsc {
+	struct rcar_gen3_thermal_priv *priv;
 	void __iomem *base;
 	struct thermal_zone_device *zone;
 	struct equation_coefs coef;
-	int tj_t;
 	int thcode[3];
 };
 
@@ -92,6 +92,7 @@ struct rcar_gen3_thermal_priv {
 	struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
 	struct thermal_zone_device_ops ops;
 	unsigned int num_tscs;
+	int tj_t;
 	int ptat[3];
 	const struct rcar_thermal_info *info;
 };
@@ -146,15 +147,15 @@ static void rcar_gen3_thermal_calc_coefs(struct rcar_gen3_thermal_priv *priv,
 	 * Division is not scaled in BSP and if scaled it might overflow
 	 * the dividend (4095 * 4095 << 14 > INT_MAX) so keep it unscaled
 	 */
-	tsc->tj_t = (FIXPT_INT((priv->ptat[1] - priv->ptat[2]) * (ths_tj_1 - TJ_3))
-		     / (priv->ptat[0] - priv->ptat[2])) + FIXPT_INT(TJ_3);
+	priv->tj_t = (FIXPT_INT((priv->ptat[1] - priv->ptat[2]) * (ths_tj_1 - TJ_3))
+		      / (priv->ptat[0] - priv->ptat[2])) + FIXPT_INT(TJ_3);
 
 	tsc->coef.a1 = FIXPT_DIV(FIXPT_INT(tsc->thcode[1] - tsc->thcode[2]),
-				 tsc->tj_t - FIXPT_INT(TJ_3));
+				 priv->tj_t - FIXPT_INT(TJ_3));
 	tsc->coef.b1 = FIXPT_INT(tsc->thcode[2]) - tsc->coef.a1 * TJ_3;
 
 	tsc->coef.a2 = FIXPT_DIV(FIXPT_INT(tsc->thcode[1] - tsc->thcode[0]),
-				 tsc->tj_t - FIXPT_INT(ths_tj_1));
+				 priv->tj_t - FIXPT_INT(ths_tj_1));
 	tsc->coef.b2 = FIXPT_INT(tsc->thcode[0]) - tsc->coef.a2 * ths_tj_1;
 }
 
@@ -196,10 +197,11 @@ static int rcar_gen3_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
 static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc,
 					      int mcelsius)
 {
+	struct rcar_gen3_thermal_priv *priv = tsc->priv;
 	int celsius, val;
 
 	celsius = DIV_ROUND_CLOSEST(mcelsius, 1000);
-	if (celsius <= INT_FIXPT(tsc->tj_t))
+	if (celsius <= INT_FIXPT(priv->tj_t))
 		val = celsius * tsc->coef.a1 + tsc->coef.b1;
 	else
 		val = celsius * tsc->coef.a2 + tsc->coef.b2;
@@ -512,6 +514,7 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 			goto error_unregister;
 		}
 
+		tsc->priv = priv;
 		tsc->base = devm_ioremap_resource(dev, res);
 		if (IS_ERR(tsc->base)) {
 			ret = PTR_ERR(tsc->base);
-- 
2.44.0

Re: [PATCH 1/3] thermal: rcar_gen3: Move Tj_T storage to shared private data

[Geert Uytterhoeven]

Hi Niklas,

On Thu, Mar 7, 2024 at 12:03 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The calculated Tj_T constant is calculated from the PTAT data either
> read from the first TSC zone on the device if calibration data is fused,
> or from fallback values in the driver itself. The value calculated is
> shared among all TSC zones.
>
> Move the Tj_T constant to the shared private data structure instead of
> duplicating it in each TSC private data. This requires adding a pointer
> to the shared data to the TSC private data structure. This back pointer
> make it easier to curter rework the temperature conversion logic.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Thanks for your patch!

> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -81,10 +81,10 @@ struct rcar_thermal_info {
>  };
>
>  struct rcar_gen3_thermal_tsc {
> +       struct rcar_gen3_thermal_priv *priv;

I had hoped you could do without this, but I couldn't find a better way.
Even the contents of &priv->ops are copied (twice!) inside the thermal
core, so you can't go through that...

>         void __iomem *base;
>         struct thermal_zone_device *zone;
>         struct equation_coefs coef;
> -       int tj_t;
>         int thcode[3];
>  };
>
> @@ -92,6 +92,7 @@ struct rcar_gen3_thermal_priv {
>         struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM];
>         struct thermal_zone_device_ops ops;
>         unsigned int num_tscs;
> +       int tj_t;

Insert below ptat[3], as tj_t is calculated from ptat[3], and to better
approach reverse Christmas-tree ordering?

>         int ptat[3];
>         const struct rcar_thermal_info *info;
>  };
> @@ -146,15 +147,15 @@ static void rcar_gen3_thermal_calc_coefs(struct rcar_gen3_thermal_priv *priv,
>          * Division is not scaled in BSP and if scaled it might overflow
>          * the dividend (4095 * 4095 << 14 > INT_MAX) so keep it unscaled
>          */
> -       tsc->tj_t = (FIXPT_INT((priv->ptat[1] - priv->ptat[2]) * (ths_tj_1 - TJ_3))
> -                    / (priv->ptat[0] - priv->ptat[2])) + FIXPT_INT(TJ_3);
> +       priv->tj_t = (FIXPT_INT((priv->ptat[1] - priv->ptat[2]) * (ths_tj_1 - TJ_3))
> +                     / (priv->ptat[0] - priv->ptat[2])) + FIXPT_INT(TJ_3);

Please move the calculation of priv->tj_t to rcar_gen3_thermal_probe()
or rcar_gen3_thermal_read_fuses(), so it is no longer done multiple
times.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] thermal: rcar_gen3: Move Tj_T storage to shared private data

[Geert Uytterhoeven]

On Thu, Mar 7, 2024 at 12:03 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The calculated Tj_T constant is calculated from the PTAT data either
> read from the first TSC zone on the device if calibration data is fused,
> or from fallback values in the driver itself. The value calculated is
> shared among all TSC zones.
>
> Move the Tj_T constant to the shared private data structure instead of
> duplicating it in each TSC private data. This requires adding a pointer
> to the shared data to the TSC private data structure. This back pointer
> make it easier to curter rework the temperature conversion logic.

further?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 2/3] thermal: rcar_gen3: Update temperature approximation calculation

[Niklas Söderlund]

The initial driver used a formula to approximation the temperature and
register value reversed engineered form an out-of-tree BSP driver. This
was needed as the datasheet at the time did not contain any information
on how to do this. Later Gen3 (Rev 2.30) and Gen4 (all) now contains
this information.

Update the approximation formula to use the datasheets information
instead of the reversed engineered one.

On an idle M3-N without fused calibration values for PTAT and THCODE the
old formula reports,

    zone0: 52000
    zone1: 53000
    zone2: 52500

While the new formula under the same circumstances reports,

    zone0: 52500
    zone1: 54000
    zone2: 54000

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_gen3_thermal.c | 137 +++++++++++++++-------------
 1 file changed, 76 insertions(+), 61 deletions(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index da1b971b287d..56fba675b986 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -65,26 +65,29 @@
 
 #define TSC_MAX_NUM	5
 
-/* Structure for thermal temperature calculation */
-struct equation_coefs {
-	int a1;
-	int b1;
-	int a2;
-	int b2;
-};
-
 struct rcar_gen3_thermal_priv;
 
 struct rcar_thermal_info {
-	int ths_tj_1;
+	int scale;
+	int adj_below;
+	int adj_above;
 	void (*read_fuses)(struct rcar_gen3_thermal_priv *priv);
 };
 
+struct equation_set_coef {
+	int a;
+	int b;
+};
+
 struct rcar_gen3_thermal_tsc {
 	struct rcar_gen3_thermal_priv *priv;
 	void __iomem *base;
 	struct thermal_zone_device *zone;
-	struct equation_coefs coef;
+	/* Different coefficients are used depending on a threshold. */
+	struct {
+		struct equation_set_coef below;
+		struct equation_set_coef above;
+	} coef;
 	int thcode[3];
 };
 
@@ -112,51 +115,41 @@ static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
 /*
  * Linear approximation for temperature
  *
- * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a
+ * [temp] = ((thadj - [reg]) * a) / b + adj
+ * [reg] = thadj - ([temp] - adj) * b / a
  *
  * The constants a and b are calculated using two triplets of int values PTAT
  * and THCODE. PTAT and THCODE can either be read from hardware or use hard
  * coded values from driver. The formula to calculate a and b are taken from
- * BSP and sparsely documented and understood.
+ * the datasheet. Different calculations are needed for a and b depending on
+ * if the input variable ([temp] or [reg]) are above or below a threshold. The
+ * threshold is also calculated from PTAT and THCODE using formula from the
+ * datasheet.
  *
- * Examining the linear formula and the formula used to calculate constants a
- * and b while knowing that the span for PTAT and THCODE values are between
- * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
- * Integer also needs to be signed so that leaves 7 bits for binary
- * fixed point scaling.
+ * The constant thadj is one of the THCODE values, which one to use depends on
+ * the threshold and input value.
+ *
+ * The constants adj is taken verbatim from the datasheet. Two values exists,
+ * which one to use depends on the input value and the calculated threshold.
+ * Furthermore different SoCs models supported by the driver have different sets
+ * of values. The values for each model is stored in the device match data.
  */
 
-#define FIXPT_SHIFT 7
-#define FIXPT_INT(_x) ((_x) << FIXPT_SHIFT)
-#define INT_FIXPT(_x) ((_x) >> FIXPT_SHIFT)
-#define FIXPT_DIV(_a, _b) DIV_ROUND_CLOSEST(((_a) << FIXPT_SHIFT), (_b))
-#define FIXPT_TO_MCELSIUS(_x) ((_x) * 1000 >> FIXPT_SHIFT)
-
 #define RCAR3_THERMAL_GRAN 500 /* mili Celsius */
 
-/* no idea where these constants come from */
-#define TJ_3 -41
-
 static void rcar_gen3_thermal_calc_coefs(struct rcar_gen3_thermal_priv *priv,
-					 struct rcar_gen3_thermal_tsc *tsc,
-					 int ths_tj_1)
+					 struct rcar_gen3_thermal_tsc *tsc)
 {
-	/* TODO: Find documentation and document constant calculation formula */
+	priv->tj_t =
+		DIV_ROUND_CLOSEST((priv->ptat[1] - priv->ptat[2]) * priv->info->scale,
+				  priv->ptat[0] - priv->ptat[2])
+		+ priv->info->adj_below;
 
-	/*
-	 * Division is not scaled in BSP and if scaled it might overflow
-	 * the dividend (4095 * 4095 << 14 > INT_MAX) so keep it unscaled
-	 */
-	priv->tj_t = (FIXPT_INT((priv->ptat[1] - priv->ptat[2]) * (ths_tj_1 - TJ_3))
-		      / (priv->ptat[0] - priv->ptat[2])) + FIXPT_INT(TJ_3);
+	tsc->coef.below.a = priv->info->scale * (priv->ptat[2] - priv->ptat[1]);
+	tsc->coef.above.a = priv->info->scale * (priv->ptat[0] - priv->ptat[1]);
 
-	tsc->coef.a1 = FIXPT_DIV(FIXPT_INT(tsc->thcode[1] - tsc->thcode[2]),
-				 priv->tj_t - FIXPT_INT(TJ_3));
-	tsc->coef.b1 = FIXPT_INT(tsc->thcode[2]) - tsc->coef.a1 * TJ_3;
-
-	tsc->coef.a2 = FIXPT_DIV(FIXPT_INT(tsc->thcode[1] - tsc->thcode[0]),
-				 priv->tj_t - FIXPT_INT(ths_tj_1));
-	tsc->coef.b2 = FIXPT_INT(tsc->thcode[0]) - tsc->coef.a2 * ths_tj_1;
+	tsc->coef.below.b = (priv->ptat[2] - priv->ptat[0]) * (tsc->thcode[2] - tsc->thcode[1]);
+	tsc->coef.above.b = (priv->ptat[0] - priv->ptat[2]) * (tsc->thcode[1] - tsc->thcode[0]);
 }
 
 static int rcar_gen3_thermal_round(int temp)
@@ -172,19 +165,29 @@ static int rcar_gen3_thermal_round(int temp)
 static int rcar_gen3_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
 {
 	struct rcar_gen3_thermal_tsc *tsc = thermal_zone_device_priv(tz);
-	int mcelsius, val;
-	int reg;
+	struct rcar_gen3_thermal_priv *priv = tsc->priv;
+	const struct equation_set_coef *coef;
+	int adj, mcelsius, reg, thcode;
 
 	/* Read register and convert to mili Celsius */
 	reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
 
-	if (reg <= tsc->thcode[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);
+	if (reg < tsc->thcode[1]) {
+		adj = priv->info->adj_below;
+		coef = &tsc->coef.below;
+		thcode = tsc->thcode[2];
+	} else {
+		adj = priv->info->adj_above;
+		coef = &tsc->coef.above;
+		thcode = tsc->thcode[0];
+	}
+
+	/*
+	 * The dividend can't be grown as it might overflow, instead shorten the
+	 * divisor to convert to millidegree Celsius. If we convert after the
+	 * division precision is lost to a full degree Celsius.
+	 */
+	mcelsius = DIV_ROUND_CLOSEST(coef->a * (thcode - reg), coef->b / 1000) + adj * 1000;
 
 	/* Guaranteed operating range is -40C to 125C. */
 
@@ -198,15 +201,21 @@ static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc,
 					      int mcelsius)
 {
 	struct rcar_gen3_thermal_priv *priv = tsc->priv;
-	int celsius, val;
+	const struct equation_set_coef *coef;
+	int adj, celsius, thcode;
 
 	celsius = DIV_ROUND_CLOSEST(mcelsius, 1000);
-	if (celsius <= INT_FIXPT(priv->tj_t))
-		val = celsius * tsc->coef.a1 + tsc->coef.b1;
-	else
-		val = celsius * tsc->coef.a2 + tsc->coef.b2;
+	if (celsius < priv->tj_t) {
+		coef = &tsc->coef.below;
+		adj = priv->info->adj_below;
+		thcode = tsc->thcode[2];
+	} else {
+		coef = &tsc->coef.above;
+		adj = priv->info->adj_above;
+		thcode = tsc->thcode[0];
+	}
 
-	return INT_FIXPT(val);
+	return thcode - DIV_ROUND_CLOSEST((celsius - adj) * coef->b, coef->a);
 }
 
 static int rcar_gen3_thermal_set_trips(struct thermal_zone_device *tz, int low, int high)
@@ -371,17 +380,23 @@ static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_priv *priv,
 }
 
 static const struct rcar_thermal_info rcar_m3w_thermal_info = {
-	.ths_tj_1 = 116,
+	.scale = 157,
+	.adj_below = -41,
+	.adj_above = 116,
 	.read_fuses = rcar_gen3_thermal_read_fuses_gen3,
 };
 
 static const struct rcar_thermal_info rcar_gen3_thermal_info = {
-	.ths_tj_1 = 126,
+	.scale = 167,
+	.adj_below = -41,
+	.adj_above = 126,
 	.read_fuses = rcar_gen3_thermal_read_fuses_gen3,
 };
 
 static const struct rcar_thermal_info rcar_gen4_thermal_info = {
-	.ths_tj_1 = 126,
+	.scale = 167,
+	.adj_below = -41,
+	.adj_above = 126,
 	.read_fuses = rcar_gen3_thermal_read_fuses_gen4,
 };
 
@@ -533,7 +548,7 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
 
 		rcar_gen3_thermal_init(priv, tsc);
-		rcar_gen3_thermal_calc_coefs(priv, tsc, priv->info->ths_tj_1);
+		rcar_gen3_thermal_calc_coefs(priv, tsc);
 
 		zone = devm_thermal_of_zone_register(dev, i, tsc, &priv->ops);
 		if (IS_ERR(zone)) {
-- 
2.44.0

Re: [PATCH 2/3] thermal: rcar_gen3: Update temperature approximation calculation

[Geert Uytterhoeven]

Hi Niklas,

Thanks for your patch!

On Thu, Mar 7, 2024 at 12:03 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The initial driver used a formula to approximation the temperature and

approximate

> register value reversed engineered form an out-of-tree BSP driver. This

values ... from

> was needed as the datasheet at the time did not contain any information
> on how to do this. Later Gen3 (Rev 2.30) and Gen4 (all) now contains
> this information.
>
> Update the approximation formula to use the datasheets information

datasheet's

> instead of the reversed engineered one.

reverse-engineered

> On an idle M3-N without fused calibration values for PTAT and THCODE the
> old formula reports,
>
>     zone0: 52000
>     zone1: 53000
>     zone2: 52500
>
> While the new formula under the same circumstances reports,
>
>     zone0: 52500
>     zone1: 54000
>     zone2: 54000
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c

> @@ -112,51 +115,41 @@ static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
>  /*
>   * Linear approximation for temperature
>   *
> - * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a
> + * [temp] = ((thadj - [reg]) * a) / b + adj
> + * [reg] = thadj - ([temp] - adj) * b / a
>   *
>   * The constants a and b are calculated using two triplets of int values PTAT
>   * and THCODE. PTAT and THCODE can either be read from hardware or use hard
>   * coded values from driver. The formula to calculate a and b are taken from

the driver

> - * BSP and sparsely documented and understood.
> + * the datasheet. Different calculations are needed for a and b depending on
> + * if the input variable ([temp] or [reg]) are above or below a threshold. The

variables

> + * threshold is also calculated from PTAT and THCODE using formula from the

formulas

> + * datasheet.
>   *
> - * Examining the linear formula and the formula used to calculate constants a
> - * and b while knowing that the span for PTAT and THCODE values are between
> - * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
> - * Integer also needs to be signed so that leaves 7 bits for binary
> - * fixed point scaling.
> + * The constant thadj is one of the THCODE values, which one to use depends on
> + * the threshold and input value.
> + *
> + * The constants adj is taken verbatim from the datasheet. Two values exists,
> + * which one to use depends on the input value and the calculated threshold.
> + * Furthermore different SoCs models supported by the driver have different sets

SoC

> + * of values. The values for each model is stored in the device match data.

are

>   */

> @@ -172,19 +165,29 @@ static int rcar_gen3_thermal_round(int temp)
>  static int rcar_gen3_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
>  {
>         struct rcar_gen3_thermal_tsc *tsc = thermal_zone_device_priv(tz);
> -       int mcelsius, val;
> -       int reg;
> +       struct rcar_gen3_thermal_priv *priv = tsc->priv;
> +       const struct equation_set_coef *coef;
> +       int adj, mcelsius, reg, thcode;
>
>         /* Read register and convert to mili Celsius */
>         reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
>
> -       if (reg <= tsc->thcode[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);
> +       if (reg < tsc->thcode[1]) {
> +               adj = priv->info->adj_below;
> +               coef = &tsc->coef.below;
> +               thcode = tsc->thcode[2];
> +       } else {
> +               adj = priv->info->adj_above;
> +               coef = &tsc->coef.above;
> +               thcode = tsc->thcode[0];
> +       }
> +
> +       /*
> +        * The dividend can't be grown as it might overflow, instead shorten the
> +        * divisor to convert to millidegree Celsius. If we convert after the
> +        * division precision is lost to a full degree Celsius.
> +        */
> +       mcelsius = DIV_ROUND_CLOSEST(coef->a * (thcode - reg), coef->b / 1000) + adj * 1000;

Don't you lose a lot of precision by pre-dividing b by 1000?

>
>         /* Guaranteed operating range is -40C to 125C. */
>
> @@ -198,15 +201,21 @@ static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc,
>                                               int mcelsius)
>  {
>         struct rcar_gen3_thermal_priv *priv = tsc->priv;
> -       int celsius, val;
> +       const struct equation_set_coef *coef;
> +       int adj, celsius, thcode;
>
>         celsius = DIV_ROUND_CLOSEST(mcelsius, 1000);

This is pre-existing, but I think it would be good if you could avoid
this (early) division by 1000.


> -       if (celsius <= INT_FIXPT(priv->tj_t))
> -               val = celsius * tsc->coef.a1 + tsc->coef.b1;
> -       else
> -               val = celsius * tsc->coef.a2 + tsc->coef.b2;
> +       if (celsius < priv->tj_t) {
> +               coef = &tsc->coef.below;
> +               adj = priv->info->adj_below;
> +               thcode = tsc->thcode[2];
> +       } else {
> +               coef = &tsc->coef.above;
> +               adj = priv->info->adj_above;
> +               thcode = tsc->thcode[0];
> +       }
>
> -       return INT_FIXPT(val);
> +       return thcode - DIV_ROUND_CLOSEST((celsius - adj) * coef->b, coef->a);
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] thermal: rcar_gen3: Update temperature approximation calculation

[Geert Uytterhoeven]

Hi Niklas,

On Wed, Mar 20, 2024 at 2:22 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Mar 7, 2024 at 12:03 PM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > The initial driver used a formula to approximation the temperature and
> > register value reversed engineered form an out-of-tree BSP driver. This
> > was needed as the datasheet at the time did not contain any information
> > on how to do this. Later Gen3 (Rev 2.30) and Gen4 (all) now contains
> > this information.
> >
> > Update the approximation formula to use the datasheets information
> > instead of the reversed engineered one.
>
> > On an idle M3-N without fused calibration values for PTAT and THCODE the
> > old formula reports,
> >
> >     zone0: 52000
> >     zone1: 53000
> >     zone2: 52500
> >
> > While the new formula under the same circumstances reports,
> >
> >     zone0: 52500
> >     zone1: 54000
> >     zone2: 54000
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > @@ -172,19 +165,29 @@ static int rcar_gen3_thermal_round(int temp)
> >  static int rcar_gen3_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> >  {
> >         struct rcar_gen3_thermal_tsc *tsc = thermal_zone_device_priv(tz);
> > -       int mcelsius, val;
> > -       int reg;
> > +       struct rcar_gen3_thermal_priv *priv = tsc->priv;
> > +       const struct equation_set_coef *coef;
> > +       int adj, mcelsius, reg, thcode;
> >
> >         /* Read register and convert to mili Celsius */
> >         reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> >
> > -       if (reg <= tsc->thcode[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);
> > +       if (reg < tsc->thcode[1]) {
> > +               adj = priv->info->adj_below;
> > +               coef = &tsc->coef.below;
> > +               thcode = tsc->thcode[2];
> > +       } else {
> > +               adj = priv->info->adj_above;
> > +               coef = &tsc->coef.above;
> > +               thcode = tsc->thcode[0];
> > +       }
> > +
> > +       /*
> > +        * The dividend can't be grown as it might overflow, instead shorten the
> > +        * divisor to convert to millidegree Celsius. If we convert after the
> > +        * division precision is lost to a full degree Celsius.
> > +        */
> > +       mcelsius = DIV_ROUND_CLOSEST(coef->a * (thcode - reg), coef->b / 1000) + adj * 1000;
>
> Don't you lose a lot of precision by pre-dividing b by 1000?

After reading PATCH 3/3: instead of calculating millidegree Celsius,
and rounding to a granularity of 0.1 degree Celsius later, perhaps it
makes more sense to calculate decidegree Celsius first (still avoiding
overflow?), and multiply by 100 later?
Then rcar_gen3_thermal_round() can be removed.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] thermal: rcar_gen3: Update temperature approximation calculation

[Niklas Söderlund]

Hi Geert,

Thanks for your feedback.

On 2024-03-20 14:22:31 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> Thanks for your patch!
> 
> On Thu, Mar 7, 2024 at 12:03 PM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > The initial driver used a formula to approximation the temperature and
> 
> approximate
> 
> > register value reversed engineered form an out-of-tree BSP driver. This
> 
> values ... from
> 
> > was needed as the datasheet at the time did not contain any information
> > on how to do this. Later Gen3 (Rev 2.30) and Gen4 (all) now contains
> > this information.
> >
> > Update the approximation formula to use the datasheets information
> 
> datasheet's
> 
> > instead of the reversed engineered one.
> 
> reverse-engineered
> 
> > On an idle M3-N without fused calibration values for PTAT and THCODE the
> > old formula reports,
> >
> >     zone0: 52000
> >     zone1: 53000
> >     zone2: 52500
> >
> > While the new formula under the same circumstances reports,
> >
> >     zone0: 52500
> >     zone1: 54000
> >     zone2: 54000
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> 
> > @@ -112,51 +115,41 @@ static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
> >  /*
> >   * Linear approximation for temperature
> >   *
> > - * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a
> > + * [temp] = ((thadj - [reg]) * a) / b + adj
> > + * [reg] = thadj - ([temp] - adj) * b / a
> >   *
> >   * The constants a and b are calculated using two triplets of int values PTAT
> >   * and THCODE. PTAT and THCODE can either be read from hardware or use hard
> >   * coded values from driver. The formula to calculate a and b are taken from
> 
> the driver
> 
> > - * BSP and sparsely documented and understood.
> > + * the datasheet. Different calculations are needed for a and b depending on
> > + * if the input variable ([temp] or [reg]) are above or below a threshold. The
> 
> variables
> 
> > + * threshold is also calculated from PTAT and THCODE using formula from the
> 
> formulas
> 
> > + * datasheet.
> >   *
> > - * Examining the linear formula and the formula used to calculate constants a
> > - * and b while knowing that the span for PTAT and THCODE values are between
> > - * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
> > - * Integer also needs to be signed so that leaves 7 bits for binary
> > - * fixed point scaling.
> > + * The constant thadj is one of the THCODE values, which one to use depends on
> > + * the threshold and input value.
> > + *
> > + * The constants adj is taken verbatim from the datasheet. Two values exists,
> > + * which one to use depends on the input value and the calculated threshold.
> > + * Furthermore different SoCs models supported by the driver have different sets
> 
> SoC
> 
> > + * of values. The values for each model is stored in the device match data.
> 
> are
> 
> >   */
> 
> > @@ -172,19 +165,29 @@ static int rcar_gen3_thermal_round(int temp)
> >  static int rcar_gen3_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> >  {
> >         struct rcar_gen3_thermal_tsc *tsc = thermal_zone_device_priv(tz);
> > -       int mcelsius, val;
> > -       int reg;
> > +       struct rcar_gen3_thermal_priv *priv = tsc->priv;
> > +       const struct equation_set_coef *coef;
> > +       int adj, mcelsius, reg, thcode;
> >
> >         /* Read register and convert to mili Celsius */
> >         reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
> >
> > -       if (reg <= tsc->thcode[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);
> > +       if (reg < tsc->thcode[1]) {
> > +               adj = priv->info->adj_below;
> > +               coef = &tsc->coef.below;
> > +               thcode = tsc->thcode[2];
> > +       } else {
> > +               adj = priv->info->adj_above;
> > +               coef = &tsc->coef.above;
> > +               thcode = tsc->thcode[0];
> > +       }
> > +
> > +       /*
> > +        * The dividend can't be grown as it might overflow, instead shorten the
> > +        * divisor to convert to millidegree Celsius. If we convert after the
> > +        * division precision is lost to a full degree Celsius.
> > +        */
> > +       mcelsius = DIV_ROUND_CLOSEST(coef->a * (thcode - reg), coef->b / 1000) + adj * 1000;
> 
> Don't you lose a lot of precision by pre-dividing b by 1000?

I do, but the docs say the measurement is only accurate to +/- 2 degrees 
C anyhow so I don't see a real issue losing precision which at worst is 
1 degree C. Of course if a smart way to avoid this lose without the risk of
overflowing that would be ideal.

I see in the follow up reply to this you suggest a way to increase the 
precision by a factor of 10, I will use that in next version.


> 
> >
> >         /* Guaranteed operating range is -40C to 125C. */
> >
> > @@ -198,15 +201,21 @@ static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc,
> >                                               int mcelsius)
> >  {
> >         struct rcar_gen3_thermal_priv *priv = tsc->priv;
> > -       int celsius, val;
> > +       const struct equation_set_coef *coef;
> > +       int adj, celsius, thcode;
> >
> >         celsius = DIV_ROUND_CLOSEST(mcelsius, 1000);
> 
> This is pre-existing, but I think it would be good if you could avoid
> this (early) division by 1000.

I agree, I plan to look into that in a follow series. In this series I 
wanted to focus on getting the approximations match what's in the 
data-sheets.

> 
> 
> > -       if (celsius <= INT_FIXPT(priv->tj_t))
> > -               val = celsius * tsc->coef.a1 + tsc->coef.b1;
> > -       else
> > -               val = celsius * tsc->coef.a2 + tsc->coef.b2;
> > +       if (celsius < priv->tj_t) {
> > +               coef = &tsc->coef.below;
> > +               adj = priv->info->adj_below;
> > +               thcode = tsc->thcode[2];
> > +       } else {
> > +               coef = &tsc->coef.above;
> > +               adj = priv->info->adj_above;
> > +               thcode = tsc->thcode[0];
> > +       }
> >
> > -       return INT_FIXPT(val);
> > +       return thcode - DIV_ROUND_CLOSEST((celsius - adj) * coef->b, coef->a);
> >  }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

-- 
Kind Regards,
Niklas Söderlund

[PATCH 3/3] thermal: rcar_gen3: Increase granularity of readings

[Niklas Söderlund]

Later versions of the datasheet (Gen3 rev 2.30) have examples of
temperature approximations with one decimal. Increase the granularity
when reporting temperatures to match this.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_gen3_thermal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 56fba675b986..fa4c6da8e460 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -135,7 +135,7 @@ static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
  * of values. The values for each model is stored in the device match data.
  */
 
-#define RCAR3_THERMAL_GRAN 500 /* mili Celsius */
+#define RCAR3_THERMAL_GRAN 100 /* millidegree Celsius */
 
 static void rcar_gen3_thermal_calc_coefs(struct rcar_gen3_thermal_priv *priv,
 					 struct rcar_gen3_thermal_tsc *tsc)
-- 
2.44.0

Re: [PATCH 3/3] thermal: rcar_gen3: Increase granularity of readings

[Geert Uytterhoeven]

On Thu, Mar 7, 2024 at 12:03 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Later versions of the datasheet (Gen3 rev 2.30) have examples of
> temperature approximations with one decimal. Increase the granularity
> when reporting temperatures to match this.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds