RE: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x

[<Tristram.Ha@microchip.com>]

> On Thursday, 19 November 2020, 00:40:18 CET, Vladimir Oltean wrote:
> > On Wed, Nov 18, 2020 at 09:30:01PM +0100, Christian Eggers wrote:
> > > This series adds support for PTP to the KSZ956x and KSZ9477 devices.
> > >
> > > There is only little documentation for PTP available on the data sheet
> > > [1] (more or less only the register reference). Questions to the
> > > Microchip support were seldom answered comprehensively or in
> reasonable
> > > time. So this is more or less the result of reverse engineering.
> >
> > [...]
> > One thing that should definitely not be part of this series though is
> > patch 11/12. Christian, given the conversation we had on your previous
> > patch:
> > https://lore.kernel.org/netdev/20201113025311.jpkplhmacjz6lkc5@skbuf/
> sorry, I didn't read that carefully enough. Some of the other requested
> changes
> were quite challenging for me. Additionally, finding the UDP checksum bug
> needed some time for identifying because I didn't recognize that when it got
> introduced.
> 
> > as well as the documentation patch that was submitted in the meantime:
> > https://lore.kernel.org/netdev/20201117213826.18235-1-
> a.fatoum@pengutronix.de/
> I am not subscribed to the list.
> 
> > obviously you chose to completely disregard that. May we know why? How
> > are you even making use of the PTP_CLK_REQ_PPS feature?
> Of course I will drop that patch from the next series.

These are general comments about this PTP patch.

The initial proposal in tag_ksz.c is for the switch driver to provide callback functions
to handle receiving and transmitting.  Then each switch driver can be added to
process the tail tag in its own driver and leave tag_ksz.c unchanged.

It was rejected because of wanting to keep tag_ksz.c code and switch driver code
separate and concern about performance.

Now tag_ksz.c is filled with PTP code that is not relevant for other switches and will
need to be changed again when another switch driver with PTP function is added.

Can we implement that callback mechanism?

One issue with transmission with PTP enabled is that the tail tag needs to contain 4
additional bytes.  When the PTP function is off the bytes are not added.  This should
be monitored all the time.

The extra 4 bytes are only used for 1-step Pdelay_Resp.  It should contain the receive
timestamp of previous Pdelay_Req with latency adjusted.  The correction field in
Pdelay_Resp should be zero.  It may be a hardware bug to have wrong UDP checksum
when the message is sent.

I think the right implementation is for the driver to remember this receive timestamp
of Pdelay_Req and puts it in the tail tag when it sees a 1-step Pdelay_Resp is sent.

There is one more requirement that is a little difficult to do.  The calculated peer delay
needs to be programmed in hardware register, but the regular PTP stack has no way to
send that command.  I think the driver has to do its own calculation by snooping on the
Pdelay_Req/Pdelay_Resp/Pdelay_Resp_Follow_Up messages.

The receive and transmit latencies are different for different connected speed.  So the
driver needs to change them when the link changes.  For that reason the PTP stack
should not use its own latency values as generally the application does not care about
the linked speed.