Message-ID: <CACLnSDikAToGRvfZAhTcT0NCtMj+N9z-GGzRQ5qmpHsvCr2QSA@mail.gmail.com>
Date: Sat, 10 Jan 2026 23:29:16 +0200
From: Vitaly Grinberg <vgrinber@...hat.com>
To: "Nitka, Grzegorz" <grzegorz.nitka@...el.com>
Cc: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>, 
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, 
	"Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>, "horms@...nel.org" <horms@...nel.org>, 
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>, 
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "pmenzel@...gen.mpg.de" <pmenzel@...gen.mpg.de>, 
	"Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>
Subject: Re: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for
 unmanaged DPLL on E830 NIC

Hi Grzegors,
Thanks very much for your reply! Added some clarifications inline.

On Wed, Jan 7, 2026 at 11:33 PM Nitka, Grzegorz
<grzegorz.nitka@...el.com> wrote:
>
> > -----Original Message-----
> > From: Vitaly Grinberg <vgrinber@...hat.com>
> > Sent: Tuesday, December 16, 2025 3:42 PM
> > To: Nitka, Grzegorz <grzegorz.nitka@...el.com>
> > Cc: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>; Nguyen,
> > Anthony L <anthony.l.nguyen@...el.com>; Kubalewski, Arkadiusz
> > <arkadiusz.kubalewski@...el.com>; horms@...nel.org; intel-wired-
> > lan@...ts.osuosl.org; linux-doc@...r.kernel.org; linux-
> > kernel@...r.kernel.org; netdev@...r.kernel.org;
> > pmenzel@...gen.mpg.de; Kitszel, Przemyslaw
> > <przemyslaw.kitszel@...el.com>
> > Subject: Re:[Intel-wired-lan] [PATCH v5 iwl-next] ice: add support for
> > unmanaged DPLL on E830 NIC
> >
> > Will a notification be provided when the lock is re-acquired?
> >
>
> Hi Vitaly, thanks for your comments.
> We discussed it offline already, but I think I need more clarifications.
>
> Regarding above question ... yes, 'lock' recovery shall be reported in the same way.
> Maybe the name of health status is a little bit misleading (ICE_AQC_HEALTH_STATUS_INFO_LOSS_OF_LOCK),
> However health_info struct contains the current lock status (either 'locked' or 'unlocked').

Great, thanks for clarifying this!

> > Another concern is the absence of periodical pin notifications. With the E810,
> > users received the active pin notifications every 1 second. However, the
> > unmanaged DPLL appears to lack this functionality. User implementations
> > currently rely on these periodical notifications to derive the overall clock
> > state, metrics and events from the phase offset. It seems that unmanaged
> > DPLL users will be forced to support two distinct types of DPLLs: one that
> > sends periodical pin notifications and one that does not. Crucially, this
> > difference does not appear to be reflected in the device capabilities,
> > meaning users cannot know in advance whether to expect these
> > notifications.
>
> After reading it one more time, I'm not sure if I get it right in the first place.
> With this patch implementation, there is dpll change notification applied.
> By dpll notification I mean calling dpll_device_change_ntf function.
> Isn't it what you're looking for?
> Notification is triggered only in case when lock status has changed.
> It's unmanaged DPLL so the implementation is a little bit simplified, based on FW notification.
> There is no need for polling thread like it's done for E810.
> But even in case of E810, where polling is applied (2 samples per second), notification is triggered only in case of
> dpll/pin status change, not every 1 second.
> So please clarify, so either I don't understand the question (please note, I'm only covering the main author)
> or notification mechanism, at least about dpll lock state, is already implemented.
>

Yes, device lock status change notification is definitely what we are
looking for, but there is more. Let me clarify the user perspective.
The e810-based telco system relies on both device and pin
notifications. Phase offset included in pin notifications is critical
because the e810 DPLL "Locked" state is too coarse for Telco
requirements.
It is true that pin notifications are only sent on change; however,
since the phase offset varies slightly with every measurement, the
driver detects a change every second. This effectively turns the
event-driven notification into a periodic one. The e810-based
application strongly relies on the fact that phase offset
notifications are unsolicited and the driver sends them from time to
time.
Now, with the unmanaged DPLL, no pin notification will be sent. Last
time I checked, the device and pin information looked like this:
Device:
 {'clock-id': 1165870453030569040,
  'id': 4,
  'lock-status': 'locked',
  'mode': 'automatic',
  'mode-supported': ['automatic'],
  'module-name': 'ice',
  'type': 'eec'},

Input pin:
{
  "id": 17,
  "module-name": "ice",
  "clock-id": 1165870453030569040,
  "board-label": "1588-TIME_SYNC",
  "type": "ext",
  "capabilities": [],
  "frequency": 10000000,
  "phase-adjust-min": 0,
  "phase-adjust-max": 0,
  "parent-device": [
    {
      "parent-id": 4,
      "state": "connected",
      "direction": "input"
    }
  ]
}

I see a few challenges for the user here. The biggest one is that the
application can't tell the difference between a device that will
report phase offsets and this unmanaged device that never will.
A possible way forward would be adding a capability flag to the DPLL
API so users don't have to guess.
However, the preferred solution would be to simply mirror the E810
behavior (sending phase offset). This preserves the existing API
contract and prevents users, who have already built applications for
this interface, from needing to implement special handling for a new
hardware variant that behaves differently.
There are additional inconsistencies in the existing structure I
wanted to bring to your attention.
1. I'm not entirely sure how a 1588-TIME_SYNC pin can have a parent
device of type "eec". EEC is all about frequency synchronization, and
yet the pin named 1588-TIME_SYNC is clearly a phase pin. This also
doesn't play well with existing implementations, where EEC circuits
deal with frequency, PPS circuits deal with phase, and there is clear
distinction between the two with regard to the meaning of "being
locked".
2. Since it is also an external embedded sync input pin, could it be
possible to expose this information and include `esync-frequency` and
`esync-pulse`? That could be useful for configuring the leading DPLL
that drives the unmanaged one.

Powered by blists - more mailing lists