* [PATCH 1/2] hwmon: (aspeed-pwm-tacho) reduce fan_tach period
@ 2017-06-21 1:12 Patrick Venture
2017-06-21 1:12 ` [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep Patrick Venture
0 siblings, 1 reply; 6+ messages in thread
From: Patrick Venture @ 2017-06-21 1:12 UTC (permalink / raw)
To: venture, joel, jdelvare, linux
Cc: linux-hwmon, raltherr, emilyshaffer, peterh
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.
Signed-off-by: Patrick Venture <venture@google.com>
---
drivers/hwmon/aspeed-pwm-tacho.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
index 86e2ea8287a7..b2ab5612d8a4 100644
--- a/drivers/hwmon/aspeed-pwm-tacho.c
+++ b/drivers/hwmon/aspeed-pwm-tacho.c
@@ -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 0xFF
struct aspeed_pwm_tacho_data {
--
2.13.1.611.g7e3b11ae1-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep
2017-06-21 1:12 [PATCH 1/2] hwmon: (aspeed-pwm-tacho) reduce fan_tach period Patrick Venture
@ 2017-06-21 1:12 ` Patrick Venture
2017-06-21 1:32 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Patrick Venture @ 2017-06-21 1:12 UTC (permalink / raw)
To: venture, joel, jdelvare, linux
Cc: linux-hwmon, raltherr, emilyshaffer, peterh
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 <venture@google.com>
---
drivers/hwmon/aspeed-pwm-tacho.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
index b2ab5612d8a4..a31d2648fb3a 100644
--- a/drivers/hwmon/aspeed-pwm-tacho.c
+++ b/drivers/hwmon/aspeed-pwm-tacho.c
@@ -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_data
static 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];
--
2.13.1.611.g7e3b11ae1-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep
2017-06-21 1:12 ` [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep Patrick Venture
@ 2017-06-21 1:32 ` Guenter Roeck
2017-06-21 1:39 ` Guenter Roeck
2017-06-21 1:42 ` Patrick Venture
0 siblings, 2 replies; 6+ messages in thread
From: Guenter Roeck @ 2017-06-21 1:32 UTC (permalink / raw)
To: Patrick Venture, joel, jdelvare
Cc: linux-hwmon, raltherr, emilyshaffer, peterh
On 06/20/2017 06:12 PM, Patrick Venture wrote:
> 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 <venture@google.com>
> ---
> drivers/hwmon/aspeed-pwm-tacho.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
> index b2ab5612d8a4..a31d2648fb3a 100644
> --- a/drivers/hwmon/aspeed-pwm-tacho.c
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -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.
> 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_data
> static 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);
> + }
No hot loop, please. Please use usleep_range() or similar within the loop,
and a well defined timeout.
Guenter
>
> raw_data = val & RESULT_VALUE_MASK;
> tach_div = priv->type_fan_tach_clock_division[type];
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep
2017-06-21 1:32 ` Guenter Roeck
@ 2017-06-21 1:39 ` Guenter Roeck
2017-06-21 1:42 ` Patrick Venture
1 sibling, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2017-06-21 1:39 UTC (permalink / raw)
To: Patrick Venture, joel, jdelvare
Cc: linux-hwmon, raltherr, emilyshaffer, peterh
On 06/20/2017 06:32 PM, Guenter Roeck wrote:
> On 06/20/2017 06:12 PM, Patrick Venture wrote:
>> 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 <venture@google.com>
>> ---
>> drivers/hwmon/aspeed-pwm-tacho.c | 23 +++++++++++++++++------
>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
>> index b2ab5612d8a4..a31d2648fb3a 100644
>> --- a/drivers/hwmon/aspeed-pwm-tacho.c
>> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
>> @@ -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.
>
>> 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_data
>> static 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);
>> + }
>
> 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
> Guenter
>
>> raw_data = val & RESULT_VALUE_MASK;
>> tach_div = priv->type_fan_tach_clock_division[type];
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep
2017-06-21 1:32 ` Guenter Roeck
2017-06-21 1:39 ` Guenter Roeck
@ 2017-06-21 1:42 ` Patrick Venture
2017-06-21 1:52 ` Guenter Roeck
1 sibling, 1 reply; 6+ messages in thread
From: Patrick Venture @ 2017-06-21 1:42 UTC (permalink / raw)
To: Guenter Roeck
Cc: Joel Stanley, jdelvare, linux-hwmon, Rick Altherr, Emily Shaffer,
Peter Hanson
On Tue, Jun 20, 2017 at 6:32 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 06/20/2017 06:12 PM, Patrick Venture wrote:
>>
>> 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 <venture@google.com>
>> ---
>> drivers/hwmon/aspeed-pwm-tacho.c | 23 +++++++++++++++++------
>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c
>> b/drivers/hwmon/aspeed-pwm-tacho.c
>> index b2ab5612d8a4..a31d2648fb3a 100644
>> --- a/drivers/hwmon/aspeed-pwm-tacho.c
>> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
>> @@ -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.
>
>
>> 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_data
>> static 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);
>> + }
>
>
> 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.
> Guenter
>
>
>> raw_data = val & RESULT_VALUE_MASK;
>> tach_div = priv->type_fan_tach_clock_division[type];
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep
2017-06-21 1:42 ` Patrick Venture
@ 2017-06-21 1:52 ` Guenter Roeck
0 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2017-06-21 1:52 UTC (permalink / raw)
To: Patrick Venture
Cc: Joel Stanley, jdelvare, linux-hwmon, Rick Altherr, Emily Shaffer,
Peter Hanson
On 06/20/2017 06:42 PM, Patrick Venture wrote:
> On Tue, Jun 20, 2017 at 6:32 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 06/20/2017 06:12 PM, Patrick Venture wrote:
>>>
>>> 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 <venture@google.com>
>>> ---
>>> drivers/hwmon/aspeed-pwm-tacho.c | 23 +++++++++++++++++------
>>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c
>>> b/drivers/hwmon/aspeed-pwm-tacho.c
>>> index b2ab5612d8a4..a31d2648fb3a 100644
>>> --- a/drivers/hwmon/aspeed-pwm-tacho.c
>>> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
>>> @@ -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.
>>
>>
>>> 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_data
>>> static 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);
>>> + }
>>
>>
>> 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
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
> 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.
>
>> Guenter
>>
>>
>>> 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-21 1:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-21 1:12 [PATCH 1/2] hwmon: (aspeed-pwm-tacho) reduce fan_tach period Patrick Venture
2017-06-21 1:12 ` [PATCH 2/2] hwmon: (aspeed-pwm-tacho) Poll for fan_tach instead of sleep Patrick Venture
2017-06-21 1:32 ` Guenter Roeck
2017-06-21 1:39 ` Guenter Roeck
2017-06-21 1:42 ` Patrick Venture
2017-06-21 1:52 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.