Hi Niklas,
Thanks for your patch!
On Thu, Mar 7, 2024 at 12:03 PM Niklas Söderlund
<niklas.soderlund+renesas@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> --- 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@xxxxxxxxxxxxxx
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
