Message-Id: <20200228180226.22986-1-michael@walle.cc>
Date:   Fri, 28 Feb 2020 19:02:24 +0100
From:   Michael Walle <michael@...le.cc>
To:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Richard Cochran <richardcochran@...il.com>,
        Michael Walle <michael@...le.cc>
Subject: [RFC PATCH v2 0/2] AT8031 PHY timestamping support

I've put some additional time into figuring out how the PHY works since my
last RFC. My previous comments on the interrupt handling was caused by my
broken setup.

This patchset is the current state of my work for adding PHY
timestamping support. I just wanted to post this to the mailinglist
before I never do it. Maybe its a starting point for other people. That
being said, I wouldn't mind comments ;) Also I like to share my findings
about the PHY. The PHY has three major caveats which IMHO makes it really
hard to work properly.

 (1) The PHY doesn't support atomic reading of the (timestamp,
     messageType, sequenceId) tuple. The workaround is to read the
     timestamp again and check if it has changed. Actually, you'd have
     to read the complete tuple again.
 (2) The PHY generates an interrupt on every PTP packet not only on
     event messages. Thus the interrupt handler may read the capture
     registers and then determine that nothing has changed. This also
     means we have to remember the last read timestamp. I make the
     assumption that the timestamp is unique.
 (3) The biggest drawback is that the PHY only provide one set of RX and
     TX capture registers. It is possible that the timestamp will change
     when another PTP event message is received while we are still
     reading the timestamp of the previously received packet.

Example A
The driver basically works when there is low PTP traffic. Eg. the
following works pretty good.

ptp4l -m -2 -i eth0 -f ptp.cfg -s

ptp.cfg:
  [global]
  tx_timestamp_timeout 100

Example B
But if you're using a P2P clock with peer delay requests this whole
thing falls apart because of caveat (3). You'll often see messages like
  received SYNC without timestamp
or
 received PDELAY_RESP without timestamp
in linuxptp. Sometimes it working for some time and then it starts to
loosing packets. I suspect this depends on how the PDELAY messages are
interleaved with the SYNC message. If there is not enough time to until
the next event message is received either of these two messages won't
have a timestamp.

ptp4l -m -f gPTP.cfg -i eth0 -s

gPTP.cfg is the one from stock linuxptp with tx_timestamp_timeout set to
100.

The PHY also supports appending the timestamp to the actual ethernet frame,
but this seems to only work when the PHY is connected via RGMII. I've never
get it to work with a SGMII connection.

The first patch might actually be useful outside of this series. See also
  https://lore.kernel.org/netdev/bd47f8e1ebc04fa98856ed8d89b91419@walle.cc/

Changes since RFC v1:
net: phy: let the driver register its own IRQ handler
 - fixed mistake for calling phy_request_interrupt(). Thanks Heiner.
 - removed phy_drv_interrupt(). just calling phy_mac_interrupt()

net: phy: at803x: add PTP support for AR8031
 - moved rereading the timestamp out of at8031_read_ts() to
   at8031_get_rxts()/at8031_get_txts().
 - call phy_mac_interrupt() instead of phy_drv_interrupt()

Please note that Heiner suggested that I should use handle_interrupt()
instead of registering my own handler. I've still included my old patch
here because the discussion is still ongoing and the proposed fix is only
working for this use case. See also
  https://lore.kernel.org/netdev/2e4371354d84231abf3a63deae1a0d04@walle.cc/

-michael

Michael Walle (2):
  net: phy: let the driver register its own IRQ handler
  net: phy: at803x: add PTP support for AR8031

 drivers/net/phy/Kconfig      |  17 +
 drivers/net/phy/at803x.c     | 855 ++++++++++++++++++++++++++++++++++-
 drivers/net/phy/phy_device.c |   6 +-
 include/linux/phy.h          |   1 +
 4 files changed, 852 insertions(+), 27 deletions(-)

-- 
2.20.1

Powered by blists - more mailing lists