On Wed, Sep 21, 2022 at 12:06:26PM +0200, Julien Panis wrote:
> ECAP hardware on TI AM62x SoC supports capture feature. It can be used
> to timestamp events (falling/rising edges) detected on input signal.
>
> This commit adds capture driver support for ECAP hardware on AM62x SoC.
>
> In the ECAP hardware, capture pin can also be configured to be in
> PWM mode. Current implementation only supports capture operating mode.
> Hardware also supports timebase sync between multiple instances, but
> this driver supports simple independent capture functionality.
>
> Signed-off-by: Julien Panis <jpanis@xxxxxxxxxxxx>
Hi Julien,
This driver is almost there; some comments follow below.
> +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_CEVT ; i++) {
> + if (regval & ECAP_CAPPOL_BIT(i))
> + ev_mode |= ECAP_EV_MODE_BIT(i);
> + }
Looks like this for loop is just remapping the set bits in regval to
ev_mode. You can use bitmap_remap() here instead to simplify this
section of code.
> +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_CEVT ; i++) {
> + if (ev_mode & ECAP_EV_MODE_BIT(i))
> + regval |= ECAP_CAPPOL_BIT(i);
> + }
Use bitmap_remap() here as well (just in the reverse direction).
> +static int ecap_cnt_count_write(struct counter_device *counter,
> + struct counter_count *count, u64 val)
> +{
> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> + if (ecap_dev->enabled)
> + return -EBUSY;
> + if (val > 0)
> + return -EINVAL;
The ECAP_TSCNT_REG can be set to an arbitrary count value so there's no
need to restrict val here to 0. Instead, check that val is within the
ceiling value (<= U32_MAX) and return -ERANGE if it is not.
> +static int ecap_cnt_watch_validate(struct counter_device *counter,
> + const struct counter_watch *watch)
> +{
> + if ((watch->channel <= ECAP_CEVT_LAST && watch->event == COUNTER_EVENT_CAPTURE) ||
> + (watch->channel == ECAP_CNTOVF && watch->event == COUNTER_EVENT_OVERFLOW))
> + return 0;
> +
> + return -EINVAL;
COUNTER_EVENT_OVERFLOW shouldn't be on a separate channel; I'll explain
why later below in the interrupt handler review.
For this callback, you can separate the channel and event type checks to
their own blocks:
if (watch->channel > ECAP_CEVT_LAST)
return -EINVAL;
switch (watch->event) {
case COUNTER_EVENT_CAPTURE:
case COUNTER_EVENT_OVERFLOW:
return 0;
default:
return -EINVAL;
}
> +static int ecap_cnt_pol_read(struct counter_device *counter,
> + struct counter_signal *signal,
> + size_t idx, enum counter_signal_polarity *pol)
> +{
> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> + pm_runtime_get_sync(counter->parent);
> + *pol = regmap_test_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(idx)) ?
> + COUNTER_SIGNAL_POLARITY_NEGATIVE :
> + COUNTER_SIGNAL_POLARITY_POSITIVE;
This single line is doing a lot of things so I would rather see it
broken down. Save the regmap_test_bits() to a temporary local variable
first before evaluating to set *pol. This allows you to move the *pol
set operation to outside of the pm runtime syncs, possibly giving you a
marginal improvement in latency as well.
> +static inline int ecap_cnt_cap_read(struct counter_device *counter,
> + struct counter_count *count,
> + size_t idx, u64 *cap)
> +{
> + return ecap_cnt_count_get_val(counter, ECAP_CAP_REG(idx), cap);
> +}
I don't remember if we've discussed this before, but if these capture
registers can be set then it'll be useful to provide a corresponding
ecap_cnt_cap_write() function for them as well.
> +static int ecap_cnt_nb_ovf_write(struct counter_device *counter,
> + struct counter_count *count, u64 val)
> +{
> + struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> + if (ecap_dev->enabled)
> + return -EBUSY;
> + if (val > 0)
> + return -EINVAL;
Similar to the count_write() callback, check that val is <= U32_MAX and
return -ERANGE otherwise.
> +static DEFINE_COUNTER_ARRAY_U64(ecap_cnt_cap_array, ECAP_NB_CEVT);
> +
> +static struct counter_comp ecap_cnt_count_ext[] = {
> + COUNTER_COMP_COUNT_ARRAY_U64("capture", ecap_cnt_cap_read, NULL, ecap_cnt_cap_array),
Use the DEFINE_COUNTER_ARRAY_CAPTURE() and COUNTER_COMP_ARRAY_CAPTURE()
macros.
> +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;
> +
> + regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
> +
> + /* Check capture events */
> + for (i = 0 ; i < ECAP_NB_CEVT ; i++) {
> + if (flg & ECAP_EVT_FLG_BIT(i)) {
> + counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, i);
> + clr |= ECAP_EVT_CLR_BIT(i);
> + }
> + }
> +
> + /* Check counter overflow */
> + if (flg & ECAP_EVT_FLG_BIT(ECAP_CNTOVF)) {
> + atomic_inc(&ecap_dev->nb_ovf);
> + counter_push_event(counter_dev, COUNTER_EVENT_OVERFLOW, ECAP_CNTOVF);
COUNTER_EVENT_OVERFLOW doesn't conflict with COUNTER_EVENT_CAPTURE
(they're different event types) so they can both be pushed on the same
channel; in general, events of different type can share the same event
channels. In this case, you should push COUNTER_EVENT_OVERFLOW to all
four channels whenever you detect an overflow, so that users can receive
those events regardless of which channels they are watching:
counter_push_event(counter_dev, COUNTER_EVENT_OVERFLOW, 0);
counter_push_event(counter_dev, COUNTER_EVENT_OVERFLOW, 1);
counter_push_event(counter_dev, COUNTER_EVENT_OVERFLOW, 2);
counter_push_event(counter_dev, COUNTER_EVENT_OVERFLOW, 3);
There's no additional cost to pushing to these channels because events
get dropped by the Counter chrdev code if the user is not watching the
particular event on a channel.
> +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) {
> + /*
> + * Disabling capture has the following effects:
> + * - interrupts are disabled
> + * - loading of capture registers is disabled
> + * - timebase counter is stopped
> + */
> + ecap_cnt_capture_disable(counter_dev);
> + ecap_cnt_count_get_val(counter_dev, ECAP_TSCNT_REG, &ecap_dev->pm_ctx.time_cntr);
I see time_cntr define as u64, but if count value is always <= U32_MAX
you could redefine both ecap_cnt_count_get_val()'s val and time_cntr to
u32 instead.
> +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);
> +
> + clk_enable(ecap_dev->clk);
> +
> + 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) {
> + ecap_cnt_count_set_val(counter_dev, ECAP_TSCNT_REG, ecap_dev->pm_ctx.time_cntr);
Same as above would apply for ecap_cnt_count_set_val() too I think.
William Breathitt Gray
Attachment:
signature.asc
Description: PGP signature
