Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 10 Jun 2023 18:22:09 +0200
From:   Jiri Pirko <jiri@resnulli.us>
To:     Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
Cc:     kuba@kernel.org, vadfed@meta.com, jonathan.lemon@gmail.com,
        pabeni@redhat.com, corbet@lwn.net, davem@davemloft.net,
        edumazet@google.com, vadfed@fb.com, jesse.brandeburg@intel.com,
        anthony.l.nguyen@intel.com, saeedm@nvidia.com, leon@kernel.org,
        richardcochran@gmail.com, sj@kernel.org, javierm@redhat.com,
        ricardo.canuelo@collabora.com, mst@redhat.com, tzimmermann@suse.de,
        michal.michalik@intel.com, gregkh@linuxfoundation.org,
        jacek.lawrynowicz@linux.intel.com, airlied@redhat.com,
        ogabbay@kernel.org, arnd@arndb.de, nipun.gupta@amd.com,
        axboe@kernel.dk, linux@zary.sk, masahiroy@kernel.org,
        benjamin.tissoires@redhat.com, geert+renesas@glider.be,
        milena.olech@intel.com, kuniyu@amazon.com, liuhangbin@gmail.com,
        hkallweit1@gmail.com, andy.ren@getcruise.com, razor@blackwall.org,
        idosch@nvidia.com, lucien.xin@gmail.com, nicolas.dichtel@6wind.com,
        phil@nwl.cc, claudiajkang@gmail.com, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        intel-wired-lan@lists.osuosl.org, linux-rdma@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, poros@redhat.com,
        mschmidt@redhat.com, linux-clk@vger.kernel.org,
        vadim.fedorenko@linux.dev
Subject: Re: [RFC PATCH v8 02/10] dpll: spec: Add Netlink spec in YAML
Message-ID: <ZISjMUcpmUTBXIOA@nanopsycho>
References: <20230609121853.3607724-1-arkadiusz.kubalewski@intel.com>
 <20230609121853.3607724-3-arkadiusz.kubalewski@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230609121853.3607724-3-arkadiusz.kubalewski@intel.com>
Precedence: bulk

Fri, Jun 09, 2023 at 02:18:45PM CEST, arkadiusz.kubalewski@intel.com wrote:
>Add a protocol spec for DPLL.
>Add code generated from the spec.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>Signed-off-by: Michal Michalik <michal.michalik@intel.com>
>Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
> Documentation/netlink/specs/dpll.yaml | 466 ++++++++++++++++++++++++++
> drivers/dpll/dpll_nl.c                | 161 +++++++++
> drivers/dpll/dpll_nl.h                |  50 +++
> include/uapi/linux/dpll.h             | 184 ++++++++++
> 4 files changed, 861 insertions(+)
> create mode 100644 Documentation/netlink/specs/dpll.yaml
> create mode 100644 drivers/dpll/dpll_nl.c
> create mode 100644 drivers/dpll/dpll_nl.h
> create mode 100644 include/uapi/linux/dpll.h
>
>diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
>new file mode 100644
>index 000000000000..f7317003d312
>--- /dev/null
>+++ b/Documentation/netlink/specs/dpll.yaml
>@@ -0,0 +1,466 @@
>+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
>+
>+name: dpll
>+
>+doc: DPLL subsystem.
>+
>+definitions:
>+  -
>+    type: enum
>+    name: mode
>+    doc: |
>+      working-modes a dpll can support, differentiate if and how dpll selects

s/working-modes/working modes/
s/differentiate/differentiates/
?


>+      one of its inputs to syntonize with it, valid values for DPLL_A_MODE
>+      attribute
>+    entries:
>+      -
>+        name: manual
>+        doc: input can be only selected by sending a request to dpll
>+        value: 1
>+      -
>+        name: automatic
>+        doc: highest prio, valid input, auto selected by dpll

s/valid input, auto selected by dpll/input pin auto selected by dpll/
?


>+      -
>+        name: holdover
>+        doc: dpll forced into holdover mode
>+      -
>+        name: freerun
>+        doc: dpll driven on system clk

Thinking about modes "holdover" and "freerun".
1) You don't use them anywhere in this patchset, please remove them
   until they are needed. ptp_ocp and ice uses automatic, mlx5 uses
   manual. Btw, are there any other unused parts of UAPI? If yes, could
   you please remove them too?

2) I don't think it is correct to have them.
   a) to achieve holdover:
      if state is LOCKED_HO_ACQ you just disconnect all input pins.
   b) to achieve freerun:
      if state LOCKED you just disconnect all input pins.
   So don't mangle the mode with status.


>+    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: unlocked
>+        doc: |
>+          dpll was not yet locked to any valid input (or is in mode:
>+            DPLL_MODE_FREERUN)

Don't forget to remove the mention of mode freerun from here.


>+        value: 1
>+      -
>+        name: locked
>+        doc: |
>+          dpll is locked to a valid signal, but no holdover available
>+      -
>+        name: locked-ho-acq
>+        doc: |
>+          dpll is locked and holdover acquired
>+      -
>+        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)

Don't forget to remove the mention of mode holdover from here.


