On Mon, Aug 08, 2022 at 10:58:22AM +0200, Julien Panis wrote:
> On 08/08/2022 02:30, William Breathitt Gray wrote:
> > Hi Julien,
> >
> > I've taken a cursory look over the TI ECAP reference guide and your
> > descriptions in this thread. I think a device driver for this would fit
> > better in the Counter subsystem than IIO.
> >
> > First I want to correct a minor misunderstanding: the "timestamp"
> > member of struct counter_event is simply a way to identify Counter
> > events on the system as a way of grouping multiple Counter watches. In
> > other words, the "timestamp" member here represents when a Counter event
> > was detected by the system, not when an event was logged on the counter
> > device hardware. Instead, hardware timestamps such as the CAPx registers
> > would be provided by the "value" member of struct counter_event.
> >
> > Now, I have a few ideas for how we could expose the timestamps using a
> > Counter device driver, but first I want to make sure I understand
> > correctly what's happening in this device. If I understand correctly, we
> > have the following device components:
> >
> > * CTR: 32-bit counter timer
> > * Mod4: 2-bit counter
> > * CAP1-CAP4: four 32-bit registers, each indepedently store a timestamp
> > * ECAP: input signal providing event trigger edges
> >
> > Four edge polarities are configured corresponding to each CAPx register,
> > yet the input signal is still the same single ECAP pin. The event that
> > is fired is instead determined by the Mod4 counter: when Mod4 is 0 and
> > the edge of ECAP matches the polarity configured for CAP1 then an event
> > is triggered which saves the current CTR value to CAP1 and increments
> > Mod4 to 1, etc.
> >
> > Is my understanding of how this device behaves correct?
>
> Hi William. Thank you for your help.
> Yes, your understanding of how this device behaves is correct.
>
> >
> > If so, then one possible way to represent this device in the Counter
> > sysfs tree is something like this:
> >
> > * CTR: /sys/bus/counter/devices/counterX/count0/count
> > * Mod4: /sys/bus/counter/devices/counterX/count1/count
> > * CAP1: /sys/bus/counter/devices/counterX/count1/cap1
> > * CAP2: /sys/bus/counter/devices/counterX/count1/cap2
> > * CAP3: /sys/bus/counter/devices/counterX/count1/cap3
> > * CAP4: /sys/bus/counter/devices/counterX/count1/cap4
> > * ECAP: /sys/bus/counter/devices/counterX/signal0/signal
> > * polarity1: /sys/bus/counter/devices/counterX/signal0/cap1_polarity
> > * polarity2: /sys/bus/counter/devices/counterX/signal0/cap2_polarity
> > * polarity3: /sys/bus/counter/devices/counterX/signal0/cap3_polarity
> > * polarity4: /sys/bus/counter/devices/counterX/signal0/cap4_polarity
> >
> > This is just a tentative arrangement (you could also include "enable"
> > attributes as well), but it should give you an idea of how it could be
> > organized.
> >
> > In your driver, you could then use counter_push_event() whenever you get
> > an event triggered. In userspace, your application will add Counter
> > watches for the CAPx registers they want. When an event triggers,
> > userspace can then received all four CAP register values at the same
> > time via the respective /dev/counterX character device node.
> >
> > Would this design work for your needs?
>
> Yes, that would work for my needs.
> The "how" is not fully clear to me yet, since I never used counter
> subsystem. But the
> best way to understand better how it works is probably to start working with
> it. :-)
> So, next patch version will be based on counter subsystem.

The Counter subsystem is relatively nascent so the number of existing
Counter device drivers to study is unfortunately sparse. If you
encounter any trouble along the way as you work on this, please don't
hestitate to reach out and I'll be happy to answer any questions you may
have. That said, here are some more hints that can help guide you. :-)

