[v4,3/3] counter: capture-tiecap: capture driver support for ECAP

ECAP hardware on AM62x SoC supports capture feature. It can be used
to timestamp events (falling/rising edges) detected on signal input pin.

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>
---
 drivers/counter/Kconfig          |  14 +
 drivers/counter/Makefile         |   1 +
 drivers/counter/capture-tiecap.c | 634 +++++++++++++++++++++++++++++++
 include/uapi/linux/counter.h     |   2 +
 4 files changed, 651 insertions(+)
 create mode 100644 drivers/counter/capture-tiecap.c

On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
> ECAP hardware on AM62x SoC supports capture feature. It can be used
> to timestamp events (falling/rising edges) detected on signal input pin.
> 
> 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>
Hi Julien,

You picked up the Counter paradigm pretty quickly, good job! Some
suggestions and comments inline below.

> ---
>  drivers/counter/Kconfig          |  14 +
>  drivers/counter/Makefile         |   1 +
>  drivers/counter/capture-tiecap.c | 634 +++++++++++++++++++++++++++++++
>  include/uapi/linux/counter.h     |   2 +
>  4 files changed, 651 insertions(+)
>  create mode 100644 drivers/counter/capture-tiecap.c
> 
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 5edd155f1911..580640807a94 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -101,4 +101,18 @@ config INTEL_QEP
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called intel-qep.
>  
> +config CAPTURE_TIECAP
> +	tristate "TI eCAP capture driver"
> +	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  Select this option to enable the Texas Instruments Enhanced Capture
> +	  (eCAP) driver in input mode.
> +
> +	  It can be used to timestamp events (falling/rising edges) detected
> +	  on signal input pin.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called capture-tiecap.
> +

The naming convention I typically see is to lead with the company name
followed by device model; e.g. ti-ecap-capture. That should result in
better grouping with the existing drivers in the drivers/counter
directory. It's a minor benefit, so let's use that convention unless
there's a reason why capture-tiecap is better here.

