| Message ID | 20230428002009.2948020-2-vadfed@meta.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Create common DPLL configuration API | expand |
Fri, Apr 28, 2023 at 02:20:02AM CEST, vadfed@meta.com wrote: >From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> > >Add a protocol spec for DPLL. >Add code generated from the spec. > >Signed-off-by: Jakub Kicinski <kuba@kernel.org> >Signed-off-by: Michal Michalik <michal.michalik@intel.com> >Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >--- > Documentation/netlink/specs/dpll.yaml | 472 ++++++++++++++++++++++++++ > drivers/dpll/dpll_nl.c | 126 +++++++ > drivers/dpll/dpll_nl.h | 42 +++ > include/uapi/linux/dpll.h | 202 +++++++++++ > 4 files changed, 842 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..67ca0f6cf2d5 >--- /dev/null >+++ b/Documentation/netlink/specs/dpll.yaml >@@ -0,0 +1,472 @@ >+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) >+ >+name: dpll >+ >+doc: DPLL subsystem. >+ >+definitions: >+ - >+ type: enum >+ name: mode >+ doc: | >+ working-modes a dpll can support, differentiate if and how dpll selects >+ one of its sources to syntonize with it, valid values for DPLL_A_MODE >+ attribute >+ entries: >+ - >+ name: unspec In general, why exactly do we need unspec values in enums and CMDs? What is the usecase. If there isn't please remove. >+ 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 Remove "no holdover available". This is not a state, this is a mode configuration. If holdover is or isn't available, is a runtime info. >+ - >+ name: nco >+ doc: dpll driven by Numerically Controlled Oscillator >+ render-max: true >+ - >+ type: enum >+ name: lock-status >+ doc: | >+ provides information of dpll device lock status, valid values for >+ DPLL_A_LOCK_STATUS attribute >+ entries: >+ - >+ name: unspec >+ doc: unspecified value >+ - >+ name: unlocked >+ doc: | >+ dpll was not yet locked to any valid source (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 Is it needed to mention the holdover mode. It's slightly confusing, because user might understand that the lock-status is always "holdover" in case of "holdover" mode. But it could be "unlocked", can't it? Perhaps I don't understand the flows there correctly :/ >+ render-max: true >+ - >+ type: const >+ name: temp-divider >+ value: 10 >+ doc: | >+ temperature divider allowing userspace to calculate the >+ temperature as float with single digit precision. >+ Value of (DPLL_A_TEMP / DPLL_TEMP_DIVIDER) is integer part of >+ tempearture value. s/tempearture/temperature/ Didn't checkpatch warn you? >+ Value of (DPLL_A_TEMP % DPLL_TEMP_DIVIDER) is fractional part of >+ temperature value. >+ - >+ type: enum >+ name: type >+ doc: type of dpll, valid values for DPLL_A_TYPE attribute >+ entries: >+ - >+ name: 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: pin-type >+ doc: | >+ defines possible 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 Is this somehow related to the mode "nco" (Numerically Controlled Oscillator)? >+ - >+ name: gnss >+ doc: GNSS recovered clock >+ render-max: true >+ - >+ type: enum >+ name: pin-direction >+ doc: | >+ defines possible direction of a pin, valid values for >+ DPLL_A_PIN_DIRECTION attribute >+ entries: >+ - >+ name: 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: const >+ name: pin-frequency-1-hz >+ value: 1 >+ - >+ type: const >+ name: pin-frequency-10-mhz >+ value: 10000000 >+ - >+ type: enum >+ name: pin-state >+ doc: | >+ defines possible states of a pin, valid values for >+ DPLL_A_PIN_STATE attribute >+ entries: >+ - >+ name: unspec >+ doc: unspecified value >+ - >+ name: connected >+ doc: pin connected, active source of phase locked loop >+ - >+ name: disconnected >+ doc: pin disconnected, not considered as a valid source >+ - >+ name: selectable >+ doc: pin enabled for automatic source selection >+ render-max: true >+ - >+ type: flags >+ name: pin-caps >+ doc: | >+ defines possible capabilities of a pin, valid flags on >+ DPLL_A_PIN_CAPS attribute >+ entries: >+ - >+ name: direction-can-change >+ - >+ name: priority-can-change >+ - >+ name: state-can-change >+ - >+ 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 Please have a separate create/delete/change values for pins. >+ doc: | >+ attribute of dpll device or pin changed, reason is to be found with >+ an attribute type (DPLL_A_*) received with the event >+ >+ >+attribute-sets: >+ - >+ name: dpll >+ enum-name: dplla >+ attributes: >+ - >+ name: device >+ type: nest >+ value: 1 Why not 0? Also, Plese don't have this attr as a first one. It is related to PIN_GET/SET cmd, it should be somewhere among related attributes. Definitelly, the handle ATTR/ATTTs should be the first one/ones. >+ 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: lock-status >+ type: u8 >+ enum: lock-status >+ - >+ name: temp >+ type: s32 >+ - >+ name: clock-id >+ type: u64 >+ - >+ name: type >+ type: u8 >+ enum: type >+ - >+ name: pin-idx >+ type: u32 >+ - >+ name: pin-label >+ type: string >+ - >+ name: pin-type >+ type: u8 >+ enum: pin-type >+ - >+ name: pin-direction >+ type: u8 >+ enum: pin-direction >+ - >+ name: pin-frequency >+ type: u64 >+ - >+ name: pin-frequency-supported >+ type: nest >+ multi-attr: true >+ nested-attributes: pin-frequency-range >+ - >+ name: pin-frequency-min >+ type: u64 >+ - >+ name: pin-frequency-max >+ type: u64 >+ - >+ name: pin-prio >+ type: u32 >+ - >+ name: pin-state >+ type: u8 >+ enum: pin-state >+ - >+ name: pin-parent >+ type: nest >+ multi-attr: true >+ nested-attributes: pin-parent >+ - >+ 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: lock-status >+ type: u8 >+ enum: lock-status >+ - >+ name: temp >+ type: s32 >+ - >+ name: clock-id >+ type: u64 >+ - >+ name: type >+ type: u8 >+ enum: type >+ - >+ name: pin-prio >+ type: u32 >+ value: 19 Do you still need to pass values for a subset? That is odd. Well, I think is is odd to pass anything other than names in subset definition, the rest of the info is in the original attribute set definition, isn't it? Jakub? >+ - >+ name: pin-state >+ type: u8 >+ enum: pin-state >+ - >+ name: pin-parent >+ subset-of: dpll >+ attributes: >+ - >+ name: pin-state >+ type: u8 >+ value: 20 >+ enum: pin-state >+ - >+ name: pin-parent-idx >+ type: u32 >+ value: 22 >+ - >+ name: pin-rclk-device >+ type: string >+ - >+ name: pin-frequency-range >+ subset-of: dpll >+ attributes: >+ - >+ name: pin-frequency-min >+ type: u64 >+ value: 17 >+ - >+ name: pin-frequency-max >+ type: u64 >+ >+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 ] I may be missing something, but why do you enforce adming perm for get/dump cmds? >+ >+ 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 I might be missing something, but this means "device" netdev attribute DPLL_A_DEVICE, right? If yes, that is incorrect and you should list all the device attrs. >+ >+ - >+ 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: &pin-attrs >+ attributes: >+ - pin-idx >+ - pin-label >+ - pin-type >+ - pin-direction >+ - pin-frequency >+ - pin-frequency-supported >+ - pin-parent >+ - pin-rclk-device >+ - pin-dpll-caps >+ - device >+ >+ dump: >+ pre: dpll-pin-pre-dumpit >+ post: dpll-pin-post-dumpit >+ request: >+ attributes: >+ - id >+ - bus-name >+ - dev-name >+ reply: *pin-attrs >+ >+ - >+ 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-state >+ - pin-parent-idx >+ >+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..2f8643f401b0 >--- /dev/null >+++ b/drivers/dpll/dpll_nl.c >@@ -0,0 +1,126 @@ >+// SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR 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), I know it is a matter of the generator script, still have to note it hurts my eyes to see "5" here :) >+}; >+ >+/* 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_U64, }, >+ [DPLL_A_PIN_DIRECTION] = NLA_POLICY_MAX(NLA_U8, 2), >+ [DPLL_A_PIN_PRIO] = { .type = NLA_U32, }, >+ [DPLL_A_PIN_STATE] = NLA_POLICY_MAX(NLA_U8, 3), >+ [DPLL_A_PIN_PARENT_IDX] = { .type = NLA_U32, }, >+}; >+ >+/* Ops table for dpll */ >+static const struct genl_split_ops dpll_nl_ops[] = { >+ { >+ .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..57ab2da562ba >--- /dev/null >+++ b/drivers/dpll/dpll_nl.h >@@ -0,0 +1,42 @@ >+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR 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..e188bc189754 >--- /dev/null >+++ b/include/uapi/linux/dpll.h >@@ -0,0 +1,202 @@ >+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */ >+/* 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 >+ >+/** >+ * enum dpll_mode - working-modes a dpll can support, differentiate if and how >+ * dpll selects one of its sources to syntonize with it, valid values for >+ * DPLL_A_MODE attribute >+ * @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_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 source (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) >+}; >+ >+#define DPLL_TEMP_DIVIDER 10 >+ >+/** >+ * 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_pin_type - defines possible 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_direction - defines possible direction of a pin, valid values >+ * for DPLL_A_PIN_DIRECTION attribute >+ * @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) >+}; >+ >+#define DPLL_PIN_FREQUENCY_1_HZ 1 >+#define DPLL_PIN_FREQUENCY_10_MHZ 10000000 >+ >+/** >+ * enum dpll_pin_state - defines possible states of a pin, valid values for >+ * DPLL_A_PIN_STATE attribute >+ * @DPLL_PIN_STATE_UNSPEC: unspecified value >+ * @DPLL_PIN_STATE_CONNECTED: pin connected, active source of phase locked loop >+ * @DPLL_PIN_STATE_DISCONNECTED: pin disconnected, not considered as a valid >+ * source >+ * @DPLL_PIN_STATE_SELECTABLE: pin enabled for automatic source selection >+ */ >+enum dpll_pin_state { >+ DPLL_PIN_STATE_UNSPEC, >+ DPLL_PIN_STATE_CONNECTED, >+ DPLL_PIN_STATE_DISCONNECTED, >+ DPLL_PIN_STATE_SELECTABLE, >+ >+ __DPLL_PIN_STATE_MAX, >+ DPLL_PIN_STATE_MAX = (__DPLL_PIN_STATE_MAX - 1) >+}; >+ >+/** >+ * enum dpll_pin_caps - defines possible capabilities of a pin, valid flags on >+ * DPLL_A_PIN_CAPS attribute >+ */ >+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 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 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_LOCK_STATUS, >+ DPLL_A_TEMP, >+ DPLL_A_CLOCK_ID, >+ DPLL_A_TYPE, >+ DPLL_A_PIN_IDX, >+ DPLL_A_PIN_LABEL, >+ DPLL_A_PIN_TYPE, >+ DPLL_A_PIN_DIRECTION, >+ DPLL_A_PIN_FREQUENCY, >+ DPLL_A_PIN_FREQUENCY_SUPPORTED, >+ DPLL_A_PIN_FREQUENCY_MIN, >+ DPLL_A_PIN_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 = 1, >+ 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 >
On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: > >+definitions: > >+ - > >+ 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, valid values for DPLL_A_MODE > >+ attribute > >+ entries: > >+ - > >+ name: unspec > > In general, why exactly do we need unspec values in enums and CMDs? > What is the usecase. If there isn't please remove. +1 > >+ doc: unspecified value > >+ - > >+ name: manual I think the documentation calls this "forced", still. > >+ 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 > > Remove "no holdover available". This is not a state, this is a mode > configuration. If holdover is or isn't available, is a runtime info. Agreed, seems a little confusing now. Should we expose the system clk as a pin to be able to force lock to it? Or there's some extra magic at play here? > >+ - > >+ name: nco > >+ doc: dpll driven by Numerically Controlled Oscillator Noob question, what is NCO in terms of implementation? We source the signal from an arbitrary pin and FW / driver does the control? Or we always use system refclk and then tune? > >+ render-max: true > >+ - > >+ type: enum > >+ name: lock-status > >+ doc: | > >+ provides information of dpll device lock status, valid values for > >+ DPLL_A_LOCK_STATUS attribute > >+ entries: > >+ - > >+ name: unspec > >+ doc: unspecified value > >+ - > >+ name: unlocked > >+ doc: | > >+ dpll was not yet locked to any valid source (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 > > Is it needed to mention the holdover mode. It's slightly confusing, > because user might understand that the lock-status is always "holdover" > in case of "holdover" mode. But it could be "unlocked", can't it? > Perhaps I don't understand the flows there correctly :/ Hm, if we want to make sure that holdover mode must result in holdover state then we need some extra atomicity requirements on the SET operation. To me it seems logical enough that after setting holdover mode we'll end up either in holdover or unlocked status, depending on lock status when request reached the HW. > >+ render-max: true > >+ - > >+ type: const > >+ name: temp-divider > >+ value: 10 > >+ doc: | > >+ temperature divider allowing userspace to calculate the > >+ temperature as float with single digit precision. > >+ Value of (DPLL_A_TEMP / DPLL_TEMP_DIVIDER) is integer part of > >+ tempearture value. > > s/tempearture/temperature/ > > Didn't checkpatch warn you? Also can we give it a more healthy engineering margin? DPLL_A_TEMP is u32, silicon melts at around 1400C, so we really can afford to make the divisor 1000. > >+ 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: lock-status > >+ type: u8 > >+ enum: lock-status > >+ - > >+ name: temp > >+ type: s32 > >+ - > >+ name: clock-id > >+ type: u64 > >+ - > >+ name: type > >+ type: u8 > >+ enum: type > >+ - > >+ name: pin-prio > >+ type: u32 > >+ value: 19 > > Do you still need to pass values for a subset? That is odd. Well, I > think is is odd to pass anything other than names in subset definition, > the rest of the info is in the original attribute set definition, > isn't it? > Jakub? Probably stale code, related bug was fixed in YNL a few months back. Explicit value should no longer be needed.
Thu, May 04, 2023 at 11:24:51PM CEST, kuba@kernel.org wrote: >On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: [...] > >> >+ 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: lock-status >> >+ type: u8 >> >+ enum: lock-status >> >+ - >> >+ name: temp >> >+ type: s32 >> >+ - >> >+ name: clock-id >> >+ type: u64 >> >+ - >> >+ name: type >> >+ type: u8 >> >+ enum: type >> >+ - >> >+ name: pin-prio >> >+ type: u32 >> >+ value: 19 >> >> Do you still need to pass values for a subset? That is odd. Well, I >> think is is odd to pass anything other than names in subset definition, >> the rest of the info is in the original attribute set definition, >> isn't it? >> Jakub? > >Probably stale code, related bug was fixed in YNL a few months back. >Explicit value should no longer be needed. What about the rest, like type, enum, multi-attr etc. Are they needed for subset? If yes, why?
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, May 4, 2023 2:03 PM > >Fri, Apr 28, 2023 at 02:20:02AM CEST, vadfed@meta.com wrote: >>From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >> >>Add a protocol spec for DPLL. >>Add code generated from the spec. >> >>Signed-off-by: Jakub Kicinski <kuba@kernel.org> >>Signed-off-by: Michal Michalik <michal.michalik@intel.com> >>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>--- >> Documentation/netlink/specs/dpll.yaml | 472 ++++++++++++++++++++++++++ >> drivers/dpll/dpll_nl.c | 126 +++++++ >> drivers/dpll/dpll_nl.h | 42 +++ >> include/uapi/linux/dpll.h | 202 +++++++++++ >> 4 files changed, 842 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..67ca0f6cf2d5 >>--- /dev/null >>+++ b/Documentation/netlink/specs/dpll.yaml >>@@ -0,0 +1,472 @@ >>+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3- >>Clause) >>+ >>+name: dpll >>+ >>+doc: DPLL subsystem. >>+ >>+definitions: >>+ - >>+ type: enum >>+ name: mode >>+ doc: | >>+ working-modes a dpll can support, differentiate if and how dpll >>selects >>+ one of its sources to syntonize with it, valid values for DPLL_A_MODE >>+ attribute >>+ entries: >>+ - >>+ name: unspec > >In general, why exactly do we need unspec values in enums and CMDs? >What is the usecase. If there isn't please remove. > Sure, fixed. > >>+ 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 > >Remove "no holdover available". This is not a state, this is a mode >configuration. If holdover is or isn't available, is a runtime info. > Fiexd. > >>+ - >>+ name: nco >>+ doc: dpll driven by Numerically Controlled Oscillator >>+ render-max: true >>+ - >>+ type: enum >>+ name: lock-status >>+ doc: | >>+ provides information of dpll device lock status, valid values for >>+ DPLL_A_LOCK_STATUS attribute >>+ entries: >>+ - >>+ name: unspec >>+ doc: unspecified value >>+ - >>+ name: unlocked >>+ doc: | >>+ dpll was not yet locked to any valid source (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 > >Is it needed to mention the holdover mode. It's slightly confusing, >because user might understand that the lock-status is always "holdover" >in case of "holdover" mode. But it could be "unlocked", can't it? >Perhaps I don't understand the flows there correctly :/ > Yes, it could be unlocked even when user requests the 'holdover' mode, i.e. when the dpll was not locked to a valid source before requesting the mode. Improved the docs: name: holdover doc: | dpll is in holdover state - lost a valid lock or was forced by selecting DPLL_MODE_HOLDOVER mode (latter possible only when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, if it was not, the dpll's lock-status will remain DPLL_LOCK_STATUS_UNLOCKED even if user requests DPLL_MODE_HOLDOVER) Is that better? > >>+ render-max: true >>+ - >>+ type: const >>+ name: temp-divider >>+ value: 10 >>+ doc: | >>+ temperature divider allowing userspace to calculate the >>+ temperature as float with single digit precision. >>+ Value of (DPLL_A_TEMP / DPLL_TEMP_DIVIDER) is integer part of >>+ tempearture value. > >s/tempearture/temperature/ > >Didn't checkpatch warn you? > Fixed, thanks! No, I don't think it did. > >>+ Value of (DPLL_A_TEMP % DPLL_TEMP_DIVIDER) is fractional part of >>+ temperature value. >>+ - >>+ type: enum >>+ name: type >>+ doc: type of dpll, valid values for DPLL_A_TYPE attribute >>+ entries: >>+ - >>+ name: 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: pin-type >>+ doc: | >>+ defines possible 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 > >Is this somehow related to the mode "nco" (Numerically Controlled >Oscillator)? > Yes. > > >>+ - >>+ name: gnss >>+ doc: GNSS recovered clock >>+ render-max: true >>+ - >>+ type: enum >>+ name: pin-direction >>+ doc: | >>+ defines possible direction of a pin, valid values for >>+ DPLL_A_PIN_DIRECTION attribute >>+ entries: >>+ - >>+ name: 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: const >>+ name: pin-frequency-1-hz >>+ value: 1 >>+ - >>+ type: const >>+ name: pin-frequency-10-mhz >>+ value: 10000000 >>+ - >>+ type: enum >>+ name: pin-state >>+ doc: | >>+ defines possible states of a pin, valid values for >>+ DPLL_A_PIN_STATE attribute >>+ entries: >>+ - >>+ name: unspec >>+ doc: unspecified value >>+ - >>+ name: connected >>+ doc: pin connected, active source of phase locked loop >>+ - >>+ name: disconnected >>+ doc: pin disconnected, not considered as a valid source >>+ - >>+ name: selectable >>+ doc: pin enabled for automatic source selection >>+ render-max: true >>+ - >>+ type: flags >>+ name: pin-caps >>+ doc: | >>+ defines possible capabilities of a pin, valid flags on >>+ DPLL_A_PIN_CAPS attribute >>+ entries: >>+ - >>+ name: direction-can-change >>+ - >>+ name: priority-can-change >>+ - >>+ name: state-can-change >>+ - >>+ 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 > >Please have a separate create/delete/change values for pins. > Makes sense, but details, pin creation doesn't occur from uAPI perspective, as the pins itself are not visible to the user. They are visible after they are registered with a device, thus we would have to do something like: - pin-register - pin-unregister - pin-change Does it make sense? > >>+ doc: | >>+ attribute of dpll device or pin changed, reason is to be found >>with >>+ an attribute type (DPLL_A_*) received with the event >>+ >>+ >>+attribute-sets: >>+ - >>+ name: dpll >>+ enum-name: dplla >>+ attributes: >>+ - >>+ name: device >>+ type: nest >>+ value: 1 > >Why not 0? > Sorry I don't recall what exact technical reasons are behind it, but all netlink attributes I have found have 0 value attribute unused/unspec. >Also, Plese don't have this attr as a first one. It is related to >PIN_GET/SET cmd, it should be somewhere among related attributes. > >Definitelly, the handle ATTR/ATTTs should be the first one/ones. > Sure, fixed. > > >>+ 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: lock-status >>+ type: u8 >>+ enum: lock-status >>+ - >>+ name: temp >>+ type: s32 >>+ - >>+ name: clock-id >>+ type: u64 >>+ - >>+ name: type >>+ type: u8 >>+ enum: type >>+ - >>+ name: pin-idx >>+ type: u32 >>+ - >>+ name: pin-label >>+ type: string >>+ - >>+ name: pin-type >>+ type: u8 >>+ enum: pin-type >>+ - >>+ name: pin-direction >>+ type: u8 >>+ enum: pin-direction >>+ - >>+ name: pin-frequency >>+ type: u64 >>+ - >>+ name: pin-frequency-supported >>+ type: nest >>+ multi-attr: true >>+ nested-attributes: pin-frequency-range >>+ - >>+ name: pin-frequency-min >>+ type: u64 >>+ - >>+ name: pin-frequency-max >>+ type: u64 >>+ - >>+ name: pin-prio >>+ type: u32 >>+ - >>+ name: pin-state >>+ type: u8 >>+ enum: pin-state >>+ - >>+ name: pin-parent >>+ type: nest >>+ multi-attr: true >>+ nested-attributes: pin-parent >>+ - >>+ 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: lock-status >>+ type: u8 >>+ enum: lock-status >>+ - >>+ name: temp >>+ type: s32 >>+ - >>+ name: clock-id >>+ type: u64 >>+ - >>+ name: type >>+ type: u8 >>+ enum: type >>+ - >>+ name: pin-prio >>+ type: u32 >>+ value: 19 > >Do you still need to pass values for a subset? That is odd. Well, I >think is is odd to pass anything other than names in subset definition, >the rest of the info is in the original attribute set definition, >isn't it? >Jakub? > Yes it is fixed, I will remove those. > >>+ - >>+ name: pin-state >>+ type: u8 >>+ enum: pin-state >>+ - >>+ name: pin-parent >>+ subset-of: dpll >>+ attributes: >>+ - >>+ name: pin-state >>+ type: u8 >>+ value: 20 >>+ enum: pin-state >>+ - >>+ name: pin-parent-idx >>+ type: u32 >>+ value: 22 >>+ - >>+ name: pin-rclk-device >>+ type: string >>+ - >>+ name: pin-frequency-range >>+ subset-of: dpll >>+ attributes: >>+ - >>+ name: pin-frequency-min >>+ type: u64 >>+ value: 17 >>+ - >>+ name: pin-frequency-max >>+ type: u64 >>+ >>+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 ] > >I may be missing something, but why do you enforce adming perm for >get/dump cmds? > 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. > >>+ >>+ 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 > >I might be missing something, but this means "device" netdev attribute >DPLL_A_DEVICE, right? If yes, that is incorrect and you should list all >the device attrs. > Actually this means that attributes expected in response to this command are from `device` subset. But I see your point, will make `device` subset only for pin's nested attributes, and here will list device attributes. > >>+ >>+ - >>+ 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: &pin-attrs >>+ attributes: >>+ - pin-idx >>+ - pin-label >>+ - pin-type >>+ - pin-direction >>+ - pin-frequency >>+ - pin-frequency-supported >>+ - pin-parent >>+ - pin-rclk-device >>+ - pin-dpll-caps >>+ - device >>+ >>+ dump: >>+ pre: dpll-pin-pre-dumpit >>+ post: dpll-pin-post-dumpit >>+ request: >>+ attributes: >>+ - id >>+ - bus-name >>+ - dev-name >>+ reply: *pin-attrs >>+ >>+ - >>+ 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-state >>+ - pin-parent-idx >>+ >>+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..2f8643f401b0 >>--- /dev/null >>+++ b/drivers/dpll/dpll_nl.c >>@@ -0,0 +1,126 @@ >>+// SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR 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), > >I know it is a matter of the generator script, still have to note it >hurts my eyes to see "5" here :) > Yes, that's true. Thanks! Arkadiusz > >>+}; >>+ >>+/* 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_U64, }, >>+ [DPLL_A_PIN_DIRECTION] = NLA_POLICY_MAX(NLA_U8, 2), >>+ [DPLL_A_PIN_PRIO] = { .type = NLA_U32, }, >>+ [DPLL_A_PIN_STATE] = NLA_POLICY_MAX(NLA_U8, 3), >>+ [DPLL_A_PIN_PARENT_IDX] = { .type = NLA_U32, }, >>+}; >>+ >>+/* Ops table for dpll */ >>+static const struct genl_split_ops dpll_nl_ops[] = { >>+ { >>+ .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..57ab2da562ba >>--- /dev/null >>+++ b/drivers/dpll/dpll_nl.h >>@@ -0,0 +1,42 @@ >>+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR 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..e188bc189754 >>--- /dev/null >>+++ b/include/uapi/linux/dpll.h >>@@ -0,0 +1,202 @@ >>+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3- >>Clause) */ >>+/* 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 >>+ >>+/** >>+ * enum dpll_mode - working-modes a dpll can support, differentiate if and >>how >>+ * dpll selects one of its sources to syntonize with it, valid values for >>+ * DPLL_A_MODE attribute >>+ * @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_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 >>source (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) >>+}; >>+ >>+#define DPLL_TEMP_DIVIDER 10 >>+ >>+/** >>+ * 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_pin_type - defines possible 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_direction - defines possible direction of a pin, valid >>values >>+ * for DPLL_A_PIN_DIRECTION attribute >>+ * @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) >>+}; >>+ >>+#define DPLL_PIN_FREQUENCY_1_HZ 1 >>+#define DPLL_PIN_FREQUENCY_10_MHZ 10000000 >>+ >>+/** >>+ * enum dpll_pin_state - defines possible states of a pin, valid values for >>+ * DPLL_A_PIN_STATE attribute >>+ * @DPLL_PIN_STATE_UNSPEC: unspecified value >>+ * @DPLL_PIN_STATE_CONNECTED: pin connected, active source of phase >locked loop >>+ * @DPLL_PIN_STATE_DISCONNECTED: pin disconnected, not considered as a valid >>+ * source >>+ * @DPLL_PIN_STATE_SELECTABLE: pin enabled for automatic source selection >>+ */ >>+enum dpll_pin_state { >>+ DPLL_PIN_STATE_UNSPEC, >>+ DPLL_PIN_STATE_CONNECTED, >>+ DPLL_PIN_STATE_DISCONNECTED, >>+ DPLL_PIN_STATE_SELECTABLE, >>+ >>+ __DPLL_PIN_STATE_MAX, >>+ DPLL_PIN_STATE_MAX = (__DPLL_PIN_STATE_MAX - 1) >>+}; >>+ >>+/** >>+ * enum dpll_pin_caps - defines possible capabilities of a pin, valid >>flags on >>+ * DPLL_A_PIN_CAPS attribute >>+ */ >>+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 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 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_LOCK_STATUS, >>+ DPLL_A_TEMP, >>+ DPLL_A_CLOCK_ID, >>+ DPLL_A_TYPE, >>+ DPLL_A_PIN_IDX, >>+ DPLL_A_PIN_LABEL, >>+ DPLL_A_PIN_TYPE, >>+ DPLL_A_PIN_DIRECTION, >>+ DPLL_A_PIN_FREQUENCY, >>+ DPLL_A_PIN_FREQUENCY_SUPPORTED, >>+ DPLL_A_PIN_FREQUENCY_MIN, >>+ DPLL_A_PIN_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 = 1, >>+ 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 >>
>From: Jakub Kicinski <kuba@kernel.org> >Sent: Thursday, May 4, 2023 11:25 PM > >On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: >> >+definitions: >> >+ - >> >+ 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, valid values for >>DPLL_A_MODE >> >+ attribute >> >+ entries: >> >+ - >> >+ name: unspec >> >> In general, why exactly do we need unspec values in enums and CMDs? >> What is the usecase. If there isn't please remove. > >+1 > Sure, fixed. >> >+ doc: unspecified value >> >+ - >> >+ name: manual > >I think the documentation calls this "forced", still. > Yes, good catch, fixed docs. >> >+ 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 >> >> Remove "no holdover available". This is not a state, this is a mode >> configuration. If holdover is or isn't available, is a runtime info. > >Agreed, seems a little confusing now. Should we expose the system clk >as a pin to be able to force lock to it? Or there's some extra magic >at play here? In freerun you cannot lock to anything it, it just uses system clock from one of designated chip wires (which is not a part of source pins pool) to feed the dpll. Dpll would only stabilize that signal and pass it further. Locking itself is some kind of magic, as it usually takes at least ~15 seconds before it locks to a signal once it is selected. > >> >+ - >> >+ name: nco >> >+ doc: dpll driven by Numerically Controlled Oscillator > >Noob question, what is NCO in terms of implementation? >We source the signal from an arbitrary pin and FW / driver does >the control? Or we always use system refclk and then tune? > Documentation of chip we are using, stated NCO as similar to FREERUN, and it runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and dividers before it reaches the output). It doesn't count as an source pin, it uses signal form dedicated wire for SYSTEM CLOCK. In this case control over output frequency is done by synchronizer chip firmware, but still it will not lock to any source pin signal. >> >+ render-max: true >> >+ - >> >+ type: enum >> >+ name: lock-status >> >+ doc: | >> >+ provides information of dpll device lock status, valid values for >> >+ DPLL_A_LOCK_STATUS attribute >> >+ entries: >> >+ - >> >+ name: unspec >> >+ doc: unspecified value >> >+ - >> >+ name: unlocked >> >+ doc: | >> >+ dpll was not yet locked to any valid source (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 >> >> Is it needed to mention the holdover mode. It's slightly confusing, >> because user might understand that the lock-status is always "holdover" >> in case of "holdover" mode. But it could be "unlocked", can't it? >> Perhaps I don't understand the flows there correctly :/ > >Hm, if we want to make sure that holdover mode must result in holdover >state then we need some extra atomicity requirements on the SET >operation. To me it seems logical enough that after setting holdover >mode we'll end up either in holdover or unlocked status, depending on >lock status when request reached the HW. > Improved the docs: name: holdover doc: | dpll is in holdover state - lost a valid lock or was forced by selecting DPLL_MODE_HOLDOVER mode (latter possible only when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, if it was not, the dpll's lock-status will remain DPLL_LOCK_STATUS_UNLOCKED even if user requests DPLL_MODE_HOLDOVER) Is that better? What extra atomicity you have on your mind? Do you suggest to validate and allow (in dpll_netlink.c) only for 'unlocked' or 'holdover' states of dpll, once DPLL_MODE_HOLDOVER was successfully requested by the user? >> >+ render-max: true >> >+ - >> >+ type: const >> >+ name: temp-divider >> >+ value: 10 >> >+ doc: | >> >+ temperature divider allowing userspace to calculate the >> >+ temperature as float with single digit precision. >> >+ Value of (DPLL_A_TEMP / DPLL_TEMP_DIVIDER) is integer part of >> >+ tempearture value. >> >> s/tempearture/temperature/ >> >> Didn't checkpatch warn you? > >Also can we give it a more healthy engineering margin? >DPLL_A_TEMP is u32, silicon melts at around 1400C, >so we really can afford to make the divisor 1000. > Sure, fixed. >> >+ 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: lock-status >> >+ type: u8 >> >+ enum: lock-status >> >+ - >> >+ name: temp >> >+ type: s32 >> >+ - >> >+ name: clock-id >> >+ type: u64 >> >+ - >> >+ name: type >> >+ type: u8 >> >+ enum: type >> >+ - >> >+ name: pin-prio >> >+ type: u32 >> >+ value: 19 >> >> Do you still need to pass values for a subset? That is odd. Well, I >> think is is odd to pass anything other than names in subset definition, >> the rest of the info is in the original attribute set definition, >> isn't it? >> Jakub? > >Probably stale code, related bug was fixed in YNL a few months back. >Explicit value should no longer be needed. Yes, checked it works without them, I am removing values for next version. Thanks! Arkadiusz
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Friday, May 5, 2023 12:30 PM > >Thu, May 04, 2023 at 11:24:51PM CEST, kuba@kernel.org wrote: >>On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: > >[...] > >> >>> >+ 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: lock-status >>> >+ type: u8 >>> >+ enum: lock-status >>> >+ - >>> >+ name: temp >>> >+ type: s32 >>> >+ - >>> >+ name: clock-id >>> >+ type: u64 >>> >+ - >>> >+ name: type >>> >+ type: u8 >>> >+ enum: type >>> >+ - >>> >+ name: pin-prio >>> >+ type: u32 >>> >+ value: 19 >>> >>> Do you still need to pass values for a subset? That is odd. Well, I >>> think is is odd to pass anything other than names in subset definition, >>> the rest of the info is in the original attribute set definition, >>> isn't it? >>> Jakub? >> >>Probably stale code, related bug was fixed in YNL a few months back. >>Explicit value should no longer be needed. > >What about the rest, like type, enum, multi-attr etc. Are they needed >for subset? If yes, why? > > It seems the name and type is needed. Without type generation scripts fails. For now fixed with having only name/type on subset attributes. Thanks! Arkadiusz
Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Jakub Kicinski <kuba@kernel.org> >>Sent: Thursday, May 4, 2023 11:25 PM >> >>On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: >>> >+definitions: >>> >+ - >>> >+ 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, valid values for >>>DPLL_A_MODE >>> >+ attribute >>> >+ entries: >>> >+ - >>> >+ name: unspec >>> >>> In general, why exactly do we need unspec values in enums and CMDs? >>> What is the usecase. If there isn't please remove. >> >>+1 >> > >Sure, fixed. > >>> >+ doc: unspecified value >>> >+ - >>> >+ name: manual >> >>I think the documentation calls this "forced", still. >> > >Yes, good catch, fixed docs. > >>> >+ 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 >>> >>> Remove "no holdover available". This is not a state, this is a mode >>> configuration. If holdover is or isn't available, is a runtime info. >> >>Agreed, seems a little confusing now. Should we expose the system clk >>as a pin to be able to force lock to it? Or there's some extra magic >>at play here? > >In freerun you cannot lock to anything it, it just uses system clock from >one of designated chip wires (which is not a part of source pins pool) to feed >the dpll. Dpll would only stabilize that signal and pass it further. >Locking itself is some kind of magic, as it usually takes at least ~15 seconds >before it locks to a signal once it is selected. > >> >>> >+ - >>> >+ name: nco >>> >+ doc: dpll driven by Numerically Controlled Oscillator >> >>Noob question, what is NCO in terms of implementation? >>We source the signal from an arbitrary pin and FW / driver does >>the control? Or we always use system refclk and then tune? >> > >Documentation of chip we are using, stated NCO as similar to FREERUN, and it So how exactly this is different to freerun? Does user care or he would be fine with "freerun" in this case? My point is, isn't "NCO" some device specific thing that should be abstracted out here? >runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and >dividers before it reaches the output). >It doesn't count as an source pin, it uses signal form dedicated wire for >SYSTEM CLOCK. >In this case control over output frequency is done by synchronizer chip >firmware, but still it will not lock to any source pin signal. > >>> >+ render-max: true >>> >+ - >>> >+ type: enum >>> >+ name: lock-status >>> >+ doc: | >>> >+ provides information of dpll device lock status, valid values for >>> >+ DPLL_A_LOCK_STATUS attribute >>> >+ entries: >>> >+ - >>> >+ name: unspec >>> >+ doc: unspecified value >>> >+ - >>> >+ name: unlocked >>> >+ doc: | >>> >+ dpll was not yet locked to any valid source (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 >>> >>> Is it needed to mention the holdover mode. It's slightly confusing, >>> because user might understand that the lock-status is always "holdover" >>> in case of "holdover" mode. But it could be "unlocked", can't it? >>> Perhaps I don't understand the flows there correctly :/ >> >>Hm, if we want to make sure that holdover mode must result in holdover >>state then we need some extra atomicity requirements on the SET >>operation. To me it seems logical enough that after setting holdover >>mode we'll end up either in holdover or unlocked status, depending on >>lock status when request reached the HW. >> > >Improved the docs: > name: holdover > doc: | > dpll is in holdover state - lost a valid lock or was forced > by selecting DPLL_MODE_HOLDOVER mode (latter possible only > when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, > if it was not, the dpll's lock-status will remain "if it was not" does not really cope with the sentence above that. Could you iron-out the phrasing a bit please? > DPLL_LOCK_STATUS_UNLOCKED even if user requests > DPLL_MODE_HOLDOVER) >Is that better? > >What extra atomicity you have on your mind? >Do you suggest to validate and allow (in dpll_netlink.c) only for 'unlocked' >or 'holdover' states of dpll, once DPLL_MODE_HOLDOVER was successfully >requested by the user? > >>> >+ render-max: true >>> >+ - >>> >+ type: const >>> >+ name: temp-divider >>> >+ value: 10 >>> >+ doc: | >>> >+ temperature divider allowing userspace to calculate the >>> >+ temperature as float with single digit precision. >>> >+ Value of (DPLL_A_TEMP / DPLL_TEMP_DIVIDER) is integer part of >>> >+ tempearture value. >>> >>> s/tempearture/temperature/ >>> >>> Didn't checkpatch warn you? >> >>Also can we give it a more healthy engineering margin? >>DPLL_A_TEMP is u32, silicon melts at around 1400C, >>so we really can afford to make the divisor 1000. >> > >Sure, fixed. > >>> >+ 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: lock-status >>> >+ type: u8 >>> >+ enum: lock-status >>> >+ - >>> >+ name: temp >>> >+ type: s32 >>> >+ - >>> >+ name: clock-id >>> >+ type: u64 >>> >+ - >>> >+ name: type >>> >+ type: u8 >>> >+ enum: type >>> >+ - >>> >+ name: pin-prio >>> >+ type: u32 >>> >+ value: 19 >>> >>> Do you still need to pass values for a subset? That is odd. Well, I >>> think is is odd to pass anything other than names in subset definition, >>> the rest of the info is in the original attribute set definition, >>> isn't it? >>> Jakub? >> >>Probably stale code, related bug was fixed in YNL a few months back. >>Explicit value should no longer be needed. > >Yes, checked it works without them, I am removing values for next version. > >Thanks! >Arkadiusz
Thu, May 11, 2023 at 09:44:30AM CEST, arkadiusz.kubalewski@intel.com wrote: > >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Friday, May 5, 2023 12:30 PM >> >>Thu, May 04, 2023 at 11:24:51PM CEST, kuba@kernel.org wrote: >>>On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: >> >>[...] >> >>> >>>> >+ 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: lock-status >>>> >+ type: u8 >>>> >+ enum: lock-status >>>> >+ - >>>> >+ name: temp >>>> >+ type: s32 >>>> >+ - >>>> >+ name: clock-id >>>> >+ type: u64 >>>> >+ - >>>> >+ name: type >>>> >+ type: u8 >>>> >+ enum: type >>>> >+ - >>>> >+ name: pin-prio >>>> >+ type: u32 >>>> >+ value: 19 >>>> >>>> Do you still need to pass values for a subset? That is odd. Well, I >>>> think is is odd to pass anything other than names in subset definition, >>>> the rest of the info is in the original attribute set definition, >>>> isn't it? >>>> Jakub? >>> >>>Probably stale code, related bug was fixed in YNL a few months back. >>>Explicit value should no longer be needed. >> >>What about the rest, like type, enum, multi-attr etc. Are they needed >>for subset? If yes, why? >> >> > >It seems the name and type is needed. Without type generation scripts fails. >For now fixed with having only name/type on subset attributes. Okay. Jakub, any idea why "type" is needed here? > >Thanks! >Arkadiusz
Thu, May 11, 2023 at 09:38:04AM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Thursday, May 4, 2023 2:03 PM >> >>Fri, Apr 28, 2023 at 02:20:02AM CEST, vadfed@meta.com wrote: >>>From: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>> >>>Add a protocol spec for DPLL. >>>Add code generated from the spec. >>> >>>Signed-off-by: Jakub Kicinski <kuba@kernel.org> >>>Signed-off-by: Michal Michalik <michal.michalik@intel.com> >>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com> >>>Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >>>--- >>> Documentation/netlink/specs/dpll.yaml | 472 ++++++++++++++++++++++++++ >>> drivers/dpll/dpll_nl.c | 126 +++++++ >>> drivers/dpll/dpll_nl.h | 42 +++ >>> include/uapi/linux/dpll.h | 202 +++++++++++ >>> 4 files changed, 842 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..67ca0f6cf2d5 >>>--- /dev/null >>>+++ b/Documentation/netlink/specs/dpll.yaml >>>@@ -0,0 +1,472 @@ >>>+# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3- >>>Clause) >>>+ >>>+name: dpll >>>+ >>>+doc: DPLL subsystem. >>>+ >>>+definitions: >>>+ - >>>+ type: enum >>>+ name: mode >>>+ doc: | >>>+ working-modes a dpll can support, differentiate if and how dpll >>>selects >>>+ one of its sources to syntonize with it, valid values for DPLL_A_MODE >>>+ attribute >>>+ entries: >>>+ - >>>+ name: unspec >> >>In general, why exactly do we need unspec values in enums and CMDs? >>What is the usecase. If there isn't please remove. >> > >Sure, fixed. > >> >>>+ 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 >> >>Remove "no holdover available". This is not a state, this is a mode >>configuration. If holdover is or isn't available, is a runtime info. >> > >Fiexd. > >> >>>+ - >>>+ name: nco >>>+ doc: dpll driven by Numerically Controlled Oscillator >>>+ render-max: true >>>+ - >>>+ type: enum >>>+ name: lock-status >>>+ doc: | >>>+ provides information of dpll device lock status, valid values for >>>+ DPLL_A_LOCK_STATUS attribute >>>+ entries: >>>+ - >>>+ name: unspec >>>+ doc: unspecified value >>>+ - >>>+ name: unlocked >>>+ doc: | >>>+ dpll was not yet locked to any valid source (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 >> >>Is it needed to mention the holdover mode. It's slightly confusing, >>because user might understand that the lock-status is always "holdover" >>in case of "holdover" mode. But it could be "unlocked", can't it? >>Perhaps I don't understand the flows there correctly :/ >> > >Yes, it could be unlocked even when user requests the 'holdover' mode, i.e. >when the dpll was not locked to a valid source before requesting the mode. >Improved the docs: > name: holdover > doc: | > dpll is in holdover state - lost a valid lock or was forced > by selecting DPLL_MODE_HOLDOVER mode (latter possible only > when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, > if it was not, the dpll's lock-status will remain > DPLL_LOCK_STATUS_UNLOCKED even if user requests > DPLL_MODE_HOLDOVER) >Is that better? See my comment to this in the other branch of this thread. > >> >>>+ render-max: true >>>+ - >>>+ type: const >>>+ name: temp-divider >>>+ value: 10 >>>+ doc: | >>>+ temperature divider allowing userspace to calculate the >>>+ temperature as float with single digit precision. >>>+ Value of (DPLL_A_TEMP / DPLL_TEMP_DIVIDER) is integer part of >>>+ tempearture value. >> >>s/tempearture/temperature/ >> >>Didn't checkpatch warn you? >> > >Fixed, thanks! >No, I don't think it did. > >> >>>+ Value of (DPLL_A_TEMP % DPLL_TEMP_DIVIDER) is fractional part of >>>+ temperature value. >>>+ - >>>+ type: enum >>>+ name: type >>>+ doc: type of dpll, valid values for DPLL_A_TYPE attribute >>>+ entries: >>>+ - >>>+ name: 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: pin-type >>>+ doc: | >>>+ defines possible 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 >> >>Is this somehow related to the mode "nco" (Numerically Controlled >>Oscillator)? >> > >Yes. How? Why do we need to expose it as a pin then? > >> >> >>>+ - >>>+ name: gnss >>>+ doc: GNSS recovered clock >>>+ render-max: true >>>+ - >>>+ type: enum >>>+ name: pin-direction >>>+ doc: | >>>+ defines possible direction of a pin, valid values for >>>+ DPLL_A_PIN_DIRECTION attribute >>>+ entries: >>>+ - >>>+ name: 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: const >>>+ name: pin-frequency-1-hz >>>+ value: 1 >>>+ - >>>+ type: const >>>+ name: pin-frequency-10-mhz >>>+ value: 10000000 >>>+ - >>>+ type: enum >>>+ name: pin-state >>>+ doc: | >>>+ defines possible states of a pin, valid values for >>>+ DPLL_A_PIN_STATE attribute >>>+ entries: >>>+ - >>>+ name: unspec >>>+ doc: unspecified value >>>+ - >>>+ name: connected >>>+ doc: pin connected, active source of phase locked loop >>>+ - >>>+ name: disconnected >>>+ doc: pin disconnected, not considered as a valid source >>>+ - >>>+ name: selectable >>>+ doc: pin enabled for automatic source selection >>>+ render-max: true >>>+ - >>>+ type: flags >>>+ name: pin-caps >>>+ doc: | >>>+ defines possible capabilities of a pin, valid flags on >>>+ DPLL_A_PIN_CAPS attribute >>>+ entries: >>>+ - >>>+ name: direction-can-change >>>+ - >>>+ name: priority-can-change >>>+ - >>>+ name: state-can-change >>>+ - >>>+ 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 >> >>Please have a separate create/delete/change values for pins. >> > >Makes sense, but details, pin creation doesn't occur from uAPI perspective, >as the pins itself are not visible to the user. They are visible after they >are registered with a device, thus we would have to do something like: >- pin-register >- pin-unregister >- pin-change > >Does it make sense? From perspective of user, it is "creation/new" or "deletion/del". Object appears of dissapears in UAPI, no matter how this is implemented in kernel. If you call it register/unregister, that exposes unnecessary internal kernel notation. No strong feeling though if you insist on register/unregister, it just sounds odd and funny. Anyway, one way or another, be in-sync naming wise with device events. > >> >>>+ doc: | >>>+ attribute of dpll device or pin changed, reason is to be found >>>with >>>+ an attribute type (DPLL_A_*) received with the event >>>+ >>>+ >>>+attribute-sets: >>>+ - >>>+ name: dpll >>>+ enum-name: dplla >>>+ attributes: >>>+ - >>>+ name: device >>>+ type: nest >>>+ value: 1 >> >>Why not 0? >> > >Sorry I don't recall what exact technical reasons are behind it, but all >netlink attributes I have found have 0 value attribute unused/unspec. I don't see why that is needed, I may be missing something though. Up to you. > >>Also, Plese don't have this attr as a first one. It is related to >>PIN_GET/SET cmd, it should be somewhere among related attributes. >> >>Definitelly, the handle ATTR/ATTTs should be the first one/ones. >> > >Sure, fixed. > >> >> >>>+ 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: lock-status >>>+ type: u8 >>>+ enum: lock-status >>>+ - >>>+ name: temp >>>+ type: s32 >>>+ - >>>+ name: clock-id >>>+ type: u64 >>>+ - >>>+ name: type >>>+ type: u8 >>>+ enum: type >>>+ - >>>+ name: pin-idx >>>+ type: u32 >>>+ - >>>+ name: pin-label >>>+ type: string >>>+ - >>>+ name: pin-type >>>+ type: u8 >>>+ enum: pin-type >>>+ - >>>+ name: pin-direction >>>+ type: u8 >>>+ enum: pin-direction >>>+ - >>>+ name: pin-frequency >>>+ type: u64 >>>+ - >>>+ name: pin-frequency-supported >>>+ type: nest >>>+ multi-attr: true >>>+ nested-attributes: pin-frequency-range >>>+ - >>>+ name: pin-frequency-min >>>+ type: u64 >>>+ - >>>+ name: pin-frequency-max >>>+ type: u64 >>>+ - >>>+ name: pin-prio >>>+ type: u32 >>>+ - >>>+ name: pin-state >>>+ type: u8 >>>+ enum: pin-state >>>+ - >>>+ name: pin-parent >>>+ type: nest >>>+ multi-attr: true >>>+ nested-attributes: pin-parent >>>+ - >>>+ 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: lock-status >>>+ type: u8 >>>+ enum: lock-status >>>+ - >>>+ name: temp >>>+ type: s32 >>>+ - >>>+ name: clock-id >>>+ type: u64 >>>+ - >>>+ name: type >>>+ type: u8 >>>+ enum: type >>>+ - >>>+ name: pin-prio >>>+ type: u32 >>>+ value: 19 >> >>Do you still need to pass values for a subset? That is odd. Well, I >>think is is odd to pass anything other than names in subset definition, >>the rest of the info is in the original attribute set definition, >>isn't it? >>Jakub? >> > >Yes it is fixed, I will remove those. > >> >>>+ - >>>+ name: pin-state >>>+ type: u8 >>>+ enum: pin-state >>>+ - >>>+ name: pin-parent >>>+ subset-of: dpll >>>+ attributes: >>>+ - >>>+ name: pin-state >>>+ type: u8 >>>+ value: 20 >>>+ enum: pin-state >>>+ - >>>+ name: pin-parent-idx >>>+ type: u32 >>>+ value: 22 >>>+ - >>>+ name: pin-rclk-device >>>+ type: string >>>+ - >>>+ name: pin-frequency-range >>>+ subset-of: dpll >>>+ attributes: >>>+ - >>>+ name: pin-frequency-min >>>+ type: u64 >>>+ value: 17 >>>+ - >>>+ name: pin-frequency-max >>>+ type: u64 >>>+ >>>+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 ] >> >>I may be missing something, but why do you enforce adming perm for >>get/dump cmds? >> > >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. Hmm, I wonder why other read cmds usually don't need this. In fact, is there some read netlink cmd in kernel now which needs it? > >> >>>+ >>>+ 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 >> >>I might be missing something, but this means "device" netdev attribute >>DPLL_A_DEVICE, right? If yes, that is incorrect and you should list all >>the device attrs. >> > >Actually this means that attributes expected in response to this command are >from `device` subset. >But I see your point, will make `device` subset only for pin's nested >attributes, and here will list device attributes. Yes, that is my point. The fix you describes sounds fine. > >> >>>+ >>>+ - >>>+ 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: &pin-attrs >>>+ attributes: >>>+ - pin-idx >>>+ - pin-label >>>+ - pin-type >>>+ - pin-direction >>>+ - pin-frequency >>>+ - pin-frequency-supported >>>+ - pin-parent >>>+ - pin-rclk-device >>>+ - pin-dpll-caps >>>+ - device >>>+ >>>+ dump: >>>+ pre: dpll-pin-pre-dumpit >>>+ post: dpll-pin-post-dumpit >>>+ request: >>>+ attributes: >>>+ - id >>>+ - bus-name >>>+ - dev-name >>>+ reply: *pin-attrs >>>+ >>>+ - >>>+ 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-state >>>+ - pin-parent-idx >>>+ >>>+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..2f8643f401b0 >>>--- /dev/null >>>+++ b/drivers/dpll/dpll_nl.c >>>@@ -0,0 +1,126 @@ >>>+// SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR 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), >> >>I know it is a matter of the generator script, still have to note it >>hurts my eyes to see "5" here :) >> > >Yes, that's true. > >Thanks! >Arkadiusz > >> >>>+}; >>>+ >>>+/* 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_U64, }, >>>+ [DPLL_A_PIN_DIRECTION] = NLA_POLICY_MAX(NLA_U8, 2), >>>+ [DPLL_A_PIN_PRIO] = { .type = NLA_U32, }, >>>+ [DPLL_A_PIN_STATE] = NLA_POLICY_MAX(NLA_U8, 3), >>>+ [DPLL_A_PIN_PARENT_IDX] = { .type = NLA_U32, }, >>>+}; >>>+ >>>+/* Ops table for dpll */ >>>+static const struct genl_split_ops dpll_nl_ops[] = { >>>+ { >>>+ .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..57ab2da562ba >>>--- /dev/null >>>+++ b/drivers/dpll/dpll_nl.h >>>@@ -0,0 +1,42 @@ >>>+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR 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..e188bc189754 >>>--- /dev/null >>>+++ b/include/uapi/linux/dpll.h >>>@@ -0,0 +1,202 @@ >>>+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3- >>>Clause) */ >>>+/* 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 >>>+ >>>+/** >>>+ * enum dpll_mode - working-modes a dpll can support, differentiate if and >>>how >>>+ * dpll selects one of its sources to syntonize with it, valid values for >>>+ * DPLL_A_MODE attribute >>>+ * @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_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 >>>source (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) >>>+}; >>>+ >>>+#define DPLL_TEMP_DIVIDER 10 >>>+ >>>+/** >>>+ * 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_pin_type - defines possible 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_direction - defines possible direction of a pin, valid >>>values >>>+ * for DPLL_A_PIN_DIRECTION attribute >>>+ * @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) >>>+}; >>>+ >>>+#define DPLL_PIN_FREQUENCY_1_HZ 1 >>>+#define DPLL_PIN_FREQUENCY_10_MHZ 10000000 >>>+ >>>+/** >>>+ * enum dpll_pin_state - defines possible states of a pin, valid values for >>>+ * DPLL_A_PIN_STATE attribute >>>+ * @DPLL_PIN_STATE_UNSPEC: unspecified value >>>+ * @DPLL_PIN_STATE_CONNECTED: pin connected, active source of phase >>locked loop >>>+ * @DPLL_PIN_STATE_DISCONNECTED: pin disconnected, not considered as a valid >>>+ * source >>>+ * @DPLL_PIN_STATE_SELECTABLE: pin enabled for automatic source selection >>>+ */ >>>+enum dpll_pin_state { >>>+ DPLL_PIN_STATE_UNSPEC, >>>+ DPLL_PIN_STATE_CONNECTED, >>>+ DPLL_PIN_STATE_DISCONNECTED, >>>+ DPLL_PIN_STATE_SELECTABLE, >>>+ >>>+ __DPLL_PIN_STATE_MAX, >>>+ DPLL_PIN_STATE_MAX = (__DPLL_PIN_STATE_MAX - 1) >>>+}; >>>+ >>>+/** >>>+ * enum dpll_pin_caps - defines possible capabilities of a pin, valid >>>flags on >>>+ * DPLL_A_PIN_CAPS attribute >>>+ */ >>>+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 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 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_LOCK_STATUS, >>>+ DPLL_A_TEMP, >>>+ DPLL_A_CLOCK_ID, >>>+ DPLL_A_TYPE, >>>+ DPLL_A_PIN_IDX, >>>+ DPLL_A_PIN_LABEL, >>>+ DPLL_A_PIN_TYPE, >>>+ DPLL_A_PIN_DIRECTION, >>>+ DPLL_A_PIN_FREQUENCY, >>>+ DPLL_A_PIN_FREQUENCY_SUPPORTED, >>>+ DPLL_A_PIN_FREQUENCY_MIN, >>>+ DPLL_A_PIN_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 = 1, >>>+ 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 >>>
On Thu, 11 May 2023 10:00:35 +0200 Jiri Pirko wrote: >> It seems the name and type is needed. Without type generation scripts fails. >> For now fixed with having only name/type on subset attributes. > > Okay. Jakub, any idea why "type" is needed here? I don't remember.
On Thu, 11 May 2023 07:40:26 +0000 Kubalewski, Arkadiusz wrote: > >> Remove "no holdover available". This is not a state, this is a mode > >> configuration. If holdover is or isn't available, is a runtime info. > > > >Agreed, seems a little confusing now. Should we expose the system clk > >as a pin to be able to force lock to it? Or there's some extra magic > >at play here? > > In freerun you cannot lock to anything it, it just uses system clock from > one of designated chip wires (which is not a part of source pins pool) to feed > the dpll. Dpll would only stabilize that signal and pass it further. > Locking itself is some kind of magic, as it usually takes at least ~15 seconds > before it locks to a signal once it is selected. Okay, I guess that makes sense. I was wondering if there may be a DPLLs which allow other input clocks to bypass the PLL logic, and output purely a stabilized signal. In which case we should model this as a generic PLL bypass, FREERUN being just one special case where we're bypassing with the system clock. But that may well be a case of "software guy thinking", so if nobody thinks this can happen in practice we can keep FREERUN. > >Noob question, what is NCO in terms of implementation? > >We source the signal from an arbitrary pin and FW / driver does > >the control? Or we always use system refclk and then tune? > > > > Documentation of chip we are using, stated NCO as similar to FREERUN, and it > runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and > dividers before it reaches the output). > It doesn't count as an source pin, it uses signal form dedicated wire for > SYSTEM CLOCK. > In this case control over output frequency is done by synchronizer chip > firmware, but still it will not lock to any source pin signal. Reading wikipedia it sounds like NCO is just a way of generating a waveform from synchronous logic. Does the DPLL not allow changing clock frequency when locked? I.e. feeding it one frequency and outputting another? Because I think that'd be done by an NCO, no? > >> Is it needed to mention the holdover mode. It's slightly confusing, > >> because user might understand that the lock-status is always "holdover" > >> in case of "holdover" mode. But it could be "unlocked", can't it? > >> Perhaps I don't understand the flows there correctly :/ > > > >Hm, if we want to make sure that holdover mode must result in holdover > >state then we need some extra atomicity requirements on the SET > >operation. To me it seems logical enough that after setting holdover > >mode we'll end up either in holdover or unlocked status, depending on > >lock status when request reached the HW. > > > > Improved the docs: > name: holdover > doc: | > dpll is in holdover state - lost a valid lock or was forced > by selecting DPLL_MODE_HOLDOVER mode (latter possible only > when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, > if it was not, the dpll's lock-status will remain > DPLL_LOCK_STATUS_UNLOCKED even if user requests > DPLL_MODE_HOLDOVER) > Is that better? Yes, modulo breaking it up into sentences, as Jiri says. > What extra atomicity you have on your mind? > Do you suggest to validate and allow (in dpll_netlink.c) only for 'unlocked' > or 'holdover' states of dpll, once DPLL_MODE_HOLDOVER was successfully > requested by the user? No, I was saying that making sure that we end up in holdover (rather than unlocked) when user requested holdover is hard, and we shouldn't even try to implement that.
On Thu, 11 May 2023 07:38:04 +0000 Kubalewski, Arkadiusz wrote: > >>+ - > >>+ 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 > > > >Please have a separate create/delete/change values for pins. > > > > Makes sense, but details, pin creation doesn't occur from uAPI perspective, > as the pins itself are not visible to the user. They are visible after they > are registered with a device, thus we would have to do something like: > - pin-register > - pin-unregister > - pin-change > > Does it make sense? I missed this, notifications should be declared under operations. Please look at netdev.yaml for an example. I thought about implementing this model where events are separate explicitly but I think it's an unnecessary complication.
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, May 11, 2023 10:00 AM > >Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@intel.com wrote: >>>From: Jakub Kicinski <kuba@kernel.org> >>>Sent: Thursday, May 4, 2023 11:25 PM >>> >>>On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: >>>> >+definitions: >>>> >+ - >>>> >+ 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, valid values for >>>>DPLL_A_MODE >>>> >+ attribute >>>> >+ entries: >>>> >+ - >>>> >+ name: unspec >>>> >>>> In general, why exactly do we need unspec values in enums and CMDs? >>>> What is the usecase. If there isn't please remove. >>> >>>+1 >>> >> >>Sure, fixed. >> >>>> >+ doc: unspecified value >>>> >+ - >>>> >+ name: manual >>> >>>I think the documentation calls this "forced", still. >>> >> >>Yes, good catch, fixed docs. >> >>>> >+ 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 >>>> >>>> Remove "no holdover available". This is not a state, this is a mode >>>> configuration. If holdover is or isn't available, is a runtime info. >>> >>>Agreed, seems a little confusing now. Should we expose the system clk >>>as a pin to be able to force lock to it? Or there's some extra magic >>>at play here? >> >>In freerun you cannot lock to anything it, it just uses system clock from >>one of designated chip wires (which is not a part of source pins pool) to >>feed the dpll. Dpll would only stabilize that signal and pass it further. >>Locking itself is some kind of magic, as it usually takes at least ~15 >>seconds before it locks to a signal once it is selected. >> >>> >>>> >+ - >>>> >+ name: nco >>>> >+ doc: dpll driven by Numerically Controlled Oscillator >>> >>>Noob question, what is NCO in terms of implementation? >>>We source the signal from an arbitrary pin and FW / driver does >>>the control? Or we always use system refclk and then tune? >>> >> >>Documentation of chip we are using, stated NCO as similar to FREERUN, and >it > >So how exactly this is different to freerun? Does user care or he would >be fine with "freerun" in this case? My point is, isn't "NCO" some >device specific thing that should be abstracted out here? > Sure, it is device specific, some synchronizing circuits would have this capability, while others would not. Should be abstracted out? It is a good question.. shall user know that he is in freerun with possibility to control the frequency or not? Let's say we remove NCO, and have dpll with enabled FREERUN mode and pins supporting multiple output frequencies. How the one would know if those frequencies are supported only in MANUAL/AUTOMATIC modes or also in the FREERUN mode? In other words: As the user can I change a frequency of a dpll if active mode is FREERUN? I would say it is better to have such mode, we could argue on naming though. > >>runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and >>dividers before it reaches the output). >>It doesn't count as an source pin, it uses signal form dedicated wire for >>SYSTEM CLOCK. >>In this case control over output frequency is done by synchronizer chip >>firmware, but still it will not lock to any source pin signal. >> >>>> >+ render-max: true >>>> >+ - >>>> >+ type: enum >>>> >+ name: lock-status >>>> >+ doc: | >>>> >+ provides information of dpll device lock status, valid values for >>>> >+ DPLL_A_LOCK_STATUS attribute >>>> >+ entries: >>>> >+ - >>>> >+ name: unspec >>>> >+ doc: unspecified value >>>> >+ - >>>> >+ name: unlocked >>>> >+ doc: | >>>> >+ dpll was not yet locked to any valid source (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 >>>> >>>> Is it needed to mention the holdover mode. It's slightly confusing, >>>> because user might understand that the lock-status is always "holdover" >>>> in case of "holdover" mode. But it could be "unlocked", can't it? >>>> Perhaps I don't understand the flows there correctly :/ >>> >>>Hm, if we want to make sure that holdover mode must result in holdover >>>state then we need some extra atomicity requirements on the SET >>>operation. To me it seems logical enough that after setting holdover >>>mode we'll end up either in holdover or unlocked status, depending on >>>lock status when request reached the HW. >>> >> >>Improved the docs: >> name: holdover >> doc: | >> dpll is in holdover state - lost a valid lock or was forced >> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >> if it was not, the dpll's lock-status will remain > >"if it was not" does not really cope with the sentence above that. Could >you iron-out the phrasing a bit please? Hmmm, name: holdover doc: | dpll is in holdover state - lost a valid lock or was forced by selecting DPLL_MODE_HOLDOVER mode (latter possible only when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, if dpll lock-state was not DPLL_LOCK_STATUS_LOCKED, the dpll's lock-state shall remain DPLL_LOCK_STATUS_UNLOCKED even if DPLL_MODE_HOLDOVER was requested) Hope this is better? Thank you! Arkadiusz [...]
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, May 11, 2023 10:14 AM > >Thu, May 11, 2023 at 09:38:04AM CEST, arkadiusz.kubalewski@intel.com wrote: [...] >>>>+ - >>>>+ name: holdover >>>>+ doc: | >>>>+ dpll is in holdover state - lost a valid lock or was forced by >>>>+ selecting DPLL_MODE_HOLDOVER mode >>> >>>Is it needed to mention the holdover mode. It's slightly confusing, >>>because user might understand that the lock-status is always "holdover" >>>in case of "holdover" mode. But it could be "unlocked", can't it? >>>Perhaps I don't understand the flows there correctly :/ >>> >> >>Yes, it could be unlocked even when user requests the 'holdover' mode, >i.e. >>when the dpll was not locked to a valid source before requesting the mode. >>Improved the docs: >> name: holdover >> doc: | >> dpll is in holdover state - lost a valid lock or was forced >> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >> if it was not, the dpll's lock-status will remain >> DPLL_LOCK_STATUS_UNLOCKED even if user requests >> DPLL_MODE_HOLDOVER) >>Is that better? > >See my comment to this in the other branch of this thread. > Sure, gonna reply there. [...] >>>>+ - >>>>+ name: int-oscillator >>>>+ doc: device internal oscillator >>> >>>Is this somehow related to the mode "nco" (Numerically Controlled >>>Oscillator)? >>> >> >>Yes. > >How? Why do we need to expose it as a pin then? > Sorry, I messed up something with that answer.. It is not related to NCO (as NCO uses SYSTEM CLOCK to produce frequency). It is a type of a pin which source or output is somehow internal. I.e. our dpll's can syntonize to the 1 PPS clock signal produced by network chip. As for other use-cases it could serve as way of having one or more oscillators on board connected to input pins. [...] >>>>+ 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 >>> >>>Please have a separate create/delete/change values for pins. >>> >> >>Makes sense, but details, pin creation doesn't occur from uAPI perspective, >>as the pins itself are not visible to the user. They are visible after they >>are registered with a device, thus we would have to do something like: >>- pin-register >>- pin-unregister >>- pin-change >> >>Does it make sense? > >From perspective of user, it is "creation/new" or "deletion/del". >Object appears of dissapears in UAPI, no matter how this is implemented >in kernel. If you call it register/unregister, that exposes unnecessary >internal kernel notation. > >No strong feeling though if you insist on register/unregister, it just >sounds odd and funny. > >Anyway, one way or another, be in-sync naming wise with device events. > Sure going in sync with device event names seems best option, will fix as suggested: - pin-create - pin-delete - pin-change > > >> >>> >>>>+ doc: | >>>>+ attribute of dpll device or pin changed, reason is to be >found >>>>with >>>>+ an attribute type (DPLL_A_*) received with the event >>>>+ >>>>+ >>>>+attribute-sets: >>>>+ - >>>>+ name: dpll >>>>+ enum-name: dplla >>>>+ attributes: >>>>+ - >>>>+ name: device >>>>+ type: nest >>>>+ value: 1 >>> >>>Why not 0? >>> >> >>Sorry I don't recall what exact technical reasons are behind it, but all >>netlink attributes I have found have 0 value attribute unused/unspec. > >I don't see why that is needed, I may be missing something though. >Up to you. > Will leave it as is, if no other comments. [...] >>>>+ attribute-set: dpll >>>>+ flags: [ admin-perm ] >>> >>>I may be missing something, but why do you enforce adming perm for >>>get/dump cmds? >>> >> >>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. > >Hmm, I wonder why other read cmds usually don't need this. In fact, >is there some read netlink cmd in kernel now which needs it? > Seems drivers/net/team/team.c uses it for get, but anyway this is a "least privilege" security principle, if regular user cannot do anything with that information, there is no point on providing it. > >> >>> >>>>+ >>>>+ 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 >>> >>>I might be missing something, but this means "device" netdev attribute >>>DPLL_A_DEVICE, right? If yes, that is incorrect and you should list all >>>the device attrs. >>> >> >>Actually this means that attributes expected in response to this command are >>from `device` subset. >>But I see your point, will make `device` subset only for pin's nested >>attributes, and here will list device attributes. > >Yes, that is my point. The fix you describes sounds fine. > > Thank you! Arkadiusz [...]
>From: Jakub Kicinski <kuba@kernel.org> >Sent: Thursday, May 11, 2023 5:21 PM > >On Thu, 11 May 2023 07:40:26 +0000 Kubalewski, Arkadiusz wrote: >> >> Remove "no holdover available". This is not a state, this is a mode >> >> configuration. If holdover is or isn't available, is a runtime info. >> > >> >Agreed, seems a little confusing now. Should we expose the system clk >> >as a pin to be able to force lock to it? Or there's some extra magic >> >at play here? >> >> In freerun you cannot lock to anything it, it just uses system clock from >> one of designated chip wires (which is not a part of source pins pool) to >> feed >> the dpll. Dpll would only stabilize that signal and pass it further. >> Locking itself is some kind of magic, as it usually takes at least ~15 >> seconds >> before it locks to a signal once it is selected. > >Okay, I guess that makes sense. > >I was wondering if there may be a DPLLs which allow other input clocks >to bypass the PLL logic, and output purely a stabilized signal. In >which case we should model this as a generic PLL bypass, FREERUN being >just one special case where we're bypassing with the system clock. > >But that may well be a case of "software guy thinking", so if nobody >thinks this can happen in practice we can keep FREERUN. > Well I am not saying such use-case doesn't exist, but haven't heard about it. >> >Noob question, what is NCO in terms of implementation? >> >We source the signal from an arbitrary pin and FW / driver does >> >the control? Or we always use system refclk and then tune? >> > >> >> Documentation of chip we are using, stated NCO as similar to FREERUN, and it >> runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and >> dividers before it reaches the output). >> It doesn't count as an source pin, it uses signal form dedicated wire for >> SYSTEM CLOCK. >> In this case control over output frequency is done by synchronizer chip >> firmware, but still it will not lock to any source pin signal. > >Reading wikipedia it sounds like NCO is just a way of generating >a waveform from synchronous logic. > >Does the DPLL not allow changing clock frequency when locked? >I.e. feeding it one frequency and outputting another? Well our dpll (actually synchronizer chip) does that in AUTOMATIC/MANUAL modes, i.e. you feed 1 PPS from gnss and output feed for PHY's ~156 MHZ, so I guess this is pretty common for complex synchronizer chips, although AFAIK this is achieved with additional signal synthesizers after the PLL logic. >Because I think that'd be done by an NCO, no? From docs I can also see that chip has additional designated dpll for NCO mode, and this statement: "Numerically controlled oscillator (NCO) behavior allows system software to steer DPLL frequency or synthesizer frequency with resolution better than 0.005 ppt." I am certainly not an expert on this, but seems like the NCO mode for this chip is better than FREERUN, since signal produced on output is somehow higher quality. > >> >> Is it needed to mention the holdover mode. It's slightly confusing, >> >> because user might understand that the lock-status is always "holdover" >> >> in case of "holdover" mode. But it could be "unlocked", can't it? >> >> Perhaps I don't understand the flows there correctly :/ >> > >> >Hm, if we want to make sure that holdover mode must result in holdover >> >state then we need some extra atomicity requirements on the SET >> >operation. To me it seems logical enough that after setting holdover >> >mode we'll end up either in holdover or unlocked status, depending on >> >lock status when request reached the HW. >> > >> >> Improved the docs: >> name: holdover >> doc: | >> dpll is in holdover state - lost a valid lock or was forced >> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >> if it was not, the dpll's lock-status will remain >> DPLL_LOCK_STATUS_UNLOCKED even if user requests >> DPLL_MODE_HOLDOVER) >> Is that better? > >Yes, modulo breaking it up into sentences, as Jiri says. > Sure, will do. >> What extra atomicity you have on your mind? >> Do you suggest to validate and allow (in dpll_netlink.c) only for 'unlocked' >> or 'holdover' states of dpll, once DPLL_MODE_HOLDOVER was successfully >> requested by the user? > >No, I was saying that making sure that we end up in holdover (rather >than unlocked) when user requested holdover is hard, and we shouldn't >even try to implement that. Okay. Thank you! Arkadiusz
>From: Jakub Kicinski <kuba@kernel.org> >Sent: Thursday, May 11, 2023 5:27 PM > >On Thu, 11 May 2023 07:38:04 +0000 Kubalewski, Arkadiusz wrote: >> >>+ - >> >>+ 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 >> > >> >Please have a separate create/delete/change values for pins. >> > >> >> Makes sense, but details, pin creation doesn't occur from uAPI perspective, >> as the pins itself are not visible to the user. They are visible after they >> are registered with a device, thus we would have to do something like: >> - pin-register >> - pin-unregister >> - pin-change >> >> Does it make sense? > >I missed this, notifications should be declared under operations. > >Please look at netdev.yaml for an example. > >I thought about implementing this model where events are separate >explicitly but I think it's an unnecessary complication. Ok, will do. Thank you! Arkadiusz
On Thu, 11 May 2023 20:53:40 +0000 Kubalewski, Arkadiusz wrote: > >Because I think that'd be done by an NCO, no? > > From docs I can also see that chip has additional designated dpll for NCO mode, > and this statement: > "Numerically controlled oscillator (NCO) behavior allows system software to steer > DPLL frequency or synthesizer frequency with resolution better than 0.005 ppt." > > I am certainly not an expert on this, but seems like the NCO mode for this chip > is better than FREERUN, since signal produced on output is somehow higher quality. Herm, this seems complicated. Do you have a use case for this? Maybe we can skip it :D My reading of the quote is that there is an NCO which SW can control precisely. But that does not answer the questions: - is the NCO driven by system clock or can it be used in locked mode? - what is the "system software"? FW which based on temperature information, and whatever else compensates drift of system clock? or there are exposed registers to control the NCO?
Thu, May 11, 2023 at 10:51:43PM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Thursday, May 11, 2023 10:00 AM >> >>Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jakub Kicinski <kuba@kernel.org> >>>>Sent: Thursday, May 4, 2023 11:25 PM >>>> >>>>On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: >>>>> >+definitions: >>>>> >+ - >>>>> >+ 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, valid values for >>>>>DPLL_A_MODE >>>>> >+ attribute >>>>> >+ entries: >>>>> >+ - >>>>> >+ name: unspec >>>>> >>>>> In general, why exactly do we need unspec values in enums and CMDs? >>>>> What is the usecase. If there isn't please remove. >>>> >>>>+1 >>>> >>> >>>Sure, fixed. >>> >>>>> >+ doc: unspecified value >>>>> >+ - >>>>> >+ name: manual >>>> >>>>I think the documentation calls this "forced", still. >>>> >>> >>>Yes, good catch, fixed docs. >>> >>>>> >+ 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 >>>>> >>>>> Remove "no holdover available". This is not a state, this is a mode >>>>> configuration. If holdover is or isn't available, is a runtime info. >>>> >>>>Agreed, seems a little confusing now. Should we expose the system clk >>>>as a pin to be able to force lock to it? Or there's some extra magic >>>>at play here? >>> >>>In freerun you cannot lock to anything it, it just uses system clock from >>>one of designated chip wires (which is not a part of source pins pool) to >>>feed the dpll. Dpll would only stabilize that signal and pass it further. >>>Locking itself is some kind of magic, as it usually takes at least ~15 >>>seconds before it locks to a signal once it is selected. >>> >>>> >>>>> >+ - >>>>> >+ name: nco >>>>> >+ doc: dpll driven by Numerically Controlled Oscillator >>>> >>>>Noob question, what is NCO in terms of implementation? >>>>We source the signal from an arbitrary pin and FW / driver does >>>>the control? Or we always use system refclk and then tune? >>>> >>> >>>Documentation of chip we are using, stated NCO as similar to FREERUN, and >>it >> >>So how exactly this is different to freerun? Does user care or he would >>be fine with "freerun" in this case? My point is, isn't "NCO" some >>device specific thing that should be abstracted out here? >> > >Sure, it is device specific, some synchronizing circuits would have this >capability, while others would not. >Should be abstracted out? It is a good question.. shall user know that he is in >freerun with possibility to control the frequency or not? >Let's say we remove NCO, and have dpll with enabled FREERUN mode and pins >supporting multiple output frequencies. >How the one would know if those frequencies are supported only in >MANUAL/AUTOMATIC modes or also in the FREERUN mode? >In other words: As the user can I change a frequency of a dpll if active >mode is FREERUN? Okay, I think I'm deep in the DPLL infra you are pushing, but my understanding that you can control frequency in NCO mode is not present :/ That only means it may be confusing and not described properly. How do you control this frequency exactly? I see no such knob. Can't the oscilator be modeled as a pin and then you are not in freerun but locked this "internal pin"? We know how to control frequency there. > >I would say it is better to have such mode, we could argue on naming though. > >> >>>runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and >>>dividers before it reaches the output). >>>It doesn't count as an source pin, it uses signal form dedicated wire for >>>SYSTEM CLOCK. >>>In this case control over output frequency is done by synchronizer chip >>>firmware, but still it will not lock to any source pin signal. >>> >>>>> >+ render-max: true >>>>> >+ - >>>>> >+ type: enum >>>>> >+ name: lock-status >>>>> >+ doc: | >>>>> >+ provides information of dpll device lock status, valid values for >>>>> >+ DPLL_A_LOCK_STATUS attribute >>>>> >+ entries: >>>>> >+ - >>>>> >+ name: unspec >>>>> >+ doc: unspecified value >>>>> >+ - >>>>> >+ name: unlocked >>>>> >+ doc: | >>>>> >+ dpll was not yet locked to any valid source (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 >>>>> >>>>> Is it needed to mention the holdover mode. It's slightly confusing, >>>>> because user might understand that the lock-status is always "holdover" >>>>> in case of "holdover" mode. But it could be "unlocked", can't it? >>>>> Perhaps I don't understand the flows there correctly :/ >>>> >>>>Hm, if we want to make sure that holdover mode must result in holdover >>>>state then we need some extra atomicity requirements on the SET >>>>operation. To me it seems logical enough that after setting holdover >>>>mode we'll end up either in holdover or unlocked status, depending on >>>>lock status when request reached the HW. >>>> >>> >>>Improved the docs: >>> name: holdover >>> doc: | >>> dpll is in holdover state - lost a valid lock or was forced >>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >>> if it was not, the dpll's lock-status will remain >> >>"if it was not" does not really cope with the sentence above that. Could >>you iron-out the phrasing a bit please? > > >Hmmm, > name: holdover > doc: | > dpll is in holdover state - lost a valid lock or was forced > by selecting DPLL_MODE_HOLDOVER mode (latter possible only > when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, > if dpll lock-state was not DPLL_LOCK_STATUS_LOCKED, the > dpll's lock-state shall remain DPLL_LOCK_STATUS_UNLOCKED > even if DPLL_MODE_HOLDOVER was requested) > >Hope this is better? Okay. > > >Thank you! >Arkadiusz > >[...]
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Monday, May 15, 2023 11:31 AM > >Thu, May 11, 2023 at 10:51:43PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Thursday, May 11, 2023 10:00 AM >>> >>>Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>>From: Jakub Kicinski <kuba@kernel.org> >>>>>Sent: Thursday, May 4, 2023 11:25 PM >>>>> >>>>>On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: >>>>>> >+definitions: >>>>>> >+ - >>>>>> >+ 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, valid values for >>>>>>DPLL_A_MODE >>>>>> >+ attribute >>>>>> >+ entries: >>>>>> >+ - >>>>>> >+ name: unspec >>>>>> >>>>>> In general, why exactly do we need unspec values in enums and CMDs? >>>>>> What is the usecase. If there isn't please remove. >>>>> >>>>>+1 >>>>> >>>> >>>>Sure, fixed. >>>> >>>>>> >+ doc: unspecified value >>>>>> >+ - >>>>>> >+ name: manual >>>>> >>>>>I think the documentation calls this "forced", still. >>>>> >>>> >>>>Yes, good catch, fixed docs. >>>> >>>>>> >+ 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 >>>>>> >>>>>> Remove "no holdover available". This is not a state, this is a mode >>>>>> configuration. If holdover is or isn't available, is a runtime info. >>>>> >>>>>Agreed, seems a little confusing now. Should we expose the system clk >>>>>as a pin to be able to force lock to it? Or there's some extra magic >>>>>at play here? >>>> >>>>In freerun you cannot lock to anything it, it just uses system clock from >>>>one of designated chip wires (which is not a part of source pins pool) to >>>>feed the dpll. Dpll would only stabilize that signal and pass it further. >>>>Locking itself is some kind of magic, as it usually takes at least ~15 >>>>seconds before it locks to a signal once it is selected. >>>> >>>>> >>>>>> >+ - >>>>>> >+ name: nco >>>>>> >+ doc: dpll driven by Numerically Controlled Oscillator >>>>> >>>>>Noob question, what is NCO in terms of implementation? >>>>>We source the signal from an arbitrary pin and FW / driver does >>>>>the control? Or we always use system refclk and then tune? >>>>> >>>> >>>>Documentation of chip we are using, stated NCO as similar to FREERUN, and >>>>it >>> >>>So how exactly this is different to freerun? Does user care or he would >>>be fine with "freerun" in this case? My point is, isn't "NCO" some >>>device specific thing that should be abstracted out here? >>> >> >>Sure, it is device specific, some synchronizing circuits would have this >>capability, while others would not. >>Should be abstracted out? It is a good question.. shall user know that he is >>in >>freerun with possibility to control the frequency or not? >>Let's say we remove NCO, and have dpll with enabled FREERUN mode and pins >>supporting multiple output frequencies. >>How the one would know if those frequencies are supported only in >>MANUAL/AUTOMATIC modes or also in the FREERUN mode? >>In other words: As the user can I change a frequency of a dpll if active >>mode is FREERUN? > >Okay, I think I'm deep in the DPLL infra you are pushing, but my >understanding that you can control frequency in NCO mode is not present >:/ That only means it may be confusing and not described properly. >How do you control this frequency exactly? I see no such knob. > The set frequency is there already, although we miss phase offset I guess. But I have changed my mind on having this in the kernel.. Initially I have added this mode as our HW supports it, while thinking that dpll subsystem shall have this, and we will implement it one day.. But as we have not implemented it yet, let's leave work and discussion on this mode for the future, when someone will actually try to implement it. >Can't the oscilator be modeled as a pin and then you are not in freerun >but locked this "internal pin"? We know how to control frequency there. > Hmm, yeah probably could work this way. Thank you! Arkadiusz > >> >>I would say it is better to have such mode, we could argue on naming though. >> >>> >>>>runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and >>>>dividers before it reaches the output). >>>>It doesn't count as an source pin, it uses signal form dedicated wire for >>>>SYSTEM CLOCK. >>>>In this case control over output frequency is done by synchronizer chip >>>>firmware, but still it will not lock to any source pin signal. >>>> >>>>>> >+ render-max: true >>>>>> >+ - >>>>>> >+ type: enum >>>>>> >+ name: lock-status >>>>>> >+ doc: | >>>>>> >+ provides information of dpll device lock status, valid values for >>>>>> >+ DPLL_A_LOCK_STATUS attribute >>>>>> >+ entries: >>>>>> >+ - >>>>>> >+ name: unspec >>>>>> >+ doc: unspecified value >>>>>> >+ - >>>>>> >+ name: unlocked >>>>>> >+ doc: | >>>>>> >+ dpll was not yet locked to any valid source (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 >>>>>> >>>>>> Is it needed to mention the holdover mode. It's slightly confusing, >>>>>> because user might understand that the lock-status is always "holdover" >>>>>> in case of "holdover" mode. But it could be "unlocked", can't it? >>>>>> Perhaps I don't understand the flows there correctly :/ >>>>> >>>>>Hm, if we want to make sure that holdover mode must result in holdover >>>>>state then we need some extra atomicity requirements on the SET >>>>>operation. To me it seems logical enough that after setting holdover >>>>>mode we'll end up either in holdover or unlocked status, depending on >>>>>lock status when request reached the HW. >>>>> >>>> >>>>Improved the docs: >>>> name: holdover >>>> doc: | >>>> dpll is in holdover state - lost a valid lock or was forced >>>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >>>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >>>> if it was not, the dpll's lock-status will remain >>> >>>"if it was not" does not really cope with the sentence above that. Could >>>you iron-out the phrasing a bit please? >> >> >>Hmmm, >> name: holdover >> doc: | >> dpll is in holdover state - lost a valid lock or was forced >> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >> if dpll lock-state was not DPLL_LOCK_STATUS_LOCKED, the >> dpll's lock-state shall remain DPLL_LOCK_STATUS_UNLOCKED >> even if DPLL_MODE_HOLDOVER was requested) >> >>Hope this is better? > >Okay. > >> >> >>Thank you! >>Arkadiusz >> >>[...]
>From: Jakub Kicinski <kuba@kernel.org> >Sent: Friday, May 12, 2023 1:29 AM > >On Thu, 11 May 2023 20:53:40 +0000 Kubalewski, Arkadiusz wrote: >> >Because I think that'd be done by an NCO, no? >> >> From docs I can also see that chip has additional designated dpll for NCO >>mode, >> and this statement: >> "Numerically controlled oscillator (NCO) behavior allows system software >>to steer >> DPLL frequency or synthesizer frequency with resolution better than 0.005 >>ppt." >> >> I am certainly not an expert on this, but seems like the NCO mode for >this chip >> is better than FREERUN, since signal produced on output is somehow higher >quality. > >Herm, this seems complicated. Do you have a use case for this? >Maybe we can skip it :D > True! Actually yeah, I agree let's skip it, we don't have implemented example and there is no need for it for now, we will have to discuss it someday if someone want to give control over it to the user, and as Jiri already pointed, probably it could be done with using internal oscillator type of pin and just configurable frequency. Thus I will remove the NCO from the dpll modes. >My reading of the quote is that there is an NCO which SW can control >precisely. But that does not answer the questions: > - is the NCO driven by system clock Yes it is, other part of documentation lead to this conclusion. > or can it be used in locked mode? There is also a behavior called "NCO-hybrid", and as I understand works somehow as you described, dpll can lock to something but additional offset based on so called "System Clock Input (Jitter Reference)" pin is applied to the signal (the pin is also used for normal NCO mode). > - what is the "system software"? FW which based on temperature > information, and whatever else compensates drift of system clock? > or there are exposed registers to control the NCO? The synchronizer chip firmware provides the knobs in form of registers to control frequency offset for all the outputs, as well as for dpll's frequency offset. Thank you, Arkadiusz
Tue, May 16, 2023 at 02:05:38PM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Monday, May 15, 2023 11:31 AM >> >>Thu, May 11, 2023 at 10:51:43PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko <jiri@resnulli.us> >>>>Sent: Thursday, May 11, 2023 10:00 AM >>>> >>>>Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>>>From: Jakub Kicinski <kuba@kernel.org> >>>>>>Sent: Thursday, May 4, 2023 11:25 PM >>>>>> >>>>>>On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: >>>>>>> >+definitions: >>>>>>> >+ - >>>>>>> >+ 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, valid values for >>>>>>>DPLL_A_MODE >>>>>>> >+ attribute >>>>>>> >+ entries: >>>>>>> >+ - >>>>>>> >+ name: unspec >>>>>>> >>>>>>> In general, why exactly do we need unspec values in enums and CMDs? >>>>>>> What is the usecase. If there isn't please remove. >>>>>> >>>>>>+1 >>>>>> >>>>> >>>>>Sure, fixed. >>>>> >>>>>>> >+ doc: unspecified value >>>>>>> >+ - >>>>>>> >+ name: manual >>>>>> >>>>>>I think the documentation calls this "forced", still. >>>>>> >>>>> >>>>>Yes, good catch, fixed docs. >>>>> >>>>>>> >+ 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 >>>>>>> >>>>>>> Remove "no holdover available". This is not a state, this is a mode >>>>>>> configuration. If holdover is or isn't available, is a runtime info. >>>>>> >>>>>>Agreed, seems a little confusing now. Should we expose the system clk >>>>>>as a pin to be able to force lock to it? Or there's some extra magic >>>>>>at play here? >>>>> >>>>>In freerun you cannot lock to anything it, it just uses system clock from >>>>>one of designated chip wires (which is not a part of source pins pool) to >>>>>feed the dpll. Dpll would only stabilize that signal and pass it further. >>>>>Locking itself is some kind of magic, as it usually takes at least ~15 >>>>>seconds before it locks to a signal once it is selected. >>>>> >>>>>> >>>>>>> >+ - >>>>>>> >+ name: nco >>>>>>> >+ doc: dpll driven by Numerically Controlled Oscillator >>>>>> >>>>>>Noob question, what is NCO in terms of implementation? >>>>>>We source the signal from an arbitrary pin and FW / driver does >>>>>>the control? Or we always use system refclk and then tune? >>>>>> >>>>> >>>>>Documentation of chip we are using, stated NCO as similar to FREERUN, and >>>>>it >>>> >>>>So how exactly this is different to freerun? Does user care or he would >>>>be fine with "freerun" in this case? My point is, isn't "NCO" some >>>>device specific thing that should be abstracted out here? >>>> >>> >>>Sure, it is device specific, some synchronizing circuits would have this >>>capability, while others would not. >>>Should be abstracted out? It is a good question.. shall user know that he is >>>in >>>freerun with possibility to control the frequency or not? >>>Let's say we remove NCO, and have dpll with enabled FREERUN mode and pins >>>supporting multiple output frequencies. >>>How the one would know if those frequencies are supported only in >>>MANUAL/AUTOMATIC modes or also in the FREERUN mode? >>>In other words: As the user can I change a frequency of a dpll if active >>>mode is FREERUN? >> >>Okay, I think I'm deep in the DPLL infra you are pushing, but my >>understanding that you can control frequency in NCO mode is not present >>:/ That only means it may be confusing and not described properly. >>How do you control this frequency exactly? I see no such knob. >> > >The set frequency is there already, although we miss phase offset I guess. Yeah, but on a pin, right? > >But I have changed my mind on having this in the kernel.. >Initially I have added this mode as our HW supports it, while thinking that >dpll subsystem shall have this, and we will implement it one day.. >But as we have not implemented it yet, let's leave work and discussion on >this mode for the future, when someone will actually try to implement it. Yeah, let's drop it then. One less confusing thing to wrap a head around :) > >>Can't the oscilator be modeled as a pin and then you are not in freerun >>but locked this "internal pin"? We know how to control frequency there. >> > >Hmm, yeah probably could work this way. > > >Thank you! >Arkadiusz > >> >>> >>>I would say it is better to have such mode, we could argue on naming though. >>> >>>> >>>>>runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and >>>>>dividers before it reaches the output). >>>>>It doesn't count as an source pin, it uses signal form dedicated wire for >>>>>SYSTEM CLOCK. >>>>>In this case control over output frequency is done by synchronizer chip >>>>>firmware, but still it will not lock to any source pin signal. >>>>> >>>>>>> >+ render-max: true >>>>>>> >+ - >>>>>>> >+ type: enum >>>>>>> >+ name: lock-status >>>>>>> >+ doc: | >>>>>>> >+ provides information of dpll device lock status, valid values for >>>>>>> >+ DPLL_A_LOCK_STATUS attribute >>>>>>> >+ entries: >>>>>>> >+ - >>>>>>> >+ name: unspec >>>>>>> >+ doc: unspecified value >>>>>>> >+ - >>>>>>> >+ name: unlocked >>>>>>> >+ doc: | >>>>>>> >+ dpll was not yet locked to any valid source (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 >>>>>>> >>>>>>> Is it needed to mention the holdover mode. It's slightly confusing, >>>>>>> because user might understand that the lock-status is always "holdover" >>>>>>> in case of "holdover" mode. But it could be "unlocked", can't it? >>>>>>> Perhaps I don't understand the flows there correctly :/ >>>>>> >>>>>>Hm, if we want to make sure that holdover mode must result in holdover >>>>>>state then we need some extra atomicity requirements on the SET >>>>>>operation. To me it seems logical enough that after setting holdover >>>>>>mode we'll end up either in holdover or unlocked status, depending on >>>>>>lock status when request reached the HW. >>>>>> >>>>> >>>>>Improved the docs: >>>>> name: holdover >>>>> doc: | >>>>> dpll is in holdover state - lost a valid lock or was forced >>>>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >>>>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >>>>> if it was not, the dpll's lock-status will remain >>>> >>>>"if it was not" does not really cope with the sentence above that. Could >>>>you iron-out the phrasing a bit please? >>> >>> >>>Hmmm, >>> name: holdover >>> doc: | >>> dpll is in holdover state - lost a valid lock or was forced >>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >>> if dpll lock-state was not DPLL_LOCK_STATUS_LOCKED, the >>> dpll's lock-state shall remain DPLL_LOCK_STATUS_UNLOCKED >>> even if DPLL_MODE_HOLDOVER was requested) >>> >>>Hope this is better? >> >>Okay. >> >>> >>> >>>Thank you! >>>Arkadiusz >>> >>>[...]
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Tuesday, May 16, 2023 4:34 PM > >Tue, May 16, 2023 at 02:05:38PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Monday, May 15, 2023 11:31 AM >>> >>>Thu, May 11, 2023 at 10:51:43PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>>From: Jiri Pirko <jiri@resnulli.us> >>>>>Sent: Thursday, May 11, 2023 10:00 AM >>>>> >>>>>Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@intel.com >>>>>wrote: >>>>>>>From: Jakub Kicinski <kuba@kernel.org> >>>>>>>Sent: Thursday, May 4, 2023 11:25 PM >>>>>>> >>>>>>>On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: >>>>>>>> >+definitions: >>>>>>>> >+ - >>>>>>>> >+ 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, valid values for >>>>>>>> >DPLL_A_MODE >>>>>>>> >+ attribute >>>>>>>> >+ entries: >>>>>>>> >+ - >>>>>>>> >+ name: unspec >>>>>>>> >>>>>>>> In general, why exactly do we need unspec values in enums and CMDs? >>>>>>>> What is the usecase. If there isn't please remove. >>>>>>> >>>>>>>+1 >>>>>>> >>>>>> >>>>>>Sure, fixed. >>>>>> >>>>>>>> >+ doc: unspecified value >>>>>>>> >+ - >>>>>>>> >+ name: manual >>>>>>> >>>>>>>I think the documentation calls this "forced", still. >>>>>>> >>>>>> >>>>>>Yes, good catch, fixed docs. >>>>>> >>>>>>>> >+ 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 >>>>>>>> >>>>>>>> Remove "no holdover available". This is not a state, this is a mode >>>>>>>> configuration. If holdover is or isn't available, is a runtime info. >>>>>>> >>>>>>>Agreed, seems a little confusing now. Should we expose the system clk >>>>>>>as a pin to be able to force lock to it? Or there's some extra magic >>>>>>>at play here? >>>>>> >>>>>>In freerun you cannot lock to anything it, it just uses system clock from >>>>>>one of designated chip wires (which is not a part of source pins pool) to >>>>>>feed the dpll. Dpll would only stabilize that signal and pass it further. >>>>>>Locking itself is some kind of magic, as it usually takes at least ~15 >>>>>>seconds before it locks to a signal once it is selected. >>>>>> >>>>>>> >>>>>>>> >+ - >>>>>>>> >+ name: nco >>>>>>>> >+ doc: dpll driven by Numerically Controlled Oscillator >>>>>>> >>>>>>>Noob question, what is NCO in terms of implementation? >>>>>>>We source the signal from an arbitrary pin and FW / driver does >>>>>>>the control? Or we always use system refclk and then tune? >>>>>>> >>>>>> >>>>>>Documentation of chip we are using, stated NCO as similar to FREERUN, and >>>>>>it >>>>> >>>>>So how exactly this is different to freerun? Does user care or he would >>>>>be fine with "freerun" in this case? My point is, isn't "NCO" some >>>>>device specific thing that should be abstracted out here? >>>>> >>>> >>>>Sure, it is device specific, some synchronizing circuits would have this >>>>capability, while others would not. >>>>Should be abstracted out? It is a good question.. shall user know that he >>>>is in >>>>freerun with possibility to control the frequency or not? >>>>Let's say we remove NCO, and have dpll with enabled FREERUN mode and pins >>>>supporting multiple output frequencies. >>>>How the one would know if those frequencies are supported only in >>>>MANUAL/AUTOMATIC modes or also in the FREERUN mode? >>>>In other words: As the user can I change a frequency of a dpll if active >>>>mode is FREERUN? >>> >>>Okay, I think I'm deep in the DPLL infra you are pushing, but my >>>understanding that you can control frequency in NCO mode is not present >>>:/ That only means it may be confusing and not described properly. >>>How do you control this frequency exactly? I see no such knob. >>> >> >>The set frequency is there already, although we miss phase offset I guess. > >Yeah, but on a pin, right? > > Yes frequency of an output pin is configurable, phase offset for a dpll or output is not there, we might think of adding it.. > >> >>But I have changed my mind on having this in the kernel.. >>Initially I have added this mode as our HW supports it, while thinking that >>dpll subsystem shall have this, and we will implement it one day.. >>But as we have not implemented it yet, let's leave work and discussion on >>this mode for the future, when someone will actually try to implement it. > >Yeah, let's drop it then. One less confusing thing to wrap a head around :) > Dropped. Thank you! Arkadiusz > >> >>>Can't the oscilator be modeled as a pin and then you are not in freerun >>>but locked this "internal pin"? We know how to control frequency there. >>> >> >>Hmm, yeah probably could work this way. >> >> >>Thank you! >>Arkadiusz >> >>> >>>> >>>>I would say it is better to have such mode, we could argue on naming >>>>>though. >>>> >>>>> >>>>>>runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and >>>>>>dividers before it reaches the output). >>>>>>It doesn't count as an source pin, it uses signal form dedicated wire for >>>>>>SYSTEM CLOCK. >>>>>>In this case control over output frequency is done by synchronizer chip >>>>>>firmware, but still it will not lock to any source pin signal. >>>>>> >>>>>>>> >+ render-max: true >>>>>>>> >+ - >>>>>>>> >+ type: enum >>>>>>>> >+ name: lock-status >>>>>>>> >+ doc: | >>>>>>>> >+ provides information of dpll device lock status, valid >>>>>>>> >>values for >>>>>>>> >+ DPLL_A_LOCK_STATUS attribute >>>>>>>> >+ entries: >>>>>>>> >+ - >>>>>>>> >+ name: unspec >>>>>>>> >+ doc: unspecified value >>>>>>>> >+ - >>>>>>>> >+ name: unlocked >>>>>>>> >+ doc: | >>>>>>>> >+ dpll was not yet locked to any valid source (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 >>>>>>>> >>>>>>>> Is it needed to mention the holdover mode. It's slightly confusing, >>>>>>>> because user might understand that the lock-status is always "holdover" >>>>>>>> in case of "holdover" mode. But it could be "unlocked", can't it? >>>>>>>> Perhaps I don't understand the flows there correctly :/ >>>>>>> >>>>>>>Hm, if we want to make sure that holdover mode must result in holdover >>>>>>>state then we need some extra atomicity requirements on the SET >>>>>>>operation. To me it seems logical enough that after setting holdover >>>>>>>mode we'll end up either in holdover or unlocked status, depending on >>>>>>>lock status when request reached the HW. >>>>>>> >>>>>> >>>>>>Improved the docs: >>>>>> name: holdover >>>>>> doc: | >>>>>> dpll is in holdover state - lost a valid lock or was forced >>>>>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >>>>>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >>>>>> if it was not, the dpll's lock-status will remain >>>>> >>>>>"if it was not" does not really cope with the sentence above that. Could >>>>>you iron-out the phrasing a bit please? >>>> >>>> >>>>Hmmm, >>>> name: holdover >>>> doc: | >>>> dpll is in holdover state - lost a valid lock or was forced >>>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >>>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >>>> if dpll lock-state was not DPLL_LOCK_STATUS_LOCKED, the >>>> dpll's lock-state shall remain DPLL_LOCK_STATUS_UNLOCKED >>>> even if DPLL_MODE_HOLDOVER was requested) >>>> >>>>Hope this is better? >>> >>>Okay. >>> >>>> >>>> >>>>Thank you! >>>>Arkadiusz >>>> >>>>[...]
Thu, May 18, 2023 at 03:24:45PM CEST, arkadiusz.kubalewski@intel.com wrote: > >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Tuesday, May 16, 2023 4:34 PM >> >>Tue, May 16, 2023 at 02:05:38PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko <jiri@resnulli.us> >>>>Sent: Monday, May 15, 2023 11:31 AM >>>> >>>>Thu, May 11, 2023 at 10:51:43PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>>>From: Jiri Pirko <jiri@resnulli.us> >>>>>>Sent: Thursday, May 11, 2023 10:00 AM >>>>>> >>>>>>Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@intel.com >>>>>>wrote: >>>>>>>>From: Jakub Kicinski <kuba@kernel.org> >>>>>>>>Sent: Thursday, May 4, 2023 11:25 PM >>>>>>>> >>>>>>>>On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote: >>>>>>>>> >+definitions: >>>>>>>>> >+ - >>>>>>>>> >+ 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, valid values for >>>>>>>>> >DPLL_A_MODE >>>>>>>>> >+ attribute >>>>>>>>> >+ entries: >>>>>>>>> >+ - >>>>>>>>> >+ name: unspec >>>>>>>>> >>>>>>>>> In general, why exactly do we need unspec values in enums and CMDs? >>>>>>>>> What is the usecase. If there isn't please remove. >>>>>>>> >>>>>>>>+1 >>>>>>>> >>>>>>> >>>>>>>Sure, fixed. >>>>>>> >>>>>>>>> >+ doc: unspecified value >>>>>>>>> >+ - >>>>>>>>> >+ name: manual >>>>>>>> >>>>>>>>I think the documentation calls this "forced", still. >>>>>>>> >>>>>>> >>>>>>>Yes, good catch, fixed docs. >>>>>>> >>>>>>>>> >+ 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 >>>>>>>>> >>>>>>>>> Remove "no holdover available". This is not a state, this is a mode >>>>>>>>> configuration. If holdover is or isn't available, is a runtime info. >>>>>>>> >>>>>>>>Agreed, seems a little confusing now. Should we expose the system clk >>>>>>>>as a pin to be able to force lock to it? Or there's some extra magic >>>>>>>>at play here? >>>>>>> >>>>>>>In freerun you cannot lock to anything it, it just uses system clock from >>>>>>>one of designated chip wires (which is not a part of source pins pool) to >>>>>>>feed the dpll. Dpll would only stabilize that signal and pass it further. >>>>>>>Locking itself is some kind of magic, as it usually takes at least ~15 >>>>>>>seconds before it locks to a signal once it is selected. >>>>>>> >>>>>>>> >>>>>>>>> >+ - >>>>>>>>> >+ name: nco >>>>>>>>> >+ doc: dpll driven by Numerically Controlled Oscillator >>>>>>>> >>>>>>>>Noob question, what is NCO in terms of implementation? >>>>>>>>We source the signal from an arbitrary pin and FW / driver does >>>>>>>>the control? Or we always use system refclk and then tune? >>>>>>>> >>>>>>> >>>>>>>Documentation of chip we are using, stated NCO as similar to FREERUN, and >>>>>>>it >>>>>> >>>>>>So how exactly this is different to freerun? Does user care or he would >>>>>>be fine with "freerun" in this case? My point is, isn't "NCO" some >>>>>>device specific thing that should be abstracted out here? >>>>>> >>>>> >>>>>Sure, it is device specific, some synchronizing circuits would have this >>>>>capability, while others would not. >>>>>Should be abstracted out? It is a good question.. shall user know that he >>>>>is in >>>>>freerun with possibility to control the frequency or not? >>>>>Let's say we remove NCO, and have dpll with enabled FREERUN mode and pins >>>>>supporting multiple output frequencies. >>>>>How the one would know if those frequencies are supported only in >>>>>MANUAL/AUTOMATIC modes or also in the FREERUN mode? >>>>>In other words: As the user can I change a frequency of a dpll if active >>>>>mode is FREERUN? >>>> >>>>Okay, I think I'm deep in the DPLL infra you are pushing, but my >>>>understanding that you can control frequency in NCO mode is not present >>>>:/ That only means it may be confusing and not described properly. >>>>How do you control this frequency exactly? I see no such knob. >>>> >>> >>>The set frequency is there already, although we miss phase offset I guess. >> >>Yeah, but on a pin, right? >> >> > >Yes frequency of an output pin is configurable, phase offset for a dpll or >output is not there, we might think of adding it.. But wait, we are talking about controllig freq of NCO, that has to be a different knob. Anyway, it is dropped, so the discussion is poitless now. > >> >>> >>>But I have changed my mind on having this in the kernel.. >>>Initially I have added this mode as our HW supports it, while thinking that >>>dpll subsystem shall have this, and we will implement it one day.. >>>But as we have not implemented it yet, let's leave work and discussion on >>>this mode for the future, when someone will actually try to implement it. >> >>Yeah, let's drop it then. One less confusing thing to wrap a head around :) >> > >Dropped. > >Thank you! >Arkadiusz > >> >>> >>>>Can't the oscilator be modeled as a pin and then you are not in freerun >>>>but locked this "internal pin"? We know how to control frequency there. >>>> >>> >>>Hmm, yeah probably could work this way. >>> >>> >>>Thank you! >>>Arkadiusz >>> >>>> >>>>> >>>>>I would say it is better to have such mode, we could argue on naming >>>>>>though. >>>>> >>>>>> >>>>>>>runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and >>>>>>>dividers before it reaches the output). >>>>>>>It doesn't count as an source pin, it uses signal form dedicated wire for >>>>>>>SYSTEM CLOCK. >>>>>>>In this case control over output frequency is done by synchronizer chip >>>>>>>firmware, but still it will not lock to any source pin signal. >>>>>>> >>>>>>>>> >+ render-max: true >>>>>>>>> >+ - >>>>>>>>> >+ type: enum >>>>>>>>> >+ name: lock-status >>>>>>>>> >+ doc: | >>>>>>>>> >+ provides information of dpll device lock status, valid >>>>>>>>> >>values for >>>>>>>>> >+ DPLL_A_LOCK_STATUS attribute >>>>>>>>> >+ entries: >>>>>>>>> >+ - >>>>>>>>> >+ name: unspec >>>>>>>>> >+ doc: unspecified value >>>>>>>>> >+ - >>>>>>>>> >+ name: unlocked >>>>>>>>> >+ doc: | >>>>>>>>> >+ dpll was not yet locked to any valid source (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 >>>>>>>>> >>>>>>>>> Is it needed to mention the holdover mode. It's slightly confusing, >>>>>>>>> because user might understand that the lock-status is always "holdover" >>>>>>>>> in case of "holdover" mode. But it could be "unlocked", can't it? >>>>>>>>> Perhaps I don't understand the flows there correctly :/ >>>>>>>> >>>>>>>>Hm, if we want to make sure that holdover mode must result in holdover >>>>>>>>state then we need some extra atomicity requirements on the SET >>>>>>>>operation. To me it seems logical enough that after setting holdover >>>>>>>>mode we'll end up either in holdover or unlocked status, depending on >>>>>>>>lock status when request reached the HW. >>>>>>>> >>>>>>> >>>>>>>Improved the docs: >>>>>>> name: holdover >>>>>>> doc: | >>>>>>> dpll is in holdover state - lost a valid lock or was forced >>>>>>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >>>>>>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >>>>>>> if it was not, the dpll's lock-status will remain >>>>>> >>>>>>"if it was not" does not really cope with the sentence above that. Could >>>>>>you iron-out the phrasing a bit please? >>>>> >>>>> >>>>>Hmmm, >>>>> name: holdover >>>>> doc: | >>>>> dpll is in holdover state - lost a valid lock or was forced >>>>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only >>>>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED, >>>>> if dpll lock-state was not DPLL_LOCK_STATUS_LOCKED, the >>>>> dpll's lock-state shall remain DPLL_LOCK_STATUS_UNLOCKED >>>>> even if DPLL_MODE_HOLDOVER was requested) >>>>> >>>>>Hope this is better? >>>> >>>>Okay. >>>> >>>>> >>>>> >>>>>Thank you! >>>>>Arkadiusz >>>>> >>>>>[...]
diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml new file mode 100644 index 000000000000..67ca0f6cf2d5 --- /dev/null +++ b/Documentation/netlink/specs/dpll.yaml @@ -0,0 +1,472 @@ +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) + +name: dpll + +doc: DPLL subsystem. + +definitions: + - + type: enum + name: mode + doc: | + working-modes a dpll can support, differentiate if and how dpll selects + one of its sources to syntonize with it, valid values for DPLL_A_MODE + attribute + 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: 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 source (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: const + name: temp-divider + value: 10 + doc: | + temperature divider allowing userspace to calculate the + temperature as float with single digit precision. + Value of (DPLL_A_TEMP / DPLL_TEMP_DIVIDER) is integer part of + tempearture value. + Value of (DPLL_A_TEMP % DPLL_TEMP_DIVIDER) is fractional part of + temperature value. + - + type: enum + name: type + doc: type of dpll, valid values for DPLL_A_TYPE attribute + entries: + - + name: 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: pin-type + doc: | + defines possible 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-direction + doc: | + defines possible direction of a pin, valid values for + DPLL_A_PIN_DIRECTION attribute + 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: const + name: pin-frequency-1-hz + value: 1 + - + type: const + name: pin-frequency-10-mhz + value: 10000000 + - + type: enum + name: pin-state + doc: | + defines possible states of a pin, valid values for + DPLL_A_PIN_STATE attribute + entries: + - + name: unspec + doc: unspecified value + - + name: connected + doc: pin connected, active source of phase locked loop + - + name: disconnected + doc: pin disconnected, not considered as a valid source + - + name: selectable + doc: pin enabled for automatic source selection + render-max: true + - + type: flags + name: pin-caps + doc: | + defines possible capabilities of a pin, valid flags on + DPLL_A_PIN_CAPS attribute + entries: + - + name: direction-can-change + - + name: priority-can-change + - + name: state-can-change + - + 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 + + +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: lock-status + type: u8 + enum: lock-status + - + name: temp + type: s32 + - + name: clock-id + type: u64 + - + name: type + type: u8 + enum: type + - + name: pin-idx + type: u32 + - + name: pin-label + type: string + - + name: pin-type + type: u8 + enum: pin-type + - + name: pin-direction + type: u8 + enum: pin-direction + - + name: pin-frequency + type: u64 + - + name: pin-frequency-supported + type: nest + multi-attr: true + nested-attributes: pin-frequency-range + - + name: pin-frequency-min + type: u64 + - + name: pin-frequency-max + type: u64 + - + name: pin-prio + type: u32 + - + name: pin-state + type: u8 + enum: pin-state + - + name: pin-parent + type: nest + multi-attr: true + nested-attributes: pin-parent + - + 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: lock-status + type: u8 + enum: lock-status + - + name: temp + type: s32 + - + name: clock-id + type: u64 + - + name: type + type: u8 + enum: type + - + name: pin-prio + type: u32 + value: 19 + - + name: pin-state + type: u8 + enum: pin-state + - + name: pin-parent + subset-of: dpll + attributes: + - + name: pin-state + type: u8 + value: 20 + enum: pin-state + - + name: pin-parent-idx + type: u32 + value: 22 + - + name: pin-rclk-device + type: string + - + name: pin-frequency-range + subset-of: dpll + attributes: + - + name: pin-frequency-min + type: u64 + value: 17 + - + name: pin-frequency-max + type: u64 + +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: &pin-attrs + attributes: + - pin-idx + - pin-label + - pin-type + - pin-direction + - pin-frequency + - pin-frequency-supported + - pin-parent + - pin-rclk-device + - pin-dpll-caps + - device + + dump: + pre: dpll-pin-pre-dumpit + post: dpll-pin-post-dumpit + request: + attributes: + - id + - bus-name + - dev-name + reply: *pin-attrs + + - + 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-state + - pin-parent-idx + +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..2f8643f401b0 --- /dev/null +++ b/drivers/dpll/dpll_nl.c @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR 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_U64, }, + [DPLL_A_PIN_DIRECTION] = NLA_POLICY_MAX(NLA_U8, 2), + [DPLL_A_PIN_PRIO] = { .type = NLA_U32, }, + [DPLL_A_PIN_STATE] = NLA_POLICY_MAX(NLA_U8, 3), + [DPLL_A_PIN_PARENT_IDX] = { .type = NLA_U32, }, +}; + +/* Ops table for dpll */ +static const struct genl_split_ops dpll_nl_ops[] = { + { + .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..57ab2da562ba --- /dev/null +++ b/drivers/dpll/dpll_nl.h @@ -0,0 +1,42 @@ +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR 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..e188bc189754 --- /dev/null +++ b/include/uapi/linux/dpll.h @@ -0,0 +1,202 @@ +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */ +/* 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 + +/** + * enum dpll_mode - working-modes a dpll can support, differentiate if and how + * dpll selects one of its sources to syntonize with it, valid values for + * DPLL_A_MODE attribute + * @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_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 source (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) +}; + +#define DPLL_TEMP_DIVIDER 10 + +/** + * 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_pin_type - defines possible 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_direction - defines possible direction of a pin, valid values + * for DPLL_A_PIN_DIRECTION attribute + * @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) +}; + +#define DPLL_PIN_FREQUENCY_1_HZ 1 +#define DPLL_PIN_FREQUENCY_10_MHZ 10000000 + +/** + * enum dpll_pin_state - defines possible states of a pin, valid values for + * DPLL_A_PIN_STATE attribute + * @DPLL_PIN_STATE_UNSPEC: unspecified value + * @DPLL_PIN_STATE_CONNECTED: pin connected, active source of phase locked loop + * @DPLL_PIN_STATE_DISCONNECTED: pin disconnected, not considered as a valid + * source + * @DPLL_PIN_STATE_SELECTABLE: pin enabled for automatic source selection + */ +enum dpll_pin_state { + DPLL_PIN_STATE_UNSPEC, + DPLL_PIN_STATE_CONNECTED, + DPLL_PIN_STATE_DISCONNECTED, + DPLL_PIN_STATE_SELECTABLE, + + __DPLL_PIN_STATE_MAX, + DPLL_PIN_STATE_MAX = (__DPLL_PIN_STATE_MAX - 1) +}; + +/** + * enum dpll_pin_caps - defines possible capabilities of a pin, valid flags on + * DPLL_A_PIN_CAPS attribute + */ +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 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 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_LOCK_STATUS, + DPLL_A_TEMP, + DPLL_A_CLOCK_ID, + DPLL_A_TYPE, + DPLL_A_PIN_IDX, + DPLL_A_PIN_LABEL, + DPLL_A_PIN_TYPE, + DPLL_A_PIN_DIRECTION, + DPLL_A_PIN_FREQUENCY, + DPLL_A_PIN_FREQUENCY_SUPPORTED, + DPLL_A_PIN_FREQUENCY_MIN, + DPLL_A_PIN_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 = 1, + 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 */