RE: [PATCH] net: mscc: ocelot: support PPS signal generation

["Y.b. Lu" <yangbo.lu@nxp.com>]

> -----Original Message-----
> From: Richard Cochran <richardcochran@gmail.com>
> Sent: Friday, December 27, 2019 10:08 AM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; netdev <netdev@vger.kernel.org>;
> David S . Miller <davem@davemloft.net>; Claudiu Manoil
> <claudiu.manoil@nxp.com>; Vladimir Oltean <vladimir.oltean@nxp.com>;
> Alexandre Belloni <alexandre.belloni@bootlin.com>; Microchip Linux Driver
> Support <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH] net: mscc: ocelot: support PPS signal generation
> 
> On Thu, Dec 26, 2019 at 11:17:26AM +0000, Y.b. Lu wrote:
> > > -----Original Message-----
> > > From: Vladimir Oltean <olteanv@gmail.com>
> > > Also, I think what you have implemented here is periodic output
> > > (PTP_CLK_REQ_PEROUT) not PPS [input] (PTP_CLK_REQ_PPS). I have found
> > > the PTP documentation to be rather confusing on what PTP_CLK_REQ_PPS
> > > means, so I'm adding Richard in the hope that he may clarify (also
> > > what's different between PTP_CLK_REQ_PPS and PTP_CLK_REQ_PPS).
> 
> The PTP_CLK_REQ_PPS is for generating events for the kernel's PPS
> subsystem.  (See drivers/pps).  This has nothing to do with actual PPS
> signals.
> 
> > My understand is PTP_CLK_REQ_PEROUT is for periodical output,
> 
> Yes.
> 
> > and PTP_CLK_REQ_PPS is for PPS event handling.
> 
> No.
> 
> Some cards generate an interrupt at the full second roll over.  The
> interrupt service routine can feed a system time stamp into the
> kernel's pps subsystem for use by NTP.
> 
> If your device is generating an actual PPS output signal, then you
> should implement the PTP_CLK_REQ_PEROUT method.
> 

I'm a little confused.
It seems PTP_CLK_REQ_PEROUT method needs req.perout.start and req.perout.period to generate periodical *clock* signal, while PPS is *pulse* signal every second.
For the two cases (1Hz clock signal and 1 pulse very second), how to configure with PTP_CLK_REQ_PEROUT method?


> Bonus points for making the signal fully programmable!

For some hardware, each pin has fixed function. And some hardware, each pin could be programable for function.
The Ocelot PTP pin is programable, but initially the software author may plan to set fixed function for each pin.
Do you suggest we make all pins function programable?

In include/soc/mscc/ocelot.h,
enum ocelot_clk_pins {
        ALT_PPS_PIN     = 1,
        EXT_CLK_PIN,
        ALT_LDST_PIN,
        TOD_ACC_PIN
};

BTW, current ptp clock code is embedded in ocelot.c.
More and more functions will be added in the future, and the interrupt implementation in SoC is different between Ocelot and VSC9959.
Do you think it's proper to separate common code as a single PTP driver?
Thanks.

> 
> Thanks,
> Richard