[02/25] thermal/drivers/hisi: Remove the multiple sensors support
diff mbox

Message ID 1507658570-32675-2-git-send-email-daniel.lezcano@linaro.org
State Changes Requested
Delegated to: Eduardo Valentin
Headers show

Commit Message

Daniel Lezcano Oct. 10, 2017, 6:02 p.m. UTC
By essence, the tsensor does not really support multiple sensor at the same
time. It allows to set a sensor and use it to get the temperature, another
sensor could be switched but with a delay of 3-5ms. It is difficult to read
simultaneously several sensors without a big delay.

Today, just one sensor is used, it is not necessary to deal with multiple
sensors in the code. Remove them and if it is needed in the future add them
on top of a code which will be clean up in the meantime.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/hisi_thermal.c | 75 +++++++++++-------------------------------
 1 file changed, 19 insertions(+), 56 deletions(-)

Comments

Eduardo Valentin Oct. 17, 2017, 3:54 a.m. UTC | #1
On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
> By essence, the tsensor does not really support multiple sensor at the same
> time. It allows to set a sensor and use it to get the temperature, another
> sensor could be switched but with a delay of 3-5ms. It is difficult to read
> simultaneously several sensors without a big delay.
> 

Is 3-5 ms enough to loose an event? Is this really a problem?

> Today, just one sensor is used, it is not necessary to deal with multiple

How come? Today the driver exposes all sensors to userspace. Removing
them, means you are changing the amount of sensors userspace really
sees.

Are you sure about this?

Can you please elaborate how we are only using one of the four sensors?

> sensors in the code. Remove them and if it is needed in the future add them
> on top of a code which will be clean up in the meantime.

It is just not clear to me why having 1 sensor support is better than
having 4, as per the hw supported tsensor.

> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

I would prefer to get confirmation from folks from hisilicon here.

BTW, you should be copying previous author of the code.

