Message-ID: <ZGH7uvxD55Pan0gf@nanopsycho>
Date: Mon, 15 May 2023 11:30:34 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
Cc: Jakub Kicinski <kuba@...nel.org>, Vadim Fedorenko <vadfed@...a.com>,
	Jonathan Lemon <jonathan.lemon@...il.com>,
	Paolo Abeni <pabeni@...hat.com>,
	"Olech, Milena" <milena.olech@...el.com>,
	"Michalik, Michal" <michal.michalik@...el.com>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	poros <poros@...hat.com>, mschmidt <mschmidt@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
	Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Subject: Re: [RFC PATCH v7 1/8] dpll: spec: Add Netlink spec in YAML

Thu, May 11, 2023 at 10:51:43PM CEST, arkadiusz.kubalewski@...el.com wrote:
>>From: Jiri Pirko <jiri@...nulli.us>
>>Sent: Thursday, May 11, 2023 10:00 AM
>>
>>Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@...el.com wrote:
>>>>From: Jakub Kicinski <kuba@...nel.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.

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.


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

Powered by blists - more mailing lists