RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks

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>,
	"Kubalewski, Arkadiusz" <arkadiusz.kubalewski@intel.com>,
	"richardcochran@gmail.com" <richardcochran@gmail.com>,
	"Byagowi, Ahmad" <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>,
	"mkubecek@suse.cz" <mkubecek@suse.cz>,
	"saeed@kernel.org" <saeed@kernel.org>,
	"michael.chan@broadcom.com" <michael.chan@broadcom.com>,
	"petrm@nvidia.com" <petrm@nvidia.com>,
	"Vadim Fedorenko" <vfedorenko@novek.ru>
Subject: RE: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
Date: Wed, 15 Dec 2021 12:13:47 +0000	[thread overview]
Message-ID: <MW5PR11MB5812E5A30C05E2F1EAB2D9D5EA769@MW5PR11MB5812.namprd11.prod.outlook.com> (raw)
In-Reply-To: <YbXhXstRpzpQRBR8@shredder>

> -----Original Message-----
> From: Ido Schimmel <idosch@idosch.org>
> Sent: Sunday, December 12, 2021 12:48 PM
> To: Machnikowski, Maciej <maciej.machnikowski@intel.com>
> Subject: Re: [PATCH v5 net-next 0/4] Add ethtool interface for RClocks
> 
> On Fri, Dec 10, 2021 at 02:45:46PM +0100, Maciej Machnikowski wrote:
> > Synchronous Ethernet networks use a physical layer clock to syntonize
> > the frequency across different network elements.
> >
> > Basic SyncE node defined in the ITU-T G.8264 consist of an Ethernet
> > Equipment Clock (EEC) and have the ability to synchronize to reference
> > frequency sources.
> >
> > This patch series is a prerequisite for EEC object and adds ability
> > to enable recovered clocks in the physical layer of the netdev object.
> > Recovered clocks can be used as one of the reference signal by the EEC.
> 
> The dependency is the other way around. It doesn't make sense to add
> APIs to configure the inputs of an object that doesn't exist. First add
> the EEC object, then we can talk about APIs to configure its inputs from
> netdevs.

This API configures frequency outputs of the PTY layer of
a PHY/integrated MAC. It does not configure any inputs nor it interacts
with the EEC. The goal of it is to expose the clock to the piece that
requires it as a reference one (a DPLL/FPGA/anything else).

I don't agree with the statement that we must have EEC object first,
as we can already configure different frequency sources using different
subsystems. The source of signal should be separated from its
consumer.

> With these four patches alone, user space doesn't know how many EECs
> there are in the system, it doesn't know the mapping from netdev to EEC,
> it doesn't know the state of the EEC, it doesn't know which source is
> chosen in case more than one source is enabled. Patch #3 tries to work
> around it by having ice print to kernel log, when the information should
> really be exposed via the EEC object.

The goal of them is to add API for recovered clocks - not for EECs. This part 
is there for observability and will still be there when EEC is in place.
Those will need to be addressed by the DPLL subsystem.

> +		dev_warn(ice_pf_to_dev(pf),
> +			 "<DPLL%i> state changed to: %d, pin %d",
> +			 ICE_CGU_DPLL_SYNCE,
> +			 pf->synce_dpll_state,
> +			 pin);
> 
> >
> > Further work is required to add the DPLL subsystem, link it to the
> > netdev object and create API to read the EEC DPLL state.
> 
> When the EEC object materializes, we might find out that this API needs
> to be changed / reworked / removed, but we won't be able to do that
> given it's uAPI. I don't know where the confidence that it won't happen
> stems from when there are so many question marks around this new
> object.

This API follows the functionality of other frequency outputs that exist
in the kernel, like PTP period file for frequency output of PTP clock
or other GPIOs. I highly doubt it'll change - we may extend it to add mapping
between pins, but like I indicated - this will not always be known to SW.

Regards
Maciek