> ---
>  drivers/thermal/hisi_thermal.c | 75 +++++++++++-------------------------------
>  1 file changed, 19 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
> index f3b50b0..687efd4 100644
> --- a/drivers/thermal/hisi_thermal.c
> +++ b/drivers/thermal/hisi_thermal.c
> @@ -39,6 +39,7 @@
>  #define HISI_TEMP_RESET			(100000)
>  
>  #define HISI_MAX_SENSORS		4
> +#define HISI_DEFAULT_SENSOR		2
>  
>  struct hisi_thermal_sensor {
>  	struct hisi_thermal_data *thermal;
> @@ -53,9 +54,8 @@ struct hisi_thermal_data {
>  	struct mutex thermal_lock;    /* protects register data */
>  	struct platform_device *pdev;
>  	struct clk *clk;
> -	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
> -
> -	int irq, irq_bind_sensor;
> +	struct hisi_thermal_sensor sensors;
> +	int irq;
>  	bool irq_enabled;
>  
>  	void __iomem *regs;
> @@ -113,7 +113,7 @@ static void hisi_thermal_enable_bind_irq_sensor
>  
>  	mutex_lock(&data->thermal_lock);
>  
> -	sensor = &data->sensors[data->irq_bind_sensor];
> +	sensor = &data->sensors;
>  
>  	/* setting the hdak time */
>  	writel(0x0, data->regs + TEMP0_CFG);
> @@ -160,31 +160,8 @@ static int hisi_thermal_get_temp(void *_sensor, int *temp)
>  	struct hisi_thermal_sensor *sensor = _sensor;
>  	struct hisi_thermal_data *data = sensor->thermal;
>  
> -	int sensor_id = -1, i;
> -	long max_temp = 0;
> -
>  	*temp = hisi_thermal_get_sensor_temp(data, sensor);
>  
> -	sensor->sensor_temp = *temp;
> -
> -	for (i = 0; i < HISI_MAX_SENSORS; i++) {
> -		if (!data->sensors[i].tzd)
> -			continue;
> -
> -		if (data->sensors[i].sensor_temp >= max_temp) {
> -			max_temp = data->sensors[i].sensor_temp;
> -			sensor_id = i;
> -		}
> -	}
> -
> -	/* If no sensor has been enabled, then skip to enable irq */
> -	if (sensor_id == -1)
> -		return 0;
> -
> -	mutex_lock(&data->thermal_lock);
> -	data->irq_bind_sensor = sensor_id;
> -	mutex_unlock(&data->thermal_lock);
> -
>  	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%d, thres=%d\n",
>  		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
>  	/*
> @@ -197,7 +174,7 @@ static int hisi_thermal_get_temp(void *_sensor, int *temp)
>  		return 0;
>  	}
>  
> -	if (max_temp < sensor->thres_temp) {
> +	if (*temp < sensor->thres_temp) {
>  		data->irq_enabled = true;
>  		hisi_thermal_enable_bind_irq_sensor(data);
>  		enable_irq(data->irq);
> @@ -224,22 +201,16 @@ static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
>  {
>  	struct hisi_thermal_data *data = dev;
>  	struct hisi_thermal_sensor *sensor;
> -	int i;
>  
>  	mutex_lock(&data->thermal_lock);
> -	sensor = &data->sensors[data->irq_bind_sensor];
> +	sensor = &data->sensors;
>  
>  	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
>  		 sensor->thres_temp / 1000);
>  	mutex_unlock(&data->thermal_lock);
>  
> -	for (i = 0; i < HISI_MAX_SENSORS; i++) {
> -		if (!data->sensors[i].tzd)
> -			continue;
> -
> -		thermal_zone_device_update(data->sensors[i].tzd,
> -					   THERMAL_EVENT_UNSPECIFIED);
> -	}
> +	thermal_zone_device_update(data->sensors.tzd,
> +				   THERMAL_EVENT_UNSPECIFIED);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -296,7 +267,6 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  {
>  	struct hisi_thermal_data *data;
>  	struct resource *res;
> -	int i;
>  	int ret;
>  
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> @@ -347,16 +317,17 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  	hisi_thermal_enable_bind_irq_sensor(data);
>  	data->irq_enabled = true;
>  
> -	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
> -		ret = hisi_thermal_register_sensor(pdev, data,
> -						   &data->sensors[i], i);
> -		if (ret)
> -			dev_err(&pdev->dev,
> -				"failed to register thermal sensor: %d\n", ret);
> -		else
> -			hisi_thermal_toggle_sensor(&data->sensors[i], true);
> +	ret = hisi_thermal_register_sensor(pdev, data,
> +					   &data->sensors,
> +					   HISI_DEFAULT_SENSOR);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register thermal sensor: %d\n",
> +			ret);
> +		return ret;
>  	}
>  
> +	hisi_thermal_toggle_sensor(&data->sensors, true);
> +
>  	enable_irq(data->irq);
>  
>  	return 0;
> @@ -365,17 +336,9 @@ static int hisi_thermal_probe(struct platform_device *pdev)
>  static int hisi_thermal_remove(struct platform_device *pdev)
>  {
>  	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
> -	int i;
> -
> -	for (i = 0; i < HISI_MAX_SENSORS; i++) {
> -		struct hisi_thermal_sensor *sensor = &data->sensors[i];
> -
> -		if (!sensor->tzd)
> -			continue;
> -
> -		hisi_thermal_toggle_sensor(sensor, false);
> -	}
> +	struct hisi_thermal_sensor *sensor = &data->sensors;
>  
> +	hisi_thermal_toggle_sensor(sensor, false);
>  	hisi_thermal_disable_sensor(data);
>  	clk_disable_unprepare(data->clk);
>  
> -- 
> 2.7.4
>
Daniel Lezcano Oct. 17, 2017, 12:28 p.m. UTC | #2
On 17/10/2017 05:54, Eduardo Valentin wrote:
> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>> By essence, the tsensor does not really support multiple sensor at the same
>> time. It allows to set a sensor and use it to get the temperature, another
>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>> simultaneously several sensors without a big delay.
>>
> 
> Is 3-5 ms enough to loose an event? Is this really a problem?

There are several aspects:

 - the multiple sensors is not needed here

 - the temperature controller is not designed to read several sensors at
the same time, we switch the sensor and that clears some internal
buffers and re-init the controller

 - some boards can take 40°C in 1 sec, the temperature increase is
insanely fast and reading several sensors add an extra 15ms.

So from my point of view, trying to read multiple sensors with *this*
tsensor is pointless.

>> Today, just one sensor is used, it is not necessary to deal with multiple
> 
> How come? Today the driver exposes all sensors to userspace. Removing
> them, means you are changing the amount of sensors userspace really
> sees.
> 
> Are you sure about this?
> 
> Can you please elaborate how we are only using one of the four sensors?

Only one has been defined in the DT since the beginning and no one is
using multiple sensors.

>> sensors in the code. Remove them and if it is needed in the future add them
>> on top of a code which will be clean up in the meantime.
> 
> It is just not clear to me why having 1 sensor support is better than
> having 4, as per the hw supported tsensor.

The tsensor supports 4 sensors but not at the same time. You choose one
and use it, AFAICS from the documentation and behavior.

If multiple sensors is needed later, I will be happy to re-introduce it
on top of a sanitized driver.

>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> I would prefer to get confirmation from folks from hisilicon here.
> 
> BTW, you should be copying previous author of the code.

Hmm, I thought Leo was in copy (added). Kevin is from hisilicon and in
charge of the thermal aspect of the hikey.
Eduardo Valentin Oct. 17, 2017, 6:25 p.m. UTC | #3
Hello,

On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
> On 17/10/2017 05:54, Eduardo Valentin wrote:
> > On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
> >> By essence, the tsensor does not really support multiple sensor at the same
> >> time. It allows to set a sensor and use it to get the temperature, another
> >> sensor could be switched but with a delay of 3-5ms. It is difficult to read
> >> simultaneously several sensors without a big delay.
> >>
> > 
> > Is 3-5 ms enough to loose an event? Is this really a problem?
> 
> There are several aspects:
> 
>  - the multiple sensors is not needed here

Well, that is debatable, I cannot really agree or disagree with the
above statement without understanding the use cases and most important,
the location of each sensor. What is the location of each sensor?

> 
>  - the temperature controller is not designed to read several sensors at
> the same time, we switch the sensor and that clears some internal
> buffers and re-init the controller

Which is still very helpful in case you have multiple hotspots that you
want to track and they are exposed on different workloads. Sacrificing
the availability of sensors is something needs a better justification
other than "current code uses only one".


> 
>  - some boards can take 40°C in 1 sec, the temperature increase is
> insanely fast and reading several sensors add an extra 15ms.
> 


Ok... What is the difference in update rate with and without the switch
of sensors? With the above worst case, you have about 4/6 mC/ms. Can
your tsensor support that resolution for a single sensor? What is the
maximum resolution a tsensor can support? What is the penalty added with
switch?

Based on this data, and the above 3-5ms, that  means you would miss about
~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
enough justification to drop three extra sensors?

> So from my point of view, trying to read multiple sensors with *this*
> tsensor is pointless.
> 
> >> Today, just one sensor is used, it is not necessary to deal with multiple
> > 
> > How come? Today the driver exposes all sensors to userspace. Removing
> > them, means you are changing the amount of sensors userspace really
> > sees.
> > 
> > Are you sure about this?
> > 
> > Can you please elaborate how we are only using one of the four sensors?
> 
> Only one has been defined in the DT since the beginning and no one is
> using multiple sensors.
> 
> >> sensors in the code. Remove them and if it is needed in the future add them
> >> on top of a code which will be clean up in the meantime.
> > 
> > It is just not clear to me why having 1 sensor support is better than
> > having 4, as per the hw supported tsensor.
> 
> The tsensor supports 4 sensors but not at the same time. You choose one
> and use it, AFAICS from the documentation and behavior.
> 
> If multiple sensors is needed later, I will be happy to re-introduce it
> on top of a sanitized driver.

Once again, having one at time is better than only one, depending on
sensor location and usage of the chip. Say you have two sensors, one at
CPU domain another at a coprocessor, say a DSP. The representation of
the CPU sensor is typically not representative of the DSP, and
vice-versa, even though one may still collect a few of the heat of what
the other detects first. Sometimes takes several hundreds of ms  
(sometimes seconds) to see heat wave from one to the other. Having both,
but the reads one at a time, may give you the chance to see a heat
increase on one side (say DSP) within 5 ms time frame (assuming your
reported worst case above), faster than physically waiting the heat
to really reach the other sensor (say CPU) after several hundreds of
ms (sometimes seconds).


> 
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > 
> > I would prefer to get confirmation from folks from hisilicon here.
> > 
> > BTW, you should be copying previous author of the code.
> 
> Hmm, I thought Leo was in copy (added). Kevin is from hisilicon and in
> charge of the thermal aspect of the hikey.
> 
> 
> 
> 
> 
> -- 
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>
Daniel Lezcano Oct. 17, 2017, 7:03 p.m. UTC | #4
On 17/10/2017 20:25, Eduardo Valentin wrote:
> Hello,
> 
> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>>>> By essence, the tsensor does not really support multiple sensor at the same
>>>> time. It allows to set a sensor and use it to get the temperature, another
>>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>>>> simultaneously several sensors without a big delay.
>>>>
>>>
>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>
>> There are several aspects:
>>
>>  - the multiple sensors is not needed here
> 
> Well, that is debatable, I cannot really agree or disagree with the
> above statement without understanding the use cases and most important,
> the location of each sensor. What is the location of each sensor?
> 
>>
>>  - the temperature controller is not designed to read several sensors at
>> the same time, we switch the sensor and that clears some internal
>> buffers and re-init the controller
> 
> Which is still very helpful in case you have multiple hotspots that you
> want to track and they are exposed on different workloads. Sacrificing
> the availability of sensors is something needs a better justification
> other than "current code uses only one".
> 
> 
>>
>>  - some boards can take 40°C in 1 sec, the temperature increase is
>> insanely fast and reading several sensors add an extra 15ms.
>>
> 
> 
> Ok... What is the difference in update rate with and without the switch
> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
> your tsensor support that resolution for a single sensor? What is the
> maximum resolution a tsensor can support? What is the penalty added with
> switch?
> 
> Based on this data, and the above 3-5ms, that  means you would miss about
> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
> enough justification to drop three extra sensors?

Ok if I refer to the documentation the rate is 0.768 ms with the current
configuration.

The driver is currently bogus: register overwritten, bouncing interrupt,
unneeded lock, ... So the proposition was to remove the multiple sensors
support, clean the driver, and re-introduce it if there is a need.

If I remember correctly Leo, author of the driver, agreed on this. Leo ?

Note, I'm not strongly against multiple sensors support in the driver if
you think it is convenient but it is much simpler to remove the current
code as it is not used and put it back on top of a sane foundation
instead of circumventing that on the existing code.
Eduardo Valentin Oct. 17, 2017, 9:07 p.m. UTC | #5
On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
> On 17/10/2017 20:25, Eduardo Valentin wrote:
> > Hello,
> > 
> > On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
> >> On 17/10/2017 05:54, Eduardo Valentin wrote:
> >>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
> >>>> By essence, the tsensor does not really support multiple sensor at the same
> >>>> time. It allows to set a sensor and use it to get the temperature, another
> >>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
> >>>> simultaneously several sensors without a big delay.
> >>>>
> >>>
> >>> Is 3-5 ms enough to loose an event? Is this really a problem?
> >>
> >> There are several aspects:
> >>
> >>  - the multiple sensors is not needed here
> > 
> > Well, that is debatable, I cannot really agree or disagree with the
> > above statement without understanding the use cases and most important,
> > the location of each sensor. What is the location of each sensor?
> > 
> >>
> >>  - the temperature controller is not designed to read several sensors at
> >> the same time, we switch the sensor and that clears some internal
> >> buffers and re-init the controller
> > 
> > Which is still very helpful in case you have multiple hotspots that you
> > want to track and they are exposed on different workloads. Sacrificing
> > the availability of sensors is something needs a better justification
> > other than "current code uses only one".
> > 
> > 
> >>
> >>  - some boards can take 40°C in 1 sec, the temperature increase is
> >> insanely fast and reading several sensors add an extra 15ms.
> >>
> > 
> > 
> > Ok... What is the difference in update rate with and without the switch
> > of sensors? With the above worst case, you have about 4/6 mC/ms. Can
> > your tsensor support that resolution for a single sensor? What is the
> > maximum resolution a tsensor can support? What is the penalty added with
> > switch?
> > 
> > Based on this data, and the above 3-5ms, that  means you would miss about
> > ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
> > above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
> > enough justification to drop three extra sensors?
> 
> Ok if I refer to the documentation the rate is 0.768 ms with the current
> configuration.
> 
> The driver is currently bogus: register overwritten, bouncing interrupt,
> unneeded lock, ... So the proposition was to remove the multiple sensors
> support, clean the driver, and re-introduce it if there is a need.
> 
> If I remember correctly Leo, author of the driver, agreed on this. Leo ?
> 
> Note, I'm not strongly against multiple sensors support in the driver if
> you think it is convenient but it is much simpler to remove the current
> code as it is not used and put it back on top of a sane foundation
> instead of circumventing that on the existing code.
> 
> 

I am also fine with the above strategy, as long as you are sure you are
not breaking anyone (specially userspace). Also, it would be good to get
a reviewed-by from hisilicon just to confirm (Leo?).

Besides, once you get his reviewed-by, and add it to the patches,
can you please resend the series with the minor issues I
mentioned (a few minor checkpatch issues and one compilation warn that
is added to the driver after the series is applied).

> 
> 
> 
> -- 
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
>
Daniel Lezcano Oct. 17, 2017, 9:10 p.m. UTC | #6
On 17/10/2017 23:07, Eduardo Valentin wrote:
> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
>> On 17/10/2017 20:25, Eduardo Valentin wrote:
>>> Hello,
>>>
>>> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>>>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>>>>>> By essence, the tsensor does not really support multiple sensor at the same
>>>>>> time. It allows to set a sensor and use it to get the temperature, another
>>>>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>>>>>> simultaneously several sensors without a big delay.
>>>>>>
>>>>>
>>>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>>>
>>>> There are several aspects:
>>>>
>>>>  - the multiple sensors is not needed here
>>>
>>> Well, that is debatable, I cannot really agree or disagree with the
>>> above statement without understanding the use cases and most important,
>>> the location of each sensor. What is the location of each sensor?
>>>
>>>>
>>>>  - the temperature controller is not designed to read several sensors at
>>>> the same time, we switch the sensor and that clears some internal
>>>> buffers and re-init the controller
>>>
>>> Which is still very helpful in case you have multiple hotspots that you
>>> want to track and they are exposed on different workloads. Sacrificing
>>> the availability of sensors is something needs a better justification
>>> other than "current code uses only one".
>>>
>>>
>>>>
>>>>  - some boards can take 40°C in 1 sec, the temperature increase is
>>>> insanely fast and reading several sensors add an extra 15ms.
>>>>
>>>
>>>
>>> Ok... What is the difference in update rate with and without the switch
>>> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
>>> your tsensor support that resolution for a single sensor? What is the
>>> maximum resolution a tsensor can support? What is the penalty added with
>>> switch?
>>>
>>> Based on this data, and the above 3-5ms, that  means you would miss about
>>> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
>>> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
>>> enough justification to drop three extra sensors?
>>
>> Ok if I refer to the documentation the rate is 0.768 ms with the current
>> configuration.
>>
>> The driver is currently bogus: register overwritten, bouncing interrupt,
>> unneeded lock, ... So the proposition was to remove the multiple sensors
>> support, clean the driver, and re-introduce it if there is a need.
>>
>> If I remember correctly Leo, author of the driver, agreed on this. Leo ?
>>
>> Note, I'm not strongly against multiple sensors support in the driver if
>> you think it is convenient but it is much simpler to remove the current
>> code as it is not used and put it back on top of a sane foundation
>> instead of circumventing that on the existing code.
>>
>>
> 
> I am also fine with the above strategy, as long as you are sure you are
> not breaking anyone (specially userspace). Also, it would be good to get
> a reviewed-by from hisilicon just to confirm (Leo?).
> 
> Besides, once you get his reviewed-by, and add it to the patches,
> can you please resend the series with the minor issues I
> mentioned (a few minor checkpatch issues and one compilation warn that
> is added to the driver after the series is applied).

Yes, sure. I'm on it.

I will send it tomorrow.

Thanks.

  -- Daniel
Leo Yan Oct. 18, 2017, 1:48 a.m. UTC | #7
On Tue, Oct 17, 2017 at 02:07:08PM -0700, Eduardo Valentin wrote:
> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
> > On 17/10/2017 20:25, Eduardo Valentin wrote:
> > > Hello,
> > > 
> > > On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
> > >> On 17/10/2017 05:54, Eduardo Valentin wrote:
> > >>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
> > >>>> By essence, the tsensor does not really support multiple sensor at the same
> > >>>> time. It allows to set a sensor and use it to get the temperature, another
> > >>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
> > >>>> simultaneously several sensors without a big delay.
> > >>>>
> > >>>
> > >>> Is 3-5 ms enough to loose an event? Is this really a problem?
> > >>
> > >> There are several aspects:
> > >>
> > >>  - the multiple sensors is not needed here
> > > 
> > > Well, that is debatable, I cannot really agree or disagree with the
> > > above statement without understanding the use cases and most important,
> > > the location of each sensor. What is the location of each sensor?
> > > 
> > >>
> > >>  - the temperature controller is not designed to read several sensors at
> > >> the same time, we switch the sensor and that clears some internal
> > >> buffers and re-init the controller
> > > 
> > > Which is still very helpful in case you have multiple hotspots that you
> > > want to track and they are exposed on different workloads. Sacrificing
> > > the availability of sensors is something needs a better justification
> > > other than "current code uses only one".
> > > 
> > > 
> > >>
> > >>  - some boards can take 40°C in 1 sec, the temperature increase is
> > >> insanely fast and reading several sensors add an extra 15ms.
> > >>
> > > 
> > > 
> > > Ok... What is the difference in update rate with and without the switch
> > > of sensors? With the above worst case, you have about 4/6 mC/ms. Can
> > > your tsensor support that resolution for a single sensor? What is the
> > > maximum resolution a tsensor can support? What is the penalty added with
> > > switch?
> > > 
> > > Based on this data, and the above 3-5ms, that  means you would miss about
> > > ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
> > > above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
> > > enough justification to drop three extra sensors?
> > 
> > Ok if I refer to the documentation the rate is 0.768 ms with the current
> > configuration.
> > 
> > The driver is currently bogus: register overwritten, bouncing interrupt,
> > unneeded lock, ... So the proposition was to remove the multiple sensors
> > support, clean the driver, and re-introduce it if there is a need.
> > 
> > If I remember correctly Leo, author of the driver, agreed on this. Leo ?
> > 
> > Note, I'm not strongly against multiple sensors support in the driver if
> > you think it is convenient but it is much simpler to remove the current
> > code as it is not used and put it back on top of a sane foundation
> > instead of circumventing that on the existing code.
> > 
> > 
> 
> I am also fine with the above strategy, as long as you are sure you are
> not breaking anyone (specially userspace). Also, it would be good to get
> a reviewed-by from hisilicon just to confirm (Leo?).

Sorry I missed to reply this patch. And yes, I have tested and
reviewed it at my side:

Reviewed-by: Leo Yan <leo.yan@linaro.org>

P.s. I am working for Linaro; I am continously co-working with
Hisilicon to maintain this driver due it's important for Hikey/Hikey960
two boards stability; this driver also is important for our daily
profiling for power and performance. Eduardo, so please let us know if
you still need ack from Hisilicon engineer.

> Besides, once you get his reviewed-by, and add it to the patches,
> can you please resend the series with the minor issues I
> mentioned (a few minor checkpatch issues and one compilation warn that
> is added to the driver after the series is applied).
> 
> > 
> > 
> > 
> > -- 
> >  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> > 
> > Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> > <http://twitter.com/#!/linaroorg> Twitter |
> > <http://www.linaro.org/linaro-blog/> Blog
> >
Tao Wang Oct. 18, 2017, 1:49 a.m. UTC | #8
在 2017/10/18 5:07, Eduardo Valentin 写道:
> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
>> On 17/10/2017 20:25, Eduardo Valentin wrote:
>>> Hello,
>>>
>>> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>>>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>>>>>> By essence, the tsensor does not really support multiple sensor at the same
>>>>>> time. It allows to set a sensor and use it to get the temperature, another
>>>>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>>>>>> simultaneously several sensors without a big delay.
>>>>>>
>>>>>
>>>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>>>
>>>> There are several aspects:
>>>>
>>>>   - the multiple sensors is not needed here
>>>
>>> Well, that is debatable, I cannot really agree or disagree with the
>>> above statement without understanding the use cases and most important,
>>> the location of each sensor. What is the location of each sensor?
>>>
>>>>
>>>>   - the temperature controller is not designed to read several sensors at
>>>> the same time, we switch the sensor and that clears some internal
>>>> buffers and re-init the controller
>>>
>>> Which is still very helpful in case you have multiple hotspots that you
>>> want to track and they are exposed on different workloads. Sacrificing
>>> the availability of sensors is something needs a better justification
>>> other than "current code uses only one".
>>>
>>>
>>>>
>>>>   - some boards can take 40°C in 1 sec, the temperature increase is
>>>> insanely fast and reading several sensors add an extra 15ms.
>>>>
>>>
>>>
>>> Ok... What is the difference in update rate with and without the switch
>>> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
>>> your tsensor support that resolution for a single sensor? What is the
>>> maximum resolution a tsensor can support? What is the penalty added with
>>> switch?
>>>
>>> Based on this data, and the above 3-5ms, that  means you would miss about
>>> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
>>> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
>>> enough justification to drop three extra sensors?
>>
>> Ok if I refer to the documentation the rate is 0.768 ms with the current
>> configuration.
>>
>> The driver is currently bogus: register overwritten, bouncing interrupt,
>> unneeded lock, ... So the proposition was to remove the multiple sensors
>> support, clean the driver, and re-introduce it if there is a need.
>>
>> If I remember correctly Leo, author of the driver, agreed on this. Leo ?
>>
>> Note, I'm not strongly against multiple sensors support in the driver if
>> you think it is convenient but it is much simpler to remove the current
>> code as it is not used and put it back on top of a sane foundation
>> instead of circumventing that on the existing code.
>>
>>
> 
> I am also fine with the above strategy, as long as you are sure you are
> not breaking anyone (specially userspace). Also, it would be good to get
> a reviewed-by from hisilicon just to confirm (Leo?).
I agree with Daniel, we only care about CPU temperature on hikey, and currently
mutiple sensors support are actually not used.
> 
> Besides, once you get his reviewed-by, and add it to the patches,
> can you please resend the series with the minor issues I
> mentioned (a few minor checkpatch issues and one compilation warn that
> is added to the driver after the series is applied).
> 
>>
>>
>>
>> -- 
>>   <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
>>
> 
>
Eduardo Valentin Oct. 18, 2017, 3:51 p.m. UTC | #9
On Wed, Oct 18, 2017 at 09:48:21AM +0800, Leo Yan wrote:
> On Tue, Oct 17, 2017 at 02:07:08PM -0700, Eduardo Valentin wrote:
> > On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
> > > On 17/10/2017 20:25, Eduardo Valentin wrote:
> > > > Hello,
> > > > 
> > > > On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
> > > >> On 17/10/2017 05:54, Eduardo Valentin wrote:
> > > >>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
> > > >>>> By essence, the tsensor does not really support multiple sensor at the same
> > > >>>> time. It allows to set a sensor and use it to get the temperature, another
> > > >>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
> > > >>>> simultaneously several sensors without a big delay.
> > > >>>>
> > > >>>
> > > >>> Is 3-5 ms enough to loose an event? Is this really a problem?
> > > >>
> > > >> There are several aspects:
> > > >>
> > > >>  - the multiple sensors is not needed here
> > > > 
> > > > Well, that is debatable, I cannot really agree or disagree with the
> > > > above statement without understanding the use cases and most important,
> > > > the location of each sensor. What is the location of each sensor?
> > > > 
> > > >>
> > > >>  - the temperature controller is not designed to read several sensors at
> > > >> the same time, we switch the sensor and that clears some internal
> > > >> buffers and re-init the controller
> > > > 
> > > > Which is still very helpful in case you have multiple hotspots that you
> > > > want to track and they are exposed on different workloads. Sacrificing
> > > > the availability of sensors is something needs a better justification
> > > > other than "current code uses only one".
> > > > 
> > > > 
> > > >>
> > > >>  - some boards can take 40°C in 1 sec, the temperature increase is
> > > >> insanely fast and reading several sensors add an extra 15ms.
> > > >>
> > > > 
> > > > 
> > > > Ok... What is the difference in update rate with and without the switch
> > > > of sensors? With the above worst case, you have about 4/6 mC/ms. Can
> > > > your tsensor support that resolution for a single sensor? What is the
> > > > maximum resolution a tsensor can support? What is the penalty added with
> > > > switch?
> > > > 
> > > > Based on this data, and the above 3-5ms, that  means you would miss about
> > > > ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
> > > > above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
> > > > enough justification to drop three extra sensors?
> > > 
> > > Ok if I refer to the documentation the rate is 0.768 ms with the current
> > > configuration.
> > > 
> > > The driver is currently bogus: register overwritten, bouncing interrupt,
> > > unneeded lock, ... So the proposition was to remove the multiple sensors
> > > support, clean the driver, and re-introduce it if there is a need.
> > > 
> > > If I remember correctly Leo, author of the driver, agreed on this. Leo ?
> > > 
> > > Note, I'm not strongly against multiple sensors support in the driver if
> > > you think it is convenient but it is much simpler to remove the current
> > > code as it is not used and put it back on top of a sane foundation
> > > instead of circumventing that on the existing code.
> > > 
> > > 
> > 
> > I am also fine with the above strategy, as long as you are sure you are
> > not breaking anyone (specially userspace). Also, it would be good to get
> > a reviewed-by from hisilicon just to confirm (Leo?).
> 
> Sorry I missed to reply this patch. And yes, I have tested and
> reviewed it at my side:
> 
> Reviewed-by: Leo Yan <leo.yan@linaro.org>
> 
> P.s. I am working for Linaro; I am continously co-working with
> Hisilicon to maintain this driver due it's important for Hikey/Hikey960
> two boards stability; this driver also is important for our daily
> profiling for power and performance. Eduardo, so please let us know if
> you still need ack from Hisilicon engineer.


Yeah, I think adding your Reviewed-by and Kevin's is enough for this
series to go through. As I asked Daniel already, only few minor stuff
needs to be fixed along with the addition of the reviewed-by's.

> 
> > Besides, once you get his reviewed-by, and add it to the patches,
> > can you please resend the series with the minor issues I
> > mentioned (a few minor checkpatch issues and one compilation warn that
> > is added to the driver after the series is applied).
> > 
> > > 
> > > 
> > > 
> > > -- 
> > >  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> > > 
> > > Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> > > <http://twitter.com/#!/linaroorg> Twitter |
> > > <http://www.linaro.org/linaro-blog/> Blog
> > >
Daniel Lezcano Oct. 18, 2017, 4:23 p.m. UTC | #10
On 18/10/2017 17:51, Eduardo Valentin wrote:
> On Wed, Oct 18, 2017 at 09:48:21AM +0800, Leo Yan wrote:
>> On Tue, Oct 17, 2017 at 02:07:08PM -0700, Eduardo Valentin wrote:
>>> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
>>>> On 17/10/2017 20:25, Eduardo Valentin wrote:
>>>>> Hello,
>>>>>
>>>>> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>>>>>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>>>>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>>>>>>>> By essence, the tsensor does not really support multiple sensor at the same
>>>>>>>> time. It allows to set a sensor and use it to get the temperature, another
>>>>>>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>>>>>>>> simultaneously several sensors without a big delay.
>>>>>>>>
>>>>>>>
>>>>>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>>>>>
>>>>>> There are several aspects:
>>>>>>
>>>>>>  - the multiple sensors is not needed here
>>>>>
>>>>> Well, that is debatable, I cannot really agree or disagree with the
>>>>> above statement without understanding the use cases and most important,
>>>>> the location of each sensor. What is the location of each sensor?
>>>>>
>>>>>>
>>>>>>  - the temperature controller is not designed to read several sensors at
>>>>>> the same time, we switch the sensor and that clears some internal
>>>>>> buffers and re-init the controller
>>>>>
>>>>> Which is still very helpful in case you have multiple hotspots that you
>>>>> want to track and they are exposed on different workloads. Sacrificing
>>>>> the availability of sensors is something needs a better justification
>>>>> other than "current code uses only one".
>>>>>
>>>>>
>>>>>>
>>>>>>  - some boards can take 40°C in 1 sec, the temperature increase is
>>>>>> insanely fast and reading several sensors add an extra 15ms.
>>>>>>
>>>>>
>>>>>
>>>>> Ok... What is the difference in update rate with and without the switch
>>>>> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
>>>>> your tsensor support that resolution for a single sensor? What is the
>>>>> maximum resolution a tsensor can support? What is the penalty added with
>>>>> switch?
>>>>>
>>>>> Based on this data, and the above 3-5ms, that  means you would miss about
>>>>> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
>>>>> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
>>>>> enough justification to drop three extra sensors?
>>>>
>>>> Ok if I refer to the documentation the rate is 0.768 ms with the current
>>>> configuration.
>>>>
>>>> The driver is currently bogus: register overwritten, bouncing interrupt,
>>>> unneeded lock, ... So the proposition was to remove the multiple sensors
>>>> support, clean the driver, and re-introduce it if there is a need.
>>>>
>>>> If I remember correctly Leo, author of the driver, agreed on this. Leo ?
>>>>
>>>> Note, I'm not strongly against multiple sensors support in the driver if
>>>> you think it is convenient but it is much simpler to remove the current
>>>> code as it is not used and put it back on top of a sane foundation
>>>> instead of circumventing that on the existing code.
>>>>
>>>>
>>>
>>> I am also fine with the above strategy, as long as you are sure you are
>>> not breaking anyone (specially userspace). Also, it would be good to get
>>> a reviewed-by from hisilicon just to confirm (Leo?).
>>
>> Sorry I missed to reply this patch. And yes, I have tested and
>> reviewed it at my side:
>>
>> Reviewed-by: Leo Yan <leo.yan@linaro.org>
>>
>> P.s. I am working for Linaro; I am continously co-working with
>> Hisilicon to maintain this driver due it's important for Hikey/Hikey960
>> two boards stability; this driver also is important for our daily
>> profiling for power and performance. Eduardo, so please let us know if
>> you still need ack from Hisilicon engineer.
> 
> 
> Yeah, I think adding your Reviewed-by and Kevin's is enough for this
> series to go through. As I asked Daniel already, only few minor stuff
> needs to be fixed along with the addition of the reviewed-by's.

The different warnings you reported are fixed and the reviewed-by's /
acked-by's added. I think the patches 19-25 may need an extra look, so I
will resend all the other patches meanwhile.

Does it sound good?

Patch
diff mbox

diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c
index f3b50b0..687efd4 100644
--- a/drivers/thermal/hisi_thermal.c
+++ b/drivers/thermal/hisi_thermal.c
@@ -39,6 +39,7 @@ 
 #define HISI_TEMP_RESET			(100000)
 
 #define HISI_MAX_SENSORS		4
+#define HISI_DEFAULT_SENSOR		2
 
 struct hisi_thermal_sensor {
 	struct hisi_thermal_data *thermal;
@@ -53,9 +54,8 @@  struct hisi_thermal_data {
 	struct mutex thermal_lock;    /* protects register data */
 	struct platform_device *pdev;
 	struct clk *clk;
-	struct hisi_thermal_sensor sensors[HISI_MAX_SENSORS];
-
-	int irq, irq_bind_sensor;
+	struct hisi_thermal_sensor sensors;
+	int irq;
 	bool irq_enabled;
 
 	void __iomem *regs;
@@ -113,7 +113,7 @@  static void hisi_thermal_enable_bind_irq_sensor
 
 	mutex_lock(&data->thermal_lock);
 
-	sensor = &data->sensors[data->irq_bind_sensor];
+	sensor = &data->sensors;
 
 	/* setting the hdak time */
 	writel(0x0, data->regs + TEMP0_CFG);
@@ -160,31 +160,8 @@  static int hisi_thermal_get_temp(void *_sensor, int *temp)
 	struct hisi_thermal_sensor *sensor = _sensor;
 	struct hisi_thermal_data *data = sensor->thermal;
 
-	int sensor_id = -1, i;
-	long max_temp = 0;
-
 	*temp = hisi_thermal_get_sensor_temp(data, sensor);
 
-	sensor->sensor_temp = *temp;
-
-	for (i = 0; i < HISI_MAX_SENSORS; i++) {
-		if (!data->sensors[i].tzd)
-			continue;
-
-		if (data->sensors[i].sensor_temp >= max_temp) {
-			max_temp = data->sensors[i].sensor_temp;
-			sensor_id = i;
-		}
-	}
-
-	/* If no sensor has been enabled, then skip to enable irq */
-	if (sensor_id == -1)
-		return 0;
-
-	mutex_lock(&data->thermal_lock);
-	data->irq_bind_sensor = sensor_id;
-	mutex_unlock(&data->thermal_lock);
-
 	dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%d, thres=%d\n",
 		sensor->id, data->irq_enabled, *temp, sensor->thres_temp);
 	/*
@@ -197,7 +174,7 @@  static int hisi_thermal_get_temp(void *_sensor, int *temp)
 		return 0;
 	}
 
-	if (max_temp < sensor->thres_temp) {
+	if (*temp < sensor->thres_temp) {
 		data->irq_enabled = true;
 		hisi_thermal_enable_bind_irq_sensor(data);
 		enable_irq(data->irq);
@@ -224,22 +201,16 @@  static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev)
 {
 	struct hisi_thermal_data *data = dev;
 	struct hisi_thermal_sensor *sensor;
-	int i;
 
 	mutex_lock(&data->thermal_lock);
-	sensor = &data->sensors[data->irq_bind_sensor];
+	sensor = &data->sensors;
 
 	dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n",
 		 sensor->thres_temp / 1000);
 	mutex_unlock(&data->thermal_lock);
 
-	for (i = 0; i < HISI_MAX_SENSORS; i++) {
-		if (!data->sensors[i].tzd)
-			continue;
-
-		thermal_zone_device_update(data->sensors[i].tzd,
-					   THERMAL_EVENT_UNSPECIFIED);
-	}
+	thermal_zone_device_update(data->sensors.tzd,
+				   THERMAL_EVENT_UNSPECIFIED);
 
 	return IRQ_HANDLED;
 }
@@ -296,7 +267,6 @@  static int hisi_thermal_probe(struct platform_device *pdev)
 {
 	struct hisi_thermal_data *data;
 	struct resource *res;
-	int i;
 	int ret;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
@@ -347,16 +317,17 @@  static int hisi_thermal_probe(struct platform_device *pdev)
 	hisi_thermal_enable_bind_irq_sensor(data);
 	data->irq_enabled = true;
 
-	for (i = 0; i < HISI_MAX_SENSORS; ++i) {
-		ret = hisi_thermal_register_sensor(pdev, data,
-						   &data->sensors[i], i);
-		if (ret)
-			dev_err(&pdev->dev,
-				"failed to register thermal sensor: %d\n", ret);
-		else
-			hisi_thermal_toggle_sensor(&data->sensors[i], true);
+	ret = hisi_thermal_register_sensor(pdev, data,
+					   &data->sensors,
+					   HISI_DEFAULT_SENSOR);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register thermal sensor: %d\n",
+			ret);
+		return ret;
 	}
 
+	hisi_thermal_toggle_sensor(&data->sensors, true);
+
 	enable_irq(data->irq);
 
 	return 0;
@@ -365,17 +336,9 @@  static int hisi_thermal_probe(struct platform_device *pdev)
 static int hisi_thermal_remove(struct platform_device *pdev)
 {
 	struct hisi_thermal_data *data = platform_get_drvdata(pdev);
-	int i;
-
-	for (i = 0; i < HISI_MAX_SENSORS; i++) {
-		struct hisi_thermal_sensor *sensor = &data->sensors[i];
-
-		if (!sensor->tzd)
-			continue;
-
-		hisi_thermal_toggle_sensor(sensor, false);
-	}
+	struct hisi_thermal_sensor *sensor = &data->sensors;
 
+	hisi_thermal_toggle_sensor(sensor, false);
 	hisi_thermal_disable_sensor(data);
 	clk_disable_unprepare(data->clk);