Re: [RFC PATCH v8 02/10] dpll: spec: Add Netlink spec in YAML
From: Jiri Pirko <jiri@resnulli.us>
Date: 2023-06-10 16:22:20
Also in:
intel-wired-lan, linux-clk, linux-doc, linux-rdma, lkml
Fri, Jun 09, 2023 at 02:18:45PM CEST, arkadiusz.kubalewski@intel.com wrote:
quoted hunk ↗ jump to hunk
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 <redacted> 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.hdiff --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
[...]