Message-ID: <20201020001040.avkzgltrijaz4ujb@skbuf>
Date:   Tue, 20 Oct 2020 03:10:40 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Christian Eggers <ceggers@...i.de>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Helmut Grohne <helmut.grohne@...enta.de>,
        Paul Barker <pbarker@...sulko.com>,
        Codrin Ciubotariu <codrin.ciubotariu@...rochip.com>,
        George McCollister <george.mccollister@...il.com>,
        Marek Vasut <marex@...x.de>,
        Tristram Ha <Tristram.Ha@...rochip.com>,
        "David S . Miller" <davem@...emloft.net>,
        Woojung Huh <woojung.huh@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next 7/9] net: dsa: microchip: ksz9477: add
 hardware time stamping support

On Mon, Oct 19, 2020 at 07:24:33PM +0200, Christian Eggers wrote:
> Add routines required for TX hardware time stamping.
> 
> The KSZ9563 only supports one step time stamping
> (HWTSTAMP_TX_ONESTEP_P2P), which requires linuxptp-2.0 or later. PTP
> mode is permanently enabled (changes tail tag; depends on
> CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP).TX time stamps are reported via an
> interrupt / device registers whilst RX time stamps are reported via an
> additional tail tag.
> 
> One step TX time stamping of PDelay_Resp requires the RX time stamp from
> the associated PDelay_Req message. linuxptp assumes that the RX time
> stamp has already been subtracted from the PDelay_Req correction field
> (as done by the TI PHYTER). linuxptp will echo back the value of the
> correction field in the PDelay_Resp message.
> 
> In order to be compatible to this already established interface, the
> KSZ9563 code emulates this behavior. When processing the PDelay_Resp
> message, the time stamp is moved back from the correction field to the
> tail tag, as the hardware doesn't support negative values on this field.
> Of course, the UDP checksums (if any) have to be corrected after this
> (for both directions).
> 
> The PTP hardware performs internal detection of PTP frames (likely
> similar as ptp_classify_raw() and ptp_parse_header()). As these filters
> cannot be disabled, the current delay mode (E2E/P2P) and the clock mode
> (master/slave) must be configured via sysfs attributes. Time stamping
> will only be performed on PTP packets matching the current mode
> settings.
> 
> Everything has been tested on a Microchip KSZ9563 switch.

I looked a little bit at the KSZ9563 datasheet and I'm more confused
than I was before opening it.

-----------------------------[cut here]-----------------------------
The device supports V2 (2008) of the IEEE 1588 PTP specification and can
be programmed as either an end-to-end (E2E) or peer-to-peer (P2P)
transparent clock (TC) between ports. In addition, the host port can be
programmed as either a slave or master ordinary clock (OC) port.
Ingress timestamp capture, egress timestamp recording, correction field
update with residence time and link delay, delay turn-around time
insertion, egress timestamp insertion, and checksum update are
supported.
-----------------------------[cut here]-----------------------------

So it's a 1-step transparent clock, fair enough. That works autonomously
without any sort of involvement from the operating system, you know
that, right? This is stateless functionality.

BUT, if that is the case, what do you need PTP support in the kernel
for? What profiles are you using with linuxptp? What benefit does it
bring you if you report timestamps to the operating system, for
terminated 1588 traffic? Why would you even terminate 1588 traffic on
the host CPU? I fail to understand many of the use cases that this
switch is tailored for.

Also, I know that Microchip support does a pretty bad job at giving
useful answers, and the datasheet isn't quite clear either (looks like
there's info that has been copied from other switches, like for 2-step
timestamping, then removed, and too much was removed because now nothing
is clear) so you'll have to give your best shot at explaining some
things.


Global PTP Message Config 1 Register
------------------------------------

Bit 2: Selection of P2P or E2E
1 = Peer-to-peer (P2P) transparent clock mode
0 = End-to-end (E2E) transparent clock mode

What does this bit do exactly?
Does it change the switch's behavior as an autonomous 1-step transparent
clock? Or does it have anything to do with how/which timestamps are
delivered to the CPU? The point is, why do you care to configure this?
Sysfs is not going to fly without a solid explanation, which you did not
provide here.

My understanding of E2E vs P2P TC is that an E2E TC will correct the
timestamps of Pdelay messages, while a P2P TC won't. The P2P TC must
speak proper PDelay and not forward those packets sheepishly. Which
starts to answer my question, I believe... So my comment above, that the
1-step TC functionality doesn't require any involvement from the CPU, is
only correct for E2E TC, am I right? For P2P TC, you would need the host
CPU to speak peer delay. But you wouldn't need it for anything else (the
SYNC messages would have no reason to go to the CPU, would they?). So,
again, what profile are you using with linuxptp for this one?

If my understanding is right, maybe you want to just leave the switch
operate in E2E TC mode by default, and put it into P2P TC as soon as
your .port_hwtstamp_set() method is called?


Ok, on to my next question....

Bit 1: Selection of Master or Slave
1 = Host port is PTP master ordinary clock
0 = Host port is PTP slave ordinary clock

What does this _actually_ do? Here I really have no idea. I can only
imagine that this has again to do with the 1-step TC operation, and that
it's treating the host port as a switched endpoint, and this has to do
with the port states of the P2P TC. I'm so confused by this one that I
don't even know what to ask...
Ok, let's put it differently. You bothered to add a sysfs for it, so you
must be using it for something. What are you using it for?

Powered by blists - more mailing lists