RE: [RFC PATCH v7 1/8] dpll: spec: Add Netlink spec in YAML

From: Jiri Pirko <jiri@resnulli.us>
Sent: Monday, May 15, 2023 11:31 AM

Thu, May 11, 2023 at 10:51:43PM CEST, arkadiusz.kubalewski@intel.com wrote:

quoted

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Thursday, May 11, 2023 10:00 AM

Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@intel.com wrote:

quoted

quoted

From: Jakub Kicinski <kuba@kernel.org>
Sent: Thursday, May 4, 2023 11:25 PM

On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote:

quoted

quoted

+definitions:
+  -
+    type: enum
+    name: mode
+    doc: |
+      working-modes a dpll can support, differentiate if and how dpll

selects

quoted

+      one of its sources to syntonize with it, valid values for

DPLL_A_MODE

quoted

+      attribute
+    entries:
+      -
+        name: unspec

In general, why exactly do we need unspec values in enums and CMDs?
What is the usecase. If there isn't please remove.

+1

Sure, fixed.

quoted

quoted

quoted

+        doc: unspecified value
+      -
+        name: manual

I think the documentation calls this "forced", still.

Yes, good catch, fixed docs.

quoted

quoted

quoted

+        doc: source can be only selected by sending a request to dpll
+      -
+        name: automatic
+        doc: highest prio, valid source, auto selected by dpll
+      -
+        name: holdover
+        doc: dpll forced into holdover mode
+      -
+        name: freerun
+        doc: dpll driven on system clk, no holdover available

Remove "no holdover available". This is not a state, this is a mode
configuration. If holdover is or isn't available, is a runtime info.

Agreed, seems a little confusing now. Should we expose the system clk
as a pin to be able to force lock to it? Or there's some extra magic
at play here?

In freerun you cannot lock to anything it, it just uses system clock from
one of designated chip wires (which is not a part of source pins pool) to
feed the dpll. Dpll would only stabilize that signal and pass it further.
Locking itself is some kind of magic, as it usually takes at least ~15
seconds before it locks to a signal once it is selected.

quoted

quoted

quoted

+      -
+        name: nco
+        doc: dpll driven by Numerically Controlled Oscillator

Noob question, what is NCO in terms of implementation?
We source the signal from an arbitrary pin and FW / driver does
the control? Or we always use system refclk and then tune?

Documentation of chip we are using, stated NCO as similar to FREERUN, and
it

So how exactly this is different to freerun? Does user care or he would
be fine with "freerun" in this case? My point is, isn't "NCO" some
device specific thing that should be abstracted out here?

Sure, it is device specific, some synchronizing circuits would have this
capability, while others would not.
Should be abstracted out? It is a good question.. shall user know that he is
in
freerun with possibility to control the frequency or not?
Let's say we remove NCO, and have dpll with enabled FREERUN mode and pins
supporting multiple output frequencies.
How the one would know if those frequencies are supported only in
MANUAL/AUTOMATIC modes or also in the FREERUN mode?
In other words: As the user can I change a frequency of a dpll if active
mode is FREERUN?

Okay, I think I'm deep in the DPLL infra you are pushing, but my
understanding that you can control frequency in NCO mode is not present
:/ That only means it may be confusing and not described properly.
How do you control this frequency exactly? I see no such knob.

The set frequency is there already, although we miss phase offset I guess.

But I have changed my mind on having this in the kernel..
Initially I have added this mode as our HW supports it, while thinking that
dpll subsystem shall have this, and we will implement it one day..
But as we have not implemented it yet, let's leave work and discussion on
this mode for the future, when someone will actually try to implement it.

Can't the oscilator be modeled as a pin and then you are not in freerun
but locked this "internal pin"? We know how to control frequency there.

Hmm, yeah probably could work this way.


Thank you!
Arkadiusz

quoted

I would say it is better to have such mode, we could argue on naming though.

quoted

quoted

runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and
dividers before it reaches the output).
It doesn't count as an source pin, it uses signal form dedicated wire for
SYSTEM CLOCK.
In this case control over output frequency is done by synchronizer chip
firmware, but still it will not lock to any source pin signal.

quoted

quoted

quoted

+    render-max: true
+  -
+    type: enum
+    name: lock-status
+    doc: |
+      provides information of dpll device lock status, valid values for
+      DPLL_A_LOCK_STATUS attribute
+    entries:
+      -
+        name: unspec
+        doc: unspecified value
+      -
+        name: unlocked
+        doc: |
+          dpll was not yet locked to any valid source (or is in one of
+          modes: DPLL_MODE_FREERUN, DPLL_MODE_NCO)
+      -
+        name: calibrating
+        doc: dpll is trying to lock to a valid signal
+      -
+        name: locked
+        doc: dpll is locked
+      -
+        name: holdover
+        doc: |
+          dpll is in holdover state - lost a valid lock or was forced by
+          selecting DPLL_MODE_HOLDOVER mode

Is it needed to mention the holdover mode. It's slightly confusing,
because user might understand that the lock-status is always "holdover"
in case of "holdover" mode. But it could be "unlocked", can't it?
Perhaps I don't understand the flows there correctly :/

Hm, if we want to make sure that holdover mode must result in holdover
state then we need some extra atomicity requirements on the SET
operation. To me it seems logical enough that after setting holdover
mode we'll end up either in holdover or unlocked status, depending on
lock status when request reached the HW.

Improved the docs:
       name: holdover
       doc: |
         dpll is in holdover state - lost a valid lock or was forced
         by selecting DPLL_MODE_HOLDOVER mode (latter possible only
         when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED,
  if it was not, the dpll's lock-status will remain

"if it was not" does not really cope with the sentence above that. Could
you iron-out the phrasing a bit please?

Hmmm,
       name: holdover
       doc: |
         dpll is in holdover state - lost a valid lock or was forced
         by selecting DPLL_MODE_HOLDOVER mode (latter possible only
         when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED,
         if dpll lock-state was not DPLL_LOCK_STATUS_LOCKED, the
         dpll's lock-state shall remain DPLL_LOCK_STATUS_UNLOCKED
         even if DPLL_MODE_HOLDOVER was requested)

Hope this is better?

Okay.

quoted

Thank you!
Arkadiusz

[...]