[PATCH net-next v1 0/2] Add Embedded SYNC feature for a dpll's pin

[PATCH net-next v1 0/2] Add Embedded SYNC feature for a dpll's pin

[Arkadiusz Kubalewski]

Introduce and allow DPLL subsystem users to get/set capabilities of
Embedded SYNC on a dpll's pin.

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

Arkadiusz Kubalewski (2):
  dpll: add Embedded SYNC feature for a pin
  ice: add callbacks for Embedded SYNC enablement on dpll pins

 Documentation/driver-api/dpll.rst         |  21 ++
 Documentation/netlink/specs/dpll.yaml     |  41 ++++
 drivers/dpll/dpll_netlink.c               | 127 ++++++++++++
 drivers/dpll/dpll_nl.c                    |   5 +-
 drivers/net/ethernet/intel/ice/ice_dpll.c | 241 +++++++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_dpll.h |   1 +
 include/linux/dpll.h                      |  10 +
 include/uapi/linux/dpll.h                 |  23 +++
 8 files changed, 464 insertions(+), 5 deletions(-)

-- 
2.38.1

[PATCH net-next v1 1/2] dpll: add Embedded SYNC feature for a pin

[Arkadiusz Kubalewski]

Implement and document new pin attributes for providing Embedded SYNC
capabilities to the DPLL subsystem users through a netlink pin-get
do/dump messages. Allow the user to set Embedded SYNC frequency with
pin-set do netlink message.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 Documentation/driver-api/dpll.rst     |  21 +++++
 Documentation/netlink/specs/dpll.yaml |  41 +++++++++
 drivers/dpll/dpll_netlink.c           | 127 ++++++++++++++++++++++++++
 drivers/dpll/dpll_nl.c                |   5 +-
 include/linux/dpll.h                  |  10 ++
 include/uapi/linux/dpll.h             |  23 +++++
 6 files changed, 225 insertions(+), 2 deletions(-)

diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst
index ea8d16600e16..d7d091d268a1 100644
--- a/Documentation/driver-api/dpll.rst
+++ b/Documentation/driver-api/dpll.rst
@@ -214,6 +214,27 @@ offset values are fractional with 3-digit decimal places and shell be
 divided with ``DPLL_PIN_PHASE_OFFSET_DIVIDER`` to get integer part and
 modulo divided to get fractional part.
 
+Embedded SYNC
+=============
+
+Device may provide ability to use Embedded SYNC feature. It allows
+to embed additional SYNC signal into the base frequency of a pin - a one
+special pulse of base frequency signal every time SYNC signal pulse
+happens. The user can configure the frequency of Embedded SYNC.
+The Embedded SYNC capability is always related to a given base frequency
+and HW capabilities. The user is provided a range of embedded sync
+frequencies supported, depending on current base frequency configured for
+the pin.
+
+  ========================================= =================================
+  ``DPLL_A_PIN_E_SYNC_FREQUENCY``           current embedded SYNC frequency
+  ``DPLL_A_PIN_E_SYNC_FREQUENCY_SUPPORTED`` nest available embedded SYNC
+                                            frequency ranges
+    ``DPLL_A_PIN_FREQUENCY_MIN``            attr minimum value of frequency
+    ``DPLL_A_PIN_FREQUENCY_MAX``            attr maximum value of frequency
+  ``DPLL_A_PIN_E_SYNC_PULSE``               pulse type of embedded SYNC
+  ========================================= =================================
+
 Configuration commands group
 ============================
 
diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
index 94132d30e0e0..0aabf6f1fc2f 100644
--- a/Documentation/netlink/specs/dpll.yaml
+++ b/Documentation/netlink/specs/dpll.yaml
@@ -210,6 +210,25 @@ definitions:
       integer part of a measured phase offset value.
       Value of (DPLL_A_PHASE_OFFSET % DPLL_PHASE_OFFSET_DIVIDER) is a
       fractional part of a measured phase offset value.
+  -
+    type: enum
+    name: pin-e-sync-pulse
+    doc: |
+      defines possible pulse length ratio between high and low state when
+      embedded sync signal occurs on base clock signal frequency
+    entries:
+      -
+        name: none
+        doc: embedded sync not enabled
+      -
+        name: 25-75
+        doc: when embedded sync signal occurs 25% of signal's period is in
+          high state, 75% of signal's period is in low state
+      -
+        name: 75-25
+        doc: when embedded sync signal occurs 75% of signal's period is in
+          high state, 25% of signal's period is in low state
+    render-max: true
 
 attribute-sets:
   -
@@ -345,6 +364,24 @@ attribute-sets:
           Value is in PPM (parts per million).
           This may be implemented for example for pin of type
           PIN_TYPE_SYNCE_ETH_PORT.
+      -
+        name: e-sync-frequency
+        type: u64
+        doc: |
+          Embedded Sync frequency. If provided a non-zero value, the pin is
+          configured with an embedded sync signal into its base frequency.
+      -
+        name: e-sync-frequency-supported
+        type: nest
+        nested-attributes: frequency-range
+        doc: |
+          If provided a pin is capable of enabling embedded sync frequency
+          into it's base frequency signal.
+      -
+        name: e-sync-pulse
+        type: u32
+        enum: pin-e-sync-pulse
+        doc: Embedded sync signal ratio.
   -
     name: pin-parent-device
     subset-of: pin
@@ -510,6 +547,9 @@ operations:
             - phase-adjust-max
             - phase-adjust
             - fractional-frequency-offset
+            - e-sync-frequency
+            - e-sync-frequency-supported
+            - e-sync-pulse
 
       dump:
         request:
@@ -536,6 +576,7 @@ operations:
             - parent-device
             - parent-pin
             - phase-adjust
+            - e-sync-frequency
     -
       name: pin-create-ntf
       doc: Notification about pin appearing
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index 98e6ad8528d3..5ae2c0adb98e 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -342,6 +342,50 @@ dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
 	return 0;
 }
 
+static int
+dpll_msg_add_pin_esync(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;
+	enum dpll_pin_e_sync_pulse pulse;
+	struct dpll_pin_frequency range;
+	struct nlattr *nest;
+	u64 esync;
+	int ret;
+
+	if (!ops->e_sync_get)
+		return 0;
+	ret = ops->e_sync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
+			      dpll_priv(dpll), &esync, &range, &pulse, extack);
+	if (ret == -EOPNOTSUPP)
+		return 0;
+	else if (ret)
+		return ret;
+	if (nla_put_64bit(msg, DPLL_A_PIN_E_SYNC_FREQUENCY, sizeof(esync),
+			  &esync, DPLL_A_PIN_PAD))
+		return -EMSGSIZE;
+	if (nla_put_u32(msg, DPLL_A_PIN_E_SYNC_PULSE, pulse))
+		return -EMSGSIZE;
+
+	nest = nla_nest_start(msg, DPLL_A_PIN_E_SYNC_FREQUENCY_SUPPORTED);
+	if (!nest)
+		return -EMSGSIZE;
+	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, sizeof(range.min),
+			  &range.min, DPLL_A_PIN_PAD)) {
+		nla_nest_cancel(msg, nest);
+		return -EMSGSIZE;
+	}
+	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, sizeof(range.max),
+			  &range.max, DPLL_A_PIN_PAD)) {
+		nla_nest_cancel(msg, nest);
+		return -EMSGSIZE;
+	}
+	nla_nest_end(msg, nest);
+
+	return 0;
+}
+
 static bool dpll_pin_is_freq_supported(struct dpll_pin *pin, u32 freq)
 {
 	int fs;
@@ -481,6 +525,9 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
 	if (ret)
 		return ret;
 	ret = dpll_msg_add_ffo(msg, pin, ref, extack);
+	if (ret)
+		return ret;
+	ret = dpll_msg_add_pin_esync(msg, pin, ref, extack);
 	if (ret)
 		return ret;
 	if (xa_empty(&pin->parent_refs))
@@ -738,6 +785,81 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 	return ret;
 }
 
+static int
+dpll_pin_e_sync_set(struct dpll_pin *pin, struct nlattr *a,
+		    struct netlink_ext_ack *extack)
+{
+	u64 esync = nla_get_u64(a), old_esync;
+	struct dpll_pin_ref *ref, *failed;
+	enum dpll_pin_e_sync_pulse pulse;
+	struct dpll_pin_frequency range;
+	const struct dpll_pin_ops *ops;
+	struct dpll_device *dpll;
+	unsigned long i;
+	int ret;
+
+	xa_for_each(&pin->dpll_refs, i, ref) {
+		ops = dpll_pin_ops(ref);
+		if (!ops->e_sync_set ||
+		    !ops->e_sync_get) {
+			NL_SET_ERR_MSG(extack,
+				       "embedded sync feature is not supported by this device");
+			return -EOPNOTSUPP;
+		}
+	}
+	ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
+	ops = dpll_pin_ops(ref);
+	dpll = ref->dpll;
+	ret = ops->e_sync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
+			      dpll_priv(dpll), &old_esync, &range, &pulse, extack);
+	if (ret) {
+		NL_SET_ERR_MSG(extack, "unable to get current embedded sync frequency value");
+		return ret;
+	}
+	if (esync == old_esync)
+		return 0;
+	if (esync > range.max || esync < range.min) {
+		NL_SET_ERR_MSG_ATTR(extack, a,
+				    "requested embedded sync frequency value is not supported by this device");
+		return -EINVAL;
+	}
+
+	xa_for_each(&pin->dpll_refs, i, ref) {
+		void *pin_dpll_priv;
+
+		ops = dpll_pin_ops(ref);
+		dpll = ref->dpll;
+		pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin);
+		ret = ops->e_sync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll),
+				      esync, extack);
+		if (ret) {
+			failed = ref;
+			NL_SET_ERR_MSG_FMT(extack,
+					   "embedded sync frequency set failed for dpll_id:%u",
+					   dpll->id);
+			goto rollback;
+		}
+	}
+	__dpll_pin_change_ntf(pin);
+
+	return 0;
+
+rollback:
+	xa_for_each(&pin->dpll_refs, i, ref) {
+		void *pin_dpll_priv;
+
+		if (ref == failed)
+			break;
+		ops = dpll_pin_ops(ref);
+		dpll = ref->dpll;
+		pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin);
+		if (ops->e_sync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll),
+				    old_esync, extack))
+			NL_SET_ERR_MSG(extack, "set embedded sync frequency rollback failed");
+	}
+	return ret;
+}
+
 static int
 dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32 parent_idx,
 			  enum dpll_pin_state state,
