RE: [PATCH 6/6] ptp_ocelot: support 4 programmable pins

[Y.b. Lu]

> -----Original Message-----
> From: Richard Cochran <richardcochran@xxxxxxxxx>
> Sent: Tuesday, March 24, 2020 9:20 PM
> To: Y.b. Lu <yangbo.lu@xxxxxxx>
> Cc: Vladimir Oltean <olteanv@xxxxxxxxx>; lkml
> <linux-kernel@xxxxxxxxxxxxxxx>; netdev <netdev@xxxxxxxxxxxxxxx>; David S .
> Miller <davem@xxxxxxxxxxxxx>; Vladimir Oltean <vladimir.oltean@xxxxxxx>;
> Claudiu Manoil <claudiu.manoil@xxxxxxx>; Andrew Lunn <andrew@xxxxxxx>;
> Vivien Didelot <vivien.didelot@xxxxxxxxx>; Florian Fainelli
> <f.fainelli@xxxxxxxxx>; Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx>;
> Microchip Linux Driver Support <UNGLinuxDriver@xxxxxxxxxxxxx>
> Subject: Re: [PATCH 6/6] ptp_ocelot: support 4 programmable pins
> 
> On Tue, Mar 24, 2020 at 05:21:27AM +0000, Y.b. Lu wrote:
> > In my one previous patch, I was suggested to implement PPS with
> programmable pin periodic clock function.
> > But I didnât find how should PPS be implemented with periodic clock
> function after checking ptp driver.
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchw
> ork.ozlabs.org%2Fpatch%2F1215464%2F&amp;data=02%7C01%7Cyangbo.lu
> %40nxp.com%7Cbfdbd209ae014cd8484b08d7cff60c13%7C686ea1d3bc2b4c6
> fa92cd99c5c301635%7C0%7C0%7C637206527981191161&amp;sdata=oy9m
> T%2Bl69H%2BmpzM9T2kPXQNSMm5w%2FowLhzziUJX2gZc%3D&amp;reserv
> ed=0
> 
> Yes, for generating a 1-PPS output waveform, users call ioctl
> PTP_CLK_REQ_PEROUT with ptp_perout_request.period={1,0}.
> 
> If your device can't control the start time, then it can accept an
> unspecified time of ptp_perout_request.start={0,0}.

Get it. Thanks a lot.

> 
> > Vladimir talked with me, for the special PPS case, we may consider,
> > if (req.perout.period.sec ==1 && req.perout.period.nsec == 0) and configure
> WAVEFORM_LOW to be equal to req_perout.start.nsec.
> >
> > Richard, do you think is it ok?
> 
> Sound okay to me (but I don't know about WAVEFORM_LOW).

Sorry. I should have explain more. There is a SYNC bit in Ocelot PTP hardware for PPS generation.
WAFEFORM_LOW register could be used to adjust phase.

RM says,
"For the CLOCK action, the sync option makes the pin generate a single pulse, <WAFEFORM_LOW>
nanoseconds after the time of day has increased the seconds. The pulse will get a width of
<WAVEFORM_HIGH> nanoseconds."

Then I will add PPS case in next version patch.
Thanks.

> 
> > And another problem I am facing is, in .enable() callback
> (PTP_CLK_REQ_PEROUT request) I defined.
> >                 /*
> >                  * TODO: support disabling function
> >                  * When ptp_disable_pinfunc() is to disable function,
> >                  * it has already held pincfg_mux.
> >                  * However ptp_find_pin() in .enable() called also needs
> >                  * to hold pincfg_mux.
> >                  * This causes dead lock. So, just return for function
> >                  * disabling, and this needs fix-up.
> >                  */
> > Hope some suggestions here.
> 
> See my reply to the patch.
> 
> Thanks,
> Richard