On Fri, Mar 17, 2023 at 12:47:39AM -0700, Alexander Komrakov wrote:
> Some applications like Android needs elapsed realtime timestamping
> to PPS pulse for its clock management. Add sysfs node for this.
>
> Signed-off-by: Alexander Komrakov <[email protected]>
> ---
> Changes in v4:
> - Remove "staging",wrap changelog text at 72 columns and remove sysfs_emit()
Why remove sysfs_emit()? That's the correct thing to use, please use
it.
> - clock_gettime removed, trailing whitespaces removed, <tab> added, spaces removed
> - Information Real-time assert event added to Documentation/driver-api/pps.rst
>
> Documentation/ABI/testing/sysfs-pps | 27 +++++++++++++++++++++++
> Documentation/driver-api/pps.rst | 8 +++++++
> drivers/pps/Makefile | 4 ++++
> drivers/pps/kapi.c | 23 ++++++++++++++++---
> drivers/pps/sysfs.c | 34 +++++++++++++++++++++++++++++
> include/linux/pps_kernel.h | 2 ++
> 6 files changed, 95 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-pps b/Documentation/ABI/testing/sysfs-pps
> index 25028c7bc37d..031ec89e1ed6 100644
> --- a/Documentation/ABI/testing/sysfs-pps
> +++ b/Documentation/ABI/testing/sysfs-pps
> @@ -1,3 +1,30 @@
> +What: /sys/class/pps/pps0/assert_elapsed
> +Date: October 2021
> +Contact: Alexander Komrakov <[email protected]>
> +Description:
> + The /sys/class/pps/ppsX/assert_elapsed file reports the
> + elapsed real-time assert events and the elapsed
> + real-time assert sequence number of the X-th source
> + in the form:
> +
> + <secs>.<nsec>#<sequence>
> +
> + If the source has no elapsed real-time assert events
> + the content of this file is empty.
> +
> +What: /sys/class/pps/ppsX/clear_elapsed
> +Date: October 2021
> +Contact: Alexander Komrakov <[email protected]>
> +Description:
> + The /sys/class/pps/ppsX/clear_elapsed file reports the elapsed
> + real-time clear events and the elapsed real-time clear
> + sequence number of the X-th source in the form:
> +
> + <secs>.<nsec>#<sequence>
> +
> + If the source has no elapsed real-time clear events the
> + content of this file is empty.
> +
> What: /sys/class/pps/
> Date: February 2008
> Contact: Rodolfo Giometti <[email protected]>
> diff --git a/Documentation/driver-api/pps.rst b/Documentation/driver-api/pps.rst
> index 2d6b99766ee8..80bd7bf048e8 100644
> --- a/Documentation/driver-api/pps.rst
> +++ b/Documentation/driver-api/pps.rst
> @@ -167,6 +167,14 @@ sequence number. Other files are:
> * path: reports the PPS source's device path, that is the device the
> PPS source is connected to (if it exists).
>
> +Real-time assert event::
> +
> + Calculate the monotonic clock from the timespec clock to generate PPS elapsed real-time event value and store the result into /sys/class/pps/pps0/assert_elapsed.
> + Because we have requirements to make sure the delta between standard time, say the GPS Time, and elapsedRealtime < 1 millisecond,
> + regular linux clock timestamp is not enough for our use case.
> + The pin PPS will generate elapsedRealtime event at 1 sec boundary which is an exact value of the monotonic clock from the kernel PPS driver
> + /sys/class/pps/pps0/assert_elapsed.
Please wrap your lines properly :(
> +
>
> Testing the PPS support
> -----------------------
> diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
> index ceaf65cc1f1d..443501310445 100644
> --- a/drivers/pps/Makefile
> +++ b/drivers/pps/Makefile
> @@ -8,4 +8,8 @@ pps_core-$(CONFIG_NTP_PPS) += kc.o
> obj-$(CONFIG_PPS) := pps_core.o
> obj-y += clients/ generators/
>
> +ifeq ($(CONFIG_ELAPSED_REALTIME_PPS),y)
> +EXTRA_CFLAGS += -DENABLE_ELAPSED_REALTIME_PPS
> +endif
Why do you need a new CFLAG? Why not just look at the CONFIG entry
instead? And your .c code does not use this new CFLAG at all, so why is
it even here?
> +
> ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index d9d566f70ed1..8a97f4cae9e1 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -162,11 +162,20 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> unsigned long flags;
> int captured = 0;
> struct pps_ktime ts_real = { .sec = 0, .nsec = 0, .flags = 0 };
> + struct pps_ktime ts_real_elapsed = { .sec = 0, .nsec = 0, .flags = 0 };
> + struct timespec64 ts64 = { .tv_sec = 0, .tv_nsec = 0 };
>
> /* check event type */
> BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
> + /* Calculate the monotonic clock from the timespec clock and stores the result in pps_ktime format
> + ktime_get_boottime_ts64() : because elapsed realtime includes time spent in sleep */
Properly wrap your lines.
Also a blenk line after the BUG_ON()?
> + ktime_get_boottime_ts64(&ts64);
> + timespec_to_pps_ktime(&ts_real_elapsed,ts64);
>
> - dev_dbg(pps->dev, "PPS event at %lld.%09ld\n",
> + dev_dbg(pps->dev, "PPS event (monotonic) at %lld.%09d\n",
> + (s64)ts_real_elapsed.sec, ts_real_elapsed.nsec);
> +
> + dev_dbg(pps->dev, "PPS event (timestamp) at %lld.%09ld\n",
> (s64)ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
>
> timespec_to_pps_ktime(&ts_real, ts->ts_real);
> @@ -181,11 +190,15 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> pps->current_mode = pps->params.mode;
> if (event & pps->params.mode & PPS_CAPTUREASSERT) {
> /* We have to add an offset? */
> - if (pps->params.mode & PPS_OFFSETASSERT)
> + if (pps->params.mode & PPS_OFFSETASSERT) {
> + pps_add_offset(&ts_real_elapsed,
> + &pps->params.assert_off_tu);
> pps_add_offset(&ts_real,
> &pps->params.assert_off_tu);
> + }
>
> /* Save the time stamp */
> + pps->assert_elapsed_tu = ts_real_elapsed;
> pps->assert_tu = ts_real;
> pps->assert_sequence++;
> dev_dbg(pps->dev, "capture assert seq #%u\n",
> @@ -195,11 +208,15 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> }
> if (event & pps->params.mode & PPS_CAPTURECLEAR) {
> /* We have to add an offset? */
> - if (pps->params.mode & PPS_OFFSETCLEAR)
> + if (pps->params.mode & PPS_OFFSETCLEAR) {
> + pps_add_offset(&ts_real_elapsed,
> + &pps->params.clear_off_tu);
> pps_add_offset(&ts_real,
> &pps->params.clear_off_tu);
> + }
>
> /* Save the time stamp */
> + pps->clear_elapsed_tu = ts_real_elapsed;
> pps->clear_tu = ts_real;
> pps->clear_sequence++;
> dev_dbg(pps->dev, "capture clear seq #%u\n",
> diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
> index 134bc33f6ad0..24f505cd7233 100644
> --- a/drivers/pps/sysfs.c
> +++ b/drivers/pps/sysfs.c
> @@ -10,6 +10,7 @@
> #include <linux/module.h>
> #include <linux/string.h>
> #include <linux/pps_kernel.h>
> +#include <linux/sysfs.h>
Why is this .h needed now and not previously?
>
> /*
> * Attribute functions
> @@ -29,6 +30,21 @@ static ssize_t assert_show(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_RO(assert);
>
> +static ssize_t assert_elapsed_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct pps_device *pps = dev_get_drvdata(dev);
> +
> + if (!(pps->info.mode & PPS_CAPTUREASSERT))
> + return 0;
Why are you not returning an error?
> +
> + return sprintf(buf, "%lld.%09d#%d\n",
> + (long long) pps->assert_elapsed_tu.sec,
> + pps->assert_elapsed_tu.nsec,
> + pps->assert_sequence);
sysfs_emit() please.
> +}
> +static DEVICE_ATTR_RO(assert_elapsed);
> +
> static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> @@ -43,6 +59,22 @@ static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_RO(clear);
>
> +static ssize_t clear_elapsed_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pps_device *pps = dev_get_drvdata(dev);
> +
> + if (!(pps->info.mode & PPS_CAPTURECLEAR))
> + return 0;
Again, why not an error?
And why are these sysfs files even present if the mode is not set
properly? Can the mode be set while the device is attached or is this
only defined at probe time? If at probe time, just never create these
files.
> +
> + return sprintf(buf, "%lld.%09d#%d\n",
> + (long long) pps->clear_elapsed_tu.sec,
> + pps->clear_elapsed_tu.nsec,
> + pps->clear_sequence);
Again, sysfs_emit() please.
thanks,
greg k-h
On Fri, Mar 17, 2023 at 02:51:50AM -0700, Alex Komrakov wrote:
<snip>
Please fix up your email client as it is sending html email which is
rejected by the mailing list and can not properly quote emails to engage
in proper discussion of changes. The kernel documentation has some
hints for how to do this.
thanks,
greg k-h
On 17/03/23 10:51, Alex Komrakov wrote:
>> + if (!(pps->info.mode & PPS_CAPTURECLEAR))
>> + return 0; Why are you not returning an error?
> [AK] I used the style in this file sysfs.c.
> assert_show() and clear_show() have the same condition.
> When '& PPS_CAPTURECLEAR' -- 0 means no interrupt asserted and it is not error
> Probably Rodolfo can get more info why return 0
It's just as Alex said, if the PPS source has no PPS_CAPTUREASSERT or
PPS_CAPTURECLEAR mode it should not print ASSERT and CLEAR info.
> And why are these sysfs files even present if the mode is not set
> properly? Can the mode be set while the device is attached or is this
> only defined at probe time? If at probe time, just never create these
> files.
> [AK] we can understand mode is set when interrupts asserted and
> file assert_elapsed will be updated.
PPS source's "mode bits" can be set at runtime via PPS_SETPARAMS.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
On Fri, Mar 17, 2023 at 03:04:31PM +0100, Rodolfo Giometti wrote:
> On 17/03/23 10:51, Alex Komrakov wrote:
> > > +? ? ?if (!(pps->info.mode & PPS_CAPTURECLEAR))
> > > +? ? ? ? ? ? ?return 0; ? Why are you not returning an error?
> > [AK] I used the style in this file sysfs.c.
> > ?assert_show() and clear_show()? have the same condition.
> > When '& PPS_CAPTURECLEAR' -- 0 means no interrupt asserted? and it is not error
> > Probably Rodolfo can get more info why return 0
>
> It's just as Alex said, if the PPS source has no PPS_CAPTUREASSERT or
> PPS_CAPTURECLEAR mode it should not print ASSERT and CLEAR info.
But shouldn't you return an error instead of an empty string?
> > And why are these sysfs files even present if the mode is not set
> > properly?? Can the mode be set while the device is attached or is this
> > only defined at probe time?? If at probe time, just never create these
> > files.
> > [AK] we can understand mode is set when interrupts?asserted?and
> > file?assert_elapsed will be updated.
>
> PPS source's "mode bits" can be set at runtime via PPS_SETPARAMS.
Ok, that's good to know. But I think the error return value is a better
indication that something went wrong here and this attribute does not
work for this device at this point in time.
thanks,
greg k-h
On 17/03/23 15:22, Greg KH wrote:
> On Fri, Mar 17, 2023 at 03:04:31PM +0100, Rodolfo Giometti wrote:
>> On 17/03/23 10:51, Alex Komrakov wrote:
>>>> + if (!(pps->info.mode & PPS_CAPTURECLEAR))
>>>> + return 0; Why are you not returning an error?
>>> [AK] I used the style in this file sysfs.c.
>>> assert_show() and clear_show() have the same condition.
>>> When '& PPS_CAPTURECLEAR' -- 0 means no interrupt asserted and it is not error
>>> Probably Rodolfo can get more info why return 0
>>
>> It's just as Alex said, if the PPS source has no PPS_CAPTUREASSERT or
>> PPS_CAPTURECLEAR mode it should not print ASSERT and CLEAR info.
>
> But shouldn't you return an error instead of an empty string?
This is not an error... it's just a disabled capability. :)
>>> And why are these sysfs files even present if the mode is not set
>>> properly? Can the mode be set while the device is attached or is this
>>> only defined at probe time? If at probe time, just never create these
>>> files.
>>> [AK] we can understand mode is set when interrupts asserted and
>>> file assert_elapsed will be updated.
>>
>> PPS source's "mode bits" can be set at runtime via PPS_SETPARAMS.
>
> Ok, that's good to know. But I think the error return value is a better
> indication that something went wrong here and this attribute does not
> work for this device at this point in time.
I see... however I suppose several code relays on this behavior.
If we decide to change it, which should be the better way to do it? Any
suggestions are appreciated.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
On Fri, Mar 17, 2023 at 05:34:28PM +0100, Rodolfo Giometti wrote:
> On 17/03/23 15:22, Greg KH wrote:
> > On Fri, Mar 17, 2023 at 03:04:31PM +0100, Rodolfo Giometti wrote:
> > > On 17/03/23 10:51, Alex Komrakov wrote:
> > > > > +? ? ?if (!(pps->info.mode & PPS_CAPTURECLEAR))
> > > > > +? ? ? ? ? ? ?return 0; ? Why are you not returning an error?
> > > > [AK] I used the style in this file sysfs.c.
> > > > ?assert_show() and clear_show()? have the same condition.
> > > > When '& PPS_CAPTURECLEAR' -- 0 means no interrupt asserted? and it is not error
> > > > Probably Rodolfo can get more info why return 0
> > >
> > > It's just as Alex said, if the PPS source has no PPS_CAPTUREASSERT or
> > > PPS_CAPTURECLEAR mode it should not print ASSERT and CLEAR info.
> >
> > But shouldn't you return an error instead of an empty string?
>
> This is not an error... it's just a disabled capability. :)
Then maybe return "0" or something like that?
> > > > And why are these sysfs files even present if the mode is not set
> > > > properly?? Can the mode be set while the device is attached or is this
> > > > only defined at probe time?? If at probe time, just never create these
> > > > files.
> > > > [AK] we can understand mode is set when interrupts?asserted?and
> > > > file?assert_elapsed will be updated.
> > >
> > > PPS source's "mode bits" can be set at runtime via PPS_SETPARAMS.
> >
> > Ok, that's good to know. But I think the error return value is a better
> > indication that something went wrong here and this attribute does not
> > work for this device at this point in time.
>
> I see... however I suppose several code relays on this behavior.
If that's the case, then you are right, you can't change it, and I'll
stop complaining here :)
What tools use these sysfs files? How did you test your changes?
thanks,
greg k-h
On Fri, Mar 17, 2023 at 01:32:16PM -0700, Alex Komrakov wrote:
> AOSP time tags reported location with elapsed realtime and pps provides
> best accuracy.
<snip>
Again, please do not send HTML and please do not top-post as the mailing
list rejects HTML email and top-posting makes it impossible to have a
technical discussion on your changes.
thanks,
greg k-h
On 17/03/23 18:06, Greg KH wrote:
> On Fri, Mar 17, 2023 at 05:34:28PM +0100, Rodolfo Giometti wrote:
>> On 17/03/23 15:22, Greg KH wrote:
>>> On Fri, Mar 17, 2023 at 03:04:31PM +0100, Rodolfo Giometti wrote:
>>>> On 17/03/23 10:51, Alex Komrakov wrote:
>>>>>> + if (!(pps->info.mode & PPS_CAPTURECLEAR))
>>>>>> + return 0; Why are you not returning an error?
>>>>> [AK] I used the style in this file sysfs.c.
>>>>> assert_show() and clear_show() have the same condition.
>>>>> When '& PPS_CAPTURECLEAR' -- 0 means no interrupt asserted and it is not error
>>>>> Probably Rodolfo can get more info why return 0
>>>>
>>>> It's just as Alex said, if the PPS source has no PPS_CAPTUREASSERT or
>>>> PPS_CAPTURECLEAR mode it should not print ASSERT and CLEAR info.
>>>
>>> But shouldn't you return an error instead of an empty string?
>>
>> This is not an error... it's just a disabled capability. :)
>
> Then maybe return "0" or something like that?
Yes, it could be a valid solution.
>>>>> And why are these sysfs files even present if the mode is not set
>>>>> properly? Can the mode be set while the device is attached or is this
>>>>> only defined at probe time? If at probe time, just never create these
>>>>> files.
>>>>> [AK] we can understand mode is set when interrupts asserted and
>>>>> file assert_elapsed will be updated.
>>>>
>>>> PPS source's "mode bits" can be set at runtime via PPS_SETPARAMS.
>>>
>>> Ok, that's good to know. But I think the error return value is a better
>>> indication that something went wrong here and this attribute does not
>>> work for this device at this point in time.
>>
>> I see... however I suppose several code relays on this behavior.
>
> If that's the case, then you are right, you can't change it, and I'll
> stop complaining here :)
>
> What tools use these sysfs files?
Mainly are debugging tools via scripts. Normal usage should be via C API.
> How did you test your changes?
As above, I use scripts whose get access to sysfs to test a PPS source
functionality. Regarding the C API I use the pps-tools:
https://github.com/redlab-i/pps-tools
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti