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

Commit Message

Jiri Pirko Oct. 9, 2024, 12:25 p.m. UTC

From: Jiri Pirko <[e-mail]>

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 <[e-mail]>
---
 Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
 drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
 include/linux/dpll.h                  |  4 ++++
 include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
 4 files changed, 75 insertions(+)

Comments

Vadim Fedorenko Oct. 9, 2024, 1:33 p.m. UTC | #1

On 09/10/2024 13:25, Jiri Pirko wrote:
> From: Jiri Pirko <[e-mail]>
> 
> 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.

The idea is good, it matches with the work Maciek is doing now in terms
of improving POSIX clock interface. See a comment below.

> Signed-off-by: Jiri Pirko <[e-mail]>
> ---
>   Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
>   drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>   include/linux/dpll.h                  |  4 ++++
>   include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>   4 files changed, 75 insertions(+)
> 
> diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
> index f2894ca35de8..77a8e9ddb254 100644
> --- a/Documentation/netlink/specs/dpll.yaml
> +++ b/Documentation/netlink/specs/dpll.yaml
> @@ -85,6 +85,30 @@ 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.
> +    entries:
> +      -
> +        name: prc
> +        value: 1
> +      -
> +        name: ssu-a
> +      -
> +        name: ssu-b
> +      -
> +        name: eec1
> +      -
> +        name: prtc
> +      -
> +        name: eprtc
> +      -
> +        name: eeec
> +      -
> +        name: eprc
> +    render-max: true
>     -
>       type: const
>       name: temp-divider
> @@ -252,6 +276,10 @@ attribute-sets:
>           name: lock-status-error
>           type: u32
>           enum: lock-status-error
> +      -
> +        name: clock-quality-level
> +        type: u32
> +        enum: clock-quality-level
>     -
>       name: pin
>       enum-name: dpll_a_pin
> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
> index fc0280dcddd1..689a6d0ff049 100644
> --- a/drivers/dpll/dpll_netlink.c
> +++ b/drivers/dpll/dpll_netlink.c
> @@ -169,6 +169,25 @@ 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);
> +	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), &ql, extack);
> +	if (ret)
> +		return ret;
> +	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 +576,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..e99cdb8ab02c 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,
> +				       enum dpll_clock_quality_level *ql,
> +				       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..0572f9376da4 100644
> --- a/include/uapi/linux/dpll.h
> +++ b/include/uapi/linux/dpll.h
> @@ -79,6 +79,26 @@ enum dpll_lock_status_error {
>   	DPLL_LOCK_STATUS_ERROR_MAX = (__DPLL_LOCK_STATUS_ERROR_MAX - 1)
>   };
>   
> +/**
> + * enum dpll_clock_quality_level - if previous status change was done due to a
> + *   failure, this provides information of dpll device lock status error. Valid
> + *   values for DPLL_A_LOCK_STATUS_ERROR attribute
> + */
> +enum dpll_clock_quality_level {
> +	DPLL_CLOCK_QUALITY_LEVEL_PRC = 1,
> +	DPLL_CLOCK_QUALITY_LEVEL_SSU_A,
> +	DPLL_CLOCK_QUALITY_LEVEL_SSU_B,
> +	DPLL_CLOCK_QUALITY_LEVEL_EEC1,
> +	DPLL_CLOCK_QUALITY_LEVEL_PRTC,
> +	DPLL_CLOCK_QUALITY_LEVEL_EPRTC,
> +	DPLL_CLOCK_QUALITY_LEVEL_EEEC,
> +	DPLL_CLOCK_QUALITY_LEVEL_EPRC,

I think it would be great to provide some explanation of levels here.
People coming from SDH area may not be familiar with some of them. Or at
least mention ITU-T/IEEE recommendations documents to get the meanings
of these levels.

> +
> +	/* private: */
> +	__DPLL_CLOCK_QUALITY_LEVEL_MAX,
> +	DPLL_CLOCK_QUALITY_LEVEL_MAX = (__DPLL_CLOCK_QUALITY_LEVEL_MAX - 1)
> +};
> +
>   #define DPLL_TEMP_DIVIDER	1000
>   
>   /**
> @@ -180,6 +200,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)

Arkadiusz Kubalewski Oct. 9, 2024, 1:38 p.m. UTC | #2

>From: Jiri Pirko <[e-mail]>
>Sent: Wednesday, October 9, 2024 2:26 PM
>
>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 <[e-mail]>
>---
> Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
> include/linux/dpll.h                  |  4 ++++
> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
> 4 files changed, 75 insertions(+)
>
>diff --git a/Documentation/netlink/specs/dpll.yaml
>b/Documentation/netlink/specs/dpll.yaml
>index f2894ca35de8..77a8e9ddb254 100644
>--- a/Documentation/netlink/specs/dpll.yaml
>+++ b/Documentation/netlink/specs/dpll.yaml
>@@ -85,6 +85,30 @@ 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.

Hi Jiri,

Thanks for your work on this!

I do like the idea, but this part is a bit tricky.

I assume it is all about clock/quality levels as mentioned in
ITU-T spec "Table 11-7" of REC-G.8264?

Then what about table 11-8?

And in general about option 2(3?) networks?

AFAIR there are 3 (I don't think 3rd is relevant? But still defined
In REC-G.781, also REC-G.781 doesn't provide clock types at all,
just Quality Levels).

Assuming 2(3?) network options shall be available, either user can
select the one which is shown, or driver just provides all (if can,
one/none otherwise)?

If we don't want to give the user control and just let the
driver to either provide this or not, my suggestion would be to name
the attribute appropriately: "clock-quality-level-o1" to make clear
provided attribute belongs to option 1 network.

Then, if there would be need for different network options, just new
attribute and defines could be introduced without hassle for backward
compatibility.

Does it make sense?

Thank you!
Arkadiusz

>+    entries:
>+      -
>+        name: prc
>+        value: 1
>+      -
>+        name: ssu-a
>+      -
>+        name: ssu-b
>+      -
>+        name: eec1
>+      -
>+        name: prtc
>+      -
>+        name: eprtc
>+      -
>+        name: eeec
>+      -
>+        name: eprc
>+    render-max: true
>   -
>     type: const
>     name: temp-divider
>@@ -252,6 +276,10 @@ attribute-sets:
>         name: lock-status-error
>         type: u32
>         enum: lock-status-error
>+      -
>+        name: clock-quality-level
>+        type: u32
>+        enum: clock-quality-level
>   -
>     name: pin
>     enum-name: dpll_a_pin
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index fc0280dcddd1..689a6d0ff049 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -169,6 +169,25 @@ 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);
>+	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), &ql, extack);
>+	if (ret)
>+		return ret;
>+	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 +576,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..e99cdb8ab02c 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,
>+				       enum dpll_clock_quality_level *ql,
>+				       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..0572f9376da4 100644
>--- a/include/uapi/linux/dpll.h
>+++ b/include/uapi/linux/dpll.h
>@@ -79,6 +79,26 @@ enum dpll_lock_status_error {
> 	DPLL_LOCK_STATUS_ERROR_MAX = (__DPLL_LOCK_STATUS_ERROR_MAX - 1)
> };
>
>+/**
>+ * enum dpll_clock_quality_level - if previous status change was done due to
>a
>+ *   failure, this provides information of dpll device lock status error.
>Valid
>+ *   values for DPLL_A_LOCK_STATUS_ERROR attribute
>+ */
>+enum dpll_clock_quality_level {
>+	DPLL_CLOCK_QUALITY_LEVEL_PRC = 1,
>+	DPLL_CLOCK_QUALITY_LEVEL_SSU_A,
>+	DPLL_CLOCK_QUALITY_LEVEL_SSU_B,
>+	DPLL_CLOCK_QUALITY_LEVEL_EEC1,
>+	DPLL_CLOCK_QUALITY_LEVEL_PRTC,
>+	DPLL_CLOCK_QUALITY_LEVEL_EPRTC,
>+	DPLL_CLOCK_QUALITY_LEVEL_EEEC,
>+	DPLL_CLOCK_QUALITY_LEVEL_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 +200,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.46.1

Jiri Pirko Oct. 9, 2024, 1:39 p.m. UTC | #3

Wed, Oct 09, 2024 at 03:33:13PM CEST, [e-mail] wrote:
>On 09/10/2024 13:25, Jiri Pirko wrote:
>> From: Jiri Pirko <[e-mail]>
>> 
>> 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.
>
>The idea is good, it matches with the work Maciek is doing now in terms
>of improving POSIX clock interface. See a comment below.
>
>> Signed-off-by: Jiri Pirko <[e-mail]>
>> ---
>>   Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
>>   drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>>   include/linux/dpll.h                  |  4 ++++
>>   include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>>   4 files changed, 75 insertions(+)
>> 
>> diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
>> index f2894ca35de8..77a8e9ddb254 100644
>> --- a/Documentation/netlink/specs/dpll.yaml
>> +++ b/Documentation/netlink/specs/dpll.yaml
>> @@ -85,6 +85,30 @@ 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.
>> +    entries:
>> +      -
>> +        name: prc
>> +        value: 1
>> +      -
>> +        name: ssu-a
>> +      -
>> +        name: ssu-b
>> +      -
>> +        name: eec1
>> +      -
>> +        name: prtc
>> +      -
>> +        name: eprtc
>> +      -
>> +        name: eeec
>> +      -
>> +        name: eprc
>> +    render-max: true
>>     -
>>       type: const
>>       name: temp-divider
>> @@ -252,6 +276,10 @@ attribute-sets:
>>           name: lock-status-error
>>           type: u32
>>           enum: lock-status-error
>> +      -
>> +        name: clock-quality-level
>> +        type: u32
>> +        enum: clock-quality-level
>>     -
>>       name: pin
>>       enum-name: dpll_a_pin
>> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>> index fc0280dcddd1..689a6d0ff049 100644
>> --- a/drivers/dpll/dpll_netlink.c
>> +++ b/drivers/dpll/dpll_netlink.c
>> @@ -169,6 +169,25 @@ 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);
>> +	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), &ql, extack);
>> +	if (ret)
>> +		return ret;
>> +	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 +576,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..e99cdb8ab02c 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,
>> +				       enum dpll_clock_quality_level *ql,
>> +				       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..0572f9376da4 100644
>> --- a/include/uapi/linux/dpll.h
>> +++ b/include/uapi/linux/dpll.h
>> @@ -79,6 +79,26 @@ enum dpll_lock_status_error {
>>   	DPLL_LOCK_STATUS_ERROR_MAX = (__DPLL_LOCK_STATUS_ERROR_MAX - 1)
>>   };
>> +/**
>> + * enum dpll_clock_quality_level - if previous status change was done due to a
>> + *   failure, this provides information of dpll device lock status error. Valid
>> + *   values for DPLL_A_LOCK_STATUS_ERROR attribute
>> + */
>> +enum dpll_clock_quality_level {
>> +	DPLL_CLOCK_QUALITY_LEVEL_PRC = 1,
>> +	DPLL_CLOCK_QUALITY_LEVEL_SSU_A,
>> +	DPLL_CLOCK_QUALITY_LEVEL_SSU_B,
>> +	DPLL_CLOCK_QUALITY_LEVEL_EEC1,
>> +	DPLL_CLOCK_QUALITY_LEVEL_PRTC,
>> +	DPLL_CLOCK_QUALITY_LEVEL_EPRTC,
>> +	DPLL_CLOCK_QUALITY_LEVEL_EEEC,
>> +	DPLL_CLOCK_QUALITY_LEVEL_EPRC,
>
>I think it would be great to provide some explanation of levels here.
>People coming from SDH area may not be familiar with some of them. Or at
>least mention ITU-T/IEEE recommendations documents to get the meanings
>of these levels.

Okay, I can put a reference to ITU document.


>
>> +
>> +	/* private: */
>> +	__DPLL_CLOCK_QUALITY_LEVEL_MAX,
>> +	DPLL_CLOCK_QUALITY_LEVEL_MAX = (__DPLL_CLOCK_QUALITY_LEVEL_MAX - 1)
>> +};
>> +
>>   #define DPLL_TEMP_DIVIDER	1000
>>   /**
>> @@ -180,6 +200,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)
>

Jiri Pirko Oct. 9, 2024, 2:06 p.m. UTC | #4

Wed, Oct 09, 2024 at 03:38:38PM CEST, [e-mail] wrote:
>>From: Jiri Pirko <[e-mail]>
>>Sent: Wednesday, October 9, 2024 2:26 PM
>>
>>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 <[e-mail]>
>>---
>> Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
>> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>> include/linux/dpll.h                  |  4 ++++
>> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>> 4 files changed, 75 insertions(+)
>>
>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>b/Documentation/netlink/specs/dpll.yaml
>>index f2894ca35de8..77a8e9ddb254 100644
>>--- a/Documentation/netlink/specs/dpll.yaml
>>+++ b/Documentation/netlink/specs/dpll.yaml
>>@@ -85,6 +85,30 @@ 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.
>
>Hi Jiri,
>
>Thanks for your work on this!
>
>I do like the idea, but this part is a bit tricky.
>
>I assume it is all about clock/quality levels as mentioned in
>ITU-T spec "Table 11-7" of REC-G.8264?

For now, yes. That is the usecase I have currently. But, if anyone
will have a need to introduce any sort of different quality, I don't
see why not.

>
>Then what about table 11-8?

The names do not overlap. So if anyone need to add those, he is free to
do it.


>
>And in general about option 2(3?) networks?
>
>AFAIR there are 3 (I don't think 3rd is relevant? But still defined
>In REC-G.781, also REC-G.781 doesn't provide clock types at all,
>just Quality Levels).
>
>Assuming 2(3?) network options shall be available, either user can
>select the one which is shown, or driver just provides all (if can,
>one/none otherwise)?
>
>If we don't want to give the user control and just let the
>driver to either provide this or not, my suggestion would be to name
>the attribute appropriately: "clock-quality-level-o1" to make clear
>provided attribute belongs to option 1 network.

I was thinking about that but there are 2 groups of names in both
tables:
1) different quality levels and names. Then "o1/2" in the name is not
   really needed, as the name itself is the differentiator.
2) same quality leves in both options. Those are:
   PRTC
   ePRTC
   eEEC
   ePRC
   And for thesee, using "o1/2" prefix would lead to have 2 enum values
   for exactly the same quality level.

But, talking about prefixes, perhaps I can put "ITU" as a prefix
to indicate this is ITU standartized clock quality leaving option
for some other clock quality namespace to appear?

[..]

Arkadiusz Kubalewski Oct. 10, 2024, 9:53 a.m. UTC | #5

>From: Jiri Pirko <[e-mail]>
>Sent: Wednesday, October 9, 2024 4:07 PM
>
>Wed, Oct 09, 2024 at 03:38:38PM CEST, [e-mail] wrote:
>>>From: Jiri Pirko <[e-mail]>
>>>Sent: Wednesday, October 9, 2024 2:26 PM
>>>
>>>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 <[e-mail]>
>>>---
>>> Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
>>> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>>> include/linux/dpll.h                  |  4 ++++
>>> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>>> 4 files changed, 75 insertions(+)
>>>
>>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>>b/Documentation/netlink/specs/dpll.yaml
>>>index f2894ca35de8..77a8e9ddb254 100644
>>>--- a/Documentation/netlink/specs/dpll.yaml
>>>+++ b/Documentation/netlink/specs/dpll.yaml
>>>@@ -85,6 +85,30 @@ 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.
>>
>>Hi Jiri,
>>
>>Thanks for your work on this!
>>
>>I do like the idea, but this part is a bit tricky.
>>
>>I assume it is all about clock/quality levels as mentioned in ITU-T
>>spec "Table 11-7" of REC-G.8264?
>
>For now, yes. That is the usecase I have currently. But, if anyone will have a
>need to introduce any sort of different quality, I don't see why not.
>
>>
>>Then what about table 11-8?
>
>The names do not overlap. So if anyone need to add those, he is free to do it.
>

Not true, some names do overlap: ePRC/eEEC/ePRTC/PRTC.
As you already pointed below :)

>
>>
>>And in general about option 2(3?) networks?
>>
>>AFAIR there are 3 (I don't think 3rd is relevant? But still defined In
>>REC-G.781, also REC-G.781 doesn't provide clock types at all, just
>>Quality Levels).
>>
>>Assuming 2(3?) network options shall be available, either user can
>>select the one which is shown, or driver just provides all (if can,
>>one/none otherwise)?
>>
>>If we don't want to give the user control and just let the driver to
>>either provide this or not, my suggestion would be to name the
>>attribute appropriately: "clock-quality-level-o1" to make clear
>>provided attribute belongs to option 1 network.
>
>I was thinking about that but there are 2 groups of names in both
>tables:
>1) different quality levels and names. Then "o1/2" in the name is not
>   really needed, as the name itself is the differentiator.
>2) same quality leves in both options. Those are:
>   PRTC
>   ePRTC
>   eEEC
>   ePRC
>   And for thesee, using "o1/2" prefix would lead to have 2 enum values
>   for exactly the same quality level.
>

Those names overlap but corresponding SSM is different depending on
the network option, providing one of those without network option will
confuse users.

For me one enum list for clock types/quality sounds good.

>But, talking about prefixes, perhaps I can put "ITU" as a prefix to indicate
>this is ITU standartized clock quality leaving option for some other clock
>quality namespace to appear?
>
>[..]

Sure, also makes sense.

But I still believe the attribute name shall also contain the info that
it conveys an option1 clock type. As the device can meet both specifications
at once, we need to make sure user knows that.

Thank you!
Arkadiusz

Jiri Pirko Oct. 10, 2024, 11:36 a.m. UTC | #6

Thu, Oct 10, 2024 at 11:53:30AM CEST, [e-mail] wrote:
>>From: Jiri Pirko <[e-mail]>
>>Sent: Wednesday, October 9, 2024 4:07 PM
>>
>>Wed, Oct 09, 2024 at 03:38:38PM CEST, [e-mail] wrote:
>>>>From: Jiri Pirko <[e-mail]>
>>>>Sent: Wednesday, October 9, 2024 2:26 PM
>>>>
>>>>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 <[e-mail]>
>>>>---
>>>> Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
>>>> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>>>> include/linux/dpll.h                  |  4 ++++
>>>> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>>>> 4 files changed, 75 insertions(+)
>>>>
>>>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>>>b/Documentation/netlink/specs/dpll.yaml
>>>>index f2894ca35de8..77a8e9ddb254 100644
>>>>--- a/Documentation/netlink/specs/dpll.yaml
>>>>+++ b/Documentation/netlink/specs/dpll.yaml
>>>>@@ -85,6 +85,30 @@ 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.
>>>
>>>Hi Jiri,
>>>
>>>Thanks for your work on this!
>>>
>>>I do like the idea, but this part is a bit tricky.
>>>
>>>I assume it is all about clock/quality levels as mentioned in ITU-T
>>>spec "Table 11-7" of REC-G.8264?
>>
>>For now, yes. That is the usecase I have currently. But, if anyone will have a
>>need to introduce any sort of different quality, I don't see why not.
>>
>>>
>>>Then what about table 11-8?
>>
>>The names do not overlap. So if anyone need to add those, he is free to do it.
>>
>
>Not true, some names do overlap: ePRC/eEEC/ePRTC/PRTC.
>As you already pointed below :)

Yep, sure.

>
>>
>>>
>>>And in general about option 2(3?) networks?
>>>
>>>AFAIR there are 3 (I don't think 3rd is relevant? But still defined In
>>>REC-G.781, also REC-G.781 doesn't provide clock types at all, just
>>>Quality Levels).
>>>
>>>Assuming 2(3?) network options shall be available, either user can
>>>select the one which is shown, or driver just provides all (if can,
>>>one/none otherwise)?
>>>
>>>If we don't want to give the user control and just let the driver to
>>>either provide this or not, my suggestion would be to name the
>>>attribute appropriately: "clock-quality-level-o1" to make clear
>>>provided attribute belongs to option 1 network.
>>
>>I was thinking about that but there are 2 groups of names in both
>>tables:
>>1) different quality levels and names. Then "o1/2" in the name is not
>>   really needed, as the name itself is the differentiator.
>>2) same quality leves in both options. Those are:
>>   PRTC
>>   ePRTC
>>   eEEC
>>   ePRC
>>   And for thesee, using "o1/2" prefix would lead to have 2 enum values
>>   for exactly the same quality level.
>>
>
>Those names overlap but corresponding SSM is different depending on
>the network option, providing one of those without network option will
>confuse users.

The ssm code is different, but that is irrelevant in context of this
UAPI. Clock quality levels are the same, that's what matters, isn't it?


>
>For me one enum list for clock types/quality sounds good.
>
>>But, talking about prefixes, perhaps I can put "ITU" as a prefix to indicate
>>this is ITU standartized clock quality leaving option for some other clock
>>quality namespace to appear?
>>
>>[..]
>
>Sure, also makes sense.
>
>But I still believe the attribute name shall also contain the info that
>it conveys an option1 clock type. As the device can meet both specifications
>at once, we need to make sure user knows that.

As I described, I don't see any reason why. Just adds unnecessary
redundancy to uapi.


>
>Thank you!
>Arkadiusz

Arkadiusz Kubalewski Oct. 10, 2024, 1:48 p.m. UTC | #7

>From: Jiri Pirko <[e-mail]>
>Sent: Thursday, October 10, 2024 1:36 PM
>
>Thu, Oct 10, 2024 at 11:53:30AM CEST, [e-mail] wrote:
>>>From: Jiri Pirko <[e-mail]>
>>>Sent: Wednesday, October 9, 2024 4:07 PM
>>>
>>>Wed, Oct 09, 2024 at 03:38:38PM CEST, [e-mail] wrote:
>>>>>From: Jiri Pirko <[e-mail]>
>>>>>Sent: Wednesday, October 9, 2024 2:26 PM
>>>>>
>>>>>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 <[e-mail]>
>>>>>---
>>>>> Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
>>>>> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>>>>> include/linux/dpll.h                  |  4 ++++
>>>>> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>>>>> 4 files changed, 75 insertions(+)
>>>>>
>>>>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>>>>b/Documentation/netlink/specs/dpll.yaml
>>>>>index f2894ca35de8..77a8e9ddb254 100644
>>>>>--- a/Documentation/netlink/specs/dpll.yaml
>>>>>+++ b/Documentation/netlink/specs/dpll.yaml
>>>>>@@ -85,6 +85,30 @@ 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.
>>>>
>>>>Hi Jiri,
>>>>
>>>>Thanks for your work on this!
>>>>
>>>>I do like the idea, but this part is a bit tricky.
>>>>
>>>>I assume it is all about clock/quality levels as mentioned in ITU-T
>>>>spec "Table 11-7" of REC-G.8264?
>>>
>>>For now, yes. That is the usecase I have currently. But, if anyone will have
>>>a
>>>need to introduce any sort of different quality, I don't see why not.
>>>
>>>>
>>>>Then what about table 11-8?
>>>
>>>The names do not overlap. So if anyone need to add those, he is free to do
>>>it.
>>>
>>
>>Not true, some names do overlap: ePRC/eEEC/ePRTC/PRTC.
>>As you already pointed below :)
>
>Yep, sure.
>
>>
>>>
>>>>
>>>>And in general about option 2(3?) networks?
>>>>
>>>>AFAIR there are 3 (I don't think 3rd is relevant? But still defined In
>>>>REC-G.781, also REC-G.781 doesn't provide clock types at all, just
>>>>Quality Levels).
>>>>
>>>>Assuming 2(3?) network options shall be available, either user can
>>>>select the one which is shown, or driver just provides all (if can,
>>>>one/none otherwise)?
>>>>
>>>>If we don't want to give the user control and just let the driver to
>>>>either provide this or not, my suggestion would be to name the
>>>>attribute appropriately: "clock-quality-level-o1" to make clear
>>>>provided attribute belongs to option 1 network.
>>>
>>>I was thinking about that but there are 2 groups of names in both
>>>tables:
>>>1) different quality levels and names. Then "o1/2" in the name is not
>>>   really needed, as the name itself is the differentiator.
>>>2) same quality leves in both options. Those are:
>>>   PRTC
>>>   ePRTC
>>>   eEEC
>>>   ePRC
>>>   And for thesee, using "o1/2" prefix would lead to have 2 enum values
>>>   for exactly the same quality level.
>>>
>>
>>Those names overlap but corresponding SSM is different depending on
>>the network option, providing one of those without network option will
>>confuse users.
>
>The ssm code is different, but that is irrelevant in context of this
>UAPI. Clock quality levels are the same, that's what matters, isn't it?
>

This is relevant to user if the clock provides both.
I.e., given clock meets requirements for both Option1:PRC and
Option2:PRS.
How would you provide both of those to the user?

The patch implements only option1 but the attribute shall
be named adequately. So the user doesn't have to look for it
or guessing around.
After all it is not just DPLL_A_CLOCK_QUALITY_LEVEL.
It is either DPLL_A_CLOCK_QUALITY_LEVEL_OPTION1=X or a tuple:
DPLL_A_CLOCK_QUALITY_LEVEL=X + DPLL_A_CLOCK_QUALITY_OPTION=1.
mlx code in 2/2 indicates this is option 1.
Why uapi shall be silent about it?

Thank you!
Arkadiusz

>
>>
>>For me one enum list for clock types/quality sounds good.
>>
>>>But, talking about prefixes, perhaps I can put "ITU" as a prefix to indicate
>>>this is ITU standartized clock quality leaving option for some other clock
>>>quality namespace to appear?
>>>
>>>[..]
>>
>>Sure, also makes sense.
>>
>>But I still believe the attribute name shall also contain the info that
>>it conveys an option1 clock type. As the device can meet both specifications
>>at once, we need to make sure user knows that.
>
>As I described, I don't see any reason why. Just adds unnecessary
>redundancy to uapi.
>
>
>>
>>Thank you!
>>Arkadiusz

Jiri Pirko Oct. 10, 2024, 2:36 p.m. UTC | #8

Thu, Oct 10, 2024 at 03:48:02PM CEST, [e-mail] wrote:
>>From: Jiri Pirko <[e-mail]>
>>Sent: Thursday, October 10, 2024 1:36 PM
>>
>>Thu, Oct 10, 2024 at 11:53:30AM CEST, [e-mail] wrote:
>>>>From: Jiri Pirko <[e-mail]>
>>>>Sent: Wednesday, October 9, 2024 4:07 PM
>>>>
>>>>Wed, Oct 09, 2024 at 03:38:38PM CEST, [e-mail] wrote:
>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>Sent: Wednesday, October 9, 2024 2:26 PM
>>>>>>
>>>>>>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 <[e-mail]>
>>>>>>---
>>>>>> Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
>>>>>> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>>>>>> include/linux/dpll.h                  |  4 ++++
>>>>>> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>>>>>> 4 files changed, 75 insertions(+)
>>>>>>
>>>>>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>>>>>b/Documentation/netlink/specs/dpll.yaml
>>>>>>index f2894ca35de8..77a8e9ddb254 100644
>>>>>>--- a/Documentation/netlink/specs/dpll.yaml
>>>>>>+++ b/Documentation/netlink/specs/dpll.yaml
>>>>>>@@ -85,6 +85,30 @@ 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.
>>>>>
>>>>>Hi Jiri,
>>>>>
>>>>>Thanks for your work on this!
>>>>>
>>>>>I do like the idea, but this part is a bit tricky.
>>>>>
>>>>>I assume it is all about clock/quality levels as mentioned in ITU-T
>>>>>spec "Table 11-7" of REC-G.8264?
>>>>
>>>>For now, yes. That is the usecase I have currently. But, if anyone will have
>>>>a
>>>>need to introduce any sort of different quality, I don't see why not.
>>>>
>>>>>
>>>>>Then what about table 11-8?
>>>>
>>>>The names do not overlap. So if anyone need to add those, he is free to do
>>>>it.
>>>>
>>>
>>>Not true, some names do overlap: ePRC/eEEC/ePRTC/PRTC.
>>>As you already pointed below :)
>>
>>Yep, sure.
>>
>>>
>>>>
>>>>>
>>>>>And in general about option 2(3?) networks?
>>>>>
>>>>>AFAIR there are 3 (I don't think 3rd is relevant? But still defined In
>>>>>REC-G.781, also REC-G.781 doesn't provide clock types at all, just
>>>>>Quality Levels).
>>>>>
>>>>>Assuming 2(3?) network options shall be available, either user can
>>>>>select the one which is shown, or driver just provides all (if can,
>>>>>one/none otherwise)?
>>>>>
>>>>>If we don't want to give the user control and just let the driver to
>>>>>either provide this or not, my suggestion would be to name the
>>>>>attribute appropriately: "clock-quality-level-o1" to make clear
>>>>>provided attribute belongs to option 1 network.
>>>>
>>>>I was thinking about that but there are 2 groups of names in both
>>>>tables:
>>>>1) different quality levels and names. Then "o1/2" in the name is not
>>>>   really needed, as the name itself is the differentiator.
>>>>2) same quality leves in both options. Those are:
>>>>   PRTC
>>>>   ePRTC
>>>>   eEEC
>>>>   ePRC
>>>>   And for thesee, using "o1/2" prefix would lead to have 2 enum values
>>>>   for exactly the same quality level.
>>>>
>>>
>>>Those names overlap but corresponding SSM is different depending on
>>>the network option, providing one of those without network option will
>>>confuse users.
>>
>>The ssm code is different, but that is irrelevant in context of this
>>UAPI. Clock quality levels are the same, that's what matters, isn't it?
>>
>
>This is relevant to user if the clock provides both.
>I.e., given clock meets requirements for both Option1:PRC and
>Option2:PRS.
>How would you provide both of those to the user?

Currently, the attr is single value. So you imply that there is usecase
to report multiple clock quality at a single time?

Even with that. "PRC" and "PRS" names are enough to differenciate.
option prefix is redundant.


>
>The patch implements only option1 but the attribute shall
>be named adequately. So the user doesn't have to look for it
>or guessing around.
>After all it is not just DPLL_A_CLOCK_QUALITY_LEVEL.
>It is either DPLL_A_CLOCK_QUALITY_LEVEL_OPTION1=X or a tuple:
>DPLL_A_CLOCK_QUALITY_LEVEL=X + DPLL_A_CLOCK_QUALITY_OPTION=1.

Why exactly do you need to expose "option"? What's the usecase?


>mlx code in 2/2 indicates this is option 1.
>Why uapi shall be silent about it?

Why is that needed? Also, uapi should provide some sort of abstraction.
"option1/2" is very ITU/SyncE specific. The idea is to be able to reuse
"quality-level" attr for non-synce usecases.


>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>For me one enum list for clock types/quality sounds good.
>>>
>>>>But, talking about prefixes, perhaps I can put "ITU" as a prefix to indicate
>>>>this is ITU standartized clock quality leaving option for some other clock
>>>>quality namespace to appear?
>>>>
>>>>[..]
>>>
>>>Sure, also makes sense.
>>>
>>>But I still believe the attribute name shall also contain the info that
>>>it conveys an option1 clock type. As the device can meet both specifications
>>>at once, we need to make sure user knows that.
>>
>>As I described, I don't see any reason why. Just adds unnecessary
>>redundancy to uapi.
>>
>>
>>>
>>>Thank you!
>>>Arkadiusz

Arkadiusz Kubalewski Oct. 10, 2024, 4:02 p.m. UTC | #9

>From: Jiri Pirko <[e-mail]>
>Sent: Thursday, October 10, 2024 4:37 PM
>
>Thu, Oct 10, 2024 at 03:48:02PM CEST, [e-mail] wrote:
>>>From: Jiri Pirko <[e-mail]>
>>>Sent: Thursday, October 10, 2024 1:36 PM
>>>
>>>Thu, Oct 10, 2024 at 11:53:30AM CEST, [e-mail] wrote:
>>>>>From: Jiri Pirko <[e-mail]>
>>>>>Sent: Wednesday, October 9, 2024 4:07 PM
>>>>>
>>>>>Wed, Oct 09, 2024 at 03:38:38PM CEST, [e-mail]
>>>>>wrote:
>>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>>Sent: Wednesday, October 9, 2024 2:26 PM
>>>>>>>
>>>>>>>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 <[e-mail]>
>>>>>>>---
>>>>>>> Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
>>>>>>> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>>>>>>> include/linux/dpll.h                  |  4 ++++
>>>>>>> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>>>>>>> 4 files changed, 75 insertions(+)
>>>>>>>
>>>>>>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>>>>>>b/Documentation/netlink/specs/dpll.yaml
>>>>>>>index f2894ca35de8..77a8e9ddb254 100644
>>>>>>>--- a/Documentation/netlink/specs/dpll.yaml
>>>>>>>+++ b/Documentation/netlink/specs/dpll.yaml
>>>>>>>@@ -85,6 +85,30 @@ 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.
>>>>>>
>>>>>>Hi Jiri,
>>>>>>
>>>>>>Thanks for your work on this!
>>>>>>
>>>>>>I do like the idea, but this part is a bit tricky.
>>>>>>
>>>>>>I assume it is all about clock/quality levels as mentioned in ITU-T
>>>>>>spec "Table 11-7" of REC-G.8264?
>>>>>
>>>>>For now, yes. That is the usecase I have currently. But, if anyone will
>>>>>have
>>>>>a
>>>>>need to introduce any sort of different quality, I don't see why not.
>>>>>
>>>>>>
>>>>>>Then what about table 11-8?
>>>>>
>>>>>The names do not overlap. So if anyone need to add those, he is free to do
>>>>>it.
>>>>>
>>>>
>>>>Not true, some names do overlap: ePRC/eEEC/ePRTC/PRTC.
>>>>As you already pointed below :)
>>>
>>>Yep, sure.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>And in general about option 2(3?) networks?
>>>>>>
>>>>>>AFAIR there are 3 (I don't think 3rd is relevant? But still defined In
>>>>>>REC-G.781, also REC-G.781 doesn't provide clock types at all, just
>>>>>>Quality Levels).
>>>>>>
>>>>>>Assuming 2(3?) network options shall be available, either user can
>>>>>>select the one which is shown, or driver just provides all (if can,
>>>>>>one/none otherwise)?
>>>>>>
>>>>>>If we don't want to give the user control and just let the driver to
>>>>>>either provide this or not, my suggestion would be to name the
>>>>>>attribute appropriately: "clock-quality-level-o1" to make clear
>>>>>>provided attribute belongs to option 1 network.
>>>>>
>>>>>I was thinking about that but there are 2 groups of names in both
>>>>>tables:
>>>>>1) different quality levels and names. Then "o1/2" in the name is not
>>>>>   really needed, as the name itself is the differentiator.
>>>>>2) same quality leves in both options. Those are:
>>>>>   PRTC
>>>>>   ePRTC
>>>>>   eEEC
>>>>>   ePRC
>>>>>   And for thesee, using "o1/2" prefix would lead to have 2 enum values
>>>>>   for exactly the same quality level.
>>>>>
>>>>
>>>>Those names overlap but corresponding SSM is different depending on
>>>>the network option, providing one of those without network option will
>>>>confuse users.
>>>
>>>The ssm code is different, but that is irrelevant in context of this
>>>UAPI. Clock quality levels are the same, that's what matters, isn't it?
>>>
>>
>>This is relevant to user if the clock provides both.
>>I.e., given clock meets requirements for both Option1:PRC and
>>Option2:PRS.
>>How would you provide both of those to the user?
>
>Currently, the attr is single value. So you imply that there is usecase
>to report multiple clock quality at a single time?
>

Yes, correct. The userspace would decide which one to use.

>Even with that. "PRC" and "PRS" names are enough to differenciate.
>option prefix is redundant.
>

I do not ask for option prefix in the enum names, but specify somehow
the option you do provide, and ability easily expand the uapi to provide
both at the same time.. Backend can wait for someone to actually
implement the option2, but we don't want to change uapi later, right?

>
>>
>>The patch implements only option1 but the attribute shall
>>be named adequately. So the user doesn't have to look for it
>>or guessing around.
>>After all it is not just DPLL_A_CLOCK_QUALITY_LEVEL.
>>It is either DPLL_A_CLOCK_QUALITY_LEVEL_OPTION1=X or a tuple:
>>DPLL_A_CLOCK_QUALITY_LEVEL=X + DPLL_A_CLOCK_QUALITY_OPTION=1.
>
>Why exactly do you need to expose "option"? What's the usecase?
>

The use case is to simply provide accurate information.
With proposed changes the user will not know if provided class of
ePRC is option 1 or 2.

>
>>mlx code in 2/2 indicates this is option 1.
>>Why uapi shall be silent about it?
>
>Why is that needed? Also, uapi should provide some sort of abstraction.
>"option1/2" is very ITU/SyncE specific. The idea is to be able to reuse
>"quality-level" attr for non-synce usecases.
>

Well, actually great point, makes most sense to me.
Then the design shall be some kind of list of tuples?

Like:
--dump get-device
{
  'clock-id': 4658613174691233804,
  'id':1,
  'type':eec,
  ...

  'clock_spec':
  [
    {
      "type": itu-option1,
      "quality-level": eprc
    },
    {
      "type": itu-option2,
      "quality-level": eprc
    },
    ...
  ]
  ...
}

With assumption that for now only one "type" of itu-option1, but with
ability to easily expand the uapi.

The "quality-level" is already defined, and seems fine to me.

Does it make sense?

Thank you!
Arkadiusz

>
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>>
>>>>For me one enum list for clock types/quality sounds good.
>>>>
>>>>>But, talking about prefixes, perhaps I can put "ITU" as a prefix to
>>>>>indicate
>>>>>this is ITU standartized clock quality leaving option for some other clock
>>>>>quality namespace to appear?
>>>>>
>>>>>[..]
>>>>
>>>>Sure, also makes sense.
>>>>
>>>>But I still believe the attribute name shall also contain the info that
>>>>it conveys an option1 clock type. As the device can meet both
>>>>specifications
>>>>at once, we need to make sure user knows that.
>>>
>>>As I described, I don't see any reason why. Just adds unnecessary
>>>redundancy to uapi.
>>>
>>>
>>>>
>>>>Thank you!
>>>>Arkadiusz

Jiri Pirko Oct. 11, 2024, 6:45 a.m. UTC | #10

Thu, Oct 10, 2024 at 06:02:56PM CEST, [e-mail] wrote:
>>From: Jiri Pirko <[e-mail]>
>>Sent: Thursday, October 10, 2024 4:37 PM
>>
>>Thu, Oct 10, 2024 at 03:48:02PM CEST, [e-mail] wrote:
>>>>From: Jiri Pirko <[e-mail]>
>>>>Sent: Thursday, October 10, 2024 1:36 PM
>>>>
>>>>Thu, Oct 10, 2024 at 11:53:30AM CEST, [e-mail] wrote:
>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>Sent: Wednesday, October 9, 2024 4:07 PM
>>>>>>
>>>>>>Wed, Oct 09, 2024 at 03:38:38PM CEST, [e-mail]
>>>>>>wrote:
>>>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>>>Sent: Wednesday, October 9, 2024 2:26 PM
>>>>>>>>
>>>>>>>>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 <[e-mail]>
>>>>>>>>---
>>>>>>>> Documentation/netlink/specs/dpll.yaml | 28 +++++++++++++++++++++++++++
>>>>>>>> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>>>>>>>> include/linux/dpll.h                  |  4 ++++
>>>>>>>> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>>>>>>>> 4 files changed, 75 insertions(+)
>>>>>>>>
>>>>>>>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>>>>>>>b/Documentation/netlink/specs/dpll.yaml
>>>>>>>>index f2894ca35de8..77a8e9ddb254 100644
>>>>>>>>--- a/Documentation/netlink/specs/dpll.yaml
>>>>>>>>+++ b/Documentation/netlink/specs/dpll.yaml
>>>>>>>>@@ -85,6 +85,30 @@ 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.
>>>>>>>
>>>>>>>Hi Jiri,
>>>>>>>
>>>>>>>Thanks for your work on this!
>>>>>>>
>>>>>>>I do like the idea, but this part is a bit tricky.
>>>>>>>
>>>>>>>I assume it is all about clock/quality levels as mentioned in ITU-T
>>>>>>>spec "Table 11-7" of REC-G.8264?
>>>>>>
>>>>>>For now, yes. That is the usecase I have currently. But, if anyone will
>>>>>>have
>>>>>>a
>>>>>>need to introduce any sort of different quality, I don't see why not.
>>>>>>
>>>>>>>
>>>>>>>Then what about table 11-8?
>>>>>>
>>>>>>The names do not overlap. So if anyone need to add those, he is free to do
>>>>>>it.
>>>>>>
>>>>>
>>>>>Not true, some names do overlap: ePRC/eEEC/ePRTC/PRTC.
>>>>>As you already pointed below :)
>>>>
>>>>Yep, sure.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>And in general about option 2(3?) networks?
>>>>>>>
>>>>>>>AFAIR there are 3 (I don't think 3rd is relevant? But still defined In
>>>>>>>REC-G.781, also REC-G.781 doesn't provide clock types at all, just
>>>>>>>Quality Levels).
>>>>>>>
>>>>>>>Assuming 2(3?) network options shall be available, either user can
>>>>>>>select the one which is shown, or driver just provides all (if can,
>>>>>>>one/none otherwise)?
>>>>>>>
>>>>>>>If we don't want to give the user control and just let the driver to
>>>>>>>either provide this or not, my suggestion would be to name the
>>>>>>>attribute appropriately: "clock-quality-level-o1" to make clear
>>>>>>>provided attribute belongs to option 1 network.
>>>>>>
>>>>>>I was thinking about that but there are 2 groups of names in both
>>>>>>tables:
>>>>>>1) different quality levels and names. Then "o1/2" in the name is not
>>>>>>   really needed, as the name itself is the differentiator.
>>>>>>2) same quality leves in both options. Those are:
>>>>>>   PRTC
>>>>>>   ePRTC
>>>>>>   eEEC
>>>>>>   ePRC
>>>>>>   And for thesee, using "o1/2" prefix would lead to have 2 enum values
>>>>>>   for exactly the same quality level.
>>>>>>
>>>>>
>>>>>Those names overlap but corresponding SSM is different depending on
>>>>>the network option, providing one of those without network option will
>>>>>confuse users.
>>>>
>>>>The ssm code is different, but that is irrelevant in context of this
>>>>UAPI. Clock quality levels are the same, that's what matters, isn't it?
>>>>
>>>
>>>This is relevant to user if the clock provides both.
>>>I.e., given clock meets requirements for both Option1:PRC and
>>>Option2:PRS.
>>>How would you provide both of those to the user?
>>
>>Currently, the attr is single value. So you imply that there is usecase
>>to report multiple clock quality at a single time?
>>
>
>Yes, correct. The userspace would decide which one to use.

Wait what? What do you mean by "which one to "use""?


>
>>Even with that. "PRC" and "PRS" names are enough to differenciate.
>>option prefix is redundant.
>>
>
>I do not ask for option prefix in the enum names, but specify somehow
>the option you do provide, and ability easily expand the uapi to provide
>both at the same time.. Backend can wait for someone to actually
>implement the option2, but we don't want to change uapi later, right?

So far, I fail to see what is the need for exposing the "option" info. I
may be missing something.


>
>>
>>>
>>>The patch implements only option1 but the attribute shall
>>>be named adequately. So the user doesn't have to look for it
>>>or guessing around.
>>>After all it is not just DPLL_A_CLOCK_QUALITY_LEVEL.
>>>It is either DPLL_A_CLOCK_QUALITY_LEVEL_OPTION1=X or a tuple:
>>>DPLL_A_CLOCK_QUALITY_LEVEL=X + DPLL_A_CLOCK_QUALITY_OPTION=1.
>>
>>Why exactly do you need to expose "option"? What's the usecase?
>>
>
>The use case is to simply provide accurate information.
>With proposed changes the user will not know if provided class of
>ePRC is option 1 or 2.

How exactly does those 2 differ in terms of clock quality? If they
don't, why to differenciate them?


>
>>
>>>mlx code in 2/2 indicates this is option 1.
>>>Why uapi shall be silent about it?
>>
>>Why is that needed? Also, uapi should provide some sort of abstraction.
>>"option1/2" is very ITU/SyncE specific. The idea is to be able to reuse
>>"quality-level" attr for non-synce usecases.
>>
>
>Well, actually great point, makes most sense to me.
>Then the design shall be some kind of list of tuples?
>
>Like:
>--dump get-device
>{
>  'clock-id': 4658613174691233804,
>  'id':1,
>  'type':eec,
>  ...
>
>  'clock_spec':
>  [
>    {
>      "type": itu-option1,
>      "quality-level": eprc
>    },
>    {
>      "type": itu-option2,
>      "quality-level": eprc
>    },
>    ...
>  ]
>  ...
>}
>
>With assumption that for now only one "type" of itu-option1, but with
>ability to easily expand the uapi.
>
>The "quality-level" is already defined, and seems fine to me.
>
>Does it make sense?

Sort of. I would still very much like to avoid exposing multiple values
at a time as it complicates the implementation, namely driver op.




>
>Thank you!
>Arkadiusz
>
>>
>>>
>>>Thank you!
>>>Arkadiusz
>>>
>>>>
>>>>>
>>>>>For me one enum list for clock types/quality sounds good.
>>>>>
>>>>>>But, talking about prefixes, perhaps I can put "ITU" as a prefix to
>>>>>>indicate
>>>>>>this is ITU standartized clock quality leaving option for some other clock
>>>>>>quality namespace to appear?
>>>>>>
>>>>>>[..]
>>>>>
>>>>>Sure, also makes sense.
>>>>>
>>>>>But I still believe the attribute name shall also contain the info that
>>>>>it conveys an option1 clock type. As the device can meet both
>>>>>specifications
>>>>>at once, we need to make sure user knows that.
>>>>
>>>>As I described, I don't see any reason why. Just adds unnecessary
>>>>redundancy to uapi.
>>>>
>>>>
>>>>>
>>>>>Thank you!
>>>>>Arkadiusz

Arkadiusz Kubalewski Oct. 11, 2024, 2:25 p.m. UTC | #11

>From: Jiri Pirko <[e-mail]>
>Sent: Friday, October 11, 2024 8:46 AM
>
>Thu, Oct 10, 2024 at 06:02:56PM CEST, [e-mail] wrote:
>>>From: Jiri Pirko <[e-mail]>
>>>Sent: Thursday, October 10, 2024 4:37 PM
>>>
>>>Thu, Oct 10, 2024 at 03:48:02PM CEST, [e-mail] wrote:
>>>>>From: Jiri Pirko <[e-mail]>
>>>>>Sent: Thursday, October 10, 2024 1:36 PM
>>>>>
>>>>>Thu, Oct 10, 2024 at 11:53:30AM CEST, [e-mail]
>>>>>wrote:
>>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>>Sent: Wednesday, October 9, 2024 4:07 PM
>>>>>>>
>>>>>>>Wed, Oct 09, 2024 at 03:38:38PM CEST, [e-mail]
>>>>>>>wrote:
>>>>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>>>>Sent: Wednesday, October 9, 2024 2:26 PM
>>>>>>>>>
>>>>>>>>>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 <[e-mail]>
>>>>>>>>>---
>>>>>>>>> Documentation/netlink/specs/dpll.yaml | 28
>>>>>>>>>+++++++++++++++++++++++++++
>>>>>>>>> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>>>>>>>>> include/linux/dpll.h                  |  4 ++++
>>>>>>>>> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>>>>>>>>> 4 files changed, 75 insertions(+)
>>>>>>>>>
>>>>>>>>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>b/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>index f2894ca35de8..77a8e9ddb254 100644
>>>>>>>>>--- a/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>+++ b/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>@@ -85,6 +85,30 @@ 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.
>>>>>>>>
>>>>>>>>Hi Jiri,
>>>>>>>>
>>>>>>>>Thanks for your work on this!
>>>>>>>>
>>>>>>>>I do like the idea, but this part is a bit tricky.
>>>>>>>>
>>>>>>>>I assume it is all about clock/quality levels as mentioned in ITU-T
>>>>>>>>spec "Table 11-7" of REC-G.8264?
>>>>>>>
>>>>>>>For now, yes. That is the usecase I have currently. But, if anyone will
>>>>>>>have
>>>>>>>a
>>>>>>>need to introduce any sort of different quality, I don't see why not.
>>>>>>>
>>>>>>>>
>>>>>>>>Then what about table 11-8?
>>>>>>>
>>>>>>>The names do not overlap. So if anyone need to add those, he is free to
>>>>>>>do
>>>>>>>it.
>>>>>>>
>>>>>>
>>>>>>Not true, some names do overlap: ePRC/eEEC/ePRTC/PRTC.
>>>>>>As you already pointed below :)
>>>>>
>>>>>Yep, sure.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>And in general about option 2(3?) networks?
>>>>>>>>
>>>>>>>>AFAIR there are 3 (I don't think 3rd is relevant? But still defined In
>>>>>>>>REC-G.781, also REC-G.781 doesn't provide clock types at all, just
>>>>>>>>Quality Levels).
>>>>>>>>
>>>>>>>>Assuming 2(3?) network options shall be available, either user can
>>>>>>>>select the one which is shown, or driver just provides all (if can,
>>>>>>>>one/none otherwise)?
>>>>>>>>
>>>>>>>>If we don't want to give the user control and just let the driver to
>>>>>>>>either provide this or not, my suggestion would be to name the
>>>>>>>>attribute appropriately: "clock-quality-level-o1" to make clear
>>>>>>>>provided attribute belongs to option 1 network.
>>>>>>>
>>>>>>>I was thinking about that but there are 2 groups of names in both
>>>>>>>tables:
>>>>>>>1) different quality levels and names. Then "o1/2" in the name is not
>>>>>>>   really needed, as the name itself is the differentiator.
>>>>>>>2) same quality leves in both options. Those are:
>>>>>>>   PRTC
>>>>>>>   ePRTC
>>>>>>>   eEEC
>>>>>>>   ePRC
>>>>>>>   And for thesee, using "o1/2" prefix would lead to have 2 enum values
>>>>>>>   for exactly the same quality level.
>>>>>>>
>>>>>>
>>>>>>Those names overlap but corresponding SSM is different depending on
>>>>>>the network option, providing one of those without network option will
>>>>>>confuse users.
>>>>>
>>>>>The ssm code is different, but that is irrelevant in context of this
>>>>>UAPI. Clock quality levels are the same, that's what matters, isn't it?
>>>>>
>>>>
>>>>This is relevant to user if the clock provides both.
>>>>I.e., given clock meets requirements for both Option1:PRC and
>>>>Option2:PRS.
>>>>How would you provide both of those to the user?
>>>
>>>Currently, the attr is single value. So you imply that there is usecase
>>>to report multiple clock quality at a single time?
>>>
>>
>>Yes, correct. The userspace would decide which one to use.
>
>Wait what? What do you mean by "which one to "use""?
>

Generally if you provide some information to user, he shall use it
somehow, right?

Maybe you could explain the use case as you are the one developing it?
Is this to tell user a quality level of your device itself - without
anything connected?
Does it change during runtime? (i.e. locking to a neighbor?)

I already explained my concerns: if device would be providing multiple
clock qualities for different specification options (i.e. ITU-T Option
1/2(/3)) and they would differ for single device with regards to the
meeting certain class spec requirements, the list of those would be
required. Since the names for both do overlap it would be not possible
to let the user know accurately without changes to the uapi.

>
>>
>>>Even with that. "PRC" and "PRS" names are enough to differenciate.
>>>option prefix is redundant.
>>>
>>
>>I do not ask for option prefix in the enum names, but specify somehow
>>the option you do provide, and ability easily expand the uapi to provide
>>both at the same time.. Backend can wait for someone to actually
>>implement the option2, but we don't want to change uapi later, right?
>
>So far, I fail to see what is the need for exposing the "option" info. I
>may be missing something.
>

See above and below :)

>
>>
>>>
>>>>
>>>>The patch implements only option1 but the attribute shall
>>>>be named adequately. So the user doesn't have to look for it
>>>>or guessing around.
>>>>After all it is not just DPLL_A_CLOCK_QUALITY_LEVEL.
>>>>It is either DPLL_A_CLOCK_QUALITY_LEVEL_OPTION1=X or a tuple:
>>>>DPLL_A_CLOCK_QUALITY_LEVEL=X + DPLL_A_CLOCK_QUALITY_OPTION=1.
>>>
>>>Why exactly do you need to expose "option"? What's the usecase?
>>>
>>
>>The use case is to simply provide accurate information.
>>With proposed changes the user will not know if provided class of
>>ePRC is option 1 or 2.
>
>How exactly does those 2 differ in terms of clock quality? If they
>don't, why to differenciate them?

All I could find is that the same name of clock class the quality
shall not differ. But again I am not saying we shall differentiate
the clock class.. Rather an ability to provide each one, keeping in
mind that device can meet different O1 vs O2 (vs 03.

How would we extend the interface to provide Option 2 clock class?
I still see some entropy, I mean higher then expected..
Let's say device meets PRC for Option1 and PRS for Option2, how would
we provide this?
The same attribute twice? This seems also possibly confusing for
certain cases.

>
>
>>
>>>
>>>>mlx code in 2/2 indicates this is option 1.
>>>>Why uapi shall be silent about it?
>>>
>>>Why is that needed? Also, uapi should provide some sort of abstraction.
>>>"option1/2" is very ITU/SyncE specific. The idea is to be able to reuse
>>>"quality-level" attr for non-synce usecases.
>>>
>>
>>Well, actually great point, makes most sense to me.
>>Then the design shall be some kind of list of tuples?
>>
>>Like:
>>--dump get-device
>>{
>>  'clock-id': 4658613174691233804,
>>  'id':1,
>>  'type':eec,
>>  ...
>>
>>  'clock_spec':
>>  [
>>    {
>>      "type": itu-option1,
>>      "quality-level": eprc
>>    },
>>    {
>>      "type": itu-option2,
>>      "quality-level": eprc
>>    },
>>    ...
>>  ]
>>  ...
>>}
>>
>>With assumption that for now only one "type" of itu-option1, but with
>>ability to easily expand the uapi.
>>
>>The "quality-level" is already defined, and seems fine to me.
>>
>>Does it make sense?
>
>Sort of. I would still very much like to avoid exposing multiple values
>at a time as it complicates the implementation, namely driver op.
>
>

Well, true.

Thank you!
Arkadiusz

>
>
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>>
>>>>Thank you!
>>>>Arkadiusz
>>>>
>>>>>
>>>>>>
>>>>>>For me one enum list for clock types/quality sounds good.
>>>>>>
>>>>>>>But, talking about prefixes, perhaps I can put "ITU" as a prefix to
>>>>>>>indicate
>>>>>>>this is ITU standartized clock quality leaving option for some other
>>>>>>>clock
>>>>>>>quality namespace to appear?
>>>>>>>
>>>>>>>[..]
>>>>>>
>>>>>>Sure, also makes sense.
>>>>>>
>>>>>>But I still believe the attribute name shall also contain the info that
>>>>>>it conveys an option1 clock type. As the device can meet both
>>>>>>specifications
>>>>>>at once, we need to make sure user knows that.
>>>>>
>>>>>As I described, I don't see any reason why. Just adds unnecessary
>>>>>redundancy to uapi.
>>>>>
>>>>>
>>>>>>
>>>>>>Thank you!
>>>>>>Arkadiusz

Jiri Pirko Oct. 11, 2024, 3:57 p.m. UTC | #12

Fri, Oct 11, 2024 at 04:25:09PM CEST, [e-mail] wrote:
>>From: Jiri Pirko <[e-mail]>
>>Sent: Friday, October 11, 2024 8:46 AM
>>
>>Thu, Oct 10, 2024 at 06:02:56PM CEST, [e-mail] wrote:
>>>>From: Jiri Pirko <[e-mail]>
>>>>Sent: Thursday, October 10, 2024 4:37 PM
>>>>
>>>>Thu, Oct 10, 2024 at 03:48:02PM CEST, [e-mail] wrote:
>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>Sent: Thursday, October 10, 2024 1:36 PM
>>>>>>
>>>>>>Thu, Oct 10, 2024 at 11:53:30AM CEST, [e-mail]
>>>>>>wrote:
>>>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>>>Sent: Wednesday, October 9, 2024 4:07 PM
>>>>>>>>
>>>>>>>>Wed, Oct 09, 2024 at 03:38:38PM CEST, [e-mail]
>>>>>>>>wrote:
>>>>>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>>>>>Sent: Wednesday, October 9, 2024 2:26 PM
>>>>>>>>>>
>>>>>>>>>>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 <[e-mail]>
>>>>>>>>>>---
>>>>>>>>>> Documentation/netlink/specs/dpll.yaml | 28
>>>>>>>>>>+++++++++++++++++++++++++++
>>>>>>>>>> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>>>>>>>>>> include/linux/dpll.h                  |  4 ++++
>>>>>>>>>> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>>>>>>>>>> 4 files changed, 75 insertions(+)
>>>>>>>>>>
>>>>>>>>>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>>b/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>>index f2894ca35de8..77a8e9ddb254 100644
>>>>>>>>>>--- a/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>>+++ b/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>>@@ -85,6 +85,30 @@ 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.
>>>>>>>>>
>>>>>>>>>Hi Jiri,
>>>>>>>>>
>>>>>>>>>Thanks for your work on this!
>>>>>>>>>
>>>>>>>>>I do like the idea, but this part is a bit tricky.
>>>>>>>>>
>>>>>>>>>I assume it is all about clock/quality levels as mentioned in ITU-T
>>>>>>>>>spec "Table 11-7" of REC-G.8264?
>>>>>>>>
>>>>>>>>For now, yes. That is the usecase I have currently. But, if anyone will
>>>>>>>>have
>>>>>>>>a
>>>>>>>>need to introduce any sort of different quality, I don't see why not.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>Then what about table 11-8?
>>>>>>>>
>>>>>>>>The names do not overlap. So if anyone need to add those, he is free to
>>>>>>>>do
>>>>>>>>it.
>>>>>>>>
>>>>>>>
>>>>>>>Not true, some names do overlap: ePRC/eEEC/ePRTC/PRTC.
>>>>>>>As you already pointed below :)
>>>>>>
>>>>>>Yep, sure.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>And in general about option 2(3?) networks?
>>>>>>>>>
>>>>>>>>>AFAIR there are 3 (I don't think 3rd is relevant? But still defined In
>>>>>>>>>REC-G.781, also REC-G.781 doesn't provide clock types at all, just
>>>>>>>>>Quality Levels).
>>>>>>>>>
>>>>>>>>>Assuming 2(3?) network options shall be available, either user can
>>>>>>>>>select the one which is shown, or driver just provides all (if can,
>>>>>>>>>one/none otherwise)?
>>>>>>>>>
>>>>>>>>>If we don't want to give the user control and just let the driver to
>>>>>>>>>either provide this or not, my suggestion would be to name the
>>>>>>>>>attribute appropriately: "clock-quality-level-o1" to make clear
>>>>>>>>>provided attribute belongs to option 1 network.
>>>>>>>>
>>>>>>>>I was thinking about that but there are 2 groups of names in both
>>>>>>>>tables:
>>>>>>>>1) different quality levels and names. Then "o1/2" in the name is not
>>>>>>>>   really needed, as the name itself is the differentiator.
>>>>>>>>2) same quality leves in both options. Those are:
>>>>>>>>   PRTC
>>>>>>>>   ePRTC
>>>>>>>>   eEEC
>>>>>>>>   ePRC
>>>>>>>>   And for thesee, using "o1/2" prefix would lead to have 2 enum values
>>>>>>>>   for exactly the same quality level.
>>>>>>>>
>>>>>>>
>>>>>>>Those names overlap but corresponding SSM is different depending on
>>>>>>>the network option, providing one of those without network option will
>>>>>>>confuse users.
>>>>>>
>>>>>>The ssm code is different, but that is irrelevant in context of this
>>>>>>UAPI. Clock quality levels are the same, that's what matters, isn't it?
>>>>>>
>>>>>
>>>>>This is relevant to user if the clock provides both.
>>>>>I.e., given clock meets requirements for both Option1:PRC and
>>>>>Option2:PRS.
>>>>>How would you provide both of those to the user?
>>>>
>>>>Currently, the attr is single value. So you imply that there is usecase
>>>>to report multiple clock quality at a single time?
>>>>
>>>
>>>Yes, correct. The userspace would decide which one to use.
>>
>>Wait what? What do you mean by "which one to "use""?
>>
>
>Generally if you provide some information to user, he shall use it
>somehow, right?
>
>Maybe you could explain the use case as you are the one developing it?
>Is this to tell user a quality level of your device itself - without
>anything connected?
>Does it change during runtime? (i.e. locking to a neighbor?)

Nope. It is the quality of clock the device is running. Locking does not
change that. We propagate this info over SyncE ESMC message.


>
>I already explained my concerns: if device would be providing multiple
>clock qualities for different specification options (i.e. ITU-T Option
>1/2(/3)) and they would differ for single device with regards to the
>meeting certain class spec requirements, the list of those would be
>required. Since the names for both do overlap it would be not possible
>to let the user know accurately without changes to the uapi.
>
>>
>>>
>>>>Even with that. "PRC" and "PRS" names are enough to differenciate.
>>>>option prefix is redundant.
>>>>
>>>
>>>I do not ask for option prefix in the enum names, but specify somehow
>>>the option you do provide, and ability easily expand the uapi to provide
>>>both at the same time.. Backend can wait for someone to actually
>>>implement the option2, but we don't want to change uapi later, right?
>>
>>So far, I fail to see what is the need for exposing the "option" info. I
>>may be missing something.
>>
>
>See above and below :)
>
>>
>>>
>>>>
>>>>>
>>>>>The patch implements only option1 but the attribute shall
>>>>>be named adequately. So the user doesn't have to look for it
>>>>>or guessing around.
>>>>>After all it is not just DPLL_A_CLOCK_QUALITY_LEVEL.
>>>>>It is either DPLL_A_CLOCK_QUALITY_LEVEL_OPTION1=X or a tuple:
>>>>>DPLL_A_CLOCK_QUALITY_LEVEL=X + DPLL_A_CLOCK_QUALITY_OPTION=1.
>>>>
>>>>Why exactly do you need to expose "option"? What's the usecase?
>>>>
>>>
>>>The use case is to simply provide accurate information.
>>>With proposed changes the user will not know if provided class of
>>>ePRC is option 1 or 2.
>>
>>How exactly does those 2 differ in terms of clock quality? If they
>>don't, why to differenciate them?
>
>All I could find is that the same name of clock class the quality
>shall not differ. But again I am not saying we shall differentiate
>the clock class.. Rather an ability to provide each one, keeping in
>mind that device can meet different O1 vs O2 (vs 03.
>
>How would we extend the interface to provide Option 2 clock class?
>I still see some entropy, I mean higher then expected..
>Let's say device meets PRC for Option1 and PRS for Option2, how would
>we provide this?
>The same attribute twice? This seems also possibly confusing for
>certain cases.

DPLL_A_CLOCK_QUALITY_LEVEL with value DPLL_CLOCK_QUALITY_LEVEL_ITU_PRC
DPLL_A_CLOCK_QUALITY_LEVEL with value DPLL_CLOCK_QUALITY_LEVEL_ITU_PRS

same attribute 2 times with different value.


But, in the meantime, I learned that for example ePRTC certification
may differ for option 1 and option 2. So we need to provide it. I will
just put it into the enum name. Looks as the best solution, UAPI and
internal op api-wise.


>
>>
>>
>>>
>>>>
>>>>>mlx code in 2/2 indicates this is option 1.
>>>>>Why uapi shall be silent about it?
>>>>
>>>>Why is that needed? Also, uapi should provide some sort of abstraction.
>>>>"option1/2" is very ITU/SyncE specific. The idea is to be able to reuse
>>>>"quality-level" attr for non-synce usecases.
>>>>
>>>
>>>Well, actually great point, makes most sense to me.
>>>Then the design shall be some kind of list of tuples?
>>>
>>>Like:
>>>--dump get-device
>>>{
>>>  'clock-id': 4658613174691233804,
>>>  'id':1,
>>>  'type':eec,
>>>  ...
>>>
>>>  'clock_spec':
>>>  [
>>>    {
>>>      "type": itu-option1,
>>>      "quality-level": eprc
>>>    },
>>>    {
>>>      "type": itu-option2,
>>>      "quality-level": eprc
>>>    },
>>>    ...
>>>  ]
>>>  ...
>>>}
>>>
>>>With assumption that for now only one "type" of itu-option1, but with
>>>ability to easily expand the uapi.
>>>
>>>The "quality-level" is already defined, and seems fine to me.
>>>
>>>Does it make sense?
>>
>>Sort of. I would still very much like to avoid exposing multiple values
>>at a time as it complicates the implementation, namely driver op.
>>
>>
>
>Well, true.
>
>Thank you!
>Arkadiusz
>
>>
>>
>>>
>>>Thank you!
>>>Arkadiusz
>>>
>>>>
>>>>>
>>>>>Thank you!
>>>>>Arkadiusz
>>>>>
>>>>>>
>>>>>>>
>>>>>>>For me one enum list for clock types/quality sounds good.
>>>>>>>
>>>>>>>>But, talking about prefixes, perhaps I can put "ITU" as a prefix to
>>>>>>>>indicate
>>>>>>>>this is ITU standartized clock quality leaving option for some other
>>>>>>>>clock
>>>>>>>>quality namespace to appear?
>>>>>>>>
>>>>>>>>[..]
>>>>>>>
>>>>>>>Sure, also makes sense.
>>>>>>>
>>>>>>>But I still believe the attribute name shall also contain the info that
>>>>>>>it conveys an option1 clock type. As the device can meet both
>>>>>>>specifications
>>>>>>>at once, we need to make sure user knows that.
>>>>>>
>>>>>>As I described, I don't see any reason why. Just adds unnecessary
>>>>>>redundancy to uapi.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>Thank you!
>>>>>>>Arkadiusz
>

Arkadiusz Kubalewski Oct. 11, 2024, 7:50 p.m. UTC | #13

>From: Jiri Pirko <[e-mail]>
>Sent: Friday, October 11, 2024 5:57 PM
>
>Fri, Oct 11, 2024 at 04:25:09PM CEST, [e-mail] wrote:
>>>From: Jiri Pirko <[e-mail]>
>>>Sent: Friday, October 11, 2024 8:46 AM
>>>
>>>Thu, Oct 10, 2024 at 06:02:56PM CEST, [e-mail] wrote:
>>>>>From: Jiri Pirko <[e-mail]>
>>>>>Sent: Thursday, October 10, 2024 4:37 PM
>>>>>
>>>>>Thu, Oct 10, 2024 at 03:48:02PM CEST, [e-mail]
>>>>>wrote:
>>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>>Sent: Thursday, October 10, 2024 1:36 PM
>>>>>>>
>>>>>>>Thu, Oct 10, 2024 at 11:53:30AM CEST, [e-mail]
>>>>>>>wrote:
>>>>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>>>>Sent: Wednesday, October 9, 2024 4:07 PM
>>>>>>>>>
>>>>>>>>>Wed, Oct 09, 2024 at 03:38:38PM CEST, [e-mail]
>>>>>>>>>wrote:
>>>>>>>>>>>From: Jiri Pirko <[e-mail]>
>>>>>>>>>>>Sent: Wednesday, October 9, 2024 2:26 PM
>>>>>>>>>>>
>>>>>>>>>>>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 <[e-mail]>
>>>>>>>>>>>---
>>>>>>>>>>> Documentation/netlink/specs/dpll.yaml | 28
>>>>>>>>>>>+++++++++++++++++++++++++++
>>>>>>>>>>> drivers/dpll/dpll_netlink.c           | 22 +++++++++++++++++++++
>>>>>>>>>>> include/linux/dpll.h                  |  4 ++++
>>>>>>>>>>> include/uapi/linux/dpll.h             | 21 ++++++++++++++++++++
>>>>>>>>>>> 4 files changed, 75 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>>>b/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>>>index f2894ca35de8..77a8e9ddb254 100644
>>>>>>>>>>>--- a/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>>>+++ b/Documentation/netlink/specs/dpll.yaml
>>>>>>>>>>>@@ -85,6 +85,30 @@ 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.
>>>>>>>>>>
>>>>>>>>>>Hi Jiri,
>>>>>>>>>>
>>>>>>>>>>Thanks for your work on this!
>>>>>>>>>>
>>>>>>>>>>I do like the idea, but this part is a bit tricky.
>>>>>>>>>>
>>>>>>>>>>I assume it is all about clock/quality levels as mentioned in ITU-T
>>>>>>>>>>spec "Table 11-7" of REC-G.8264?
>>>>>>>>>
>>>>>>>>>For now, yes. That is the usecase I have currently. But, if anyone
>>>>>>>>>will
>>>>>>>>>have
>>>>>>>>>a
>>>>>>>>>need to introduce any sort of different quality, I don't see why not.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>Then what about table 11-8?
>>>>>>>>>
>>>>>>>>>The names do not overlap. So if anyone need to add those, he is free
>>>>>>>>>to
>>>>>>>>>do
>>>>>>>>>it.
>>>>>>>>>
>>>>>>>>
>>>>>>>>Not true, some names do overlap: ePRC/eEEC/ePRTC/PRTC.
>>>>>>>>As you already pointed below :)
>>>>>>>
>>>>>>>Yep, sure.
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>And in general about option 2(3?) networks?
>>>>>>>>>>
>>>>>>>>>>AFAIR there are 3 (I don't think 3rd is relevant? But still defined
>>>>>>>>>>In
>>>>>>>>>>REC-G.781, also REC-G.781 doesn't provide clock types at all, just
>>>>>>>>>>Quality Levels).
>>>>>>>>>>
>>>>>>>>>>Assuming 2(3?) network options shall be available, either user can
>>>>>>>>>>select the one which is shown, or driver just provides all (if can,
>>>>>>>>>>one/none otherwise)?
>>>>>>>>>>
>>>>>>>>>>If we don't want to give the user control and just let the driver to
>>>>>>>>>>either provide this or not, my suggestion would be to name the
>>>>>>>>>>attribute appropriately: "clock-quality-level-o1" to make clear
>>>>>>>>>>provided attribute belongs to option 1 network.
>>>>>>>>>
>>>>>>>>>I was thinking about that but there are 2 groups of names in both
>>>>>>>>>tables:
>>>>>>>>>1) different quality levels and names. Then "o1/2" in the name is not
>>>>>>>>>   really needed, as the name itself is the differentiator.
>>>>>>>>>2) same quality leves in both options. Those are:
>>>>>>>>>   PRTC
>>>>>>>>>   ePRTC
>>>>>>>>>   eEEC
>>>>>>>>>   ePRC
>>>>>>>>>   And for thesee, using "o1/2" prefix would lead to have 2 enum
>>>>>>>>>values
>>>>>>>>>   for exactly the same quality level.
>>>>>>>>>
>>>>>>>>
>>>>>>>>Those names overlap but corresponding SSM is different depending on
>>>>>>>>the network option, providing one of those without network option will
>>>>>>>>confuse users.
>>>>>>>
>>>>>>>The ssm code is different, but that is irrelevant in context of this
>>>>>>>UAPI. Clock quality levels are the same, that's what matters, isn't it?
>>>>>>>
>>>>>>
>>>>>>This is relevant to user if the clock provides both.
>>>>>>I.e., given clock meets requirements for both Option1:PRC and
>>>>>>Option2:PRS.
>>>>>>How would you provide both of those to the user?
>>>>>
>>>>>Currently, the attr is single value. So you imply that there is usecase
>>>>>to report multiple clock quality at a single time?
>>>>>
>>>>
>>>>Yes, correct. The userspace would decide which one to use.
>>>
>>>Wait what? What do you mean by "which one to "use""?
>>>
>>
>>Generally if you provide some information to user, he shall use it
>>somehow, right?
>>
>>Maybe you could explain the use case as you are the one developing it?
>>Is this to tell user a quality level of your device itself - without
>>anything connected?
>>Does it change during runtime? (i.e. locking to a neighbor?)
>
>Nope. It is the quality of clock the device is running. Locking does not
>change that. We propagate this info over SyncE ESMC message.
>

Ok, but we shall also add such explanation somewhere in docs. To make sure
everyone knows that it is provided as kind of "default" clock quality.
Whenever dpll is in unlocked state, just to make sure there is no confusion
on userspace.

>
>>
>>I already explained my concerns: if device would be providing multiple
>>clock qualities for different specification options (i.e. ITU-T Option
>>1/2(/3)) and they would differ for single device with regards to the
>>meeting certain class spec requirements, the list of those would be
>>required. Since the names for both do overlap it would be not possible
>>to let the user know accurately without changes to the uapi.
>>
>>>
>>>>
>>>>>Even with that. "PRC" and "PRS" names are enough to differenciate.
>>>>>option prefix is redundant.
>>>>>
>>>>
>>>>I do not ask for option prefix in the enum names, but specify somehow
>>>>the option you do provide, and ability easily expand the uapi to provide
>>>>both at the same time.. Backend can wait for someone to actually
>>>>implement the option2, but we don't want to change uapi later, right?
>>>
>>>So far, I fail to see what is the need for exposing the "option" info. I
>>>may be missing something.
>>>
>>
>>See above and below :)
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>The patch implements only option1 but the attribute shall
>>>>>>be named adequately. So the user doesn't have to look for it
>>>>>>or guessing around.
>>>>>>After all it is not just DPLL_A_CLOCK_QUALITY_LEVEL.
>>>>>>It is either DPLL_A_CLOCK_QUALITY_LEVEL_OPTION1=X or a tuple:
>>>>>>DPLL_A_CLOCK_QUALITY_LEVEL=X + DPLL_A_CLOCK_QUALITY_OPTION=1.
>>>>>
>>>>>Why exactly do you need to expose "option"? What's the usecase?
>>>>>
>>>>
>>>>The use case is to simply provide accurate information.
>>>>With proposed changes the user will not know if provided class of
>>>>ePRC is option 1 or 2.
>>>
>>>How exactly does those 2 differ in terms of clock quality? If they
>>>don't, why to differenciate them?
>>
>>All I could find is that the same name of clock class the quality
>>shall not differ. But again I am not saying we shall differentiate
>>the clock class.. Rather an ability to provide each one, keeping in
>>mind that device can meet different O1 vs O2 (vs 03.
>>
>>How would we extend the interface to provide Option 2 clock class?
>>I still see some entropy, I mean higher then expected..
>>Let's say device meets PRC for Option1 and PRS for Option2, how would
>>we provide this?
>>The same attribute twice? This seems also possibly confusing for
>>certain cases.
>
>DPLL_A_CLOCK_QUALITY_LEVEL with value DPLL_CLOCK_QUALITY_LEVEL_ITU_PRC
>DPLL_A_CLOCK_QUALITY_LEVEL with value DPLL_CLOCK_QUALITY_LEVEL_ITU_PRS
>
>same attribute 2 times with different value.
>
>
>But, in the meantime, I learned that for example ePRTC certification
>may differ for option 1 and option 2. So we need to provide it. I will
>just put it into the enum name. Looks as the best solution, UAPI and
>internal op api-wise.
>

Fine, but then we also shall have 'multi-attr: true' for this attribute
within this patch?

Thank you!
Arkadiusz

[..]

Patch

diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
index f2894ca35de8..77a8e9ddb254 100644
--- a/Documentation/netlink/specs/dpll.yaml
+++ b/Documentation/netlink/specs/dpll.yaml
@@ -85,6 +85,30 @@  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.
+    entries:
+      -
+        name: prc
+        value: 1
+      -
+        name: ssu-a
+      -
+        name: ssu-b
+      -
+        name: eec1
+      -
+        name: prtc
+      -
+        name: eprtc
+      -
+        name: eeec
+      -
+        name: eprc
+    render-max: true
   -
     type: const
     name: temp-divider
@@ -252,6 +276,10 @@  attribute-sets:
         name: lock-status-error
         type: u32
         enum: lock-status-error
+      -
+        name: clock-quality-level
+        type: u32
+        enum: clock-quality-level
   -
     name: pin
     enum-name: dpll_a_pin
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index fc0280dcddd1..689a6d0ff049 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -169,6 +169,25 @@  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);
+	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), &ql, extack);
+	if (ret)
+		return ret;
+	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 +576,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..e99cdb8ab02c 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,
+				       enum dpll_clock_quality_level *ql,
+				       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..0572f9376da4 100644
--- a/include/uapi/linux/dpll.h
+++ b/include/uapi/linux/dpll.h
@@ -79,6 +79,26 @@  enum dpll_lock_status_error {
 	DPLL_LOCK_STATUS_ERROR_MAX = (__DPLL_LOCK_STATUS_ERROR_MAX - 1)
 };
 
+/**
+ * enum dpll_clock_quality_level - if previous status change was done due to a
+ *   failure, this provides information of dpll device lock status error. Valid
+ *   values for DPLL_A_LOCK_STATUS_ERROR attribute
+ */
+enum dpll_clock_quality_level {
+	DPLL_CLOCK_QUALITY_LEVEL_PRC = 1,
+	DPLL_CLOCK_QUALITY_LEVEL_SSU_A,
+	DPLL_CLOCK_QUALITY_LEVEL_SSU_B,
+	DPLL_CLOCK_QUALITY_LEVEL_EEC1,
+	DPLL_CLOCK_QUALITY_LEVEL_PRTC,
+	DPLL_CLOCK_QUALITY_LEVEL_EPRTC,
+	DPLL_CLOCK_QUALITY_LEVEL_EEEC,
+	DPLL_CLOCK_QUALITY_LEVEL_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 +200,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)