| Message ID | 1507658570-32675-2-git-send-email-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
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 >
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.
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 >
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.
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 >
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
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 > >
在 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 >> > >
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 > > >
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?
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);
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(-)