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@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?
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@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
--
Kind Regards,
Niklas Söderlund
