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

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

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