>  endif # COUNTER
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index 8fde6c100ebc..7fac3e0985b5 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
>  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
>  obj-$(CONFIG_MICROCHIP_TCB_CAPTURE)	+= microchip-tcb-capture.o
>  obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
> +obj-$(CONFIG_CAPTURE_TIECAP)	+= capture-tiecap.o
> diff --git a/drivers/counter/capture-tiecap.c b/drivers/counter/capture-tiecap.c
> new file mode 100644
> index 000000000000..0601c65ef203
> --- /dev/null
> +++ b/drivers/counter/capture-tiecap.c
> @@ -0,0 +1,634 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * ECAP Capture driver
> + *
> + * Copyright (C) 2022 Julien Panis <jpanis@baylibre.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/counter.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#define ECAP_DRV_NAME "ecap"
> +
> +/* Registers */
> +#define ECAP_NB_CAP			4
> +
> +#define ECAP_TSCNT_REG			0x00
> +
> +#define ECAP_CAP_REG(i)		(((i) << 2) + 0x08)
> +
> +#define ECAP_ECCTL_REG			0x28
> +#define ECAP_CAPPOL_BIT(i)		BIT((i) << 1)
> +#define ECAP_EV_MODE_MASK		GENMASK(7, 0)
> +#define ECAP_CAPLDEN_BIT		BIT(8)
> +#define ECAP_CONT_ONESHT_BIT		BIT(16)
> +#define ECAP_STOPVALUE_MASK		GENMASK(18, 17)
> +#define ECAP_REARM_RESET_BIT		BIT(19)
> +#define ECAP_TSCNTSTP_BIT		BIT(20)
> +#define ECAP_SYNCO_DIS_MASK		GENMASK(23, 22)
> +#define ECAP_CAP_APWM_BIT		BIT(25)
> +#define ECAP_ECCTL_EN_MASK		(ECAP_CAPLDEN_BIT | ECAP_TSCNTSTP_BIT)
> +#define ECAP_ECCTL_CFG_MASK		(ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK	\
> +					| ECAP_ECCTL_EN_MASK | ECAP_REARM_RESET_BIT	\
> +					| ECAP_CAP_APWM_BIT | ECAP_CONT_ONESHT_BIT)
> +
> +#define ECAP_ECINT_EN_FLG_REG		0x2c
> +#define ECAP_NB_CEVT			(ECAP_NB_CAP + 1)
> +#define ECAP_CEVT_EN_MASK		GENMASK(ECAP_NB_CEVT, ECAP_NB_CAP)
> +#define ECAP_CEVT_FLG_BIT(i)		BIT((i) + 17)
> +
> +#define ECAP_ECINT_CLR_FRC_REG	0x30
> +#define ECAP_INT_CLR_BIT		BIT(0)
> +#define ECAP_CEVT_CLR_BIT(i)		BIT((i) + 1)
> +#define ECAP_CEVT_CLR_MASK		GENMASK(ECAP_NB_CEVT, 0)
> +
> +#define ECAP_PID_REG			0x5c
> +
> +/*
> + * Event modes
> + * One bit for each CAPx register : 1 = falling edge / 0 = rising edge
> + * e.g. mode = 13 = 0xd = 0b1101
> + * -> falling edge for CAP1-3-4 / rising edge for CAP2
> + */
> +#define ECAP_EV_MODE_BIT(i)		BIT(i)
> +
> +/* ECAP input */
> +#define ECAP_SIGNAL 0
> +
> +static const struct regmap_config ecap_cnt_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.max_register = ECAP_PID_REG,
> +};
> +
> +/**
> + * struct ecap_cnt_dev - device private data structure
> + * @enabled:      device state
> + * @clk:          device clock
> + * @clk_rate:     device clock rate
> + * @regmap:       device register map
> + * @elapsed_time: elapsed time since capture start
> + * @lock:         synchronization lock to prevent race conditions when computing times
> + * @pm_ctx:       device context for PM operations
> + */
> +struct ecap_cnt_dev {
> +	bool enabled;
> +	struct clk *clk;
> +	unsigned long clk_rate;
> +	struct regmap *regmap;
> +	__aligned_u64 elapsed_time;
> +	spinlock_t lock;
> +	struct {
> +		u8 ev_mode;
> +		unsigned int time_cntr;
> +	} pm_ctx;
> +};
> +
> +static u8 ecap_cnt_capture_get_evmode(struct counter_device *counter)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +	u8 ev_mode = 0;
> +	unsigned int regval;
> +	int i;
> +
> +	pm_runtime_get_sync(counter->parent);
> +	regmap_read(ecap_dev->regmap, ECAP_ECCTL_REG, &regval);
> +	pm_runtime_put_sync(counter->parent);
> +
> +	for (i = 0 ; i < ECAP_NB_CAP ; i++) {
> +		if (regval & ECAP_CAPPOL_BIT(i))
> +			ev_mode |= ECAP_EV_MODE_BIT(i);
> +	}
> +
> +	return ev_mode;
> +}
> +
> +static void ecap_cnt_capture_set_evmode(struct counter_device *counter, u8 ev_mode)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +	unsigned int regval = 0;
> +	int i;
> +
> +	for (i = 0 ; i < ECAP_NB_CAP ; i++) {
> +		if (ev_mode & ECAP_EV_MODE_BIT(i))
> +			regval |= ECAP_CAPPOL_BIT(i);
> +	}
> +
> +	pm_runtime_get_sync(counter->parent);
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_EV_MODE_MASK, regval);
> +	pm_runtime_put_sync(counter->parent);
> +}
> +
> +static void ecap_cnt_capture_enable(struct counter_device *counter, bool rearm)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +	unsigned int regval;
> +
> +	pm_runtime_get_sync(counter->parent);
> +
> +	/* Enable interrupts on events */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG,
> +			   ECAP_CEVT_EN_MASK, ECAP_CEVT_EN_MASK);
> +
> +	/* Run counter */
> +	regval = ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK;
> +	if (rearm) {
> +		ecap_dev->elapsed_time = 0;
> +		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, 0);
> +		regval |= ECAP_REARM_RESET_BIT;
> +	}
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_CFG_MASK, regval);
> +}
> +
> +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_CEVT_EN_MASK, 0);
> +
> +	/* Stop counter */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_EN_MASK, 0);
> +
> +	pm_runtime_put_sync(counter->parent);
> +}
> +
> +static int ecap_cnt_count_get_ns(struct counter_device *counter, unsigned int reg, u64 *val)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +	unsigned int regval;
> +	unsigned long flags;
> +
> +	pm_runtime_get_sync(counter->parent);
> +	spin_lock_irqsave(&ecap_dev->lock, flags);
> +
> +	/* time_ns = 10^9 * (time_cycles + time_cumul) / clk_rate */
> +	regmap_read(ecap_dev->regmap, reg, &regval);
> +	*val = (regval + ecap_dev->elapsed_time) * NSEC_PER_SEC;
> +	do_div(*val, ecap_dev->clk_rate);
> +
> +	spin_unlock_irqrestore(&ecap_dev->lock, flags);
> +	pm_runtime_put_sync(counter->parent);
> +
> +	return 0;
> +}
> +
> +static inline int ecap_cnt_count_read(struct counter_device *counter,
> +				      struct counter_count *count,
> +				      u64 *val)
> +{
> +	return ecap_cnt_count_get_ns(counter, ECAP_TSCNT_REG, val);
> +}

