Re: [PATCH V7 09/12] thermal: tegra: add thermtrip function

[Wei Ni]

On 2016年03月16日 03:49, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Mar 15, 2016 at 05:12:12PM +0800, Wei Ni wrote:
>>
>>
>> On 2016年03月15日 03:16, Eduardo Valentin wrote:
 Old 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 
 ---
  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.
> 
> 
> Can you be more specific? I don't recall a case that core would call any
> driver callbacks before setting up the data structures properly.

Sorry for the confusion, I mean this data structure is the private data pointer,
it doesn't handled by thermal_core. Let me explain more:
In this tegra soctherm driver, I run following steps in probe routine:
step1:
z = devm_thermal_zone_of_sensor_register(*dev, sensor_id, zone, ops);
register thermal zone device, in here, the parameter "zone" is the private data
pointer "structure tegra_thermctl_zone".
step 2:
After register, it return the "z", and I set it to the private data:
zone->tz = z;

In the .set_trip() callback, it will pass back this private data pointer.
So if the callback was called between step1 and step2, at that time the zone->tz
didn't be set yet, it will cause problems, although I didn't hit this case.

This check doesn't relate with core driver, it is used to check my private data
pointer.

Wei.

> 
>>>
> 
> * Unknown Key
> * 0x7DA4E256
> 

Re: [PATCH V7 09/12] thermal: tegra: add thermtrip function

[Wei Ni]

On 2016年03月16日 03:49, Eduardo Valentin wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Mar 15, 2016 at 05:12:12PM +0800, Wei Ni wrote:
>>
>>
>> On 2016年03月15日 03:16, Eduardo Valentin wrote:
 Old 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 
 ---
  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.
> 
> 
> Can you be more specific? I don't recall a case that core would call any
> driver callbacks before setting up the data structures properly.

Sorry for the confusion, I mean this data structure is the private data pointer,
it doesn't handled by thermal_core. Let me explain more:
In this tegra soctherm driver, I run following steps in probe routine:
step1:
z = devm_thermal_zone_of_sensor_register(*dev, sensor_id, zone, ops);
register thermal zone device, in here, the parameter "zone" is the private data
pointer "structure tegra_thermctl_zone".
step 2:
After register, it return the "z", and I set it to the private data:
zone->tz = z;

In the .set_trip() callback, it will pass back this private data pointer.
So if the callback was called between step1 and step2, at that time the zone->tz
didn't be set yet, it will cause problems, although I didn't hit this case.

This check doesn't relate with core driver, it is used to check my private data
pointer.

Wei.

> 
>>>
> 
> * Unknown Key
> * 0x7DA4E256
> 

Re: [PATCH V7 09/12] thermal: tegra: add thermtrip function

[Eduardo Valentin]

On Tue, Mar 15, 2016 at 05:12:12PM +0800, Wei Ni wrote:
> 
> 
> 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 
> >> ---
> >>  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.


Can you be more specific? I don't recall a case that core would call any
driver callbacks before setting up the data structures properly.

> > 


signature.asc
Description: Digital signature

Re: [PATCH V7 09/12] thermal: tegra: add thermtrip function

[Eduardo Valentin]

On Tue, Mar 15, 2016 at 05:12:12PM +0800, Wei Ni wrote:
> 
> 
> 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 
> >> ---
> >>  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.


Can you be more specific? I don't recall a case that core would call any
driver callbacks before setting up the data structures properly.

> > 


signature.asc
Description: Digital signature

Re: [PATCH V7 09/12] thermal: tegra: add thermtrip function

[Wei Ni]

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 
>> ---
>>  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, );
>> +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
>> + 

Re: [PATCH V7 09/12] thermal: tegra: add thermtrip function

[Wei Ni]

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 
>> ---
>>  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, );
>> +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 

Re: [PATCH V7 09/12] thermal: tegra: add thermtrip function

[Eduardo Valentin]

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 
> ---
>  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?

> + 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)?

> +
> + ret = tz->ops->get_trip_type(tz, trip, );
> + 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, 

Re: [PATCH V7 09/12] thermal: tegra: add thermtrip function

[Eduardo Valentin]

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 
> ---
>  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?

> + 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)?

> +
> + ret = tz->ops->get_trip_type(tz, trip, );
> + 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,