[RFC,v6,1/6] dpll: spec: Add Netlink spec in YAML

Commit Message

Vadim Fedorenko March 12, 2023, 2:28 a.m. UTC

From: Arkadiusz Kubalewski <[e-mail]>

Add a protocol spec for DPLL.
Add code generated from the spec.

Signed-off-by: Jakub Kicinski <[e-mail]>
Signed-off-by: Michal Michalik <[e-mail]>
Signed-off-by: Arkadiusz Kubalewski <[e-mail]>
Signed-off-by: Vadim Fedorenko <[e-mail]>
---
 Documentation/netlink/specs/dpll.yaml | 514 ++++++++++++++++++++++++++
 drivers/dpll/dpll_nl.c                | 126 +++++++
 drivers/dpll/dpll_nl.h                |  42 +++
 include/uapi/linux/dpll.h             | 196 ++++++++++
 4 files changed, 878 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

Comments

Jiri Pirko March 14, 2023, 2:44 p.m. UTC | #1

Sun, Mar 12, 2023 at 03:28:02AM CET, [e-mail] wrote:
>From: Arkadiusz Kubalewski <[e-mail]>
>
>Add a protocol spec for DPLL.
>Add code generated from the spec.
>
>Signed-off-by: Jakub Kicinski <[e-mail]>
>Signed-off-by: Michal Michalik <[e-mail]>
>Signed-off-by: Arkadiusz Kubalewski <[e-mail]>
>Signed-off-by: Vadim Fedorenko <[e-mail]>
>---
> Documentation/netlink/specs/dpll.yaml | 514 ++++++++++++++++++++++++++
> drivers/dpll/dpll_nl.c                | 126 +++++++
> drivers/dpll/dpll_nl.h                |  42 +++
> include/uapi/linux/dpll.h             | 196 ++++++++++
> 4 files changed, 878 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..03e9f0e2d3d2
>--- /dev/null
>+++ b/Documentation/netlink/specs/dpll.yaml
>@@ -0,0 +1,514 @@
>+name: dpll
>+
>+doc: DPLL subsystem.
>+
>+definitions:
>+  -
>+    type: const
>+    name: temp-divider
>+    value: 10
>+  -
>+    type: const
>+    name: pin-freq-1-hz
>+    value: 1
>+  -
>+    type: const
>+    name: pin-freq-10-mhz
>+    value: 10000000
>+  -
>+    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 (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
>+    render-max: true
>+  -
>+    type: enum
>+    name: pin-type
>+    doc: Enumerates types of a pin, valid values for DPLL_A_PIN_TYPE attribute
>+    entries:
>+      -
>+        name: unspec
>+        doc: unspecified value
>+      -
>+        name: mux
>+        doc: aggregates another layer of selectable pins
>+      -
>+        name: ext
>+        doc: external source
>+      -
>+        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-state
>+    doc: available pin modes
>+    entries:
>+      -
>+        name: unspec
>+        doc: unspecified value
>+      -
>+        name: connected
>+        doc: pin connected
>+      -
>+        name: disconnected
>+        doc: pin disconnected
>+    render-max: true
>+  -
>+    type: enum
>+    name: pin-direction
>+    doc: available pin direction
>+    entries:
>+      -
>+        name: unspec
>+        doc: unspecified value
>+      -
>+        name: source
>+        doc: pin used as a source of a signal
>+      -
>+        name: output
>+        doc: pin used to output the signal
>+    render-max: true
>+  -
>+    type: enum
>+    name: mode

Could you please sort the enums so they are in the same order as the
attribute which carries them? That would also put the device and pin
enums together.


>+    doc: |
>+      working-modes a dpll can support, differentiate if and how dpll selects
>+      one of its sources to syntonize with it
>+    entries:
>+      -
>+        name: unspec
>+        doc: unspecified value
>+      -
>+        name: manual
>+        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
>+      -
>+        name: nco
>+        doc: dpll driven by Numerically Controlled Oscillator
>+    render-max: true
>+  -
>+    type: enum
>+    name: type
>+    doc: type of dpll, valid values for DPLL_A_TYPE attribute
>+    entries:
>+      -
>+        name: unspec
>+        doc: unspecified value
>+      -
>+        name: pps
>+        doc: dpll produces Pulse-Per-Second signal
>+      -
>+        name: eec
>+        doc: dpll drives the Ethernet Equipment Clock
>+    render-max: true
>+  -
>+    type: enum
>+    name: event
>+    doc: events of dpll generic netlink family
>+    entries:
>+      -
>+        name: unspec
>+        doc: invalid event type
>+      -
>+        name: device-create
>+        doc: dpll device created
>+      -
>+        name: device-delete
>+        doc: dpll device deleted
>+      -
>+        name: device-change
>+        doc: |
>+          attribute of dpll device or pin changed, reason is to be found with
>+          an attribute type (DPLL_A_*) received with the event
>+  -
>+    type: flags
>+    name: pin-caps
>+    doc: define capabilities of a pin
>+    entries:
>+      -
>+        name: direction-can-change
>+      -
>+        name: priority-can-change
>+      -
>+        name: state-can-change
>+
>+
>+attribute-sets:
>+  -
>+    name: dpll
>+    enum-name: dplla
>+    attributes:
>+      -
>+        name: device
>+        type: nest
>+        value: 1
>+        multi-attr: true
>+        nested-attributes: device

What is this "device" and what is it good for? Smells like some leftover
and with the nested scheme looks quite odd.


>+      -
>+        name: id
>+        type: u32
>+      -
>+        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: source-pin-idx
>+        type: u32
>+      -
>+        name: lock-status
>+        type: u8
>+        enum: lock-status
>+      -
>+        name: temp

Could you put some comment regarding the divider here?


>+        type: s32
>+      -
>+        name: clock-id
>+        type: u64
>+      -
>+        name: type
>+        type: u8
>+        enum: type
>+      -
>+        name: pin
>+        type: nest
>+        multi-attr: true
>+        nested-attributes: pin
>+        value: 12
>+      -
>+        name: pin-idx
>+        type: u32
>+      -
>+        name: pin-description
>+        type: string
>+      -
>+        name: pin-type
>+        type: u8
>+        enum: pin-type
>+      -
>+        name: pin-direction
>+        type: u8
>+        enum: pin-direction
>+      -
>+        name: pin-frequency
>+        type: u32
>+      -
>+        name: pin-frequency-supported
>+        type: u32
>+        multi-attr: true
>+      -
>+        name: pin-any-frequency-min
>+        type: u32
>+      -
>+        name: pin-any-frequency-max
>+        type: u32

These 2 attrs somehow overlap with pin-frequency-supported,
which is a bit confusing, I think that pin-frequency-supported
should carry all supported frequencies. How about to have something
like:
        name: pin-frequency-supported
        type: nest
	multi-attr: true
        nested-attributes: pin-frequency-range

and then:
      name: pin-frequency-range
      subset-of: dpll
      attributes:
        -
          name: pin-frequency-min
          type: u32
        -
          name: pin-frequency-max
          type: u32
          doc: should be put only to specify range when value differs
	       from pin-frequency-min

That could carry list of frequencies and ranges, in this case multiple
ones if possibly needed.



>+      -
>+        name: pin-prio
>+        type: u32
>+      -
>+        name: pin-state
>+        type: u8
>+        enum: pin-state
>+      -
>+        name: pin-parent
>+        type: nest
>+        multi-attr: true
>+        nested-attributes: pin
>+        value: 23

Value 23? What's this?
You have it specified for some attrs all over the place.
What is the reason for it?


>+      -
>+        name: pin-parent-idx
>+        type: u32
>+      -
>+        name: pin-rclk-device
>+        type: string
>+      -
>+        name: pin-dpll-caps
>+        type: u32
>+  -
>+    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: source-pin-idx
>+        type: u32
>+      -
>+        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
>+        type: nest
>+        value: 12
>+        multi-attr: true
>+        nested-attributes: pin

This does not belong here.


>+      -
>+        name: pin-prio
>+        type: u32
>+        value: 21
>+      -
>+        name: pin-state
>+        type: u8
>+        enum: pin-state
>+      -
>+        name: pin-dpll-caps
>+        type: u32
>+        value: 26

All these 3 do not belong here are well.



>+  -
>+    name: pin
>+    subset-of: dpll
>+    attributes:
>+      -
>+        name: device
>+        type: nest
>+        value: 1
>+        multi-attr: true
>+        nested-attributes: device
>+      -
>+        name: pin-idx
>+        type: u32
>+        value: 13
>+      -
>+        name: pin-description
>+        type: string
>+      -
>+        name: pin-type
>+        type: u8
>+        enum: pin-type
>+      -
>+        name: pin-direction
>+        type: u8
>+        enum: pin-direction
>+      -
>+        name: pin-frequency
>+        type: u32
>+      -
>+        name: pin-frequency-supported
>+        type: u32
>+        multi-attr: true
>+      -
>+        name: pin-any-frequency-min
>+        type: u32
>+      -
>+        name: pin-any-frequency-max
>+        type: u32
>+      -
>+        name: pin-prio
>+        type: u32
>+      -
>+        name: pin-state
>+        type: u8
>+        enum: pin-state
>+      -
>+        name: pin-parent
>+        type: nest
>+        multi-attr: true

Multiple parents? How is that supposed to work?


>+        nested-attributes: pin-parent
>+        value: 23
>+      -
>+        name: pin-rclk-device
>+        type: string
>+        value: 25
>+      -
>+        name: pin-dpll-caps
>+        type: u32

Missing "enum: "


>+  -
>+    name: pin-parent
>+    subset-of: dpll
>+    attributes:
>+      -
>+        name: pin-state
>+        type: u8
>+        value: 22
>+        enum: pin-state
>+      -
>+        name: pin-parent-idx
>+        type: u32
>+        value: 24
>+      -
>+        name: pin-rclk-device
>+        type: string

Yeah, as I wrote in the other email, this really smells to
have like a simple string like this. What is it supposed to be?


>+
>+
>+operations:
>+  list:
>+    -
>+      name: unspec
>+      doc: unused
>+
>+    -
>+      name: device-get
>+      doc: |
>+        Get list of DPLL devices (dump) or attributes of a single dpll device
>+      attribute-set: dpll

Shouldn't this be "device"?


>+      flags: [ admin-perm ]
>+
>+      do:
>+        pre: dpll-pre-doit
>+        post: dpll-post-doit
>+        request:
>+          attributes:
>+            - id
>+            - bus-name
>+            - dev-name
>+        reply:
>+          attributes:
>+            - device
>+
>+      dump:
>+        pre: dpll-pre-dumpit
>+        post: dpll-post-dumpit
>+        reply:
>+          attributes:
>+            - device
>+
>+    -
>+      name: device-set
>+      doc: Set attributes for a DPLL device
>+      attribute-set: dpll

"device" here as well?


>+      flags: [ admin-perm ]
>+
>+      do:
>+        pre: dpll-pre-doit
>+        post: dpll-post-doit
>+        request:
>+          attributes:
>+            - id
>+            - bus-name
>+            - dev-name
>+            - mode

Hmm, shouldn't source-pin-index be here as well?

>+
>+    -
>+      name: pin-get
>+      doc: |
>+        Get list of pins and its attributes.
>+        - dump request without any attributes given - list all the pins in the system
>+        - dump request with target dpll - list all the pins registered with a given dpll device
>+        - do request with target dpll and target pin - single pin attributes
>+      attribute-set: dpll

"pin"?


>+      flags: [ admin-perm ]
>+
>+      do:
>+        pre: dpll-pin-pre-doit
>+        post: dpll-pin-post-doit
>+        request:
>+          attributes:
>+            - id
>+            - bus-name
>+            - dev-name
>+            - pin-idx
>+        reply:
>+          attributes:
>+            - pin
>+
>+      dump:
>+        pre: dpll-pin-pre-dumpit
>+        post: dpll-pin-post-dumpit
>+        request:
>+          attributes:
>+            - id
>+            - bus-name
>+            - dev-name
>+        reply:
>+          attributes:
>+            - pin
>+
>+    -
>+      name: pin-set
>+      doc: Set attributes of a target pin
>+      attribute-set: dpll

"pin"?


>+      flags: [ admin-perm ]
>+
>+      do:
>+        pre: dpll-pin-pre-doit
>+        post: dpll-pin-post-doit
>+        request:
>+          attributes:
>+            - id
>+            - bus-name
>+            - dev-name
>+            - pin-idx
>+            - pin-frequency
>+            - pin-direction
>+            - pin-prio
>+            - pin-parent-idx
>+            - pin-state
>+
>+mcast-groups:
>+  list:
>+    -
>+      name: monitor
>diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c
>new file mode 100644
>index 000000000000..099d1e30ca7c
>--- /dev/null
>+++ b/drivers/dpll/dpll_nl.c
>@@ -0,0 +1,126 @@
>+// SPDX-License-Identifier: BSD-3-Clause
>+/* Do not edit directly, auto-generated from: */
>+/*	Documentation/netlink/specs/dpll.yaml */
>+/* YNL-GEN kernel source */
>+
>+#include <net/netlink.h>
>+#include <net/genetlink.h>
>+
>+#include "dpll_nl.h"
>+
>+#include <linux/dpll.h>
>+
>+/* DPLL_CMD_DEVICE_GET - do */
>+static const struct nla_policy dpll_device_get_nl_policy[DPLL_A_BUS_NAME + 1] = {
>+	[DPLL_A_ID] = { .type = NLA_U32, },
>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>+};
>+
>+/* DPLL_CMD_DEVICE_SET - do */
>+static const struct nla_policy dpll_device_set_nl_policy[DPLL_A_MODE + 1] = {
>+	[DPLL_A_ID] = { .type = NLA_U32, },
>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>+	[DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),

Hmm, any idea why the generator does not put define name
here instead of "5"?


>+};
>+
>+/* DPLL_CMD_PIN_GET - do */
>+static const struct nla_policy dpll_pin_get_do_nl_policy[DPLL_A_PIN_IDX + 1] = {
>+	[DPLL_A_ID] = { .type = NLA_U32, },
>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>+	[DPLL_A_PIN_IDX] = { .type = NLA_U32, },
>+};
>+
>+/* DPLL_CMD_PIN_GET - dump */
>+static const struct nla_policy dpll_pin_get_dump_nl_policy[DPLL_A_BUS_NAME + 1] = {
>+	[DPLL_A_ID] = { .type = NLA_U32, },
>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>+};
>+
>+/* DPLL_CMD_PIN_SET - do */
>+static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PARENT_IDX + 1] = {
>+	[DPLL_A_ID] = { .type = NLA_U32, },
>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>+	[DPLL_A_PIN_IDX] = { .type = NLA_U32, },
>+	[DPLL_A_PIN_FREQUENCY] = { .type = NLA_U32, },

4.3GHz would be the limit, isn't it future proof to make this rather u64?


>+	[DPLL_A_PIN_DIRECTION] = NLA_POLICY_MAX(NLA_U8, 2),
>+	[DPLL_A_PIN_PRIO] = { .type = NLA_U32, },
>+	[DPLL_A_PIN_PARENT_IDX] = { .type = NLA_U32, },

This is a bit odd. The size is DPLL_A_PIN_PARENT_IDX + 1, yet PIN_STATE
is last. I found that the order is not according to the enum in the yaml
operation definition. Perhaps the generator can sort this?


>+	[DPLL_A_PIN_STATE] = NLA_POLICY_MAX(NLA_U8, 2),
>+};
>+
>+/* Ops table for dpll */
>+static const struct genl_split_ops dpll_nl_ops[6] = {
>+	{
>+		.cmd		= DPLL_CMD_DEVICE_GET,
>+		.pre_doit	= dpll_pre_doit,
>+		.doit		= dpll_nl_device_get_doit,
>+		.post_doit	= dpll_post_doit,
>+		.policy		= dpll_device_get_nl_policy,
>+		.maxattr	= DPLL_A_BUS_NAME,
>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>+	},
>+	{
>+		.cmd	= DPLL_CMD_DEVICE_GET,
>+		.start	= dpll_pre_dumpit,
>+		.dumpit	= dpll_nl_device_get_dumpit,
>+		.done	= dpll_post_dumpit,
>+		.flags	= GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
>+	},
>+	{
>+		.cmd		= DPLL_CMD_DEVICE_SET,
>+		.pre_doit	= dpll_pre_doit,
>+		.doit		= dpll_nl_device_set_doit,
>+		.post_doit	= dpll_post_doit,
>+		.policy		= dpll_device_set_nl_policy,
>+		.maxattr	= DPLL_A_MODE,
>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>+	},
>+	{
>+		.cmd		= DPLL_CMD_PIN_GET,
>+		.pre_doit	= dpll_pin_pre_doit,
>+		.doit		= dpll_nl_pin_get_doit,
>+		.post_doit	= dpll_pin_post_doit,
>+		.policy		= dpll_pin_get_do_nl_policy,
>+		.maxattr	= DPLL_A_PIN_IDX,
>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>+	},
>+	{
>+		.cmd		= DPLL_CMD_PIN_GET,
>+		.start		= dpll_pin_pre_dumpit,
>+		.dumpit		= dpll_nl_pin_get_dumpit,
>+		.done		= dpll_pin_post_dumpit,
>+		.policy		= dpll_pin_get_dump_nl_policy,
>+		.maxattr	= DPLL_A_BUS_NAME,
>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
>+	},
>+	{
>+		.cmd		= DPLL_CMD_PIN_SET,
>+		.pre_doit	= dpll_pin_pre_doit,
>+		.doit		= dpll_nl_pin_set_doit,
>+		.post_doit	= dpll_pin_post_doit,
>+		.policy		= dpll_pin_set_nl_policy,
>+		.maxattr	= DPLL_A_PIN_PARENT_IDX,
>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>+	},
>+};
>+
>+static const struct genl_multicast_group dpll_nl_mcgrps[] = {
>+	[DPLL_NLGRP_MONITOR] = { "monitor", },
>+};
>+
>+struct genl_family dpll_nl_family __ro_after_init = {
>+	.name		= DPLL_FAMILY_NAME,
>+	.version	= DPLL_FAMILY_VERSION,
>+	.netnsok	= true,
>+	.parallel_ops	= true,
>+	.module		= THIS_MODULE,
>+	.split_ops	= dpll_nl_ops,
>+	.n_split_ops	= ARRAY_SIZE(dpll_nl_ops),
>+	.mcgrps		= dpll_nl_mcgrps,
>+	.n_mcgrps	= ARRAY_SIZE(dpll_nl_mcgrps),
>+};
>diff --git a/drivers/dpll/dpll_nl.h b/drivers/dpll/dpll_nl.h
>new file mode 100644
>index 000000000000..3a354dfb9a31
>--- /dev/null
>+++ b/drivers/dpll/dpll_nl.h
>@@ -0,0 +1,42 @@
>+/* SPDX-License-Identifier: BSD-3-Clause */
>+/* Do not edit directly, auto-generated from: */
>+/*	Documentation/netlink/specs/dpll.yaml */
>+/* YNL-GEN kernel header */
>+
>+#ifndef _LINUX_DPLL_GEN_H
>+#define _LINUX_DPLL_GEN_H
>+
>+#include <net/netlink.h>
>+#include <net/genetlink.h>
>+
>+#include <linux/dpll.h>
>+
>+int dpll_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>+		  struct genl_info *info);
>+int dpll_pin_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>+		      struct genl_info *info);
>+void
>+dpll_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>+	       struct genl_info *info);
>+void
>+dpll_pin_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>+		   struct genl_info *info);
>+int dpll_pre_dumpit(struct netlink_callback *cb);
>+int dpll_pin_pre_dumpit(struct netlink_callback *cb);
>+int dpll_post_dumpit(struct netlink_callback *cb);
>+int dpll_pin_post_dumpit(struct netlink_callback *cb);
>+
>+int dpll_nl_device_get_doit(struct sk_buff *skb, struct genl_info *info);
>+int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
>+int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info);
>+int dpll_nl_pin_get_doit(struct sk_buff *skb, struct genl_info *info);
>+int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
>+int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info);
>+
>+enum {
>+	DPLL_NLGRP_MONITOR,
>+};
>+
>+extern struct genl_family dpll_nl_family;
>+
>+#endif /* _LINUX_DPLL_GEN_H */
>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>new file mode 100644
>index 000000000000..ece6fe685d08
>--- /dev/null
>+++ b/include/uapi/linux/dpll.h
>@@ -0,0 +1,196 @@
>+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>+/* Do not edit directly, auto-generated from: */
>+/*	Documentation/netlink/specs/dpll.yaml */
>+/* YNL-GEN uapi header */
>+
>+#ifndef _UAPI_LINUX_DPLL_H
>+#define _UAPI_LINUX_DPLL_H
>+
>+#define DPLL_FAMILY_NAME	"dpll"
>+#define DPLL_FAMILY_VERSION	1
>+
>+#define DPLL_TEMP_DIVIDER	10

Some comment to this define would be nice.


>+#define DPLL_PIN_FREQ_1_HZ	1
>+#define DPLL_PIN_FREQ_10_MHZ	10000000

Have this as enum. Spell out "frequency" to be consistent with the
related attribute name.


>+
>+/**
>+ * enum dpll_lock_status - Provides information of dpll device lock status,
>+ *   valid values for DPLL_A_LOCK_STATUS attribute
>+ * @DPLL_LOCK_STATUS_UNSPEC: unspecified value
>+ * @DPLL_LOCK_STATUS_UNLOCKED: dpll was not yet locked to any valid (or is in

"any valid source" perhaps?


>+ *   one of modes: DPLL_MODE_FREERUN, DPLL_MODE_NCO)
>+ * @DPLL_LOCK_STATUS_CALIBRATING: dpll is trying to lock to a valid signal
>+ * @DPLL_LOCK_STATUS_LOCKED: dpll is locked
>+ * @DPLL_LOCK_STATUS_HOLDOVER: dpll is in holdover state - lost a valid lock or
>+ *   was forced by selecting DPLL_MODE_HOLDOVER mode
>+ */
>+enum dpll_lock_status {
>+	DPLL_LOCK_STATUS_UNSPEC,
>+	DPLL_LOCK_STATUS_UNLOCKED,
>+	DPLL_LOCK_STATUS_CALIBRATING,
>+	DPLL_LOCK_STATUS_LOCKED,
>+	DPLL_LOCK_STATUS_HOLDOVER,
>+
>+	__DPLL_LOCK_STATUS_MAX,
>+	DPLL_LOCK_STATUS_MAX = (__DPLL_LOCK_STATUS_MAX - 1)
>+};
>+
>+/**
>+ * enum dpll_pin_type - Enumerates types of a pin, valid values for
>+ *   DPLL_A_PIN_TYPE attribute
>+ * @DPLL_PIN_TYPE_UNSPEC: unspecified value
>+ * @DPLL_PIN_TYPE_MUX: aggregates another layer of selectable pins
>+ * @DPLL_PIN_TYPE_EXT: external source
>+ * @DPLL_PIN_TYPE_SYNCE_ETH_PORT: ethernet port PHY's recovered clock
>+ * @DPLL_PIN_TYPE_INT_OSCILLATOR: device internal oscillator
>+ * @DPLL_PIN_TYPE_GNSS: GNSS recovered clock
>+ */
>+enum dpll_pin_type {
>+	DPLL_PIN_TYPE_UNSPEC,
>+	DPLL_PIN_TYPE_MUX,
>+	DPLL_PIN_TYPE_EXT,
>+	DPLL_PIN_TYPE_SYNCE_ETH_PORT,
>+	DPLL_PIN_TYPE_INT_OSCILLATOR,
>+	DPLL_PIN_TYPE_GNSS,
>+
>+	__DPLL_PIN_TYPE_MAX,
>+	DPLL_PIN_TYPE_MAX = (__DPLL_PIN_TYPE_MAX - 1)
>+};
>+
>+/**
>+ * enum dpll_pin_state - available pin modes

For some of the enums, you say for what attribute it serves as a value
set, for some you don't. Please unify the approach. I think it is
valuable to say that for every enum.


>+ * @DPLL_PIN_STATE_UNSPEC: unspecified value
>+ * @DPLL_PIN_STATE_CONNECTED: pin connected
>+ * @DPLL_PIN_STATE_DISCONNECTED: pin disconnected
>+ */
>+enum dpll_pin_state {
>+	DPLL_PIN_STATE_UNSPEC,
>+	DPLL_PIN_STATE_CONNECTED,
>+	DPLL_PIN_STATE_DISCONNECTED,
>+
>+	__DPLL_PIN_STATE_MAX,
>+	DPLL_PIN_STATE_MAX = (__DPLL_PIN_STATE_MAX - 1)
>+};
>+
>+/**
>+ * enum dpll_pin_direction - available pin direction
>+ * @DPLL_PIN_DIRECTION_UNSPEC: unspecified value
>+ * @DPLL_PIN_DIRECTION_SOURCE: pin used as a source of a signal
>+ * @DPLL_PIN_DIRECTION_OUTPUT: pin used to output the signal
>+ */
>+enum dpll_pin_direction {
>+	DPLL_PIN_DIRECTION_UNSPEC,
>+	DPLL_PIN_DIRECTION_SOURCE,
>+	DPLL_PIN_DIRECTION_OUTPUT,
>+
>+	__DPLL_PIN_DIRECTION_MAX,
>+	DPLL_PIN_DIRECTION_MAX = (__DPLL_PIN_DIRECTION_MAX - 1)
>+};
>+
>+/**
>+ * enum dpll_mode - working-modes a dpll can support, differentiate if and how
>+ *   dpll selects one of its sources to syntonize with it
>+ * @DPLL_MODE_UNSPEC: unspecified value
>+ * @DPLL_MODE_MANUAL: source can be only selected by sending a request to dpll
>+ * @DPLL_MODE_AUTOMATIC: highest prio, valid source, auto selected by dpll
>+ * @DPLL_MODE_HOLDOVER: dpll forced into holdover mode
>+ * @DPLL_MODE_FREERUN: dpll driven on system clk, no holdover available
>+ * @DPLL_MODE_NCO: dpll driven by Numerically Controlled Oscillator
>+ */
>+enum dpll_mode {
>+	DPLL_MODE_UNSPEC,
>+	DPLL_MODE_MANUAL,
>+	DPLL_MODE_AUTOMATIC,
>+	DPLL_MODE_HOLDOVER,
>+	DPLL_MODE_FREERUN,
>+	DPLL_MODE_NCO,
>+
>+	__DPLL_MODE_MAX,
>+	DPLL_MODE_MAX = (__DPLL_MODE_MAX - 1)
>+};
>+
>+/**
>+ * enum dpll_type - type of dpll, valid values for DPLL_A_TYPE attribute
>+ * @DPLL_TYPE_UNSPEC: unspecified value
>+ * @DPLL_TYPE_PPS: dpll produces Pulse-Per-Second signal
>+ * @DPLL_TYPE_EEC: dpll drives the Ethernet Equipment Clock
>+ */
>+enum dpll_type {
>+	DPLL_TYPE_UNSPEC,
>+	DPLL_TYPE_PPS,
>+	DPLL_TYPE_EEC,
>+
>+	__DPLL_TYPE_MAX,
>+	DPLL_TYPE_MAX = (__DPLL_TYPE_MAX - 1)
>+};
>+
>+/**
>+ * enum dpll_event - events of dpll generic netlink family
>+ * @DPLL_EVENT_UNSPEC: invalid event type
>+ * @DPLL_EVENT_DEVICE_CREATE: dpll device created
>+ * @DPLL_EVENT_DEVICE_DELETE: dpll device deleted
>+ * @DPLL_EVENT_DEVICE_CHANGE: attribute of dpll device or pin changed, reason
>+ *   is to be found with an attribute type (DPLL_A_*) received with the event
>+ */
>+enum dpll_event {
>+	DPLL_EVENT_UNSPEC,
>+	DPLL_EVENT_DEVICE_CREATE,
>+	DPLL_EVENT_DEVICE_DELETE,
>+	DPLL_EVENT_DEVICE_CHANGE,
>+};
>+
>+/**
>+ * enum dpll_pin_caps - define capabilities of a pin
>+ */
>+enum dpll_pin_caps {
>+	DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE = 1,
>+	DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE = 2,
>+	DPLL_PIN_CAPS_STATE_CAN_CHANGE = 4,
>+};
>+
>+enum dplla {
>+	DPLL_A_DEVICE = 1,
>+	DPLL_A_ID,
>+	DPLL_A_DEV_NAME,
>+	DPLL_A_BUS_NAME,
>+	DPLL_A_MODE,
>+	DPLL_A_MODE_SUPPORTED,
>+	DPLL_A_SOURCE_PIN_IDX,
>+	DPLL_A_LOCK_STATUS,
>+	DPLL_A_TEMP,
>+	DPLL_A_CLOCK_ID,
>+	DPLL_A_TYPE,
>+	DPLL_A_PIN,
>+	DPLL_A_PIN_IDX,
>+	DPLL_A_PIN_DESCRIPTION,

I commented this name in the other email. This does not describe
anything. It is a label on a connector, isn't it? Why don't to call it
"label"?


>+	DPLL_A_PIN_TYPE,
>+	DPLL_A_PIN_DIRECTION,
>+	DPLL_A_PIN_FREQUENCY,
>+	DPLL_A_PIN_FREQUENCY_SUPPORTED,
>+	DPLL_A_PIN_ANY_FREQUENCY_MIN,
>+	DPLL_A_PIN_ANY_FREQUENCY_MAX,

Drop "any". Not good for anything.


>+	DPLL_A_PIN_PRIO,
>+	DPLL_A_PIN_STATE,
>+	DPLL_A_PIN_PARENT,
>+	DPLL_A_PIN_PARENT_IDX,
>+	DPLL_A_PIN_RCLK_DEVICE,
>+	DPLL_A_PIN_DPLL_CAPS,

Just DPLL_A_PIN_CAPS is enough, that would be also consistent with the
enum name.

>+
>+	__DPLL_A_MAX,
>+	DPLL_A_MAX = (__DPLL_A_MAX - 1)
>+};
>+
>+enum {
>+	DPLL_CMD_UNSPEC,
>+	DPLL_CMD_DEVICE_GET,
>+	DPLL_CMD_DEVICE_SET,
>+	DPLL_CMD_PIN_GET,
>+	DPLL_CMD_PIN_SET,
>+
>+	__DPLL_CMD_MAX,
>+	DPLL_CMD_MAX = (__DPLL_CMD_MAX - 1)
>+};
>+
>+#define DPLL_MCGRP_MONITOR	"monitor"
>+
>+#endif /* _UAPI_LINUX_DPLL_H */
>-- 
>2.34.1
>

