Re: [PATCH 1/5] ptp: Added a brand new class driver for ptp clocks.

[Richard Cochran <richardcochran-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Fri, Aug 27, 2010 at 03:30:39PM -0700, John Stultz wrote:
> On Fri, 2010-08-27 at 14:38 +0200, Richard Cochran wrote:
> > We have not introduced new PPS interface. We use existing PPS subsystem.
> 
> Doesn't the pps subsystem have its own way to control the pps signal
> interrupt? I'm not totally sure here, but given your point above that
> having multiple pps events it seems like they should be selectable. It
> seems something that we'd want to control via the global pps interface,
> rather then having a pps-enable flag on every random bit of hardware
> that can support it.

The PPS subsystem offers no way to disable PPS interrupts. 

> > > Same for the timestamps and periodic output (ie: and how do they differ
> > > from reading or setting a timer on CLOCK_PTP?)
> > 
> > The posix timer calls won't work:
> > 
> > I have a PTP hardware clocks with multiple external timestamp
> > channels. Using timer_gettime, how can I specify (or decode) the
> > channel of interest to me?
> 
> I guess I'm not following you here. Again, I'm not super familiar with
> the hardware involved. Could you clarify a bit more? What do you mean by
> external timestamp? Is this what is used on the packet timestamping? 

No, the packet timestamp occurs in the PHY, MAC, or on the MII bus and
is an essential feature to support the PTP.

An external timestamp is just a wire going into the clock and is an
optional feature to make the clock more useful. The clock can latch
the current time value when an edge is dectected on the wire. Using
external timestamps, you correlate real world events with the absolute
time in the clock.

Typically, a clock offers two or more such input channels (wires), but
timer_gettime does not offer a way to differentiate between them, and
thus is not suitable.

> The posix clock id interface is frustrating because the flat static
> enumeration is really limiting.
> 
> I wonder if a dynamic enumeration for posix clocks would be possibly a
> way to go?

I am perfectly happy with this.

> In other words, a driver registers a clock with the system, and the
> system reserves a clock_id for it from the designated available pool and
> assigns it back. Then userland could query the driver via something like
> sysfs to get the clockid for the hardware. 
> 
> Would that maybe work? 

I have now posted a sample implementation of this idea. Do you like it?

> > The sysfs will include one class device for each PTP clock. Each clock
> > has a sysfs attribute with the corresponding clock id.
> 
> Do you mean the clock_id # in the posix clocks namespace?

Yes.

> > I would also be happy with the character device idea already
> > posted. Just pick one of the two, and I'll resubmit the patch set...
> 
> Personally, with regard to exposing the ptp clock to userland I'm more
> comfortable via the chardev. However, the posix clocks/timer api is
> really close to what you need, so I'm not totally set against it. And
> maybe the dynamic enumeration would resolve my main problems with it?

Okay, I have posted a draft of the dynamic idea. Can you support it?

> That said, with the chardev method, I don't like the idea of duplicating
> the existing time apis via a systemtime device. Dropping that from your
> earlier patch would ease the majority of my objections.

Well, the clock interface needs to offer basic services:

1. Set time
2. Get time
3. Jump offset
4. Adjust frequency

This is similar to what the posix clock and ntp API offer. Using a
chardev, should I make the ioctls really different, just for the
purpose of being different?

To me, it makes more sense to offer a familiar interface.

I was perfectly happy with the chardev idea. In fact, that is the way
I first implemented it. Now, I have also gone ahead and implemented
the dynamic posix clock idea, too.

> > At this point I would just like to go forward with one of the two
> > proposed APIs. I had modelled the character device on the posix clock
> > calls in order to make it immediately familar, and I think it is a
> > viable approach. After the lkml discussion, I think it is even cleaner
> > and nicer to just offer a new clock id.

I would like to repeat the sentiment in this last paragraph! I already
implemented and would be content with either form for the new clock
control API:

1. Character device
2. POSIX clock with dynamic ids

Please, just take your pick ;^)

Thanks,
Richard