* [PATCH v3] pps: Add elapsed realtime timestamping
@ 2023-03-15 0:52 Alexander Komrakov
2023-03-15 5:02 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Alexander Komrakov @ 2023-03-15 0:52 UTC (permalink / raw)
To: linux-kernel; +Cc: giometti, gregkh, Alexander Komrakov
[-- Attachment #1: Type: text/plain, Size: 7818 bytes --]
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 <alexander.komrakov@broadcom.com>
---
Changes in v3:
- Remove "staging",wrap changelog text at 72 columns and remove sysfs_emit()
- .gittconfg updated, clock_gettime removed, trailing whitespaces remvoved
- COONFIG_ELAPSED_REALTIME_PPS added to enable elapsed assert/clear sysfs
Documentation/ABI/testing/sysfs-pps | 27 ++++++++++++++++++
drivers/pps/Makefile | 4 +++
drivers/pps/kapi.c | 24 ++++++++++++++--
drivers/pps/sysfs.c | 44 +++++++++++++++++++++++++++--
include/linux/pps_kernel.h | 2 ++
5 files changed, 96 insertions(+), 5 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 <alexander.komrakov@broadcom.com>
+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 <alexander.komrakov@broadcom.com>
+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 <giometti@linux.it>
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
+
ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index d9d566f70ed1..69b432873ce7 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -23,6 +23,7 @@
/*
* Local functions
*/
+ #define NANOSEC_PER_SEC 1000000000 /* 10^9 */
static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
{
@@ -162,11 +163,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 */
+ 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 +191,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 +209,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..9e100e287ba7 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>
/*
* Attribute functions
@@ -24,11 +25,28 @@ static ssize_t assert_show(struct device *dev, struct device_attribute *attr,
return 0;
return sprintf(buf, "%lld.%09d#%d\n",
- (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
- pps->assert_sequence);
+ (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
+ pps->assert_sequence);
}
static DEVICE_ATTR_RO(assert);
+#ifdef ENABLE_ELAPSED_REALTIME_PPS
+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;
+
+ return sprintf(buf, "%lld.%09d#%d\n",
+ (long long) pps->assert_elapsed_tu.sec,
+ pps->assert_elapsed_tu.nsec,
+ pps->assert_sequence);
+}
+static DEVICE_ATTR_RO(assert_elapsed);
+#endif
+
static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
@@ -43,6 +61,24 @@ static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR_RO(clear);
+#ifdef ENABLE_ELAPSED_REALTIME_PPS
+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;
+
+ return sprintf(buf, "%lld.%09d#%d\n",
+ (long long) pps->clear_elapsed_tu.sec,
+ pps->clear_elapsed_tu.nsec,
+ pps->clear_sequence);
+}
+static DEVICE_ATTR_RO(clear_elapsed);
+#endif
+
static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
@@ -82,6 +118,10 @@ static DEVICE_ATTR_RO(path);
static struct attribute *pps_attrs[] = {
&dev_attr_assert.attr,
&dev_attr_clear.attr,
+#ifdef ENABLE_ELAPSED_REALTIME_PPS
+ &dev_attr_assert_elapsed.attr,
+ &dev_attr_clear_elapsed.attr,
+#endif
&dev_attr_mode.attr,
&dev_attr_echo.attr,
&dev_attr_name.attr,
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 78c8ac4951b5..1fecaaf4c8b9 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -47,6 +47,8 @@ struct pps_device {
__u32 assert_sequence; /* PPS assert event seq # */
__u32 clear_sequence; /* PPS clear event seq # */
+ struct pps_ktime assert_elapsed_tu; /* PPS elapsed rt assert seq # */
+ struct pps_ktime clear_elapsed_tu; /* PPS elapsed rt clear event seq */
struct pps_ktime assert_tu;
struct pps_ktime clear_tu;
int current_mode; /* PPS mode at event time */
--
2.25.1
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4222 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] pps: Add elapsed realtime timestamping
2023-03-15 0:52 [PATCH v3] pps: Add elapsed realtime timestamping Alexander Komrakov
@ 2023-03-15 5:02 ` Greg KH
2023-03-15 7:44 ` Rodolfo Giometti
0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2023-03-15 5:02 UTC (permalink / raw)
To: Alexander Komrakov; +Cc: linux-kernel, giometti
On Tue, Mar 14, 2023 at 05:52:26PM -0700, Alexander Komrakov wrote:
> Some applications like Android needs elapsed realtime timestamping
> to PPS pulse for its clock management. Add sysfs node for this.
What exact applications will use this?
>
> Signed-off-by: Alexander Komrakov <alexander.komrakov@broadcom.com>
> ---
> Changes in v3:
> - Remove "staging",wrap changelog text at 72 columns and remove sysfs_emit()
Why are you not using sysfs_emit()? That is the correct call to use.
> - .gittconfg updated, clock_gettime removed, trailing whitespaces remvoved
What .gitconfig?
> - COONFIG_ELAPSED_REALTIME_PPS added to enable elapsed assert/clear sysfs
"OO"? And as I say below, just use the config option as-is, do not
create a new one without a Kconfig change.
>
> Documentation/ABI/testing/sysfs-pps | 27 ++++++++++++++++++
> drivers/pps/Makefile | 4 +++
> drivers/pps/kapi.c | 24 ++++++++++++++--
> drivers/pps/sysfs.c | 44 +++++++++++++++++++++++++++--
> include/linux/pps_kernel.h | 2 ++
> 5 files changed, 96 insertions(+), 5 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 <alexander.komrakov@broadcom.com>
> +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 is a "real-time assert event"?
> +
> +What: /sys/class/pps/ppsX/clear_elapsed
> +Date: October 2021
> +Contact: Alexander Komrakov <alexander.komrakov@broadcom.com>
> +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 <giometti@linux.it>
> 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
Why are you defining a new flag just for a config option? Why not just
check that config option in the code instead? That removes a different
flag that will be impossible to track down over time.
> +endif
> +
> ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index d9d566f70ed1..69b432873ce7 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -23,6 +23,7 @@
> /*
> * Local functions
> */
> + #define NANOSEC_PER_SEC 1000000000 /* 10^9 */
Why isn't this in units.h? (hint, it is already, use that one.)
>
> static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
> {
> @@ -162,11 +163,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);
You are willing to crash a system because of a tiny driver issue? That
is not good, please fix up in a later patch.
> + /* 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 */
> + 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 +191,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 +209,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..9e100e287ba7 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 now needed and it wasn't before?
>
> /*
> * Attribute functions
> @@ -24,11 +25,28 @@ static ssize_t assert_show(struct device *dev, struct device_attribute *attr,
> return 0;
>
> return sprintf(buf, "%lld.%09d#%d\n",
> - (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
> - pps->assert_sequence);
> + (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
> + pps->assert_sequence);
Why make this change?
> }
> static DEVICE_ATTR_RO(assert);
>
> +#ifdef ENABLE_ELAPSED_REALTIME_PPS
> +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;
> +
> + return sprintf(buf, "%lld.%09d#%d\n",
sysfs files should be using sysfs_emit()
> + (long long) pps->assert_elapsed_tu.sec,
> + pps->assert_elapsed_tu.nsec,
> + pps->assert_sequence);
> +}
> +static DEVICE_ATTR_RO(assert_elapsed);
> +#endif
> +
> static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> @@ -43,6 +61,24 @@ static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
> }
> static DEVICE_ATTR_RO(clear);
>
> +#ifdef ENABLE_ELAPSED_REALTIME_PPS
> +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;
> +
> + return sprintf(buf, "%lld.%09d#%d\n",
> + (long long) pps->clear_elapsed_tu.sec,
> + pps->clear_elapsed_tu.nsec,
> + pps->clear_sequence);
> +}
> +static DEVICE_ATTR_RO(clear_elapsed);
> +#endif
> +
> static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> @@ -82,6 +118,10 @@ static DEVICE_ATTR_RO(path);
> static struct attribute *pps_attrs[] = {
> &dev_attr_assert.attr,
> &dev_attr_clear.attr,
> +#ifdef ENABLE_ELAPSED_REALTIME_PPS
> + &dev_attr_assert_elapsed.attr,
> + &dev_attr_clear_elapsed.attr,
> +#endif
Don't use #ifdef in a .c file, these attributes should only show up if
you determine at runtime they should not be there.
But why woudn't they just always be there all the time anyway? Why do
you need a new config option?
> &dev_attr_mode.attr,
> &dev_attr_echo.attr,
> &dev_attr_name.attr,
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index 78c8ac4951b5..1fecaaf4c8b9 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -47,6 +47,8 @@ struct pps_device {
>
> __u32 assert_sequence; /* PPS assert event seq # */
> __u32 clear_sequence; /* PPS clear event seq # */
> + struct pps_ktime assert_elapsed_tu; /* PPS elapsed rt assert seq # */
> + struct pps_ktime clear_elapsed_tu; /* PPS elapsed rt clear event seq */
no tabs?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] pps: Add elapsed realtime timestamping
2023-03-15 5:02 ` Greg KH
@ 2023-03-15 7:44 ` Rodolfo Giometti
0 siblings, 0 replies; 3+ messages in thread
From: Rodolfo Giometti @ 2023-03-15 7:44 UTC (permalink / raw)
To: Alexander Komrakov, Greg KH; +Cc: linux-kernel
On 15/03/23 06:02, Greg KH wrote:
> On Tue, Mar 14, 2023 at 05:52:26PM -0700, Alexander Komrakov wrote:
>> Some applications like Android needs elapsed realtime timestamping
>> to PPS pulse for its clock management. Add sysfs node for this.
>
> What exact applications will use this?
You should also prove a little documentation about what a "realtime
timestamping" is and why we need it.
>> Signed-off-by: Alexander Komrakov <alexander.komrakov@broadcom.com>
>> ---
>> Changes in v3:
>> - Remove "staging",wrap changelog text at 72 columns and remove sysfs_emit()
>
> Why are you not using sysfs_emit()? That is the correct call to use.
>
>> - .gittconfg updated, clock_gettime removed, trailing whitespaces remvoved
>
> What .gitconfig?
>
>> - COONFIG_ELAPSED_REALTIME_PPS added to enable elapsed assert/clear sysfs
>
> "OO"? And as I say below, just use the config option as-is, do not
> create a new one without a Kconfig change.
I suppose we can just drop this CONFIG entry.
>> Documentation/ABI/testing/sysfs-pps | 27 ++++++++++++++++++
>> drivers/pps/Makefile | 4 +++
>> drivers/pps/kapi.c | 24 ++++++++++++++--
>> drivers/pps/sysfs.c | 44 +++++++++++++++++++++++++++--
>> include/linux/pps_kernel.h | 2 ++
>> 5 files changed, 96 insertions(+), 5 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 <alexander.komrakov@broadcom.com>
>> +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 is a "real-time assert event"?
Please add these information to Documentation/driver-api/pps.rst too (maybe add
a note a the end of the "SYSFS support" section.
>> +
>> +What: /sys/class/pps/ppsX/clear_elapsed
>> +Date: October 2021
>> +Contact: Alexander Komrakov <alexander.komrakov@broadcom.com>
>> +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 <giometti@linux.it>
>> 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
>
> Why are you defining a new flag just for a config option? Why not just
> check that config option in the code instead? That removes a different
> flag that will be impossible to track down over time.
>
>> +endif
>> +
>> ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
>> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
>> index d9d566f70ed1..69b432873ce7 100644
>> --- a/drivers/pps/kapi.c
>> +++ b/drivers/pps/kapi.c
>> @@ -23,6 +23,7 @@
>> /*
>> * Local functions
>> */
>> + #define NANOSEC_PER_SEC 1000000000 /* 10^9 */
>
> Why isn't this in units.h? (hint, it is already, use that one.)
>
>
>>
>> static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
>> {
>> @@ -162,11 +163,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);
>
> You are willing to crash a system because of a tiny driver issue? That
> is not good, please fix up in a later patch.
@Greg: I suppose you address this comment to me... :) Do you prefere a WARN_ON()
here?
>> + /* 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 */
>> + 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 +191,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 +209,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..9e100e287ba7 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 now needed and it wasn't before?
>
>>
>> /*
>> * Attribute functions
>> @@ -24,11 +25,28 @@ static ssize_t assert_show(struct device *dev, struct device_attribute *attr,
>> return 0;
>>
>> return sprintf(buf, "%lld.%09d#%d\n",
>> - (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
>> - pps->assert_sequence);
>> + (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
>> + pps->assert_sequence);
>
> Why make this change?
>
>
>
>> }
>> static DEVICE_ATTR_RO(assert);
>>
>> +#ifdef ENABLE_ELAPSED_REALTIME_PPS
>> +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;
>> +
>> + return sprintf(buf, "%lld.%09d#%d\n",
>
> sysfs files should be using sysfs_emit()
>
>> + (long long) pps->assert_elapsed_tu.sec,
>> + pps->assert_elapsed_tu.nsec,
>> + pps->assert_sequence);
>> +}
>> +static DEVICE_ATTR_RO(assert_elapsed);
>> +#endif
>> +
>> static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
>> char *buf)
>> {
>> @@ -43,6 +61,24 @@ static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
>> }
>> static DEVICE_ATTR_RO(clear);
>>
>> +#ifdef ENABLE_ELAPSED_REALTIME_PPS
>> +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;
>> +
>> + return sprintf(buf, "%lld.%09d#%d\n",
>> + (long long) pps->clear_elapsed_tu.sec,
>> + pps->clear_elapsed_tu.nsec,
>> + pps->clear_sequence);
>> +}
>> +static DEVICE_ATTR_RO(clear_elapsed);
>> +#endif
>> +
>> static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
>> char *buf)
>> {
>> @@ -82,6 +118,10 @@ static DEVICE_ATTR_RO(path);
>> static struct attribute *pps_attrs[] = {
>> &dev_attr_assert.attr,
>> &dev_attr_clear.attr,
>> +#ifdef ENABLE_ELAPSED_REALTIME_PPS
>> + &dev_attr_assert_elapsed.attr,
>> + &dev_attr_clear_elapsed.attr,
>> +#endif
>
> Don't use #ifdef in a .c file, these attributes should only show up if
> you determine at runtime they should not be there.
>
> But why woudn't they just always be there all the time anyway? Why do
> you need a new config option?
>
>
>> &dev_attr_mode.attr,
>> &dev_attr_echo.attr,
>> &dev_attr_name.attr,
>> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
>> index 78c8ac4951b5..1fecaaf4c8b9 100644
>> --- a/include/linux/pps_kernel.h
>> +++ b/include/linux/pps_kernel.h
>> @@ -47,6 +47,8 @@ struct pps_device {
>>
>> __u32 assert_sequence; /* PPS assert event seq # */
>> __u32 clear_sequence; /* PPS clear event seq # */
>> + struct pps_ktime assert_elapsed_tu; /* PPS elapsed rt assert seq # */
>> + struct pps_ktime clear_elapsed_tu; /* PPS elapsed rt clear event seq */
>
> no tabs?
>
> thanks,
>
> greg k-h
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@enneenne.com
Linux Device Driver giometti@linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-03-15 7:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-15 0:52 [PATCH v3] pps: Add elapsed realtime timestamping Alexander Komrakov
2023-03-15 5:02 ` Greg KH
2023-03-15 7:44 ` Rodolfo Giometti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®