[patch net-next 0/3] dpll: expose fractional frequency offset value to user

[patch net-next 0/3] dpll: expose fractional frequency offset value to user

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Allow to expose pin fractional frequency offset value over new DPLL
generic netlink attribute. Add an op to get the value from the driver.
Implement this new op in mlx5 driver.

Jiri Pirko (3):
  dpll: expose fractional frequency offset value to user
  net/mlx5: DPLL, Use struct to get values from
    mlx5_dpll_synce_status_get()
  net/mlx5: DPLL, Implement fractional frequency offset get pin op

 Documentation/netlink/specs/dpll.yaml         | 11 +++
 drivers/dpll/dpll_netlink.c                   | 24 +++++
 .../net/ethernet/mellanox/mlx5/core/dpll.c    | 94 ++++++++++++-------
 include/linux/dpll.h                          |  3 +
 include/uapi/linux/dpll.h                     |  1 +
 5 files changed, 98 insertions(+), 35 deletions(-)

-- 
2.43.0

[patch net-next 1/3] dpll: expose fractional frequency offset value to user

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Add a new netlink attribute to expose fractional frequency offset value
for a pin. Add an op to get the value from the driver.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 Documentation/netlink/specs/dpll.yaml | 11 +++++++++++
 drivers/dpll/dpll_netlink.c           | 24 ++++++++++++++++++++++++
 include/linux/dpll.h                  |  3 +++
 include/uapi/linux/dpll.h             |  1 +
 4 files changed, 39 insertions(+)

diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
index cf8abe1c0550..b14aed18065f 100644
--- a/Documentation/netlink/specs/dpll.yaml
+++ b/Documentation/netlink/specs/dpll.yaml
@@ -296,6 +296,16 @@ attribute-sets:
       -
         name: phase-offset
         type: s64
+      -
+        name: fractional-frequency-offset
+        type: sint
+        doc: |
+          The FFO (Fractional Frequency Offset) between the RX and TX
+          symbol rate on the media associated with the pin:
+          (rx_frequency-tx_frequency)/rx_frequency
+          Value is in PPM (parts per million).
+          This may be implemented for example for pin of type
+          PIN_TYPE_SYNCE_ETH_PORT.
   -
     name: pin-parent-device
     subset-of: pin
@@ -460,6 +470,7 @@ operations:
             - phase-adjust-min
             - phase-adjust-max
             - phase-adjust
+            - fractional-frequency-offset
 
       dump:
         pre: dpll-lock-dumpit
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index 21c627e9401a..3370dbddb86b 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -263,6 +263,27 @@ dpll_msg_add_phase_offset(struct sk_buff *msg, struct dpll_pin *pin,
 	return 0;
 }
 
+static int dpll_msg_add_ffo(struct sk_buff *msg, struct dpll_pin *pin,
+			    struct dpll_pin_ref *ref,
+			    struct netlink_ext_ack *extack)
+{
+	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
+	struct dpll_device *dpll = ref->dpll;
+	s64 ffo;
+	int ret;
+
+	if (!ops->ffo_get)
+		return 0;
+	ret = ops->ffo_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
+			   dpll, dpll_priv(dpll), &ffo, extack);
+	if (ret) {
+		if (ret == -ENODATA)
+			return 0;
+		return ret;
+	}
+	return nla_put_sint(msg, DPLL_A_PIN_FRACTIONAL_FREQUENCY_OFFSET, ffo);
+}
+
 static int
 dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
 		      struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
@@ -440,6 +461,9 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
 			prop->phase_range.max))
 		return -EMSGSIZE;
 	ret = dpll_msg_add_pin_phase_adjust(msg, pin, ref, extack);
+	if (ret)
+		return ret;
+	ret = dpll_msg_add_ffo(msg, pin, ref, extack);
 	if (ret)
 		return ret;
 	if (xa_empty(&pin->parent_refs))
diff --git a/include/linux/dpll.h b/include/linux/dpll.h
index b1a5f9ca8ee5..9cf896ea1d41 100644
--- a/include/linux/dpll.h
+++ b/include/linux/dpll.h
@@ -77,6 +77,9 @@ struct dpll_pin_ops {
 				const struct dpll_device *dpll, void *dpll_priv,
 				const s32 phase_adjust,
 				struct netlink_ext_ack *extack);
+	int (*ffo_get)(const struct dpll_pin *pin, void *pin_priv,
+		       const struct dpll_device *dpll, void *dpll_priv,
+		       s64 *ffo, struct netlink_ext_ack *extack);
 };
 
 struct dpll_pin_frequency {
diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
index 715a491d2727..b4e947f9bfbc 100644
--- a/include/uapi/linux/dpll.h
+++ b/include/uapi/linux/dpll.h
@@ -179,6 +179,7 @@ enum dpll_a_pin {
 	DPLL_A_PIN_PHASE_ADJUST_MAX,
 	DPLL_A_PIN_PHASE_ADJUST,
 	DPLL_A_PIN_PHASE_OFFSET,
+	DPLL_A_PIN_FRACTIONAL_FREQUENCY_OFFSET,
 
 	__DPLL_A_PIN_MAX,
 	DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1)
-- 
2.43.0

[patch net-next 2/3] net/mlx5: DPLL, Use struct to get values from mlx5_dpll_synce_status_get()

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Instead of passing separate args, introduce
struct mlx5_dpll_synce_status to hold the values obtained by
mlx5_dpll_synce_status_get().

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/dpll.c    | 63 +++++++++----------
 1 file changed, 28 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
index a7ffd61fe248..dbe09d2f2069 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
@@ -36,11 +36,15 @@ static int mlx5_dpll_clock_id_get(struct mlx5_core_dev *mdev, u64 *clock_id)
 	return 0;
 }
 
+struct mlx5_dpll_synce_status {
+	enum mlx5_msees_admin_status admin_status;
+	enum mlx5_msees_oper_status oper_status;
+	bool ho_acq;
+};
+
 static int
 mlx5_dpll_synce_status_get(struct mlx5_core_dev *mdev,
-			   enum mlx5_msees_admin_status *admin_status,
-			   enum mlx5_msees_oper_status *oper_status,
-			   bool *ho_acq)
+			   struct mlx5_dpll_synce_status *synce_status)
 {
 	u32 out[MLX5_ST_SZ_DW(msees_reg)] = {};
 	u32 in[MLX5_ST_SZ_DW(msees_reg)] = {};
@@ -50,11 +54,9 @@ mlx5_dpll_synce_status_get(struct mlx5_core_dev *mdev,
 				   MLX5_REG_MSEES, 0, 0);
 	if (err)
 		return err;
-	if (admin_status)
-		*admin_status = MLX5_GET(msees_reg, out, admin_status);
-	*oper_status = MLX5_GET(msees_reg, out, oper_status);
-	if (ho_acq)
-		*ho_acq = MLX5_GET(msees_reg, out, ho_acq);
+	synce_status->admin_status = MLX5_GET(msees_reg, out, admin_status);
+	synce_status->oper_status = MLX5_GET(msees_reg, out, oper_status);
+	synce_status->ho_acq = MLX5_GET(msees_reg, out, ho_acq);
 	return 0;
 }
 
@@ -74,14 +76,14 @@ mlx5_dpll_synce_status_set(struct mlx5_core_dev *mdev,
 }
 
 static enum dpll_lock_status
