On Fri, Jun 10, 2022 at 04:21:40PM +0530, SyncMonk Technologies wrote:
> When master and slave instance interacting with each other operating
> at different interface speed, delay assymetry needs to be compensated
> as described in G.8271 appendix V.
>
> Signed-off-by: Greg Armstrong <greg.armstrong...@renesas.com>
> Signed-off-by: Leon Goldin <leon.goldin...@renesas.com>
> Signed-off-by: Devasish Dey <devasish....@syncmonk.net>
> Signed-off-by: Vipin Sharma <vipin.sha...@syncmonk.net>
> ---
> clock.c | 14 ++++++++++
> clock.h | 14 ++++++++++
> config.c | 1 +
> configs/default.cfg | 1 +
> interface.c | 50 ++++++++++++++++++++++++++++++++++
> interface.h | 21 +++++++++++++++
> pdt.h | 1 +
> port.c | 5 ++++
> port_private.h | 1 +
> port_signaling.c | 34 ++++++++++++++++++++++++
> sk.c | 65 +++++++++++++++++++++++++++++++++++++++++++++
> sk.h | 18 +++++++++++++
> tlv.c | 1 +
> tlv.h | 16 +++++++++++
> unicast_service.c | 33 +++++++++++++++++++++++
> 15 files changed, 275 insertions(+)
Please document the new option in the ptp4l man page.
I think this should be split into multiple patches, e.g:
- new function reading link speed via ethtool
- add the speed field to the interface
- add server support
- add client support
> --- a/clock.c
> +++ b/clock.c
> @@ -137,6 +137,7 @@ struct clock {
> struct monitor *slave_event_monitor;
> int step_window_counter;
> int step_window;
> + bool iface_rate_tlv;
> };
Is there a reason this is specific to the clock and not the port?
> +uint64_t interface_bitperiod(struct interface *iface)
> +{
> + uint64_t if_atto_sec_bit_period = 0;
> +
> + if (!iface->if_info.valid)
> + return if_atto_sec_bit_period;
> +
> + switch (iface->if_info.speed) {
> + case 1:
> + if_atto_sec_bit_period = 0x0DE0B6B3A7640000;
This value from the table in the spec is for 1 bps, not 1 Mbps. But
please just drop the table and calculate the value from the speed.
> +static int process_interface_rate(struct port *p,
> + struct msg_interface_rate_tlv *r)
> +{
> + Integer64 delayAsymmetry;
> + double nsDelay;
> +
> + if (clock_interface_rate_tlv (p->clock) &&
> + interface_ifinfo_valid(p->iface)) {
> + /* Delay Asymmetry Calculation */
> + delayAsymmetry = r->interfaceBitPeriod -
> + interface_bitperiod(p->iface);
> + delayAsymmetry = delayAsymmetry/2;
> + nsDelay = (double)delayAsymmetry / 1000000000;
> + if (nsDelay) {
This condition seems wrong. Consider what will happen when the server
switches from an asymmetric speed to a symmetric speed. The client
will stick with the old asymmetry even when it's no longer valid.
> + delayAsymmetry =
> + (r->numberOfBitsAfterTimestamp -
> + r->numberOfBitsBeforeTimestamp) * nsDelay;
> + if (delayAsymmetry != p->portAsymmetry) {
> + /* Updating the nanosecond part */
> + p->asymmetry +=
> + ((delayAsymmetry - p->portAsymmetry) << 16);
> + p->portAsymmetry = delayAsymmetry;
> + }
> @@ -161,10 +191,14 @@ int process_signaling(struct port *p, struct
> ptp_message *m)
>
> case TLV_ORGANIZATION_EXTENSION:
> r = (struct msg_interval_req_tlv *) extra->tlv;
> + rate = (struct msg_interface_rate_tlv *) extra->tlv;
>
> if (0 == memcmp(r->id, ieee8021_id,
> sizeof(ieee8021_id)) &&
> r->subtype[0] == 0 && r->subtype[1] == 0 &&
> r->subtype[2] == 2)
> err = process_interval_request(p, r);
> + else if (0 == memcmp(r->id, itu_t_id, sizeof(itu_t_id))
> &&
> + r->subtype[0] == 0 && r->subtype[1] == 0 &&
> r->subtype[2] == 2)
> + err = process_interface_rate(p, rate);
Before the new function is called, shouldn't there be a check whether
the message comes from the currently selected master? There could be
multiple configured masters. If they had different link speeds, the
correction would be switching with each received grant.
> @@ -256,6 +280,15 @@ static int unicast_service_reply(struct port *p, struct
> ptp_message *dst,
> if (err) {
> goto out;
> }
> + if (clock_interface_rate_tlv (p->clock) && duration > 0 &&
> + clock_telecom_profile(p->clock) &&
Does the server need to check for the telecom profile? I think the new
option could be useful in other profiles too. You can add a
recommendation to the man page, but ultimately I think the user should
decide.
> + interface_ifinfo_valid(p->iface)) {
> + err = attach_interface_rate(msg,
> + interface_bitperiod(p->iface), 64, 720);
> + if (err) {
> + goto out;
> + }
> + }
--
Miroslav Lichvar
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