Although we've been using CAPx to refer to these registers, in the
context of sysfs it'll be better to call the attributes "capture1",
"capture2", etc.; that will make their use as capture buffers more
obvious. Furthermore, despite my previous example, I think it's better
to have these exist underneath the CTR hierarchy rather than Mod4
because they are captures of the CTR value:

* CTR: /sys/bus/counter/devices/counterX/count0/count
* CAP1: /sys/bus/counter/devices/counterX/count0/capture1
* CAP2: /sys/bus/counter/devices/counterX/count0/capture2
* CAP3: /sys/bus/counter/devices/counterX/count0/capture3
* CAP4: /sys/bus/counter/devices/counterX/count0/capture4

In your device driver, you would define a struct counter_count to
represent CTR. In this struct counter_count there is an "ext" member
where you provide an array of struct count_comp. Each CAPx will have a
corresponding struct count_comp; it'll look something like this::

static struct counter_comp ctr_count_ext[] = {
COUNTER_COMP_COUNT_U64("capture1", cap1_read, NULL),
COUNTER_COMP_COUNT_U64("capture2", cap2_read, NULL),
COUNTER_COMP_COUNT_U64("capture3", cap3_read, NULL),
COUNTER_COMP_COUNT_U64("capture4", cap4_read, NULL),
};

As you already know, counter_push_event() lets you push Counter events
in your interrupt handler. I recommend introducing a new event type
under enum counter_event_type in the include/uapi/linux/counter.h header
file; something like COUNTER_EVENT_CAPTURE should be descriptive enough.

The "channel" member of struct counter_watch refers to Counter event
channels; The purpose here is to allow us to support concurrent events
of the same type (e.g. two COUNTER_EVENT_OVERFLOW but for different
Counts). If I understand the TI ECAP device correctly, we'll be getting
a COUNTER_EVENT_CAPTURE event whenever a CAPx register is updated with a
new capture. It's up to you if you want to group them under the same
channel or separate channels for each CAPx; you would just pass the
channel in counter_push_event() to indicate which COUNTER_EVENT_CAPTURE
event is being handled.

Finally, you can take a look at tools/counter/counter_example.c as an
example userspace application. The interesting bits for you are
COUNTER_ADD_WATCH_IOCTL/COUNTER_ENABLE_EVENTS_IOCTL ioctl calls and
reading the event data out of the queue. You will need to first define
an array of struct counter_watch indicating that you want to watch the
"capture" attributes of the CTR count; something like this (assuming
event channel 0)::

/* assuming capture attributes are under the count0 directory */
#define CAPTURE_WATCH(_id, _channel) \
{ \
.component.type = COUNTER_COMPONENT_EXTENSION, \
.component.scope = COUNTER_SCOPE_COUNT, \
.component.parent = 0, \
.component.id = _id, \
.event = COUNTER_EVENT_CAPTURE, \
.channel = _channel, \
}
/* get id from respective "captureX_component_id" attributes */
static struct counter_watch watches[4] = {
CAPTURE_WATCH(42, 0),
CAPTURE_WATCH(43, 0),
CAPTURE_WATCH(44, 0),
CAPTURE_WATCH(45, 0),
};

Later you add the watches, enable events, and finally read the event
data as it arrives::

struct counter_event event_data[4];

ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[0]);
ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[1]);
ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[2]);
ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[3]);
ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);

for (;;) {
read(fd, event_data, sizeof(event_data));
printf("cap1: %llu", event_data[0].value);
printf("cap2: %llu", event_data[1].value);
printf("cap3: %llu", event_data[2].value);
printf("cap4: %llu", event_data[3].value);
}

If you want to keep track of channel, you can take a look under the
event_data[i].watch.channel member.

William Breathitt Gray

Attachments:

(No filename) (8.00 kB)
signature.asc (235.00 B)
Download all attachments

2022-08-09 06:25:18

by Julien Panis

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