-mlx5_dpll_lock_status_get(enum mlx5_msees_oper_status oper_status, bool ho_acq)
+mlx5_dpll_lock_status_get(struct mlx5_dpll_synce_status *synce_status)
 {
-	switch (oper_status) {
+	switch (synce_status->oper_status) {
 	case MLX5_MSEES_OPER_STATUS_SELF_TRACK:
 		fallthrough;
 	case MLX5_MSEES_OPER_STATUS_OTHER_TRACK:
-		return ho_acq ? DPLL_LOCK_STATUS_LOCKED_HO_ACQ :
-				DPLL_LOCK_STATUS_LOCKED;
+		return synce_status->ho_acq ? DPLL_LOCK_STATUS_LOCKED_HO_ACQ :
+					      DPLL_LOCK_STATUS_LOCKED;
 	case MLX5_MSEES_OPER_STATUS_HOLDOVER:
 		fallthrough;
 	case MLX5_MSEES_OPER_STATUS_FAIL_HOLDOVER:
@@ -92,12 +94,11 @@ mlx5_dpll_lock_status_get(enum mlx5_msees_oper_status oper_status, bool ho_acq)
 }
 
 static enum dpll_pin_state
-mlx5_dpll_pin_state_get(enum mlx5_msees_admin_status admin_status,
-			enum mlx5_msees_oper_status oper_status)
+mlx5_dpll_pin_state_get(struct mlx5_dpll_synce_status *synce_status)
 {
-	return (admin_status == MLX5_MSEES_ADMIN_STATUS_TRACK &&
-		(oper_status == MLX5_MSEES_OPER_STATUS_SELF_TRACK ||
-		 oper_status == MLX5_MSEES_OPER_STATUS_OTHER_TRACK)) ?
+	return (synce_status->admin_status == MLX5_MSEES_ADMIN_STATUS_TRACK &&
+		(synce_status->oper_status == MLX5_MSEES_OPER_STATUS_SELF_TRACK ||
+		 synce_status->oper_status == MLX5_MSEES_OPER_STATUS_OTHER_TRACK)) ?
 	       DPLL_PIN_STATE_CONNECTED : DPLL_PIN_STATE_DISCONNECTED;
 }
 
@@ -106,17 +107,14 @@ static int mlx5_dpll_device_lock_status_get(const struct dpll_device *dpll,
 					    enum dpll_lock_status *status,
 					    struct netlink_ext_ack *extack)
 {
-	enum mlx5_msees_oper_status oper_status;
+	struct mlx5_dpll_synce_status synce_status;
 	struct mlx5_dpll *mdpll = priv;
-	bool ho_acq;
 	int err;
 
-	err = mlx5_dpll_synce_status_get(mdpll->mdev, NULL,
-					 &oper_status, &ho_acq);
+	err = mlx5_dpll_synce_status_get(mdpll->mdev, &synce_status);
 	if (err)
 		return err;
-
-	*status = mlx5_dpll_lock_status_get(oper_status, ho_acq);
+	*status = mlx5_dpll_lock_status_get(&synce_status);
 	return 0;
 }
 
@@ -151,16 +149,14 @@ static int mlx5_dpll_state_on_dpll_get(const struct dpll_pin *pin,
 				       enum dpll_pin_state *state,
 				       struct netlink_ext_ack *extack)
 {
-	enum mlx5_msees_admin_status admin_status;
-	enum mlx5_msees_oper_status oper_status;
+	struct mlx5_dpll_synce_status synce_status;
 	struct mlx5_dpll *mdpll = pin_priv;
 	int err;
 
-	err = mlx5_dpll_synce_status_get(mdpll->mdev, &admin_status,
-					 &oper_status, NULL);
+	err = mlx5_dpll_synce_status_get(mdpll->mdev, &synce_status);
 	if (err)
 		return err;
-	*state = mlx5_dpll_pin_state_get(admin_status, oper_status);
+	*state = mlx5_dpll_pin_state_get(&synce_status);
 	return 0;
 }
 
@@ -202,19 +198,16 @@ static void mlx5_dpll_periodic_work(struct work_struct *work)
 {
 	struct mlx5_dpll *mdpll = container_of(work, struct mlx5_dpll,
 					       work.work);
-	enum mlx5_msees_admin_status admin_status;
-	enum mlx5_msees_oper_status oper_status;
+	struct mlx5_dpll_synce_status synce_status;
 	enum dpll_lock_status lock_status;
 	enum dpll_pin_state pin_state;
-	bool ho_acq;
 	int err;
 
-	err = mlx5_dpll_synce_status_get(mdpll->mdev, &admin_status,
-					 &oper_status, &ho_acq);
+	err = mlx5_dpll_synce_status_get(mdpll->mdev, &synce_status);
 	if (err)
 		goto err_out;
-	lock_status = mlx5_dpll_lock_status_get(oper_status, ho_acq);
-	pin_state = mlx5_dpll_pin_state_get(admin_status, oper_status);
+	lock_status = mlx5_dpll_lock_status_get(&synce_status);
+	pin_state = mlx5_dpll_pin_state_get(&synce_status);
 
 	if (!mdpll->last.valid)
 		goto invalid_out;
-- 
2.43.0

[patch net-next 3/3] net/mlx5: DPLL, Implement fractional frequency offset get pin op

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Implement ffo_get() pin op filling it up to MSEED.frequency_diff value.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/dpll.c    | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
index dbe09d2f2069..18fed2b34fb1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
@@ -40,6 +40,8 @@ struct mlx5_dpll_synce_status {
 	enum mlx5_msees_admin_status admin_status;
 	enum mlx5_msees_oper_status oper_status;
 	bool ho_acq;
+	bool oper_freq_measure;
+	s32 frequency_diff;
 };
 
 static int
@@ -57,6 +59,8 @@ mlx5_dpll_synce_status_get(struct mlx5_core_dev *mdev,
 	synce_status->admin_status = MLX5_GET(msees_reg, out, admin_status);
 	synce_status->oper_status = MLX5_GET(msees_reg, out, oper_status);
 	synce_status->ho_acq = MLX5_GET(msees_reg, out, ho_acq);
+	synce_status->oper_freq_measure = MLX5_GET(msees_reg, out, oper_freq_measure);
+	synce_status->frequency_diff = MLX5_GET(msees_reg, out, frequency_diff);
 	return 0;
 }
 
@@ -69,8 +73,10 @@ mlx5_dpll_synce_status_set(struct mlx5_core_dev *mdev,
 
 	MLX5_SET(msees_reg, in, field_select,
 		 MLX5_MSEES_FIELD_SELECT_ENABLE |
+		 MLX5_MSEES_FIELD_SELECT_ADMIN_FREQ_MEASURE |
 		 MLX5_MSEES_FIELD_SELECT_ADMIN_STATUS);
 	MLX5_SET(msees_reg, in, admin_status, admin_status);
+	MLX5_SET(msees_reg, in, admin_freq_measure, true);
 	return mlx5_core_access_reg(mdev, in, sizeof(in), out, sizeof(out),
 				    MLX5_REG_MSEES, 0, 1);
 }
@@ -102,6 +108,16 @@ mlx5_dpll_pin_state_get(struct mlx5_dpll_synce_status *synce_status)
 	       DPLL_PIN_STATE_CONNECTED : DPLL_PIN_STATE_DISCONNECTED;
 }
 
+static int
+mlx5_dpll_pin_ffo_get(struct mlx5_dpll_synce_status *synce_status,
+		      s64 *ffo)
+{
+	if (!synce_status->oper_freq_measure)
+		return -ENODATA;
+	*ffo = synce_status->frequency_diff;
+	return 0;
+}
+
 static int mlx5_dpll_device_lock_status_get(const struct dpll_device *dpll,
 					    void *priv,
 					    enum dpll_lock_status *status,
@@ -175,10 +191,25 @@ static int mlx5_dpll_state_on_dpll_set(const struct dpll_pin *pin,
 					  MLX5_MSEES_ADMIN_STATUS_FREE_RUNNING);
 }
 
+static int mlx5_dpll_ffo_get(const struct dpll_pin *pin, void *pin_priv,
+			     const struct dpll_device *dpll, void *dpll_priv,
+			     s64 *ffo, struct netlink_ext_ack *extack)
+{
+	struct mlx5_dpll_synce_status synce_status;
+	struct mlx5_dpll *mdpll = pin_priv;
+	int err;
+
+	err = mlx5_dpll_synce_status_get(mdpll->mdev, &synce_status);
+	if (err)
+		return err;
+	return mlx5_dpll_pin_ffo_get(&synce_status, ffo);
+}
+
 static const struct dpll_pin_ops mlx5_dpll_pins_ops = {
 	.direction_get = mlx5_dpll_pin_direction_get,
 	.state_on_dpll_get = mlx5_dpll_state_on_dpll_get,
 	.state_on_dpll_set = mlx5_dpll_state_on_dpll_set,
+	.ffo_get = mlx5_dpll_ffo_get,
 };
 
 static const struct dpll_pin_properties mlx5_dpll_pin_properties = {
-- 
2.43.0

Re: [patch net-next 0/3] dpll: expose fractional frequency offset value to user

[Jakub Kicinski]

On Wed,  3 Jan 2024 14:28:35 +0100 Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Allow to expose pin fractional frequency offset value over new DPLL
> generic netlink attribute. Add an op to get the value from the driver.
> Implement this new op in mlx5 driver.

Arkadiusz, Vadim, acks?
-- 
pw-bot: needs-ack

Re: [patch net-next 0/3] dpll: expose fractional frequency offset value to user

[Vadim Fedorenko]

On 03/01/2024 13:28, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Allow to expose pin fractional frequency offset value over new DPLL
> generic netlink attribute. Add an op to get the value from the driver.
> Implement this new op in mlx5 driver.
> 
> Jiri Pirko (3):
>    dpll: expose fractional frequency offset value to user
>    net/mlx5: DPLL, Use struct to get values from
>      mlx5_dpll_synce_status_get()
>    net/mlx5: DPLL, Implement fractional frequency offset get pin op
> 
>   Documentation/netlink/specs/dpll.yaml         | 11 +++
>   drivers/dpll/dpll_netlink.c                   | 24 +++++
>   .../net/ethernet/mellanox/mlx5/core/dpll.c    | 94 ++++++++++++-------
>   include/linux/dpll.h                          |  3 +
>   include/uapi/linux/dpll.h                     |  1 +
>   5 files changed, 98 insertions(+), 35 deletions(-)
> 

Interesting attribute, it's good that hardware can expose this info.

Did you think about building some monitoring/alerts based on it?

For the series (I'm not sure if it's enough for mlx5, but the
refactoring looks nice):

Acked-By: Vadim Fedorenko <vadim.fedorenko@linux.dev>

RE: [patch net-next 2/3] net/mlx5: DPLL, Use struct to get values from mlx5_dpll_synce_status_get()

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Wednesday, January 3, 2024 2:29 PM
>
>From: Jiri Pirko <jiri@nvidia.com>
>
>Instead of passing separate args, introduce
>struct mlx5_dpll_synce_status to hold the values obtained by
>mlx5_dpll_synce_status_get().
>
>Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>---
> .../net/ethernet/mellanox/mlx5/core/dpll.c    | 63 +++++++++----------
> 1 file changed, 28 insertions(+), 35 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>index a7ffd61fe248..dbe09d2f2069 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>@@ -36,11 +36,15 @@ static int mlx5_dpll_clock_id_get(struct mlx5_core_dev
>*mdev, u64 *clock_id)
> 	return 0;
> }
>
>+struct mlx5_dpll_synce_status {
>+	enum mlx5_msees_admin_status admin_status;
>+	enum mlx5_msees_oper_status oper_status;
>+	bool ho_acq;
>+};
>+
> static int
> mlx5_dpll_synce_status_get(struct mlx5_core_dev *mdev,
>-			   enum mlx5_msees_admin_status *admin_status,
>-			   enum mlx5_msees_oper_status *oper_status,
>-			   bool *ho_acq)
>+			   struct mlx5_dpll_synce_status *synce_status)
> {
> 	u32 out[MLX5_ST_SZ_DW(msees_reg)] = {};
> 	u32 in[MLX5_ST_SZ_DW(msees_reg)] = {};
>@@ -50,11 +54,9 @@ mlx5_dpll_synce_status_get(struct mlx5_core_dev *mdev,
> 				   MLX5_REG_MSEES, 0, 0);
> 	if (err)
> 		return err;
>-	if (admin_status)
>-		*admin_status = MLX5_GET(msees_reg, out, admin_status);
>-	*oper_status = MLX5_GET(msees_reg, out, oper_status);
>-	if (ho_acq)
>-		*ho_acq = MLX5_GET(msees_reg, out, ho_acq);
>+	synce_status->admin_status = MLX5_GET(msees_reg, out, admin_status);
>+	synce_status->oper_status = MLX5_GET(msees_reg, out, oper_status);
>+	synce_status->ho_acq = MLX5_GET(msees_reg, out, ho_acq);
> 	return 0;
> }
>
>@@ -74,14 +76,14 @@ mlx5_dpll_synce_status_set(struct mlx5_core_dev *mdev,
> }
>
> static enum dpll_lock_status
>-mlx5_dpll_lock_status_get(enum mlx5_msees_oper_status oper_status, bool
>ho_acq)
>+mlx5_dpll_lock_status_get(struct mlx5_dpll_synce_status *synce_status)
> {
>-	switch (oper_status) {
>+	switch (synce_status->oper_status) {
> 	case MLX5_MSEES_OPER_STATUS_SELF_TRACK:
> 		fallthrough;
> 	case MLX5_MSEES_OPER_STATUS_OTHER_TRACK:
>-		return ho_acq ? DPLL_LOCK_STATUS_LOCKED_HO_ACQ :
>-				DPLL_LOCK_STATUS_LOCKED;
>+		return synce_status->ho_acq ? DPLL_LOCK_STATUS_LOCKED_HO_ACQ :
>+					      DPLL_LOCK_STATUS_LOCKED;
> 	case MLX5_MSEES_OPER_STATUS_HOLDOVER:
> 		fallthrough;
> 	case MLX5_MSEES_OPER_STATUS_FAIL_HOLDOVER:
>@@ -92,12 +94,11 @@ mlx5_dpll_lock_status_get(enum mlx5_msees_oper_status
>oper_status, bool ho_acq)
> }
>
> static enum dpll_pin_state
>-mlx5_dpll_pin_state_get(enum mlx5_msees_admin_status admin_status,
>-			enum mlx5_msees_oper_status oper_status)
>+mlx5_dpll_pin_state_get(struct mlx5_dpll_synce_status *synce_status)
> {
>-	return (admin_status == MLX5_MSEES_ADMIN_STATUS_TRACK &&
>-		(oper_status == MLX5_MSEES_OPER_STATUS_SELF_TRACK ||
>-		 oper_status == MLX5_MSEES_OPER_STATUS_OTHER_TRACK)) ?
>+	return (synce_status->admin_status == MLX5_MSEES_ADMIN_STATUS_TRACK
>&&
>+		(synce_status->oper_status == MLX5_MSEES_OPER_STATUS_SELF_TRACK
>||
>+		 synce_status->oper_status ==
>MLX5_MSEES_OPER_STATUS_OTHER_TRACK)) ?
> 	       DPLL_PIN_STATE_CONNECTED : DPLL_PIN_STATE_DISCONNECTED;
> }
>
>@@ -106,17 +107,14 @@ static int mlx5_dpll_device_lock_status_get(const
>struct dpll_device *dpll,
> 					    enum dpll_lock_status *status,
> 					    struct netlink_ext_ack *extack)
> {
>-	enum mlx5_msees_oper_status oper_status;
>+	struct mlx5_dpll_synce_status synce_status;
> 	struct mlx5_dpll *mdpll = priv;
>-	bool ho_acq;
> 	int err;
>
>-	err = mlx5_dpll_synce_status_get(mdpll->mdev, NULL,
>-					 &oper_status, &ho_acq);
>+	err = mlx5_dpll_synce_status_get(mdpll->mdev, &synce_status);
> 	if (err)
> 		return err;
>-
>-	*status = mlx5_dpll_lock_status_get(oper_status, ho_acq);
>+	*status = mlx5_dpll_lock_status_get(&synce_status);
> 	return 0;
> }
>
>@@ -151,16 +149,14 @@ static int mlx5_dpll_state_on_dpll_get(const struct
>dpll_pin *pin,
> 				       enum dpll_pin_state *state,
> 				       struct netlink_ext_ack *extack)
> {
>-	enum mlx5_msees_admin_status admin_status;
>-	enum mlx5_msees_oper_status oper_status;
>+	struct mlx5_dpll_synce_status synce_status;
> 	struct mlx5_dpll *mdpll = pin_priv;
> 	int err;
>
>-	err = mlx5_dpll_synce_status_get(mdpll->mdev, &admin_status,
>-					 &oper_status, NULL);
>+	err = mlx5_dpll_synce_status_get(mdpll->mdev, &synce_status);
> 	if (err)
> 		return err;
>-	*state = mlx5_dpll_pin_state_get(admin_status, oper_status);
>+	*state = mlx5_dpll_pin_state_get(&synce_status);
> 	return 0;
> }
>
>@@ -202,19 +198,16 @@ static void mlx5_dpll_periodic_work(struct
>work_struct *work)
> {
> 	struct mlx5_dpll *mdpll = container_of(work, struct mlx5_dpll,
> 					       work.work);
>-	enum mlx5_msees_admin_status admin_status;
>-	enum mlx5_msees_oper_status oper_status;
>+	struct mlx5_dpll_synce_status synce_status;
> 	enum dpll_lock_status lock_status;
> 	enum dpll_pin_state pin_state;
>-	bool ho_acq;
> 	int err;
>
>-	err = mlx5_dpll_synce_status_get(mdpll->mdev, &admin_status,
>-					 &oper_status, &ho_acq);
>+	err = mlx5_dpll_synce_status_get(mdpll->mdev, &synce_status);
> 	if (err)
> 		goto err_out;
>-	lock_status = mlx5_dpll_lock_status_get(oper_status, ho_acq);
>-	pin_state = mlx5_dpll_pin_state_get(admin_status, oper_status);
>+	lock_status = mlx5_dpll_lock_status_get(&synce_status);
>+	pin_state = mlx5_dpll_pin_state_get(&synce_status);
>
> 	if (!mdpll->last.valid)
> 		goto invalid_out;
>--
>2.43.0

Hi Jiri,

Looks good to me.

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

RE: [patch net-next 1/3] dpll: expose fractional frequency offset value to user

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Wednesday, January 3, 2024 2:29 PM
>
>From: Jiri Pirko <jiri@nvidia.com>
>
>Add a new netlink attribute to expose fractional frequency offset value
>for a pin. Add an op to get the value from the driver.
>
>Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>---
> Documentation/netlink/specs/dpll.yaml | 11 +++++++++++
> drivers/dpll/dpll_netlink.c           | 24 ++++++++++++++++++++++++
> include/linux/dpll.h                  |  3 +++
> include/uapi/linux/dpll.h             |  1 +
> 4 files changed, 39 insertions(+)
>
>diff --git a/Documentation/netlink/specs/dpll.yaml
>b/Documentation/netlink/specs/dpll.yaml
>index cf8abe1c0550..b14aed18065f 100644
>--- a/Documentation/netlink/specs/dpll.yaml
>+++ b/Documentation/netlink/specs/dpll.yaml
>@@ -296,6 +296,16 @@ attribute-sets:
>       -
>         name: phase-offset
>         type: s64
>+      -
>+        name: fractional-frequency-offset
>+        type: sint
>+        doc: |
>+          The FFO (Fractional Frequency Offset) between the RX and TX
>+          symbol rate on the media associated with the pin:
>+          (rx_frequency-tx_frequency)/rx_frequency
>+          Value is in PPM (parts per million).
>+          This may be implemented for example for pin of type
>+          PIN_TYPE_SYNCE_ETH_PORT.
>   -
>     name: pin-parent-device
>     subset-of: pin
>@@ -460,6 +470,7 @@ operations:
>             - phase-adjust-min
>             - phase-adjust-max
>             - phase-adjust
>+            - fractional-frequency-offset
>
>       dump:
>         pre: dpll-lock-dumpit
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index 21c627e9401a..3370dbddb86b 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -263,6 +263,27 @@ dpll_msg_add_phase_offset(struct sk_buff *msg, struct
>dpll_pin *pin,
> 	return 0;
> }
>
>+static int dpll_msg_add_ffo(struct sk_buff *msg, struct dpll_pin *pin,
>+			    struct dpll_pin_ref *ref,
>+			    struct netlink_ext_ack *extack)
>+{
>+	const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>+	struct dpll_device *dpll = ref->dpll;
>+	s64 ffo;
>+	int ret;
>+
>+	if (!ops->ffo_get)
>+		return 0;
>+	ret = ops->ffo_get(pin, dpll_pin_on_dpll_priv(dpll, pin),
>+			   dpll, dpll_priv(dpll), &ffo, extack);
>+	if (ret) {
>+		if (ret == -ENODATA)
>+			return 0;
>+		return ret;
>+	}
>+	return nla_put_sint(msg, DPLL_A_PIN_FRACTIONAL_FREQUENCY_OFFSET,
>ffo);
>+}
>+
> static int
> dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
> 		      struct dpll_pin_ref *ref, struct netlink_ext_ack *extack)
>@@ -440,6 +461,9 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct
>dpll_pin *pin,
> 			prop->phase_range.max))
> 		return -EMSGSIZE;
> 	ret = dpll_msg_add_pin_phase_adjust(msg, pin, ref, extack);
>+	if (ret)
>+		return ret;
>+	ret = dpll_msg_add_ffo(msg, pin, ref, extack);
> 	if (ret)
> 		return ret;
> 	if (xa_empty(&pin->parent_refs))
>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>index b1a5f9ca8ee5..9cf896ea1d41 100644
>--- a/include/linux/dpll.h
>+++ b/include/linux/dpll.h
>@@ -77,6 +77,9 @@ struct dpll_pin_ops {
> 				const struct dpll_device *dpll, void *dpll_priv,
> 				const s32 phase_adjust,
> 				struct netlink_ext_ack *extack);
>+	int (*ffo_get)(const struct dpll_pin *pin, void *pin_priv,
>+		       const struct dpll_device *dpll, void *dpll_priv,
>+		       s64 *ffo, struct netlink_ext_ack *extack);
> };
>
> struct dpll_pin_frequency {
>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>index 715a491d2727..b4e947f9bfbc 100644
>--- a/include/uapi/linux/dpll.h
>+++ b/include/uapi/linux/dpll.h
>@@ -179,6 +179,7 @@ enum dpll_a_pin {
> 	DPLL_A_PIN_PHASE_ADJUST_MAX,
> 	DPLL_A_PIN_PHASE_ADJUST,
> 	DPLL_A_PIN_PHASE_OFFSET,
>+	DPLL_A_PIN_FRACTIONAL_FREQUENCY_OFFSET,
>
> 	__DPLL_A_PIN_MAX,
> 	DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1)
>--
>2.43.0

Hi Jiri,

In general looks good.

But one thing, there is no update to Documentation/driver-api/dpll.rst
Why not mention this new netlink attribute and some explanation for the
userspace in the non-source-code documentation?

Thanks!
Arkadiusz

RE: [patch net-next 3/3] net/mlx5: DPLL, Implement fractional frequency offset get pin op

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Wednesday, January 3, 2024 2:29 PM
>
>From: Jiri Pirko <jiri@nvidia.com>
>
>Implement ffo_get() pin op filling it up to MSEED.frequency_diff value.
>
>Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>---
> .../net/ethernet/mellanox/mlx5/core/dpll.c    | 31 +++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>index dbe09d2f2069..18fed2b34fb1 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>@@ -40,6 +40,8 @@ struct mlx5_dpll_synce_status {
> 	enum mlx5_msees_admin_status admin_status;
> 	enum mlx5_msees_oper_status oper_status;
> 	bool ho_acq;
>+	bool oper_freq_measure;
>+	s32 frequency_diff;
> };
>
> static int
>@@ -57,6 +59,8 @@ mlx5_dpll_synce_status_get(struct mlx5_core_dev *mdev,
> 	synce_status->admin_status = MLX5_GET(msees_reg, out, admin_status);
> 	synce_status->oper_status = MLX5_GET(msees_reg, out, oper_status);
> 	synce_status->ho_acq = MLX5_GET(msees_reg, out, ho_acq);
>+	synce_status->oper_freq_measure = MLX5_GET(msees_reg, out,
>oper_freq_measure);
>+	synce_status->frequency_diff = MLX5_GET(msees_reg, out,
>frequency_diff);
> 	return 0;
> }
>
>@@ -69,8 +73,10 @@ mlx5_dpll_synce_status_set(struct mlx5_core_dev *mdev,
>
> 	MLX5_SET(msees_reg, in, field_select,
> 		 MLX5_MSEES_FIELD_SELECT_ENABLE |
>+		 MLX5_MSEES_FIELD_SELECT_ADMIN_FREQ_MEASURE |
> 		 MLX5_MSEES_FIELD_SELECT_ADMIN_STATUS);
> 	MLX5_SET(msees_reg, in, admin_status, admin_status);
>+	MLX5_SET(msees_reg, in, admin_freq_measure, true);
> 	return mlx5_core_access_reg(mdev, in, sizeof(in), out, sizeof(out),
> 				    MLX5_REG_MSEES, 0, 1);
> }
>@@ -102,6 +108,16 @@ mlx5_dpll_pin_state_get(struct mlx5_dpll_synce_status
>*synce_status)
> 	       DPLL_PIN_STATE_CONNECTED : DPLL_PIN_STATE_DISCONNECTED;
> }
>
>+static int
>+mlx5_dpll_pin_ffo_get(struct mlx5_dpll_synce_status *synce_status,
>+		      s64 *ffo)
>+{
>+	if (!synce_status->oper_freq_measure)
>+		return -ENODATA;
>+	*ffo = synce_status->frequency_diff;
>+	return 0;
>+}
>+
> static int mlx5_dpll_device_lock_status_get(const struct dpll_device
>*dpll,
> 					    void *priv,
> 					    enum dpll_lock_status *status,
>@@ -175,10 +191,25 @@ static int mlx5_dpll_state_on_dpll_set(const struct
>dpll_pin *pin,
> 					  MLX5_MSEES_ADMIN_STATUS_FREE_RUNNING);
> }
>
>+static int mlx5_dpll_ffo_get(const struct dpll_pin *pin, void *pin_priv,
>+			     const struct dpll_device *dpll, void *dpll_priv,
>+			     s64 *ffo, struct netlink_ext_ack *extack)
>+{
>+	struct mlx5_dpll_synce_status synce_status;
>+	struct mlx5_dpll *mdpll = pin_priv;
>+	int err;
>+
>+	err = mlx5_dpll_synce_status_get(mdpll->mdev, &synce_status);
>+	if (err)
>+		return err;
>+	return mlx5_dpll_pin_ffo_get(&synce_status, ffo);
>+}
>+
> static const struct dpll_pin_ops mlx5_dpll_pins_ops = {
> 	.direction_get = mlx5_dpll_pin_direction_get,
> 	.state_on_dpll_get = mlx5_dpll_state_on_dpll_get,
> 	.state_on_dpll_set = mlx5_dpll_state_on_dpll_set,
>+	.ffo_get = mlx5_dpll_ffo_get,
> };
>
> static const struct dpll_pin_properties mlx5_dpll_pin_properties = {
>--
>2.43.0

Hi Jiri,

Looks good to me.

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

Re: [patch net-next 1/3] dpll: expose fractional frequency offset value to user

[Jakub Kicinski]

On Fri, 5 Jan 2024 13:32:00 +0000 Kubalewski, Arkadiusz wrote:
> But one thing, there is no update to Documentation/driver-api/dpll.rst
> Why not mention this new netlink attribute and some explanation for the
> userspace in the non-source-code documentation?

Now that we generate web docs from the specs as well:
https://docs.kernel.org/next/networking/netlink_spec/dpll.html
I reckon documenting in the spec may be good enough?

RE: [patch net-next 0/3] dpll: expose fractional frequency offset value to user

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Wednesday, January 3, 2024 2:29 PM
>
>From: Jiri Pirko <jiri@nvidia.com>
>
>Allow to expose pin fractional frequency offset value over new DPLL generic
>netlink attribute. Add an op to get the value from the driver.
>Implement this new op in mlx5 driver.
>
>Jiri Pirko (3):
>  dpll: expose fractional frequency offset value to user
>  net/mlx5: DPLL, Use struct to get values from
>    mlx5_dpll_synce_status_get()
>  net/mlx5: DPLL, Implement fractional frequency offset get pin op
>
> Documentation/netlink/specs/dpll.yaml         | 11 +++
> drivers/dpll/dpll_netlink.c                   | 24 +++++
> .../net/ethernet/mellanox/mlx5/core/dpll.c    | 94 ++++++++++++-------
> include/linux/dpll.h                          |  3 +
> include/uapi/linux/dpll.h                     |  1 +
> 5 files changed, 98 insertions(+), 35 deletions(-)
>
>--
>2.43.0

Sure, LGTM.

Acked-By: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

RE: [patch net-next 1/3] dpll: expose fractional frequency offset value to user

[Kubalewski, Arkadiusz]

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Friday, January 5, 2024 3:22 PM
>
>On Fri, 5 Jan 2024 13:32:00 +0000 Kubalewski, Arkadiusz wrote:
>> But one thing, there is no update to Documentation/driver-api/dpll.rst
>> Why not mention this new netlink attribute and some explanation for
>> the userspace in the non-source-code documentation?
>
>Now that we generate web docs from the specs as well:
>https://docs.kernel.org/next/networking/netlink_spec/dpll.html
>I reckon documenting in the spec may be good enough?

Yes, that makes sense. Thanks!
Arkadiusz

Re: [patch net-next 0/3] dpll: expose fractional frequency offset value to user

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  3 Jan 2024 14:28:35 +0100 you wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Allow to expose pin fractional frequency offset value over new DPLL
> generic netlink attribute. Add an op to get the value from the driver.
> Implement this new op in mlx5 driver.
> 
> Jiri Pirko (3):
>   dpll: expose fractional frequency offset value to user
>   net/mlx5: DPLL, Use struct to get values from
>     mlx5_dpll_synce_status_get()
>   net/mlx5: DPLL, Implement fractional frequency offset get pin op
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] dpll: expose fractional frequency offset value to user
    https://git.kernel.org/netdev/net-next/c/8a6286c1804e
  - [net-next,2/3] net/mlx5: DPLL, Use struct to get values from mlx5_dpll_synce_status_get()
    https://git.kernel.org/netdev/net-next/c/e6d86938a40a
  - [net-next,3/3] net/mlx5: DPLL, Implement fractional frequency offset get pin op
    https://git.kernel.org/netdev/net-next/c/f035dca34ede

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [patch net-next 0/3] dpll: expose fractional frequency offset value to user

