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

From: Jiri Pirko
Date: Sat Jun 10 2023 - 12:22:20 EST

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


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

[...]