On 08/08/2022 18:19, William Breathitt Gray wrote:
> On Mon, Aug 08, 2022 at 10:58:22AM +0200, Julien Panis wrote:
>> On 08/08/2022 02:30, William Breathitt Gray wrote:
>>> Hi Julien,
>>>
>>> I've taken a cursory look over the TI ECAP reference guide and your
>>> descriptions in this thread. I think a device driver for this would fit
>>> better in the Counter subsystem than IIO.
>>>
>>> First I want to correct a minor misunderstanding: the "timestamp"
>>> member of struct counter_event is simply a way to identify Counter
>>> events on the system as a way of grouping multiple Counter watches. In
>>> other words, the "timestamp" member here represents when a Counter event
>>> was detected by the system, not when an event was logged on the counter
>>> device hardware. Instead, hardware timestamps such as the CAPx registers
>>> would be provided by the "value" member of struct counter_event.
>>>
>>> Now, I have a few ideas for how we could expose the timestamps using a
>>> Counter device driver, but first I want to make sure I understand
>>> correctly what's happening in this device. If I understand correctly, we
>>> have the following device components:
>>>
>>> * CTR: 32-bit counter timer
>>> * Mod4: 2-bit counter
>>> * CAP1-CAP4: four 32-bit registers, each indepedently store a timestamp
>>> * ECAP: input signal providing event trigger edges
>>>
>>> Four edge polarities are configured corresponding to each CAPx register,
>>> yet the input signal is still the same single ECAP pin. The event that
>>> is fired is instead determined by the Mod4 counter: when Mod4 is 0 and
>>> the edge of ECAP matches the polarity configured for CAP1 then an event
>>> is triggered which saves the current CTR value to CAP1 and increments
>>> Mod4 to 1, etc.
>>>
>>> Is my understanding of how this device behaves correct?
>> Hi William. Thank you for your help.
>> Yes, your understanding of how this device behaves is correct.
>>
>>> If so, then one possible way to represent this device in the Counter
>>> sysfs tree is something like this:
>>>
>>> * CTR: /sys/bus/counter/devices/counterX/count0/count
>>> * Mod4: /sys/bus/counter/devices/counterX/count1/count
>>> * CAP1: /sys/bus/counter/devices/counterX/count1/cap1
>>> * CAP2: /sys/bus/counter/devices/counterX/count1/cap2
>>> * CAP3: /sys/bus/counter/devices/counterX/count1/cap3
>>> * CAP4: /sys/bus/counter/devices/counterX/count1/cap4
>>> * ECAP: /sys/bus/counter/devices/counterX/signal0/signal
>>> * polarity1: /sys/bus/counter/devices/counterX/signal0/cap1_polarity
>>> * polarity2: /sys/bus/counter/devices/counterX/signal0/cap2_polarity
>>> * polarity3: /sys/bus/counter/devices/counterX/signal0/cap3_polarity
>>> * polarity4: /sys/bus/counter/devices/counterX/signal0/cap4_polarity
>>>
>>> This is just a tentative arrangement (you could also include "enable"
>>> attributes as well), but it should give you an idea of how it could be
>>> organized.
>>>
>>> In your driver, you could then use counter_push_event() whenever you get
>>> an event triggered. In userspace, your application will add Counter
>>> watches for the CAPx registers they want. When an event triggers,
>>> userspace can then received all four CAP register values at the same
>>> time via the respective /dev/counterX character device node.
>>>
>>> Would this design work for your needs?
>> Yes, that would work for my needs.
>> The "how" is not fully clear to me yet, since I never used counter
>> subsystem. But the
>> best way to understand better how it works is probably to start working with
>> it. :-)
>> So, next patch version will be based on counter subsystem.
> The Counter subsystem is relatively nascent so the number of existing
> Counter device drivers to study is unfortunately sparse. If you
> encounter any trouble along the way as you work on this, please don't
> hestitate to reach out and I'll be happy to answer any questions you may
> have. That said, here are some more hints that can help guide you. :-)
>
> Although we've been using CAPx to refer to these registers, in the
> context of sysfs it'll be better to call the attributes "capture1",
> "capture2", etc.; that will make their use as capture buffers more
> obvious. Furthermore, despite my previous example, I think it's better
> to have these exist underneath the CTR hierarchy rather than Mod4
> because they are captures of the CTR value:
>
> * CTR: /sys/bus/counter/devices/counterX/count0/count
> * CAP1: /sys/bus/counter/devices/counterX/count0/capture1
> * CAP2: /sys/bus/counter/devices/counterX/count0/capture2
> * CAP3: /sys/bus/counter/devices/counterX/count0/capture3
> * CAP4: /sys/bus/counter/devices/counterX/count0/capture4
>
> In your device driver, you would define a struct counter_count to
> represent CTR. In this struct counter_count there is an "ext" member
> where you provide an array of struct count_comp. Each CAPx will have a
> corresponding struct count_comp; it'll look something like this::
>
> static struct counter_comp ctr_count_ext[] = {
> COUNTER_COMP_COUNT_U64("capture1", cap1_read, NULL),
> COUNTER_COMP_COUNT_U64("capture2", cap2_read, NULL),
> COUNTER_COMP_COUNT_U64("capture3", cap3_read, NULL),
> COUNTER_COMP_COUNT_U64("capture4", cap4_read, NULL),
> };
>
> As you already know, counter_push_event() lets you push Counter events
> in your interrupt handler. I recommend introducing a new event type
> under enum counter_event_type in the include/uapi/linux/counter.h header
> file; something like COUNTER_EVENT_CAPTURE should be descriptive enough.
>
> The "channel" member of struct counter_watch refers to Counter event
> channels; The purpose here is to allow us to support concurrent events
> of the same type (e.g. two COUNTER_EVENT_OVERFLOW but for different
> Counts). If I understand the TI ECAP device correctly, we'll be getting
> a COUNTER_EVENT_CAPTURE event whenever a CAPx register is updated with a
> new capture. It's up to you if you want to group them under the same
> channel or separate channels for each CAPx; you would just pass the
> channel in counter_push_event() to indicate which COUNTER_EVENT_CAPTURE
> event is being handled.

