| Message ID | 20211017013343.3385923-6-david@lechnology.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | counter: ti-eqep: implement features for speed measurement | expand |
On Sat, Oct 16, 2021 at 08:33:40PM -0500, David Lechner wrote: > This adds support to the TI eQEP counter driver for a latched position. > This is a new extension that gets the counter count that was recorded > when an event was triggered. A new device-level latch_mode attribute is > added to select the trigger. Edge capture unit support will be needed > to make full use of this, but "Unit timeout" mode can already be used > to calculate high speeds. > > The unit timer could also have attributes for latched_time and > latched_period that use the same trigger. However this is not a use > case at this time, so they can be added later if needed. I see that "latched_count" holds the captured counter count; would this "latched_time" hold the captured unit timer time? If so, does that mean setting the latch mode to "Unit timeout" always results in a "latched_time" equal to 0 (assuming that's when the timeout event triggers)? > > Signed-off-by: David Lechner <david@lechnology.com> > --- > drivers/counter/ti-eqep.c | 50 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c > index 1ba7f3c7cb7e..ef899655ad1d 100644 > --- a/drivers/counter/ti-eqep.c > +++ b/drivers/counter/ti-eqep.c > @@ -405,12 +405,28 @@ static int ti_eqep_direction_read(struct counter_device *counter, > return 0; > } > > +static int ti_eqep_position_latched_count_read(struct counter_device *counter, > + struct counter_count *count, > + u64 *value) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + u32 qposlat; > + > + regmap_read(priv->regmap32, QPOSLAT, &qposlat); > + > + *value = qposlat; > + > + return 0; > +} > + > static struct counter_comp ti_eqep_position_ext[] = { > COUNTER_COMP_CEILING(ti_eqep_position_ceiling_read, > ti_eqep_position_ceiling_write), > COUNTER_COMP_ENABLE(ti_eqep_position_enable_read, > ti_eqep_position_enable_write), > COUNTER_COMP_DIRECTION(ti_eqep_direction_read), > + COUNTER_COMP_COUNT_U64("latched_count", > + ti_eqep_position_latched_count_read, NULL), > }; > > static struct counter_signal ti_eqep_signals[] = { > @@ -463,6 +479,38 @@ static struct counter_count ti_eqep_counts[] = { > }, > }; > > +static int ti_eqep_latch_mode_read(struct counter_device *counter, > + u32 *value) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + u32 qepctl; > + > + regmap_read(priv->regmap16, QEPCTL, &qepctl); > + *value = !!(qepctl & QEPCTL_QCLM); > + > + return 0; > +} > + > +static int ti_eqep_latch_mode_write(struct counter_device *counter, > + u32 value) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + > + if (value) > + regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_QCLM); > + else > + regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_QCLM); > + > + return 0; > +} > + > +static const char *const ti_eqep_latch_mode_names[] = { > + "Read count", > + "Unit timeout", > +}; > + > +static DEFINE_COUNTER_ENUM(ti_eqep_latch_modes, ti_eqep_latch_mode_names); > + > static int ti_eqep_unit_timer_time_read(struct counter_device *counter, > u64 *value) > { > @@ -553,6 +601,8 @@ static int ti_eqep_unit_timer_enable_write(struct counter_device *counter, > } > > static struct counter_comp ti_eqep_device_ext[] = { > + COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read, > + ti_eqep_latch_mode_write, ti_eqep_latch_modes), It seems more appropriate to move this alongside "latched_count" as Count extension because this is setting the trigger mode to latch the respective Count's count. Or does this particular extension also affect the "latched_time" capture for the unit timer? William Breathitt Gray > COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read, > ti_eqep_unit_timer_time_write), > COUNTER_COMP_DEVICE_U64("unit_timer_period", > -- > 2.25.1 >
On 10/27/21 2:44 AM, William Breathitt Gray wrote: > On Sat, Oct 16, 2021 at 08:33:40PM -0500, David Lechner wrote: >> This adds support to the TI eQEP counter driver for a latched position. >> This is a new extension that gets the counter count that was recorded >> when an event was triggered. A new device-level latch_mode attribute is >> added to select the trigger. Edge capture unit support will be needed >> to make full use of this, but "Unit timeout" mode can already be used >> to calculate high speeds. >> >> The unit timer could also have attributes for latched_time and >> latched_period that use the same trigger. However this is not a use >> case at this time, so they can be added later if needed. > > I see that "latched_count" holds the captured counter count; would this > "latched_time" hold the captured unit timer time? If so, does that mean > setting the latch mode to "Unit timeout" always results in a > "latched_time" equal to 0 (assuming that's when the timeout event > triggers)? > Some `latched_*` attributes will only be useful for one `latched_mode` selection but not the other. These latched registers are used to measure speed. There are two ways to do this. A) measuring the change in position over a fixed time and B) measuring the change in time for a fixed change in position. So for A) latched_mode would be set to trigger on timeout and we would use the latched_position for the measurement. For B) we would set the latched_mode to trigger on reading the count register and use the latched_time as the measurement. ... >> static struct counter_comp ti_eqep_device_ext[] = { >> + COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read, >> + ti_eqep_latch_mode_write, ti_eqep_latch_modes), > > It seems more appropriate to move this alongside "latched_count" as > Count extension because this is setting the trigger mode to latch the > respective Count's count. Or does this particular extension also affect > the "latched_time" capture for the unit timer? > In hardware, there are at least 3 registers that get latched that I recall. They are in different subsystems (main count, unit timer, edge capture). So as you have guessed, that is the reason for having the trigger selection at the device level.
On Wed, Oct 27, 2021 at 10:40:15AM -0500, David Lechner wrote: > On 10/27/21 2:44 AM, William Breathitt Gray wrote: > > On Sat, Oct 16, 2021 at 08:33:40PM -0500, David Lechner wrote: > >> This adds support to the TI eQEP counter driver for a latched position. > >> This is a new extension that gets the counter count that was recorded > >> when an event was triggered. A new device-level latch_mode attribute is > >> added to select the trigger. Edge capture unit support will be needed > >> to make full use of this, but "Unit timeout" mode can already be used > >> to calculate high speeds. > >> > >> The unit timer could also have attributes for latched_time and > >> latched_period that use the same trigger. However this is not a use > >> case at this time, so they can be added later if needed. > > > > I see that "latched_count" holds the captured counter count; would this > > "latched_time" hold the captured unit timer time? If so, does that mean > > setting the latch mode to "Unit timeout" always results in a > > "latched_time" equal to 0 (assuming that's when the timeout event > > triggers)? > > > > Some `latched_*` attributes will only be useful for one `latched_mode` > selection but not the other. > > These latched registers are used to measure speed. There are two ways > to do this. A) measuring the change in position over a fixed time and > B) measuring the change in time for a fixed change in position. So for > A) latched_mode would be set to trigger on timeout and we would use > the latched_position for the measurement. For B) we would set the > latched_mode to trigger on reading the count register and use the > latched_time as the measurement. > > ... > > >> static struct counter_comp ti_eqep_device_ext[] = { > >> + COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read, > >> + ti_eqep_latch_mode_write, ti_eqep_latch_modes), > > > > It seems more appropriate to move this alongside "latched_count" as > > Count extension because this is setting the trigger mode to latch the > > respective Count's count. Or does this particular extension also affect > > the "latched_time" capture for the unit timer? > > > > In hardware, there are at least 3 registers that get latched that I > recall. They are in different subsystems (main count, unit timer, > edge capture). So as you have guessed, that is the reason for having > the trigger selection at the device level. Ah, I see what's going on now. I think supporting these latch registers will involve some further considerations. I'll continue my reply in the respective docs patch where you've gone more in depth about the hardware. William Breathitt Gray
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c index 1ba7f3c7cb7e..ef899655ad1d 100644 --- a/drivers/counter/ti-eqep.c +++ b/drivers/counter/ti-eqep.c @@ -405,12 +405,28 @@ static int ti_eqep_direction_read(struct counter_device *counter, return 0; } +static int ti_eqep_position_latched_count_read(struct counter_device *counter, + struct counter_count *count, + u64 *value) +{ + struct ti_eqep_cnt *priv = counter->priv; + u32 qposlat; + + regmap_read(priv->regmap32, QPOSLAT, &qposlat); + + *value = qposlat; + + return 0; +} + static struct counter_comp ti_eqep_position_ext[] = { COUNTER_COMP_CEILING(ti_eqep_position_ceiling_read, ti_eqep_position_ceiling_write), COUNTER_COMP_ENABLE(ti_eqep_position_enable_read, ti_eqep_position_enable_write), COUNTER_COMP_DIRECTION(ti_eqep_direction_read), + COUNTER_COMP_COUNT_U64("latched_count", + ti_eqep_position_latched_count_read, NULL), }; static struct counter_signal ti_eqep_signals[] = { @@ -463,6 +479,38 @@ static struct counter_count ti_eqep_counts[] = { }, }; +static int ti_eqep_latch_mode_read(struct counter_device *counter, + u32 *value) +{ + struct ti_eqep_cnt *priv = counter->priv; + u32 qepctl; + + regmap_read(priv->regmap16, QEPCTL, &qepctl); + *value = !!(qepctl & QEPCTL_QCLM); + + return 0; +} + +static int ti_eqep_latch_mode_write(struct counter_device *counter, + u32 value) +{ + struct ti_eqep_cnt *priv = counter->priv; + + if (value) + regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_QCLM); + else + regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_QCLM); + + return 0; +} + +static const char *const ti_eqep_latch_mode_names[] = { + "Read count", + "Unit timeout", +}; + +static DEFINE_COUNTER_ENUM(ti_eqep_latch_modes, ti_eqep_latch_mode_names); + static int ti_eqep_unit_timer_time_read(struct counter_device *counter, u64 *value) { @@ -553,6 +601,8 @@ static int ti_eqep_unit_timer_enable_write(struct counter_device *counter, } static struct counter_comp ti_eqep_device_ext[] = { + COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read, + ti_eqep_latch_mode_write, ti_eqep_latch_modes), COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read, ti_eqep_unit_timer_time_write), COUNTER_COMP_DEVICE_U64("unit_timer_period",
This adds support to the TI eQEP counter driver for a latched position. This is a new extension that gets the counter count that was recorded when an event was triggered. A new device-level latch_mode attribute is added to select the trigger. Edge capture unit support will be needed to make full use of this, but "Unit timeout" mode can already be used to calculate high speeds. The unit timer could also have attributes for latched_time and latched_period that use the same trigger. However this is not a use case at this time, so they can be added later if needed. Signed-off-by: David Lechner <david@lechnology.com> --- drivers/counter/ti-eqep.c | 50 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)