[v8,0/4] ECAP support on TI AM62x SoC

The Enhanced Capture (ECAP) module can be used to timestamp events
detected on signal input pin. It can be used for time measurements
of pulse train signals.

ECAP module includes 4 timestamp capture registers. For all 4 sequenced
timestamp capture events (0->1->2->3->0->...), edge polarity (falling/rising
edge) can be selected.

This driver leverages counter subsystem to :
- select edge polarity for all 4 capture events (event mode)
- log timestamps for each capture event
Event polarity, and CAP0/1/2/3 timestamps give all the information
about the input pulse train. Further information can easily be computed :
period and/or duty cycle if frequency is constant, elapsed time between
pulses, etc...

This patchset must be applied on top of the following counter subsystem patchset :
https://lore.kernel.org/all/cover.1663693757.git.william.gray@linaro.org/

Modifications since v7 :
	- Simplify ecap_cnt_capture_get/set_evmode() functions
	- Modify ecap_cnt_count_write() callback
	- Modify ecap_cnt_watch_validate() callback
	- Modify ecap_cnt_pol_read() callback
	- Add ecap_cnt_cap_write() callback
	- Modify ecap_cnt_nb_ovf_write() callback
	- Use DEFINE_COUNTER_ARRAY_CAPTURE() and COUNTER_COMP_ARRAY_CAPTURE() macros
	- Push COUNTER_EVENT_OVERFLOW to all 4 channels in ISR
	- Redefine time_cntr and related functions as u32
	- Remove ecap_cnt_capture_set_evmode() function call in probe function

Userspace commands :
	### CLOCK SIGNAL ###
	cd /sys/bus/counter/devices/counter0/signal0

	# Get frequency
	cat frequency

	### INPUT SIGNAL ###
	cd /sys/bus/counter/devices/counter0/signal1

	# Get polarity for each capture event
	cat polarity0
	cat polarity1
	cat polarity2
	cat polarity3

	# Set polarity for each capture event
	echo positive > polarity0
	echo negative > polarity1
	echo positive > polarity2
	echo negative > polarity3

	### COUNT ###
	cd /sys/bus/counter/devices/counter0/count0

	# Get ceiling (counter max value)
	cat ceiling

	# Reset number of overflows & current timebase counter value
	echo 0 > num_overflows
	echo 0 > count

	# Run ECAP
	echo 1 > enable

	# Get number of overflows & current timebase counter value
	cat num_overflows
	cat count

	# Get captured timestamps
	cat capture0
	cat capture1
	cat capture2
	cat capture3

	# Note that counter watches can also be used to get
	# data from userspace application
	# -> see tools/counter/counter_example.c

	# Pause ECAP
	echo 0 > enable

Julien Panis (4):
  dt-bindings: counter: add ti,am62-ecap-capture.yaml
  Documentation: ABI: sysfs-bus-counter: add frequency & num_overflows
    items
  counter: ti-ecap-capture: capture driver support for ECAP
  MAINTAINERS: add TI ECAP driver info

 Documentation/ABI/testing/sysfs-bus-counter   |  14 +
 .../counter/ti,am62-ecap-capture.yaml         |  61 ++
 MAINTAINERS                                   |   7 +
 drivers/counter/Kconfig                       |  15 +
 drivers/counter/Makefile                      |   1 +
 drivers/counter/ti-ecap-capture.c             | 621 ++++++++++++++++++
 6 files changed, 719 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
 create mode 100644 drivers/counter/ti-ecap-capture.c

On Thu, Sep 22, 2022 at 07:04:01PM +0200, Julien Panis wrote:
> ECAP hardware on TI AM62x SoC supports capture feature. It can be used
> to timestamp events (falling/rising edges) detected on input signal.
> 
> This commit adds capture driver support for ECAP hardware on AM62x SoC.
> 
> In the ECAP hardware, capture pin can also be configured to be in
> PWM mode. Current implementation only supports capture operating mode.
> Hardware also supports timebase sync between multiple instances, but
> this driver supports simple independent capture functionality.
> 
> Signed-off-by: Julien Panis <jpanis@baylibre.com>

