Hi Jagmeet, At this stage I think the implementers of products with 802.1AS relay (previously bridge) capability would benefit greatly if they could use linuxptp. To that end, I think the participants in linuxptp should integrate 802.1AS relay support however they see fit... BC... TC... I don't care. The main requirement is to meet the "shalls" in the 802.1AS standard, so that linuxptp interoperates well with other implementations.
That being said, the "does not conform to the complete specifications for a P2P Transparent Clock in IEEE Std 1588" was placed into 802.1AS-2020 intentionally. I think of it this way: 1. If you have a TC implementation, is it possible to use that TC implementation for a subset of an 802.1AS time-aware relay implementation? Yes 2. Is an 802.1AS time-aware relay considered a 1588 TC? No Many major Ethernet switch semiconductor vendors participate in 802.1. Some of those switches have hardware support for TC. When 802.1AS syncLocked is true, it is possible to take advantage of such TC support, and some would argue that is intentional in the design of the 802.1AS standard. I suppose the same thinking could apply to linuxptp... TC code might make sense when 802.1AS syncLocked is true. Nevertheless, there are aspects of 802.1AS relay that clearly fall into the BC category, like syncLocked false, and running the BMCA. I think that's why many of us involved in the 802.1AS standard tend to say "BC" when asked "What is it?". Overall, it seems safer to start with an assumption that it is a BC, and then take advantage of TC implementation if/when it makes sense. Doing it the other way (i.e., assuming TC overall and BC for the exceptions) seems a bit risky to me. Rodney Cummings From: Jagmeet Singh Hanspal <jagmeet.hans...@gmail.com> Sent: Friday, February 21, 2020 10:33 AM To: Y.b. Lu <yangbo...@nxp.com> Cc: Rodney Cummings <rodney.cummi...@ni.com>; linuxptp-devel@lists.sourceforge.net; Richard Cochran <richardcoch...@gmail.com>; Erik Hons <erik.h...@ni.com> Subject: [EXTERNAL] Re: [Linuxptp-devel] [RFC V2] Add IEEE 802.1AS-2011 time-aware bridge support Hi, > I realize all of that is anecdotal, in that > a TC-based fork of Linuxptp might have performed well also. Nevertheless, I'm > concerned that a fully TC-based implementation would move the code away > from conformance with 802.1AS-2011, and interoperation would break as a > result. I'd like to know why the move towards BC based .1AS implementation, when the section 7.5 in .1AS 2020 shows that the AVB profile is like a very controlled BC, more like a P2P TC (+BMC) with its usage of Sync/Follow-up and PDelay message set (instead of Delay_Req / Delay_Resp pairs). Also, whether the syntonization of the bridge serves the purpose to send forward the corrections (residence + link delay) and calculation of rate-ratios, or a phase-sync is also required in the bridges implementing PTP relay, in which case BC might make more sense? Regards, Jagmeet On Thu, Oct 24, 2019 at 9:28 AM Y.b. Lu <yangbo...@nxp.com<mailto:yangbo...@nxp.com>> wrote: Hi Richard, May I have your comments? Hi Rodney, Please see my comments inline. Thanks a lot. Best regards, Yangbo Lu > -----Original Message----- > From: Rodney Cummings <rodney.cummi...@ni.com<mailto:rodney.cummi...@ni.com>> > Sent: Tuesday, October 15, 2019 1:12 AM > To: Y.b. Lu <yangbo...@nxp.com<mailto:yangbo...@nxp.com>>; > linuxptp-devel@lists.sourceforge.net<mailto:linuxptp-devel@lists.sourceforge.net>; > Richard Cochran <richardcoch...@gmail.com<mailto:richardcoch...@gmail.com>>; > Erik Hons <erik.h...@ni.com<mailto:erik.h...@ni.com>> > Subject: RE: [RFC V2] Add IEEE 802.1AS-2011 time-aware bridge support > > Thanks Yangbo, > > I'd like to interject some overall comments/questions before digging into > comments on the code. > > In my opinion I prefer this v2 (BC) over the previous v1 (TC). > > To me, it isn't so much about the name. It is more about aligning the code > with > the 802.1AS-2011 requirements ("shall"s). My employer (National Instruments) > has been shipping our own BC-based fork of Linuxptp for years now (e.g. uses > bc_dispatch() and bc_event() as is). That code has passed UNH-IOL > conformance for 802.1AS-2011, performed well in 802.1AS-2011 plugfests (i.e. > ICC, Avnu, ISPCS), and interoperated in customer applications with many other > 802.1AS-2011 end-stations and bridges. I realize all of that is anecdotal, in > that > a TC-based fork of Linuxptp might have performed well also. Nevertheless, I'm > concerned that a fully TC-based implementation would move the code away > from conformance with 802.1AS-2011, and interoperation would break as a > result. > [Y.b. Lu] To me, I think BC is proper after you clarified that, although I thought it's TC before. > Assuming the group is okay with BC, there is probably one other major > question to answer before we go much further: > Are we okay implementing the 802.1AS-2011 BC specifics in clock.c, > or do we prefer to move most of that to a different file (bridge.c, > gptp_relay.c, or whatever)? > [Y.b. Lu] I'm ok with any method. We can have Richard's comment. From my patch, there were not too much changes to reuse BC. > There's a short list of items that make 802.1AS-2011 BC different from > 1588-2008 P2P Default BC: > > 1. Transfer Sync / FollowUp differences from Slave (receive) to Master > (transmit) ports. > 2. Support distinct sync intervals on Slave and Master ports (like a typical > BC). > > The v2 patch covers item 1 and 2, which are the core differences. > Nevertheless, > it might be good to keep other differences in mind (i.e. future patches) > before > we decide the preceding question: > [Y.b. Lu] Yes. The patch had core changes, so that it could be reviewed to decide which method is good to add it in linuxptp. > 3. Transfer Pdelay differences from Slave to Master ports (cumulative rate > ratio). [Y.b. Lu] I have already done that in patch. But it's different with standard. I'm very confused on this, and doubt the standard. See below changes. + /* Update follow_up TLV */ + gm_rr *= nrr; + fui->cumulativeScaledRateOffset = gm_rr * POW2_41 - POW2_41; I can't understand why standard gets rateRatio with, rateRatio += (neighborRateRatio - 1.0); not, rateRatio *= neighborRateRatio; As I understand, the rateRatio got from neighbor should be freq_gm / freq_neighbor. The neighborRateRato should be freq_neighbor / freq_cureent. So rateRatio for current device (freq_gm / freq_cureent) should be rateRatio * neighborRateRato. I will appreciate if you can help me to get it right. > 4. BMCA: Skip over UNCALIBRATED and PRE_MASTER states. > 5. BMCA: Remove foreign master qualification (i.e. change threshold from 2 to > 1). > 6. Check performance requirement in 802.1AS-2011 B.1.1 for neighbor rate > ratio computation. > 7. BMCA: FAULTY causes a State Decision Event. 802.1AS-2011's methodology > for faults is to avoid waiting in that FAULTY state in hopes that management > will notice. Instead, 802.1AS moves on to search for a valid non-faulty state > (SDE). If supported, the fault is logged so that management can notice later, > but that logging isn't a requirement. [Y.b. Lu] Thanks. No problem for me to generate patches to support these. I don't think these will introduce much changes either. I tend to not create new files for TAB/PRI. > > There's one other item in the upcoming revision (presumably 802.1AS-2020) > that is not explicitly stated in 802.1AS-2011: > > 8. If-and-only-if the sync interval is the same for a Slave and Master port > pair, > "this PTP Port, when operating as a Master port, shall transmit a Sync as soon > as possible after the Slave port received a Sync (ignoring syncInterval)." To > Richard's credit, this is TC-like behavior. In the code, it presumably > represents a > call to tc_fwd_sync() as was done in Yangbo's v1 patch. When we dig into the > details, there might be some 802.1AS-specifics, so maybe it will warrant a new > gptp_relay_fwd_sync() function. > > Since 802.1AS-2020 isn't published yet, I'd be perfectly happy ignoring item 8 > for now. We can always revisit the issue later on. > > In the upcoming revision of 1588, all of items 1-8 are supported as options > that a PTP Profile (like 802.1AS) can list as required. For example, the 1588 > revision allows UNCALIBRATED and PRE_MASTER to be skipped (whereas > 1588-2008 requires those states). Therefore, items 1-8 will be conformant > 1588 behavior that any profile can use. I mention this because if we want to > avoid "gptp" or "802.1AS" in the naming of items 1-8, that sounds fine, > because they are not necessarily exclusive to 802.1AS in the future. [Y.b. Lu] Thank you for sharing these information. That's much helpful :) > > Rodney > > -----Original Message----- > > From: Yangbo Lu <yangbo...@nxp.com<mailto:yangbo...@nxp.com>> > > Sent: Monday, October 14, 2019 5:27 AM > > To: > > linuxptp-devel@lists.sourceforge.net<mailto:linuxptp-devel@lists.sourceforge.net>; > > Rodney Cummings > > <rodney.cummi...@ni.com<mailto:rodney.cummi...@ni.com>>; Richard Cochran > <richardcoch...@gmail.com<mailto:richardcoch...@gmail.com>>; > > Erik Hons <erik.h...@ni.com<mailto:erik.h...@ni.com>> > > Cc: Yangbo Lu <yangbo...@nxp.com<mailto:yangbo...@nxp.com>> > > Subject: [EXTERNAL] [RFC V2] Add IEEE 802.1AS-2011 time-aware bridge > > support > > > > This patch is to add IEEE 802.1AS-2011 time-aware bridge support based > > on current BC clock type. It implements only time information relay, > > and BMCA was not touched. To run it, the profile gPTP.cfg could be > > used with multiple interfaces specified using -i option. > > > > The main code changes are, > > - Create syfu_relay_info structure for time information relay. > > - Implement port_syfu_relay_info_insert() to update follow_up (with TLV) > > message with time information for relay. > > > > Signed-off-by: Yangbo Lu <yangbo...@nxp.com<mailto:yangbo...@nxp.com>> > > --- > > Changes for v2: > > - Implemented based on BC instead of TC. > > --- > > clock.c | 43 ++++++++++++++++++++++++++++++++++++- > > clock.h | 45 +++++++++++++++++++++++++++++++++++++++ > > port.c | 75 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- > > 3 files changed, 158 insertions(+), 5 deletions(-) > > > > diff --git a/clock.c b/clock.c > > index 146576a..1865ecb 100644 > > --- a/clock.c > > +++ b/clock.c > > @@ -45,7 +45,6 @@ > > #include "util.h" > > > > #define N_CLOCK_PFD (N_POLLFD + 1) /* one extra per port, for the > > fault timer */ -#define POW2_41 ((double)(1ULL << 41)) > > > > struct port { > > LIST_ENTRY(port) list; > > @@ -123,6 +122,7 @@ struct clock { > > int stats_interval; > > struct clockcheck *sanity_check; > > struct interface uds_interface; > > + struct syfu_relay_info syfu_relay; > > LIST_HEAD(clock_subscribers_head, clock_subscriber) subscribers; }; > > > > @@ -1117,6 +1117,8 @@ struct clock *clock_create(enum clock_type type, > > struct config *config, > > } > > } > > > > + memset(&c->syfu_relay, 0, sizeof(struct syfu_relay_info)); > > + > > /* Initialize the parentDS. */ > > clock_update_grandmaster(c); > > c->dad.pds.parentStats = 0; > > @@ -1208,6 +1210,15 @@ void clock_follow_up_info(struct clock *c, > > struct follow_up_info_tlv *f) > > sizeof(c->status.lastGmPhaseChange)); > > } > > > > +static void clock_get_follow_up_info(struct clock *c, struct > > +follow_up_info_tlv *f) { > > + f->cumulativeScaledRateOffset = c- > > >status.cumulativeScaledRateOffset; > > + f->scaledLastGmPhaseChange = c->status.scaledLastGmPhaseChange; > > + f->gmTimeBaseIndicator = c->status.gmTimeBaseIndicator; > > + memcpy(&f->lastGmPhaseChange, &c->status.lastGmPhaseChange, > > + sizeof(f->lastGmPhaseChange)); } > > + > > int clock_free_running(struct clock *c) { > > return c->free_running ? 1 : 0; > > @@ -1583,6 +1594,16 @@ void clock_peer_delay(struct clock *c, tmv_t > > ppd, tmv_t req, tmv_t rx, > > stats_add_value(c->stats.delay, tmv_dbl(ppd)); } > > > > +tmv_t clock_get_path_delay(struct clock *c) { > > + return c->path_delay; > > +} > > + > > +double clock_get_nrr(struct clock *c) { > > + return c->nrr; > > +} > > + > > int clock_slave_only(struct clock *c) { > > return c->dds.flags & DDS_SLAVE_ONLY; @@ -1776,6 +1797,7 @@ static > > void handle_state_decision_event(struct clock > > *c) > > c->path_delay = c->initial_delay; > > c->nrr = 1.0; > > fresh_best = 1; > > + clock_disable_syfu_relay(c); > > } > > > > c->best = best; > > @@ -1847,3 +1869,22 @@ enum servo_state clock_servo_state(struct clock > > *c) { > > return c->servo_state; > > } > > + > > +void clock_prepare_syfu_relay(struct clock *c, struct ptp_message *sync, > > + struct ptp_message *fup) > > +{ > > + c->syfu_relay.precise_origin_ts = timestamp_to_tmv(fup->ts.pdu); > > + c->syfu_relay.correction = fup->header.correction; > > + clock_get_follow_up_info(c, &c->syfu_relay.fup_info_tlv); > > + c->syfu_relay.avail = 1; > > +} > > + > > +void clock_disable_syfu_relay(struct clock *c) { > > + c->syfu_relay.avail = 0; > > +} > > + > > +struct syfu_relay_info *clock_get_syfu_relay(struct clock *c) { > > + return &c->syfu_relay; > > +} > > diff --git a/clock.h b/clock.h > > index 9d3133a..8ff1181 100644 > > --- a/clock.h > > +++ b/clock.h > > @@ -29,8 +29,18 @@ > > #include "tmv.h" > > #include "transport.h" > > > > +#define POW2_41 ((double)(1ULL << 41)) > > + > > struct ptp_message; /*forward declaration*/ > > > > +struct syfu_relay_info { > > + tmv_t precise_origin_ts; > > + Integer64 correction; > > + struct follow_up_info_tlv fup_info_tlv; > > + /* Auxiliary info */ > > + int avail; > > +}; > > + > > /** Opaque type. */ > > struct clock; > > > > @@ -240,6 +250,20 @@ void clock_peer_delay(struct clock *c, tmv_t ppd, > > tmv_t req, tmv_t rx, > > double nrr); > > > > /** > > + * Get the path delay as measured on a slave port. > > + * @param c The clock instance. > > + * @return The path delay as measured on a slave port. > > + */ > > +tmv_t clock_get_path_delay(struct clock *c); > > + > > +/** > > + * Get the neighbor rate ratio as measured on a slave port. > > + * @param c The clock instance. > > + * @return The neighbor rate ratio as measured on a slave > > port. > > + */ > > +double clock_get_nrr(struct clock *c); > > + > > +/** > > * Set clock sde > > * @param c A pointer to a clock instance obtained with > > clock_create(). > > * @param sde Pass one (1) if need a decision event and zero if not. > > @@ -359,4 +383,25 @@ void clock_check_ts(struct clock *c, uint64_t ts); > > */ > > double clock_rate_ratio(struct clock *c); > > > > +/** > > + * Prepare sync/follow_up relay. > > + * @param c The clock instance. > > + * @param sync The sync message. > > + * @param fup The follow_up message. > > + */ > > +void clock_prepare_syfu_relay(struct clock *c, struct ptp_message *sync, > > + struct ptp_message *fup); > > + > > +/** > > + * Disable sync/follow_up relay. > > + * @param c The clock instance. > > + */ > > +void clock_disable_syfu_relay(struct clock *c); > > + > > +/** > > + * Get sync/follow_up relay information. > > + * @param c The clock instance. > > + * @return The sync/follow_up relay information. > > + */ > > +struct syfu_relay_info *clock_get_syfu_relay(struct clock *c); > > #endif > > diff --git a/port.c b/port.c > > index 07ad3f0..073e71a 100644 > > --- a/port.c > > +++ b/port.c > > @@ -1241,6 +1241,10 @@ static void port_syfufsm(struct port *p, enum > > syfu_event event, > > break; > > case FUP_MATCH: > > syn = p->last_syncfup; > > + if (p->follow_up_info) > > + clock_prepare_syfu_relay(p->clock, syn, m); > > + else > > + clock_disable_syfu_relay(p->clock); > > port_synchronize(p, syn->hwts.ts, m->ts.pdu, > > syn->header.correction, > > m->header.correction, > > @@ -1261,6 +1265,10 @@ static void port_syfufsm(struct port *p, enum > > syfu_event event, > > break; > > case SYNC_MATCH: > > fup = p->last_syncfup; > > + if (p->follow_up_info) > > + clock_prepare_syfu_relay(p->clock, m, fup); > > + else > > + clock_disable_syfu_relay(p->clock); > > port_synchronize(p, m->hwts.ts, fup->ts.pdu, > > m->header.correction, > > fup->header.correction, > > @@ -1450,6 +1458,60 @@ int port_tx_announce(struct port *p, struct > > address > > *dst) > > return err; > > } > > > > +static void port_syfu_relay_info_insert(struct port *p, > > + struct ptp_message *sync, > > + struct ptp_message *fup) > > +{ > > + struct syfu_relay_info *syfu_relay = clock_get_syfu_relay(p->clock); > > + struct follow_up_info_tlv *fui_relay = &syfu_relay->fup_info_tlv; > > + struct follow_up_info_tlv *fui = follow_up_info_extract(fup); > > + tmv_t ingress, egress, residence, path_delay; > > + double gm_rr, nrr, rr; > > + struct timestamp ts; > > + > > + if (syfu_relay->avail == 0) > > + return; > > + > > + fup->follow_up.preciseOriginTimestamp = > > + tmv_to_Timestamp(syfu_relay->precise_origin_ts); > > + fup->header.correction = syfu_relay->correction; > > + > > + /* Calculate residence time. */ > > + ingress = clock_ingress_time(p->clock); > > + egress = sync->hwts.ts; > > + residence = tmv_sub(egress, ingress); > > + rr = clock_rate_ratio(p->clock); > > + if (rr != 1.0) { > > + residence = dbl_tmv(tmv_dbl(residence) * rr); > > + } > > + > > + gm_rr = 1.0 + (fui_relay->cumulativeScaledRateOffset + 0.0) / > > POW2_41; > > + nrr = clock_get_nrr(p->clock); > > + > > + /* Add corrected residence time into correction. */ > > + fup->header.correction += tmv_to_TimeInterval(residence) * gm_rr * > > +nrr; > > + > > + /* Add corrected path delay into correction. */ > > + path_delay = clock_get_path_delay(p->clock); > > + fup->header.correction += tmv_to_TimeInterval(path_delay) * gm_rr; > > + > > + /* Update follow_up TLV */ > > + gm_rr *= nrr; > > + fui->cumulativeScaledRateOffset = gm_rr * POW2_41 - POW2_41; > > + fui->scaledLastGmPhaseChange = fui_relay->scaledLastGmPhaseChange; > > + fui->gmTimeBaseIndicator = fui_relay->gmTimeBaseIndicator; > > + memcpy(&fui->lastGmPhaseChange, &fui_relay->lastGmPhaseChange, > > + sizeof(fui->lastGmPhaseChange)); > > + > > + ts.sec = fup->follow_up.preciseOriginTimestamp.seconds_msb; > > + ts.sec = ts.sec << 32 | fup- > > >follow_up.preciseOriginTimestamp.seconds_lsb; > > + ts.nsec = fup->follow_up.preciseOriginTimestamp.nanoseconds; > > + pr_debug("port %hu: syfu_relay info:", portnum(p)); > > + pr_debug("port %hu: precise_origin_ts %" PRIu64 ".%u", portnum(p), > > ts.sec, ts.nsec); > > + pr_debug("port %hu: correction %" PRId64, portnum(p), fup- > > >header.correction >> 16); > > + pr_debug("port %hu: fup_info %.9f", portnum(p), gm_rr); > > +} > > + > > int port_tx_sync(struct port *p, struct address *dst) { > > struct ptp_message *msg, *fup; > > @@ -1543,10 +1605,15 @@ int port_tx_sync(struct port *p, struct > > address > > *dst) > > fup->address = *dst; > > fup->header.flagField[0] |= UNICAST; > > } > > - if (p->follow_up_info && follow_up_info_append(fup)) { > > - pr_err("port %hu: append fup info failed", portnum(p)); > > - err = -1; > > - goto out; > > + > > + if (p->follow_up_info) { > > + if (follow_up_info_append(fup)) { > > + pr_err("port %hu: append fup info failed", portnum(p)); > > + err = -1; > > + goto out; > > + } > > + > > + port_syfu_relay_info_insert(p, msg, fup); > > } > > > > err = port_prepare_and_send(p, fup, TRANS_GENERAL); > > -- > > 2.7.4 _______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net<mailto:Linuxptp-devel@lists.sourceforge.net> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel<https://urldefense.com/v3/__https:/lists.sourceforge.net/lists/listinfo/linuxptp-devel__;!!FbZ0ZwI3Qg!5cxJqlGn7PeakSUBI5AhnovPYFi0CpKBT4TdEq_8xaFSBwt_iZNStabDFezwiFHZpA$> -- Best Regards, ~ Jagmeet Singh Hanspal ~
_______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
