Message-ID: <YwFZmN9H+Fjqm8sf@fedora>
Date:   Sat, 20 Aug 2022 18:00:56 -0400
From:   William Breathitt Gray <william.gray@...aro.org>
To:     Julien Panis <jpanis@...libre.com>
Cc:     robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, mranostay@...com,
        fabien.lahoudere@...labora.com, gwendal@...omium.org,
        enric.balletbo@...labora.com, bleung@...omium.org,
        groeck@...omium.org, jic23@...nel.org, david@...hnology.com,
        robertcnelson@...il.com
Subject: Re: [PATCH v5 0/3] ECAP support on TI AM62x SoC

On Wed, Aug 17, 2022 at 04:16:17PM +0200, Julien Panis wrote:
> The Enhanced Capture (ECAP) module can be used to timestamp events
> detected on signal input pin. It can be used for time measurements
> of pulse train signals.
> 
> ECAP module includes 4 timestamp capture registers. For all 4 sequenced
> timestamp capture events (1->2->3->4->1->...), edge polarity (falling/rising
> edge) can be selected.
> 
> This driver leverages counter subsystem to :
> - select edge polarity for all 4 capture events (event mode)
> - log timestamps for each capture event
> Event polarity, and CAP1/2/3/4 timestamps give all the information
> about the input pulse train. Further information can easily be computed :
> period and/or duty cycle if frequency is constant, elapsed time between
> pulses, etc...
> 
> Modifications since v4:
> 	- Modify yaml commit message prefix (dt-bindings)
> 	- Modify driver file name & Makefile (ti-ecap-capture)
> 	- Modify compilation flag name in Kconfig (TI_ECAP_CAPTURE)
> 	- Select REGMAP_MMIO in Kconfig
> 	- Add capture items to sysfs-bus-counter ABI documentation
> 	- Cleanup probe function (dev_err_probe & devm_clk_get_enabled & devm_add_action_or_reset for PM)
> 	- Enable/Disable device clock in suspend/resume functions
> 	- Add PM explanations
> 	- Add ECAP clock signal & 'frequency' sysfs entry
> 	- Replace elapsed_time & spinlock by nb_ovf (atomic_t) & 'count_cumul' sysfs entry
> 	- Add counter overflow event
> 	- Modify 'name' sysfs entry for signal0 & signal1 & count0
> 	- Modify watch_validate function
> 	- Add macros for callbacks related to cap1/2/3/4
> 
> Userspace commands :
> 	### SIGNAL INPUT ###
> 	cd /sys/bus/counter/devices/counter0/signal0
> 
> 	# Get available polarities for each capture event
> 	cat polarity1_available
> 	cat polarity2_available
> 	cat polarity3_available
> 	cat polarity4_available
> 
> 	# Get polarity for each capture event
> 	cat polarity1
> 	cat polarity2
> 	cat polarity3
> 	cat polarity4
> 
> 	# Set polarity for each capture event
> 	echo rising edge > polarity1
> 	echo falling edge > polarity2
> 	echo rising edge > polarity3
> 	echo falling edge > polarity4
> 
> 	### SIGNAL CLOCK ###
> 	cd /sys/bus/counter/devices/counter0/signal1
> 
> 	# Get clock rate
> 	cat frequency
> 
> 	### COUNT ###
> 	cd /sys/bus/counter/devices/counter0/count0
> 
> 	# Run ECAP
> 	echo 1 > enable
> 
> 	# Get current timebase counter value
> 	cat count
> 
> 	# Get cumulated counter value
> 	cat count_cumul
> 
> 	# Get captured timestamps
> 	cat capture1
> 	cat capture2
> 	cat capture3
> 	cat capture4
> 
> 	# Note that counter watches can also be used to get
> 	# data from userspace application
> 	# -> see tools/counter/counter_example.c
> 
> 	# Stop ECAP
> 	echo 0 > enable
> 
> Julien Panis (3):
>   dt-bindings: counter: add ti,am62-ecap-capture.yaml
>   Documentation: ABI: sysfs-bus-counter: add capture items
>   counter: ti-ecap-capture: capture driver support for ECAP
> 
>  Documentation/ABI/testing/sysfs-bus-counter   |  49 ++
>  .../counter/ti,am62-ecap-capture.yaml         |  61 ++
>  drivers/counter/Kconfig                       |  15 +
>  drivers/counter/Makefile                      |   1 +
>  drivers/counter/ti-ecap-capture.c             | 624 ++++++++++++++++++
>  include/uapi/linux/counter.h                  |   2 +
>  6 files changed, 752 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
>  create mode 100644 drivers/counter/ti-ecap-capture.c
> 
> -- 
> 2.25.1

Hello Julien,

I'm CCing a number of other developers here who have indicated interest
in counter timestamp functionality in past, just in case they would like
to particpate in this discussion.

Adding buffers to the Counter subsystem will be a user-visible ABI
change, so I want to make sure we get the interface design correct
before we merge any of those changes; once it's exposed to userspace it
can't be changed. However, we can still improve your patches while we
develop this interface, so revisions are welcome even if I can't merge
your counter driver until we iron out the Counter subsystem buffer
interface.

So in the v4 patchset we discussed introducing a new counter_comp_type
COUNTER_COMP_BUFFER_U64 enum constant with respective counter_comp read
callbacks::

     int (*count_buffer_u64_read)(struct counter_device *counter,
                                  struct counter_count *count,
                                  size_t index, u64 *val); 

Drive authors can then handle buffer reads by receiving an "index",
locating the value at that buffer offset, and returning the value via
the "val" u64 pointer.

Defining a buffer as Count extensions could be done using a helper
macro::

     COUNTER_COMP_COUNT_BUFFER_U64("capture", ecap_cnt_cap_read, 4)

Originally I considered unrolling this into four COUNTER_COMP_COUNT_U64,
but I'm unsure if that is possible in GCC. Regardless, I believe it's
feasible to implement this in counter-chrdev.c by passing the buffer
length in the "priv" member of struct counter_comp and handling it when
creating sysfs attributes. I might be able to write a prototype for this
in the next couple weeks.

In the end, we should have four buffer elements exposed as sysfs
attributes under the respective count directory:

* /sys/bus/counter/devices/counterX/count0/capture0
* /sys/bus/counter/devices/counterX/count0/capture1
* /sys/bus/counter/devices/counterX/count0/capture2
* /sys/bus/counter/devices/counterX/count0/capture3 

One worry I do have is whether this will scale well enough; I can
imagine some future device having a timestamp history buffer of much
later than four elements. In cases with large buffers, it might be more
practical to expose a FIFO queue to deliver buffer data. However, the
existing Counter character device interface isn't designed for data of
arbitrary length, so we'd likely have to introduce a secondary character
device to provide the queue.

We can postpone implementation of such a queue until the need arises and
focus on just the sysfs interface for this particular driver. If we
expose each element of the buffer as its own sysfs attribute, then the
existing Counter character device interface gets access to these
elements for free without any additional code changes to that part of
the Counter subsystem.

So my concerns right now are making sure this is a sane design and the
right path forward to expose device buffer elements in sysfs.

William Breathitt Gray

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists