| Message ID | 20220810140724.182389-4-jpanis@baylibre.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ECAP support on TI AM62x SoC | expand |
On Wed, 10 Aug 2022 16:07:24 +0200 Julien Panis <jpanis@baylibre.com> 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, Been too long since I reviewed a counter driver, so I'll have to leave that side of things to William (or find a spare few days to remind myself of the details!) A few superficial things I notice below whilst taking a quick look. It might be worth adding a bit of description around what the result of the various runtime pm calls is (what's actually getting powered down?) Jonathan > +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); We now have dev_clk_get_enabled() in upstream - finally! Text book usecase for it here. > + 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); Unless there is a very good reason to mix devm and unmanaged code, it's best to not do so as it leads to much head scratching over whether there are race conditions. Here I can't see a reason not to use devm_add_action_or_reset() to make the pm_runtime_disabled() into devm managed. Any setting of enabled occured after probe() was done so that's fine as unmanaged or you could register another devm_ callback if you prefer, but with a comment explaining the path to it needing to do anything. > + } > + > + 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; > +} > +
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, ®val); > + 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, ®val); > + *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 14/08/2022 19:03, William Breathitt Gray wrote: > 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. Hi William, Thank you for your review. My answers to your questions 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. Another driver implementing ECAP PWM functionality was named "pwm-tiecap". That's why I chose this "capture-tiecap" name. But that's OK for ti-ecap-capture, it's better indeed to use the same naming convention for all counter drivers. I will also add "select REGMAP_MMIO" to Kconfig, similarly to others drivers (ti-eqep for instance). > >> 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, ®val); >> + 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, ®val); >> + *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). 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) > >> + >> +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. I tried to use a macro but did not find any clean way to do that. I will try again, maybe some Zephyr code could be inspiring for that kind of static definition using macros. > >> +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? Yes, it is what's happening here : no more increasing count value. > >> +}; >> + >> +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? 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. > >> + 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. I agree with this suggestion. > > 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 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
On 16/08/2022 17:12, William Breathitt Gray wrote: > 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. There is no dedicated register for that, but it would be possible to expose this value (when interruptions are triggered, we would just need to parse flags 1/2/3/4 to determine mod4 counter current state). That would not be very useful, though. > >>>> +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); I prefer using only 1 signal if you don't mind. I think it's less confusing for the user. > > 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
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. + 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, ®val); + 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, ®val); + *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); +} + +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; +} + +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; + + 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; + + 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); +} + +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); +} + +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), + }, +}; + +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], + }, +}; + +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), +}; + +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++) { + 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); + } + + 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, }; /**
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