Hello Julien,

Comments follow inline below.

> +/**
> + * struct ecap_cnt_dev - device private data structure
> + * @enabled: device state
> + * @clk:     device clock
> + * @regmap:  device register map
> + * @nb_ovf:  number of overflows since capture start
> + * @pm_ctx:  device context for PM operations
> + */
> +struct ecap_cnt_dev {
> +	bool enabled;
> +	struct clk *clk;
> +	struct regmap *regmap;
> +	atomic_t nb_ovf;
> +	struct {
> +		u8 ev_mode;
> +		u32 time_cntr;
> +	} pm_ctx;
> +};

Provide documentation for the ev_mode and time_cntr members. You
probably need a lock as well to protect access to this structure or
you'll end up with race problems.


> +static void ecap_cnt_capture_enable(struct counter_device *counter)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	pm_runtime_get_sync(counter->parent);
> +
> +	/* Enable interrupts on events */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG,
> +			   ECAP_EVT_EN_MASK, ECAP_EVT_EN_MASK);
> +
> +	/* Run counter */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_CFG_MASK,
> +			   ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK);
> +}
> +
> +static void ecap_cnt_capture_disable(struct counter_device *counter)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	/* Disable interrupts on events */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, ECAP_EVT_EN_MASK, 0);
> +
> +	/* Stop counter */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_EN_MASK, 0);

Shouldn't the counter be stopped before stopping the interrupts?

> +static int ecap_cnt_count_get_val(struct counter_device *counter, unsigned int reg, u32 *val)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +	unsigned int regval;
> +
> +	pm_runtime_get_sync(counter->parent);
> +	regmap_read(ecap_dev->regmap, reg, &regval);
> +	pm_runtime_put_sync(counter->parent);
> +
> +	*val = regval;
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_count_set_val(struct counter_device *counter, unsigned int reg, u32 val)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	pm_runtime_get_sync(counter->parent);
> +	regmap_write(ecap_dev->regmap, reg, val);
> +	pm_runtime_put_sync(counter->parent);
> +
> +	return 0;
> +}

The ecap_cnt_count_get_val() and ecap_cnt_count_set_val() functions only
ever return 0. Redefine them as void functions and eliminate the
unnecessary returns.

> +static int ecap_cnt_count_write(struct counter_device *counter,
> +				struct counter_count *count, u64 val)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	if (ecap_dev->enabled)
> +		return -EBUSY;

You should return -EBUSY when the requested operation cannot be
completed because the device currently performing a task -- i.e. the
requested operation would stall or otherwise fail if forced. In this
case, the count value actually can be set while the device is enabled,
if I'm not mistaken; the count just continues increasing from the new
written value (i.e. no stall/failure). Therefore, there's not need to
return -EBUSY here and this check can be eliminated.

> +static int ecap_cnt_pol_write(struct counter_device *counter,
> +			      struct counter_signal *signal,
> +			      size_t idx, enum counter_signal_polarity pol)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	if (ecap_dev->enabled)
> +		return -EBUSY;

I suspect this check can go away for the same reason as above.

> +static int ecap_cnt_cap_write(struct counter_device *counter,
> +			      struct counter_count *count,
> +			      size_t idx, u64 cap)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	if (ecap_dev->enabled)
> +		return -EBUSY;

Same comment as above.

> +static int ecap_cnt_nb_ovf_write(struct counter_device *counter,
> +				 struct counter_count *count, u64 val)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	if (ecap_dev->enabled)
> +		return -EBUSY;

Same comment as above.

