[PATCH net-next v3 0/2] dpll: expose clock quality level

[PATCH net-next v3 0/2] dpll: expose clock quality level

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Some device driver might know the quality of the clock it is running.
In order to expose the information to the user, introduce new netlink
attribute and dpll device op. Implement the op in mlx5 driver.

Example:
$ ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --dump device-get
[{'clock-id': 13316852727532664826,
  'clock-quality-level': ['itu-opt1-eeec'],    <<<<<<<<<<<<<<<<<
  'id': 0,
  'lock-status': 'unlocked',
  'lock-status-error': 'none',
  'mode': 'manual',
  'mode-supported': ['manual'],
  'module-name': 'mlx5_dpll',
  'type': 'eec'}]

---
v2->v3:
- changed "itu" prefix to "itu-opt1"
- changed driver op to pass bitmap to allow to set multiple qualities
  and pass it to user over multiple attrs
- enhanced the documentation a bit
v1->v2:
- extended quality enum documentation
- added "itu" prefix to the enum values

Jiri Pirko (2):
  dpll: add clock quality level attribute and op
  net/mlx5: DPLL, Add clock quality level op implementation

 Documentation/netlink/specs/dpll.yaml         | 35 ++++++++
 drivers/dpll/dpll_netlink.c                   | 24 ++++++
 .../net/ethernet/mellanox/mlx5/core/dpll.c    | 81 +++++++++++++++++++
 include/linux/dpll.h                          |  4 +
 include/uapi/linux/dpll.h                     | 24 ++++++
 5 files changed, 168 insertions(+)

-- 
2.47.0

[PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

In order to allow driver expose quality level of the clock it is
running, introduce a new netlink attr with enum to carry it to the
userspace. Also, introduce an op the dpll netlink code calls into the
driver to obtain the value.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v2->v3:
- changed "itu" prefix to "itu-opt1"
- changed driver op to pass bitmap to allow to set multiple qualities
  and pass it to user over multiple attrs
- enhanced the documentation a bit
v1->v2:
- extended quality enum documentation
- added "itu" prefix to the enum values
---
 Documentation/netlink/specs/dpll.yaml | 35 +++++++++++++++++++++++++++
 drivers/dpll/dpll_netlink.c           | 24 ++++++++++++++++++
 include/linux/dpll.h                  |  4 +++
 include/uapi/linux/dpll.h             | 24 ++++++++++++++++++
 4 files changed, 87 insertions(+)

diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
index f2894ca35de8..0bd708e136ff 100644
--- a/Documentation/netlink/specs/dpll.yaml
+++ b/Documentation/netlink/specs/dpll.yaml
@@ -85,6 +85,36 @@ definitions:
           This may happen for example if dpll device was previously
           locked on an input pin of type PIN_TYPE_SYNCE_ETH_PORT.
     render-max: true
+  -
+    type: enum
+    name: clock-quality-level
+    doc: |
+      level of quality of a clock device. This mainly applies when
+      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
+      The current list is defined according to the table 11-7 contained
+      in ITU-T G.8264/Y.1364 document. One may extend this list freely
+      by other ITU-T defined clock qualities, or different ones defined
+      by another standardization body (for those, please use
+      different prefix).
+    entries:
+      -
+        name: itu-opt1-prc
+        value: 1
+      -
+        name: itu-opt1-ssu-a
+      -
+        name: itu-opt1-ssu-b
+      -
+        name: itu-opt1-eec1
+      -
+        name: itu-opt1-prtc
+      -
+        name: itu-opt1-eprtc
+      -
+        name: itu-opt1-eeec
+      -
+        name: itu-opt1-eprc
+    render-max: true
   -
     type: const
     name: temp-divider
@@ -252,6 +282,11 @@ attribute-sets:
         name: lock-status-error
         type: u32
         enum: lock-status-error
+      -
+        name: clock-quality-level
+        type: u32
+        enum: clock-quality-level
+        multi-attr: true
   -
     name: pin
     enum-name: dpll_a_pin
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index fc0280dcddd1..c130f87147fa 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -169,6 +169,27 @@ dpll_msg_add_temp(struct sk_buff *msg, struct dpll_device *dpll,
 	return 0;
 }
 
+static int
+dpll_msg_add_clock_quality_level(struct sk_buff *msg, struct dpll_device *dpll,
+				 struct netlink_ext_ack *extack)
+{
+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
+	DECLARE_BITMAP(qls, DPLL_CLOCK_QUALITY_LEVEL_MAX) = { 0 };
+	enum dpll_clock_quality_level ql;
+	int ret;
+
+	if (!ops->clock_quality_level_get)
+		return 0;
+	ret = ops->clock_quality_level_get(dpll, dpll_priv(dpll), qls, extack);
+	if (ret)
+		return ret;
+	for_each_set_bit(ql, qls, DPLL_CLOCK_QUALITY_LEVEL_MAX)
+		if (nla_put_u32(msg, DPLL_A_CLOCK_QUALITY_LEVEL, ql))
+			return -EMSGSIZE;
+
+	return 0;
+}
+
 static int
 dpll_msg_add_pin_prio(struct sk_buff *msg, struct dpll_pin *pin,
 		      struct dpll_pin_ref *ref,
@@ -557,6 +578,9 @@ dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
 	if (ret)
 		return ret;
 	ret = dpll_msg_add_lock_status(msg, dpll, extack);
+	if (ret)
+		return ret;
+	ret = dpll_msg_add_clock_quality_level(msg, dpll, extack);
 	if (ret)
 		return ret;
 	ret = dpll_msg_add_mode(msg, dpll, extack);
diff --git a/include/linux/dpll.h b/include/linux/dpll.h
index 81f7b623d0ba..5e4f9ab1cf75 100644
--- a/include/linux/dpll.h
+++ b/include/linux/dpll.h
@@ -26,6 +26,10 @@ struct dpll_device_ops {
 			       struct netlink_ext_ack *extack);
 	int (*temp_get)(const struct dpll_device *dpll, void *dpll_priv,
 			s32 *temp, struct netlink_ext_ack *extack);
+	int (*clock_quality_level_get)(const struct dpll_device *dpll,
+				       void *dpll_priv,
+				       unsigned long *qls,
+				       struct netlink_ext_ack *extack);
 };
 
 struct dpll_pin_ops {
diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
index b0654ade7b7e..4b37542eace3 100644
--- a/include/uapi/linux/dpll.h
+++ b/include/uapi/linux/dpll.h
@@ -79,6 +79,29 @@ enum dpll_lock_status_error {
 	DPLL_LOCK_STATUS_ERROR_MAX = (__DPLL_LOCK_STATUS_ERROR_MAX - 1)
 };
 
+/**
+ * enum dpll_clock_quality_level - level of quality of a clock device. This
+ *   mainly applies when the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
+ *   The current list is defined according to the table 11-7 contained in ITU-T
+ *   G.8264/Y.1364 document. One may extend this list freely by other ITU-T
+ *   defined clock qualities, or different ones defined by another
+ *   standardization body (for those, please use different prefix).
+ */
+enum dpll_clock_quality_level {
+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_PRC = 1,
+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_SSU_A,
+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_SSU_B,
+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EEC1,
+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_PRTC,
+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EPRTC,
+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EEEC,
+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EPRC,
+
+	/* private: */
+	__DPLL_CLOCK_QUALITY_LEVEL_MAX,
+	DPLL_CLOCK_QUALITY_LEVEL_MAX = (__DPLL_CLOCK_QUALITY_LEVEL_MAX - 1)
+};
+
 #define DPLL_TEMP_DIVIDER	1000
 
 /**
@@ -180,6 +203,7 @@ enum dpll_a {
 	DPLL_A_TEMP,
 	DPLL_A_TYPE,
 	DPLL_A_LOCK_STATUS_ERROR,
+	DPLL_A_CLOCK_QUALITY_LEVEL,
 
 	__DPLL_A_MAX,
 	DPLL_A_MAX = (__DPLL_A_MAX - 1)
-- 
2.47.0

[PATCH net-next v3 2/2] net/mlx5: DPLL, Add clock quality level op implementation

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Use MSECQ register to query clock quality from firmware. Implement the
dpll op and fill-up the quality level value properly.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v2->v3:
- changed to fill-up quality level to bitmap
- changed "itu" prefix to "itu-opt1"
v1->v2:
- added "itu" prefix to the enum values
---
 .../net/ethernet/mellanox/mlx5/core/dpll.c    | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
index 904e08de852e..31142f6cc372 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
@@ -166,9 +166,90 @@ static int mlx5_dpll_device_mode_get(const struct dpll_device *dpll,
 	return 0;
 }
 
+enum {
+	MLX5_DPLL_SSM_CODE_PRC = 0b0010,
+	MLX5_DPLL_SSM_CODE_SSU_A = 0b0100,
+	MLX5_DPLL_SSM_CODE_SSU_B = 0b1000,
+	MLX5_DPLL_SSM_CODE_EEC1 = 0b1011,
+	MLX5_DPLL_SSM_CODE_PRTC = 0b0010,
+	MLX5_DPLL_SSM_CODE_EPRTC = 0b0010,
+	MLX5_DPLL_SSM_CODE_EEEC = 0b1011,
+	MLX5_DPLL_SSM_CODE_EPRC = 0b0010,
+};
+
+enum {
+	MLX5_DPLL_ENHANCED_SSM_CODE_PRC = 0xff,
+	MLX5_DPLL_ENHANCED_SSM_CODE_SSU_A = 0xff,
+	MLX5_DPLL_ENHANCED_SSM_CODE_SSU_B = 0xff,
+	MLX5_DPLL_ENHANCED_SSM_CODE_EEC1 = 0xff,
+	MLX5_DPLL_ENHANCED_SSM_CODE_PRTC = 0x20,
+	MLX5_DPLL_ENHANCED_SSM_CODE_EPRTC = 0x21,
+	MLX5_DPLL_ENHANCED_SSM_CODE_EEEC = 0x22,
+	MLX5_DPLL_ENHANCED_SSM_CODE_EPRC = 0x23,
+};
+
+#define __MLX5_DPLL_SSM_COMBINED_CODE(ssm_code, enhanced_ssm_code)		\
+	((ssm_code) | ((enhanced_ssm_code) << 8))
+
+#define MLX5_DPLL_SSM_COMBINED_CODE(type)					\
+	__MLX5_DPLL_SSM_COMBINED_CODE(MLX5_DPLL_SSM_CODE_##type,		\
+				      MLX5_DPLL_ENHANCED_SSM_CODE_##type)
+
+static int mlx5_dpll_clock_quality_level_get(const struct dpll_device *dpll,
+					     void *priv, unsigned long *qls,
+					     struct netlink_ext_ack *extack)
+{
+	u8 network_option, ssm_code, enhanced_ssm_code;
+	u32 out[MLX5_ST_SZ_DW(msecq_reg)] = {};
+	u32 in[MLX5_ST_SZ_DW(msecq_reg)] = {};
+	struct mlx5_dpll *mdpll = priv;
+	int err;
+
+	err = mlx5_core_access_reg(mdpll->mdev, in, sizeof(in),
+				   out, sizeof(out), MLX5_REG_MSECQ, 0, 0);
+	if (err)
+		return err;
+	network_option = MLX5_GET(msecq_reg, out, network_option);
+	if (network_option != 1)
+		goto errout;
+	ssm_code = MLX5_GET(msecq_reg, out, local_ssm_code);
+	enhanced_ssm_code = MLX5_GET(msecq_reg, out, local_enhanced_ssm_code);
+
+	switch (__MLX5_DPLL_SSM_COMBINED_CODE(ssm_code, enhanced_ssm_code)) {
+	case MLX5_DPLL_SSM_COMBINED_CODE(PRC):
+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_PRC, qls);
+		return 0;
+	case MLX5_DPLL_SSM_COMBINED_CODE(SSU_A):
+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_SSU_A, qls);
+		return 0;
+	case MLX5_DPLL_SSM_COMBINED_CODE(SSU_B):
+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_SSU_B, qls);
+		return 0;
+	case MLX5_DPLL_SSM_COMBINED_CODE(EEC1):
+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EEC1, qls);
+		return 0;
+	case MLX5_DPLL_SSM_COMBINED_CODE(PRTC):
+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_PRTC, qls);
+		return 0;
+	case MLX5_DPLL_SSM_COMBINED_CODE(EPRTC):
+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EPRTC, qls);
+		return 0;
+	case MLX5_DPLL_SSM_COMBINED_CODE(EEEC):
+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EEEC, qls);
+		return 0;
+	case MLX5_DPLL_SSM_COMBINED_CODE(EPRC):
+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EPRC, qls);
+		return 0;
+	}
+errout:
+	NL_SET_ERR_MSG_MOD(extack, "Invalid clock quality level obtained from firmware\n");
+	return -EINVAL;
+}
+
 static const struct dpll_device_ops mlx5_dpll_device_ops = {
 	.lock_status_get = mlx5_dpll_device_lock_status_get,
 	.mode_get = mlx5_dpll_device_mode_get,
+	.clock_quality_level_get = mlx5_dpll_clock_quality_level_get,
 };
 
 static int mlx5_dpll_pin_direction_get(const struct dpll_pin *pin,
-- 
2.47.0

RE: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Monday, October 14, 2024 10:12 AM
>To: netdev@vger.kernel.org
>
>From: Jiri Pirko <jiri@nvidia.com>
>
>In order to allow driver expose quality level of the clock it is
>running, introduce a new netlink attr with enum to carry it to the
>userspace. Also, introduce an op the dpll netlink code calls into the
>driver to obtain the value.
>
>Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>---
>v2->v3:
>- changed "itu" prefix to "itu-opt1"
>- changed driver op to pass bitmap to allow to set multiple qualities
>  and pass it to user over multiple attrs
>- enhanced the documentation a bit
>v1->v2:
>- extended quality enum documentation
>- added "itu" prefix to the enum values
>---
> Documentation/netlink/specs/dpll.yaml | 35 +++++++++++++++++++++++++++
> drivers/dpll/dpll_netlink.c           | 24 ++++++++++++++++++
> include/linux/dpll.h                  |  4 +++
> include/uapi/linux/dpll.h             | 24 ++++++++++++++++++
> 4 files changed, 87 insertions(+)
>
>diff --git a/Documentation/netlink/specs/dpll.yaml
>b/Documentation/netlink/specs/dpll.yaml
>index f2894ca35de8..0bd708e136ff 100644
>--- a/Documentation/netlink/specs/dpll.yaml
>+++ b/Documentation/netlink/specs/dpll.yaml
>@@ -85,6 +85,36 @@ definitions:
>           This may happen for example if dpll device was previously
>           locked on an input pin of type PIN_TYPE_SYNCE_ETH_PORT.
>     render-max: true
>+  -
>+    type: enum
>+    name: clock-quality-level
>+    doc: |
>+      level of quality of a clock device. This mainly applies when
>+      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
>+      The current list is defined according to the table 11-7 contained
>+      in ITU-T G.8264/Y.1364 document. One may extend this list freely
>+      by other ITU-T defined clock qualities, or different ones defined
>+      by another standardization body (for those, please use
>+      different prefix).
>+    entries:
>+      -
>+        name: itu-opt1-prc
>+        value: 1
>+      -
>+        name: itu-opt1-ssu-a
>+      -
>+        name: itu-opt1-ssu-b
>+      -
>+        name: itu-opt1-eec1
>+      -
>+        name: itu-opt1-prtc
>+      -
>+        name: itu-opt1-eprtc
>+      -
>+        name: itu-opt1-eeec
>+      -
>+        name: itu-opt1-eprc
>+    render-max: true
>   -
>     type: const
>     name: temp-divider
>@@ -252,6 +282,11 @@ attribute-sets:
>         name: lock-status-error
>         type: u32
>         enum: lock-status-error
>+      -
>+        name: clock-quality-level
>+        type: u32
>+        enum: clock-quality-level
>+        multi-attr: true
>   -
>     name: pin
>     enum-name: dpll_a_pin
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index fc0280dcddd1..c130f87147fa 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -169,6 +169,27 @@ dpll_msg_add_temp(struct sk_buff *msg, struct dpll_device
>*dpll,
> 	return 0;
> }
>
>+static int
>+dpll_msg_add_clock_quality_level(struct sk_buff *msg, struct dpll_device
>*dpll,
>+				 struct netlink_ext_ack *extack)
>+{
>+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>+	DECLARE_BITMAP(qls, DPLL_CLOCK_QUALITY_LEVEL_MAX) = { 0 };
>+	enum dpll_clock_quality_level ql;
>+	int ret;
>+
>+	if (!ops->clock_quality_level_get)
>+		return 0;
>+	ret = ops->clock_quality_level_get(dpll, dpll_priv(dpll), qls, extack);
>+	if (ret)
>+		return ret;
>+	for_each_set_bit(ql, qls, DPLL_CLOCK_QUALITY_LEVEL_MAX)
>+		if (nla_put_u32(msg, DPLL_A_CLOCK_QUALITY_LEVEL, ql))
>+			return -EMSGSIZE;
>+
>+	return 0;
>+}
>+
> static int
> dpll_msg_add_pin_prio(struct sk_buff *msg, struct dpll_pin *pin,
> 		      struct dpll_pin_ref *ref,
>@@ -557,6 +578,9 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>sk_buff *msg,
> 	if (ret)
> 		return ret;
> 	ret = dpll_msg_add_lock_status(msg, dpll, extack);
>+	if (ret)
>+		return ret;
>+	ret = dpll_msg_add_clock_quality_level(msg, dpll, extack);
> 	if (ret)
> 		return ret;
> 	ret = dpll_msg_add_mode(msg, dpll, extack);
>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>index 81f7b623d0ba..5e4f9ab1cf75 100644
>--- a/include/linux/dpll.h
>+++ b/include/linux/dpll.h
>@@ -26,6 +26,10 @@ struct dpll_device_ops {
> 			       struct netlink_ext_ack *extack);
> 	int (*temp_get)(const struct dpll_device *dpll, void *dpll_priv,
> 			s32 *temp, struct netlink_ext_ack *extack);
>+	int (*clock_quality_level_get)(const struct dpll_device *dpll,
>+				       void *dpll_priv,
>+				       unsigned long *qls,
>+				       struct netlink_ext_ack *extack);
> };
>
> struct dpll_pin_ops {
>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>index b0654ade7b7e..4b37542eace3 100644
>--- a/include/uapi/linux/dpll.h
>+++ b/include/uapi/linux/dpll.h
>@@ -79,6 +79,29 @@ enum dpll_lock_status_error {
> 	DPLL_LOCK_STATUS_ERROR_MAX = (__DPLL_LOCK_STATUS_ERROR_MAX - 1)
> };
>
>+/**
>+ * enum dpll_clock_quality_level - level of quality of a clock device. This
>+ *   mainly applies when the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
>+ *   The current list is defined according to the table 11-7 contained in
>ITU-T
>+ *   G.8264/Y.1364 document. One may extend this list freely by other ITU-T
>+ *   defined clock qualities, or different ones defined by another
>+ *   standardization body (for those, please use different prefix).
>+ */
>+enum dpll_clock_quality_level {
>+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_PRC = 1,
>+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_SSU_A,
>+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_SSU_B,
>+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EEC1,
>+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_PRTC,
>+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EPRTC,
>+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EEEC,
>+	DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EPRC,
>+
>+	/* private: */
>+	__DPLL_CLOCK_QUALITY_LEVEL_MAX,
>+	DPLL_CLOCK_QUALITY_LEVEL_MAX = (__DPLL_CLOCK_QUALITY_LEVEL_MAX - 1)
>+};
>+
> #define DPLL_TEMP_DIVIDER	1000
>
> /**
>@@ -180,6 +203,7 @@ enum dpll_a {
> 	DPLL_A_TEMP,
> 	DPLL_A_TYPE,
> 	DPLL_A_LOCK_STATUS_ERROR,
>+	DPLL_A_CLOCK_QUALITY_LEVEL,
>
> 	__DPLL_A_MAX,
> 	DPLL_A_MAX = (__DPLL_A_MAX - 1)
>--
>2.47.0

LGTM, Thank you!
Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

RE: [PATCH net-next v3 2/2] net/mlx5: DPLL, Add clock quality level op implementation

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Monday, October 14, 2024 10:12 AM
>
>From: Jiri Pirko <jiri@nvidia.com>
>
>Use MSECQ register to query clock quality from firmware. Implement the
>dpll op and fill-up the quality level value properly.
>
>Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>---
>v2->v3:
>- changed to fill-up quality level to bitmap
>- changed "itu" prefix to "itu-opt1"
>v1->v2:
>- added "itu" prefix to the enum values
>---
> .../net/ethernet/mellanox/mlx5/core/dpll.c    | 81 +++++++++++++++++++
> 1 file changed, 81 insertions(+)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>index 904e08de852e..31142f6cc372 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>@@ -166,9 +166,90 @@ static int mlx5_dpll_device_mode_get(const struct
>dpll_device *dpll,
> 	return 0;
> }
>
>+enum {
>+	MLX5_DPLL_SSM_CODE_PRC = 0b0010,
>+	MLX5_DPLL_SSM_CODE_SSU_A = 0b0100,
>+	MLX5_DPLL_SSM_CODE_SSU_B = 0b1000,
>+	MLX5_DPLL_SSM_CODE_EEC1 = 0b1011,
>+	MLX5_DPLL_SSM_CODE_PRTC = 0b0010,
>+	MLX5_DPLL_SSM_CODE_EPRTC = 0b0010,
>+	MLX5_DPLL_SSM_CODE_EEEC = 0b1011,
>+	MLX5_DPLL_SSM_CODE_EPRC = 0b0010,
>+};
>+
>+enum {
>+	MLX5_DPLL_ENHANCED_SSM_CODE_PRC = 0xff,
>+	MLX5_DPLL_ENHANCED_SSM_CODE_SSU_A = 0xff,
>+	MLX5_DPLL_ENHANCED_SSM_CODE_SSU_B = 0xff,
>+	MLX5_DPLL_ENHANCED_SSM_CODE_EEC1 = 0xff,
>+	MLX5_DPLL_ENHANCED_SSM_CODE_PRTC = 0x20,
>+	MLX5_DPLL_ENHANCED_SSM_CODE_EPRTC = 0x21,
>+	MLX5_DPLL_ENHANCED_SSM_CODE_EEEC = 0x22,
>+	MLX5_DPLL_ENHANCED_SSM_CODE_EPRC = 0x23,
>+};
>+
>+#define __MLX5_DPLL_SSM_COMBINED_CODE(ssm_code, enhanced_ssm_code)		\
>+	((ssm_code) | ((enhanced_ssm_code) << 8))
>+
>+#define MLX5_DPLL_SSM_COMBINED_CODE(type)					\
>+	__MLX5_DPLL_SSM_COMBINED_CODE(MLX5_DPLL_SSM_CODE_##type,		\
>+				      MLX5_DPLL_ENHANCED_SSM_CODE_##type)
>+
>+static int mlx5_dpll_clock_quality_level_get(const struct dpll_device *dpll,
>+					     void *priv, unsigned long *qls,
>+					     struct netlink_ext_ack *extack)
>+{
>+	u8 network_option, ssm_code, enhanced_ssm_code;
>+	u32 out[MLX5_ST_SZ_DW(msecq_reg)] = {};
>+	u32 in[MLX5_ST_SZ_DW(msecq_reg)] = {};
>+	struct mlx5_dpll *mdpll = priv;
>+	int err;
>+
>+	err = mlx5_core_access_reg(mdpll->mdev, in, sizeof(in),
>+				   out, sizeof(out), MLX5_REG_MSECQ, 0, 0);
>+	if (err)
>+		return err;
>+	network_option = MLX5_GET(msecq_reg, out, network_option);
>+	if (network_option != 1)
>+		goto errout;
>+	ssm_code = MLX5_GET(msecq_reg, out, local_ssm_code);
>+	enhanced_ssm_code = MLX5_GET(msecq_reg, out, local_enhanced_ssm_code);
>+
>+	switch (__MLX5_DPLL_SSM_COMBINED_CODE(ssm_code, enhanced_ssm_code)) {
>+	case MLX5_DPLL_SSM_COMBINED_CODE(PRC):
>+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_PRC, qls);
>+		return 0;
>+	case MLX5_DPLL_SSM_COMBINED_CODE(SSU_A):
>+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_SSU_A, qls);
>+		return 0;
>+	case MLX5_DPLL_SSM_COMBINED_CODE(SSU_B):
>+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_SSU_B, qls);
>+		return 0;
>+	case MLX5_DPLL_SSM_COMBINED_CODE(EEC1):
>+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EEC1, qls);
>+		return 0;
>+	case MLX5_DPLL_SSM_COMBINED_CODE(PRTC):
>+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_PRTC, qls);
>+		return 0;
>+	case MLX5_DPLL_SSM_COMBINED_CODE(EPRTC):
>+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EPRTC, qls);
>+		return 0;
>+	case MLX5_DPLL_SSM_COMBINED_CODE(EEEC):
>+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EEEC, qls);
>+		return 0;
>+	case MLX5_DPLL_SSM_COMBINED_CODE(EPRC):
>+		__set_bit(DPLL_CLOCK_QUALITY_LEVEL_ITU_OPT1_EPRC, qls);
>+		return 0;
>+	}
>+errout:
>+	NL_SET_ERR_MSG_MOD(extack, "Invalid clock quality level obtained from
>firmware\n");
>+	return -EINVAL;
>+}
>+
> static const struct dpll_device_ops mlx5_dpll_device_ops = {
> 	.lock_status_get = mlx5_dpll_device_lock_status_get,
> 	.mode_get = mlx5_dpll_device_mode_get,
>+	.clock_quality_level_get = mlx5_dpll_clock_quality_level_get,
> };
>
> static int mlx5_dpll_pin_direction_get(const struct dpll_pin *pin,
>--
>2.47.0

LGTM,
Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jakub Kicinski]

On Mon, 14 Oct 2024 10:11:32 +0200 Jiri Pirko wrote:
> +    type: enum
> +    name: clock-quality-level
> +    doc: |
> +      level of quality of a clock device. This mainly applies when
> +      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
> +      The current list is defined according to the table 11-7 contained
> +      in ITU-T G.8264/Y.1364 document. One may extend this list freely
> +      by other ITU-T defined clock qualities, or different ones defined
> +      by another standardization body (for those, please use
> +      different prefix).

uAPI extensibility aside - doesn't this belong to clock info?
I'm slightly worried we're stuffing this attr into DPLL because
we have netlink for DPLL but no good way to extend clock info.

> +    entries:
> +      -
> +        name: itu-opt1-prc
> +        value: 1
> +      -
> +        name: itu-opt1-ssu-a
> +      -
> +        name: itu-opt1-ssu-b
> +      -
> +        name: itu-opt1-eec1
> +      -
> +        name: itu-opt1-prtc
> +      -
> +        name: itu-opt1-eprtc
> +      -
> +        name: itu-opt1-eeec
> +      -
> +        name: itu-opt1-eprc
> +    render-max: true

Why render max? Just to align with other unnecessary max defines in
the file?

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jiri Pirko]

Tue, Oct 15, 2024 at 04:26:38PM CEST, kuba@kernel.org wrote:
>On Mon, 14 Oct 2024 10:11:32 +0200 Jiri Pirko wrote:
>> +    type: enum
>> +    name: clock-quality-level
>> +    doc: |
>> +      level of quality of a clock device. This mainly applies when
>> +      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
>> +      The current list is defined according to the table 11-7 contained
>> +      in ITU-T G.8264/Y.1364 document. One may extend this list freely
>> +      by other ITU-T defined clock qualities, or different ones defined
>> +      by another standardization body (for those, please use
>> +      different prefix).
>
>uAPI extensibility aside - doesn't this belong to clock info?
>I'm slightly worried we're stuffing this attr into DPLL because
>we have netlink for DPLL but no good way to extend clock info.

Not sure what do you mean by "clock info". Dpll device and clock is kind
of the same thing. The dpll device is identified by clock-id. I see no
other attributes on the way this direction to more extend dpll attr
namespace.


>
>> +    entries:
>> +      -
>> +        name: itu-opt1-prc
>> +        value: 1
>> +      -
>> +        name: itu-opt1-ssu-a
>> +      -
>> +        name: itu-opt1-ssu-b
>> +      -
>> +        name: itu-opt1-eec1
>> +      -
>> +        name: itu-opt1-prtc
>> +      -
>> +        name: itu-opt1-eprtc
>> +      -
>> +        name: itu-opt1-eeec
>> +      -
>> +        name: itu-opt1-eprc
>> +    render-max: true
>
>Why render max? Just to align with other unnecessary max defines in
>the file?

Yeah, why not?

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Vadim Fedorenko]

On 15/10/2024 15:26, Jakub Kicinski wrote:
> On Mon, 14 Oct 2024 10:11:32 +0200 Jiri Pirko wrote:
>> +    type: enum
>> +    name: clock-quality-level
>> +    doc: |
>> +      level of quality of a clock device. This mainly applies when
>> +      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
>> +      The current list is defined according to the table 11-7 contained
>> +      in ITU-T G.8264/Y.1364 document. One may extend this list freely
>> +      by other ITU-T defined clock qualities, or different ones defined
>> +      by another standardization body (for those, please use
>> +      different prefix).
> 
> uAPI extensibility aside - doesn't this belong to clock info?
> I'm slightly worried we're stuffing this attr into DPLL because
> we have netlink for DPLL but no good way to extend clock info.

There is a work going on by Maciek Machnikowski about extending clock
info. But the progress is kinda slow..

>> +    entries:
>> +      -
>> +        name: itu-opt1-prc
>> +        value: 1
>> +      -
>> +        name: itu-opt1-ssu-a
>> +      -
>> +        name: itu-opt1-ssu-b
>> +      -
>> +        name: itu-opt1-eec1
>> +      -
>> +        name: itu-opt1-prtc
>> +      -
>> +        name: itu-opt1-eprtc
>> +      -
>> +        name: itu-opt1-eeec
>> +      -
>> +        name: itu-opt1-eprc
>> +    render-max: true
> 
> Why render max? Just to align with other unnecessary max defines in
> the file?

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jiri Pirko]

Tue, Oct 15, 2024 at 04:50:41PM CEST, vadim.fedorenko@linux.dev wrote:
>On 15/10/2024 15:26, Jakub Kicinski wrote:
>> On Mon, 14 Oct 2024 10:11:32 +0200 Jiri Pirko wrote:
>> > +    type: enum
>> > +    name: clock-quality-level
>> > +    doc: |
>> > +      level of quality of a clock device. This mainly applies when
>> > +      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
>> > +      The current list is defined according to the table 11-7 contained
>> > +      in ITU-T G.8264/Y.1364 document. One may extend this list freely
>> > +      by other ITU-T defined clock qualities, or different ones defined
>> > +      by another standardization body (for those, please use
>> > +      different prefix).
>> 
>> uAPI extensibility aside - doesn't this belong to clock info?
>> I'm slightly worried we're stuffing this attr into DPLL because
>> we have netlink for DPLL but no good way to extend clock info.
>
>There is a work going on by Maciek Machnikowski about extending clock
>info. But the progress is kinda slow..

Do you have some info about this? A list of attrs at least would help.

>
>> > +    entries:
>> > +      -
>> > +        name: itu-opt1-prc
>> > +        value: 1
>> > +      -
>> > +        name: itu-opt1-ssu-a
>> > +      -
>> > +        name: itu-opt1-ssu-b
>> > +      -
>> > +        name: itu-opt1-eec1
>> > +      -
>> > +        name: itu-opt1-prtc
>> > +      -
>> > +        name: itu-opt1-eprtc
>> > +      -
>> > +        name: itu-opt1-eeec
>> > +      -
>> > +        name: itu-opt1-eprc
>> > +    render-max: true
>> 
>> Why render max? Just to align with other unnecessary max defines in
>> the file?
>

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jakub Kicinski]

On Tue, 15 Oct 2024 16:38:52 +0200 Jiri Pirko wrote:
> Tue, Oct 15, 2024 at 04:26:38PM CEST, kuba@kernel.org wrote:
> >On Mon, 14 Oct 2024 10:11:32 +0200 Jiri Pirko wrote:  
> >> +    type: enum
> >> +    name: clock-quality-level
> >> +    doc: |
> >> +      level of quality of a clock device. This mainly applies when
> >> +      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
> >> +      The current list is defined according to the table 11-7 contained
> >> +      in ITU-T G.8264/Y.1364 document. One may extend this list freely
> >> +      by other ITU-T defined clock qualities, or different ones defined
> >> +      by another standardization body (for those, please use
> >> +      different prefix).  
> >
> >uAPI extensibility aside - doesn't this belong to clock info?
> >I'm slightly worried we're stuffing this attr into DPLL because
> >we have netlink for DPLL but no good way to extend clock info.  
> 
> Not sure what do you mean by "clock info". Dpll device and clock is kind
> of the same thing. The dpll device is identified by clock-id. I see no
> other attributes on the way this direction to more extend dpll attr
> namespace.

I'm not an expert but I think the standard definition of a DPLL
does not include a built-in oscillator, if that's what you mean.

> >> +    entries:
> >> +      -
> >> +        name: itu-opt1-prc
> >> +        value: 1
> >> +      -
> >> +        name: itu-opt1-ssu-a
> >> +      -
> >> +        name: itu-opt1-ssu-b
> >> +      -
> >> +        name: itu-opt1-eec1
> >> +      -
> >> +        name: itu-opt1-prtc
> >> +      -
> >> +        name: itu-opt1-eprtc
> >> +      -
> >> +        name: itu-opt1-eeec
> >> +      -
> >> +        name: itu-opt1-eprc
> >> +    render-max: true  
> >
> >Why render max? Just to align with other unnecessary max defines in
> >the file?  
> 
> Yeah, why not?

If it wasn't pointless it would be the default for our code gen.
Please remove it unless you can point at some code that will likely
need it. We can always add it later, we can't remove it.

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Vadim Fedorenko]

On 15/10/2024 15:56, Jiri Pirko wrote:
> Tue, Oct 15, 2024 at 04:50:41PM CEST, vadim.fedorenko@linux.dev wrote:
>> On 15/10/2024 15:26, Jakub Kicinski wrote:
>>> On Mon, 14 Oct 2024 10:11:32 +0200 Jiri Pirko wrote:
>>>> +    type: enum
>>>> +    name: clock-quality-level
>>>> +    doc: |
>>>> +      level of quality of a clock device. This mainly applies when
>>>> +      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
>>>> +      The current list is defined according to the table 11-7 contained
>>>> +      in ITU-T G.8264/Y.1364 document. One may extend this list freely
>>>> +      by other ITU-T defined clock qualities, or different ones defined
>>>> +      by another standardization body (for those, please use
>>>> +      different prefix).
>>>
>>> uAPI extensibility aside - doesn't this belong to clock info?
>>> I'm slightly worried we're stuffing this attr into DPLL because
>>> we have netlink for DPLL but no good way to extend clock info.
>>
>> There is a work going on by Maciek Machnikowski about extending clock
>> info. But the progress is kinda slow..
> 
> Do you have some info about this? A list of attrs at least would help.

The mailing list conversation started in this thread:
https://lore.kernel.org/netdev/20240813125602.155827-1-maciek@machnikowski.net/

But the idea was presented back at the latest Netdevconf:
https://netdevconf.org/0x18/sessions/tutorial/introduction-to-ptp-on-linux-apis.html

>>
>>>> +    entries:
>>>> +      -
>>>> +        name: itu-opt1-prc
>>>> +        value: 1
>>>> +      -
>>>> +        name: itu-opt1-ssu-a
>>>> +      -
>>>> +        name: itu-opt1-ssu-b
>>>> +      -
>>>> +        name: itu-opt1-eec1
>>>> +      -
>>>> +        name: itu-opt1-prtc
>>>> +      -
>>>> +        name: itu-opt1-eprtc
>>>> +      -
>>>> +        name: itu-opt1-eeec
>>>> +      -
>>>> +        name: itu-opt1-eprc
>>>> +    render-max: true
>>>
>>> Why render max? Just to align with other unnecessary max defines in
>>> the file?
>>

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jiri Pirko]

Tue, Oct 15, 2024 at 05:01:12PM CEST, vadim.fedorenko@linux.dev wrote:
>On 15/10/2024 15:56, Jiri Pirko wrote:
>> Tue, Oct 15, 2024 at 04:50:41PM CEST, vadim.fedorenko@linux.dev wrote:
>> > On 15/10/2024 15:26, Jakub Kicinski wrote:
>> > > On Mon, 14 Oct 2024 10:11:32 +0200 Jiri Pirko wrote:
>> > > > +    type: enum
>> > > > +    name: clock-quality-level
>> > > > +    doc: |
>> > > > +      level of quality of a clock device. This mainly applies when
>> > > > +      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
>> > > > +      The current list is defined according to the table 11-7 contained
>> > > > +      in ITU-T G.8264/Y.1364 document. One may extend this list freely
>> > > > +      by other ITU-T defined clock qualities, or different ones defined
>> > > > +      by another standardization body (for those, please use
>> > > > +      different prefix).
>> > > 
>> > > uAPI extensibility aside - doesn't this belong to clock info?
>> > > I'm slightly worried we're stuffing this attr into DPLL because
>> > > we have netlink for DPLL but no good way to extend clock info.
>> > 
>> > There is a work going on by Maciek Machnikowski about extending clock
>> > info. But the progress is kinda slow..
>> 
>> Do you have some info about this? A list of attrs at least would help.
>
>The mailing list conversation started in this thread:
>https://lore.kernel.org/netdev/20240813125602.155827-1-maciek@machnikowski.net/

What's the relation to ptp? I'm missing something here.

>
>But the idea was presented back at the latest Netdevconf:
>https://netdevconf.org/0x18/sessions/tutorial/introduction-to-ptp-on-linux-apis.html
>
>> > 
>> > > > +    entries:
>> > > > +      -
>> > > > +        name: itu-opt1-prc
>> > > > +        value: 1
>> > > > +      -
>> > > > +        name: itu-opt1-ssu-a
>> > > > +      -
>> > > > +        name: itu-opt1-ssu-b
>> > > > +      -
>> > > > +        name: itu-opt1-eec1
>> > > > +      -
>> > > > +        name: itu-opt1-prtc
>> > > > +      -
>> > > > +        name: itu-opt1-eprtc
>> > > > +      -
>> > > > +        name: itu-opt1-eeec
>> > > > +      -
>> > > > +        name: itu-opt1-eprc
>> > > > +    render-max: true
>> > > 
>> > > Why render max? Just to align with other unnecessary max defines in
>> > > the file?
>> > 
>

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jiri Pirko]

Tue, Oct 15, 2024 at 05:01:08PM CEST, kuba@kernel.org wrote:
>On Tue, 15 Oct 2024 16:38:52 +0200 Jiri Pirko wrote:
>> Tue, Oct 15, 2024 at 04:26:38PM CEST, kuba@kernel.org wrote:
>> >On Mon, 14 Oct 2024 10:11:32 +0200 Jiri Pirko wrote:  
>> >> +    type: enum
>> >> +    name: clock-quality-level
>> >> +    doc: |
>> >> +      level of quality of a clock device. This mainly applies when
>> >> +      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
>> >> +      The current list is defined according to the table 11-7 contained
>> >> +      in ITU-T G.8264/Y.1364 document. One may extend this list freely
>> >> +      by other ITU-T defined clock qualities, or different ones defined
>> >> +      by another standardization body (for those, please use
>> >> +      different prefix).  
>> >
>> >uAPI extensibility aside - doesn't this belong to clock info?
>> >I'm slightly worried we're stuffing this attr into DPLL because
>> >we have netlink for DPLL but no good way to extend clock info.  
>> 
>> Not sure what do you mean by "clock info". Dpll device and clock is kind
>> of the same thing. The dpll device is identified by clock-id. I see no
>> other attributes on the way this direction to more extend dpll attr
>> namespace.
>
>I'm not an expert but I think the standard definition of a DPLL
>does not include a built-in oscillator, if that's what you mean.

Okay. Then the clock-id we have also does not make much sense.
Anyway, what is your desire exactly? Do you want to have a nest attr
clock-info to contain this quality-level attr? Or something else?


>
>> >> +    entries:
>> >> +      -
>> >> +        name: itu-opt1-prc
>> >> +        value: 1
>> >> +      -
>> >> +        name: itu-opt1-ssu-a
>> >> +      -
>> >> +        name: itu-opt1-ssu-b
>> >> +      -
>> >> +        name: itu-opt1-eec1
>> >> +      -
>> >> +        name: itu-opt1-prtc
>> >> +      -
>> >> +        name: itu-opt1-eprtc
>> >> +      -
>> >> +        name: itu-opt1-eeec
>> >> +      -
>> >> +        name: itu-opt1-eprc
>> >> +    render-max: true  
>> >
>> >Why render max? Just to align with other unnecessary max defines in
>> >the file?  
>> 
>> Yeah, why not?
>
>If it wasn't pointless it would be the default for our code gen.
>Please remove it unless you can point at some code that will likely
>need it. We can always add it later, we can't remove it.

Ok

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jiri Pirko]

Tue, Oct 15, 2024 at 05:01:08PM CEST, kuba@kernel.org wrote:
>On Tue, 15 Oct 2024 16:38:52 +0200 Jiri Pirko wrote:
>> Tue, Oct 15, 2024 at 04:26:38PM CEST, kuba@kernel.org wrote:
>> >On Mon, 14 Oct 2024 10:11:32 +0200 Jiri Pirko wrote:  
>> >> +    type: enum
>> >> +    name: clock-quality-level
>> >> +    doc: |
>> >> +      level of quality of a clock device. This mainly applies when
>> >> +      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
>> >> +      The current list is defined according to the table 11-7 contained
>> >> +      in ITU-T G.8264/Y.1364 document. One may extend this list freely
>> >> +      by other ITU-T defined clock qualities, or different ones defined
>> >> +      by another standardization body (for those, please use
>> >> +      different prefix).  
>> >
>> >uAPI extensibility aside - doesn't this belong to clock info?
>> >I'm slightly worried we're stuffing this attr into DPLL because
>> >we have netlink for DPLL but no good way to extend clock info.  
>> 
>> Not sure what do you mean by "clock info". Dpll device and clock is kind
>> of the same thing. The dpll device is identified by clock-id. I see no
>> other attributes on the way this direction to more extend dpll attr
>> namespace.
>
>I'm not an expert but I think the standard definition of a DPLL
>does not include a built-in oscillator, if that's what you mean.
>
>> >> +    entries:
>> >> +      -
>> >> +        name: itu-opt1-prc
>> >> +        value: 1
>> >> +      -
>> >> +        name: itu-opt1-ssu-a
>> >> +      -
>> >> +        name: itu-opt1-ssu-b
>> >> +      -
>> >> +        name: itu-opt1-eec1
>> >> +      -
>> >> +        name: itu-opt1-prtc
>> >> +      -
>> >> +        name: itu-opt1-eprtc
>> >> +      -
>> >> +        name: itu-opt1-eeec
>> >> +      -
>> >> +        name: itu-opt1-eprc
>> >> +    render-max: true  
>> >
>> >Why render max? Just to align with other unnecessary max defines in
>> >the file?  
>> 
>> Yeah, why not?
>
>If it wasn't pointless it would be the default for our code gen.
>Please remove it unless you can point at some code that will likely
>need it. We can always add it later, we can't remove it.

Well, I use it internally to define the length of bitmap. Does that
justify? I mean, it would be very odd to define the bitmap length
differently.

Thanks!

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jiri Pirko]

Wed, Oct 16, 2024 at 10:19:57AM CEST, jiri@resnulli.us wrote:
>Tue, Oct 15, 2024 at 05:01:08PM CEST, kuba@kernel.org wrote:
>>On Tue, 15 Oct 2024 16:38:52 +0200 Jiri Pirko wrote:
>>> Tue, Oct 15, 2024 at 04:26:38PM CEST, kuba@kernel.org wrote:
>>> >On Mon, 14 Oct 2024 10:11:32 +0200 Jiri Pirko wrote:  
>>> >> +    type: enum
>>> >> +    name: clock-quality-level
>>> >> +    doc: |
>>> >> +      level of quality of a clock device. This mainly applies when
>>> >> +      the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED.
>>> >> +      The current list is defined according to the table 11-7 contained
>>> >> +      in ITU-T G.8264/Y.1364 document. One may extend this list freely
>>> >> +      by other ITU-T defined clock qualities, or different ones defined
>>> >> +      by another standardization body (for those, please use
>>> >> +      different prefix).  
>>> >
>>> >uAPI extensibility aside - doesn't this belong to clock info?
>>> >I'm slightly worried we're stuffing this attr into DPLL because
>>> >we have netlink for DPLL but no good way to extend clock info.  
>>> 
>>> Not sure what do you mean by "clock info". Dpll device and clock is kind
>>> of the same thing. The dpll device is identified by clock-id. I see no
>>> other attributes on the way this direction to more extend dpll attr
>>> namespace.
>>
>>I'm not an expert but I think the standard definition of a DPLL
>>does not include a built-in oscillator, if that's what you mean.
>
>Okay. Then the clock-id we have also does not make much sense.
>Anyway, what is your desire exactly? Do you want to have a nest attr
>clock-info to contain this quality-level attr? Or something else?

Jakub? What's your wish?

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jakub Kicinski]

On Wed, 16 Oct 2024 10:19:57 +0200 Jiri Pirko wrote:
> >> Not sure what do you mean by "clock info". Dpll device and clock is kind
> >> of the same thing. The dpll device is identified by clock-id. I see no
> >> other attributes on the way this direction to more extend dpll attr
> >> namespace.  
> >
> >I'm not an expert but I think the standard definition of a DPLL
> >does not include a built-in oscillator, if that's what you mean.  
> 
> Okay. Then the clock-id we have also does not make much sense.
> Anyway, what is your desire exactly? Do you want to have a nest attr
> clock-info to contain this quality-level attr? Or something else?

I thought clock-id is basically clockid_t, IOW a reference.
I wish that the information about timekeepers was exposed 
by the time subsystem rather than DPLL. Something like clock_getres().

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jiri Pirko]

Mon, Oct 28, 2024 at 06:14:03PM CET, kuba@kernel.org wrote:
>On Wed, 16 Oct 2024 10:19:57 +0200 Jiri Pirko wrote:
>> >> Not sure what do you mean by "clock info". Dpll device and clock is kind
>> >> of the same thing. The dpll device is identified by clock-id. I see no
>> >> other attributes on the way this direction to more extend dpll attr
>> >> namespace.  
>> >
>> >I'm not an expert but I think the standard definition of a DPLL
>> >does not include a built-in oscillator, if that's what you mean.  
>> 
>> Okay. Then the clock-id we have also does not make much sense.
>> Anyway, what is your desire exactly? Do you want to have a nest attr
>> clock-info to contain this quality-level attr? Or something else?
>
>I thought clock-id is basically clockid_t, IOW a reference.
>I wish that the information about timekeepers was exposed 
>by the time subsystem rather than DPLL. Something like clock_getres().

Hmm. From what I understand, the quality of the clock as it is defined
by the ITU standard is an attribute of the DPLL device. DPLL device
in our model is basically a board, which might combine oscillator,
synchronizer and possibly other devices. The clock quality is determined
by this combination and I understand that the ITU certification is also
applied to this device.

That's why it makes sense to have the clock quality as the DPLL
attribute. Makes sense?

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jakub Kicinski]

On Tue, 29 Oct 2024 13:52:12 +0100 Jiri Pirko wrote:
> >I thought clock-id is basically clockid_t, IOW a reference.
> >I wish that the information about timekeepers was exposed 
> >by the time subsystem rather than DPLL. Something like clock_getres().  
> 
> Hmm. From what I understand, the quality of the clock as it is defined
> by the ITU standard is an attribute of the DPLL device. DPLL device
> in our model is basically a board, which might combine oscillator,
> synchronizer and possibly other devices. The clock quality is determined
> by this combination and I understand that the ITU certification is also
> applied to this device.
> 
> That's why it makes sense to have the clock quality as the DPLL
> attribute. Makes sense?

Hm, reading more carefully sounds like it's the quality of the holdover
clock. Can we say that in the documentation? "This mainly applies when
the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED" is a bit of a mouthful.
I think I missed the "not" first time reading it.

Is it marked as multi-attr in case non-ITU clock quality is defined
later? Or is it legit to set to ITU bits at once?

Re: [PATCH net-next v3 1/2] dpll: add clock quality level attribute and op

[Jiri Pirko]

Tue, Oct 29, 2024 at 02:51:29PM CET, kuba@kernel.org wrote:
>On Tue, 29 Oct 2024 13:52:12 +0100 Jiri Pirko wrote:
>> >I thought clock-id is basically clockid_t, IOW a reference.
>> >I wish that the information about timekeepers was exposed 
>> >by the time subsystem rather than DPLL. Something like clock_getres().  
>> 
>> Hmm. From what I understand, the quality of the clock as it is defined
>> by the ITU standard is an attribute of the DPLL device. DPLL device
>> in our model is basically a board, which might combine oscillator,
>> synchronizer and possibly other devices. The clock quality is determined
>> by this combination and I understand that the ITU certification is also
>> applied to this device.
>> 
>> That's why it makes sense to have the clock quality as the DPLL
>> attribute. Makes sense?
>
>Hm, reading more carefully sounds like it's the quality of the holdover
>clock. Can we say that in the documentation? "This mainly applies when

Yes.


>the dpll lock-status is not DPLL_LOCK_STATUS_LOCKED" is a bit of a mouthful.
>I think I missed the "not" first time reading it.

Okay, will try to re-phrase to avoid confusion.


>
>Is it marked as multi-attr in case non-ITU clock quality is defined
>later? Or is it legit to set to ITU bits at once?

For non-ITU as well as for possibly advertizing quality of different ITU
options (option 1 and option 2).