Message-ID: <LV4PR11MB9491EB644FC83676522107669B8FA@LV4PR11MB9491.namprd11.prod.outlook.com>
Date: Wed, 14 Jan 2026 10:23:25 +0000
From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To: "Grinberg, Vitaly" <vgrinber@...hat.com>, "Nitka, Grzegorz"
	<grzegorz.nitka@...el.com>
CC: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>, "Nguyen, Anthony
 L" <anthony.l.nguyen@...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

>From: Vitaly Grinberg <vgrinber@...hat.com>
>Sent: Saturday, January 10, 2026 10:29 PM
>
>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.

There is no phase-offset field as pointed in the above example.
No 'phase-offset' attribute -> no such capability.
Why isn’t that enough?

>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.

This is not currently possible from driver perspective.
We miss the FW API for it.

>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".

This dpll device type was established based on the main purpose of dpll
device which is to drive the network ports phy clocks with it.

>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.

Sure, esync caps should be provided, as the commit message example shown:
+    'esync-frequency': 1,
+    'esync-frequency-supported': [{'frequency-max': 1, 'frequency-min': 1}],
+    'esync-pulse': 25,

Thank you!
Arkadiusz

Powered by blists - more mailing lists