This conversion to nanoseconds is an interpretation of the count value
that belongs in userspace. Instead of converting to nanoseconds here,
simply expose the count value directly and add extensions to expose the
elapsed time (a Count extension) and clock rate (a Signal extension) as
well; with these three values you can derive nanoseconds in userspace
using the time_ns equation in your ecap_cnt_count_get_ns() comment.

> +static int ecap_cnt_function_read(struct counter_device *counter,
> +				  struct counter_count *count,
> +				  enum counter_function *function)
> +{
> +	*function = COUNTER_FUNCTION_INCREASE;
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_action_read(struct counter_device *counter,
> +				struct counter_count *count,
> +				struct counter_synapse *synapse,
> +				enum counter_synapse_action *action)
> +{
> +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> +
> +	return 0;
> +}

Right now you have a Signal defined for the ECAPSIG line, but there is
at least one more relevant Signal to define: the clock updating ECAPCNT.
The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
clock Signal, but for the ECAPSIG Signal you will need to report a
Synapse action based on the polarity of the next capture (i.e. whether
high or low).

> +
> +static int ecap_cnt_watch_validate(struct counter_device *counter,
> +				   const struct counter_watch *watch)
> +{
> +	struct counter_event_node *event_node;
> +
> +	if (watch->channel || watch->event != COUNTER_EVENT_CAPTURE)
> +		return -EINVAL;

The intention of this conditional is more obvious if you explicitly
check for watch->channel != 0.

> +
> +	list_for_each_entry(event_node, &counter->next_events_list, l)
> +		if (watch->component.type != COUNTER_COMPONENT_EXTENSION ||
> +		    watch->component.scope != COUNTER_SCOPE_COUNT ||
> +		    watch->component.parent ||
> +		    watch->component.id >= ECAP_NB_CAP)
> +			return -EINVAL;

You don't need this list_for_each_entry loop at all. The purpose of the
watch_validate() callback is to make sure a watch isn't added with a
channel or event configuration that conflicts with existing watches in
the next_events_list list.

For example, the 104-QUAD-8 device only allows one particular event to
occur on any given channel. The quad8_watch_validate() function makes
sure that all watches for a particular channel are set for the same
event; if a watch for that channel is set for a different event, then
that watch is invalid because the 104-QUAD-8 device cannot handle such
a request for two different events on the same channel.

Regarding the TI ECAP device, it looks like it only supports that one
COUNTER_EVENT_CAPTURE event so you don't need to check the other watches
in the next_events_list list. Your conditional check above for
watch->channel and watch->event should be enough by itself.

> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_enable_read(struct counter_device *counter,
> +				struct counter_count *count,
> +				u8 *enable)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	*enable = ecap_dev->enabled;
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_enable_write(struct counter_device *counter,
> +				 struct counter_count *count,
> +				 u8 enable)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	if (enable > 1)
> +		return -EINVAL;
> +	if (enable == ecap_dev->enabled)
> +		return 0;
> +	if (enable)
> +		ecap_cnt_capture_enable(counter, true);
> +	else
> +		ecap_cnt_capture_disable(counter);
> +	ecap_dev->enabled = enable;
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_cap_get_pol(struct counter_device *counter,
> +				struct counter_signal *signal,
> +				u32 *pol, u8 inst)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	pm_runtime_get_sync(counter->parent);
> +	*pol = regmap_test_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
> +	pm_runtime_put_sync(counter->parent);
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_cap_set_pol(struct counter_device *counter,
> +				struct counter_signal *signal,
> +				u32 pol, u8 inst)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	if (ecap_dev->enabled)
> +		return -EBUSY;
> +	if (pol > 1)
> +		return -EINVAL;
> +
> +	pm_runtime_get_sync(counter->parent);
> +	if (pol)
> +		regmap_set_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
> +	else
> +		regmap_clear_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
> +	pm_runtime_put_sync(counter->parent);
> +
> +	return 0;
> +}
> +
> +static inline int ecap_cnt_cap_read(struct counter_device *counter,
> +				    struct counter_count *count,
> +				    u64 *cap, u8 inst)
> +{
> +	return ecap_cnt_count_get_ns(counter, ECAP_CAP_REG(inst), cap);
> +}

Return the captured count value directly here; userspace can handle the
nanosecond interpretation for the same reason explained earlier for the
ECAPCNT count value.

> +static inline int ecap_cnt_cap1_get_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 *pol)
> +{
> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 0);
> +}
> +
> +static inline int ecap_cnt_cap2_get_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 *pol)
> +{
> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 1);
> +}
> +
> +static inline int ecap_cnt_cap3_get_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 *pol)
> +{
> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 2);
> +}
> +
> +static inline int ecap_cnt_cap4_get_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 *pol)
> +{
> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 3);
> +}
> +
> +static inline int ecap_cnt_cap1_set_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 pol)
> +{
> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 0);
> +}
> +
> +static inline int ecap_cnt_cap2_set_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 pol)
> +{
> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 1);
> +}
> +
> +static inline int ecap_cnt_cap3_set_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 pol)
> +{
> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 2);
> +}
> +
> +static inline int ecap_cnt_cap4_set_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 pol)
> +{
> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 3);
> +}
> +
> +static inline int ecap_cnt_cap1_read(struct counter_device *counter,
> +				     struct counter_count *count, u64 *cap)
> +{
> +	return ecap_cnt_cap_read(counter, count, cap, 0);
> +}
> +
> +static inline int ecap_cnt_cap2_read(struct counter_device *counter,
> +				     struct counter_count *count, u64 *cap)
> +{
> +	return ecap_cnt_cap_read(counter, count, cap, 1);
> +}
> +
> +static inline int ecap_cnt_cap3_read(struct counter_device *counter,
> +				     struct counter_count *count, u64 *cap)
> +{
> +	return ecap_cnt_cap_read(counter, count, cap, 2);
> +}
> +
> +static inline int ecap_cnt_cap4_read(struct counter_device *counter,
> +				     struct counter_count *count, u64 *cap)
> +{
> +	return ecap_cnt_cap_read(counter, count, cap, 3);
> +}