Arkadiusz Kubalewski March 16, 2023, 1:15 p.m. UTC | #2

>From: Jiri Pirko <[e-mail]>
>Sent: Tuesday, March 14, 2023 3:44 PM
>
>Sun, Mar 12, 2023 at 03:28:02AM CET, [e-mail] wrote:
>>From: Arkadiusz Kubalewski <[e-mail]>
>>
>>Add a protocol spec for DPLL.
>>Add code generated from the spec.
>>
>>Signed-off-by: Jakub Kicinski <[e-mail]>
>>Signed-off-by: Michal Michalik <[e-mail]>
>>Signed-off-by: Arkadiusz Kubalewski <[e-mail]>
>>Signed-off-by: Vadim Fedorenko <[e-mail]>
>>---
>> Documentation/netlink/specs/dpll.yaml | 514 ++++++++++++++++++++++++++
>> drivers/dpll/dpll_nl.c                | 126 +++++++
>> drivers/dpll/dpll_nl.h                |  42 +++
>> include/uapi/linux/dpll.h             | 196 ++++++++++
>> 4 files changed, 878 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..03e9f0e2d3d2
>>--- /dev/null
>>+++ b/Documentation/netlink/specs/dpll.yaml
>>@@ -0,0 +1,514 @@
>>+name: dpll
>>+
>>+doc: DPLL subsystem.
>>+
>>+definitions:
>>+  -
>>+    type: const
>>+    name: temp-divider
>>+    value: 10
>>+  -
>>+    type: const
>>+    name: pin-freq-1-hz
>>+    value: 1
>>+  -
>>+    type: const
>>+    name: pin-freq-10-mhz
>>+    value: 10000000
>>+  -
>>+    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 (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
>>+    render-max: true
>>+  -
>>+    type: enum
>>+    name: pin-type
>>+    doc: Enumerates types of a pin, valid values for DPLL_A_PIN_TYPE
>>attribute
>>+    entries:
>>+      -
>>+        name: unspec
>>+        doc: unspecified value
>>+      -
>>+        name: mux
>>+        doc: aggregates another layer of selectable pins
>>+      -
>>+        name: ext
>>+        doc: external source
>>+      -
>>+        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-state
>>+    doc: available pin modes
>>+    entries:
>>+      -
>>+        name: unspec
>>+        doc: unspecified value
>>+      -
>>+        name: connected
>>+        doc: pin connected
>>+      -
>>+        name: disconnected
>>+        doc: pin disconnected
>>+    render-max: true
>>+  -
>>+    type: enum
>>+    name: pin-direction
>>+    doc: available pin direction
>>+    entries:
>>+      -
>>+        name: unspec
>>+        doc: unspecified value
>>+      -
>>+        name: source
>>+        doc: pin used as a source of a signal
>>+      -
>>+        name: output
>>+        doc: pin used to output the signal
>>+    render-max: true
>>+  -
>>+    type: enum
>>+    name: mode
>
>Could you please sort the enums so they are in the same order as the
>attribute which carries them? That would also put the device and pin
>enums together.
>

Fixed.

>
>>+    doc: |
>>+      working-modes a dpll can support, differentiate if and how dpll
>>selects
>>+      one of its sources to syntonize with it
>>+    entries:
>>+      -
>>+        name: unspec
>>+        doc: unspecified value
>>+      -
>>+        name: manual
>>+        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
>>+      -
>>+        name: nco
>>+        doc: dpll driven by Numerically Controlled Oscillator
>>+    render-max: true
>>+  -
>>+    type: enum
>>+    name: type
>>+    doc: type of dpll, valid values for DPLL_A_TYPE attribute
>>+    entries:
>>+      -
>>+        name: unspec
>>+        doc: unspecified value
>>+      -
>>+        name: pps
>>+        doc: dpll produces Pulse-Per-Second signal
>>+      -
>>+        name: eec
>>+        doc: dpll drives the Ethernet Equipment Clock
>>+    render-max: true
>>+  -
>>+    type: enum
>>+    name: event
>>+    doc: events of dpll generic netlink family
>>+    entries:
>>+      -
>>+        name: unspec
>>+        doc: invalid event type
>>+      -
>>+        name: device-create
>>+        doc: dpll device created
>>+      -
>>+        name: device-delete
>>+        doc: dpll device deleted
>>+      -
>>+        name: device-change
>>+        doc: |
>>+          attribute of dpll device or pin changed, reason is to be found
>>with
>>+          an attribute type (DPLL_A_*) received with the event
>>+  -
>>+    type: flags
>>+    name: pin-caps
>>+    doc: define capabilities of a pin
>>+    entries:
>>+      -
>>+        name: direction-can-change
>>+      -
>>+        name: priority-can-change
>>+      -
>>+        name: state-can-change
>>+
>>+
>>+attribute-sets:
>>+  -
>>+    name: dpll
>>+    enum-name: dplla
>>+    attributes:
>>+      -
>>+        name: device
>>+        type: nest
>>+        value: 1
>>+        multi-attr: true
>>+        nested-attributes: device
>
>What is this "device" and what is it good for? Smells like some leftover
>and with the nested scheme looks quite odd.
>

No, it is nested attribute type, used when multiple devices are returned with
netlink:

- dump of device-get command where all devices are returned, each one nested
inside it:
[{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id': 0},
             {'bus-name': 'pci', 'dev-name': '0000:21:00.0_1', 'id': 1}]}]

- do/dump of pin-get, in case of shared pins, each pin contains number of dpll
handles it connects with:
[{'pin': [{'device': [{'bus-name': 'pci',
                       'dev-name': '0000:21:00.0_0',
                       'id': 0,
                       'pin-prio': 6,
                       'pin-state': {'doc': 'pin connected',
                                     'name': 'connected'}},
                      {'bus-name': 'pci',
                       'dev-name': '0000:21:00.0_1',
                       'id': 1,
                       'pin-prio': 8,
                       'pin-state': {'doc': 'pin connected',
                                     'name': 'connected'}}],
           'pin-direction': {'doc': 'pin used as a source of a signal',
                             'name': 'source'},
           'pin-frequency': 1,
           'pin-frequency-supported': [1, 10000000],
           'pin-idx': 0,
	...

>
>>+      -
>>+        name: id
>>+        type: u32
>>+      -
>>+        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: source-pin-idx
>>+        type: u32
>>+      -
>>+        name: lock-status
>>+        type: u8
>>+        enum: lock-status
>>+      -
>>+        name: temp
>
>Could you put some comment regarding the divider here?
>

Sure, fixed.

>
>>+        type: s32
>>+      -
>>+        name: clock-id
>>+        type: u64
>>+      -
>>+        name: type
>>+        type: u8
>>+        enum: type
>>+      -
>>+        name: pin
>>+        type: nest
>>+        multi-attr: true
>>+        nested-attributes: pin
>>+        value: 12
>>+      -
>>+        name: pin-idx
>>+        type: u32
>>+      -
>>+        name: pin-description
>>+        type: string
>>+      -
>>+        name: pin-type
>>+        type: u8
>>+        enum: pin-type
>>+      -
>>+        name: pin-direction
>>+        type: u8
>>+        enum: pin-direction
>>+      -
>>+        name: pin-frequency
>>+        type: u32
>>+      -
>>+        name: pin-frequency-supported
>>+        type: u32
>>+        multi-attr: true
>>+      -
>>+        name: pin-any-frequency-min
>>+        type: u32
>>+      -
>>+        name: pin-any-frequency-max
>>+        type: u32
>
>These 2 attrs somehow overlap with pin-frequency-supported,
>which is a bit confusing, I think that pin-frequency-supported
>should carry all supported frequencies. How about to have something
>like:
>        name: pin-frequency-supported
>        type: nest
>	multi-attr: true
>        nested-attributes: pin-frequency-range
>
>and then:
>      name: pin-frequency-range
>      subset-of: dpll
>      attributes:
>        -
>          name: pin-frequency-min
>          type: u32
>        -
>          name: pin-frequency-max
>          type: u32
>          doc: should be put only to specify range when value differs
>	       from pin-frequency-min
>
>That could carry list of frequencies and ranges, in this case multiple
>ones if possibly needed.
>

Sure

>
>
>>+      -
>>+        name: pin-prio
>>+        type: u32
>>+      -
>>+        name: pin-state
>>+        type: u8
>>+        enum: pin-state
>>+      -
>>+        name: pin-parent
>>+        type: nest
>>+        multi-attr: true
>>+        nested-attributes: pin
>>+        value: 23
>
>Value 23? What's this?
>You have it specified for some attrs all over the place.
>What is the reason for it?
>

Actually this particular one is not needed (also value: 12 on pin above),
I will remove those.
But the others you are refering to (the ones in nested attribute list),
are required because of cli.py parser issue, maybe Kuba knows a better way to
prevent the issue?
Basically, without those values, cli.py brakes on parsing responses, after
every "jump" to nested attribute list it is assigning first attribute there
with value=0, thus there is a need to assign a proper value, same as it is on
'main' attribute list.

>
>>+      -
>>+        name: pin-parent-idx
>>+        type: u32
>>+      -
>>+        name: pin-rclk-device
>>+        type: string
>>+      -
>>+        name: pin-dpll-caps
>>+        type: u32
>>+  -
>>+    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: source-pin-idx
>>+        type: u32
>>+      -
>>+        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
>>+        type: nest
>>+        value: 12
>>+        multi-attr: true
>>+        nested-attributes: pin
>
>This does not belong here.
>

What do you mean?
With device-get 'do' request the list of pins connected to the dpll is
returned, each pin is nested in this attribute.
This is required by parser to work.

>
>>+      -
>>+        name: pin-prio
>>+        type: u32
>>+        value: 21
>>+      -
>>+        name: pin-state
>>+        type: u8
>>+        enum: pin-state
>>+      -
>>+        name: pin-dpll-caps
>>+        type: u32
>>+        value: 26
>
>All these 3 do not belong here are well.
>

Same as above explanation.

>
>
>>+  -
>>+    name: pin
>>+    subset-of: dpll
>>+    attributes:
>>+      -
>>+        name: device
>>+        type: nest
>>+        value: 1
>>+        multi-attr: true
>>+        nested-attributes: device
>>+      -
>>+        name: pin-idx
>>+        type: u32
>>+        value: 13
>>+      -
>>+        name: pin-description
>>+        type: string
>>+      -
>>+        name: pin-type
>>+        type: u8
>>+        enum: pin-type
>>+      -
>>+        name: pin-direction
>>+        type: u8
>>+        enum: pin-direction
>>+      -
>>+        name: pin-frequency
>>+        type: u32
>>+      -
>>+        name: pin-frequency-supported
>>+        type: u32
>>+        multi-attr: true
>>+      -
>>+        name: pin-any-frequency-min
>>+        type: u32
>>+      -
>>+        name: pin-any-frequency-max
>>+        type: u32
>>+      -
>>+        name: pin-prio
>>+        type: u32
>>+      -
>>+        name: pin-state
>>+        type: u8
>>+        enum: pin-state
>>+      -
>>+        name: pin-parent
>>+        type: nest
>>+        multi-attr: true
>
>Multiple parents? How is that supposed to work?
>

As we have agreed, MUXed pins can have multiple parents.
In our case:
/tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do pin-get --json '{"id": 0, "pin-idx":13}'
{'pin': [{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id': 0},
                     {'bus-name': 'pci',
                      'dev-name': '0000:21:00.0_1',
                      'id': 1}],
          'pin-description': '0000:21:00.0',
          'pin-direction': {'doc': 'pin used as a source of a signal',
                            'name': 'source'},
          'pin-idx': 13,
          'pin-parent': [{'pin-parent-idx': 2,
                          'pin-state': {'doc': 'pin disconnected',
                                        'name': 'disconnected'}},
                         {'pin-parent-idx': 3,
                          'pin-state': {'doc': 'pin disconnected',
                                        'name': 'disconnected'}}],
          'pin-rclk-device': '0000:21:00.0',
          'pin-type': {'doc': "ethernet port PHY's recovered clock",
                       'name': 'synce-eth-port'}}]}


>
>>+        nested-attributes: pin-parent
>>+        value: 23
>>+      -
>>+        name: pin-rclk-device
>>+        type: string
>>+        value: 25
>>+      -
>>+        name: pin-dpll-caps
>>+        type: u32
>
>Missing "enum: "
>

It is actually a bitmask, this is why didn't set as enum, with enum type
parser won't parse it.

>
>>+  -
>>+    name: pin-parent
>>+    subset-of: dpll
>>+    attributes:
>>+      -
>>+        name: pin-state
>>+        type: u8
>>+        value: 22
>>+        enum: pin-state
>>+      -
>>+        name: pin-parent-idx
>>+        type: u32
>>+        value: 24
>>+      -
>>+        name: pin-rclk-device
>>+        type: string
>
>Yeah, as I wrote in the other email, this really smells to
>have like a simple string like this. What is it supposed to be?
>

Yes, let's discuss there.

>
>>+
>>+
>>+operations:
>>+  list:
>>+    -
>>+      name: unspec
>>+      doc: unused
>>+
>>+    -
>>+      name: device-get
>>+      doc: |
>>+        Get list of DPLL devices (dump) or attributes of a single dpll
>device
>>+      attribute-set: dpll
>
>Shouldn't this be "device"?
>

It would brake the parser, again I hope Jakub Kicinski could take a look on
this.

>
>>+      flags: [ admin-perm ]
>>+
>>+      do:
>>+        pre: dpll-pre-doit
>>+        post: dpll-post-doit
>>+        request:
>>+          attributes:
>>+            - id
>>+            - bus-name
>>+            - dev-name
>>+        reply:
>>+          attributes:
>>+            - device
>>+
>>+      dump:
>>+        pre: dpll-pre-dumpit
>>+        post: dpll-post-dumpit
>>+        reply:
>>+          attributes:
>>+            - device
>>+
>>+    -
>>+      name: device-set
>>+      doc: Set attributes for a DPLL device
>>+      attribute-set: dpll
>
>"device" here as well?
>

Same as above.

>
>>+      flags: [ admin-perm ]
>>+
>>+      do:
>>+        pre: dpll-pre-doit
>>+        post: dpll-post-doit
>>+        request:
>>+          attributes:
>>+            - id
>>+            - bus-name
>>+            - dev-name
>>+            - mode
>
>Hmm, shouldn't source-pin-index be here as well?

No, there is no set for this.
For manual mode user selects the pin by setting enabled state on the one
he needs to recover signal from.

source-pin-index is read only, returns active source.

>
>>+
>>+    -
>>+      name: pin-get
>>+      doc: |
>>+        Get list of pins and its attributes.
>>+        - dump request without any attributes given - list all the pins
>in the system
>>+        - dump request with target dpll - list all the pins registered
>with a given dpll device
>>+        - do request with target dpll and target pin - single pin
>attributes
>>+      attribute-set: dpll
>
>"pin"?
>

Same case as with dpll/device above.

>
>>+      flags: [ admin-perm ]
>>+
>>+      do:
>>+        pre: dpll-pin-pre-doit
>>+        post: dpll-pin-post-doit
>>+        request:
>>+          attributes:
>>+            - id
>>+            - bus-name
>>+            - dev-name
>>+            - pin-idx
>>+        reply:
>>+          attributes:
>>+            - pin
>>+
>>+      dump:
>>+        pre: dpll-pin-pre-dumpit
>>+        post: dpll-pin-post-dumpit
>>+        request:
>>+          attributes:
>>+            - id
>>+            - bus-name
>>+            - dev-name
>>+        reply:
>>+          attributes:
>>+            - pin
>>+
>>+    -
>>+      name: pin-set
>>+      doc: Set attributes of a target pin
>>+      attribute-set: dpll
>
>"pin"?
>

Same case as with dpll/device above.

>
>>+      flags: [ admin-perm ]
>>+
>>+      do:
>>+        pre: dpll-pin-pre-doit
>>+        post: dpll-pin-post-doit
>>+        request:
>>+          attributes:
>>+            - id
>>+            - bus-name
>>+            - dev-name
>>+            - pin-idx
>>+            - pin-frequency
>>+            - pin-direction
>>+            - pin-prio
>>+            - pin-parent-idx
>>+            - pin-state
>>+
>>+mcast-groups:
>>+  list:
>>+    -
>>+      name: monitor
>>diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c
>>new file mode 100644
>>index 000000000000..099d1e30ca7c
>>--- /dev/null
>>+++ b/drivers/dpll/dpll_nl.c
>>@@ -0,0 +1,126 @@
>>+// SPDX-License-Identifier: BSD-3-Clause
>>+/* Do not edit directly, auto-generated from: */
>>+/*	Documentation/netlink/specs/dpll.yaml */
>>+/* YNL-GEN kernel source */
>>+
>>+#include <net/netlink.h>
>>+#include <net/genetlink.h>
>>+
>>+#include "dpll_nl.h"
>>+
>>+#include <linux/dpll.h>
>>+
>>+/* DPLL_CMD_DEVICE_GET - do */
>>+static const struct nla_policy dpll_device_get_nl_policy[DPLL_A_BUS_NAME
>>+ 1] = {
>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>+};
>>+
>>+/* DPLL_CMD_DEVICE_SET - do */
>>+static const struct nla_policy dpll_device_set_nl_policy[DPLL_A_MODE + 1]
>>= {
>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>+	[DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),
>
>Hmm, any idea why the generator does not put define name
>here instead of "5"?
>

Not really, it probably needs a fix for this.

>
>>+};
>>+
>>+/* DPLL_CMD_PIN_GET - do */
>>+static const struct nla_policy dpll_pin_get_do_nl_policy[DPLL_A_PIN_IDX +
>>1] = {
>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>+	[DPLL_A_PIN_IDX] = { .type = NLA_U32, },
>>+};
>>+
>>+/* DPLL_CMD_PIN_GET - dump */
>>+static const struct nla_policy
>>dpll_pin_get_dump_nl_policy[DPLL_A_BUS_NAME + 1] = {
>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>+};
>>+
>>+/* DPLL_CMD_PIN_SET - do */
>>+static const struct nla_policy
>>dpll_pin_set_nl_policy[DPLL_A_PIN_PARENT_IDX + 1] = {
>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>+	[DPLL_A_PIN_IDX] = { .type = NLA_U32, },
>>+	[DPLL_A_PIN_FREQUENCY] = { .type = NLA_U32, },
>
>4.3GHz would be the limit, isn't it future proof to make this rather u64?
>

Sure, fixed.

>
>>+	[DPLL_A_PIN_DIRECTION] = NLA_POLICY_MAX(NLA_U8, 2),
>>+	[DPLL_A_PIN_PRIO] = { .type = NLA_U32, },
>>+	[DPLL_A_PIN_PARENT_IDX] = { .type = NLA_U32, },
>
>This is a bit odd. The size is DPLL_A_PIN_PARENT_IDX + 1, yet PIN_STATE
>is last. I found that the order is not according to the enum in the yaml
>operation definition. Perhaps the generator can sort this?
>

Fixed. This related to the order on list of attributes expected on ops
definition.

>
>>+	[DPLL_A_PIN_STATE] = NLA_POLICY_MAX(NLA_U8, 2),
>>+};
>>+
>>+/* Ops table for dpll */
>>+static const struct genl_split_ops dpll_nl_ops[6] = {
>>+	{
>>+		.cmd		= DPLL_CMD_DEVICE_GET,
>>+		.pre_doit	= dpll_pre_doit,
>>+		.doit		= dpll_nl_device_get_doit,
>>+		.post_doit	= dpll_post_doit,
>>+		.policy		= dpll_device_get_nl_policy,
>>+		.maxattr	= DPLL_A_BUS_NAME,
>>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>+	},
>>+	{
>>+		.cmd	= DPLL_CMD_DEVICE_GET,
>>+		.start	= dpll_pre_dumpit,
>>+		.dumpit	= dpll_nl_device_get_dumpit,
>>+		.done	= dpll_post_dumpit,
>>+		.flags	= GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
>>+	},
>>+	{
>>+		.cmd		= DPLL_CMD_DEVICE_SET,
>>+		.pre_doit	= dpll_pre_doit,
>>+		.doit		= dpll_nl_device_set_doit,
>>+		.post_doit	= dpll_post_doit,
>>+		.policy		= dpll_device_set_nl_policy,
>>+		.maxattr	= DPLL_A_MODE,
>>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>+	},
>>+	{
>>+		.cmd		= DPLL_CMD_PIN_GET,
>>+		.pre_doit	= dpll_pin_pre_doit,
>>+		.doit		= dpll_nl_pin_get_doit,
>>+		.post_doit	= dpll_pin_post_doit,
>>+		.policy		= dpll_pin_get_do_nl_policy,
>>+		.maxattr	= DPLL_A_PIN_IDX,
>>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>+	},
>>+	{
>>+		.cmd		= DPLL_CMD_PIN_GET,
>>+		.start		= dpll_pin_pre_dumpit,
>>+		.dumpit		= dpll_nl_pin_get_dumpit,
>>+		.done		= dpll_pin_post_dumpit,
>>+		.policy		= dpll_pin_get_dump_nl_policy,
>>+		.maxattr	= DPLL_A_BUS_NAME,
>>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
>>+	},
>>+	{
>>+		.cmd		= DPLL_CMD_PIN_SET,
>>+		.pre_doit	= dpll_pin_pre_doit,
>>+		.doit		= dpll_nl_pin_set_doit,
>>+		.post_doit	= dpll_pin_post_doit,
>>+		.policy		= dpll_pin_set_nl_policy,
>>+		.maxattr	= DPLL_A_PIN_PARENT_IDX,
>>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>+	},
>>+};
>>+
>>+static const struct genl_multicast_group dpll_nl_mcgrps[] = {
>>+	[DPLL_NLGRP_MONITOR] = { "monitor", },
>>+};
>>+
>>+struct genl_family dpll_nl_family __ro_after_init = {
>>+	.name		= DPLL_FAMILY_NAME,
>>+	.version	= DPLL_FAMILY_VERSION,
>>+	.netnsok	= true,
>>+	.parallel_ops	= true,
>>+	.module		= THIS_MODULE,
>>+	.split_ops	= dpll_nl_ops,
>>+	.n_split_ops	= ARRAY_SIZE(dpll_nl_ops),
>>+	.mcgrps		= dpll_nl_mcgrps,
>>+	.n_mcgrps	= ARRAY_SIZE(dpll_nl_mcgrps),
>>+};
>>diff --git a/drivers/dpll/dpll_nl.h b/drivers/dpll/dpll_nl.h
>>new file mode 100644
>>index 000000000000..3a354dfb9a31
>>--- /dev/null
>>+++ b/drivers/dpll/dpll_nl.h
>>@@ -0,0 +1,42 @@
>>+/* SPDX-License-Identifier: BSD-3-Clause */
>>+/* Do not edit directly, auto-generated from: */
>>+/*	Documentation/netlink/specs/dpll.yaml */
>>+/* YNL-GEN kernel header */
>>+
>>+#ifndef _LINUX_DPLL_GEN_H
>>+#define _LINUX_DPLL_GEN_H
>>+
>>+#include <net/netlink.h>
>>+#include <net/genetlink.h>
>>+
>>+#include <linux/dpll.h>
>>+
>>+int dpll_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>>+		  struct genl_info *info);
>>+int dpll_pin_pre_doit(const struct genl_split_ops *ops, struct sk_buff
>*skb,
>>+		      struct genl_info *info);
>>+void
>>+dpll_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>>+	       struct genl_info *info);
>>+void
>>+dpll_pin_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>>+		   struct genl_info *info);
>>+int dpll_pre_dumpit(struct netlink_callback *cb);
>>+int dpll_pin_pre_dumpit(struct netlink_callback *cb);
>>+int dpll_post_dumpit(struct netlink_callback *cb);
>>+int dpll_pin_post_dumpit(struct netlink_callback *cb);
>>+
>>+int dpll_nl_device_get_doit(struct sk_buff *skb, struct genl_info *info);
>>+int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct
>netlink_callback *cb);
>>+int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info);
>>+int dpll_nl_pin_get_doit(struct sk_buff *skb, struct genl_info *info);
>>+int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback
>*cb);
>>+int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info);
>>+
>>+enum {
>>+	DPLL_NLGRP_MONITOR,
>>+};
>>+
>>+extern struct genl_family dpll_nl_family;
>>+
>>+#endif /* _LINUX_DPLL_GEN_H */
>>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>>new file mode 100644
>>index 000000000000..ece6fe685d08
>>--- /dev/null
>>+++ b/include/uapi/linux/dpll.h
>>@@ -0,0 +1,196 @@
>>+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>+/* Do not edit directly, auto-generated from: */
>>+/*	Documentation/netlink/specs/dpll.yaml */
>>+/* YNL-GEN uapi header */
>>+
>>+#ifndef _UAPI_LINUX_DPLL_H
>>+#define _UAPI_LINUX_DPLL_H
>>+
>>+#define DPLL_FAMILY_NAME	"dpll"
>>+#define DPLL_FAMILY_VERSION	1
>>+
>>+#define DPLL_TEMP_DIVIDER	10
>
>Some comment to this define would be nice.
>

I added to the spec, but it is not generated by the regen script.

>
>>+#define DPLL_PIN_FREQ_1_HZ	1
>>+#define DPLL_PIN_FREQ_10_MHZ	10000000
>
>Have this as enum. Spell out "frequency" to be consistent with the
>related attribute name.
>

Sure, fixed.

