Hi Vladimir and Richard,

-----Original Message-----
From: Vladimir Oltean <olteanv@gmail.com>
Sent: Friday, March 20, 2020 9:21 PM
To: Y.b. Lu <yangbo.lu@nxp.com>
Cc: lkml <redacted>; netdev <redacted>;
David S . Miller [off-list ref]; Richard Cochran
[off-list ref]; Vladimir Oltean [off-list ref];
Claudiu Manoil [off-list ref]; Andrew Lunn [off-list ref];
Vivien Didelot [off-list ref]; Florian Fainelli
[off-list ref]; Alexandre Belloni [off-list ref];
Microchip Linux Driver Support [off-list ref]
Subject: Re: [PATCH 6/6] ptp_ocelot: support 4 programmable pins

Hi Yangbo,

On Fri, 20 Mar 2020 at 12:42, Yangbo Lu [off-list ref] wrote:

quoted

Support 4 programmable pins for only one function periodic
signal for now. Since the hardware is not able to support
absolute start time, driver starts periodic signal immediately.

Are you absolutely sure it doesn't support absolute start time?
Because that would mean it's pretty useless if the phase of the PTP
clock signal is out of control.

I'm absolutely sure that absolute start time is not supported for periodic clock unless reference manual is wrong.
And I don’t think we need to consider phase for periodic clock which is with a specified period.

But PPS is different. Pulse should be generated must after seconds increased.
The waveform_high/low should be configurable for phase and pulse width if supported.
This is supported by hardware but was not implemented by this patch. I was considering to add later.

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://patchwork.ozlabs.org/patch/1215464/

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?

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.
Thanks a lot.

I tested your patch on the LS1028A-RDB board using the following commands:

# Select PEROUT function and assign a channel to each of pins
SWITCH_1588_DAT0 and SWITCH_1588_DAT1
echo '2 0' > /sys/class/ptp/ptp1/pins/switch_1588_dat0
echo '2 1' > /sys/class/ptp/ptp1/pins/switch_1588_dat1
# Generate pulses with 1 second period on channel 0
echo '0 0 0 1 0' > /sys/class/ptp/ptp1/period
# Generate pulses with 1 second period on channel 1
echo '1 0 0 1 0' > /sys/class/ptp/ptp1/period

And here is what I get:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdrive.g
oogle.com%2Fopen%3Fid%3D1ErWufJL0TWv6hKDQdF1pRL5gn4hn4X-r&
data=02%7C01%7Cyangbo.lu%40nxp.com%7Cbd3e65bdaabb4999737d08d7c
cd17eee%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63720307
2457124468&sdata=4D97D9ZoA%2FDJeSAN%2Fha4zNuZL6GwRLNxpNY
QiLsOsyM%3D&reserved=0

So the periodic output really starts 'now' just like the print says,
so the output from DAT0 is not even in sync with DAT1.

quoted

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---

Thanks,
-Vladimir