[PATCH 0/2] Add DTE driver for Cygnus

[PATCH 0/2] Add DTE driver for Cygnus

[Jonathan Richardson]

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

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] misc: Add DT binding for cygnus Digital Timing Engine (DTE) driver.

[Jonathan Richardson]

Reviewed-by: Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Tested-by: Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jonathan Richardson <jonathar-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 .../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

diff --git a/Documentation/devicetree/bindings/arm/bcm/brcm,cygnus-dte.txt b/Documentation/devicetree/bindings/arm/bcm/brcm,cygnus-dte.txt
new file mode 100644
index 0000000..f30ca86
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/bcm/brcm,cygnus-dte.txt
@@ -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>;
+
+	};
+};
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Jonathan Richardson]

Reviewed-by: Scott Branden <sbranden@broadcom.com>
Tested-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
---
 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

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 006242c..2a223c3 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -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"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 7d5c4cd..4067b95 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -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
diff --git a/drivers/misc/bcm_cygnus_dte.c b/drivers/misc/bcm_cygnus_dte.c
new file mode 100644
index 0000000..8bf4f93
--- /dev/null
+++ b/drivers/misc/bcm_cygnus_dte.c
@@ -0,0 +1,927 @@
+/*
+ * Copyright 2015 Broadcom Corporation.  All rights reserved.
+ *
+ * Unless you and Broadcom execute a separate written software license
+ * agreement governing use of this software, this software is licensed to you
+ * under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+/*
+ * This driver implements the Broadcom DTE Digital Timing Engine Driver (DTE).
+ * The DTE allows for hardware time stamping of network packets, audio/video
+ * samples and/or GPIO inputs in a common adjustable time base.
+ */
+
+#include <linux/hw_random.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/fs.h>
+#include <linux/cdev.h>
+#include <linux/printk.h>
+#include <linux/delay.h>
+#include <linux/kfifo.h>
+#include <linux/interrupt.h>
+#include <linux/uaccess.h>
+#include <linux/bcm_cygnus_dte.h>
+
+#define DTE_SW_FIFO_SIZE 64
+#define DTE_HW_FIFO_SIZE 16
+#define DTE_MAX_DEVS 1
+#define DTE_DEVICE_NAME	"dte"
+
+/* Registers */
+#define DTE_CTRL_REG_BASE                        0x600
+#define DTE_CTRL_REG__INTERRUPT                  16
+#define DTE_NEXT_SOI_REG_BASE                    0x610
+#define DTE_ILEN_REG_BASE                        0x614
+#define DTE_LTS_FIFO_REG_BASE                    0x640
+#define DTE_LTS_CSR_REG_BASE                     0x644
+#define DTE_LTS_CSR_REG__FIFO_EMPTY              4
+#define DTE_LTS_CSR_REG__FIFO_OVERFLOW           3
+#define DTE_LTS_CSR_REG__FIFO_UNDERFLOW          2
+#define DTE_NCO_LOW_TIME_REG_BASE                0x650
+#define DTE_NCO_TIME_REG_BASE                    0x654
+#define DTE_NCO_OVERFLOW_REG_BASE                0x658
+#define DTE_NCO_INC_REG_BASE                     0x65c
+#define DTE_LTS_DIV_54_REG_BASE                  0x660
+#define DTE_LTS_DIV_76_REG_BASE                  0x664
+#define DTE_LTS_DIV_98_REG_BASE                  0x668
+#define DTE_LTS_DIV_1110_REG_BASE                0x66c
+#define DTE_LTS_DIV_1312_REG_BASE                0x670
+#define DTE_LTS_DIV_14_REG_BASE                  0x674
+#define DTE_LTS_DIV_MASK                         0xffff
+#define DTE_LTS_DIV_54_REG__DIV_VAL_4_R          0
+#define DTE_LTS_DIV_54_REG__DIV_VAL_5_R          16
+#define DTE_LTS_DIV_76_REG__DIV_VAL_6_R          0
+#define DTE_LTS_DIV_76_REG__DIV_VAL_7_R          16
+#define DTE_LTS_DIV_98_REG__DIV_VAL_8_R          0
+#define DTE_LTS_DIV_98_REG__DIV_VAL_9_R          16
+#define DTE_LTS_DIV_1110_REG__DIV_VAL_10_R       0
+#define DTE_LTS_DIV_1110_REG__DIV_VAL_11_R       16
+#define DTE_LTS_DIV_1312_REG__DIV_VAL_12_R       0
+#define DTE_LTS_DIV_1312_REG__DIV_VAL_13_R       16
+#define DTE_LTS_DIV_14_REG__DIV_VAL_15_R         0
+#define DTE_LTS_DIV_14_REG__DIV_VAL_14_R         16
+#define DTE_LTS_SRC_EN_REG_BASE                  0x680
+#define DTE_INTR_STATUS_REG                      0x6A0
+#define DTE_INTR_STATUS_ISO_INTR_SHIFT           0
+#define DTE_INTR_STATUS_ISO_INTR_MASK            0x1
+#define DTE_INTR_STATUS_FIFO_LEVEL_INTR_SHIFT    1
+#define DTE_INTR_STATUS_FIFO_LEVEL_INTR_MASK     0x7
+#define DTE_INTR_STATUS_TIMEOUT_INTR_SHIFT       4
+#define DTE_INTR_STATUS_TIMEOUT_INTR_MASK        0x1
+#define DTE_INTR_MASK_REG                        0x6A4
+#define ISO_INTR_MASK_SHIFT                      0
+
+/*
+ * There are 3 time registers in the DTE block;
+ * NCO_OVERFLOW[7:0] (sum3)
+ * NCO_TIME[31:0] (sum2)
+ * NCO_LOW_TIME[31:0] (sum1).
+ * These can be considered as a 72 bit overall timestamp[71:0]
+ * in ns with a format of 44.28
+ *
+ * The DTE block runs at 125MHz, ie; every 8ns,
+ * NCO_INC is added to the timestamp[71:0].
+ * NCO_INC represents fractional ns in 4.28 format.
+ *
+ * The DTE client Timstamps have a resoultion of ns and is constructed
+ * from the 28 LS bits of sum2 and 4 MS bits of sum1.
+ *
+ * Register            |     sum3    |          sum2     |         sum1       |
+ *                     |7           0|31  28            0|31    28:27        0|
+ * Timestamp[71:0]     |71                                      28:27        0|
+ * NCO_INC[31:0]                                         |31    28:27        0|
+ * DTE client TS[31:0]                      |31                  0|
+ */
+
+/* NCO nominal increment = 8ns DTE operates at 125 MHz */
+#define NCO_INC_NOMINAL 0x80000000
+
+struct bcm_cygnus_dte {
+	struct list_head node;
+	struct platform_device *pdev;
+	struct cdev dte_cdev;
+	struct class *dte_class;
+	struct kfifo recv_fifo[DTE_CLIENT_MAX];
+	dev_t  devt;
+	uint32_t fifoof;
+	uint32_t fifouf;
+	uint32_t kfifoof[DTE_CLIENT_MAX];
+	spinlock_t lock;
+	struct mutex mutex;
+	struct timespec ts_ref;           /* Reference base timespec */
+	uint32_t timestamp_overflow_last; /* Last timestamp overflow */
+	void __iomem *audioeav;
+};
+
+static LIST_HEAD(dtedev_list);
+
+struct dte_client_mapping {
+	enum dte_client client_index;
+	int lts_index;
+	uint32_t reg_offset;
+	int shift;
+};
+
+#define DTE_CLIENT_MAPPING_ENTRY(client, lts_index, regbase) \
+	{ DTE_CLIENT_##client, \
+	  lts_index,\
+	  DTE_LTS_##regbase##_BASE, \
+	  DTE_LTS_##regbase##__DIV_VAL_##lts_index##_R }
+
+static const struct dte_client_mapping dte_client_map_table[DTE_CLIENT_MAX] = {
+	DTE_CLIENT_MAPPING_ENTRY(I2S0_BITCLOCK,  4, DIV_54_REG),
+	DTE_CLIENT_MAPPING_ENTRY(I2S1_BITCLOCK,  5, DIV_54_REG),
+	DTE_CLIENT_MAPPING_ENTRY(I2S2_BITCLOCK,  6, DIV_76_REG),
+	DTE_CLIENT_MAPPING_ENTRY(I2S0_WORDCLOCK, 7, DIV_76_REG),
+	DTE_CLIENT_MAPPING_ENTRY(I2S1_WORDCLOCK, 8, DIV_98_REG),
+	DTE_CLIENT_MAPPING_ENTRY(I2S2_WORDCLOCK, 9, DIV_98_REG),
+	DTE_CLIENT_MAPPING_ENTRY(LCD_CLFP,      10, DIV_1110_REG),
+	DTE_CLIENT_MAPPING_ENTRY(LCD_CLLP,      11, DIV_1110_REG),
+	DTE_CLIENT_MAPPING_ENTRY(GPIO14,        12, DIV_1312_REG),
+	DTE_CLIENT_MAPPING_ENTRY(GPIO15,        13, DIV_1312_REG),
+	DTE_CLIENT_MAPPING_ENTRY(GPIO22,        14, DIV_14_REG),
+	DTE_CLIENT_MAPPING_ENTRY(GPIO23,        15, DIV_14_REG)
+};
+
+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) {
+		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);
+
+int dte_enable_timestamp(
+		struct bcm_cygnus_dte *cygnus_dte,
+		enum dte_client client,
+		int enable)
+{
+	int src_ena;
+
+	if (!cygnus_dte)
+		return -EINVAL;
+
+	if (client < DTE_CLIENT_MIN || client >= DTE_CLIENT_MAX)
+		return -EINVAL;
+
+	spin_lock(&cygnus_dte->lock);
+
+	src_ena = readl(cygnus_dte->audioeav +
+		DTE_LTS_SRC_EN_REG_BASE);
+	if (enable)
+		src_ena |= BIT(dte_client_map_table[client].lts_index);
+	else
+		src_ena &= ~BIT(dte_client_map_table[client].lts_index);
+
+	writel(src_ena,
+		cygnus_dte->audioeav + DTE_LTS_SRC_EN_REG_BASE);
+
+	spin_unlock(&cygnus_dte->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(dte_enable_timestamp);
+
+int dte_set_client_divider(
+		struct bcm_cygnus_dte *cygnus_dte,
+		enum dte_client client,
+		int divider)
+{
+	int lts_div;
+
+	if (!cygnus_dte)
+		return -EINVAL;
+
+	if (client < DTE_CLIENT_MIN || client >= DTE_CLIENT_MAX)
+		return -EINVAL;
+
+	/* Divider not supported for Divider 15 (GPIO23) */
+	if (client == DTE_CLIENT_GPIO23)
+		return -EINVAL;
+
+	/* Check for maximum divider size */
+	if (divider > DTE_LTS_DIV_MASK)
+		return -EINVAL;
+
+	spin_lock(&cygnus_dte->lock);
+
+	lts_div = readl(cygnus_dte->audioeav +
+			dte_client_map_table[client].reg_offset);
+	lts_div &= ~(DTE_LTS_DIV_MASK << dte_client_map_table[client].shift);
+	lts_div |= (divider << dte_client_map_table[client].shift);
+	writel(lts_div, cygnus_dte->audioeav +
+		dte_client_map_table[client].reg_offset);
+
+	spin_unlock(&cygnus_dte->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(dte_set_client_divider);
+
+int dte_set_irq_interval(
+		struct bcm_cygnus_dte *cygnus_dte,
+		uint32_t nanosec)
+{
+	int next_soi;
+	int current_time;
+	int intr_mask;
+
+	if (!cygnus_dte)
+		return -EINVAL;
+
+	spin_lock(&cygnus_dte->lock);
+
+	/*
+	 * To ensure proper operation of the isochronous time interval
+	 * generation (ITG) timing pulse requires programming of
+	 * 1) Next Start of Interrupt Time (ns) (NEXT_SOI)
+	 * 2) Interval Length (ns) (ILEN)
+	 * The block first compares the value of the NEXT_SOI with
+	 * that of the 29-bit value of time DTE_NCO_TIME_REG_BASE
+	 * that has been left-shifted by 4 or multiplied by 16 for
+	 * generating the first interrupt.  Thereafter upon completion
+	 * of every ILEN number of nano-seconds it generates an ITG
+	 * interrupt and the NEXT_SOI register is auto-incremented by ILEN
+	 */
+
+	/* Get the current time (sum2) (units of 16ns) */
+	current_time = readl(cygnus_dte->audioeav + DTE_NCO_TIME_REG_BASE);
+
+	/*
+	 * Set the Start of Next Interval (units of ns) to trigger on next
+	 * interval
+	 */
+	next_soi = (current_time << 4) + (nanosec);
+	writel(next_soi, cygnus_dte->audioeav + DTE_NEXT_SOI_REG_BASE);
+
+	/* configure interval length (units of ns) */
+	writel(nanosec, cygnus_dte->audioeav + DTE_ILEN_REG_BASE);
+
+	/* enable isochronous interrupt */
+	intr_mask = readl(cygnus_dte->audioeav + DTE_INTR_MASK_REG);
+	intr_mask &= ~(1 << ISO_INTR_MASK_SHIFT);
+	writel(intr_mask, cygnus_dte->audioeav + DTE_INTR_MASK_REG);
+
+	spin_unlock(&cygnus_dte->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(dte_set_irq_interval);
+
+int dte_get_timestamp(
+		struct bcm_cygnus_dte *cygnus_dte,
+		enum dte_client client,
+		struct timespec *ts)
+{
+
+	if (!cygnus_dte)
+		return -EINVAL;
+
+	if (client < DTE_CLIENT_MIN || client >= DTE_CLIENT_MAX)
+		return -EINVAL;
+
+	if (kfifo_out(
+		&cygnus_dte->recv_fifo[client],
+		ts, sizeof(struct timespec)) == 0)
+		return -EPERM;
+
+	return 0;
+}
+EXPORT_SYMBOL(dte_get_timestamp);
+
+int dte_set_time(
+		struct bcm_cygnus_dte *cygnus_dte,
+		struct timespec *ts)
+{
+	int nco_incr;
+
+	if (!cygnus_dte)
+		return -EINVAL;
+
+	spin_lock(&cygnus_dte->lock);
+
+	/* Disable NCO Increment */
+	nco_incr = readl(cygnus_dte->audioeav + DTE_NCO_INC_REG_BASE);
+	writel(0, cygnus_dte->audioeav + DTE_NCO_INC_REG_BASE);
+
+	/* Reset Timers */
+	writel(0, cygnus_dte->audioeav + DTE_NCO_LOW_TIME_REG_BASE);
+	writel(0, cygnus_dte->audioeav + DTE_NCO_TIME_REG_BASE);
+	writel(0, cygnus_dte->audioeav + DTE_NCO_OVERFLOW_REG_BASE);
+
+	/* Initialize last overflow value to track wrap condition */
+	cygnus_dte->timestamp_overflow_last = 0;
+
+	/* Initialize Reference timespec */
+	cygnus_dte->ts_ref = *ts;
+
+	/* re-enable nco increment */
+	writel(nco_incr, cygnus_dte->audioeav + DTE_NCO_INC_REG_BASE);
+
+	spin_unlock(&cygnus_dte->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(dte_set_time);
+
+int dte_get_time(
+		struct bcm_cygnus_dte *cygnus_dte,
+		struct timespec *ts)
+{
+	uint32_t current_time_sum1;
+	uint32_t current_time_sum2;
+	uint32_t current_time_sum3;
+	uint32_t timestamp_overflow;
+	uint64_t ns;
+
+	if (!cygnus_dte)
+		return -EINVAL;
+
+	spin_lock(&cygnus_dte->lock);
+
+	/* Read Timers */
+	current_time_sum1 = readl(cygnus_dte->audioeav +
+				DTE_NCO_LOW_TIME_REG_BASE);
+	current_time_sum2 = readl(cygnus_dte->audioeav +
+				DTE_NCO_TIME_REG_BASE);
+	current_time_sum3 = readl(cygnus_dte->audioeav +
+				DTE_NCO_OVERFLOW_REG_BASE);
+/*
+ * Register            |     sum3    |          sum2     |         sum1       |
+ *                     |7           0|31  28            0|31    28:27        0|
+ * Timestamp[71:0]     |71                                      28:27        0|
+ */
+
+	/* Current time in units of ns */
+	ns = (((uint64_t)(current_time_sum3 & 0xff) << 36) |
+		((uint64_t)current_time_sum2 << 4) |
+		(uint64_t)((current_time_sum1 >> 28) & 0xf));
+
+	/*
+	 * Determined if wraparound has occurred
+	 * Timestamp overflow only includes the bottom 8 bits of sum3
+	 * and the top 4 bits of sum2
+	 * Units of 2^32 ns = 4.294967296 sec
+	 */
+	timestamp_overflow = (ns >> 32) & 0xfff;
+	if (timestamp_overflow < cygnus_dte->timestamp_overflow_last)
+		/*
+		 * Wrap around has occurred but not yet reflected in
+		 * the reference timespec
+		 * Full wrap around amount is 44bits in ns
+		 * Precisely 17592.186044416 sec = ~4.887 hrs
+		 */
+		ns += (uint64_t)0x1<<44;
+
+	/* Convert current timestamp(ns) to timespec */
+	*ts = ns_to_timespec(ns);
+
+	/* Add the current time to the reference time */
+	*ts = timespec_add(cygnus_dte->ts_ref, *ts);
+
+	spin_unlock(&cygnus_dte->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(dte_get_time);
+
+int dte_adj_time(
+		struct bcm_cygnus_dte *cygnus_dte,
+		int64_t delta)
+{
+	struct timespec ts_delta;
+
+	if (!cygnus_dte)
+		return -EINVAL;
+
+	ts_delta = ns_to_timespec(delta);
+
+	spin_lock(&cygnus_dte->lock);
+
+	/* Update Reference timespec */
+	cygnus_dte->ts_ref = timespec_add(cygnus_dte->ts_ref, ts_delta);
+
+	spin_unlock(&cygnus_dte->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(dte_adj_time);
+
+int dte_adj_freq(
+		struct bcm_cygnus_dte *cygnus_dte,
+		int32_t ppb)
+{
+	uint64_t diff;
+	uint32_t mult = NCO_INC_NOMINAL;
+	uint32_t nco_incr;
+	int neg_adj = 0;
+
+	if (!cygnus_dte)
+		return -EINVAL;
+
+	if (ppb < 0) {
+		ppb = -ppb;
+		neg_adj = 1;
+	}
+
+	diff = mult;
+	diff *= ppb;
+	diff = div_u64(diff, 1000000000ULL);
+
+	nco_incr = neg_adj ? mult - diff : mult + diff;
+
+	spin_lock(&cygnus_dte->lock);
+
+	/* Update NCO Increment */
+	writel(nco_incr, cygnus_dte->audioeav + DTE_NCO_INC_REG_BASE);
+
+	spin_unlock(&cygnus_dte->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(dte_adj_freq);
+
+static int dte_open(struct inode *pnode, struct file *fp)
+{
+	/* look up device info for this device file */
+	struct bcm_cygnus_dte *cygnus_dte = container_of(pnode->i_cdev,
+			struct bcm_cygnus_dte, dte_cdev);
+
+	fp->private_data = cygnus_dte;
+
+	return 0;
+}
+
+static long dte_ioctl(struct file *filep,
+	unsigned int cmd, unsigned long arg)
+{
+	int ret = 0;
+	struct dte_data dte_data;
+	struct dte_timestamp dte_timestamp;
+	struct timespec ts;
+	int64_t delta;
+	int32_t ppb;
+	struct bcm_cygnus_dte *cygnus_dte = filep->private_data;
+
+	mutex_lock(&cygnus_dte->mutex);
+	switch (cmd) {
+	case DTE_IOCTL_SET_DIVIDER:
+		if (copy_from_user(&dte_data, (struct dte_data *)arg,
+				sizeof(struct dte_data))) {
+			ret = -EACCES;
+			goto err_out;
+		}
+		if (dte_set_client_divider(cygnus_dte,
+				dte_data.client, dte_data.data))
+			ret = -EPERM;
+		break;
+
+	case DTE_IOCTL_ENABLE_TIMESTAMP:
+		if (copy_from_user(&dte_data, (struct dte_data *)arg,
+				sizeof(struct dte_data))) {
+			ret = -EACCES;
+			goto err_out;
+		}
+		if (dte_enable_timestamp(cygnus_dte,
+				dte_data.client, dte_data.data))
+			ret = -EPERM;
+		break;
+
+	case DTE_IOCTL_SET_IRQ_INTERVAL:
+		if (copy_from_user(&dte_data, (struct dte_data *)arg,
+				sizeof(struct dte_data))) {
+			ret = -EACCES;
+			goto err_out;
+		}
+		if (dte_set_irq_interval(cygnus_dte, dte_data.data))
+			ret = -EPERM;
+		break;
+
+	case DTE_IOCTL_GET_TIMESTAMP:
+		if (copy_from_user(&dte_timestamp, (struct dte_timestamp *)arg,
+				sizeof(struct dte_timestamp))) {
+			ret = -EACCES;
+			goto err_out;
+		}
+		if (dte_get_timestamp(cygnus_dte,
+				dte_timestamp.client,
+				&dte_timestamp.ts)) {
+			ret = -EPERM;
+			goto err_out;
+		}
+		if (copy_to_user((struct dte_timestamp *)arg, &dte_timestamp,
+				sizeof(struct dte_timestamp))) {
+			ret = -EACCES;
+			goto err_out;
+		}
+		break;
+
+	case DTE_IOCTL_SET_TIME:
+		if (copy_from_user(&ts, (struct timespec *)arg,
+				sizeof(struct timespec))) {
+			ret = -EACCES;
+			goto err_out;
+		}
+		if (dte_set_time(cygnus_dte, &ts))
+			ret = -EPERM;
+		break;
+
+	case DTE_IOCTL_GET_TIME:
+		if (dte_get_time(cygnus_dte, &ts))
+			ret = -EPERM;
+		if (copy_to_user((struct timespec *)arg, &ts,
+				sizeof(struct timespec))) {
+			ret = -EACCES;
+			goto err_out;
+		}
+		break;
+
+	case DTE_IOCTL_ADJ_TIME:
+		if (copy_from_user(&delta, (int64_t *)arg,
+				sizeof(int64_t))) {
+			ret = -EACCES;
+			goto err_out;
+		}
+		if (dte_adj_time(cygnus_dte, delta))
+			ret = -EPERM;
+		break;
+
+	case DTE_IOCTL_ADJ_FREQ:
+		if (copy_from_user(&ppb, (int32_t *)arg,
+				sizeof(int32_t))) {
+			ret = -EACCES;
+			goto err_out;
+		}
+		if (dte_adj_freq(cygnus_dte, ppb))
+			ret = -EPERM;
+		break;
+
+	default:
+		ret = -EINVAL;
+		goto err_out;
+	}
+
+err_out:
+	mutex_unlock(&cygnus_dte->mutex);
+	return ret;
+}
+
+static const struct file_operations dte_cdev_fops = {
+	.open           = dte_open,
+	.unlocked_ioctl = dte_ioctl,
+};
+
+static irqreturn_t bcm_cygnus_dte_isr_threaded(int irq, void *drv_ctx)
+{
+	uint32_t fifo_csr;
+	uint32_t rd_data;
+	uint32_t current_time_sum2;
+	uint32_t current_time_sum3;
+	uint32_t i = 0;
+	uint32_t active_clients;
+	int client;
+	int inlen;
+	struct platform_device *pdev = (struct platform_device *)drv_ctx;
+	struct bcm_cygnus_dte *cygnus_dte;
+	uint32_t timestamp_overflow;
+	uint32_t timestamp_ns;
+	uint64_t ns;
+	struct timespec ts_stamp;
+	uint32_t status;
+
+	cygnus_dte = (struct bcm_cygnus_dte *)platform_get_drvdata(pdev);
+
+	/* clear interrupt bit */
+	writel(1<<DTE_CTRL_REG__INTERRUPT,
+		cygnus_dte->audioeav + DTE_CTRL_REG_BASE);
+
+	/* clear all interrupts in status register */
+	status = (DTE_INTR_STATUS_ISO_INTR_MASK <<
+		DTE_INTR_STATUS_ISO_INTR_SHIFT) |
+		(DTE_INTR_STATUS_FIFO_LEVEL_INTR_MASK <<
+		DTE_INTR_STATUS_FIFO_LEVEL_INTR_SHIFT) |
+		(DTE_INTR_STATUS_TIMEOUT_INTR_MASK <<
+		DTE_INTR_STATUS_TIMEOUT_INTR_SHIFT);
+	writel(status, cygnus_dte->audioeav + DTE_INTR_STATUS_REG);
+
+	/* get data from dte fifo */
+	do {
+		fifo_csr = readl(cygnus_dte->audioeav +
+			DTE_LTS_CSR_REG_BASE);
+		/* fifo empty */
+		if (fifo_csr & BIT(DTE_LTS_CSR_REG__FIFO_EMPTY))
+			break;
+
+		/* overflow error */
+		if (fifo_csr & BIT(DTE_LTS_CSR_REG__FIFO_OVERFLOW))
+			break;
+
+		spin_lock(&cygnus_dte->lock);
+
+		/* first event contains active clients */
+		active_clients = readl(cygnus_dte->audioeav +
+			DTE_LTS_FIFO_REG_BASE);
+
+		/* then timestamp */
+		rd_data = readl(cygnus_dte->audioeav +
+			DTE_LTS_FIFO_REG_BASE);
+
+		/* Save the actual timestamp */
+		timestamp_ns = rd_data;
+
+		/* Get the timestamp overflow counters */
+		current_time_sum2 = readl(cygnus_dte->audioeav +
+			DTE_NCO_TIME_REG_BASE);
+		current_time_sum3 = readl(cygnus_dte->audioeav +
+			DTE_NCO_OVERFLOW_REG_BASE);
+		spin_unlock(&cygnus_dte->lock);
+
+		/*
+		 * Timestamp overflow only includes the bottom
+		 * 8 bits of sum3 and the top 4 bits of sum2.
+		 *
+		 * Combine sum3 and sum2 to the timestamp_overflow.
+		 * Units of 2^32 ns = 4.294967296 sec
+		 */
+		timestamp_overflow =
+			((current_time_sum3 & 0xff) << 4) |
+			((current_time_sum2 >> 28) & 0xffffff);
+		/*
+		 * If the current time is less than the fifo timestamped time
+		 * then wrap around must have occurred.
+		 * Convert current_time_sum2 to ns before comparison
+		 */
+		if ((current_time_sum2 << 4) <= timestamp_ns) {
+			if (timestamp_overflow > 0)
+				/*
+				 * Decrement the overflow counter since the fifo
+				 * timestamp occurred before overflow counter
+				 * had incremented
+				 */
+				timestamp_overflow--;
+			else
+				/*
+				 * If the overflow counter is zero *and*
+				 * the timestamp wrapped then the overflow
+				 * counter also wrapped.
+				 * Set to max value of 12 bits wide.
+				 * sum3 (8 bits) and sum2 (top 4 bits)
+				 */
+				timestamp_overflow = 0xfff;
+		}
+
+		spin_lock(&cygnus_dte->lock);
+		/*
+		 * Update reference timespec if wrap of the timestamp overflow
+		 * occurred
+		 */
+		if (timestamp_overflow < cygnus_dte->timestamp_overflow_last) {
+			struct timespec ts_wrapamount;
+			/*
+			 * Wrap around occurred.
+			 * Full wrap around amount is 44 bits in ns.
+			 * Includes sum3 (8bits), sum2 (32 bits), sum1 (4 bits)
+			 * Precisely 17592.186044416 sec = ~4.887 hrs
+			 */
+			ts_wrapamount = ns_to_timespec((uint64_t)0x1<<44);
+			/* Increment the reference */
+			cygnus_dte->ts_ref =
+				timespec_add(cygnus_dte->ts_ref, ts_wrapamount);
+		}
+		/* Track the last timestamp overflow value */
+		cygnus_dte->timestamp_overflow_last = timestamp_overflow;
+
+		/* Convert current timestamp(ns) to timespec */
+		ns = ((uint64_t)timestamp_overflow << 32) +
+			timestamp_ns;
+		ts_stamp = ns_to_timespec(ns);
+
+		/* Add the current time to the reference time */
+		ts_stamp = timespec_add(cygnus_dte->ts_ref, ts_stamp);
+		spin_unlock(&cygnus_dte->lock);
+
+		/*
+		 * The valid timestamp may be valid for more than one client
+		 * Examine all the active clients and insert timestamp
+		 * to the appropriate software fifo for each client asserted
+		 * Clients[0:3] are unused.
+		 * Shift down by 4 to zero index the first valid input client
+		 */
+		active_clients >>= 4;
+
+		for_each_set_bit(client,
+			(unsigned long *)&active_clients, DTE_CLIENT_MAX) {
+			/* Insert full timestamps into software fifo */
+			inlen = kfifo_in(
+				&cygnus_dte->recv_fifo[client],
+				&ts_stamp,
+				sizeof(struct timespec));
+
+			if (inlen != sizeof(struct timespec)) {
+				cygnus_dte->kfifoof[client]++;
+				dev_err(&cygnus_dte->pdev->dev,
+					"kfifoof[%d] = %d\n",
+					client,
+					cygnus_dte->kfifoof[client]);
+			}
+		}
+	} while (++i < DTE_HW_FIFO_SIZE); /* at most get FIFO size */
+
+	if (fifo_csr & BIT(DTE_LTS_CSR_REG__FIFO_UNDERFLOW)) {
+		cygnus_dte->fifouf++;
+		dev_err(&cygnus_dte->pdev->dev,
+			"fifouf = %d\n", cygnus_dte->fifouf);
+	}
+	if (fifo_csr & BIT(DTE_LTS_CSR_REG__FIFO_OVERFLOW)) {
+		cygnus_dte->fifoof++;
+		dev_err(&cygnus_dte->pdev->dev,
+			"fifoof =%d\n", cygnus_dte->fifoof);
+	}
+
+	/* overflow/underflow error, reset fifo */
+	if (fifo_csr & 0xc) {
+		writel(0, cygnus_dte->audioeav + DTE_LTS_CSR_REG_BASE);
+		dev_err(&cygnus_dte->pdev->dev, "Resetting HW FIFO\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
+static const struct of_device_id bcm_cygnus_dte_of_match[] = {
+	{ .compatible = "brcm,cygnus-dte", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm_cygnus_dte_of_match);
+
+static int bcm_cygnus_dte_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct bcm_cygnus_dte *cygnus_dte;
+	int ret = -ENOMEM;
+	dev_t devt;
+	struct device *retdev;
+	struct class *dte_class;
+	int client;
+	int irq;
+
+	dev_info(dev, "Entering bcm_cygnus_dte_probe\n");
+
+	cygnus_dte = devm_kzalloc(dev, sizeof(*cygnus_dte), GFP_KERNEL);
+	if (!cygnus_dte)
+		return -ENOMEM;
+
+	mutex_init(&cygnus_dte->mutex);
+
+	/* Audio DTE memory mapped registers */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	cygnus_dte->audioeav = devm_ioremap_resource(dev, res);
+	if (IS_ERR(cygnus_dte->audioeav)) {
+		dev_err(&pdev->dev, "%s IO remap audioeav failed\n", __func__);
+		return PTR_ERR(cygnus_dte->audioeav);
+	}
+
+	spin_lock_init(&cygnus_dte->lock);
+
+	/* get interrupt */
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "%s platform_get_irq failed\n", __func__);
+		return -ENODEV;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq,
+				NULL, bcm_cygnus_dte_isr_threaded,
+				IRQF_ONESHOT, pdev->name, pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "request_irq error %d\n", ret);
+		return ret;
+	}
+
+	for (client = DTE_CLIENT_MIN; client < DTE_CLIENT_MAX; client++) {
+		ret = kfifo_alloc(&cygnus_dte->recv_fifo[client],
+				DTE_SW_FIFO_SIZE * sizeof(struct timespec),
+				GFP_KERNEL);
+		if (ret) {
+			dev_err(dev, "Failed kfifo alloc\n");
+			goto err_free_kfifo;
+		}
+	}
+
+	/* create device */
+	cdev_init(&cygnus_dte->dte_cdev, &dte_cdev_fops);
+
+	cygnus_dte->pdev = pdev;
+
+	/* create class for mdev auto create node /dev/dte */
+	dte_class = class_create(THIS_MODULE, DTE_DEVICE_NAME);
+	if (IS_ERR(dte_class)) {
+		ret = PTR_ERR(dte_class);
+		dev_err(&pdev->dev, "can't register static dte class\n");
+		goto err_free_kfifo;
+	}
+	cygnus_dte->dte_class = dte_class;
+
+	ret = alloc_chrdev_region(&devt, 0, DTE_MAX_DEVS, DTE_DEVICE_NAME);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"%s Alloc char device region failed\n", __func__);
+		goto err_class_destroy;
+	}
+
+	ret = cdev_add(&cygnus_dte->dte_cdev, devt, 1);
+	if (ret) {
+		dev_err(dev, "Failed adding dte cdev file\n");
+		goto err_unreg_chardev;
+	}
+
+	retdev = device_create(cygnus_dte->dte_class, NULL,
+			devt, NULL, DTE_DEVICE_NAME);
+	if (IS_ERR(retdev)) {
+		dev_err(&pdev->dev, "can't create dte device\n ");
+		ret = PTR_ERR(retdev);
+		goto err_cdev_delete;
+	}
+
+	list_add_tail(&cygnus_dte->node, &dtedev_list);
+	platform_set_drvdata(pdev, cygnus_dte);
+	cygnus_dte->devt = devt;
+
+	dev_info(dev, "Exiting bcm_cygnus_dte_probe\n");
+
+	return 0;
+
+err_cdev_delete:
+	cdev_del(&cygnus_dte->dte_cdev);
+err_unreg_chardev:
+	unregister_chrdev_region(devt, DTE_MAX_DEVS);
+err_class_destroy:
+	class_destroy(cygnus_dte->dte_class);
+err_free_kfifo:
+	for (client = DTE_CLIENT_MIN; client < DTE_CLIENT_MAX; client++)
+		kfifo_free(&cygnus_dte->recv_fifo[client]);
+	return ret;
+}
+
+static int bcm_cygnus_dte_remove(struct platform_device *pdev)
+{
+	int client;
+	int intr_mask;
+	struct bcm_cygnus_dte *cygnus_dte = platform_get_drvdata(pdev);
+
+	/* disable isochronous interrupt */
+	intr_mask = readl(cygnus_dte->audioeav + DTE_INTR_MASK_REG);
+	intr_mask |= 1 << ISO_INTR_MASK_SHIFT;
+	writel(intr_mask, cygnus_dte->audioeav + DTE_INTR_MASK_REG);
+
+	list_del(&cygnus_dte->node);
+	device_destroy(cygnus_dte->dte_class, cygnus_dte->devt);
+	class_destroy(cygnus_dte->dte_class);
+	unregister_chrdev_region(cygnus_dte->devt, DTE_MAX_DEVS);
+	platform_set_drvdata(pdev, NULL);
+	cdev_del(&cygnus_dte->dte_cdev);
+	for (client = DTE_CLIENT_MIN; client < DTE_CLIENT_MAX; client++)
+		kfifo_free(&cygnus_dte->recv_fifo[client]);
+
+	return 0;
+}
+
+static struct platform_driver bcm_cygnus_dte_driver = {
+	.driver = {
+		.name = "cygnus-dte",
+		.of_match_table = bcm_cygnus_dte_of_match,
+	},
+	.probe    = bcm_cygnus_dte_probe,
+	.remove   = bcm_cygnus_dte_remove,
+};
+module_platform_driver(bcm_cygnus_dte_driver);
+
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("Broadcom Cygnus DTE Digital Timing Engine driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/bcm_cygnus_dte.h b/include/linux/bcm_cygnus_dte.h
new file mode 100644
index 0000000..6d785c2
--- /dev/null
+++ b/include/linux/bcm_cygnus_dte.h
@@ -0,0 +1,99 @@
+/*
+ * Copyright (C) 2015, Broadcom Corporation. All Rights Reserved.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
+ * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
+ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+#ifndef _BCM_CYGNUS_DTE_H_
+#define _BCM_CYGNUS_DTE_H_
+
+/**
+ * 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,
+	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,
+};
+
+#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)
+#define DTE_IOCTL_ADJ_TIME          DTE_IOW(0x06, int64_t)
+#define DTE_IOCTL_ADJ_FREQ          DTE_IOW(0x07, int32_t)
+
+struct dte_data {
+	enum dte_client client;
+	unsigned int data;
+};
+
+struct dte_timestamp {
+	enum dte_client client;
+	struct timespec ts;
+};
+
+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);
+extern int dte_set_client_divider(
+		struct bcm_cygnus_dte *cygnus_dte,
+		enum dte_client client,
+		int divider);
+extern int dte_set_irq_interval(
+		struct bcm_cygnus_dte *cygnus_dte,
+		uint32_t nanosec);
+extern int dte_get_timestamp(
+		struct bcm_cygnus_dte *cygnus_dte,
+		enum dte_client client,
+		struct timespec *ts);
+extern int dte_set_time(
+		struct bcm_cygnus_dte *cygnus_dte,
+		struct timespec *ts);
+extern int dte_get_time(
+		struct bcm_cygnus_dte *cygnus_dte,
+		struct timespec *ts);
+extern int dte_adj_time(
+		struct bcm_cygnus_dte *cygnus_dte,
+		int64_t delta);
+extern int dte_adj_freq(
+		struct bcm_cygnus_dte *cygnus_dte,
+		int32_t ppb);
+#endif
-- 
1.7.9.5

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Arnd Bergmann]

On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote:
> Reviewed-by: Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Tested-by: Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jonathan Richardson <jonathar-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>

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.

> +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) {
> +		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.

> +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.

> +	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.

> +/**
> + * 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,
> +	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.

> +#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.

> +#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?

> +struct dte_data {
> +	enum dte_client client;
> +	unsigned int data;
> +};

No 'enum' in ioctl data, always use '__u32' etc.

> +
> +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.

> +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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Jonathan Richardson]

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.

On 15-04-23 01:04 AM, Arnd Bergmann wrote:
> On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote:
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> Tested-by: Scott Branden <sbranden@broadcom.com>
>> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> 
> 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.
> 
>> +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) {
>> +		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.
> 
>> +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.
> 
>> +	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.
> 
>> +/**
>> + * 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,
>> +	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.
> 
>> +#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.
> 
>> +#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?
> 
>> +struct dte_data {
>> +	enum dte_client client;
>> +	unsigned int data;
>> +};
> 
> No 'enum' in ioctl data, always use '__u32' etc.
> 
>> +
>> +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.
> 
>> +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

[Jonathan Richardson]

Arnd, some more info is provided and also some questions before I can
proceed.

On 15-04-23 01:04 AM, Arnd Bergmann wrote:
> On Wednesday 22 April 2015 16:22:03 Jonathan Richardson wrote:
>> Reviewed-by: Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> Tested-by: Scott Branden <sbranden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Jonathan Richardson <jonathar-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
> 
> 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).

> 
> 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.
> 
>> +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) {
>> +		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.
> 
>> +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.
> 
>> +	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?

> 
>> +/**
>> + * 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,
>> +	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.
> 
>> +#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.
> 
>> +#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.

> 
>> +struct dte_data {
>> +	enum dte_client client;
>> +	unsigned int data;
>> +};
> 
> No 'enum' in ioctl data, always use '__u32' etc.
> 
>> +
>> +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.
> 
>> +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
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[One Thousand Gnomes]

> 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)

> 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().

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).

> 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

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Arnd Bergmann]

On Friday 01 May 2015 20:30:04 One Thousand Gnomes wrote:
> 
> > 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

[Jonathan Richardson]

On 15-05-01 12:40 PM, Arnd Bergmann wrote:
> On Friday 01 May 2015 20:30:04 One Thousand Gnomes wrote:
>>
>>> 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
> 

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.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Jonathan Richardson]

A bit more info is required here.

> 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.

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.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Arnd Bergmann]

On Friday 08 May 2015 13:02:17 Jonathan Richardson wrote:
> On 15-05-01 12:40 PM, Arnd Bergmann wrote:
> > On Friday 01 May 2015 20:30:04 One Thousand Gnomes wrote:
> >>
> >>> 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
> > 
> 
> 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.

> 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).

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.
 
> 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.

Seems reasonable, or again you could add a new command with 
a different structure if Richard prefers that.

> 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.

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 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?

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

[Richard Cochran]

On Wed, May 13, 2015 at 02:19:53PM +0200, Arnd Bergmann wrote:
> 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

[Richard Cochran]

On Wed, May 13, 2015 at 04:37:59PM +0200, Richard Cochran wrote:
> On Wed, May 13, 2015 at 02:19:53PM +0200, Arnd Bergmann wrote:
> > 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

[Richard Cochran]

On Fri, May 08, 2015 at 01:02:17PM -0700, Jonathan Richardson wrote:
> 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".
 
> 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.
 
> 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.

I don't get this, either.
 
> 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.

> 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.

The PTP interface supports poll/read just fine already.

> 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

[Jonathan Richardson]

On 15-05-13 08:35 AM, Richard Cochran wrote:
> On Fri, May 08, 2015 at 01:02:17PM -0700, Jonathan Richardson wrote:
>> 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".

ptp_clock_adjtime() casts it to an unsigned and returns an error:

   if ((unsigned long) ts.tv_nsec >= NSEC_PER_SEC)
       return -EINVAL;
>  
>> 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.

>  
>> 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.
> 
> I don't get this, either.

See reply to previous email.

>  
>> 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.
> 
>> 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.
> 
> 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

> 
>> 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
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Richard Cochran]

On Wed, May 13, 2015 at 12:50:02PM -0700, Jonathan Richardson wrote:
> 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;
	}

> >> 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.

> >> 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.
> > 
> > I don't get this, either.
> 
> See reply to previous email.

Didn't help me too much :(

> > 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

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

[Jonathan Richardson]

On 15-05-13 01:27 PM, Richard Cochran wrote:
> On Wed, May 13, 2015 at 12:50:02PM -0700, Jonathan Richardson wrote:
>> 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;
> 	}

That works, thanks.

> 
>>>> 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?

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.

> 
> 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.

> 
>>>> 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.
>>>
>>> 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).

> 
>>> 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
> 
> 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.

> 
> 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.

> 
> 
> Thanks,
> Richard
> 

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Richard Cochran]

On Wed, May 13, 2015 at 04:25:49PM -0700, Jonathan Richardson wrote:
> On 15-05-13 01:27 PM, Richard Cochran wrote:
> > 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.

> 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.)

> 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.

> > 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.

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Jonathan Richardson]

On 15-05-14 04:30 AM, Richard Cochran wrote:
> On Wed, May 13, 2015 at 04:25:49PM -0700, Jonathan Richardson wrote:
>> On 15-05-13 01:27 PM, Richard Cochran wrote:
>>> 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.
> 
>> 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.)
> 
>> 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.
> 
>>> 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.
> 
> 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.

> 
> 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

[Richard Cochran]

On Wed, May 20, 2015 at 04:38:02PM -0700, Jonathan Richardson wrote:
> 
> 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.

> 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.

> 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

[Jonathan Richardson]

On 15-05-20 11:33 PM, Richard Cochran wrote:
> On Wed, May 20, 2015 at 04:38:02PM -0700, Jonathan Richardson wrote:
>>
>> 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.
> 
>> 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.
> 
>> 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

> 
> Thanks,
> Richard
> 

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Richard Cochran]

On Fri, May 01, 2015 at 12:01:07PM -0700, Jonathan Richardson wrote:
> 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.

These 32 channels should either go to kernel consumers (i2s audio) or,
if there aren't any, to the generic PTP ioctl.

> The DTE timestamper creates timestamps based on the current clock wall
> time.

What do you mean by "wall time"?  CLOCK_REALTIME?

> 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.
 
> 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.

> 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().

You can convert ppb into your time base using simple math.
 
> 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 any need for use space to set dividers.  Let the driver do
that.

Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Jonathan Richardson]

Richard, glad you retrieved our driver. Comments below.

On 15-05-13 08:21 AM, Richard Cochran wrote:
> On Fri, May 01, 2015 at 12:01:07PM -0700, Jonathan Richardson wrote:
>> 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.
> 
> These 32 channels should either go to kernel consumers (i2s audio) or,
> if there aren't any, to the generic PTP ioctl.
> 
>> 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.

> 
>> 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?

>  
>> 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.

> 
>> 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().
> 
> You can convert ppb into your time base using simple math.

Yes, this isn't a problem.

>  
>> 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 any need for use space to set dividers.  Let the driver do
> that.

See above.

Thanks.

> 
> Thanks,
> Richard
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] misc: Add initial Digital Timing Engine (DTE) driver for cygnus

[Richard Cochran]

On Wed, May 13, 2015 at 12:38:22PM -0700, Jonathan Richardson wrote:
> 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

[Jonathan Richardson]

Richard, glad you retrieved our driver. Comments below.

On 15-05-13 08:21 AM, Richard Cochran wrote:
> On Fri, May 01, 2015 at 12:01:07PM -0700, Jonathan Richardson wrote:
>> 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.
> 
> These 32 channels should either go to kernel consumers (i2s audio) or,
> if there aren't any, to the generic PTP ioctl.
> 
>> 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.

> 
>> 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?

>  
>> 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.

> 
>> 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().
> 
> You can convert ppb into your time base using simple math.

Yes, this isn't a problem.

>  
>> 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 any need for use space to set dividers.  Let the driver do
> that.

See above.

Thanks.

> 
> Thanks,
> Richard
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html