Re: [PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-09-04 Thread Leo Yan 
On Mon, Sep 04, 2017 at 01:29:19PM +0200, Daniel Lezcano wrote:
> On 04/09/2017 02:50, Leo Yan wrote:
> 
> [ ... ]
> 
> >> That is something I asked myself but I finally decided to not change the
> >> current behavior and let that be added in a separate patch.
> > 
> > This is fine.
> 
> Hi Leo,
> 
> can you tag this patch if you agree with it?

Yeah; I have tested this patch at my side:

Reviewed-by: Leo Yan 
Tested-by: Leo Yan 

> 
> -- 
>   Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:   Facebook |
>  Twitter |
>  Blog
>

Re: [PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-09-04 Thread Leo Yan 
On Mon, Sep 04, 2017 at 01:29:19PM +0200, Daniel Lezcano wrote:
> On 04/09/2017 02:50, Leo Yan wrote:
> 
> [ ... ]
> 
> >> That is something I asked myself but I finally decided to not change the
> >> current behavior and let that be added in a separate patch.
> > 
> > This is fine.
> 
> Hi Leo,
> 
> can you tag this patch if you agree with it?

Yeah; I have tested this patch at my side:

Reviewed-by: Leo Yan 
Tested-by: Leo Yan 

> 
> -- 
>   Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:   Facebook |
>  Twitter |
>  Blog
>

Re: [PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-09-04 Thread Daniel Lezcano 
On 04/09/2017 02:50, Leo Yan wrote:

[ ... ]

>> That is something I asked myself but I finally decided to not change the
>> current behavior and let that be added in a separate patch.
> 
> This is fine.

Hi Leo,

can you tag this patch if you agree with it?

-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-09-04 Thread Daniel Lezcano 
On 04/09/2017 02:50, Leo Yan wrote:

[ ... ]

>> That is something I asked myself but I finally decided to not change the
>> current behavior and let that be added in a separate patch.
> 
> This is fine.

Hi Leo,

can you tag this patch if you agree with it?

-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-09-03 Thread Leo Yan 
On Sat, Sep 02, 2017 at 03:10:29PM +0200, Daniel Lezcano wrote:

[...]

> On 02/09/2017 05:29, Leo Yan wrote:
> > 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(-6)
> >>  #define HISI_TEMP_RESET   (10)
> >>  #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?
> 
> Yes, it is. I tried 5°C but I find it too long. After several tests, I
> found 3.5°C looked fine.
> 
> [ ... ]
> 
> >> + * 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?
> No, what I meant is if the hysteresis is too large, eg. 45 - 65, then
> the interrupt will come after the temperature goes below 45 and that may
> take a long time, especially if the temperature is fluctuating between
> the two hysteresis values.
> 
> The only setup preventing an interrupt to happen is when crossing the
> low value hysteresis is not possible in practical. For example, the
> temperature of the SoC is ~44°C at idle time. If we set the threshold to
> 65°C and the lag to the max 24°C, the interrupt will raised when we go
> below 41°C and that situation can't happen.
> 
> In the current situation, the interrupt is only used to raise an alarm.

Thanks for the explanation, Ionela (have Cced.) reminded me the
interrupt for 'alarm on' is fatal, if we use polling method then the
delay can be 1000ms, but with interrupt to alarm we can handle the
temperature rasing immediately.

> The get temperature is resulting from a polling, so the delay between
> the alarm on/off is not issue for now.

Understood.

> >> + *
> >> + * [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);
> >>  }
> 
> 
> [ ... ]
> 
> >> -  thermal_zone_device_update(data->sensors.tzd,
> >> - THERMAL_EVENT_UNSPECIFIED);
> >> +  } else if (temp < sensor->thres_temp) {
> >> +  dev_crit(>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()?
> 
> That is something I asked myself but I finally decided to not change the
> current behavior and let that be added in a separate patch.

This is fine.

Thanks,
Leo Yan

Re: [PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-09-03 Thread Leo Yan 
On Sat, Sep 02, 2017 at 03:10:29PM +0200, Daniel Lezcano wrote:

[...]

> On 02/09/2017 05:29, Leo Yan wrote:
> > 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(-6)
> >>  #define HISI_TEMP_RESET   (10)
> >>  #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?
> 
> Yes, it is. I tried 5°C but I find it too long. After several tests, I
> found 3.5°C looked fine.
> 
> [ ... ]
> 
> >> + * 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?
> No, what I meant is if the hysteresis is too large, eg. 45 - 65, then
> the interrupt will come after the temperature goes below 45 and that may
> take a long time, especially if the temperature is fluctuating between
> the two hysteresis values.
> 
> The only setup preventing an interrupt to happen is when crossing the
> low value hysteresis is not possible in practical. For example, the
> temperature of the SoC is ~44°C at idle time. If we set the threshold to
> 65°C and the lag to the max 24°C, the interrupt will raised when we go
> below 41°C and that situation can't happen.
> 
> In the current situation, the interrupt is only used to raise an alarm.

Thanks for the explanation, Ionela (have Cced.) reminded me the
interrupt for 'alarm on' is fatal, if we use polling method then the
delay can be 1000ms, but with interrupt to alarm we can handle the
temperature rasing immediately.

> The get temperature is resulting from a polling, so the delay between
> the alarm on/off is not issue for now.

Understood.

> >> + *
> >> + * [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);
> >>  }
> 
> 
> [ ... ]
> 
> >> -  thermal_zone_device_update(data->sensors.tzd,
> >> - THERMAL_EVENT_UNSPECIFIED);
> >> +  } else if (temp < sensor->thres_temp) {
> >> +  dev_crit(>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()?
> 
> That is something I asked myself but I finally decided to not change the
> current behavior and let that be added in a separate patch.

This is fine.

Thanks,
Leo Yan

Re: [PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-09-02 Thread Daniel Lezcano 
On 02/09/2017 05:29, Leo Yan wrote:
> 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  (-6)
>>  #define HISI_TEMP_RESET (10)
>>  #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?

Yes, it is. I tried 5°C but I find it too long. After several tests, I
found 3.5°C looked fine.

[ ... ]

>> + * 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?
No, what I meant is if the hysteresis is too large, eg. 45 - 65, then
the interrupt will come after the temperature goes below 45 and that may
take a long time, especially if the temperature is fluctuating between
the two hysteresis values.

The only setup preventing an interrupt to happen is when crossing the
low value hysteresis is not possible in practical. For example, the
temperature of the SoC is ~44°C at idle time. If we set the threshold to
65°C and the lag to the max 24°C, the interrupt will raised when we go
below 41°C and that situation can't happen.

In the current situation, the interrupt is only used to raise an alarm.
The get temperature is resulting from a polling, so the delay between
the alarm on/off is not issue for now.

>> + *
>> + * [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);
>>  }


[ ... ]

>> -thermal_zone_device_update(data->sensors.tzd,
>> -   THERMAL_EVENT_UNSPECIFIED);
>> +} else if (temp < sensor->thres_temp) {
>> +dev_crit(>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()?

That is something I asked myself but I finally decided to not change the
current behavior and let that be added in a separate patch.


[ ... ]



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-09-02 Thread Daniel Lezcano 
On 02/09/2017 05:29, Leo Yan wrote:
> 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  (-6)
>>  #define HISI_TEMP_RESET (10)
>>  #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?

Yes, it is. I tried 5°C but I find it too long. After several tests, I
found 3.5°C looked fine.

[ ... ]

>> + * 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?
No, what I meant is if the hysteresis is too large, eg. 45 - 65, then
the interrupt will come after the temperature goes below 45 and that may
take a long time, especially if the temperature is fluctuating between
the two hysteresis values.

The only setup preventing an interrupt to happen is when crossing the
low value hysteresis is not possible in practical. For example, the
temperature of the SoC is ~44°C at idle time. If we set the threshold to
65°C and the lag to the max 24°C, the interrupt will raised when we go
below 41°C and that situation can't happen.

In the current situation, the interrupt is only used to raise an alarm.
The get temperature is resulting from a polling, so the delay between
the alarm on/off is not issue for now.

>> + *
>> + * [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);
>>  }


[ ... ]

>> -thermal_zone_device_update(data->sensors.tzd,
>> -   THERMAL_EVENT_UNSPECIFIED);
>> +} else if (temp < sensor->thres_temp) {
>> +dev_crit(>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()?

That is something I asked myself but I finally decided to not change the
current behavior and let that be added in a separate patch.


[ ... ]



-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-09-01 Thread Leo Yan 
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   (-6)
>  #define HISI_TEMP_RESET  (10)
>  #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(>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(>thermal_lock);
> -
> - return val;
> -}
> -
> -static void hisi_thermal_enable_bind_irq_sensor
> - (struct hisi_thermal_data *data)
> -{
> - struct hisi_thermal_sensor *sensor;
> -
> - mutex_lock(>thermal_lock);
> -
> - sensor = >sensors;
> -
> - /* setting the hdak time */
> - hisi_thermal_hdak_set(data->regs, 0);
> -
> - /* disable module

Re: [PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-09-01 Thread Leo Yan 
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   (-6)
>  #define HISI_TEMP_RESET  (10)
>  #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(>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(>thermal_lock);
> -
> - return val;
> -}
> -
> -static void hisi_thermal_enable_bind_irq_sensor
> - (struct hisi_thermal_data *data)
> -{
> - struct hisi_thermal_sensor *sensor;
> -
> - mutex_lock(>thermal_lock);
> -
> - sensor = >sensors;
> -
> - /* setting the hdak time */
> - hisi_thermal_hdak_set(data->regs, 0);
> -
> - /* disable module firstly */
> -

[PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-08-30 Thread Daniel Lezcano 
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 (-6)
 #define HISI_TEMP_RESET(10)
 #define HISI_TEMP_STEP (784)
+#define HISI_TEMP_LAG  (3500)
 
 #define HISI_MAX_SENSORS   4
 #define HISI_DEFAULT_SENSOR2
@@ -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.
+ *
+ * [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(>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(>thermal_lock);
-
-   return val;
-}
-
-static void hisi_thermal_enable_bind_irq_sensor
-   (struct hisi_thermal_data *data)
-{
-   struct hisi_thermal_sensor *sensor;
-
-   mutex_lock(>thermal_lock);
-
-   sensor = >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);
-   
-

[PATCH 09/13] thermal/drivers/hisi: Remove costly sensor inspection

2017-08-30 Thread Daniel Lezcano 
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 (-6)
 #define HISI_TEMP_RESET(10)
 #define HISI_TEMP_STEP (784)
+#define HISI_TEMP_LAG  (3500)
 
 #define HISI_MAX_SENSORS   4
 #define HISI_DEFAULT_SENSOR2
@@ -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.
+ *
+ * [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(>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(>thermal_lock);
-
-   return val;
-}
-
-static void hisi_thermal_enable_bind_irq_sensor
-   (struct hisi_thermal_data *data)
-{
-   struct hisi_thermal_sensor *sensor;
-
-   mutex_lock(>thermal_lock);
-
-   sensor = >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);
-
-