hwmon: (aspeed-pwm-tacho) reduce fan_tach period
Subsystems (2)
PATCH 1/2 hwmon: (aspeed-pwm-tacho) reduce fan_tach period
Reduce the fan_tach period such that the fan controller uses a shorter period to measure the rpm.
Testing: This was tested on an ast2400 sitting on a quanta-q71l.
--- drivers/hwmon/aspeed-pwm-tacho.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/hwmon/aspeed-pwm-tacho.c +1 -1
@@ -160,7 +160,7 @@* 11: reserved. */ #define M_TACH_MODE 0x02/* 10b */ -#define M_TACH_UNIT 0x1000 +#define M_TACH_UNIT 0x00c0#define INIT_FAN_CTRL 0xFFstruct aspeed_pwm_tacho_data {
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
PATCH 2/2 hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep
The reference driver polled but mentioned it was possible to sleep for a computed period to know when it's ready to read. However, polling is quick and works. This also improves responsiveness from the driver.
Testing: tested on ast2400 on quanta-q71l
--- drivers/hwmon/aspeed-pwm-tacho.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
drivers/hwmon/aspeed-pwm-tacho.c +17 -6
@@ -163,6 +163,9 @@#define M_TACH_UNIT 0x00c0#define INIT_FAN_CTRL 0xFF +/* How many times we poll the fan_tach controller for rpm data. */ +#define FAN_TACH_RPM_TIMEOUT 25 +struct aspeed_pwm_tacho_data {struct regmap *regmap;unsigned long clk_freq;
@@ -508,7 +511,7 @@ static u32 aspeed_get_fan_tach_ch_measure_period(struct aspeed_pwm_tacho_datastatic int aspeed_get_fan_tach_ch_rpm (struct aspeed_pwm_tacho_data *priv,u8 fan_tach_ch) { -u32 raw_data, tach_div, clk_source, sec, val; +u32 raw_data, tach_div, clk_source, sec, val, timeout;u8 fan_tach_ch_source, type, mode, both;regmap_write (priv->regmap,ASPEED_PTCR_TRIGGER ,0 );
@@ -517,12 +520,20 @@ static int aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv, fan_tach_ch_source = priv->fan_tach_ch_source[fan_tach_ch]; type = priv->pwm_port_type[fan_tach_ch_source]; - sec = (1000 /aspeed_get_fan_tach_ch_measure_period (priv, type)); -msleep (sec); -regmap_read (priv->regmap,ASPEED_PTCR_RESULT , &val); -if (!(val &RESULT_STATUS_MASK )) -return -ETIMEDOUT ; +/* +* Instead of sleeping first, poll register for result. +* This is based on the reference driver's approach which expects to +* receive a value quickly. +*/ + timeout =0 ; +while (!(val &RESULT_STATUS_MASK )) { + timeout++; +if (timeout >FAN_TACH_RPM_TIMEOUT ) +return -ETIMEDOUT ; + +regmap_read (priv->regmap,ASPEED_PTCR_RESULT , &val); + } raw_data = val &RESULT_VALUE_MASK ; tach_div = priv->type_fan_tach_clock_division[type];
Signature · 1 line
2.13.1.611.g7e3b11ae1-goog
Guenter Roeck
maintainerRe: [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep
Quoted 16 lines · Patrick Venture
The reference driver polled but mentioned it was possible to sleep for a computed period to know when it's ready to read. However, polling is quick and works. This also improves responsiveness from the driver.
Testing: tested on ast2400 on quanta-q71l
Signed-off-by Patrick Venture <[email protected]>--- drivers/hwmon/aspeed-pwm-tacho.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)drivers/hwmon/aspeed-pwm-tacho.c +0
-0
drivers/hwmon/aspeed-pwm-tacho.c line 168
Quoted 6 lines
drivers/hwmon/aspeed-pwm-tacho.c +3
-0@@ -163,6 +163,9 @@#define M_TACH_UNIT 0x00c0#define INIT_FAN_CTRL 0xFF +/* How many times we poll the fan_tach controller for rpm data. */ +#define FAN_TACH_RPM_TIMEOUT 25 +
Not such a good idea. It means that the poll timeout depends on the local CPU speed. At some point, with some CPUs, this is going to fail.
Quoted 25 lines
drivers/hwmon/aspeed-pwm-tacho.c +4
-6struct aspeed_pwm_tacho_data {struct regmap *regmap;unsigned long clk_freq;@@ -508,7 +511,7 @@ static u32 aspeed_get_fan_tach_ch_measure_period(struct aspeed_pwm_tacho_datastatic int aspeed_get_fan_tach_ch_rpm (struct aspeed_pwm_tacho_data *priv,u8 fan_tach_ch) { -u32 raw_data, tach_div, clk_source, sec, val; +u32 raw_data, tach_div, clk_source, sec, val, timeout;u8 fan_tach_ch_source, type, mode, both;regmap_write (priv->regmap,ASPEED_PTCR_TRIGGER ,0 );@@ -517,12 +520,20 @@ static int aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv, fan_tach_ch_source = priv->fan_tach_ch_source[fan_tach_ch]; type = priv->pwm_port_type[fan_tach_ch_source]; - sec = (1000 /aspeed_get_fan_tach_ch_measure_period (priv, type)); -msleep (sec); -regmap_read (priv->regmap,ASPEED_PTCR_RESULT , &val); -if (!(val &RESULT_STATUS_MASK )) -return -ETIMEDOUT ; +/* +* Instead of sleeping first, poll register for result. +* This is based on the reference driver's approach which expects to
drivers/hwmon/aspeed-pwm-tacho.c static int aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv, · line 536
Quoted 10 lines
drivers/hwmon/aspeed-pwm-tacho.c +10
-0+* receive a value quickly. +*/ + timeout =0 ; +while (!(val &RESULT_STATUS_MASK )) { + timeout++; +if (timeout >FAN_TACH_RPM_TIMEOUT ) +return -ETIMEDOUT ; + +regmap_read (priv->regmap,ASPEED_PTCR_RESULT , &val); + }
No hot loop, please. Please use usleep_range() or similar within the loop, and a well defined timeout.
Guenter
Quoted 4 lines
drivers/hwmon/aspeed-pwm-tacho.c +0
-0raw_data = val &RESULT_VALUE_MASK ; tach_div = priv->type_fan_tach_clock_division[type];
Guenter Roeck
maintainerRe: [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep
Quoted 54 lines · Guenter Roeck
Quoted 16 lines · Patrick Venture
The reference driver polled but mentioned it was possible to sleep for a computed period to know when it's ready to read. However, polling is quick and works. This also improves responsiveness from the driver.
Testing: tested on ast2400 on quanta-q71l
Signed-off-by Patrick Venture <[email protected]>--- drivers/hwmon/aspeed-pwm-tacho.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)drivers/hwmon/aspeed-pwm-tacho.c +0
-0drivers/hwmon/aspeed-pwm-tacho.c line 167
Quoted 5 lines
drivers/hwmon/aspeed-pwm-tacho.c +3
-0@@ -163,6 +163,9 @@#define M_TACH_UNIT 0x00c0#define INIT_FAN_CTRL 0xFF +/* How many times we poll the fan_tach controller for rpm data. */ +#define FAN_TACH_RPM_TIMEOUT 25 +Not such a good idea. It means that the poll timeout depends on the local CPU speed. At some point, with some CPUs, this is going to fail.
Quoted 28 lines
drivers/hwmon/aspeed-pwm-tacho.c +9
-6struct aspeed_pwm_tacho_data {struct regmap *regmap;unsigned long clk_freq;@@ -508,7 +511,7 @@ static u32 aspeed_get_fan_tach_ch_measure_period(struct aspeed_pwm_tacho_datastatic int aspeed_get_fan_tach_ch_rpm (struct aspeed_pwm_tacho_data *priv,u8 fan_tach_ch) { -u32 raw_data, tach_div, clk_source, sec, val; +u32 raw_data, tach_div, clk_source, sec, val, timeout;u8 fan_tach_ch_source, type, mode, both;regmap_write (priv->regmap,ASPEED_PTCR_TRIGGER ,0 );@@ -517,12 +520,20 @@ static int aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv, fan_tach_ch_source = priv->fan_tach_ch_source[fan_tach_ch]; type = priv->pwm_port_type[fan_tach_ch_source]; - sec = (1000 /aspeed_get_fan_tach_ch_measure_period (priv, type)); -msleep (sec); -regmap_read (priv->regmap,ASPEED_PTCR_RESULT , &val); -if (!(val &RESULT_STATUS_MASK )) -return -ETIMEDOUT ; +/* +* Instead of sleeping first, poll register for result. +* This is based on the reference driver's approach which expects to +* receive a value quickly. +*/ + timeout =0 ; +while (!(val &RESULT_STATUS_MASK )) { + timeout++;
Quoted 9 lines
drivers/hwmon/aspeed-pwm-tacho.c static int aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv, · line 536
Quoted 5 lines
drivers/hwmon/aspeed-pwm-tacho.c +5
-0+if (timeout >FAN_TACH_RPM_TIMEOUT ) +return -ETIMEDOUT ; + +regmap_read (priv->regmap,ASPEED_PTCR_RESULT , &val); + }No hot loop, please. Please use
usleep_range()or similar within the loop, and a well defined timeout.
You might possibly consider using regmap_read_poll_timeout().
Guenter
Quoted 6 lines
Guenter
Quoted 3 lines
drivers/hwmon/aspeed-pwm-tacho.c +0
-0raw_data = val &RESULT_VALUE_MASK ; tach_div = priv->type_fan_tach_clock_division[type];
Re: [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep
In reply to Guenter RoeckQuoted 64 lines · Guenter Roeck
Quoted 18 lines · Patrick Venture
The reference driver polled but mentioned it was possible to sleep for a computed period to know when it's ready to read. However, polling is quick and works. This also improves responsiveness from the driver.
Testing: tested on ast2400 on quanta-q71l
Signed-off-by Patrick Venture <[email protected]>--- drivers/hwmon/aspeed-pwm-tacho.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)drivers/hwmon/aspeed-pwm-tacho.c +0
-0drivers/hwmon/aspeed-pwm-tacho.c line 167
Quoted 5 lines
drivers/hwmon/aspeed-pwm-tacho.c +2
-0@@ -163,6 +163,9 @@#define M_TACH_UNIT 0x00c0#define INIT_FAN_CTRL 0xFF +/* How many times we poll the fan_tach controller for rpm data. */ +#define FAN_TACH_RPM_TIMEOUT 25 +Not such a good idea. It means that the poll timeout depends on the local CPU speed.
At some point, with some CPUs, this is going to fail.Quoted 33 lines
drivers/hwmon/aspeed-pwm-tacho.c +0
-0struct aspeed_pwm_tacho_data {struct regmap *regmap;unsigned long clk_freq;@@ -508,7 +511,7 @@ static u32aspeed_get_fan_tach_ch_measure_period(struct
aspeed_pwm_tacho_data
static int aspeed_get_fan_tach_ch_rpm(structaspeed_pwm_tacho_data
*priv,u8 fan_tach_ch){
drivers/hwmon/aspeed-pwm-tacho.c +4
-5-u32 raw_data, tach_div, clk_source, sec, val; +u32 raw_data, tach_div, clk_source, sec, val, timeout;u8 fan_tach_ch_source, type, mode, both;regmap_write (priv->regmap,ASPEED_PTCR_TRIGGER ,0 );@@ -517,12 +520,20 @@ static int aspeed_get_fan_tach_ch_rpm(structaspeed_pwm_tacho_data *priv, fan_tach_ch_source = priv->fan_tach_ch_source[fan_tach_ch]; type = priv->pwm_port_type[fan_tach_ch_source]; - sec = (1000 /aspeed_get_fan_tach_ch_measure_period (priv, type)); -msleep (sec); -regmap_read (priv->regmap,ASPEED_PTCR_RESULT , &val); -if (!(val &RESULT_STATUS_MASK )) -return -ETIMEDOUT ; +/* +* Instead of sleeping first, poll register for result. +* This is based on the reference driver's approach which expects to
drivers/hwmon/aspeed-pwm-tacho.c +6
-0+* receive a value quickly. +*/ + timeout =0 ; +while (!(val &RESULT_STATUS_MASK )) { + timeout++; +if (timeout >FAN_TACH_RPM_TIMEOUT )
Quoted 9 lines
drivers/hwmon/aspeed-pwm-tacho.c static int aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv, · line 536
Quoted 4 lines
drivers/hwmon/aspeed-pwm-tacho.c +4
-0+return -ETIMEDOUT ; + +regmap_read (priv->regmap,ASPEED_PTCR_RESULT , &val); + }No hot loop, please. Please use
usleep_range()or similar within the loop, and a well defined timeout.
Thanks, that's a good call.
I'll implement a variation in the morning and test it to see if there are any subtle changes required and try to pick good ranges or something similar. We need responsiveness down to under 0.0125s so eight can be read sequentially ten times per second. The controller itself actually seems to internally cache a value and only gets updated once ever 0.08s. Just as an aside.
I'll check out "regmap_read_poll_timeout" as well per follow-on email.
Quoted 7 lines
Guenter
Quoted 3 lines
raw_data = val & RESULT_VALUE_MASK; tach_div = priv->type_fan_tach_clock_division[type];
Guenter Roeck
maintainerRe: [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep
Quoted 75 lines · Patrick Venture
Quoted 64 lines · Guenter Roeck
Quoted 18 lines · Patrick Venture
The reference driver polled but mentioned it was possible to sleep for a computed period to know when it's ready to read. However, polling is quick and works. This also improves responsiveness from the driver.
Testing: tested on ast2400 on quanta-q71l
Signed-off-by Patrick Venture <[email protected]>--- drivers/hwmon/aspeed-pwm-tacho.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)drivers/hwmon/aspeed-pwm-tacho.c +0
-0drivers/hwmon/aspeed-pwm-tacho.c line 167
Quoted 5 lines
drivers/hwmon/aspeed-pwm-tacho.c +2
-0@@ -163,6 +163,9 @@#define M_TACH_UNIT 0x00c0#define INIT_FAN_CTRL 0xFF +/* How many times we poll the fan_tach controller for rpm data. */ +#define FAN_TACH_RPM_TIMEOUT 25 +Not such a good idea. It means that the poll timeout depends on the local CPU speed.
At some point, with some CPUs, this is going to fail.Quoted 33 lines
drivers/hwmon/aspeed-pwm-tacho.c +0
-0struct aspeed_pwm_tacho_data {struct regmap *regmap;unsigned long clk_freq;@@ -508,7 +511,7 @@ static u32aspeed_get_fan_tach_ch_measure_period(struct
aspeed_pwm_tacho_data
static int aspeed_get_fan_tach_ch_rpm(structaspeed_pwm_tacho_data
*priv,u8 fan_tach_ch){
drivers/hwmon/aspeed-pwm-tacho.c +4
-5-u32 raw_data, tach_div, clk_source, sec, val; +u32 raw_data, tach_div, clk_source, sec, val, timeout;u8 fan_tach_ch_source, type, mode, both;regmap_write (priv->regmap,ASPEED_PTCR_TRIGGER ,0 );@@ -517,12 +520,20 @@ static int aspeed_get_fan_tach_ch_rpm(structaspeed_pwm_tacho_data *priv, fan_tach_ch_source = priv->fan_tach_ch_source[fan_tach_ch]; type = priv->pwm_port_type[fan_tach_ch_source]; - sec = (1000 /aspeed_get_fan_tach_ch_measure_period (priv, type)); -msleep (sec); -regmap_read (priv->regmap,ASPEED_PTCR_RESULT , &val); -if (!(val &RESULT_STATUS_MASK )) -return -ETIMEDOUT ; +/* +* Instead of sleeping first, poll register for result. +* This is based on the reference driver's approach which expects to
drivers/hwmon/aspeed-pwm-tacho.c +6
-0+* receive a value quickly. +*/ + timeout =0 ; +while (!(val &RESULT_STATUS_MASK )) { + timeout++; +if (timeout >FAN_TACH_RPM_TIMEOUT )Quoted 9 lines
drivers/hwmon/aspeed-pwm-tacho.c static int aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv, · line 536
Quoted 4 lines
drivers/hwmon/aspeed-pwm-tacho.c +4
-0+return -ETIMEDOUT ; + +regmap_read (priv->regmap,ASPEED_PTCR_RESULT , &val); + }No hot loop, please. Please use
usleep_range()or similar within the loop, and a well defined timeout.
Quoted 5 lines
Thanks, that's a good call.
I'll implement a variation in the morning and test it to see if there are any subtle changes required and try to pick good ranges or something similar. We need responsiveness down to under 0.0125s so
That is forever ;-). 12.5 ms -> picking something like a 500 uS sleep period for regmap_read_poll_timeout() and the old maximum sleep time for its timeout_us parameter should do it.
Guenter
Quoted 18 lines
eight can be read sequentially ten times per second. The controller itself actually seems to internally cache a value and only gets updated once ever 0.08s. Just as an aside.
I'll check out "
regmap_read_poll_timeout" as well per follow-on email.Quoted 7 lines
Guenter
Quoted 3 lines
raw_data = val & RESULT_VALUE_MASK; tach_div = priv->type_fan_tach_clock_division[type];--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html