Hello dears,
I was checking the patches provided by Yangbo and also got confused with the same point regarding the rateRatio calculation. The implemented way (gm_rr *= nrr) seems to be the correct one in my opinion. Any clarification regarding the method explained in the standard (rateRatio += neighborRateRatio - 1.0)? Additionally, I would like to raise the same discussion started earlier by Michael. Some hardware already supports two PTP clocks, one can be free running and used to transport 802.1AS synchronization while the other is a synchronized clock that can be used by TSN as an example. Do you think it is worth it to add this feature to linuxptp? Of course, this will require extra efforts outside the linuxptp circle since we will need to define two PHCs per interface and pass cross-timestamps up through the kernel. Would you please give us your expert opinion on this? Thank you. Kind Regards ------------------------------------------ M.Sc. Kamil Alkhouri Tel. +49 781 205-4871 Fax. +49 781 205-454871 Email: kamil.alkho...@hs-offenburg.de Hochschule Offenburg Institut für verlässliche Embedded Systems und Kommunikationselektronik (ivESK) Badstraße 24 77652 Offenburg http://www.hs-offenburg.de This E-mail and any files transmitted with it are official and intended solely for the use of the individual or entity to whom they are addressed. If you are not the intended recipient, the E-mail and any files have been transmitted to you in error and any copying, distribution or other use of the information contained in them is prohibited. Diese E-Mail und jede mit ihr übermittelte Datei sind dienstlich und ausschließlich fuer die Nutzung durch die Person oder Stelle bestimmt, die als Empfänger benannt ist. Sind Sie nicht der genannte Empfänger, ist diese E-Mail und jede enthaltene Datei fälschlicherweise an Sie übermittelt worden. Das Kopieren, Verteilen und jede sonstige Nutzung der enthaltenen Informationen ist verboten. ---------- Original Message --------- From: Y.b. Lu <yangbo...@nxp.com> Date: Thu, Oct 24, 2019 at 5:58 AM Subject: Re: [Linuxptp-devel] [RFC V2] Add IEEE 802.1AS-2011 time-aware bridge support To: Rodney Cummings <rodney.cummi...@ni.com>, linuxptp-devel@lists.sourceforge.net <linuxptp-devel@lists.sourceforge.net>, Richard Cochran <richardcoch...@gmail.com>, Erik Hons <erik.h...@ni.com> 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> > Sent: Tuesday, October 15, 2019 1:12 AM > To: Y.b. Lu <yangbo...@nxp.com>; linuxptp-devel@lists.sourceforge.net; > Richard Cochran <richardcoch...@gmail.com>; Erik Hons <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 > 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 informa > > From: Yangbo Lu <yangbo...@nxp.com> > > Sent: Monday, October 14, 2019 5:27 AM > > To: linuxptp-devel@lists.sourceforge.net; Rodney Cummings > > <rodney.cummi...@ni.com>; Richard Cochran > <richardcoch...@gmail.com>; > > Erik Hons <erik.h...@ni.com> > > Cc: Yangbo Lu <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> > > --- > > 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.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, > > > > *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 https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
_______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
