Message-ID: <ZFygbd1H+VdvCTyH@nanopsycho>
Date: Thu, 11 May 2023 09:59:41 +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 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?


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


>          DPLL_LOCK_STATUS_UNLOCKED even if user requests
>          DPLL_MODE_HOLDOVER)
>Is that better?
>
>What extra atomicity you have on your mind?
>Do you suggest to validate and allow (in dpll_netlink.c) only for 'unlocked'
>or 'holdover' states of dpll, once DPLL_MODE_HOLDOVER was successfully
>requested by the user?
>
>>> >+    render-max: true
>>> >+  -
>>> >+    type: const
>>> >+    name: temp-divider
>>> >+    value: 10
>>> >+    doc: |
>>> >+      temperature divider allowing userspace to calculate the
>>> >+      temperature as float with single digit precision.
>>> >+      Value of (DPLL_A_TEMP / DPLL_TEMP_DIVIDER) is integer part of
>>> >+      tempearture value.
>>>
>>> s/tempearture/temperature/
>>>
>>> Didn't checkpatch warn you?
>>
>>Also can we give it a more healthy engineering margin?
>>DPLL_A_TEMP is u32, silicon melts at around 1400C,
>>so we really can afford to make the divisor 1000.
>>
>
>Sure, fixed.
>
>>> >+    name: device
>>> >+    subset-of: dpll
>>> >+    attributes:
>>> >+      -
>>> >+        name: id
>>> >+        type: u32
>>> >+        value: 2
>>> >+      -
>>> >+        name: dev-name
>>> >+        type: string
>>> >+      -
>>> >+        name: bus-name
>>> >+        type: string
>>> >+      -
>>> >+        name: mode
>>> >+        type: u8
>>> >+        enum: mode
>>> >+      -
>>> >+        name: mode-supported
>>> >+        type: u8
>>> >+        enum: mode
>>> >+        multi-attr: true
>>> >+      -
>>> >+        name: lock-status
>>> >+        type: u8
>>> >+        enum: lock-status
>>> >+      -
>>> >+        name: temp
>>> >+        type: s32
>>> >+      -
>>> >+        name: clock-id
>>> >+        type: u64
>>> >+      -
>>> >+        name: type
>>> >+        type: u8
>>> >+        enum: type
>>> >+      -
>>> >+        name: pin-prio
>>> >+        type: u32
>>> >+        value: 19
>>>
>>> Do you still need to pass values for a subset? That is odd. Well, I
>>> think is is odd to pass anything other than names in subset definition,
>>> the rest of the info is in the original attribute set definition,
>>> isn't it?
>>> Jakub?
>>
>>Probably stale code, related bug was fixed in YNL a few months back.
>>Explicit value should no longer be needed.
>
>Yes, checked it works without them, I am removing values for next version.
>
>Thanks!
>Arkadiusz

Powered by blists - more mailing lists