On Tue, Feb 01, 2011 at 06:00:31PM -0800, John Stultz wrote:

On Thu, 2011-01-27 at 11:59 +0100, Richard Cochran wrote:

You may want to tweak the kconfig a bit here. If I don't have pps
enabled, if I go into the "PTP clock support" page, I get an empty
screen.

Similarly, its not very discoverable to figure out what you need to
enable to get the driver options to show up, as they depend the drivers
enabled in the networking device section.

Okay, I'll see what I can come up with.

quoted

+#define PTP_MAX_ALARMS 4
+#define PTP_MAX_CLOCKS (MAX_CLOCKS/2)

Why MAX_CLOCKS/2 ? Should this scale as MAX_CLOCKS is increased?
Or do you really just mean 8?

This is just left over from when I thought the PHCs would use the
static clock IDs. I'll fix that.

quoted

+static void enqueue_external_timestamp(struct timestamp_event_queue *queue,
+				       struct ptp_clock_event *src)
+{
+	struct ptp_extts_event *dst;
+	u32 remainder;
+
+	dst = &queue->buf[queue->tail];
+
+	dst->index = src->index;
+	dst->t.sec = div_u64_rem(src->timestamp, 1000000000, &remainder);
+	dst->t.nsec = remainder;
+
+	if (!queue_free(queue))
+		queue->overflow++;
+
+	queue->tail = (queue->tail + 1) % PTP_MAX_TIMESTAMPS;
+}

So what is serializing access to the timestamp_event_queue here? I don't
see any usage of tsevq_mux by the callers. Am I missing it? It looks
like its called from interrupt context, so do you really need a spinlock
and not a mutex here?

The external timestamp FIFO is written only from interrupt context.
The readers are from user space via read() or sysfs. The readers must
hold a mutex. As you know, FIFOs with exactly one reader and one
writer don't need locking.

However, looking again at my own code (after spending a long time in
the posicx clock stuff), I notice that, although FIFO overflow is
detected, I do not offer a way for user space to find this out or to
clear the error. I'll fix that.

quoted

+#define PTP_MAX_TIMESTAMPS 128
+
+struct timestamp_event_queue {
+	struct ptp_extts_event buf[PTP_MAX_TIMESTAMPS];
+	int head;
+	int tail;
+	int overflow;
+};
+
+struct ptp_clock {
+	struct posix_clock clock;
+	struct device *dev;
+	struct ptp_clock_info *info;
+	dev_t devid;
+	int index; /* index into clocks.map */
+	struct pps_device *pps_source;
+	struct timestamp_event_queue tsevq; /* simple fifo for time stamps */
+	struct mutex tsevq_mux; /* one process at a time reading the fifo */
+	wait_queue_head_t tsev_wq;
+};
+
+static inline int queue_cnt(struct timestamp_event_queue *q)
+{
+	int cnt = q->tail - q->head;
+	return cnt < 0 ? PTP_MAX_TIMESTAMPS + cnt : cnt;
+}

This probably needs a comment as to its locking rules. Something like
"Callers must hold tsevq_mux."

I'll add a comment explaining the readers mutex and why no
reader/writer locking is needed.

quoted

+struct ptp_clock_time {
+	__s64 sec;  /* seconds */
+	__u32 nsec; /* nanoseconds */
+	__u32 reserved;
+};
+
+struct ptp_clock_caps {
+	int max_adj;   /* Maximum frequency adjustment in parts per billon. */
+	int n_alarm;   /* Number of programmable alarms. */
+	int n_ext_ts;  /* Number of external time stamp channels. */
+	int n_per_out; /* Number of programmable periodic signals. */
+	int pps;       /* Whether the clock supports a PPS callback. */
+};
+
+struct ptp_extts_request {
+	unsigned int index; /* Which channel to configure. */
+	unsigned int flags; /* Bit field for PTP_xxx flags. */
+};
+
+struct ptp_perout_request {
+	struct ptp_clock_time start;  /* Absolute start time. */
+	struct ptp_clock_time period; /* Desired period, zero means disable. */
+	unsigned int index;           /* Which channel to configure. */
+	unsigned int flags;           /* Reserved for future use. */
+};

Since these are all new API/ABI structures, would it be wise to pad
these out a bit more? You've got a couple of reserved fields, which is
good, but if you think we're going to expand this at all, we may want to
have a bit more wiggle room. The timex structure had something like 12
unused ints (which came in handy when the tai field was added).

Not sure what the wider opinion is on this though.

Okay, I'll pad them a bit more.

However, I don't intend to ever offer more than simple functionality
here. A general purpose DAQ API is not so easy to define (look at
comedi, for example). Also, the capabilities of the current crop of
clocks varies quite a bit.

So, I think the PHC should offer a PPS, simple period outputs, and
simple external timestamping. If someone want more complex DAQ like
functions, then they can offer that through comedi or whatever.

quoted

+struct ptp_extts_event {
+	struct ptp_clock_time t; /* Time event occured. */
+	unsigned int index;      /* Which channel produced the event. */
+};

Same padding suggestion for this as well.

Okay, and thanks for the review,

Richard