| Message ID | 20230315103950.2679317-4-stephan.gerhold@kernkonzept.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | thermal: qcom: tsens: Fix MDM9607, add MSM8909 | expand |
On 15.03.2023 11:39, Stephan Gerhold wrote: > According to the msm-3.18 vendor kernel from Qualcomm, mdm9607 needs > "correction factors" to adjust for additional offsets observed after the > factory calibration values in the fuses [1, 2]. > > The fixed offsets should be applied unless there is a special > calibration mode value that indicates that no offsets are needed [3]. > > Note that the new calibration mode values are called differently in this > patch compared to the vendor kernel: > - TSENS_TWO_POINT_CALIB_N_WA -> ONE_PT_CALIB2_NO_OFFSET > - TSENS_TWO_POINT_CALIB_N_OFFSET_WA -> TWO_PT_CALIB_NO_OFFSET > This is because close inspection of the calibration function [3] reveals > that TSENS_TWO_POINT_CALIB_N_WA is actually a "one point" calibration > because the if statements skip all "point2" related code for it. > > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d9d2db1b82bf3f72f5de0803d55e6849eb5b671e > [2]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d75aef53a760e8ff7bac54049d00c8b2ee1b193e > [3]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LE.UM.4.3.2.r1-04200-9x07/drivers/thermal/msm-tsens.c#L2987-3136 > > Cc: Konrad Dybcio <konrad.dybcio@linaro.org> > Fixes: a2149ab815fc ("thermal/drivers/qcom/tsens-v0_1: Add support for MDM9607") > Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> BTW, did you notice some crazy readouts or would this have gone unnoticed had you not dug in the code? Konrad > Changes in v3: None > Changes in v2: New patch > --- > drivers/thermal/qcom/tsens-v0_1.c | 11 +++++++++++ > drivers/thermal/qcom/tsens.c | 16 +++++++++++++++- > drivers/thermal/qcom/tsens.h | 4 ++++ > 3 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c > index 1b454de3928d..e69889dd524a 100644 > --- a/drivers/thermal/qcom/tsens-v0_1.c > +++ b/drivers/thermal/qcom/tsens-v0_1.c > @@ -229,6 +229,17 @@ static int __init init_9607(struct tsens_priv *priv) > for (i = 0; i < priv->num_sensors; ++i) > priv->sensor[i].slope = 3000; > > + priv->sensor[0].p1_calib_offset = 1; > + priv->sensor[0].p2_calib_offset = 1; > + priv->sensor[1].p1_calib_offset = -4; > + priv->sensor[1].p2_calib_offset = -2; > + priv->sensor[2].p1_calib_offset = 4; > + priv->sensor[2].p2_calib_offset = 8; > + priv->sensor[3].p1_calib_offset = -3; > + priv->sensor[3].p2_calib_offset = -5; > + priv->sensor[4].p1_calib_offset = -4; > + priv->sensor[4].p2_calib_offset = -4; > + > return init_common(priv); > } > > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c > index 8020ead2794e..32d2907f76e5 100644 > --- a/drivers/thermal/qcom/tsens.c > +++ b/drivers/thermal/qcom/tsens.c > @@ -134,10 +134,12 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2, > p1[i] = p1[i] + (base1 << shift); > break; > case TWO_PT_CALIB: > + case TWO_PT_CALIB_NO_OFFSET: > for (i = 0; i < priv->num_sensors; i++) > p2[i] = (p2[i] + base2) << shift; > fallthrough; > case ONE_PT_CALIB2: > + case ONE_PT_CALIB2_NO_OFFSET: > for (i = 0; i < priv->num_sensors; i++) > p1[i] = (p1[i] + base1) << shift; > break; > @@ -149,6 +151,18 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2, > } > } > > + /* Apply calibration offset workaround except for _NO_OFFSET modes */ > + switch (mode) { > + case TWO_PT_CALIB: > + for (i = 0; i < priv->num_sensors; i++) > + p2[i] += priv->sensor[i].p2_calib_offset; > + fallthrough; > + case ONE_PT_CALIB2: > + for (i = 0; i < priv->num_sensors; i++) > + p1[i] += priv->sensor[i].p1_calib_offset; > + break; > + } > + > return mode; > } > > @@ -254,7 +268,7 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1, > > if (!priv->sensor[i].slope) > priv->sensor[i].slope = SLOPE_DEFAULT; > - if (mode == TWO_PT_CALIB) { > + if (mode == TWO_PT_CALIB || mode == TWO_PT_CALIB_NO_OFFSET) { > /* > * slope (m) = adc_code2 - adc_code1 (y2 - y1)/ > * temp_120_degc - temp_30_degc (x2 - x1) > diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h > index dba9cd38f637..880be6be5c3f 100644 > --- a/drivers/thermal/qcom/tsens.h > +++ b/drivers/thermal/qcom/tsens.h > @@ -10,6 +10,8 @@ > #define ONE_PT_CALIB 0x1 > #define ONE_PT_CALIB2 0x2 > #define TWO_PT_CALIB 0x3 > +#define ONE_PT_CALIB2_NO_OFFSET 0x6 > +#define TWO_PT_CALIB_NO_OFFSET 0x7 > #define CAL_DEGC_PT1 30 > #define CAL_DEGC_PT2 120 > #define SLOPE_FACTOR 1000 > @@ -57,6 +59,8 @@ struct tsens_sensor { > unsigned int hw_id; > int slope; > u32 status; > + int p1_calib_offset; > + int p2_calib_offset; > }; > > /**
On Sat, Mar 18, 2023 at 03:02:35PM +0100, Konrad Dybcio wrote: > On 15.03.2023 11:39, Stephan Gerhold wrote: > > According to the msm-3.18 vendor kernel from Qualcomm, mdm9607 needs > > "correction factors" to adjust for additional offsets observed after the > > factory calibration values in the fuses [1, 2]. > > > > The fixed offsets should be applied unless there is a special > > calibration mode value that indicates that no offsets are needed [3]. > > > > Note that the new calibration mode values are called differently in this > > patch compared to the vendor kernel: > > - TSENS_TWO_POINT_CALIB_N_WA -> ONE_PT_CALIB2_NO_OFFSET > > - TSENS_TWO_POINT_CALIB_N_OFFSET_WA -> TWO_PT_CALIB_NO_OFFSET > > This is because close inspection of the calibration function [3] reveals > > that TSENS_TWO_POINT_CALIB_N_WA is actually a "one point" calibration > > because the if statements skip all "point2" related code for it. > > > > [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d9d2db1b82bf3f72f5de0803d55e6849eb5b671e > > [2]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d75aef53a760e8ff7bac54049d00c8b2ee1b193e > > [3]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LE.UM.4.3.2.r1-04200-9x07/drivers/thermal/msm-tsens.c#L2987-3136 > > > > Cc: Konrad Dybcio <konrad.dybcio@linaro.org> > > Fixes: a2149ab815fc ("thermal/drivers/qcom/tsens-v0_1: Add support for MDM9607") > > Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> > > --- > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > BTW, did you notice some crazy readouts or would this have gone > unnoticed had you not dug in the code? > I'm afraid it would likely have remained unnoticed. I think these offsets only make a small difference but it's still good to have them for slightly more accurate readings. In v1 of this series I had the offsets for MSM8909 already (hardcoded into the old calibration function with all the bit shifts/masks etc). It was more coincidence that I checked MDM9607 for v2 because I had to make the code more generic with the new per-sensor nvmem cells. ;) Thanks, Stephan
diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c index 1b454de3928d..e69889dd524a 100644 --- a/drivers/thermal/qcom/tsens-v0_1.c +++ b/drivers/thermal/qcom/tsens-v0_1.c @@ -229,6 +229,17 @@ static int __init init_9607(struct tsens_priv *priv) for (i = 0; i < priv->num_sensors; ++i) priv->sensor[i].slope = 3000; + priv->sensor[0].p1_calib_offset = 1; + priv->sensor[0].p2_calib_offset = 1; + priv->sensor[1].p1_calib_offset = -4; + priv->sensor[1].p2_calib_offset = -2; + priv->sensor[2].p1_calib_offset = 4; + priv->sensor[2].p2_calib_offset = 8; + priv->sensor[3].p1_calib_offset = -3; + priv->sensor[3].p2_calib_offset = -5; + priv->sensor[4].p1_calib_offset = -4; + priv->sensor[4].p2_calib_offset = -4; + return init_common(priv); } diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c index 8020ead2794e..32d2907f76e5 100644 --- a/drivers/thermal/qcom/tsens.c +++ b/drivers/thermal/qcom/tsens.c @@ -134,10 +134,12 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2, p1[i] = p1[i] + (base1 << shift); break; case TWO_PT_CALIB: + case TWO_PT_CALIB_NO_OFFSET: for (i = 0; i < priv->num_sensors; i++) p2[i] = (p2[i] + base2) << shift; fallthrough; case ONE_PT_CALIB2: + case ONE_PT_CALIB2_NO_OFFSET: for (i = 0; i < priv->num_sensors; i++) p1[i] = (p1[i] + base1) << shift; break; @@ -149,6 +151,18 @@ int tsens_read_calibration(struct tsens_priv *priv, int shift, u32 *p1, u32 *p2, } } + /* Apply calibration offset workaround except for _NO_OFFSET modes */ + switch (mode) { + case TWO_PT_CALIB: + for (i = 0; i < priv->num_sensors; i++) + p2[i] += priv->sensor[i].p2_calib_offset; + fallthrough; + case ONE_PT_CALIB2: + for (i = 0; i < priv->num_sensors; i++) + p1[i] += priv->sensor[i].p1_calib_offset; + break; + } + return mode; } @@ -254,7 +268,7 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1, if (!priv->sensor[i].slope) priv->sensor[i].slope = SLOPE_DEFAULT; - if (mode == TWO_PT_CALIB) { + if (mode == TWO_PT_CALIB || mode == TWO_PT_CALIB_NO_OFFSET) { /* * slope (m) = adc_code2 - adc_code1 (y2 - y1)/ * temp_120_degc - temp_30_degc (x2 - x1) diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h index dba9cd38f637..880be6be5c3f 100644 --- a/drivers/thermal/qcom/tsens.h +++ b/drivers/thermal/qcom/tsens.h @@ -10,6 +10,8 @@ #define ONE_PT_CALIB 0x1 #define ONE_PT_CALIB2 0x2 #define TWO_PT_CALIB 0x3 +#define ONE_PT_CALIB2_NO_OFFSET 0x6 +#define TWO_PT_CALIB_NO_OFFSET 0x7 #define CAL_DEGC_PT1 30 #define CAL_DEGC_PT2 120 #define SLOPE_FACTOR 1000 @@ -57,6 +59,8 @@ struct tsens_sensor { unsigned int hw_id; int slope; u32 status; + int p1_calib_offset; + int p2_calib_offset; }; /**
According to the msm-3.18 vendor kernel from Qualcomm, mdm9607 needs "correction factors" to adjust for additional offsets observed after the factory calibration values in the fuses [1, 2]. The fixed offsets should be applied unless there is a special calibration mode value that indicates that no offsets are needed [3]. Note that the new calibration mode values are called differently in this patch compared to the vendor kernel: - TSENS_TWO_POINT_CALIB_N_WA -> ONE_PT_CALIB2_NO_OFFSET - TSENS_TWO_POINT_CALIB_N_OFFSET_WA -> TWO_PT_CALIB_NO_OFFSET This is because close inspection of the calibration function [3] reveals that TSENS_TWO_POINT_CALIB_N_WA is actually a "one point" calibration because the if statements skip all "point2" related code for it. [1]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d9d2db1b82bf3f72f5de0803d55e6849eb5b671e [2]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d75aef53a760e8ff7bac54049d00c8b2ee1b193e [3]: https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LE.UM.4.3.2.r1-04200-9x07/drivers/thermal/msm-tsens.c#L2987-3136 Cc: Konrad Dybcio <konrad.dybcio@linaro.org> Fixes: a2149ab815fc ("thermal/drivers/qcom/tsens-v0_1: Add support for MDM9607") Signed-off-by: Stephan Gerhold <stephan.gerhold@kernkonzept.com> --- Changes in v3: None Changes in v2: New patch --- drivers/thermal/qcom/tsens-v0_1.c | 11 +++++++++++ drivers/thermal/qcom/tsens.c | 16 +++++++++++++++- drivers/thermal/qcom/tsens.h | 4 ++++ 3 files changed, 30 insertions(+), 1 deletion(-)