>
>>+
>>+/**
>>+ * enum dpll_lock_status - Provides information of dpll device lock
>>status,
>>+ *   valid values for DPLL_A_LOCK_STATUS attribute
>>+ * @DPLL_LOCK_STATUS_UNSPEC: unspecified value
>>+ * @DPLL_LOCK_STATUS_UNLOCKED: dpll was not yet locked to any valid (or
>is in
>
>"any valid source" perhaps?
>

Yes, fixed.

>
>>+ *   one of modes: DPLL_MODE_FREERUN, DPLL_MODE_NCO)
>>+ * @DPLL_LOCK_STATUS_CALIBRATING: dpll is trying to lock to a valid
>>signal
>>+ * @DPLL_LOCK_STATUS_LOCKED: dpll is locked
>>+ * @DPLL_LOCK_STATUS_HOLDOVER: dpll is in holdover state - lost a valid
>>lock or
>>+ *   was forced by selecting DPLL_MODE_HOLDOVER mode
>>+ */
>>+enum dpll_lock_status {
>>+	DPLL_LOCK_STATUS_UNSPEC,
>>+	DPLL_LOCK_STATUS_UNLOCKED,
>>+	DPLL_LOCK_STATUS_CALIBRATING,
>>+	DPLL_LOCK_STATUS_LOCKED,
>>+	DPLL_LOCK_STATUS_HOLDOVER,
>>+
>>+	__DPLL_LOCK_STATUS_MAX,
>>+	DPLL_LOCK_STATUS_MAX = (__DPLL_LOCK_STATUS_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_pin_type - Enumerates types of a pin, valid values for
>>+ *   DPLL_A_PIN_TYPE attribute
>>+ * @DPLL_PIN_TYPE_UNSPEC: unspecified value
>>+ * @DPLL_PIN_TYPE_MUX: aggregates another layer of selectable pins
>>+ * @DPLL_PIN_TYPE_EXT: external source
>>+ * @DPLL_PIN_TYPE_SYNCE_ETH_PORT: ethernet port PHY's recovered clock
>>+ * @DPLL_PIN_TYPE_INT_OSCILLATOR: device internal oscillator
>>+ * @DPLL_PIN_TYPE_GNSS: GNSS recovered clock
>>+ */
>>+enum dpll_pin_type {
>>+	DPLL_PIN_TYPE_UNSPEC,
>>+	DPLL_PIN_TYPE_MUX,
>>+	DPLL_PIN_TYPE_EXT,
>>+	DPLL_PIN_TYPE_SYNCE_ETH_PORT,
>>+	DPLL_PIN_TYPE_INT_OSCILLATOR,
>>+	DPLL_PIN_TYPE_GNSS,
>>+
>>+	__DPLL_PIN_TYPE_MAX,
>>+	DPLL_PIN_TYPE_MAX = (__DPLL_PIN_TYPE_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_pin_state - available pin modes
>
>For some of the enums, you say for what attribute it serves as a value
>set, for some you don't. Please unify the approach. I think it is
>valuable to say that for every enum.
>

Sure, fixed.

>
>>+ * @DPLL_PIN_STATE_UNSPEC: unspecified value
>>+ * @DPLL_PIN_STATE_CONNECTED: pin connected
>>+ * @DPLL_PIN_STATE_DISCONNECTED: pin disconnected
>>+ */
>>+enum dpll_pin_state {
>>+	DPLL_PIN_STATE_UNSPEC,
>>+	DPLL_PIN_STATE_CONNECTED,
>>+	DPLL_PIN_STATE_DISCONNECTED,
>>+
>>+	__DPLL_PIN_STATE_MAX,
>>+	DPLL_PIN_STATE_MAX = (__DPLL_PIN_STATE_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_pin_direction - available pin direction
>>+ * @DPLL_PIN_DIRECTION_UNSPEC: unspecified value
>>+ * @DPLL_PIN_DIRECTION_SOURCE: pin used as a source of a signal
>>+ * @DPLL_PIN_DIRECTION_OUTPUT: pin used to output the signal
>>+ */
>>+enum dpll_pin_direction {
>>+	DPLL_PIN_DIRECTION_UNSPEC,
>>+	DPLL_PIN_DIRECTION_SOURCE,
>>+	DPLL_PIN_DIRECTION_OUTPUT,
>>+
>>+	__DPLL_PIN_DIRECTION_MAX,
>>+	DPLL_PIN_DIRECTION_MAX = (__DPLL_PIN_DIRECTION_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_mode - working-modes a dpll can support, differentiate if
>>and how
>>+ *   dpll selects one of its sources to syntonize with it
>>+ * @DPLL_MODE_UNSPEC: unspecified value
>>+ * @DPLL_MODE_MANUAL: source can be only selected by sending a request to
>>dpll
>>+ * @DPLL_MODE_AUTOMATIC: highest prio, valid source, auto selected by
>>dpll
>>+ * @DPLL_MODE_HOLDOVER: dpll forced into holdover mode
>>+ * @DPLL_MODE_FREERUN: dpll driven on system clk, no holdover available
>>+ * @DPLL_MODE_NCO: dpll driven by Numerically Controlled Oscillator
>>+ */
>>+enum dpll_mode {
>>+	DPLL_MODE_UNSPEC,
>>+	DPLL_MODE_MANUAL,
>>+	DPLL_MODE_AUTOMATIC,
>>+	DPLL_MODE_HOLDOVER,
>>+	DPLL_MODE_FREERUN,
>>+	DPLL_MODE_NCO,
>>+
>>+	__DPLL_MODE_MAX,
>>+	DPLL_MODE_MAX = (__DPLL_MODE_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_type - type of dpll, valid values for DPLL_A_TYPE attribute
>>+ * @DPLL_TYPE_UNSPEC: unspecified value
>>+ * @DPLL_TYPE_PPS: dpll produces Pulse-Per-Second signal
>>+ * @DPLL_TYPE_EEC: dpll drives the Ethernet Equipment Clock
>>+ */
>>+enum dpll_type {
>>+	DPLL_TYPE_UNSPEC,
>>+	DPLL_TYPE_PPS,
>>+	DPLL_TYPE_EEC,
>>+
>>+	__DPLL_TYPE_MAX,
>>+	DPLL_TYPE_MAX = (__DPLL_TYPE_MAX - 1)
>>+};
>>+
>>+/**
>>+ * enum dpll_event - events of dpll generic netlink family
>>+ * @DPLL_EVENT_UNSPEC: invalid event type
>>+ * @DPLL_EVENT_DEVICE_CREATE: dpll device created
>>+ * @DPLL_EVENT_DEVICE_DELETE: dpll device deleted
>>+ * @DPLL_EVENT_DEVICE_CHANGE: attribute of dpll device or pin changed,
>>reason
>>+ *   is to be found with an attribute type (DPLL_A_*) received with the
>>event
>>+ */
>>+enum dpll_event {
>>+	DPLL_EVENT_UNSPEC,
>>+	DPLL_EVENT_DEVICE_CREATE,
>>+	DPLL_EVENT_DEVICE_DELETE,
>>+	DPLL_EVENT_DEVICE_CHANGE,
>>+};
>>+
>>+/**
>>+ * enum dpll_pin_caps - define capabilities of a pin
>>+ */
>>+enum dpll_pin_caps {
>>+	DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE = 1,
>>+	DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE = 2,
>>+	DPLL_PIN_CAPS_STATE_CAN_CHANGE = 4,
>>+};
>>+
>>+enum dplla {
>>+	DPLL_A_DEVICE = 1,
>>+	DPLL_A_ID,
>>+	DPLL_A_DEV_NAME,
>>+	DPLL_A_BUS_NAME,
>>+	DPLL_A_MODE,
>>+	DPLL_A_MODE_SUPPORTED,
>>+	DPLL_A_SOURCE_PIN_IDX,
>>+	DPLL_A_LOCK_STATUS,
>>+	DPLL_A_TEMP,
>>+	DPLL_A_CLOCK_ID,
>>+	DPLL_A_TYPE,
>>+	DPLL_A_PIN,
>>+	DPLL_A_PIN_IDX,
>>+	DPLL_A_PIN_DESCRIPTION,
>
>I commented this name in the other email. This does not describe
>anything. It is a label on a connector, isn't it? Why don't to call it
>"label"?
>

Sure, fixed.

>
>>+	DPLL_A_PIN_TYPE,
>>+	DPLL_A_PIN_DIRECTION,
>>+	DPLL_A_PIN_FREQUENCY,
>>+	DPLL_A_PIN_FREQUENCY_SUPPORTED,
>>+	DPLL_A_PIN_ANY_FREQUENCY_MIN,
>>+	DPLL_A_PIN_ANY_FREQUENCY_MAX,
>
>Drop "any". Not good for anything.
>

Sure, fixed.

>
>>+	DPLL_A_PIN_PRIO,
>>+	DPLL_A_PIN_STATE,
>>+	DPLL_A_PIN_PARENT,
>>+	DPLL_A_PIN_PARENT_IDX,
>>+	DPLL_A_PIN_RCLK_DEVICE,
>>+	DPLL_A_PIN_DPLL_CAPS,
>
>Just DPLL_A_PIN_CAPS is enough, that would be also consistent with the
>enum name.

Sure, fixed.

>
>>+
>>+	__DPLL_A_MAX,
>>+	DPLL_A_MAX = (__DPLL_A_MAX - 1)
>>+};
>>+
>>+enum {
>>+	DPLL_CMD_UNSPEC,
>>+	DPLL_CMD_DEVICE_GET,
>>+	DPLL_CMD_DEVICE_SET,
>>+	DPLL_CMD_PIN_GET,
>>+	DPLL_CMD_PIN_SET,
>>+
>>+	__DPLL_CMD_MAX,
>>+	DPLL_CMD_MAX = (__DPLL_CMD_MAX - 1)
>>+};
>>+
>>+#define DPLL_MCGRP_MONITOR	"monitor"
>>+
>>+#endif /* _UAPI_LINUX_DPLL_H */
>>--
>>2.34.1
>>

Jiri Pirko March 16, 2023, 1:45 p.m. UTC | #3

Thu, Mar 16, 2023 at 02:15:59PM CET, [e-mail] wrote:
>>From: Jiri Pirko <[e-mail]>
>>Sent: Tuesday, March 14, 2023 3:44 PM
>>
>>Sun, Mar 12, 2023 at 03:28:02AM CET, [e-mail] wrote:
>>>From: Arkadiusz Kubalewski <[e-mail]>
>>>
>>>Add a protocol spec for DPLL.
>>>Add code generated from the spec.
>>>
>>>Signed-off-by: Jakub Kicinski <[e-mail]>
>>>Signed-off-by: Michal Michalik <[e-mail]>
>>>Signed-off-by: Arkadiusz Kubalewski <[e-mail]>
>>>Signed-off-by: Vadim Fedorenko <[e-mail]>
>>>---
>>> Documentation/netlink/specs/dpll.yaml | 514 ++++++++++++++++++++++++++
>>> drivers/dpll/dpll_nl.c                | 126 +++++++
>>> drivers/dpll/dpll_nl.h                |  42 +++
>>> include/uapi/linux/dpll.h             | 196 ++++++++++
>>> 4 files changed, 878 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..03e9f0e2d3d2
>>>--- /dev/null
>>>+++ b/Documentation/netlink/specs/dpll.yaml
>>>@@ -0,0 +1,514 @@
>>>+name: dpll
>>>+
>>>+doc: DPLL subsystem.
>>>+
>>>+definitions:
>>>+  -
>>>+    type: const
>>>+    name: temp-divider
>>>+    value: 10
>>>+  -
>>>+    type: const
>>>+    name: pin-freq-1-hz
>>>+    value: 1
>>>+  -
>>>+    type: const
>>>+    name: pin-freq-10-mhz
>>>+    value: 10000000
>>>+  -
>>>+    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 (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
>>>+    render-max: true
>>>+  -
>>>+    type: enum
>>>+    name: pin-type
>>>+    doc: Enumerates types of a pin, valid values for DPLL_A_PIN_TYPE
>>>attribute
>>>+    entries:
>>>+      -
>>>+        name: unspec
>>>+        doc: unspecified value
>>>+      -
>>>+        name: mux
>>>+        doc: aggregates another layer of selectable pins
>>>+      -
>>>+        name: ext
>>>+        doc: external source
>>>+      -
>>>+        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-state
>>>+    doc: available pin modes
>>>+    entries:
>>>+      -
>>>+        name: unspec
>>>+        doc: unspecified value
>>>+      -
>>>+        name: connected
>>>+        doc: pin connected
>>>+      -
>>>+        name: disconnected
>>>+        doc: pin disconnected
>>>+    render-max: true
>>>+  -
>>>+    type: enum
>>>+    name: pin-direction
>>>+    doc: available pin direction
>>>+    entries:
>>>+      -
>>>+        name: unspec
>>>+        doc: unspecified value
>>>+      -
>>>+        name: source
>>>+        doc: pin used as a source of a signal
>>>+      -
>>>+        name: output
>>>+        doc: pin used to output the signal
>>>+    render-max: true
>>>+  -
>>>+    type: enum
>>>+    name: mode
>>
>>Could you please sort the enums so they are in the same order as the
>>attribute which carries them? That would also put the device and pin
>>enums together.
>>
>
>Fixed.
>
>>
>>>+    doc: |
>>>+      working-modes a dpll can support, differentiate if and how dpll
>>>selects
>>>+      one of its sources to syntonize with it
>>>+    entries:
>>>+      -
>>>+        name: unspec
>>>+        doc: unspecified value
>>>+      -
>>>+        name: manual
>>>+        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
>>>+      -
>>>+        name: nco
>>>+        doc: dpll driven by Numerically Controlled Oscillator
>>>+    render-max: true
>>>+  -
>>>+    type: enum
>>>+    name: type
>>>+    doc: type of dpll, valid values for DPLL_A_TYPE attribute
>>>+    entries:
>>>+      -
>>>+        name: unspec
>>>+        doc: unspecified value
>>>+      -
>>>+        name: pps
>>>+        doc: dpll produces Pulse-Per-Second signal
>>>+      -
>>>+        name: eec
>>>+        doc: dpll drives the Ethernet Equipment Clock
>>>+    render-max: true
>>>+  -
>>>+    type: enum
>>>+    name: event
>>>+    doc: events of dpll generic netlink family
>>>+    entries:
>>>+      -
>>>+        name: unspec
>>>+        doc: invalid event type
>>>+      -
>>>+        name: device-create
>>>+        doc: dpll device created
>>>+      -
>>>+        name: device-delete
>>>+        doc: dpll device deleted
>>>+      -
>>>+        name: device-change
>>>+        doc: |
>>>+          attribute of dpll device or pin changed, reason is to be found
>>>with
>>>+          an attribute type (DPLL_A_*) received with the event
>>>+  -
>>>+    type: flags
>>>+    name: pin-caps
>>>+    doc: define capabilities of a pin
>>>+    entries:
>>>+      -
>>>+        name: direction-can-change
>>>+      -
>>>+        name: priority-can-change
>>>+      -
>>>+        name: state-can-change
>>>+
>>>+
>>>+attribute-sets:
>>>+  -
>>>+    name: dpll
>>>+    enum-name: dplla
>>>+    attributes:
>>>+      -
>>>+        name: device
>>>+        type: nest
>>>+        value: 1
>>>+        multi-attr: true
>>>+        nested-attributes: device
>>
>>What is this "device" and what is it good for? Smells like some leftover
>>and with the nested scheme looks quite odd.
>>
>
>No, it is nested attribute type, used when multiple devices are returned with
>netlink:
>
>- dump of device-get command where all devices are returned, each one nested
>inside it:
>[{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id': 0},
>             {'bus-name': 'pci', 'dev-name': '0000:21:00.0_1', 'id': 1}]}]

Okay, why is it nested here? The is one netlink msg per dpll device
instance. Is this the real output of you made that up?

Device nest should not be there for DEVICE_GET, does not make sense.


>
>- do/dump of pin-get, in case of shared pins, each pin contains number of dpll
>handles it connects with:
>[{'pin': [{'device': [{'bus-name': 'pci',
>                       'dev-name': '0000:21:00.0_0',
>                       'id': 0,
>                       'pin-prio': 6,
>                       'pin-state': {'doc': 'pin connected',
>                                     'name': 'connected'}},
>                      {'bus-name': 'pci',
>                       'dev-name': '0000:21:00.0_1',
>                       'id': 1,
>                       'pin-prio': 8,
>                       'pin-state': {'doc': 'pin connected',
>                                     'name': 'connected'}}],

Okay, here I understand it contains device specific pin items. Makes
sense!


>           'pin-direction': {'doc': 'pin used as a source of a signal',
>                             'name': 'source'},
>           'pin-frequency': 1,
>           'pin-frequency-supported': [1, 10000000],
>           'pin-idx': 0,
>	...
>
>>
>>>+      -
>>>+        name: id
>>>+        type: u32
>>>+      -
>>>+        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: source-pin-idx
>>>+        type: u32
>>>+      -
>>>+        name: lock-status
>>>+        type: u8
>>>+        enum: lock-status
>>>+      -
>>>+        name: temp
>>
>>Could you put some comment regarding the divider here?
>>
>
>Sure, fixed.
>
>>
>>>+        type: s32
>>>+      -
>>>+        name: clock-id
>>>+        type: u64
>>>+      -
>>>+        name: type
>>>+        type: u8
>>>+        enum: type
>>>+      -
>>>+        name: pin
>>>+        type: nest
>>>+        multi-attr: true
>>>+        nested-attributes: pin
>>>+        value: 12
>>>+      -
>>>+        name: pin-idx
>>>+        type: u32
>>>+      -
>>>+        name: pin-description
>>>+        type: string
>>>+      -
>>>+        name: pin-type
>>>+        type: u8
>>>+        enum: pin-type
>>>+      -
>>>+        name: pin-direction
>>>+        type: u8
>>>+        enum: pin-direction
>>>+      -
>>>+        name: pin-frequency
>>>+        type: u32
>>>+      -
>>>+        name: pin-frequency-supported
>>>+        type: u32
>>>+        multi-attr: true
>>>+      -
>>>+        name: pin-any-frequency-min
>>>+        type: u32
>>>+      -
>>>+        name: pin-any-frequency-max
>>>+        type: u32
>>
>>These 2 attrs somehow overlap with pin-frequency-supported,
>>which is a bit confusing, I think that pin-frequency-supported
>>should carry all supported frequencies. How about to have something
>>like:
>>        name: pin-frequency-supported
>>        type: nest
>>	multi-attr: true
>>        nested-attributes: pin-frequency-range
>>
>>and then:
>>      name: pin-frequency-range
>>      subset-of: dpll
>>      attributes:
>>        -
>>          name: pin-frequency-min
>>          type: u32
>>        -
>>          name: pin-frequency-max
>>          type: u32
>>          doc: should be put only to specify range when value differs
>>	       from pin-frequency-min
>>
>>That could carry list of frequencies and ranges, in this case multiple
>>ones if possibly needed.
>>
>
>Sure
>
>>
>>
>>>+      -
>>>+        name: pin-prio
>>>+        type: u32
>>>+      -
>>>+        name: pin-state
>>>+        type: u8
>>>+        enum: pin-state
>>>+      -
>>>+        name: pin-parent
>>>+        type: nest
>>>+        multi-attr: true
>>>+        nested-attributes: pin
>>>+        value: 23
>>
>>Value 23? What's this?
>>You have it specified for some attrs all over the place.
>>What is the reason for it?
>>
>
>Actually this particular one is not needed (also value: 12 on pin above),
>I will remove those.
>But the others you are refering to (the ones in nested attribute list),
>are required because of cli.py parser issue, maybe Kuba knows a better way to
>prevent the issue?
>Basically, without those values, cli.py brakes on parsing responses, after
>every "jump" to nested attribute list it is assigning first attribute there
>with value=0, thus there is a need to assign a proper value, same as it is on
>'main' attribute list.

That's weird. Looks like a bug then?


>
>>
>>>+      -
>>>+        name: pin-parent-idx
>>>+        type: u32
>>>+      -
>>>+        name: pin-rclk-device
>>>+        type: string
>>>+      -
>>>+        name: pin-dpll-caps
>>>+        type: u32
>>>+  -
>>>+    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: source-pin-idx
>>>+        type: u32
>>>+      -
>>>+        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
>>>+        type: nest
>>>+        value: 12
>>>+        multi-attr: true
>>>+        nested-attributes: pin
>>
>>This does not belong here.
>>
>
>What do you mean?
>With device-get 'do' request the list of pins connected to the dpll is
>returned, each pin is nested in this attribute.

No, wait a sec. You have 2 object types: device and pin. Each have
separate netlink CMDs to get and dump individual objects.
Don't mix those together like this. I thought it became clear in the
past. :/


>This is required by parser to work.
>
>>
>>>+      -
>>>+        name: pin-prio
>>>+        type: u32
>>>+        value: 21
>>>+      -
>>>+        name: pin-state
>>>+        type: u8
>>>+        enum: pin-state
>>>+      -
>>>+        name: pin-dpll-caps
>>>+        type: u32
>>>+        value: 26
>>
>>All these 3 do not belong here are well.
>>
>
>Same as above explanation.

Same as above reply.


>
>>
>>
>>>+  -
>>>+    name: pin
>>>+    subset-of: dpll
>>>+    attributes:
>>>+      -
>>>+        name: device
>>>+        type: nest
>>>+        value: 1
>>>+        multi-attr: true
>>>+        nested-attributes: device
>>>+      -
>>>+        name: pin-idx
>>>+        type: u32
>>>+        value: 13
>>>+      -
>>>+        name: pin-description
>>>+        type: string
>>>+      -
>>>+        name: pin-type
>>>+        type: u8
>>>+        enum: pin-type
>>>+      -
>>>+        name: pin-direction
>>>+        type: u8
>>>+        enum: pin-direction
>>>+      -
>>>+        name: pin-frequency
>>>+        type: u32
>>>+      -
>>>+        name: pin-frequency-supported
>>>+        type: u32
>>>+        multi-attr: true
>>>+      -
>>>+        name: pin-any-frequency-min
>>>+        type: u32
>>>+      -
>>>+        name: pin-any-frequency-max
>>>+        type: u32
>>>+      -
>>>+        name: pin-prio
>>>+        type: u32
>>>+      -
>>>+        name: pin-state
>>>+        type: u8
>>>+        enum: pin-state
>>>+      -
>>>+        name: pin-parent
>>>+        type: nest
>>>+        multi-attr: true
>>
>>Multiple parents? How is that supposed to work?
>>
>
>As we have agreed, MUXed pins can have multiple parents.
>In our case:
>/tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do pin-get --json '{"id": 0, "pin-idx":13}'
>{'pin': [{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id': 0},
>                     {'bus-name': 'pci',
>                      'dev-name': '0000:21:00.0_1',
>                      'id': 1}],
>          'pin-description': '0000:21:00.0',
>          'pin-direction': {'doc': 'pin used as a source of a signal',
>                            'name': 'source'},
>          'pin-idx': 13,
>          'pin-parent': [{'pin-parent-idx': 2,
>                          'pin-state': {'doc': 'pin disconnected',
>                                        'name': 'disconnected'}},
>                         {'pin-parent-idx': 3,
>                          'pin-state': {'doc': 'pin disconnected',
>                                        'name': 'disconnected'}}],
>          'pin-rclk-device': '0000:21:00.0',
>          'pin-type': {'doc': "ethernet port PHY's recovered clock",
>                       'name': 'synce-eth-port'}}]}

Got it, it is still a bit hard to me to follow this. Could you
perhaps extend the Documentation to describe in more details
with examples? Would help a lot for slower people like me to understand
what's what.


>
>
>>
>>>+        nested-attributes: pin-parent
>>>+        value: 23
>>>+      -
>>>+        name: pin-rclk-device
>>>+        type: string
>>>+        value: 25
>>>+      -
>>>+        name: pin-dpll-caps
>>>+        type: u32
>>
>>Missing "enum: "
>>
>
>It is actually a bitmask, this is why didn't set as enum, with enum type
>parser won't parse it.

Ah! Got it. Perhaps a docs note with the enum pointer then?


>
>>
>>>+  -
>>>+    name: pin-parent
>>>+    subset-of: dpll
>>>+    attributes:
>>>+      -
>>>+        name: pin-state
>>>+        type: u8
>>>+        value: 22
>>>+        enum: pin-state
>>>+      -
>>>+        name: pin-parent-idx
>>>+        type: u32
>>>+        value: 24
>>>+      -
>>>+        name: pin-rclk-device
>>>+        type: string
>>
>>Yeah, as I wrote in the other email, this really smells to
>>have like a simple string like this. What is it supposed to be?
>>
>
>Yes, let's discuss there.

Yep.

>
>>
>>>+
>>>+
>>>+operations:
>>>+  list:
>>>+    -
>>>+      name: unspec
>>>+      doc: unused
>>>+
>>>+    -
>>>+      name: device-get
>>>+      doc: |
>>>+        Get list of DPLL devices (dump) or attributes of a single dpll
>>device
>>>+      attribute-set: dpll
>>
>>Shouldn't this be "device"?
>>
>
>It would brake the parser, again I hope Jakub Kicinski could take a look on
>this.

Odd.

>
>>
>>>+      flags: [ admin-perm ]
>>>+
>>>+      do:
>>>+        pre: dpll-pre-doit
>>>+        post: dpll-post-doit
>>>+        request:
>>>+          attributes:
>>>+            - id
>>>+            - bus-name
>>>+            - dev-name
>>>+        reply:
>>>+          attributes:
>>>+            - device
>>>+
>>>+      dump:
>>>+        pre: dpll-pre-dumpit
>>>+        post: dpll-post-dumpit
>>>+        reply:
>>>+          attributes:
>>>+            - device
>>>+
>>>+    -
>>>+      name: device-set
>>>+      doc: Set attributes for a DPLL device
>>>+      attribute-set: dpll
>>
>>"device" here as well?
>>
>
>Same as above.
>
>>
>>>+      flags: [ admin-perm ]
>>>+
>>>+      do:
>>>+        pre: dpll-pre-doit
>>>+        post: dpll-post-doit
>>>+        request:
>>>+          attributes:
>>>+            - id
>>>+            - bus-name
>>>+            - dev-name
>>>+            - mode
>>
>>Hmm, shouldn't source-pin-index be here as well?
>
>No, there is no set for this.
>For manual mode user selects the pin by setting enabled state on the one
>he needs to recover signal from.
>
>source-pin-index is read only, returns active source.

Okay, got it. Then why do we have this assymetric approach? Just have
the enabled state to serve the user to see which one is selected, no?
This would help to avoid confusion (like mine) and allow not to create
inconsistencies (like no pin enabled yet driver to return some source
pin index)


>
>>
>>>+
>>>+    -
>>>+      name: pin-get
>>>+      doc: |
>>>+        Get list of pins and its attributes.
>>>+        - dump request without any attributes given - list all the pins
>>in the system
>>>+        - dump request with target dpll - list all the pins registered
>>with a given dpll device
>>>+        - do request with target dpll and target pin - single pin
>>attributes
>>>+      attribute-set: dpll
>>
>>"pin"?
>>
>
>Same case as with dpll/device above.
>
>>
>>>+      flags: [ admin-perm ]
>>>+
>>>+      do:
>>>+        pre: dpll-pin-pre-doit
>>>+        post: dpll-pin-post-doit
>>>+        request:
>>>+          attributes:
>>>+            - id
>>>+            - bus-name
>>>+            - dev-name
>>>+            - pin-idx
>>>+        reply:
>>>+          attributes:
>>>+            - pin
>>>+
>>>+      dump:
>>>+        pre: dpll-pin-pre-dumpit
>>>+        post: dpll-pin-post-dumpit
>>>+        request:
>>>+          attributes:
>>>+            - id
>>>+            - bus-name
>>>+            - dev-name
>>>+        reply:
>>>+          attributes:
>>>+            - pin
>>>+
>>>+    -
>>>+      name: pin-set
>>>+      doc: Set attributes of a target pin
>>>+      attribute-set: dpll
>>
>>"pin"?
>>
>
>Same case as with dpll/device above.
>
>>
>>>+      flags: [ admin-perm ]
>>>+
>>>+      do:
>>>+        pre: dpll-pin-pre-doit
>>>+        post: dpll-pin-post-doit
>>>+        request:
>>>+          attributes:
>>>+            - id
>>>+            - bus-name
>>>+            - dev-name
>>>+            - pin-idx
>>>+            - pin-frequency
>>>+            - pin-direction
>>>+            - pin-prio
>>>+            - pin-parent-idx
>>>+            - pin-state
>>>+
>>>+mcast-groups:
>>>+  list:
>>>+    -
>>>+      name: monitor
>>>diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c
>>>new file mode 100644
>>>index 000000000000..099d1e30ca7c
>>>--- /dev/null
>>>+++ b/drivers/dpll/dpll_nl.c
>>>@@ -0,0 +1,126 @@
>>>+// SPDX-License-Identifier: BSD-3-Clause
>>>+/* Do not edit directly, auto-generated from: */
>>>+/*	Documentation/netlink/specs/dpll.yaml */
>>>+/* YNL-GEN kernel source */
>>>+
>>>+#include <net/netlink.h>
>>>+#include <net/genetlink.h>
>>>+
>>>+#include "dpll_nl.h"
>>>+
>>>+#include <linux/dpll.h>
>>>+
>>>+/* DPLL_CMD_DEVICE_GET - do */
>>>+static const struct nla_policy dpll_device_get_nl_policy[DPLL_A_BUS_NAME
>>>+ 1] = {
>>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>+};
>>>+
>>>+/* DPLL_CMD_DEVICE_SET - do */
>>>+static const struct nla_policy dpll_device_set_nl_policy[DPLL_A_MODE + 1]
>>>= {
>>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>+	[DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),
>>
>>Hmm, any idea why the generator does not put define name
>>here instead of "5"?
>>
>
>Not really, it probably needs a fix for this.

Yeah.


>
>>
>>>+};
>>>+
>>>+/* DPLL_CMD_PIN_GET - do */
>>>+static const struct nla_policy dpll_pin_get_do_nl_policy[DPLL_A_PIN_IDX +
>>>1] = {
>>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>+	[DPLL_A_PIN_IDX] = { .type = NLA_U32, },
>>>+};
>>>+
>>>+/* DPLL_CMD_PIN_GET - dump */
>>>+static const struct nla_policy
>>>dpll_pin_get_dump_nl_policy[DPLL_A_BUS_NAME + 1] = {
>>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>+};
>>>+
>>>+/* DPLL_CMD_PIN_SET - do */
>>>+static const struct nla_policy
>>>dpll_pin_set_nl_policy[DPLL_A_PIN_PARENT_IDX + 1] = {
>>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>+	[DPLL_A_PIN_IDX] = { .type = NLA_U32, },
>>>+	[DPLL_A_PIN_FREQUENCY] = { .type = NLA_U32, },
>>
>>4.3GHz would be the limit, isn't it future proof to make this rather u64?
>>
>
>Sure, fixed.
>
>>
>>>+	[DPLL_A_PIN_DIRECTION] = NLA_POLICY_MAX(NLA_U8, 2),
>>>+	[DPLL_A_PIN_PRIO] = { .type = NLA_U32, },
>>>+	[DPLL_A_PIN_PARENT_IDX] = { .type = NLA_U32, },
>>
>>This is a bit odd. The size is DPLL_A_PIN_PARENT_IDX + 1, yet PIN_STATE
>>is last. I found that the order is not according to the enum in the yaml
>>operation definition. Perhaps the generator can sort this?
>>
>
>Fixed. This related to the order on list of attributes expected on ops
>definition.

Yep.


>
>>
>>>+	[DPLL_A_PIN_STATE] = NLA_POLICY_MAX(NLA_U8, 2),
>>>+};
>>>+
>>>+/* Ops table for dpll */
>>>+static const struct genl_split_ops dpll_nl_ops[6] = {
>>>+	{
>>>+		.cmd		= DPLL_CMD_DEVICE_GET,
>>>+		.pre_doit	= dpll_pre_doit,
>>>+		.doit		= dpll_nl_device_get_doit,
>>>+		.post_doit	= dpll_post_doit,
>>>+		.policy		= dpll_device_get_nl_policy,
>>>+		.maxattr	= DPLL_A_BUS_NAME,
>>>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>>+	},
>>>+	{
>>>+		.cmd	= DPLL_CMD_DEVICE_GET,
>>>+		.start	= dpll_pre_dumpit,
>>>+		.dumpit	= dpll_nl_device_get_dumpit,
>>>+		.done	= dpll_post_dumpit,
>>>+		.flags	= GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
>>>+	},
>>>+	{
>>>+		.cmd		= DPLL_CMD_DEVICE_SET,
>>>+		.pre_doit	= dpll_pre_doit,
>>>+		.doit		= dpll_nl_device_set_doit,
>>>+		.post_doit	= dpll_post_doit,
>>>+		.policy		= dpll_device_set_nl_policy,
>>>+		.maxattr	= DPLL_A_MODE,
>>>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>>+	},
>>>+	{
>>>+		.cmd		= DPLL_CMD_PIN_GET,
>>>+		.pre_doit	= dpll_pin_pre_doit,
>>>+		.doit		= dpll_nl_pin_get_doit,
>>>+		.post_doit	= dpll_pin_post_doit,
>>>+		.policy		= dpll_pin_get_do_nl_policy,
>>>+		.maxattr	= DPLL_A_PIN_IDX,
>>>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>>+	},
>>>+	{
>>>+		.cmd		= DPLL_CMD_PIN_GET,
>>>+		.start		= dpll_pin_pre_dumpit,
>>>+		.dumpit		= dpll_nl_pin_get_dumpit,
>>>+		.done		= dpll_pin_post_dumpit,
>>>+		.policy		= dpll_pin_get_dump_nl_policy,
>>>+		.maxattr	= DPLL_A_BUS_NAME,
>>>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
>>>+	},
>>>+	{
>>>+		.cmd		= DPLL_CMD_PIN_SET,
>>>+		.pre_doit	= dpll_pin_pre_doit,
>>>+		.doit		= dpll_nl_pin_set_doit,
>>>+		.post_doit	= dpll_pin_post_doit,
>>>+		.policy		= dpll_pin_set_nl_policy,
>>>+		.maxattr	= DPLL_A_PIN_PARENT_IDX,
>>>+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>>+	},
>>>+};
>>>+
>>>+static const struct genl_multicast_group dpll_nl_mcgrps[] = {
>>>+	[DPLL_NLGRP_MONITOR] = { "monitor", },
>>>+};
>>>+
>>>+struct genl_family dpll_nl_family __ro_after_init = {
>>>+	.name		= DPLL_FAMILY_NAME,
>>>+	.version	= DPLL_FAMILY_VERSION,
>>>+	.netnsok	= true,
>>>+	.parallel_ops	= true,
>>>+	.module		= THIS_MODULE,
>>>+	.split_ops	= dpll_nl_ops,
>>>+	.n_split_ops	= ARRAY_SIZE(dpll_nl_ops),
>>>+	.mcgrps		= dpll_nl_mcgrps,
>>>+	.n_mcgrps	= ARRAY_SIZE(dpll_nl_mcgrps),
>>>+};
>>>diff --git a/drivers/dpll/dpll_nl.h b/drivers/dpll/dpll_nl.h
>>>new file mode 100644
>>>index 000000000000..3a354dfb9a31
>>>--- /dev/null
>>>+++ b/drivers/dpll/dpll_nl.h
>>>@@ -0,0 +1,42 @@
>>>+/* SPDX-License-Identifier: BSD-3-Clause */
>>>+/* Do not edit directly, auto-generated from: */
>>>+/*	Documentation/netlink/specs/dpll.yaml */
>>>+/* YNL-GEN kernel header */
>>>+
>>>+#ifndef _LINUX_DPLL_GEN_H
>>>+#define _LINUX_DPLL_GEN_H
>>>+
>>>+#include <net/netlink.h>
>>>+#include <net/genetlink.h>
>>>+
>>>+#include <linux/dpll.h>
>>>+
>>>+int dpll_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>>>+		  struct genl_info *info);
>>>+int dpll_pin_pre_doit(const struct genl_split_ops *ops, struct sk_buff
>>*skb,
>>>+		      struct genl_info *info);
>>>+void
>>>+dpll_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>>>+	       struct genl_info *info);
>>>+void
>>>+dpll_pin_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
>>>+		   struct genl_info *info);
>>>+int dpll_pre_dumpit(struct netlink_callback *cb);
>>>+int dpll_pin_pre_dumpit(struct netlink_callback *cb);
>>>+int dpll_post_dumpit(struct netlink_callback *cb);
>>>+int dpll_pin_post_dumpit(struct netlink_callback *cb);
>>>+
>>>+int dpll_nl_device_get_doit(struct sk_buff *skb, struct genl_info *info);
>>>+int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct
>>netlink_callback *cb);
>>>+int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info);
>>>+int dpll_nl_pin_get_doit(struct sk_buff *skb, struct genl_info *info);
>>>+int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback
>>*cb);
>>>+int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info);
>>>+
>>>+enum {
>>>+	DPLL_NLGRP_MONITOR,
>>>+};
>>>+
>>>+extern struct genl_family dpll_nl_family;
>>>+
>>>+#endif /* _LINUX_DPLL_GEN_H */
>>>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>>>new file mode 100644
>>>index 000000000000..ece6fe685d08
>>>--- /dev/null
>>>+++ b/include/uapi/linux/dpll.h
>>>@@ -0,0 +1,196 @@
>>>+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>>+/* Do not edit directly, auto-generated from: */
>>>+/*	Documentation/netlink/specs/dpll.yaml */
>>>+/* YNL-GEN uapi header */
>>>+
>>>+#ifndef _UAPI_LINUX_DPLL_H
>>>+#define _UAPI_LINUX_DPLL_H
>>>+
>>>+#define DPLL_FAMILY_NAME	"dpll"
>>>+#define DPLL_FAMILY_VERSION	1
>>>+
>>>+#define DPLL_TEMP_DIVIDER	10
>>
>>Some comment to this define would be nice.
>>
>
>I added to the spec, but it is not generated by the regen script.
>
>>
>>>+#define DPLL_PIN_FREQ_1_HZ	1
>>>+#define DPLL_PIN_FREQ_10_MHZ	10000000
>>
>>Have this as enum. Spell out "frequency" to be consistent with the
>>related attribute name.
>>
>
>Sure, fixed.
>
>>
>>>+
>>>+/**
>>>+ * enum dpll_lock_status - Provides information of dpll device lock
>>>status,
>>>+ *   valid values for DPLL_A_LOCK_STATUS attribute
>>>+ * @DPLL_LOCK_STATUS_UNSPEC: unspecified value
>>>+ * @DPLL_LOCK_STATUS_UNLOCKED: dpll was not yet locked to any valid (or
>>is in
>>
>>"any valid source" perhaps?
>>
>
>Yes, fixed.
>
>>
>>>+ *   one of modes: DPLL_MODE_FREERUN, DPLL_MODE_NCO)
>>>+ * @DPLL_LOCK_STATUS_CALIBRATING: dpll is trying to lock to a valid
>>>signal
>>>+ * @DPLL_LOCK_STATUS_LOCKED: dpll is locked
>>>+ * @DPLL_LOCK_STATUS_HOLDOVER: dpll is in holdover state - lost a valid
>>>lock or
>>>+ *   was forced by selecting DPLL_MODE_HOLDOVER mode
>>>+ */
>>>+enum dpll_lock_status {
>>>+	DPLL_LOCK_STATUS_UNSPEC,
>>>+	DPLL_LOCK_STATUS_UNLOCKED,
>>>+	DPLL_LOCK_STATUS_CALIBRATING,
>>>+	DPLL_LOCK_STATUS_LOCKED,
>>>+	DPLL_LOCK_STATUS_HOLDOVER,
>>>+
>>>+	__DPLL_LOCK_STATUS_MAX,
>>>+	DPLL_LOCK_STATUS_MAX = (__DPLL_LOCK_STATUS_MAX - 1)
>>>+};
>>>+
>>>+/**
>>>+ * enum dpll_pin_type - Enumerates types of a pin, valid values for
>>>+ *   DPLL_A_PIN_TYPE attribute
>>>+ * @DPLL_PIN_TYPE_UNSPEC: unspecified value
>>>+ * @DPLL_PIN_TYPE_MUX: aggregates another layer of selectable pins
>>>+ * @DPLL_PIN_TYPE_EXT: external source
>>>+ * @DPLL_PIN_TYPE_SYNCE_ETH_PORT: ethernet port PHY's recovered clock
>>>+ * @DPLL_PIN_TYPE_INT_OSCILLATOR: device internal oscillator
>>>+ * @DPLL_PIN_TYPE_GNSS: GNSS recovered clock
>>>+ */
>>>+enum dpll_pin_type {
>>>+	DPLL_PIN_TYPE_UNSPEC,
>>>+	DPLL_PIN_TYPE_MUX,
>>>+	DPLL_PIN_TYPE_EXT,
>>>+	DPLL_PIN_TYPE_SYNCE_ETH_PORT,
>>>+	DPLL_PIN_TYPE_INT_OSCILLATOR,
>>>+	DPLL_PIN_TYPE_GNSS,
>>>+
>>>+	__DPLL_PIN_TYPE_MAX,
>>>+	DPLL_PIN_TYPE_MAX = (__DPLL_PIN_TYPE_MAX - 1)
>>>+};
>>>+
>>>+/**
>>>+ * enum dpll_pin_state - available pin modes
>>
>>For some of the enums, you say for what attribute it serves as a value
>>set, for some you don't. Please unify the approach. I think it is
>>valuable to say that for every enum.
>>
>
>Sure, fixed.
>
>>
>>>+ * @DPLL_PIN_STATE_UNSPEC: unspecified value
>>>+ * @DPLL_PIN_STATE_CONNECTED: pin connected
>>>+ * @DPLL_PIN_STATE_DISCONNECTED: pin disconnected
>>>+ */
>>>+enum dpll_pin_state {
>>>+	DPLL_PIN_STATE_UNSPEC,
>>>+	DPLL_PIN_STATE_CONNECTED,
>>>+	DPLL_PIN_STATE_DISCONNECTED,
>>>+
>>>+	__DPLL_PIN_STATE_MAX,
>>>+	DPLL_PIN_STATE_MAX = (__DPLL_PIN_STATE_MAX - 1)
>>>+};
>>>+
>>>+/**
>>>+ * enum dpll_pin_direction - available pin direction
>>>+ * @DPLL_PIN_DIRECTION_UNSPEC: unspecified value
>>>+ * @DPLL_PIN_DIRECTION_SOURCE: pin used as a source of a signal
>>>+ * @DPLL_PIN_DIRECTION_OUTPUT: pin used to output the signal
>>>+ */
>>>+enum dpll_pin_direction {
>>>+	DPLL_PIN_DIRECTION_UNSPEC,
>>>+	DPLL_PIN_DIRECTION_SOURCE,
>>>+	DPLL_PIN_DIRECTION_OUTPUT,
>>>+
>>>+	__DPLL_PIN_DIRECTION_MAX,
>>>+	DPLL_PIN_DIRECTION_MAX = (__DPLL_PIN_DIRECTION_MAX - 1)
>>>+};
>>>+
>>>+/**
>>>+ * enum dpll_mode - working-modes a dpll can support, differentiate if
>>>and how
>>>+ *   dpll selects one of its sources to syntonize with it
>>>+ * @DPLL_MODE_UNSPEC: unspecified value
>>>+ * @DPLL_MODE_MANUAL: source can be only selected by sending a request to
>>>dpll
>>>+ * @DPLL_MODE_AUTOMATIC: highest prio, valid source, auto selected by
>>>dpll
>>>+ * @DPLL_MODE_HOLDOVER: dpll forced into holdover mode
>>>+ * @DPLL_MODE_FREERUN: dpll driven on system clk, no holdover available
>>>+ * @DPLL_MODE_NCO: dpll driven by Numerically Controlled Oscillator
>>>+ */
>>>+enum dpll_mode {
>>>+	DPLL_MODE_UNSPEC,
>>>+	DPLL_MODE_MANUAL,
>>>+	DPLL_MODE_AUTOMATIC,
>>>+	DPLL_MODE_HOLDOVER,
>>>+	DPLL_MODE_FREERUN,
>>>+	DPLL_MODE_NCO,
>>>+
>>>+	__DPLL_MODE_MAX,
>>>+	DPLL_MODE_MAX = (__DPLL_MODE_MAX - 1)
>>>+};
>>>+
>>>+/**
>>>+ * enum dpll_type - type of dpll, valid values for DPLL_A_TYPE attribute
>>>+ * @DPLL_TYPE_UNSPEC: unspecified value
>>>+ * @DPLL_TYPE_PPS: dpll produces Pulse-Per-Second signal
>>>+ * @DPLL_TYPE_EEC: dpll drives the Ethernet Equipment Clock
>>>+ */
>>>+enum dpll_type {
>>>+	DPLL_TYPE_UNSPEC,
>>>+	DPLL_TYPE_PPS,
>>>+	DPLL_TYPE_EEC,
>>>+
>>>+	__DPLL_TYPE_MAX,
>>>+	DPLL_TYPE_MAX = (__DPLL_TYPE_MAX - 1)
>>>+};
>>>+
>>>+/**
>>>+ * enum dpll_event - events of dpll generic netlink family
>>>+ * @DPLL_EVENT_UNSPEC: invalid event type
>>>+ * @DPLL_EVENT_DEVICE_CREATE: dpll device created
>>>+ * @DPLL_EVENT_DEVICE_DELETE: dpll device deleted
>>>+ * @DPLL_EVENT_DEVICE_CHANGE: attribute of dpll device or pin changed,
>>>reason
>>>+ *   is to be found with an attribute type (DPLL_A_*) received with the
>>>event
>>>+ */
>>>+enum dpll_event {
>>>+	DPLL_EVENT_UNSPEC,
>>>+	DPLL_EVENT_DEVICE_CREATE,
>>>+	DPLL_EVENT_DEVICE_DELETE,
>>>+	DPLL_EVENT_DEVICE_CHANGE,
>>>+};
>>>+
>>>+/**
>>>+ * enum dpll_pin_caps - define capabilities of a pin
>>>+ */
>>>+enum dpll_pin_caps {
>>>+	DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE = 1,
>>>+	DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE = 2,
>>>+	DPLL_PIN_CAPS_STATE_CAN_CHANGE = 4,
>>>+};
>>>+
>>>+enum dplla {
>>>+	DPLL_A_DEVICE = 1,
>>>+	DPLL_A_ID,
>>>+	DPLL_A_DEV_NAME,
>>>+	DPLL_A_BUS_NAME,
>>>+	DPLL_A_MODE,
>>>+	DPLL_A_MODE_SUPPORTED,
>>>+	DPLL_A_SOURCE_PIN_IDX,
>>>+	DPLL_A_LOCK_STATUS,
>>>+	DPLL_A_TEMP,
>>>+	DPLL_A_CLOCK_ID,
>>>+	DPLL_A_TYPE,
>>>+	DPLL_A_PIN,
>>>+	DPLL_A_PIN_IDX,
>>>+	DPLL_A_PIN_DESCRIPTION,
>>
>>I commented this name in the other email. This does not describe
>>anything. It is a label on a connector, isn't it? Why don't to call it
>>"label"?
>>
>
>Sure, fixed.
>
>>
>>>+	DPLL_A_PIN_TYPE,
>>>+	DPLL_A_PIN_DIRECTION,
>>>+	DPLL_A_PIN_FREQUENCY,
>>>+	DPLL_A_PIN_FREQUENCY_SUPPORTED,
>>>+	DPLL_A_PIN_ANY_FREQUENCY_MIN,
>>>+	DPLL_A_PIN_ANY_FREQUENCY_MAX,
>>
>>Drop "any". Not good for anything.
>>
>
>Sure, fixed.
>
>>
>>>+	DPLL_A_PIN_PRIO,
>>>+	DPLL_A_PIN_STATE,
>>>+	DPLL_A_PIN_PARENT,
>>>+	DPLL_A_PIN_PARENT_IDX,
>>>+	DPLL_A_PIN_RCLK_DEVICE,
>>>+	DPLL_A_PIN_DPLL_CAPS,
>>
>>Just DPLL_A_PIN_CAPS is enough, that would be also consistent with the
>>enum name.
>
>Sure, fixed.


Thanks for all your work on this!

Jiri Pirko March 16, 2023, 3:19 p.m. UTC | #4

Thu, Mar 16, 2023 at 02:45:10PM CET, [e-mail] wrote:
>Thu, Mar 16, 2023 at 02:15:59PM CET, [e-mail] wrote:

[...]


>>>>+      flags: [ admin-perm ]
>>>>+
>>>>+      do:
>>>>+        pre: dpll-pre-doit
>>>>+        post: dpll-post-doit
>>>>+        request:
>>>>+          attributes:
>>>>+            - id
>>>>+            - bus-name
>>>>+            - dev-name
>>>>+            - mode
>>>
>>>Hmm, shouldn't source-pin-index be here as well?
>>
>>No, there is no set for this.
>>For manual mode user selects the pin by setting enabled state on the one
>>he needs to recover signal from.
>>
>>source-pin-index is read only, returns active source.
>
>Okay, got it. Then why do we have this assymetric approach? Just have
>the enabled state to serve the user to see which one is selected, no?
>This would help to avoid confusion (like mine) and allow not to create
>inconsistencies (like no pin enabled yet driver to return some source
>pin index)

Actually, for mlx5 implementation, would be non-trivial to implement
this, as each of the pin/port is instantiated and controlled by separate
pci backend.

Could you please remove, it is not needed and has potential and real
issues.

[...]

Arkadiusz Kubalewski March 17, 2023, 12:52 a.m. UTC | #5

>From: Jiri Pirko <[e-mail]>
>Sent: Thursday, March 16, 2023 2:45 PM
>

[...]

>>>>+attribute-sets:
>>>>+  -
>>>>+    name: dpll
>>>>+    enum-name: dplla
>>>>+    attributes:
>>>>+      -
>>>>+        name: device
>>>>+        type: nest
>>>>+        value: 1
>>>>+        multi-attr: true
>>>>+        nested-attributes: device
>>>
>>>What is this "device" and what is it good for? Smells like some leftover
>>>and with the nested scheme looks quite odd.
>>>
>>
>>No, it is nested attribute type, used when multiple devices are returned
>>with netlink:
>>
>>- dump of device-get command where all devices are returned, each one nested
>>inside it:
>>[{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id': 0},
>>             {'bus-name': 'pci', 'dev-name': '0000:21:00.0_1', 'id': 1}]}]
>
>Okay, why is it nested here? The is one netlink msg per dpll device
>instance. Is this the real output of you made that up?
>
>Device nest should not be there for DEVICE_GET, does not make sense.
>

This was returned by CLI parser on ice with cmd:
$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml /
--dump device-get

Please note this relates to 'dump' request , it is rather expected that there
are multiple dplls returned, thus we need a nest attribute for each one.

>
>>
>>- do/dump of pin-get, in case of shared pins, each pin contains number of
>dpll
>>handles it connects with:
>>[{'pin': [{'device': [{'bus-name': 'pci',
>>                       'dev-name': '0000:21:00.0_0',
>>                       'id': 0,
>>                       'pin-prio': 6,
>>                       'pin-state': {'doc': 'pin connected',
>>                                     'name': 'connected'}},
>>                      {'bus-name': 'pci',
>>                       'dev-name': '0000:21:00.0_1',
>>                       'id': 1,
>>                       'pin-prio': 8,
>>                       'pin-state': {'doc': 'pin connected',
>>                                     'name': 'connected'}}],
>
>Okay, here I understand it contains device specific pin items. Makes
>sense!
>

Good.

[...]

>>
>>>
>>>
>>>>+      -
>>>>+        name: pin-prio
>>>>+        type: u32
>>>>+      -
>>>>+        name: pin-state
>>>>+        type: u8
>>>>+        enum: pin-state
>>>>+      -
>>>>+        name: pin-parent
>>>>+        type: nest
>>>>+        multi-attr: true
>>>>+        nested-attributes: pin
>>>>+        value: 23
>>>
>>>Value 23? What's this?
>>>You have it specified for some attrs all over the place.
>>>What is the reason for it?
>>>
>>
>>Actually this particular one is not needed (also value: 12 on pin above),
>>I will remove those.
>>But the others you are refering to (the ones in nested attribute list),
>>are required because of cli.py parser issue, maybe Kuba knows a better way
>to
>>prevent the issue?
>>Basically, without those values, cli.py brakes on parsing responses, after
>>every "jump" to nested attribute list it is assigning first attribute
>there
>>with value=0, thus there is a need to assign a proper value, same as it is
>on
>>'main' attribute list.
>
>That's weird. Looks like a bug then?
>

Guess we could call it a bug, I haven't investigated the parser that much,
AFAIR, other specs are doing the same way.

>
>>
>>>
>>>>+      -
>>>>+        name: pin-parent-idx
>>>>+        type: u32
>>>>+      -
>>>>+        name: pin-rclk-device
>>>>+        type: string
>>>>+      -
>>>>+        name: pin-dpll-caps
>>>>+        type: u32
>>>>+  -
>>>>+    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: source-pin-idx
>>>>+        type: u32
>>>>+      -
>>>>+        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
>>>>+        type: nest
>>>>+        value: 12
>>>>+        multi-attr: true
>>>>+        nested-attributes: pin
>>>
>>>This does not belong here.
>>>
>>
>>What do you mean?
>>With device-get 'do' request the list of pins connected to the dpll is
>>returned, each pin is nested in this attribute.
>
>No, wait a sec. You have 2 object types: device and pin. Each have
>separate netlink CMDs to get and dump individual objects.
>Don't mix those together like this. I thought it became clear in the
>past. :/
>

For pins we must, as pins without a handle to a dpll are pointless.
Same as a dpll without pins, right?

'do' of DEVICE_GET could just dump it's own status, without the list of pins,
but it feels easier for handling it's state on userspace counterpart if that
command also returns currently registered pins. Don't you think so?

>
>>This is required by parser to work.
>>
>>>
>>>>+      -
>>>>+        name: pin-prio
>>>>+        type: u32
>>>>+        value: 21
>>>>+      -
>>>>+        name: pin-state
>>>>+        type: u8
>>>>+        enum: pin-state
>>>>+      -
>>>>+        name: pin-dpll-caps
>>>>+        type: u32
>>>>+        value: 26
>>>
>>>All these 3 do not belong here are well.
>>>
>>
>>Same as above explanation.
>
>Same as above reply.
>
>
>>
>>>
>>>
>>>>+  -
>>>>+    name: pin
>>>>+    subset-of: dpll
>>>>+    attributes:
>>>>+      -
>>>>+        name: device
>>>>+        type: nest
>>>>+        value: 1
>>>>+        multi-attr: true
>>>>+        nested-attributes: device
>>>>+      -
>>>>+        name: pin-idx
>>>>+        type: u32
>>>>+        value: 13
>>>>+      -
>>>>+        name: pin-description
>>>>+        type: string
>>>>+      -
>>>>+        name: pin-type
>>>>+        type: u8
>>>>+        enum: pin-type
>>>>+      -
>>>>+        name: pin-direction
>>>>+        type: u8
>>>>+        enum: pin-direction
>>>>+      -
>>>>+        name: pin-frequency
>>>>+        type: u32
>>>>+      -
>>>>+        name: pin-frequency-supported
>>>>+        type: u32
>>>>+        multi-attr: true
>>>>+      -
>>>>+        name: pin-any-frequency-min
>>>>+        type: u32
>>>>+      -
>>>>+        name: pin-any-frequency-max
>>>>+        type: u32
>>>>+      -
>>>>+        name: pin-prio
>>>>+        type: u32
>>>>+      -
>>>>+        name: pin-state
>>>>+        type: u8
>>>>+        enum: pin-state
>>>>+      -
>>>>+        name: pin-parent
>>>>+        type: nest
>>>>+        multi-attr: true
>>>
>>>Multiple parents? How is that supposed to work?
>>>
>>
>>As we have agreed, MUXed pins can have multiple parents.
>>In our case:
>>/tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do
>>pin-get --json '{"id": 0, "pin-idx":13}'
>>{'pin': [{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0',
>>'id': 0},
>>                     {'bus-name': 'pci',
>>                      'dev-name': '0000:21:00.0_1',
>>                      'id': 1}],
>>          'pin-description': '0000:21:00.0',
>>          'pin-direction': {'doc': 'pin used as a source of a signal',
>>                            'name': 'source'},
>>          'pin-idx': 13,
>>          'pin-parent': [{'pin-parent-idx': 2,
>>                          'pin-state': {'doc': 'pin disconnected',
>>                                        'name': 'disconnected'}},
>>                         {'pin-parent-idx': 3,
>>                          'pin-state': {'doc': 'pin disconnected',
>>                                        'name': 'disconnected'}}],
>>          'pin-rclk-device': '0000:21:00.0',
>>          'pin-type': {'doc': "ethernet port PHY's recovered clock",
>>                       'name': 'synce-eth-port'}}]}
>
>Got it, it is still a bit hard to me to follow this. Could you
>perhaps extend the Documentation to describe in more details
>with examples? Would help a lot for slower people like me to understand
>what's what.
>

Actually this is already explained in "MUX-type pins" paragraph of
Documentation/networking/dpll.rst.
Do we want to duplicate this explanation here?


>
>>
>>
>>>
>>>>+        nested-attributes: pin-parent
>>>>+        value: 23
>>>>+      -
>>>>+        name: pin-rclk-device
>>>>+        type: string
>>>>+        value: 25
>>>>+      -
>>>>+        name: pin-dpll-caps
>>>>+        type: u32
>>>
>>>Missing "enum: "
>>>
>>
>>It is actually a bitmask, this is why didn't set as enum, with enum type
>>parser won't parse it.
>
>Ah! Got it. Perhaps a docs note with the enum pointer then?
>

Same as above, explained in Documentation/networking/dpll.rst, do wan't to
duplicate?

>
>>
>>>
>>>>+  -
>>>>+    name: pin-parent
>>>>+    subset-of: dpll
>>>>+    attributes:
>>>>+      -
>>>>+        name: pin-state
>>>>+        type: u8
>>>>+        value: 22
>>>>+        enum: pin-state
>>>>+      -
>>>>+        name: pin-parent-idx
>>>>+        type: u32
>>>>+        value: 24
>>>>+      -
>>>>+        name: pin-rclk-device
>>>>+        type: string
>>>
>>>Yeah, as I wrote in the other email, this really smells to
>>>have like a simple string like this. What is it supposed to be?
>>>
>>
>>Yes, let's discuss there.
>
>Yep.
>
>>
>>>
>>>>+
>>>>+
>>>>+operations:
>>>>+  list:
>>>>+    -
>>>>+      name: unspec
>>>>+      doc: unused
>>>>+
>>>>+    -
>>>>+      name: device-get
>>>>+      doc: |
>>>>+        Get list of DPLL devices (dump) or attributes of a single dpll
>>>device
>>>>+      attribute-set: dpll
>>>
>>>Shouldn't this be "device"?
>>>
>>
>>It would brake the parser, again I hope Jakub Kicinski could take a look
>>on this.
>
>Odd.
>

Yes, seems a bit odd.

>>
>>>
>>>>+      flags: [ admin-perm ]
>>>>+
>>>>+      do:
>>>>+        pre: dpll-pre-doit
>>>>+        post: dpll-post-doit
>>>>+        request:
>>>>+          attributes:
>>>>+            - id
>>>>+            - bus-name
>>>>+            - dev-name
>>>>+        reply:
>>>>+          attributes:
>>>>+            - device
>>>>+
>>>>+      dump:
>>>>+        pre: dpll-pre-dumpit
>>>>+        post: dpll-post-dumpit
>>>>+        reply:
>>>>+          attributes:
>>>>+            - device
>>>>+
>>>>+    -
>>>>+      name: device-set
>>>>+      doc: Set attributes for a DPLL device
>>>>+      attribute-set: dpll
>>>
>>>"device" here as well?
>>>
>>
>>Same as above.
>>
>>>
>>>>+      flags: [ admin-perm ]
>>>>+
>>>>+      do:
>>>>+        pre: dpll-pre-doit
>>>>+        post: dpll-post-doit
>>>>+        request:
>>>>+          attributes:
>>>>+            - id
>>>>+            - bus-name
>>>>+            - dev-name
>>>>+            - mode
>>>
>>>Hmm, shouldn't source-pin-index be here as well?
>>
>>No, there is no set for this.
>>For manual mode user selects the pin by setting enabled state on the one
>>he needs to recover signal from.
>>
>>source-pin-index is read only, returns active source.
>
>Okay, got it. Then why do we have this assymetric approach? Just have
>the enabled state to serve the user to see which one is selected, no?
>This would help to avoid confusion (like mine) and allow not to create
>inconsistencies (like no pin enabled yet driver to return some source
>pin index)
>

This is due to automatic mode were multiple pins are enabled, but actual
selection is done on hardware level with priorities.

[...]

>>>>+
>>>>+/* DPLL_CMD_DEVICE_SET - do */
>>>>+static const struct nla_policy dpll_device_set_nl_policy[DPLL_A_MODE +
>>>>1]
>>>>= {
>>>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>>+	[DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),
>>>
>>>Hmm, any idea why the generator does not put define name
>>>here instead of "5"?
>>>
>>
>>Not really, it probably needs a fix for this.
>
>Yeah.
>

Well, once we done with review maybe we could also fix those, or ask
Jakub if he could help :)


[...]

>>>
>>>>+	DPLL_A_PIN_PRIO,
>>>>+	DPLL_A_PIN_STATE,
>>>>+	DPLL_A_PIN_PARENT,
>>>>+	DPLL_A_PIN_PARENT_IDX,
>>>>+	DPLL_A_PIN_RCLK_DEVICE,
>>>>+	DPLL_A_PIN_DPLL_CAPS,
>>>
>>>Just DPLL_A_PIN_CAPS is enough, that would be also consistent with the
>>>enum name.
>>
>>Sure, fixed.
>
>
>Thanks for all your work on this!

Thanks for a great review! :)

Arkadiusz Kubalewski March 17, 2023, 12:53 a.m. UTC | #6

>From: Jiri Pirko <[e-mail]>
>Sent: Thursday, March 16, 2023 4:20 PM
>
>Thu, Mar 16, 2023 at 02:45:10PM CET, [e-mail] wrote:
>>Thu, Mar 16, 2023 at 02:15:59PM CET, [e-mail] wrote:
>
>[...]
>
>
>>>>>+      flags: [ admin-perm ]
>>>>>+
>>>>>+      do:
>>>>>+        pre: dpll-pre-doit
>>>>>+        post: dpll-post-doit
>>>>>+        request:
>>>>>+          attributes:
>>>>>+            - id
>>>>>+            - bus-name
>>>>>+            - dev-name
>>>>>+            - mode
>>>>
>>>>Hmm, shouldn't source-pin-index be here as well?
>>>
>>>No, there is no set for this.
>>>For manual mode user selects the pin by setting enabled state on the one
>>>he needs to recover signal from.
>>>
>>>source-pin-index is read only, returns active source.
>>
>>Okay, got it. Then why do we have this assymetric approach? Just have
>>the enabled state to serve the user to see which one is selected, no?
>>This would help to avoid confusion (like mine) and allow not to create
>>inconsistencies (like no pin enabled yet driver to return some source
>>pin index)
>
>Actually, for mlx5 implementation, would be non-trivial to implement
>this, as each of the pin/port is instantiated and controlled by separate
>pci backend.
>
>Could you please remove, it is not needed and has potential and real
>issues.
>
>[...]

Sorry I cannot, for priority based automatic selection mode multiple sources
are enabled at any time - selection is done automatically by the chip.
Thus for that case, this attribute is only way of getting an active source.
Although, maybe we could allow driver to not implement it, would this help
for your case? As it seems only required for automatic mode selection.

Thank you,
Arkadiusz

Jiri Pirko March 17, 2023, 10:05 a.m. UTC | #7

Fri, Mar 17, 2023 at 01:52:44AM CET, [e-mail] wrote:
>>From: Jiri Pirko <[e-mail]>
>>Sent: Thursday, March 16, 2023 2:45 PM
>>
>
>[...]
>
>>>>>+attribute-sets:
>>>>>+  -
>>>>>+    name: dpll
>>>>>+    enum-name: dplla
>>>>>+    attributes:
>>>>>+      -
>>>>>+        name: device
>>>>>+        type: nest
>>>>>+        value: 1
>>>>>+        multi-attr: true
>>>>>+        nested-attributes: device
>>>>
>>>>What is this "device" and what is it good for? Smells like some leftover
>>>>and with the nested scheme looks quite odd.
>>>>
>>>
>>>No, it is nested attribute type, used when multiple devices are returned
>>>with netlink:
>>>
>>>- dump of device-get command where all devices are returned, each one nested
>>>inside it:
>>>[{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id': 0},
>>>             {'bus-name': 'pci', 'dev-name': '0000:21:00.0_1', 'id': 1}]}]
>>
>>Okay, why is it nested here? The is one netlink msg per dpll device
>>instance. Is this the real output of you made that up?
>>
>>Device nest should not be there for DEVICE_GET, does not make sense.
>>
>
>This was returned by CLI parser on ice with cmd:
>$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml /
>--dump device-get
>
>Please note this relates to 'dump' request , it is rather expected that there
>are multiple dplls returned, thus we need a nest attribute for each one.

No, you definitelly don't need to nest them. Dump format and get format
should be exactly the same. Please remove the nest.

See how that is done in devlink for example: devlink_nl_fill()
This functions fills up one object in the dump. No nesting.
I'm not aware of such nesting approach anywhere in kernel dumps, does
not make sense at all.


>
>>
>>>
>>>- do/dump of pin-get, in case of shared pins, each pin contains number of
>>dpll
>>>handles it connects with:
>>>[{'pin': [{'device': [{'bus-name': 'pci',
>>>                       'dev-name': '0000:21:00.0_0',
>>>                       'id': 0,
>>>                       'pin-prio': 6,
>>>                       'pin-state': {'doc': 'pin connected',
>>>                                     'name': 'connected'}},
>>>                      {'bus-name': 'pci',
>>>                       'dev-name': '0000:21:00.0_1',
>>>                       'id': 1,
>>>                       'pin-prio': 8,
>>>                       'pin-state': {'doc': 'pin connected',
>>>                                     'name': 'connected'}}],
>>
>>Okay, here I understand it contains device specific pin items. Makes
>>sense!
>>
>
>Good.

Make sure you don't nest the pin objects for dump (DPLL_A_PIN). Same
reason as above.
I don't see a need for DPLL_A_PIN attr existence, please remove it.




>
>[...]
>
>>>
>>>>
>>>>
>>>>>+      -
>>>>>+        name: pin-prio
>>>>>+        type: u32
>>>>>+      -
>>>>>+        name: pin-state
>>>>>+        type: u8
>>>>>+        enum: pin-state
>>>>>+      -
>>>>>+        name: pin-parent
>>>>>+        type: nest
>>>>>+        multi-attr: true
>>>>>+        nested-attributes: pin
>>>>>+        value: 23
>>>>
>>>>Value 23? What's this?
>>>>You have it specified for some attrs all over the place.
>>>>What is the reason for it?
>>>>
>>>
>>>Actually this particular one is not needed (also value: 12 on pin above),
>>>I will remove those.
>>>But the others you are refering to (the ones in nested attribute list),
>>>are required because of cli.py parser issue, maybe Kuba knows a better way
>>to
>>>prevent the issue?
>>>Basically, without those values, cli.py brakes on parsing responses, after
>>>every "jump" to nested attribute list it is assigning first attribute
>>there
>>>with value=0, thus there is a need to assign a proper value, same as it is
>>on
>>>'main' attribute list.
>>
>>That's weird. Looks like a bug then?
>>
>
>Guess we could call it a bug, I haven't investigated the parser that much,
>AFAIR, other specs are doing the same way.
>
>>
>>>
>>>>
>>>>>+      -
>>>>>+        name: pin-parent-idx
>>>>>+        type: u32
>>>>>+      -
>>>>>+        name: pin-rclk-device
>>>>>+        type: string
>>>>>+      -
>>>>>+        name: pin-dpll-caps
>>>>>+        type: u32
>>>>>+  -
>>>>>+    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: source-pin-idx
>>>>>+        type: u32
>>>>>+      -
>>>>>+        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
>>>>>+        type: nest
>>>>>+        value: 12
>>>>>+        multi-attr: true
>>>>>+        nested-attributes: pin
>>>>
>>>>This does not belong here.
>>>>
>>>
>>>What do you mean?
>>>With device-get 'do' request the list of pins connected to the dpll is
>>>returned, each pin is nested in this attribute.
>>
>>No, wait a sec. You have 2 object types: device and pin. Each have
>>separate netlink CMDs to get and dump individual objects.
>>Don't mix those together like this. I thought it became clear in the
>>past. :/
>>
>
>For pins we must, as pins without a handle to a dpll are pointless.

I'm not talking about per device specific items for pins (state and
prio). That is something else, it's a pin-device tuple. Completely fine.



>Same as a dpll without pins, right?
>
>'do' of DEVICE_GET could just dump it's own status, without the list of pins,

Yes please.


>but it feels easier for handling it's state on userspace counterpart if that
>command also returns currently registered pins. Don't you think so?

No, definitelly not. Please make the object separation clear. Device and
pins are different objects, they have different commands to work with.
Don't mix them together.


>
>>
>>>This is required by parser to work.
>>>
>>>>
>>>>>+      -
>>>>>+        name: pin-prio
>>>>>+        type: u32
>>>>>+        value: 21
>>>>>+      -
>>>>>+        name: pin-state
>>>>>+        type: u8
>>>>>+        enum: pin-state
>>>>>+      -
>>>>>+        name: pin-dpll-caps
>>>>>+        type: u32
>>>>>+        value: 26
>>>>
>>>>All these 3 do not belong here are well.
>>>>
>>>
>>>Same as above explanation.
>>
>>Same as above reply.
>>
>>
>>>
>>>>
>>>>
>>>>>+  -
>>>>>+    name: pin
>>>>>+    subset-of: dpll
>>>>>+    attributes:
>>>>>+      -
>>>>>+        name: device
>>>>>+        type: nest
>>>>>+        value: 1
>>>>>+        multi-attr: true
>>>>>+        nested-attributes: device
>>>>>+      -
>>>>>+        name: pin-idx
>>>>>+        type: u32
>>>>>+        value: 13
>>>>>+      -
>>>>>+        name: pin-description
>>>>>+        type: string
>>>>>+      -
>>>>>+        name: pin-type
>>>>>+        type: u8
>>>>>+        enum: pin-type
>>>>>+      -
>>>>>+        name: pin-direction
>>>>>+        type: u8
>>>>>+        enum: pin-direction
>>>>>+      -
>>>>>+        name: pin-frequency
>>>>>+        type: u32
>>>>>+      -
>>>>>+        name: pin-frequency-supported
>>>>>+        type: u32
>>>>>+        multi-attr: true
>>>>>+      -
>>>>>+        name: pin-any-frequency-min
>>>>>+        type: u32
>>>>>+      -
>>>>>+        name: pin-any-frequency-max
>>>>>+        type: u32
>>>>>+      -
>>>>>+        name: pin-prio
>>>>>+        type: u32
>>>>>+      -
>>>>>+        name: pin-state
>>>>>+        type: u8
>>>>>+        enum: pin-state
>>>>>+      -
>>>>>+        name: pin-parent
>>>>>+        type: nest
>>>>>+        multi-attr: true
>>>>
>>>>Multiple parents? How is that supposed to work?
>>>>
>>>
>>>As we have agreed, MUXed pins can have multiple parents.
>>>In our case:
>>>/tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do
>>>pin-get --json '{"id": 0, "pin-idx":13}'
>>>{'pin': [{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0',
>>>'id': 0},
>>>                     {'bus-name': 'pci',
>>>                      'dev-name': '0000:21:00.0_1',
>>>                      'id': 1}],
>>>          'pin-description': '0000:21:00.0',
>>>          'pin-direction': {'doc': 'pin used as a source of a signal',
>>>                            'name': 'source'},
>>>          'pin-idx': 13,
>>>          'pin-parent': [{'pin-parent-idx': 2,
>>>                          'pin-state': {'doc': 'pin disconnected',
>>>                                        'name': 'disconnected'}},
>>>                         {'pin-parent-idx': 3,
>>>                          'pin-state': {'doc': 'pin disconnected',
>>>                                        'name': 'disconnected'}}],
>>>          'pin-rclk-device': '0000:21:00.0',
>>>          'pin-type': {'doc': "ethernet port PHY's recovered clock",
>>>                       'name': 'synce-eth-port'}}]}
>>
>>Got it, it is still a bit hard to me to follow this. Could you
>>perhaps extend the Documentation to describe in more details
>>with examples? Would help a lot for slower people like me to understand
>>what's what.
>>
>
>Actually this is already explained in "MUX-type pins" paragraph of
>Documentation/networking/dpll.rst.
>Do we want to duplicate this explanation here?

No, please extend the docs. As I wrote above, could you add some
examples, like the one you pasted above. Examples always help to
undestand things much better.


>
>
>>
>>>
>>>
>>>>
>>>>>+        nested-attributes: pin-parent
>>>>>+        value: 23
>>>>>+      -
>>>>>+        name: pin-rclk-device
>>>>>+        type: string
>>>>>+        value: 25
>>>>>+      -
>>>>>+        name: pin-dpll-caps
>>>>>+        type: u32
>>>>
>>>>Missing "enum: "
>>>>
>>>
>>>It is actually a bitmask, this is why didn't set as enum, with enum type
>>>parser won't parse it.
>>
>>Ah! Got it. Perhaps a docs note with the enum pointer then?
>>
>
>Same as above, explained in Documentation/networking/dpll.rst, do wan't to
>duplicate?

For this, yes. Some small doc note here would be quite convenient.

Also, I almost forgot: Please don't use NLA_U32 for caps flags. Please
use NLA_BITFIELD32 which was introduced for exactly this purpose. Allows
to do nicer validation as well.


>
>>
>>>
>>>>
>>>>>+  -
>>>>>+    name: pin-parent
>>>>>+    subset-of: dpll
>>>>>+    attributes:
>>>>>+      -
>>>>>+        name: pin-state
>>>>>+        type: u8
>>>>>+        value: 22
>>>>>+        enum: pin-state
>>>>>+      -
>>>>>+        name: pin-parent-idx
>>>>>+        type: u32
>>>>>+        value: 24
>>>>>+      -
>>>>>+        name: pin-rclk-device
>>>>>+        type: string
>>>>
>>>>Yeah, as I wrote in the other email, this really smells to
>>>>have like a simple string like this. What is it supposed to be?
>>>>
>>>
>>>Yes, let's discuss there.
>>
>>Yep.
>>
>>>
>>>>
>>>>>+
>>>>>+
>>>>>+operations:
>>>>>+  list:
>>>>>+    -
>>>>>+      name: unspec
>>>>>+      doc: unused
>>>>>+
>>>>>+    -
>>>>>+      name: device-get
>>>>>+      doc: |
>>>>>+        Get list of DPLL devices (dump) or attributes of a single dpll
>>>>device
>>>>>+      attribute-set: dpll
>>>>
>>>>Shouldn't this be "device"?
>>>>
>>>
>>>It would brake the parser, again I hope Jakub Kicinski could take a look
>>>on this.
>>
>>Odd.
>>
>
>Yes, seems a bit odd.
>
>>>
>>>>
>>>>>+      flags: [ admin-perm ]
>>>>>+
>>>>>+      do:
>>>>>+        pre: dpll-pre-doit
>>>>>+        post: dpll-post-doit
>>>>>+        request:
>>>>>+          attributes:
>>>>>+            - id
>>>>>+            - bus-name
>>>>>+            - dev-name
>>>>>+        reply:
>>>>>+          attributes:
>>>>>+            - device
>>>>>+
>>>>>+      dump:
>>>>>+        pre: dpll-pre-dumpit
>>>>>+        post: dpll-post-dumpit
>>>>>+        reply:
>>>>>+          attributes:
>>>>>+            - device
>>>>>+
>>>>>+    -
>>>>>+      name: device-set
>>>>>+      doc: Set attributes for a DPLL device
>>>>>+      attribute-set: dpll
>>>>
>>>>"device" here as well?
>>>>
>>>
>>>Same as above.
>>>
>>>>
>>>>>+      flags: [ admin-perm ]
>>>>>+
>>>>>+      do:
>>>>>+        pre: dpll-pre-doit
>>>>>+        post: dpll-post-doit
>>>>>+        request:
>>>>>+          attributes:
>>>>>+            - id
>>>>>+            - bus-name
>>>>>+            - dev-name
>>>>>+            - mode
>>>>
>>>>Hmm, shouldn't source-pin-index be here as well?
>>>
>>>No, there is no set for this.
>>>For manual mode user selects the pin by setting enabled state on the one
>>>he needs to recover signal from.
>>>
>>>source-pin-index is read only, returns active source.
>>
>>Okay, got it. Then why do we have this assymetric approach? Just have
>>the enabled state to serve the user to see which one is selected, no?
>>This would help to avoid confusion (like mine) and allow not to create
>>inconsistencies (like no pin enabled yet driver to return some source
>>pin index)
>>
>
>This is due to automatic mode were multiple pins are enabled, but actual
>selection is done on hardware level with priorities.

Okay, this is confusing and I believe wrong.
You have dual meaning for pin state attribute with states
STATE_CONNECTED/DISCONNECTED:

1) Manual mode, MUX pins (both share the same model):
   There is only one pin with STATE_CONNECTED. The others are in
   STATE_DISCONNECTED
   User changes a state of a pin to make the selection.

   Example:
     $ dplltool pin dump
       pin 1 state connected
       pin 2 state disconnected
     $ dplltool pin 2 set state connected
     $ dplltool pin dump
       pin 1 state disconnected
       pin 2 state connected

2) Automatic mode:
   The user by setting "state" decides it the pin should be considered
   by the device for auto selection.

   Example:
     $ dplltool pin dump:
       pin 1 state connected prio 10
       pin 2 state connected prio 15
     $ dplltool dpll x get:
       dpll x source-pin-index 1

So in manual mode, STATE_CONNECTED means the dpll is connected to this
source pin. However, in automatic mode it means something else. It means
the user allows this pin to be considered for auto selection. The fact
the pin is selected source is exposed over source-pin-index.

Instead of this, I believe that the semantics of
STATE_CONNECTED/DISCONNECTED should be the same for automatic mode as
well. Unlike the manual mode/mux, where the state is written by user, in
automatic mode the state should be only written by the driver. User
attemts to set the state should fail with graceful explanation (DPLL
netlink/core code should handle that, w/o driver interaction)

Suggested automatic mode example:
     $ dplltool pin dump:
       pin 1 state connected prio 10 connectable true
       pin 2 state disconnected prio 15 connectable true
     $ dplltool pin 1 set connectable false
     $ dplltool pin dump:
       pin 1 state disconnected prio 10 connectable false
       pin 2 state connected prio 15 connectable true
     $ dplltool pin 1 set state connected
       -EOPNOTSUPP

Note there is no "source-pin-index" at all. Replaced by pin state here.
There is a new attribute called "connectable", the user uses this
attribute to tell the device, if this source pin could be considered for
auto selection or not.

Could be called perhaps "selectable", does not matter. The point is, the
meaning of the "state" attribute is consistent for automatic mode,
manual mode and mux pin.

Makes sense?


>
>[...]
>
>>>>>+
>>>>>+/* DPLL_CMD_DEVICE_SET - do */
>>>>>+static const struct nla_policy dpll_device_set_nl_policy[DPLL_A_MODE +
>>>>>1]
>>>>>= {
>>>>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>>>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>>>+	[DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),
>>>>
>>>>Hmm, any idea why the generator does not put define name
>>>>here instead of "5"?
>>>>
>>>
>>>Not really, it probably needs a fix for this.
>>
>>Yeah.
>>
>
>Well, once we done with review maybe we could also fix those, or ask
>Jakub if he could help :)
>
>
>[...]
>
>>>>
>>>>>+	DPLL_A_PIN_PRIO,
>>>>>+	DPLL_A_PIN_STATE,
>>>>>+	DPLL_A_PIN_PARENT,
>>>>>+	DPLL_A_PIN_PARENT_IDX,
>>>>>+	DPLL_A_PIN_RCLK_DEVICE,
>>>>>+	DPLL_A_PIN_DPLL_CAPS,
>>>>
>>>>Just DPLL_A_PIN_CAPS is enough, that would be also consistent with the
>>>>enum name.
>>>
>>>Sure, fixed.
>>
>>
>>Thanks for all your work on this!
>
>Thanks for a great review! :)

Glad to help.

Jiri Pirko March 17, 2023, 10:07 a.m. UTC | #8

Fri, Mar 17, 2023 at 01:53:49AM CET, [e-mail] wrote:
>>From: Jiri Pirko <[e-mail]>
>>Sent: Thursday, March 16, 2023 4:20 PM
>>
>>Thu, Mar 16, 2023 at 02:45:10PM CET, [e-mail] wrote:
>>>Thu, Mar 16, 2023 at 02:15:59PM CET, [e-mail] wrote:
>>
>>[...]
>>
>>
>>>>>>+      flags: [ admin-perm ]
>>>>>>+
>>>>>>+      do:
>>>>>>+        pre: dpll-pre-doit
>>>>>>+        post: dpll-post-doit
>>>>>>+        request:
>>>>>>+          attributes:
>>>>>>+            - id
>>>>>>+            - bus-name
>>>>>>+            - dev-name
>>>>>>+            - mode
>>>>>
>>>>>Hmm, shouldn't source-pin-index be here as well?
>>>>
>>>>No, there is no set for this.
>>>>For manual mode user selects the pin by setting enabled state on the one
>>>>he needs to recover signal from.
>>>>
>>>>source-pin-index is read only, returns active source.
>>>
>>>Okay, got it. Then why do we have this assymetric approach? Just have
>>>the enabled state to serve the user to see which one is selected, no?
>>>This would help to avoid confusion (like mine) and allow not to create
>>>inconsistencies (like no pin enabled yet driver to return some source
>>>pin index)
>>
>>Actually, for mlx5 implementation, would be non-trivial to implement
>>this, as each of the pin/port is instantiated and controlled by separate
>>pci backend.
>>
>>Could you please remove, it is not needed and has potential and real
>>issues.
>>
>>[...]
>
>Sorry I cannot, for priority based automatic selection mode multiple sources
>are enabled at any time - selection is done automatically by the chip.
>Thus for that case, this attribute is only way of getting an active source.
>Although, maybe we could allow driver to not implement it, would this help
>for your case? As it seems only required for automatic mode selection.

Please see the other reply for this patch where I describe what I
think is wrong about this approach and suggesting a solution.


>
>Thank you,
>Arkadiusz

Jiri Pirko March 17, 2023, 2:29 p.m. UTC | #9

Fri, Mar 17, 2023 at 11:05:26AM CET, [e-mail] wrote:
>Fri, Mar 17, 2023 at 01:52:44AM CET, [e-mail] wrote:
>>>From: Jiri Pirko <[e-mail]>
>>>Sent: Thursday, March 16, 2023 2:45 PM
>>>
>>
>>[...]
>>
>>>>>>+attribute-sets:
>>>>>>+  -
>>>>>>+    name: dpll
>>>>>>+    enum-name: dplla
>>>>>>+    attributes:
>>>>>>+      -
>>>>>>+        name: device
>>>>>>+        type: nest
>>>>>>+        value: 1
>>>>>>+        multi-attr: true
>>>>>>+        nested-attributes: device
>>>>>
>>>>>What is this "device" and what is it good for? Smells like some leftover
>>>>>and with the nested scheme looks quite odd.
>>>>>
>>>>
>>>>No, it is nested attribute type, used when multiple devices are returned
>>>>with netlink:
>>>>
>>>>- dump of device-get command where all devices are returned, each one nested
>>>>inside it:
>>>>[{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id': 0},
>>>>             {'bus-name': 'pci', 'dev-name': '0000:21:00.0_1', 'id': 1}]}]
>>>
>>>Okay, why is it nested here? The is one netlink msg per dpll device
>>>instance. Is this the real output of you made that up?
>>>
>>>Device nest should not be there for DEVICE_GET, does not make sense.
>>>
>>
>>This was returned by CLI parser on ice with cmd:
>>$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml /
>>--dump device-get
>>
>>Please note this relates to 'dump' request , it is rather expected that there
>>are multiple dplls returned, thus we need a nest attribute for each one.
>
>No, you definitelly don't need to nest them. Dump format and get format
>should be exactly the same. Please remove the nest.

Another example:
$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/netdev.yaml --dump dev-get
[{'ifindex': 1, 'xdp-features': set()},
 {'ifindex': 2, 'xdp-features': {'basic', 'rx-sg', 'redirect'}},
 {'ifindex': 3, 'xdp-features': {'basic', 'rx-sg', 'redirect'}},
 {'ifindex': 4, 'xdp-features': set()},
 {'ifindex': 5, 'xdp-features': {'basic', 'rx-sg', 'xsk-zerocopy', 'redirect'}},
 {'ifindex': 6, 'xdp-features': {'basic', 'rx-sg', 'xsk-zerocopy', 'redirect'}}]

[...]

Arkadiusz Kubalewski March 17, 2023, 3:14 p.m. UTC | #10

>From: Jiri Pirko <[e-mail]>
>Sent: Friday, March 17, 2023 11:05 AM
>
>Fri, Mar 17, 2023 at 01:52:44AM CET, [e-mail] wrote:
>>>From: Jiri Pirko <[e-mail]>
>>>Sent: Thursday, March 16, 2023 2:45 PM
>>>
>>
>>[...]
>>
>>>>>>+attribute-sets:
>>>>>>+  -
>>>>>>+    name: dpll
>>>>>>+    enum-name: dplla
>>>>>>+    attributes:
>>>>>>+      -
>>>>>>+        name: device
>>>>>>+        type: nest
>>>>>>+        value: 1
>>>>>>+        multi-attr: true
>>>>>>+        nested-attributes: device
>>>>>
>>>>>What is this "device" and what is it good for? Smells like some leftover
>>>>>and with the nested scheme looks quite odd.
>>>>>
>>>>
>>>>No, it is nested attribute type, used when multiple devices are returned
>>>>with netlink:
>>>>
>>>>- dump of device-get command where all devices are returned, each one
>>>>nested
>>>>inside it:
>>>>[{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id': 0},
>>>>             {'bus-name': 'pci', 'dev-name': '0000:21:00.0_1', 'id':
>>>>1}]}]
>>>
>>>Okay, why is it nested here? The is one netlink msg per dpll device
>>>instance. Is this the real output of you made that up?
>>>
>>>Device nest should not be there for DEVICE_GET, does not make sense.
>>>
>>
>>This was returned by CLI parser on ice with cmd:
>>$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml /
>>--dump device-get
>>
>>Please note this relates to 'dump' request , it is rather expected that
>there
>>are multiple dplls returned, thus we need a nest attribute for each one.
>
>No, you definitelly don't need to nest them. Dump format and get format
>should be exactly the same. Please remove the nest.
>
>See how that is done in devlink for example: devlink_nl_fill()
>This functions fills up one object in the dump. No nesting.
>I'm not aware of such nesting approach anywhere in kernel dumps, does
>not make sense at all.
>

Yeah it make sense to have same output on `do` and `dump`, but this is also
achievable with nest DPLL_A_DEVICE, still don't need put extra header for it.
The difference would be that on `dump` multiple DPLL_A_DEVICE are provided,
on `do` only one.

Will try to fix it.
Although could you please explain why it make sense to put extra header
(exactly the same header) multiple times in one netlink response message? 

>
>>
>>>
>>>>
>>>>- do/dump of pin-get, in case of shared pins, each pin contains number
>of
>>>dpll
>>>>handles it connects with:
>>>>[{'pin': [{'device': [{'bus-name': 'pci',
>>>>                       'dev-name': '0000:21:00.0_0',
>>>>                       'id': 0,
>>>>                       'pin-prio': 6,
>>>>                       'pin-state': {'doc': 'pin connected',
>>>>                                     'name': 'connected'}},
>>>>                      {'bus-name': 'pci',
>>>>                       'dev-name': '0000:21:00.0_1',
>>>>                       'id': 1,
>>>>                       'pin-prio': 8,
>>>>                       'pin-state': {'doc': 'pin connected',
>>>>                                     'name': 'connected'}}],
>>>
>>>Okay, here I understand it contains device specific pin items. Makes
>>>sense!
>>>
>>
>>Good.
>
>Make sure you don't nest the pin objects for dump (DPLL_A_PIN). Same
>reason as above.
>I don't see a need for DPLL_A_PIN attr existence, please remove it.
>
>

Sure, will try.

>
>
>>
>>[...]
>>
>>>>
>>>>>
>>>>>
>>>>>>+      -
>>>>>>+        name: pin-prio
>>>>>>+        type: u32
>>>>>>+      -
>>>>>>+        name: pin-state
>>>>>>+        type: u8
>>>>>>+        enum: pin-state
>>>>>>+      -
>>>>>>+        name: pin-parent
>>>>>>+        type: nest
>>>>>>+        multi-attr: true
>>>>>>+        nested-attributes: pin
>>>>>>+        value: 23
>>>>>
>>>>>Value 23? What's this?
>>>>>You have it specified for some attrs all over the place.
>>>>>What is the reason for it?
>>>>>
>>>>
>>>>Actually this particular one is not needed (also value: 12 on pin above),
>>>>I will remove those.
>>>>But the others you are refering to (the ones in nested attribute list),
>>>>are required because of cli.py parser issue, maybe Kuba knows a better way
>>>to
>>>>prevent the issue?
>>>>Basically, without those values, cli.py brakes on parsing responses, after
>>>>every "jump" to nested attribute list it is assigning first attribute
>>>there
>>>>with value=0, thus there is a need to assign a proper value, same as it is
>>>on
>>>>'main' attribute list.
>>>
>>>That's weird. Looks like a bug then?
>>>
>>
>>Guess we could call it a bug, I haven't investigated the parser that much,
>>AFAIR, other specs are doing the same way.
>>
>>>
>>>>
>>>>>
>>>>>>+      -
>>>>>>+        name: pin-parent-idx
>>>>>>+        type: u32
>>>>>>+      -
>>>>>>+        name: pin-rclk-device
>>>>>>+        type: string
>>>>>>+      -
>>>>>>+        name: pin-dpll-caps
>>>>>>+        type: u32
>>>>>>+  -
>>>>>>+    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: source-pin-idx
>>>>>>+        type: u32
>>>>>>+      -
>>>>>>+        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
>>>>>>+        type: nest
>>>>>>+        value: 12
>>>>>>+        multi-attr: true
>>>>>>+        nested-attributes: pin
>>>>>
>>>>>This does not belong here.
>>>>>
>>>>
>>>>What do you mean?
>>>>With device-get 'do' request the list of pins connected to the dpll is
>>>>returned, each pin is nested in this attribute.
>>>
>>>No, wait a sec. You have 2 object types: device and pin. Each have
>>>separate netlink CMDs to get and dump individual objects.
>>>Don't mix those together like this. I thought it became clear in the
>>>past. :/
>>>
>>
>>For pins we must, as pins without a handle to a dpll are pointless.
>
>I'm not talking about per device specific items for pins (state and
>prio). That is something else, it's a pin-device tuple. Completely fine.
>
>
>
>>Same as a dpll without pins, right?
>>
>>'do' of DEVICE_GET could just dump it's own status, without the list of
>pins,
>
>Yes please.
>
>
>>but it feels easier for handling it's state on userspace counterpart if
>>that command also returns currently registered pins. Don't you think so?
>
>No, definitelly not. Please make the object separation clear. Device and
>pins are different objects, they have different commands to work with.
>Don't mix them together.
>

It does, since the user app wouldn't have to dump/get pins continuously.
But yeah, as object separation argument makes sense will try to fix it.

>
>>
>>>
>>>>This is required by parser to work.
>>>>
>>>>>
>>>>>>+      -
>>>>>>+        name: pin-prio
>>>>>>+        type: u32
>>>>>>+        value: 21
>>>>>>+      -
>>>>>>+        name: pin-state
>>>>>>+        type: u8
>>>>>>+        enum: pin-state
>>>>>>+      -
>>>>>>+        name: pin-dpll-caps
>>>>>>+        type: u32
>>>>>>+        value: 26
>>>>>
>>>>>All these 3 do not belong here are well.
>>>>>
>>>>
>>>>Same as above explanation.
>>>
>>>Same as above reply.
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>>+  -
>>>>>>+    name: pin
>>>>>>+    subset-of: dpll
>>>>>>+    attributes:
>>>>>>+      -
>>>>>>+        name: device
>>>>>>+        type: nest
>>>>>>+        value: 1
>>>>>>+        multi-attr: true
>>>>>>+        nested-attributes: device
>>>>>>+      -
>>>>>>+        name: pin-idx
>>>>>>+        type: u32
>>>>>>+        value: 13
>>>>>>+      -
>>>>>>+        name: pin-description
>>>>>>+        type: string
>>>>>>+      -
>>>>>>+        name: pin-type
>>>>>>+        type: u8
>>>>>>+        enum: pin-type
>>>>>>+      -
>>>>>>+        name: pin-direction
>>>>>>+        type: u8
>>>>>>+        enum: pin-direction
>>>>>>+      -
>>>>>>+        name: pin-frequency
>>>>>>+        type: u32
>>>>>>+      -
>>>>>>+        name: pin-frequency-supported
>>>>>>+        type: u32
>>>>>>+        multi-attr: true
>>>>>>+      -
>>>>>>+        name: pin-any-frequency-min
>>>>>>+        type: u32
>>>>>>+      -
>>>>>>+        name: pin-any-frequency-max
>>>>>>+        type: u32
>>>>>>+      -
>>>>>>+        name: pin-prio
>>>>>>+        type: u32
>>>>>>+      -
>>>>>>+        name: pin-state
>>>>>>+        type: u8
>>>>>>+        enum: pin-state
>>>>>>+      -
>>>>>>+        name: pin-parent
>>>>>>+        type: nest
>>>>>>+        multi-attr: true
>>>>>
>>>>>Multiple parents? How is that supposed to work?
>>>>>
>>>>
>>>>As we have agreed, MUXed pins can have multiple parents.
>>>>In our case:
>>>>/tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do
>>>>pin-get --json '{"id": 0, "pin-idx":13}'
>>>>{'pin': [{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0',
>>>>'id': 0},
>>>>                     {'bus-name': 'pci',
>>>>                      'dev-name': '0000:21:00.0_1',
>>>>                      'id': 1}],
>>>>          'pin-description': '0000:21:00.0',
>>>>          'pin-direction': {'doc': 'pin used as a source of a signal',
>>>>                            'name': 'source'},
>>>>          'pin-idx': 13,
>>>>          'pin-parent': [{'pin-parent-idx': 2,
>>>>                          'pin-state': {'doc': 'pin disconnected',
>>>>                                        'name': 'disconnected'}},
>>>>                         {'pin-parent-idx': 3,
>>>>                          'pin-state': {'doc': 'pin disconnected',
>>>>                                        'name': 'disconnected'}}],
>>>>          'pin-rclk-device': '0000:21:00.0',
>>>>          'pin-type': {'doc': "ethernet port PHY's recovered clock",
>>>>                       'name': 'synce-eth-port'}}]}
>>>
>>>Got it, it is still a bit hard to me to follow this. Could you
>>>perhaps extend the Documentation to describe in more details
>>>with examples? Would help a lot for slower people like me to understand
>>>what's what.
>>>
>>
>>Actually this is already explained in "MUX-type pins" paragraph of
>>Documentation/networking/dpll.rst.
>>Do we want to duplicate this explanation here?
>
>No, please extend the docs. As I wrote above, could you add some
>examples, like the one you pasted above. Examples always help to
>undestand things much better.
>

Sure, fixed.

>
>>
>>
>>>
>>>>
>>>>
>>>>>
>>>>>>+        nested-attributes: pin-parent
>>>>>>+        value: 23
>>>>>>+      -
>>>>>>+        name: pin-rclk-device
>>>>>>+        type: string
>>>>>>+        value: 25
>>>>>>+      -
>>>>>>+        name: pin-dpll-caps
>>>>>>+        type: u32
>>>>>
>>>>>Missing "enum: "
>>>>>
>>>>
>>>>It is actually a bitmask, this is why didn't set as enum, with enum type
>>>>parser won't parse it.
>>>
>>>Ah! Got it. Perhaps a docs note with the enum pointer then?
>>>
>>
>>Same as above, explained in Documentation/networking/dpll.rst, do wan't to
>>duplicate?
>
>For this, yes. Some small doc note here would be quite convenient.
>
>Also, I almost forgot: Please don't use NLA_U32 for caps flags. Please
>use NLA_BITFIELD32 which was introduced for exactly this purpose. Allows
>to do nicer validation as well.
>

Actually BITFIELD32 is to much for this case, the bit itself would be enough
as we don't need validity bit.
But yeah, as there is no BITMASK yet, will try to change to BITFIELD32.

[...]

>>>>>
>>>>>Hmm, shouldn't source-pin-index be here as well?
>>>>
>>>>No, there is no set for this.
>>>>For manual mode user selects the pin by setting enabled state on the one
>>>>he needs to recover signal from.
>>>>
>>>>source-pin-index is read only, returns active source.
>>>
>>>Okay, got it. Then why do we have this assymetric approach? Just have
>>>the enabled state to serve the user to see which one is selected, no?
>>>This would help to avoid confusion (like mine) and allow not to create
>>>inconsistencies (like no pin enabled yet driver to return some source
>>>pin index)
>>>
>>
>>This is due to automatic mode were multiple pins are enabled, but actual
>>selection is done on hardware level with priorities.
>
>Okay, this is confusing and I believe wrong.
>You have dual meaning for pin state attribute with states
>STATE_CONNECTED/DISCONNECTED:
>
>1) Manual mode, MUX pins (both share the same model):
>   There is only one pin with STATE_CONNECTED. The others are in
>   STATE_DISCONNECTED
>   User changes a state of a pin to make the selection.
>
>   Example:
>     $ dplltool pin dump
>       pin 1 state connected
>       pin 2 state disconnected
>     $ dplltool pin 2 set state connected
>     $ dplltool pin dump
>       pin 1 state disconnected
>       pin 2 state connected
>
>2) Automatic mode:
>   The user by setting "state" decides it the pin should be considered
>   by the device for auto selection.
>
>   Example:
>     $ dplltool pin dump:
>       pin 1 state connected prio 10
>       pin 2 state connected prio 15
>     $ dplltool dpll x get:
>       dpll x source-pin-index 1
>
>So in manual mode, STATE_CONNECTED means the dpll is connected to this
>source pin. However, in automatic mode it means something else. It means
>the user allows this pin to be considered for auto selection. The fact
>the pin is selected source is exposed over source-pin-index.
>
>Instead of this, I believe that the semantics of
>STATE_CONNECTED/DISCONNECTED should be the same for automatic mode as
>well. Unlike the manual mode/mux, where the state is written by user, in
>automatic mode the state should be only written by the driver. User
>attemts to set the state should fail with graceful explanation (DPLL
>netlink/core code should handle that, w/o driver interaction)
>
>Suggested automatic mode example:
>     $ dplltool pin dump:
>       pin 1 state connected prio 10 connectable true
>       pin 2 state disconnected prio 15 connectable true
>     $ dplltool pin 1 set connectable false
>     $ dplltool pin dump:
>       pin 1 state disconnected prio 10 connectable false
>       pin 2 state connected prio 15 connectable true
>     $ dplltool pin 1 set state connected
>       -EOPNOTSUPP
>
>Note there is no "source-pin-index" at all. Replaced by pin state here.
>There is a new attribute called "connectable", the user uses this
>attribute to tell the device, if this source pin could be considered for
>auto selection or not.
>
>Could be called perhaps "selectable", does not matter. The point is, the
>meaning of the "state" attribute is consistent for automatic mode,
>manual mode and mux pin.
>
>Makes sense?
>

Great idea!
I will add third enum for pin-state: DPLL_PIN_STATE_SELECTABLE.
In the end we will have this:
              +--------------------------------+
              | valid DPLL_A_PIN_STATE values  |
	      +---------------+----------------+
+------------+| requested:    | returned:      |
|DPLL_A_MODE:||               |                |
|------------++--------------------------------|
|AUTOMATIC   ||- SELECTABLE   | - SELECTABLE   |
|            ||- DISCONNECTED | - DISCONNECTED |
|            ||               | - CONNECTED    |
|------------++--------------------------------|
|MANUAL      ||- CONNECTED    | - CONNECTED    |
|            ||- DISCONNECTED | - DISCONNECTED |
+------------++---------------+----------------+

Thank you,
Arkadiusz

>
>>
>>[...]
>>
>>>>>>+
>>>>>>+/* DPLL_CMD_DEVICE_SET - do */
>>>>>>+static const struct nla_policy dpll_device_set_nl_policy[DPLL_A_MODE +
>>>>>>1]
>>>>>>= {
>>>>>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>>>>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>>>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>>>>+	[DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),
>>>>>
>>>>>Hmm, any idea why the generator does not put define name
>>>>>here instead of "5"?
>>>>>
>>>>
>>>>Not really, it probably needs a fix for this.
>>>
>>>Yeah.
>>>
>>
>>Well, once we done with review maybe we could also fix those, or ask
>>Jakub if he could help :)
>>
>>
>>[...]
>>
>>>>>
>>>>>>+	DPLL_A_PIN_PRIO,
>>>>>>+	DPLL_A_PIN_STATE,
>>>>>>+	DPLL_A_PIN_PARENT,
>>>>>>+	DPLL_A_PIN_PARENT_IDX,
>>>>>>+	DPLL_A_PIN_RCLK_DEVICE,
>>>>>>+	DPLL_A_PIN_DPLL_CAPS,
>>>>>
>>>>>Just DPLL_A_PIN_CAPS is enough, that would be also consistent with the
>>>>>enum name.
>>>>
>>>>Sure, fixed.
>>>
>>>
>>>Thanks for all your work on this!
>>
>>Thanks for a great review! :)
>
>Glad to help.

