by Amit Kucheria

[permalink] [raw]

Subject: Re: [PATCH v4 5/7] drivers: thermal: tsens: add interrupt support for 9860 driver

On Wed, Jul 22, 2020 at 12:52 AM <[email protected]> wrote:
>
>
>
> > -----Messaggio originale-----
> > Da: Amit Kucheria <[email protected]>
> > Inviato: lunedì 20 luglio 2020 11:41
> > A: Ansuel Smith <[email protected]>
> > Cc: Rob Herring <[email protected]>; Andy Gross <[email protected]>;
> > Bjorn Andersson <[email protected]>; Zhang Rui
> > <[email protected]>; Daniel Lezcano <[email protected]>;
> > Michael Turquette <[email protected]>; Stephen Boyd
> > <[email protected]>; Linux PM list <[email protected]>; linux-arm-
> > msm <[email protected]>; DTML
> > <[email protected]>; Linux Kernel Mailing List <linux-
> > [email protected]>; linux-clk <[email protected]>
> > 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 <[email protected]>
> > 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 <[email protected]>
> > > ---
> > > 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?

OK. Let's start common init code and custom get_temp and adc-to-degc functions.

Regards,
Amit