* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox