[PATCH net-next v2 0/4] dpll: add all inputs phase offset monitor

[PATCH net-next v2 0/4] dpll: add all inputs phase offset monitor

[Arkadiusz Kubalewski]

Add simple dpll device level feature and capabilties infrastructure over
netlink dpll interface.
Using new infrastructure add new feature: ALL_INPUTS_PHASE_OFFSET_MONITOR.
Allow users control with two new attributes:
- DPLL_A_CAPABILITIES - for checking if dpll device is capable,
- DPLL_A_FEATURES - for enable/disable a features.
Implement feature in ice driver for dpll-enabled devices.

Verify capability:
$ ./tools/net/ynl/pyynl/cli.py \
 --spec Documentation/netlink/specs/dpll.yaml \
 --dump device-get
[{'capabilities': set(),
  'clock-id': 4658613174691613800,
  'features': set(),
  'id': 0,
  'lock-status': 'locked-ho-acq',
  'mode': 'automatic',
  'mode-supported': ['automatic'],
  'module-name': 'ice',
  'type': 'eec'},
 {'capabilities': {'all-inputs-phase-offset-monitor'},
  'clock-id': 4658613174691613800,
  'features': set(),
  'id': 1,
  'lock-status': 'locked-ho-acq',
  'mode': 'automatic',
  'mode-supported': ['automatic'],
  'module-name': 'ice',
  'type': 'pps'}]

Enable the feature:
$ ./tools/net/ynl/pyynl/cli.py \
 --spec Documentation/netlink/specs/dpll.yaml \
 --do device-set --json '{"id":1, \
 "features":"all-inputs-phase-offset-monitor"}'

Verify feature is enabled:
$ ./tools/net/ynl/pyynl/cli.py \
 --spec Documentation/netlink/specs/dpll.yaml \
 --dump device-get