Jiri Pirko March 17, 2023, 4:20 p.m. UTC | #11

Fri, Mar 17, 2023 at 04:14:45PM CET, [e-mail] wrote:
>
>>From: Jiri Pirko <[e-mail]>
>>Sent: Friday, March 17, 2023 11:05 AM
>>
>>Fri, Mar 17, 2023 at 01:52:44AM CET, [e-mail] wrote:
>>>>From: Jiri Pirko <[e-mail]>
>>>>Sent: Thursday, March 16, 2023 2:45 PM
>>>>
>>>
>>>[...]
>>>
>>>>>>>+attribute-sets:
>>>>>>>+  -
>>>>>>>+    name: dpll
>>>>>>>+    enum-name: dplla
>>>>>>>+    attributes:
>>>>>>>+      -
>>>>>>>+        name: device
>>>>>>>+        type: nest
>>>>>>>+        value: 1
>>>>>>>+        multi-attr: true
>>>>>>>+        nested-attributes: device
>>>>>>
>>>>>>What is this "device" and what is it good for? Smells like some leftover
>>>>>>and with the nested scheme looks quite odd.
>>>>>>
>>>>>
>>>>>No, it is nested attribute type, used when multiple devices are returned
>>>>>with netlink:
>>>>>
>>>>>- dump of device-get command where all devices are returned, each one
>>>>>nested
>>>>>inside it:
>>>>>[{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id': 0},
>>>>>             {'bus-name': 'pci', 'dev-name': '0000:21:00.0_1', 'id':
>>>>>1}]}]
>>>>
>>>>Okay, why is it nested here? The is one netlink msg per dpll device
>>>>instance. Is this the real output of you made that up?
>>>>
>>>>Device nest should not be there for DEVICE_GET, does not make sense.
>>>>
>>>
>>>This was returned by CLI parser on ice with cmd:
>>>$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml /
>>>--dump device-get
>>>
>>>Please note this relates to 'dump' request , it is rather expected that
>>there
>>>are multiple dplls returned, thus we need a nest attribute for each one.
>>
>>No, you definitelly don't need to nest them. Dump format and get format
>>should be exactly the same. Please remove the nest.
>>
>>See how that is done in devlink for example: devlink_nl_fill()
>>This functions fills up one object in the dump. No nesting.
>>I'm not aware of such nesting approach anywhere in kernel dumps, does
>>not make sense at all.
>>
>
>Yeah it make sense to have same output on `do` and `dump`, but this is also
>achievable with nest DPLL_A_DEVICE, still don't need put extra header for it.
>The difference would be that on `dump` multiple DPLL_A_DEVICE are provided,
>on `do` only one.

