Add support for ipq8064 tsens
v10 cover Add support for ipq8064 tsens
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
v10:
* Fix wrong tsens init for ver_0 (crit_trips needs to be set in tsens_register) v9:
* Fix warning from Documentation bot
v8:
* Drop MIN and MAX THRESH and use CRIT_THRESH instead
* Fix broken documentation patch
v7:
* Rework calibrate function to use get_temp_common
* Fix wrong required in the Documentation for ipq8064
* Fix hardware bug in sensor enable function
v6:
* Fix spelling error (can't find the problem with variable misallignment)
* Rework big if-else
* Remove extra comments
* Add description about different interrupts
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 (8):
drivers: thermal: tsens: Add VER_0 tsens version
drivers: thermal: tsens: Don't hardcode sensor slope
drivers: thermal: tsens: Convert msm8960 to reg_field
drivers: thermal: tsens: Use init_common for msm8960
drivers: thermal: tsens: Fix bug in sensor enable for msm8960
drivers: thermal: tsens: Use get_temp_common for msm8960
drivers: thermal: tsens: Add support for ipq8064-tsens
dt-bindings: thermal: tsens: Document ipq8064 bindings
.../bindings/thermal/qcom-tsens.yaml | 56 ++++- drivers/thermal/qcom/tsens-8960.c | 203 ++++++++++-------- drivers/thermal/qcom/tsens.c | 181 +++++++++++++--- drivers/thermal/qcom/tsens.h | 4 +- 4 files changed, 314 insertions(+), 130 deletions(-)
Signature · 1 line
2.30.0
PATCH v10 1/8 drivers: thermal: tsens: Add VER_0 tsens version
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.
--- drivers/thermal/qcom/tsens.c | 175 +++++++++++++++++++++++++++++------ drivers/thermal/qcom/tsens.h | 4 +- 2 files changed, 151 insertions(+), 28 deletions(-)
drivers/thermal/qcom/tsens.c +148 -27
@@ -12,6 +12,7 @@#include <linux/of.h> #include <linux/of_address.h> #include <linux/of_platform.h> +#include <linux/mfd/syscon.h> #include <linux/platform_device.h> #include <linux/pm.h> #include <linux/regmap.h>
@@ -515,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 ;
@@ -530,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);
@@ -584,18 +601,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 */
@@ -608,15 +628,29 @@ int get_temp_common(const struct tsens_sensor *s, int *temp) {struct tsens_priv *priv = s->priv;int hw_id = s->hw_id; -int last_temp =0 , ret; +int last_temp =0 , ret, trdy; +unsigned long timeout; - ret =regmap_field_read (priv->rf[LAST_TEMP_0 + hw_id], &last_temp); -if (ret) -return ret; + timeout = jiffies +usecs_to_jiffies (TIMEOUT_US ); +do { +if (priv->rf[TRDY ]) { + ret =regmap_field_read (priv->rf[TRDY ], &trdy); +if (ret) +return ret; +if (!trdy) +continue ; + } + + ret =regmap_field_read (priv->rf[LAST_TEMP_0 + hw_id], &last_temp); +if (ret) +return ret; - *temp =code_to_degc (last_temp, s) *1000 ; + *temp =code_to_degc (last_temp, s) *1000 ; -return 0 ; +return 0 ; + }while (time_before (jiffies, timeout)); + +return -ETIMEDOUT ; }#ifdef CONFIG_DEBUG_FS
@@ -738,19 +772,31 @@ int __init init_common(struct tsens_priv *priv) priv->tm_offset =0x1000 ; } - res =platform_get_resource (op,IORESOURCE_MEM ,0 ); - tm_base =devm_ioremap_resource (dev, res); -if (IS_ERR (tm_base)) { - ret =PTR_ERR (tm_base); - goto err_put_device; +if (tsens_version (priv) >=VER_0_1 ) { + res =platform_get_resource (op,IORESOURCE_MEM ,0 ); + tm_base =devm_ioremap_resource (dev, res); +if (IS_ERR (tm_base)) { + ret =PTR_ERR (tm_base); + goto err_put_device; + } + + priv->tm_map =devm_regmap_init_mmio (dev, tm_base, &tsens_config); + }else {/* VER_0 share the same gcc regs using a syscon */ +struct device *parent = priv->dev->parent; + +if (parent) + priv->tm_map =syscon_node_to_regmap (parent->of_node); } - priv->tm_map =devm_regmap_init_mmio (dev, tm_base, &tsens_config); -if (IS_ERR (priv->tm_map)) { +if (IS_ERR_OR_NULL (priv->tm_map)) { ret =PTR_ERR (priv->tm_map); goto err_put_device; } +/* VER_0 have only tm_map */ +if (!priv->srot_map) + priv->srot_map = priv->tm_map; +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,
@@ -769,6 +815,10 @@ int __init init_common(struct tsens_priv *priv) ret =PTR_ERR (priv->rf[TSENS_EN ]); goto err_put_device; } +/* in VER_0 TSENS need to be explicitly enabled */ +if (tsens_version (priv) ==VER_0 ) +regmap_field_write (priv->rf[TSENS_EN ],1 ); + ret =regmap_field_read (priv->rf[TSENS_EN ], &enabled);if (ret) goto err_put_device;
@@ -791,6 +841,57 @@ int __init init_common(struct tsens_priv *priv) goto err_put_device; } + 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; + } + + 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; + } + +if (tsens_version (priv) <VER_0_1 ) { + priv->rf[CRIT_THRESH_0 ] =devm_regmap_field_alloc ( + dev, priv->tm_map, priv->fields[CRIT_THRESH_0 ]); +if (IS_ERR (priv->rf[CRIT_THRESH_0 ])) { + ret =PTR_ERR (priv->rf[CRIT_THRESH_0 ]); + goto err_put_device; + } + + priv->rf[CRIT_THRESH_1 ] =devm_regmap_field_alloc ( + dev, priv->tm_map, priv->fields[CRIT_THRESH_1 ]); +if (IS_ERR (priv->rf[CRIT_THRESH_1 ])) { + ret =PTR_ERR (priv->rf[CRIT_THRESH_1 ]); + 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; + } +/* 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++) {
@@ -844,7 +945,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:
@@ -930,7 +1035,7 @@ static int tsens_register_irq(struct tsens_priv *priv, char *irqname, irq_handler_t thread_fn) {struct platform_device *pdev; -int ret, irq; +int ret, irq, irq_type =IRQF_ONESHOT ; pdev =of_find_device_by_node (priv->dev->of_node);if (!pdev)
@@ -943,9 +1048,12 @@ 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 , +/* VER_0 interrupt is TRIGGER_RISING, VER_0_1 and up is ONESHOT */ +if (tsens_version (priv) ==VER_0 ) + irq_type =IRQF_TRIGGER_RISING ; + + ret =devm_request_threaded_irq (&pdev->dev, irq, thread_fn, +NULL , irq_type,dev_name (&pdev->dev), priv);if (ret) dev_err(&pdev->dev, "%s: failed to get irq\n",
@@ -975,6 +1083,19 @@ static int tsens_register(struct tsens_priv *priv) priv->ops->enable (priv, i); } +/* VER_0 require to set MIN and MAX THRESH +* These 2 regs are set using the: +* - CRIT_THRESH_0 for MAX THRESH hardcoded to 120°C +* - CRIT_THRESH_1 for MIN THRESH hardcoded to 0°C +*/ +if (tsens_version (priv) <VER_0_1 ) { +regmap_field_write (priv->rf[CRIT_THRESH_0 ], +tsens_mC_to_hw (priv->sensor,120000 )); + +regmap_field_write (priv->rf[CRIT_THRESH_1 ], +tsens_mC_to_hw (priv->sensor,0 )); + } + ret =tsens_register_irq (priv,"uplow" , tsens_irq_thread);if (ret <0 )return ret;
drivers/thermal/qcom/tsens.h +3 -1
@@ -13,6 +13,7 @@#define CAL_DEGC_PT2 120#define SLOPE_FACTOR 1000#define SLOPE_DEFAULT 3200 +#define TIMEOUT_US 100#define THRESHOLD_MAX_ADC_CODE 0x3ff#define THRESHOLD_MIN_ADC_CODE 0x0
@@ -25,7 +26,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 , };
Signature · 1 line
2.30.0
Re: [PATCH v10 1/8] drivers: thermal: tsens: Add VER_0 tsens version
Hi Ansuel!
Apologies for delay in the review..
This particular patch throws checkpatch check warnings. Please run checkpatch.pl --strict and fix them. Rest of the comments below
Quoted 93 lines · Ansuel Smith
VER_0is 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 | 175 +++++++++++++++++++++++++++++------ drivers/thermal/qcom/tsens.h | 4 +- 2 files changed, 151 insertions(+), 28 deletions(-)drivers/thermal/qcom/tsens.c +31
-12@@ -12,6 +12,7 @@#include <linux/of.h> #include <linux/of_address.h> #include <linux/of_platform.h> +#include <linux/mfd/syscon.h> #include <linux/platform_device.h> #include <linux/pm.h> #include <linux/regmap.h> @@ -515,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 ;@@ -530,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);@@ -584,18 +601,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 */ @@ -608,15 +628,29 @@ int get_temp_common(const struct tsens_sensor *s, int *temp) {struct tsens_priv *priv = s->priv;int hw_id = s->hw_id; -int last_temp =0 , ret; +int last_temp =0 , ret, trdy; +unsigned long timeout; - ret =regmap_field_read (priv->rf[LAST_TEMP_0 + hw_id], &last_temp); -if (ret)
drivers/thermal/qcom/tsens.c int get_temp_common(const struct tsens_sensor *s, int *temp) · line 642
Quoted 10 lines
drivers/thermal/qcom/tsens.c +9
-1-return ret; + timeout = jiffies +usecs_to_jiffies (TIMEOUT_US ); + do { + if (priv->rf[TRDY]) { + ret = regmap_field_read(priv->rf[TRDY], &trdy); + if (ret) + return ret; + if (!trdy) + continue; + }
Did you test this on v0.1 sensor and ensure that the trdy handshake works there as well? I don't have a platform to test this. But the safer option here will be to do the hand shake only for v0.
Quoted 67 lines
drivers/thermal/qcom/tsens.c +32
-9+ + ret =regmap_field_read (priv->rf[LAST_TEMP_0 + hw_id], &last_temp); +if (ret) +return ret; - *temp =code_to_degc (last_temp, s) *1000 ; + *temp =code_to_degc (last_temp, s) *1000 ; -return 0 ; +return 0 ; + }while (time_before (jiffies, timeout)); + +return -ETIMEDOUT ; }#ifdef CONFIG_DEBUG_FS @@ -738,19 +772,31 @@ int __init init_common(struct tsens_priv *priv) priv->tm_offset =0x1000 ; } - res =platform_get_resource (op,IORESOURCE_MEM ,0 ); - tm_base =devm_ioremap_resource (dev, res); -if (IS_ERR (tm_base)) { - ret =PTR_ERR (tm_base); - goto err_put_device; +if (tsens_version (priv) >=VER_0_1 ) { + res =platform_get_resource (op,IORESOURCE_MEM ,0 ); + tm_base =devm_ioremap_resource (dev, res); +if (IS_ERR (tm_base)) { + ret =PTR_ERR (tm_base); + goto err_put_device; + } + + priv->tm_map =devm_regmap_init_mmio (dev, tm_base, &tsens_config); + }else {/* VER_0 share the same gcc regs using a syscon */ +struct device *parent = priv->dev->parent; + +if (parent) + priv->tm_map =syscon_node_to_regmap (parent->of_node); } - priv->tm_map =devm_regmap_init_mmio (dev, tm_base, &tsens_config); -if (IS_ERR (priv->tm_map)) { +if (IS_ERR_OR_NULL (priv->tm_map)) { ret =PTR_ERR (priv->tm_map); goto err_put_device; } +/* VER_0 have only tm_map */ +if (!priv->srot_map) + priv->srot_map = priv->tm_map; +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,@@ -769,6 +815,10 @@ int __init init_common(struct tsens_priv *priv) ret =PTR_ERR (priv->rf[TSENS_EN ]); goto err_put_device; } +/* in VER_0 TSENS need to be explicitly enabled */ +if (tsens_version (priv) ==VER_0 ) +regmap_field_write (priv->rf[TSENS_EN ],1 ); + ret =regmap_field_read (priv->rf[TSENS_EN ], &enabled);if (ret) goto err_put_device;
drivers/thermal/qcom/tsens.c int __init init_common(struct tsens_priv *priv) · line 845
Quoted 5 lines
drivers/thermal/qcom/tsens.c +2
-0@@ -791,6 +841,57 @@ int __init init_common(struct tsens_priv *priv) goto err_put_device; } + priv->rf[TSENS_EN ] =devm_regmap_field_alloc (dev, priv->tm_map, + priv->fields[TSENS_EN ]);
Why are you reallocating TSENS_EN here? It is already done above. Also the base map is wrong. It should be priv->srot_map
drivers/thermal/qcom/tsens.c int __init init_common(struct tsens_priv *priv) · line 852
Quoted 7 lines
drivers/thermal/qcom/tsens.c +7
-0+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 ]);
/s/priv->tm_map/priv->srot_map.
drivers/thermal/qcom/tsens.c int __init init_common(struct tsens_priv *priv) · line 853
Quoted 1 line
drivers/thermal/qcom/tsens.c +1
-0+if (IS_ERR (priv->rf[TSENS_EN ])) {
check for TSENS_SW_RST and not TSENS_EN..
drivers/thermal/qcom/tsens.c int __init init_common(struct tsens_priv *priv) · line 854
Quoted 1 line
drivers/thermal/qcom/tsens.c +1
-0+ ret =PTR_ERR (priv->rf[TSENS_EN ]);
check for TSENS_SW_RST and not TSENS_EN
Quoted 6 lines
drivers/thermal/qcom/tsens.c +6
-0+ goto err_put_device; + } + + 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 ])) {
drivers/thermal/qcom/tsens.c int __init init_common(struct tsens_priv *priv) · line 870
Quoted 10 lines
drivers/thermal/qcom/tsens.c +10
-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; + }
Does not the code in for loop for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) { do this for you ? If not, you should move it inside the if condition below. And in that case you need not loop enter the for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {.
Quoted 53 lines
drivers/thermal/qcom/tsens.c +30
-2+ +if (tsens_version (priv) <VER_0_1 ) { + priv->rf[CRIT_THRESH_0 ] =devm_regmap_field_alloc ( + dev, priv->tm_map, priv->fields[CRIT_THRESH_0 ]); +if (IS_ERR (priv->rf[CRIT_THRESH_0 ])) { + ret =PTR_ERR (priv->rf[CRIT_THRESH_0 ]); + goto err_put_device; + } + + priv->rf[CRIT_THRESH_1 ] =devm_regmap_field_alloc ( + dev, priv->tm_map, priv->fields[CRIT_THRESH_1 ]); +if (IS_ERR (priv->rf[CRIT_THRESH_1 ])) { + ret =PTR_ERR (priv->rf[CRIT_THRESH_1 ]); + 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; + } +/* 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++) {@@ -844,7 +945,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:@@ -930,7 +1035,7 @@ static int tsens_register_irq(struct tsens_priv *priv, char *irqname, irq_handler_t thread_fn) {struct platform_device *pdev; -int ret, irq; +int ret, irq, irq_type =IRQF_ONESHOT ; pdev =of_find_device_by_node (priv->dev->of_node);if (!pdev)@@ -943,9 +1048,12 @@ static int tsens_register_irq(struct tsens_priv *priv, char *irqname,if (irq == -ENXIO ) ret =0 ; }else {
drivers/thermal/qcom/tsens.c static int tsens_register_irq(struct tsens_priv *priv, char *irqname, · line 1057
Quoted 10 lines
drivers/thermal/qcom/tsens.c +6
-3- ret =devm_request_threaded_irq (&pdev->dev, irq, -NULL , thread_fn, -IRQF_ONESHOT , +/* VER_0 interrupt is TRIGGER_RISING, VER_0_1 and up is ONESHOT */ +if (tsens_version (priv) ==VER_0 ) + irq_type =IRQF_TRIGGER_RISING ; + + ret =devm_request_threaded_irq (&pdev->dev, irq, thread_fn, +NULL , irq_type,dev_name (&pdev->dev), priv);
You have changed the interrupt handling from threaded context to interrupt context. I cannot see any reason to do this.
Quoted 45 lines
drivers/thermal/qcom/tsens.c +13
-0if (ret) dev_err(&pdev->dev, "%s: failed to get irq\n",@@ -975,6 +1083,19 @@ static int tsens_register(struct tsens_priv *priv) priv->ops->enable (priv, i); } +/* VER_0 require to set MIN and MAX THRESH +* These 2 regs are set using the: +* - CRIT_THRESH_0 for MAX THRESH hardcoded to 120°C +* - CRIT_THRESH_1 for MIN THRESH hardcoded to 0°C +*/ +if (tsens_version (priv) <VER_0_1 ) { +regmap_field_write (priv->rf[CRIT_THRESH_0 ], +tsens_mC_to_hw (priv->sensor,120000 )); + +regmap_field_write (priv->rf[CRIT_THRESH_1 ], +tsens_mC_to_hw (priv->sensor,0 )); + } + ret =tsens_register_irq (priv,"uplow" , tsens_irq_thread);if (ret <0 )return ret;drivers/thermal/qcom/tsens.h +3
-1@@ -13,6 +13,7 @@#define CAL_DEGC_PT2 120#define SLOPE_FACTOR 1000#define SLOPE_DEFAULT 3200 +#define TIMEOUT_US 100#define THRESHOLD_MAX_ADC_CODE 0x3ff#define THRESHOLD_MIN_ADC_CODE 0x0@@ -25,7 +26,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 , };
Signature · 2 lines
Warm Regards Thara
PATCH v10 2/8 drivers: thermal: tsens: Don't hardcode sensor slope
In reply to Ansuel SmithFunction compute_intercept_slope hardcode the sensor slope to SLOPE_DEFAULT. Change this and use the default value only if a slope is not defined. This is needed for tsens VER_0 that has a hardcoded slope table.
--- drivers/thermal/qcom/tsens.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/thermal/qcom/tsens.c +2 -1
@@ -86,7 +86,8 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,"%s: sensor%d - data_point1:%#x data_point2:%#x\n" , __func__, i, p1[i], p2[i]); - priv->sensor[i].slope =SLOPE_DEFAULT ; +if (!priv->sensor[i].slope) + priv->sensor[i].slope =SLOPE_DEFAULT ;if (mode ==TWO_PT_CALIB ) {/* * slope (m) = adc_code2 - adc_code1 (y2 - y1)/
Signature · 1 line
2.30.0
Re: [PATCH v10 2/8] drivers: thermal: tsens: Don't hardcode sensor slope
Quoted 6 lines · Ansuel Smith
Function
compute_intercept_slopehardcode the sensor slope toSLOPE_DEFAULT. Change this and use the default value only if a slope is not defined. This is needed for tsensVER_0that has a hardcoded slope table.Signed-off-by Ansuel Smith <[email protected]>
Warm Regards
Thara
Quoted 19 lines
--- drivers/thermal/qcom/tsens.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/thermal/qcom/tsens.c +2
-1@@ -86,7 +86,8 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,"%s: sensor%d - data_point1:%#x data_point2:%#x\n" , __func__, i, p1[i], p2[i]); - priv->sensor[i].slope =SLOPE_DEFAULT ; +if (!priv->sensor[i].slope) + priv->sensor[i].slope =SLOPE_DEFAULT ;if (mode ==TWO_PT_CALIB ) {/* * slope (m) = adc_code2 - adc_code1 (y2 - y1)/
Signature · 2 lines
Warm Regards Thara
PATCH v10 3/8 drivers: thermal: tsens: Convert msm8960 to reg_field
In reply to Ansuel SmithConvert msm9860 driver to reg_field to use the init_common function.
--- drivers/thermal/qcom/tsens-8960.c | 80 ++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-)
drivers/thermal/qcom/tsens-8960.c +79 -1
@@ -51,11 +51,22 @@#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 +#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-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 +280,71 @@ 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 */ +/* NO INTERRUPT ENABLE */ + +/* 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 and MAX_THRESH_0 are not present in the regfield +* Recycle CRIT_THRESH_0 and 1 to set the required regs to hardcoded temp +* MIN_THRESH_0 -> CRIT_THRESH_1 +* MAX_THRESH_0 -> CRIT_THRESH_0 +*/ + [CRIT_THRESH_1 ] =REG_FIELD (THRESHOLD_ADDR ,16 ,23 ), + [CRIT_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 +358,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, };
Signature · 1 line
2.30.0
Re: [PATCH v10 3/8] drivers: thermal: tsens: Convert msm8960 to reg_field
Quoted 2 lines · Ansuel Smith
Convert msm9860 driver to
reg_fieldto use theinit_commonfunction.
Hi!
Now that you have done this, you should clean up the unused bitmasks/offsets etc intsens-8960.c file as well as a separate patch. I only see the
need to maintain SLP_CLK_ENA_8660 and SLP_CLK_ENA. Everything else can be removed and the s/w can use priv->rf[_field_] for access. Otherwise for this patch
Warm Regards
Thara
Quoted 114 lines
Signed-off-by Ansuel Smith <[email protected]>--- drivers/thermal/qcom/tsens-8960.c | 80 ++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-)drivers/thermal/qcom/tsens-8960.c +79
-1@@ -51,11 +51,22 @@#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 +#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-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 +280,71 @@ 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 */ +/* NO INTERRUPT ENABLE */ + +/* 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 and MAX_THRESH_0 are not present in the regfield +* Recycle CRIT_THRESH_0 and 1 to set the required regs to hardcoded temp +* MIN_THRESH_0 -> CRIT_THRESH_1 +* MAX_THRESH_0 -> CRIT_THRESH_0 +*/ + [CRIT_THRESH_1 ] =REG_FIELD (THRESHOLD_ADDR ,16 ,23 ), + [CRIT_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 +358,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, };
PATCH v10 4/8 drivers: thermal: tsens: Use init_common for msm8960
In reply to Ansuel SmithUse init_common and drop custom init for msm8960.
--- drivers/thermal/qcom/tsens-8960.c | 52 +------------------------------ 1 file changed, 1 insertion(+), 51 deletions(-)
drivers/thermal/qcom/tsens-8960.c +1 -51
@@ -173,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;
@@ -346,7 +296,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,
Signature · 1 line
2.30.0
Re: [PATCH v10 4/8] drivers: thermal: tsens: Use init_common for msm8960
Quoted 3 lines · Ansuel Smith
Use
init_commonand drop custom init for msm8960.Signed-off-by Ansuel Smith <[email protected]>
Warm Regards
Thara
Quoted 74 lines
--- >
drivers/thermal/qcom/tsens-8960.c| 52 +------------------------------1 file changed, 1 insertion(+), 51 deletions(-)drivers/thermal/qcom/tsens-8960.c +1
-51@@ -173,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;@@ -346,7 +296,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,
PATCH v10 5/8 drivers: thermal: tsens: Fix bug in sensor enable for msm8960
In reply to Ansuel SmithIt'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.
--- drivers/thermal/qcom/tsens-8960.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
drivers/thermal/qcom/tsens-8960.c +17 -2
@@ -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
Signature · 1 line
2.30.0
Re: [PATCH v10 5/8] drivers: thermal: tsens: Fix bug in sensor enable for msm8960
Quoted 3 lines · Ansuel Smith
It's present a hardware bug in tsens
VER_0where 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.
Quoted 22 lines
Signed-off-by Ansuel Smith <[email protected]>--- drivers/thermal/qcom/tsens-8960.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)drivers/thermal/qcom/tsens-8960.c +1
-1@@ -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 */
drivers/thermal/qcom/tsens-8960.c static int enable_8960(struct tsens_priv *priv, int id) · line 136
Quoted 6 lines
drivers/thermal/qcom/tsens-8960.c +2
-1@@ -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.
drivers/thermal/qcom/tsens-8960.c static int enable_8960(struct tsens_priv *priv, int id) · line 140
Quoted 4 lines
drivers/thermal/qcom/tsens-8960.c +4
-0+* 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.
drivers/thermal/qcom/tsens-8960.c static int enable_8960(struct tsens_priv *priv, int id) · line 149
Quoted 9 lines
drivers/thermal/qcom/tsens-8960.c +8
-0+*/ +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 ?
Quoted 9 lines
drivers/thermal/qcom/tsens-8960.c +2
-0if (ret)return ret; + reg |=MEASURE_PERIOD ; +if (priv->num_sensors >1 ) reg |= mask |SLP_CLK_ENA |EN ;else
Signature · 2 lines
Warm Regards Thara
PATCH v10 6/8 drivers: thermal: tsens: Use get_temp_common for msm8960
In reply to Ansuel SmithRework calibrate function to use common function. Derive the offset from a missing hardcoded slope table and the data from the nvmem calib efuses.
--- drivers/thermal/qcom/tsens-8960.c | 56 +++++++++---------------------- 1 file changed, 15 insertions(+), 41 deletions(-)
drivers/thermal/qcom/tsens-8960.c +15 -41
@@ -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,
Signature · 1 line
2.30.0
Re: [PATCH v10 6/8] drivers: thermal: tsens: Use get_temp_common for msm8960
Quoted 3 lines · Ansuel Smith
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,
Warm Regards
Thara
Quoted 101 lines
Signed-off-by Ansuel Smith <[email protected]>--- drivers/thermal/qcom/tsens-8960.c | 56 +++++++++---------------------- 1 file changed, 15 insertions(+), 41 deletions(-)drivers/thermal/qcom/tsens-8960.c +15
-41@@ -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,
PATCH v10 7/8 drivers: thermal: tsens: Add support for ipq8064-tsens
In reply to Ansuel SmithAdd support for tsens present in ipq806x SoCs based on generic msm8960 tsens driver.
--- drivers/thermal/qcom/tsens.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/thermal/qcom/tsens.c +3 -0
@@ -1001,6 +1001,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, }, {
Signature · 1 line
2.30.0
Re: [PATCH v10 7/8] drivers: thermal: tsens: Add support for ipq8064-tsens
Quoted 4 lines · Ansuel Smith
Add support for tsens present in ipq806x SoCs based on generic msm8960 tsens driver.
Signed-off-by Ansuel Smith <[email protected]>
Warm Regards
Thara
Quoted 19 lines
--- drivers/thermal/qcom/tsens.c | 3 +++ 1 file changed, 3 insertions(+)drivers/thermal/qcom/tsens.c +3
-0@@ -1001,6 +1001,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, }, {
PATCH v10 8/8 dt-bindings: thermal: tsens: Document ipq8064 bindings
In reply to Ansuel SmithDocument 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.
--- .../bindings/thermal/qcom-tsens.yaml | 56 ++++++++++++++++--- 1 file changed, 48 insertions(+), 8 deletions(-)
Documentation/devicetree/bindings/thermal/qcom-tsens.yaml +48 -8
@@ -19,6 +19,11 @@ description: | properties: compatible: oneOf: + - description: msm9860 TSENS based + items: + - enum: + - qcom,ipq8064-tsens + - description: v0.1 of TSENS items: - enum:
@@ -73,7 +78,9 @@ properties: maxItems: 2 items: - const: calib - - const: calib_sel + - enum: + - calib_backup + - calib_sel "#qcom,sensors": description:
@@ -88,12 +95,20 @@ properties: Number of cells required to uniquely identify the thermal sensors. Since we have multiple sensors this is set to 1 +required: + - compatible + - interrupts + - interrupt-names + - "#thermal-sensor-cells" + - "#qcom,sensors" + allOf: - if: properties: compatible: contains: enum: + - qcom,ipq8064-tsens - qcom,msm8916-tsens - qcom,msm8974-tsens - qcom,msm8976-tsens
@@ -114,17 +129,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 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_calib_backup>; + nvmem-cell-names = "calib", "calib_backup"; + interrupts = <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "uplow"; + + #qcom,sensors = <11>; + #thermal-sensor-cells = <1>; + }; + }; + - | #include <dt-bindings/interrupt-controller/arm-gic.h> // Example 1 (legacy: for pre v1 IP):
Signature · 1 line
2.30.0
Re: [PATCH v10 8/8] dt-bindings: thermal: tsens: Document ipq8064 bindings
Quoted 8 lines · Ansuel Smith
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 | 56 ++++++++++++++++--- 1 file changed, 48 insertions(+), 8 deletions(-)
Hi Ansuel,
Quoted 7 lines · Ansuel Smith
This patchset convert msm8960 to
reg_filed, useint_commoninstead of a custom function and fix wrong tsensget_tempfunction 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
I don't have major concerns with the series except there is no comment from the maintainer / reviewer of the sensor.
Given it is based on Amit's work, I can assume they are correct.
I added Thara in Cc hoping she has time to review the changes. If nobody complains with the series, I'll merge them in the next days
Thanks
-- Daniel
Signature · 5 lines
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
Quoted 12 lines · Daniel Lezcano
Hi Ansuel,
Quoted 7 lines · Ansuel Smith
This patchset convert msm8960 to
reg_filed, useint_commoninstead of a custom function and fix wrong tsensget_tempfunction 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
Quoted 7 lines
I don't have major concerns with the series except there is no comment from the maintainer / reviewer of the sensor.
Given it is based on Amit's work, I can assume they are correct.
I added Thara in Cc hoping she has time to review the changes. If nobody complains with the series, I'll merge them in the next days
Hi Ansuel/Daniel,
Just wanted to let you know that I have started looking into this and review this within next week or two.
Quoted 6 lines
Thanks
-- Daniel
Signature · 2 lines
Warm Regards Thara
Quoted 21 lines · Thara Gopinath
Quoted 18 lines · Daniel Lezcano
Hi Ansuel,
Quoted 8 lines · Ansuel Smith
This patchset convert msm8960 to
reg_filed, useint_commoninstead of a custom function and fix wrong tsensget_tempfunction 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-breakageI don't have major concerns with the series except there is no comment from the maintainer / reviewer of the sensor.
Given it is based on Amit's work, I can assume they are correct.
Quoted 7 lines
Quoted 2 lines
I added Thara in Cc hoping she has time to review the changes. If nobody complains with the series, I'll merge them in the next days
Hi Ansuel/Daniel,
Just wanted to let you know that I have started looking into this and review this within next week or two.
Great, thank you
Signature · 5 lines
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog