[Linuxptp-devel] tx/rx timestamp correction in linuxptp

Post by Delio Brignoli
Hello Richard,
The board we are working with does not timestamp packets in the PHY and reports timestamps that are off by a different amount for the TX and RX path. We are considering correcting for the delays in linuxptp. I still do not have a well defined plan as to how to implement this but it would boil down to adding two configuration options and code in both the recv and send path to apply the correction terms. I am wondering if, in principle, a patch that implements this feature would be accepted upstream or not.

Since this is a hardware issue, why not do the correction in the
driver?

Thanks,
Richard

Keller, Jacob E

2013-08-19 18:22:45 UTC

-----Original Message-----
Sent: Monday, August 19, 2013 8:53 AM
To: Delio Brignoli
Subject: Re: [Linuxptp-devel] tx/rx timestamp correction in linuxptp

Post by Delio Brignoli
Hello Richard,
The board we are working with does not timestamp packets in the PHY

and reports timestamps that are off by a different amount for the TX and
RX path. We are considering correcting for the delays in linuxptp. I still do
not have a well defined plan as to how to implement this but it would boil
down to adding two configuration options and code in both the recv and
send path to apply the correction terms. I am wondering if, in principle, a
patch that implements this feature would be accepted upstream or not.
Since this is a hardware issue, why not do the correction in the
driver?
Thanks,
Richard

I agree. If you know for sure the exact offset value, just update it in the driver's rx and tx routines for timestamping. That would be preferred to adding this in at a higher layer and it removes the requirement that other users know this value if they use your board. This way, the driver always does the right thing.

Regards,
Jake

Delio Brignoli

2013-08-19 19:14:11 UTC

Since this is a hardware issue, why not do the correction in the
driver?

That's the answer I was expecting ;-) I agree, it should go in the driver but we are weighting our options; knowing if it would (or not) be accepted upstream helps us making a decision.

In our case the delay is added by the phy but the time-stamping happens before (on TX) or after (on RX). So the delays are logically associated with the PHY but the correction has to be done in the ethernet driver. The phy can change between hardware revisions and so will the delays. I can of course add a private IOCTL to the ethernet driver to set the delays associated with the unit's hardware revision -but- the right place to store the delays would be the phy driver. Assuming we did the right thing and have the phy driver know the delay values, how would the ethernet driver retrieve them? Last time I checked I couldn't find a generic mechanism to do that i.e. I would have to hack two drivers instead of one.

Thanks
--
Delio

Richard Cochran

2013-08-20 08:03:22 UTC

Post by Delio Brignoli
That's the answer I was expecting ;-) I agree, it should go in the driver but we are weighting our options; knowing if it would (or not) be accepted upstream helps us making a decision.

Can you make the correction using the delayAsymmetry option?

Post by Delio Brignoli
In our case the delay is added by the phy but the time-stamping happens before (on TX) or after (on RX). So the delays are logically associated with the PHY but the correction has to be done in the ethernet driver. The phy can change between hardware revisions and so will the delays. I can of course add a private IOCTL to the ethernet driver to set the delays associated with the unit's hardware revision -but- the right place to store the delays would be the phy driver. Assuming we did the right thing and have the phy driver know the delay values, how would the ethernet driver retrieve them? Last time I checked I couldn't find a generic mechanism to do that i.e. I would have to hack two drivers instead of one.

Is your PHY using phylib?

If so, you could add the correction fields to phy_device or
phy_driver, depending on whether the corrections are the same for
every PHY of the same type, or whether each PHY has its own
personality.

I don't know what kind of delays you have, but I have read that the
delay through some PHYs changes over time, for example, frame by
frame, after link down/up, through aging, etc.

Thanks,
Richard

Delio Brignoli

2013-08-20 10:20:42 UTC

Post by Richard Cochran
Can you make the correction using the delayAsymmetry option?