@@ -1039,6 +1161,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info)
 			if (ret)
 				return ret;
 			break;
+		case DPLL_A_PIN_E_SYNC_FREQUENCY:
+			ret = dpll_pin_e_sync_set(pin, a, info->extack);
+			if (ret)
+				return ret;
+			break;
 		}
 	}
 
diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c
index 1e95f5397cfc..ba79a47f3a17 100644
--- a/drivers/dpll/dpll_nl.c
+++ b/drivers/dpll/dpll_nl.c
@@ -62,7 +62,7 @@ static const struct nla_policy dpll_pin_get_dump_nl_policy[DPLL_A_PIN_ID + 1] =
 };
 
 /* DPLL_CMD_PIN_SET - do */
-static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST + 1] = {
+static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_E_SYNC_FREQUENCY + 1] = {
 	[DPLL_A_PIN_ID] = { .type = NLA_U32, },
 	[DPLL_A_PIN_FREQUENCY] = { .type = NLA_U64, },
 	[DPLL_A_PIN_DIRECTION] = NLA_POLICY_RANGE(NLA_U32, 1, 2),
@@ -71,6 +71,7 @@ static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST +
 	[DPLL_A_PIN_PARENT_DEVICE] = NLA_POLICY_NESTED(dpll_pin_parent_device_nl_policy),
 	[DPLL_A_PIN_PARENT_PIN] = NLA_POLICY_NESTED(dpll_pin_parent_pin_nl_policy),
 	[DPLL_A_PIN_PHASE_ADJUST] = { .type = NLA_S32, },
+	[DPLL_A_PIN_E_SYNC_FREQUENCY] = { .type = NLA_U64, },
 };
 
 /* Ops table for dpll */
@@ -138,7 +139,7 @@ static const struct genl_split_ops dpll_nl_ops[] = {
 		.doit		= dpll_nl_pin_set_doit,
 		.post_doit	= dpll_pin_post_doit,
 		.policy		= dpll_pin_set_nl_policy,
-		.maxattr	= DPLL_A_PIN_PHASE_ADJUST,
+		.maxattr	= DPLL_A_PIN_E_SYNC_FREQUENCY,
 		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
 	},
 };
diff --git a/include/linux/dpll.h b/include/linux/dpll.h
index d275736230b3..137ab4bcb60e 100644
--- a/include/linux/dpll.h
+++ b/include/linux/dpll.h
@@ -15,6 +15,7 @@
 
 struct dpll_device;
 struct dpll_pin;
+struct dpll_pin_frequency;
 
 struct dpll_device_ops {
 	int (*mode_get)(const struct dpll_device *dpll, void *dpll_priv,
@@ -83,6 +84,15 @@ struct dpll_pin_ops {
 	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);
+	int (*e_sync_set)(const struct dpll_pin *pin, void *pin_priv,
+			  const struct dpll_device *dpll, void *dpll_priv,
+			  u64 e_sync_freq, struct netlink_ext_ack *extack);
+	int (*e_sync_get)(const struct dpll_pin *pin, void *pin_priv,
+			  const struct dpll_device *dpll, void *dpll_priv,
+			  u64 *e_sync_freq,
+			  struct dpll_pin_frequency *e_sync_range,
+			  enum dpll_pin_e_sync_pulse *pulse,
+			  struct netlink_ext_ack *extack);
 };
 
 struct dpll_pin_frequency {
diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
index 0c13d7f1a1bc..2a80a6fb0d1d 100644
--- a/include/uapi/linux/dpll.h
+++ b/include/uapi/linux/dpll.h
@@ -169,6 +169,26 @@ enum dpll_pin_capabilities {
 
 #define DPLL_PHASE_OFFSET_DIVIDER	1000
 
+/**
+ * enum dpll_pin_e_sync_pulse - defines possible pulse length ratio between
+ *   high and low state when embedded sync signal occurs on base clock signal
+ *   frequency
+ * @DPLL_PIN_E_SYNC_PULSE_NONE: embedded sync not enabled
+ * @DPLL_PIN_E_SYNC_PULSE_25_75: when embedded sync signal occurs 25% of
+ *   signal's period is in high state, 75% of signal's period is in low state
+ * @DPLL_PIN_E_SYNC_PULSE_75_25: when embedded sync signal occurs 75% of
+ *   signal's period is in high state, 25% of signal's period is in low state
+ */
+enum dpll_pin_e_sync_pulse {
+	DPLL_PIN_E_SYNC_PULSE_NONE,
+	DPLL_PIN_E_SYNC_PULSE_25_75,
+	DPLL_PIN_E_SYNC_PULSE_75_25,
+
+	/* private: */
+	__DPLL_PIN_E_SYNC_PULSE_MAX,
+	DPLL_PIN_E_SYNC_PULSE_MAX = (__DPLL_PIN_E_SYNC_PULSE_MAX - 1)
+};
+
 enum dpll_a {
 	DPLL_A_ID = 1,
 	DPLL_A_MODULE_NAME,
@@ -210,6 +230,9 @@ enum dpll_a_pin {
 	DPLL_A_PIN_PHASE_ADJUST,
 	DPLL_A_PIN_PHASE_OFFSET,
 	DPLL_A_PIN_FRACTIONAL_FREQUENCY_OFFSET,
+	DPLL_A_PIN_E_SYNC_FREQUENCY,
+	DPLL_A_PIN_E_SYNC_FREQUENCY_SUPPORTED,
+	DPLL_A_PIN_E_SYNC_PULSE,
 
 	__DPLL_A_PIN_MAX,
 	DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1)
-- 
2.38.1

Re: [PATCH net-next v1 1/2] dpll: add Embedded SYNC feature for a pin

[Jiri Pirko]

Thu, Aug 08, 2024 at 01:20:12PM CEST, arkadiusz.kubalewski@intel.com wrote:
>Implement and document new pin attributes for providing Embedded SYNC
>capabilities to the DPLL subsystem users through a netlink pin-get
>do/dump messages. Allow the user to set Embedded SYNC frequency with
>pin-set do netlink message.
>
>Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
> Documentation/driver-api/dpll.rst     |  21 +++++
> Documentation/netlink/specs/dpll.yaml |  41 +++++++++
> drivers/dpll/dpll_netlink.c           | 127 ++++++++++++++++++++++++++
> drivers/dpll/dpll_nl.c                |   5 +-
> include/linux/dpll.h                  |  10 ++
> include/uapi/linux/dpll.h             |  23 +++++
> 6 files changed, 225 insertions(+), 2 deletions(-)
>
>diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-api/dpll.rst
>index ea8d16600e16..d7d091d268a1 100644
>--- a/Documentation/driver-api/dpll.rst
>+++ b/Documentation/driver-api/dpll.rst
>@@ -214,6 +214,27 @@ offset values are fractional with 3-digit decimal places and shell be
> divided with ``DPLL_PIN_PHASE_OFFSET_DIVIDER`` to get integer part and
> modulo divided to get fractional part.
> 
>+Embedded SYNC
>+=============
>+
>+Device may provide ability to use Embedded SYNC feature. It allows
>+to embed additional SYNC signal into the base frequency of a pin - a one
>+special pulse of base frequency signal every time SYNC signal pulse
>+happens. The user can configure the frequency of Embedded SYNC.
>+The Embedded SYNC capability is always related to a given base frequency
>+and HW capabilities. The user is provided a range of embedded sync
>+frequencies supported, depending on current base frequency configured for
>+the pin.
>+
>+  ========================================= =================================
>+  ``DPLL_A_PIN_E_SYNC_FREQUENCY``           current embedded SYNC frequency
>+  ``DPLL_A_PIN_E_SYNC_FREQUENCY_SUPPORTED`` nest available embedded SYNC
>+                                            frequency ranges
>+    ``DPLL_A_PIN_FREQUENCY_MIN``            attr minimum value of frequency
>+    ``DPLL_A_PIN_FREQUENCY_MAX``            attr maximum value of frequency
>+  ``DPLL_A_PIN_E_SYNC_PULSE``               pulse type of embedded SYNC
>+  ========================================= =================================
>+
> Configuration commands group
> ============================
> 
>diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
>index 94132d30e0e0..0aabf6f1fc2f 100644
>--- a/Documentation/netlink/specs/dpll.yaml
>+++ b/Documentation/netlink/specs/dpll.yaml
>@@ -210,6 +210,25 @@ definitions:
>       integer part of a measured phase offset value.
>       Value of (DPLL_A_PHASE_OFFSET % DPLL_PHASE_OFFSET_DIVIDER) is a
>       fractional part of a measured phase offset value.
>+  -
>+    type: enum
>+    name: pin-e-sync-pulse
>+    doc: |
>+      defines possible pulse length ratio between high and low state when
>+      embedded sync signal occurs on base clock signal frequency
>+    entries:
>+      -
>+        name: none
>+        doc: embedded sync not enabled
>+      -
>+        name: 25-75
>+        doc: when embedded sync signal occurs 25% of signal's period is in
>+          high state, 75% of signal's period is in low state
>+      -
>+        name: 75-25

It is very odd to name enums like this.
Why can't this be:

    name: e-sync-pulse-ratio
    type: u32
    doc: Embedded sync signal ratio. Value of 0 to 100. Defines the high
    state percentage.

?


>+        doc: when embedded sync signal occurs 75% of signal's period is in
>+          high state, 25% of signal's period is in low state
>+    render-max: true
> 
> attribute-sets:
>   -
>@@ -345,6 +364,24 @@ attribute-sets:
>           Value is in PPM (parts per million).
>           This may be implemented for example for pin of type
>           PIN_TYPE_SYNCE_ETH_PORT.
>+      -
>+        name: e-sync-frequency
>+        type: u64
>+        doc: |
>+          Embedded Sync frequency. If provided a non-zero value, the pin is

Why non-zero? Why the attr cannot be omitted instead?


>+          configured with an embedded sync signal into its base frequency.
>+      -
>+        name: e-sync-frequency-supported
>+        type: nest
>+        nested-attributes: frequency-range
>+        doc: |
>+          If provided a pin is capable of enabling embedded sync frequency
>+          into it's base frequency signal.
>+      -
>+        name: e-sync-pulse
>+        type: u32
>+        enum: pin-e-sync-pulse
>+        doc: Embedded sync signal ratio.
>   -
>     name: pin-parent-device
>     subset-of: pin
>@@ -510,6 +547,9 @@ operations:
>             - phase-adjust-max
>             - phase-adjust
>             - fractional-frequency-offset
>+            - e-sync-frequency
>+            - e-sync-frequency-supported
>+            - e-sync-pulse
> 
>       dump:
>         request:
>@@ -536,6 +576,7 @@ operations:
>             - parent-device
>             - parent-pin
>             - phase-adjust
>+            - e-sync-frequency
>     -
>       name: pin-create-ntf
>       doc: Notification about pin appearing
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index 98e6ad8528d3..5ae2c0adb98e 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -342,6 +342,50 @@ dpll_msg_add_pin_freq(struct sk_buff *msg, struct dpll_pin *pin,
> 	return 0;
> }
> 
>+static int
>+dpll_msg_add_pin_esync(struct sk_buff *msg, struct dpll_pin *pin,

This is "esync", attributes are "E_SYNC". Why can't they be named
"ESYNC" too? Same comment to another "e_sync" names (vars, ops, etc).


>+		       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;
>+	enum dpll_pin_e_sync_pulse pulse;
>+	struct dpll_pin_frequency range;
>+	struct nlattr *nest;
>+	u64 esync;
>+	int ret;
>+
>+	if (!ops->e_sync_get)
>+		return 0;
>+	ret = ops->e_sync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>+			      dpll_priv(dpll), &esync, &range, &pulse, extack);
>+	if (ret == -EOPNOTSUPP)
>+		return 0;
>+	else if (ret)
>+		return ret;
>+	if (nla_put_64bit(msg, DPLL_A_PIN_E_SYNC_FREQUENCY, sizeof(esync),
>+			  &esync, DPLL_A_PIN_PAD))
>+		return -EMSGSIZE;
>+	if (nla_put_u32(msg, DPLL_A_PIN_E_SYNC_PULSE, pulse))
>+		return -EMSGSIZE;
>+
>+	nest = nla_nest_start(msg, DPLL_A_PIN_E_SYNC_FREQUENCY_SUPPORTED);
>+	if (!nest)
>+		return -EMSGSIZE;
>+	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, sizeof(range.min),
>+			  &range.min, DPLL_A_PIN_PAD)) {
>+		nla_nest_cancel(msg, nest);
>+		return -EMSGSIZE;
>+	}
>+	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, sizeof(range.max),
>+			  &range.max, DPLL_A_PIN_PAD)) {

Don't you want to have the MIN-MAX here multiple times. I mean, in
theory, can the device support 2 fixed frequencies for example?
Have it at least for UAPI so this is easily extendable.



>+		nla_nest_cancel(msg, nest);
>+		return -EMSGSIZE;
>+	}
>+	nla_nest_end(msg, nest);
>+
>+	return 0;
>+}
>+
> static bool dpll_pin_is_freq_supported(struct dpll_pin *pin, u32 freq)
> {
> 	int fs;
>@@ -481,6 +525,9 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin *pin,
> 	if (ret)
> 		return ret;
> 	ret = dpll_msg_add_ffo(msg, pin, ref, extack);
>+	if (ret)
>+		return ret;
>+	ret = dpll_msg_add_pin_esync(msg, pin, ref, extack);
> 	if (ret)
> 		return ret;
> 	if (xa_empty(&pin->parent_refs))
>@@ -738,6 +785,81 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
> 	return ret;
> }
> 
>+static int
>+dpll_pin_e_sync_set(struct dpll_pin *pin, struct nlattr *a,
>+		    struct netlink_ext_ack *extack)
>+{
>+	u64 esync = nla_get_u64(a), old_esync;

"freq"/"old_freq". That aligns with the existing code.


>+	struct dpll_pin_ref *ref, *failed;
>+	enum dpll_pin_e_sync_pulse pulse;
>+	struct dpll_pin_frequency range;
>+	const struct dpll_pin_ops *ops;
>+	struct dpll_device *dpll;
>+	unsigned long i;
>+	int ret;
>+
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		ops = dpll_pin_ops(ref);
>+		if (!ops->e_sync_set ||

No need for line break.


>+		    !ops->e_sync_get) {
>+			NL_SET_ERR_MSG(extack,
>+				       "embedded sync feature is not supported by this device");
>+			return -EOPNOTSUPP;
>+		}
>+	}
>+	ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
>+	ops = dpll_pin_ops(ref);
>+	dpll = ref->dpll;
>+	ret = ops->e_sync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>+			      dpll_priv(dpll), &old_esync, &range, &pulse, extack);

