by Thara Gopinath

[permalink] [raw]

Subject: Re: [PATCH v10 5/8] drivers: thermal: tsens: Fix bug in sensor enable for msm8960

On 2/17/21 2:40 PM, Ansuel Smith wrote:
> It's present a hardware bug in tsens VER_0 where if sensors upper to id
> 6 are enabled selectively, underfined results are expected. Fix this by
> enabling all the remaining sensor in one step.

It took me a while to understand this. It is most likely me! But please
consider rewording.

>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/thermal/qcom/tsens-8960.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
> index 86585f439985..248aaa65b5b0 100644
> --- a/drivers/thermal/qcom/tsens-8960.c
> +++ b/drivers/thermal/qcom/tsens-8960.c
> @@ -27,9 +27,9 @@
> #define EN BIT(0)
> #define SW_RST BIT(1)
> #define SENSOR0_EN BIT(3)
> +#define MEASURE_PERIOD BIT(18)
> #define SLP_CLK_ENA BIT(26)
> #define SLP_CLK_ENA_8660 BIT(24)
> -#define MEASURE_PERIOD 1
> #define SENSOR0_SHIFT 3
>
> /* INT_STATUS_ADDR bitmasks */
> @@ -132,11 +132,26 @@ static int enable_8960(struct tsens_priv *priv, int id)
> if (ret)
> return ret;
>
> - mask = BIT(id + SENSOR0_SHIFT);
> + /* HARDWARE BUG:
> + * On platform with more than 5 sensors, all the remaining

Isn't it 6 ? At least according to code below it is.. You are checking
for id > 5.

> + * sensors needs to be enabled all togheder or underfined
> + * results are expected. (Sensor 6-7 disabled, Sensor 3
> + * disabled...) In the original driver, all the sensors
> + * are enabled in one step hence this bug is not triggered.

Also with this change, you should add a check in this function to see if
the sensors are already enabled and if yes return back. The enabling
call from tsens.c happens for every single sensor. But at sensor number
6 you are enabling rest of the sensors. There is absolutely no reason to
keep doing this for rest of the sensors.

> + */
> + if (id > 5)
> + mask = GENMASK(10, 6);
> + else
> + mask = BIT(id);
> +
> + mask <<= SENSOR0_SHIFT;
> +
> ret = regmap_write(priv->tm_map, CNTL_ADDR, reg | SW_RST);

I know this is not part of this patch. But you mention above that
earlier you were enabling all sensors one shot. Now that this is being
done one at a time, is it needed to do a SW_RST every time ?

> if (ret)
> return ret;
>
> + reg |= MEASURE_PERIOD;
> +
> if (priv->num_sensors > 1)
> reg |= mask | SLP_CLK_ENA | EN;
> else
>

--
Warm Regards
Thara

2021-03-18 21:17:31

by Thara Gopinath

[permalink] [raw]

Subject: Re: [PATCH v10 6/8] drivers: thermal: tsens: Use get_temp_common for msm8960

On 2/17/21 2:40 PM, Ansuel Smith wrote:
> Rework calibrate function to use common function. Derive the offset from
> a missing hardcoded slope table and the data from the nvmem calib
> efuses.

You are also changing get_temp to use get_temp_common instead of
get_temp_8960 in this patch. Please add it to commit description as
well.I will also consider changing the subject header to something more
generic like
"drivers: thermal: tsens: Replace custom 8960 apis with generic apis"
or anything better.

Otherwise,
Acked-by: Thara Gopinath <[email protected]>

Warm Regards
Thara

>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/thermal/qcom/tsens-8960.c | 56 +++++++++----------------------
> 1 file changed, 15 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
> index 248aaa65b5b0..43ebe4d54672 100644
> --- a/drivers/thermal/qcom/tsens-8960.c
> +++ b/drivers/thermal/qcom/tsens-8960.c
> @@ -67,6 +67,13 @@
> #define S9_STATUS_OFF 0x3674
> #define S10_STATUS_OFF 0x3678
>
> +/* Original slope - 200 to compensate mC to C inaccuracy */
> +u32 tsens_msm8960_slope[] = {
> + 976, 976, 954, 976,
> + 911, 932, 932, 999,
> + 932, 999, 932
> + };
> +
> static int suspend_8960(struct tsens_priv *priv)
> {
> int ret;
> @@ -192,9 +199,7 @@ static int calibrate_8960(struct tsens_priv *priv)
> {
> int i;
> char *data;
> -
> - ssize_t num_read = priv->num_sensors;
> - struct tsens_sensor *s = priv->sensor;
> + u32 p1[11];
>
> data = qfprom_read(priv->dev, "calib");
> if (IS_ERR(data))
> @@ -202,49 +207,18 @@ static int calibrate_8960(struct tsens_priv *priv)
> if (IS_ERR(data))
> return PTR_ERR(data);
>
> - for (i = 0; i < num_read; i++, s++)
> - s->offset = data[i];
> + for (i = 0; i < priv->num_sensors; i++) {
> + p1[i] = data[i];
> + priv->sensor[i].slope = tsens_msm8960_slope[i];
> + }
> +
> + compute_intercept_slope(priv, p1, NULL, ONE_PT_CALIB);
>
> kfree(data);
>
> return 0;
> }
>
> -/* Temperature on y axis and ADC-code on x-axis */
> -static inline int code_to_mdegC(u32 adc_code, const struct tsens_sensor *s)
> -{
> - int slope, offset;
> -
> - slope = thermal_zone_get_slope(s->tzd);
> - offset = CAL_MDEGC - slope * s->offset;
> -
> - return adc_code * slope + offset;
> -}
> -
> -static int get_temp_8960(const struct tsens_sensor *s, int *temp)
> -{
> - int ret;
> - u32 code, trdy;
> - struct tsens_priv *priv = s->priv;
> - unsigned long timeout;
> -
> - timeout = jiffies + usecs_to_jiffies(TIMEOUT_US);
> - do {
> - ret = regmap_read(priv->tm_map, INT_STATUS_ADDR, &trdy);
> - if (ret)
> - return ret;
> - if (!(trdy & TRDY_MASK))
> - continue;
> - ret = regmap_read(priv->tm_map, s->status, &code);
> - if (ret)
> - return ret;
> - *temp = code_to_mdegC(code, s);
> - return 0;
> - } while (time_before(jiffies, timeout));
> -
> - return -ETIMEDOUT;
> -}
> -
> static struct tsens_features tsens_8960_feat = {
> .ver_major = VER_0,
> .crit_int = 0,
> @@ -313,7 +287,7 @@ static const struct reg_field tsens_8960_regfields[MAX_REGFIELDS] = {
> static const struct tsens_ops ops_8960 = {
> .init = init_common,
> .calibrate = calibrate_8960,
> - .get_temp = get_temp_8960,
> + .get_temp = get_temp_common,
> .enable = enable_8960,
> .disable = disable_8960,
> .suspend = suspend_8960,
>