From: "Machnikowski, Maciej" <maciej.machnikowski@intel.com> To: Ido Schimmel <idosch@idosch.org> 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> Subject: RE: [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Date: Tue, 21 Sep 2021 13:37:37 +0000 [thread overview] Message-ID: <PH0PR11MB4951E98FCEC0F1EA230BA1DAEAA19@PH0PR11MB4951.namprd11.prod.outlook.com> (raw) In-Reply-To: <YUnbCzBOPP9hWQ5c@shredder> > -----Original Message----- > From: Ido Schimmel <idosch@idosch.org> > Sent: Tuesday, September 21, 2021 3:16 PM > To: Machnikowski, Maciej <maciej.machnikowski@intel.com> > Subject: Re: [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE > message to get SyncE status > > On Fri, Sep 03, 2021 at 05:14:35PM +0200, Maciej Machnikowski wrote: > > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > > index eebd3894fe89..78a8a5af8cd8 100644 > > --- a/include/uapi/linux/if_link.h > > +++ b/include/uapi/linux/if_link.h > > @@ -1273,4 +1273,35 @@ enum { > > > > #define IFLA_MCTP_MAX (__IFLA_MCTP_MAX - 1) > > > > +/* SyncE section */ > > + > > +enum if_eec_state { > > + IF_EEC_STATE_INVALID = 0, > > + IF_EEC_STATE_FREERUN, > > + IF_EEC_STATE_LOCKACQ, > > + IF_EEC_STATE_LOCKREC, > > + IF_EEC_STATE_LOCKED, > > + IF_EEC_STATE_HOLDOVER, > > + IF_EEC_STATE_OPEN_LOOP, > > + __IF_EEC_STATE_MAX, > > Can you document these states? I'm not clear on what LOCKACQ, LOCKREC > and OPEN_LOOP mean. I also don't see ice using them and it's not really > a good practice to add new uAPI without any current users. > I'll fix that enum to use generic states defined in G.781 which are limited to: - Freerun - LockedACQ (locked, acquiring holdover memory) - Locked (locked with holdover acquired) - Holdover > From v1 I understand that these states were copied from commit > 797d3186544f ("ptp: ptp_clockmatrix: Add wait_for_sys_apll_dpll_lock.") > from Renesas. > > Figure 11 in the following PDF describes the states, but it seems > specific to the particular device and probably shouldn't be exposed to > user space as-is: > https://www.renesas.com/us/en/document/dst/8a34041-datasheet > > I have a few questions about this being a per-netdev attribute: > > 1. My understanding is that in the multi-port adapter you are working > with you have a single EEC that is used to set the Tx frequency of all > the ports. Is that correct? That's correct. > 2. Assuming the above is correct, is it possible that one port is in > LOCKED state and another (for some reason) is in HOLDOVER state? If yes, > then I agree about this being a per-netdev attribute. The interface can > also be extended with another attribute specifying the HOLDOVER reason. > For example, netdev being down. All ports driven by a single EEC will report the same state. > Regardless, I agree with previous comments made about this belonging in > ethtool rather than rtnetlink. Will take a look at it - as it will require support in linuxptp as well. > > +}; > > + > > +#define IF_EEC_STATE_MAX (__IF_EEC_STATE_MAX - 1) > > +#define EEC_SRC_PORT (1 << 0) /* recovered clock from the > port is > > + * currently the source for the EEC > > + */ > > I'm not sure about this one. If we are going to expose this as a > per-netdev attribute (see more below), any reason it can't be added as > another state (e.g., IF_EEC_STATE_LOCKED_SRC)? OK! That's a great idea! Yet we'll need LOCKED_SRC and LOCKED_ACQ_SRC, but still sounds good. > IIUC, in the common case of a simple NE the source of the EEC is always > one of the ports, but in the case of a PRC the source is most likely > external (e.g., GPS). True > If so, I think we need a way to represent the EEC as a separate object > with the ability to set its source and report it via the same interface. > I'm unclear on how exactly an external source looks like, but for the > netdev case maybe something like this: > > devlink clock show [ dev clock CLOCK ] > devlink clock set DEV clock CLOCK [ { src_type SRC_TYPE } ] > SRC_TYPE : = { port DEV/PORT_INDEX } Unfortunately, EEC lives in 2 worlds - it belongs to a netdev (in very simple deployments the EEC may be a part of the PHY and only allow synchronizing the TX frequency to a single lane/port), but also can go outside of netdev and be a boar-wise frequency source. > The only source type above is 'port' with the ability to set the > relevant port, but more can be added. Obviously, 'devlink clock show' > will give you the current source in addition to other information such > as frequency difference with respect to the input frequency. We considered devlink interface for configuring the clock/DPLL, but a new concept was born at the list to add a DPLL subsystem that will cover more use cases, like a TimeCard. > Finally, can you share more info about the relation to the PHC? My > understanding is that one of the primary use cases for SyncE is to drive > all the PHCs in the network using the same frequency. How do you imagine > this configuration is going to look like? Can the above interface be > extended for that? That would be a configurable parameter/option of the PTP program. Just like it can check the existence of link on a given port, it'll also be able to check if we use EEC and it's locked. If it is, and is a source of PHC frequency - the ptp tool can decide to not apply frequency corrections to the PHC, just like the ptp4l does when nullf servo is used, but can do that dynamically. > All of the above might be complete nonsense as I'm still trying to wrap > my head around the subject. It's certainly not! Thanks for your input!
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 net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Date: Tue, 21 Sep 2021 13:37:37 +0000 [thread overview] Message-ID: <PH0PR11MB4951E98FCEC0F1EA230BA1DAEAA19@PH0PR11MB4951.namprd11.prod.outlook.com> (raw) In-Reply-To: <YUnbCzBOPP9hWQ5c@shredder> > -----Original Message----- > From: Ido Schimmel <idosch@idosch.org> > Sent: Tuesday, September 21, 2021 3:16 PM > To: Machnikowski, Maciej <maciej.machnikowski@intel.com> > Subject: Re: [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE > message to get SyncE status > > On Fri, Sep 03, 2021 at 05:14:35PM +0200, Maciej Machnikowski wrote: > > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > > index eebd3894fe89..78a8a5af8cd8 100644 > > --- a/include/uapi/linux/if_link.h > > +++ b/include/uapi/linux/if_link.h > > @@ -1273,4 +1273,35 @@ enum { > > > > #define IFLA_MCTP_MAX (__IFLA_MCTP_MAX - 1) > > > > +/* SyncE section */ > > + > > +enum if_eec_state { > > + IF_EEC_STATE_INVALID = 0, > > + IF_EEC_STATE_FREERUN, > > + IF_EEC_STATE_LOCKACQ, > > + IF_EEC_STATE_LOCKREC, > > + IF_EEC_STATE_LOCKED, > > + IF_EEC_STATE_HOLDOVER, > > + IF_EEC_STATE_OPEN_LOOP, > > + __IF_EEC_STATE_MAX, > > Can you document these states? I'm not clear on what LOCKACQ, LOCKREC > and OPEN_LOOP mean. I also don't see ice using them and it's not really > a good practice to add new uAPI without any current users. > I'll fix that enum to use generic states defined in G.781 which are limited to: - Freerun - LockedACQ (locked, acquiring holdover memory) - Locked (locked with holdover acquired) - Holdover > From v1 I understand that these states were copied from commit > 797d3186544f ("ptp: ptp_clockmatrix: Add wait_for_sys_apll_dpll_lock.") > from Renesas. > > Figure 11 in the following PDF describes the states, but it seems > specific to the particular device and probably shouldn't be exposed to > user space as-is: > https://www.renesas.com/us/en/document/dst/8a34041-datasheet > > I have a few questions about this being a per-netdev attribute: > > 1. My understanding is that in the multi-port adapter you are working > with you have a single EEC that is used to set the Tx frequency of all > the ports. Is that correct? That's correct. > 2. Assuming the above is correct, is it possible that one port is in > LOCKED state and another (for some reason) is in HOLDOVER state? If yes, > then I agree about this being a per-netdev attribute. The interface can > also be extended with another attribute specifying the HOLDOVER reason. > For example, netdev being down. All ports driven by a single EEC will report the same state. > Regardless, I agree with previous comments made about this belonging in > ethtool rather than rtnetlink. Will take a look at it - as it will require support in linuxptp as well. > > +}; > > + > > +#define IF_EEC_STATE_MAX (__IF_EEC_STATE_MAX - 1) > > +#define EEC_SRC_PORT (1 << 0) /* recovered clock from the > port is > > + * currently the source for the EEC > > + */ > > I'm not sure about this one. If we are going to expose this as a > per-netdev attribute (see more below), any reason it can't be added as > another state (e.g., IF_EEC_STATE_LOCKED_SRC)? OK! That's a great idea! Yet we'll need LOCKED_SRC and LOCKED_ACQ_SRC, but still sounds good. > IIUC, in the common case of a simple NE the source of the EEC is always > one of the ports, but in the case of a PRC the source is most likely > external (e.g., GPS). True > If so, I think we need a way to represent the EEC as a separate object > with the ability to set its source and report it via the same interface. > I'm unclear on how exactly an external source looks like, but for the > netdev case maybe something like this: > > devlink clock show [ dev clock CLOCK ] > devlink clock set DEV clock CLOCK [ { src_type SRC_TYPE } ] > SRC_TYPE : = { port DEV/PORT_INDEX } Unfortunately, EEC lives in 2 worlds - it belongs to a netdev (in very simple deployments the EEC may be a part of the PHY and only allow synchronizing the TX frequency to a single lane/port), but also can go outside of netdev and be a boar-wise frequency source. > The only source type above is 'port' with the ability to set the > relevant port, but more can be added. Obviously, 'devlink clock show' > will give you the current source in addition to other information such > as frequency difference with respect to the input frequency. We considered devlink interface for configuring the clock/DPLL, but a new concept was born at the list to add a DPLL subsystem that will cover more use cases, like a TimeCard. > Finally, can you share more info about the relation to the PHC? My > understanding is that one of the primary use cases for SyncE is to drive > all the PHCs in the network using the same frequency. How do you imagine > this configuration is going to look like? Can the above interface be > extended for that? That would be a configurable parameter/option of the PTP program. Just like it can check the existence of link on a given port, it'll also be able to check if we use EEC and it's locked. If it is, and is a source of PHC frequency - the ptp tool can decide to not apply frequency corrections to the PHC, just like the ptp4l does when nullf servo is used, but can do that dynamically. > All of the above might be complete nonsense as I'm still trying to wrap > my head around the subject. It's certainly not! Thanks for your input!
next prev parent reply other threads:[~2021-09-21 13:51 UTC|newest] Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-03 15:14 [RFC v4 net-next 0/2] Add RTNL interface for SyncE Maciej Machnikowski 2021-09-03 15:14 ` [Intel-wired-lan] " Maciej Machnikowski 2021-09-03 15:14 ` [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Maciej Machnikowski 2021-09-03 15:14 ` [Intel-wired-lan] " Maciej Machnikowski 2021-09-03 16:18 ` Stephen Hemminger 2021-09-03 16:18 ` [Intel-wired-lan] " Stephen Hemminger 2021-09-03 22:20 ` Machnikowski, Maciej 2021-09-03 22:20 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-03 22:14 ` Jakub Kicinski 2021-09-03 22:14 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-06 18:30 ` Machnikowski, Maciej 2021-09-06 18:30 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-06 18:39 ` Jakub Kicinski 2021-09-06 18:39 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-06 19:01 ` Machnikowski, Maciej 2021-09-06 19:01 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-07 1:01 ` Jakub Kicinski 2021-09-07 1:01 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-07 8:50 ` Machnikowski, Maciej 2021-09-07 8:50 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-07 14:55 ` Jakub Kicinski 2021-09-07 14:55 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-07 15:47 ` Machnikowski, Maciej 2021-09-07 15:47 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-07 19:47 ` Jakub Kicinski 2021-09-07 19:47 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-08 8:03 ` Machnikowski, Maciej 2021-09-08 8:03 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-08 16:21 ` Jakub Kicinski 2021-09-08 16:21 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-08 17:30 ` Machnikowski, Maciej 2021-09-08 17:30 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-08 19:34 ` Andrew Lunn 2021-09-08 19:34 ` [Intel-wired-lan] " Andrew Lunn 2021-09-08 20:27 ` Machnikowski, Maciej 2021-09-08 20:27 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-08 22:20 ` Jakub Kicinski 2021-09-08 22:20 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-08 22:59 ` Andrew Lunn 2021-09-08 22:59 ` [Intel-wired-lan] " Andrew Lunn 2021-09-09 2:09 ` Richard Cochran 2021-09-09 2:09 ` [Intel-wired-lan] " Richard Cochran 2021-09-09 8:18 ` Machnikowski, Maciej 2021-09-09 8:18 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-10 14:14 ` Richard Cochran 2021-09-10 14:14 ` [Intel-wired-lan] " Richard Cochran 2021-09-08 22:18 ` Jakub Kicinski 2021-09-08 22:18 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-08 23:14 ` Andrew Lunn 2021-09-08 23:14 ` [Intel-wired-lan] " Andrew Lunn 2021-09-08 23:58 ` Jakub Kicinski 2021-09-08 23:58 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-09 8:26 ` Machnikowski, Maciej 2021-09-09 8:26 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-09 9:24 ` Machnikowski, Maciej 2021-09-09 9:24 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-09 10:15 ` David Miller 2021-09-09 10:15 ` [Intel-wired-lan] " David Miller 2021-09-09 8:11 ` Machnikowski, Maciej 2021-09-09 8:11 ` [Intel-wired-lan] " Machnikowski, Maciej 2021-09-13 8:50 ` Ido Schimmel 2021-09-13 8:50 ` [Intel-wired-lan] " Ido Schimmel 2021-09-21 13:36 ` Ido Schimmel 2021-09-21 13:36 ` [Intel-wired-lan] " Ido Schimmel 2021-09-21 13:15 ` Ido Schimmel 2021-09-21 13:15 ` [Intel-wired-lan] " Ido Schimmel 2021-09-21 13:37 ` Machnikowski, Maciej [this message] 2021-09-21 13:37 ` Machnikowski, Maciej 2021-09-21 14:58 ` Ido Schimmel 2021-09-21 14:58 ` [Intel-wired-lan] " Ido Schimmel 2021-09-21 21:14 ` Jakub Kicinski 2021-09-21 21:14 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-22 6:22 ` Ido Schimmel 2021-09-22 6:22 ` [Intel-wired-lan] " Ido Schimmel 2021-09-03 15:14 ` [PATCH net-next 2/2] ice: add support for reading SyncE DPLL state Maciej Machnikowski 2021-09-03 15:14 ` [Intel-wired-lan] " Maciej Machnikowski 2021-09-03 22:06 ` Jakub Kicinski 2021-09-03 22:06 ` [Intel-wired-lan] " Jakub Kicinski 2021-09-21 13:25 ` Ido Schimmel 2021-09-21 13:25 ` [Intel-wired-lan] " Ido Schimmel -- strict thread matches above, loose matches on Subject: below -- 2021-08-31 11:52 [PATCH net-next 0/2] Add RTNL interface for SyncE EEC state Maciej Machnikowski 2021-08-31 11:52 ` [PATCH net-next 1/2] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Maciej Machnikowski 2021-08-31 13:44 ` Jakub Kicinski
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=PH0PR11MB4951E98FCEC0F1EA230BA1DAEAA19@PH0PR11MB4951.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=netdev@vger.kernel.org \ --cc=richardcochran@gmail.com \ /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.