Add DTE driver for Cygnus
cover Add DTE driver for Cygnus
This patchset includes the Digital Timing Engine (DTE) driver for Cygnus SoC's.
Jonathan Richardson (2):
misc: Add DT binding for cygnus Digital Timing Engine (DTE) driver.
misc: Add initial Digital Timing Engine (DTE) driver for cygnus
.../bindings/arm/bcm/brcm,cygnus-dte.txt | 24 + drivers/misc/Kconfig | 11 + drivers/misc/Makefile | 1 + drivers/misc/bcm_cygnus_dte.c | 927 ++++++++++++++++++++ include/linux/bcm_cygnus_dte.h | 99 +++ 5 files changed, 1062 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/bcm/brcm,cygnus-dte.txt create mode 100644 drivers/misc/bcm_cygnus_dte.c create mode 100644 include/linux/bcm_cygnus_dte.h
Signature · 1 line
1.7.9.5
PATCH 1/2 misc: Add DT binding for cygnus Digital Timing Engine (DTE) driver.
--- .../bindings/arm/bcm/brcm,cygnus-dte.txt | 24 ++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/bcm/brcm,cygnus-dte.txt
Documentation/devicetree/bindings/arm/bcm/brcm,cygnus-dte.txt new file +24 -0
@@ -0,0 +1,24 @@ +Broadcom Cygnus DTE Digital Timing Engine device tree bindings + +The DTE block allows for hardware timestamping of network packets, +audio/video samples and/or GPIO inputs in a common adjustable time base. + +Required SoC Specific Properties: +- compatible: should be "brcm,cygnus-dte" + +- reg: Requires two separate register sets + Base address of the Audio EAV Regsiters for DTE block and length of memory + mapped region. + +- interrupts: The DTE interrupt number to the cpu. + +Example: + +/ { + dte: dte@0x180af000 { + compatible = "brcm,cygnus-dte"; + reg = <0x180af000 0x1000>; + interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>; + + }; +};
Signature · 1 line
1.7.9.5
PATCH 2/2 misc: Add initial Digital Timing Engine (DTE) driver for cygnus
In reply to Jonathan Richardson--- drivers/misc/Kconfig | 11 + drivers/misc/Makefile | 1 + drivers/misc/bcm_cygnus_dte.c | 927 ++++++++++++++++++++++++++++++++++++++++ include/linux/bcm_cygnus_dte.h | 99 +++++ 4 files changed, 1038 insertions(+) create mode 100644 drivers/misc/bcm_cygnus_dte.c create mode 100644 include/linux/bcm_cygnus_dte.h
drivers/misc/Kconfig +11 -0
@@ -515,6 +515,17 @@ config VEXPRESS_SYSCFG bus. System Configuration interface is one of the possible means of generating transactions on this bus. +config BCM_CYGNUS_DTE + tristate "Cygnus DTE support" + depends on ARCH_BCM_CYGNUS + default ARCH_BCM_CYGNUS + help + Enable support for a Digital Timing Engine (DTE) that allows for hardware + time stamping of network packets, audio/video samples and/or GPIO inputs + in a common adjustable time base. + + If unsure, say N. + source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig"
drivers/misc/Makefile +1 -0
@@ -56,3 +56,4 @@ obj-$(CONFIG_GENWQE) += genwqe/ obj-$(CONFIG_ECHO) += echo/ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o obj-$(CONFIG_CXL_BASE) += cxl/ +obj-$(CONFIG_BCM_CYGNUS_DTE) += bcm_cygnus_dte.o
drivers/misc/bcm_cygnus_dte.c new file +927 -0
927 lines not on this page · raw message
include/linux/bcm_cygnus_dte.h new file +99 -0
99 lines not on this page · raw message
Signature · 1 line
1.7.9.5
Arnd Bergmann
maintainerRe: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 3 lines · On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote:
Reviewed-by Scott Branden <[email protected]>Tested-by Scott Branden <[email protected]>Signed-off-by Jonathan Richardson <[email protected]>
No description at all?
You are introducing a new subsystem here, which means you get to put a lot of thought into the API design, to ensure that it works with other drivers of the same type, and that we don't already have a subsystem that does what you need here.
Please write a few pages of text about the tradeoffs that went into the internal and user-facing API design, and explain the differences to the existing infrastructure we have for the clocksource, clockevent, k_clock, posix timers, posix timers, timerfd, rtc, and ptp frameworks, in particular why your hardware cannot fit into the existing frameworks and has to have a new one.
Quoted 9 lines
drivers/misc/bcm_cygnus_dte.c new file +9
-0+struct bcm_cygnus_dte *dte_get_dev_from_devname (const char *devname) +{ +struct bcm_cygnus_dte *cygnus_dte =NULL ; +bool found = false; + +if (!devname) +return NULL ; + +list_for_each_entry (cygnus_dte, &dtedev_list, node) {
drivers/misc/bcm_cygnus_dte.c line 178
Quoted 10 lines
drivers/misc/bcm_cygnus_dte.c new file +10
-0+if (!strcmp (dev_name (&cygnus_dte->pdev->dev), devname)) { +/* Matched on device name */ + found = true; +break ; + } + } + +return found ? cygnus_dte :NULL ; +} +EXPORT_SYMBOL (dte_get_dev_from_devname);
No, don't match on a device name. If you must have a reference, use a phandle in DT.
drivers/misc/bcm_cygnus_dte.c line 300
Quoted 4 lines
drivers/misc/bcm_cygnus_dte.c new file +4
-0+int dte_get_timestamp ( +struct bcm_cygnus_dte *cygnus_dte, +enum dte_client client, +struct timespec *ts)
Please use timespec64 or ktime_t for internal interfaces.
drivers/misc/bcm_cygnus_dte.c line 494
Quoted 1 line
drivers/misc/bcm_cygnus_dte.c new file +1
-0+case DTE_IOCTL_SET_DIVIDER :
For the IOCTLs, please write a documentation in man-page form. No need for troff formatting, plain text is fine.
Quoted 9 lines
include/linux/bcm_cygnus_dte.h new file +9
-0+/** +* DTE Client +*/ +enum dte_client { +DTE_CLIENT_MIN =0 , +DTE_CLIENT_I2S0_BITCLOCK =0 , +DTE_CLIENT_I2S1_BITCLOCK , +DTE_CLIENT_I2S2_BITCLOCK , +DTE_CLIENT_I2S0_WORDCLOCK ,
include/linux/bcm_cygnus_dte.h line 41
Quoted 10 lines
include/linux/bcm_cygnus_dte.h new file +10
-0+DTE_CLIENT_I2S1_WORDCLOCK , +DTE_CLIENT_I2S2_WORDCLOCK , +DTE_CLIENT_LCD_CLFP , +DTE_CLIENT_LCD_CLLP , +DTE_CLIENT_GPIO14 , +DTE_CLIENT_GPIO15 , +DTE_CLIENT_GPIO22 , +DTE_CLIENT_GPIO23 , + DTE_CLIENT_MAX, +};
Make this more abstract, so we can reuse the API for other vendors.
include/linux/bcm_cygnus_dte.h line 54
Quoted 12 lines
include/linux/bcm_cygnus_dte.h new file +12
-0+#define DTE_IOCTL_BASE 'd' +#define DTE_IO (nr) _IO(DTE_IOCTL_BASE, nr) +#define DTE_IOR (nr, type) _IOR(DTE_IOCTL_BASE, nr, type) +#define DTE_IOW (nr, type) _IOW(DTE_IOCTL_BASE, nr, type) +#define DTE_IOWR (nr, type) _IOWR(DTE_IOCTL_BASE, nr, type) + +#define DTE_IOCTL_SET_DIVIDER DTE_IOW(0x00, struct dte_data) +#define DTE_IOCTL_ENABLE_TIMESTAMP DTE_IOW(0x01, struct dte_data) +#define DTE_IOCTL_SET_IRQ_INTERVAL DTE_IOW(0x02, struct dte_data) +#define DTE_IOCTL_GET_TIMESTAMP DTE_IOWR(0x03, struct dte_timestamp) +#define DTE_IOCTL_SET_TIME DTE_IOW(0x04, struct timespec) +#define DTE_IOCTL_GET_TIME DTE_IOR(0x05, struct timespec)
Instead of timespec, use a pair of '__u64' values, or alternatively just a '__u64' for nanoseconds if the API does not have to cover times before 1970 or after 2262.
include/linux/bcm_cygnus_dte.h line 56
Quoted 2 lines
include/linux/bcm_cygnus_dte.h new file +2
-0+#define DTE_IOCTL_ADJ_TIME DTE_IOW(0x06, int64_t) +#define DTE_IOCTL_ADJ_FREQ DTE_IOW(0x07, int32_t)
Maybe 'struct timex' for the adjustment?
Also, how about adding new syscalls along the lines of the timerfd stuff instead of using ioctl?
include/linux/bcm_cygnus_dte.h line 61
Quoted 4 lines
include/linux/bcm_cygnus_dte.h new file +4
-0+struct dte_data { +enum dte_client client; +unsigned int data; +};
No 'enum' in ioctl data, always use '__u32' etc.
include/linux/bcm_cygnus_dte.h line 66
Quoted 5 lines
include/linux/bcm_cygnus_dte.h new file +5
-0+ +struct dte_timestamp { +enum dte_client client; +struct timespec ts; +};
Instead of timespec, use a pair of '__u64' values, or alternatively just a '__u64' for nanoseconds if the API does not have to cover times before 1970 or after 2262.
include/linux/bcm_cygnus_dte.h line 75
Quoted 8 lines
include/linux/bcm_cygnus_dte.h new file +8
-0+struct bcm_cygnus_dte ; + +extern struct bcm_cygnus_dte *dte_get_dev_from_devname ( +const char *devname); +extern int dte_enable_timestamp ( +struct bcm_cygnus_dte *cygnus_dte, +enum dte_client client, +int enable);
Put the internal declarations into one header in include/linux, and the user space facing ones in another one in include/uapi/linux.
Arnd
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Hi Arnd,
Thanks for the initial feedback. I'll investigate your suggestions and get back to you if I have any questions before making some of the API changes you've suggested.
Quoted 133 lines · Arnd Bergmann
Quoted 3 lines · On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote:
Reviewed-by Scott Branden <[email protected]>Tested-by Scott Branden <[email protected]>Signed-off-by Jonathan Richardson <[email protected]>No description at all?
You are introducing a new subsystem here, which means you get to put a lot of thought into the API design, to ensure that it works with other drivers of the same type, and that we don't already have a subsystem that does what you need here.
Please write a few pages of text about the tradeoffs that went into the internal and user-facing API design, and explain the differences to the existing infrastructure we have for the clocksource, clockevent,
k_clock, posix timers, posix timers, timerfd, rtc, and ptp frameworks, in particular why your hardware cannot fit into the existing frameworks and has to have a new one.Quoted 9 lines
drivers/misc/bcm_cygnus_dte.c new file +9
-0+struct bcm_cygnus_dte *dte_get_dev_from_devname (const char *devname) +{ +struct bcm_cygnus_dte *cygnus_dte =NULL ; +bool found = false; + +if (!devname) +return NULL ; + +list_for_each_entry (cygnus_dte, &dtedev_list, node) {drivers/misc/bcm_cygnus_dte.c line 178
Quoted 10 lines
drivers/misc/bcm_cygnus_dte.c new file +10
-0+if (!strcmp (dev_name (&cygnus_dte->pdev->dev), devname)) { +/* Matched on device name */ + found = true; +break ; + } + } + +return found ? cygnus_dte :NULL ; +} +EXPORT_SYMBOL (dte_get_dev_from_devname);No, don't match on a device name. If you must have a reference, use a phandle in DT.
drivers/misc/bcm_cygnus_dte.c line 300
Quoted 4 lines
drivers/misc/bcm_cygnus_dte.c new file +4
-0+int dte_get_timestamp ( +struct bcm_cygnus_dte *cygnus_dte, +enum dte_client client, +struct timespec *ts)Please use timespec64 or
ktime_tfor internal interfaces.drivers/misc/bcm_cygnus_dte.c line 494
Quoted 1 line
drivers/misc/bcm_cygnus_dte.c new file +1
-0+case DTE_IOCTL_SET_DIVIDER :For the IOCTLs, please write a documentation in man-page form. No need for troff formatting, plain text is fine.
Quoted 9 lines
include/linux/bcm_cygnus_dte.h new file +9
-0+/** +* DTE Client +*/ +enum dte_client { +DTE_CLIENT_MIN =0 , +DTE_CLIENT_I2S0_BITCLOCK =0 , +DTE_CLIENT_I2S1_BITCLOCK , +DTE_CLIENT_I2S2_BITCLOCK , +DTE_CLIENT_I2S0_WORDCLOCK ,include/linux/bcm_cygnus_dte.h line 41
Quoted 10 lines
include/linux/bcm_cygnus_dte.h new file +10
-0+DTE_CLIENT_I2S1_WORDCLOCK , +DTE_CLIENT_I2S2_WORDCLOCK , +DTE_CLIENT_LCD_CLFP , +DTE_CLIENT_LCD_CLLP , +DTE_CLIENT_GPIO14 , +DTE_CLIENT_GPIO15 , +DTE_CLIENT_GPIO22 , +DTE_CLIENT_GPIO23 , + DTE_CLIENT_MAX, +};Make this more abstract, so we can reuse the API for other vendors.
include/linux/bcm_cygnus_dte.h line 54
Quoted 12 lines
include/linux/bcm_cygnus_dte.h new file +12
-0+#define DTE_IOCTL_BASE 'd' +#define DTE_IO (nr) _IO(DTE_IOCTL_BASE, nr) +#define DTE_IOR (nr, type) _IOR(DTE_IOCTL_BASE, nr, type) +#define DTE_IOW (nr, type) _IOW(DTE_IOCTL_BASE, nr, type) +#define DTE_IOWR (nr, type) _IOWR(DTE_IOCTL_BASE, nr, type) + +#define DTE_IOCTL_SET_DIVIDER DTE_IOW(0x00, struct dte_data) +#define DTE_IOCTL_ENABLE_TIMESTAMP DTE_IOW(0x01, struct dte_data) +#define DTE_IOCTL_SET_IRQ_INTERVAL DTE_IOW(0x02, struct dte_data) +#define DTE_IOCTL_GET_TIMESTAMP DTE_IOWR(0x03, struct dte_timestamp) +#define DTE_IOCTL_SET_TIME DTE_IOW(0x04, struct timespec) +#define DTE_IOCTL_GET_TIME DTE_IOR(0x05, struct timespec)Instead of timespec, use a pair of '
__u64' values, or alternatively just a '__u64' for nanoseconds if the API does not have to cover times before 1970 or after 2262.include/linux/bcm_cygnus_dte.h line 56
Quoted 2 lines
include/linux/bcm_cygnus_dte.h new file +2
-0+#define DTE_IOCTL_ADJ_TIME DTE_IOW(0x06, int64_t) +#define DTE_IOCTL_ADJ_FREQ DTE_IOW(0x07, int32_t)Maybe 'struct timex' for the adjustment?
Also, how about adding new syscalls along the lines of the timerfd stuff instead of using ioctl?
include/linux/bcm_cygnus_dte.h line 61
Quoted 4 lines
include/linux/bcm_cygnus_dte.h new file +4
-0+struct dte_data { +enum dte_client client; +unsigned int data; +};No 'enum' in ioctl data, always use '
__u32' etc.include/linux/bcm_cygnus_dte.h line 66
Quoted 5 lines
include/linux/bcm_cygnus_dte.h new file +5
-0+ +struct dte_timestamp { +enum dte_client client; +struct timespec ts; +};Instead of timespec, use a pair of '
__u64' values, or alternatively just a '__u64' for nanoseconds if the API does not have to cover times before 1970 or after 2262.include/linux/bcm_cygnus_dte.h line 75
Quoted 8 lines
include/linux/bcm_cygnus_dte.h new file +8
-0+struct bcm_cygnus_dte ; + +extern struct bcm_cygnus_dte *dte_get_dev_from_devname ( +const char *devname); +extern int dte_enable_timestamp ( +struct bcm_cygnus_dte *cygnus_dte, +enum dte_client client, +int enable);Put the internal declarations into one header in
include/linux, and the user space facing ones in another one ininclude/uapi/linux.Arnd
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
In reply to Arnd BergmannArnd, some more info is provided and also some questions before I can proceed.
Quoted 6 lines · Arnd Bergmann
Quoted 3 lines · On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote:
Reviewed-by Scott Branden <[email protected]>Tested-by Scott Branden <[email protected]>Signed-off-by Jonathan Richardson <[email protected]>No description at all?
The DTE creates timestamps of hardware based events such as GPIO, I2S signals for audio, etc. It was also intended to provide 802.1AS / PTP timestamps for network packets. The h/w has up to 32 "clients" -- the hardware inputs into a timestamping engine. These clients are specific to the chip the DTE is used on. For Cygnus you can see what they are in our 'enum dte_client' from bcm_cygnus_dte.h.
The DTE timestamper creates timestamps based on the current clock wall time. When an event occurs it stores the timestamp in a h/w FIFO. Each client also has a divider that can be set to control the rate that timestamps are generated at by the timestamper and these are adjustable at run time.
The isochronous interrupt is used to pull the timestamps out of the h/w FIFO and store them in a s/w FIFO until they are pulled out of that from user space.
The API we provide in the driver enables clients (inputs into the timestamper) using dte_enable_timestamp(). The clients divider is set with dte_set_client_divider(). The isochronous interrupt frequency - the rate at which the ISR fires to pull timestamps from h/w to s/w FIFO is dte_set_irq_interval().
Timestamps generated by the timestamper for clients are polled for using dte_get_timestamp().
The clock is controlled with the remaining functions: dte_set_time(), dte_get_time(), dte_adj_time(), dte_adj_freq(). These functions provide the time base for the timestamper but also provide a clock that was meant to be used later by an Ethernet driver for PTP.
The ioctls correspond to all of these functions for the user space API.
The idea for future PTP support was to use the dte_get_time() to get timestamps for the packets. This may make the driver more suitable to be PTP driver.
It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP plus timestamping 32 other hardware inputs that can be enabled at any time with timestamps being generated at varying frequencies. As clients are enabled that generate timestamps at higher frequencies, the isochronous interrupt frequency needs to be increased so that overflows in the the h/w and s/w FIFO's don't occur (this frequency could possibly be automated instead of changing it manually as we currently do).
Quoted 43 lines
You are introducing a new subsystem here, which means you get to put a lot of thought into the API design, to ensure that it works with other drivers of the same type, and that we don't already have a subsystem that does what you need here.
Please write a few pages of text about the tradeoffs that went into the internal and user-facing API design, and explain the differences to the existing infrastructure we have for the clocksource, clockevent,
k_clock, posix timers, posix timers, timerfd, rtc, and ptp frameworks, in particular why your hardware cannot fit into the existing frameworks and has to have a new one.Quoted 9 lines
drivers/misc/bcm_cygnus_dte.c new file +9
-0+struct bcm_cygnus_dte *dte_get_dev_from_devname (const char *devname) +{ +struct bcm_cygnus_dte *cygnus_dte =NULL ; +bool found = false; + +if (!devname) +return NULL ; + +list_for_each_entry (cygnus_dte, &dtedev_list, node) {drivers/misc/bcm_cygnus_dte.c line 178
Quoted 10 lines
drivers/misc/bcm_cygnus_dte.c new file +10
-0+if (!strcmp (dev_name (&cygnus_dte->pdev->dev), devname)) { +/* Matched on device name */ + found = true; +break ; + } + } + +return found ? cygnus_dte :NULL ; +} +EXPORT_SYMBOL (dte_get_dev_from_devname);No, don't match on a device name. If you must have a reference, use a phandle in DT.
drivers/misc/bcm_cygnus_dte.c line 300
Quoted 4 lines
drivers/misc/bcm_cygnus_dte.c new file +4
-0+int dte_get_timestamp ( +struct bcm_cygnus_dte *cygnus_dte, +enum dte_client client, +struct timespec *ts)Please use timespec64 or
ktime_tfor internal interfaces.
Quoted 4 lines
drivers/misc/bcm_cygnus_dte.c line 494
Quoted 1 line
drivers/misc/bcm_cygnus_dte.c new file +1
-0+case DTE_IOCTL_SET_DIVIDER :For the IOCTLs, please write a documentation in man-page form. No need for troff formatting, plain text is fine.
Sure, where should this man page live in the kernel?
Quoted 40 lines
Quoted 10 lines
include/linux/bcm_cygnus_dte.h new file +9
-0+/** +* DTE Client +*/ +enum dte_client { +DTE_CLIENT_MIN =0 , +DTE_CLIENT_I2S0_BITCLOCK =0 , +DTE_CLIENT_I2S1_BITCLOCK , +DTE_CLIENT_I2S2_BITCLOCK , +DTE_CLIENT_I2S0_WORDCLOCK ,include/linux/bcm_cygnus_dte.h line 41
Quoted 10 lines
include/linux/bcm_cygnus_dte.h new file +10
-0+DTE_CLIENT_I2S1_WORDCLOCK , +DTE_CLIENT_I2S2_WORDCLOCK , +DTE_CLIENT_LCD_CLFP , +DTE_CLIENT_LCD_CLLP , +DTE_CLIENT_GPIO14 , +DTE_CLIENT_GPIO15 , +DTE_CLIENT_GPIO22 , +DTE_CLIENT_GPIO23 , + DTE_CLIENT_MAX, +};Make this more abstract, so we can reuse the API for other vendors.
include/linux/bcm_cygnus_dte.h line 54
Quoted 12 lines
include/linux/bcm_cygnus_dte.h new file +12
-0+#define DTE_IOCTL_BASE 'd' +#define DTE_IO (nr) _IO(DTE_IOCTL_BASE, nr) +#define DTE_IOR (nr, type) _IOR(DTE_IOCTL_BASE, nr, type) +#define DTE_IOW (nr, type) _IOW(DTE_IOCTL_BASE, nr, type) +#define DTE_IOWR (nr, type) _IOWR(DTE_IOCTL_BASE, nr, type) + +#define DTE_IOCTL_SET_DIVIDER DTE_IOW(0x00, struct dte_data) +#define DTE_IOCTL_ENABLE_TIMESTAMP DTE_IOW(0x01, struct dte_data) +#define DTE_IOCTL_SET_IRQ_INTERVAL DTE_IOW(0x02, struct dte_data) +#define DTE_IOCTL_GET_TIMESTAMP DTE_IOWR(0x03, struct dte_timestamp) +#define DTE_IOCTL_SET_TIME DTE_IOW(0x04, struct timespec) +#define DTE_IOCTL_GET_TIME DTE_IOR(0x05, struct timespec)Instead of timespec, use a pair of '
__u64' values, or alternatively just a '__u64' for nanoseconds if the API does not have to cover times before 1970 or after 2262.
Quoted 7 lines
include/linux/bcm_cygnus_dte.h line 56
Quoted 2 lines
include/linux/bcm_cygnus_dte.h new file +2
-0+#define DTE_IOCTL_ADJ_TIME DTE_IOW(0x06, int64_t) +#define DTE_IOCTL_ADJ_FREQ DTE_IOW(0x07, int32_t)Maybe 'struct timex' for the adjustment?
Also, how about adding new syscalls along the lines of the timerfd stuff instead of using ioctl?
It looks like the driver could almost be a PTP driver instead of a char driver controlled with ioctls. PTP does this already using clock_gettime(), clock_settime(), clock_adjtime(). But we want to set the frequency as well as adjust the clock and I don't see how that is possible with the stripped down timex data passed to the driver from ptp_clock_adjtime().
We have the additional requirement of controlling multiple clients and retrieving the timestamps, etc. The PTP driver allows for some control of external time stamp channels already using 'n_ext_ts' in struct ptp_clock_info. We could use that to enable clients and get timestamps, but we also need to be able to change dividers for the clients at run time if desired. It doesn't look like additional ioctls could be passed to a PTP driver because ptp_ioctl() is the ioctl handler.
I don't see how any of the API's currently do what we need other than the flexibility a char device provides. Just wanted to get your thoughts on this before I start looking at an entirely new user and kernel API for DTE. Really what we need is PTP with extended external timestamping channel control, unless I'm missing something. If people are flexible with extending that I could propose something. Let me know which way you prefer to go. Thanks.
Quoted 33 lines
include/linux/bcm_cygnus_dte.h line 61
Quoted 5 lines
include/linux/bcm_cygnus_dte.h new file +4
-0+struct dte_data { +enum dte_client client; +unsigned int data; +};No 'enum' in ioctl data, always use '
__u32' etc.include/linux/bcm_cygnus_dte.h line 66
Quoted 5 lines
include/linux/bcm_cygnus_dte.h new file +5
-0+ +struct dte_timestamp { +enum dte_client client; +struct timespec ts; +};Instead of timespec, use a pair of '
__u64' values, or alternatively just a '__u64' for nanoseconds if the API does not have to cover times before 1970 or after 2262.include/linux/bcm_cygnus_dte.h line 75
Quoted 8 lines
include/linux/bcm_cygnus_dte.h new file +8
-0+struct bcm_cygnus_dte ; + +extern struct bcm_cygnus_dte *dte_get_dev_from_devname ( +const char *devname); +extern int dte_enable_timestamp ( +struct bcm_cygnus_dte *cygnus_dte, +enum dte_client client, +int enable);Put the internal declarations into one header in
include/linux, and the user space facing ones in another one ininclude/uapi/linux.Arnd
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 7 lines
It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP plus timestamping 32 other hardware inputs that can be enabled at any time with timestamps being generated at varying frequencies. As clients are enabled that generate timestamps at higher frequencies, the isochronous interrupt frequency needs to be increased so that overflows in the the h/w and s/w FIFO's don't occur (this frequency could possibly be automated instead of changing it manually as we currently do).
Nice that this is finally happening mainstream having worked on an early design for this about 25 years ago 8)
Quoted 6 lines
It looks like the driver could almost be a PTP driver instead of a char driver controlled with ioctls. PTP does this already using
clock_gettime(),clock_settime(),clock_adjtime(). But we want to set the frequency as well as adjust the clock and I don't see how that is possible with the stripped down timex data passed to the driver fromptp_clock_adjtime().
Agreed. You also want the plumbing so that socket sk_buffs can be timestamped by it when it is monitoring the IRQ (the stamp code in the kernel actually got added way back when for exactly this type of card).
Quoted 3 lines
channel control, unless I'm missing something. If people are flexible with extending that I could propose something. Let me know which way you prefer to go. Thanks.
I would strongly favour fixing PTP to do this right. Otherwise we will have 2 or 3 adhoc drivers, then end up moving them to PTP and then end up dealing with the compat mess.
The only other question I'd ponder is whether aspects of this best fit PTP or IIO or both depending upon purpose. There's after all no fundamental reason a driver can't provide services to both types of consumer depending upon the need.
Alan
Arnd Bergmann
maintainerRe: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 8 lines · On Friday 01 May 2015 20:30:04 One Thousand Gnomes wrote:
Quoted 4 lines
channel control, unless I'm missing something. If people are flexible with extending that I could propose something. Let me know which way you prefer to go. Thanks.
I would strongly favour fixing PTP to do this right. Otherwise we will have 2 or 3 adhoc drivers, then end up moving them to PTP and then end up dealing with the compat mess.
Agreed. I was hoping that there was already a subsystem in the kernel that could be extended to work with this driver. If all that is needed is to pass more fields of the existing timex to ptp drivers, that should be fairly easy to do.
We also have some padding bytes available in struct timex in case some extra data needs to be passed, or we could add another clock_* system call if it's absolutely required to have another entry into the driver, and connect that through struct k_clock and posix_clock_operations(). I would hope we don't even need that.
Arnd
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 15 lines · Arnd Bergmann
Quoted 8 lines · On Friday 01 May 2015 20:30:04 One Thousand Gnomes wrote:
Quoted 4 lines
channel control, unless I'm missing something. If people are flexible with extending that I could propose something. Let me know which way you prefer to go. Thanks.
I would strongly favour fixing PTP to do this right. Otherwise we will have 2 or 3 adhoc drivers, then end up moving them to PTP and then end up dealing with the compat mess.
Agreed. I was hoping that there was already a subsystem in the kernel that could be extended to work with this driver. If all that is needed is to pass more fields of the existing timex to ptp drivers, that should be fairly easy to do.
Quoted 8 lines
We also have some padding bytes available in struct timex in case some extra data needs to be passed, or we could add another clock_* system call if it's absolutely required to have another entry into the driver, and connect that through struct
k_clockandposix_clock_operations(). I would hope we don't even need that.Arnd
Thanks Arnd/Alan for the feedback. I looked into PTP further to see what exactly would need changing.
For the clock functions I think we can use the existing framework unchanged with one exception: ptp_clock_adjtime() doesn't allow negative time adjustments and we would like to allow this.
The other client events (set IRQ interval, enable client, set client divider, get client timestamp) could be handled as follows:
IRQ interval: I mentioned before that we may be able to calculate the isochronous interrupt rate automatically but this isn't possible because the DTE doesn't know the frequency of the clients. One solution is to use the 'PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency. Not really a timer but good enough for our purposes.
Enable Client: The external timestamping channel handling in PTP allows channels to be enabled so we can re-use this (PTP_EXTTS_REQUEST ioctl).
Set divider: There is no ability to set a frequency or do anything to a channel. We could re-use the PTP_EXTTS_REQUEST ioctl but extend 'struct ptp_extts_request' by using the reserved field rsv to allow setting an integer value representing either a frequency or divider value in our case - some value to configure a channel. A new flag would have to be added to the existing PTP_ENABLE_FEATURE, RISING and FALLING EDGE.
Get timestamp: This is a bit more complicated. Currently the PTP driver does list management for timestamps from external timestamp channels. Timestamps from all channels go into the same list. In our driver we have a s/w FIFO for each client and it closely matches the h/w FIFO and handles any overflow. We would like to keep it this way because it also allows multiple user space processes to only fetch timestamps for the client it's handling. We could add a new ioctl to get a timestamp from the driver instead of doing it through ptp_read() but it would be nice if we could let ptp_read() allow the driver to do timestamp management instead of PTP. Maybe provide an option to obtain the timestamps from a container in the driver instead of the one managed by PTP. I like being able to use read/poll to obtain data instead of polling the kernel with ioctls as we are currently doing. Also, avoiding the kmalloc in ptp_read would be nice because this of the frequency it would be called at. Do you have any preference on how to handle this?
I've tried to minimize the changes to PTP.
Alan, what is IIO? I'm not familiar.
Thanks.
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
A bit more info is required here.
Quoted 16 lines
Get timestamp: This is a bit more complicated. Currently the PTP driver does list management for timestamps from external timestamp channels. Timestamps from all channels go into the same list. In our driver we have a s/w FIFO for each client and it closely matches the h/w FIFO and handles any overflow. We would like to keep it this way because it also allows multiple user space processes to only fetch timestamps for the client it's handling. We could add a new ioctl to get a timestamp from the driver instead of doing it through
ptp_read()but it would be nice if we could letptp_read()allow the driver to do timestamp management instead of PTP. Maybe provide an option to obtain the timestamps from a container in the driver instead of the one managed by PTP. I like being able to use read/poll to obtain data instead of polling the kernel with ioctls as we are currently doing. Also, avoiding the kmalloc inptp_readwould be nice because this of the frequency it would be called at. Do you have any preference on how to handle this?
Quoted 1 line
I've tried to minimize the changes to PTP.
Using read() we can't specify a channel to get the timestamp for. It would imply that one user space process would have to read timestamps for all channels/clients and then leave it up to user space IPC to get them to other processes, which isn't great. That means either we introduce an ioctl such as the DTE_GET_TIMESTAMP we had before which allows us to specify a channel, or we need to look at creating one dev node per external timestamping channel.
The ioctl limitation is that it pounds the kernel polling for timestamps and the multiple dev nodes per channel is a big change to PTP. I will have to look further into this to really have a good idea of what the implications would be. Advice/ideas?
Thanks.
Arnd Bergmann
maintainerRe: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
In reply to Jonathan RichardsonQuoted 25 lines · On Friday 08 May 2015 13:02:17 Jonathan Richardson wrote:
Quoted 15 lines · Arnd Bergmann
Quoted 8 lines · On Friday 01 May 2015 20:30:04 One Thousand Gnomes wrote:
Quoted 4 lines
channel control, unless I'm missing something. If people are flexible with extending that I could propose something. Let me know which way you prefer to go. Thanks.
I would strongly favour fixing PTP to do this right. Otherwise we will have 2 or 3 adhoc drivers, then end up moving them to PTP and then end up dealing with the compat mess.
Agreed. I was hoping that there was already a subsystem in the kernel that could be extended to work with this driver. If all that is needed is to pass more fields of the existing timex to ptp drivers, that should be fairly easy to do.
Quoted 8 lines
We also have some padding bytes available in struct timex in case some extra data needs to be passed, or we could add another clock_* system call if it's absolutely required to have another entry into the driver, and connect that through struct
k_clockandposix_clock_operations(). I would hope we don't even need that.Arnd
Quoted 6 lines
Thanks Arnd/Alan for the feedback. I looked into PTP further to see what exactly would need changing.
For the clock functions I think we can use the existing framework unchanged with one exception:
ptp_clock_adjtime()doesn't allow negative time adjustments and we would like to allow this.
Ah, very good. Extending this one function should be fairly straightforward, just discuss with the PTP maintainer how to best do it.
Quoted 11 lines
The other client events (set IRQ interval, enable client, set client divider, get client timestamp) could be handled as follows:
IRQ interval: I mentioned before that we may be able to calculate the isochronous interrupt rate automatically but this isn't possible because the DTE doesn't know the frequency of the clients. One solution is to use the '
PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency. Not really a timer but good enough for our purposes.Enable Client: The external timestamping channel handling in PTP allows channels to be enabled so we can re-use this (
PTP_EXTTS_REQUESTioctl).
In each case, it might be better to add a new ioctl command instead, if the existing command does not fit perfectly. This again would be up to Richard.
Quoted 6 lines
Set divider: There is no ability to set a frequency or do anything to a channel. We could re-use the
PTP_EXTTS_REQUESTioctl but extend 'structptp_extts_request' by using the reserved field rsv to allow setting an integer value representing either a frequency or divider value in our case - some value to configure a channel. A new flag would have to be added to the existingPTP_ENABLE_FEATURE, RISING and FALLING EDGE.
Seems reasonable, or again you could add a new command with a different structure if Richard prefers that.
Quoted 17 lines
Get timestamp: This is a bit more complicated. Currently the PTP driver does list management for timestamps from external timestamp channels. Timestamps from all channels go into the same list. In our driver we have a s/w FIFO for each client and it closely matches the h/w FIFO and handles any overflow. We would like to keep it this way because it also allows multiple user space processes to only fetch timestamps for the client it's handling. We could add a new ioctl to get a timestamp from the driver instead of doing it through
ptp_read()but it would be nice if we could letptp_read()allow the driver to do timestamp management instead of PTP. Maybe provide an option to obtain the timestamps from a container in the driver instead of the one managed by PTP. I like being able to use read/poll to obtain data instead of polling the kernel with ioctls as we are currently doing. Also, avoiding the kmalloc inptp_readwould be nice because this of the frequency it would be called at. Do you have any preference on how to handle this?I've tried to minimize the changes to PTP.
Quoted 8 lines · On Tuesday 12 May 2015 18:03:40 Jonathan Richardson wrote:
Using
read()we can't specify a channel to get the timestamp for. It would imply that one user space process would have to read timestamps for all channels/clients and then leave it up to user space IPC to get them to other processes, which isn't great. That means either we introduce an ioctl such as theDTE_GET_TIMESTAMPwe had before which allows us to specify a channel, or we need to look at creating one dev node per external timestamping channel.
Quoted 4 lines
The ioctl limitation is that it pounds the kernel polling for timestamps and the multiple dev nodes per channel is a big change to PTP. I will have to look further into this to really have a good idea of what the implications would be. Advice/ideas?
Would it help if you register each channel as a separate ptp clock? That way you could open them separately from user space in order to just read from one of them.
Arnd
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 2 lines · Arnd Bergmann
Ah, very good. Extending this one function should be fairly straightforward, just discuss with the PTP maintainer how to best do it.
Sorry, I deleted the original post, and I have zero clue what is is all about. Can you give me a link to the thread?
Better yet, just repost the series with the proposed PTP changes.
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 6 lines · Richard Cochran
Quoted 2 lines · Arnd Bergmann
Ah, very good. Extending this one function should be fairly straightforward, just discuss with the PTP maintainer how to best do it.
Sorry, I deleted the original post, and I have zero clue what is is all about. Can you give me a link to the thread?
I found 0/2 and 2/2 in the Gmail Trash.
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
In reply to Jonathan RichardsonQuoted 3 lines · Jonathan Richardson
For the clock functions I think we can use the existing framework unchanged with one exception:
ptp_clock_adjtime()doesn't allow negative time adjustments and we would like to allow this.
???
/**
* struct ptp_clock_info - decribes a PTP hardware clock
...
* @adjtime: Shifts the time of the hardware clock.
* parameter delta: Desired change in nanoseconds.
...
int (*adjtime)(struct ptp_clock_info *ptp, s64 delta);
That s64 is 's' as in "signed".
Quoted 5 lines
IRQ interval: I mentioned before that we may be able to calculate the isochronous interrupt rate automatically but this isn't possible because the DTE doesn't know the frequency of the clients. One solution is to use the '
PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency. Not really a timer but good enough for our purposes.
As I said in my other reply, I don't understand what the problem is.
Quoted 6 lines
Set divider: There is no ability to set a frequency or do anything to a channel. We could re-use the
PTP_EXTTS_REQUESTioctl but extend 'structptp_extts_request' by using the reserved field rsv to allow setting an integer value representing either a frequency or divider value in our case - some value to configure a channel. A new flag would have to be added to the existingPTP_ENABLE_FEATURE, RISING and FALLING EDGE.
I don't get this, either.
Quoted 7 lines
Get timestamp: This is a bit more complicated. Currently the PTP driver does list management for timestamps from external timestamp channels. Timestamps from all channels go into the same list. In our driver we have a s/w FIFO for each client and it closely matches the h/w FIFO and handles any overflow. We would like to keep it this way because it also allows multiple user space processes to only fetch timestamps for the client it's handling.
But having many readers is less efficient and more complex.
Also, we can adjust the buffer if needed to prevent HW FIFO overflows.
Quoted 7 lines
We could add a new ioctl to get a timestamp from
the driver instead of doing it throughptp_read()but it would be nice if we could letptp_read()allow the driver to do timestamp management instead of PTP. Maybe provide an option to obtain the timestamps from a container in the driver instead of the one managed by PTP. I like being able to use read/poll to obtain data instead of polling the kernel with ioctls as we are currently doing.
The PTP interface supports poll/read just fine already.
Quoted 3 lines
Also, avoiding the kmalloc in
ptp_read
would be nice because this of the frequency it would be called at. Do you have any preference on how to handle this?
Originally I had the buffer on the stack, but DaveM didn't like it, saying performance is no excuse for not doing it "the right way".
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 11 lines · Richard Cochran
Quoted 3 lines · Jonathan Richardson
For the clock functions I think we can use the existing framework unchanged with one exception:
ptp_clock_adjtime()doesn't allow negative time adjustments and we would like to allow this.???
/**
* structptp_clock_info- decribes a PTP hardware clock
...
Quoted 7 lines
* @adjtime: Shifts the time of the hardware clock.
* parameter delta: Desired change in nanoseconds.
...int (*adjtime)(struct ptp_clock_info *ptp, s64 delta);That s64 is 's' as in "signed".
ptp_clock_adjtime() casts it to an unsigned and returns an error:
if ((unsigned long) ts.tv_nsec >= NSEC_PER_SEC)
return -EINVAL;
Quoted 8 lines
Quoted 6 lines
IRQ interval: I mentioned before that we may be able to calculate the isochronous interrupt rate automatically but this isn't possible because the DTE doesn't know the frequency of the clients. One solution is to use the '
PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency. Not really a timer but good enough for our purposes.As I said in my other reply, I don't understand what the problem is.
See reply to previous email. We can use this ioctl or add a new one as Arnd suggested. It doesn't matter to me.
Quoted 9 lines
Quoted 7 lines
Set divider: There is no ability to set a frequency or do anything to a channel. We could re-use the
PTP_EXTTS_REQUESTioctl but extend 'structptp_extts_request' by using the reserved field rsv to allow setting an integer value representing either a frequency or divider value in our case - some value to configure a channel. A new flag would have to be added to the existingPTP_ENABLE_FEATURE, RISING and FALLING EDGE.I don't get this, either.
See reply to previous email.
Quoted 21 lines
Quoted 8 lines
Get timestamp: This is a bit more complicated. Currently the PTP driver does list management for timestamps from external timestamp channels. Timestamps from all channels go into the same list. In our driver we have a s/w FIFO for each client and it closely matches the h/w FIFO and handles any overflow. We would like to keep it this way because it also allows multiple user space processes to only fetch timestamps for the client it's handling.
But having many readers is less efficient and more complex.
Also, we can adjust the buffer if needed to prevent HW FIFO overflows.
Quoted 7 lines
We could add a new ioctl to get a timestamp from
the driver instead of doing it throughptp_read()but it would be nice if we could letptp_read()allow the driver to do timestamp management instead of PTP. Maybe provide an option to obtain the timestamps from a container in the driver instead of the one managed by PTP. I like being able to use read/poll to obtain data instead of polling the kernel with ioctls as we are currently doing.
Quoted 1 line
The PTP interface supports poll/read just fine already.
Yes that's why I wanted to re-use it. As it currently is, it's not going to work for reasons I explained in previous follow up yesterday:
http://marc.info/?l=linux-kernel&m=143147907431947&w=2
Quoted 11 lines
Quoted 4 lines
Also, avoiding the kmalloc in
ptp_read
would be nice because this of the frequency it would be called at. Do you have any preference on how to handle this?Originally I had the buffer on the stack, but DaveM didn't like it, saying performance is no excuse for not doing it "the right way".
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 4 lines · Jonathan Richardson
ptp_clock_adjtime()casts it to an unsigned and returns an error:if ((unsigned long) ts.tv_nsec >=
NSEC_PER_SEC)return -EINVAL;
The value of a timeval is the sum of its fields, but the field tv_usec must always be non-negative. The tv_sec field can be negative. So, your application simply needs to do this:
if (tx.time.tv_usec < 0) {
tx.time.tv_sec -= 1;
tx.time.tv_usec += 1000000000;
}Quoted 10 lines
Quoted 7 lines
Quoted 5 lines
IRQ interval: I mentioned before that we may be able to calculate the isochronous interrupt rate automatically but this isn't possible because the DTE doesn't know the frequency of the clients. One solution is to use the '
PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency. Not really a timer but good enough for our purposes.As I said in my other reply, I don't understand what the problem is.
See reply to previous email. We can use this ioctl or add a new one as Arnd suggested. It doesn't matter to me.
Still makes no sense. Why should the interrupt rate depend on the clock frequency?
If the ISR is delivering batches of time stamps, then the interrupt rate aught to be the highest rate that the system can support.
Quoted 10 lines
Quoted 8 lines
Quoted 6 lines
Set divider: There is no ability to set a frequency or do anything to a channel. We could re-use the
PTP_EXTTS_REQUESTioctl but extend 'structptp_extts_request' by using the reserved field rsv to allow setting an integer value representing either a frequency or divider value in our case - some value to configure a channel. A new flag would have to be added to the existingPTP_ENABLE_FEATURE, RISING and FALLING EDGE.I don't get this, either.
See reply to previous email.
Didn't help me too much :(
Quoted 6 lines
Quoted 1 line
The PTP interface supports poll/read just fine already.
Yes that's why I wanted to re-use it. As it currently is, it's not going to work for reasons I explained in previous follow up yesterday:
You said:
one user space process would have to read timestamps for all channels/clients and then leave it up to user space IPC to get them to other processes, which isn't great.
I disagree. I think having tons of fifos isn't great.
Ideally, there will be kernel consumers for most of the channels, and they will forward the time stamps in a way appropriate to their subsystem. For example, network devices will use so_timestamping. Any "leftover" channels can go through the generic PTP interface.
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 6 lines · Richard Cochran
Quoted 4 lines · Jonathan Richardson
ptp_clock_adjtime()casts it to an unsigned and returns an error:if ((unsigned long) ts.tv_nsec >=
NSEC_PER_SEC)return -EINVAL;
Quoted 8 lines
The value of a timeval is the sum of its fields, but the field
tv_usecmust always be non-negative. Thetv_secfield can be negative. So, your application simply needs to do this:if (tx.time.tv_usec < 0) { tx.time.tv_sec -= 1; tx.time.tv_usec += 1000000000; }
That works, thanks.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
IRQ interval: I mentioned before that we may be able to calculate the isochronous interrupt rate automatically but this isn't possible because the DTE doesn't know the frequency of the clients. One solution is to use the '
PTP_PEROUT_REQUEST' ioctl to set a periodic timer frequency. Not really a timer but good enough for our purposes.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
As I said in my other reply, I don't understand what the problem is.
See reply to previous email. We can use this ioctl or add a new one as Arnd suggested. It doesn't matter to me.
Still makes no sense. Why should the interrupt rate depend on the clock frequency?
The isochronous interrupt rate is just a way to poll the hardware FIFO for timestamps and transfer them into a s/w FIFO where they're available to be read from user space.
The rate depends on how frequently clients will be generating timestamps at and the driver doesn't know that. That's why we set it from user space. If the interrupt rate is too fast it could bog down the system, and it can be very fast. The highest rate is almost always going to be too fast. If it's set too slow then there will be FIFO overflows. Since only user space knows the frequency of the input channel, it's up to the user to set an appropriate rate. A good rate may be twice the frequency of the highest frequency client. Since what clients are being used at runtime is unknown to us and will vary from system to system we really don't know what to set this rate to. One user may only be interested in audio, another may only be interested in something connected to a GPIO. One user may be running audio at a fast sample rate, another very slow, etc.
The only way I can see it working is if we enabled a client (external time stamping channel in PTP) with an expected frequency, then set the IRQ interval to some pre-determined rate (ie- 2x) and then adjust the rate as new clients are enabled and divider values are set.
Quoted 3 lines
If the ISR is delivering batches of time stamps, then the interrupt rate aught to be the highest rate that the system can support.
It to nanosecond precision so it can be ridiculously quick.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Set divider: There is no ability to set a frequency or do anything to a channel. We could re-use the
PTP_EXTTS_REQUESTioctl but extend 'structptp_extts_request' by using the reserved field rsv to allow setting an integer value representing either a frequency or divider value in our case - some value to configure a channel. A new flag would have to be added to the existingPTP_ENABLE_FEATURE, RISING and FALLING EDGE.
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
I don't get this, either.
See reply to previous email.
Didn't help me too much :(
Hope the description above helped. Timestamping can be controlled from user space. Since we don't know which clients are being or the frequency of the input, we don't know what to set the divider to. The user knows what they have hooked up to a channel, the rate of it, and the rate they want timestamps generated at (divided down to).
Quoted 8 lines
Quoted 7 lines
Quoted 2 lines
The PTP interface supports poll/read just fine already.
Yes that's why I wanted to re-use it. As it currently is, it's not going to work for reasons I explained in previous follow up yesterday:
Quoted 7 lines
You said:
one user space process would have to read timestamps for all channels/clients and then leave it up to user space IPC to get them to other processes, which isn't great.
I disagree. I think having tons of fifos isn't great.
Maybe an example will help. We have user space process A interested in channel 1, and B in channel 2. A timestamp for channel 2 is in the FIFO (the FIFO holds timestamps for all channels). What happens if client A does a read for a timestamp and it's for channel 2? The timestamp has been pulled out of the s/w FIFO in the driver. What do we do with it now?
Having separate FIFO's allows process A to only retrieve channel 1 timestamps.
Our original non PTP driver ioctl (DTE_GET_TIMESTAMP) solved that problem because we could specify a channel. We're now trying to adapt it to PTP so we don't have to write a new "DTE" user and kernel side API.
Quoted 5 lines
Ideally, there will be kernel consumers for most of the channels, and they will forward the time stamps in a way appropriate to their subsystem. For example, network devices will use
so_timestamping. Any "leftover" channels can go through the generic PTP interface.
I'll look more into so_timestamping to see how that's used but we wanted one generic interface to get timestamps from channels because anything can be hooked up to a channel.
Quoted 5 lines
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 5 lines · Jonathan Richardson
Quoted 2 lines · Richard Cochran
If the ISR is delivering batches of time stamps, then the interrupt rate aught to be the highest rate that the system can support.
It to nanosecond precision so it can be ridiculously quick.
Your interrupt rate directly controls the worst case delay between the time the event occurs and the time the application (or stack) obtains the time stamp. You want this delay to be as short as possible, but you cannot afford to bog down the entire operating system. I would suggest a fixed value of 1000 Hz (with perhaps a debugfs knob).
What a poor hardware design. Oh well.
Quoted 2 lines
Having separate FIFO's allows process A to only retrieve channel 1 timestamps.
Having lots of different processes reading time stamps directly, and trying to match them up with various HW events after the fact, is not way to go. Instead, kernel time stamp consumers should pair the time stamps with the associated events and then provide the time information conveniently over the existing APIs. (See also my last comment.)
Quoted 3 lines
Our original non PTP driver ioctl (
DTE_GET_TIMESTAMP) solved that problem because we could specify a channel. We're now trying to adapt it to PTP so we don't have to write a new "DTE" user and kernel side API.
For mainline, we want the best interface possible. Sometimes that means that programs based on out-of-tree interfaces will need to be changed.
Quoted 8 lines
Quoted 4 lines
Ideally, there will be kernel consumers for most of the channels, and they will forward the time stamps in a way appropriate to their subsystem. For example, network devices will use
so_timestamping. Any "leftover" channels can go through the generic PTP interface.I'll look more into
so_timestampingto see how that's used but we wanted one generic interface to get timestamps from channels because anything can be hooked up to a channel.
I definitely do *not* want one generic interface. The task of matching time stamps with hardware events belongs to the kernel. For random GPIOs, the existing PTP ioctl is plenty good enough, but for other devices (network, audio) more work is needed.
One huge lacuna in your patch series is the code that associates the time stamps with events. How is this supposed to work?
So far you have:
+enum dte_client {
+ DTE_CLIENT_MIN = 0, + DTE_CLIENT_I2S0_BITCLOCK = 0, + DTE_CLIENT_I2S1_BITCLOCK, + DTE_CLIENT_I2S2_BITCLOCK, + DTE_CLIENT_I2S0_WORDCLOCK, + DTE_CLIENT_I2S1_WORDCLOCK, + DTE_CLIENT_I2S2_WORDCLOCK, + DTE_CLIENT_LCD_CLFP, + DTE_CLIENT_LCD_CLLP, + DTE_CLIENT_GPIO14, + DTE_CLIENT_GPIO15, + DTE_CLIENT_GPIO22, + DTE_CLIENT_GPIO23, + DTE_CLIENT_MAX,
+};
Network devices are not present at all. No idea why LCD signals are included. For the i2s, this appears to stamp the audio clock. But which audio sample has been stamped? Or do you only care about frequency and not phase?
For the next round, please include John Stulz, Thomas Gleixner, netdev, and the appropriate audio list on CC.
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 38 lines · Richard Cochran
Quoted 5 lines · Jonathan Richardson
Quoted 2 lines · Richard Cochran
If the ISR is delivering batches of time stamps, then the interrupt rate aught to be the highest rate that the system can support.
It to nanosecond precision so it can be ridiculously quick.
Your interrupt rate directly controls the worst case delay between the time the event occurs and the time the application (or stack) obtains the time stamp. You want this delay to be as short as possible, but you cannot afford to bog down the entire operating system. I would suggest a fixed value of 1000 Hz (with perhaps a debugfs knob).
What a poor hardware design. Oh well.
Quoted 2 lines
Having separate FIFO's allows process A to only retrieve channel 1 timestamps.
Having lots of different processes reading time stamps directly, and trying to match them up with various HW events after the fact, is not way to go. Instead, kernel time stamp consumers should pair the time stamps with the associated events and then provide the time information conveniently over the existing APIs. (See also my last comment.)
Quoted 3 lines
Our original non PTP driver ioctl (
DTE_GET_TIMESTAMP) solved that problem because we could specify a channel. We're now trying to adapt it to PTP so we don't have to write a new "DTE" user and kernel side API.For mainline, we want the best interface possible. Sometimes that means that programs based on out-of-tree interfaces will need to be changed.
Quoted 5 lines
Quoted 4 lines
Ideally, there will be kernel consumers for most of the channels, and they will forward the time stamps in a way appropriate to their subsystem. For example, network devices will use
so_timestamping. Any "leftover" channels can go through the generic PTP interface.
Quoted 8 lines
Quoted 3 lines
I'll look more into
so_timestampingto see how that's used but we wanted one generic interface to get timestamps from channels because anything can be hooked up to a channel.I definitely do *not* want one generic interface. The task of matching time stamps with hardware events belongs to the kernel. For random GPIOs, the existing PTP ioctl is plenty good enough, but for other devices (network, audio) more work is needed.
Richard, this design isn't going to work. We need to have both kernel and user space consumers. We don't want all GPIO's in a common timestamp buffer either, as it presents problems I mentioned previously. Currently the network input is a gpio. After some discussion here I think we'll have to keep this driver out of the kernel for now.
Thanks.
Quoted 34 lines
One huge lacuna in your patch series is the code that associates the time stamps with events. How is this supposed to work?
So far you have:
(quoted diff) +16
-0+enum dte_client { + DTE_CLIENT_MIN = 0, + DTE_CLIENT_I2S0_BITCLOCK = 0, + DTE_CLIENT_I2S1_BITCLOCK, + DTE_CLIENT_I2S2_BITCLOCK, + DTE_CLIENT_I2S0_WORDCLOCK, + DTE_CLIENT_I2S1_WORDCLOCK, + DTE_CLIENT_I2S2_WORDCLOCK, + DTE_CLIENT_LCD_CLFP, + DTE_CLIENT_LCD_CLLP, + DTE_CLIENT_GPIO14, + DTE_CLIENT_GPIO15, + DTE_CLIENT_GPIO22, + DTE_CLIENT_GPIO23, + DTE_CLIENT_MAX, +};Network devices are not present at all. No idea why LCD signals are included. For the i2s, this appears to stamp the audio clock. But which audio sample has been stamped? Or do you only care about frequency and not phase?
For the next round, please include John Stulz, Thomas Gleixner, netdev, and the appropriate audio list on CC.
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 3 lines · Jonathan Richardson
Richard, this design isn't going to work. We need to have both kernel and user space consumers.
But you did not implement even a single kernel consumer.
Quoted 3 lines
We don't want all GPIO's in a common timestamp
buffer either, as it presents problems I mentioned previously. Currently the network input is a gpio.
That won't work. Consider what happens when the MAC drops a packet.
Quoted 2 lines
After some discussion here I think we'll have to keep this driver out of the kernel for now.
Fine with me.
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 11 lines · Richard Cochran
Quoted 3 lines · Jonathan Richardson
Richard, this design isn't going to work. We need to have both kernel and user space consumers.
But you did not implement even a single kernel consumer.
Quoted 3 lines
We don't want all GPIO's in a common timestamp
buffer either, as it presents problems I mentioned previously. Currently the network input is a gpio.
Quoted 6 lines
That won't work. Consider what happens when the MAC drops a packet.
Quoted 2 lines
After some discussion here I think we'll have to keep this driver out of the kernel for now.
Fine with me.
We'll keep this discussion and try to figure out the best interface keeping in mind what you've said as we move further along with it. Not a wasted discussion. Requirements of this need to be better defined and thought out also.
Thanks,
Jon
Quoted 4 lines
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
In reply to Jonathan RichardsonQuoted 6 lines · Jonathan Richardson
The DTE creates timestamps of hardware based events such as GPIO, I2S signals for audio, etc. It was also intended to provide 802.1AS / PTP timestamps for network packets. The h/w has up to 32 "clients" -- the hardware inputs into a timestamping engine. These clients are specific to the chip the DTE is used on. For Cygnus you can see what they are in our 'enum
dte_client' frombcm_cygnus_dte.h.
These 32 channels should either go to kernel consumers (i2s audio) or, if there aren't any, to the generic PTP ioctl.
Quoted 2 lines
The DTE timestamper creates timestamps based on the current clock wall time.
What do you mean by "wall time"? CLOCK_REALTIME?
Quoted 4 lines
When an event occurs it stores the timestamp in a h/w FIFO. Each client also has a divider that can be set to control the rate that timestamps are generated at by the timestamper and these are adjustable at run time.
This does not make sense. If you time stamp events, then the rate is determined by the events themselves.
Quoted 7 lines
It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP plus timestamping 32 other hardware inputs that can be enabled at any time with timestamps being generated at varying frequencies. As clients are enabled that generate timestamps at higher frequencies, the isochronous interrupt frequency needs to be increased so that overflows in the the h/w and s/w FIFO's don't occur (this frequency could possibly be automated instead of changing it manually as we currently do).
Yes, the driver should configure an appropriate rate automatically.
Quoted 6 lines
It looks like the driver could almost be a PTP driver instead of a char driver controlled with ioctls. PTP does this already using
clock_gettime(),clock_settime(),clock_adjtime(). But we want to set the frequency as well as adjust the clock and I don't see how that is possible with the stripped down timex data passed to the driver fromptp_clock_adjtime().
You can convert ppb into your time base using simple math.
Quoted 7 lines
We have the additional requirement of controlling multiple clients and retrieving the timestamps, etc. The PTP driver allows for some control of external time stamp channels already using '
n_ext_ts' in structptp_clock_info. We could use that to enable clients and get timestamps, but we also need to be able to change dividers for the clients at run time if desired. It doesn't look like additional ioctls could be passed to a PTP driver becauseptp_ioctl()is the ioctl handler.
I don't see any need for use space to set dividers. Let the driver do that.
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Richard, glad you retrieved our driver. Comments below.
Quoted 8 lines · Richard Cochran
Quoted 6 lines · Jonathan Richardson
The DTE creates timestamps of hardware based events such as GPIO, I2S signals for audio, etc. It was also intended to provide 802.1AS / PTP timestamps for network packets. The h/w has up to 32 "clients" -- the hardware inputs into a timestamping engine. These clients are specific to the chip the DTE is used on. For Cygnus you can see what they are in our 'enum
dte_client' frombcm_cygnus_dte.h.
Quoted 7 lines
These 32 channels should either go to kernel consumers (i2s audio) or, if there aren't any, to the generic PTP ioctl.
Quoted 2 lines
The DTE timestamper creates timestamps based on the current clock wall time.
What do you mean by "wall time"?
CLOCK_REALTIME?
It's just the incrementing local NCO time. Whatever the value of it is is the timestamp time. When we add the timestamp to the s/w FIFO we adjust the time to a reference time.
Quoted 8 lines
Quoted 5 lines
When an event occurs it stores the timestamp in a h/w FIFO. Each client also has a divider that can be set to control the rate that timestamps are generated at by the timestamper and these are adjustable at run time.
This does not make sense. If you time stamp events, then the rate is determined by the events themselves.
The input events may occur at a rate greater than we want to generate timestamps at. Maybe we need a better term than divider. The h/w sees x input signals over x period of time and then samples the input signal at a divided down period of time to limit the timestamp rate. Timestamp input sample rate?
Quoted 10 lines
Quoted 8 lines
It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP plus timestamping 32 other hardware inputs that can be enabled at any time with timestamps being generated at varying frequencies. As clients are enabled that generate timestamps at higher frequencies, the isochronous interrupt frequency needs to be increased so that overflows in the the h/w and s/w FIFO's don't occur (this frequency could possibly be automated instead of changing it manually as we currently do).
Yes, the driver should configure an appropriate rate automatically.
I was wrong. It can't. The DTE doesn't know anything about the inputs hooked up to it nor the frequency of the input events. It needs to be set by user space and the value can change at run time whenever we want to change it.
Quoted 9 lines
Quoted 7 lines
It looks like the driver could almost be a PTP driver instead of a char driver controlled with ioctls. PTP does this already using
clock_gettime(),clock_settime(),clock_adjtime(). But we want to set the frequency as well as adjust the clock and I don't see how that is possible with the stripped down timex data passed to the driver fromptp_clock_adjtime().You can convert ppb into your time base using simple math.
Yes, this isn't a problem.
Quoted 11 lines
Quoted 8 lines
We have the additional requirement of controlling multiple clients and retrieving the timestamps, etc. The PTP driver allows for some control of external time stamp channels already using '
n_ext_ts' in structptp_clock_info. We could use that to enable clients and get timestamps, but we also need to be able to change dividers for the clients at run time if desired. It doesn't look like additional ioctls could be passed to a PTP driver becauseptp_ioctl()is the ioctl handler.I don't see any need for use space to set dividers. Let the driver do that.
See above.
Thanks.
Quoted 4 lines
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
Quoted 5 lines · Jonathan Richardson
The input events may occur at a rate greater than we want to generate timestamps at. Maybe we need a better term than divider. The h/w sees x input signals over x period of time and then samples the input signal at a divided down period of time to limit the timestamp rate. Timestamp input sample rate?
Uhm, you mean time stamp resolution?
Or discarding events?
-EDOESNOTPARSE;
Thanks,
Richard
Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus
In reply to Richard CochranRichard, glad you retrieved our driver. Comments below.
Quoted 8 lines · Richard Cochran
Quoted 6 lines · Jonathan Richardson
The DTE creates timestamps of hardware based events such as GPIO, I2S signals for audio, etc. It was also intended to provide 802.1AS / PTP timestamps for network packets. The h/w has up to 32 "clients" -- the hardware inputs into a timestamping engine. These clients are specific to the chip the DTE is used on. For Cygnus you can see what they are in our 'enum
dte_client' frombcm_cygnus_dte.h.
Quoted 7 lines
These 32 channels should either go to kernel consumers (i2s audio) or, if there aren't any, to the generic PTP ioctl.
Quoted 2 lines
The DTE timestamper creates timestamps based on the current clock wall time.
What do you mean by "wall time"?
CLOCK_REALTIME?
It's just the incrementing local NCO time. Whatever the value of it is is the timestamp time. When we add the timestamp to the s/w FIFO we adjust the time to a reference time.
Quoted 8 lines
Quoted 5 lines
When an event occurs it stores the timestamp in a h/w FIFO. Each client also has a divider that can be set to control the rate that timestamps are generated at by the timestamper and these are adjustable at run time.
This does not make sense. If you time stamp events, then the rate is determined by the events themselves.
The input events may occur at a rate greater than we want to generate timestamps at. Maybe we need a better term than divider. The h/w sees x input signals over x period of time and then samples the input signal at a divided down period of time to limit the timestamp rate. Timestamp input sample rate?
Quoted 10 lines
Quoted 8 lines
It's a bit more than a PTP hardware clock on a NIC. It's a clock for PTP plus timestamping 32 other hardware inputs that can be enabled at any time with timestamps being generated at varying frequencies. As clients are enabled that generate timestamps at higher frequencies, the isochronous interrupt frequency needs to be increased so that overflows in the the h/w and s/w FIFO's don't occur (this frequency could possibly be automated instead of changing it manually as we currently do).
Yes, the driver should configure an appropriate rate automatically.
I was wrong. It can't. The DTE doesn't know anything about the inputs hooked up to it nor the frequency of the input events. It needs to be set by user space and the value can change at run time whenever we want to change it.
Quoted 9 lines
Quoted 7 lines
It looks like the driver could almost be a PTP driver instead of a char driver controlled with ioctls. PTP does this already using
clock_gettime(),clock_settime(),clock_adjtime(). But we want to set the frequency as well as adjust the clock and I don't see how that is possible with the stripped down timex data passed to the driver fromptp_clock_adjtime().You can convert ppb into your time base using simple math.
Yes, this isn't a problem.
Quoted 11 lines
Quoted 8 lines
We have the additional requirement of controlling multiple clients and retrieving the timestamps, etc. The PTP driver allows for some control of external time stamp channels already using '
n_ext_ts' in structptp_clock_info. We could use that to enable clients and get timestamps, but we also need to be able to change dividers for the clients at run time if desired. It doesn't look like additional ioctls could be passed to a PTP driver becauseptp_ioctl()is the ioctl handler.I don't see any need for use space to set dividers. Let the driver do that.
See above.
Thanks.
Quoted 4 lines
Thanks,
Richard