>+    render-max: true
>+  -
>+    type: const
>+    name: temp-divider
>+    value: 1000
>+    doc: |
>+      temperature divider allowing userspace to calculate the
>+      temperature as float with three digit decimal precision.
>+      Value of (DPLL_A_TEMP / DPLL_TEMP_DIVIDER) is integer part of
>+      temperature value.
>+      Value of (DPLL_A_TEMP % DPLL_TEMP_DIVIDER) is fractional part of
>+      temperature value.
>+  -
>+    type: enum
>+    name: type
>+    doc: type of dpll, valid values for DPLL_A_TYPE attribute
>+    entries:
>+      -
>+        name: pps
>+        doc: dpll produces Pulse-Per-Second signal
>+        value: 1
>+      -
>+        name: eec
>+        doc: dpll drives the Ethernet Equipment Clock
>+    render-max: true
>+  -
>+    type: enum
>+    name: pin-type
>+    doc: |
>+      defines possible types of a pin, valid values for DPLL_A_PIN_TYPE
>+      attribute
>+    entries:
>+      -
>+        name: mux
>+        doc: aggregates another layer of selectable pins
>+        value: 1
>+      -
>+        name: ext
>+        doc: external input
>+      -
>+        name: synce-eth-port
>+        doc: ethernet port PHY's recovered clock
>+      -
>+        name: int-oscillator
>+        doc: device internal oscillator
>+      -
>+        name: gnss
>+        doc: GNSS recovered clock
>+    render-max: true
>+  -
>+    type: enum
>+    name: pin-direction
>+    doc: |
>+      defines possible direction of a pin, valid values for
>+      DPLL_A_PIN_DIRECTION attribute
>+    entries:
>+      -
>+        name: input
>+        doc: pin used as a input of a signal

I don't think I have any objections against "input", but out of
curiosity, why you changed that from "source"?


>+        value: 1
>+      -
>+        name: output
>+        doc: pin used to output the signal
>+    render-max: true
>+  -
>+    type: const
>+    name: pin-frequency-1-hz
>+    value: 1
>+  -
>+    type: const
>+    name: pin-frequency-10-khz
>+    value: 10000
>+  -
>+    type: const
>+    name: pin-frequency-77_5-khz
>+    value: 77500
>+  -
>+    type: const
>+    name: pin-frequency-10-mhz
>+    value: 10000000
>+  -
>+    type: enum
>+    name: pin-state
>+    doc: |
>+      defines possible states of a pin, valid values for
>+      DPLL_A_PIN_STATE attribute
>+    entries:
>+      -
>+        name: connected
>+        doc: pin connected, active input of phase locked loop
>+        value: 1
>+      -
>+        name: disconnected
>+        doc: pin disconnected, not considered as a valid input
>+      -
>+        name: selectable
>+        doc: pin enabled for automatic input selection
>+    render-max: true
>+  -
>+    type: flags
>+    name: pin-caps
>+    doc: |
>+      defines possible capabilities of a pin, valid flags on
>+      DPLL_A_PIN_CAPS attribute
>+    entries:
>+      -
>+        name: direction-can-change
>+      -
>+        name: priority-can-change
>+      -
>+        name: state-can-change
>+
>+attribute-sets:
>+  -
>+    name: dpll
>+    enum-name: dpll_a
>+    attributes:
>+      -
>+        name: id
>+        type: u32
>+        value: 1
>+      -
>+        name: module-name
>+        type: string
>+      -
>+        name: clock-id
>+        type: u64
>+      -
>+        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: type
>+        type: u8
>+        enum: type
>+      -
>+        name: pin-id
>+        type: u32
>+      -
>+        name: pin-board-label
>+        type: string
>+      -
>+        name: pin-panel-label
>+        type: string
>+      -
>+        name: pin-package-label
>+        type: string

Wouldn't it make sense to add some small documentation blocks to the
attrs? IDK.


>+      -
>+        name: pin-type
>+        type: u8
>+        enum: pin-type
>+      -
>+        name: pin-direction
>+        type: u8
>+        enum: pin-direction
>+      -
>+        name: pin-frequency
>+        type: u64
>+      -
>+        name: pin-frequency-supported
>+        type: nest
>+        multi-attr: true
>+        nested-attributes: pin-frequency-range
>+      -
>+        name: pin-frequency-min
>+        type: u64
>+      -
>+        name: pin-frequency-max
>+        type: u64
>+      -
>+        name: pin-prio
>+        type: u32
>+      -
>+        name: pin-state
>+        type: u8
>+        enum: pin-state
>+      -
>+        name: pin-dpll-caps
>+        type: u32
>+      -
>+        name: pin-parent
>+        type: nest
>+        multi-attr: true
>+        nested-attributes: pin-parent
>+  -
>+    name: pin-parent
>+    subset-of: dpll
>+    attributes:
>+      -
>+        name: id
>+        type: u32
>+      -
>+        name: pin-direction
>+        type: u8
>+      -
>+        name: pin-prio
>+        type: u32
>+      -
>+        name: pin-state
>+        type: u8
>+      -
>+        name: pin-id
>+        type: u32
>+
>+  -
>+    name: pin-frequency-range
>+    subset-of: dpll
>+    attributes:
>+      -
>+        name: pin-frequency-min
>+        type: u64
>+      -
>+        name: pin-frequency-max
>+        type: u64

[...]