2 options, indeed :
1) By grouping them under the same channel, the only thing I'm not a
great fan of is that
I will need to use separate functions to read capture registers
(cap1_read/cap2_read/
cap3_read/cap4_read), and also to set/get polarity.
2) By using separate channels, code will look more simple but it is
likely to be a little bit
confusing for the user.
I will probably use option 2.

> Finally, you can take a look at tools/counter/counter_example.c as an
> example userspace application. The interesting bits for you are
> COUNTER_ADD_WATCH_IOCTL/COUNTER_ENABLE_EVENTS_IOCTL ioctl calls and
> reading the event data out of the queue. You will need to first define
> an array of struct counter_watch indicating that you want to watch the
> "capture" attributes of the CTR count; something like this (assuming
> event channel 0)::
>
> /* assuming capture attributes are under the count0 directory */
> #define CAPTURE_WATCH(_id, _channel) \
> { \
> .component.type = COUNTER_COMPONENT_EXTENSION, \
> .component.scope = COUNTER_SCOPE_COUNT, \
> .component.parent = 0, \
> .component.id = _id, \
> .event = COUNTER_EVENT_CAPTURE, \
> .channel = _channel, \
> }
> /* get id from respective "captureX_component_id" attributes */
> static struct counter_watch watches[4] = {
> CAPTURE_WATCH(42, 0),
> CAPTURE_WATCH(43, 0),
> CAPTURE_WATCH(44, 0),
> CAPTURE_WATCH(45, 0),
> };
>
> Later you add the watches, enable events, and finally read the event
> data as it arrives::
>
> struct counter_event event_data[4];
>
> ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[0]);
> ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[1]);
> ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[2]);
> ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[3]);
> ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
>
> for (;;) {
> read(fd, event_data, sizeof(event_data));
> printf("cap1: %llu", event_data[0].value);
> printf("cap2: %llu", event_data[1].value);
> printf("cap3: %llu", event_data[2].value);
> printf("cap4: %llu", event_data[3].value);
> }
>
> If you want to keep track of channel, you can take a look under the
> event_data[i].watch.channel member.
>
> William Breathitt Gray

