RE: [PATCH] thermal: Add msm tsens thermal sensor driver

["Narendran Rajan" <nrajan@codeaurora.com>]

> -----Original Message-----
> From: Srinivas Kandagatla [mailto:srinivas.kandagatla@linaro.org]
> Sent: Wednesday, January 28, 2015 10:06 PM
> To: Narendran Rajan; 'Narendran Rajan'; 'Zhang Rui'; 'Eduardo Valentin'
> Cc: 'Linux ARM MSM'; 'Linux PM'; 'Siddartha Mohanadoss'; 'Stephen Boyd'
> Subject: Re: [PATCH] thermal: Add msm tsens thermal sensor driver
> 
> 
> 
> On 27/01/15 22:31, Narendran Rajan wrote:
> >
> >> -----Original Message-----
> >> From: Srinivas Kandagatla [mailto:srinivas.kandagatla@linaro.org]
> >> Sent: Monday, January 26, 2015 11:15 PM
> >> To: Narendran Rajan; Zhang Rui; Eduardo Valentin
> >> Cc: Linux ARM MSM; Linux PM; Siddartha Mohanadoss; Stephen Boyd
> >> Subject: Re: [PATCH] thermal: Add msm tsens thermal sensor driver
> >>
> >> Thankyou for sending the patch..
> >> here are some comments
> >>
> >
> > Thanks Srini for the detail review.
> >
> >> On 27/01/15 04:09, Narendran Rajan wrote:
> >> ...
> >>
> ...
> 
> >>
> >>> +#include <linux/io.h>
> >>> +#include <linux/mfd/syscon.h>
> >>> +
> >>> +/* Trips: from very hot to very cold */ enum tsens_trip_type {
> >>> +	TSENS_TRIP_STAGE3,
> >>> +	TSENS_TRIP_STAGE2,
> >>> +	TSENS_TRIP_STAGE1,
> >>> +	TSENS_TRIP_STAGE0,
> >>> +	TSENS_TRIP_NUM,
> >>> +};
> >> Do you need this if trips are actually coming from DT?
> >>
> >
> > Yes, for HW driven trip points. This feature still under development
> > in the core framework.
> > Please see https://patchwork.ozlabs.org/patch/364812/
> >
> > I should have added this under THERMAL_TSENS8960_HWTRIPS as rest of
> > the code.  Thx
> >
> >>> +
> >>> +#define TSENS_CAL_MDEGC				30000
> >>> +
> >>> +#define TSENS_MAX_SENSORS			11
> >>> +
> >>> +#define TSENS_8960_CONFIG_ADDR			0x3640
> >>> +#define TSENS_8960_CONFIG			0x9b
> >>> +#define TSENS_8960_CONFIG_MASK			0xf
> >>> +
> >>> +#define TSENS_CNTL_ADDR				0x3620
> >>> +#define TSENS_CNTL_RESUME_MASK			0xfffffff9
> >>> +#define TSENS_EN				BIT(0)
> >>> +#define TSENS_SW_RST				BIT(1)
> >>> +#define SENSOR0_EN				BIT(3)
> >>> +#define TSENS_MIN_STATUS_MASK			BIT(0)
> >>> +#define TSENS_LOWER_STATUS_CLR			BIT(1)
> >>> +#define TSENS_UPPER_STATUS_CLR			BIT(2)
> >>> +#define TSENS_MAX_STATUS_MASK			BIT(3)
> >>> +#define TSENS_MEASURE_PERIOD			1
> >>> +#define TSENS_8960_SLP_CLK_ENA			BIT(26)
> >>> +#define TSENS_8660_SLP_CLK_ENA			BIT(24)
> >>> +#define TSENS_8064_STATUS_CNTL			0x3660
> >>> +
> >>> +#define TSENS_THRESHOLD_ADDR			0x3624
> >>> +#define TSENS_THRESHOLD_MAX_CODE		0xff
> >>> +#define TSENS_THRESHOLD_MIN_CODE		0
> >>> +#define TSENS_THRESHOLD_MAX_LIMIT_SHIFT		24
> >>> +#define TSENS_THRESHOLD_MIN_LIMIT_SHIFT		16
> >>> +#define TSENS_THRESHOLD_UPPER_LIMIT_SHIFT	8
> >>> +#define TSENS_THRESHOLD_LOWER_LIMIT_SHIFT	0
> >>> +
> >>> +/* Initial temperature threshold values */
> >>> +#define TSENS_LOWER_LIMIT_TH			0x50
> >>> +#define TSENS_UPPER_LIMIT_TH			0xdf
> >>> +#define TSENS_MIN_LIMIT_TH			0x0
> >>> +#define TSENS_MAX_LIMIT_TH			0xff
> >>> +
> >>> +#define TSENS_S0_STATUS_ADDR			0x3628
> >>> +
> >>> +#define TSENS_INT_STATUS_ADDR			0x363c
> >>> +#define TSENS_LOWER_INT_MASK			BIT(1)
> >>> +#define TSENS_UPPER_INT_MASK			BIT(2)
> >>> +#define TSENS_MAX_INT_MASK			BIT(3)
> >>> +#define TSENS_TRDY_MASK				BIT(7)
> >>> +
> >>> +#define TSENS_SENSOR_SHIFT			16
> >>> +#define TSENS_REDUND_SHIFT			24
> >>> +#define TSENS_SENSOR0_SHIFT			3
> >>> +
> >>> +#define TSENS_8660_QFPROM_ADDR			0x00bc
> >> ?? Why is this offset defined in the driver, Is'nt this supposed to
> >> come
> > via dt?
> 
> ???