Line over 80cols? Didn't checkpatch warn you?


>+	if (ret) {
>+		NL_SET_ERR_MSG(extack, "unable to get current embedded sync frequency value");
>+		return ret;
>+	}
>+	if (esync == old_esync)
>+		return 0;
>+	if (esync > range.max || esync < range.min) {
>+		NL_SET_ERR_MSG_ATTR(extack, a,
>+				    "requested embedded sync frequency value is not supported by this device");
>+		return -EINVAL;
>+	}
>+
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		void *pin_dpll_priv;
>+
>+		ops = dpll_pin_ops(ref);
>+		dpll = ref->dpll;
>+		pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin);
>+		ret = ops->e_sync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll),
>+				      esync, extack);
>+		if (ret) {
>+			failed = ref;
>+			NL_SET_ERR_MSG_FMT(extack,
>+					   "embedded sync frequency set failed for dpll_id:%u",
>+					   dpll->id);
>+			goto rollback;
>+		}
>+	}
>+	__dpll_pin_change_ntf(pin);
>+
>+	return 0;
>+
>+rollback:
>+	xa_for_each(&pin->dpll_refs, i, ref) {
>+		void *pin_dpll_priv;
>+
>+		if (ref == failed)
>+			break;
>+		ops = dpll_pin_ops(ref);
>+		dpll = ref->dpll;
>+		pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin);
>+		if (ops->e_sync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll),
>+				    old_esync, extack))
>+			NL_SET_ERR_MSG(extack, "set embedded sync frequency rollback failed");
>+	}
>+	return ret;
>+}
>+
> static int
> dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32 parent_idx,
> 			  enum dpll_pin_state state,
>@@ -1039,6 +1161,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct genl_info *info)
> 			if (ret)
> 				return ret;
> 			break;
>+		case DPLL_A_PIN_E_SYNC_FREQUENCY:
>+			ret = dpll_pin_e_sync_set(pin, a, info->extack);
>+			if (ret)
>+				return ret;
>+			break;
> 		}
> 	}
> 
>diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c
>index 1e95f5397cfc..ba79a47f3a17 100644
>--- a/drivers/dpll/dpll_nl.c
>+++ b/drivers/dpll/dpll_nl.c
>@@ -62,7 +62,7 @@ static const struct nla_policy dpll_pin_get_dump_nl_policy[DPLL_A_PIN_ID + 1] =
> };
> 
> /* DPLL_CMD_PIN_SET - do */
>-static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST + 1] = {
>+static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_E_SYNC_FREQUENCY + 1] = {
> 	[DPLL_A_PIN_ID] = { .type = NLA_U32, },
> 	[DPLL_A_PIN_FREQUENCY] = { .type = NLA_U64, },
> 	[DPLL_A_PIN_DIRECTION] = NLA_POLICY_RANGE(NLA_U32, 1, 2),
>@@ -71,6 +71,7 @@ static const struct nla_policy dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST +
> 	[DPLL_A_PIN_PARENT_DEVICE] = NLA_POLICY_NESTED(dpll_pin_parent_device_nl_policy),
> 	[DPLL_A_PIN_PARENT_PIN] = NLA_POLICY_NESTED(dpll_pin_parent_pin_nl_policy),
> 	[DPLL_A_PIN_PHASE_ADJUST] = { .type = NLA_S32, },
>+	[DPLL_A_PIN_E_SYNC_FREQUENCY] = { .type = NLA_U64, },
> };
> 
> /* Ops table for dpll */
>@@ -138,7 +139,7 @@ static const struct genl_split_ops dpll_nl_ops[] = {
> 		.doit		= dpll_nl_pin_set_doit,
> 		.post_doit	= dpll_pin_post_doit,
> 		.policy		= dpll_pin_set_nl_policy,
>-		.maxattr	= DPLL_A_PIN_PHASE_ADJUST,
>+		.maxattr	= DPLL_A_PIN_E_SYNC_FREQUENCY,
> 		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> 	},
> };
>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>index d275736230b3..137ab4bcb60e 100644
>--- a/include/linux/dpll.h
>+++ b/include/linux/dpll.h
>@@ -15,6 +15,7 @@
> 
> struct dpll_device;
> struct dpll_pin;
>+struct dpll_pin_frequency;
> 
> struct dpll_device_ops {
> 	int (*mode_get)(const struct dpll_device *dpll, void *dpll_priv,
>@@ -83,6 +84,15 @@ struct dpll_pin_ops {
> 	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);
>+	int (*e_sync_set)(const struct dpll_pin *pin, void *pin_priv,
>+			  const struct dpll_device *dpll, void *dpll_priv,
>+			  u64 e_sync_freq, struct netlink_ext_ack *extack);
>+	int (*e_sync_get)(const struct dpll_pin *pin, void *pin_priv,
>+			  const struct dpll_device *dpll, void *dpll_priv,
>+			  u64 *e_sync_freq,
>+			  struct dpll_pin_frequency *e_sync_range,
>+			  enum dpll_pin_e_sync_pulse *pulse,
>+			  struct netlink_ext_ack *extack);
> };
> 
> struct dpll_pin_frequency {
>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>index 0c13d7f1a1bc..2a80a6fb0d1d 100644
>--- a/include/uapi/linux/dpll.h
>+++ b/include/uapi/linux/dpll.h
>@@ -169,6 +169,26 @@ enum dpll_pin_capabilities {
> 
> #define DPLL_PHASE_OFFSET_DIVIDER	1000
> 
>+/**
>+ * enum dpll_pin_e_sync_pulse - defines possible pulse length ratio between
>+ *   high and low state when embedded sync signal occurs on base clock signal
>+ *   frequency
>+ * @DPLL_PIN_E_SYNC_PULSE_NONE: embedded sync not enabled
>+ * @DPLL_PIN_E_SYNC_PULSE_25_75: when embedded sync signal occurs 25% of
>+ *   signal's period is in high state, 75% of signal's period is in low state
>+ * @DPLL_PIN_E_SYNC_PULSE_75_25: when embedded sync signal occurs 75% of
>+ *   signal's period is in high state, 25% of signal's period is in low state
>+ */
>+enum dpll_pin_e_sync_pulse {
>+	DPLL_PIN_E_SYNC_PULSE_NONE,
>+	DPLL_PIN_E_SYNC_PULSE_25_75,
>+	DPLL_PIN_E_SYNC_PULSE_75_25,
>+
>+	/* private: */
>+	__DPLL_PIN_E_SYNC_PULSE_MAX,
>+	DPLL_PIN_E_SYNC_PULSE_MAX = (__DPLL_PIN_E_SYNC_PULSE_MAX - 1)
>+};
>+
> enum dpll_a {
> 	DPLL_A_ID = 1,
> 	DPLL_A_MODULE_NAME,
>@@ -210,6 +230,9 @@ enum dpll_a_pin {
> 	DPLL_A_PIN_PHASE_ADJUST,
> 	DPLL_A_PIN_PHASE_OFFSET,
> 	DPLL_A_PIN_FRACTIONAL_FREQUENCY_OFFSET,
>+	DPLL_A_PIN_E_SYNC_FREQUENCY,
>+	DPLL_A_PIN_E_SYNC_FREQUENCY_SUPPORTED,
>+	DPLL_A_PIN_E_SYNC_PULSE,
> 
> 	__DPLL_A_PIN_MAX,
> 	DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1)
>-- 
>2.38.1
>

RE: [PATCH net-next v1 1/2] dpll: add Embedded SYNC feature for a pin

[Kubalewski, Arkadiusz]

>-----Original Message-----
>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Thursday, August 8, 2024 1:54 PM
>
>Thu, Aug 08, 2024 at 01:20:12PM CEST, arkadiusz.kubalewski@intel.com wrote:
>>Implement and document new pin attributes for providing Embedded SYNC
>>capabilities to the DPLL subsystem users through a netlink pin-get
>>do/dump messages. Allow the user to set Embedded SYNC frequency with
>>pin-set do netlink message.
>>
>>Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>---
>> Documentation/driver-api/dpll.rst     |  21 +++++
>> Documentation/netlink/specs/dpll.yaml |  41 +++++++++
>> drivers/dpll/dpll_netlink.c           | 127 ++++++++++++++++++++++++++
>> drivers/dpll/dpll_nl.c                |   5 +-
>> include/linux/dpll.h                  |  10 ++
>> include/uapi/linux/dpll.h             |  23 +++++
>> 6 files changed, 225 insertions(+), 2 deletions(-)
>>
>>diff --git a/Documentation/driver-api/dpll.rst b/Documentation/driver-
>>api/dpll.rst
>>index ea8d16600e16..d7d091d268a1 100644
>>--- a/Documentation/driver-api/dpll.rst
>>+++ b/Documentation/driver-api/dpll.rst
>>@@ -214,6 +214,27 @@ offset values are fractional with 3-digit decimal places
>>and shell be
>> divided with ``DPLL_PIN_PHASE_OFFSET_DIVIDER`` to get integer part and
>> modulo divided to get fractional part.
>>
>>+Embedded SYNC
>>+=============
>>+
>>+Device may provide ability to use Embedded SYNC feature. It allows
>>+to embed additional SYNC signal into the base frequency of a pin - a one
>>+special pulse of base frequency signal every time SYNC signal pulse
>>+happens. The user can configure the frequency of Embedded SYNC.
>>+The Embedded SYNC capability is always related to a given base frequency
>>+and HW capabilities. The user is provided a range of embedded sync
>>+frequencies supported, depending on current base frequency configured for
>>+the pin.
>>+
>>+  =========================================
>>=================================
>>+  ``DPLL_A_PIN_E_SYNC_FREQUENCY``           current embedded SYNC frequency
>>+  ``DPLL_A_PIN_E_SYNC_FREQUENCY_SUPPORTED`` nest available embedded SYNC
>>+                                            frequency ranges
>>+    ``DPLL_A_PIN_FREQUENCY_MIN``            attr minimum value of frequency
>>+    ``DPLL_A_PIN_FREQUENCY_MAX``            attr maximum value of frequency
>>+  ``DPLL_A_PIN_E_SYNC_PULSE``               pulse type of embedded SYNC
>>+  =========================================
>>=================================
>>+
>> Configuration commands group
>> ============================
>>
>>diff --git a/Documentation/netlink/specs/dpll.yaml
>>b/Documentation/netlink/specs/dpll.yaml
>>index 94132d30e0e0..0aabf6f1fc2f 100644
>>--- a/Documentation/netlink/specs/dpll.yaml
>>+++ b/Documentation/netlink/specs/dpll.yaml
>>@@ -210,6 +210,25 @@ definitions:
>>       integer part of a measured phase offset value.
>>       Value of (DPLL_A_PHASE_OFFSET % DPLL_PHASE_OFFSET_DIVIDER) is a
>>       fractional part of a measured phase offset value.
>>+  -
>>+    type: enum
>>+    name: pin-e-sync-pulse
>>+    doc: |
>>+      defines possible pulse length ratio between high and low state when
>>+      embedded sync signal occurs on base clock signal frequency
>>+    entries:
>>+      -
>>+        name: none
>>+        doc: embedded sync not enabled
>>+      -
>>+        name: 25-75
>>+        doc: when embedded sync signal occurs 25% of signal's period is in
>>+          high state, 75% of signal's period is in low state
>>+      -
>>+        name: 75-25
>
>It is very odd to name enums like this.
>Why can't this be:
>
>    name: e-sync-pulse-ratio
>    type: u32
>    doc: Embedded sync signal ratio. Value of 0 to 100. Defines the high
>    state percentage.
>
>?
>

I don't know if the other's are actually used by some vendors, but sure
sounds good.

>
>>+        doc: when embedded sync signal occurs 75% of signal's period is in
>>+          high state, 25% of signal's period is in low state
>>+    render-max: true
>>
>> attribute-sets:
>>   -
>>@@ -345,6 +364,24 @@ attribute-sets:
>>           Value is in PPM (parts per million).
>>           This may be implemented for example for pin of type
>>           PIN_TYPE_SYNCE_ETH_PORT.
>>+      -
>>+        name: e-sync-frequency
>>+        type: u64
>>+        doc: |
>>+          Embedded Sync frequency. If provided a non-zero value, the pin is
>
>Why non-zero? Why the attr cannot be omitted instead?
>

Zero is also a turn-off value for set, but sure for get it can be omitted for
a netlink return message.

>
>>+          configured with an embedded sync signal into its base frequency.
>>+      -
>>+        name: e-sync-frequency-supported
>>+        type: nest
>>+        nested-attributes: frequency-range
>>+        doc: |
>>+          If provided a pin is capable of enabling embedded sync frequency
>>+          into it's base frequency signal.
>>+      -
>>+        name: e-sync-pulse
>>+        type: u32
>>+        enum: pin-e-sync-pulse
>>+        doc: Embedded sync signal ratio.
>>   -
>>     name: pin-parent-device
>>     subset-of: pin
>>@@ -510,6 +547,9 @@ operations:
>>             - phase-adjust-max
>>             - phase-adjust
>>             - fractional-frequency-offset
>>+            - e-sync-frequency
>>+            - e-sync-frequency-supported
>>+            - e-sync-pulse
>>
>>       dump:
>>         request:
>>@@ -536,6 +576,7 @@ operations:
>>             - parent-device
>>             - parent-pin
>>             - phase-adjust
>>+            - e-sync-frequency
>>     -
>>       name: pin-create-ntf
>>       doc: Notification about pin appearing
>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>index 98e6ad8528d3..5ae2c0adb98e 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -342,6 +342,50 @@ dpll_msg_add_pin_freq(struct sk_buff *msg, struct
>>dpll_pin *pin,
>> 	return 0;
>> }
>>
>>+static int
>>+dpll_msg_add_pin_esync(struct sk_buff *msg, struct dpll_pin *pin,
>
>This is "esync", attributes are "E_SYNC". Why can't they be named
>"ESYNC" too? Same comment to another "e_sync" names (vars, ops, etc).
>

Sure, will change.

>
>>+		       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;
>>+	enum dpll_pin_e_sync_pulse pulse;
>>+	struct dpll_pin_frequency range;
>>+	struct nlattr *nest;
>>+	u64 esync;
>>+	int ret;
>>+
>>+	if (!ops->e_sync_get)
>>+		return 0;
>>+	ret = ops->e_sync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>>+			      dpll_priv(dpll), &esync, &range, &pulse, extack);
>>+	if (ret == -EOPNOTSUPP)
>>+		return 0;
>>+	else if (ret)
>>+		return ret;
>>+	if (nla_put_64bit(msg, DPLL_A_PIN_E_SYNC_FREQUENCY, sizeof(esync),
>>+			  &esync, DPLL_A_PIN_PAD))
>>+		return -EMSGSIZE;
>>+	if (nla_put_u32(msg, DPLL_A_PIN_E_SYNC_PULSE, pulse))
>>+		return -EMSGSIZE;
>>+
>>+	nest = nla_nest_start(msg, DPLL_A_PIN_E_SYNC_FREQUENCY_SUPPORTED);
>>+	if (!nest)
>>+		return -EMSGSIZE;
>>+	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MIN, sizeof(range.min),
>>+			  &range.min, DPLL_A_PIN_PAD)) {
>>+		nla_nest_cancel(msg, nest);
>>+		return -EMSGSIZE;
>>+	}
>>+	if (nla_put_64bit(msg, DPLL_A_PIN_FREQUENCY_MAX, sizeof(range.max),
>>+			  &range.max, DPLL_A_PIN_PAD)) {
>
>Don't you want to have the MIN-MAX here multiple times. I mean, in
>theory, can the device support 2 fixed frequencies for example?
>Have it at least for UAPI so this is easily extendable.
>

Sure, makes sense, will fix.

>
>
>>+		nla_nest_cancel(msg, nest);
>>+		return -EMSGSIZE;
>>+	}
>>+	nla_nest_end(msg, nest);
>>+
>>+	return 0;
>>+}
>>+
>> static bool dpll_pin_is_freq_supported(struct dpll_pin *pin, u32 freq)
>> {
>> 	int fs;
>>@@ -481,6 +525,9 @@ dpll_cmd_pin_get_one(struct sk_buff *msg, struct dpll_pin
>>*pin,
>> 	if (ret)
>> 		return ret;
>> 	ret = dpll_msg_add_ffo(msg, pin, ref, extack);
>>+	if (ret)
>>+		return ret;
>>+	ret = dpll_msg_add_pin_esync(msg, pin, ref, extack);
>> 	if (ret)
>> 		return ret;
>> 	if (xa_empty(&pin->parent_refs))
>>@@ -738,6 +785,81 @@ dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr
>>*a,
>> 	return ret;
>> }
>>
>>+static int
>>+dpll_pin_e_sync_set(struct dpll_pin *pin, struct nlattr *a,
>>+		    struct netlink_ext_ack *extack)
>>+{
>>+	u64 esync = nla_get_u64(a), old_esync;
>
>"freq"/"old_freq". That aligns with the existing code.
>

Ok, will fix.

>
>>+	struct dpll_pin_ref *ref, *failed;
>>+	enum dpll_pin_e_sync_pulse pulse;
>>+	struct dpll_pin_frequency range;
>>+	const struct dpll_pin_ops *ops;
>>+	struct dpll_device *dpll;
>>+	unsigned long i;
>>+	int ret;
>>+
>>+	xa_for_each(&pin->dpll_refs, i, ref) {
>>+		ops = dpll_pin_ops(ref);
>>+		if (!ops->e_sync_set ||
>
>No need for line break.
>

Sure, will fix.

>
>>+		    !ops->e_sync_get) {
>>+			NL_SET_ERR_MSG(extack,
>>+				       "embedded sync feature is not supported by
>>this device");
>>+			return -EOPNOTSUPP;
>>+		}
>>+	}
>>+	ref = dpll_xa_ref_dpll_first(&pin->dpll_refs);
>>+	ops = dpll_pin_ops(ref);
>>+	dpll = ref->dpll;
>>+	ret = ops->e_sync_get(pin, dpll_pin_on_dpll_priv(dpll, pin), dpll,
>>+			      dpll_priv(dpll), &old_esync, &range, &pulse, extack);
>
>Line over 80cols? Didn't checkpatch warn you?
>

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
But sure, I will use 80.

Thank you!
Arkadiusz

>
>>+	if (ret) {
>>+		NL_SET_ERR_MSG(extack, "unable to get current embedded sync
>>frequency value");
>>+		return ret;
>>+	}
>>+	if (esync == old_esync)
>>+		return 0;
>>+	if (esync > range.max || esync < range.min) {
>>+		NL_SET_ERR_MSG_ATTR(extack, a,
>>+				    "requested embedded sync frequency value is not
>>supported by this device");
>>+		return -EINVAL;
>>+	}
>>+
>>+	xa_for_each(&pin->dpll_refs, i, ref) {
>>+		void *pin_dpll_priv;
>>+
>>+		ops = dpll_pin_ops(ref);
>>+		dpll = ref->dpll;
>>+		pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin);
>>+		ret = ops->e_sync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll),
>>+				      esync, extack);
>>+		if (ret) {
>>+			failed = ref;
>>+			NL_SET_ERR_MSG_FMT(extack,
>>+					   "embedded sync frequency set failed for
>>dpll_id:%u",
>>+					   dpll->id);
>>+			goto rollback;
>>+		}
>>+	}
>>+	__dpll_pin_change_ntf(pin);
>>+
>>+	return 0;
>>+
>>+rollback:
>>+	xa_for_each(&pin->dpll_refs, i, ref) {
>>+		void *pin_dpll_priv;
>>+
>>+		if (ref == failed)
>>+			break;
>>+		ops = dpll_pin_ops(ref);
>>+		dpll = ref->dpll;
>>+		pin_dpll_priv = dpll_pin_on_dpll_priv(dpll, pin);
>>+		if (ops->e_sync_set(pin, pin_dpll_priv, dpll, dpll_priv(dpll),
>>+				    old_esync, extack))
>>+			NL_SET_ERR_MSG(extack, "set embedded sync frequency
>>rollback failed");
>>+	}
>>+	return ret;
>>+}
>>+
>> static int
>> dpll_pin_on_pin_state_set(struct dpll_pin *pin, u32 parent_idx,
>> 			  enum dpll_pin_state state,
>>@@ -1039,6 +1161,11 @@ dpll_pin_set_from_nlattr(struct dpll_pin *pin, struct
>>genl_info *info)
>> 			if (ret)
>> 				return ret;
>> 			break;
>>+		case DPLL_A_PIN_E_SYNC_FREQUENCY:
>>+			ret = dpll_pin_e_sync_set(pin, a, info->extack);
>>+			if (ret)
>>+				return ret;
>>+			break;
>> 		}
>> 	}
>>
>>diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c
>>index 1e95f5397cfc..ba79a47f3a17 100644
>>--- a/drivers/dpll/dpll_nl.c
>>+++ b/drivers/dpll/dpll_nl.c
>>@@ -62,7 +62,7 @@ static const struct nla_policy
>>dpll_pin_get_dump_nl_policy[DPLL_A_PIN_ID + 1] =
>> };
>>
>> /* DPLL_CMD_PIN_SET - do */
>>-static const struct nla_policy
>>dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST + 1] = {
>>+static const struct nla_policy
>>dpll_pin_set_nl_policy[DPLL_A_PIN_E_SYNC_FREQUENCY + 1] = {
>> 	[DPLL_A_PIN_ID] = { .type = NLA_U32, },
>> 	[DPLL_A_PIN_FREQUENCY] = { .type = NLA_U64, },
>> 	[DPLL_A_PIN_DIRECTION] = NLA_POLICY_RANGE(NLA_U32, 1, 2),
>>@@ -71,6 +71,7 @@ static const struct nla_policy
>>dpll_pin_set_nl_policy[DPLL_A_PIN_PHASE_ADJUST +
>> 	[DPLL_A_PIN_PARENT_DEVICE] =
>>NLA_POLICY_NESTED(dpll_pin_parent_device_nl_policy),
>> 	[DPLL_A_PIN_PARENT_PIN] =
>>NLA_POLICY_NESTED(dpll_pin_parent_pin_nl_policy),
>> 	[DPLL_A_PIN_PHASE_ADJUST] = { .type = NLA_S32, },
>>+	[DPLL_A_PIN_E_SYNC_FREQUENCY] = { .type = NLA_U64, },
>> };
>>
>> /* Ops table for dpll */
>>@@ -138,7 +139,7 @@ static const struct genl_split_ops dpll_nl_ops[] = {
>> 		.doit		= dpll_nl_pin_set_doit,
>> 		.post_doit	= dpll_pin_post_doit,
>> 		.policy		= dpll_pin_set_nl_policy,
>>-		.maxattr	= DPLL_A_PIN_PHASE_ADJUST,
>>+		.maxattr	= DPLL_A_PIN_E_SYNC_FREQUENCY,
>> 		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>> 	},
>> };
>>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>index d275736230b3..137ab4bcb60e 100644
>>--- a/include/linux/dpll.h
>>+++ b/include/linux/dpll.h
>>@@ -15,6 +15,7 @@
>>
>> struct dpll_device;
>> struct dpll_pin;
>>+struct dpll_pin_frequency;
>>
>> struct dpll_device_ops {
>> 	int (*mode_get)(const struct dpll_device *dpll, void *dpll_priv,
>>@@ -83,6 +84,15 @@ struct dpll_pin_ops {
>> 	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);
>>+	int (*e_sync_set)(const struct dpll_pin *pin, void *pin_priv,
>>+			  const struct dpll_device *dpll, void *dpll_priv,
>>+			  u64 e_sync_freq, struct netlink_ext_ack *extack);
>>+	int (*e_sync_get)(const struct dpll_pin *pin, void *pin_priv,
>>+			  const struct dpll_device *dpll, void *dpll_priv,
>>+			  u64 *e_sync_freq,
>>+			  struct dpll_pin_frequency *e_sync_range,
>>+			  enum dpll_pin_e_sync_pulse *pulse,
>>+			  struct netlink_ext_ack *extack);
>> };
>>
>> struct dpll_pin_frequency {
>>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>>index 0c13d7f1a1bc..2a80a6fb0d1d 100644
>>--- a/include/uapi/linux/dpll.h
>>+++ b/include/uapi/linux/dpll.h
>>@@ -169,6 +169,26 @@ enum dpll_pin_capabilities {
>>
>> #define DPLL_PHASE_OFFSET_DIVIDER	1000
>>
>>+/**
>>+ * enum dpll_pin_e_sync_pulse - defines possible pulse length ratio between
>>+ *   high and low state when embedded sync signal occurs on base clock
>>signal
>>+ *   frequency
>>+ * @DPLL_PIN_E_SYNC_PULSE_NONE: embedded sync not enabled
>>+ * @DPLL_PIN_E_SYNC_PULSE_25_75: when embedded sync signal occurs 25% of
>>+ *   signal's period is in high state, 75% of signal's period is in low
>>state
>>+ * @DPLL_PIN_E_SYNC_PULSE_75_25: when embedded sync signal occurs 75% of
>>+ *   signal's period is in high state, 25% of signal's period is in low
>>state
>>+ */
>>+enum dpll_pin_e_sync_pulse {
>>+	DPLL_PIN_E_SYNC_PULSE_NONE,
>>+	DPLL_PIN_E_SYNC_PULSE_25_75,
>>+	DPLL_PIN_E_SYNC_PULSE_75_25,
>>+
>>+	/* private: */
>>+	__DPLL_PIN_E_SYNC_PULSE_MAX,
>>+	DPLL_PIN_E_SYNC_PULSE_MAX = (__DPLL_PIN_E_SYNC_PULSE_MAX - 1)
>>+};
>>+
>> enum dpll_a {
>> 	DPLL_A_ID = 1,
>> 	DPLL_A_MODULE_NAME,
>>@@ -210,6 +230,9 @@ enum dpll_a_pin {
>> 	DPLL_A_PIN_PHASE_ADJUST,
>> 	DPLL_A_PIN_PHASE_OFFSET,
>> 	DPLL_A_PIN_FRACTIONAL_FREQUENCY_OFFSET,
>>+	DPLL_A_PIN_E_SYNC_FREQUENCY,
>>+	DPLL_A_PIN_E_SYNC_FREQUENCY_SUPPORTED,
>>+	DPLL_A_PIN_E_SYNC_PULSE,
>>
>> 	__DPLL_A_PIN_MAX,
>> 	DPLL_A_PIN_MAX = (__DPLL_A_PIN_MAX - 1)
>>--
>>2.38.1
>>

Re: [PATCH net-next v1 1/2] dpll: add Embedded SYNC feature for a pin

[Jakub Kicinski]

On Thu,  8 Aug 2024 13:20:12 +0200 Arkadiusz Kubalewski wrote:
> +Device may provide ability to use Embedded SYNC feature. It allows
> +to embed additional SYNC signal into the base frequency of a pin - a one
> +special pulse of base frequency signal every time SYNC signal pulse
> +happens. The user can configure the frequency of Embedded SYNC.
> +The Embedded SYNC capability is always related to a given base frequency
> +and HW capabilities. The user is provided a range of embedded sync
> +frequencies supported, depending on current base frequency configured for
> +the pin.

Interesting, noob question perhaps, is the signal somehow well
known or the implementation is vendor specific so both ends have
to be from the same vendor? May be worth calling that out, either way.

RE: [PATCH net-next v1 1/2] dpll: add Embedded SYNC feature for a pin

[Kubalewski, Arkadiusz]

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Saturday, August 10, 2024 6:16 AM
>
>On Thu,  8 Aug 2024 13:20:12 +0200 Arkadiusz Kubalewski wrote:
>> +Device may provide ability to use Embedded SYNC feature. It allows
>> +to embed additional SYNC signal into the base frequency of a pin - a one
>> +special pulse of base frequency signal every time SYNC signal pulse
>> +happens. The user can configure the frequency of Embedded SYNC.
>> +The Embedded SYNC capability is always related to a given base frequency
>> +and HW capabilities. The user is provided a range of embedded sync
>> +frequencies supported, depending on current base frequency configured for
>> +the pin.
>
>Interesting, noob question perhaps, is the signal somehow well
>known or the implementation is vendor specific so both ends have
>to be from the same vendor? May be worth calling that out, either way.

Unfortunately, I don't have good answer for your question. I went over docs
from 4 vendors of Network Synchronizer Integrated Circuits and only one
supported it.

I also haven't heard anything about standardized way for this, but I believe it
is hard to call it vendor specific solution - IMHO there is nothing fancy that
would make it hard for different vendors to implement/use. The "special"
pulse-ratio and embedded sync frequency seems to be all the info needed to
configure both ends.

Thank you!
Arkadiusz

[PATCH net-next v1 2/2] ice: add callbacks for Embedded SYNC enablement on dpll pins

[Arkadiusz Kubalewski]

Allow the user to get and set configuration of Embedded SYNC feature
on the ice driver dpll pins.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_dpll.c | 241 +++++++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_dpll.h |   1 +
 2 files changed, 239 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index e92be6f130a3..0664bbe98769 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -394,8 +394,8 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct ice_dpll_pin *pin,
 
 	switch (pin_type) {
 	case ICE_DPLL_PIN_TYPE_INPUT:
-		ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, NULL, NULL,
-					       NULL, &pin->flags[0],
+		ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, &pin->status,
+					       NULL, NULL, &pin->flags[0],
 					       &pin->freq, &pin->phase_adjust);
 		if (ret)
 			goto err;
@@ -430,7 +430,7 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct ice_dpll_pin *pin,
 			goto err;
 
 		parent &= ICE_AQC_GET_CGU_OUT_CFG_DPLL_SRC_SEL;
-		if (ICE_AQC_SET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) {
+		if (ICE_AQC_GET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) {
 			pin->state[pf->dplls.eec.dpll_idx] =
 				parent == pf->dplls.eec.dpll_idx ?
 				DPLL_PIN_STATE_CONNECTED :
@@ -1098,6 +1098,237 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
 	return 0;
 }
 
+/**
+ * ice_dpll_output_e_sync_set - callback for setting embedded sync
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @e_sync_freq: requested embedded sync frequency
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Handler for setting embedded sync frequency value
+ * on output pin.
+ *
+ * Context: Acquires pf->dplls.lock
+ * Return:
+ * * 0 - success
+ * * negative - error
+ */
+static int
+ice_dpll_output_e_sync_set(const struct dpll_pin *pin, void *pin_priv,
+			   const struct dpll_device *dpll, void *dpll_priv,
+			   u64 e_sync_freq, struct netlink_ext_ack *extack)
+{
+	struct ice_dpll_pin *p = pin_priv;
+	struct ice_dpll *d = dpll_priv;
+	struct ice_pf *pf = d->pf;
+	u8 flags = 0;
+	int ret;
+
+	if (ice_dpll_is_reset(pf, extack))
+		return -EBUSY;
+	mutex_lock(&pf->dplls.lock);
+	if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_OUT_EN)
+		flags = ICE_AQC_SET_CGU_OUT_CFG_OUT_EN;
+	if (e_sync_freq == DPLL_PIN_FREQUENCY_1_HZ) {
+		if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) {
+			ret = 0;
+		} else {
+			flags |= ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN;
+			ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags,
+							0, 0, 0);
+		}
+	} else {
+		if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN)) {
+			ret = 0;
+		} else {
+			flags &= ~ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN;
+			ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags,
+							0, 0, 0);
+		}
+	}
+	mutex_unlock(&pf->dplls.lock);
+	if (ret)
+		NL_SET_ERR_MSG_FMT(extack,
+				   "err:%d %s failed to set e-sync freq\n",
+				   ret,
+				   ice_aq_str(pf->hw.adminq.sq_last_status));
+	return ret;
+}
+
+/**
+ * ice_dpll_output_e_sync_get - callback for getting embedded sync config
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @e_sync_freq: on success holds embedded sync frequency of a pin
+ * @e_sync_range: on success holds embedded sync frequency range for a pin
+ * @pulse: on success holds embedded sync pulse type
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Handler for getting embedded sync frequency value
+ * and capabilities on output pin.
+ *
+ * Context: Acquires pf->dplls.lock
+ * Return:
+ * * 0 - success
+ * * negative - error
+ */
+static int
+ice_dpll_output_e_sync_get(const struct dpll_pin *pin, void *pin_priv,
+			   const struct dpll_device *dpll, void *dpll_priv,
+			   u64 *e_sync_freq,
+			   struct dpll_pin_frequency *e_sync_range,
+			   enum dpll_pin_e_sync_pulse *pulse,
+			   struct netlink_ext_ack *extack)
+{
+	struct ice_dpll_pin *p = pin_priv;
+	struct ice_dpll *d = dpll_priv;
+	struct ice_pf *pf = d->pf;
+
+	if (ice_dpll_is_reset(pf, extack))
+		return -EBUSY;
+	mutex_lock(&pf->dplls.lock);
+	if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_ABILITY)) {
+		mutex_unlock(&pf->dplls.lock);
+		return -EOPNOTSUPP;
+	}
+	*pulse = DPLL_PIN_E_SYNC_PULSE_NONE;
+	e_sync_range->min = 0;
+	if (p->freq == DPLL_PIN_FREQUENCY_10_MHZ) {
+		e_sync_range->max = DPLL_PIN_FREQUENCY_1_HZ;
+		if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) {
+			*e_sync_freq = DPLL_PIN_FREQUENCY_1_HZ;
+			*pulse = DPLL_PIN_E_SYNC_PULSE_25_75;
+		} else {
+			*e_sync_freq = 0;
+		}
+	} else {
+		e_sync_range->max = 0;
+		*e_sync_freq = 0;
+	}
+	mutex_unlock(&pf->dplls.lock);
+	return 0;
+}
+
+/**
+ * ice_dpll_input_e_sync_set - callback for setting embedded sync
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @e_sync_freq: requested embedded sync frequency
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Handler for setting embedded sync frequency value
+ * on input pin.
+ *
+ * Context: Acquires pf->dplls.lock
+ * Return:
+ * * 0 - success
+ * * negative - error
+ */
+static int
+ice_dpll_input_e_sync_set(const struct dpll_pin *pin, void *pin_priv,
+			  const struct dpll_device *dpll, void *dpll_priv,
+			  u64 e_sync_freq, struct netlink_ext_ack *extack)
+{
+	struct ice_dpll_pin *p = pin_priv;
+	struct ice_dpll *d = dpll_priv;
+	struct ice_pf *pf = d->pf;
+	u8 flags_en = 0;
+	int ret;
+
+	if (ice_dpll_is_reset(pf, extack))
+		return -EBUSY;
+	mutex_lock(&pf->dplls.lock);
+	if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_INPUT_EN)
+		flags_en = ICE_AQC_SET_CGU_IN_CFG_FLG2_INPUT_EN;
+	if (e_sync_freq == DPLL_PIN_FREQUENCY_1_HZ) {
+		if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) {
+			ret = 0;
+		} else {
+			flags_en |= ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN;
+			ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0,
+						       flags_en, 0, 0);
+		}
+	} else {
+		if (!(p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN)) {
+			ret = 0;
+		} else {
+			flags_en &= ~ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN;
+			ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0,
+						       flags_en, 0, 0);
+		}
+	}
+	mutex_unlock(&pf->dplls.lock);
+	if (ret)
+		NL_SET_ERR_MSG_FMT(extack,
+				   "err:%d %s failed to set e-sync freq\n",
+				   ret,
+				   ice_aq_str(pf->hw.adminq.sq_last_status));
+
+	return ret;
+}
+
+/**
+ * ice_dpll_input_e_sync_get - callback for getting embedded sync config
+ * @pin: pointer to a pin
+ * @pin_priv: private data pointer passed on pin registration
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @e_sync_freq: on success holds embedded sync frequency of a pin
+ * @e_sync_range: on success holds embedded sync frequency range for a pin
+ * @pulse: on success holds embedded sync pulse type
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Handler for getting embedded sync frequency value
+ * and capabilities on input pin.
+ *
+ * Context: Acquires pf->dplls.lock
+ * Return:
+ * * 0 - success
+ * * negative - error
+ */
+static int
+ice_dpll_input_e_sync_get(const struct dpll_pin *pin, void *pin_priv,
+			  const struct dpll_device *dpll, void *dpll_priv,
+			  u64 *e_sync_freq,
+			  struct dpll_pin_frequency *e_sync_range,
+			  enum dpll_pin_e_sync_pulse *pulse,
+			  struct netlink_ext_ack *extack)
+{
+	struct ice_dpll_pin *p = pin_priv;
+	struct ice_dpll *d = dpll_priv;
+	struct ice_pf *pf = d->pf;
+
+	if (ice_dpll_is_reset(pf, extack))
+		return -EBUSY;
+	mutex_lock(&pf->dplls.lock);
+	if (!(p->status & ICE_AQC_GET_CGU_IN_CFG_STATUS_ESYNC_CAP)) {
+		mutex_unlock(&pf->dplls.lock);
+		return -EOPNOTSUPP;
+	}
+	*pulse = DPLL_PIN_E_SYNC_PULSE_NONE;
+	e_sync_range->min = 0;
+	if (p->freq == DPLL_PIN_FREQUENCY_10_MHZ) {
+		e_sync_range->max = DPLL_PIN_FREQUENCY_1_HZ;
+		if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) {
+			*e_sync_freq = DPLL_PIN_FREQUENCY_1_HZ;
+			*pulse = DPLL_PIN_E_SYNC_PULSE_25_75;
+		} else {
+			*e_sync_freq = 0;
+		}
+	} else {
+		e_sync_range->max = 0;
+		*e_sync_freq = 0;
+	}
+	mutex_unlock(&pf->dplls.lock);
+	return 0;
+}
+
 /**
  * ice_dpll_rclk_state_on_pin_set - set a state on rclk pin
  * @pin: pointer to a pin
@@ -1222,6 +1453,8 @@ static const struct dpll_pin_ops ice_dpll_input_ops = {
 	.phase_adjust_get = ice_dpll_pin_phase_adjust_get,
 	.phase_adjust_set = ice_dpll_input_phase_adjust_set,
 	.phase_offset_get = ice_dpll_phase_offset_get,
+	.e_sync_set = ice_dpll_input_e_sync_set,
+	.e_sync_get = ice_dpll_input_e_sync_get,
 };
 
 static const struct dpll_pin_ops ice_dpll_output_ops = {
@@ -1232,6 +1465,8 @@ static const struct dpll_pin_ops ice_dpll_output_ops = {
 	.direction_get = ice_dpll_output_direction,
 	.phase_adjust_get = ice_dpll_pin_phase_adjust_get,
 	.phase_adjust_set = ice_dpll_output_phase_adjust_set,
+	.e_sync_set = ice_dpll_output_e_sync_set,
+	.e_sync_get = ice_dpll_output_e_sync_get,
 };
 
 static const struct dpll_device_ops ice_dpll_ops = {
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
index 93172e93995b..c320f1bf7d6d 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
@@ -31,6 +31,7 @@ struct ice_dpll_pin {
 	struct dpll_pin_properties prop;
 	u32 freq;
 	s32 phase_adjust;
+	u8 status;
 };
 
 /** ice_dpll - store info required for DPLL control
-- 
2.38.1

Re: [PATCH net-next v1 2/2] ice: add callbacks for Embedded SYNC enablement on dpll pins

[Jiri Pirko]

Thu, Aug 08, 2024 at 01:20:13PM CEST, arkadiusz.kubalewski@intel.com wrote:
>Allow the user to get and set configuration of Embedded SYNC feature
>on the ice driver dpll pins.
>
>Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
> drivers/net/ethernet/intel/ice/ice_dpll.c | 241 +++++++++++++++++++++-
> drivers/net/ethernet/intel/ice/ice_dpll.h |   1 +
> 2 files changed, 239 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
>index e92be6f130a3..0664bbe98769 100644
>--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
>@@ -394,8 +394,8 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct ice_dpll_pin *pin,
> 
> 	switch (pin_type) {
> 	case ICE_DPLL_PIN_TYPE_INPUT:
>-		ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, NULL, NULL,
>-					       NULL, &pin->flags[0],
>+		ret = ice_aq_get_input_pin_cfg(&pf->hw, pin->idx, &pin->status,
>+					       NULL, NULL, &pin->flags[0],
> 					       &pin->freq, &pin->phase_adjust);
> 		if (ret)
> 			goto err;
>@@ -430,7 +430,7 @@ ice_dpll_pin_state_update(struct ice_pf *pf, struct ice_dpll_pin *pin,
> 			goto err;
> 
> 		parent &= ICE_AQC_GET_CGU_OUT_CFG_DPLL_SRC_SEL;
>-		if (ICE_AQC_SET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) {
>+		if (ICE_AQC_GET_CGU_OUT_CFG_OUT_EN & pin->flags[0]) {
> 			pin->state[pf->dplls.eec.dpll_idx] =
> 				parent == pf->dplls.eec.dpll_idx ?
> 				DPLL_PIN_STATE_CONNECTED :
>@@ -1098,6 +1098,237 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
> 	return 0;
> }
> 
>+/**
>+ * ice_dpll_output_e_sync_set - callback for setting embedded sync
>+ * @pin: pointer to a pin
>+ * @pin_priv: private data pointer passed on pin registration
>+ * @dpll: registered dpll pointer
>+ * @dpll_priv: private data pointer passed on dpll registration
>+ * @e_sync_freq: requested embedded sync frequency
>+ * @extack: error reporting
>+ *
>+ * Dpll subsystem callback. Handler for setting embedded sync frequency value
>+ * on output pin.
>+ *
>+ * Context: Acquires pf->dplls.lock
>+ * Return:
>+ * * 0 - success
>+ * * negative - error
>+ */
>+static int
>+ice_dpll_output_e_sync_set(const struct dpll_pin *pin, void *pin_priv,
>+			   const struct dpll_device *dpll, void *dpll_priv,
>+			   u64 e_sync_freq, struct netlink_ext_ack *extack)
>+{
>+	struct ice_dpll_pin *p = pin_priv;
>+	struct ice_dpll *d = dpll_priv;
>+	struct ice_pf *pf = d->pf;
>+	u8 flags = 0;
>+	int ret;
>+
>+	if (ice_dpll_is_reset(pf, extack))
>+		return -EBUSY;
>+	mutex_lock(&pf->dplls.lock);
>+	if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_OUT_EN)
>+		flags = ICE_AQC_SET_CGU_OUT_CFG_OUT_EN;
>+	if (e_sync_freq == DPLL_PIN_FREQUENCY_1_HZ) {
>+		if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) {
>+			ret = 0;
>+		} else {
>+			flags |= ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN;
>+			ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags,
>+							0, 0, 0);
>+		}
>+	} else {
>+		if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN)) {
>+			ret = 0;
>+		} else {
>+			flags &= ~ICE_AQC_SET_CGU_OUT_CFG_ESYNC_EN;
>+			ret = ice_aq_set_output_pin_cfg(&pf->hw, p->idx, flags,
>+							0, 0, 0);
>+		}
>+	}
>+	mutex_unlock(&pf->dplls.lock);
>+	if (ret)
>+		NL_SET_ERR_MSG_FMT(extack,
>+				   "err:%d %s failed to set e-sync freq\n",

Odd. Ret is pass all the way up to the userspace. Putting it to message
does not make any sense to me.


>+				   ret,
>+				   ice_aq_str(pf->hw.adminq.sq_last_status));
>+	return ret;
>+}
>+
>+/**
>+ * ice_dpll_output_e_sync_get - callback for getting embedded sync config
>+ * @pin: pointer to a pin
>+ * @pin_priv: private data pointer passed on pin registration
>+ * @dpll: registered dpll pointer
>+ * @dpll_priv: private data pointer passed on dpll registration
>+ * @e_sync_freq: on success holds embedded sync frequency of a pin
>+ * @e_sync_range: on success holds embedded sync frequency range for a pin
>+ * @pulse: on success holds embedded sync pulse type
>+ * @extack: error reporting
>+ *
>+ * Dpll subsystem callback. Handler for getting embedded sync frequency value
>+ * and capabilities on output pin.
>+ *
>+ * Context: Acquires pf->dplls.lock
>+ * Return:
>+ * * 0 - success
>+ * * negative - error
>+ */
>+static int
>+ice_dpll_output_e_sync_get(const struct dpll_pin *pin, void *pin_priv,
>+			   const struct dpll_device *dpll, void *dpll_priv,
>+			   u64 *e_sync_freq,
>+			   struct dpll_pin_frequency *e_sync_range,
>+			   enum dpll_pin_e_sync_pulse *pulse,
>+			   struct netlink_ext_ack *extack)
>+{
>+	struct ice_dpll_pin *p = pin_priv;
>+	struct ice_dpll *d = dpll_priv;
>+	struct ice_pf *pf = d->pf;
>+
>+	if (ice_dpll_is_reset(pf, extack))
>+		return -EBUSY;
>+	mutex_lock(&pf->dplls.lock);
>+	if (!(p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_ABILITY)) {
>+		mutex_unlock(&pf->dplls.lock);
>+		return -EOPNOTSUPP;
>+	}
>+	*pulse = DPLL_PIN_E_SYNC_PULSE_NONE;
>+	e_sync_range->min = 0;
>+	if (p->freq == DPLL_PIN_FREQUENCY_10_MHZ) {
>+		e_sync_range->max = DPLL_PIN_FREQUENCY_1_HZ;
>+		if (p->flags[0] & ICE_AQC_GET_CGU_OUT_CFG_ESYNC_EN) {
>+			*e_sync_freq = DPLL_PIN_FREQUENCY_1_HZ;
>+			*pulse = DPLL_PIN_E_SYNC_PULSE_25_75;
>+		} else {
>+			*e_sync_freq = 0;
>+		}
>+	} else {
>+		e_sync_range->max = 0;
>+		*e_sync_freq = 0;
>+	}
>+	mutex_unlock(&pf->dplls.lock);
>+	return 0;
>+}
>+
>+/**
>+ * ice_dpll_input_e_sync_set - callback for setting embedded sync
>+ * @pin: pointer to a pin
>+ * @pin_priv: private data pointer passed on pin registration
>+ * @dpll: registered dpll pointer
>+ * @dpll_priv: private data pointer passed on dpll registration
>+ * @e_sync_freq: requested embedded sync frequency
>+ * @extack: error reporting
>+ *
>+ * Dpll subsystem callback. Handler for setting embedded sync frequency value
>+ * on input pin.
>+ *
>+ * Context: Acquires pf->dplls.lock
>+ * Return:
>+ * * 0 - success
>+ * * negative - error
>+ */
>+static int
>+ice_dpll_input_e_sync_set(const struct dpll_pin *pin, void *pin_priv,
>+			  const struct dpll_device *dpll, void *dpll_priv,
>+			  u64 e_sync_freq, struct netlink_ext_ack *extack)
>+{
>+	struct ice_dpll_pin *p = pin_priv;
>+	struct ice_dpll *d = dpll_priv;
>+	struct ice_pf *pf = d->pf;
>+	u8 flags_en = 0;
>+	int ret;
>+
>+	if (ice_dpll_is_reset(pf, extack))
>+		return -EBUSY;
>+	mutex_lock(&pf->dplls.lock);
>+	if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_INPUT_EN)
>+		flags_en = ICE_AQC_SET_CGU_IN_CFG_FLG2_INPUT_EN;
>+	if (e_sync_freq == DPLL_PIN_FREQUENCY_1_HZ) {
>+		if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) {
>+			ret = 0;
>+		} else {
>+			flags_en |= ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN;
>+			ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0,
>+						       flags_en, 0, 0);
>+		}
>+	} else {
>+		if (!(p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN)) {
>+			ret = 0;
>+		} else {
>+			flags_en &= ~ICE_AQC_SET_CGU_IN_CFG_FLG2_ESYNC_EN;
>+			ret = ice_aq_set_input_pin_cfg(&pf->hw, p->idx, 0,
>+						       flags_en, 0, 0);
>+		}
>+	}
>+	mutex_unlock(&pf->dplls.lock);
>+	if (ret)
>+		NL_SET_ERR_MSG_FMT(extack,
>+				   "err:%d %s failed to set e-sync freq\n",

Same here.


>+				   ret,
>+				   ice_aq_str(pf->hw.adminq.sq_last_status));
>+
>+	return ret;
>+}
>+
>+/**
>+ * ice_dpll_input_e_sync_get - callback for getting embedded sync config
>+ * @pin: pointer to a pin
>+ * @pin_priv: private data pointer passed on pin registration
>+ * @dpll: registered dpll pointer
>+ * @dpll_priv: private data pointer passed on dpll registration
>+ * @e_sync_freq: on success holds embedded sync frequency of a pin
>+ * @e_sync_range: on success holds embedded sync frequency range for a pin
>+ * @pulse: on success holds embedded sync pulse type
>+ * @extack: error reporting
>+ *
>+ * Dpll subsystem callback. Handler for getting embedded sync frequency value
>+ * and capabilities on input pin.
>+ *
>+ * Context: Acquires pf->dplls.lock
>+ * Return:
>+ * * 0 - success
>+ * * negative - error
>+ */
>+static int
>+ice_dpll_input_e_sync_get(const struct dpll_pin *pin, void *pin_priv,
>+			  const struct dpll_device *dpll, void *dpll_priv,
>+			  u64 *e_sync_freq,
>+			  struct dpll_pin_frequency *e_sync_range,
>+			  enum dpll_pin_e_sync_pulse *pulse,
>+			  struct netlink_ext_ack *extack)
>+{
>+	struct ice_dpll_pin *p = pin_priv;
>+	struct ice_dpll *d = dpll_priv;
>+	struct ice_pf *pf = d->pf;
>+
>+	if (ice_dpll_is_reset(pf, extack))
>+		return -EBUSY;
>+	mutex_lock(&pf->dplls.lock);
>+	if (!(p->status & ICE_AQC_GET_CGU_IN_CFG_STATUS_ESYNC_CAP)) {
>+		mutex_unlock(&pf->dplls.lock);
>+		return -EOPNOTSUPP;
>+	}
>+	*pulse = DPLL_PIN_E_SYNC_PULSE_NONE;
>+	e_sync_range->min = 0;
>+	if (p->freq == DPLL_PIN_FREQUENCY_10_MHZ) {
>+		e_sync_range->max = DPLL_PIN_FREQUENCY_1_HZ;
>+		if (p->flags[0] & ICE_AQC_GET_CGU_IN_CFG_FLG2_ESYNC_EN) {
>+			*e_sync_freq = DPLL_PIN_FREQUENCY_1_HZ;
>+			*pulse = DPLL_PIN_E_SYNC_PULSE_25_75;
>+		} else {
>+			*e_sync_freq = 0;
>+		}
>+	} else {
>+		e_sync_range->max = 0;
>+		*e_sync_freq = 0;
>+	}
>+	mutex_unlock(&pf->dplls.lock);
>+	return 0;
>+}
>+
> /**
>  * ice_dpll_rclk_state_on_pin_set - set a state on rclk pin
>  * @pin: pointer to a pin
>@@ -1222,6 +1453,8 @@ static const struct dpll_pin_ops ice_dpll_input_ops = {
> 	.phase_adjust_get = ice_dpll_pin_phase_adjust_get,
> 	.phase_adjust_set = ice_dpll_input_phase_adjust_set,
> 	.phase_offset_get = ice_dpll_phase_offset_get,
>+	.e_sync_set = ice_dpll_input_e_sync_set,
>+	.e_sync_get = ice_dpll_input_e_sync_get,
> };
> 
> static const struct dpll_pin_ops ice_dpll_output_ops = {
>@@ -1232,6 +1465,8 @@ static const struct dpll_pin_ops ice_dpll_output_ops = {
> 	.direction_get = ice_dpll_output_direction,
> 	.phase_adjust_get = ice_dpll_pin_phase_adjust_get,
> 	.phase_adjust_set = ice_dpll_output_phase_adjust_set,
>+	.e_sync_set = ice_dpll_output_e_sync_set,
>+	.e_sync_get = ice_dpll_output_e_sync_get,
> };
> 
> static const struct dpll_device_ops ice_dpll_ops = {
>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
>index 93172e93995b..c320f1bf7d6d 100644
>--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
>@@ -31,6 +31,7 @@ struct ice_dpll_pin {
> 	struct dpll_pin_properties prop;
> 	u32 freq;
> 	s32 phase_adjust;
>+	u8 status;
> };
> 
> /** ice_dpll - store info required for DPLL control
>-- 
>2.38.1
>