I do not think so. As I understand it, setting delayAsymmetry would not change the value of pdelay. Instead we want pdelay to be closer to the real 'wire' propagation time.

Is your PHY using phylib?

Yes, albeit an old version <https://github.com/audioscience/linux-omap3/tree/asi1230-master/drivers/net/phy>

Post by Richard Cochran
If so, you could add the correction fields to phy_device or
phy_driver, depending on whether the corrections are the same for
every PHY of the same type, or whether each PHY has its own
personality.

I am probably missing something or didn't explain myself well enough in my previous email because I do not see how having the correction fields in the PHY's private struct would work. I assume you mean using the priv field of struct phy_device because I cannot change the phy_device and phy_driver structs. If so, the struct pointed to by priv would be private to the phy driver and even if I got hold of the pointer in the ethernet driver I wouldn't be able to use it without introducing a dependency between the two drivers (which is of course undesirable).

Post by Richard Cochran
I don't know what kind of delays you have, but I have read that the
delay through some PHYs changes over time, for example, frame by
frame, after link down/up, through aging, etc.

Yes, the correction would not be exact but would make the estimate peer delay close to the expected wire propagation delay.

--
Delio

Miroslav Lichvar

2013-08-20 12:13:49 UTC

Yes, the correction would not be exact but would make the estimate
peer delay close to the expected wire propagation delay.

I was wondering about that recently when I noticed that the reported
delay for two i210 cards connected directly with a 3 metre cable was
around 600 nanoseconds.

If it's something than can't be determined reliably from a hw database
and needs to be measured, adding new options would make sense to me.

OTOH, the only thing that should really matter is the difference
between the tx/rx delays, not their absolute values. The
delayAsymmetry option should take care of that.

--
Miroslav Lichvar

Richard Cochran

2013-08-20 18:20:38 UTC

Post by Richard Cochran
Can you make the correction using the delayAsymmetry option?

I do not think so. As I understand it, setting delayAsymmetry would not change the value of pdelay. Instead we want pdelay to be closer to the real 'wire' propagation time.

What do you mean by "real 'wire' propagation time", the inbound delay,
or the outbound delay?

Lets call:
- the path delay from master (or initiator) to slave (responder) T1,
- the path delay from slave (responder) to master (or initiator) T2.

By definition, the meanPathDelay (aka neighborPropDelay) is (T1+T2)/2,
and delayAsymmetry is (T1-T2)/2. So the delayAsymmetry has no effect
on meanPathDelay, but it does effect the offset from master, and that
is the critial value.

Why can't you add fields to phy_device?

Thanks,
Richard

Richard Cochran

2013-08-20 18:35:47 UTC

Oops, I mixed up the P2P roles...

Post by Richard Cochran
- the path delay from master (or initiator) to slave (responder) T1,

^^^^^^^^^ ^^^^^^^^^
responder initiator

Post by Richard Cochran
- the path delay from slave (responder) to master (or initiator) T2.

^^^^^^^^^ ^^^^^^^^^
initiator responder

Post by Richard Cochran
By definition, the meanPathDelay (aka neighborPropDelay) is (T1+T2)/2,
and delayAsymmetry is (T1-T2)/2. So the delayAsymmetry has no effect
on meanPathDelay, but it does effect the offset from master, and that
is the critial value.

Thanks,
Richard

Delio Brignoli

2013-08-21 08:44:28 UTC

Post by Richard Cochran
Can you make the correction using the delayAsymmetry option?

I do not think so. As I understand it, setting delayAsymmetry would not change the value of pdelay. Instead we want pdelay to be closer to the real 'wire' propagation time.

What do you mean by "real 'wire' propagation time", the inbound delay,
or the outbound delay?
- the path delay from master (or initiator) to slave (responder) T1,
- the path delay from slave (responder) to master (or initiator) T2.
By definition, the meanPathDelay (aka neighborPropDelay) is (T1+T2)/2,
and delayAsymmetry is (T1-T2)/2. So the delayAsymmetry has no effect
on meanPathDelay, but it does effect the offset from master, and that
is the critial value.