Correct, needs to be removed.

> >>> +#define TSENS_8660_CONFIG			1
> >>> +#define TSENS_8660_CONFIG_SHIFT			28
> >>> +#define TSENS_8660_CONFIG_MASK			(3 <<
> >> TSENS_8660_CONFIG_SHIFT)
> >>> +
> >>> +struct tsens_device;
> >>> +
> >>> +struct tsens_sensor {
> >>> +	struct thermal_zone_device	*tz_dev;
> >>> +	enum thermal_device_mode	mode;
> >>> +	unsigned int			sensor_num;
> >>> +	int				offset;
> >>> +	u32				slope;
> >>> +	struct tsens_device		*tmdev;
> >>> +	u32                             status;
> >>> +};
> >>> +
> >>> +struct tsens_device {
> >>> +	bool			prev_reading_avail;
> >>> +	unsigned int		num_sensors;
> >>> +	int			pm_tsens_thr_data;
> >>> +	int			pm_tsens_cntl;
> >>> +	unsigned int            calib_offset;
> >>> +	unsigned int            backup_calib_offset;
> >>> +	struct work_struct	tsens_work;
> >>> +	struct regmap		*map;
> >>> +	struct regmap_field	*status_field;
> >>> +	struct tsens_sensor	sensor[0];
> >>> +};
> >>> +
> >>> +static struct device *tsens_dev;
> >> Hmm.. I think you should remove this global variable and find a
> >> better way
> > to
> >> get hold of this.
> >>
> > Didn't find anything simple enough. A few other drivers seems to use
> > global as well. Will look around. If you have some quick tips let me
please
> know.
> 
> Add this to struct tsens_device.
> 

Thx, this would work.

> >
> ...
> 
> >>> +#ifdef THERMAL_TSENS8960_HWTRIPS
> >>
> >> Should this be a kernel config?
> >>    Or by default this should'nt be On?
> >> Do you need this code if the trip values are actually coming from DT?
> >>
> >
> > This feature need thermal core framework change as well.
> > This is not yet landed in upstream, but is used in some downstream
> > Kernels (like chrome kernel). Hence under #ifdef and disabled by
default.
> >
> 
> Am not sure if this is right thing to do it now, the email thread you
pointed to
> be last updated in 1st Aug 2014 .

Agree, the both Stephen and you have pointed out not to base the code on
this work.
Let me redo the patch to remove the dependency on HWTRIPs and have just a
polling based 
Solution.