Please don't. This root nesting is not correct.


>
>Will try to fix it.
>Although could you please explain why it make sense to put extra header
>(exactly the same header) multiple times in one netlink response message? 

This is how it's done for all netlink dumps as far as I know.
The reason might be that the userspace is parsing exactly the same
message as if it would be DOIT message.


>
>>
>>>
>>>>
>>>>>
>>>>>- do/dump of pin-get, in case of shared pins, each pin contains number
>>of
>>>>dpll
>>>>>handles it connects with:
>>>>>[{'pin': [{'device': [{'bus-name': 'pci',
>>>>>                       'dev-name': '0000:21:00.0_0',
>>>>>                       'id': 0,
>>>>>                       'pin-prio': 6,
>>>>>                       'pin-state': {'doc': 'pin connected',
>>>>>                                     'name': 'connected'}},
>>>>>                      {'bus-name': 'pci',
>>>>>                       'dev-name': '0000:21:00.0_1',
>>>>>                       'id': 1,
>>>>>                       'pin-prio': 8,
>>>>>                       'pin-state': {'doc': 'pin connected',
>>>>>                                     'name': 'connected'}}],
>>>>
>>>>Okay, here I understand it contains device specific pin items. Makes
>>>>sense!
>>>>
>>>
>>>Good.
>>
>>Make sure you don't nest the pin objects for dump (DPLL_A_PIN). Same
>>reason as above.
>>I don't see a need for DPLL_A_PIN attr existence, please remove it.
>>
>>
>
>Sure, will try.

Cool.


>
>>
>>
>>>
>>>[...]
>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>+      -
>>>>>>>+        name: pin-prio
>>>>>>>+        type: u32
>>>>>>>+      -
>>>>>>>+        name: pin-state
>>>>>>>+        type: u8
>>>>>>>+        enum: pin-state
>>>>>>>+      -
>>>>>>>+        name: pin-parent
>>>>>>>+        type: nest
>>>>>>>+        multi-attr: true
>>>>>>>+        nested-attributes: pin
>>>>>>>+        value: 23
>>>>>>
>>>>>>Value 23? What's this?
>>>>>>You have it specified for some attrs all over the place.
>>>>>>What is the reason for it?
>>>>>>
>>>>>
>>>>>Actually this particular one is not needed (also value: 12 on pin above),
>>>>>I will remove those.
>>>>>But the others you are refering to (the ones in nested attribute list),
>>>>>are required because of cli.py parser issue, maybe Kuba knows a better way
>>>>to
>>>>>prevent the issue?
>>>>>Basically, without those values, cli.py brakes on parsing responses, after
>>>>>every "jump" to nested attribute list it is assigning first attribute
>>>>there
>>>>>with value=0, thus there is a need to assign a proper value, same as it is
>>>>on
>>>>>'main' attribute list.
>>>>
>>>>That's weird. Looks like a bug then?
>>>>
>>>
>>>Guess we could call it a bug, I haven't investigated the parser that much,
>>>AFAIR, other specs are doing the same way.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>+      -
>>>>>>>+        name: pin-parent-idx
>>>>>>>+        type: u32
>>>>>>>+      -
>>>>>>>+        name: pin-rclk-device
>>>>>>>+        type: string
>>>>>>>+      -
>>>>>>>+        name: pin-dpll-caps
>>>>>>>+        type: u32
>>>>>>>+  -
>>>>>>>+    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: source-pin-idx
>>>>>>>+        type: u32
>>>>>>>+      -
>>>>>>>+        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
>>>>>>>+        type: nest
>>>>>>>+        value: 12
>>>>>>>+        multi-attr: true
>>>>>>>+        nested-attributes: pin
>>>>>>
>>>>>>This does not belong here.
>>>>>>
>>>>>
>>>>>What do you mean?
>>>>>With device-get 'do' request the list of pins connected to the dpll is
>>>>>returned, each pin is nested in this attribute.
>>>>
>>>>No, wait a sec. You have 2 object types: device and pin. Each have
>>>>separate netlink CMDs to get and dump individual objects.
>>>>Don't mix those together like this. I thought it became clear in the
>>>>past. :/
>>>>
>>>
>>>For pins we must, as pins without a handle to a dpll are pointless.
>>
>>I'm not talking about per device specific items for pins (state and
>>prio). That is something else, it's a pin-device tuple. Completely fine.
>>
>>
>>
>>>Same as a dpll without pins, right?
>>>
>>>'do' of DEVICE_GET could just dump it's own status, without the list of
>>pins,
>>
>>Yes please.
>>
>>
>>>but it feels easier for handling it's state on userspace counterpart if
>>>that command also returns currently registered pins. Don't you think so?
>>
>>No, definitelly not. Please make the object separation clear. Device and
>>pins are different objects, they have different commands to work with.
>>Don't mix them together.
>>
>
>It does, since the user app wouldn't have to dump/get pins continuously.

I don't follow. For every pin change there is going to be pin object
dumped over monitoring netlink.


>But yeah, as object separation argument makes sense will try to fix it.

Awesome.


>
>>
>>>
>>>>
>>>>>This is required by parser to work.
>>>>>
>>>>>>
>>>>>>>+      -
>>>>>>>+        name: pin-prio
>>>>>>>+        type: u32
>>>>>>>+        value: 21
>>>>>>>+      -
>>>>>>>+        name: pin-state
>>>>>>>+        type: u8
>>>>>>>+        enum: pin-state
>>>>>>>+      -
>>>>>>>+        name: pin-dpll-caps
>>>>>>>+        type: u32
>>>>>>>+        value: 26
>>>>>>
>>>>>>All these 3 do not belong here are well.
>>>>>>
>>>>>
>>>>>Same as above explanation.
>>>>
>>>>Same as above reply.
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>+  -
>>>>>>>+    name: pin
>>>>>>>+    subset-of: dpll
>>>>>>>+    attributes:
>>>>>>>+      -
>>>>>>>+        name: device
>>>>>>>+        type: nest
>>>>>>>+        value: 1
>>>>>>>+        multi-attr: true
>>>>>>>+        nested-attributes: device
>>>>>>>+      -
>>>>>>>+        name: pin-idx
>>>>>>>+        type: u32
>>>>>>>+        value: 13
>>>>>>>+      -
>>>>>>>+        name: pin-description
>>>>>>>+        type: string
>>>>>>>+      -
>>>>>>>+        name: pin-type
>>>>>>>+        type: u8
>>>>>>>+        enum: pin-type
>>>>>>>+      -
>>>>>>>+        name: pin-direction
>>>>>>>+        type: u8
>>>>>>>+        enum: pin-direction
>>>>>>>+      -
>>>>>>>+        name: pin-frequency
>>>>>>>+        type: u32
>>>>>>>+      -
>>>>>>>+        name: pin-frequency-supported
>>>>>>>+        type: u32
>>>>>>>+        multi-attr: true
>>>>>>>+      -
>>>>>>>+        name: pin-any-frequency-min
>>>>>>>+        type: u32
>>>>>>>+      -
>>>>>>>+        name: pin-any-frequency-max
>>>>>>>+        type: u32
>>>>>>>+      -
>>>>>>>+        name: pin-prio
>>>>>>>+        type: u32
>>>>>>>+      -
>>>>>>>+        name: pin-state
>>>>>>>+        type: u8
>>>>>>>+        enum: pin-state
>>>>>>>+      -
>>>>>>>+        name: pin-parent
>>>>>>>+        type: nest
>>>>>>>+        multi-attr: true
>>>>>>
>>>>>>Multiple parents? How is that supposed to work?
>>>>>>
>>>>>
>>>>>As we have agreed, MUXed pins can have multiple parents.
>>>>>In our case:
>>>>>/tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do
>>>>>pin-get --json '{"id": 0, "pin-idx":13}'
>>>>>{'pin': [{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0',
>>>>>'id': 0},
>>>>>                     {'bus-name': 'pci',
>>>>>                      'dev-name': '0000:21:00.0_1',
>>>>>                      'id': 1}],
>>>>>          'pin-description': '0000:21:00.0',
>>>>>          'pin-direction': {'doc': 'pin used as a source of a signal',
>>>>>                            'name': 'source'},
>>>>>          'pin-idx': 13,
>>>>>          'pin-parent': [{'pin-parent-idx': 2,
>>>>>                          'pin-state': {'doc': 'pin disconnected',
>>>>>                                        'name': 'disconnected'}},
>>>>>                         {'pin-parent-idx': 3,
>>>>>                          'pin-state': {'doc': 'pin disconnected',
>>>>>                                        'name': 'disconnected'}}],
>>>>>          'pin-rclk-device': '0000:21:00.0',
>>>>>          'pin-type': {'doc': "ethernet port PHY's recovered clock",
>>>>>                       'name': 'synce-eth-port'}}]}
>>>>
>>>>Got it, it is still a bit hard to me to follow this. Could you
>>>>perhaps extend the Documentation to describe in more details
>>>>with examples? Would help a lot for slower people like me to understand
>>>>what's what.
>>>>
>>>
>>>Actually this is already explained in "MUX-type pins" paragraph of
>>>Documentation/networking/dpll.rst.
>>>Do we want to duplicate this explanation here?
>>
>>No, please extend the docs. As I wrote above, could you add some
>>examples, like the one you pasted above. Examples always help to
>>undestand things much better.
>>
>
>Sure, fixed.
>
>>
>>>
>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>>+        nested-attributes: pin-parent
>>>>>>>+        value: 23
>>>>>>>+      -
>>>>>>>+        name: pin-rclk-device
>>>>>>>+        type: string
>>>>>>>+        value: 25
>>>>>>>+      -
>>>>>>>+        name: pin-dpll-caps
>>>>>>>+        type: u32
>>>>>>
>>>>>>Missing "enum: "
>>>>>>
>>>>>
>>>>>It is actually a bitmask, this is why didn't set as enum, with enum type
>>>>>parser won't parse it.
>>>>
>>>>Ah! Got it. Perhaps a docs note with the enum pointer then?
>>>>
>>>
>>>Same as above, explained in Documentation/networking/dpll.rst, do wan't to
>>>duplicate?
>>
>>For this, yes. Some small doc note here would be quite convenient.
>>
>>Also, I almost forgot: Please don't use NLA_U32 for caps flags. Please
>>use NLA_BITFIELD32 which was introduced for exactly this purpose. Allows
>>to do nicer validation as well.
>>
>
>Actually BITFIELD32 is to much for this case, the bit itself would be enough
>as we don't need validity bit.
>But yeah, as there is no BITMASK yet, will try to change to BITFIELD32.
>
>[...]
>
>>>>>>
>>>>>>Hmm, shouldn't source-pin-index be here as well?
>>>>>
>>>>>No, there is no set for this.
>>>>>For manual mode user selects the pin by setting enabled state on the one
>>>>>he needs to recover signal from.
>>>>>
>>>>>source-pin-index is read only, returns active source.
>>>>
>>>>Okay, got it. Then why do we have this assymetric approach? Just have
>>>>the enabled state to serve the user to see which one is selected, no?
>>>>This would help to avoid confusion (like mine) and allow not to create
>>>>inconsistencies (like no pin enabled yet driver to return some source
>>>>pin index)
>>>>
>>>
>>>This is due to automatic mode were multiple pins are enabled, but actual
>>>selection is done on hardware level with priorities.
>>
>>Okay, this is confusing and I believe wrong.
>>You have dual meaning for pin state attribute with states
>>STATE_CONNECTED/DISCONNECTED:
>>
>>1) Manual mode, MUX pins (both share the same model):
>>   There is only one pin with STATE_CONNECTED. The others are in
>>   STATE_DISCONNECTED
>>   User changes a state of a pin to make the selection.
>>
>>   Example:
>>     $ dplltool pin dump
>>       pin 1 state connected
>>       pin 2 state disconnected
>>     $ dplltool pin 2 set state connected
>>     $ dplltool pin dump
>>       pin 1 state disconnected
>>       pin 2 state connected
>>
>>2) Automatic mode:
>>   The user by setting "state" decides it the pin should be considered
>>   by the device for auto selection.
>>
>>   Example:
>>     $ dplltool pin dump:
>>       pin 1 state connected prio 10
>>       pin 2 state connected prio 15
>>     $ dplltool dpll x get:
>>       dpll x source-pin-index 1
>>
>>So in manual mode, STATE_CONNECTED means the dpll is connected to this
>>source pin. However, in automatic mode it means something else. It means
>>the user allows this pin to be considered for auto selection. The fact
>>the pin is selected source is exposed over source-pin-index.
>>
>>Instead of this, I believe that the semantics of
>>STATE_CONNECTED/DISCONNECTED should be the same for automatic mode as
>>well. Unlike the manual mode/mux, where the state is written by user, in
>>automatic mode the state should be only written by the driver. User
>>attemts to set the state should fail with graceful explanation (DPLL
>>netlink/core code should handle that, w/o driver interaction)
>>
>>Suggested automatic mode example:
>>     $ dplltool pin dump:
>>       pin 1 state connected prio 10 connectable true
>>       pin 2 state disconnected prio 15 connectable true
>>     $ dplltool pin 1 set connectable false
>>     $ dplltool pin dump:
>>       pin 1 state disconnected prio 10 connectable false
>>       pin 2 state connected prio 15 connectable true
>>     $ dplltool pin 1 set state connected
>>       -EOPNOTSUPP
>>
>>Note there is no "source-pin-index" at all. Replaced by pin state here.
>>There is a new attribute called "connectable", the user uses this
>>attribute to tell the device, if this source pin could be considered for
>>auto selection or not.
>>
>>Could be called perhaps "selectable", does not matter. The point is, the
>>meaning of the "state" attribute is consistent for automatic mode,
>>manual mode and mux pin.
>>
>>Makes sense?
>>
>
>Great idea!
>I will add third enum for pin-state: DPLL_PIN_STATE_SELECTABLE.
>In the end we will have this:
>              +--------------------------------+
>              | valid DPLL_A_PIN_STATE values  |
>	      +---------------+----------------+
>+------------+| requested:    | returned:      |
>|DPLL_A_MODE:||               |                |
>|------------++--------------------------------|
>|AUTOMATIC   ||- SELECTABLE   | - SELECTABLE   |
>|            ||- DISCONNECTED | - DISCONNECTED |
>|            ||               | - CONNECTED    |

