From: "Machnikowski, Maciej" <maciej.machnikowski@intel.com> To: Petr Machata <petrm@nvidia.com> Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "intel-wired-lan@lists.osuosl.org" <intel-wired-lan@lists.osuosl.org>, "richardcochran@gmail.com" <richardcochran@gmail.com>, "abyagowi@fb.com" <abyagowi@fb.com>, "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>, "davem@davemloft.net" <davem@davemloft.net>, "kuba@kernel.org" <kuba@kernel.org>, "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>, "idosch@idosch.org" <idosch@idosch.org>, "mkubecek@suse.cz" <mkubecek@suse.cz>, "saeed@kernel.org" <saeed@kernel.org>, "michael.chan@broadcom.com" <michael.chan@broadcom.com> Subject: RE: [PATCH v3 net-next 6/6] docs: net: Add description of SyncE interfaces Date: Mon, 15 Nov 2021 10:24:46 +0000 [thread overview] Message-ID: <MW5PR11MB5812E733ED2BB621A3249CD5EA989@MW5PR11MB5812.namprd11.prod.outlook.com> (raw) In-Reply-To: <87tugic17a.fsf@nvidia.com> > -----Original Message----- > From: Petr Machata <petrm@nvidia.com> > Sent: Thursday, November 11, 2021 1:43 PM > To: Machnikowski, Maciej <maciej.machnikowski@intel.com> > Subject: Re: [PATCH v3 net-next 6/6] docs: net: Add description of SyncE > interfaces > > > Maciej Machnikowski <maciej.machnikowski@intel.com> writes: > > > Add Documentation/networking/synce.rst describing new RTNL messages > > and respective NDO ops supporting SyncE (Synchronous Ethernet). > > > > Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com> > > --- ... > > +RTM_GETEECSTATE > > +---------------- > > +Reads the state of the EEC or equivalent physical clock synchronizer. > > +This message returns the following attributes: > > +IFLA_EEC_STATE - current state of the EEC or equivalent clock generator. > > + The states returned in this attribute are aligned to the > > + ITU-T G.781 and are: > > + IF_EEC_STATE_INVALID - state is not valid > > + IF_EEC_STATE_FREERUN - clock is free-running > > + IF_EEC_STATE_LOCKED - clock is locked to the reference, > > + but the holdover memory is not valid > > + IF_EEC_STATE_LOCKED_HO_ACQ - clock is locked to the > reference > > + and holdover memory is valid > > + IF_EEC_STATE_HOLDOVER - clock is in holdover mode > > +State is read from the netdev calling the: > > +int (*ndo_get_eec_state)(struct net_device *dev, enum if_eec_state > *state, > > + u32 *src_idx, struct netlink_ext_ack *extack); > > + > > +IFLA_EEC_SRC_IDX - optional attribute returning the index of the > reference > > + that is used for the current IFLA_EEC_STATE, i.e., > > + the index of the pin that the EEC is locked to. > > + > > +Will be returned only if the ndo_get_eec_src is implemented. > > \ No newline at end of file > > Just to be clear, I have much the same objections to this UAPI as I had > to v2: > > - RTM_GETEECSTATE will become obsolete as soon as DPLL object is added. Yes for more complex devices and no for simple ones > - Reporting pins through the netdevices that use them allows for > configurations that are likely invalid, like disjoint "frequency > bridges". Not sure if I understand that comment. In target application a given netdev will receive an ESMC message containing the quality of the clock that it has on the receive side. The upper layer software will see QL_PRC on one port and QL_EEC on other and will need to enable clock output from the port that received QL_PRC, as it's the higher clock class. Once the EEC reports Locked state all other ports that are traceable to a given EEC (either using the DPLL subsystem, or the config file) will start reporting QL_PRC to downstream devices. > - It's not clear what enabling several pins means, and it's not clear > whether this genericity is not going to be an issue in the future when > we know what enabling more pins means. It means that the recovered frequency will appear on 2 or more physical pins of the package. > - No way as a user to tell whether two interfaces that report the same > pins are actually connected to the same EEC. How many EEC's are there, > in the system, anyway? For now we can fix that with the config file, for future we will be able to trace them to the same EEC. It's like BC in PTP - you can rely on automated mode, but can also override it with the config file. > In particular, I think that the proposed UAPIs should belong to a DPLL > object. That object must know about the pins, so have it enumerate them. > That object needs to know about which pin/s to track, so configure it > there. That object has the state, so have it report it. Really, it looks > basically 1:1 vs. the proposed API, except the object over which the > UAPIs should be defined is a DPLL, not a netdev. RCLK pin API does not belong to the DPLL and never will. That part will always belong to the netdev.
WARNING: multiple messages have this Message-ID (diff)
From: Machnikowski, Maciej <maciej.machnikowski@intel.com> To: intel-wired-lan@osuosl.org Subject: [Intel-wired-lan] [PATCH v3 net-next 6/6] docs: net: Add description of SyncE interfaces Date: Mon, 15 Nov 2021 10:24:46 +0000 [thread overview] Message-ID: <MW5PR11MB5812E733ED2BB621A3249CD5EA989@MW5PR11MB5812.namprd11.prod.outlook.com> (raw) In-Reply-To: <87tugic17a.fsf@nvidia.com> > -----Original Message----- > From: Petr Machata <petrm@nvidia.com> > Sent: Thursday, November 11, 2021 1:43 PM > To: Machnikowski, Maciej <maciej.machnikowski@intel.com> > Subject: Re: [PATCH v3 net-next 6/6] docs: net: Add description of SyncE > interfaces > > > Maciej Machnikowski <maciej.machnikowski@intel.com> writes: > > > Add Documentation/networking/synce.rst describing new RTNL messages > > and respective NDO ops supporting SyncE (Synchronous Ethernet). > > > > Signed-off-by: Maciej Machnikowski <maciej.machnikowski@intel.com> > > --- ... > > +RTM_GETEECSTATE > > +---------------- > > +Reads the state of the EEC or equivalent physical clock synchronizer. > > +This message returns the following attributes: > > +IFLA_EEC_STATE - current state of the EEC or equivalent clock generator. > > + The states returned in this attribute are aligned to the > > + ITU-T G.781 and are: > > + IF_EEC_STATE_INVALID - state is not valid > > + IF_EEC_STATE_FREERUN - clock is free-running > > + IF_EEC_STATE_LOCKED - clock is locked to the reference, > > + but the holdover memory is not valid > > + IF_EEC_STATE_LOCKED_HO_ACQ - clock is locked to the > reference > > + and holdover memory is valid > > + IF_EEC_STATE_HOLDOVER - clock is in holdover mode > > +State is read from the netdev calling the: > > +int (*ndo_get_eec_state)(struct net_device *dev, enum if_eec_state > *state, > > + u32 *src_idx, struct netlink_ext_ack *extack); > > + > > +IFLA_EEC_SRC_IDX - optional attribute returning the index of the > reference > > + that is used for the current IFLA_EEC_STATE, i.e., > > + the index of the pin that the EEC is locked to. > > + > > +Will be returned only if the ndo_get_eec_src is implemented. > > \ No newline at end of file > > Just to be clear, I have much the same objections to this UAPI as I had > to v2: > > - RTM_GETEECSTATE will become obsolete as soon as DPLL object is added. Yes for more complex devices and no for simple ones > - Reporting pins through the netdevices that use them allows for > configurations that are likely invalid, like disjoint "frequency > bridges". Not sure if I understand that comment. In target application a given netdev will receive an ESMC message containing the quality of the clock that it has on the receive side. The upper layer software will see QL_PRC on one port and QL_EEC on other and will need to enable clock output from the port that received QL_PRC, as it's the higher clock class. Once the EEC reports Locked state all other ports that are traceable to a given EEC (either using the DPLL subsystem, or the config file) will start reporting QL_PRC to downstream devices. > - It's not clear what enabling several pins means, and it's not clear > whether this genericity is not going to be an issue in the future when > we know what enabling more pins means. It means that the recovered frequency will appear on 2 or more physical pins of the package. > - No way as a user to tell whether two interfaces that report the same > pins are actually connected to the same EEC. How many EEC's are there, > in the system, anyway? For now we can fix that with the config file, for future we will be able to trace them to the same EEC. It's like BC in PTP - you can rely on automated mode, but can also override it with the config file. > In particular, I think that the proposed UAPIs should belong to a DPLL > object. That object must know about the pins, so have it enumerate them. > That object needs to know about which pin/s to track, so configure it > there. That object has the state, so have it report it. Really, it looks > basically 1:1 vs. the proposed API, except the object over which the > UAPIs should be defined is a DPLL, not a netdev. RCLK pin API does not belong to the DPLL and never will. That part will always belong to the netdev.
next prev parent reply other threads:[~2021-11-15 10:25 UTC|newest] Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-10 11:44 [PATCH v3 net-next 0/6] Add RTNL interface for SyncE Maciej Machnikowski 2021-11-10 11:44 ` [Intel-wired-lan] " Maciej Machnikowski 2021-11-10 11:44 ` [PATCH v3 net-next 1/6] ice: add support detecting features based on netlist Maciej Machnikowski 2021-11-10 11:44 ` [Intel-wired-lan] " Maciej Machnikowski 2021-11-10 11:44 ` [PATCH v3 net-next 2/6] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Maciej Machnikowski 2021-11-10 11:44 ` [Intel-wired-lan] " Maciej Machnikowski 2021-11-11 16:01 ` Sabrina Dubroca 2021-11-11 16:01 ` [Intel-wired-lan] " Sabrina Dubroca 2021-11-11 16:22 ` Florian Westphal 2021-11-11 16:22 ` [Intel-wired-lan] " Florian Westphal 2021-11-16 14:40 ` Machnikowski, Maciej 2021-11-16 14:40 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-11-16 15:41 ` Florian Westphal 2021-11-16 15:41 ` [Intel-wired-lan] " Florian Westphal 2021-11-16 19:30 ` Jakub Kicinski 2021-11-16 19:30 ` [Intel-wired-lan] " Jakub Kicinski 2021-11-16 14:37 ` Machnikowski, Maciej 2021-11-16 14:37 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-11-10 11:44 ` [PATCH v3 net-next 3/6] ice: add support for reading SyncE DPLL state Maciej Machnikowski 2021-11-10 11:44 ` [Intel-wired-lan] " Maciej Machnikowski 2021-11-10 11:44 ` [PATCH v3 net-next 4/6] rtnetlink: Add support for SyncE recovered clock configuration Maciej Machnikowski 2021-11-10 11:44 ` [Intel-wired-lan] " Maciej Machnikowski 2021-11-10 11:44 ` [PATCH v3 net-next 5/6] ice: add support for SyncE recovered clocks Maciej Machnikowski 2021-11-10 11:44 ` [Intel-wired-lan] " Maciej Machnikowski 2021-11-10 11:44 ` [PATCH v3 net-next 6/6] docs: net: Add description of SyncE interfaces Maciej Machnikowski 2021-11-10 11:44 ` [Intel-wired-lan] " Maciej Machnikowski 2021-11-11 12:43 ` Petr Machata 2021-11-11 12:43 ` [Intel-wired-lan] " Petr Machata 2021-11-15 10:24 ` Machnikowski, Maciej [this message] 2021-11-15 10:24 ` Machnikowski, Maciej 2021-11-16 11:07 ` Petr Machata 2021-11-16 11:07 ` [Intel-wired-lan] " Petr Machata 2021-11-16 11:52 ` Petr Machata 2021-11-16 11:52 ` [Intel-wired-lan] " Petr Machata 2021-11-16 14:26 ` Machnikowski, Maciej 2021-11-16 14:26 ` [Intel-wired-lan] " Machnikowski, Maciej
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=MW5PR11MB5812E733ED2BB621A3249CD5EA989@MW5PR11MB5812.namprd11.prod.outlook.com \ --to=maciej.machnikowski@intel.com \ --cc=abyagowi@fb.com \ --cc=anthony.l.nguyen@intel.com \ --cc=davem@davemloft.net \ --cc=idosch@idosch.org \ --cc=intel-wired-lan@lists.osuosl.org \ --cc=kuba@kernel.org \ --cc=linux-kselftest@vger.kernel.org \ --cc=michael.chan@broadcom.com \ --cc=mkubecek@suse.cz \ --cc=netdev@vger.kernel.org \ --cc=petrm@nvidia.com \ --cc=richardcochran@gmail.com \ --cc=saeed@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.