There's a lot of boilerplate code here for each ECAP register read/write
callback. Try defining a macro to build these so you can define each
read/write callback pair more succinctly.

> +static const struct counter_ops ecap_cnt_ops = {
> +	.count_read = ecap_cnt_count_read,
> +	.function_read = ecap_cnt_function_read,
> +	.action_read = ecap_cnt_action_read,
> +	.watch_validate = ecap_cnt_watch_validate,
> +};
> +
> +static const enum counter_function ecap_cnt_functions[] = {
> +	COUNTER_FUNCTION_INCREASE,
> +};
> +
> +static const enum counter_synapse_action ecap_cnt_actions[] = {
> +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +};
> +
> +static const char *const ecap_cnt_cap_polarities[] = {
> +	"rising",
> +	"falling",
> +};
> +
> +static DEFINE_COUNTER_ENUM(ecap_cnt_cap_avail_pol, ecap_cnt_cap_polarities);
> +
> +static struct counter_comp ecap_cnt_signal_ext[] = {
> +	COUNTER_COMP_SIGNAL_ENUM("polarity1", ecap_cnt_cap1_get_pol,
> +				 ecap_cnt_cap1_set_pol, ecap_cnt_cap_avail_pol),
> +	COUNTER_COMP_SIGNAL_ENUM("polarity2", ecap_cnt_cap2_get_pol,
> +				 ecap_cnt_cap2_set_pol, ecap_cnt_cap_avail_pol),
> +	COUNTER_COMP_SIGNAL_ENUM("polarity3", ecap_cnt_cap3_get_pol,
> +				 ecap_cnt_cap3_set_pol, ecap_cnt_cap_avail_pol),
> +	COUNTER_COMP_SIGNAL_ENUM("polarity4", ecap_cnt_cap4_get_pol,
> +				 ecap_cnt_cap4_set_pol, ecap_cnt_cap_avail_pol),
> +};
> +
> +static struct counter_signal ecap_cnt_signals[] = {
> +	{
> +		.id = ECAP_SIGNAL,
> +		.name = "ECAPSIG",
> +		.ext = ecap_cnt_signal_ext,
> +		.num_ext = ARRAY_SIZE(ecap_cnt_signal_ext),
> +	},
> +};

As mentioned earlier, define another Signal here representing the clock
feeding ECAPCNT. You should also define another extensions array for
this new Signal that will provide the clock rate for userspace to read.

> +static struct counter_synapse ecap_cnt_synapses[] = {
> +	{
> +		.actions_list = ecap_cnt_actions,
> +		.num_actions = ARRAY_SIZE(ecap_cnt_actions),
> +		.signal = &ecap_cnt_signals[ECAP_SIGNAL],
> +	},
> +};

You will need to define another Synapse here for the clock Signal
mentioned above; similarly, you need to define another actions array for
the possible edge triggers.

