| Message ID | 20170621011228.25128-2-venture@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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]; > -- 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
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]; >> > -- 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
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]; >> > -- 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
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 > -- 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
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];
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(-)