> +static struct counter_count ecap_cnt_counts[] = {
> +	{
> +		.id = 0,

The id member is for differentiating between multiple Counts. You only
have one Count in this driver so you don't need to set it because you
never use it.

William Breathitt Gray

On Fri, Sep 23, 2022 at 02:32:26PM +0200, Julien Panis wrote:
> 
> 
> On 23/09/2022 14:17, Julien Panis wrote:
> > 
> > 
> > On 23/09/2022 13:35, William Breathitt Gray wrote:
> > > On Fri, Sep 23, 2022 at 09:23:26AM +0200, Julien Panis wrote:
> > > > 
> > > > On 23/09/2022 03:08, William Breathitt Gray wrote:
> > > > > On Thu, Sep 22, 2022 at 07:04:01PM +0200, Julien Panis wrote:
> > > > > > ECAP hardware on TI AM62x SoC supports capture feature.
> > > > > > It can be used
> > > > > > to timestamp events (falling/rising edges) detected on input signal.
> > > > > > 
> > > > > > This commit adds capture driver support for ECAP
> > > > > > hardware on AM62x SoC.
> > > > > > 
> > > > > > In the ECAP hardware, capture pin can also be configured to be in
> > > > > > PWM mode. Current implementation only supports capture
> > > > > > operating mode.
> > > > > > Hardware also supports timebase sync between multiple instances, but
> > > > > > this driver supports simple independent capture functionality.
> > > > > > 
> > > > > > Signed-off-by: Julien Panis <jpanis@baylibre.com>
> > > > > Hello Julien,
> > > > > 
> > > > > Comments follow inline below.
> > > > > 
> > > > > > +/**
> > > > > > + * struct ecap_cnt_dev - device private data structure
> > > > > > + * @enabled: device state
> > > > > > + * @clk:     device clock
> > > > > > + * @regmap:  device register map
> > > > > > + * @nb_ovf:  number of overflows since capture start
> > > > > > + * @pm_ctx:  device context for PM operations
> > > > > > + */
> > > > > > +struct ecap_cnt_dev {
> > > > > > +    bool enabled;
> > > > > > +    struct clk *clk;
> > > > > > +    struct regmap *regmap;
> > > > > > +    atomic_t nb_ovf;
> > > > > > +    struct {
> > > > > > +        u8 ev_mode;
> > > > > > +        u32 time_cntr;
> > > > > > +    } pm_ctx;
> > > > > > +};
> > > > > Provide documentation for the ev_mode and time_cntr members. You
> > > > > probably need a lock as well to protect access to this structure or
> > > > > you'll end up with race problems.
> > > > Hi William,
> > > > 
> > > > How can I end up with race problems ? pm_ctx members are only
> > > > accessed at
> > > > suspend (after capture/IRQ are disabled) and resume (before
> > > > capture/IRQ are
> > > > re-enabled).
> > > > Is there any risk I did not identify ?
> > > > 
> > > > Julien
> > > I was thinking of the ecap_cnt_dev enabled member. The Counter callbacks
> > > may execute in concurrent threads, so races can appear when you access
> > > members of the ecap_cnt_dev structure in these callbacks.
> > > 
> > > Take for example this section of ecap_cnt_enable_write():
> > > 
> > >          if (enable == ecap_dev->enabled)
> > >                  return 0;
> > >          if (enable)
> > >                  ecap_cnt_capture_enable(counter);
> > >          else
> > >                  ecap_cnt_capture_disable(counter);
> > >          ecap_dev->enabled = enable
> > > 
> > > Suppose two threads try to enable the count capture. A race condition is
> > > present where the two threads could see ecap_dev->enabled as false and
> > > both proceed to call ecap_cnt_capture_enable(). This results in
> > > pm_runtime_get_sync() bumping the usage count twice and we're left with
> > > a mismatch the next time ecap_cnt_capture_disable() is called.
> > > 
> > > William Breathitt Gray
> > 
> > OK, If I understand well there's the same problem with IO access with
> > regmap ?
> > Julien
> 
> [ERRATUM] It seems that some spinlock is already used by regmap API.
> So, only the 'enabled' member needs a lock.

Keep in mind that although individual regmap API calls can be considered
atomic, the order of the calls made could be a point of race if there
are concurrent threads. However, in your particular driver it looks
you're fine as long as access to 'enabled' is protected.

William Breathitt Gray