Date:	Tue, 15 Mar 2016 17:12:12 +0800
From:	Wei Ni <wni@...dia.com>
To:	Eduardo Valentin <edubezval@...il.com>
CC:	<rui.zhang@...el.com>, <thierry.reding@...il.com>,
	<MLongnecker@...dia.com>, <swarren@...dotorg.org>,
	<mikko.perttunen@...si.fi>, <linux-tegra@...r.kernel.org>,
	<linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V7 09/12] thermal: tegra: add thermtrip function



On 2016年03月15日 03:16, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> On Fri, Mar 11, 2016 at 11:11:12AM +0800, Wei Ni wrote:
>> Add support for hardware critical thermal limits to the
>> SOC_THERM driver. It use the Linux thermal framework to
>> create critical trip temp, and set it to SOC_THERM hardware.
>> If these limits are breached, the chip will reset, and if
>> appropriately configured, will turn off the PMIC.
>>
>> This support is critical for safe usage of the chip.
>>
>> Signed-off-by: Wei Ni <wni@...dia.com>
>> ---
>>  drivers/thermal/tegra/soctherm.c          | 166 +++++++++++++++++++++++++++++-
>>  drivers/thermal/tegra/soctherm.h          |   7 ++
>>  drivers/thermal/tegra/tegra124-soctherm.c |  24 +++++
>>  drivers/thermal/tegra/tegra210-soctherm.c |  24 +++++
>>  4 files changed, 216 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
>> index 02ac6d2e5a20..dbaab160baba 100644
>> --- a/drivers/thermal/tegra/soctherm.c
>> +++ b/drivers/thermal/tegra/soctherm.c
>> @@ -73,9 +73,14 @@
>>  #define REG_SET_MASK(r, m, v)	(((r) & ~(m)) | \
>>  				 (((v) & (m >> (ffs(m) - 1))) << (ffs(m) - 1)))
>>  
>> +static const int min_low_temp = -127000;
>> +static const int max_high_temp = 127000;
>> +
>>  struct tegra_thermctl_zone {
>>  	void __iomem *reg;
>> -	u32 mask;
>> +	struct device *dev;
>> +	struct thermal_zone_device *tz;
> 
> 
> Why not using tz->dev for the *dev above?

The tz is thermal_zone_device, this structure doesn't have *dev.
It only have the member "struct device device;", but this device is created for
the thermal class, not this tegra_soctherm device.

> 
>> +	const struct tegra_tsensor_group *sg;
>>  };
>>  
>>  struct tegra_soctherm {
>> @@ -145,22 +150,158 @@ static int tegra_thermctl_get_temp(void *data, int *out_temp)
>>  	u32 val;
>>  
>>  	val = readl(zone->reg);
>> -	val = REG_GET_MASK(val, zone->mask);
>> +	val = REG_GET_MASK(val, zone->sg->sensor_temp_mask);
>>  	*out_temp = translate_temp(val);
>>  
>>  	return 0;
>>  }
>>  
>> +static int
>> +thermtrip_program(struct device *dev, const struct tegra_tsensor_group *sg,
>> +		  int trip_temp);
>> +
>> +static int tegra_thermctl_set_trip_temp(void *data, int trip, int temp)
>> +{
>> +	struct tegra_thermctl_zone *zone = data;
>> +	struct thermal_zone_device *tz = zone->tz;
>> +	const struct tegra_tsensor_group *sg = zone->sg;
>> +	struct device *dev = zone->dev;
>> +	enum thermal_trip_type type;
>> +	int ret;
>> +
>> +	if (!tz)
>> +		return -EINVAL;
> 
> 
> Is the above check needed? If you saw a case in which your function is
> called without tz, would it be the case we have a but in the probe (or
> even worse, in thermal-core)?

This tz isn't from thermal-core, it's from the "void *data".
This *data is the private structure "struct tegra_thermctl_zone *zone = data;".
It is registered in devm_thermal_zone_of_sensor_register(*dev, sensor_id, *data,
*ops). And when it register successful, I will set zone->tz = z, in here, the
zone is the private data.
Let's consider a special case, once the thermal_zone_of_sensor_register
successful and didn't run to "zone->tz = z" yet, then the thermal_core implement
.set_trip(), then it may cause problems in here, although it's difficult to hit
this case. So I think we need to do this check.

> 
>> +
>> +	ret = tz->ops->get_trip_type(tz, trip, &type);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (type != THERMAL_TRIP_CRITICAL)
>> +		return 0;
>> +
>> +	return thermtrip_program(dev, sg, temp);
>> +}
>> +
>>  static const struct thermal_zone_of_device_ops tegra_of_thermal_ops = {
>>  	.get_temp = tegra_thermctl_get_temp,
>> +	.set_trip_temp = tegra_thermctl_set_trip_temp,
>>  };
>>  
>> +/**
>> + * enforce_temp_range() - check and enforce temperature range [min, max]
>> + * @trip_temp: the trip temperature to check
>> + *
>> + * Checks and enforces the permitted temperature range that SOC_THERM
>> + * HW can support This is
>> + * done while taking care of precision.
>> + *
>> + * Return: The precision adjusted capped temperature in millicelsius.
>> + */
>> +static int enforce_temp_range(struct device *dev, int trip_temp)
>> +{
>> +	int temp;
>> +
>> +	temp = clamp_val(trip_temp, min_low_temp, max_high_temp);
>> +	if (temp != trip_temp)
>> +		dev_info(dev, "soctherm: trip temperature %d forced to %d\n",
>> +			 trip_temp, temp);
>> +	return temp;
>> +}
>> +
>> +/**
>> + * thermtrip_program() - Configures the hardware to shut down the
>> + * system if a given sensor group reaches a given temperature
>> + * @dev: ptr to the struct device for the SOC_THERM IP block
>> + * @sg: pointer to the sensor group to set the thermtrip temperature for
>> + * @trip_temp: the temperature in millicelsius to trigger the thermal trip at
>> + *
>> + * Sets the thermal trip threshold of the given sensor group to be the
>> + * @trip_temp.  If this threshold is crossed, the hardware will shut
>> + * down.
>> + *
>> + * Note that, although @trip_temp is specified in millicelsius, the
>> + * hardware is programmed in degrees Celsius.
>> + *
>> + * Return: 0 upon success, or %-EINVAL upon failure.
>> + */
>> +static int thermtrip_program(struct device *dev,
>> +			     const struct tegra_tsensor_group *sg,
>> +			     int trip_temp)
>> +{
>> +	struct tegra_soctherm *ts = dev_get_drvdata(dev);
>> +	int temp;
>> +	u32 r;
>> +
>> +	if (!dev || !sg)
>> +		return -EINVAL;
>> +
>> +	if (!sg->thermtrip_threshold_mask)
>> +		return -EINVAL;
>> +
>> +	temp = enforce_temp_range(dev, trip_temp) / ts->soc->thresh_grain;
>> +
>> +	r = readl(ts->regs + THERMCTL_THERMTRIP_CTL);
>> +	r = REG_SET_MASK(r, sg->thermtrip_threshold_mask, temp);
>> +	r = REG_SET_MASK(r, sg->thermtrip_enable_mask, 1);
>> +	r = REG_SET_MASK(r, sg->thermtrip_any_en_mask, 0);
>> +	writel(r, ts->regs + THERMCTL_THERMTRIP_CTL);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * tegra_soctherm_set_hwtrips() - set HW trip point from DT data
>> + * @dev: struct device * of the SOC_THERM instance
>> + *
>> + * Configure the SOC_THERM HW trip points, setting "THERMTRIP"
>> + * trip points , using "critical" type trip_temp from thermal
>> + * zone.
>> + * After they have been configured, THERMTRIP will take action
>> + * when the configured SoC thermal sensor group reaches a
>> + * certain temperature.
>> + *
>> + * Return: 0 upon success, or a negative error code on failure.
>> + * "Success" does not mean that trips was enabled; it could also
>> + * mean that no node was found in DT.
>> + * THERMTRIP has been enabled successfully when a message similar to
>> + * this one appears on the serial console:
>> + * "thermtrip: will shut down when sensor group XXX reaches YYYYYY mC"
>> + */
>> +static int tegra_soctherm_set_hwtrips(struct device *dev,
>> +				      const struct tegra_tsensor_group *sg,
>> +				      struct thermal_zone_device *tz)
>> +{
>> +	int temperature;
>> +	int ret;
>> +
>> +	ret = tz->ops->get_crit_temp(tz, &temperature);
>> +	if (ret) {
>> +		dev_warn(dev, "thermtrip: %s: missing critical temperature\n",
>> +			sg->name);
>> +		return ret;
>> +	}
>> +
>> +	ret = thermtrip_program(dev, sg, temperature);
>> +	if (ret) {
>> +		dev_err(dev, "thermtrip: %s: error during enable\n",
>> +			sg->name);
>> +		return ret;
>> +	}
>> +
>> +	dev_info(dev,
>> +		 "thermtrip: will shut down when %s reaches %d mC\n",
>> +		 sg->name, temperature);
>> +
>> +	return 0;
>> +}
>> +
>>  #ifdef CONFIG_DEBUG_FS
>>  static int regs_show(struct seq_file *s, void *data)
>>  {
>>  	struct platform_device *pdev = s->private;
>>  	struct tegra_soctherm *ts = platform_get_drvdata(pdev);
>>  	const struct tegra_tsensor *tsensors = ts->soc->tsensors;
>> +	const struct tegra_tsensor_group **ttgs = ts->soc->ttgs;
>>  	u32 r, state;
>>  	int i;
>>  
>> @@ -233,6 +374,17 @@ static int regs_show(struct seq_file *s, void *data)
>>  	state = REG_GET_MASK(r, SENSOR_TEMP2_MEM_TEMP_MASK);
>>  	seq_printf(s, " MEM(%d)\n", translate_temp(state));
>>  
>> +	r = readl(ts->regs + THERMCTL_THERMTRIP_CTL);
>> +	state = REG_GET_MASK(r, ttgs[0]->thermtrip_any_en_mask);
>> +	seq_printf(s, "Thermtrip Any En(%d)\n", state);
>> +	for (i = 0; i < ts->soc->num_ttgs; i++) {
>> +		state = REG_GET_MASK(r, ttgs[i]->thermtrip_enable_mask);
>> +		seq_printf(s, "     %s En(%d) ", ttgs[i]->name, state);
>> +		state = REG_GET_MASK(r, ttgs[i]->thermtrip_threshold_mask);
>> +		state *= ts->soc->thresh_grain;
>> +		seq_printf(s, "Thresh(%d)\n", state);
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> @@ -388,8 +540,6 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
>>  	writel(pdiv, tegra->regs + SENSOR_PDIV);
>>  	writel(hotspot, tegra->regs + SENSOR_HOTSPOT_OFF);
>>  
>> -	/* Initialize thermctl sensors */
>> -
>>  	for (i = 0; i < soc->num_ttgs; ++i) {
>>  		struct tegra_thermctl_zone *zone =
>>  			devm_kzalloc(&pdev->dev, sizeof(*zone), GFP_KERNEL);
>> @@ -399,7 +549,8 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
>>  		}
>>  
>>  		zone->reg = tegra->regs + soc->ttgs[i]->sensor_temp_offset;
>> -		zone->mask = soc->ttgs[i]->sensor_temp_mask;
>> +		zone->dev = &pdev->dev;
>> +		zone->sg = soc->ttgs[i];
>>  
>>  		z = devm_thermal_zone_of_sensor_register(&pdev->dev,
>>  							 soc->ttgs[i]->id, zone,
>> @@ -410,6 +561,11 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
>>  				err);
>>  			goto disable_clocks;
>>  		}
>> +
>> +		zone->tz = z;
>> +
>> +		/* Configure hw trip points */
>> +		tegra_soctherm_set_hwtrips(&pdev->dev, soc->ttgs[i], z);
>>  	}
>>  
> <cut>
> 
>> 1.9.1
>>
> 
> * Unknown Key
> * 0x7DA4E256
> 

Powered by blists - more mailing lists