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

[Jiri Pirko <jiri@resnulli.us>]

Thu, May 18, 2023 at 03:24:45PM CEST, arkadiusz.kubalewski@intel.com wrote:
>
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Tuesday, May 16, 2023 4:34 PM
>>
>>Tue, May 16, 2023 at 02:05:38PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>>>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:
>>>>>>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:
>>>>>>>>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:
>>>>>>>>> >+definitions:
>>>>>>>>> >+  -
>>>>>>>>> >+    type: enum
>>>>>>>>> >+    name: mode
>>>>>>>>> >+    doc: |
>>>>>>>>> >+      working-modes a dpll can support, differentiate if and how
>>>>>>>>> >dpll selects
>>>>>>>>> >+      one of its sources to syntonize with it, valid values for
>>>>>>>>> >DPLL_A_MODE
>>>>>>>>> >+      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.
>>>>>>>
>>>>>>>>> >+        doc: unspecified value
>>>>>>>>> >+      -
>>>>>>>>> >+        name: manual
>>>>>>>>
>>>>>>>>I think the documentation calls this "forced", still.
>>>>>>>>
>>>>>>>
>>>>>>>Yes, good catch, fixed docs.
>>>>>>>
>>>>>>>>> >+        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.
>>>>>>>
>>>>>>>>
>>>>>>>>> >+      -
>>>>>>>>> >+        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.
>>
>>Yeah, but on a pin, right?
>>
>>
>
>Yes frequency of an output pin is configurable, phase offset for a dpll or
>output is not there, we might think of adding it..

But wait, we are talking about controllig freq of NCO, that has to be a
different knob. Anyway, it is dropped, so the discussion is poitless
now.



>
>>
>>>
>>>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.
>>
>>Yeah, let's drop it then. One less confusing thing to wrap a head around :)
>>
>
>Dropped.
>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>>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
>>>
>>>>
>>>>>
>>>>>I would say it is better to have such mode, we could argue on naming
>>>>>>though.
>>>>>
>>>>>>
>>>>>>>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.
>>>>>>>
>>>>>>>>> >+    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.
>>>>
>>>>>
>>>>>
>>>>>Thank you!
>>>>>Arkadiusz
>>>>>
>>>>>[...]