| Message ID | 20200225230819.7325-1-michael@walle.cc |
|---|---|
| Headers | show |
| Series | AT8031 PHY timestamping support | expand |
On Wed, Feb 26, 2020 at 12:08:17AM +0100, Michael Walle wrote: > 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 ;) The code basically works but there are three > major caveats: > > (1) The reading of timestamps via MDIO sometimes return wrong values. What > I see is that a part of the timestamp corresponds to the new timestamp > while another part still contains old values. Thus at the moment, I'm > reading the registers twice. I don't know if the reading actually > affects the update of the timestamp or the different timing (my MDIO > bus is rather slow, so reading the timestamp a second time take some > amount of time; but I've also tested with some delays and it didn't > had any effects). There is also no possibility to read the timestamp > atomically :( Hi Michael That sounds fundamentally broken. Which would be odd. Sometimes there is a way to take a snapshot of the value. Reading the first word could trigger this snapshot. Or the last word, or some status register. One would hope the datasheet would talk about this. Andrew
Am 26. Februar 2020 00:50:40 MEZ schrieb Andrew Lunn <andrew@lunn.ch>: >On Wed, Feb 26, 2020 at 12:08:17AM +0100, Michael Walle wrote: >> 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 ;) The code basically works but there are >three >> major caveats: >> >> (1) The reading of timestamps via MDIO sometimes return wrong >values. What >> I see is that a part of the timestamp corresponds to the new >timestamp >> while another part still contains old values. Thus at the moment, >I'm >> reading the registers twice. I don't know if the reading actually >> affects the update of the timestamp or the different timing (my >MDIO >> bus is rather slow, so reading the timestamp a second time take >some >> amount of time; but I've also tested with some delays and it didn't >> had any effects). There is also no possibility to read the >timestamp >> atomically :( > >Hi Michael > >That sounds fundamentally broken. Which would be odd. Sometimes there >is a way to take a snapshot of the value. Reading the first word could >trigger this snapshot. Or the last word, or some status register. One >would hope the datasheet would talk about this. Hi Andrew This might be the case, but the datasheet (some older revision can be found on the internet, maybe you find something) doesn't mention it. Nor does the PTP "guide" (I don't know the exact name, I'd have to check at work) of this PHY. Besides the timestamp there's also the sequence number and the source port id which would need to be read atomically together with the timestamp. I might give it a try reading the whole tx or rx block (sequenceId, sourcePortId, timestamp) sequentially. -michael
On Wed, Feb 26, 2020 at 01:07:26AM +0100, Michael Walle wrote: > Am 26. Februar 2020 00:50:40 MEZ schrieb Andrew Lunn <andrew@lunn.ch>: > >That sounds fundamentally broken. Right. It can't work unless the PHY latches the time stamp. > This might be the case, but the datasheet (some older revision can > be found on the internet, maybe you find something) doesn't mention > it. Nor does the PTP "guide" (I don't know the exact name, I'd have > to check at work) of this PHY. Besides the timestamp there's also > the sequence number and the source port id which would need to be > read atomically together with the timestamp. Maybe the part is not intended to be used at all in this way? AFAICT, PHYs like this are meant to feed a "PTP frame detected" pulse into the time stamping unit on the attached MAC. The interrupt serves to allow the SW to gather the matching fields from the frame. Thanks, Richard
Am 2020-02-26 03:54, schrieb Richard Cochran: > On Wed, Feb 26, 2020 at 01:07:26AM +0100, Michael Walle wrote: >> Am 26. Februar 2020 00:50:40 MEZ schrieb Andrew Lunn <andrew@lunn.ch>: >> >That sounds fundamentally broken. > > Right. It can't work unless the PHY latches the time stamp. To make things worse, it only has one slot for RX and one slot for TX timestamps. >> This might be the case, but the datasheet (some older revision can >> be found on the internet, maybe you find something) doesn't mention >> it. Nor does the PTP "guide" (I don't know the exact name, I'd have >> to check at work) of this PHY. Besides the timestamp there's also >> the sequence number and the source port id which would need to be >> read atomically together with the timestamp. > > Maybe the part is not intended to be used at all in this way? > > AFAICT, PHYs like this are meant to feed a "PTP frame detected" pulse > into the time stamping unit on the attached MAC. The interrupt serves > to allow the SW to gather the matching fields from the frame. But then there would need to be such a hardware pin, correct? Unless you'd misuse the INT# for it. Also, why should the PHY then have a PHC which can be adjusted. >> That sounds fundamentally broken. Which would be odd. Sometimes there >> is a way to take a snapshot of the value. Reading the first word could >> trigger this snapshot. Or the last word, or some status register. One >> would hope the datasheet would talk about this. > > This might be the case, but the datasheet (some older revision can > be found on the internet, maybe you find something) doesn't mention > it. Nor does the PTP "guide" (I don't know the exact name, I'd have > to check at work). BTW, the name of the document is "AR8031 1588v2 Precision Time Protocol, Application Note, 80-Y0618-15 Rev. A", which describes a use case where the RTC (ie PHC) is in the AR8031. I don't argue that the PHY is not broken, only that Atheros at least intended to have that use case. -michael
On Wed, Feb 26, 2020 at 12:30:48PM +0100, Michael Walle wrote: > But then there would need to be such a hardware pin, correct? Unless > you'd misuse the INT# for it. Also, why should the PHY then have a PHC > which can be adjusted. I see. Oh well. I'll just have to remember the AR8031 on my list of "ptp hardware to be avoided". :( Richard
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 ;) The code basically works but there are three major caveats: (1) The reading of timestamps via MDIO sometimes return wrong values. What I see is that a part of the timestamp corresponds to the new timestamp while another part still contains old values. Thus at the moment, I'm reading the registers twice. I don't know if the reading actually affects the update of the timestamp or the different timing (my MDIO bus is rather slow, so reading the timestamp a second time take some amount of time; but I've also tested with some delays and it didn't had any effects). There is also no possibility to read the timestamp atomically :( (2) It seems to be the case that the PHY generates an interrupt on every PTP message, eg. even if it is not an event message meaning that a new timestamp is ready. Thus we might read the timestamp too often. (3) Sometimes the TX timestamp is missing. It seems that in this case the PHY doesn't generate an interrupt. If you check for any RX_PTP/TX_PTP interrupt pending and then read both timestamps (remember that get_rxts/get_txts checks that the timestamp has actually changed) it seems to work though. if (mask & (AT8031_INTR_RX_PTP | AT8031_INTR_TX_PTP)) { at8031_get_rxts(phydev); at8031_get_txts(phydev); } Please note that the patch doesn't contain the code above. Replacing the IRQ handling with the code make PTP actually work, but I'm not satisfied with that solution, esp. reading the timestamps multiple times over MDIO. So currently I'm stuck and unfortunately, I'm not able to get support from Atheros/our FAE. 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/ -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 | 879 ++++++++++++++++++++++++++++++++++- drivers/net/phy/phy.c | 15 + drivers/net/phy/phy_device.c | 6 +- include/linux/phy.h | 2 + 5 files changed, 892 insertions(+), 27 deletions(-)