RE: [RFC PATCH v8 02/10] dpll: spec: Add Netlink spec in YAML

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@xxxxxxxxxxx>
>Sent: Saturday, June 10, 2023 6:22 PM
>
>Fri, Jun 09, 2023 at 02:18:45PM CEST, arkadiusz.kubalewski@xxxxxxxxx wrote:
>>Add a protocol spec for DPLL.
>>Add code generated from the spec.
>>
>>Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>
>>Signed-off-by: Michal Michalik <michal.michalik@xxxxxxxxx>
>>Signed-off-by: Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
>>---
>> 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/
>?

Fixed.

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

Fixed.

>
>
>>+      -
>>+        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.
>

Well this is not entierly true, the mode is not a state.
Technically in those modes the user would not be able to set any states
on the pins.
The modes are supported on the synchronizer chips we are using, altough
the ice driver does not have this support enabled yet.
So I am removing those for now, if they would be needed, we will submit the
patches for it.

>
>>+    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.
>

Fixed.

>
>>+        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.
>

Fixed.

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

Agreed to previous version review comment from Jakub,
to use either: input/output or source/sink naming scheme.

>
>>+        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.
>

Actually already tried that, but after all they did not generate any docs
for attr enums. So this need also fix in ynl-gen-c.py

I think this would be useful, but only if we could use them in the dpll.rst.
Right now it is not case, but we already try to incorporate other enums
description there, for now it is broken, but will try to fix this in the near
future.

Thank you!
Arkadiusz

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