This patchset convert msm8960 to reg_filed, use int_common instead
of a custom function and fix wrong tsens get_temp function for msm8960.
Ipq8064 SoCs tsens driver is based on 8960 tsens driver. Ipq8064 needs
to be registered as a gcc child as the tsens regs on this platform are
shared with the controller.
This is based on work and code here
https://git.linaro.org/people/amit.kucheria/kernel.git/log/?h=wrk3/tsens-8960-breakage
v5:
* Conver driver to use reg_fiedl
* Use init_common
* Drop custom set_trip and set_interrupt
* Use common set_trip and set_interrupt
* Fix bad get_temp function
* Add missing hardcoded slope
v4:
* Fix compilation error and warning reported by the bot
v3:
* Change driver to register as child instead of use phandle
v2:
* Fix dt-bindings problems
Ansuel Smith (7):
drivers: thermal: tsens: Add VER_0 tsens version
drivers: thermal: tsens: Convert msm8960 to reg_field
drivers: thermal: tsens: Use init_common for msm8960
drivers: thermal: tsens: Fix wrong get_temp for msm8960
drivers: thermal: tsens: Change calib_backup name for msm8960
drivers: thermal: tsens: Add support for ipq8064-tsens
dt-bindings: thermal: tsens: Document ipq8064 bindings
.../bindings/thermal/qcom-tsens.yaml | 50 ++++-
drivers/thermal/qcom/tsens-8960.c | 172 +++++++++++-------
drivers/thermal/qcom/tsens.c | 156 +++++++++++++---
drivers/thermal/qcom/tsens.h | 7 +-
4 files changed, 284 insertions(+), 101 deletions(-)
--
2.27.0
VER_0 is used to describe device based on tsens version before v0.1.
These device are devices based on msm8960 for example apq8064 or
ipq806x.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/thermal/qcom/tsens.c | 160 +++++++++++++++++++++++++++--------
drivers/thermal/qcom/tsens.h | 7 +-
2 files changed, 132 insertions(+), 35 deletions(-)
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 9fe9a2b26705..78840c1bc5d2 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -516,6 +516,15 @@ static irqreturn_t tsens_irq_thread(int irq, void *data)
dev_dbg(priv->dev, "[%u] %s: no violation: %d\n",
hw_id, __func__, temp);
}
+
+ if (tsens_version(priv) < VER_0_1) {
+ /* Constraint: There is only 1 interrupt control register for all
+ * 11 temperature sensor. So monitoring more than 1 sensor based
+ * on interrupts will yield inconsistent result. To overcome this
+ * issue we will monitor only sensor 0 which is the master sensor.
+ */
+ break;
+ }
}
return IRQ_HANDLED;
@@ -531,6 +540,13 @@ static int tsens_set_trips(void *_sensor, int low, int high)
int high_val, low_val, cl_high, cl_low;
u32 hw_id = s->hw_id;
+ if (tsens_version(priv) < VER_0_1) {
+ /* Pre v0.1 IP had a single register for each type of interrupt
+ * and thresholds
+ */
+ hw_id = 0;
+ }
+
dev_dbg(dev, "[%u] %s: proposed thresholds: (%d:%d)\n",
hw_id, __func__, low, high);
@@ -550,6 +566,12 @@ static int tsens_set_trips(void *_sensor, int low, int high)
tsens_set_interrupt(priv, hw_id, LOWER, true);
tsens_set_interrupt(priv, hw_id, UPPER, true);
+ /* VER_0 require to set MIN and MAX THRESH */
+ if (tsens_version(priv) < VER_0_1) {
+ regmap_field_write(priv->rf[MIN_THRESH_0], 0);
+ regmap_field_write(priv->rf[MAX_THRESH_0], 120000);
+ }
+
spin_unlock_irqrestore(&priv->ul_lock, flags);
dev_dbg(dev, "[%u] %s: (%d:%d)->(%d:%d)\n",
@@ -584,18 +606,21 @@ int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp)
u32 valid;
int ret;
- ret = regmap_field_read(priv->rf[valid_idx], &valid);
- if (ret)
- return ret;
- while (!valid) {
- /* Valid bit is 0 for 6 AHB clock cycles.
- * At 19.2MHz, 1 AHB clock is ~60ns.
- * We should enter this loop very, very rarely.
- */
- ndelay(400);
+ /* VER_0 doesn't have VALID bit */
+ if (tsens_version(priv) >= VER_0_1) {
ret = regmap_field_read(priv->rf[valid_idx], &valid);
if (ret)
return ret;
+ while (!valid) {
+ /* Valid bit is 0 for 6 AHB clock cycles.
+ * At 19.2MHz, 1 AHB clock is ~60ns.
+ * We should enter this loop very, very rarely.
+ */
+ ndelay(400);
+ ret = regmap_field_read(priv->rf[valid_idx], &valid);
+ if (ret)
+ return ret;
+ }
}
/* Valid bit is set, OK to read the temperature */
@@ -765,8 +790,8 @@ int __init init_common(struct tsens_priv *priv)
if (tsens_version(priv) > VER_0_1) {
for (i = VER_MAJOR; i <= VER_STEP; i++) {
- priv->rf[i] = devm_regmap_field_alloc(dev, priv->srot_map,
- priv->fields[i]);
+ priv->rf[i] = devm_regmap_field_alloc(
+ dev, priv->srot_map, priv->fields[i]);
if (IS_ERR(priv->rf[i]))
return PTR_ERR(priv->rf[i]);
}
@@ -775,12 +800,80 @@ int __init init_common(struct tsens_priv *priv)
goto err_put_device;
}
- priv->rf[TSENS_EN] = devm_regmap_field_alloc(dev, priv->srot_map,
- priv->fields[TSENS_EN]);
- if (IS_ERR(priv->rf[TSENS_EN])) {
- ret = PTR_ERR(priv->rf[TSENS_EN]);
- goto err_put_device;
+ if (tsens_version(priv) >= VER_0_1) {
+ priv->rf[TSENS_EN] = devm_regmap_field_alloc(
+ dev, priv->srot_map, priv->fields[TSENS_EN]);
+ if (IS_ERR(priv->rf[TSENS_EN])) {
+ ret = PTR_ERR(priv->rf[TSENS_EN]);
+ goto err_put_device;
+ }
+
+ priv->rf[SENSOR_EN] = devm_regmap_field_alloc(
+ dev, priv->srot_map, priv->fields[SENSOR_EN]);
+ if (IS_ERR(priv->rf[SENSOR_EN])) {
+ ret = PTR_ERR(priv->rf[SENSOR_EN]);
+ goto err_put_device;
+ }
+ priv->rf[INT_EN] = devm_regmap_field_alloc(
+ dev, priv->tm_map, priv->fields[INT_EN]);
+ if (IS_ERR(priv->rf[INT_EN])) {
+ ret = PTR_ERR(priv->rf[INT_EN]);
+ goto err_put_device;
+ }
+ } else {
+ priv->rf[TSENS_EN] = devm_regmap_field_alloc(
+ dev, priv->tm_map, priv->fields[TSENS_EN]);
+ if (IS_ERR(priv->rf[TSENS_EN])) {
+ ret = PTR_ERR(priv->rf[TSENS_EN]);
+ goto err_put_device;
+ }
+
+ priv->rf[TSENS_SW_RST] = devm_regmap_field_alloc(
+ dev, priv->tm_map, priv->fields[TSENS_EN]);
+ if (IS_ERR(priv->rf[TSENS_EN])) {
+ ret = PTR_ERR(priv->rf[TSENS_EN]);
+ goto err_put_device;
+ }
+
+ /* enable TSENS */
+ regmap_field_write(priv->rf[TSENS_EN], 1);
+
+ priv->rf[LOW_INT_CLEAR_0] = devm_regmap_field_alloc(
+ dev, priv->tm_map, priv->fields[LOW_INT_CLEAR_0]);
+ if (IS_ERR(priv->rf[LOW_INT_CLEAR_0])) {
+ ret = PTR_ERR(priv->rf[LOW_INT_CLEAR_0]);
+ goto err_put_device;
+ }
+
+ priv->rf[UP_INT_CLEAR_0] = devm_regmap_field_alloc(
+ dev, priv->tm_map, priv->fields[UP_INT_CLEAR_0]);
+ if (IS_ERR(priv->rf[UP_INT_CLEAR_0])) {
+ ret = PTR_ERR(priv->rf[UP_INT_CLEAR_0]);
+ goto err_put_device;
+ }
+
+ priv->rf[MIN_THRESH_0] = devm_regmap_field_alloc(
+ dev, priv->tm_map, priv->fields[MIN_THRESH_0]);
+ if (IS_ERR(priv->rf[MIN_THRESH_0])) {
+ ret = PTR_ERR(priv->rf[MIN_THRESH_0]);
+ goto err_put_device;
+ }
+
+ priv->rf[MAX_THRESH_0] = devm_regmap_field_alloc(
+ dev, priv->tm_map, priv->fields[MAX_THRESH_0]);
+ if (IS_ERR(priv->rf[MAX_THRESH_0])) {
+ ret = PTR_ERR(priv->rf[MAX_THRESH_0]);
+ goto err_put_device;
+ }
+
+ priv->rf[TRDY] = devm_regmap_field_alloc(dev, priv->tm_map,
+ priv->fields[TRDY]);
+ if (IS_ERR(priv->rf[TRDY])) {
+ ret = PTR_ERR(priv->rf[TRDY]);
+ goto err_put_device;
+ }
}
+
ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
if (ret)
goto err_put_device;
@@ -790,19 +883,6 @@ int __init init_common(struct tsens_priv *priv)
goto err_put_device;
}
- priv->rf[SENSOR_EN] = devm_regmap_field_alloc(dev, priv->srot_map,
- priv->fields[SENSOR_EN]);
- if (IS_ERR(priv->rf[SENSOR_EN])) {
- ret = PTR_ERR(priv->rf[SENSOR_EN]);
- goto err_put_device;
- }
- priv->rf[INT_EN] = devm_regmap_field_alloc(dev, priv->tm_map,
- priv->fields[INT_EN]);
- if (IS_ERR(priv->rf[INT_EN])) {
- ret = PTR_ERR(priv->rf[INT_EN]);
- goto err_put_device;
- }
-
/* This loop might need changes if enum regfield_ids is reordered */
for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
for (i = 0; i < priv->feat->max_sensors; i++) {
@@ -856,7 +936,11 @@ int __init init_common(struct tsens_priv *priv)
}
spin_lock_init(&priv->ul_lock);
- tsens_enable_irq(priv);
+
+ /* VER_0 interrupt doesn't need to be enabled */
+ if (tsens_version(priv) >= VER_0_1)
+ tsens_enable_irq(priv);
+
tsens_debug_init(op);
err_put_device:
@@ -952,10 +1036,18 @@ static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
if (irq == -ENXIO)
ret = 0;
} else {
- ret = devm_request_threaded_irq(&pdev->dev, irq,
- NULL, thread_fn,
- IRQF_ONESHOT,
- dev_name(&pdev->dev), priv);
+ /* VER_0 have a different interrupt type */
+ if (tsens_version(priv) > VER_0)
+ ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+ thread_fn, IRQF_ONESHOT,
+ dev_name(&pdev->dev),
+ priv);
+ else
+ ret = devm_request_threaded_irq(&pdev->dev, irq,
+ thread_fn, NULL,
+ IRQF_TRIGGER_RISING,
+ dev_name(&pdev->dev),
+ priv);
if (ret)
dev_err(&pdev->dev, "%s: failed to get irq\n",
__func__);
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index 59d01162c66a..f1120791737c 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -25,7 +25,8 @@ struct tsens_priv;
/* IP version numbers in ascending order */
enum tsens_ver {
- VER_0_1 = 0,
+ VER_0 = 0,
+ VER_0_1,
VER_1_X,
VER_2_X,
};
@@ -441,6 +442,10 @@ enum regfield_ids {
CRIT_THRESH_14,
CRIT_THRESH_15,
+ /* VER_0 MIN MAX THRESH */
+ MIN_THRESH_0,
+ MAX_THRESH_0,
+
/* WATCHDOG */
WDOG_BARK_STATUS,
WDOG_BARK_CLEAR,
--
2.27.0
Covert msm9860 driver to reg_filed to use the init_common
function.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/thermal/qcom/tsens-8960.c | 74 +++++++++++++++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
index 2a28a5af209e..45cd0cdff2f5 100644
--- a/drivers/thermal/qcom/tsens-8960.c
+++ b/drivers/thermal/qcom/tsens-8960.c
@@ -56,6 +56,18 @@
#define TRDY_MASK BIT(7)
#define TIMEOUT_US 100
+#define S0_STATUS_OFF 0x3628
+#define S1_STATUS_OFF 0x362c
+#define S2_STATUS_OFF 0x3630
+#define S3_STATUS_OFF 0x3634
+#define S4_STATUS_OFF 0x3638
+#define S5_STATUS_OFF 0x3664 /* Sensors 5 thru 10 found on apq8064/msm8960 */
+#define S6_STATUS_OFF 0x3668
+#define S7_STATUS_OFF 0x366c
+#define S8_STATUS_OFF 0x3670
+#define S9_STATUS_OFF 0x3674
+#define S10_STATUS_OFF 0x3678
+
static int suspend_8960(struct tsens_priv *priv)
{
int ret;
@@ -269,6 +281,66 @@ static int get_temp_8960(const struct tsens_sensor *s, int *temp)
return -ETIMEDOUT;
}
+static struct tsens_features tsens_8960_feat = {
+ .ver_major = VER_0,
+ .crit_int = 0,
+ .adc = 1,
+ .srot_split = 0,
+ .max_sensors = 11,
+};
+
+static const struct reg_field tsens_8960_regfields[MAX_REGFIELDS] = {
+ /* ----- SROT ------ */
+ /* No VERSION information */
+
+ /* CNTL */
+ [TSENS_EN] = REG_FIELD(CNTL_ADDR, 0, 0),
+ [TSENS_SW_RST] = REG_FIELD(CNTL_ADDR, 1, 1),
+ /* 8960 has 5 sensors, 8660 has 11, we only handle 5 */
+ [SENSOR_EN] = REG_FIELD(CNTL_ADDR, 3, 7),
+
+ /* ----- TM ------ */
+ /* INTERRUPT ENABLE */
+ // [INT_EN] = REG_FIELD(TM_INT_EN_OFF, 0, 0),
+
+ /* Single UPPER/LOWER TEMPERATURE THRESHOLD for all sensors */
+ [LOW_THRESH_0] = REG_FIELD(THRESHOLD_ADDR, 0, 7),
+ [UP_THRESH_0] = REG_FIELD(THRESHOLD_ADDR, 8, 15),
+ [MIN_THRESH_0] = REG_FIELD(THRESHOLD_ADDR, 16, 23),
+ [MAX_THRESH_0] = REG_FIELD(THRESHOLD_ADDR, 24, 31),
+
+ // /* UPPER/LOWER INTERRUPT [CLEAR/STATUS] */
+ // /* 1 == clear, 0 == normal operation */
+ [LOW_INT_CLEAR_0] = REG_FIELD(CNTL_ADDR, 9, 9),
+ [UP_INT_CLEAR_0] = REG_FIELD(CNTL_ADDR, 10, 10),
+
+ /* NO CRITICAL INTERRUPT SUPPORT on 8960 */
+
+ /* Sn_STATUS */
+ [LAST_TEMP_0] = REG_FIELD(S0_STATUS_OFF, 0, 7),
+ [LAST_TEMP_1] = REG_FIELD(S1_STATUS_OFF, 0, 7),
+ [LAST_TEMP_2] = REG_FIELD(S2_STATUS_OFF, 0, 7),
+ [LAST_TEMP_3] = REG_FIELD(S3_STATUS_OFF, 0, 7),
+ [LAST_TEMP_4] = REG_FIELD(S4_STATUS_OFF, 0, 7),
+ [LAST_TEMP_5] = REG_FIELD(S5_STATUS_OFF, 0, 7),
+ [LAST_TEMP_6] = REG_FIELD(S6_STATUS_OFF, 0, 7),
+ [LAST_TEMP_7] = REG_FIELD(S7_STATUS_OFF, 0, 7),
+ [LAST_TEMP_8] = REG_FIELD(S8_STATUS_OFF, 0, 7),
+ [LAST_TEMP_9] = REG_FIELD(S9_STATUS_OFF, 0, 7),
+ [LAST_TEMP_10] = REG_FIELD(S10_STATUS_OFF, 0, 7),
+
+ /* No VALID field on 8960 */
+ /* TSENS_INT_STATUS bits: 1 == threshold violated */
+ [MIN_STATUS_0] = REG_FIELD(INT_STATUS_ADDR, 0, 0),
+ [LOWER_STATUS_0] = REG_FIELD(INT_STATUS_ADDR, 1, 1),
+ [UPPER_STATUS_0] = REG_FIELD(INT_STATUS_ADDR, 2, 2),
+ /* No CRITICAL field on 8960 */
+ [MAX_STATUS_0] = REG_FIELD(INT_STATUS_ADDR, 3, 3),
+
+ /* TRDY: 1=ready, 0=in progress */
+ [TRDY] = REG_FIELD(INT_STATUS_ADDR, 7, 7),
+};
+
static const struct tsens_ops ops_8960 = {
.init = init_8960,
.calibrate = calibrate_8960,
@@ -282,4 +354,6 @@ static const struct tsens_ops ops_8960 = {
struct tsens_plat_data data_8960 = {
.num_sensors = 11,
.ops = &ops_8960,
+ .feat = &tsens_8960_feat,
+ .fields = tsens_8960_regfields,
};
--
2.27.0
Use init_common and drop custom init for msm8960.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/thermal/qcom/tsens-8960.c | 53 +------------------------------
1 file changed, 1 insertion(+), 52 deletions(-)
diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
index 45cd0cdff2f5..d545cf9888fd 100644
--- a/drivers/thermal/qcom/tsens-8960.c
+++ b/drivers/thermal/qcom/tsens-8960.c
@@ -51,7 +51,6 @@
#define MIN_LIMIT_TH 0x0
#define MAX_LIMIT_TH 0xff
-#define S0_STATUS_ADDR 0x3628
#define INT_STATUS_ADDR 0x363c
#define TRDY_MASK BIT(7)
#define TIMEOUT_US 100
@@ -174,56 +173,6 @@ static void disable_8960(struct tsens_priv *priv)
regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl);
}
-static int init_8960(struct tsens_priv *priv)
-{
- int ret, i;
- u32 reg_cntl;
-
- priv->tm_map = dev_get_regmap(priv->dev, NULL);
- if (!priv->tm_map)
- return -ENODEV;
-
- /*
- * The status registers for each sensor are discontiguous
- * because some SoCs have 5 sensors while others have more
- * but the control registers stay in the same place, i.e
- * directly after the first 5 status registers.
- */
- for (i = 0; i < priv->num_sensors; i++) {
- if (i >= 5)
- priv->sensor[i].status = S0_STATUS_ADDR + 40;
- priv->sensor[i].status += i * 4;
- }
-
- reg_cntl = SW_RST;
- ret = regmap_update_bits(priv->tm_map, CNTL_ADDR, SW_RST, reg_cntl);
- if (ret)
- return ret;
-
- if (priv->num_sensors > 1) {
- reg_cntl |= SLP_CLK_ENA | (MEASURE_PERIOD << 18);
- reg_cntl &= ~SW_RST;
- ret = regmap_update_bits(priv->tm_map, CONFIG_ADDR,
- CONFIG_MASK, CONFIG);
- } else {
- reg_cntl |= SLP_CLK_ENA_8660 | (MEASURE_PERIOD << 16);
- reg_cntl &= ~CONFIG_MASK_8660;
- reg_cntl |= CONFIG_8660 << CONFIG_SHIFT_8660;
- }
-
- reg_cntl |= GENMASK(priv->num_sensors - 1, 0) << SENSOR0_SHIFT;
- ret = regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl);
- if (ret)
- return ret;
-
- reg_cntl |= EN;
- ret = regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl);
- if (ret)
- return ret;
-
- return 0;
-}
-
static int calibrate_8960(struct tsens_priv *priv)
{
int i;
@@ -342,7 +291,7 @@ static const struct reg_field tsens_8960_regfields[MAX_REGFIELDS] = {
};
static const struct tsens_ops ops_8960 = {
- .init = init_8960,
+ .init = init_common,
.calibrate = calibrate_8960,
.get_temp = get_temp_8960,
.enable = enable_8960,
--
2.27.0
msm8960 based tsens have an hardcoded slope. Fix the calibrate function
with the new added slope, change code_to_mdegC to use slope and conver
get_temp to use reg_field.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/thermal/qcom/tsens-8960.c | 43 +++++++++++++++++++++++--------
1 file changed, 32 insertions(+), 11 deletions(-)
diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
index d545cf9888fd..42ab8f79bf5b 100644
--- a/drivers/thermal/qcom/tsens-8960.c
+++ b/drivers/thermal/qcom/tsens-8960.c
@@ -67,6 +67,14 @@
#define S9_STATUS_OFF 0x3674
#define S10_STATUS_OFF 0x3678
+#define TSENS_FACTOR 1
+
+u32 tsens_msm8960_slope[] = {
+ 1176, 1176, 1154, 1176,
+ 1111, 1132, 1132, 1199,
+ 1132, 1199, 1132
+ };
+
static int suspend_8960(struct tsens_priv *priv)
{
int ret;
@@ -187,8 +195,10 @@ 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 < num_read; i++, s++) {
+ s->slope = tsens_msm8960_slope[i];
+ s->offset = CAL_MDEGC - (data[i] * s->slope);
+ }
kfree(data);
@@ -198,32 +208,43 @@ static int calibrate_8960(struct tsens_priv *priv)
/* 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;
+ int num, degc;
+
+ num = (adc_code * s->slope) + s->offset;
- slope = thermal_zone_get_slope(s->tzd);
- offset = CAL_MDEGC - slope * s->offset;
+ if (num == 0)
+ degc = num;
+ else if (num > 0)
+ degc = (num + TSENS_FACTOR / 2)
+ / TSENS_FACTOR;
+ else
+ degc = (num - TSENS_FACTOR / 2)
+ / TSENS_FACTOR;
- return adc_code * slope + offset;
+ return degc;
}
static int get_temp_8960(const struct tsens_sensor *s, int *temp)
{
int ret;
- u32 code, trdy;
+ u32 last_temp = 0, 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);
+ ret = regmap_field_read(priv->rf[TRDY], &trdy);
if (ret)
return ret;
- if (!(trdy & TRDY_MASK))
+ if (!trdy)
continue;
- ret = regmap_read(priv->tm_map, s->status, &code);
+
+ ret = regmap_field_read(priv->rf[LAST_TEMP_0 + s->hw_id], &last_temp);
if (ret)
return ret;
- *temp = code_to_mdegC(code, s);
+
+ *temp = code_to_mdegC(last_temp, s);
+
return 0;
} while (time_before(jiffies, timeout));
--
2.27.0
Add support for tsens present in ipq806x SoCs based on generic msm8960
tsens driver.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/thermal/qcom/tsens.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 78840c1bc5d2..5eb036767e8d 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -991,6 +991,9 @@ static SIMPLE_DEV_PM_OPS(tsens_pm_ops, tsens_suspend, tsens_resume);
static const struct of_device_id tsens_table[] = {
{
+ .compatible = "qcom,ipq8064-tsens",
+ .data = &data_8960,
+ }, {
.compatible = "qcom,msm8916-tsens",
.data = &data_8916,
}, {
--
2.27.0
Document the use of bindings used for msm8960 tsens based devices.
msm8960 use the same gcc regs and is set as a child of the qcom gcc.
Signed-off-by: Ansuel Smith <[email protected]>
---
.../bindings/thermal/qcom-tsens.yaml | 50 ++++++++++++++++---
1 file changed, 43 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
index d7be931b42d2..9d480e3943a2 100644
--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
@@ -19,6 +19,11 @@ description: |
properties:
compatible:
oneOf:
+ - description: msm9860 TSENS based
+ items:
+ - enum:
+ - qcom,ipq8064-tsens
+
- description: v0.1 of TSENS
items:
- enum:
@@ -85,12 +90,18 @@ properties:
Number of cells required to uniquely identify the thermal sensors. Since
we have multiple sensors this is set to 1
+required:
+ - compatible
+ - interrupts
+ - "#thermal-sensor-cells"
+
allOf:
- if:
properties:
compatible:
contains:
enum:
+ - qcom,ipq8064-tsens
- qcom,msm8916-tsens
- qcom,msm8974-tsens
- qcom,msm8976-tsens
@@ -111,17 +122,42 @@ allOf:
interrupt-names:
minItems: 2
-required:
- - compatible
- - reg
- - "#qcom,sensors"
- - interrupts
- - interrupt-names
- - "#thermal-sensor-cells"
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,tsens-v0_1
+ - qcom,tsens-v1
+ - qcom,tsens-v2
+
+ then:
+ required:
+ - reg
+ - interrupt-names
+ - "#qcom,sensors"
additionalProperties: false
examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ // Example msm9860 based SoC (ipq8064):
+ gcc: clock-controller {
+
+ /* ... */
+
+ tsens: thermal-sensor {
+ compatible = "qcom,ipq8064-tsens";
+
+ nvmem-cells = <&tsens_calib>, <&tsens_calsel>;
+ nvmem-cell-names = "calib", "calib_sel";
+ interrupts = <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH>;
+
+ #thermal-sensor-cells = <1>;
+ };
+ };
+
- |
#include <dt-bindings/interrupt-controller/arm-gic.h>
// Example 1 (legacy: for pre v1 IP):
--
2.27.0
Follow standard naming for calib secondary rom and change calib_backup
to tsens_calsel.
Signed-off-by: Ansuel Smith <[email protected]>
---
drivers/thermal/qcom/tsens-8960.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
index 42ab8f79bf5b..b286641003aa 100644
--- a/drivers/thermal/qcom/tsens-8960.c
+++ b/drivers/thermal/qcom/tsens-8960.c
@@ -191,7 +191,7 @@ static int calibrate_8960(struct tsens_priv *priv)
data = qfprom_read(priv->dev, "calib");
if (IS_ERR(data))
- data = qfprom_read(priv->dev, "calib_backup");
+ data = qfprom_read(priv->dev, "tsens_calsel");
if (IS_ERR(data))
return PTR_ERR(data);
--
2.27.0
On Sat, 25 Jul 2020 20:14:03 +0200, Ansuel Smith wrote:
> Document the use of bindings used for msm8960 tsens based devices.
> msm8960 use the same gcc regs and is set as a child of the qcom gcc.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> .../bindings/thermal/qcom-tsens.yaml | 50 ++++++++++++++++---
> 1 file changed, 43 insertions(+), 7 deletions(-)
>
Reviewed-by: Rob Herring <[email protected]>
On Sat, Jul 25, 2020 at 11:44 PM Ansuel Smith <[email protected]> wrote:
>
> VER_0 is used to describe device based on tsens version before v0.1.
> These device are devices based on msm8960 for example apq8064 or
> ipq806x.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/thermal/qcom/tsens.c | 160 +++++++++++++++++++++++++++--------
> drivers/thermal/qcom/tsens.h | 7 +-
> 2 files changed, 132 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 9fe9a2b26705..78840c1bc5d2 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -516,6 +516,15 @@ static irqreturn_t tsens_irq_thread(int irq, void *data)
> dev_dbg(priv->dev, "[%u] %s: no violation: %d\n",
> hw_id, __func__, temp);
> }
> +
> + if (tsens_version(priv) < VER_0_1) {
> + /* Constraint: There is only 1 interrupt control register for all
> + * 11 temperature sensor. So monitoring more than 1 sensor based
> + * on interrupts will yield inconsistent result. To overcome this
> + * issue we will monitor only sensor 0 which is the master sensor.
> + */
> + break;
> + }
> }
>
> return IRQ_HANDLED;
> @@ -531,6 +540,13 @@ static int tsens_set_trips(void *_sensor, int low, int high)
> int high_val, low_val, cl_high, cl_low;
> u32 hw_id = s->hw_id;
>
> + if (tsens_version(priv) < VER_0_1) {
> + /* Pre v0.1 IP had a single register for each type of interrupt
> + * and thresholds
> + */
> + hw_id = 0;
> + }
> +
> dev_dbg(dev, "[%u] %s: proposed thresholds: (%d:%d)\n",
> hw_id, __func__, low, high);
>
> @@ -550,6 +566,12 @@ static int tsens_set_trips(void *_sensor, int low, int high)
> tsens_set_interrupt(priv, hw_id, LOWER, true);
> tsens_set_interrupt(priv, hw_id, UPPER, true);
>
> + /* VER_0 require to set MIN and MAX THRESH */
> + if (tsens_version(priv) < VER_0_1) {
> + regmap_field_write(priv->rf[MIN_THRESH_0], 0);
> + regmap_field_write(priv->rf[MAX_THRESH_0], 120000);
Since MIN_THRESH_0 and MAX_THRESH_0 are the only two threshold on pre
0.1 IP, just (mis)use the already predefined LOW_THRESH_0 and
UP_THRESH_0 in regfield_ids in init_common() below? Then we won't need
this special casing here. All the special casing ugliness can then
stay in init_common() with comments.
> + }
> +
> spin_unlock_irqrestore(&priv->ul_lock, flags);
>
> dev_dbg(dev, "[%u] %s: (%d:%d)->(%d:%d)\n",
> @@ -584,18 +606,21 @@ int get_temp_tsens_valid(const struct tsens_sensor *s, int *temp)
> u32 valid;
> int ret;
>
> - ret = regmap_field_read(priv->rf[valid_idx], &valid);
> - if (ret)
> - return ret;
> - while (!valid) {
> - /* Valid bit is 0 for 6 AHB clock cycles.
> - * At 19.2MHz, 1 AHB clock is ~60ns.
> - * We should enter this loop very, very rarely.
> - */
> - ndelay(400);
> + /* VER_0 doesn't have VALID bit */
> + if (tsens_version(priv) >= VER_0_1) {
Since 8960 needs a custom get_temp function, is this change really needed?
> ret = regmap_field_read(priv->rf[valid_idx], &valid);
> if (ret)
> return ret;
> + while (!valid) {
> + /* Valid bit is 0 for 6 AHB clock cycles.
> + * At 19.2MHz, 1 AHB clock is ~60ns.
> + * We should enter this loop very, very rarely.
> + */
> + ndelay(400);
> + ret = regmap_field_read(priv->rf[valid_idx], &valid);
> + if (ret)
> + return ret;
> + }
> }
>
> /* Valid bit is set, OK to read the temperature */
> @@ -765,8 +790,8 @@ int __init init_common(struct tsens_priv *priv)
>
> if (tsens_version(priv) > VER_0_1) {
> for (i = VER_MAJOR; i <= VER_STEP; i++) {
> - priv->rf[i] = devm_regmap_field_alloc(dev, priv->srot_map,
> - priv->fields[i]);
> + priv->rf[i] = devm_regmap_field_alloc(
> + dev, priv->srot_map, priv->fields[i]);
This doesn't change any code, simply reformats the code to 80 columns.
Avoid adding such lines to other features, makes it harder to review
changes.
Please ignore the 80 column warning here and elsewhere below when it
is only going over by a few characters. Run checkpatch on your patches
which has now increased the number of columns to 100 now.
> if (IS_ERR(priv->rf[i]))
> return PTR_ERR(priv->rf[i]);
> }
> @@ -775,12 +800,80 @@ int __init init_common(struct tsens_priv *priv)
> goto err_put_device;
> }
>
> - priv->rf[TSENS_EN] = devm_regmap_field_alloc(dev, priv->srot_map,
> - priv->fields[TSENS_EN]);
> - if (IS_ERR(priv->rf[TSENS_EN])) {
> - ret = PTR_ERR(priv->rf[TSENS_EN]);
> - goto err_put_device;
> + if (tsens_version(priv) >= VER_0_1) {
> + priv->rf[TSENS_EN] = devm_regmap_field_alloc(
> + dev, priv->srot_map, priv->fields[TSENS_EN]);
> + if (IS_ERR(priv->rf[TSENS_EN])) {
> + ret = PTR_ERR(priv->rf[TSENS_EN]);
> + goto err_put_device;
> + }
> +
> + priv->rf[SENSOR_EN] = devm_regmap_field_alloc(
> + dev, priv->srot_map, priv->fields[SENSOR_EN]);
> + if (IS_ERR(priv->rf[SENSOR_EN])) {
> + ret = PTR_ERR(priv->rf[SENSOR_EN]);
> + goto err_put_device;
> + }
> + priv->rf[INT_EN] = devm_regmap_field_alloc(
> + dev, priv->tm_map, priv->fields[INT_EN]);
> + if (IS_ERR(priv->rf[INT_EN])) {
> + ret = PTR_ERR(priv->rf[INT_EN]);
> + goto err_put_device;
> + }
> + } else {
Let's not create two big sections with if-else for 8960 and everything
else. For example, what is wrong with using common code for TSENS_EN?
If the concern is memory wasted trying to allocate fields not present
on this older platform, perhaps consider adding a check in the loop to
break early in case of 8960?
> + priv->rf[TSENS_EN] = devm_regmap_field_alloc(
> + dev, priv->tm_map, priv->fields[TSENS_EN]);
> + if (IS_ERR(priv->rf[TSENS_EN])) {
> + ret = PTR_ERR(priv->rf[TSENS_EN]);
> + goto err_put_device;
> + }
> +
> + priv->rf[TSENS_SW_RST] = devm_regmap_field_alloc(
> + dev, priv->tm_map, priv->fields[TSENS_EN]);
> + if (IS_ERR(priv->rf[TSENS_EN])) {
> + ret = PTR_ERR(priv->rf[TSENS_EN]);
> + goto err_put_device;
> + }
> +
> + /* enable TSENS */
> + regmap_field_write(priv->rf[TSENS_EN], 1);
> +
> + priv->rf[LOW_INT_CLEAR_0] = devm_regmap_field_alloc(
> + dev, priv->tm_map, priv->fields[LOW_INT_CLEAR_0]);
> + if (IS_ERR(priv->rf[LOW_INT_CLEAR_0])) {
> + ret = PTR_ERR(priv->rf[LOW_INT_CLEAR_0]);
> + goto err_put_device;
> + }
> +
> + priv->rf[UP_INT_CLEAR_0] = devm_regmap_field_alloc(
> + dev, priv->tm_map, priv->fields[UP_INT_CLEAR_0]);
> + if (IS_ERR(priv->rf[UP_INT_CLEAR_0])) {
> + ret = PTR_ERR(priv->rf[UP_INT_CLEAR_0]);
> + goto err_put_device;
> + }
> +
> + priv->rf[MIN_THRESH_0] = devm_regmap_field_alloc(
> + dev, priv->tm_map, priv->fields[MIN_THRESH_0]);
> + if (IS_ERR(priv->rf[MIN_THRESH_0])) {
> + ret = PTR_ERR(priv->rf[MIN_THRESH_0]);
> + goto err_put_device;
> + }
> +
> + priv->rf[MAX_THRESH_0] = devm_regmap_field_alloc(
> + dev, priv->tm_map, priv->fields[MAX_THRESH_0]);
> + if (IS_ERR(priv->rf[MAX_THRESH_0])) {
> + ret = PTR_ERR(priv->rf[MAX_THRESH_0]);
> + goto err_put_device;
> + }
> +
> + priv->rf[TRDY] = devm_regmap_field_alloc(dev, priv->tm_map,
> + priv->fields[TRDY]);
> + if (IS_ERR(priv->rf[TRDY])) {
> + ret = PTR_ERR(priv->rf[TRDY]);
> + goto err_put_device;
> + }
> }
> +
> ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
> if (ret)
> goto err_put_device;
> @@ -790,19 +883,6 @@ int __init init_common(struct tsens_priv *priv)
> goto err_put_device;
> }
>
> - priv->rf[SENSOR_EN] = devm_regmap_field_alloc(dev, priv->srot_map,
> - priv->fields[SENSOR_EN]);
> - if (IS_ERR(priv->rf[SENSOR_EN])) {
> - ret = PTR_ERR(priv->rf[SENSOR_EN]);
> - goto err_put_device;
> - }
> - priv->rf[INT_EN] = devm_regmap_field_alloc(dev, priv->tm_map,
> - priv->fields[INT_EN]);
> - if (IS_ERR(priv->rf[INT_EN])) {
> - ret = PTR_ERR(priv->rf[INT_EN]);
> - goto err_put_device;
> - }
> -
> /* This loop might need changes if enum regfield_ids is reordered */
> for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
> for (i = 0; i < priv->feat->max_sensors; i++) {
> @@ -856,7 +936,11 @@ int __init init_common(struct tsens_priv *priv)
> }
>
> spin_lock_init(&priv->ul_lock);
> - tsens_enable_irq(priv);
> +
> + /* VER_0 interrupt doesn't need to be enabled */
> + if (tsens_version(priv) >= VER_0_1)
> + tsens_enable_irq(priv);
> +
> tsens_debug_init(op);
>
> err_put_device:
> @@ -952,10 +1036,18 @@ static int tsens_register_irq(struct tsens_priv *priv, char *irqname,
> if (irq == -ENXIO)
> ret = 0;
> } else {
> - ret = devm_request_threaded_irq(&pdev->dev, irq,
> - NULL, thread_fn,
> - IRQF_ONESHOT,
> - dev_name(&pdev->dev), priv);
> + /* VER_0 have a different interrupt type */
Say how it is different.
> + if (tsens_version(priv) > VER_0)
> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> + thread_fn, IRQF_ONESHOT,
> + dev_name(&pdev->dev),
> + priv);
> + else
> + ret = devm_request_threaded_irq(&pdev->dev, irq,
> + thread_fn, NULL,
> + IRQF_TRIGGER_RISING,
> + dev_name(&pdev->dev),
> + priv);
> if (ret)
> dev_err(&pdev->dev, "%s: failed to get irq\n",
> __func__);
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index 59d01162c66a..f1120791737c 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -25,7 +25,8 @@ struct tsens_priv;
>
> /* IP version numbers in ascending order */
> enum tsens_ver {
> - VER_0_1 = 0,
> + VER_0 = 0,
> + VER_0_1,
> VER_1_X,
> VER_2_X,
> };
> @@ -441,6 +442,10 @@ enum regfield_ids {
> CRIT_THRESH_14,
> CRIT_THRESH_15,
>
> + /* VER_0 MIN MAX THRESH */
> + MIN_THRESH_0,
> + MAX_THRESH_0,
> +
> /* WATCHDOG */
> WDOG_BARK_STATUS,
> WDOG_BARK_CLEAR,
> --
> 2.27.0
>
On Sat, Jul 25, 2020 at 11:44 PM Ansuel Smith <[email protected]> wrote:
>
> Covert msm9860 driver to reg_filed to use the init_common
typo: field
> function.
>
> Signed-off-by: Ansuel Smith <[email protected]>
> ---
> drivers/thermal/qcom/tsens-8960.c | 74 +++++++++++++++++++++++++++++++
> 1 file changed, 74 insertions(+)
>
> diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
> index 2a28a5af209e..45cd0cdff2f5 100644
> --- a/drivers/thermal/qcom/tsens-8960.c
> +++ b/drivers/thermal/qcom/tsens-8960.c
> @@ -56,6 +56,18 @@
> #define TRDY_MASK BIT(7)
> #define TIMEOUT_US 100
>
> +#define S0_STATUS_OFF 0x3628
> +#define S1_STATUS_OFF 0x362c
> +#define S2_STATUS_OFF 0x3630
> +#define S3_STATUS_OFF 0x3634
> +#define S4_STATUS_OFF 0x3638
> +#define S5_STATUS_OFF 0x3664 /* Sensors 5 thru 10 found on apq8064/msm8960 */
Run checkpatch and fix spelling. :-) Or just say 'sensor 5-10'
> +#define S6_STATUS_OFF 0x3668
> +#define S7_STATUS_OFF 0x366c
> +#define S8_STATUS_OFF 0x3670
> +#define S9_STATUS_OFF 0x3674
> +#define S10_STATUS_OFF 0x3678
> +
> static int suspend_8960(struct tsens_priv *priv)
> {
> int ret;
> @@ -269,6 +281,66 @@ static int get_temp_8960(const struct tsens_sensor *s, int *temp)
> return -ETIMEDOUT;
> }
>
> +static struct tsens_features tsens_8960_feat = {
> + .ver_major = VER_0,
> + .crit_int = 0,
> + .adc = 1,
> + .srot_split = 0,
> + .max_sensors = 11,
Align the equal to like in other files please.
> +};
> +
> +static const struct reg_field tsens_8960_regfields[MAX_REGFIELDS] = {
> + /* ----- SROT ------ */
> + /* No VERSION information */
> +
> + /* CNTL */
> + [TSENS_EN] = REG_FIELD(CNTL_ADDR, 0, 0),
> + [TSENS_SW_RST] = REG_FIELD(CNTL_ADDR, 1, 1),
> + /* 8960 has 5 sensors, 8660 has 11, we only handle 5 */
> + [SENSOR_EN] = REG_FIELD(CNTL_ADDR, 3, 7),
> +
> + /* ----- TM ------ */
> + /* INTERRUPT ENABLE */
> + // [INT_EN] = REG_FIELD(TM_INT_EN_OFF, 0, 0),
Get rid of these comments and at the very least use C-style comments.
> +
> + /* Single UPPER/LOWER TEMPERATURE THRESHOLD for all sensors */
> + [LOW_THRESH_0] = REG_FIELD(THRESHOLD_ADDR, 0, 7),
> + [UP_THRESH_0] = REG_FIELD(THRESHOLD_ADDR, 8, 15),
> + [MIN_THRESH_0] = REG_FIELD(THRESHOLD_ADDR, 16, 23),
> + [MAX_THRESH_0] = REG_FIELD(THRESHOLD_ADDR, 24, 31),
> +
> + // /* UPPER/LOWER INTERRUPT [CLEAR/STATUS] */
> + // /* 1 == clear, 0 == normal operation */
Get rid of these comments and at the very least use C-style comments.
> + [LOW_INT_CLEAR_0] = REG_FIELD(CNTL_ADDR, 9, 9),
> + [UP_INT_CLEAR_0] = REG_FIELD(CNTL_ADDR, 10, 10),
> +
> + /* NO CRITICAL INTERRUPT SUPPORT on 8960 */
> +
> + /* Sn_STATUS */
> + [LAST_TEMP_0] = REG_FIELD(S0_STATUS_OFF, 0, 7),
> + [LAST_TEMP_1] = REG_FIELD(S1_STATUS_OFF, 0, 7),
> + [LAST_TEMP_2] = REG_FIELD(S2_STATUS_OFF, 0, 7),
> + [LAST_TEMP_3] = REG_FIELD(S3_STATUS_OFF, 0, 7),
> + [LAST_TEMP_4] = REG_FIELD(S4_STATUS_OFF, 0, 7),
> + [LAST_TEMP_5] = REG_FIELD(S5_STATUS_OFF, 0, 7),
> + [LAST_TEMP_6] = REG_FIELD(S6_STATUS_OFF, 0, 7),
> + [LAST_TEMP_7] = REG_FIELD(S7_STATUS_OFF, 0, 7),
> + [LAST_TEMP_8] = REG_FIELD(S8_STATUS_OFF, 0, 7),
> + [LAST_TEMP_9] = REG_FIELD(S9_STATUS_OFF, 0, 7),
> + [LAST_TEMP_10] = REG_FIELD(S10_STATUS_OFF, 0, 7),
> +
> + /* No VALID field on 8960 */
> + /* TSENS_INT_STATUS bits: 1 == threshold violated */
> + [MIN_STATUS_0] = REG_FIELD(INT_STATUS_ADDR, 0, 0),
> + [LOWER_STATUS_0] = REG_FIELD(INT_STATUS_ADDR, 1, 1),
> + [UPPER_STATUS_0] = REG_FIELD(INT_STATUS_ADDR, 2, 2),
> + /* No CRITICAL field on 8960 */
> + [MAX_STATUS_0] = REG_FIELD(INT_STATUS_ADDR, 3, 3),
> +
> + /* TRDY: 1=ready, 0=in progress */
> + [TRDY] = REG_FIELD(INT_STATUS_ADDR, 7, 7),
> +};
> +
> static const struct tsens_ops ops_8960 = {
> .init = init_8960,
> .calibrate = calibrate_8960,
> @@ -282,4 +354,6 @@ static const struct tsens_ops ops_8960 = {
> struct tsens_plat_data data_8960 = {
> .num_sensors = 11,
> .ops = &ops_8960,
> + .feat = &tsens_8960_feat,
> + .fields = tsens_8960_regfields,
> };
> --
> 2.27.0
>
> -----Messaggio originale-----
> Da: Amit Kucheria <[email protected]>
> Inviato: martedì 11 agosto 2020 14:58
> A: Ansuel Smith <[email protected]>
> Cc: Andy Gross <[email protected]>; Bjorn Andersson
> <[email protected]>; Zhang Rui <[email protected]>; Daniel
> Lezcano <[email protected]>; Rob Herring <[email protected]>;
> Linux PM list <[email protected]>; linux-arm-msm <linux-arm-
> [email protected]>; open list:OPEN FIRMWARE AND FLATTENED DEVICE
> TREE BINDINGS <[email protected]>; LKML <linux-
> [email protected]>
> Oggetto: Re: [RFC PATCH v5 1/7] drivers: thermal: tsens: Add VER_0 tsens
> version
>
> On Sat, Jul 25, 2020 at 11:44 PM Ansuel Smith <[email protected]>
> wrote:
> >
> > VER_0 is used to describe device based on tsens version before v0.1.
> > These device are devices based on msm8960 for example apq8064 or
> > ipq806x.
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
> > ---
> > drivers/thermal/qcom/tsens.c | 160 +++++++++++++++++++++++++++--
> ------
> > drivers/thermal/qcom/tsens.h | 7 +-
> > 2 files changed, 132 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > index 9fe9a2b26705..78840c1bc5d2 100644
> > --- a/drivers/thermal/qcom/tsens.c
> > +++ b/drivers/thermal/qcom/tsens.c
> > @@ -516,6 +516,15 @@ static irqreturn_t tsens_irq_thread(int irq, void
> *data)
> > dev_dbg(priv->dev, "[%u] %s: no violation: %d\n",
> > hw_id, __func__, temp);
> > }
> > +
> > + if (tsens_version(priv) < VER_0_1) {
> > + /* Constraint: There is only 1 interrupt control register for
> all
> > + * 11 temperature sensor. So monitoring more than 1
> sensor based
> > + * on interrupts will yield inconsistent result. To overcome
> this
> > + * issue we will monitor only sensor 0 which is the master
> sensor.
> > + */
> > + break;
> > + }
> > }
> >
> > return IRQ_HANDLED;
> > @@ -531,6 +540,13 @@ static int tsens_set_trips(void *_sensor, int low,
> int high)
> > int high_val, low_val, cl_high, cl_low;
> > u32 hw_id = s->hw_id;
> >
> > + if (tsens_version(priv) < VER_0_1) {
> > + /* Pre v0.1 IP had a single register for each type of interrupt
> > + * and thresholds
> > + */
> > + hw_id = 0;
> > + }
> > +
> > dev_dbg(dev, "[%u] %s: proposed thresholds: (%d:%d)\n",
> > hw_id, __func__, low, high);
> >
> > @@ -550,6 +566,12 @@ static int tsens_set_trips(void *_sensor, int low,
> int high)
> > tsens_set_interrupt(priv, hw_id, LOWER, true);
> > tsens_set_interrupt(priv, hw_id, UPPER, true);
> >
> > + /* VER_0 require to set MIN and MAX THRESH */
> > + if (tsens_version(priv) < VER_0_1) {
> > + regmap_field_write(priv->rf[MIN_THRESH_0], 0);
> > + regmap_field_write(priv->rf[MAX_THRESH_0], 120000);
>
> Since MIN_THRESH_0 and MAX_THRESH_0 are the only two threshold on
> pre
> 0.1 IP, just (mis)use the already predefined LOW_THRESH_0 and
> UP_THRESH_0 in regfield_ids in init_common() below? Then we won't need
> this special casing here. All the special casing ugliness can then
> stay in init_common() with comments.
>
> > + }
> > +
> > spin_unlock_irqrestore(&priv->ul_lock, flags);
> >
> > dev_dbg(dev, "[%u] %s: (%d:%d)->(%d:%d)\n",
> > @@ -584,18 +606,21 @@ int get_temp_tsens_valid(const struct
> tsens_sensor *s, int *temp)
> > u32 valid;
> > int ret;
> >
> > - ret = regmap_field_read(priv->rf[valid_idx], &valid);
> > - if (ret)
> > - return ret;
> > - while (!valid) {
> > - /* Valid bit is 0 for 6 AHB clock cycles.
> > - * At 19.2MHz, 1 AHB clock is ~60ns.
> > - * We should enter this loop very, very rarely.
> > - */
> > - ndelay(400);
> > + /* VER_0 doesn't have VALID bit */
> > + if (tsens_version(priv) >= VER_0_1) {
>
> Since 8960 needs a custom get_temp function, is this change really
> needed?
>
The get_temp_tsens_valid function is used to setup interrupt, think this is
the best way instead of add an if and call get_temp in the setup interrupt
function (instead of using get_temp_valid)
> > ret = regmap_field_read(priv->rf[valid_idx], &valid);
> > if (ret)
> > return ret;
> > + while (!valid) {
> > + /* Valid bit is 0 for 6 AHB clock cycles.
> > + * At 19.2MHz, 1 AHB clock is ~60ns.
> > + * We should enter this loop very, very rarely.
> > + */
> > + ndelay(400);
> > + ret = regmap_field_read(priv->rf[valid_idx], &valid);
> > + if (ret)
> > + return ret;
> > + }
> > }
> >
> > /* Valid bit is set, OK to read the temperature */
> > @@ -765,8 +790,8 @@ int __init init_common(struct tsens_priv *priv)
> >
> > if (tsens_version(priv) > VER_0_1) {
> > for (i = VER_MAJOR; i <= VER_STEP; i++) {
> > - priv->rf[i] = devm_regmap_field_alloc(dev, priv->srot_map,
> > - priv->fields[i]);
> > + priv->rf[i] = devm_regmap_field_alloc(
> > + dev, priv->srot_map, priv->fields[i]);
>
> This doesn't change any code, simply reformats the code to 80 columns.
> Avoid adding such lines to other features, makes it harder to review
> changes.
>
> Please ignore the 80 column warning here and elsewhere below when it
> is only going over by a few characters. Run checkpatch on your patches
> which has now increased the number of columns to 100 now.
>
>
> > if (IS_ERR(priv->rf[i]))
> > return PTR_ERR(priv->rf[i]);
> > }
> > @@ -775,12 +800,80 @@ int __init init_common(struct tsens_priv
> *priv)
> > goto err_put_device;
> > }
> >
> > - priv->rf[TSENS_EN] = devm_regmap_field_alloc(dev, priv->srot_map,
> > - priv->fields[TSENS_EN]);
> > - if (IS_ERR(priv->rf[TSENS_EN])) {
> > - ret = PTR_ERR(priv->rf[TSENS_EN]);
> > - goto err_put_device;
> > + if (tsens_version(priv) >= VER_0_1) {
> > + priv->rf[TSENS_EN] = devm_regmap_field_alloc(
> > + dev, priv->srot_map, priv->fields[TSENS_EN]);
> > + if (IS_ERR(priv->rf[TSENS_EN])) {
> > + ret = PTR_ERR(priv->rf[TSENS_EN]);
> > + goto err_put_device;
> > + }
> > +
> > + priv->rf[SENSOR_EN] = devm_regmap_field_alloc(
> > + dev, priv->srot_map, priv->fields[SENSOR_EN]);
> > + if (IS_ERR(priv->rf[SENSOR_EN])) {
> > + ret = PTR_ERR(priv->rf[SENSOR_EN]);
> > + goto err_put_device;
> > + }
> > + priv->rf[INT_EN] = devm_regmap_field_alloc(
> > + dev, priv->tm_map, priv->fields[INT_EN]);
> > + if (IS_ERR(priv->rf[INT_EN])) {
> > + ret = PTR_ERR(priv->rf[INT_EN]);
> > + goto err_put_device;
> > + }
> > + } else {
>
> Let's not create two big sections with if-else for 8960 and everything
> else. For example, what is wrong with using common code for TSENS_EN?
>
> If the concern is memory wasted trying to allocate fields not present
> on this older platform, perhaps consider adding a check in the loop to
> break early in case of 8960?
>
About TSENS_EN the old platform doesn't have SROT so I need to use TM_MAP.
Should I set the srot map to match the tm map so we can use the common function?
Aside from this problem, I will try to remove the big if-else.
> > + priv->rf[TSENS_EN] = devm_regmap_field_alloc(
> > + dev, priv->tm_map, priv->fields[TSENS_EN]);
> > + if (IS_ERR(priv->rf[TSENS_EN])) {
> > + ret = PTR_ERR(priv->rf[TSENS_EN]);
> > + goto err_put_device;
> > + }
> > +
> > + priv->rf[TSENS_SW_RST] = devm_regmap_field_alloc(
> > + dev, priv->tm_map, priv->fields[TSENS_EN]);
> > + if (IS_ERR(priv->rf[TSENS_EN])) {
> > + ret = PTR_ERR(priv->rf[TSENS_EN]);
> > + goto err_put_device;
> > + }
> > +
> > + /* enable TSENS */
> > + regmap_field_write(priv->rf[TSENS_EN], 1);
> > +
> > + priv->rf[LOW_INT_CLEAR_0] = devm_regmap_field_alloc(
> > + dev, priv->tm_map, priv->fields[LOW_INT_CLEAR_0]);
> > + if (IS_ERR(priv->rf[LOW_INT_CLEAR_0])) {
> > + ret = PTR_ERR(priv->rf[LOW_INT_CLEAR_0]);
> > + goto err_put_device;
> > + }
> > +
> > + priv->rf[UP_INT_CLEAR_0] = devm_regmap_field_alloc(
> > + dev, priv->tm_map, priv->fields[UP_INT_CLEAR_0]);
> > + if (IS_ERR(priv->rf[UP_INT_CLEAR_0])) {
> > + ret = PTR_ERR(priv->rf[UP_INT_CLEAR_0]);
> > + goto err_put_device;
> > + }
> > +
> > + priv->rf[MIN_THRESH_0] = devm_regmap_field_alloc(
> > + dev, priv->tm_map, priv->fields[MIN_THRESH_0]);
> > + if (IS_ERR(priv->rf[MIN_THRESH_0])) {
> > + ret = PTR_ERR(priv->rf[MIN_THRESH_0]);
> > + goto err_put_device;
> > + }
> > +
> > + priv->rf[MAX_THRESH_0] = devm_regmap_field_alloc(
> > + dev, priv->tm_map, priv->fields[MAX_THRESH_0]);
> > + if (IS_ERR(priv->rf[MAX_THRESH_0])) {
> > + ret = PTR_ERR(priv->rf[MAX_THRESH_0]);
> > + goto err_put_device;
> > + }
> > +
> > + priv->rf[TRDY] = devm_regmap_field_alloc(dev, priv->tm_map,
> > + priv->fields[TRDY]);
> > + if (IS_ERR(priv->rf[TRDY])) {
> > + ret = PTR_ERR(priv->rf[TRDY]);
> > + goto err_put_device;
> > + }
> > }
> > +
> > ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
> > if (ret)
> > goto err_put_device;
> > @@ -790,19 +883,6 @@ int __init init_common(struct tsens_priv *priv)
> > goto err_put_device;
> > }
> >
> > - priv->rf[SENSOR_EN] = devm_regmap_field_alloc(dev, priv-
> >srot_map,
> > - priv->fields[SENSOR_EN]);
> > - if (IS_ERR(priv->rf[SENSOR_EN])) {
> > - ret = PTR_ERR(priv->rf[SENSOR_EN]);
> > - goto err_put_device;
> > - }
> > - priv->rf[INT_EN] = devm_regmap_field_alloc(dev, priv->tm_map,
> > - priv->fields[INT_EN]);
> > - if (IS_ERR(priv->rf[INT_EN])) {
> > - ret = PTR_ERR(priv->rf[INT_EN]);
> > - goto err_put_device;
> > - }
> > -
> > /* This loop might need changes if enum regfield_ids is reordered */
> > for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
> > for (i = 0; i < priv->feat->max_sensors; i++) {
> > @@ -856,7 +936,11 @@ int __init init_common(struct tsens_priv *priv)
> > }
> >
> > spin_lock_init(&priv->ul_lock);
> > - tsens_enable_irq(priv);
> > +
> > + /* VER_0 interrupt doesn't need to be enabled */
> > + if (tsens_version(priv) >= VER_0_1)
> > + tsens_enable_irq(priv);
> > +
> > tsens_debug_init(op);
> >
> > err_put_device:
> > @@ -952,10 +1036,18 @@ static int tsens_register_irq(struct tsens_priv
> *priv, char *irqname,
> > if (irq == -ENXIO)
> > ret = 0;
> > } else {
> > - ret = devm_request_threaded_irq(&pdev->dev, irq,
> > - NULL, thread_fn,
> > - IRQF_ONESHOT,
> > - dev_name(&pdev->dev), priv);
> > + /* VER_0 have a different interrupt type */
>
> Say how it is different.
>
>
> > + if (tsens_version(priv) > VER_0)
> > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> > + thread_fn, IRQF_ONESHOT,
> > + dev_name(&pdev->dev),
> > + priv);
> > + else
> > + ret = devm_request_threaded_irq(&pdev->dev, irq,
> > + thread_fn, NULL,
> > + IRQF_TRIGGER_RISING,
> > + dev_name(&pdev->dev),
> > + priv);
> > if (ret)
> > dev_err(&pdev->dev, "%s: failed to get irq\n",
> > __func__);
> > diff --git a/drivers/thermal/qcom/tsens.h
> b/drivers/thermal/qcom/tsens.h
> > index 59d01162c66a..f1120791737c 100644
> > --- a/drivers/thermal/qcom/tsens.h
> > +++ b/drivers/thermal/qcom/tsens.h
> > @@ -25,7 +25,8 @@ struct tsens_priv;
> >
> > /* IP version numbers in ascending order */
> > enum tsens_ver {
> > - VER_0_1 = 0,
> > + VER_0 = 0,
> > + VER_0_1,
> > VER_1_X,
> > VER_2_X,
> > };
> > @@ -441,6 +442,10 @@ enum regfield_ids {
> > CRIT_THRESH_14,
> > CRIT_THRESH_15,
> >
> > + /* VER_0 MIN MAX THRESH */
> > + MIN_THRESH_0,
> > + MAX_THRESH_0,
> > +
> > /* WATCHDOG */
> > WDOG_BARK_STATUS,
> > WDOG_BARK_CLEAR,
> > --
> > 2.27.0
> >
On Tue, Aug 11, 2020 at 6:48 PM <[email protected]> wrote:
>
>
>
> > -----Messaggio originale-----
> > Da: Amit Kucheria <[email protected]>
> >
> > > if (IS_ERR(priv->rf[i]))
> > > return PTR_ERR(priv->rf[i]);
> > > }
> > > @@ -775,12 +800,80 @@ int __init init_common(struct tsens_priv
> > *priv)
> > > goto err_put_device;
> > > }
> > >
> > > - priv->rf[TSENS_EN] = devm_regmap_field_alloc(dev, priv->srot_map,
> > > - priv->fields[TSENS_EN]);
> > > - if (IS_ERR(priv->rf[TSENS_EN])) {
> > > - ret = PTR_ERR(priv->rf[TSENS_EN]);
> > > - goto err_put_device;
> > > + if (tsens_version(priv) >= VER_0_1) {
> > > + priv->rf[TSENS_EN] = devm_regmap_field_alloc(
> > > + dev, priv->srot_map, priv->fields[TSENS_EN]);
> > > + if (IS_ERR(priv->rf[TSENS_EN])) {
> > > + ret = PTR_ERR(priv->rf[TSENS_EN]);
> > > + goto err_put_device;
> > > + }
> > > +
> > > + priv->rf[SENSOR_EN] = devm_regmap_field_alloc(
> > > + dev, priv->srot_map, priv->fields[SENSOR_EN]);
> > > + if (IS_ERR(priv->rf[SENSOR_EN])) {
> > > + ret = PTR_ERR(priv->rf[SENSOR_EN]);
> > > + goto err_put_device;
> > > + }
> > > + priv->rf[INT_EN] = devm_regmap_field_alloc(
> > > + dev, priv->tm_map, priv->fields[INT_EN]);
> > > + if (IS_ERR(priv->rf[INT_EN])) {
> > > + ret = PTR_ERR(priv->rf[INT_EN]);
> > > + goto err_put_device;
> > > + }
> > > + } else {
> >
> > Let's not create two big sections with if-else for 8960 and everything
> > else. For example, what is wrong with using common code for TSENS_EN?
> >
> > If the concern is memory wasted trying to allocate fields not present
> > on this older platform, perhaps consider adding a check in the loop to
> > break early in case of 8960?
> >
>
> About TSENS_EN the old platform doesn't have SROT so I need to use TM_MAP.
> Should I set the srot map to match the tm map so we can use the common function?
> Aside from this problem, I will try to remove the big if-else.
Ick. I guess srot_map and tm_map pointing to the same region is the
lesser of two evils? It makes it so this will be constrained to a
single place in init_common().