"selectable" is something the user sets.
"connected"/"disconnected" is in case of auto mode something that driver
sets.

Looks a bit odd to mix them together. That is why I suggested
to have sepectable as a separate attr. But up to you. Please make sure
you sanitize the user/driver set of this attr in dpll code.


>|------------++--------------------------------|
>|MANUAL      ||- CONNECTED    | - CONNECTED    |
>|            ||- DISCONNECTED | - DISCONNECTED |
>+------------++---------------+----------------+
>
>Thank you,
>Arkadiusz
>
>>
>>>
>>>[...]
>>>
>>>>>>>+
>>>>>>>+/* DPLL_CMD_DEVICE_SET - do */
>>>>>>>+static const struct nla_policy dpll_device_set_nl_policy[DPLL_A_MODE +
>>>>>>>1]
>>>>>>>= {
>>>>>>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>>>>>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>>>>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>>>>>+	[DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),
>>>>>>
>>>>>>Hmm, any idea why the generator does not put define name
>>>>>>here instead of "5"?
>>>>>>
>>>>>
>>>>>Not really, it probably needs a fix for this.
>>>>
>>>>Yeah.
>>>>
>>>
>>>Well, once we done with review maybe we could also fix those, or ask
>>>Jakub if he could help :)
>>>
>>>
>>>[...]
>>>
>>>>>>
>>>>>>>+	DPLL_A_PIN_PRIO,
>>>>>>>+	DPLL_A_PIN_STATE,
>>>>>>>+	DPLL_A_PIN_PARENT,
>>>>>>>+	DPLL_A_PIN_PARENT_IDX,
>>>>>>>+	DPLL_A_PIN_RCLK_DEVICE,
>>>>>>>+	DPLL_A_PIN_DPLL_CAPS,
>>>>>>
>>>>>>Just DPLL_A_PIN_CAPS is enough, that would be also consistent with the
>>>>>>enum name.
>>>>>
>>>>>Sure, fixed.
>>>>
>>>>
>>>>Thanks for all your work on this!
>>>
>>>Thanks for a great review! :)
>>
>>Glad to help.

