Message-ID: <987f9f02-151c-22a6-6afc-0c7a17e1da62@ti.com>
Date:   Mon, 24 Feb 2020 06:46:51 +0530
From:   "J, KEERTHY" <j-keerthy@...com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
CC:     <rui.zhang@...el.com>, <robh+dt@...nel.org>,
        <amit.kucheria@...durent.com>, <t-kristo@...com>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-pm@...r.kernel.org>,
        <mark.rutland@....com>
Subject: Re: [PATCH 2/4] thermal: k3: Add support for bandgap sensors



On 2/21/2020 3:43 AM, Daniel Lezcano wrote:
> On Thu, Feb 13, 2020 at 03:54:38PM +0530, Keerthy wrote:
>> The bandgap provides current and voltage reference for its internal
>> circuits and other analog IP blocks. The analog-to-digital
>> converter (ADC) produces an output value that is proportional
>> to the silicon temperature.
>>
>> Currently reading temperatures and trend computing is supported
>> as there are no active/passive cooling agent supported.
>>
>> Signed-off-by: Keerthy <j-keerthy@...com>
>> ---
>>   drivers/thermal/Kconfig      |  12 ++
>>   drivers/thermal/Makefile     |   1 +
>>   drivers/thermal/k3_bandgap.c | 342 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 355 insertions(+)
>>   create mode 100644 drivers/thermal/k3_bandgap.c
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index 5a05db5438d6..fa598eddc7ac 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -251,6 +251,18 @@ config IMX_THERMAL
>>   	  cpufreq is used as the cooling device to throttle CPUs when the
>>   	  passive trip is crossed.
>>   
>> +config K3_THERMAL
>> +	bool "Texas Instruments K3 thermal support"
>> +	depends on THERMAL
> 
> All the Kconfig is under the THERMAL option, so this dependency is not
> necessary.

Okay

