Re: [PATCH V7 7/8] ptp: Added a clock driver for the IXP46x.

Previous thread: [PATCH 1/2] applesmc: Use PnP rather than hardcoding resources and devices by Matthew Garrett on Thursday, December 16, 2010 - 8:33 am. (33 messages)

Next thread: [PATCH 2/2] regulator: Optimise out noop voltage changes by Mark Brown on Thursday, December 16, 2010 - 8:49 am. (1 message)
From: Richard Cochran
Date: Thursday, December 16, 2010 - 8:41 am

Here comes PTP Hardware Clock (PHC) support, version 7, and hopefully
everyone will like it.

The first four patches provide infrastructure supporting dynamic POSIX
clock devices. This new code will be useful for other kinds of new
clocks, not just PHCs. The last four patches implement the PHC code.

Table of Contents
=================
1 Introduction 
2 Patch Set Overview 
3 Previous Discussions 
4 Design Issues 
    4.1 Clock Operations 
    4.2 System Calls for Clock Tuning 
        4.2.1 Using the POSIX Clock API 
        4.2.2 Tuning a POSIX Clock 
        4.2.3 Dynamic POSIX Clock IDs 
    4.3 Synchronizing the Linux System Time 
    4.4 Ancillary PHC Operations 
    4.5 User timers 
5 Drivers 
    5.1 Supported Hardware Clocks 
    5.2 Open Driver Issues 
        5.2.1 DP83640 
        5.2.2 IXP465 
6 Diff Stat

1 Introduction 
~~~~~~~~~~~~~~~

  The aim of this patch set is to add support for PTP Hardware Clocks
  (PHCs) into the Linux kernel.  Support for obtaining timestamps from
  a PHC already exists via the SO_TIMESTAMPING socket option,
  integrated in kernel version 2.6.30.  This patch set completes the
  picture by allow user space programs to adjust the PHC and to
  control its ancillary features.

2 Patch Set Overview 
~~~~~~~~~~~~~~~~~~~~~

  - Patch 1 adds an ADJ_SETOFFSET mode bit to the NTP timex
    structure. The new bit allows a correction of a time offset.

  - Patch 2 adds a new system call, clock_adjtime(), which is like the
    NTP adjtimex call for POSIX clocks.

  - Patches 3 and 4 add dynamic POSIX clocks.

  - The remaining patches add the core PHC code, complete with three
    drivers.