Jiri Pirko March 17, 2023, 4:23 p.m. UTC | #12

Sun, Mar 12, 2023 at 03:28:02AM CET, [e-mail] wrote:
>From: Arkadiusz Kubalewski <[e-mail]>

[...]

>+/* Ops table for dpll */
>+static const struct genl_split_ops dpll_nl_ops[6] = {

It's odd to see 6 here. Should by just []
But it's an issue of Jakub's generator.

[...]

Jiri Pirko March 17, 2023, 4:53 p.m. UTC | #13

Sun, Mar 12, 2023 at 03:28:02AM CET, [e-mail] wrote:
>From: Arkadiusz Kubalewski <[e-mail]>
>

[...]


>+      name: device-get
>+      doc: |
>+        Get list of DPLL devices (dump) or attributes of a single dpll device
>+      attribute-set: dpll
>+      flags: [ admin-perm ]
>+

[...]


>+    -
>+      name: pin-get
>+      doc: |
>+        Get list of pins and its attributes.
>+        - dump request without any attributes given - list all the pins in the system
>+        - dump request with target dpll - list all the pins registered with a given dpll device
>+        - do request with target dpll and target pin - single pin attributes
>+      attribute-set: dpll
>+      flags: [ admin-perm ]

Any particular reason to have admin cap required for get operations?
If not, please remove.

Arkadiusz Kubalewski March 17, 2023, 6:22 p.m. UTC | #14

>From: Jiri Pirko <[e-mail]>
>Sent: Friday, March 17, 2023 5:21 PM
>
>Fri, Mar 17, 2023 at 04:14:45PM CET, [e-mail] wrote:
>>
>>>From: Jiri Pirko <[e-mail]>
>>>Sent: Friday, March 17, 2023 11:05 AM
>>>
>>>Fri, Mar 17, 2023 at 01:52:44AM CET, [e-mail]
>>>wrote:
>>>>>From: Jiri Pirko <[e-mail]>
>>>>>Sent: Thursday, March 16, 2023 2:45 PM
>>>>>
>>>>
>>>>[...]
>>>>
>>>>>>>>+attribute-sets:
>>>>>>>>+  -
>>>>>>>>+    name: dpll
>>>>>>>>+    enum-name: dplla
>>>>>>>>+    attributes:
>>>>>>>>+      -
>>>>>>>>+        name: device
>>>>>>>>+        type: nest
>>>>>>>>+        value: 1
>>>>>>>>+        multi-attr: true
>>>>>>>>+        nested-attributes: device
>>>>>>>
>>>>>>>What is this "device" and what is it good for? Smells like some
>>>>>>>leftover
>>>>>>>and with the nested scheme looks quite odd.
>>>>>>>
>>>>>>
>>>>>>No, it is nested attribute type, used when multiple devices are returned
>>>>>>with netlink:
>>>>>>
>>>>>>- dump of device-get command where all devices are returned, each one
>>>>>>nested
>>>>>>inside it:
>>>>>>[{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id':0},
>>>>>>             {'bus-name': 'pci', 'dev-name': '0000:21:00.0_1', 'id':1}]}]
>>>>>
>>>>>Okay, why is it nested here? The is one netlink msg per dpll device
>>>>>instance. Is this the real output of you made that up?
>>>>>
>>>>>Device nest should not be there for DEVICE_GET, does not make sense.
>>>>>
>>>>
>>>>This was returned by CLI parser on ice with cmd:
>>>>$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml /
>>>>--dump device-get
>>>>
>>>>Please note this relates to 'dump' request , it is rather expected that
>>>there
>>>>are multiple dplls returned, thus we need a nest attribute for each one.
>>>
>>>No, you definitelly don't need to nest them. Dump format and get format
>>>should be exactly the same. Please remove the nest.
>>>
>>>See how that is done in devlink for example: devlink_nl_fill()
>>>This functions fills up one object in the dump. No nesting.
>>>I'm not aware of such nesting approach anywhere in kernel dumps, does
>>>not make sense at all.
>>>
>>
>>Yeah it make sense to have same output on `do` and `dump`, but this is also
>>achievable with nest DPLL_A_DEVICE, still don't need put extra header for it.
>>The difference would be that on `dump` multiple DPLL_A_DEVICE are provided,
>>on `do` only one.
>
>Please don't. This root nesting is not correct.
>
>
>>
>>Will try to fix it.
>>Although could you please explain why it make sense to put extra header
>>(exactly the same header) multiple times in one netlink response message?
>
>This is how it's done for all netlink dumps as far as I know.

So we just following something but we cannot explain why?

>The reason might be that the userspace is parsing exactly the same
>message as if it would be DOIT message.
>

This argument is achievable on both approaches.

[...]


>>>>>>>
>>>>>>>Hmm, shouldn't source-pin-index be here as well?
>>>>>>
>>>>>>No, there is no set for this.
>>>>>>For manual mode user selects the pin by setting enabled state on the
>one
>>>>>>he needs to recover signal from.
>>>>>>
>>>>>>source-pin-index is read only, returns active source.
>>>>>
>>>>>Okay, got it. Then why do we have this assymetric approach? Just have
>>>>>the enabled state to serve the user to see which one is selected, no?
>>>>>This would help to avoid confusion (like mine) and allow not to create
>>>>>inconsistencies (like no pin enabled yet driver to return some source
>>>>>pin index)
>>>>>
>>>>
>>>>This is due to automatic mode were multiple pins are enabled, but actual
>>>>selection is done on hardware level with priorities.
>>>
>>>Okay, this is confusing and I believe wrong.
>>>You have dual meaning for pin state attribute with states
>>>STATE_CONNECTED/DISCONNECTED:
>>>
>>>1) Manual mode, MUX pins (both share the same model):
>>>   There is only one pin with STATE_CONNECTED. The others are in
>>>   STATE_DISCONNECTED
>>>   User changes a state of a pin to make the selection.
>>>
>>>   Example:
>>>     $ dplltool pin dump
>>>       pin 1 state connected
>>>       pin 2 state disconnected
>>>     $ dplltool pin 2 set state connected
>>>     $ dplltool pin dump
>>>       pin 1 state disconnected
>>>       pin 2 state connected
>>>
>>>2) Automatic mode:
>>>   The user by setting "state" decides it the pin should be considered
>>>   by the device for auto selection.
>>>
>>>   Example:
>>>     $ dplltool pin dump:
>>>       pin 1 state connected prio 10
>>>       pin 2 state connected prio 15
>>>     $ dplltool dpll x get:
>>>       dpll x source-pin-index 1
>>>
>>>So in manual mode, STATE_CONNECTED means the dpll is connected to this
>>>source pin. However, in automatic mode it means something else. It means
>>>the user allows this pin to be considered for auto selection. The fact
>>>the pin is selected source is exposed over source-pin-index.
>>>
>>>Instead of this, I believe that the semantics of
>>>STATE_CONNECTED/DISCONNECTED should be the same for automatic mode as
>>>well. Unlike the manual mode/mux, where the state is written by user, in
>>>automatic mode the state should be only written by the driver. User
>>>attemts to set the state should fail with graceful explanation (DPLL
>>>netlink/core code should handle that, w/o driver interaction)
>>>
>>>Suggested automatic mode example:
>>>     $ dplltool pin dump:
>>>       pin 1 state connected prio 10 connectable true
>>>       pin 2 state disconnected prio 15 connectable true
>>>     $ dplltool pin 1 set connectable false
>>>     $ dplltool pin dump:
>>>       pin 1 state disconnected prio 10 connectable false
>>>       pin 2 state connected prio 15 connectable true
>>>     $ dplltool pin 1 set state connected
>>>       -EOPNOTSUPP
>>>
>>>Note there is no "source-pin-index" at all. Replaced by pin state here.
>>>There is a new attribute called "connectable", the user uses this
>>>attribute to tell the device, if this source pin could be considered for
>>>auto selection or not.
>>>
>>>Could be called perhaps "selectable", does not matter. The point is, the
>>>meaning of the "state" attribute is consistent for automatic mode,
>>>manual mode and mux pin.
>>>
>>>Makes sense?
>>>
>>
>>Great idea!
>>I will add third enum for pin-state: DPLL_PIN_STATE_SELECTABLE.
>>In the end we will have this:
>>              +--------------------------------+
>>              | valid DPLL_A_PIN_STATE values  |
>>	      +---------------+----------------+
>>+------------+| requested:    | returned:      |
>>|DPLL_A_MODE:||               |                |
>>|------------++--------------------------------|
>>|AUTOMATIC   ||- SELECTABLE   | - SELECTABLE   |
>>|            ||- DISCONNECTED | - DISCONNECTED |
>>|            ||               | - CONNECTED    |
>
>"selectable" is something the user sets.

Yes.

>"connected"/"disconnected" is in case of auto mode something that driver
>sets.
>

No. Not really.
"CONNECTED" is only set by driver once a pin is choosen.
"SELECTABLE" is set by the user if he needs to enable a pin for selection,
it is also default state of a pin if it was not selected ("CONNECTED")
"DISCONNECTED" is set by the user if he needs to disable a pin from selection.

>Looks a bit odd to mix them together. That is why I suggested
>to have sepectable as a separate attr. But up to you. Please make sure
>you sanitize the user/driver set of this attr in dpll code.
>

What is odd?
What do you mean by "sanitize the user/driver set of this attr in dpll code"?


Thank you,
Arkadiusz

>
>>|------------++--------------------------------|
>>|MANUAL      ||- CONNECTED    | - CONNECTED    |
>>|            ||- DISCONNECTED | - DISCONNECTED |
>>+------------++---------------+----------------+
>>
>>Thank you,
>>Arkadiusz
>>
>>>
>>>>
>>>>[...]
>>>>
>>>>>>>>+
>>>>>>>>+/* DPLL_CMD_DEVICE_SET - do */
>>>>>>>>+static const struct nla_policy
>dpll_device_set_nl_policy[DPLL_A_MODE +
>>>>>>>>1]
>>>>>>>>= {
>>>>>>>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>>>>>>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>>>>>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>>>>>>+	[DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),
>>>>>>>
>>>>>>>Hmm, any idea why the generator does not put define name
>>>>>>>here instead of "5"?
>>>>>>>
>>>>>>
>>>>>>Not really, it probably needs a fix for this.
>>>>>
>>>>>Yeah.
>>>>>
>>>>
>>>>Well, once we done with review maybe we could also fix those, or ask
>>>>Jakub if he could help :)
>>>>
>>>>
>>>>[...]
>>>>
>>>>>>>
>>>>>>>>+	DPLL_A_PIN_PRIO,
>>>>>>>>+	DPLL_A_PIN_STATE,
>>>>>>>>+	DPLL_A_PIN_PARENT,
>>>>>>>>+	DPLL_A_PIN_PARENT_IDX,
>>>>>>>>+	DPLL_A_PIN_RCLK_DEVICE,
>>>>>>>>+	DPLL_A_PIN_DPLL_CAPS,
>>>>>>>
>>>>>>>Just DPLL_A_PIN_CAPS is enough, that would be also consistent with the
>>>>>>>enum name.
>>>>>>
>>>>>>Sure, fixed.
>>>>>
>>>>>
>>>>>Thanks for all your work on this!
>>>>
>>>>Thanks for a great review! :)
>>>
>>>Glad to help.

Arkadiusz Kubalewski March 17, 2023, 6:50 p.m. UTC | #15

>From: Jiri Pirko <[e-mail]>
>Sent: Friday, March 17, 2023 5:54 PM
>
>Sun, Mar 12, 2023 at 03:28:02AM CET, [e-mail] wrote:
>>From: Arkadiusz Kubalewski <[e-mail]>
>>
>
>[...]
>
>
>>+      name: device-get
>>+      doc: |
>>+        Get list of DPLL devices (dump) or attributes of a single dpll
>>device
>>+      attribute-set: dpll
>>+      flags: [ admin-perm ]
>>+
>
>[...]
>
>
>>+    -
>>+      name: pin-get
>>+      doc: |
>>+        Get list of pins and its attributes.
>>+        - dump request without any attributes given - list all the pins
>>in the system
>>+        - dump request with target dpll - list all the pins registered
>>with a given dpll device
>>+        - do request with target dpll and target pin - single pin
>>attributes
>>+      attribute-set: dpll
>>+      flags: [ admin-perm ]
>
>Any particular reason to have admin cap required for get operations?
>If not, please remove.

Yes, security reasons, we don't want regular users to spam-query the driver
ops. Also explained in docs:
All netlink commands require ``GENL_ADMIN_PERM``. This is to prevent
any spamming/D.o.S. from unauthorized userspace applications.

Thank you,
Arkadiusz

Jiri Pirko March 20, 2023, 8:10 a.m. UTC | #16

Fri, Mar 17, 2023 at 07:22:46PM CET, [e-mail] wrote:
>>From: Jiri Pirko <[e-mail]>
>>Sent: Friday, March 17, 2023 5:21 PM
>>
>>Fri, Mar 17, 2023 at 04:14:45PM CET, [e-mail] wrote:
>>>
>>>>From: Jiri Pirko <[e-mail]>
>>>>Sent: Friday, March 17, 2023 11:05 AM
>>>>
>>>>Fri, Mar 17, 2023 at 01:52:44AM CET, [e-mail]
>>>>wrote:
>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>Sent: Thursday, March 16, 2023 2:45 PM
>>>>>>
>>>>>
>>>>>[...]
>>>>>
>>>>>>>>>+attribute-sets:
>>>>>>>>>+  -
>>>>>>>>>+    name: dpll
>>>>>>>>>+    enum-name: dplla
>>>>>>>>>+    attributes:
>>>>>>>>>+      -
>>>>>>>>>+        name: device
>>>>>>>>>+        type: nest
>>>>>>>>>+        value: 1
>>>>>>>>>+        multi-attr: true
>>>>>>>>>+        nested-attributes: device
>>>>>>>>
>>>>>>>>What is this "device" and what is it good for? Smells like some
>>>>>>>>leftover
>>>>>>>>and with the nested scheme looks quite odd.
>>>>>>>>
>>>>>>>
>>>>>>>No, it is nested attribute type, used when multiple devices are returned
>>>>>>>with netlink:
>>>>>>>
>>>>>>>- dump of device-get command where all devices are returned, each one
>>>>>>>nested
>>>>>>>inside it:
>>>>>>>[{'device': [{'bus-name': 'pci', 'dev-name': '0000:21:00.0_0', 'id':0},
>>>>>>>             {'bus-name': 'pci', 'dev-name': '0000:21:00.0_1', 'id':1}]}]
>>>>>>
>>>>>>Okay, why is it nested here? The is one netlink msg per dpll device
>>>>>>instance. Is this the real output of you made that up?
>>>>>>
>>>>>>Device nest should not be there for DEVICE_GET, does not make sense.
>>>>>>
>>>>>
>>>>>This was returned by CLI parser on ice with cmd:
>>>>>$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml /
>>>>>--dump device-get
>>>>>
>>>>>Please note this relates to 'dump' request , it is rather expected that
>>>>there
>>>>>are multiple dplls returned, thus we need a nest attribute for each one.
>>>>
>>>>No, you definitelly don't need to nest them. Dump format and get format
>>>>should be exactly the same. Please remove the nest.
>>>>
>>>>See how that is done in devlink for example: devlink_nl_fill()
>>>>This functions fills up one object in the dump. No nesting.
>>>>I'm not aware of such nesting approach anywhere in kernel dumps, does
>>>>not make sense at all.
>>>>
>>>
>>>Yeah it make sense to have same output on `do` and `dump`, but this is also
>>>achievable with nest DPLL_A_DEVICE, still don't need put extra header for it.
>>>The difference would be that on `dump` multiple DPLL_A_DEVICE are provided,
>>>on `do` only one.
>>
>>Please don't. This root nesting is not correct.
>>
>>
>>>
>>>Will try to fix it.
>>>Although could you please explain why it make sense to put extra header
>>>(exactly the same header) multiple times in one netlink response message?
>>
>>This is how it's done for all netlink dumps as far as I know.
>
>So we just following something but we cannot explain why?

I thought it is obvious. With what you suggest, each generic netlink
user would have to have all commands that implement both do and dump
with message format of a root nest. Not only that, for the sake of
consistency, all the rest of the commands would have to implement same
root nesting format. Both ways, to and from kernel.
So no matter what, all the messages would look like:

CMD_X MSG:
  SUBSYS_ATTR_ROOT
    SUBSYS_ATTR_1
    SUBSYS_ATTR_2
    ***
    SUBSYS_ATTR_N

What I say, this extra level of nesting is not needed and in fact it is
not good for anything. Remove it and just have:

CMD_X MSG:
  SUBSYS_ATTR_1
  SUBSYS_ATTR_2
  ***
  SUBSYS_ATTR_N

Clear and simple. For dump, you repeat the header, I don't see why that
is problem. All implementations are doing that, all userspace apps are
used to it. It is simple and consistent even if you consider multiple
skbs used for dump. I'm lacking to understand your need to reinvent
the wheel here.



>
>>The reason might be that the userspace is parsing exactly the same
>>message as if it would be DOIT message.
>>
>
>This argument is achievable on both approaches.
>
>[...]
>
>
>>>>>>>>
>>>>>>>>Hmm, shouldn't source-pin-index be here as well?
>>>>>>>
>>>>>>>No, there is no set for this.
>>>>>>>For manual mode user selects the pin by setting enabled state on the
>>one
>>>>>>>he needs to recover signal from.
>>>>>>>
>>>>>>>source-pin-index is read only, returns active source.
>>>>>>
>>>>>>Okay, got it. Then why do we have this assymetric approach? Just have
>>>>>>the enabled state to serve the user to see which one is selected, no?
>>>>>>This would help to avoid confusion (like mine) and allow not to create
>>>>>>inconsistencies (like no pin enabled yet driver to return some source
>>>>>>pin index)
>>>>>>
>>>>>
>>>>>This is due to automatic mode were multiple pins are enabled, but actual
>>>>>selection is done on hardware level with priorities.
>>>>
>>>>Okay, this is confusing and I believe wrong.
>>>>You have dual meaning for pin state attribute with states
>>>>STATE_CONNECTED/DISCONNECTED:
>>>>
>>>>1) Manual mode, MUX pins (both share the same model):
>>>>   There is only one pin with STATE_CONNECTED. The others are in
>>>>   STATE_DISCONNECTED
>>>>   User changes a state of a pin to make the selection.
>>>>
>>>>   Example:
>>>>     $ dplltool pin dump
>>>>       pin 1 state connected
>>>>       pin 2 state disconnected
>>>>     $ dplltool pin 2 set state connected
>>>>     $ dplltool pin dump
>>>>       pin 1 state disconnected
>>>>       pin 2 state connected
>>>>
>>>>2) Automatic mode:
>>>>   The user by setting "state" decides it the pin should be considered
>>>>   by the device for auto selection.
>>>>
>>>>   Example:
>>>>     $ dplltool pin dump:
>>>>       pin 1 state connected prio 10
>>>>       pin 2 state connected prio 15
>>>>     $ dplltool dpll x get:
>>>>       dpll x source-pin-index 1
>>>>
>>>>So in manual mode, STATE_CONNECTED means the dpll is connected to this
>>>>source pin. However, in automatic mode it means something else. It means
>>>>the user allows this pin to be considered for auto selection. The fact
>>>>the pin is selected source is exposed over source-pin-index.
>>>>
>>>>Instead of this, I believe that the semantics of
>>>>STATE_CONNECTED/DISCONNECTED should be the same for automatic mode as
>>>>well. Unlike the manual mode/mux, where the state is written by user, in
>>>>automatic mode the state should be only written by the driver. User
>>>>attemts to set the state should fail with graceful explanation (DPLL
>>>>netlink/core code should handle that, w/o driver interaction)
>>>>
>>>>Suggested automatic mode example:
>>>>     $ dplltool pin dump:
>>>>       pin 1 state connected prio 10 connectable true
>>>>       pin 2 state disconnected prio 15 connectable true
>>>>     $ dplltool pin 1 set connectable false
>>>>     $ dplltool pin dump:
>>>>       pin 1 state disconnected prio 10 connectable false
>>>>       pin 2 state connected prio 15 connectable true
>>>>     $ dplltool pin 1 set state connected
>>>>       -EOPNOTSUPP
>>>>
>>>>Note there is no "source-pin-index" at all. Replaced by pin state here.
>>>>There is a new attribute called "connectable", the user uses this
>>>>attribute to tell the device, if this source pin could be considered for
>>>>auto selection or not.
>>>>
>>>>Could be called perhaps "selectable", does not matter. The point is, the
>>>>meaning of the "state" attribute is consistent for automatic mode,
>>>>manual mode and mux pin.
>>>>
>>>>Makes sense?
>>>>
>>>
>>>Great idea!
>>>I will add third enum for pin-state: DPLL_PIN_STATE_SELECTABLE.
>>>In the end we will have this:
>>>              +--------------------------------+
>>>              | valid DPLL_A_PIN_STATE values  |
>>>	      +---------------+----------------+
>>>+------------+| requested:    | returned:      |
>>>|DPLL_A_MODE:||               |                |
>>>|------------++--------------------------------|
>>>|AUTOMATIC   ||- SELECTABLE   | - SELECTABLE   |
>>>|            ||- DISCONNECTED | - DISCONNECTED |
>>>|            ||               | - CONNECTED    |
>>
>>"selectable" is something the user sets.
>
>Yes.
>
>>"connected"/"disconnected" is in case of auto mode something that driver
>>sets.
>>
>
>No. Not really.
>"CONNECTED" is only set by driver once a pin is choosen.
>"SELECTABLE" is set by the user if he needs to enable a pin for selection,
>it is also default state of a pin if it was not selected ("CONNECTED")
>"DISCONNECTED" is set by the user if he needs to disable a pin from selection.

Sure.


>
>>Looks a bit odd to mix them together. That is why I suggested
>>to have sepectable as a separate attr. But up to you. Please make sure
>>you sanitize the user/driver set of this attr in dpll code.
>>
>
>What is odd?

To mix them together in a single attr. But I'm fine with that.


>What do you mean by "sanitize the user/driver set of this attr in dpll code"?

What I mean is that in each mode there has to be clearly documented
which entity changes state, how and under which circumstance. This
behaviour needs to be enforced in the dpll code.


>
>
>Thank you,
>Arkadiusz
>
>>
>>>|------------++--------------------------------|
>>>|MANUAL      ||- CONNECTED    | - CONNECTED    |
>>>|            ||- DISCONNECTED | - DISCONNECTED |
>>>+------------++---------------+----------------+
>>>
>>>Thank you,
>>>Arkadiusz
>>>
>>>>
>>>>>
>>>>>[...]
>>>>>
>>>>>>>>>+
>>>>>>>>>+/* DPLL_CMD_DEVICE_SET - do */
>>>>>>>>>+static const struct nla_policy
>>dpll_device_set_nl_policy[DPLL_A_MODE +
>>>>>>>>>1]
>>>>>>>>>= {
>>>>>>>>>+	[DPLL_A_ID] = { .type = NLA_U32, },
>>>>>>>>>+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
>>>>>>>>>+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
>>>>>>>>>+	[DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),
>>>>>>>>
>>>>>>>>Hmm, any idea why the generator does not put define name
>>>>>>>>here instead of "5"?
>>>>>>>>
>>>>>>>
>>>>>>>Not really, it probably needs a fix for this.
>>>>>>
>>>>>>Yeah.
>>>>>>
>>>>>
>>>>>Well, once we done with review maybe we could also fix those, or ask
>>>>>Jakub if he could help :)
>>>>>
>>>>>
>>>>>[...]
>>>>>
>>>>>>>>
>>>>>>>>>+	DPLL_A_PIN_PRIO,
>>>>>>>>>+	DPLL_A_PIN_STATE,
>>>>>>>>>+	DPLL_A_PIN_PARENT,
>>>>>>>>>+	DPLL_A_PIN_PARENT_IDX,
>>>>>>>>>+	DPLL_A_PIN_RCLK_DEVICE,
>>>>>>>>>+	DPLL_A_PIN_DPLL_CAPS,
>>>>>>>>
>>>>>>>>Just DPLL_A_PIN_CAPS is enough, that would be also consistent with the
>>>>>>>>enum name.
>>>>>>>
>>>>>>>Sure, fixed.
>>>>>>
>>>>>>
>>>>>>Thanks for all your work on this!
>>>>>
>>>>>Thanks for a great review! :)
>>>>
>>>>Glad to help.

