Message-ID: <1d4418a1-d91b-9558-7d7e-8d9940bf0470@enneenne.com>
Date:   Mon, 3 Jul 2023 11:32:05 +0200
From:   Rodolfo Giometti <giometti@...eenne.com>
To:     "Farber, Eliav" <farbere@...zon.com>, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     ronenk@...zon.com, talel@...zon.com, hhhawa@...zon.com,
        jonnyc@...zon.com, itamark@...zon.com, shellykz@...zon.com,
        amitlavi@...zon.com, almogbs@...zon.com
Subject: Re: [PATCH 1/5] pps: add pulse-width calculation in nsec

On 02/07/23 14:20, Farber, Eliav wrote:
> On 6/27/2023 5:27 PM, Rodolfo Giometti wrote:
>> On 25/06/23 16:21, Eliav Farber wrote:
>>> This change adds PPS pulse-width calculation in nano seconds.
>>> Width time can be calculated for both assert time and reset time.
>>>
>>> Calculation can be done only if capture ASSERT and capture CLEAR modes
>>> are both enabled.
>>>
>>> Assert width is calculated as:
>>>    clear-time - assert-time
>>> and clear width is calculated as:
>>>    assert-time - clear-time
>>>
>>> Read-only sysfs were added to get the last pulse-width time and event
>>> sequence.
>>> Examples:
>>>   * cat /sys/class/pps/pps0/pulse_width_assert
>>>     20001450#85
>>>   * cat /sys/class/pps/pps1/pulse_width_clear
>>>     979893314#16
>>>
>>> Signed-off-by: Eliav Farber <farbere@...zon.com>
>>> ---
>>>   drivers/pps/kapi.c         | 49 ++++++++++++++++++++++++++++++++++++++
>>>   drivers/pps/pps.c          |  9 +++++++
>>>   drivers/pps/sysfs.c        | 30 +++++++++++++++++++++++
>>>   include/linux/pps_kernel.h |  3 +++
>>>   include/uapi/linux/pps.h   | 19 +++++++++++++++
>>>   5 files changed, 110 insertions(+)
>>>
>>> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
>>> index d9d566f70ed1..deeecfc0a3ee 100644
>>> --- a/drivers/pps/kapi.c
>>> +++ b/drivers/pps/kapi.c
>>> @@ -82,6 +82,14 @@ struct pps_device *pps_register_source(struct 
>>> pps_source_info *info,
>>>               goto pps_register_source_exit;
>>>       }
>>>
>>> +     if ((info->mode & PPS_WIDTHBOTH) &&
>>> +         ((info->mode & PPS_CAPTUREBOTH) != PPS_CAPTUREBOTH)) {
>>> +             pr_err("%s: width can't be calculated without both captures 
>>> (mode = 0x%x)\n",
>>> +                    info->name, info->mode);
>>> +             err = -EINVAL;
>>> +             goto pps_register_source_exit;
>>> +     }
>>
>> See the comment below where you define PPS_WIDTHBOTH.
>>
>>>       /* Allocate memory for the new PPS source struct */
>>>       pps = kzalloc(sizeof(struct pps_device), GFP_KERNEL);
>>>       if (pps == NULL) {
>>> @@ -143,6 +151,39 @@ void pps_unregister_source(struct pps_device *pps)
>>>   }
>>>   EXPORT_SYMBOL(pps_unregister_source);
>>>
>>> +static u64 pps_ktime_sub(struct pps_ktime *ts1, struct pps_ktime *ts2)
>>> +{
>>> +     if (ts1->sec == ts2->sec)
>>> +             return (ts1->nsec > ts2->nsec) ? (ts1->nsec - ts2->nsec) : 
>>> (ts2->nsec - ts1->nsec);
>>> +
>>> +     if (ts1->sec > ts2->sec)
>>> +             return (ts1->sec - ts2->sec) * NSEC_PER_SEC + ts1->nsec - 
>>> ts2->nsec;
>>> +
>>> +     return (ts2->sec - ts1->sec) * NSEC_PER_SEC + ts2->nsec - ts1->nsec;
>>> +}
>>> +
>>> +static void pps_calc_clear_width(struct pps_device *pps)
>>> +{
>>> +     if (pps->clear_sequence == 0)
>>> +             return;
>>> +
>>> +     pps->clear_width.sequence++;
>>
>> I don't understand the meaning of this field... regarding assert and clear it
>> states the n-th sample but in this case...? Why do you need it?
> 
> For assert and clear, the sequence parameter is basically the counter
> of assert/clear events.
> Similarly, I wanted to have a counter for the number of pulses which
> there width was counted.
> The sequence was used by me in the sysfs to show the pulse counter and
> pulse width in nano-seconds.
> Will counter make more sense instead of sequence?
> Initially, I used the assert_sequence and clear_sequence as the pulse
> counter, but there were few cases to handle.
> In case first interrupt happened during a pulse, then
> assert_sequence != clear_sequence, but if not then
> assert_sequence == clear_sequence.
> So I preferred to add an new independent value.

OK.

>>> +     pps->clear_width.nsec = pps_ktime_sub(&pps->assert_tu, &pps->clear_tu);
>>> +     dev_dbg(pps->dev, "PPS clear width = %llu#%u\n",
>>> +             pps->clear_width.nsec, pps->clear_width.sequence);
>>> +}
>>> +
>>> +static void pps_calc_assert_width(struct pps_device *pps)
>>> +{
>>> +     if (pps->assert_sequence == 0)
>>> +             return;
>>> +
>>> +     pps->assert_width.sequence++;
>>
>> Ditto.
>>
>>> +     pps->assert_width.nsec = pps_ktime_sub(&pps->clear_tu, &pps->assert_tu);
>>> +     dev_dbg(pps->dev, "PPS assert width = %llu#%u\n",
>>> +             pps->assert_width.nsec, pps->assert_width.sequence);
>>> +}
>>> +
>>>   /* pps_event - register a PPS event into the system
>>>    * @pps: the PPS device
>>>    * @ts: the event timestamp
>>> @@ -191,6 +232,10 @@ void pps_event(struct pps_device *pps, struct 
>>> pps_event_time *ts, int event,
>>>               dev_dbg(pps->dev, "capture assert seq #%u\n",
>>>                       pps->assert_sequence);
>>>
>>> +             /* Calculate clear pulse-width */
>>> +             if (pps->params.mode & PPS_WIDTHCLEAR)
>>> +                     pps_calc_clear_width(pps);
>>> +
>>>               captured = ~0;
>>>       }
>>>       if (event & pps->params.mode & PPS_CAPTURECLEAR) {
>>> @@ -205,6 +250,10 @@ void pps_event(struct pps_device *pps, struct 
>>> pps_event_time *ts, int event,
>>>               dev_dbg(pps->dev, "capture clear seq #%u\n",
>>>                       pps->clear_sequence);
>>>
>>> +             /* Calculate assert pulse-width */
>>> +             if (pps->params.mode & PPS_WIDTHASSERT)
>>> +                     pps_calc_assert_width(pps);
>>> +
>>>               captured = ~0;
>>>       }
>>>
>>> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
>>> index 5d19baae6a38..8299a272af11 100644
>>> --- a/drivers/pps/pps.c
>>> +++ b/drivers/pps/pps.c
>>> @@ -195,6 +195,11 @@ static long pps_cdev_ioctl(struct file *file,
>>>               fdata.info.clear_tu = pps->clear_tu;
>>>               fdata.info.current_mode = pps->current_mode;
>>>
>>> +             memcpy(&fdata.info.assert_width, &pps->assert_width,
>>> +                    sizeof(struct pps_kwidth));
>>> +             memcpy(&fdata.info.clear_width, &pps->clear_width,
>>> +                    sizeof(struct pps_kwidth));
>>> +
>>>               spin_unlock_irq(&pps->lock);
>>>
>>>               err = copy_to_user(uarg, &fdata, sizeof(struct pps_fdata));
>>> @@ -283,6 +288,10 @@ static long pps_cdev_compat_ioctl(struct file *file,
>>>                               sizeof(struct pps_ktime_compat));
>>>               memcpy(&compat.info.clear_tu, &pps->clear_tu,
>>>                               sizeof(struct pps_ktime_compat));
>>> +             memcpy(&compat.info.assert_width, &pps->assert_width,
>>> +                    sizeof(struct pps_kwidth_compat));
>>> +             memcpy(&compat.info.clear_width, &pps->clear_width,
>>> +                    sizeof(struct pps_kwidth_compat));
>>>
>>>               spin_unlock_irq(&pps->lock);
>>>
>>> diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
>>> index 134bc33f6ad0..3e34de77dba6 100644
>>> --- a/drivers/pps/sysfs.c
>>> +++ b/drivers/pps/sysfs.c
>>> @@ -79,6 +79,34 @@ static ssize_t path_show(struct device *dev, struct 
>>> device_attribute *attr,
>>>   }
>>>   static DEVICE_ATTR_RO(path);
>>>
>>> +static ssize_t pulse_width_assert_show(struct device *dev,
>>> +                                    struct device_attribute *attr,
>>> +                                    char *buf)
>>> +{
>>> +     struct pps_device *pps = dev_get_drvdata(dev);
>>> +
>>> +     if (!(pps->info.mode & PPS_WIDTHASSERT))
>>> +             return 0;
>>> +
>>> +     return sprintf(buf, "%llu#%u\n",
>>> +                    pps->assert_width.nsec, pps->assert_width.sequence);
>>> +}
>>> +static DEVICE_ATTR_RO(pulse_width_assert);
>>> +
>>> +static ssize_t pulse_width_clear_show(struct device *dev,
>>> +                                   struct device_attribute *attr,
>>> +                                   char *buf)
>>> +{
>>> +     struct pps_device *pps = dev_get_drvdata(dev);
>>> +
>>> +     if (!(pps->info.mode & PPS_WIDTHCLEAR))
>>> +             return 0;
>>> +
>>> +     return sprintf(buf, "%llu#%u\n",
>>> +                    pps->clear_width.nsec, pps->clear_width.sequence);
>>> +}
>>> +static DEVICE_ATTR_RO(pulse_width_clear);
>>> +
>>>   static struct attribute *pps_attrs[] = {
>>>       &dev_attr_assert.attr,
>>>       &dev_attr_clear.attr,
>>> @@ -86,6 +114,8 @@ static struct attribute *pps_attrs[] = {
>>>       &dev_attr_echo.attr,
>>>       &dev_attr_name.attr,
>>>       &dev_attr_path.attr,
>>> +     &dev_attr_pulse_width_assert.attr,
>>> +     &dev_attr_pulse_width_clear.attr,
>>>       NULL,
>>>   };
>>>
>>> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
>>> index 78c8ac4951b5..15f2338095c6 100644
>>> --- a/include/linux/pps_kernel.h
>>> +++ b/include/linux/pps_kernel.h
>>> @@ -51,6 +51,9 @@ struct pps_device {
>>>       struct pps_ktime clear_tu;
>>>       int current_mode;                       /* PPS mode at event time */
>>>
>>> +     struct pps_kwidth assert_width;         /* PPS assert pulse-width time 
>>> and event seq # */
>>> +     struct pps_kwidth clear_width;          /* PPS clear pulse-width time 
>>> and event seq # */
>>> +
>>>       unsigned int last_ev;                   /* last PPS event id */
>>>       wait_queue_head_t queue;                /* PPS event queue */
>>>
>>> diff --git a/include/uapi/linux/pps.h b/include/uapi/linux/pps.h
>>> index 009ebcd8ced5..dd93dac0afc1 100644
>>> --- a/include/uapi/linux/pps.h
>>> +++ b/include/uapi/linux/pps.h
>>> @@ -64,12 +64,24 @@ struct pps_ktime_compat {
>>>   } __attribute__((packed, aligned(4)));
>>>   #define PPS_TIME_INVALID    (1<<0)  /* used to specify timeout==NULL */
>>>
>>> +struct pps_kwidth {
>>> +     __u64 nsec;
>>> +     __u32 sequence;
>>> +};
>>> +
>>> +struct pps_kwidth_compat {
>>> +     __u64 nsec;
>>> +     __u32 sequence;
>>> +} __attribute__((packed, aligned(4)));
>>
>> Why do you need a new type? Since both assert_width and clear_width are time
>> quantities as far as assert_tu and clear_tu, they can be of the same type, can't
>> they? Or, at least they can simply be __u64 since having an assert_width or
>> clear_width longer than 1 second is a non-sense...
> 
> For each pulse I wanted to save width in nsec (without sec) and
> counter.
> I need it twice for both assert and clear, hence I added a new
> structure for it.

I see, but I prefere you do as in struct pps_kinfo where times are times and 
sequence numbers are numbers, and not mixing them.

>>>   struct pps_kinfo {
>>>       __u32 assert_sequence;          /* seq. num. of assert event */
>>>       __u32 clear_sequence;           /* seq. num. of clear event */
>>>       struct pps_ktime assert_tu;     /* time of assert event */
>>>       struct pps_ktime clear_tu;      /* time of clear event */
>>>       int current_mode;               /* current mode bits */
>>> +     struct pps_kwidth assert_width; /* assert pulse-width time and seq. 
>>> num. */
>>> +     struct pps_kwidth clear_width;  /* clear pulse-width time and seq. num. */
>>>   };
>>
>> Altering this structure may break userspace code... also rfc2783 at section-3.2
>> states that:
>>
>>    The API defines these new data structures:
>>
>>       typedef struct {
>>           pps_seq_t   assert_sequence;        /* assert event seq # */
>>           pps_seq_t   clear_sequence;         /* clear event seq # */
>>           pps_timeu_t assert_tu;
>>           pps_timeu_t clear_tu;
>>           int         current_mode;           /* current mode bits */
>>       } pps_info_t;
>>
>> So, I'm not willing to change this structure just to add this new data that I
>> don't even know where it's used...
>>
>> If you just read these information via sysfs, please drop these part.
> 
> ACK. I'll drop this part.
> 
>>>   struct pps_kinfo_compat {
>>> @@ -78,6 +90,8 @@ struct pps_kinfo_compat {
>>>       struct pps_ktime_compat assert_tu;      /* time of assert event */
>>>       struct pps_ktime_compat clear_tu;       /* time of clear event */
>>>       int current_mode;                       /* current mode bits */
>>> +     struct pps_kwidth_compat assert_width;  /* assert pulse-width time and 
>>> seq. num. */
>>> +     struct pps_kwidth_compat clear_width;   /* clear pulse-width time and 
>>> seq. num. */
>>>   };
>>>
>>>   struct pps_kparams {
>>> @@ -96,6 +110,11 @@ struct pps_kparams {
>>>   #define PPS_CAPTURECLEAR    0x02    /* capture clear events */
>>>   #define PPS_CAPTUREBOTH             0x03    /* capture assert and clear 
>>> events */
>>>
>>> +/* Pulse-width calculation */
>>> +#define PPS_WIDTHASSERT              0x04    /* calculate assert width */
>>> +#define PPS_WIDTHCLEAR               0x08    /* calculate clear width */
>>> +#define PPS_WIDTHBOTH                0x0c    /* calculate assert and clear 
>>> width */
>>> +
>>
>> I don't understand why a process should ask for just PPS_WIDTHASSERT or
>> PPS_WIDTHCLEAR... I think you can avoid defining these values and just enabling
>> pulse width calculation when both assert and clear events are available. 
> 
> ACK. I'll drop the new defines and enable width calculation when
> PPS_CAPTUREASSERT and PPS_CAPTURECLEAR are both defined.
> 
>>>   #define PPS_OFFSETASSERT    0x10    /* apply compensation for assert event */
>>>   #define PPS_OFFSETCLEAR             0x20    /* apply compensation for clear 
>>> event */
>>
>> However, the real point is: since an userpsace program can retrieve the time of
>> assert and clear events, why it cannot compute the pulses width by itself? :)
> 
> The userpsace program can retrieve the time of assert and clear events,
> but it is not always clear how to compute it.
> Initially that was how I did it:
> Read both times, make sure sequence of both times was identical, and
> then compute: clear_time – assert_time.
> But as I mentioned, when using wide pulses, it might be that when
> driver starts, it is a the middle of a pulse.
> In that case clear_time will be captured first (seq #1).
> Then assert_time is captured (seq #1).
> However, assert pulse width can only be calculated for the second
> clear-time sequence and first assert-time sequence.
> So to simplify this for the user, I added the calculation to the
> driver.
> Hope this was clear.
> Please let me know if this satisfies you, and then I’ll share a second
> version of patches which fixes all the other comments you gave.

Mmm... kernel drivers should implement mechanisms and not policies and since 
RFC2783 doesn't state this computation I think you are implementing a policy.

Let me suggest to add this piece of code to the pps-utils (maybe within the 
ppstest.c utility).

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@...eenne.com
Linux Device Driver                          giometti@...ux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Powered by blists - more mailing lists