On 01/08/2022 16:08, Julien Panis wrote:
On 31/07/2022 17:41, Jonathan Cameron wrote:
On Thu, 28 Jul 2022 19:51:24 +0200
Julien Panis <jpanis@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
Hi Julien,
So this isn't the first ecap driver we've had proposed, but the previous
one was a few years ago and never reached v2.
https://lore.kernel.org/all/20200818153614.6438-1-dts86@xxxxxxxxxxx/
Honestly I can't remember much about it, but maybe the discussion around
that will be worth a reread.
Hi Jonathan, thank you for your review.
I read the discussion about previous attempt, before submitting this
patch. There were
interesting comments indeed.
But in this previous attempt, only one-shot pulses were handled
(moreover, global IRQ flag
was not cleared, so I'm not sure that IRQ could be raised more than
once).
However, ECAP can be used to make time measurements for any type of
"square waveform".
That's why I tried to make this event mode configurable. Besides,
using a continuous mode allows
handling much more signal types (not only single pulses).
The use of ABI here is unusual. So I'd definitely like to see some
documentation
probably as a file in the main kernel documentation to explain what
the interface
is an how that relates to what is being captured.
OK, I will add some userspace documentation.
First thing to note here is the channel type of IIO_INDEX is now not
actually
used any more because we moved all the relevant drivers over to the
counter
subsystem (and we failed to mark it deprecated).
I evaluated this counter subsystem before starting development.
Counting events is not "a priori"
the goal when using ECAP.
Nevertheless, maybe "counter_push_event" function could do the job. If
I use counter API :
# Option 1 : CAP1/2/3/4 registers could be seen as 4 channels of the
same counter...
but there are not channels, there are just sequential timestamps
actually. So I'm afraid this leads
to misunderstanding for the user.
Moreover, the user will have to read several entries (counts 1/2/3/4)
to gather timestamps from
the same input signal, which is not very convenient.
# Option 2 : Either CAP 1/2/3/4 events could be gathered in a single
channel...but then it will not
be possible to configure their polarity (rising/falling edge)
individually (unless I did
not understand well counter framework documentation).
So, even with counter framework, it will lead to some diverted use of
the framwork, since ECAP
is a very specific hardware that do not fit 100% counter philosophy.
I admit that ECAP do not fit 100% IIO philosophy either.
Maybe misc API would be more relevant actually. Any opinion about it
will be welcome. :-)
[Answering my own mail]
I got a closer look at counter framework. It is not suitable at all for
ECAP. Initially, I thought that
"counter_push_event" function could be used, but the only timestamp
handled by this function
is a software timestamp. I strongly doubt that counter framework
maintainer would accept
some modification here to support hardware timestamp : a patch rejection
would be
legitimate, since a counter is dedicated to "event counting". Whereas
ECAP is dedicated to
"event timestamping".
Beside, ECAP has 4 timestamp registers but they are used to capture
timestamps for a
single input pin (only 1 channel). In ECAP context, 'index X" is used to
identify CAP X
(used to capture event X detected on a single pin, with X = 0/1/2/3/0...).
In counter framework, "index X" is used to identify channel X (among
several pins).
So, the word "index" has not the same meaning in counter framework than
in ECAP device.
Somehow, this ECAP index (0/1/2/3 for CAP1/2/3/4 registers) must be
logged with timestamp
because it is an important part of signal info for the user (raw
consecutive timestamps
are not enough).
So, here is my proposal for my next version :
(1) Replace IIO_INDEX by IIO_COUNT channel (already used in
"stm32-timer-trigger.c" driver)
# In ECAP documentation, the word "index" is not used. The word used to
speak about this
0->1->2->3->0 sequenced counter is "Mod4 counter".
(2) Configure event mode with 4 sysfs entries (to remove the mix of
buffers and events interfaces)
# User will see 4 files (1 file for each CAP timestamp) named
"falling_edge_active_0/1/2/3".
Writing 1 will select falling edge/ Writing 0 will select rising edge.
Would it be an acceptable alternative for you, Jonathan ? Would either
(1) and/or (2) be a "no-go" ?
Anyhow, I've reviewed below, but need docs to discuss this in depth.
In particular
the mix of buffers and events interfaces is unlikely to be an
acceptable path
forwards.
OK, I will consider alternatives.
Jonathan