> 
>> +	depends on ARCH_K3 || COMPILE_TEST
>> +	help
>> +	  If you say yes here you get thermal support for the Texas Instruments
>> +	  K3 SoC family. The current chip supported is:
>> +	   - AM654
>> +
>> +	  This includes temperature reading functionality and also trend
>> +	  computation.
>> +
> 
> [ ... ]
> 
>> +
>> +#define K3_VTM_ADC_BEGIN_VAL		540
>> +#define K3_VTM_ADC_END_VAL		944
>> +
>> +static const int k3_adc_to_temp[K3_VTM_ADC_END_VAL - K3_VTM_ADC_BEGIN_VAL
>> +				+ 1] = {
> 
> No need to specify a size for the array that can be done with:
> 
> static const int k3_adc_to_temp[] = {
> 
> And then use ARRAY_SIZE macro.

Okay. I will address all the comments mentioned below.

Thanks for the review.

- Keerthy

> 
>> +	-40000, -40000, -40000, -40000, -39800, -39400, -39000, -38600, -38200,
>> +	-37800, -37400, -37000, -36600, -36200, -35800, -35300, -34700, -34200,
>> +	-33800, -33400, -33000, -32600, -32200, -31800, -31400, -31000, -30600,
>> +	-30200, -29800, -29400, -29000, -28600, -28200, -27700, -27100, -26600,
>> +	-26200, -25800, -25400, -25000, -24600, -24200, -23800, -23400, -23000,
>> +	-22600, -22200, -21800, -21400, -21000, -20500, -19900, -19400, -19000,
>> +	-18600, -18200, -17800, -17400, -17000, -16600, -16200, -15800, -15400,
>> +	-15000, -14600, -14200, -13800, -13400, -13000, -12500, -11900, -11400,
>> +	-11000, -10600, -10200, -9800, -9400, -9000, -8600, -8200, -7800, -7400,
>> +	-7000, -6600, -6200, -5800, -5400, -5000, -4500, -3900, -3400, -3000,
>> +	-2600, -2200, -1800, -1400, -1000, -600, -200, 200, 600, 1000, 1400,
>> +	1800, 2200, 2600, 3000, 3400, 3900, 4500, 5000, 5400, 5800, 6200, 6600,
>> +	7000, 7400, 7800, 8200, 8600, 9000, 9400, 9800, 10200, 10600, 11000,
>> +	11400, 11800, 12200, 12700, 13300, 13800, 14200, 14600, 15000, 15400,
>> +	15800, 16200, 16600, 17000, 17400, 17800, 18200, 18600, 19000, 19400,
>> +	19800, 20200, 20600, 21000, 21400, 21900, 22500, 23000, 23400, 23800,
>> +	24200, 24600, 25000, 25400, 25800, 26200, 26600, 27000, 27400, 27800,
>> +	28200, 28600, 29000, 29400, 29800, 30200, 30600, 31000, 31400, 31900,
>> +	32500, 33000, 33400, 33800, 34200, 34600, 35000, 35400, 35800, 36200,
>> +	36600, 37000, 37400, 37800, 38200, 38600, 39000, 39400, 39800, 40200,
>> +	40600, 41000, 41400, 41800, 42200, 42600, 43100, 43700, 44200, 44600,
>> +	45000, 45400, 45800, 46200, 46600, 47000, 47400, 47800, 48200, 48600,
>> +	49000, 49400, 49800, 50200, 50600, 51000, 51400, 51800, 52200, 52600,
>> +	53000, 53400, 53800, 54200, 54600, 55000, 55400, 55900, 56500, 57000,
>> +	57400, 57800, 58200, 58600, 59000, 59400, 59800, 60200, 60600, 61000,
>> +	61400, 61800, 62200, 62600, 63000, 63400, 63800, 64200, 64600, 65000,
>> +	65400, 65800, 66200, 66600, 67000, 67400, 67800, 68200, 68600, 69000,
>> +	69400, 69800, 70200, 70600, 71000, 71500, 72100, 72600, 73000, 73400,
>> +	73800, 74200, 74600, 75000, 75400, 75800, 76200, 76600, 77000, 77400,
>> +	77800, 78200, 78600, 79000, 79400, 79800, 80200, 80600, 81000, 81400,
>> +	81800, 82200, 82600, 83000, 83400, 83800, 84200, 84600, 85000, 85400,
>> +	85800, 86200, 86600, 87000, 87400, 87800, 88200, 88600, 89000, 89400,
>> +	89800, 90200, 90600, 91000, 91400, 91800, 92200, 92600, 93000, 93400,
>> +	93800, 94200, 94600, 95000, 95400, 95800, 96200, 96600, 97000, 97500,
>> +	98100, 98600, 99000, 99400, 99800, 100200, 100600, 101000, 101400,
>> +	101800, 102200, 102600, 103000, 103400, 103800, 104200, 104600, 105000,
>> +	105400, 105800, 106200, 106600, 107000, 107400, 107800, 108200, 108600,
>> +	109000, 109400, 109800, 110200, 110600, 111000, 111400, 111800, 112200,
>> +	112600, 113000, 113400, 113800, 114200, 114600, 115000, 115400, 115800,
>> +	116200, 116600, 117000, 117400, 117800, 118200, 118600, 119000, 119400,
>> +	119800, 120200, 120600, 121000, 121400, 121800, 122200, 122600, 123000,
>> +	123400, 123800, 124200, 124600, 124900, 125000,
>> +};
>> +
>> +struct k3_thermal_data;
>> +
>> +struct k3_bandgap {
>> +	struct device *dev;
> 
> This field is useless, the function using it can use the local dev variable
> 
>> +	void __iomem *base;
>> +	const struct k3_bandgap_data *conf;
>> +	spinlock_t lock; /* shields this struct */
> 
> Where is used this lock?
> 
>> +	int ts_cnt;
> 
> This field is unused.
> 
>> +	u32 errata;
>> +	struct k3_thermal_data *ts_data[K3_VTM_MAX_NUM_TS];
> 
> This field is unused.
> 
>> +};
>> +
>> +struct k3_vtm_driver_data {
>> +	u32 errata;
>> +};
>> +
>> +/* common data structures */
>> +struct k3_thermal_data {
>> +	struct thermal_zone_device *ti_thermal;
>> +	struct thermal_cooling_device *cool_dev;
> 
> This field is unused
> 
>> +	struct k3_bandgap *bgp;
>> +	enum thermal_device_mode mode;
> 
> This field is unused
> 
>> +	struct work_struct thermal_wq;
> 
> Where is used this workq?
> 
>> +	int sensor_id;
>> +	u32 ctrl_offset;
>> +	u32 stat_offset;
>> +	int prev_temp;
>> +};
>> +
>> +static unsigned int vtm_get_best_value(unsigned int s0, unsigned int s1,
>> +				       unsigned int s2)
>> +{
>> +	int d01 = abs(s0 - s1);
>> +	int d02 = abs(s0 - s2);
>> +	int d12 = abs(s1 - s2);
>> +
>> +	if (d01 <= d02 && d01 <= d12)
>> +		return (s0 + s1) / 2;
>> +
>> +	if (d02 <= d01 && d02 <= d12)
>> +		return (s0 + s2) / 2;
>> +
>> +	return (s1 + s2) / 2;
>> +}
>> +
>> +static int k3_bgp_read_temp(struct k3_thermal_data *devdata,
>> +			    int *temp)
>> +{
>> +	struct k3_bandgap *bgp;
>> +	unsigned int dtemp, s0, s1, s2;
>> +
>> +	bgp = devdata->bgp;
> 
> nit: missing line
> 
>> +	/**
>> +	 * Errata is applicable for am654 pg 1.0 silicon. There
>> +	 * is a variation of the order for 8-10 degree centigrade.
>> +	 * Work around that by getting the average of two closest
>> +	 * readings out of three readings everytime we want to
>> +	 * report temperatures.
>> +	 *
>> +	 * Errata workaround.
>> +	 */
> 
> nit: extra line
> 
>> +	if (bgp->errata) {
> 
> Right now only am654 is supported and has the errata. This test is pointless
> because no other compatible string is defined. If you want to set the scene for
> more platforms I suggest to add a get_temp function in the drvdata which does
> this 3 points averaging and get rid of this test (and the errata field).
> 
>> +		s0 = readl(bgp->base + devdata->stat_offset) &
>> +			K3_VTM_TS_STAT_DTEMP_MASK;
>> +		s1 = readl(bgp->base + devdata->stat_offset) &
>> +			K3_VTM_TS_STAT_DTEMP_MASK;
>> +		s2 = readl(bgp->base + devdata->stat_offset) &
>> +			K3_VTM_TS_STAT_DTEMP_MASK;
>> +		dtemp = vtm_get_best_value(s0, s1, s2);
>> +	} else {
>> +		dtemp = readl(bgp->base + devdata->stat_offset) &
>> +				K3_VTM_TS_STAT_DTEMP_MASK;
>> +	}
>> +
>> +	if (dtemp < K3_VTM_ADC_BEGIN_VAL || dtemp > K3_VTM_ADC_END_VAL)
>> +		return -EINVAL;
>> +
>> +	*temp = k3_adc_to_temp[dtemp - K3_VTM_ADC_BEGIN_VAL];
>> +
>> +	return 0;
>> +}
>> +
>> +/* thermal zone ops */
>> +/* Get temperature callback function for thermal zone */
> 
> Fix comment format
> 
> /*
>   *
>   */
> 
>> +static int k3_thermal_get_temp(void *devdata, int *temp)
>> +{
>> +	struct k3_thermal_data *data = devdata;
>> +	int ret = 0;
>> +
>> +	ret = k3_bgp_read_temp(data, temp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	data->prev_temp = *temp;
>> +
>> +	return ret;
>> +}
>> +
>> +static int k3_thermal_get_trend(void *p, int trip, enum thermal_trend *trend)
>> +{
>> +	struct k3_thermal_data *data = p;
>> +	struct k3_bandgap *bgp;
>> +	int ret = 0, temp = 0;
>> +
>> +	bgp = data->bgp;
>> +
>> +	ret = k3_bgp_read_temp(data, &temp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (temp > data->prev_temp)
>> +		*trend = THERMAL_TREND_RAISING;
>> +	else if (temp < data->prev_temp)
>> +		*trend = THERMAL_TREND_DROPPING;
>> +	else
>> +		*trend = THERMAL_TREND_STABLE;
>> +
>> +	return 0;
>> +}
> 
> This function get_trend() is not really useful, it does what the governors do.
> 
> It can be dropped.
> 
>> +static const struct thermal_zone_of_device_ops k3_of_thermal_ops = {
>> +	.get_temp = k3_thermal_get_temp,
>> +	.get_trend = k3_thermal_get_trend,
>> +};
>> +
>> +static void k3_thermal_work(struct work_struct *work)
>> +{
>> +	struct k3_thermal_data *data = container_of(work,
>> +					struct k3_thermal_data, thermal_wq);
>> +
>> +	thermal_zone_device_update(data->ti_thermal, THERMAL_EVENT_UNSPECIFIED);
>> +
>> +	dev_dbg(&data->ti_thermal->device, "updated thermal zone %s\n",
>> +		data->ti_thermal->type);
>> +}
>> +
>> +static const struct of_device_id of_k3_bandgap_match[];
>> +
>> +static int k3_bandgap_probe(struct platform_device *pdev)
>> +{
>> +	int ret = 0, cnt, val, id, reg_cnt = 0;
>> +	struct resource *res;
>> +	struct device *dev = &pdev->dev;
>> +	struct k3_bandgap *bgp;
>> +	struct k3_thermal_data *data;
>> +	const struct k3_vtm_driver_data *drv_data;
>> +
>> +	bgp = devm_kzalloc(&pdev->dev, sizeof(*bgp), GFP_KERNEL);
>> +	if (!bgp)
>> +		return -ENOMEM;
>> +
>> +	drv_data = of_device_get_match_data(&pdev->dev);
>> +	if (drv_data)
>> +		bgp->errata = drv_data->errata;
>> +
>> +	bgp->dev = dev;
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	bgp->base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(bgp->base))
>> +		return PTR_ERR(bgp->base);
>> +
>> +	pm_runtime_enable(dev);
>> +	ret = pm_runtime_get_sync(dev);
>> +	if (ret < 0) {
>> +		pm_runtime_put_noidle(dev);
>> +		pm_runtime_disable(dev);
>> +		return ret;
>> +	}
>> +
>> +	/* Get the sensor count in the VTM */
>> +	val = readl(bgp->base + K3_VTM_DEVINFO_PWR0_OFFSET);
>> +	cnt = val & K3_VTM_DEVINFO_PWR0_TEMPSENS_CT_MASK;
>> +	cnt >>= __ffs(K3_VTM_DEVINFO_PWR0_TEMPSENS_CT_MASK);
>> +	bgp->ts_cnt = cnt;
>> +
>> +	data = devm_kcalloc(bgp->dev, cnt, sizeof(*data), GFP_KERNEL);
>> +	if (!data) {
>> +		ret = -ENOMEM;
>> +		goto err_alloc;
>> +	}
>> +
>> +	/* Register the thermal sensors */
>> +	for (id = 0; id < cnt; id++) {
>> +		data[id].sensor_id = id;
>> +		data[id].bgp = bgp;
>> +		data[id].ctrl_offset = K3_VTM_TMPSENS0_CTRL_OFFSET +
>> +					id * K3_VTM_REGS_PER_TS;
>> +		data[id].stat_offset = data[id].ctrl_offset + 0x8;
>> +		INIT_WORK(&data[id].thermal_wq, k3_thermal_work);
>> +
>> +		val = readl(data[id].bgp->base + data[id].ctrl_offset);
>> +		val |= (K3_VTM_TMPSENS_CTRL_SOC |
>> +			K3_VTM_TMPSENS_CTRL_CLRZ |
>> +			K3_VTM_TMPSENS_CTRL_CLKON_REQ);
>> +		val &= ~K3_VTM_TMPSENS_CTRL_CBIASSEL;
>> +		writel(val, data[id].bgp->base + data[id].ctrl_offset);
>> +
>> +		bgp->ts_data[id] = &data[id];
>> +		data[id].ti_thermal =
>> +		devm_thermal_zone_of_sensor_register(bgp->dev, id,
>> +						     &data[id],
>> +						     &k3_of_thermal_ops);
>> +		if (IS_ERR(data[id].ti_thermal)) {
>> +			dev_err(bgp->dev, "thermal zone device is NULL\n");
>> +			ret = PTR_ERR(data[id].ti_thermal);
>> +			goto err_alloc;
>> +		}
>> +
>> +		reg_cnt++;
>> +
>> +		/* Initialize Previous temp */
>> +		k3_thermal_get_temp(&data[id], &data[id].prev_temp);
>> +	}
>> +
>> +	platform_set_drvdata(pdev, bgp);
>> +
>> +	return 0;
>> +
>> +err_alloc:
>> +	pm_runtime_put_sync(&pdev->dev);
>> +	pm_runtime_disable(&pdev->dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int k3_bandgap_remove(struct platform_device *pdev)
>> +{
>> +	pm_runtime_put_sync(&pdev->dev);
>> +	pm_runtime_disable(&pdev->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct k3_vtm_driver_data am654_data = {
>> +	.errata = 1,
>> +};
>> +
>> +static const struct of_device_id of_k3_bandgap_match[] = {
>> +	{
>> +		.compatible = "ti,am654-vtm",
>> +		.data = &am654_data,
>> +	},
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, of_k3_bandgap_match);
>> +
>> +static struct platform_driver k3_bandgap_sensor_driver = {
>> +	.probe = k3_bandgap_probe,
>> +	.remove = k3_bandgap_remove,
>> +	.driver = {
>> +		.name = "k3-soc-thermal",
>> +		.of_match_table	= of_k3_bandgap_match,
>> +	},
>> +};
>> +
>> +module_platform_driver(k3_bandgap_sensor_driver);
>> +
>> +MODULE_DESCRIPTION("K3 bandgap temperature sensor driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("J Keerthy <j-keerthy@...com>");
>> -- 
>> 2.17.1
>>
> 

Powered by blists - more mailing lists