[
 [...]
 {'capabilities': {'all-inputs-phase-offset-monitor'},
  'clock-id': 4658613174691613800,
  'features': {'all-inputs-phase-offset-monitor'},
  'id': 1,
 [...]
]

Disable the feature:
$ ./tools/net/ynl/pyynl/cli.py \
 --spec Documentation/netlink/specs/dpll.yaml \
 --do device-set --json '{"id":1, \
 "features":0}'


Arkadiusz Kubalewski (4):
  dpll: use struct dpll_device_info for dpll registration
  dpll: add features and capabilities to dpll device spec
  dpll: features_get/set callbacks
  ice: add phase offset monitor for all PPS dpll inputs

 Documentation/netlink/specs/dpll.yaml         |  25 +++
 drivers/dpll/dpll_core.c                      |  34 +--
 drivers/dpll/dpll_core.h                      |   2 +-
 drivers/dpll/dpll_netlink.c                   |  86 +++++++-
 drivers/dpll/dpll_nl.c                        |   5 +-
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  20 ++
 drivers/net/ethernet/intel/ice/ice_common.c   |  26 +++
 drivers/net/ethernet/intel/ice/ice_common.h   |   3 +
 drivers/net/ethernet/intel/ice/ice_dpll.c     | 195 +++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_dpll.h     |   7 +
 drivers/net/ethernet/intel/ice/ice_main.c     |   4 +
 .../net/ethernet/mellanox/mlx5/core/dpll.c    |  10 +-
 drivers/ptp/ptp_ocp.c                         |   7 +-
 include/linux/dpll.h                          |  16 +-
 include/uapi/linux/dpll.h                     |  13 ++
 15 files changed, 417 insertions(+), 36 deletions(-)


base-commit: bbfc077d457272bcea4f14b3a28247ade99b196d
-- 
2.38.1

[PATCH net-next v2 1/4] dpll: use struct dpll_device_info for dpll registration

[Arkadiusz Kubalewski]

Instead of passing list of properties as arguments to
dpll_device_register(..) use a dedicated struct.

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
v2:
- new commit
---
 drivers/dpll/dpll_core.c                      | 34 ++++++++++++-------
 drivers/dpll/dpll_core.h                      |  2 +-
 drivers/dpll/dpll_netlink.c                   |  7 ++--
 drivers/net/ethernet/intel/ice/ice_dpll.c     | 16 +++++----
 drivers/net/ethernet/intel/ice/ice_dpll.h     |  1 +
 .../net/ethernet/mellanox/mlx5/core/dpll.c    | 10 +++---
 drivers/ptp/ptp_ocp.c                         |  7 ++--
 include/linux/dpll.h                          | 11 ++++--
 8 files changed, 57 insertions(+), 31 deletions(-)

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index 20bdc52f63a5..af9cda45a89c 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -34,7 +34,7 @@ static u32 dpll_pin_xa_id;
 
 struct dpll_device_registration {
 	struct list_head list;
-	const struct dpll_device_ops *ops;
+	const struct dpll_device_info *info;
 	void *priv;
 };
 
@@ -327,12 +327,12 @@ EXPORT_SYMBOL_GPL(dpll_device_put);
 
 static struct dpll_device_registration *
 dpll_device_registration_find(struct dpll_device *dpll,
-			      const struct dpll_device_ops *ops, void *priv)
+			      const struct dpll_device_info *info, void *priv)
 {
 	struct dpll_device_registration *reg;
 
 	list_for_each_entry(reg, &dpll->registration_list, list) {
-		if (reg->ops == ops && reg->priv == priv)
+		if (reg->info == info && reg->priv == priv)
 			return reg;
 	}
 	return NULL;
@@ -341,8 +341,7 @@ dpll_device_registration_find(struct dpll_device *dpll,
 /**
  * dpll_device_register - register the dpll device in the subsystem
  * @dpll: pointer to a dpll
- * @type: type of a dpll
- * @ops: ops for a dpll device
+ * @info: dpll device information and operations from registerer
  * @priv: pointer to private information of owner
  *
  * Make dpll device available for user space.
@@ -352,11 +351,13 @@ dpll_device_registration_find(struct dpll_device *dpll,
  * * 0 on success
  * * negative - error value
  */
-int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
-			 const struct dpll_device_ops *ops, void *priv)
+int dpll_device_register(struct dpll_device *dpll,
+			 const struct dpll_device_info *info, void *priv)
 {
+	const struct dpll_device_ops *ops = info->ops;
 	struct dpll_device_registration *reg;
 	bool first_registration = false;
+	enum dpll_type type = info->type;
 
 	if (WARN_ON(!ops))
 		return -EINVAL;
@@ -368,7 +369,7 @@ int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
 		return -EINVAL;
 
 	mutex_lock(&dpll_lock);
-	reg = dpll_device_registration_find(dpll, ops, priv);
+	reg = dpll_device_registration_find(dpll, info, priv);
 	if (reg) {
 		mutex_unlock(&dpll_lock);
 		return -EEXIST;
@@ -379,9 +380,8 @@ int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
 		mutex_unlock(&dpll_lock);
 		return -ENOMEM;
 	}
-	reg->ops = ops;
+	reg->info = info;
 	reg->priv = priv;
-	dpll->type = type;
 	first_registration = list_empty(&dpll->registration_list);
 	list_add_tail(&reg->list, &dpll->registration_list);
 	if (!first_registration) {
@@ -408,14 +408,14 @@ EXPORT_SYMBOL_GPL(dpll_device_register);
  * Context: Acquires a lock (dpll_lock)
  */
 void dpll_device_unregister(struct dpll_device *dpll,
-			    const struct dpll_device_ops *ops, void *priv)
+			    const struct dpll_device_info *info, void *priv)
 {
 	struct dpll_device_registration *reg;
 
 	mutex_lock(&dpll_lock);
 	ASSERT_DPLL_REGISTERED(dpll);
 	dpll_device_delete_ntf(dpll);
-	reg = dpll_device_registration_find(dpll, ops, priv);
+	reg = dpll_device_registration_find(dpll, info, priv);
 	if (WARN_ON(!reg)) {
 		mutex_unlock(&dpll_lock);
 		return;
@@ -807,7 +807,15 @@ const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll)
 	struct dpll_device_registration *reg;
 
 	reg = dpll_device_registration_first(dpll);
-	return reg->ops;
+	return reg->info->ops;
+}
+
+const struct dpll_device_info *dpll_device_info(struct dpll_device *dpll)
+{
+	struct dpll_device_registration *reg;
+
+	reg = dpll_device_registration_first(dpll);
+	return reg->info;
 }
 
 static struct dpll_pin_registration *
diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
index 2b6d8ef1cdf3..baeb10d7dc1e 100644
--- a/drivers/dpll/dpll_core.h
+++ b/drivers/dpll/dpll_core.h
@@ -30,7 +30,6 @@ struct dpll_device {
 	u32 device_idx;
 	u64 clock_id;
 	struct module *module;
-	enum dpll_type type;
 	struct xarray pin_refs;
 	refcount_t refcount;
 	struct list_head registration_list;
@@ -84,6 +83,7 @@ void *dpll_pin_on_pin_priv(struct dpll_pin *parent, struct dpll_pin *pin);
 const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll);
 struct dpll_device *dpll_device_get_by_id(int id);
 const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref);
+const struct dpll_device_info *dpll_device_info(struct dpll_device *dpll);
 struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs);
 extern struct xarray dpll_device_xa;
 extern struct xarray dpll_pin_xa;
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index c130f87147fa..2de9ec08d551 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -564,6 +564,7 @@ static int
 dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
 		    struct netlink_ext_ack *extack)
 {
+	const struct dpll_device_info *info = dpll_device_info(dpll);
 	int ret;
 
 	ret = dpll_msg_add_dev_handle(msg, dpll);
@@ -589,7 +590,7 @@ dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
 	ret = dpll_msg_add_mode_supported(msg, dpll, extack);
 	if (ret)
 		return ret;
-	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
+	if (nla_put_u32(msg, DPLL_A_TYPE, info->type))
 		return -EMSGSIZE;
 
 	return 0;
@@ -1415,11 +1416,13 @@ dpll_device_find(u64 clock_id, struct nlattr *mod_name_attr,
 	unsigned long i;
 
 	xa_for_each_marked(&dpll_device_xa, i, dpll, DPLL_REGISTERED) {
+		const struct dpll_device_info *info = dpll_device_info(dpll);
+
 		cid_match = clock_id ? dpll->clock_id == clock_id : true;
 		mod_match = mod_name_attr ? (module_name(dpll->module) ?
 			!nla_strcmp(mod_name_attr,
 				    module_name(dpll->module)) : false) : true;
-		type_match = type ? dpll->type == type : true;
+		type_match = type ? info->type == type : true;
 		if (cid_match && mod_match && type_match) {
 			if (dpll_match) {
 				NL_SET_ERR_MSG(extack, "multiple matches");
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index bce3ad6ca2a6..0f7440a889ac 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -1977,7 +1977,7 @@ static void
 ice_dpll_deinit_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
 {
 	if (cgu)
-		dpll_device_unregister(d->dpll, &ice_dpll_ops, d);
+		dpll_device_unregister(d->dpll, &d->info, d);
 	dpll_device_put(d->dpll);
 }
 
@@ -1996,8 +1996,7 @@ ice_dpll_deinit_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
  * * negative - initialization failure reason
  */
 static int
-ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
-		   enum dpll_type type)
+ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
 {
 	u64 clock_id = pf->dplls.clock_id;
 	int ret;
@@ -2012,7 +2011,7 @@ ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
 	d->pf = pf;
 	if (cgu) {
 		ice_dpll_update_state(pf, d, true);
-		ret = dpll_device_register(d->dpll, type, &ice_dpll_ops, d);
+		ret = dpll_device_register(d->dpll, &d->info, d);
 		if (ret) {
 			dpll_device_put(d->dpll);
 			return ret;
@@ -2363,7 +2362,12 @@ static int ice_dpll_init_info(struct ice_pf *pf, bool cgu)
 	if (ret)
 		return ret;
 	de->mode = DPLL_MODE_AUTOMATIC;
+	de->info.type = DPLL_TYPE_EEC;
+	de->info.ops = &ice_dpll_ops;
+
 	dp->mode = DPLL_MODE_AUTOMATIC;
+	dp->info.type = DPLL_TYPE_PPS;
+	dp->info.ops = &ice_dpll_ops;
 
 	dev_dbg(ice_pf_to_dev(pf),
 		"%s - success, inputs:%u, outputs:%u rclk-parents:%u\n",
@@ -2426,10 +2430,10 @@ void ice_dpll_init(struct ice_pf *pf)
 	err = ice_dpll_init_info(pf, cgu);
 	if (err)
 		goto err_exit;
-	err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu, DPLL_TYPE_EEC);
+	err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu);
 	if (err)
 		goto deinit_info;
-	err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu, DPLL_TYPE_PPS);
+	err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu);
 	if (err)
 		goto deinit_eec;
 	err = ice_dpll_init_pins(pf, cgu);
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
index c320f1bf7d6d..9db7463e293a 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
@@ -66,6 +66,7 @@ struct ice_dpll {
 	enum dpll_mode mode;
 	struct dpll_pin *active_input;
 	struct dpll_pin *prev_input;
+	struct dpll_device_info info;
 };
 
 /** ice_dplls - store info required for CCU (clock controlling unit)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
index 1e5522a19483..f722b1de0754 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
@@ -20,6 +20,7 @@ struct mlx5_dpll {
 	} last;
 	struct notifier_block mdev_nb;
 	struct net_device *tracking_netdev;
+	struct dpll_device_info info;
 };
 
 static int mlx5_dpll_clock_id_get(struct mlx5_core_dev *mdev, u64 *clock_id)
@@ -444,8 +445,9 @@ static int mlx5_dpll_probe(struct auxiliary_device *adev,
 		goto err_free_mdpll;
 	}
 
-	err = dpll_device_register(mdpll->dpll, DPLL_TYPE_EEC,
-				   &mlx5_dpll_device_ops, mdpll);
+	mdpll->info.type = DPLL_TYPE_EEC;
+	mdpll->info.ops = &mlx5_dpll_device_ops;
+	err = dpll_device_register(mdpll->dpll, &mdpll->info, mdpll);
 	if (err)
 		goto err_put_dpll_device;
 
@@ -481,7 +483,7 @@ static int mlx5_dpll_probe(struct auxiliary_device *adev,
 err_put_dpll_pin:
 	dpll_pin_put(mdpll->dpll_pin);
 err_unregister_dpll_device:
-	dpll_device_unregister(mdpll->dpll, &mlx5_dpll_device_ops, mdpll);
+	dpll_device_unregister(mdpll->dpll, &mdpll->info, mdpll);
 err_put_dpll_device:
 	dpll_device_put(mdpll->dpll);
 err_free_mdpll:
@@ -500,7 +502,7 @@ static void mlx5_dpll_remove(struct auxiliary_device *adev)
 	dpll_pin_unregister(mdpll->dpll, mdpll->dpll_pin,
 			    &mlx5_dpll_pins_ops, mdpll);
 	dpll_pin_put(mdpll->dpll_pin);
-	dpll_device_unregister(mdpll->dpll, &mlx5_dpll_device_ops, mdpll);
+	dpll_device_unregister(mdpll->dpll, &mdpll->info, mdpll);
 	dpll_device_put(mdpll->dpll);
 	kfree(mdpll);
 
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 7945c6be1f7c..b3c5d294acb4 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -382,6 +382,7 @@ struct ptp_ocp {
 	struct ptp_ocp_sma_connector sma[OCP_SMA_NUM];
 	const struct ocp_sma_op *sma_op;
 	struct dpll_device *dpll;
+	struct dpll_device_info	dpll_info;
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -4745,7 +4746,9 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto out;
 	}
 
-	err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp);
+	bp->dpll_info.type = DPLL_TYPE_PPS;
+	bp->dpll_info.ops = &dpll_ops;
+	err = dpll_device_register(bp->dpll, &bp->dpll_info, bp);
 	if (err)
 		goto out;
 
@@ -4796,7 +4799,7 @@ ptp_ocp_remove(struct pci_dev *pdev)
 			dpll_pin_put(bp->sma[i].dpll_pin);
 		}
 	}
-	dpll_device_unregister(bp->dpll, &dpll_ops, bp);
+	dpll_device_unregister(bp->dpll, &bp->dpll_info, bp);
 	dpll_device_put(bp->dpll);
 	devlink_unregister(devlink);
 	ptp_ocp_detach(bp);
diff --git a/include/linux/dpll.h b/include/linux/dpll.h
index 5e4f9ab1cf75..0489464af958 100644
--- a/include/linux/dpll.h
+++ b/include/linux/dpll.h
@@ -97,6 +97,11 @@ struct dpll_pin_ops {
 			 struct netlink_ext_ack *extack);
 };
 
+struct dpll_device_info {
+	enum dpll_type type;
+	const struct dpll_device_ops *ops;
+};
+
 struct dpll_pin_frequency {
 	u64 min;
 	u64 max;
@@ -170,11 +175,11 @@ dpll_device_get(u64 clock_id, u32 dev_driver_id, struct module *module);
 
 void dpll_device_put(struct dpll_device *dpll);
 
-int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
-			 const struct dpll_device_ops *ops, void *priv);
+int dpll_device_register(struct dpll_device *dpll,
+			 const struct dpll_device_info *info, void *priv);
 
 void dpll_device_unregister(struct dpll_device *dpll,
-			    const struct dpll_device_ops *ops, void *priv);
+			    const struct dpll_device_info *info, void *priv);
 
 struct dpll_pin *
 dpll_pin_get(u64 clock_id, u32 dev_driver_id, struct module *module,
-- 
2.38.1

Re: [PATCH net-next v2 1/4] dpll: use struct dpll_device_info for dpll registration

[Jiri Pirko]

Tue, Apr 15, 2025 at 08:15:40PM +0200, arkadiusz.kubalewski@intel.com wrote:
>Instead of passing list of properties as arguments to
>dpll_device_register(..) use a dedicated struct.
>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
>v2:
>- new commit
>---
> drivers/dpll/dpll_core.c                      | 34 ++++++++++++-------
> drivers/dpll/dpll_core.h                      |  2 +-
> drivers/dpll/dpll_netlink.c                   |  7 ++--
> drivers/net/ethernet/intel/ice/ice_dpll.c     | 16 +++++----
> drivers/net/ethernet/intel/ice/ice_dpll.h     |  1 +
> .../net/ethernet/mellanox/mlx5/core/dpll.c    | 10 +++---
> drivers/ptp/ptp_ocp.c                         |  7 ++--
> include/linux/dpll.h                          | 11 ++++--
> 8 files changed, 57 insertions(+), 31 deletions(-)
>
>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>index 20bdc52f63a5..af9cda45a89c 100644
>--- a/drivers/dpll/dpll_core.c
>+++ b/drivers/dpll/dpll_core.c
>@@ -34,7 +34,7 @@ static u32 dpll_pin_xa_id;
> 
> struct dpll_device_registration {
> 	struct list_head list;
>-	const struct dpll_device_ops *ops;
>+	const struct dpll_device_info *info;
> 	void *priv;
> };
> 
>@@ -327,12 +327,12 @@ EXPORT_SYMBOL_GPL(dpll_device_put);
> 
> static struct dpll_device_registration *
> dpll_device_registration_find(struct dpll_device *dpll,
>-			      const struct dpll_device_ops *ops, void *priv)
>+			      const struct dpll_device_info *info, void *priv)
> {
> 	struct dpll_device_registration *reg;
> 
> 	list_for_each_entry(reg, &dpll->registration_list, list) {
>-		if (reg->ops == ops && reg->priv == priv)
>+		if (reg->info == info && reg->priv == priv)
> 			return reg;
> 	}
> 	return NULL;
>@@ -341,8 +341,7 @@ dpll_device_registration_find(struct dpll_device *dpll,
> /**
>  * dpll_device_register - register the dpll device in the subsystem
>  * @dpll: pointer to a dpll
>- * @type: type of a dpll
>- * @ops: ops for a dpll device
>+ * @info: dpll device information and operations from registerer
>  * @priv: pointer to private information of owner
>  *
>  * Make dpll device available for user space.
>@@ -352,11 +351,13 @@ dpll_device_registration_find(struct dpll_device *dpll,
>  * * 0 on success
>  * * negative - error value
>  */
>-int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
>-			 const struct dpll_device_ops *ops, void *priv)
>+int dpll_device_register(struct dpll_device *dpll,
>+			 const struct dpll_device_info *info, void *priv)

I don't like this. If you need some capabilities value, put it into ops
struct.


> {
>+	const struct dpll_device_ops *ops = info->ops;
> 	struct dpll_device_registration *reg;
> 	bool first_registration = false;
>+	enum dpll_type type = info->type;
> 
> 	if (WARN_ON(!ops))
> 		return -EINVAL;
>@@ -368,7 +369,7 @@ int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
> 		return -EINVAL;
> 
> 	mutex_lock(&dpll_lock);
>-	reg = dpll_device_registration_find(dpll, ops, priv);
>+	reg = dpll_device_registration_find(dpll, info, priv);
> 	if (reg) {
> 		mutex_unlock(&dpll_lock);
> 		return -EEXIST;
>@@ -379,9 +380,8 @@ int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
> 		mutex_unlock(&dpll_lock);
> 		return -ENOMEM;
> 	}
>-	reg->ops = ops;
>+	reg->info = info;
> 	reg->priv = priv;
>-	dpll->type = type;
> 	first_registration = list_empty(&dpll->registration_list);
> 	list_add_tail(&reg->list, &dpll->registration_list);
> 	if (!first_registration) {
>@@ -408,14 +408,14 @@ EXPORT_SYMBOL_GPL(dpll_device_register);
>  * Context: Acquires a lock (dpll_lock)
>  */
> void dpll_device_unregister(struct dpll_device *dpll,
>-			    const struct dpll_device_ops *ops, void *priv)
>+			    const struct dpll_device_info *info, void *priv)
> {
> 	struct dpll_device_registration *reg;
> 
> 	mutex_lock(&dpll_lock);
> 	ASSERT_DPLL_REGISTERED(dpll);
> 	dpll_device_delete_ntf(dpll);
>-	reg = dpll_device_registration_find(dpll, ops, priv);
>+	reg = dpll_device_registration_find(dpll, info, priv);
> 	if (WARN_ON(!reg)) {
> 		mutex_unlock(&dpll_lock);
> 		return;
>@@ -807,7 +807,15 @@ const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll)
> 	struct dpll_device_registration *reg;
> 
> 	reg = dpll_device_registration_first(dpll);
>-	return reg->ops;
>+	return reg->info->ops;
>+}
>+
>+const struct dpll_device_info *dpll_device_info(struct dpll_device *dpll)

Makes me wonder what you would need this for. I guess "nothing"?


>+{
>+	struct dpll_device_registration *reg;
>+
>+	reg = dpll_device_registration_first(dpll);
>+	return reg->info;
> }
> 
> static struct dpll_pin_registration *
>diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>index 2b6d8ef1cdf3..baeb10d7dc1e 100644
>--- a/drivers/dpll/dpll_core.h
>+++ b/drivers/dpll/dpll_core.h
>@@ -30,7 +30,6 @@ struct dpll_device {
> 	u32 device_idx;
> 	u64 clock_id;
> 	struct module *module;
>-	enum dpll_type type;
> 	struct xarray pin_refs;
> 	refcount_t refcount;
> 	struct list_head registration_list;
>@@ -84,6 +83,7 @@ void *dpll_pin_on_pin_priv(struct dpll_pin *parent, struct dpll_pin *pin);
> const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll);
> struct dpll_device *dpll_device_get_by_id(int id);
> const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref);
>+const struct dpll_device_info *dpll_device_info(struct dpll_device *dpll);
> struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs);
> extern struct xarray dpll_device_xa;
> extern struct xarray dpll_pin_xa;
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index c130f87147fa..2de9ec08d551 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -564,6 +564,7 @@ static int
> dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
> 		    struct netlink_ext_ack *extack)
> {
>+	const struct dpll_device_info *info = dpll_device_info(dpll);
> 	int ret;
> 
> 	ret = dpll_msg_add_dev_handle(msg, dpll);
>@@ -589,7 +590,7 @@ dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
> 	ret = dpll_msg_add_mode_supported(msg, dpll, extack);
> 	if (ret)
> 		return ret;
>-	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
>+	if (nla_put_u32(msg, DPLL_A_TYPE, info->type))
> 		return -EMSGSIZE;
> 
> 	return 0;
>@@ -1415,11 +1416,13 @@ dpll_device_find(u64 clock_id, struct nlattr *mod_name_attr,
> 	unsigned long i;
> 
> 	xa_for_each_marked(&dpll_device_xa, i, dpll, DPLL_REGISTERED) {
>+		const struct dpll_device_info *info = dpll_device_info(dpll);
>+
> 		cid_match = clock_id ? dpll->clock_id == clock_id : true;
> 		mod_match = mod_name_attr ? (module_name(dpll->module) ?
> 			!nla_strcmp(mod_name_attr,
> 				    module_name(dpll->module)) : false) : true;
>-		type_match = type ? dpll->type == type : true;
>+		type_match = type ? info->type == type : true;
> 		if (cid_match && mod_match && type_match) {
> 			if (dpll_match) {
> 				NL_SET_ERR_MSG(extack, "multiple matches");
>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
>index bce3ad6ca2a6..0f7440a889ac 100644
>--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
>@@ -1977,7 +1977,7 @@ static void
> ice_dpll_deinit_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
> {
> 	if (cgu)
>-		dpll_device_unregister(d->dpll, &ice_dpll_ops, d);
>+		dpll_device_unregister(d->dpll, &d->info, d);
> 	dpll_device_put(d->dpll);
> }
> 
>@@ -1996,8 +1996,7 @@ ice_dpll_deinit_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
>  * * negative - initialization failure reason
>  */
> static int
>-ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
>-		   enum dpll_type type)
>+ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
> {
> 	u64 clock_id = pf->dplls.clock_id;
> 	int ret;
>@@ -2012,7 +2011,7 @@ ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
> 	d->pf = pf;
> 	if (cgu) {
> 		ice_dpll_update_state(pf, d, true);
>-		ret = dpll_device_register(d->dpll, type, &ice_dpll_ops, d);
>+		ret = dpll_device_register(d->dpll, &d->info, d);
> 		if (ret) {
> 			dpll_device_put(d->dpll);
> 			return ret;
>@@ -2363,7 +2362,12 @@ static int ice_dpll_init_info(struct ice_pf *pf, bool cgu)
> 	if (ret)
> 		return ret;
> 	de->mode = DPLL_MODE_AUTOMATIC;
>+	de->info.type = DPLL_TYPE_EEC;
>+	de->info.ops = &ice_dpll_ops;
>+
> 	dp->mode = DPLL_MODE_AUTOMATIC;
>+	dp->info.type = DPLL_TYPE_PPS;
>+	dp->info.ops = &ice_dpll_ops;
> 
> 	dev_dbg(ice_pf_to_dev(pf),
> 		"%s - success, inputs:%u, outputs:%u rclk-parents:%u\n",
>@@ -2426,10 +2430,10 @@ void ice_dpll_init(struct ice_pf *pf)
> 	err = ice_dpll_init_info(pf, cgu);
> 	if (err)
> 		goto err_exit;
>-	err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu, DPLL_TYPE_EEC);
>+	err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu);
> 	if (err)
> 		goto deinit_info;
>-	err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu, DPLL_TYPE_PPS);
>+	err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu);
> 	if (err)
> 		goto deinit_eec;
> 	err = ice_dpll_init_pins(pf, cgu);
>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
>index c320f1bf7d6d..9db7463e293a 100644
>--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
>@@ -66,6 +66,7 @@ struct ice_dpll {
> 	enum dpll_mode mode;
> 	struct dpll_pin *active_input;
> 	struct dpll_pin *prev_input;
>+	struct dpll_device_info info;
> };
> 
> /** ice_dplls - store info required for CCU (clock controlling unit)
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>index 1e5522a19483..f722b1de0754 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>@@ -20,6 +20,7 @@ struct mlx5_dpll {
> 	} last;
> 	struct notifier_block mdev_nb;
> 	struct net_device *tracking_netdev;
>+	struct dpll_device_info info;
> };
> 
> static int mlx5_dpll_clock_id_get(struct mlx5_core_dev *mdev, u64 *clock_id)
>@@ -444,8 +445,9 @@ static int mlx5_dpll_probe(struct auxiliary_device *adev,
> 		goto err_free_mdpll;
> 	}
> 
>-	err = dpll_device_register(mdpll->dpll, DPLL_TYPE_EEC,
>-				   &mlx5_dpll_device_ops, mdpll);
>+	mdpll->info.type = DPLL_TYPE_EEC;
>+	mdpll->info.ops = &mlx5_dpll_device_ops;
>+	err = dpll_device_register(mdpll->dpll, &mdpll->info, mdpll);
> 	if (err)
> 		goto err_put_dpll_device;
> 
>@@ -481,7 +483,7 @@ static int mlx5_dpll_probe(struct auxiliary_device *adev,
> err_put_dpll_pin:
> 	dpll_pin_put(mdpll->dpll_pin);
> err_unregister_dpll_device:
>-	dpll_device_unregister(mdpll->dpll, &mlx5_dpll_device_ops, mdpll);
>+	dpll_device_unregister(mdpll->dpll, &mdpll->info, mdpll);
> err_put_dpll_device:
> 	dpll_device_put(mdpll->dpll);
> err_free_mdpll:
>@@ -500,7 +502,7 @@ static void mlx5_dpll_remove(struct auxiliary_device *adev)
> 	dpll_pin_unregister(mdpll->dpll, mdpll->dpll_pin,
> 			    &mlx5_dpll_pins_ops, mdpll);
> 	dpll_pin_put(mdpll->dpll_pin);
>-	dpll_device_unregister(mdpll->dpll, &mlx5_dpll_device_ops, mdpll);
>+	dpll_device_unregister(mdpll->dpll, &mdpll->info, mdpll);
> 	dpll_device_put(mdpll->dpll);
> 	kfree(mdpll);
> 
>diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>index 7945c6be1f7c..b3c5d294acb4 100644
>--- a/drivers/ptp/ptp_ocp.c
>+++ b/drivers/ptp/ptp_ocp.c
>@@ -382,6 +382,7 @@ struct ptp_ocp {
> 	struct ptp_ocp_sma_connector sma[OCP_SMA_NUM];
> 	const struct ocp_sma_op *sma_op;
> 	struct dpll_device *dpll;
>+	struct dpll_device_info	dpll_info;
> };
> 
> #define OCP_REQ_TIMESTAMP	BIT(0)
>@@ -4745,7 +4746,9 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> 		goto out;
> 	}
> 
>-	err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp);
>+	bp->dpll_info.type = DPLL_TYPE_PPS;
>+	bp->dpll_info.ops = &dpll_ops;
>+	err = dpll_device_register(bp->dpll, &bp->dpll_info, bp);
> 	if (err)
> 		goto out;
> 
>@@ -4796,7 +4799,7 @@ ptp_ocp_remove(struct pci_dev *pdev)
> 			dpll_pin_put(bp->sma[i].dpll_pin);
> 		}
> 	}
>-	dpll_device_unregister(bp->dpll, &dpll_ops, bp);
>+	dpll_device_unregister(bp->dpll, &bp->dpll_info, bp);
> 	dpll_device_put(bp->dpll);
> 	devlink_unregister(devlink);
> 	ptp_ocp_detach(bp);
>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>index 5e4f9ab1cf75..0489464af958 100644
>--- a/include/linux/dpll.h
>+++ b/include/linux/dpll.h
>@@ -97,6 +97,11 @@ struct dpll_pin_ops {
> 			 struct netlink_ext_ack *extack);
> };
> 
>+struct dpll_device_info {
>+	enum dpll_type type;
>+	const struct dpll_device_ops *ops;
>+};
>+
> struct dpll_pin_frequency {
> 	u64 min;
> 	u64 max;
>@@ -170,11 +175,11 @@ dpll_device_get(u64 clock_id, u32 dev_driver_id, struct module *module);
> 
> void dpll_device_put(struct dpll_device *dpll);
> 
>-int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
>-			 const struct dpll_device_ops *ops, void *priv);
>+int dpll_device_register(struct dpll_device *dpll,
>+			 const struct dpll_device_info *info, void *priv);
> 
> void dpll_device_unregister(struct dpll_device *dpll,
>-			    const struct dpll_device_ops *ops, void *priv);
>+			    const struct dpll_device_info *info, void *priv);
> 
> struct dpll_pin *
> dpll_pin_get(u64 clock_id, u32 dev_driver_id, struct module *module,
>-- 
>2.38.1
>

RE: [PATCH net-next v2 1/4] dpll: use struct dpll_device_info for dpll registration

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Wednesday, April 16, 2025 2:13 PM
>
>Tue, Apr 15, 2025 at 08:15:40PM +0200, arkadiusz.kubalewski@intel.com
>wrote:
>>Instead of passing list of properties as arguments to
>>dpll_device_register(..) use a dedicated struct.
>>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>---
>>v2:
>>- new commit
>>---
>> drivers/dpll/dpll_core.c                      | 34 ++++++++++++-------
>> drivers/dpll/dpll_core.h                      |  2 +-
>> drivers/dpll/dpll_netlink.c                   |  7 ++--
>> drivers/net/ethernet/intel/ice/ice_dpll.c     | 16 +++++----
>> drivers/net/ethernet/intel/ice/ice_dpll.h     |  1 +
>> .../net/ethernet/mellanox/mlx5/core/dpll.c    | 10 +++---
>> drivers/ptp/ptp_ocp.c                         |  7 ++--
>> include/linux/dpll.h                          | 11 ++++--
>> 8 files changed, 57 insertions(+), 31 deletions(-)
>>
>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>index 20bdc52f63a5..af9cda45a89c 100644
>>--- a/drivers/dpll/dpll_core.c
>>+++ b/drivers/dpll/dpll_core.c
>>@@ -34,7 +34,7 @@ static u32 dpll_pin_xa_id;
>>
>> struct dpll_device_registration {
>> 	struct list_head list;
>>-	const struct dpll_device_ops *ops;
>>+	const struct dpll_device_info *info;
>> 	void *priv;
>> };
>>
>>@@ -327,12 +327,12 @@ EXPORT_SYMBOL_GPL(dpll_device_put);
>>
>> static struct dpll_device_registration *
>> dpll_device_registration_find(struct dpll_device *dpll,
>>-			      const struct dpll_device_ops *ops, void *priv)
>>+			      const struct dpll_device_info *info, void *priv)
>> {
>> 	struct dpll_device_registration *reg;
>>
>> 	list_for_each_entry(reg, &dpll->registration_list, list) {
>>-		if (reg->ops == ops && reg->priv == priv)
>>+		if (reg->info == info && reg->priv == priv)
>> 			return reg;
>> 	}
>> 	return NULL;
>>@@ -341,8 +341,7 @@ dpll_device_registration_find(struct dpll_device
>>*dpll,
>> /**
>>  * dpll_device_register - register the dpll device in the subsystem
>>  * @dpll: pointer to a dpll
>>- * @type: type of a dpll
>>- * @ops: ops for a dpll device
>>+ * @info: dpll device information and operations from registerer
>>  * @priv: pointer to private information of owner
>>  *
>>  * Make dpll device available for user space.
>>@@ -352,11 +351,13 @@ dpll_device_registration_find(struct dpll_device
>>*dpll,
>>  * * 0 on success
>>  * * negative - error value
>>  */
>>-int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
>>-			 const struct dpll_device_ops *ops, void *priv)
>>+int dpll_device_register(struct dpll_device *dpll,
>>+			 const struct dpll_device_info *info, void *priv)
>
>I don't like this. If you need some capabilities value, put it into ops
>struct.
>

Hmm, this would seems strange, the _ops indicates operations, would
have to rename the struct..

In theory I could decide on capabilities per ops provided from driver..
i.e. If phase_input_monitor_feature_set()/phase_input_feature_get() are
present then capability phase_input_monitor is provided..
Makes sense?

>
>> {
>>+	const struct dpll_device_ops *ops = info->ops;
>> 	struct dpll_device_registration *reg;
>> 	bool first_registration = false;
>>+	enum dpll_type type = info->type;
>>
>> 	if (WARN_ON(!ops))
>> 		return -EINVAL;
>>@@ -368,7 +369,7 @@ int dpll_device_register(struct dpll_device *dpll,
>>enum dpll_type type,
>> 		return -EINVAL;
>>
>> 	mutex_lock(&dpll_lock);
>>-	reg = dpll_device_registration_find(dpll, ops, priv);
>>+	reg = dpll_device_registration_find(dpll, info, priv);
>> 	if (reg) {
>> 		mutex_unlock(&dpll_lock);
>> 		return -EEXIST;
>>@@ -379,9 +380,8 @@ int dpll_device_register(struct dpll_device *dpll,
>>enum dpll_type type,
>> 		mutex_unlock(&dpll_lock);
>> 		return -ENOMEM;
>> 	}
>>-	reg->ops = ops;
>>+	reg->info = info;
>> 	reg->priv = priv;
>>-	dpll->type = type;
>> 	first_registration = list_empty(&dpll->registration_list);
>> 	list_add_tail(&reg->list, &dpll->registration_list);
>> 	if (!first_registration) {
>>@@ -408,14 +408,14 @@ EXPORT_SYMBOL_GPL(dpll_device_register);
>>  * Context: Acquires a lock (dpll_lock)
>>  */
>> void dpll_device_unregister(struct dpll_device *dpll,
>>-			    const struct dpll_device_ops *ops, void *priv)
>>+			    const struct dpll_device_info *info, void *priv)
>> {
>> 	struct dpll_device_registration *reg;
>>
>> 	mutex_lock(&dpll_lock);
>> 	ASSERT_DPLL_REGISTERED(dpll);
>> 	dpll_device_delete_ntf(dpll);
>>-	reg = dpll_device_registration_find(dpll, ops, priv);
>>+	reg = dpll_device_registration_find(dpll, info, priv);
>> 	if (WARN_ON(!reg)) {
>> 		mutex_unlock(&dpll_lock);
>> 		return;
>>@@ -807,7 +807,15 @@ const struct dpll_device_ops *dpll_device_ops(struct
>>dpll_device *dpll)
>> 	struct dpll_device_registration *reg;
>>
>> 	reg = dpll_device_registration_first(dpll);
>>-	return reg->ops;
>>+	return reg->info->ops;
>>+}
>>+
>>+const struct dpll_device_info *dpll_device_info(struct dpll_device *dpll)
>
>Makes me wonder what you would need this for. I guess "nothing"?
>

Now using it to get info struct from dpll.. if struct is removed then yeah.

Thank you!
Arkadiusz

>
>>+{
>>+	struct dpll_device_registration *reg;
>>+
>>+	reg = dpll_device_registration_first(dpll);
>>+	return reg->info;
>> }
>>
>> static struct dpll_pin_registration *
>>diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>>index 2b6d8ef1cdf3..baeb10d7dc1e 100644
>>--- a/drivers/dpll/dpll_core.h
>>+++ b/drivers/dpll/dpll_core.h
>>@@ -30,7 +30,6 @@ struct dpll_device {
>> 	u32 device_idx;
>> 	u64 clock_id;
>> 	struct module *module;
>>-	enum dpll_type type;
>> 	struct xarray pin_refs;
>> 	refcount_t refcount;
>> 	struct list_head registration_list;
>>@@ -84,6 +83,7 @@ void *dpll_pin_on_pin_priv(struct dpll_pin *parent,
>>struct dpll_pin *pin);
>> const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll);
>> struct dpll_device *dpll_device_get_by_id(int id);
>> const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref);
>>+const struct dpll_device_info *dpll_device_info(struct dpll_device
>>*dpll);
>> struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs);
>> extern struct xarray dpll_device_xa;
>> extern struct xarray dpll_pin_xa;
>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>index c130f87147fa..2de9ec08d551 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -564,6 +564,7 @@ static int
>> dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
>> 		    struct netlink_ext_ack *extack)
>> {
>>+	const struct dpll_device_info *info = dpll_device_info(dpll);
>> 	int ret;
>>
>> 	ret = dpll_msg_add_dev_handle(msg, dpll);
>>@@ -589,7 +590,7 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>>sk_buff *msg,
>> 	ret = dpll_msg_add_mode_supported(msg, dpll, extack);
>> 	if (ret)
>> 		return ret;
>>-	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
>>+	if (nla_put_u32(msg, DPLL_A_TYPE, info->type))
>> 		return -EMSGSIZE;
>>
>> 	return 0;
>>@@ -1415,11 +1416,13 @@ dpll_device_find(u64 clock_id, struct nlattr
>>*mod_name_attr,
>> 	unsigned long i;
>>
>> 	xa_for_each_marked(&dpll_device_xa, i, dpll, DPLL_REGISTERED) {
>>+		const struct dpll_device_info *info = dpll_device_info(dpll);
>>+
>> 		cid_match = clock_id ? dpll->clock_id == clock_id : true;
>> 		mod_match = mod_name_attr ? (module_name(dpll->module) ?
>> 			!nla_strcmp(mod_name_attr,
>> 				    module_name(dpll->module)) : false) : true;
>>-		type_match = type ? dpll->type == type : true;
>>+		type_match = type ? info->type == type : true;
>> 		if (cid_match && mod_match && type_match) {
>> 			if (dpll_match) {
>> 				NL_SET_ERR_MSG(extack, "multiple matches");
>>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c
>>b/drivers/net/ethernet/intel/ice/ice_dpll.c
>>index bce3ad6ca2a6..0f7440a889ac 100644
>>--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
>>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
>>@@ -1977,7 +1977,7 @@ static void
>> ice_dpll_deinit_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
>> {
>> 	if (cgu)
>>-		dpll_device_unregister(d->dpll, &ice_dpll_ops, d);
>>+		dpll_device_unregister(d->dpll, &d->info, d);
>> 	dpll_device_put(d->dpll);
>> }
>>
>>@@ -1996,8 +1996,7 @@ ice_dpll_deinit_dpll(struct ice_pf *pf, struct
>>ice_dpll *d, bool cgu)
>>  * * negative - initialization failure reason
>>  */
>> static int
>>-ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
>>-		   enum dpll_type type)
>>+ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
>> {
>> 	u64 clock_id = pf->dplls.clock_id;
>> 	int ret;
>>@@ -2012,7 +2011,7 @@ ice_dpll_init_dpll(struct ice_pf *pf, struct
>>ice_dpll *d, bool cgu,
>> 	d->pf = pf;
>> 	if (cgu) {
>> 		ice_dpll_update_state(pf, d, true);
>>-		ret = dpll_device_register(d->dpll, type, &ice_dpll_ops, d);
>>+		ret = dpll_device_register(d->dpll, &d->info, d);
>> 		if (ret) {
>> 			dpll_device_put(d->dpll);
>> 			return ret;
>>@@ -2363,7 +2362,12 @@ static int ice_dpll_init_info(struct ice_pf *pf,
>>bool cgu)
>> 	if (ret)
>> 		return ret;
>> 	de->mode = DPLL_MODE_AUTOMATIC;
>>+	de->info.type = DPLL_TYPE_EEC;
>>+	de->info.ops = &ice_dpll_ops;
>>+
>> 	dp->mode = DPLL_MODE_AUTOMATIC;
>>+	dp->info.type = DPLL_TYPE_PPS;
>>+	dp->info.ops = &ice_dpll_ops;
>>
>> 	dev_dbg(ice_pf_to_dev(pf),
>> 		"%s - success, inputs:%u, outputs:%u rclk-parents:%u\n",
>>@@ -2426,10 +2430,10 @@ void ice_dpll_init(struct ice_pf *pf)
>> 	err = ice_dpll_init_info(pf, cgu);
>> 	if (err)
>> 		goto err_exit;
>>-	err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu, DPLL_TYPE_EEC);
>>+	err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu);
>> 	if (err)
>> 		goto deinit_info;
>>-	err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu, DPLL_TYPE_PPS);
>>+	err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu);
>> 	if (err)
>> 		goto deinit_eec;
>> 	err = ice_dpll_init_pins(pf, cgu);
>>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h
>>b/drivers/net/ethernet/intel/ice/ice_dpll.h
>>index c320f1bf7d6d..9db7463e293a 100644
>>--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
>>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
>>@@ -66,6 +66,7 @@ struct ice_dpll {
>> 	enum dpll_mode mode;
>> 	struct dpll_pin *active_input;
>> 	struct dpll_pin *prev_input;
>>+	struct dpll_device_info info;
>> };
>>
>> /** ice_dplls - store info required for CCU (clock controlling unit)
>>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>index 1e5522a19483..f722b1de0754 100644
>>--- a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>+++ b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>@@ -20,6 +20,7 @@ struct mlx5_dpll {
>> 	} last;
>> 	struct notifier_block mdev_nb;
>> 	struct net_device *tracking_netdev;
>>+	struct dpll_device_info info;
>> };
>>
>> static int mlx5_dpll_clock_id_get(struct mlx5_core_dev *mdev, u64
>>*clock_id)
>>@@ -444,8 +445,9 @@ static int mlx5_dpll_probe(struct auxiliary_device
>>*adev,
>> 		goto err_free_mdpll;
>> 	}
>>
>>-	err = dpll_device_register(mdpll->dpll, DPLL_TYPE_EEC,
>>-				   &mlx5_dpll_device_ops, mdpll);
>>+	mdpll->info.type = DPLL_TYPE_EEC;
>>+	mdpll->info.ops = &mlx5_dpll_device_ops;
>>+	err = dpll_device_register(mdpll->dpll, &mdpll->info, mdpll);
>> 	if (err)
>> 		goto err_put_dpll_device;
>>
>>@@ -481,7 +483,7 @@ static int mlx5_dpll_probe(struct auxiliary_device
>>*adev,
>> err_put_dpll_pin:
>> 	dpll_pin_put(mdpll->dpll_pin);
>> err_unregister_dpll_device:
>>-	dpll_device_unregister(mdpll->dpll, &mlx5_dpll_device_ops, mdpll);
>>+	dpll_device_unregister(mdpll->dpll, &mdpll->info, mdpll);
>> err_put_dpll_device:
>> 	dpll_device_put(mdpll->dpll);
>> err_free_mdpll:
>>@@ -500,7 +502,7 @@ static void mlx5_dpll_remove(struct auxiliary_device
>>*adev)
>> 	dpll_pin_unregister(mdpll->dpll, mdpll->dpll_pin,
>> 			    &mlx5_dpll_pins_ops, mdpll);
>> 	dpll_pin_put(mdpll->dpll_pin);
>>-	dpll_device_unregister(mdpll->dpll, &mlx5_dpll_device_ops, mdpll);
>>+	dpll_device_unregister(mdpll->dpll, &mdpll->info, mdpll);
>> 	dpll_device_put(mdpll->dpll);
>> 	kfree(mdpll);
>>
>>diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>index 7945c6be1f7c..b3c5d294acb4 100644
>>--- a/drivers/ptp/ptp_ocp.c
>>+++ b/drivers/ptp/ptp_ocp.c
>>@@ -382,6 +382,7 @@ struct ptp_ocp {
>> 	struct ptp_ocp_sma_connector sma[OCP_SMA_NUM];
>> 	const struct ocp_sma_op *sma_op;
>> 	struct dpll_device *dpll;
>>+	struct dpll_device_info	dpll_info;
>> };
>>
>> #define OCP_REQ_TIMESTAMP	BIT(0)
>>@@ -4745,7 +4746,9 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct
>>pci_device_id *id)
>> 		goto out;
>> 	}
>>
>>-	err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp);
>>+	bp->dpll_info.type = DPLL_TYPE_PPS;
>>+	bp->dpll_info.ops = &dpll_ops;
>>+	err = dpll_device_register(bp->dpll, &bp->dpll_info, bp);
>> 	if (err)
>> 		goto out;
>>
>>@@ -4796,7 +4799,7 @@ ptp_ocp_remove(struct pci_dev *pdev)
>> 			dpll_pin_put(bp->sma[i].dpll_pin);
>> 		}
>> 	}
>>-	dpll_device_unregister(bp->dpll, &dpll_ops, bp);
>>+	dpll_device_unregister(bp->dpll, &bp->dpll_info, bp);
>> 	dpll_device_put(bp->dpll);
>> 	devlink_unregister(devlink);
>> 	ptp_ocp_detach(bp);
>>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>index 5e4f9ab1cf75..0489464af958 100644
>>--- a/include/linux/dpll.h
>>+++ b/include/linux/dpll.h
>>@@ -97,6 +97,11 @@ struct dpll_pin_ops {
>> 			 struct netlink_ext_ack *extack);
>> };
>>
>>+struct dpll_device_info {
>>+	enum dpll_type type;
>>+	const struct dpll_device_ops *ops;
>>+};
>>+
>> struct dpll_pin_frequency {
>> 	u64 min;
>> 	u64 max;
>>@@ -170,11 +175,11 @@ dpll_device_get(u64 clock_id, u32 dev_driver_id,
>>struct module *module);
>>
>> void dpll_device_put(struct dpll_device *dpll);
>>
>>-int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
>>-			 const struct dpll_device_ops *ops, void *priv);
>>+int dpll_device_register(struct dpll_device *dpll,
>>+			 const struct dpll_device_info *info, void *priv);
>>
>> void dpll_device_unregister(struct dpll_device *dpll,
>>-			    const struct dpll_device_ops *ops, void *priv);
>>+			    const struct dpll_device_info *info, void *priv);
>>
>> struct dpll_pin *
>> dpll_pin_get(u64 clock_id, u32 dev_driver_id, struct module *module,
>>--
>>2.38.1
>>

Re: [PATCH net-next v2 1/4] dpll: use struct dpll_device_info for dpll registration

[Jiri Pirko]

Thu, Apr 17, 2025 at 11:33:13AM +0200, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Wednesday, April 16, 2025 2:13 PM
>>
>>Tue, Apr 15, 2025 at 08:15:40PM +0200, arkadiusz.kubalewski@intel.com
>>wrote:
>>>Instead of passing list of properties as arguments to
>>>dpll_device_register(..) use a dedicated struct.
>>>
>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>>---
>>>v2:
>>>- new commit
>>>---
>>> drivers/dpll/dpll_core.c                      | 34 ++++++++++++-------
>>> drivers/dpll/dpll_core.h                      |  2 +-
>>> drivers/dpll/dpll_netlink.c                   |  7 ++--
>>> drivers/net/ethernet/intel/ice/ice_dpll.c     | 16 +++++----
>>> drivers/net/ethernet/intel/ice/ice_dpll.h     |  1 +
>>> .../net/ethernet/mellanox/mlx5/core/dpll.c    | 10 +++---
>>> drivers/ptp/ptp_ocp.c                         |  7 ++--
>>> include/linux/dpll.h                          | 11 ++++--
>>> 8 files changed, 57 insertions(+), 31 deletions(-)
>>>
>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>index 20bdc52f63a5..af9cda45a89c 100644
>>>--- a/drivers/dpll/dpll_core.c
>>>+++ b/drivers/dpll/dpll_core.c
>>>@@ -34,7 +34,7 @@ static u32 dpll_pin_xa_id;
>>>
>>> struct dpll_device_registration {
>>> 	struct list_head list;
>>>-	const struct dpll_device_ops *ops;
>>>+	const struct dpll_device_info *info;
>>> 	void *priv;
>>> };
>>>
>>>@@ -327,12 +327,12 @@ EXPORT_SYMBOL_GPL(dpll_device_put);
>>>
>>> static struct dpll_device_registration *
>>> dpll_device_registration_find(struct dpll_device *dpll,
>>>-			      const struct dpll_device_ops *ops, void *priv)
>>>+			      const struct dpll_device_info *info, void *priv)
>>> {
>>> 	struct dpll_device_registration *reg;
>>>
>>> 	list_for_each_entry(reg, &dpll->registration_list, list) {
>>>-		if (reg->ops == ops && reg->priv == priv)
>>>+		if (reg->info == info && reg->priv == priv)
>>> 			return reg;
>>> 	}
>>> 	return NULL;
>>>@@ -341,8 +341,7 @@ dpll_device_registration_find(struct dpll_device
>>>*dpll,
>>> /**
>>>  * dpll_device_register - register the dpll device in the subsystem
>>>  * @dpll: pointer to a dpll
>>>- * @type: type of a dpll
>>>- * @ops: ops for a dpll device
>>>+ * @info: dpll device information and operations from registerer
>>>  * @priv: pointer to private information of owner
>>>  *
>>>  * Make dpll device available for user space.
>>>@@ -352,11 +351,13 @@ dpll_device_registration_find(struct dpll_device
>>>*dpll,
>>>  * * 0 on success
>>>  * * negative - error value
>>>  */
>>>-int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
>>>-			 const struct dpll_device_ops *ops, void *priv)
>>>+int dpll_device_register(struct dpll_device *dpll,
>>>+			 const struct dpll_device_info *info, void *priv)
>>
>>I don't like this. If you need some capabilities value, put it into ops
>>struct.
>>
>
>Hmm, this would seems strange, the _ops indicates operations, would
>have to rename the struct..

I don't think so. Happens all the time in kernel for ops to contain
other things.


>
>In theory I could decide on capabilities per ops provided from driver..
>i.e. If phase_input_monitor_feature_set()/phase_input_feature_get() are
>present then capability phase_input_monitor is provided..
>Makes sense?

Yes, that is more or less what I suggested in reply to the other patch
in this set.


>
>>
>>> {
>>>+	const struct dpll_device_ops *ops = info->ops;
>>> 	struct dpll_device_registration *reg;
>>> 	bool first_registration = false;
>>>+	enum dpll_type type = info->type;
>>>
>>> 	if (WARN_ON(!ops))
>>> 		return -EINVAL;
>>>@@ -368,7 +369,7 @@ int dpll_device_register(struct dpll_device *dpll,
>>>enum dpll_type type,
>>> 		return -EINVAL;
>>>
>>> 	mutex_lock(&dpll_lock);
>>>-	reg = dpll_device_registration_find(dpll, ops, priv);
>>>+	reg = dpll_device_registration_find(dpll, info, priv);
>>> 	if (reg) {
>>> 		mutex_unlock(&dpll_lock);
>>> 		return -EEXIST;
>>>@@ -379,9 +380,8 @@ int dpll_device_register(struct dpll_device *dpll,
>>>enum dpll_type type,
>>> 		mutex_unlock(&dpll_lock);
>>> 		return -ENOMEM;
>>> 	}
>>>-	reg->ops = ops;
>>>+	reg->info = info;
>>> 	reg->priv = priv;
>>>-	dpll->type = type;
>>> 	first_registration = list_empty(&dpll->registration_list);
>>> 	list_add_tail(&reg->list, &dpll->registration_list);
>>> 	if (!first_registration) {
>>>@@ -408,14 +408,14 @@ EXPORT_SYMBOL_GPL(dpll_device_register);
>>>  * Context: Acquires a lock (dpll_lock)
>>>  */
>>> void dpll_device_unregister(struct dpll_device *dpll,
>>>-			    const struct dpll_device_ops *ops, void *priv)
>>>+			    const struct dpll_device_info *info, void *priv)
>>> {
>>> 	struct dpll_device_registration *reg;
>>>
>>> 	mutex_lock(&dpll_lock);
>>> 	ASSERT_DPLL_REGISTERED(dpll);
>>> 	dpll_device_delete_ntf(dpll);
>>>-	reg = dpll_device_registration_find(dpll, ops, priv);
>>>+	reg = dpll_device_registration_find(dpll, info, priv);
>>> 	if (WARN_ON(!reg)) {
>>> 		mutex_unlock(&dpll_lock);
>>> 		return;
>>>@@ -807,7 +807,15 @@ const struct dpll_device_ops *dpll_device_ops(struct
>>>dpll_device *dpll)
>>> 	struct dpll_device_registration *reg;
>>>
>>> 	reg = dpll_device_registration_first(dpll);
>>>-	return reg->ops;
>>>+	return reg->info->ops;
>>>+}
>>>+
>>>+const struct dpll_device_info *dpll_device_info(struct dpll_device *dpll)
>>
>>Makes me wonder what you would need this for. I guess "nothing"?
>>
>
>Now using it to get info struct from dpll.. if struct is removed then yeah.
>
>Thank you!
>Arkadiusz
>
>>
>>>+{
>>>+	struct dpll_device_registration *reg;
>>>+
>>>+	reg = dpll_device_registration_first(dpll);
>>>+	return reg->info;
>>> }
>>>
>>> static struct dpll_pin_registration *
>>>diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>>>index 2b6d8ef1cdf3..baeb10d7dc1e 100644
>>>--- a/drivers/dpll/dpll_core.h
>>>+++ b/drivers/dpll/dpll_core.h
>>>@@ -30,7 +30,6 @@ struct dpll_device {
>>> 	u32 device_idx;
>>> 	u64 clock_id;
>>> 	struct module *module;
>>>-	enum dpll_type type;
>>> 	struct xarray pin_refs;
>>> 	refcount_t refcount;
>>> 	struct list_head registration_list;
>>>@@ -84,6 +83,7 @@ void *dpll_pin_on_pin_priv(struct dpll_pin *parent,
>>>struct dpll_pin *pin);
>>> const struct dpll_device_ops *dpll_device_ops(struct dpll_device *dpll);
>>> struct dpll_device *dpll_device_get_by_id(int id);
>>> const struct dpll_pin_ops *dpll_pin_ops(struct dpll_pin_ref *ref);
>>>+const struct dpll_device_info *dpll_device_info(struct dpll_device
>>>*dpll);
>>> struct dpll_pin_ref *dpll_xa_ref_dpll_first(struct xarray *xa_refs);
>>> extern struct xarray dpll_device_xa;
>>> extern struct xarray dpll_pin_xa;
>>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>>index c130f87147fa..2de9ec08d551 100644
>>>--- a/drivers/dpll/dpll_netlink.c
>>>+++ b/drivers/dpll/dpll_netlink.c
>>>@@ -564,6 +564,7 @@ static int
>>> dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
>>> 		    struct netlink_ext_ack *extack)
>>> {
>>>+	const struct dpll_device_info *info = dpll_device_info(dpll);
>>> 	int ret;
>>>
>>> 	ret = dpll_msg_add_dev_handle(msg, dpll);
>>>@@ -589,7 +590,7 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>>>sk_buff *msg,
>>> 	ret = dpll_msg_add_mode_supported(msg, dpll, extack);
>>> 	if (ret)
>>> 		return ret;
>>>-	if (nla_put_u32(msg, DPLL_A_TYPE, dpll->type))
>>>+	if (nla_put_u32(msg, DPLL_A_TYPE, info->type))
>>> 		return -EMSGSIZE;
>>>
>>> 	return 0;
>>>@@ -1415,11 +1416,13 @@ dpll_device_find(u64 clock_id, struct nlattr
>>>*mod_name_attr,
>>> 	unsigned long i;
>>>
>>> 	xa_for_each_marked(&dpll_device_xa, i, dpll, DPLL_REGISTERED) {
>>>+		const struct dpll_device_info *info = dpll_device_info(dpll);
>>>+
>>> 		cid_match = clock_id ? dpll->clock_id == clock_id : true;
>>> 		mod_match = mod_name_attr ? (module_name(dpll->module) ?
>>> 			!nla_strcmp(mod_name_attr,
>>> 				    module_name(dpll->module)) : false) : true;
>>>-		type_match = type ? dpll->type == type : true;
>>>+		type_match = type ? info->type == type : true;
>>> 		if (cid_match && mod_match && type_match) {
>>> 			if (dpll_match) {
>>> 				NL_SET_ERR_MSG(extack, "multiple matches");
>>>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c
>>>b/drivers/net/ethernet/intel/ice/ice_dpll.c
>>>index bce3ad6ca2a6..0f7440a889ac 100644
>>>--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
>>>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
>>>@@ -1977,7 +1977,7 @@ static void
>>> ice_dpll_deinit_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
>>> {
>>> 	if (cgu)
>>>-		dpll_device_unregister(d->dpll, &ice_dpll_ops, d);
>>>+		dpll_device_unregister(d->dpll, &d->info, d);
>>> 	dpll_device_put(d->dpll);
>>> }
>>>
>>>@@ -1996,8 +1996,7 @@ ice_dpll_deinit_dpll(struct ice_pf *pf, struct
>>>ice_dpll *d, bool cgu)
>>>  * * negative - initialization failure reason
>>>  */
>>> static int
>>>-ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu,
>>>-		   enum dpll_type type)
>>>+ice_dpll_init_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
>>> {
>>> 	u64 clock_id = pf->dplls.clock_id;
>>> 	int ret;
>>>@@ -2012,7 +2011,7 @@ ice_dpll_init_dpll(struct ice_pf *pf, struct
>>>ice_dpll *d, bool cgu,
>>> 	d->pf = pf;
>>> 	if (cgu) {
>>> 		ice_dpll_update_state(pf, d, true);
>>>-		ret = dpll_device_register(d->dpll, type, &ice_dpll_ops, d);
>>>+		ret = dpll_device_register(d->dpll, &d->info, d);
>>> 		if (ret) {
>>> 			dpll_device_put(d->dpll);
>>> 			return ret;
>>>@@ -2363,7 +2362,12 @@ static int ice_dpll_init_info(struct ice_pf *pf,
>>>bool cgu)
>>> 	if (ret)
>>> 		return ret;
>>> 	de->mode = DPLL_MODE_AUTOMATIC;
>>>+	de->info.type = DPLL_TYPE_EEC;
>>>+	de->info.ops = &ice_dpll_ops;
>>>+
>>> 	dp->mode = DPLL_MODE_AUTOMATIC;
>>>+	dp->info.type = DPLL_TYPE_PPS;
>>>+	dp->info.ops = &ice_dpll_ops;
>>>
>>> 	dev_dbg(ice_pf_to_dev(pf),
>>> 		"%s - success, inputs:%u, outputs:%u rclk-parents:%u\n",
>>>@@ -2426,10 +2430,10 @@ void ice_dpll_init(struct ice_pf *pf)
>>> 	err = ice_dpll_init_info(pf, cgu);
>>> 	if (err)
>>> 		goto err_exit;
>>>-	err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu, DPLL_TYPE_EEC);
>>>+	err = ice_dpll_init_dpll(pf, &pf->dplls.eec, cgu);
>>> 	if (err)
>>> 		goto deinit_info;
>>>-	err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu, DPLL_TYPE_PPS);
>>>+	err = ice_dpll_init_dpll(pf, &pf->dplls.pps, cgu);
>>> 	if (err)
>>> 		goto deinit_eec;
>>> 	err = ice_dpll_init_pins(pf, cgu);
>>>diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h
>>>b/drivers/net/ethernet/intel/ice/ice_dpll.h
>>>index c320f1bf7d6d..9db7463e293a 100644
>>>--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
>>>+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
>>>@@ -66,6 +66,7 @@ struct ice_dpll {
>>> 	enum dpll_mode mode;
>>> 	struct dpll_pin *active_input;
>>> 	struct dpll_pin *prev_input;
>>>+	struct dpll_device_info info;
>>> };
>>>
>>> /** ice_dplls - store info required for CCU (clock controlling unit)
>>>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>>b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>>index 1e5522a19483..f722b1de0754 100644
>>>--- a/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>>+++ b/drivers/net/ethernet/mellanox/mlx5/core/dpll.c
>>>@@ -20,6 +20,7 @@ struct mlx5_dpll {
>>> 	} last;
>>> 	struct notifier_block mdev_nb;
>>> 	struct net_device *tracking_netdev;
>>>+	struct dpll_device_info info;
>>> };
>>>
>>> static int mlx5_dpll_clock_id_get(struct mlx5_core_dev *mdev, u64
>>>*clock_id)
>>>@@ -444,8 +445,9 @@ static int mlx5_dpll_probe(struct auxiliary_device
>>>*adev,
>>> 		goto err_free_mdpll;
>>> 	}
>>>
>>>-	err = dpll_device_register(mdpll->dpll, DPLL_TYPE_EEC,
>>>-				   &mlx5_dpll_device_ops, mdpll);
>>>+	mdpll->info.type = DPLL_TYPE_EEC;
>>>+	mdpll->info.ops = &mlx5_dpll_device_ops;
>>>+	err = dpll_device_register(mdpll->dpll, &mdpll->info, mdpll);
>>> 	if (err)
>>> 		goto err_put_dpll_device;
>>>
>>>@@ -481,7 +483,7 @@ static int mlx5_dpll_probe(struct auxiliary_device
>>>*adev,
>>> err_put_dpll_pin:
>>> 	dpll_pin_put(mdpll->dpll_pin);
>>> err_unregister_dpll_device:
>>>-	dpll_device_unregister(mdpll->dpll, &mlx5_dpll_device_ops, mdpll);
>>>+	dpll_device_unregister(mdpll->dpll, &mdpll->info, mdpll);
>>> err_put_dpll_device:
>>> 	dpll_device_put(mdpll->dpll);
>>> err_free_mdpll:
>>>@@ -500,7 +502,7 @@ static void mlx5_dpll_remove(struct auxiliary_device
>>>*adev)
>>> 	dpll_pin_unregister(mdpll->dpll, mdpll->dpll_pin,
>>> 			    &mlx5_dpll_pins_ops, mdpll);
>>> 	dpll_pin_put(mdpll->dpll_pin);
>>>-	dpll_device_unregister(mdpll->dpll, &mlx5_dpll_device_ops, mdpll);
>>>+	dpll_device_unregister(mdpll->dpll, &mdpll->info, mdpll);
>>> 	dpll_device_put(mdpll->dpll);
>>> 	kfree(mdpll);
>>>
>>>diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>>index 7945c6be1f7c..b3c5d294acb4 100644
>>>--- a/drivers/ptp/ptp_ocp.c
>>>+++ b/drivers/ptp/ptp_ocp.c
>>>@@ -382,6 +382,7 @@ struct ptp_ocp {
>>> 	struct ptp_ocp_sma_connector sma[OCP_SMA_NUM];
>>> 	const struct ocp_sma_op *sma_op;
>>> 	struct dpll_device *dpll;
>>>+	struct dpll_device_info	dpll_info;
>>> };
>>>
>>> #define OCP_REQ_TIMESTAMP	BIT(0)
>>>@@ -4745,7 +4746,9 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct
>>>pci_device_id *id)
>>> 		goto out;
>>> 	}
>>>
>>>-	err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp);
>>>+	bp->dpll_info.type = DPLL_TYPE_PPS;
>>>+	bp->dpll_info.ops = &dpll_ops;
>>>+	err = dpll_device_register(bp->dpll, &bp->dpll_info, bp);
>>> 	if (err)
>>> 		goto out;
>>>
>>>@@ -4796,7 +4799,7 @@ ptp_ocp_remove(struct pci_dev *pdev)
>>> 			dpll_pin_put(bp->sma[i].dpll_pin);
>>> 		}
>>> 	}
>>>-	dpll_device_unregister(bp->dpll, &dpll_ops, bp);
>>>+	dpll_device_unregister(bp->dpll, &bp->dpll_info, bp);
>>> 	dpll_device_put(bp->dpll);
>>> 	devlink_unregister(devlink);
>>> 	ptp_ocp_detach(bp);
>>>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>>index 5e4f9ab1cf75..0489464af958 100644
>>>--- a/include/linux/dpll.h
>>>+++ b/include/linux/dpll.h
>>>@@ -97,6 +97,11 @@ struct dpll_pin_ops {
>>> 			 struct netlink_ext_ack *extack);
>>> };
>>>
>>>+struct dpll_device_info {
>>>+	enum dpll_type type;
>>>+	const struct dpll_device_ops *ops;
>>>+};
>>>+
>>> struct dpll_pin_frequency {
>>> 	u64 min;
>>> 	u64 max;
>>>@@ -170,11 +175,11 @@ dpll_device_get(u64 clock_id, u32 dev_driver_id,
>>>struct module *module);
>>>
>>> void dpll_device_put(struct dpll_device *dpll);
>>>
>>>-int dpll_device_register(struct dpll_device *dpll, enum dpll_type type,
>>>-			 const struct dpll_device_ops *ops, void *priv);
>>>+int dpll_device_register(struct dpll_device *dpll,
>>>+			 const struct dpll_device_info *info, void *priv);
>>>
>>> void dpll_device_unregister(struct dpll_device *dpll,
>>>-			    const struct dpll_device_ops *ops, void *priv);
>>>+			    const struct dpll_device_info *info, void *priv);
>>>
>>> struct dpll_pin *
>>> dpll_pin_get(u64 clock_id, u32 dev_driver_id, struct module *module,
>>>--
>>>2.38.1
>>>

Re: [PATCH net-next v2 1/4] dpll: use struct dpll_device_info for dpll registration

[Jakub Kicinski]

On Tue, 15 Apr 2025 20:15:40 +0200 Arkadiusz Kubalewski wrote:
> @@ -408,14 +408,14 @@ EXPORT_SYMBOL_GPL(dpll_device_register);
>   * Context: Acquires a lock (dpll_lock)
>   */
>  void dpll_device_unregister(struct dpll_device *dpll,
> -			    const struct dpll_device_ops *ops, void *priv)
> +			    const struct dpll_device_info *info, void *priv)

Some kdoc unhappiness here, W=1 build should lead you to it.

RE: [PATCH net-next v2 1/4] dpll: use struct dpll_device_info for dpll registration

[Kubalewski, Arkadiusz]

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Thursday, April 17, 2025 4:09 AM
>
>On Tue, 15 Apr 2025 20:15:40 +0200 Arkadiusz Kubalewski wrote:
>> @@ -408,14 +408,14 @@ EXPORT_SYMBOL_GPL(dpll_device_register);
>>   * Context: Acquires a lock (dpll_lock)
>>   */
>>  void dpll_device_unregister(struct dpll_device *dpll,
>> -			    const struct dpll_device_ops *ops, void *priv)
>> +			    const struct dpll_device_info *info, void *priv)
>
>Some kdoc unhappiness here, W=1 build should lead you to it.

Sure, will fix it.

Thank you!
Arkadiusz

[PATCH net-next v2 2/4] dpll: add features and capabilities to dpll device spec

[Arkadiusz Kubalewski]

Add infrastructure for adding simple control over dpll device level
features.
Add define for new dpll device level feature:
DPLL_FEATURES_ALL_INPUTS_PHASE_OFFSET_MONITOR - control over monitoring of
all input pins phase offsets.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Milena Olech <milena.olech@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
v2:
- adapt changes and align wiht new dpll_device_info struct
---
 Documentation/netlink/specs/dpll.yaml | 25 +++++++++++++++++++++++++
 drivers/dpll/dpll_nl.c                |  5 +++--
 include/uapi/linux/dpll.h             | 13 +++++++++++++
 3 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/Documentation/netlink/specs/dpll.yaml b/Documentation/netlink/specs/dpll.yaml
index 8feefeae5376..c9a3873e03f6 100644
--- a/Documentation/netlink/specs/dpll.yaml
+++ b/Documentation/netlink/specs/dpll.yaml
@@ -240,6 +240,18 @@ 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: flags
+    name: features
+    doc: |
+      Allow simple control (enable/disable) and status checking over features
+      available per single dpll device.
+    entries:
+      -
+        name: all-inputs-phase-offset-monitor
+        doc: |
+          select if phase offset values are measured and reported for
+          all the input pins available for given dpll device
 
 attribute-sets:
   -
@@ -293,6 +305,16 @@ attribute-sets:
           be put to message multiple times to indicate possible parallel
           quality levels (e.g. one specified by ITU option 1 and another
           one specified by option 2).
+      -
+        name: capabilities
+        type: u32
+        enum: features
+        doc: Features available for a dpll device.
+      -
+        name: features
+        type: u32
+        enum: features
+        doc: Features enabled for a dpll device.
   -
     name: pin
     enum-name: dpll_a_pin
@@ -483,6 +505,8 @@ operations:
             - temp
             - clock-id
             - type
+            - capabilities
+            - features
 
       dump:
         reply: *dev-attrs
@@ -499,6 +523,7 @@ operations:
         request:
           attributes:
             - id
+            - features
     -
       name: device-create-ntf
       doc: Notification about device appearing
diff --git a/drivers/dpll/dpll_nl.c b/drivers/dpll/dpll_nl.c
index fe9b6893d261..3712a693c458 100644
--- a/drivers/dpll/dpll_nl.c
+++ b/drivers/dpll/dpll_nl.c
@@ -37,8 +37,9 @@ static const struct nla_policy dpll_device_get_nl_policy[DPLL_A_ID + 1] = {
 };
 
 /* DPLL_CMD_DEVICE_SET - do */
-static const struct nla_policy dpll_device_set_nl_policy[DPLL_A_ID + 1] = {
+static const struct nla_policy dpll_device_set_nl_policy[DPLL_A_FEATURES + 1] = {
 	[DPLL_A_ID] = { .type = NLA_U32, },
+	[DPLL_A_FEATURES] = NLA_POLICY_MASK(NLA_U32, 0x1),
 };
 
 /* DPLL_CMD_PIN_ID_GET - do */
@@ -105,7 +106,7 @@ static const struct genl_split_ops dpll_nl_ops[] = {
 		.doit		= dpll_nl_device_set_doit,
 		.post_doit	= dpll_post_doit,
 		.policy		= dpll_device_set_nl_policy,
-		.maxattr	= DPLL_A_ID,
+		.maxattr	= DPLL_A_FEATURES,
 		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
 	},
 	{
diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
index bf97d4b6d51f..7c8e929831aa 100644
--- a/include/uapi/linux/dpll.h
+++ b/include/uapi/linux/dpll.h
@@ -192,6 +192,17 @@ enum dpll_pin_capabilities {
 
 #define DPLL_PHASE_OFFSET_DIVIDER	1000
 
+/**
+ * enum dpll_features - Allow simple control (enable/disable) and status
+ *   checking over features available per single dpll device.
+ * @DPLL_FEATURES_ALL_INPUTS_PHASE_OFFSET_MONITOR: select if phase offset
+ *   values are measured and reported for all the input pins available for
+ *   given dpll device
+ */
+enum dpll_features {
+	DPLL_FEATURES_ALL_INPUTS_PHASE_OFFSET_MONITOR = 1,
+};
+
 enum dpll_a {
 	DPLL_A_ID = 1,
 	DPLL_A_MODULE_NAME,
@@ -204,6 +215,8 @@ enum dpll_a {
 	DPLL_A_TYPE,
 	DPLL_A_LOCK_STATUS_ERROR,
 	DPLL_A_CLOCK_QUALITY_LEVEL,
+	DPLL_A_CAPABILITIES,
+	DPLL_A_FEATURES,
 
 	__DPLL_A_MAX,
 	DPLL_A_MAX = (__DPLL_A_MAX - 1)
-- 
2.38.1

[PATCH net-next v2 3/4] dpll: features_get/set callbacks

[Arkadiusz Kubalewski]

Add new callback ops for a dpll device.
- features_get(..) - to obtain currently configured features from dpll
  device,
- feature_set(..) - to allow dpll device features configuration.
Provide features attribute and allow control over it to the users if
device driver implements callbacks.

Reviewed-by: Milena Olech <milena.olech@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
v2:
- adapt changes and align wiht new dpll_device_info struct
---
 drivers/dpll/dpll_netlink.c | 79 ++++++++++++++++++++++++++++++++++++-
 include/linux/dpll.h        |  5 +++
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index 2de9ec08d551..acfdd87fcffe 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -126,6 +126,25 @@ dpll_msg_add_mode_supported(struct sk_buff *msg, struct dpll_device *dpll,
 	return 0;
 }
 
+static int
+dpll_msg_add_features(struct sk_buff *msg, struct dpll_device *dpll,
+		      struct netlink_ext_ack *extack)
+{
+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
+	u32 features;
+	int ret;
+
+	if (!ops->features_get)
+		return 0;
+	ret = ops->features_get(dpll, dpll_priv(dpll), &features, extack);
+	if (ret)
+		return ret;
+	if (nla_put_u32(msg, DPLL_A_FEATURES, features))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
 static int
 dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_device *dpll,
 			 struct netlink_ext_ack *extack)
@@ -592,6 +611,11 @@ dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
 		return ret;
 	if (nla_put_u32(msg, DPLL_A_TYPE, info->type))
 		return -EMSGSIZE;
+	if (nla_put_u32(msg, DPLL_A_CAPABILITIES, info->capabilities))
+		return -EMSGSIZE;
+	ret = dpll_msg_add_features(msg, dpll, extack);
+	if (ret)
+		return ret;
 
 	return 0;
 }
@@ -747,6 +771,34 @@ int dpll_pin_change_ntf(struct dpll_pin *pin)
 }
 EXPORT_SYMBOL_GPL(dpll_pin_change_ntf);
 
+static int
+dpll_features_set(struct dpll_device *dpll, struct nlattr *a,
+		  struct netlink_ext_ack *extack)
+{
+	const struct dpll_device_info *info = dpll_device_info(dpll);
+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
+	u32 features = nla_get_u32(a), old_features;
+	int ret;
+
+	if (features && !(info->capabilities & features)) {
+		NL_SET_ERR_MSG_ATTR(extack, a, "dpll device not capable of this features");
+		return -EOPNOTSUPP;
+	}
+	if (!ops->features_get || !ops->features_set) {
+		NL_SET_ERR_MSG(extack, "dpll device not supporting any features");
+		return -EOPNOTSUPP;
+	}
+	ret = ops->features_get(dpll, dpll_priv(dpll), &old_features, extack);
+	if (ret) {
+		NL_SET_ERR_MSG(extack, "unable to get old features value");
+		return ret;
+	}
+	if (old_features == features)
+		return -EINVAL;
+
+	return ops->features_set(dpll, dpll_priv(dpll), features, extack);
+}
+
 static int
 dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
 		  struct netlink_ext_ack *extack)
@@ -1536,10 +1588,33 @@ int dpll_nl_device_get_doit(struct sk_buff *skb, struct genl_info *info)
 	return genlmsg_reply(msg, info);
 }
 
+static int
+dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info *info)
+{
+	struct nlattr *a;
+	int rem, ret;
+
+	nla_for_each_attr(a, genlmsg_data(info->genlhdr),
+			  genlmsg_len(info->genlhdr), rem) {
+		switch (nla_type(a)) {
+		case DPLL_A_FEATURES:
+			ret = dpll_features_set(dpll, a, info->extack);
+			if (ret)
+				return ret;
+			break;
+		default:
+			break;
+		}
+	}
+
+	return 0;
+}
+
 int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	/* placeholder for set command */
-	return 0;
+	struct dpll_device *dpll = info->user_ptr[0];
+
+	return dpll_set_from_nlattr(dpll, info);
 }
 
 int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
diff --git a/include/linux/dpll.h b/include/linux/dpll.h
index 0489464af958..90c743daf64b 100644
--- a/include/linux/dpll.h
+++ b/include/linux/dpll.h
@@ -30,6 +30,10 @@ struct dpll_device_ops {
 				       void *dpll_priv,
 				       unsigned long *qls,
 				       struct netlink_ext_ack *extack);
+	int (*features_set)(const struct dpll_device *dpll, void *dpll_priv,
+			    u32 features, struct netlink_ext_ack *extack);
+	int (*features_get)(const struct dpll_device *dpll, void *dpll_priv,
+			    u32 *features, struct netlink_ext_ack *extack);
 };
 
 struct dpll_pin_ops {
@@ -99,6 +103,7 @@ struct dpll_pin_ops {
 
 struct dpll_device_info {
 	enum dpll_type type;
+	u32 capabilities;
 	const struct dpll_device_ops *ops;
 };
 
-- 
2.38.1

Re: [PATCH net-next v2 3/4] dpll: features_get/set callbacks

[Jiri Pirko]

Tue, Apr 15, 2025 at 08:15:42PM +0200, arkadiusz.kubalewski@intel.com wrote:
>Add new callback ops for a dpll device.
>- features_get(..) - to obtain currently configured features from dpll
>  device,
>- feature_set(..) - to allow dpll device features configuration.
>Provide features attribute and allow control over it to the users if
>device driver implements callbacks.
>
>Reviewed-by: Milena Olech <milena.olech@intel.com>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>---
>v2:
>- adapt changes and align wiht new dpll_device_info struct
>---
> drivers/dpll/dpll_netlink.c | 79 ++++++++++++++++++++++++++++++++++++-
> include/linux/dpll.h        |  5 +++
> 2 files changed, 82 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index 2de9ec08d551..acfdd87fcffe 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -126,6 +126,25 @@ dpll_msg_add_mode_supported(struct sk_buff *msg, struct dpll_device *dpll,
> 	return 0;
> }
> 
>+static int
>+dpll_msg_add_features(struct sk_buff *msg, struct dpll_device *dpll,
>+		      struct netlink_ext_ack *extack)
>+{
>+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>+	u32 features;
>+	int ret;
>+
>+	if (!ops->features_get)
>+		return 0;
>+	ret = ops->features_get(dpll, dpll_priv(dpll), &features, extack);
>+	if (ret)
>+		return ret;
>+	if (nla_put_u32(msg, DPLL_A_FEATURES, features))
>+		return -EMSGSIZE;
>+
>+	return 0;
>+}
>+
> static int
> dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_device *dpll,
> 			 struct netlink_ext_ack *extack)
>@@ -592,6 +611,11 @@ dpll_device_get_one(struct dpll_device *dpll, struct sk_buff *msg,
> 		return ret;
> 	if (nla_put_u32(msg, DPLL_A_TYPE, info->type))
> 		return -EMSGSIZE;
>+	if (nla_put_u32(msg, DPLL_A_CAPABILITIES, info->capabilities))
>+		return -EMSGSIZE;
>+	ret = dpll_msg_add_features(msg, dpll, extack);
>+	if (ret)
>+		return ret;
> 
> 	return 0;
> }
>@@ -747,6 +771,34 @@ int dpll_pin_change_ntf(struct dpll_pin *pin)
> }
> EXPORT_SYMBOL_GPL(dpll_pin_change_ntf);
> 
>+static int
>+dpll_features_set(struct dpll_device *dpll, struct nlattr *a,
>+		  struct netlink_ext_ack *extack)
>+{
>+	const struct dpll_device_info *info = dpll_device_info(dpll);
>+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>+	u32 features = nla_get_u32(a), old_features;
>+	int ret;
>+
>+	if (features && !(info->capabilities & features)) {
>+		NL_SET_ERR_MSG_ATTR(extack, a, "dpll device not capable of this features");
>+		return -EOPNOTSUPP;
>+	}
>+	if (!ops->features_get || !ops->features_set) {
>+		NL_SET_ERR_MSG(extack, "dpll device not supporting any features");
>+		return -EOPNOTSUPP;
>+	}
>+	ret = ops->features_get(dpll, dpll_priv(dpll), &old_features, extack);
>+	if (ret) {
>+		NL_SET_ERR_MSG(extack, "unable to get old features value");
>+		return ret;
>+	}
>+	if (old_features == features)
>+		return -EINVAL;
>+
>+	return ops->features_set(dpll, dpll_priv(dpll), features, extack);

So you allow to enable/disable them all in once. What if user want to
enable feature A and does not care about feature B that may of may not
be previously set?
How many of the features do you expect to appear in the future. I'm
asking because this could be just a bool attr with a separate op to the
driver. If we have 3, no problem. Benefit is, you may also extend it
easily to pass some non-bool configuration. My point is, what is the
benefit of features bitset here?



>+}
>+
> static int
> dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
> 		  struct netlink_ext_ack *extack)
>@@ -1536,10 +1588,33 @@ int dpll_nl_device_get_doit(struct sk_buff *skb, struct genl_info *info)
> 	return genlmsg_reply(msg, info);
> }
> 
>+static int
>+dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info *info)
>+{
>+	struct nlattr *a;
>+	int rem, ret;
>+
>+	nla_for_each_attr(a, genlmsg_data(info->genlhdr),
>+			  genlmsg_len(info->genlhdr), rem) {
>+		switch (nla_type(a)) {
>+		case DPLL_A_FEATURES:
>+			ret = dpll_features_set(dpll, a, info->extack);
>+			if (ret)
>+				return ret;
>+			break;
>+		default:
>+			break;
>+		}
>+	}
>+
>+	return 0;
>+}
>+
> int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info)
> {
>-	/* placeholder for set command */
>-	return 0;
>+	struct dpll_device *dpll = info->user_ptr[0];
>+
>+	return dpll_set_from_nlattr(dpll, info);
> }
> 
> int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>index 0489464af958..90c743daf64b 100644
>--- a/include/linux/dpll.h
>+++ b/include/linux/dpll.h
>@@ -30,6 +30,10 @@ struct dpll_device_ops {
> 				       void *dpll_priv,
> 				       unsigned long *qls,
> 				       struct netlink_ext_ack *extack);
>+	int (*features_set)(const struct dpll_device *dpll, void *dpll_priv,
>+			    u32 features, struct netlink_ext_ack *extack);
>+	int (*features_get)(const struct dpll_device *dpll, void *dpll_priv,
>+			    u32 *features, struct netlink_ext_ack *extack);
> };
> 
> struct dpll_pin_ops {
>@@ -99,6 +103,7 @@ struct dpll_pin_ops {
> 
> struct dpll_device_info {
> 	enum dpll_type type;
>+	u32 capabilities;
> 	const struct dpll_device_ops *ops;
> };
> 
>-- 
>2.38.1
>

RE: [PATCH net-next v2 3/4] dpll: features_get/set callbacks

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Wednesday, April 16, 2025 2:11 PM
>
>Tue, Apr 15, 2025 at 08:15:42PM +0200, arkadiusz.kubalewski@intel.com
>wrote:
>>Add new callback ops for a dpll device.
>>- features_get(..) - to obtain currently configured features from dpll
>>  device,
>>- feature_set(..) - to allow dpll device features configuration.
>>Provide features attribute and allow control over it to the users if
>>device driver implements callbacks.
>>
>>Reviewed-by: Milena Olech <milena.olech@intel.com>
>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>---
>>v2:
>>- adapt changes and align wiht new dpll_device_info struct
>>---
>> drivers/dpll/dpll_netlink.c | 79 ++++++++++++++++++++++++++++++++++++-
>> include/linux/dpll.h        |  5 +++
>> 2 files changed, 82 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>index 2de9ec08d551..acfdd87fcffe 100644
>>--- a/drivers/dpll/dpll_netlink.c
>>+++ b/drivers/dpll/dpll_netlink.c
>>@@ -126,6 +126,25 @@ dpll_msg_add_mode_supported(struct sk_buff *msg,
>>struct dpll_device *dpll,
>> 	return 0;
>> }
>>
>>+static int
>>+dpll_msg_add_features(struct sk_buff *msg, struct dpll_device *dpll,
>>+		      struct netlink_ext_ack *extack)
>>+{
>>+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>+	u32 features;
>>+	int ret;
>>+
>>+	if (!ops->features_get)
>>+		return 0;
>>+	ret = ops->features_get(dpll, dpll_priv(dpll), &features, extack);
>>+	if (ret)
>>+		return ret;
>>+	if (nla_put_u32(msg, DPLL_A_FEATURES, features))
>>+		return -EMSGSIZE;
>>+
>>+	return 0;
>>+}
>>+
>> static int
>> dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_device *dpll,
>> 			 struct netlink_ext_ack *extack)
>>@@ -592,6 +611,11 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>>sk_buff *msg,
>> 		return ret;
>> 	if (nla_put_u32(msg, DPLL_A_TYPE, info->type))
>> 		return -EMSGSIZE;
>>+	if (nla_put_u32(msg, DPLL_A_CAPABILITIES, info->capabilities))
>>+		return -EMSGSIZE;
>>+	ret = dpll_msg_add_features(msg, dpll, extack);
>>+	if (ret)
>>+		return ret;
>>
>> 	return 0;
>> }
>>@@ -747,6 +771,34 @@ int dpll_pin_change_ntf(struct dpll_pin *pin)
>> }
>> EXPORT_SYMBOL_GPL(dpll_pin_change_ntf);
>>
>>+static int
>>+dpll_features_set(struct dpll_device *dpll, struct nlattr *a,
>>+		  struct netlink_ext_ack *extack)
>>+{
>>+	const struct dpll_device_info *info = dpll_device_info(dpll);
>>+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>+	u32 features = nla_get_u32(a), old_features;
>>+	int ret;
>>+
>>+	if (features && !(info->capabilities & features)) {
>>+		NL_SET_ERR_MSG_ATTR(extack, a, "dpll device not capable of this
>>features");
>>+		return -EOPNOTSUPP;
>>+	}
>>+	if (!ops->features_get || !ops->features_set) {
>>+		NL_SET_ERR_MSG(extack, "dpll device not supporting any
>>features");
>>+		return -EOPNOTSUPP;
>>+	}
>>+	ret = ops->features_get(dpll, dpll_priv(dpll), &old_features,
>>extack);
>>+	if (ret) {
>>+		NL_SET_ERR_MSG(extack, "unable to get old features value");
>>+		return ret;
>>+	}
>>+	if (old_features == features)
>>+		return -EINVAL;
>>+
>>+	return ops->features_set(dpll, dpll_priv(dpll), features, extack);
>
>So you allow to enable/disable them all in once. What if user want to
>enable feature A and does not care about feature B that may of may not
>be previously set?

Assumed that user would always request full set.

>How many of the features do you expect to appear in the future. I'm
>asking because this could be just a bool attr with a separate op to the
>driver. If we have 3, no problem. Benefit is, you may also extend it
>easily to pass some non-bool configuration. My point is, what is the
>benefit of features bitset here?
>

Not much, at least right now..
Maybe one similar in nearest feature. Sure, we can go that way.

But you mean uAPI part also have this enabled somehow per feature or
leave the feature bits there?

Thank you!
Arkadiusz

>
>
>>+}
>>+
>> static int
>> dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
>> 		  struct netlink_ext_ack *extack)
>>@@ -1536,10 +1588,33 @@ int dpll_nl_device_get_doit(struct sk_buff *skb,
>struct genl_info *info)
>> 	return genlmsg_reply(msg, info);
>> }
>>
>>+static int
>>+dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info *info)
>>+{
>>+	struct nlattr *a;
>>+	int rem, ret;
>>+
>>+	nla_for_each_attr(a, genlmsg_data(info->genlhdr),
>>+			  genlmsg_len(info->genlhdr), rem) {
>>+		switch (nla_type(a)) {
>>+		case DPLL_A_FEATURES:
>>+			ret = dpll_features_set(dpll, a, info->extack);
>>+			if (ret)
>>+				return ret;
>>+			break;
>>+		default:
>>+			break;
>>+		}
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>> int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info)
>> {
>>-	/* placeholder for set command */
>>-	return 0;
>>+	struct dpll_device *dpll = info->user_ptr[0];
>>+
>>+	return dpll_set_from_nlattr(dpll, info);
>> }
>>
>> int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct
>>netlink_callback *cb)
>>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>index 0489464af958..90c743daf64b 100644
>>--- a/include/linux/dpll.h
>>+++ b/include/linux/dpll.h
>>@@ -30,6 +30,10 @@ struct dpll_device_ops {
>> 				       void *dpll_priv,
>> 				       unsigned long *qls,
>> 				       struct netlink_ext_ack *extack);
>>+	int (*features_set)(const struct dpll_device *dpll, void *dpll_priv,
>>+			    u32 features, struct netlink_ext_ack *extack);
>>+	int (*features_get)(const struct dpll_device *dpll, void *dpll_priv,
>>+			    u32 *features, struct netlink_ext_ack *extack);
>> };
>>
>> struct dpll_pin_ops {
>>@@ -99,6 +103,7 @@ struct dpll_pin_ops {
>>
>> struct dpll_device_info {
>> 	enum dpll_type type;
>>+	u32 capabilities;
>> 	const struct dpll_device_ops *ops;
>> };
>>
>>--
>>2.38.1
>>

Re: [PATCH net-next v2 3/4] dpll: features_get/set callbacks

[Jiri Pirko]

Thu, Apr 17, 2025 at 11:23:09AM +0200, arkadiusz.kubalewski@intel.com wrote:
>>From: Jiri Pirko <jiri@resnulli.us>
>>Sent: Wednesday, April 16, 2025 2:11 PM
>>
>>Tue, Apr 15, 2025 at 08:15:42PM +0200, arkadiusz.kubalewski@intel.com
>>wrote:
>>>Add new callback ops for a dpll device.
>>>- features_get(..) - to obtain currently configured features from dpll
>>>  device,
>>>- feature_set(..) - to allow dpll device features configuration.
>>>Provide features attribute and allow control over it to the users if
>>>device driver implements callbacks.
>>>
>>>Reviewed-by: Milena Olech <milena.olech@intel.com>
>>>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>>>---
>>>v2:
>>>- adapt changes and align wiht new dpll_device_info struct
>>>---
>>> drivers/dpll/dpll_netlink.c | 79 ++++++++++++++++++++++++++++++++++++-
>>> include/linux/dpll.h        |  5 +++
>>> 2 files changed, 82 insertions(+), 2 deletions(-)
>>>
>>>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>>index 2de9ec08d551..acfdd87fcffe 100644
>>>--- a/drivers/dpll/dpll_netlink.c
>>>+++ b/drivers/dpll/dpll_netlink.c
>>>@@ -126,6 +126,25 @@ dpll_msg_add_mode_supported(struct sk_buff *msg,
>>>struct dpll_device *dpll,
>>> 	return 0;
>>> }
>>>
>>>+static int
>>>+dpll_msg_add_features(struct sk_buff *msg, struct dpll_device *dpll,
>>>+		      struct netlink_ext_ack *extack)
>>>+{
>>>+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>>+	u32 features;
>>>+	int ret;
>>>+
>>>+	if (!ops->features_get)
>>>+		return 0;
>>>+	ret = ops->features_get(dpll, dpll_priv(dpll), &features, extack);
>>>+	if (ret)
>>>+		return ret;
>>>+	if (nla_put_u32(msg, DPLL_A_FEATURES, features))
>>>+		return -EMSGSIZE;
>>>+
>>>+	return 0;
>>>+}
>>>+
>>> static int
>>> dpll_msg_add_lock_status(struct sk_buff *msg, struct dpll_device *dpll,
>>> 			 struct netlink_ext_ack *extack)
>>>@@ -592,6 +611,11 @@ dpll_device_get_one(struct dpll_device *dpll, struct
>>>sk_buff *msg,
>>> 		return ret;
>>> 	if (nla_put_u32(msg, DPLL_A_TYPE, info->type))
>>> 		return -EMSGSIZE;
>>>+	if (nla_put_u32(msg, DPLL_A_CAPABILITIES, info->capabilities))
>>>+		return -EMSGSIZE;
>>>+	ret = dpll_msg_add_features(msg, dpll, extack);
>>>+	if (ret)
>>>+		return ret;
>>>
>>> 	return 0;
>>> }
>>>@@ -747,6 +771,34 @@ int dpll_pin_change_ntf(struct dpll_pin *pin)
>>> }
>>> EXPORT_SYMBOL_GPL(dpll_pin_change_ntf);
>>>
>>>+static int
>>>+dpll_features_set(struct dpll_device *dpll, struct nlattr *a,
>>>+		  struct netlink_ext_ack *extack)
>>>+{
>>>+	const struct dpll_device_info *info = dpll_device_info(dpll);
>>>+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>>+	u32 features = nla_get_u32(a), old_features;
>>>+	int ret;
>>>+
>>>+	if (features && !(info->capabilities & features)) {
>>>+		NL_SET_ERR_MSG_ATTR(extack, a, "dpll device not capable of this
>>>features");
>>>+		return -EOPNOTSUPP;
>>>+	}
>>>+	if (!ops->features_get || !ops->features_set) {
>>>+		NL_SET_ERR_MSG(extack, "dpll device not supporting any
>>>features");
>>>+		return -EOPNOTSUPP;
>>>+	}
>>>+	ret = ops->features_get(dpll, dpll_priv(dpll), &old_features,
>>>extack);
>>>+	if (ret) {
>>>+		NL_SET_ERR_MSG(extack, "unable to get old features value");
>>>+		return ret;
>>>+	}
>>>+	if (old_features == features)
>>>+		return -EINVAL;
>>>+
>>>+	return ops->features_set(dpll, dpll_priv(dpll), features, extack);
>>
>>So you allow to enable/disable them all in once. What if user want to
>>enable feature A and does not care about feature B that may of may not
>>be previously set?
>
>Assumed that user would always request full set.

Ugh.


>
>>How many of the features do you expect to appear in the future. I'm
>>asking because this could be just a bool attr with a separate op to the
>>driver. If we have 3, no problem. Benefit is, you may also extend it
>>easily to pass some non-bool configuration. My point is, what is the
>>benefit of features bitset here?
>>
>
>Not much, at least right now..
>Maybe one similar in nearest feature. Sure, we can go that way.
>
>But you mean uAPI part also have this enabled somehow per feature or
>leave the feature bits there?

I don't see reason for u32/bitfield32 bits here. Just have attr per
feature to enable/disable it, no problem. It will be easier to work with.


>
>Thank you!
>Arkadiusz
>
>>
>>
>>>+}
>>>+
>>> static int
>>> dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
>>> 		  struct netlink_ext_ack *extack)
>>>@@ -1536,10 +1588,33 @@ int dpll_nl_device_get_doit(struct sk_buff *skb,
>>struct genl_info *info)
>>> 	return genlmsg_reply(msg, info);
>>> }
>>>
>>>+static int
>>>+dpll_set_from_nlattr(struct dpll_device *dpll, struct genl_info *info)
>>>+{
>>>+	struct nlattr *a;
>>>+	int rem, ret;
>>>+
>>>+	nla_for_each_attr(a, genlmsg_data(info->genlhdr),
>>>+			  genlmsg_len(info->genlhdr), rem) {
>>>+		switch (nla_type(a)) {
>>>+		case DPLL_A_FEATURES:
>>>+			ret = dpll_features_set(dpll, a, info->extack);
>>>+			if (ret)
>>>+				return ret;
>>>+			break;
>>>+		default:
>>>+			break;
>>>+		}
>>>+	}
>>>+
>>>+	return 0;
>>>+}
>>>+
>>> int dpll_nl_device_set_doit(struct sk_buff *skb, struct genl_info *info)
>>> {
>>>-	/* placeholder for set command */
>>>-	return 0;
>>>+	struct dpll_device *dpll = info->user_ptr[0];
>>>+
>>>+	return dpll_set_from_nlattr(dpll, info);
>>> }
>>>
>>> int dpll_nl_device_get_dumpit(struct sk_buff *skb, struct
>>>netlink_callback *cb)
>>>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>>index 0489464af958..90c743daf64b 100644
>>>--- a/include/linux/dpll.h
>>>+++ b/include/linux/dpll.h
>>>@@ -30,6 +30,10 @@ struct dpll_device_ops {
>>> 				       void *dpll_priv,
>>> 				       unsigned long *qls,
>>> 				       struct netlink_ext_ack *extack);
>>>+	int (*features_set)(const struct dpll_device *dpll, void *dpll_priv,
>>>+			    u32 features, struct netlink_ext_ack *extack);
>>>+	int (*features_get)(const struct dpll_device *dpll, void *dpll_priv,
>>>+			    u32 *features, struct netlink_ext_ack *extack);
>>> };
>>>
>>> struct dpll_pin_ops {
>>>@@ -99,6 +103,7 @@ struct dpll_pin_ops {
>>>
>>> struct dpll_device_info {
>>> 	enum dpll_type type;
>>>+	u32 capabilities;
>>> 	const struct dpll_device_ops *ops;
>>> };
>>>
>>>--
>>>2.38.1
>>>

RE: [PATCH net-next v2 3/4] dpll: features_get/set callbacks

[Kubalewski, Arkadiusz]

>From: Jiri Pirko <jiri@resnulli.us>
>Sent: Thursday, April 17, 2025 11:59 AM

[...]

>>>>+static int
>>>>+dpll_features_set(struct dpll_device *dpll, struct nlattr *a,
>>>>+		  struct netlink_ext_ack *extack)
>>>>+{
>>>>+	const struct dpll_device_info *info = dpll_device_info(dpll);
>>>>+	const struct dpll_device_ops *ops = dpll_device_ops(dpll);
>>>>+	u32 features = nla_get_u32(a), old_features;
>>>>+	int ret;
>>>>+
>>>>+	if (features && !(info->capabilities & features)) {
>>>>+		NL_SET_ERR_MSG_ATTR(extack, a, "dpll device not capable of this
>>>>features");
>>>>+		return -EOPNOTSUPP;
>>>>+	}
>>>>+	if (!ops->features_get || !ops->features_set) {
>>>>+		NL_SET_ERR_MSG(extack, "dpll device not supporting any
>>>>features");
>>>>+		return -EOPNOTSUPP;
>>>>+	}
>>>>+	ret = ops->features_get(dpll, dpll_priv(dpll), &old_features,
>>>>extack);
>>>>+	if (ret) {
>>>>+		NL_SET_ERR_MSG(extack, "unable to get old features value");
>>>>+		return ret;
>>>>+	}
>>>>+	if (old_features == features)
>>>>+		return -EINVAL;
>>>>+
>>>>+	return ops->features_set(dpll, dpll_priv(dpll), features, extack);
>>>
>>>So you allow to enable/disable them all in once. What if user want to
>>>enable feature A and does not care about feature B that may of may not
>>>be previously set?
>>
>>Assumed that user would always request full set.
>
>Ugh.
>
>
>>
>>>How many of the features do you expect to appear in the future. I'm
>>>asking because this could be just a bool attr with a separate op to the
>>>driver. If we have 3, no problem. Benefit is, you may also extend it
>>>easily to pass some non-bool configuration. My point is, what is the
>>>benefit of features bitset here?
>>>
>>
>>Not much, at least right now..
>>Maybe one similar in nearest feature. Sure, we can go that way.
>>
>>But you mean uAPI part also have this enabled somehow per feature or
>>leave the feature bits there?
>
>I don't see reason for u32/bitfield32 bits here. Just have attr per
>feature to enable/disable it, no problem. It will be easier to work with.
>
>

OK. Fixed in v3.

Thank you!
Arkadiusz

[...]

[PATCH net-next v2 4/4] ice: add phase offset monitor for all PPS dpll inputs

[Arkadiusz Kubalewski]

Implement new admin command and helper function to handle/obtain CGU
measurements for input pins, initialize PPS dpll capabilities using new
command.
Add callbacks to control dpll device feature:
all-inputs-phase-offset-monitor.
Allow enable/disable of all inputs monitoring for ice PPS dpll device.
If feature is enabled provide users with measured phase offsets and
notifications.

Reviewed-by: Milena Olech <milena.olech@intel.com>
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
v2:
- adapt changes and align wiht new dpll_device_info struct
---
 .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  20 ++
 drivers/net/ethernet/intel/ice/ice_common.c   |  26 +++
 drivers/net/ethernet/intel/ice/ice_common.h   |   3 +
 drivers/net/ethernet/intel/ice/ice_dpll.c     | 179 +++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_dpll.h     |   6 +
 drivers/net/ethernet/intel/ice/ice_main.c     |   4 +
 6 files changed, 237 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
index bdee499f991a..181bc2c3b4ad 100644
--- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
+++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
@@ -2272,6 +2272,22 @@ struct ice_aqc_get_pkg_info_resp {
 	struct ice_aqc_get_pkg_info pkg_info[];
 };
 
+#define ICE_CGU_INPUT_PHASE_OFFSET_BYTES	6
+
+struct ice_cgu_input_measure {
+	u8 phase_offset[ICE_CGU_INPUT_PHASE_OFFSET_BYTES];
+	__le32 freq;
+} __packed __aligned(sizeof(__le16));
+
+#define ICE_AQC_GET_CGU_IN_MEAS_DPLL_IDX_M	ICE_M(0xf, 0)
+
+/* Get CGU input measurements command response data structure (indirect 0x0C59) */
+struct ice_aqc_get_cgu_input_measure {
+	u8 dpll_idx_opt;
+	u8 length;
+	u8 rsvd[6];
+};
+
 #define ICE_AQC_GET_CGU_MAX_PHASE_ADJ	GENMASK(30, 0)
 
 /* Get CGU abilities command response data structure (indirect 0x0C61) */
@@ -2721,6 +2737,7 @@ struct ice_aq_desc {
 		struct ice_aqc_add_get_update_free_vsi vsi_cmd;
 		struct ice_aqc_add_update_free_vsi_resp add_update_free_vsi_res;
 		struct ice_aqc_download_pkg download_pkg;
+		struct ice_aqc_get_cgu_input_measure get_cgu_input_measure;
 		struct ice_aqc_set_cgu_input_config set_cgu_input_config;
 		struct ice_aqc_get_cgu_input_config get_cgu_input_config;
 		struct ice_aqc_set_cgu_output_config set_cgu_output_config;
@@ -2772,6 +2789,8 @@ enum ice_aq_err {
 	ICE_AQ_RC_OK		= 0,  /* Success */
 	ICE_AQ_RC_EPERM		= 1,  /* Operation not permitted */
 	ICE_AQ_RC_ENOENT	= 2,  /* No such element */
+	ICE_AQ_RC_ESRCH		= 3,  /* Bad opcode */
+	ICE_AQ_RC_EAGAIN	= 8,  /* Try again */
 	ICE_AQ_RC_ENOMEM	= 9,  /* Out of memory */
 	ICE_AQ_RC_EBUSY		= 12, /* Device or resource busy */
 	ICE_AQ_RC_EEXIST	= 13, /* Object already exists */
@@ -2927,6 +2946,7 @@ enum ice_adminq_opc {
 	ice_aqc_opc_get_pkg_info_list			= 0x0C43,
 
 	/* 1588/SyncE commands/events */
+	ice_aqc_opc_get_cgu_input_measure		= 0x0C59,
 	ice_aqc_opc_get_cgu_abilities			= 0x0C61,
 	ice_aqc_opc_set_cgu_input_config		= 0x0C62,
 	ice_aqc_opc_get_cgu_input_config		= 0x0C63,
diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
index 4fedf0181c4e..48ff515d7c61 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.c
+++ b/drivers/net/ethernet/intel/ice/ice_common.c
@@ -4970,6 +4970,32 @@ ice_dis_vsi_rdma_qset(struct ice_port_info *pi, u16 count, u32 *qset_teid,
 	return status;
 }
 
+/**
+ * ice_aq_get_cgu_input_pin_measure - get input pin signal measurements
+ * @hw: pointer to the HW struct
+ * @dpll_idx: index of dpll to be measured
+ * @meas: array to be filled with results
+ * @meas_num: max number of results array can hold
+ *
+ * Get CGU measurements (0x0C59) of phase and frequency offsets for input
+ * pins on given dpll.
+ *
+ * Return: 0 on success or negative value on failure.
+ */
+int ice_aq_get_cgu_input_pin_measure(struct ice_hw *hw, u8 dpll_idx,
+				     struct ice_cgu_input_measure *meas,
+				     u16 meas_num)
+{
+	struct ice_aqc_get_cgu_input_measure *cmd;
+	struct ice_aq_desc desc;
+
+	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_get_cgu_input_measure);
+	cmd = &desc.params.get_cgu_input_measure;
+	cmd->dpll_idx_opt = dpll_idx & ICE_AQC_GET_CGU_IN_MEAS_DPLL_IDX_M;
+
+	return ice_aq_send_cmd(hw, &desc, meas, meas_num * sizeof(*meas), NULL);
+}
+
 /**
  * ice_aq_get_cgu_abilities - get cgu abilities
  * @hw: pointer to the HW struct
diff --git a/drivers/net/ethernet/intel/ice/ice_common.h b/drivers/net/ethernet/intel/ice/ice_common.h
index 64c530b39191..c70f56d897dc 100644
--- a/drivers/net/ethernet/intel/ice/ice_common.h
+++ b/drivers/net/ethernet/intel/ice/ice_common.h
@@ -229,6 +229,9 @@ void ice_replay_post(struct ice_hw *hw);
 struct ice_q_ctx *
 ice_get_lan_q_ctx(struct ice_hw *hw, u16 vsi_handle, u8 tc, u16 q_handle);
 int ice_sbq_rw_reg(struct ice_hw *hw, struct ice_sbq_msg_input *in, u16 flag);
+int ice_aq_get_cgu_input_pin_measure(struct ice_hw *hw, u8 dpll_idx,
+				     struct ice_cgu_input_measure *meas,
+				     u16 meas_num);
 int
 ice_aq_get_cgu_abilities(struct ice_hw *hw,
 			 struct ice_aqc_get_cgu_abilities *abilities);
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
index 0f7440a889ac..b9363230f6ac 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.c
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.c
@@ -11,6 +11,8 @@
 #define ICE_DPLL_RCLK_NUM_PER_PF		1
 #define ICE_DPLL_PIN_ESYNC_PULSE_HIGH_PERCENT	25
 #define ICE_DPLL_PIN_GEN_RCLK_FREQ		1953125
+#define ICE_DPLL_INPUT_REF_NUM			10
+#define ICE_DPLL_PHASE_OFFSET_PERIOD		2
 
 /**
  * enum ice_dpll_pin_type - enumerate ice pin types:
@@ -587,6 +589,63 @@ static int ice_dpll_mode_get(const struct dpll_device *dpll, void *dpll_priv,
 	return 0;
 }
 
+/**
+ * ice_dpll_features_set - set dpll's features
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @features: mask of features to be set
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Enable/disable features of dpll.
+ *
+ * Context: Acquires and releases pf->dplls.lock
+ * Return: 0 - success
+ */
+static int ice_dpll_features_set(const struct dpll_device *dpll,
+				 void *dpll_priv, u32 features,
+				 struct netlink_ext_ack *extack)
+{
+	struct ice_dpll *d = dpll_priv;
+	struct ice_pf *pf = d->pf;
+
+	mutex_lock(&pf->dplls.lock);
+	if (features & DPLL_FEATURES_ALL_INPUTS_PHASE_OFFSET_MONITOR)
+		d->phase_offset_monitor_period = ICE_DPLL_PHASE_OFFSET_PERIOD;
+	else
+		d->phase_offset_monitor_period = 0;
+	mutex_unlock(&pf->dplls.lock);
+
+	return 0;
+}
+
+/**
+ * ice_dpll_features_get - get dpll's features
+ * @dpll: registered dpll pointer
+ * @dpll_priv: private data pointer passed on dpll registration
+ * @features: on success holds currently enabled features of dpll
+ * @extack: error reporting
+ *
+ * Dpll subsystem callback. Provides currently enabled features of dpll.
+ *
+ * Context: Acquires and releases pf->dplls.lock
+ * Return: 0 - success
+ */
+static int ice_dpll_features_get(const struct dpll_device *dpll,
+				 void *dpll_priv, u32 *features,
+				 struct netlink_ext_ack *extack)
+{
+	struct ice_dpll *d = dpll_priv;
+	struct ice_pf *pf = d->pf;
+
+	mutex_lock(&pf->dplls.lock);
+	*features = 0;
+	if (d->phase_offset_monitor_period)
+		*features |= DPLL_FEATURES_ALL_INPUTS_PHASE_OFFSET_MONITOR;
+	mutex_unlock(&pf->dplls.lock);
+
+	return 0;
+}
+
 /**
  * ice_dpll_pin_state_set - set pin's state on dpll
  * @pin: pointer to a pin
@@ -1093,12 +1152,15 @@ ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
 			  const struct dpll_device *dpll, void *dpll_priv,
 			  s64 *phase_offset, struct netlink_ext_ack *extack)
 {
+	struct ice_dpll_pin *p = pin_priv;
 	struct ice_dpll *d = dpll_priv;
 	struct ice_pf *pf = d->pf;
 
 	mutex_lock(&pf->dplls.lock);
 	if (d->active_input == pin)
 		*phase_offset = d->phase_offset * ICE_DPLL_PHASE_OFFSET_FACTOR;
+	else if (d->phase_offset_monitor_period)
+		*phase_offset = p->phase_offset * ICE_DPLL_PHASE_OFFSET_FACTOR;
 	else
 		*phase_offset = 0;
 	mutex_unlock(&pf->dplls.lock);
@@ -1457,6 +1519,8 @@ static const struct dpll_pin_ops ice_dpll_output_ops = {
 static const struct dpll_device_ops ice_dpll_ops = {
 	.lock_status_get = ice_dpll_lock_status_get,
 	.mode_get = ice_dpll_mode_get,
+	.features_set = ice_dpll_features_set,
+	.features_get = ice_dpll_features_get,
 };
 
 /**
@@ -1503,6 +1567,110 @@ static void ice_dpll_notify_changes(struct ice_dpll *d)
 	}
 }
 
+/**
+ * ice_dpll_is_pps_phase_monitor - check if dpll capable of phase offset monitor
+ * @pf: pf private structure
+ *
+ * Check if firmware is capable of supporting admin command to provide
+ * phase offset monitoring on all the input pins on PPS dpll.
+ *
+ * Returns:
+ * * true - PPS dpll phase offset monitoring is supported
+ * * false - PPS dpll phase offset monitoring is not supported
+ */
+static bool ice_dpll_is_pps_phase_monitor(struct ice_pf *pf)
+{
+	struct ice_cgu_input_measure meas[ICE_DPLL_INPUT_REF_NUM];
+	int ret = ice_aq_get_cgu_input_pin_measure(&pf->hw, DPLL_TYPE_PPS, meas,
+						   ARRAY_SIZE(meas));
+
+	if (ret && pf->hw.adminq.sq_last_status == ICE_AQ_RC_ESRCH)
+		return false;
+
+	return true;
+}
+
+/**
+ * ice_dpll_pins_notify_mask - notify dpll subsystem about bulk pin changes
+ * @pins: array of ice_dpll_pin pointers registered within dpll subsystem
+ * @pin_num: number of pins
+ * @phase_offset_ntf_mask: bitmask of pin indexes to notify
+ *
+ * Iterate over array of pins and call dpll subsystem pin notify if
+ * corresponding pin index within bitmask is set.
+ *
+ * Context: Must be called while pf->dplls.lock is released.
+ */
+static void ice_dpll_pins_notify_mask(struct ice_dpll_pin *pins,
+				      u8 pin_num,
+				      u32 phase_offset_ntf_mask)
+{
+	int i = 0;
+
+	for (i = 0; i < pin_num; i++)
+		if (phase_offset_ntf_mask & (1 << i))
+			dpll_pin_change_ntf(pins[i].pin);
+}
+
+/**
+ * ice_dpll_pps_update_phase_offsets - update phase offset measurements
+ * @pf: pf private structure
+ * @phase_offset_pins_updated: returns mask of updated input pin indexes
+ *
+ * Read phase offset measurements for PPS dpll device and store values in
+ * input pins array. On success phase_offset_pins_updated - fills bitmask of
+ * updated input pin indexes, pins shall be notified.
+ *
+ * Context: Shall be called with pf->dplls.lock being locked.
+ * Returns:
+ * * 0 - success or no data available
+ * * negative - AQ failure
+ */
+static int ice_dpll_pps_update_phase_offsets(struct ice_pf *pf,
+					     u32 *phase_offset_pins_updated)
+{
+	struct ice_cgu_input_measure meas[ICE_DPLL_INPUT_REF_NUM];
+	struct ice_dpll_pin *p;
+	s64 phase_offset, tmp;
+	int i, j, ret;
+
+	*phase_offset_pins_updated = 0;
+	ret = ice_aq_get_cgu_input_pin_measure(&pf->hw, DPLL_TYPE_PPS, meas,
+					       ARRAY_SIZE(meas));
+	if (ret && pf->hw.adminq.sq_last_status == ICE_AQ_RC_EAGAIN) {
+		return 0;
+	} else if (ret) {
+		dev_err(ice_pf_to_dev(pf),
+			"failed to get input pin measurements dpll=%d, ret=%d %s\n",
+			DPLL_TYPE_PPS, ret,
+			ice_aq_str(pf->hw.adminq.sq_last_status));
+		return ret;
+	}
+	for (i = 0; i < pf->dplls.num_inputs; i++) {
+		p = &pf->dplls.inputs[i];
+		phase_offset = 0;
+		for (j = 0; j < ICE_CGU_INPUT_PHASE_OFFSET_BYTES; j++) {
+			tmp = meas[i].phase_offset[j];
+#ifdef __LITTLE_ENDIAN
+			phase_offset += tmp << 8 * j;
+#else
+			phase_offset += tmp << 8 *
+				(ICE_CGU_INPUT_PHASE_OFFSET_BYTES - 1 - j);
+#endif
+		}
+		phase_offset = sign_extend64(phase_offset, 47);
+		if (p->phase_offset != phase_offset) {
+			dev_dbg(ice_pf_to_dev(pf),
+				"phase offset changed for pin:%d old:%llx, new:%llx\n",
+				p->idx, p->phase_offset, phase_offset);
+			p->phase_offset = phase_offset;
+			*phase_offset_pins_updated |= (1 << i);
+		}
+	}
+
+	return 0;
+}
+
 /**
  * ice_dpll_update_state - update dpll state
  * @pf: pf private structure
@@ -1589,14 +1757,19 @@ static void ice_dpll_periodic_work(struct kthread_work *work)
 	struct ice_pf *pf = container_of(d, struct ice_pf, dplls);
 	struct ice_dpll *de = &pf->dplls.eec;
 	struct ice_dpll *dp = &pf->dplls.pps;
+	u32 phase_offset_ntf = 0;
 	int ret = 0;
 
 	if (ice_is_reset_in_progress(pf->state))
 		goto resched;
 	mutex_lock(&pf->dplls.lock);
+	d->periodic_counter++;
 	ret = ice_dpll_update_state(pf, de, false);
 	if (!ret)
 		ret = ice_dpll_update_state(pf, dp, false);
+	if (!ret && dp->phase_offset_monitor_period &&
+	    d->periodic_counter % dp->phase_offset_monitor_period == 0)
+		ret = ice_dpll_pps_update_phase_offsets(pf, &phase_offset_ntf);
 	if (ret) {
 		d->cgu_state_acq_err_num++;
 		/* stop rescheduling this worker */
@@ -1611,6 +1784,9 @@ static void ice_dpll_periodic_work(struct kthread_work *work)
 	mutex_unlock(&pf->dplls.lock);
 	ice_dpll_notify_changes(de);
 	ice_dpll_notify_changes(dp);
+	if (phase_offset_ntf)
+		ice_dpll_pins_notify_mask(d->inputs, d->num_inputs,
+					  phase_offset_ntf);
 
 resched:
 	/* Run twice a second or reschedule if update failed */
@@ -1986,7 +2162,6 @@ ice_dpll_deinit_dpll(struct ice_pf *pf, struct ice_dpll *d, bool cgu)
  * @pf: board private structure
  * @d: dpll to be initialized
  * @cgu: if cgu is present and controlled by this NIC
- * @type: type of dpll being initialized
  *
  * Allocate dpll instance for this board in dpll subsystem, if cgu is controlled
  * by this NIC, register dpll with the callback ops.
@@ -2368,6 +2543,8 @@ static int ice_dpll_init_info(struct ice_pf *pf, bool cgu)
 	dp->mode = DPLL_MODE_AUTOMATIC;
 	dp->info.type = DPLL_TYPE_PPS;
 	dp->info.ops = &ice_dpll_ops;
+	dp->info.capabilities = !ice_dpll_is_pps_phase_monitor(pf) ? 0 :
+				DPLL_FEATURES_ALL_INPUTS_PHASE_OFFSET_MONITOR;
 
 	dev_dbg(ice_pf_to_dev(pf),
 		"%s - success, inputs:%u, outputs:%u rclk-parents:%u\n",
diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.h b/drivers/net/ethernet/intel/ice/ice_dpll.h
index 9db7463e293a..430f7fb95690 100644
--- a/drivers/net/ethernet/intel/ice/ice_dpll.h
+++ b/drivers/net/ethernet/intel/ice/ice_dpll.h
@@ -19,6 +19,7 @@
  * @prop: pin properties
  * @freq: current frequency of a pin
  * @phase_adjust: current phase adjust value
+ * @phase_offset: monitored phase offset value
  */
 struct ice_dpll_pin {
 	struct dpll_pin *pin;
@@ -31,6 +32,7 @@ struct ice_dpll_pin {
 	struct dpll_pin_properties prop;
 	u32 freq;
 	s32 phase_adjust;
+	s64 phase_offset;
 	u8 status;
 };
 
@@ -47,6 +49,7 @@ struct ice_dpll_pin {
  * @input_prio: priorities of each input
  * @dpll_state: current dpll sync state
  * @prev_dpll_state: last dpll sync state
+ * @phase_offset_monitor_period: period for phase offset monitor read frequency
  * @active_input: pointer to active input pin
  * @prev_input: pointer to previous active input pin
  */
@@ -64,6 +67,7 @@ struct ice_dpll {
 	enum dpll_lock_status dpll_state;
 	enum dpll_lock_status prev_dpll_state;
 	enum dpll_mode mode;
+	u32 phase_offset_monitor_period;
 	struct dpll_pin *active_input;
 	struct dpll_pin *prev_input;
 	struct dpll_device_info info;
@@ -81,6 +85,7 @@ struct ice_dpll {
  * @num_inputs: number of input pins available on dpll
  * @num_outputs: number of output pins available on dpll
  * @cgu_state_acq_err_num: number of errors returned during periodic work
+ * @periodic_counter: counter of periodic work executions
  * @base_rclk_idx: idx of first pin used for clock revocery pins
  * @clock_id: clock_id of dplls
  * @input_phase_adj_max: max phase adjust value for an input pins
@@ -98,6 +103,7 @@ struct ice_dplls {
 	u8 num_inputs;
 	u8 num_outputs;
 	int cgu_state_acq_err_num;
+	u32 periodic_counter;
 	u8 base_rclk_idx;
 	u64 clock_id;
 	s32 input_phase_adj_max;
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 1fbe13ee93a8..9abc179e1bd3 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -7914,6 +7914,10 @@ const char *ice_aq_str(enum ice_aq_err aq_err)
 		return "ICE_AQ_RC_EPERM";
 	case ICE_AQ_RC_ENOENT:
 		return "ICE_AQ_RC_ENOENT";
+	case ICE_AQ_RC_ESRCH:
+		return "ICE_AQ_RC_ESRCH";
+	case ICE_AQ_RC_EAGAIN:
+		return "ICE_AQ_RC_EAGAIN";
 	case ICE_AQ_RC_ENOMEM:
 		return "ICE_AQ_RC_ENOMEM";
 	case ICE_AQ_RC_EBUSY:
-- 
2.38.1