| Message ID | 20210217194011.22649-6-ansuelsmth@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v10,1/8] drivers: thermal: tsens: Add VER_0 tsens version | expand |
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 <ansuelsmth@gmail.com> > --- > 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 >
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 + * 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. + */ + 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); if (ret) return ret; + reg |= MEASURE_PERIOD; + if (priv->num_sensors > 1) reg |= mask | SLP_CLK_ENA | EN; else
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. Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> --- drivers/thermal/qcom/tsens-8960.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)