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 <daniel.lezc...@linaro.org>
> ---
> 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
>