We care about neighborPropDelay being close to (T1+T2)/2 where T1 and T2 represent only the propagation delay of the wiring because an 802.1AS port is supposed to ignore a peer when the neighborPropDelay is larger than a certain value. Initially we could probably get away with setting the neighborPropDelayThresh configuration option to a larger value to account for the delay added by silicon but we would be still reporting the wrong value and, on top of that, the delay added by silicon depends on link speed so the neighborPropDelayThresh value would have to change with link speed.

Post by Delio Brignoli
I am probably missing something or didn't explain myself well enough
in my previous email because I do not see how having the correction
fields in the PHY's private struct would work. I assume you mean
using the priv field of struct phy_device because I cannot change
the phy_device and phy_driver structs.

Why can't you add fields to phy_device?

Because I think of phy_device (and phy_driver) as generic structs shared by all PHY devices/drivers so fields related to a specific PHY's quirks or implementation do not belong there. Hypothetically, if I added those fields and I wanted my patch accepted in the official kernel tree I would have to explain what's so special about my board that justifies adding those fields for all PHYs. Am I mistaken?

Thanks
--
Delio

Richard Cochran

2013-08-21 13:53:10 UTC

Post by Richard Cochran
Why can't you add fields to phy_device?

I would argue that all PHYs add delay, so the fields are not a special
case. The only special thing is knowing the delay, because not many
people bother about this. I think adding the fields, with a default of
zero, would make sense for mainline Linux, and I would support it.

Thanks,
Richard

Delio Brignoli

2013-08-21 14:59:29 UTC

Post by Richard Cochran
Why can't you add fields to phy_device?

The mean values of those delays are not constant, rather they depend on the state of the PHY. Link speed is the obvious example but it could depend on other things as well. We would need more than just a couple of variables to model this. A proper API would probably need to provide a mechanism to get the current values but also register for value change notification and then you'd have to get it accepted upstream.

All of this would be done to support a handful (one?) board and the ethernet driver of said board would still need to be modified. Because of time constraints I feel like I am better off adding a private IOCTL to the ethernet driver (a hack in other words) to send the delay values I need.

Thanks
--
Delio

Richard Cochran

2013-08-22 05:14:34 UTC

Post by Delio Brignoli
The mean values of those delays are not constant, rather they depend on the state of the PHY. Link speed is the obvious example but it could depend on other things as well. We would need more than just a couple of variables to model this. A proper API would probably need to provide a mechanism to get the current values but also register for value change notification and then you'd have to get it accepted upstream.

Yes, right. There is some complexity here, but I still think it would
be perfectly legitimate for upstream, even if there are few users of
the code. This could be framed as a compile time option.

Post by Delio Brignoli
All of this would be done to support a handful (one?) board and the ethernet driver of said board would still need to be modified. Because of time constraints I feel like I am better off adding a private IOCTL to the ethernet driver (a hack in other words) to send the delay values I need.

Sure, this is fine for your present needs.

I expect that in the long run, more people will want to have a general
purpose HWTS Delay Correction API.

Thanks,
Richard

Delio Brignoli

2013-08-22 09:27:19 UTC

Yes, right. There is some complexity here, but I still think it would
be perfectly legitimate for upstream, even if there are few users of
the code. This could be framed as a compile time option.

Sure, this is fine for your present needs.

We will revisit this when merging support for our board in the official tree becomes a real option. It is also possible we will move to timestamping in the PHY making all of this moot.

Post by Richard Cochran
I expect that in the long run, more people will want to have a general
purpose HWTS Delay Correction API.

Possibly, but as I see it, timestamps should be taken in the PHY because reliably correcting for delays added by the PHY and anything else on the "inside" is very hard (if possible at all).

Thanks
--
Delio

Keller, Jacob E

2013-08-26 18:30:50 UTC