Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752621AbdIBD3f (ORCPT ); Fri, 1 Sep 2017 23:29:35 -0400 Received: from mail-pg0-f51.google.com ([74.125.83.51]:35767 "EHLO mail-pg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750980AbdIBD3e (ORCPT ); Fri, 1 Sep 2017 23:29:34 -0400 X-Google-Smtp-Source: ADKCNb5gF2GT9yZ1GsrXEi5OiDYkkb9GXok0hDxlY8hkFQ20/pVHfa2B3b3/95lNhKabpSzjniQDfQ== Date: Sat, 2 Sep 2017 11:29:24 +0800 From: Leo Yan To: Daniel Lezcano Cc: rui.zhang@intel.com, edubezval@gmail.com, linux-pm@vger.kernel.org, kevin.wangtao@linaro.org, open list Subject: Re: [PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection Message-ID: <20170902032924.GB3841@leoy-linaro> References: <1504082857-21702-1-git-send-email-daniel.lezcano@linaro.org> <1504082857-21702-9-git-send-email-daniel.lezcano@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1504082857-21702-9-git-send-email-daniel.lezcano@linaro.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10770 Lines: 348 On Wed, Aug 30, 2017 at 10:47:33AM +0200, Daniel Lezcano wrote: > The sensor is all setup, bind, resetted, acked, etc... every single second. > > That was the way to workaround a problem with the interrupt bouncing again and > again. > > With the following changes, we fix all in one: > > - Do the setup, one time, at probe time > > - Add the IRQF_ONESHOT, ack the interrupt in the threaded handler > > - Remove the interrupt handler > > - Set the correct value for the LAG register > > - Remove all the irq_enabled stuff in the code as the interruption > handling is fixed > > - Remove the 3ms delay > > - Reorder the initialization routine to be in the right order > > It ends up to a nicer code and more efficient, the 3-5ms delay is removed from > the get_temp() path. > > Signed-off-by: Daniel Lezcano > --- > drivers/thermal/hisi_thermal.c | 203 +++++++++++++++++++---------------------- > 1 file changed, 93 insertions(+), 110 deletions(-) > > diff --git a/drivers/thermal/hisi_thermal.c b/drivers/thermal/hisi_thermal.c > index 3e03908..b038d8a 100644 > --- a/drivers/thermal/hisi_thermal.c > +++ b/drivers/thermal/hisi_thermal.c > @@ -39,6 +39,7 @@ > #define HISI_TEMP_BASE (-60000) > #define HISI_TEMP_RESET (100000) > #define HISI_TEMP_STEP (784) > +#define HISI_TEMP_LAG (3500) So I am curious what's the reason to select 3.5'c for lag value? Is this a heuristics result? > #define HISI_MAX_SENSORS 4 > #define HISI_DEFAULT_SENSOR 2 > @@ -58,8 +59,6 @@ struct hisi_thermal_data { > struct clk *clk; > struct hisi_thermal_sensor sensors; > int irq; > - bool irq_enabled; > - > void __iomem *regs; > }; > > @@ -97,9 +96,40 @@ static inline long hisi_thermal_round_temp(int temp) > hisi_thermal_temp_to_step(temp)); > } > > +/* > + * The lag register contains 5 bits encoding the temperature in steps. > + * > + * Each time the temperature crosses the threshold boundary, an > + * interrupt is raised. It could be when the temperature is going > + * above the threshold or below. However, if the temperature is > + * fluctuating around this value due to the load, we can receive > + * several interrupts which may not desired. > + * > + * We can setup a temperature representing the delta between the > + * threshold and the current temperature when the temperature is > + * decreasing. > + * > + * For instance: the lag register is 5?C, the threshold is 65?C, when > + * the temperature reaches 65?C an interrupt is raised and when the > + * temperature decrease to 65?C - 5?C another interrupt is raised. > + * > + * A very short lag can lead to an interrupt storm, a long lag > + * increase the latency to react to the temperature changes. In our > + * case, that is not really a problem as we are polling the > + * temperature. So here means if we set a long lag value and the interrupt is delayed, sometimes we even don't receive the interrupt; but this is acceptable due we are polling the temperature so we still can get the updated temperature value, right? > + * > + * [0:4] : lag register > + * > + * The temperature is coded in steps, cf. HISI_TEMP_STEP. > + * > + * Min : 0x00 : 0.0 ?C > + * Max : 0x1F : 24.3 ?C > + * > + * The 'value' parameter is in milliCelsius. > + */ > static inline void hisi_thermal_set_lag(void __iomem *addr, int value) > { > - writel(value, addr + TEMP0_LAG); > + writel((value / HISI_TEMP_STEP) & 0x1F, addr + TEMP0_LAG); > } > > static inline void hisi_thermal_alarm_clear(void __iomem *addr, int value) > @@ -167,71 +197,6 @@ static inline void hisi_thermal_hdak_set(void __iomem *addr, int value) > writel(readl(addr + TEMP0_CFG) | (value << 4), addr + TEMP0_CFG); > } > > -static long hisi_thermal_get_sensor_temp(struct hisi_thermal_data *data, > - struct hisi_thermal_sensor *sensor) > -{ > - long val; > - > - mutex_lock(&data->thermal_lock); > - > - /* disable interrupt */ > - hisi_thermal_alarm_enable(data->regs, 0); > - hisi_thermal_alarm_clear(data->regs, 1); > - > - /* disable module firstly */ > - hisi_thermal_enable(data->regs, 0); > - > - /* select sensor id */ > - hisi_thermal_sensor_select(data->regs, sensor->id); > - > - /* enable module */ > - hisi_thermal_enable(data->regs, 1); > - > - usleep_range(3000, 5000); > - > - val = hisi_thermal_get_temperature(data->regs); > - > - mutex_unlock(&data->thermal_lock); > - > - return val; > -} > - > -static void hisi_thermal_enable_bind_irq_sensor > - (struct hisi_thermal_data *data) > -{ > - struct hisi_thermal_sensor *sensor; > - > - mutex_lock(&data->thermal_lock); > - > - sensor = &data->sensors; > - > - /* setting the hdak time */ > - hisi_thermal_hdak_set(data->regs, 0); > - > - /* disable module firstly */ > - hisi_thermal_reset_enable(data->regs, 0); > - hisi_thermal_enable(data->regs, 0); > - > - /* select sensor id */ > - hisi_thermal_sensor_select(data->regs, sensor->id); > - > - /* enable for interrupt */ > - hisi_thermal_alarm_set(data->regs, sensor->thres_temp); > - > - hisi_thermal_reset_set(data->regs, HISI_TEMP_RESET); > - > - /* enable module */ > - hisi_thermal_reset_enable(data->regs, 1); > - hisi_thermal_enable(data->regs, 1); > - > - hisi_thermal_alarm_clear(data->regs, 0); > - hisi_thermal_alarm_enable(data->regs, 1); > - > - usleep_range(3000, 5000); > - > - mutex_unlock(&data->thermal_lock); > -} > - > static void hisi_thermal_disable_sensor(struct hisi_thermal_data *data) > { > mutex_lock(&data->thermal_lock); > @@ -249,25 +214,10 @@ static int hisi_thermal_get_temp(void *_sensor, int *temp) > struct hisi_thermal_sensor *sensor = _sensor; > struct hisi_thermal_data *data = sensor->thermal; > > - *temp = hisi_thermal_get_sensor_temp(data, sensor); > - > - dev_dbg(&data->pdev->dev, "id=%d, irq=%d, temp=%d, thres=%d\n", > - sensor->id, data->irq_enabled, *temp, sensor->thres_temp); > - /* > - * Bind irq to sensor for two cases: > - * Reenable alarm IRQ if temperature below threshold; > - * if irq has been enabled, always set it; > - */ > - if (data->irq_enabled) { > - hisi_thermal_enable_bind_irq_sensor(data); > - return 0; > - } > + *temp = hisi_thermal_get_temperature(data->regs); > > - if (*temp < sensor->thres_temp) { > - data->irq_enabled = true; > - hisi_thermal_enable_bind_irq_sensor(data); > - enable_irq(data->irq); > - } > + dev_dbg(&data->pdev->dev, "id=%d, temp=%d, thres=%d\n", > + sensor->id, *temp, sensor->thres_temp); > > return 0; > } > @@ -276,26 +226,27 @@ static const struct thermal_zone_of_device_ops hisi_of_thermal_ops = { > .get_temp = hisi_thermal_get_temp, > }; > > -static irqreturn_t hisi_thermal_alarm_irq(int irq, void *dev) > +static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev) > { > struct hisi_thermal_data *data = dev; > + struct hisi_thermal_sensor *sensor = &data->sensors; > + int temp; > > - disable_irq_nosync(irq); > - data->irq_enabled = false; > + hisi_thermal_alarm_clear(data->regs, 1); > > - return IRQ_WAKE_THREAD; > -} > + temp = hisi_thermal_get_temperature(data->regs); > > -static irqreturn_t hisi_thermal_alarm_irq_thread(int irq, void *dev) > -{ > - struct hisi_thermal_data *data = dev; > - struct hisi_thermal_sensor *sensor = &data->sensors; > + if (temp >= sensor->thres_temp) { > + dev_crit(&data->pdev->dev, "THERMAL ALARM: %d > %d\n", > + temp, sensor->thres_temp); > > - dev_crit(&data->pdev->dev, "THERMAL ALARM: T > %d\n", > - sensor->thres_temp); > + thermal_zone_device_update(data->sensors.tzd, > + THERMAL_EVENT_UNSPECIFIED); > > - thermal_zone_device_update(data->sensors.tzd, > - THERMAL_EVENT_UNSPECIFIED); > + } else if (temp < sensor->thres_temp) { > + dev_crit(&data->pdev->dev, "THERMAL ALARM stopped: %d < %d\n", > + temp, sensor->thres_temp); > + } For temperature increasing and decreasing both cases, can always call thermal_zone_device_update()? > > return IRQ_HANDLED; > } > @@ -348,6 +299,40 @@ static void hisi_thermal_toggle_sensor(struct hisi_thermal_sensor *sensor, > on ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED); > } > > +static int hisi_thermal_setup(struct hisi_thermal_data *data) > +{ > + struct hisi_thermal_sensor *sensor; > + > + sensor = &data->sensors; > + > + /* disable module firstly */ > + hisi_thermal_reset_enable(data->regs, 0); > + hisi_thermal_enable(data->regs, 0); > + > + /* select sensor id */ > + hisi_thermal_sensor_select(data->regs, sensor->id); > + > + /* setting the hdak time */ > + hisi_thermal_hdak_set(data->regs, 0); > + > + /* setting lag value between current temp and the threshold */ > + hisi_thermal_set_lag(data->regs, HISI_TEMP_LAG); > + > + /* enable for interrupt */ > + hisi_thermal_alarm_set(data->regs, sensor->thres_temp); > + > + hisi_thermal_reset_set(data->regs, HISI_TEMP_RESET); > + > + /* enable module */ > + hisi_thermal_reset_enable(data->regs, 1); > + hisi_thermal_enable(data->regs, 1); > + > + hisi_thermal_alarm_clear(data->regs, 0); > + hisi_thermal_alarm_enable(data->regs, 1); > + > + return 0; > +} > + > static int hisi_thermal_probe(struct platform_device *pdev) > { > struct hisi_thermal_data *data; > @@ -390,9 +375,6 @@ static int hisi_thermal_probe(struct platform_device *pdev) > return ret; > } > > - hisi_thermal_enable_bind_irq_sensor(data); > - data->irq_enabled = true; > - > ret = hisi_thermal_register_sensor(pdev, data, > &data->sensors, > HISI_DEFAULT_SENSOR); > @@ -402,18 +384,21 @@ static int hisi_thermal_probe(struct platform_device *pdev) > return ret; > } > > - hisi_thermal_toggle_sensor(&data->sensors, true); > + ret = hisi_thermal_setup(data); > + if (ret) { > + dev_err(&pdev->dev, "Failed to setup the sensor: %d\n", ret); > + return ret; > + } > > - ret = devm_request_threaded_irq(&pdev->dev, data->irq, > - hisi_thermal_alarm_irq, > + ret = devm_request_threaded_irq(&pdev->dev, data->irq, NULL, > hisi_thermal_alarm_irq_thread, > - 0, "hisi_thermal", data); > + IRQF_ONESHOT, "hisi_thermal", data); > if (ret < 0) { > dev_err(&pdev->dev, "failed to request alarm irq: %d\n", ret); > return ret; > } > > - enable_irq(data->irq); > + hisi_thermal_toggle_sensor(&data->sensors, true); > > return 0; > } > @@ -436,7 +421,6 @@ static int hisi_thermal_suspend(struct device *dev) > struct hisi_thermal_data *data = dev_get_drvdata(dev); > > hisi_thermal_disable_sensor(data); > - data->irq_enabled = false; > > clk_disable_unprepare(data->clk); > > @@ -452,8 +436,7 @@ static int hisi_thermal_resume(struct device *dev) > if (ret) > return ret; > > - data->irq_enabled = true; > - hisi_thermal_enable_bind_irq_sensor(data); > + hisi_thermal_setup(data); > > return 0; > } > -- > 2.7.4 >