Date:   Tue, 21 Jul 2020 21:22:06 +0200
From:   <ansuelsmth@...il.com>
To:     "'Amit Kucheria'" <amit.kucheria@...aro.org>
Cc:     "'Rob Herring'" <robh+dt@...nel.org>,
        "'Andy Gross'" <agross@...nel.org>,
        "'Bjorn Andersson'" <bjorn.andersson@...aro.org>,
        "'Zhang Rui'" <rui.zhang@...el.com>,
        "'Daniel Lezcano'" <daniel.lezcano@...aro.org>,
        "'Michael Turquette'" <mturquette@...libre.com>,
        "'Stephen Boyd'" <sboyd@...nel.org>,
        "'Linux PM list'" <linux-pm@...r.kernel.org>,
        "'linux-arm-msm'" <linux-arm-msm@...r.kernel.org>,
        "'DTML'" <devicetree@...r.kernel.org>,
        "'Linux Kernel Mailing List'" <linux-kernel@...r.kernel.org>,
        "'linux-clk'" <linux-clk@...r.kernel.org>
Subject: R: [PATCH v4 5/7] drivers: thermal: tsens: add interrupt support for 9860 driver



> -----Messaggio originale-----
> Da: Amit Kucheria <amit.kucheria@...aro.org>
> Inviato: lunedì 20 luglio 2020 11:41
> A: Ansuel Smith <ansuelsmth@...il.com>
> Cc: Rob Herring <robh+dt@...nel.org>; Andy Gross <agross@...nel.org>;
> Bjorn Andersson <bjorn.andersson@...aro.org>; Zhang Rui
> <rui.zhang@...el.com>; Daniel Lezcano <daniel.lezcano@...aro.org>;
> Michael Turquette <mturquette@...libre.com>; Stephen Boyd
> <sboyd@...nel.org>; Linux PM list <linux-pm@...r.kernel.org>; linux-arm-
> msm <linux-arm-msm@...r.kernel.org>; DTML
> <devicetree@...r.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@...r.kernel.org>; linux-clk <linux-clk@...r.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 <ansuelsmth@...il.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 <ansuelsmth@...il.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,
> &reg);
> > +       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,
> &reg);
> > +       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, &reg_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,
> &reg_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, &reg_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
> >

Powered by blists - more mailing lists