[Jiri Pirko]

Fri, Jan 05, 2024 at 12:44:23PM CET, vadim.fedorenko@linux.dev wrote:
>On 03/01/2024 13:28, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Allow to expose pin fractional frequency offset value over new DPLL
>> generic netlink attribute. Add an op to get the value from the driver.
>> Implement this new op in mlx5 driver.
>> 
>> Jiri Pirko (3):
>>    dpll: expose fractional frequency offset value to user
>>    net/mlx5: DPLL, Use struct to get values from
>>      mlx5_dpll_synce_status_get()
>>    net/mlx5: DPLL, Implement fractional frequency offset get pin op
>> 
>>   Documentation/netlink/specs/dpll.yaml         | 11 +++
>>   drivers/dpll/dpll_netlink.c                   | 24 +++++
>>   .../net/ethernet/mellanox/mlx5/core/dpll.c    | 94 ++++++++++++-------
>>   include/linux/dpll.h                          |  3 +
>>   include/uapi/linux/dpll.h                     |  1 +
>>   5 files changed, 98 insertions(+), 35 deletions(-)
>> 
>
>Interesting attribute, it's good that hardware can expose this info.
>
>Did you think about building some monitoring/alerts based on it?

The deamon we use internally just exposes this to user mainly for
debugging purposes now. Not sure about another plans with this.


>
>For the series (I'm not sure if it's enough for mlx5, but the
>refactoring looks nice):
>
>Acked-By: Vadim Fedorenko <vadim.fedorenko@linux.dev>