3 Previous Discussions 
~~~~~~~~~~~~~~~~~~~~~~~

  This patch set previously appeared on the netdev list. Since V5 of
  the character device patch set, the discussion has moved to the
  lkml.

  - IEEE 1588 hardware clock support [V5]
    [http://lkml.org/lkml/2010/8/16/90]

  - POSIX clock tuning syscall with static clock ids
    ...
From: Richard Cochran
Date: Thursday, December 16, 2010 - 8:41 am

This patch adds a new mode bit into the timex structure. When set, the bit
instructs the kernel to add the given time value to the current time.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 include/linux/timex.h |    3 ++-
 kernel/time/ntp.c     |   26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/include/linux/timex.h b/include/linux/timex.h
index 32d852f..82d4b24 100644
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -73,7 +73,7 @@ struct timex {
 	long tolerance;		/* clock frequency tolerance (ppm)
 				 * (read only)
 				 */
-	struct timeval time;	/* (read only) */
+	struct timeval time;	/* (read only, except for ADJ_SETOFFSET) */
 	long tick;		/* (modified) usecs between clock ticks */
 
 	long ppsfreq;           /* pps frequency (scaled ppm) (ro) */
@@ -101,6 +101,7 @@ struct timex {
 #define ADJ_ESTERROR		0x0008	/* estimated time error */
 #define ADJ_STATUS		0x0010	/* clock status */
 #define ADJ_TIMECONST		0x0020	/* pll time constant */
+#define ADJ_SETOFFSET		0x0040  /* add 'time' to current time */
 #define ADJ_TAI			0x0080	/* set TAI offset */
 #define ADJ_MICRO		0x1000	/* select microsecond resolution */
 #define ADJ_NANO		0x2000	/* select nanosecond resolution */
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index d232189..e9e3915 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -454,6 +454,7 @@ static inline void process_adjtimex_modes(struct timex *txc, struct timespec *ts
 int do_adjtimex(struct timex *txc)
 {
 	struct timespec ts;
+	ktime_t delta, kt;
 	int result;
 
 	/* Validate the data before disabling interrupts */
@@ -482,8 +483,33 @@ int do_adjtimex(struct timex *txc)
 			hrtimer_cancel(&leap_timer);
 	}
 
+	if (txc->modes & ADJ_SETOFFSET) {
+		/* Validate the delta value. */
+		if (txc->time.tv_sec && txc->time.tv_usec < 0)
+			return -EINVAL;
+
+		if (txc->modes & ADJ_NANO) {
+			struct timespec tmp;
+			tmp.tv_sec  = ...
From: Thomas Gleixner
Date: Thursday, December 16, 2010 - 10:48 am

I really do not like that conditional irq_disable(), especially as we
disable them further down again and we only safe the getnstimeofday()
call in the non ADJSETOFFSET code path.

So we really better do that unconditionally before getnstimeofday()
with an appropriate comment and change the write_seqlock_irq() to

Thanks,

--

From: Kuwahara,T.
Date: Friday, December 17, 2010 - 1:16 pm

The proposed new control mode, ADJ_SETOFFSET, is logically the same as
ADJ_OFFSET with timex.constant == -INFINITY.  So it is possible to do
the same thing without risking forward compatibility.  (I mean by "risking
forward compatibility" that the mode bit 0x0040 may be defined differently
by the upstream maintainer anytime in the future.)
--

From: john stultz
Date: Tuesday, December 21, 2010 - 12:37 pm

I'm not sure if this is correct. Its more like settimeofday, only giving
a relative offset to jump the clock, rather then an absolute time. It

adjtimex is a linux specific interface, which is compatible but not
identical to the ntp specified interfaces. The ntp client code already
has Linux specific modifications, so I don't think we have to worry
about 0x40 specifically being reserved by the NTP client.

thanks
-john


--

From: Kuwahara,T.
Date: Tuesday, December 21, 2010 - 2:13 pm

But struct timex is not linux-specific...
--

From: Richard Cochran
Date: Wednesday, December 22, 2010 - 12:11 am

If the concern is only about that bit, then we can choose another one
from the range 0x0f00 instead.

It seems to me that the ability to jump the clock is a very useful
feature, at least when implementing a clock servo for PTP in user
space, if not in other cases, too. I wonder why that option is missing
from the interface in the first place.

I am not aware of any serious attempt to get any kind of standardized
PTP clock support into any other OS.  As far as compatibility with NTP
is concerned, why can't we let Linux set an example and let
NTP/BSD/*nix follow us?


Richard
--

From: Alexander Gordeev
Date: Wednesday, December 22, 2010 - 2:58 am

В Tue, 21 Dec 2010 13:59:44 -0800

No, it is used neither in my patches nor in the original code. The only
change my patches do to timex.h is adding hardpps().

-- 
  Alexander
From: Kuwahara,T.
Date: Tuesday, December 21, 2010 - 1:57 pm

On Tue, Dec 21, 2010 at 4:56 PM, Richard Cochran

The timex.constant is defined as equal to the binary logarithm of the
reciprocal of
the natural frequency minus SHIFT_PLL.  In other words, the following
equation holds:

	log2(natural frequency) + time_constant + SHIFT_PLL = 0,

which means that decreasing time_constant increases natural frequency
exponentially.
And since a larger natural frequency gives a smaller settling time, a
sufficiently

How about this?

if (txc->modes & ADJ_OFFSET) {
	if (txc->constant == INT32_MIN) {
		/* step time */
	} else {
		/* slew time */
	}

Then let's just ignore the restriction.  (It's possible by setting the
timex.constant
without setting the ADJ_TIMECONST flag.)

That said, I'm somehow against the idea of using the adjtimex syscall
for that purpose.
--

From: Richard Cochran
Date: Wednesday, December 22, 2010 - 12:13 am

I have to agree with John on this one. Looks very hacky to me.

Richard
--

From: Kuwahara,T.
Date: Wednesday, December 22, 2010 - 1:27 pm

In short, time step is a special case of time slew.  Those are the same,
only different in one parameter, as is shown in my previous post.
That's why I said there's no need for adding a new mode.
--

From: john stultz
Date: Wednesday, December 22, 2010 - 5:00 pm

Sure, I get that a instantaneous offset adjustment could be represented
as a instantaneous slew of the same amount. But what is the benefit of
doing this? The ADJ_OFFSET isn't really a continuous function like you
describe. For instance, you can't slew time backwards. The amount you
can slow or speed up the clock is limited, so time will always increase.
So to have a magic value way outside the bound of normal operation that
does something special seems a bit obfuscated.

ADJ_SETOFFSET isn't slewing the clock. So I think its much clearer to
add a new mode, rather then to try to wedge the functionality into a
corner case of ADJ_OFFSET slewing just because one could mathematically
represent it the same way.

I see the cost of adding it as a special case as adding extra complexity
to an already complex subsystem. Doing things that make the code easier
to understand and read makes it more maintainable. And I don't see the
gain (other then saving a bit in the bit flag) to offset the complexity
of trying to squeeze this functionality into the ADJ_OFFSET mode.

thanks
-john


--

From: Richard Cochran
Date: Wednesday, December 22, 2010 - 11:13 pm

Well, in addition to the objections raised by John, your suggested
implementation is also shortsighted. The field timex.constant is
copied into time_constant in some code paths. Obviously, this would be
a bad thing when timex.constant==-huge.

So, you need to clarify the interaction between ADJ_OFFSET,
ADJ_TIMECONST, ADJ_TAI, timex.constant, time_constant, and MAXTC.

If you would fully implement your idea, I expect it would become
obvious that it a bit of a hack, both in the kernel code and in the
user space interface. But, if you disagree, please just post a patch
with the complete implementation...

Thanks,
Richard
--

From: Kuwahara,T.
Date: Saturday, December 25, 2010 - 1:38 pm

After all, I'd prefer your earlier patchset.  Leaving aside the
compatibility issue, there's no particular reason we have to re-use
the struct timex, which requires otherwise unnecessary conditional
branches as well as unit conversions.  Don't you agree?
--

From: Richard Cochran
Date: Thursday, December 16, 2010 - 8:42 am

A new syscall is introduced that allows tuning of a POSIX clock. The
syscall is implemented for four architectures: arm, blackfin, powerpc,
and x86.

The new syscall, clock_adjtime, takes two parameters, the clock ID,
and a pointer to a struct timex. Any ADJTIMEX(2) operation may be
requested via this system call, but various POSIX clocks may or may
not support tuning.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 arch/arm/include/asm/unistd.h      |    1 +
 arch/arm/kernel/calls.S            |    1 +
 arch/blackfin/include/asm/unistd.h |    3 +-
 arch/blackfin/mach-common/entry.S  |    1 +
 arch/powerpc/include/asm/systbl.h  |    1 +
 arch/powerpc/include/asm/unistd.h  |    3 +-
 arch/x86/ia32/ia32entry.S          |    1 +
 arch/x86/include/asm/unistd_32.h   |    3 +-
 arch/x86/include/asm/unistd_64.h   |    2 +
 arch/x86/kernel/syscall_table_32.S |    1 +
 drivers/char/mmtimer.c             |    1 +
 include/linux/posix-timers.h       |    4 +
 include/linux/syscalls.h           |    2 +
 kernel/compat.c                    |  136 +++++++++++++++++++++++-------------
 kernel/posix-cpu-timers.c          |    6 ++
 kernel/posix-timers.c              |   35 +++++++++
 16 files changed, 150 insertions(+), 51 deletions(-)

diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
index c891eb7..f58d881 100644
--- a/arch/arm/include/asm/unistd.h
+++ b/arch/arm/include/asm/unistd.h
@@ -396,6 +396,7 @@
 #define __NR_fanotify_init		(__NR_SYSCALL_BASE+367)
 #define __NR_fanotify_mark		(__NR_SYSCALL_BASE+368)
 #define __NR_prlimit64			(__NR_SYSCALL_BASE+369)
+#define __NR_clock_adjtime		(__NR_SYSCALL_BASE+370)
 
 /*
  * The following SWIs are ARM private.
diff --git a/arch/arm/kernel/calls.S b/arch/arm/kernel/calls.S
index 5c26ecc..430de4c 100644
--- a/arch/arm/kernel/calls.S
+++ b/arch/arm/kernel/calls.S
@@ -379,6 +379,7 @@
 		CALL(sys_fanotify_init)
 		CALL(sys_fanotify_mark)
 		CALL(sys_prlimit64)
+/* 370 ...
From: Arnd Bergmann
Date: Thursday, December 16, 2010 - 8:51 am

The syscall implementation looks good to me.

Acked-by: Arnd Bergmann <arnd@arndb.de>

It would be good to add it to include/asm-generic/unistd.h
as well, so that tile and future architectures get it, too.
--

From: Thomas Gleixner
Date: Thursday, December 16, 2010 - 10:55 am

Can you please split that patch into one providing the syscall itself
and for each architecture a separate one ?

Looks good otherwise.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
--

From: Richard Cochran
Date: Thursday, December 16, 2010 - 8:43 am

This patch adds support for adding and removing posix clocks. The
clock lifetime cycle is patterned after usb devices. Each clock is
represented by a standard character device. In addition, the driver
may optionally implemented custom character device operations.

The dynamic posix clocks do not yet do anything useful. This patch
merely provides some needed infrastructure.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 include/linux/posix-clock.h |  111 ++++++++++++++++++++++
 kernel/time/Makefile        |    3 +-
 kernel/time/posix-clock.c   |  217 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 330 insertions(+), 1 deletions(-)
 create mode 100644 include/linux/posix-clock.h
 create mode 100644 kernel/time/posix-clock.c

diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h
new file mode 100644
index 0000000..1ce7fb7
--- /dev/null
+++ b/include/linux/posix-clock.h
@@ -0,0 +1,111 @@
+/*
+ * posix-clock.h - support for dynamic clock devices
+ *
+ * Copyright (C) 2010 OMICRON electronics GmbH
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#ifndef _LINUX_POSIX_CLOCK_H_
+#define _LINUX_POSIX_CLOCK_H_
+
+#include <linux/fs.h>
+#include <linux/poll.h>
+#include <linux/posix-timers.h>
+
+/**
+ * struct posix_clock_fops - character device ...
From: Arnd Bergmann
Date: Thursday, December 16, 2010 - 9:16 am

Thanks for the change to a private ops structure. Three more
suggestions for this:

* I would recommend starting without a compat_ioctl operation if you can.
Just mandate that all ioctls for posix clocks are compatible and call
fops->ioctl from the posix_clock_compat_ioctl() function.
If you ever need compat_ioctl handling, it can still be added later.

* Instead of passing a void pointer, why not pass a struct posix_clock
pointer to the lower device and use container_of? That follows what
we do in other subsystems and allows you save one allocation, by
including the posix_clock into the specific clock struct like

You can easily combine the two structures and get rid of the extra
struct posix_clock_fops by moving its operations into
posix_clock_operations.

Looks really good otherwise.

	Arnd
--

From: Thomas Gleixner
Date: Thursday, December 16, 2010 - 1:56 pm

Please put some more space between the MAX_CLKDEV and BITS_PER_LONG. I


You can get away with that private pointer and all the void *
arguments to the various posix_clock_operations, if you mandate that
the posix_clock_operations are embedded into a clock specific data
structure.

So void *priv would become struct posix_clock_operations *clkops and
you can get your private data in the clock implementation with

Ths field is only set, but nowhere else used. What's the purpose ?

fp->private_data should only be set on success. And this will leak a
ref count when the clock open function fails.



	if (clk->index >= MAX_CLKDEV)
		goto no_index;

	set_bit(clk->index, clocks_map);


I still have some headache to understand that kref / delete_clock
magic here.

Thanks,

	tglx
--

From: Richard Cochran
Date: Thursday, December 16, 2010 - 11:29 pm

Yes, you are right. Sorry.

This patch series has been through several contortions, and this
current one won't be the last!

As to the other comments, thanks for your careful review. I will adapt
the patch set accordingly.

Richard
--

From: Richard Cochran
Date: Thursday, December 16, 2010 - 8:43 am

This patch lets the various posix clock and timer system calls use
dynamic posix clocks, as well as the traditional static clocks.

For the performance critical calls (eg clock_gettime) the code path
from the traditional static clocks is preserved.

The following system calls are affected:

   - clock_adjtime (brand new syscall)
   - clock_gettime
   - clock_getres
   - clock_settime
   - timer_create
   - timer_delete
   - timer_gettime
   - timer_settime

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 include/linux/posix-clock.h  |   25 +++++++
 include/linux/posix-timers.h |   28 +++++++-
 include/linux/time.h         |    2 +
 kernel/posix-timers.c        |  122 +++++++++++++++++++++++++++-----
 kernel/time/posix-clock.c    |  159 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 316 insertions(+), 20 deletions(-)

diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h
index 1ce7fb7..7ea94f5 100644
--- a/include/linux/posix-clock.h
+++ b/include/linux/posix-clock.h
@@ -108,4 +108,29 @@ struct posix_clock *posix_clock_create(struct posix_clock_operations *cops,
  */
 void posix_clock_destroy(struct posix_clock *clk);
 
+/**
+ * clockid_to_posix_clock() - obtain clock device pointer from a clock id
+ * @id: user space clock id
+ *
+ * Returns a pointer to the clock device, or NULL if the id is not a
+ * dynamic clock id.
+ */
+struct posix_clock *clockid_to_posix_clock(clockid_t id);
+
+/*
+ * The following functions are only to be called from posix-timers.c
+ */
+int pc_clock_adjtime(struct posix_clock *clk, struct timex *tx);
+int pc_clock_gettime(struct posix_clock *clk, struct timespec *ts);
+int pc_clock_getres(struct posix_clock *clk, struct timespec *ts);
+int pc_clock_settime(struct posix_clock *clk, const struct timespec *ts);
+
+int pc_timer_create(struct posix_clock *clk, struct k_itimer *kit);
+int pc_timer_delete(struct posix_clock *clk, struct k_itimer *kit);
+
+void pc_timer_gettime(struct ...
From: Thomas Gleixner
Date: Thursday, December 16, 2010 - 4:20 pm

On Thu, 16 Dec 2010, Richard Cochran wrote:

Can you please split this into infrastructure and conversion of

Then they should be in a header file which is not consumable by the


Why this extra step ? Why can't you call 

      pc_timer_gettime(timr, &cur_setting);

You already established that the timer is fd based, so let the
 

It's not a problem to check the validity of that clock fd after the
copy_from_user. That's an error case and we don't care about whether
we return EINVAL here or later. And with your current code this can
happen anyway as you don't hold a reference to the fd. And we do the

I really start to wonder whether we should cleanup that whole
CLOCK_DISPATCH macro magic and provide real inline functions for
each of the clock functions instead.

static inline int dispatch_clock_settime(const clockid_t id, struct timespec *tp)
{
	if (id >= 0) {
	   	return posix_clocks[id].clock_set ?
		        posic_clocks[id].clock_set(id, tp) : -EINVAL;
	}
	if (posix_cpu_clock(id))
		return -EINVAL;

	return pc_timers_clock_set(id, tp);
}

That is a bit of work, but the code will be simpler and we just do the
normal thing of function pointer structures. Stuff which is not
implemented does not magically become called via some common
function. There is no point in doing that. We just have to fill in the
various k_clock structs with the correct pointers in the first place
and let the NULL case return a sensible error value. The data
structure does not become larger that way. It's a little bit more init
code, but that's fine if we make the code better in general. In that
case it's not even more init code, it's just filling the data
structures which we register.

That needs to be done in two steps:

   1) cleanup CLOCK_DISPATCH
   2) add the pc_timer_* extras

That will make the thing way more palatable than working around the
restrictions of CLOCK_DISPATCH and adding the hell to each syscall.

The second step would be a patch consisting of exactly nine ...
From: Richard Cochran
Date: Friday, December 17, 2010 - 12:04 am

I'm glad you suggested that.

IMHO, the CLOCK_DISPATCH thing is calling out to be eliminated, but I


The code in the patch set is modeled after a USB driver, namely
drivers/usb/class/usbtmc.c. Alan Cox had brought up the example of a
PTP Hardware Clock appearing as a USB device. So the device might
suddenly disappear, and the zombie field is supposed to cover the case
where the hardware no longer exists, but the file pointer is still
valid.



I would summarize the discussion like this:

Alan Cox was strongly in favor of using a regular file descriptor as
the reference to the dynamic clock.

John Stultz thought it wouldn't be too bad to cycle though a number of
static ids, being careful not to every assign the same static id (in
series) to two different clocks.

I implemented Alan's idea, since it seemed like maintaining the
mapping between clocks and static ids would be extra work, but without
any real benefit.

Thanks again for your review,
Richard
--

From: Thomas Gleixner
Date: Friday, December 17, 2010 - 3:03 am

if (!id & ~CLOCKFD_MASK)
  if ((id & CLOCKFD_MASK) == CLOCKFD)


Hmm ok, so you use clk->mutex to prevent the underlying device from
going away while you call a function and clk->zombie indicates that
it's gone and clk just kept around for an open file descriptor. Now it
makes sense, but that wants a big fat comment in the code. :)

Doesn't the same problem exist for the file operations of patch 3? I
think you want the very same protection there unless you want to do
the same magic in your USB driver as well.

So you could do the following:

static int get_fd_clk(struct posix_clock_descr *cd, struct file *fp)
{
	cd->fp = fp;
	cd->clk = fp->private_data;
	mutex_lock(&cd->clk->mutex);
	if (!cd->clk->zombie)
		return 0;
	mutex_unlock(&cd->clk->mutex);
	return -ENODEV;
}

static void put_fd_clk(struct posix_clock_descr *cd)
{
	mutex_unlock(&cd->clk->mutex);
}

static int get_posix_clock(const clockid_t id, struct posix_clock_descr *cd)
{
	struct file *fp = fget(CLOCKID_TO_FD(id));
	int ret;

	if (!fp || fp->f_op->open != posix_clock_open || !fp->private_data)
	   	return -ENODEV;
	ret = get_fd_clk(cd, fp);
	if (ret)
		fput(fp);
	return ret;
}

static void put_posix_clock(struct posix_clock_descr *cd)
{
	put_fd_clk(cd);
	fput(cd->fp);
}

So your fops would call get_fd_clk() and put_fd_clk() and the pc_timer
ones get_posix_clock() and put_posix_clock() or whatever sensible
function names you come up with.


Yes, he's right. Was too tired to think about the clock ids assingment
when devices are removed and plugged. The fd mapping makes that all go
away.

Thanks,

	tglx
--

From: Richard Cochran
Date: Tuesday, December 21, 2010 - 1:00 am

It is a good, concrete suggestion. I will try it that way.

Thanks,

Richard
--

From: Richard Cochran
Date: Wednesday, December 22, 2010 - 1:21 am

In order to avoid leaking a fp reference, shouldn't this go like:

static int get_posix_clock(const clockid_t id, struct posix_clock_desc *cd)
{
	struct file *fp = fget(CLOCKID_TO_FD(id));
	int ret = -EINVAL;

	if (!fp)
		return ret;
	if (fp->f_op->open != posix_clock_open || !fp->private_data)
		goto out;
	ret = get_fd_clk(cd, fp);
out:
	if (ret)
		fput(fp);
	return ret;
}

Thanks again for your help,
Richard


--

From: Richard Cochran
Date: Thursday, December 16, 2010 - 8:44 am

This patch adds an infrastructure for hardware clocks that implement
IEEE 1588, the Precision Time Protocol (PTP). A class driver offers a
registration method to particular hardware clock drivers. Each clock is
presented as a standard POSIX clock.

The ancillary clock features are exposed in two different ways, via
the sysfs and by a character device.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 Documentation/ABI/testing/sysfs-ptp |  107 +++++++++++
 Documentation/ptp/ptp.txt           |   94 ++++++++++
 Documentation/ptp/testptp.c         |  352 +++++++++++++++++++++++++++++++++++
 Documentation/ptp/testptp.mk        |   33 ++++
 drivers/Kconfig                     |    2 +
 drivers/Makefile                    |    1 +
 drivers/ptp/Kconfig                 |   27 +++
 drivers/ptp/Makefile                |    6 +
 drivers/ptp/ptp_chardev.c           |  144 ++++++++++++++
 drivers/ptp/ptp_clock.c             |  317 +++++++++++++++++++++++++++++++
 drivers/ptp/ptp_private.h           |   68 +++++++
 drivers/ptp/ptp_sysfs.c             |  230 +++++++++++++++++++++++
 include/linux/Kbuild                |    1 +
 include/linux/ptp_clock.h           |   79 ++++++++
 include/linux/ptp_clock_kernel.h    |  139 ++++++++++++++
 15 files changed, 1600 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-ptp
 create mode 100644 Documentation/ptp/ptp.txt
 create mode 100644 Documentation/ptp/testptp.c
 create mode 100644 Documentation/ptp/testptp.mk
 create mode 100644 drivers/ptp/Kconfig
 create mode 100644 drivers/ptp/Makefile
 create mode 100644 drivers/ptp/ptp_chardev.c
 create mode 100644 drivers/ptp/ptp_clock.c
 create mode 100644 drivers/ptp/ptp_private.h
 create mode 100644 drivers/ptp/ptp_sysfs.c
 create mode 100644 include/linux/ptp_clock.h
 create mode 100644 include/linux/ptp_clock_kernel.h

diff --git a/Documentation/ABI/testing/sysfs-ptp b/Documentation/ABI/testing/sysfs-ptp
new file mode 100644
index ...
From: Arnd Bergmann
Date: Thursday, December 16, 2010 - 8:57 am

The declaration is in a C file, better move it into a header in order
to make sure the definition matches the declaration.

	Arnd
--

From: Rodolfo Giometti
Date: Thursday, December 16, 2010 - 9:08 am

Regarding the PPS subsystem it looks ok for me.

Acked-by: Rodolfo Giometti <giometti@linux.it>

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it
--

From: Richard Cochran
Date: Thursday, December 16, 2010 - 8:44 am

The eTSEC includes a PTP clock with quite a few features. This patch adds
support for the basic clock adjustment functions, plus two external time
stamps, one alarm, and the PPS callback.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 Documentation/powerpc/dts-bindings/fsl/tsec.txt |   57 +++
 arch/powerpc/boot/dts/mpc8313erdb.dts           |   14 +
 arch/powerpc/boot/dts/mpc8572ds.dts             |   14 +
 arch/powerpc/boot/dts/p2020ds.dts               |   14 +
 arch/powerpc/boot/dts/p2020rdb.dts              |   14 +
 drivers/net/Makefile                            |    1 +
 drivers/net/gianfar_ptp.c                       |  444 +++++++++++++++++++++++
 drivers/net/gianfar_ptp_reg.h                   |  113 ++++++
 drivers/ptp/Kconfig                             |   13 +
 9 files changed, 684 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/gianfar_ptp.c
 create mode 100644 drivers/net/gianfar_ptp_reg.h

diff --git a/Documentation/powerpc/dts-bindings/fsl/tsec.txt b/Documentation/powerpc/dts-bindings/fsl/tsec.txt
index edb7ae1..f6edbb8 100644
--- a/Documentation/powerpc/dts-bindings/fsl/tsec.txt
+++ b/Documentation/powerpc/dts-bindings/fsl/tsec.txt
@@ -74,3 +74,60 @@ Example:
 		interrupt-parent = <&mpic>;
 		phy-handle = <&phy0>
 	};
+
+* Gianfar PTP clock nodes
+
+General Properties:
+
+  - compatible   Should be "fsl,etsec-ptp"
+  - reg          Offset and length of the register set for the device
+  - interrupts   There should be at least two interrupts. Some devices
+                 have as many as four PTP related interrupts.
+
+Clock Properties:
+
+  - tclk-period  Timer reference clock period in nanoseconds.
+  - tmr-prsc     Prescaler, divides the output clock.
+  - tmr-add      Frequency compensation value.
+  - cksel        0= external clock, 1= eTSEC system clock, 3= RTC clock input.
+                 Currently the driver only supports choice "1".
+  - tmr-fiper1   Fixed interval period pulse generator.
+  - ...
From: Richard Cochran
Date: Thursday, December 16, 2010 - 8:44 am

This patch adds a driver for the hardware time stamping unit found on the
IXP465. The basic clock operations and an external trigger are implemented.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 arch/arm/mach-ixp4xx/include/mach/ixp46x_ts.h |   78 ++++++
 drivers/net/arm/ixp4xx_eth.c                  |  191 ++++++++++++++
 drivers/ptp/Kconfig                           |   13 +
 drivers/ptp/Makefile                          |    1 +
 drivers/ptp/ptp_ixp46x.c                      |  342 +++++++++++++++++++++++++
 5 files changed, 625 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-ixp4xx/include/mach/ixp46x_ts.h
 create mode 100644 drivers/ptp/ptp_ixp46x.c

diff --git a/arch/arm/mach-ixp4xx/include/mach/ixp46x_ts.h b/arch/arm/mach-ixp4xx/include/mach/ixp46x_ts.h
new file mode 100644
index 0000000..729a6b2
--- /dev/null
+++ b/arch/arm/mach-ixp4xx/include/mach/ixp46x_ts.h
@@ -0,0 +1,78 @@
+/*
+ * PTP 1588 clock using the IXP46X
+ *
+ * Copyright (C) 2010 OMICRON electronics GmbH
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef _IXP46X_TS_H_
+#define _IXP46X_TS_H_
+
+#define DEFAULT_ADDEND 0xF0000029
+#define TICKS_NS_SHIFT 4
+
+struct ixp46x_channel_ctl {
+	u32 Ch_Control; /* 0x40 Time Synchronization Channel Control */
+	u32 Ch_Event;   /* 0x44 Time ...
From: Pavel Machek
Date: Sunday, January 2, 2011 - 1:45 am

From: Richard Cochran
Date: Sunday, January 2, 2011 - 2:12 am

I agree that CamelCase is ugly and in bad taste. However, I make an
exception when the register level programmer's manual uses this style.

IMHO, it is better to use the exact same mnemonics as in the
manual. That way, when the next person comes along to make a change to
the code, with manual in hand, he will immediately know what is what.

Thanks,
Richard
--

From: Pavel Machek
Date: Sunday, January 2, 2011 - 2:20 am

Given the comments -- does manual really use camelCase crap?
And... the identifiers actually combine camelCase with _. Better fix
it.
							Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--

From: Richard Cochran
Date: Monday, January 3, 2011 - 10:07 am

From: Richard Cochran
Date: Sunday, January 2, 2011 - 2:19 am

BTW, I posted an updated V8 of this patch series on December 31.

Richard
--

From: Richard Cochran
Date: Thursday, December 16, 2010 - 8:45 am

This patch adds support for the PTP clock found on the DP83640.
The basic clock operations and one external time stamp have
been implemented.

Signed-off-by: Richard Cochran <richard.cochran@omicron.at>
---
 drivers/net/phy/Kconfig       |   29 ++
 drivers/net/phy/Makefile      |    1 +
 drivers/net/phy/dp83640.c     |  890 +++++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/dp83640_reg.h |  261 ++++++++++++
 4 files changed, 1181 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/phy/dp83640.c
 create mode 100644 drivers/net/phy/dp83640_reg.h

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 35fda5a..1f1399a 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -76,6 +76,35 @@ config NATIONAL_PHY
 	---help---
 	  Currently supports the DP83865 PHY.
 
+config DP83640_PHY
+	tristate "Driver for the National Semiconductor DP83640 PHYTER"
+	depends on PTP_1588_CLOCK
+	depends on NETWORK_PHY_TIMESTAMPING
+	---help---
+	  Supports the DP83640 PHYTER with IEEE 1588 features.
+
+	  This driver adds support for using the DP83640 as a PTP
+	  clock. This clock is only useful if your PTP programs are
+	  getting hardware time stamps on the PTP Ethernet packets
+	  using the SO_TIMESTAMPING API.
+
+	  In order for this to work, your MAC driver must also
+	  implement the skb_tx_timetamp() function.
+
+config DP83640_PHY_STATUS_FRAMES
+	bool "DP83640 Status Frames"
+	default y
+	depends on DP83640_PHY
+	---help---
+	  This option allows the DP83640 PHYTER driver to obtain time
+	  stamps from the PHY via special status frames, rather than
+	  reading over the MDIO bus. Using status frames is therefore
+	  more efficient. However, if enabled, this option will cause
+	  the driver to add a mutlicast address to the MAC.
+
+	  Say Y here, unless your MAC does not support multicast
+	  destination addresses.
+
 config STE10XP
 	depends on PHYLIB
 	tristate "Driver for STMicroelectronics STe10Xp PHYs"
diff --git ...
Previous thread: [PATCH 1/2] applesmc: Use PnP rather than hardcoding resources and devices by Matthew Garrett on Thursday, December 16, 2010 - 8:33 am. (33 messages)

Next thread: [PATCH 2/2] regulator: Optimise out noop voltage changes by Mark Brown on Thursday, December 16, 2010 - 8:49 am. (1 message)