> -----Messaggio originale-----
> Da: Amit Kucheria <amit.kuche...@linaro.org>
> Inviato: lunedì 20 luglio 2020 11:41
> A: Ansuel Smith <ansuels...@gmail.com>
> Cc: Rob Herring <robh...@kernel.org>; Andy Gross <agr...@kernel.org>;
> Bjorn Andersson <bjorn.anders...@linaro.org>; Zhang Rui
> <rui.zh...@intel.com>; Daniel Lezcano <daniel.lezc...@linaro.org>;
> Michael Turquette <mturque...@baylibre.com>; Stephen Boyd
> <sb...@kernel.org>; Linux PM list <linux...@vger.kernel.org>; linux-arm-
> msm <linux-arm-...@vger.kernel.org>; DTML
> <devicet...@vger.kernel.org>; Linux Kernel Mailing List <linux-
> ker...@vger.kernel.org>; linux-clk <linux-...@vger.kernel.org>
> Oggetto: Re: [PATCH v4 5/7] drivers: thermal: tsens: add interrupt support
> for 9860 driver
>
> Hi Ansuel,
>
> Thanks for this patch.
>
> On Thu, Jul 16, 2020 at 7:58 AM Ansuel Smith <ansuels...@gmail.com>
> wrote:
> >
> > Add interrupt support for 9860 tsens driver used to set thermal trip
> > point for the system.
>
> typo: 8960
>
> You've used the names 8960 and ipq8064 interchangeably throughout the
> series. AFAICT, msm8960, ipq8064 and apq8064 use the same IP version
> of tsens. Please use 8960 in all patches, descriptions and dt-binding.
> to reflect the filename for the driver.
> Then add ipq8064 and apq8064 in a comment in the driver like here to
> show that the driver also supports these other SoCs:
> https://elixir.bootlin.com/linux/v5.8-
> rc4/source/drivers/thermal/qcom/tsens-v0_1.c#L328
>
> You can also add a new compatible string for ipq8064 as a separate
> patch at the end of the series.
>
> > Signed-off-by: Ansuel Smith <ansuels...@gmail.com>
> > ---
> > drivers/thermal/qcom/tsens-8960.c | 197
> +++++++++++++++++++++++++++---
> > drivers/thermal/qcom/tsens.h | 3 +
> > 2 files changed, 186 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/thermal/qcom/tsens-8960.c
> b/drivers/thermal/qcom/tsens-8960.c
> > index 45788eb3c666..20d0bfb10f1f 100644
> > --- a/drivers/thermal/qcom/tsens-8960.c
> > +++ b/drivers/thermal/qcom/tsens-8960.c
> > @@ -8,6 +8,7 @@
> > #include <linux/bitops.h>
> > #include <linux/regmap.h>
> > #include <linux/mfd/syscon.h>
> > +#include <linux/interrupt.h>
> > #include <linux/thermal.h>
> > #include "tsens.h"
> >
> > @@ -27,7 +28,6 @@
> > /* CNTL_ADDR bitmasks */
> > #define EN BIT(0)
> > #define SW_RST BIT(1)
> > -#define SENSOR0_EN BIT(3)
> > #define SLP_CLK_ENA BIT(26)
> > #define SLP_CLK_ENA_8660 BIT(24)
> > #define MEASURE_PERIOD 1
> > @@ -41,14 +41,26 @@
> >
> > #define THRESHOLD_ADDR 0x3624
> > /* THRESHOLD_ADDR bitmasks */
> > +#define THRESHOLD_MAX_CODE 0x20000
> > +#define THRESHOLD_MIN_CODE 0
> > #define THRESHOLD_MAX_LIMIT_SHIFT 24
> > #define THRESHOLD_MIN_LIMIT_SHIFT 16
> > #define THRESHOLD_UPPER_LIMIT_SHIFT 8
> > #define THRESHOLD_LOWER_LIMIT_SHIFT 0
> > +#define THRESHOLD_MAX_LIMIT_MASK (THRESHOLD_MAX_CODE
> << \
> > + THRESHOLD_MAX_LIMIT_SHIFT)
> > +#define THRESHOLD_MIN_LIMIT_MASK (THRESHOLD_MAX_CODE <<
> \
> > + THRESHOLD_MIN_LIMIT_SHIFT)
> > +#define THRESHOLD_UPPER_LIMIT_MASK (THRESHOLD_MAX_CODE
> << \
> > + THRESHOLD_UPPER_LIMIT_SHIFT)
> > +#define THRESHOLD_LOWER_LIMIT_MASK (THRESHOLD_MAX_CODE
> << \
> > + THRESHOLD_LOWER_LIMIT_SHIFT)
> >
> > /* Initial temperature threshold values */
> > -#define LOWER_LIMIT_TH 0x50
> > -#define UPPER_LIMIT_TH 0xdf
> > +#define LOWER_LIMIT_TH_8960 0x50
> > +#define UPPER_LIMIT_TH_8960 0xdf
> > +#define LOWER_LIMIT_TH_8064 0x9d /* 95C */
> > +#define UPPER_LIMIT_TH_8064 0xa6 /* 105C */
> > #define MIN_LIMIT_TH 0x0
> > #define MAX_LIMIT_TH 0xff
> >
> > @@ -57,6 +69,170 @@
> > #define TRDY_MASK BIT(7)
> > #define TIMEOUT_US 100
> >
> > +#define TSENS_EN BIT(0)
> > +#define TSENS_SW_RST BIT(1)
> > +#define TSENS_ADC_CLK_SEL BIT(2)
> > +#define SENSOR0_EN BIT(3)
> > +#define SENSOR1_EN BIT(4)
> > +#define SENSOR2_EN BIT(5)
> > +#define SENSOR3_EN BIT(6)
> > +#define SENSOR4_EN BIT(7)
> > +#define SENSORS_EN (SENSOR0_EN | SENSOR1_EN | \
> > + SENSOR2_EN | SENSOR3_EN | SENSOR4_EN)
> > +#define TSENS_8064_SENSOR5_EN BIT(8)
> > +#define TSENS_8064_SENSOR6_EN BIT(9)
> > +#define TSENS_8064_SENSOR7_EN BIT(10)
> > +#define TSENS_8064_SENSOR8_EN BIT(11)
> > +#define TSENS_8064_SENSOR9_EN BIT(12)
> > +#define TSENS_8064_SENSOR10_EN BIT(13)
> > +#define TSENS_8064_SENSORS_EN (SENSORS_EN | \
> > + TSENS_8064_SENSOR5_EN | \
> > + TSENS_8064_SENSOR6_EN | \
> > + TSENS_8064_SENSOR7_EN | \
> > + TSENS_8064_SENSOR8_EN | \
> > + TSENS_8064_SENSOR9_EN | \
> > + TSENS_8064_SENSOR10_EN)
> > +
> > +u32 tsens_8960_slope[] = {
> > + 1176, 1176, 1154, 1176,
> > + 1111, 1132, 1132, 1199,
> > + 1132, 1199, 1132
> > + };
> > +
> > +/* 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 void notify_uspace_tsens_fn(struct work_struct *work)
> > +{
> > + struct tsens_sensor *s = container_of(work, struct tsens_sensor,
> > +
> > notify_work);
> > +
> > + sysfs_notify(&s->tzd->device.kobj, NULL, "type");
> > +}
> > +
> > +static void tsens_scheduler_fn(struct work_struct *work)
> > +{
> > + struct tsens_priv *priv =
> > + container_of(work, struct tsens_priv, tsens_work);
> > + unsigned int threshold, threshold_low, code, reg, sensor;
> > + unsigned long mask;
> > + bool upper_th_x, lower_th_x;
> > + int ret;
> > +
> > + ret = regmap_read(priv->tm_map, STATUS_CNTL_ADDR_8064,
> ®);
> > + if (ret)
> > + return;
> > + reg = reg | LOWER_STATUS_CLR | UPPER_STATUS_CLR;
> > + ret = regmap_write(priv->tm_map, STATUS_CNTL_ADDR_8064, reg);
> > + if (ret)
> > + return;
> > +
> > + mask = ~(LOWER_STATUS_CLR | UPPER_STATUS_CLR);
> > + ret = regmap_read(priv->tm_map, THRESHOLD_ADDR, &threshold);
> > + if (ret)
> > + return;
> > + threshold_low = (threshold & THRESHOLD_LOWER_LIMIT_MASK) >>
> > + THRESHOLD_LOWER_LIMIT_SHIFT;
> > + threshold = (threshold & THRESHOLD_UPPER_LIMIT_MASK) >>
> > + THRESHOLD_UPPER_LIMIT_SHIFT;
> > +
> > + ret = regmap_read(priv->tm_map, STATUS_CNTL_ADDR_8064,
> ®);
> > + if (ret)
> > + return;
> > +
> > + ret = regmap_read(priv->tm_map, CNTL_ADDR, &sensor);
> > + if (ret)
> > + return;
> > + sensor &= (uint32_t)TSENS_8064_SENSORS_EN;
> > + sensor >>= SENSOR0_SHIFT;
> > +
> > + /* 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.
> > + */
> > +
> > + /* Skip if the sensor is disabled */
> > + if (sensor & 1) {
> > + ret = regmap_read(priv->tm_map, priv->sensor[0].status,
> &code);
> > + if (ret)
> > + return;
> > + upper_th_x = code >= threshold;
> > + lower_th_x = code <= threshold_low;
> > + if (upper_th_x)
> > + mask |= UPPER_STATUS_CLR;
> > + if (lower_th_x)
> > + mask |= LOWER_STATUS_CLR;
> > + if (upper_th_x || lower_th_x) {
> > + /* Notify user space */
> > + schedule_work(&priv->sensor[0].notify_work);
> > + pr_debug("Trigger (%d degrees) for sensor %d\n",
> > + code_to_mdegC(code, &priv->sensor[0]), 0);
> > + }
> > + }
> > + regmap_write(priv->tm_map, STATUS_CNTL_ADDR_8064, reg &
> mask);
> > +}
> > +
> > +static irqreturn_t tsens_isr(int irq, void *data)
> > +{
> > + struct tsens_priv *priv = data;
> > +
> > + schedule_work(&priv->tsens_work);
> > + return IRQ_HANDLED;
>
>
> Have you considered trying to reuse the regmap and interrupt handling
> infrastructure in tsens.c that I used to convert over everything after
> IP version 0.1?
>
> I started converting over 8960 but never managed to finish testing
> this[1]. I'd be happy for you to take this over and get it working so
> the 8960 doesn't end up being a completely separate driver from the
> other platforms.
>
> [1]
> https://git.linaro.org/people/amit.kucheria/kernel.git/log/?h=wrk3/tsens-
> 8960-breakage
>
Thanks a lot for the link. I started doing some test and I think the only
general
code we will be able to use will be the init_common. The get temp and
the function to convert code to decg are very different from the one used in
8960. Do you think keep a custom get temp function is good or not?
> > +}
> > +
> > +static void hw_init(struct tsens_priv *priv)
> > +{
> > + int ret;
> > + unsigned int reg_cntl = 0, reg_cfg = 0, reg_thr = 0;
> > + unsigned int reg_status_cntl = 0;
> > +
> > + regmap_read(priv->tm_map, CNTL_ADDR, ®_cntl);
> > + regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl |
> TSENS_SW_RST);
> > +
> > + reg_cntl |= SLP_CLK_ENA | (MEASURE_PERIOD << 18) |
> > + (((1 << priv->num_sensors) - 1) << SENSOR0_SHIFT);
> > + regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl);
> > + regmap_read(priv->tm_map, STATUS_CNTL_ADDR_8064,
> ®_status_cntl);
> > + reg_status_cntl |= LOWER_STATUS_CLR | UPPER_STATUS_CLR |
> > + MIN_STATUS_MASK | MAX_STATUS_MASK;
> > + regmap_write(priv->tm_map, STATUS_CNTL_ADDR_8064,
> reg_status_cntl);
> > + reg_cntl |= TSENS_EN;
> > + regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl);
> > +
> > + regmap_read(priv->tm_map, CONFIG_ADDR, ®_cfg);
> > + if (priv->num_sensors > 1)
> > + reg_cfg = (reg_cfg & ~CONFIG_MASK) | CONFIG;
> > + else
> > + reg_cfg = (reg_cfg & ~CONFIG_MASK) |
> > + (CONFIG << CONFIG_SHIFT_8660);
> > + regmap_write(priv->tm_map, CONFIG_ADDR, reg_cfg);
> > +
> > + reg_thr |= (LOWER_LIMIT_TH_8064 <<
> THRESHOLD_LOWER_LIMIT_SHIFT) |
> > + (UPPER_LIMIT_TH_8064 << THRESHOLD_UPPER_LIMIT_SHIFT)
> |
> > + (MIN_LIMIT_TH << THRESHOLD_MIN_LIMIT_SHIFT) |
> > + (MAX_LIMIT_TH << THRESHOLD_MAX_LIMIT_SHIFT);
> > +
> > + regmap_write(priv->tm_map, THRESHOLD_ADDR, reg_thr);
> > +
> > + ret = devm_request_irq(priv->dev, priv->tsens_irq, tsens_isr,
> > + IRQF_TRIGGER_RISING, "tsens_interrupt",
> > priv);
> > + if (ret < 0) {
> > + dev_err(priv->dev, "request_irq FAIL: %d", ret);
> > + return;
> > + }
> > +
> > + INIT_WORK(&priv->tsens_work, tsens_scheduler_fn);
> > +}
> > +
> > static int suspend_8960(struct tsens_priv *priv)
> > {
> > int ret;
> > @@ -191,6 +367,8 @@ static int init_8960(struct tsens_priv *priv)
> > if (i >= 5)
> > priv->sensor[i].status = S0_STATUS_ADDR + 40;
> > priv->sensor[i].status += i * 4;
> > + priv->sensor[i].slope = tsens_8960_slope[i];
> > + INIT_WORK(&priv->sensor[i].notify_work,
> notify_uspace_tsens_fn);
> > }
> >
> > reg_cntl = SW_RST;
> > @@ -241,18 +419,9 @@ static int calibrate_8960(struct tsens_priv
> *priv)
> >
> > 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;
> > + hw_init(priv);
> >
> > - slope = thermal_zone_get_slope(s->tzd);
> > - offset = CAL_MDEGC - slope * s->offset;
> > -
> > - return adc_code * slope + offset;
>
> This code move hunk belongs in a separate patch.
>
> > + return 0;
> > }
> >
> > static int get_temp_8960(const struct tsens_sensor *s, int *temp)
> > diff --git a/drivers/thermal/qcom/tsens.h
> b/drivers/thermal/qcom/tsens.h
> > index 59d01162c66a..e66048fabcc7 100644
> > --- a/drivers/thermal/qcom/tsens.h
> > +++ b/drivers/thermal/qcom/tsens.h
> > @@ -48,6 +48,7 @@ enum tsens_irq_type {
> > struct tsens_sensor {
> > struct tsens_priv *priv;
> > struct thermal_zone_device *tzd;
> > + struct work_struct notify_work;
> > int offset;
> > unsigned int hw_id;
> > int slope;
> > @@ -559,6 +560,7 @@ struct tsens_priv {
> > struct regmap *tm_map;
> > struct regmap *srot_map;
> > u32 tm_offset;
> > + u32 tsens_irq;
> >
> > /* lock for upper/lower threshold interrupts */
> > spinlock_t ul_lock;
> > @@ -568,6 +570,7 @@ struct tsens_priv {
> > struct tsens_features *feat;
> > const struct reg_field *fields;
> > const struct tsens_ops *ops;
> > + struct work_struct tsens_work;
> >
> > struct dentry *debug_root;
> > struct dentry *debug;
> > --
> > 2.27.0
> >