Jakub Kicinski March 21, 2023, 4 a.m. UTC | #17

On Fri, 17 Mar 2023 17:23:33 +0100 Jiri Pirko wrote:
> >+/* Ops table for dpll */
> >+static const struct genl_split_ops dpll_nl_ops[6] = {  
> 
> It's odd to see 6 here. Should by just []
> But it's an issue of Jakub's generator.

In some modes of generation the array is visible outside the source file
i.e. there's a forward declaration in the header. So I had to get the
counting right, anyway. But sure, I'll send a "fix"..

Jakub Kicinski March 21, 2023, 4:05 a.m. UTC | #18

On Thu, 16 Mar 2023 13:15:59 +0000 Kubalewski, Arkadiusz wrote:
> >Value 23? What's this?
> >You have it specified for some attrs all over the place.
> >What is the reason for it?
> >  
> 
> Actually this particular one is not needed (also value: 12 on pin above),
> I will remove those.
> But the others you are refering to (the ones in nested attribute list),
> are required because of cli.py parser issue, maybe Kuba knows a better way to
> prevent the issue?
> Basically, without those values, cli.py brakes on parsing responses, after
> every "jump" to nested attribute list it is assigning first attribute there
> with value=0, thus there is a need to assign a proper value, same as it is on
> 'main' attribute list.

Are you saying the parser gets confused after returning from nested
parsing? Can you still repro this problem? I don't see any global
state in _decode()..

Jakub Kicinski March 21, 2023, 4:13 a.m. UTC | #19

On Mon, 20 Mar 2023 21:05:49 -0700 Jakub Kicinski wrote:
> > Actually this particular one is not needed (also value: 12 on pin above),
> > I will remove those.
> > But the others you are refering to (the ones in nested attribute list),
> > are required because of cli.py parser issue, maybe Kuba knows a better way to
> > prevent the issue?
> > Basically, without those values, cli.py brakes on parsing responses, after
> > every "jump" to nested attribute list it is assigning first attribute there
> > with value=0, thus there is a need to assign a proper value, same as it is on
> > 'main' attribute list.  
> 
> Are you saying the parser gets confused after returning from nested
> parsing? Can you still repro this problem? I don't see any global
> state in _decode()..

Oh, I grokked the attr structure. This should be fixed now.
Commit 7cf93538e087 ("tools: ynl: fully inherit attrs in subsets")

The entire attr structure is a bit off (/ inspired by devlink?)
Why create one huge attribute space with all attributes in it?
Try to find something in the devlink attrs, it's pages long with 
little to no reuse :| Can't pins and devices have their own spaces?

If one has to carry a link to the other you should create a subset
so there is no circular dependency. That's the only potentially tricky
thing I can think of...

Jakub Kicinski March 21, 2023, 4:20 a.m. UTC | #20

On Mon, 20 Mar 2023 21:13:54 -0700 Jakub Kicinski wrote:
> If one has to carry a link to the other you should create a subset
> so there is no circular dependency. That's the only potentially tricky
> thing I can think of...

Another option is to take a page from ethtool's book and factor out
identifiers to a nested attr from the start.

Patch

diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
new file mode 100644
index 000000000000..03e9f0e2d3d2
--- /dev/null
+++ b/Documentation/netlink/specs/dpll.yaml
@@ -0,0 +1,514 @@ 
+name: dpll
+
+doc: DPLL subsystem.
+
+definitions:
+  -
+    type: const
+    name: temp-divider
+    value: 10
+  -
+    type: const
+    name: pin-freq-1-hz
+    value: 1
+  -
+    type: const
+    name: pin-freq-10-mhz
+    value: 10000000
+  -
+    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 (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
+    render-max: true
+  -
+    type: enum
+    name: pin-type
+    doc: Enumerates types of a pin, valid values for DPLL_A_PIN_TYPE attribute
+    entries:
+      -
+        name: unspec
+        doc: unspecified value
+      -
+        name: mux
+        doc: aggregates another layer of selectable pins
+      -
+        name: ext
+        doc: external source
+      -
+        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-state
+    doc: available pin modes
+    entries:
+      -
+        name: unspec
+        doc: unspecified value
+      -
+        name: connected
+        doc: pin connected
+      -
+        name: disconnected
+        doc: pin disconnected
+    render-max: true
+  -
+    type: enum
+    name: pin-direction
+    doc: available pin direction
+    entries:
+      -
+        name: unspec
+        doc: unspecified value
+      -
+        name: source
+        doc: pin used as a source of a signal
+      -
+        name: output
+        doc: pin used to output the signal
+    render-max: true
+  -
+    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
+    entries:
+      -
+        name: unspec
+        doc: unspecified value
+      -
+        name: manual
+        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
+      -
+        name: nco
+        doc: dpll driven by Numerically Controlled Oscillator
+    render-max: true
+  -
+    type: enum
+    name: type
+    doc: type of dpll, valid values for DPLL_A_TYPE attribute
+    entries:
+      -
+        name: unspec
+        doc: unspecified value
+      -
+        name: pps
+        doc: dpll produces Pulse-Per-Second signal
+      -
+        name: eec
+        doc: dpll drives the Ethernet Equipment Clock
+    render-max: true
+  -
+    type: enum
+    name: event
+    doc: events of dpll generic netlink family
+    entries:
+      -
+        name: unspec
+        doc: invalid event type
+      -
+        name: device-create
+        doc: dpll device created
+      -
+        name: device-delete
+        doc: dpll device deleted
+      -
+        name: device-change
+        doc: |
+          attribute of dpll device or pin changed, reason is to be found with
+          an attribute type (DPLL_A_*) received with the event
+  -
+    type: flags
+    name: pin-caps
+    doc: define capabilities of a pin
+    entries:
+      -
+        name: direction-can-change
+      -
+        name: priority-can-change
+      -
+        name: state-can-change
+
+
+attribute-sets:
+  -
+    name: dpll
+    enum-name: dplla
+    attributes:
+      -
+        name: device
+        type: nest
+        value: 1
+        multi-attr: true
+        nested-attributes: device
+      -
+        name: id
+        type: u32
+      -
+        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: source-pin-idx
+        type: u32
+      -
+        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
+        type: nest
+        multi-attr: true
+        nested-attributes: pin
+        value: 12
+      -
+        name: pin-idx
+        type: u32
+      -
+        name: pin-description
+        type: string
+      -
+        name: pin-type
+        type: u8
+        enum: pin-type
+      -
+        name: pin-direction
+        type: u8
+        enum: pin-direction
+      -
+        name: pin-frequency
+        type: u32
+      -
+        name: pin-frequency-supported
+        type: u32
+        multi-attr: true
+      -
+        name: pin-any-frequency-min
+        type: u32
+      -
+        name: pin-any-frequency-max
+        type: u32
+      -
+        name: pin-prio
+        type: u32
+      -
+        name: pin-state
+        type: u8
+        enum: pin-state
+      -
+        name: pin-parent
+        type: nest
+        multi-attr: true
+        nested-attributes: pin
+        value: 23
+      -
+        name: pin-parent-idx
+        type: u32
+      -
+        name: pin-rclk-device
+        type: string
+      -
+        name: pin-dpll-caps
+        type: u32
+  -
+    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: source-pin-idx
+        type: u32
+      -
+        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
+        type: nest
+        value: 12
+        multi-attr: true
+        nested-attributes: pin
+      -
+        name: pin-prio
+        type: u32
+        value: 21
+      -
+        name: pin-state
+        type: u8
+        enum: pin-state
+      -
+        name: pin-dpll-caps
+        type: u32
+        value: 26
+  -
+    name: pin
+    subset-of: dpll
+    attributes:
+      -
+        name: device
+        type: nest
+        value: 1
+        multi-attr: true
+        nested-attributes: device
+      -
+        name: pin-idx
+        type: u32
+        value: 13
+      -
+        name: pin-description
+        type: string
+      -
+        name: pin-type
+        type: u8
+        enum: pin-type
+      -
+        name: pin-direction
+        type: u8
+        enum: pin-direction
+      -
+        name: pin-frequency
+        type: u32
+      -
+        name: pin-frequency-supported
+        type: u32
+        multi-attr: true
+      -
+        name: pin-any-frequency-min
+        type: u32
+      -
+        name: pin-any-frequency-max
+        type: u32
+      -
+        name: pin-prio
+        type: u32
+      -
+        name: pin-state
+        type: u8
+        enum: pin-state
+      -
+        name: pin-parent
+        type: nest
+        multi-attr: true
+        nested-attributes: pin-parent
+        value: 23
+      -
+        name: pin-rclk-device
+        type: string
+        value: 25
+      -
+        name: pin-dpll-caps
+        type: u32
+  -
+    name: pin-parent
+    subset-of: dpll
+    attributes:
+      -
+        name: pin-state
+        type: u8
+        value: 22
+        enum: pin-state
+      -
+        name: pin-parent-idx
+        type: u32
+        value: 24
+      -
+        name: pin-rclk-device
+        type: string
+
+
+operations:
+  list:
+    -
+      name: unspec
+      doc: unused
+
+    -
+      name: device-get
+      doc: |
+        Get list of DPLL devices (dump) or attributes of a single dpll device
+      attribute-set: dpll
+      flags: [ admin-perm ]
+
+      do:
+        pre: dpll-pre-doit
+        post: dpll-post-doit
+        request:
+          attributes:
+            - id
+            - bus-name
+            - dev-name
+        reply:
+          attributes:
+            - device
+
+      dump:
+        pre: dpll-pre-dumpit
+        post: dpll-post-dumpit
+        reply:
+          attributes:
+            - device
+
+    -
+      name: device-set
+      doc: Set attributes for a DPLL device
+      attribute-set: dpll
+      flags: [ admin-perm ]
+
+      do:
+        pre: dpll-pre-doit
+        post: dpll-post-doit
+        request:
+          attributes:
+            - id
+            - bus-name
+            - dev-name
+            - mode
+
+    -
+      name: pin-get
+      doc: |
+        Get list of pins and its attributes.
+        - dump request without any attributes given - list all the pins in the system
+        - dump request with target dpll - list all the pins registered with a given dpll device
+        - do request with target dpll and target pin - single pin attributes
+      attribute-set: dpll
+      flags: [ admin-perm ]
+
+      do:
+        pre: dpll-pin-pre-doit
+        post: dpll-pin-post-doit
+        request:
+          attributes:
+            - id
+            - bus-name
+            - dev-name
+            - pin-idx
+        reply:
+          attributes:
+            - pin
+
+      dump:
+        pre: dpll-pin-pre-dumpit
+        post: dpll-pin-post-dumpit
+        request:
+          attributes:
+            - id
+            - bus-name
+            - dev-name
+        reply:
+          attributes:
+            - pin
+
+    -
+      name: pin-set
+      doc: Set attributes of a target pin
+      attribute-set: dpll
+      flags: [ admin-perm ]
+
+      do:
+        pre: dpll-pin-pre-doit
+        post: dpll-pin-post-doit
+        request:
+          attributes:
+            - id
+            - bus-name
+            - dev-name
+            - pin-idx
+            - pin-frequency
+            - pin-direction
+            - pin-prio
+            - pin-parent-idx
+            - pin-state
+
+mcast-groups:
+  list:
+    -
+      name: monitor
diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c
new file mode 100644
index 000000000000..099d1e30ca7c
--- /dev/null
+++ b/drivers/dpll/dpll_nl.c
@@ -0,0 +1,126 @@ 
+// SPDX-License-Identifier: BSD-3-Clause
+/* Do not edit directly, auto-generated from: */
+/*	Documentation/netlink/specs/dpll.yaml */
+/* YNL-GEN kernel source */
+
+#include <net/netlink.h>
+#include <net/genetlink.h>
+
+#include "dpll_nl.h"
+
+#include <linux/dpll.h>
+
+/* DPLL_CMD_DEVICE_GET - do */
+static const struct nla_policy dpll_device_get_nl_policy[DPLL_A_BUS_NAME + 1] = {
+	[DPLL_A_ID] = { .type = NLA_U32, },
+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
+};
+
+/* DPLL_CMD_DEVICE_SET - do */
+static const struct nla_policy dpll_device_set_nl_policy[DPLL_A_MODE + 1] = {
+	[DPLL_A_ID] = { .type = NLA_U32, },
+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
+	[DPLL_A_MODE] = NLA_POLICY_MAX(NLA_U8, 5),
+};
+
+/* DPLL_CMD_PIN_GET - do */
+static const struct nla_policy dpll_pin_get_do_nl_policy[DPLL_A_PIN_IDX + 1] = {
+	[DPLL_A_ID] = { .type = NLA_U32, },
+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
+	[DPLL_A_PIN_IDX] = { .type = NLA_U32, },
+};
+
+/* DPLL_CMD_PIN_GET - dump */
+static const struct nla_policy dpll_pin_get_dump_nl_policy[DPLL_A_BUS_NAME + 1] = {
+	[DPLL_A_ID] = { .type = NLA_U32, },
+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
+};
+
+/* DPLL_CMD_PIN_SET - do */
+static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PARENT_IDX + 1] = {
+	[DPLL_A_ID] = { .type = NLA_U32, },
+	[DPLL_A_BUS_NAME] = { .type = NLA_NUL_STRING, },
+	[DPLL_A_DEV_NAME] = { .type = NLA_NUL_STRING, },
+	[DPLL_A_PIN_IDX] = { .type = NLA_U32, },
+	[DPLL_A_PIN_FREQUENCY] = { .type = NLA_U32, },
+	[DPLL_A_PIN_DIRECTION] = NLA_POLICY_MAX(NLA_U8, 2),
+	[DPLL_A_PIN_PRIO] = { .type = NLA_U32, },
+	[DPLL_A_PIN_PARENT_IDX] = { .type = NLA_U32, },
+	[DPLL_A_PIN_STATE] = NLA_POLICY_MAX(NLA_U8, 2),
+};
+
+/* Ops table for dpll */
+static const struct genl_split_ops dpll_nl_ops[6] = {
+	{
+		.cmd		= DPLL_CMD_DEVICE_GET,
+		.pre_doit	= dpll_pre_doit,
+		.doit		= dpll_nl_device_get_doit,
+		.post_doit	= dpll_post_doit,
+		.policy		= dpll_device_get_nl_policy,
+		.maxattr	= DPLL_A_BUS_NAME,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+	},
+	{
+		.cmd	= DPLL_CMD_DEVICE_GET,
+		.start	= dpll_pre_dumpit,
+		.dumpit	= dpll_nl_device_get_dumpit,
+		.done	= dpll_post_dumpit,
+		.flags	= GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
+	},
+	{
+		.cmd		= DPLL_CMD_DEVICE_SET,
+		.pre_doit	= dpll_pre_doit,
+		.doit		= dpll_nl_device_set_doit,
+		.post_doit	= dpll_post_doit,
+		.policy		= dpll_device_set_nl_policy,
+		.maxattr	= DPLL_A_MODE,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+	},
+	{
+		.cmd		= DPLL_CMD_PIN_GET,
+		.pre_doit	= dpll_pin_pre_doit,
+		.doit		= dpll_nl_pin_get_doit,
+		.post_doit	= dpll_pin_post_doit,
+		.policy		= dpll_pin_get_do_nl_policy,
+		.maxattr	= DPLL_A_PIN_IDX,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+	},
+	{
+		.cmd		= DPLL_CMD_PIN_GET,
+		.start		= dpll_pin_pre_dumpit,
+		.dumpit		= dpll_nl_pin_get_dumpit,
+		.done		= dpll_pin_post_dumpit,
+		.policy		= dpll_pin_get_dump_nl_policy,
+		.maxattr	= DPLL_A_BUS_NAME,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DUMP,
+	},
+	{
+		.cmd		= DPLL_CMD_PIN_SET,
+		.pre_doit	= dpll_pin_pre_doit,
+		.doit		= dpll_nl_pin_set_doit,
+		.post_doit	= dpll_pin_post_doit,
+		.policy		= dpll_pin_set_nl_policy,
+		.maxattr	= DPLL_A_PIN_PARENT_IDX,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+	},
+};
+
+static const struct genl_multicast_group dpll_nl_mcgrps[] = {
+	[DPLL_NLGRP_MONITOR] = { "monitor", },
+};
+
+struct genl_family dpll_nl_family __ro_after_init = {
+	.name		= DPLL_FAMILY_NAME,
+	.version	= DPLL_FAMILY_VERSION,
+	.netnsok	= true,
+	.parallel_ops	= true,
+	.module		= THIS_MODULE,
+	.split_ops	= dpll_nl_ops,
+	.n_split_ops	= ARRAY_SIZE(dpll_nl_ops),
+	.mcgrps		= dpll_nl_mcgrps,
+	.n_mcgrps	= ARRAY_SIZE(dpll_nl_mcgrps),
+};
diff --git a/drivers/dpll/dpll_nl.h b/drivers/dpll/dpll_nl.h
new file mode 100644
index 000000000000..3a354dfb9a31
--- /dev/null
+++ b/drivers/dpll/dpll_nl.h
@@ -0,0 +1,42 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause */
+/* Do not edit directly, auto-generated from: */
+/*	Documentation/netlink/specs/dpll.yaml */
+/* YNL-GEN kernel header */
+
+#ifndef _LINUX_DPLL_GEN_H
+#define _LINUX_DPLL_GEN_H
+
+#include <net/netlink.h>
+#include <net/genetlink.h>
+
+#include <linux/dpll.h>
+
+int dpll_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
+		  struct genl_info *info);
+int dpll_pin_pre_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
+		      struct genl_info *info);
+void
+dpll_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
+	       struct genl_info *info);
+void
+dpll_pin_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb,
+		   struct genl_info *info);
+int dpll_pre_dumpit(struct netlink_callback *cb);
+int dpll_pin_pre_dumpit(struct netlink_callback *cb);
+int dpll_post_dumpit(struct netlink_callback *cb);
+int dpll_pin_post_dumpit(struct netlink_callback *cb);
+
+int dpll_nl_device_get_doit(struct sk_buff *skb, struct genl_info *info);
+int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
+int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info);
+int dpll_nl_pin_get_doit(struct sk_buff *skb, struct genl_info *info);
+int dpll_nl_pin_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
+int dpll_nl_pin_set_doit(struct sk_buff *skb, struct genl_info *info);
+
+enum {
+	DPLL_NLGRP_MONITOR,
+};
+
+extern struct genl_family dpll_nl_family;
+
+#endif /* _LINUX_DPLL_GEN_H */
diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
new file mode 100644
index 000000000000..ece6fe685d08
--- /dev/null
+++ b/include/uapi/linux/dpll.h
@@ -0,0 +1,196 @@ 
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/* Do not edit directly, auto-generated from: */
+/*	Documentation/netlink/specs/dpll.yaml */
+/* YNL-GEN uapi header */
+
+#ifndef _UAPI_LINUX_DPLL_H
+#define _UAPI_LINUX_DPLL_H
+
+#define DPLL_FAMILY_NAME	"dpll"
+#define DPLL_FAMILY_VERSION	1
+
+#define DPLL_TEMP_DIVIDER	10
+#define DPLL_PIN_FREQ_1_HZ	1
+#define DPLL_PIN_FREQ_10_MHZ	10000000
+
+/**
+ * enum dpll_lock_status - Provides information of dpll device lock status,
+ *   valid values for DPLL_A_LOCK_STATUS attribute
+ * @DPLL_LOCK_STATUS_UNSPEC: unspecified value
+ * @DPLL_LOCK_STATUS_UNLOCKED: dpll was not yet locked to any valid (or is in
+ *   one of modes: DPLL_MODE_FREERUN, DPLL_MODE_NCO)
+ * @DPLL_LOCK_STATUS_CALIBRATING: dpll is trying to lock to a valid signal
+ * @DPLL_LOCK_STATUS_LOCKED: dpll is locked
+ * @DPLL_LOCK_STATUS_HOLDOVER: dpll is in holdover state - lost a valid lock or
+ *   was forced by selecting DPLL_MODE_HOLDOVER mode
+ */
+enum dpll_lock_status {
+	DPLL_LOCK_STATUS_UNSPEC,
+	DPLL_LOCK_STATUS_UNLOCKED,
+	DPLL_LOCK_STATUS_CALIBRATING,
+	DPLL_LOCK_STATUS_LOCKED,
+	DPLL_LOCK_STATUS_HOLDOVER,
+
+	__DPLL_LOCK_STATUS_MAX,
+	DPLL_LOCK_STATUS_MAX = (__DPLL_LOCK_STATUS_MAX - 1)
+};
+
+/**
+ * enum dpll_pin_type - Enumerates types of a pin, valid values for
+ *   DPLL_A_PIN_TYPE attribute
+ * @DPLL_PIN_TYPE_UNSPEC: unspecified value
+ * @DPLL_PIN_TYPE_MUX: aggregates another layer of selectable pins
+ * @DPLL_PIN_TYPE_EXT: external source
+ * @DPLL_PIN_TYPE_SYNCE_ETH_PORT: ethernet port PHY's recovered clock
+ * @DPLL_PIN_TYPE_INT_OSCILLATOR: device internal oscillator
+ * @DPLL_PIN_TYPE_GNSS: GNSS recovered clock
+ */
+enum dpll_pin_type {
+	DPLL_PIN_TYPE_UNSPEC,
+	DPLL_PIN_TYPE_MUX,
+	DPLL_PIN_TYPE_EXT,
+	DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+	DPLL_PIN_TYPE_INT_OSCILLATOR,
+	DPLL_PIN_TYPE_GNSS,
+
+	__DPLL_PIN_TYPE_MAX,
+	DPLL_PIN_TYPE_MAX = (__DPLL_PIN_TYPE_MAX - 1)
+};
+
+/**
+ * enum dpll_pin_state - available pin modes
+ * @DPLL_PIN_STATE_UNSPEC: unspecified value
+ * @DPLL_PIN_STATE_CONNECTED: pin connected
+ * @DPLL_PIN_STATE_DISCONNECTED: pin disconnected
+ */
+enum dpll_pin_state {
+	DPLL_PIN_STATE_UNSPEC,
+	DPLL_PIN_STATE_CONNECTED,
+	DPLL_PIN_STATE_DISCONNECTED,
+
+	__DPLL_PIN_STATE_MAX,
+	DPLL_PIN_STATE_MAX = (__DPLL_PIN_STATE_MAX - 1)
+};
+
+/**
+ * enum dpll_pin_direction - available pin direction
+ * @DPLL_PIN_DIRECTION_UNSPEC: unspecified value
+ * @DPLL_PIN_DIRECTION_SOURCE: pin used as a source of a signal
+ * @DPLL_PIN_DIRECTION_OUTPUT: pin used to output the signal
+ */
+enum dpll_pin_direction {
+	DPLL_PIN_DIRECTION_UNSPEC,
+	DPLL_PIN_DIRECTION_SOURCE,
+	DPLL_PIN_DIRECTION_OUTPUT,
+
+	__DPLL_PIN_DIRECTION_MAX,
+	DPLL_PIN_DIRECTION_MAX = (__DPLL_PIN_DIRECTION_MAX - 1)
+};
+
+/**
+ * enum dpll_mode - working-modes a dpll can support, differentiate if and how
+ *   dpll selects one of its sources to syntonize with it
+ * @DPLL_MODE_UNSPEC: unspecified value
+ * @DPLL_MODE_MANUAL: source can be only selected by sending a request to dpll
+ * @DPLL_MODE_AUTOMATIC: highest prio, valid source, auto selected by dpll
+ * @DPLL_MODE_HOLDOVER: dpll forced into holdover mode
+ * @DPLL_MODE_FREERUN: dpll driven on system clk, no holdover available
+ * @DPLL_MODE_NCO: dpll driven by Numerically Controlled Oscillator
+ */
+enum dpll_mode {
+	DPLL_MODE_UNSPEC,
+	DPLL_MODE_MANUAL,
+	DPLL_MODE_AUTOMATIC,
+	DPLL_MODE_HOLDOVER,
+	DPLL_MODE_FREERUN,
+	DPLL_MODE_NCO,
+
+	__DPLL_MODE_MAX,
+	DPLL_MODE_MAX = (__DPLL_MODE_MAX - 1)
+};
+
+/**
+ * enum dpll_type - type of dpll, valid values for DPLL_A_TYPE attribute
+ * @DPLL_TYPE_UNSPEC: unspecified value
+ * @DPLL_TYPE_PPS: dpll produces Pulse-Per-Second signal
+ * @DPLL_TYPE_EEC: dpll drives the Ethernet Equipment Clock
+ */
+enum dpll_type {
+	DPLL_TYPE_UNSPEC,
+	DPLL_TYPE_PPS,
+	DPLL_TYPE_EEC,
+
+	__DPLL_TYPE_MAX,
+	DPLL_TYPE_MAX = (__DPLL_TYPE_MAX - 1)
+};
+
+/**
+ * enum dpll_event - events of dpll generic netlink family
+ * @DPLL_EVENT_UNSPEC: invalid event type
+ * @DPLL_EVENT_DEVICE_CREATE: dpll device created
+ * @DPLL_EVENT_DEVICE_DELETE: dpll device deleted
+ * @DPLL_EVENT_DEVICE_CHANGE: attribute of dpll device or pin changed, reason
+ *   is to be found with an attribute type (DPLL_A_*) received with the event
+ */
+enum dpll_event {
+	DPLL_EVENT_UNSPEC,
+	DPLL_EVENT_DEVICE_CREATE,
+	DPLL_EVENT_DEVICE_DELETE,
+	DPLL_EVENT_DEVICE_CHANGE,
+};
+
+/**
+ * enum dpll_pin_caps - define capabilities of a pin
+ */
+enum dpll_pin_caps {
+	DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE = 1,
+	DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE = 2,
+	DPLL_PIN_CAPS_STATE_CAN_CHANGE = 4,
+};
+
+enum dplla {
+	DPLL_A_DEVICE = 1,
+	DPLL_A_ID,
+	DPLL_A_DEV_NAME,
+	DPLL_A_BUS_NAME,
+	DPLL_A_MODE,
+	DPLL_A_MODE_SUPPORTED,
+	DPLL_A_SOURCE_PIN_IDX,
+	DPLL_A_LOCK_STATUS,
+	DPLL_A_TEMP,
+	DPLL_A_CLOCK_ID,
+	DPLL_A_TYPE,
+	DPLL_A_PIN,
+	DPLL_A_PIN_IDX,
+	DPLL_A_PIN_DESCRIPTION,
+	DPLL_A_PIN_TYPE,
+	DPLL_A_PIN_DIRECTION,
+	DPLL_A_PIN_FREQUENCY,
+	DPLL_A_PIN_FREQUENCY_SUPPORTED,
+	DPLL_A_PIN_ANY_FREQUENCY_MIN,
+	DPLL_A_PIN_ANY_FREQUENCY_MAX,
+	DPLL_A_PIN_PRIO,
+	DPLL_A_PIN_STATE,
+	DPLL_A_PIN_PARENT,
+	DPLL_A_PIN_PARENT_IDX,
+	DPLL_A_PIN_RCLK_DEVICE,
+	DPLL_A_PIN_DPLL_CAPS,
+
+	__DPLL_A_MAX,
+	DPLL_A_MAX = (__DPLL_A_MAX - 1)
+};
+
+enum {
+	DPLL_CMD_UNSPEC,
+	DPLL_CMD_DEVICE_GET,
+	DPLL_CMD_DEVICE_SET,
+	DPLL_CMD_PIN_GET,
+	DPLL_CMD_PIN_SET,
+
+	__DPLL_CMD_MAX,
+	DPLL_CMD_MAX = (__DPLL_CMD_MAX - 1)
+};
+
+#define DPLL_MCGRP_MONITOR	"monitor"
+
+#endif /* _UAPI_LINUX_DPLL_H */