> +static struct counter_comp ecap_cnt_count_ext[] = {
> +	COUNTER_COMP_COUNT_U64("capture1", ecap_cnt_cap1_read, NULL),
> +	COUNTER_COMP_COUNT_U64("capture2", ecap_cnt_cap2_read, NULL),
> +	COUNTER_COMP_COUNT_U64("capture3", ecap_cnt_cap3_read, NULL),
> +	COUNTER_COMP_COUNT_U64("capture4", ecap_cnt_cap4_read, NULL),
> +	COUNTER_COMP_ENABLE(ecap_cnt_enable_read, ecap_cnt_enable_write),

I just want to verify: this enable extension should disable the ECAPCNT
count itself (i.e. no more increasing count value). Is that what's
happening here, or is this meant to disable just the captures?

> +};
> +
> +static struct counter_count ecap_cnt_counts[] = {
> +	{
> +		.id = 0,
> +		.name = "ECAPCNT",
> +		.functions_list = ecap_cnt_functions,
> +		.num_functions = ARRAY_SIZE(ecap_cnt_functions),
> +		.synapses = ecap_cnt_synapses,
> +		.num_synapses = ARRAY_SIZE(ecap_cnt_synapses),
> +		.ext = ecap_cnt_count_ext,
> +		.num_ext = ARRAY_SIZE(ecap_cnt_count_ext),
> +	},
> +};
> +
> +static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *counter_dev = dev_id;
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> +	unsigned int clr = 0;
> +	unsigned int flg;
> +	int i;
> +	unsigned long flags;
> +
> +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
> +
> +	for (i = ECAP_NB_CAP - 1 ; i < ECAP_NB_CEVT ; i++) {

Would you walk me through the logic for this loop. Is this for-loop
intended to loop through all four capture indices? ECAP_NB_CAP and
ECAP_NB_CEVT are defines, so your for-loop has i=3 and i<5; is this what
you want?

> +		if (flg & ECAP_CEVT_FLG_BIT(i)) {
> +			if (i < ECAP_NB_CAP) {
> +				/* Input signal edge detected on last CAP (CAP4) */
> +				counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, 0);
> +			} else {
> +				/* Counter overflow */
> +				spin_lock_irqsave(&ecap_dev->lock, flags);
> +				ecap_dev->elapsed_time += (u64)U32_MAX + 1;
> +				spin_unlock_irqrestore(&ecap_dev->lock, flags);

Should we push COUNTER_EVENT_OVERFLOW here? Since elapsed_time always
increments by the same constant (U32_MAX), it would be better to replace
this with an atomic_t "num_overflows" to track the number of overflows
with atomic_inc() and compute the particular elapsed_time later.

William Breathitt Gray

> +			}
> +
> +			clr |= ECAP_CEVT_CLR_BIT(i);
> +		}
> +	}
> +
> +	clr |= ECAP_INT_CLR_BIT;
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_CLR_FRC_REG, ECAP_CEVT_CLR_MASK, clr);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void ecap_cnt_clk_disable(void *clk)
> +{
> +	clk_disable_unprepare(clk);
> +}
> +
> +static int ecap_cnt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ecap_cnt_dev *ecap_dev;
> +	struct counter_device *counter_dev;
> +	void __iomem *mmio_base;
> +	int ret;
> +
> +	counter_dev = devm_counter_alloc(dev, sizeof(*ecap_dev));
> +	if (IS_ERR(counter_dev))
> +		return PTR_ERR(counter_dev);
> +
> +	counter_dev->name = ECAP_DRV_NAME;
> +	counter_dev->parent = dev;
> +	counter_dev->ops = &ecap_cnt_ops;
> +	counter_dev->signals = ecap_cnt_signals;
> +	counter_dev->num_signals = ARRAY_SIZE(ecap_cnt_signals);
> +	counter_dev->counts = ecap_cnt_counts;
> +	counter_dev->num_counts = ARRAY_SIZE(ecap_cnt_counts);
> +
> +	ecap_dev = counter_priv(counter_dev);
> +
> +	ecap_dev->clk = devm_clk_get(dev, "fck");
> +	if (IS_ERR(ecap_dev->clk))
> +		return dev_err_probe(dev, PTR_ERR(ecap_dev->clk), "failed to get clock\n");
> +
> +	ret = clk_prepare_enable(ecap_dev->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, ecap_cnt_clk_disable, ecap_dev->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to add clock disable action\n");
> +		return ret;
> +	}
> +
> +	ecap_dev->clk_rate = clk_get_rate(ecap_dev->clk);
> +	if (!ecap_dev->clk_rate) {
> +		dev_err(dev, "failed to get clock rate\n");
> +		return -EINVAL;
> +	}
> +
> +	mmio_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mmio_base))
> +		return PTR_ERR(mmio_base);
> +
> +	ecap_dev->regmap = devm_regmap_init_mmio(dev, mmio_base, &ecap_cnt_regmap_config);
> +	if (IS_ERR(ecap_dev->regmap))
> +		return dev_err_probe(dev, PTR_ERR(ecap_dev->regmap), "failed to init regmap\n");
> +
> +	spin_lock_init(&ecap_dev->lock);
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get irq\n");
> +		return ret;
> +	}
> +
> +	ret = devm_request_irq(dev, ret, ecap_cnt_isr, 0, pdev->name, counter_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, counter_dev);
> +	pm_runtime_enable(dev);
> +
> +	ecap_dev->enabled = 0;
> +	ecap_cnt_capture_set_evmode(counter_dev, 0);
> +
> +	ret = devm_counter_add(dev, counter_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to add counter\n");
> +		pm_runtime_disable(dev);
> +	}
> +
> +	return ret;
> +}
> +
> +static int ecap_cnt_remove(struct platform_device *pdev)
> +{
> +	struct counter_device *counter_dev = platform_get_drvdata(pdev);
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> +
> +	if (ecap_dev->enabled)
> +		ecap_cnt_capture_disable(counter_dev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_suspend(struct device *dev)
> +{
> +	struct counter_device *counter_dev = dev_get_drvdata(dev);
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> +
> +	/* If eCAP is running, stop capture then save timestamp counter */
> +	if (ecap_dev->enabled) {
> +		ecap_cnt_capture_disable(counter_dev);
> +
> +		pm_runtime_get_sync(dev);
> +		regmap_read(ecap_dev->regmap, ECAP_TSCNT_REG, &ecap_dev->pm_ctx.time_cntr);
> +		pm_runtime_put_sync(dev);
> +	}
> +
> +	ecap_dev->pm_ctx.ev_mode = ecap_cnt_capture_get_evmode(counter_dev);
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_resume(struct device *dev)
> +{
> +	struct counter_device *counter_dev = dev_get_drvdata(dev);
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> +
> +	ecap_cnt_capture_set_evmode(counter_dev, ecap_dev->pm_ctx.ev_mode);
> +
> +	/* If eCAP was running, restore timestamp counter then run capture */
> +	if (ecap_dev->enabled) {
> +		pm_runtime_get_sync(dev);
> +		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, ecap_dev->pm_ctx.time_cntr);
> +		pm_runtime_put_sync(dev);
> +
> +		ecap_cnt_capture_enable(counter_dev, false);
> +	}
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(ecap_cnt_pm_ops, ecap_cnt_suspend, ecap_cnt_resume);
> +
> +static const struct of_device_id ecap_cnt_of_match[] = {
> +	{ .compatible	= "ti,am62-ecap-capture" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ecap_cnt_of_match);
> +
> +static struct platform_driver ecap_cnt_driver = {
> +	.probe = ecap_cnt_probe,
> +	.remove = ecap_cnt_remove,
> +	.driver = {
> +		.name = "ecap-capture",
> +		.of_match_table = ecap_cnt_of_match,
> +		.pm = pm_sleep_ptr(&ecap_cnt_pm_ops),
> +	},
> +};
> +module_platform_driver(ecap_cnt_driver);
> +
> +MODULE_DESCRIPTION("ECAP Capture driver");
> +MODULE_AUTHOR("Julien Panis <jpanis@baylibre.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> index 96c5ffd368ad..4c5372c6f2a3 100644
> --- a/include/uapi/linux/counter.h
> +++ b/include/uapi/linux/counter.h
> @@ -63,6 +63,8 @@ enum counter_event_type {
>  	COUNTER_EVENT_INDEX,
>  	/* State of counter is changed */
>  	COUNTER_EVENT_CHANGE_OF_STATE,
> +	/* Count value is captured */
> +	COUNTER_EVENT_CAPTURE,
>  };
>  
>  /**
> -- 
> 2.25.1
>

On Sun, Aug 14, 2022 at 01:03:48PM -0400, William Breathitt Gray wrote:
> On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
> > +static int ecap_cnt_function_read(struct counter_device *counter,
> > +				  struct counter_count *count,
> > +				  enum counter_function *function)
> > +{
> > +	*function = COUNTER_FUNCTION_INCREASE;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ecap_cnt_action_read(struct counter_device *counter,
> > +				struct counter_count *count,
> > +				struct counter_synapse *synapse,
> > +				enum counter_synapse_action *action)
> > +{
> > +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > +
> > +	return 0;
> > +}
> 
> Right now you have a Signal defined for the ECAPSIG line, but there is
> at least one more relevant Signal to define: the clock updating ECAPCNT.
> The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
> clock Signal, but for the ECAPSIG Signal you will need to report a
> Synapse action based on the polarity of the next capture (i.e. whether
> high or low).

I need to make a correction here. IIUC, the ECAPSIG signal doesn't
affect the count value of ECAPCNT (ECAPSIG only triggers the captures),
so the Synapse action for ECAPSIG should always be
COUNTER_SYNAPSE_ACTION_NONE. You don't need to account for the capture
polarities because they're not relevant in this particular situation:
ECAPSIG doesn't trigger the ECAPCNT count function.

William Breathitt Gray

On 15/08/2022 13:20, William Breathitt Gray wrote:
> On Sun, Aug 14, 2022 at 01:03:48PM -0400, William Breathitt Gray wrote:
>> On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
>>> +static int ecap_cnt_function_read(struct counter_device *counter,
>>> +				  struct counter_count *count,
>>> +				  enum counter_function *function)
>>> +{
>>> +	*function = COUNTER_FUNCTION_INCREASE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int ecap_cnt_action_read(struct counter_device *counter,
>>> +				struct counter_count *count,
>>> +				struct counter_synapse *synapse,
>>> +				enum counter_synapse_action *action)
>>> +{
>>> +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
>>> +
>>> +	return 0;
>>> +}
>> Right now you have a Signal defined for the ECAPSIG line, but there is
>> at least one more relevant Signal to define: the clock updating ECAPCNT.
>> The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
>> clock Signal, but for the ECAPSIG Signal you will need to report a
>> Synapse action based on the polarity of the next capture (i.e. whether
>> high or low).
> I need to make a correction here. IIUC, the ECAPSIG signal doesn't
> affect the count value of ECAPCNT (ECAPSIG only triggers the captures),
> so the Synapse action for ECAPSIG should always be
> COUNTER_SYNAPSE_ACTION_NONE. You don't need to account for the capture
> polarities because they're not relevant in this particular situation:
> ECAPSIG doesn't trigger the ECAPCNT count function.
>
> William Breathitt Gray

It appears to me that you spoke about TSCNT register content (32 bits). 
So, you were
not talking about the Mod4 counter (2 bits).
Do you confirm that ?

On Tue, Aug 16, 2022 at 09:58:10AM +0200, Julien Panis wrote:
> On 14/08/2022 19:03, William Breathitt Gray wrote:
> > On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
> > > +static int ecap_cnt_function_read(struct counter_device *counter,
> > > +				  struct counter_count *count,
> > > +				  enum counter_function *function)
> > > +{
> > > +	*function = COUNTER_FUNCTION_INCREASE;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ecap_cnt_action_read(struct counter_device *counter,
> > > +				struct counter_count *count,
> > > +				struct counter_synapse *synapse,
> > > +				enum counter_synapse_action *action)
> > > +{
> > > +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > > +
> > > +	return 0;
> > > +}
> > Right now you have a Signal defined for the ECAPSIG line, but there is
> > at least one more relevant Signal to define: the clock updating ECAPCNT.
> > The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
> > clock Signal, but for the ECAPSIG Signal you will need to report a
> > Synapse action based on the polarity of the next capture (i.e. whether
> > high or low).
> 
> Just to be sure : by using the word ECAPCNT, I guess that you speak about
> the
> Mod4 counter (0->1->2->3->0...), don't you ? (2 bits)
> Or do you speak about ECAP_TSCNT_REG register content ? (32 bits)

Sorry for the confusion, I'm talking about ECAP_TSCNT_REG (32-bit) here.
You should rename this Count in your ecap_cnt_counts array from
"ECAPCNT" to "Time-Stamp Counter" to make it clearer to users as well;
it would be prudent to rename "ECAPSIG" too.

I didn't know that there was a register exposing the Mod4 counter value.
If that's true, then define a Count for the Mod4 counter in your
ecap_cnt_counts array.

> > > +static struct counter_comp ecap_cnt_count_ext[] = {
> > > +	COUNTER_COMP_COUNT_U64("capture1", ecap_cnt_cap1_read, NULL),
> > > +	COUNTER_COMP_COUNT_U64("capture2", ecap_cnt_cap2_read, NULL),
> > > +	COUNTER_COMP_COUNT_U64("capture3", ecap_cnt_cap3_read, NULL),
> > > +	COUNTER_COMP_COUNT_U64("capture4", ecap_cnt_cap4_read, NULL),
> > > +	COUNTER_COMP_ENABLE(ecap_cnt_enable_read, ecap_cnt_enable_write),
> > I just want to verify: this enable extension should disable the ECAPCNT
> > count itself (i.e. no more increasing count value). Is that what's
> > happening here, or is this meant to disable just the captures?
> 
> Yes, it is what's happening here : no more increasing count value.

Okay that's good. By the way, COUNTER_COMP_ENABLE ensures the enable
value passed to ecap_cnt_enable_write() is either 0 or 1, so you don't
need the enable > 1 check in your callback.

> > > +static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
> > > +{
> > > +	struct counter_device *counter_dev = dev_id;
> > > +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> > > +	unsigned int clr = 0;
> > > +	unsigned int flg;
> > > +	int i;
> > > +	unsigned long flags;
> > > +
> > > +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
> > > +
> > > +	for (i = ECAP_NB_CAP - 1 ; i < ECAP_NB_CEVT ; i++) {
> > Would you walk me through the logic for this loop. Is this for-loop
> > intended to loop through all four capture indices? ECAP_NB_CAP and
> > ECAP_NB_CEVT are defines, so your for-loop has i=3 and i<5; is this what
> > you want?
> 
> In previous versions (IIO subsys), this for-loop was intended to loop
> through all 4 capture indices
> and overflow flag.
> In this version, it has been modified to loop only for the last capture
> indice (the 4th)
> and overflow flag : yes, this is intentional. Only 1 event has to be pushed
> so that the user
> gets all 4 captured timestamps in a single-reading (using 4 watches).
> But if I understand well your previous suggestion, you would like tracking
> Mod4 counter value,
> don't you ? So, I will change back this for-loop, so that it loops for all
> capture indices (and
> overflow flag) -> For all 4 capture indices, Mod4 count will be updated. And
> event will still be
> pushed only for the 4th capture indice.

Instead of limiting the event push to just the 4th capture, I'd push
COUNTER_EVENT_CAPTURE on every capture but delegate them to their own
channels::

    counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, i);

The captures operate as a circular buffer, so the user can determine the
current capture index based on the watch channel reported and perform a
modulo to read the buffers in right sequence. Alternatively, they can
watch just channel 3 if they want to process only four captures at a
time.

William Breathitt Gray

On Tue, Aug 16, 2022 at 10:11:35AM +0200, Julien Panis wrote:
> On 15/08/2022 13:20, William Breathitt Gray wrote:
> > On Sun, Aug 14, 2022 at 01:03:48PM -0400, William Breathitt Gray wrote:
> > > On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
> > > > +static int ecap_cnt_function_read(struct counter_device *counter,
> > > > +				  struct counter_count *count,
> > > > +				  enum counter_function *function)
> > > > +{
> > > > +	*function = COUNTER_FUNCTION_INCREASE;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int ecap_cnt_action_read(struct counter_device *counter,
> > > > +				struct counter_count *count,
> > > > +				struct counter_synapse *synapse,
> > > > +				enum counter_synapse_action *action)
> > > > +{
> > > > +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > > > +
> > > > +	return 0;
> > > > +}
> > > Right now you have a Signal defined for the ECAPSIG line, but there is
> > > at least one more relevant Signal to define: the clock updating ECAPCNT.
> > > The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
> > > clock Signal, but for the ECAPSIG Signal you will need to report a
> > > Synapse action based on the polarity of the next capture (i.e. whether
> > > high or low).
> > I need to make a correction here. IIUC, the ECAPSIG signal doesn't
> > affect the count value of ECAPCNT (ECAPSIG only triggers the captures),
> > so the Synapse action for ECAPSIG should always be
> > COUNTER_SYNAPSE_ACTION_NONE. You don't need to account for the capture
> > polarities because they're not relevant in this particular situation:
> > ECAPSIG doesn't trigger the ECAPCNT count function.
> > 
> > William Breathitt Gray
> 
> It appears to me that you spoke about TSCNT register content (32 bits). So,
> you were
> not talking about the Mod4 counter (2 bits).
> Do you confirm that ?

Yes, I meant the TSCNT register. Sorry about that!

The Counter subsystem is providing an abstraction for users, so although
the ECAP pin signal is serving physically on the device as a clock for
the Mod4 counter, it also serves the TSCNT counter in an abstract sense
as the trigger signal to capture the TSCNT value. The Counter subsystem
paradigm allows you to define these abstract relationships between
Counts and Signals as Synapses.

William Breathitt Gray