| Message ID | 20220810140724.182389-1-jpanis@baylibre.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ECAP support on TI AM62x SoC | expand |
On Wed, Aug 10, 2022 at 04:07:21PM +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 v3: > - Migrate driver from IIO to Counter subsystem > - Minor modification in yaml ($id) to match Counter subsystem > - Add ABI documentation > > Userspace commands : > ### SIGNAL ### > 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 > polarity1 > echo falling > polarity2 > echo rising > polarity3 > echo falling > polarity4 > > ### COUNT ### > cd /sys/bus/counter/devices/counter0/count0 > > # Run ECAP > echo 1 > enable > > # Get current timebase counter value > cat count > > # 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-binding: counter: add ti,am62-ecap-capture.yaml > Documentation: ABI: add sysfs-bus-counter-ecap > counter: capture-tiecap: capture driver support for ECAP > > .../ABI/testing/sysfs-bus-counter-ecap | 64 ++ > .../counter/ti,am62-ecap-capture.yaml | 61 ++ > drivers/counter/Kconfig | 14 + > drivers/counter/Makefile | 1 + > drivers/counter/capture-tiecap.c | 634 ++++++++++++++++++ > include/uapi/linux/counter.h | 2 + > 6 files changed, 776 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ecap > create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml > create mode 100644 drivers/counter/capture-tiecap.c > > -- > 2.25.1 Something that has become apparent to me is the code repetition in this driver in order to support the capture buffer. Buffers are common components in devices, so it'll be good for us to standardize some of what we're exploring here into an interface that other drivers can also use. We have two ABIs to consider: the driver interface and the sysfs interface. For the sysfs interface, I think we'll have to expose each element individually (e.g. capture1, capture2, etc.) because sysfs attributes are suppose to expose only a single datum for any given attribute. For the driver side, we might want to introduce a new Counter component type for buffers and respective macros to streamline some of the code for driver authors. For example, a new COUNTER_COMP_BUFFER_U64 enum counter_comp_type constant could be introduced to represent a u64 buffer element; respective struct counter_comp read callbacks could be introduced:: int (*count_buffer_u64_read)(struct counter_device *counter, struct counter_count *count, size_t index, u64 *val); So a driver author can use the "index" parameter to locate the buffer element and pass back its value via the "val" parameter. To define the buffer, maybe helper macros like this could be introduced:: COUNTER_COMP_COUNT_BUFFER_U64("capture", ecap_cnt_cap_read, 4) This would define four u64 buffer elements each named prefixed with "capture" and with their read callbacks set to ecap_cnt_cap_read(). One problem however is that I'm not sure if the C preprocessor would be able to unroll the COUNTER_COMP_COUNT_BUFFER_U64 to a dynamic number of elements based on a macro parameter (maybe there is a GCC extension). I'm just throwing out ideas, so I'd like to hear some comments and suggestions from others about how we should add buffer support to the Counter subsystem. William Breathitt Gray
On 14/08/2022 20:00, William Breathitt Gray wrote: > On Wed, Aug 10, 2022 at 04:07:21PM +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 v3: >> - Migrate driver from IIO to Counter subsystem >> - Minor modification in yaml ($id) to match Counter subsystem >> - Add ABI documentation >> >> Userspace commands : >> ### SIGNAL ### >> 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 > polarity1 >> echo falling > polarity2 >> echo rising > polarity3 >> echo falling > polarity4 >> >> ### COUNT ### >> cd /sys/bus/counter/devices/counter0/count0 >> >> # Run ECAP >> echo 1 > enable >> >> # Get current timebase counter value >> cat count >> >> # 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-binding: counter: add ti,am62-ecap-capture.yaml >> Documentation: ABI: add sysfs-bus-counter-ecap >> counter: capture-tiecap: capture driver support for ECAP >> >> .../ABI/testing/sysfs-bus-counter-ecap | 64 ++ >> .../counter/ti,am62-ecap-capture.yaml | 61 ++ >> drivers/counter/Kconfig | 14 + >> drivers/counter/Makefile | 1 + >> drivers/counter/capture-tiecap.c | 634 ++++++++++++++++++ >> include/uapi/linux/counter.h | 2 + >> 6 files changed, 776 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ecap >> create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml >> create mode 100644 drivers/counter/capture-tiecap.c >> >> -- >> 2.25.1 > Something that has become apparent to me is the code repetition in this > driver in order to support the capture buffer. Buffers are common > components in devices, so it'll be good for us to standardize some of > what we're exploring here into an interface that other drivers can also > use. We have two ABIs to consider: the driver interface and the sysfs > interface. > > For the sysfs interface, I think we'll have to expose each element > individually (e.g. capture1, capture2, etc.) because sysfs attributes > are suppose to expose only a single datum for any given attribute. > > For the driver side, we might want to introduce a new Counter component > type for buffers and respective macros to streamline some of the code > for driver authors. For example, a new COUNTER_COMP_BUFFER_U64 enum > counter_comp_type constant could be introduced to represent a u64 buffer > element; respective struct counter_comp read callbacks could be > introduced:: > > int (*count_buffer_u64_read)(struct counter_device *counter, > struct counter_count *count, > size_t index, u64 *val); > > So a driver author can use the "index" parameter to locate the buffer > element and pass back its value via the "val" parameter.To define the > buffer, maybe helper macros like this could be introduced:: > > COUNTER_COMP_COUNT_BUFFER_U64("capture", ecap_cnt_cap_read, 4) > > This would define four u64 buffer elements each named prefixed with > "capture" and with their read callbacks set to ecap_cnt_cap_read(). Somehow, this "index" parameter would make things much easier for driver author. Besides, defining all these similar functions (cap1_read, ..., cap4_read) does not really make sense for ECAP driver. So, accessing this index parameter through callback and having the possibility to use the same function (cap_read) for several attributes would be great for my driver, and probably for future ones. > > One problem however is that I'm not sure if the C preprocessor would be > able to unroll the COUNTER_COMP_COUNT_BUFFER_U64 to a dynamic number of > elements based on a macro parameter (maybe there is a GCC extension). > > I'm just throwing out ideas, so I'd like to hear some comments and > suggestions from others about how we should add buffer support to the > Counter subsystem. > > William Breathitt Gray
On 14/08/2022 20:00, William Breathitt Gray wrote: > On Wed, Aug 10, 2022 at 04:07:21PM +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 v3: >> - Migrate driver from IIO to Counter subsystem >> - Minor modification in yaml ($id) to match Counter subsystem >> - Add ABI documentation >> >> Userspace commands : >> ### SIGNAL ### >> 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 > polarity1 >> echo falling > polarity2 >> echo rising > polarity3 >> echo falling > polarity4 >> >> ### COUNT ### >> cd /sys/bus/counter/devices/counter0/count0 >> >> # Run ECAP >> echo 1 > enable >> >> # Get current timebase counter value >> cat count >> >> # 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-binding: counter: add ti,am62-ecap-capture.yaml >> Documentation: ABI: add sysfs-bus-counter-ecap >> counter: capture-tiecap: capture driver support for ECAP >> >> .../ABI/testing/sysfs-bus-counter-ecap | 64 ++ >> .../counter/ti,am62-ecap-capture.yaml | 61 ++ >> drivers/counter/Kconfig | 14 + >> drivers/counter/Makefile | 1 + >> drivers/counter/capture-tiecap.c | 634 ++++++++++++++++++ >> include/uapi/linux/counter.h | 2 + >> 6 files changed, 776 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ecap >> create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml >> create mode 100644 drivers/counter/capture-tiecap.c >> >> -- >> 2.25.1 > Something that has become apparent to me is the code repetition in this > driver in order to support the capture buffer. Buffers are common > components in devices, so it'll be good for us to standardize some of > what we're exploring here into an interface that other drivers can also > use. We have two ABIs to consider: the driver interface and the sysfs > interface. > > For the sysfs interface, I think we'll have to expose each element > individually (e.g. capture1, capture2, etc.) because sysfs attributes > are suppose to expose only a single datum for any given attribute. > > For the driver side, we might want to introduce a new Counter component > type for buffers and respective macros to streamline some of the code > for driver authors. For example, a new COUNTER_COMP_BUFFER_U64 enum > counter_comp_type constant could be introduced to represent a u64 buffer > element; respective struct counter_comp read callbacks could be > introduced:: > > int (*count_buffer_u64_read)(struct counter_device *counter, > struct counter_count *count, > size_t index, u64 *val); > > So a driver author can use the "index" parameter to locate the buffer > element and pass back its value via the "val" parameter. To define the > buffer, maybe helper macros like this could be introduced:: > > COUNTER_COMP_COUNT_BUFFER_U64("capture", ecap_cnt_cap_read, 4) > > This would define four u64 buffer elements each named prefixed with > "capture" and with their read callbacks set to ecap_cnt_cap_read(). > > One problem however is that I'm not sure if the C preprocessor would be > able to unroll the COUNTER_COMP_COUNT_BUFFER_U64 to a dynamic number of > elements based on a macro parameter (maybe there is a GCC extension). > > I'm just throwing out ideas, so I'd like to hear some comments and > suggestions from others about how we should add buffer support to the > Counter subsystem. > > William Breathitt Gray Hi William, I am going to send a new version (PATCH v5) without buffers as described above (I added macros for cap1/2/3/4 similar functions). I tried taking into account the comments made by Jonathan and you. Regarding buffers, I don't know how driver upstream process is done when 'significant' subsystem modifications are being considered. Is it a problem for you if we continue driver upstream process until it's merged, and then modify counter subsystem (and ECAP driver with it) to add some buffer functionalities ? Or do you prefer adding such functionalities to counter subsystem while upstreaming ECAP driver, without any intermediate ECAP driver version ? Julien Panis