Thank you for all these explanations. After a few hours working with
counter subsystem
I began to understand how it works, but now it's fully clear.

2022-08-09 06:52:39

by Julien Panis

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

On 09/08/2022 08:19, Julien Panis wrote:
>
>
> On 08/08/2022 18:19, William Breathitt Gray wrote:
>> The Counter subsystem is relatively nascent so the number of existing
>> Counter device drivers to study is unfortunately sparse. If you
>> encounter any trouble along the way as you work on this, please don't
>> hestitate to reach out and I'll be happy to answer any questions you may
>> have. That said, here are some more hints that can help guide you. :-)
>>
>> Although we've been using CAPx to refer to these registers, in the
>> context of sysfs it'll be better to call the attributes "capture1",
>> "capture2", etc.; that will make their use as capture buffers more
>> obvious. Furthermore, despite my previous example, I think it's better
>> to have these exist underneath the CTR hierarchy rather than Mod4
>> because they are captures of the CTR value:
>>
>> * CTR: /sys/bus/counter/devices/counterX/count0/count
>> * CAP1: /sys/bus/counter/devices/counterX/count0/capture1
>> * CAP2: /sys/bus/counter/devices/counterX/count0/capture2
>> * CAP3: /sys/bus/counter/devices/counterX/count0/capture3
>> * CAP4: /sys/bus/counter/devices/counterX/count0/capture4
>>
>> In your device driver, you would define a struct counter_count to
>> represent CTR. In this struct counter_count there is an "ext" member
>> where you provide an array of struct count_comp. Each CAPx will have a
>> corresponding struct count_comp; it'll look something like this::
>>
>>          static struct counter_comp ctr_count_ext[] = {
>>                  COUNTER_COMP_COUNT_U64("capture1", cap1_read, NULL),
>>                  COUNTER_COMP_COUNT_U64("capture2", cap2_read, NULL),
>>                  COUNTER_COMP_COUNT_U64("capture3", cap3_read, NULL),
>>                  COUNTER_COMP_COUNT_U64("capture4", cap4_read, NULL),
>>          };
>>
>> As you already know, counter_push_event() lets you push Counter events
>> in your interrupt handler. I recommend introducing a new event type
>> under enum counter_event_type in the include/uapi/linux/counter.h header
>> file; something like COUNTER_EVENT_CAPTURE should be descriptive enough.
>>
>> The "channel" member of struct counter_watch refers to Counter event
>> channels; The purpose here is to allow us to support concurrent events
>> of the same type (e.g. two COUNTER_EVENT_OVERFLOW but for different
>> Counts). If I understand the TI ECAP device correctly, we'll be getting
>> a COUNTER_EVENT_CAPTURE event whenever a CAPx register is updated with a
>> new capture. It's up to you if you want to group them under the same
>> channel or separate channels for each CAPx; you would just pass the
>> channel in counter_push_event() to indicate which COUNTER_EVENT_CAPTURE
>> event is being handled.
>
> 2 options, indeed :
> 1) By grouping them under the same channel, the only thing I'm not a
> great fan of is that
> I will need to use separate functions to read capture registers
> (cap1_read/cap2_read/
> cap3_read/cap4_read), and also to set/get polarity.
> 2) By using separate channels, code will look more simple but it is
> likely to be a little bit
> confusing for the user.
> I will probably use option 2.

[ERRATUM] : Sorry, I meant that I will probably use option 1 (not 2), to
avoid confusion for the user.