Hi Niklas,
On Tue, Oct 12, 2021 at 12:58 AM Niklas Söderlund
<niklas.soderlund+renesas@xxxxxxxxxxxx> wrote:
> In production hardware the calibration values used to convert register
> values to temperatures can be read from hardware. While pre-production
> hardware still depends on pseudo values hard-coded in the driver.
>
> Add support for reading out calibration values from hardware if it's
> fused. The presence of fused calibration is indicated in the THSCP
> register.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> * Changes since RFT
> - Keep thcodes array static.
Thanks for the update!
Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
A few minor bike sheddings^W^Wnits below...
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -245,6 +252,64 @@ static const struct soc_device_attribute r8a7795es1[] = {
> { /* sentinel */ }
> };
>
> +static bool rcar_gen3_thermal_update_fuses(struct rcar_gen3_thermal_priv *priv)
This doesn't sound like a good name to me, as the function does not
update the fuses, but reads their values.
> +{
> + unsigned int i;
> + u32 thscp;
> +
> + /* Default THCODE values in case FUSEs are not set. */
> + static const int thcodes[TSC_MAX_NUM][3] = {
> + { 3397, 2800, 2221 },
> + { 3393, 2795, 2216 },
> + { 3389, 2805, 2237 },
> + { 3415, 2694, 2195 },
> + { 3356, 2724, 2244 },
> + };
Given this is used only inside the if statement below, perhaps it
should be moved there?
> +
> + /* If fuses are not set, fallback to pseudo values. */
> + thscp = rcar_gen3_thermal_read(priv->tscs[0], REG_GEN3_THSCP);
> + if ((thscp & THSCP_COR_PARA_VLD) != THSCP_COR_PARA_VLD) {
> + priv->ptat[0] = 2631;
> + priv->ptat[1] = 1509;
> + priv->ptat[2] = 435;
> +
> + for (i = 0; i < priv->num_tscs; i++) {
> + struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
> +
> + tsc->thcode[0] = thcodes[i][0];
> + tsc->thcode[1] = thcodes[i][1];
> + tsc->thcode[2] = thcodes[i][2];
> + }
> +
> + return false;
> + }
> +
> + /*
> + * Set the pseudo calibration points with fused values.
> + * PTAT is shared between all TSCs but only fused for the first
> + * TSC while THCODEs are fused for each TSC.
> + */
> + priv->ptat[0] = rcar_gen3_thermal_read(priv->tscs[0], REG_GEN3_PTAT1) &
> + GEN3_FUSE_MASK;
> + priv->ptat[1] = rcar_gen3_thermal_read(priv->tscs[0], REG_GEN3_PTAT2) &
> + GEN3_FUSE_MASK;
> + priv->ptat[2] = rcar_gen3_thermal_read(priv->tscs[0], REG_GEN3_PTAT3) &
> + GEN3_FUSE_MASK;
> +
> + for (i = 0; i < priv->num_tscs; i++) {
> + struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
> +
> + tsc->thcode[0] = rcar_gen3_thermal_read(tsc, REG_GEN3_THCODE1) &
> + GEN3_FUSE_MASK;
> + tsc->thcode[1] = rcar_gen3_thermal_read(tsc, REG_GEN3_THCODE2) &
> + GEN3_FUSE_MASK;
> + tsc->thcode[2] = rcar_gen3_thermal_read(tsc, REG_GEN3_THCODE3) &
> + GEN3_FUSE_MASK;
> + }
> +
> + return true;
> +}
> +
> static void rcar_gen3_thermal_init_r8a7795es1(struct rcar_gen3_thermal_tsc *tsc)
> {
> rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR, CTSR_THBGR);
> @@ -442,11 +493,16 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> goto error_unregister;
> }
>
> - tsc->thcode[0] = thcodes[i][0];
> - tsc->thcode[1] = thcodes[i][1];
> - tsc->thcode[2] = thcodes[i][2];
> -
> priv->tscs[i] = tsc;
> + }
> +
> + priv->num_tscs = i;
> +
> + if (rcar_gen3_thermal_update_fuses(priv))
> + dev_info(dev, "Using fused calibration values\n");
Despite our lack of test hardware having programmed fuses, using the
values from the fuses should be the normal situation, right?
So perhaps print a message when falling back to the default values
instead?
> +
> + for (i = 0; i < priv->num_tscs; i++) {
> + struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
>
> zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
> &rcar_gen3_tz_of_ops);
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