> >>
> >>> +static int
> >>> +tsens_calib_sensors8960(struct tsens_device *tmdev, struct regmap
> >>> +*map) {
> >>> +	int i;
> >>> +	u32 temp_data[TSENS_MAX_SENSORS];
> >>> +	u8 *byte_data;
> >>> +	u32 fuse, redun, num_read;
> >>> +	struct tsens_sensor *s = tmdev->sensor;
> >>> +
> >>> +	fuse = tmdev->calib_offset;
> >>> +	redun = tmdev->backup_calib_offset;
> >>> +
> >>> +	/**
> >>> +	* syscon regmap is 32-bit data, but calibration data is 8-bit.
> >>> +	* read 4 bytes from regmap in a loop and then extract bytes
> >> seprately
> >>> +	*/
> >>> +
> >> It looks like workaround for the problem in syscon driver.
> >> Please use this patch to fix it.
> >>
> > Unfortunately, this won't work. The same imem region as read as 32-bit
> > for OPP table chip characterization. Looks like this workaround needs
> > to stay Please see the patch below for reference
> > https://chromium-review.googlesource.com/#/c/231792/6
> 
> Does'nt matter If we can read a byte we can also read a word, use
> regmap_read_bulk().
> 
> Again the big plan is to abstract the qfprom access this into something
like
> http://www.spinics.net/lists/linux-arm-msm/msg13090.html
> 
> This will take care of all the code related to qfprom in the drivers.
> Please have a look and give us some comments so that we know whats best.
> 
> ...

My mistake, stride solution is indeed elegant. I just need to put a patch to
change 
the above patch in reference to read 4 bytes.

> 
> >>> +
> >>> +static int tsens_register(struct tsens_device *tmdev, int i) {
> >>> +	char name[18];
> >>> +	u32 addr = TSENS_S0_STATUS_ADDR;
> >>> +	struct tsens_sensor *s = &tmdev->sensor[i];
> >>> +
> >>> +	/*
> >>> +	* 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.
> >>> +	*/
> >>> +	if (i >= 5)
> >>> +		addr += 40;
> >>
> >> This magic values should be going in to some descriptive constants..
> >>
> > There is descriptive comment just above and this is not used anywhere
> else.
> > Perhaps this is easier to read?
> This seems to be like an register offset, So I would prefer it to be a
nice
> #define

Fair enough.

> >
> >>> +
> >>> +	addr += i * 4;
> >>> +
> >>> +	snprintf(name, sizeof(name), "tsens_tz_sensor%d", i);
> >> Hmm where is the name actually used??
> >
> > Oops, not needed in the of based registration. Will remove
> >
> >>
> >>> +	s->mode = THERMAL_DEVICE_ENABLED;
> >>> +	s->sensor_num = i;
> >>> +	s->status = addr;
> >>> +	s->tmdev = tmdev;
> >>> +	s->tz_dev = thermal_zone_of_sensor_register(tsens_dev, i, s,
> >>> +						&tsens_thermal_of_ops);
> >>> +
> >>> +	if (IS_ERR(s->tz_dev))
> >>> +		return -ENODEV;
> >> So does that mean that driver would not register tsens unless there
> >> is a
> > DT
> >> node for it?
> >> For apq8064 which has 11 sensors, its becomes necessary to add all 11
> >> sensors in DT with trip points and cooling devices. Which IMO is not
> > right.
> >> You could probably do something like:
> >>
> >> 	s->tz_dev = thermal_zone_of_sensor_register(tsens_dev, i, s,
> >> 					&tsens_thermal_of_ops);
> >>
> >> 	if (IS_ERR(s->tz_dev)) {
> >> 		s->tz_dev = thermal_zone_device_register( ... )
> >> 		if (IS_ERR(s->tz_dev))
> >> 			return -ENODEV;
> >> 	}
> >>
> >> same thing in faliure/remove case ..
> >>
> >
> > Not really. If you look at further down where tsens_register is called
> > (around ln 752), only primary register Is mandatory. If other sensor
> > registration fails, the code just  disables those sensors. Code
> > snipped pasted below
> 
> I think you missed my point.
> Ok, I agree sensor 0 is primary one and should always be registered.
> 
> What am saying is Lets say we have 11 sensors ex: in APQ8064.
> 
> Now we have got dt entries for sensor0 and sensor6.
> Your code would only register sensor0 and sensor6, because you only
> considered DT nodes.
> This is not correct, as we are not short of any resources to populate the
other
> 9 sensors. You should still register all the 11 sensors but 2 of them are
> comming from dt with there own trip point values and the rest of them are
> registered as regular non dt thermal sensors.
> 
> Doing this way would give the user to see all the sensors via sysfs If not
you
> are loosing the real value of all the 9 sensors in the system.
> 
> have a look at other drivers like TI you will understand what Am saying.
> 

Ah, thx for the clarification. 
Alternate thought : 
What if the system want to leverage only say 2 sensors out of 11. The user
has the flexibility 
to select the sensors by specifying this in the dt node. If I do what is
done in TI drivers, all sensors
get registered and polled regardless of this being used in the userspace
thermal daemon.

As a data point, the user-space daemon that exists seems to use only a sub
set of sensors.

> 
> > --------------------->cut<----------------------
> > 	/*
> > 	 * Register sensor 0 separately. This sensor is always
> > 	 * expected to be present and if this fails, thermal
> > 	 * sensor probe would fail
> > 	 * Other sensors are optional and if registration fails
> > 	 * disable the sensor and continue
> > 	*/
> > 	ret = tsens_register(tmdev, 0);
> > 	if (ret < 0) {
> > 		dev_err(tsens_dev, "Registering failed for primary sensor");
> > 		ret = -ENODEV;
> > 		goto fail;
> > 	} else {
> > 		tsens_tz_set_mode(&tmdev->sensor[0],
> > THERMAL_DEVICE_ENABLED);
> > 	}
> > --------------------->cut<----------------------
> >
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int tsens_probe(struct platform_device *pdev) {
> >>> +	struct device_node *np = pdev->dev.of_node;
> >>> +	struct device_node *base_node;
> >>> +	struct platform_device *base_pdev;
> >>> +	int ret, i, irq, num;
> >>> +	struct tsens_sensor *s;
> >>> +	struct tsens_device *tmdev;
> >>> +	struct regmap *map, *imem_regmap;
> >>> +	struct reg_field *field;
> >>> +	static struct reg_field status_0 = {
> >>> +		.reg = TSENS_8064_STATUS_CNTL,
> >>> +		.lsb = 0,
> >>> +		.msb = 3,
> >>> +	};
> >>> +	static struct reg_field status_8 = {
> >>> +		.reg = TSENS_CNTL_ADDR,
> >>> +		.lsb = 8,
> >>> +		.msb = 11,
> >>> +	};
> >>> +
> >>> +	tsens_dev = &pdev->dev;
> >>> +
> >>> +	num = of_property_count_u32_elems(np, "qcom,tsens-slopes");
> >>> +	if (num <= 0) {
> >>> +		dev_err(tsens_dev, "invalid tsens slopes\n");
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	tmdev = devm_kzalloc(tsens_dev, sizeof(*tmdev) +
> >>> +			     num * sizeof(struct tsens_sensor), GFP_KERNEL);
> >>> +	if (tmdev == NULL)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	tmdev->num_sensors = num;
> >>> +	for (i = 0, s = tmdev->sensor; i < num; i++, s++)
> >>> +		of_property_read_u32_index(np, "qcom,tsens-slopes", i,
> >>> +					   &s->slope);
> >>> +
> >>> +	irq = platform_get_irq(pdev, 0);
> >>> +	if (irq < 0) {
> >>> +		dev_err(tsens_dev,  "no irq resource?\n");
> >>> +		return  -EINVAL;
> >>> +	}
> >>> +
> >>> +	ret = of_property_read_u32_index(np, "qcom,calib-offsets", 0,
> >>> +					   &tmdev->calib_offset);
> >>> +	if (ret != 0) {
> >>> +		dev_err(tsens_dev,  "No calibration offset set\n");
> >>> +		return  -EINVAL;
> >>> +	}
> >>> +
> >>> +	ret = of_property_read_u32_index(np, "qcom,calib-offsets", 1,
> >>> +					   &tmdev->backup_calib_offset);
> >>> +	if (ret) {
> >>> +		dev_info(tsens_dev, "Missing backup calibration offset\n");
> >>> +		tmdev->backup_calib_offset = tmdev->calib_offset;
> >>> +	}
> >>> +
> >>> +	imem_regmap = syscon_regmap_lookup_by_phandle(np,
> >> "qcom,imem");
> >>> +	if (IS_ERR(imem_regmap)) {
> >>> +		dev_err(tsens_dev, "syscon regmap look up error\n");
> >>> +		return PTR_ERR(imem_regmap);
> >>> +	}
> >>> +
> >> Please see my comments in the bindings patch.
> >
> > As mentioned above, the syscon needs to be 32-bit for other
> > calibration data, so I guess no option.
> >
> > On base_regmap, I am good with either ways. Will wait for your further
> > comments and update accordingly in next revision.
> have a look at http://www.spinics.net/lists/linux-arm-msm/msg13090.html

Agree, my error.

> >
> >>
> >>> +	if (num == 1)
> >>> +		ret = tsens_calib_sensors8660(tmdev, imem_regmap);
> >>> +	else
> >>> +		ret = tsens_calib_sensors8960(tmdev, imem_regmap);
> >>> +
> >>> +	if (ret < 0) {
> >>> +		dev_err(tsens_dev, "tsense calibration failed\n");
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	base_node = of_parse_phandle(np, "qcom,tsens-base", 0);
> >>> +	if (base_node == NULL) {
> >>> +		dev_err(tsens_dev, "no base node present\n");
> >>> +		return  -EINVAL;
> >>> +	}
> >>> +
> >>> +	base_pdev = of_find_device_by_node(base_node);
> >>> +	if (base_pdev == NULL) {
> >>> +		dev_err(tsens_dev, "no base pdev node\n");
> >>> +		return  -ENODEV;
> >>> +	}
> >>> +
> >>> +	tmdev->map = map = dev_get_regmap(&base_pdev->dev, NULL);
> >>> +	if (map == NULL) {
> >>> +		dev_err(tsens_dev, "base regmap get failed\n");
> >>> +		return  -ENODEV;
> >>> +	}
> >>> +
> >>> +	/* Status bits move when the sensor bits next to them overlap */
> >>> +	if (num > 5)
> >>> +		field = &status_0;
> >>> +	else
> >>> +		field = &status_8;
> >>> +
> >>> +	tmdev->status_field = devm_regmap_field_alloc(tsens_dev, map,
> >> *field);
> >>> +	if (IS_ERR(tmdev->status_field)) {
> >>> +		dev_err(tsens_dev, "regmap alloc failed\n");
> >>> +		return PTR_ERR(tmdev->status_field);
> >>> +	}
> >>> +
> >>> +	tsens_hw_init(tmdev);
> >>> +
> >>> +	/*
> >>> +	 * Register sensor 0 separately. This sensor is always
> >>> +	 * expected to be present and if this fails, thermal
> >>> +	 * sensor probe would fail
> >>> +	 * Other sensors are optional and if registration fails
> >>> +	 * disable the sensor and continue
> >>> +	*/
> >>> +	ret = tsens_register(tmdev, 0);
> >>> +	if (ret < 0) {
> >>> +		dev_err(tsens_dev, "Registering failed for primary sensor");
> >>> +		ret = -ENODEV;
> >>> +		goto fail;
> >>> +	} else {
> >>> +		tsens_tz_set_mode(&tmdev->sensor[0],
> >> THERMAL_DEVICE_ENABLED);
> >>> +	}
> >>> +
> >>> +	for (i = 1;  i < tmdev->num_sensors; i++) {
> >>> +		ret = tsens_register(tmdev, i);
> >>> +
> >>> +		if (ret < 0) {
> >>> +			dev_err(tsens_dev,
> >>> +				"Registering failed. Sensor(%i), disabled",
> > i);
> >>> +			tsens_tz_set_mode(&tmdev->sensor[i],
> >>> +				THERMAL_DEVICE_DISABLED);
> >>> +		} else {
> >>> +			tsens_tz_set_mode(&tmdev->sensor[i],
> >>> +				THERMAL_DEVICE_ENABLED);
> >>> +		}
> >>> +	}
> >>> +
> >>> +	INIT_WORK(&tmdev->tsens_work, tsens_scheduler_fn);
> >>> +
> >>> +	ret = devm_request_irq(tsens_dev, irq, tsens_isr,
> >> IRQF_TRIGGER_RISING,
> >>> +			       "tsens", &tmdev->tsens_work);
> >> Unless I missing something,...Would it actually get an interrupt
> >> without setting the proper trip values in the hw? For example in this
> >> driver we
> > only
> >> register from DT.. and the values from Dt never ends up writing into
> >> the
> > hw
> >> registers.. so this would never trigger an interrupt from trip points
> >> mentioned in DT. Unless the default reg values makes sense..
> >> still its out of sync from DT trip values.
> >>
> > Correct, in polling mode (which is what exists in thermal framework
> > today), HW interrupt do not make sense as the trip points are set to
> > default and never updated based on Dt values.
> >
> > But the code under the #ifdef THERMAL_TSENS8960_HWTRIPS supports
> the
> > HW trip point mode.
> > This code needs the additional patch
> > Please see https://patchwork.ozlabs.org/patch/364812/
> >
> > May be I will remove everything under HWTRIPs until it lands in the
> > core thermal framework?
> >
> >> Does the interrupt actually work? Last time when I tested it on 3.4
> >> It
> > never
> >> did work?..
> 
> ???

The code was indeed running in polling mode and interrupts are not getting
triggered on 
3.14 as well. 
I need to go back and check the older version to see if I am missing some
register
setting. From a register map, it has only documented interrupt status, not
enable/disable.

> 
> How do you enable/disable the interrupt in the tses IP, I did not find any
> register to do that?
> Do we have one?
> >>> +	if (ret < 0)
> >>> +		goto err_irq;
> >>> +
> >>> +	platform_set_drvdata(pdev, tmdev);
> >>> +
> >>> +	dev_info(tsens_dev, "Tsens driver initialized\n");
> >>> +
> >>> +	return 0;
> >>> +err_irq:
> >>> +	for (i = 0, s = tmdev->sensor; i < tmdev->num_sensors; i++, s++)
> >>> +		thermal_zone_of_sensor_unregister(&pdev->dev, s-
> >>> tz_dev);
> >>> +fail:
> >>> +	tsens_disable_mode(tmdev);
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +static int tsens_remove(struct platform_device *pdev) {
> >>> +	int i;
> >>> +	struct tsens_sensor *s;
> >>> +	struct tsens_device *tmdev = platform_get_drvdata(pdev);
> >>> +
> >>> +	tsens_disable_mode(tmdev);
> >>> +	for (i = 0, s = tmdev->sensor; i < tmdev->num_sensors; i++, s++)
> >>> +		thermal_zone_device_unregister(s->tz_dev);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static struct of_device_id tsens_match_table[] = {
> >>> +	{.compatible = "qcom,ipq806x-tsens"},
> >> should be "qcom,msm8960-tsens"
> >>
> >
> > Thx, will address in next version.
> >
> >>> +	{},
> >>> +};
> >>> +
> >>> +MODULE_DEVICE_TABLE(of, tsens_match_table);
> >>> +
> >>> +static struct platform_driver tsens_driver = {
> >>> +	.probe = tsens_probe,
> >>> +	.remove = tsens_remove,
> >>> +	.driver = {
> >>> +		.of_match_table = tsens_match_table,
> >>> +		.name = "tsens8960-thermal",
> >>> +		.owner = THIS_MODULE,
> >> remove the owner as it would be set anyway byt the core.
> >
> > Thx, will address in next version.
> >
> >>> +#ifdef CONFIG_PM
> >>> +		.pm	= &tsens_pm_ops,
> >>> +#endif
> >> Please use SIMPLE_DEV_PM_OPS(...)
> >>
> >
> > Thx, will address in next version.
> >
> >>> +	},
> >>> +};
> >>> +module_platform_driver(tsens_driver);
> >>> +
> >>> +MODULE_LICENSE("GPL v2");
> >>> +MODULE_DESCRIPTION("Temperature Sensor driver");
> >>> +MODULE_ALIAS("platform:tsens8960-tm");
> >>>
> >> --srini
> >

--Naren