| Message ID | 20210829080512.3573627-1-maciej.machnikowski@intel.com |
|---|---|
| Headers | show |
| Series | Add RTNL interface for SyncE | expand |
> -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: Sunday, August 29, 2021 5:10 PM > Subject: Re: [RFC v2 net-next 1/2] rtnetlink: Add new RTM_GETSYNCESTATE > message to get SyncE status > > On Sun, Aug 29, 2021 at 10:05:11AM +0200, Maciej Machnikowski wrote: > > > > This patch is SyncE-oriented. Future implementation can add additional > > functionality for reading different DPLL states using the same structure. > > I would call this more "ice oriented" than SyncE oriented. I'm not sure there is > even such a thing as "SyncE DPLL". Does that term come from 802.3? To my > understanding, that is one just way of implementing it that works on super- > Gigabit speed devices. > Hi, This interface is ITU-T G.8264 SyncE-oriented. It is meant to monitor the state of Ethernet Equipment Clock. ITU-T G.8264 recommendation defines Synchronous Ethernet equipment as a device equipped with a system clock (e.g., a synchronous Ethernet equipment clock). SyncE interfaces are able to extract the received clock and pass it to a system clock. Please take a look at the 10.2 Operation modes of the G.8264 and at the Figure A.1 which depicts the EEC. This interface is to report the status of the EEC. If you prefer EEC over DPLL I'm fine with the name change. I think it will be less confusing. > I have nothing against exposing the DPLL if you need to, however I'd like to have > an interface that support plain Gigabit as well. This could be done in a generic > way by offering Control Register 9 as described in 802.3. This part of Gigabit interface is a different part of SyncE device. It controls Master/Slave operation of auto-negotiation. You would use it in slave mode if you want your EEC to tune to the frequency recovered from network and to master if you use external source for your EEC and want to send it as a reference for another devices. The decision can be made based on the EEC state read by the interface proposed in this RFC. This is a functionality that belongs to a different interface mentioned in the next steps. Regards Maciek
On Sun, Aug 29, 2021 at 04:42:55PM +0000, Machnikowski, Maciej wrote: > Please take a look at the 10.2 Operation modes of the G.8264 and at the Figure A.1 > which depicts the EEC. This interface is to report the status of the EEC. Well, I read it, and it is still fairly high level with no mention at all of "DPLL". I hope that the new RTNL states will cover other possible EEC implementations, too. The "Reference source selection mechanism" is also quite vague. Your patch is more specific: +enum if_eec_src { + IF_EEC_SRC_INVALID = 0, + IF_EEC_SRC_UNKNOWN, + IF_EEC_SRC_SYNCE, + IF_EEC_SRC_GNSS, + IF_EEC_SRC_PTP, + IF_EEC_SRC_EXT, + __IF_EEC_SRC_MAX, +}; But I guess your list is reasonable. It can always be expanded, right? > If you prefer EEC over DPLL I'm fine with the name change. I think it will be less confusing. Yes, thanks for doing that. Thanks, Richard
On Mon, 30 Aug 2021 13:57:58 -0700 Richard Cochran wrote: > > Please take a look at the 10.2 Operation modes of the G.8264 and at the Figure A.1 > > which depicts the EEC. This interface is to report the status of the EEC. > > Well, I read it, and it is still fairly high level with no mention at > all of "DPLL". I hope that the new RTNL states will cover other > possible EEC implementations, too. > > The "Reference source selection mechanism" is also quite vague. Your > patch is more specific: > > +enum if_eec_src { > + IF_EEC_SRC_INVALID = 0, > + IF_EEC_SRC_UNKNOWN, > + IF_EEC_SRC_SYNCE, > + IF_EEC_SRC_GNSS, Hmm, IDK if this really belongs in RTNL. The OCP time card that Jonathan works on also wants to report signal lock, and it locks to GNSS. It doesn't have any networking functionality whatsoever. Can we add a genetlink family for clock info/configuration? From what I understood discussing this with Jonathan it sounded like most clocks today have a vendor-specific character device for configuration and reading status. I'm happy to write the plumbing if this seems like an okay idea but too much work for anyone to commit. > + IF_EEC_SRC_PTP, > + IF_EEC_SRC_EXT, > + __IF_EEC_SRC_MAX, > +}; > > But I guess your list is reasonable. It can always be expanded, right?
On Tue, 31 Aug 2021 10:20:18 +0000 Machnikowski, Maciej wrote: > > Hmm, IDK if this really belongs in RTNL. The OCP time card that > > Jonathan works on also wants to report signal lock, and it locks > > to GNSS. It doesn't have any networking functionality whatsoever. > > > > Can we add a genetlink family for clock info/configuration? From > > what I understood discussing this with Jonathan it sounded like most > > clocks today have a vendor-specific character device for configuration > > and reading status. > > > > I'm happy to write the plumbing if this seems like an okay idea > > but too much work for anyone to commit. > > > > I agree that this also is useful for Time card, yet it's also useful here. > PTP subsystem should implement a similar logic to this one for > DPLL-driven timers which can lock its frequency to external sources. Why would we have two APIs for doing the same thing? IIUC Richard does not want this in the PTP ioctls which is fair, but we need to cater to devices which do not have netdevs. > The reasoning behind putting it here is to enable returning the lock > to the GNSS receiver embedded on the NIC as a source for the > SyncE frequency. It helps distinguishing the embedded GNSS > from the external sources. As a result - the upper layer can report > GNSS lock based only on this message without the need to put the > embedded GNSS receiver in the config file. On the other hand - if > sync to External source is reported such SW would need to read > the source of external sync from the config file. > > And the list is expandable - if we need to define more embedded > sync source types we can always add more to it.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Tuesday, August 31, 2021 3:33 PM > Subject: Re: [RFC v2 net-next 1/2] rtnetlink: Add new RTM_GETSYNCESTATE > message to get SyncE status > > On Tue, 31 Aug 2021 10:20:18 +0000 Machnikowski, Maciej wrote: > > > Hmm, IDK if this really belongs in RTNL. The OCP time card that > > > Jonathan works on also wants to report signal lock, and it locks to > > > GNSS. It doesn't have any networking functionality whatsoever. > > > > > > Can we add a genetlink family for clock info/configuration? From > > > what I understood discussing this with Jonathan it sounded like most > > > clocks today have a vendor-specific character device for > > > configuration and reading status. > > > > > > I'm happy to write the plumbing if this seems like an okay idea but > > > too much work for anyone to commit. > > > > > > > I agree that this also is useful for Time card, yet it's also useful here. > > PTP subsystem should implement a similar logic to this one for > > DPLL-driven timers which can lock its frequency to external sources. > > Why would we have two APIs for doing the same thing? IIUC Richard does > not want this in the PTP ioctls which is fair, but we need to cater to devices > which do not have netdevs.
On Tue, 31 Aug 2021 14:07:32 +0000 Machnikowski, Maciej wrote: > > > I agree that this also is useful for Time card, yet it's also useful here. > > > PTP subsystem should implement a similar logic to this one for > > > DPLL-driven timers which can lock its frequency to external sources. > > > > Why would we have two APIs for doing the same thing? IIUC Richard does > > not want this in the PTP ioctls which is fair, but we need to cater to devices > > which do not have netdevs. > > From technical point of view - it can be explained by the fact that the DPLL > driving the SyncE logic can be separate from the one driving PTP. Also > SyncE is frequency-only oriented and doesn't care about phase and > Time of Day that PTP also needs. The GNSS lock on the PTP side will be > multi-layered, as the full lock would mean that our PTP clock is not only > syntonized, but also has its time and phase set correctly. Just because GNSS lock addresses more parameters (potentially) doesn't mean the syntonization part shouldn't be addressed by the same API. > A PTP can reuse the "physical" part of this interface later on, but it also needs > to solve more SW-specific challenges, like reporting the PTP lock on a SW level. > > I agree that having such API for PTP subsystem will be very useful, > but let's address SyncE in netdev first and build the PTP netlink on top of what > we learn here. We can always move the structures defined here to the layer > above without affecting any APIs. It's a reasonable SW design strategy to start simple. Unfortunately, it doesn't apply to stable uAPI design. You're adding a RTNL op, which will have to be supported for ever. If we add anything "later" it will be a strict addition, and will have to be backward compatible. Which I'm not sure how to do when the object we'd operate on would be completely different (clock vs netdev). As I said I can write the boilerplate code for you if you prefer, the code implementing the command and the driver interface will be almost identical. Is there a reason why RTNL is better?
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Tuesday, August 31, 2021 4:18 PM > To: Machnikowski, Maciej <maciej.machnikowski@intel.com> > Subject: Re: [RFC v2 net-next 1/2] rtnetlink: Add new RTM_GETSYNCESTATE > message to get SyncE status > > On Tue, 31 Aug 2021 14:07:32 +0000 Machnikowski, Maciej wrote: > > > > I agree that this also is useful for Time card, yet it's also useful here. > > > > PTP subsystem should implement a similar logic to this one for > > > > DPLL-driven timers which can lock its frequency to external sources. > > > > > > Why would we have two APIs for doing the same thing? IIUC Richard > does > > > not want this in the PTP ioctls which is fair, but we need to cater to > devices > > > which do not have netdevs. > > > > From technical point of view - it can be explained by the fact that the DPLL > > driving the SyncE logic can be separate from the one driving PTP. Also > > SyncE is frequency-only oriented and doesn't care about phase and > > Time of Day that PTP also needs. The GNSS lock on the PTP side will be > > multi-layered, as the full lock would mean that our PTP clock is not only > > syntonized, but also has its time and phase set correctly. > > Just because GNSS lock addresses more parameters (potentially) doesn't > mean the syntonization part shouldn't be addressed by the same API. Fair enough. > > > A PTP can reuse the "physical" part of this interface later on, but it also > needs > > to solve more SW-specific challenges, like reporting the PTP lock on a SW > level. > > > > I agree that having such API for PTP subsystem will be very useful, > > but let's address SyncE in netdev first and build the PTP netlink on top of > what > > we learn here. We can always move the structures defined here to the > layer > > above without affecting any APIs. > > It's a reasonable SW design strategy to start simple. Unfortunately, > it doesn't apply to stable uAPI design. You're adding a RTNL op, which > will have to be supported for ever. If we add anything "later" it will > be a strict addition, and will have to be backward compatible. Which > I'm not sure how to do when the object we'd operate on would be > completely different (clock vs netdev). I agree - the point I'm trying to make here is that the existence of the PTP-specific interface will not invalidate the need of having SyncE-specific one as well. Even if we report lock-states for the clock we will still need to report lock-states for devices that don't use PTP clocks, but support SyncE. (that's also a reason why RTNL is still required). The RTNL interface will also address devices that only need the frequency syntonization (especially in Radio Access Networks). > > As I said I can write the boilerplate code for you if you prefer, the > code implementing the command and the driver interface will be almost > identical. I think it's a great idea to start that in parallel to this patch. Then move the common structures to the generic layer and use them in both SyncE-specific RTNL implementation and PTP-specific part that will be added. This won't affect SyncE specific APIs. The "worst" that can happen is that the driver will put the same info for PTP part and SyncE part if that's the design someone follows. Regards Maciek > > Is there a reason why RTNL is better?
On Mon, Aug 30, 2021 at 04:29:09PM -0700, Jakub Kicinski wrote: > Hmm, IDK if this really belongs in RTNL. The OCP time card that > Jonathan works on also wants to report signal lock, and it locks > to GNSS. It doesn't have any networking functionality whatsoever. > > Can we add a genetlink family for clock info/configuration? From > what I understood discussing this with Jonathan it sounded like most > clocks today have a vendor-specific character device for configuration > and reading status. > > I'm happy to write the plumbing if this seems like an okay idea > but too much work for anyone to commit. This sounds nice. As you said later on in this thread, any API we merge now will have to last. That is why I'm being so picky here. We want new APIs to cover current HW _and_ be reasonable for the future. I don't see a DPLL as either a PTP Hardware Clock or a Network Device. It is a PLL. The kernel can and should have a way to represent the relationship between these three different kind of IP block. We already have a way to get from PHC to netdev interface. I understand that Maciej and team want to get support for their card ASAP. However, proper kernel/user API takes time. For example, the PHC stuff took one year and fourteen revisions. But it was worth the wait, because the API has help up pretty well all these years since the v3.0 kernel. There is no need to quickly merge some poorly designed interfaces. Thanks, Richard
On Tue, 31 Aug 2021 09:19:27 -0700 Richard Cochran wrote: > As you said later on in this thread, any API we merge now will have to > last. That is why I'm being so picky here. We want new APIs to cover > current HW _and_ be reasonable for the future. > > I don't see a DPLL as either a PTP Hardware Clock or a Network > Device. It is a PLL. > > The kernel can and should have a way to represent the relationship > between these three different kind of IP block. We already have a > way to get from PHC to netdev interface. Makes sense to me. I was wondering how to split things at high level into the areas you mentioned, but TBH the part I'm struggling with is the delineation of what falls under PTP. PLL by itself seems like an awfully small unit to create a subsystem for, and PTP already has aux stuff like PIN control. Then there's the whole bunch of stuff that Jonathan is adding via driver specific sysfs interfaces [1]. I was hoping the "new API" would cover his need but PLL would be a tiny part of it. IOW after looking at the code I'm not so sure how to reasonably divide things. [1] https://lore.kernel.org/netdev/20210830235236.309993-1-jonathan.lemon@gmail.com/
On Tue, 31 Aug 2021 22:09:18 +0000 Machnikowski, Maciej wrote: > OK I can strip down the RTNL EEC state interface to only report > the state without any extras, like pin. Next step would be to add > the control over recovered clock also to the netdev subsystem. > > The EEC state read is needed for recovered/source clock validation > and that's why I think it belongs to the RTNL part as it gates the QL > for each port. If you mean just reporting state and have a syncE on/off without any option for other sources that's fine by me.