[PATCH RFC net-next v2 0/2] selftests/dpll: DPLL subsystem integration tests

[PATCH RFC net-next v2 0/2] selftests/dpll: DPLL subsystem integration tests

[Michal Michalik]

The recently merged common DPLL interface discussed on a newsletter[1]
is introducing new, complex subsystem which requires proper integration
testing - this patch adds core for such framework, as well as the
initial test cases. Framework does not require neither any special
hardware nor any special system architecture.

To properly test the DPLL subsystem this patch adds fake DPLL devices and it's
pins implementation to netdevsim. Creating netdevsim devices and adding ports
to it register new DPLL devices and pins. First port of each netdevsim device
acts as a entitiy which registers two DPLL devices: EEC and PPS DPLLs. First
port also register the common pins: PPS and GNSS. Additionally each port
register also RCLK (recovered clock) pin for itself. That allow us to check
mutliple scenarios which might be problematic in real implementations (like
different ordering etc.)

Patch adds few helper scripts, which are:
1) tools/testing/selftests/dpll/run_dpll_tests.sh
    Script is checking for all dependencies, creates temporary
    environment, installs required libraries and run all tests - can be
    used standalone
2) tools/testing/selftests/dpll/ynlfamilyhandler.py˙
    Library for easier ynl use in the pytest framework - can be used
    standalone

[1] https://lore.kernel.org/netdev/169494842736.21621.10730860855645661664.git-patchwork-notify@kernel.org/

Changelog:
v1 -> v2:
- moved from separate module to implementation in netdevsim

Michal Michalik (2):
  netdevsim: implement DPLL for subsystem selftests
  selftests/dpll: add DPLL system integration selftests

 drivers/net/Kconfig                              |   1 +
 drivers/net/netdevsim/Makefile                   |   2 +-
 drivers/net/netdevsim/dpll.c                     | 438 +++++++++++++++++++++++
 drivers/net/netdevsim/dpll.h                     |  81 +++++
 drivers/net/netdevsim/netdev.c                   |  20 ++
 drivers/net/netdevsim/netdevsim.h                |   4 +
 tools/testing/selftests/Makefile                 |   1 +
 tools/testing/selftests/dpll/Makefile            |   8 +
 tools/testing/selftests/dpll/__init__.py         |   0
 tools/testing/selftests/dpll/config              |   2 +
 tools/testing/selftests/dpll/consts.py           |  34 ++
 tools/testing/selftests/dpll/dpll_utils.py       | 109 ++++++
 tools/testing/selftests/dpll/requirements.txt    |   3 +
 tools/testing/selftests/dpll/run_dpll_tests.sh   |  75 ++++
 tools/testing/selftests/dpll/test_dpll.py        | 414 +++++++++++++++++++++
 tools/testing/selftests/dpll/ynlfamilyhandler.py |  49 +++
 16 files changed, 1240 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/netdevsim/dpll.c
 create mode 100644 drivers/net/netdevsim/dpll.h
 create mode 100644 tools/testing/selftests/dpll/Makefile
 create mode 100644 tools/testing/selftests/dpll/__init__.py
 create mode 100644 tools/testing/selftests/dpll/config
 create mode 100644 tools/testing/selftests/dpll/consts.py
 create mode 100644 tools/testing/selftests/dpll/dpll_utils.py
 create mode 100644 tools/testing/selftests/dpll/requirements.txt
 create mode 100755 tools/testing/selftests/dpll/run_dpll_tests.sh
 create mode 100644 tools/testing/selftests/dpll/test_dpll.py
 create mode 100644 tools/testing/selftests/dpll/ynlfamilyhandler.py

-- 
2.9.5

base-commit: 55c900477f5b3897d9038446f72a281cae0efd86

[PATCH RFC net-next v2 1/2] netdevsim: implement DPLL for subsystem selftests

[Michal Michalik]

DPLL subsystem integration tests require a module which mimics the
behavior of real driver which supports DPLL hardware. To fully test the
subsystem the netdevsim is amended with DPLL implementation.

Signed-off-by: Michal Michalik <michal.michalik@intel.com>
---
 drivers/net/Kconfig               |   1 +
 drivers/net/netdevsim/Makefile    |   2 +-
 drivers/net/netdevsim/dpll.c      | 438 ++++++++++++++++++++++++++++++++++++++
 drivers/net/netdevsim/dpll.h      |  81 +++++++
 drivers/net/netdevsim/netdev.c    |  20 ++
 drivers/net/netdevsim/netdevsim.h |   4 +
 6 files changed, 545 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/netdevsim/dpll.c
 create mode 100644 drivers/net/netdevsim/dpll.h

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index af0da4b..633ec89 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -626,6 +626,7 @@ config NETDEVSIM
 	depends on PSAMPLE || PSAMPLE=n
 	depends on PTP_1588_CLOCK_MOCK || PTP_1588_CLOCK_MOCK=n
 	select NET_DEVLINK
+	select DPLL
 	help
 	  This driver is a developer testing tool and software model that can
 	  be used to test various control path networking APIs, especially
diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
index f8de93b..f338ffb 100644
--- a/drivers/net/netdevsim/Makefile
+++ b/drivers/net/netdevsim/Makefile
@@ -3,7 +3,7 @@
 obj-$(CONFIG_NETDEVSIM) += netdevsim.o
 
 netdevsim-objs := \
-	netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o
+	netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o dpll.o
 
 ifeq ($(CONFIG_BPF_SYSCALL),y)
 netdevsim-objs += \
diff --git a/drivers/net/netdevsim/dpll.c b/drivers/net/netdevsim/dpll.c
new file mode 100644
index 0000000..050f68e
--- /dev/null
+++ b/drivers/net/netdevsim/dpll.c
@@ -0,0 +1,438 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2023, Intel Corporation.
+ * Author: Michal Michalik <michal.michalik@intel.com>
+ */
+#include "dpll.h"
+
+static struct dpll_pin_properties *
+create_pin_properties(const char *label, enum dpll_pin_type type,
+		      unsigned long caps, u32 freq_supp_num, u64 fmin, u64 fmax)
+{
+	struct dpll_pin_frequency *freq_supp;
+	struct dpll_pin_properties *pp;
+
+	pp = kzalloc(sizeof(*pp), GFP_KERNEL);
+	if (!pp)
+		return ERR_PTR(-ENOMEM);
+
+	freq_supp = kzalloc(sizeof(*freq_supp), GFP_KERNEL);
+	if (!freq_supp)
+		goto err;
+	*freq_supp =
+		(struct dpll_pin_frequency)DPLL_PIN_FREQUENCY_RANGE(fmin, fmax);
+
+	pp->board_label = kasprintf(GFP_KERNEL, "%s_brd", label);
+	pp->panel_label = kasprintf(GFP_KERNEL, "%s_pnl", label);
+	pp->package_label = kasprintf(GFP_KERNEL, "%s_pcg", label);
+	pp->freq_supported_num = freq_supp_num;
+	pp->freq_supported = freq_supp;
+	pp->capabilities = caps;
+	pp->type = type;
+
+	return pp;
+err:
+	kfree(pp);
+	return ERR_PTR(-ENOMEM);
+}
+
+static struct dpll_pd *create_dpll_pd(int temperature, enum dpll_mode mode)
+{
+	struct dpll_pd *pd;
+
+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return ERR_PTR(-ENOMEM);
+
+	pd->temperature = temperature;
+	pd->mode = mode;
+
+	return pd;
+}
+
+static struct pin_pd *create_pin_pd(u64 frequency, u32 prio,
+				    enum dpll_pin_direction direction)
+{
+	struct pin_pd *pd;
+
+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return ERR_PTR(-ENOMEM);
+
+	pd->state_dpll = DPLL_PIN_STATE_DISCONNECTED;
+	pd->state_pin = DPLL_PIN_STATE_DISCONNECTED;
+	pd->frequency = frequency;
+	pd->direction = direction;
+	pd->prio = prio;
+
+	return pd;
+}
+
+static int
+dds_ops_mode_get(const struct dpll_device *dpll, void *dpll_priv,
+		 enum dpll_mode *mode, struct netlink_ext_ack *extack)
+{
+	*mode = ((struct dpll_pd *)(dpll_priv))->mode;
+	return 0;
+};
+
+static bool
+dds_ops_mode_supported(const struct dpll_device *dpll, void *dpll_priv,
+		       const enum dpll_mode mode,
+		       struct netlink_ext_ack *extack)
+{
+	return true;
+};
+
+static int
+dds_ops_lock_status_get(const struct dpll_device *dpll, void *dpll_priv,
+			enum dpll_lock_status *status,
+			struct netlink_ext_ack *extack)
+{
+	if (((struct dpll_pd *)dpll_priv)->mode == DPLL_MODE_MANUAL)
+		*status = DPLL_LOCK_STATUS_LOCKED;
+	else
+		*status = DPLL_LOCK_STATUS_UNLOCKED;
+	return 0;
+};
+
+static int
+dds_ops_temp_get(const struct dpll_device *dpll, void *dpll_priv, s32 *temp,
+		 struct netlink_ext_ack *extack)
+{
+	*temp = ((struct dpll_pd *)dpll_priv)->temperature;
+	return 0;
+};
+
+static int
+pin_frequency_set(const struct dpll_pin *pin, void *pin_priv,
+		  const struct dpll_device *dpll, void *dpll_priv,
+		  const u64 frequency, struct netlink_ext_ack *extack)
+{
+	((struct pin_pd *)pin_priv)->frequency = frequency;
+	return 0;
+};
+
+static int
+pin_frequency_get(const struct dpll_pin *pin, void *pin_priv,
+		  const struct dpll_device *dpll, void *dpll_priv,
+		  u64 *frequency, struct netlink_ext_ack *extack)
+{
+	*frequency = ((struct pin_pd *)pin_priv)->frequency;
+	return 0;
+};
+
+static int
+pin_direction_set(const struct dpll_pin *pin, void *pin_priv,
+		  const struct dpll_device *dpll, void *dpll_priv,
+		  const enum dpll_pin_direction direction,
+		  struct netlink_ext_ack *extack)
+{
+	((struct pin_pd *)pin_priv)->direction = direction;
+	return 0;
+};
+
+static int
+pin_direction_get(const struct dpll_pin *pin, void *pin_priv,
+		  const struct dpll_device *dpll, void *dpll_priv,
+		  enum dpll_pin_direction *direction,
+		  struct netlink_ext_ack *extack)
+{
+	*direction = ((struct pin_pd *)pin_priv)->direction;
+	return 0;
+};
+
+static int
+pin_state_on_pin_get(const struct dpll_pin *pin, void *pin_priv,
+		     const struct dpll_pin *parent_pin, void *parent_priv,
+		     enum dpll_pin_state *state,
+		     struct netlink_ext_ack *extack)
+{
+	*state = ((struct pin_pd *)pin_priv)->state_pin;
+	return 0;
+};
+
+static int
+pin_state_on_dpll_get(const struct dpll_pin *pin, void *pin_priv,
+		      const struct dpll_device *dpll, void *dpll_priv,
+		      enum dpll_pin_state *state,
+		      struct netlink_ext_ack *extack)
+{
+	*state = ((struct pin_pd *)pin_priv)->state_dpll;
+	return 0;
+};
+
+static int
+pin_state_on_pin_set(const struct dpll_pin *pin, void *pin_priv,
+		     const struct dpll_pin *parent_pin, void *parent_priv,
+		     const enum dpll_pin_state state,
+		     struct netlink_ext_ack *extack)
+{
+	((struct pin_pd *)pin_priv)->state_pin = state;
+	return 0;
+};
+
+static int
+pin_state_on_dpll_set(const struct dpll_pin *pin, void *pin_priv,
+		      const struct dpll_device *dpll, void *dpll_priv,
+		      const enum dpll_pin_state state,
+		      struct netlink_ext_ack *extack)
+{
+	((struct pin_pd *)pin_priv)->state_dpll = state;
+	return 0;
+};
+
+static int
+pin_prio_get(const struct dpll_pin *pin, void *pin_priv,
+	     const struct dpll_device *dpll, void *dpll_priv,
+	     u32 *prio, struct netlink_ext_ack *extack)
+{
+	*prio = ((struct pin_pd *)pin_priv)->prio;
+	return 0;
+};
+
+static int
+pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
+	     const struct dpll_device *dpll, void *dpll_priv,
+	     const u32 prio, struct netlink_ext_ack *extack)
+{
+	((struct pin_pd *)pin_priv)->prio = prio;
+	return 0;
+};
+
+static void
+free_pin_properties(struct dpll_pin_properties *pp)
+{
+	if (pp) {
+		kfree(pp->board_label);
+		kfree(pp->panel_label);
+		kfree(pp->package_label);
+		kfree(pp->freq_supported);
+		kfree(pp);
+	}
+}
+
+static struct dpll_device_ops dds_ops = {
+	.mode_get = dds_ops_mode_get,
+	.mode_supported = dds_ops_mode_supported,
+	.lock_status_get = dds_ops_lock_status_get,
+	.temp_get = dds_ops_temp_get,
+};
+
+static struct dpll_pin_ops pin_ops = {
+	.frequency_set = pin_frequency_set,
+	.frequency_get = pin_frequency_get,
+	.direction_set = pin_direction_set,
+	.direction_get = pin_direction_get,
+	.state_on_pin_get = pin_state_on_pin_get,
+	.state_on_dpll_get = pin_state_on_dpll_get,
+	.state_on_pin_set = pin_state_on_pin_set,
+	.state_on_dpll_set = pin_state_on_dpll_set,
+	.prio_get = pin_prio_get,
+	.prio_set = pin_prio_set,
+};
+
+int nsim_dpll_init_owner(struct nsim_dpll_info *dpll, int devid)
+{
+	/* Create EEC DPLL */
+	dpll->dpll_e = dpll_device_get(DPLLS_CLOCK_ID + devid, EEC_DPLL_DEV,
+				       THIS_MODULE);
+	if (IS_ERR(dpll->dpll_e))
+		goto dpll_e;
+	dpll->dpll_e_pd = create_dpll_pd(EEC_DPLL_TEMPERATURE,
+					 DPLL_MODE_AUTOMATIC);
+	if (IS_ERR(dpll->dpll_e))
+		goto dpll_e_pd;
+	if (dpll_device_register(dpll->dpll_e, DPLL_TYPE_EEC, &dds_ops,
+				 (void *)dpll->dpll_e_pd))
+		goto e_reg;
+
+	/* Create PPS DPLL */
+	dpll->dpll_p = dpll_device_get(DPLLS_CLOCK_ID + devid, PPS_DPLL_DEV,
+				       THIS_MODULE);
+	if (IS_ERR(dpll->dpll_p))
+		goto dpll_p;
+	dpll->dpll_p_pd = create_dpll_pd(PPS_DPLL_TEMPERATURE,
+					 DPLL_MODE_MANUAL);
+	if (IS_ERR(dpll->dpll_p_pd))
+		goto dpll_p_pd;
+	if (dpll_device_register(dpll->dpll_p, DPLL_TYPE_PPS, &dds_ops,
+				 (void *)dpll->dpll_p_pd))
+		goto p_reg;
+
+	/* Create first pin (GNSS) */
+	dpll->pp_gnss = create_pin_properties("GNSS", DPLL_PIN_TYPE_GNSS,
+					      PIN_GNSS_CAPABILITIES,
+					      1, DPLL_PIN_FREQUENCY_1_HZ,
+					      DPLL_PIN_FREQUENCY_1_HZ);
+	if (IS_ERR(dpll->pp_gnss))
+		goto pp_gnss;
+	dpll->p_gnss = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_GNSS,
+				    THIS_MODULE,
+				    dpll->pp_gnss);
+	if (IS_ERR(dpll->p_gnss))
+		goto p_gnss;
+	dpll->p_gnss_pd = create_pin_pd(DPLL_PIN_FREQUENCY_1_HZ,
+					PIN_GNSS_PRIORITY,
+					DPLL_PIN_DIRECTION_INPUT);
+	if (IS_ERR(dpll->p_gnss_pd))
+		goto p_gnss_pd;
+	if (dpll_pin_register(dpll->dpll_e, dpll->p_gnss, &pin_ops,
+			      (void *)dpll->p_gnss_pd))
+		goto e_gnss_reg;
+
+	/* Create second pin (PPS) */
+	dpll->pp_pps = create_pin_properties("PPS", DPLL_PIN_TYPE_EXT,
+					     PIN_PPS_CAPABILITIES,
+					     1, DPLL_PIN_FREQUENCY_1_HZ,
+					     DPLL_PIN_FREQUENCY_1_HZ);
+	if (IS_ERR(dpll->pp_pps))
+		goto pp_pps;
+	dpll->p_pps = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_PPS, THIS_MODULE,
+				   dpll->pp_pps);
+	if (IS_ERR(dpll->p_pps))
+		goto p_pps;
+	dpll->p_pps_pd = create_pin_pd(DPLL_PIN_FREQUENCY_1_HZ,
+				       PIN_PPS_PRIORITY,
+				       DPLL_PIN_DIRECTION_INPUT);
+	if (IS_ERR(dpll->p_pps_pd))
+		goto p_pps_pd;
+	if (dpll_pin_register(dpll->dpll_e, dpll->p_pps, &pin_ops,
+			      (void *)dpll->p_pps_pd))
+		goto e_pps_reg;
+	if (dpll_pin_register(dpll->dpll_p, dpll->p_pps, &pin_ops,
+			      (void *)dpll->p_pps_pd))
+		goto p_pps_reg;
+
+	return 0;
+
+p_pps_reg:
+	dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &pin_ops,
+			    (void *)dpll->p_pps_pd);
+e_pps_reg:
+	kfree(dpll->p_pps_pd);
+p_pps_pd:
+	dpll_pin_put(dpll->p_pps);
+p_pps:
+	free_pin_properties(dpll->pp_pps);
+pp_pps:
+	dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &pin_ops,
+			    (void *)dpll->p_gnss_pd);
+e_gnss_reg:
+	kfree(dpll->p_gnss_pd);
+p_gnss_pd:
+	dpll_pin_put(dpll->p_gnss);
+p_gnss:
+	free_pin_properties(dpll->pp_gnss);
+pp_gnss:
+	dpll_device_unregister(dpll->dpll_p, &dds_ops, (void *)dpll->dpll_p_pd);
+p_reg:
+	kfree(dpll->dpll_p_pd);
+dpll_p_pd:
+	dpll_device_put(dpll->dpll_p);
+dpll_p:
+	dpll_device_unregister(dpll->dpll_e, &dds_ops, (void *)dpll->dpll_e_pd);
+e_reg:
+	kfree(dpll->dpll_e_pd);
+dpll_e_pd:
+	dpll_device_put(dpll->dpll_e);
+dpll_e:
+	return -1;
+}
+
+void nsim_dpll_free_owner(struct nsim_dpll_info *dpll)
+{
+	/* Free GNSS pin */
+	dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &pin_ops,
+			    (void *)dpll->p_gnss_pd);
+	dpll_pin_put(dpll->p_gnss);
+	free_pin_properties(dpll->pp_gnss);
+	kfree(dpll->p_gnss_pd);
+
+	/* Free PPS pin */
+	dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &pin_ops,
+			    (void *)dpll->p_pps_pd);
+	dpll_pin_unregister(dpll->dpll_p, dpll->p_pps, &pin_ops,
+			    (void *)dpll->p_pps_pd);
+	dpll_pin_put(dpll->p_pps);
+	free_pin_properties(dpll->pp_pps);
+	kfree(dpll->p_pps_pd);
+
+	/* Free DPLL EEC */
+	dpll_device_unregister(dpll->dpll_e, &dds_ops, (void *)dpll->dpll_e_pd);
+	dpll_device_put(dpll->dpll_e);
+	kfree(dpll->dpll_e_pd);
+
+	/* Free DPLL PPS */
+	dpll_device_unregister(dpll->dpll_p, &dds_ops, (void *)dpll->dpll_p_pd);
+	dpll_device_put(dpll->dpll_p);
+	kfree(dpll->dpll_p_pd);
+}
+
+int nsim_rclk_init(struct nsim_dpll_info *dpll, int devid, unsigned int index)
+{
+	char *name = kasprintf(GFP_KERNEL, "RCLK_%i", index);
+
+	/* Get EEC DPLL */
+	dpll->dpll_e = dpll_device_get(DPLLS_CLOCK_ID + devid, EEC_DPLL_DEV,
+				       THIS_MODULE);
+	if (IS_ERR(dpll->dpll_e))
+		goto dpll;
+
+	/* Get PPS DPLL */
+	dpll->dpll_p = dpll_device_get(DPLLS_CLOCK_ID + devid, PPS_DPLL_DEV,
+				       THIS_MODULE);
+	if (IS_ERR(dpll->dpll_p))
+		goto dpll;
+
+	/* Create Recovered clock pin (RCLK) */
+	dpll->pp_rclk = create_pin_properties(name,
+					      DPLL_PIN_TYPE_SYNCE_ETH_PORT,
+					      PIN_RCLK_CAPABILITIES, 1, 1e6,
+					      125e6);
+	if (IS_ERR(dpll->pp_rclk))
+		goto dpll;
+	dpll->p_rclk = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_RCLK + index,
+				    THIS_MODULE, dpll->pp_rclk);
+	if (IS_ERR(dpll->p_rclk))
+		goto p_rclk;
+	dpll->p_rclk_pd = create_pin_pd(DPLL_PIN_FREQUENCY_10_MHZ,
+					PIN_RCLK_PRIORITY,
+					DPLL_PIN_DIRECTION_INPUT);
+	if (IS_ERR(dpll->p_rclk_pd))
+		goto p_rclk_pd;
+	if (dpll_pin_register(dpll->dpll_e, dpll->p_rclk, &pin_ops,
+			      (void *)dpll->p_rclk_pd))
+		goto dpll_e_reg;
+	if (dpll_pin_register(dpll->dpll_p, dpll->p_rclk, &pin_ops,
+			      (void *)dpll->p_rclk_pd))
+		goto dpll_p_reg;
+
+	return 0;
+
+dpll_p_reg:
+	dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk, &pin_ops,
+			    (void *)dpll->p_rclk_pd);
+dpll_e_reg:
+	kfree(dpll->p_rclk_pd);
+p_rclk_pd:
+	dpll_pin_put(dpll->p_rclk);
+p_rclk:
+	free_pin_properties(dpll->pp_rclk);
+dpll:
+	return -1;
+}
+
+void nsim_rclk_free(struct nsim_dpll_info *dpll)
+{
+	/* Free RCLK pin */
+	dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk, &pin_ops,
+			    (void *)dpll->p_rclk_pd);
+	dpll_pin_unregister(dpll->dpll_p, dpll->p_rclk, &pin_ops,
+			    (void *)dpll->p_rclk_pd);
+	dpll_pin_put(dpll->p_rclk);
+	free_pin_properties(dpll->pp_rclk);
+	kfree(dpll->p_rclk_pd);
+	dpll_device_put(dpll->dpll_e);
+	dpll_device_put(dpll->dpll_p);
+}
diff --git a/drivers/net/netdevsim/dpll.h b/drivers/net/netdevsim/dpll.h
new file mode 100644
index 0000000..17db7f7
--- /dev/null
+++ b/drivers/net/netdevsim/dpll.h
@@ -0,0 +1,81 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2023, Intel Corporation.
+ * Author: Michal Michalik <michal.michalik@intel.com>
+ */
+
+#ifndef NSIM_DPLL_H
+#define NSIM_DPLL_H
+
+#include <linux/types.h>
+#include <linux/netlink.h>
+
+#include <linux/dpll.h>
+#include <uapi/linux/dpll.h>
+
+#define EEC_DPLL_DEV 0
+#define EEC_DPLL_TEMPERATURE 20
+#define PPS_DPLL_DEV 1
+#define PPS_DPLL_TEMPERATURE 30
+#define DPLLS_CLOCK_ID 234
+
+#define PIN_GNSS 0
+#define PIN_GNSS_CAPABILITIES 2 /* DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE */
+#define PIN_GNSS_PRIORITY 5
+
+#define PIN_PPS 1
+#define PIN_PPS_CAPABILITIES 7 /* DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE
+				* || DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE
+				* || DPLL_PIN_CAPS_STATE_CAN_CHANGE
+				*/
+#define PIN_PPS_PRIORITY 6
+
+#define PIN_RCLK 2
+#define PIN_RCLK_CAPABILITIES 6 /* DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE
+				 * || DPLL_PIN_CAPS_STATE_CAN_CHANGE
+				 */
+#define PIN_RCLK_PRIORITY 7
+
+#define EEC_PINS_NUMBER 3
+#define PPS_PINS_NUMBER 2
+
+struct dpll_pd {
+	enum dpll_mode mode;
+	int temperature;
+};
+
+struct pin_pd {
+	u64 frequency;
+	enum dpll_pin_direction direction;
+	enum dpll_pin_state state_pin;
+	enum dpll_pin_state state_dpll;
+	u32 prio;
+};
+
+struct nsim_dpll_info {
+	bool owner;
+
+	struct dpll_device *dpll_e;
+	struct dpll_pd *dpll_e_pd;
+	struct dpll_device *dpll_p;
+	struct dpll_pd *dpll_p_pd;
+
+	struct dpll_pin_properties *pp_gnss;
+	struct dpll_pin *p_gnss;
+	struct pin_pd *p_gnss_pd;
+
+	struct dpll_pin_properties *pp_pps;
+	struct dpll_pin *p_pps;
+	struct pin_pd *p_pps_pd;
+
+	struct dpll_pin_properties *pp_rclk;
+	struct dpll_pin *p_rclk;
+	struct pin_pd *p_rclk_pd;
+};
+
+int nsim_dpll_init_owner(struct nsim_dpll_info *dpll, int devid);
+void nsim_dpll_free_owner(struct nsim_dpll_info *dpll);
+int nsim_rclk_init(struct nsim_dpll_info *dpll, int devid, unsigned int index);
+void nsim_rclk_free(struct nsim_dpll_info *dpll);
+
+#endif /* NSIM_DPLL_H */
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index aecaf5f..78a936f 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -25,6 +25,7 @@
 #include <net/udp_tunnel.h>
 
 #include "netdevsim.h"
+#include "dpll.h"
 
 static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
@@ -344,6 +345,20 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
 	if (err)
 		goto err_ipsec_teardown;
 	rtnl_unlock();
+
+	if (ns->nsim_dev_port->port_index == 0) {
+		err = nsim_dpll_init_owner(&ns->dpll,
+					   ns->nsim_dev->nsim_bus_dev->dev.id);
+		if (err)
+			goto err_ipsec_teardown;
+	}
+
+	err = nsim_rclk_init(&ns->dpll, ns->nsim_dev->nsim_bus_dev->dev.id,
+			     ns->nsim_dev_port->port_index);
+
+	if (err)
+		goto err_ipsec_teardown;
+
 	return 0;
 
 err_ipsec_teardown:
@@ -419,6 +434,11 @@ void nsim_destroy(struct netdevsim *ns)
 	if (nsim_dev_port_is_pf(ns->nsim_dev_port))
 		nsim_udp_tunnels_info_destroy(dev);
 	mock_phc_destroy(ns->phc);
+
+	nsim_rclk_free(&ns->dpll);
+	if (ns->nsim_dev_port->port_index == 0)
+		nsim_dpll_free_owner(&ns->dpll);
+
 	free_netdev(dev);
 }
 
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 028c825..3d0138a 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -26,6 +26,8 @@
 #include <net/xdp.h>
 #include <net/macsec.h>
 
+#include "dpll.h"
+
 #define DRV_NAME	"netdevsim"
 
 #define NSIM_XDP_MAX_MTU	4000
@@ -125,6 +127,8 @@ struct netdevsim {
 	} udp_ports;
 
 	struct nsim_ethtool ethtool;
+
+	struct nsim_dpll_info dpll;
 };
 
 struct netdevsim *
-- 
2.9.5

Re: [PATCH RFC net-next v2 1/2] netdevsim: implement DPLL for subsystem selftests

[Jiri Pirko]

Mon, Oct 30, 2023 at 05:53:25PM CET, michal.michalik@intel.com wrote:
>DPLL subsystem integration tests require a module which mimics the
>behavior of real driver which supports DPLL hardware. To fully test the
>subsystem the netdevsim is amended with DPLL implementation.
>
>Signed-off-by: Michal Michalik <michal.michalik@intel.com>
>---
> drivers/net/Kconfig               |   1 +
> drivers/net/netdevsim/Makefile    |   2 +-
> drivers/net/netdevsim/dpll.c      | 438 ++++++++++++++++++++++++++++++++++++++
> drivers/net/netdevsim/dpll.h      |  81 +++++++
> drivers/net/netdevsim/netdev.c    |  20 ++
> drivers/net/netdevsim/netdevsim.h |   4 +
> 6 files changed, 545 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/netdevsim/dpll.c
> create mode 100644 drivers/net/netdevsim/dpll.h
>
>diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>index af0da4b..633ec89 100644
>--- a/drivers/net/Kconfig
>+++ b/drivers/net/Kconfig
>@@ -626,6 +626,7 @@ config NETDEVSIM
> 	depends on PSAMPLE || PSAMPLE=n
> 	depends on PTP_1588_CLOCK_MOCK || PTP_1588_CLOCK_MOCK=n
> 	select NET_DEVLINK
>+	select DPLL
> 	help
> 	  This driver is a developer testing tool and software model that can
> 	  be used to test various control path networking APIs, especially
>diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
>index f8de93b..f338ffb 100644
>--- a/drivers/net/netdevsim/Makefile
>+++ b/drivers/net/netdevsim/Makefile
>@@ -3,7 +3,7 @@
> obj-$(CONFIG_NETDEVSIM) += netdevsim.o
> 
> netdevsim-objs := \
>-	netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o
>+	netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o dpll.o
> 
> ifeq ($(CONFIG_BPF_SYSCALL),y)
> netdevsim-objs += \
>diff --git a/drivers/net/netdevsim/dpll.c b/drivers/net/netdevsim/dpll.c
>new file mode 100644
>index 0000000..050f68e
>--- /dev/null
>+++ b/drivers/net/netdevsim/dpll.c
>@@ -0,0 +1,438 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Copyright (c) 2023, Intel Corporation.
>+ * Author: Michal Michalik <michal.michalik@intel.com>
>+ */
>+#include "dpll.h"
>+
>+static struct dpll_pin_properties *
>+create_pin_properties(const char *label, enum dpll_pin_type type,

Please make sure to follow the namespace prefix notation in your patch
everywhere, functions, structs, defines.
"nsim_"


>+		      unsigned long caps, u32 freq_supp_num, u64 fmin, u64 fmax)
>+{
>+	struct dpll_pin_frequency *freq_supp;
>+	struct dpll_pin_properties *pp;
>+
>+	pp = kzalloc(sizeof(*pp), GFP_KERNEL);
>+	if (!pp)
>+		return ERR_PTR(-ENOMEM);
>+
>+	freq_supp = kzalloc(sizeof(*freq_supp), GFP_KERNEL);
>+	if (!freq_supp)
>+		goto err;
>+	*freq_supp =
>+		(struct dpll_pin_frequency)DPLL_PIN_FREQUENCY_RANGE(fmin, fmax);

Drop the cast.


>+
>+	pp->board_label = kasprintf(GFP_KERNEL, "%s_brd", label);
>+	pp->panel_label = kasprintf(GFP_KERNEL, "%s_pnl", label);
>+	pp->package_label = kasprintf(GFP_KERNEL, "%s_pcg", label);
>+	pp->freq_supported_num = freq_supp_num;
>+	pp->freq_supported = freq_supp;
>+	pp->capabilities = caps;
>+	pp->type = type;
>+
>+	return pp;
>+err:
>+	kfree(pp);
>+	return ERR_PTR(-ENOMEM);
>+}
>+
>+static struct dpll_pd *create_dpll_pd(int temperature, enum dpll_mode mode)
>+{
>+	struct dpll_pd *pd;
>+
>+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>+	if (!pd)
>+		return ERR_PTR(-ENOMEM);
>+
>+	pd->temperature = temperature;
>+	pd->mode = mode;
>+
>+	return pd;
>+}
>+
>+static struct pin_pd *create_pin_pd(u64 frequency, u32 prio,
>+				    enum dpll_pin_direction direction)
>+{
>+	struct pin_pd *pd;
>+
>+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>+	if (!pd)
>+		return ERR_PTR(-ENOMEM);
>+
>+	pd->state_dpll = DPLL_PIN_STATE_DISCONNECTED;
>+	pd->state_pin = DPLL_PIN_STATE_DISCONNECTED;
>+	pd->frequency = frequency;
>+	pd->direction = direction;
>+	pd->prio = prio;
>+
>+	return pd;
>+}
>+
>+static int
>+dds_ops_mode_get(const struct dpll_device *dpll, void *dpll_priv,
>+		 enum dpll_mode *mode, struct netlink_ext_ack *extack)
>+{
>+	*mode = ((struct dpll_pd *)(dpll_priv))->mode;

Please have variable assigned by dpll_priv instead of this.
Also, fix in the rest of the code.


>+	return 0;
>+};
>+
>+static bool
>+dds_ops_mode_supported(const struct dpll_device *dpll, void *dpll_priv,
>+		       const enum dpll_mode mode,
>+		       struct netlink_ext_ack *extack)
>+{
>+	return true;
>+};
>+
>+static int
>+dds_ops_lock_status_get(const struct dpll_device *dpll, void *dpll_priv,
>+			enum dpll_lock_status *status,
>+			struct netlink_ext_ack *extack)
>+{
>+	if (((struct dpll_pd *)dpll_priv)->mode == DPLL_MODE_MANUAL)
>+		*status = DPLL_LOCK_STATUS_LOCKED;
>+	else
>+		*status = DPLL_LOCK_STATUS_UNLOCKED;

I don't understand the logic of this function. According to mode you
return if status is locked or not? For this, you should expose a debugfs
knob so the user can emulate changes of the HW.


>+	return 0;
>+};
>+
>+static int
>+dds_ops_temp_get(const struct dpll_device *dpll, void *dpll_priv, s32 *temp,
>+		 struct netlink_ext_ack *extack)
>+{
>+	*temp = ((struct dpll_pd *)dpll_priv)->temperature;
>+	return 0;
>+};
>+
>+static int
>+pin_frequency_set(const struct dpll_pin *pin, void *pin_priv,
>+		  const struct dpll_device *dpll, void *dpll_priv,
>+		  const u64 frequency, struct netlink_ext_ack *extack)
>+{
>+	((struct pin_pd *)pin_priv)->frequency = frequency;
>+	return 0;
>+};
>+
>+static int
>+pin_frequency_get(const struct dpll_pin *pin, void *pin_priv,
>+		  const struct dpll_device *dpll, void *dpll_priv,
>+		  u64 *frequency, struct netlink_ext_ack *extack)
>+{
>+	*frequency = ((struct pin_pd *)pin_priv)->frequency;
>+	return 0;
>+};
>+
>+static int
>+pin_direction_set(const struct dpll_pin *pin, void *pin_priv,
>+		  const struct dpll_device *dpll, void *dpll_priv,
>+		  const enum dpll_pin_direction direction,
>+		  struct netlink_ext_ack *extack)
>+{
>+	((struct pin_pd *)pin_priv)->direction = direction;
>+	return 0;
>+};
>+
>+static int
>+pin_direction_get(const struct dpll_pin *pin, void *pin_priv,
>+		  const struct dpll_device *dpll, void *dpll_priv,
>+		  enum dpll_pin_direction *direction,
>+		  struct netlink_ext_ack *extack)
>+{
>+	*direction = ((struct pin_pd *)pin_priv)->direction;
>+	return 0;
>+};
>+
>+static int
>+pin_state_on_pin_get(const struct dpll_pin *pin, void *pin_priv,
>+		     const struct dpll_pin *parent_pin, void *parent_priv,
>+		     enum dpll_pin_state *state,
>+		     struct netlink_ext_ack *extack)
>+{
>+	*state = ((struct pin_pd *)pin_priv)->state_pin;
>+	return 0;
>+};
>+
>+static int
>+pin_state_on_dpll_get(const struct dpll_pin *pin, void *pin_priv,
>+		      const struct dpll_device *dpll, void *dpll_priv,
>+		      enum dpll_pin_state *state,
>+		      struct netlink_ext_ack *extack)
>+{
>+	*state = ((struct pin_pd *)pin_priv)->state_dpll;
>+	return 0;
>+};
>+
>+static int
>+pin_state_on_pin_set(const struct dpll_pin *pin, void *pin_priv,
>+		     const struct dpll_pin *parent_pin, void *parent_priv,
>+		     const enum dpll_pin_state state,
>+		     struct netlink_ext_ack *extack)
>+{
>+	((struct pin_pd *)pin_priv)->state_pin = state;
>+	return 0;
>+};
>+
>+static int
>+pin_state_on_dpll_set(const struct dpll_pin *pin, void *pin_priv,
>+		      const struct dpll_device *dpll, void *dpll_priv,
>+		      const enum dpll_pin_state state,
>+		      struct netlink_ext_ack *extack)
>+{
>+	((struct pin_pd *)pin_priv)->state_dpll = state;
>+	return 0;
>+};
>+
>+static int
>+pin_prio_get(const struct dpll_pin *pin, void *pin_priv,
>+	     const struct dpll_device *dpll, void *dpll_priv,
>+	     u32 *prio, struct netlink_ext_ack *extack)
>+{
>+	*prio = ((struct pin_pd *)pin_priv)->prio;
>+	return 0;
>+};
>+
>+static int
>+pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
>+	     const struct dpll_device *dpll, void *dpll_priv,
>+	     const u32 prio, struct netlink_ext_ack *extack)
>+{
>+	((struct pin_pd *)pin_priv)->prio = prio;
>+	return 0;
>+};
>+
>+static void
>+free_pin_properties(struct dpll_pin_properties *pp)
>+{
>+	if (pp) {

How exactly pp could be null?


>+		kfree(pp->board_label);
>+		kfree(pp->panel_label);
>+		kfree(pp->package_label);
>+		kfree(pp->freq_supported);
>+		kfree(pp);
>+	}
>+}
>+
>+static struct dpll_device_ops dds_ops = {
>+	.mode_get = dds_ops_mode_get,
>+	.mode_supported = dds_ops_mode_supported,
>+	.lock_status_get = dds_ops_lock_status_get,
>+	.temp_get = dds_ops_temp_get,
>+};
>+
>+static struct dpll_pin_ops pin_ops = {
>+	.frequency_set = pin_frequency_set,
>+	.frequency_get = pin_frequency_get,
>+	.direction_set = pin_direction_set,
>+	.direction_get = pin_direction_get,
>+	.state_on_pin_get = pin_state_on_pin_get,
>+	.state_on_dpll_get = pin_state_on_dpll_get,
>+	.state_on_pin_set = pin_state_on_pin_set,
>+	.state_on_dpll_set = pin_state_on_dpll_set,
>+	.prio_get = pin_prio_get,
>+	.prio_set = pin_prio_set,
>+};
>+
>+int nsim_dpll_init_owner(struct nsim_dpll_info *dpll, int devid)
>+{
>+	/* Create EEC DPLL */
>+	dpll->dpll_e = dpll_device_get(DPLLS_CLOCK_ID + devid, EEC_DPLL_DEV,

"#define DPLLS_CLOCK_ID 234"

You guys always come up with some funky way of making up ids and names.
Why this can't be randomly generated u64?


>+				       THIS_MODULE);
>+	if (IS_ERR(dpll->dpll_e))
>+		goto dpll_e;
>+	dpll->dpll_e_pd = create_dpll_pd(EEC_DPLL_TEMPERATURE,
>+					 DPLL_MODE_AUTOMATIC);
>+	if (IS_ERR(dpll->dpll_e))
>+		goto dpll_e_pd;
>+	if (dpll_device_register(dpll->dpll_e, DPLL_TYPE_EEC, &dds_ops,
>+				 (void *)dpll->dpll_e_pd))

Please avoid pointless casts. (void *) cast for arg of type (void *) are
especially pointless. Please make sure to fix this in the rest of the
code as well.


>+		goto e_reg;
>+
>+	/* Create PPS DPLL */
>+	dpll->dpll_p = dpll_device_get(DPLLS_CLOCK_ID + devid, PPS_DPLL_DEV,
>+				       THIS_MODULE);
>+	if (IS_ERR(dpll->dpll_p))
>+		goto dpll_p;
>+	dpll->dpll_p_pd = create_dpll_pd(PPS_DPLL_TEMPERATURE,
>+					 DPLL_MODE_MANUAL);
>+	if (IS_ERR(dpll->dpll_p_pd))
>+		goto dpll_p_pd;
>+	if (dpll_device_register(dpll->dpll_p, DPLL_TYPE_PPS, &dds_ops,

You always use "int err" to store the return value of function calls
returning 0/-EXXX like this one.


>+				 (void *)dpll->dpll_p_pd))
>+		goto p_reg;
>+
>+	/* Create first pin (GNSS) */
>+	dpll->pp_gnss = create_pin_properties("GNSS", DPLL_PIN_TYPE_GNSS,
>+					      PIN_GNSS_CAPABILITIES,
>+					      1, DPLL_PIN_FREQUENCY_1_HZ,
>+					      DPLL_PIN_FREQUENCY_1_HZ);
>+	if (IS_ERR(dpll->pp_gnss))
>+		goto pp_gnss;
>+	dpll->p_gnss = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_GNSS,
>+				    THIS_MODULE,
>+				    dpll->pp_gnss);
>+	if (IS_ERR(dpll->p_gnss))
>+		goto p_gnss;
>+	dpll->p_gnss_pd = create_pin_pd(DPLL_PIN_FREQUENCY_1_HZ,
>+					PIN_GNSS_PRIORITY,
>+					DPLL_PIN_DIRECTION_INPUT);
>+	if (IS_ERR(dpll->p_gnss_pd))
>+		goto p_gnss_pd;
>+	if (dpll_pin_register(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>+			      (void *)dpll->p_gnss_pd))
>+		goto e_gnss_reg;
>+
>+	/* Create second pin (PPS) */
>+	dpll->pp_pps = create_pin_properties("PPS", DPLL_PIN_TYPE_EXT,
>+					     PIN_PPS_CAPABILITIES,
>+					     1, DPLL_PIN_FREQUENCY_1_HZ,
>+					     DPLL_PIN_FREQUENCY_1_HZ);
>+	if (IS_ERR(dpll->pp_pps))
>+		goto pp_pps;
>+	dpll->p_pps = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_PPS, THIS_MODULE,
>+				   dpll->pp_pps);
>+	if (IS_ERR(dpll->p_pps))
>+		goto p_pps;
>+	dpll->p_pps_pd = create_pin_pd(DPLL_PIN_FREQUENCY_1_HZ,
>+				       PIN_PPS_PRIORITY,
>+				       DPLL_PIN_DIRECTION_INPUT);
>+	if (IS_ERR(dpll->p_pps_pd))
>+		goto p_pps_pd;
>+	if (dpll_pin_register(dpll->dpll_e, dpll->p_pps, &pin_ops,
>+			      (void *)dpll->p_pps_pd))
>+		goto e_pps_reg;
>+	if (dpll_pin_register(dpll->dpll_p, dpll->p_pps, &pin_ops,
>+			      (void *)dpll->p_pps_pd))
>+		goto p_pps_reg;
>+
>+	return 0;
>+
>+p_pps_reg:
>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &pin_ops,
>+			    (void *)dpll->p_pps_pd);
>+e_pps_reg:
>+	kfree(dpll->p_pps_pd);
>+p_pps_pd:
>+	dpll_pin_put(dpll->p_pps);
>+p_pps:
>+	free_pin_properties(dpll->pp_pps);
>+pp_pps:
>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>+			    (void *)dpll->p_gnss_pd);
>+e_gnss_reg:
>+	kfree(dpll->p_gnss_pd);
>+p_gnss_pd:
>+	dpll_pin_put(dpll->p_gnss);
>+p_gnss:
>+	free_pin_properties(dpll->pp_gnss);
>+pp_gnss:
>+	dpll_device_unregister(dpll->dpll_p, &dds_ops, (void *)dpll->dpll_p_pd);
>+p_reg:
>+	kfree(dpll->dpll_p_pd);
>+dpll_p_pd:
>+	dpll_device_put(dpll->dpll_p);
>+dpll_p:
>+	dpll_device_unregister(dpll->dpll_e, &dds_ops, (void *)dpll->dpll_e_pd);
>+e_reg:
>+	kfree(dpll->dpll_e_pd);
>+dpll_e_pd:
>+	dpll_device_put(dpll->dpll_e);
>+dpll_e:
>+	return -1;
>+}
>+
>+void nsim_dpll_free_owner(struct nsim_dpll_info *dpll)
>+{
>+	/* Free GNSS pin */
>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>+			    (void *)dpll->p_gnss_pd);
>+	dpll_pin_put(dpll->p_gnss);
>+	free_pin_properties(dpll->pp_gnss);
>+	kfree(dpll->p_gnss_pd);
>+
>+	/* Free PPS pin */
>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &pin_ops,
>+			    (void *)dpll->p_pps_pd);
>+	dpll_pin_unregister(dpll->dpll_p, dpll->p_pps, &pin_ops,
>+			    (void *)dpll->p_pps_pd);
>+	dpll_pin_put(dpll->p_pps);
>+	free_pin_properties(dpll->pp_pps);
>+	kfree(dpll->p_pps_pd);
>+
>+	/* Free DPLL EEC */
>+	dpll_device_unregister(dpll->dpll_e, &dds_ops, (void *)dpll->dpll_e_pd);
>+	dpll_device_put(dpll->dpll_e);
>+	kfree(dpll->dpll_e_pd);
>+
>+	/* Free DPLL PPS */
>+	dpll_device_unregister(dpll->dpll_p, &dds_ops, (void *)dpll->dpll_p_pd);
>+	dpll_device_put(dpll->dpll_p);
>+	kfree(dpll->dpll_p_pd);
>+}
>+
>+int nsim_rclk_init(struct nsim_dpll_info *dpll, int devid, unsigned int index)
>+{
>+	char *name = kasprintf(GFP_KERNEL, "RCLK_%i", index);
>+
>+	/* Get EEC DPLL */
>+	dpll->dpll_e = dpll_device_get(DPLLS_CLOCK_ID + devid, EEC_DPLL_DEV,
>+				       THIS_MODULE);
>+	if (IS_ERR(dpll->dpll_e))
>+		goto dpll;
>+
>+	/* Get PPS DPLL */
>+	dpll->dpll_p = dpll_device_get(DPLLS_CLOCK_ID + devid, PPS_DPLL_DEV,
>+				       THIS_MODULE);
>+	if (IS_ERR(dpll->dpll_p))
>+		goto dpll;
>+
>+	/* Create Recovered clock pin (RCLK) */
>+	dpll->pp_rclk = create_pin_properties(name,
>+					      DPLL_PIN_TYPE_SYNCE_ETH_PORT,
>+					      PIN_RCLK_CAPABILITIES, 1, 1e6,
>+					      125e6);
>+	if (IS_ERR(dpll->pp_rclk))
>+		goto dpll;
>+	dpll->p_rclk = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_RCLK + index,
>+				    THIS_MODULE, dpll->pp_rclk);
>+	if (IS_ERR(dpll->p_rclk))
>+		goto p_rclk;
>+	dpll->p_rclk_pd = create_pin_pd(DPLL_PIN_FREQUENCY_10_MHZ,
>+					PIN_RCLK_PRIORITY,
>+					DPLL_PIN_DIRECTION_INPUT);
>+	if (IS_ERR(dpll->p_rclk_pd))
>+		goto p_rclk_pd;
>+	if (dpll_pin_register(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>+			      (void *)dpll->p_rclk_pd))
>+		goto dpll_e_reg;
>+	if (dpll_pin_register(dpll->dpll_p, dpll->p_rclk, &pin_ops,
>+			      (void *)dpll->p_rclk_pd))
>+		goto dpll_p_reg;
>+
>+	return 0;
>+
>+dpll_p_reg:
>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>+			    (void *)dpll->p_rclk_pd);
>+dpll_e_reg:
>+	kfree(dpll->p_rclk_pd);
>+p_rclk_pd:
>+	dpll_pin_put(dpll->p_rclk);
>+p_rclk:
>+	free_pin_properties(dpll->pp_rclk);
>+dpll:
>+	return -1;
>+}
>+
>+void nsim_rclk_free(struct nsim_dpll_info *dpll)
>+{
>+	/* Free RCLK pin */
>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>+			    (void *)dpll->p_rclk_pd);
>+	dpll_pin_unregister(dpll->dpll_p, dpll->p_rclk, &pin_ops,
>+			    (void *)dpll->p_rclk_pd);
>+	dpll_pin_put(dpll->p_rclk);
>+	free_pin_properties(dpll->pp_rclk);
>+	kfree(dpll->p_rclk_pd);
>+	dpll_device_put(dpll->dpll_e);
>+	dpll_device_put(dpll->dpll_p);
>+}
>diff --git a/drivers/net/netdevsim/dpll.h b/drivers/net/netdevsim/dpll.h
>new file mode 100644
>index 0000000..17db7f7
>--- /dev/null
>+++ b/drivers/net/netdevsim/dpll.h
>@@ -0,0 +1,81 @@
>+/* SPDX-License-Identifier: GPL-2.0 */
>+/*
>+ * Copyright (c) 2023, Intel Corporation.
>+ * Author: Michal Michalik <michal.michalik@intel.com>
>+ */
>+
>+#ifndef NSIM_DPLL_H
>+#define NSIM_DPLL_H

Why you need a separate header for this? Just put necessary parts in
netdevsim.h and leave the rest in the .c file.


>+
>+#include <linux/types.h>
>+#include <linux/netlink.h>
>+
>+#include <linux/dpll.h>
>+#include <uapi/linux/dpll.h>

Why exactly do you need to include uapi header directly?


>+
>+#define EEC_DPLL_DEV 0
>+#define EEC_DPLL_TEMPERATURE 20
>+#define PPS_DPLL_DEV 1
>+#define PPS_DPLL_TEMPERATURE 30
>+#define DPLLS_CLOCK_ID 234
>+
>+#define PIN_GNSS 0
>+#define PIN_GNSS_CAPABILITIES 2 /* DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE */
>+#define PIN_GNSS_PRIORITY 5
>+
>+#define PIN_PPS 1
>+#define PIN_PPS_CAPABILITIES 7 /* DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE
>+				* || DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE
>+				* || DPLL_PIN_CAPS_STATE_CAN_CHANGE

You are kidding, correct? :)


>+				*/
>+#define PIN_PPS_PRIORITY 6
>+
>+#define PIN_RCLK 2
>+#define PIN_RCLK_CAPABILITIES 6 /* DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE
>+				 * || DPLL_PIN_CAPS_STATE_CAN_CHANGE
>+				 */
>+#define PIN_RCLK_PRIORITY 7
>+
>+#define EEC_PINS_NUMBER 3
>+#define PPS_PINS_NUMBER 2
>+
>+struct dpll_pd {

Have not clue what do you mean by "pd".


>+	enum dpll_mode mode;
>+	int temperature;
>+};
>+
>+struct pin_pd {
>+	u64 frequency;
>+	enum dpll_pin_direction direction;
>+	enum dpll_pin_state state_pin;
>+	enum dpll_pin_state state_dpll;
>+	u32 prio;
>+};
>+
>+struct nsim_dpll_info {

Drop "info".


>+	bool owner;
>+
>+	struct dpll_device *dpll_e;
>+	struct dpll_pd *dpll_e_pd;
>+	struct dpll_device *dpll_p;
>+	struct dpll_pd *dpll_p_pd;
>+
>+	struct dpll_pin_properties *pp_gnss;

Why pointer? Just embed the properties struct, no?


>+	struct dpll_pin *p_gnss;
>+	struct pin_pd *p_gnss_pd;
>+
>+	struct dpll_pin_properties *pp_pps;
>+	struct dpll_pin *p_pps;
>+	struct pin_pd *p_pps_pd;
>+
>+	struct dpll_pin_properties *pp_rclk;
>+	struct dpll_pin *p_rclk;
>+	struct pin_pd *p_rclk_pd;
>+};
>+
>+int nsim_dpll_init_owner(struct nsim_dpll_info *dpll, int devid);
>+void nsim_dpll_free_owner(struct nsim_dpll_info *dpll);
>+int nsim_rclk_init(struct nsim_dpll_info *dpll, int devid, unsigned int index);
>+void nsim_rclk_free(struct nsim_dpll_info *dpll);
>+
>+#endif /* NSIM_DPLL_H */
>diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>index aecaf5f..78a936f 100644
>--- a/drivers/net/netdevsim/netdev.c
>+++ b/drivers/net/netdevsim/netdev.c
>@@ -25,6 +25,7 @@
> #include <net/udp_tunnel.h>
> 
> #include "netdevsim.h"
>+#include "dpll.h"
> 
> static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
> {
>@@ -344,6 +345,20 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
> 	if (err)
> 		goto err_ipsec_teardown;
> 	rtnl_unlock();
>+
>+	if (ns->nsim_dev_port->port_index == 0) {

Does not make any sense to treat port 0 any different.

Please, move the init of dpll device to drivers/net/netdevsim/dev.c
probably somewhere in nsim_drv_probe().


>+		err = nsim_dpll_init_owner(&ns->dpll,
>+					   ns->nsim_dev->nsim_bus_dev->dev.id);
>+		if (err)
>+			goto err_ipsec_teardown;
>+	}
>+
>+	err = nsim_rclk_init(&ns->dpll, ns->nsim_dev->nsim_bus_dev->dev.id,
>+			     ns->nsim_dev_port->port_index);

This is not related to netdev directly. Please move the pin init into
drivers/net/netdevsim/dev.c, probably somewhere inside
__nsim_dev_port_add()

Also, you don't call netdev_dpll_pin_set() and netdev_dpll_pin_clear().
That is actually what you should call here in netdev initialization.


>+
>+	if (err)
>+		goto err_ipsec_teardown;
>+
> 	return 0;
> 
> err_ipsec_teardown:
>@@ -419,6 +434,11 @@ void nsim_destroy(struct netdevsim *ns)
> 	if (nsim_dev_port_is_pf(ns->nsim_dev_port))
> 		nsim_udp_tunnels_info_destroy(dev);
> 	mock_phc_destroy(ns->phc);
>+
>+	nsim_rclk_free(&ns->dpll);
>+	if (ns->nsim_dev_port->port_index == 0)
>+		nsim_dpll_free_owner(&ns->dpll);
>+
> 	free_netdev(dev);
> }
> 
>diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>index 028c825..3d0138a 100644
>--- a/drivers/net/netdevsim/netdevsim.h
>+++ b/drivers/net/netdevsim/netdevsim.h
>@@ -26,6 +26,8 @@
> #include <net/xdp.h>
> #include <net/macsec.h>
> 
>+#include "dpll.h"
>+
> #define DRV_NAME	"netdevsim"
> 
> #define NSIM_XDP_MAX_MTU	4000
>@@ -125,6 +127,8 @@ struct netdevsim {
> 	} udp_ports;
> 
> 	struct nsim_ethtool ethtool;
>+
>+	struct nsim_dpll_info dpll;
> };
> 
> struct netdevsim *
>-- 
>2.9.5
>

RE: [PATCH RFC net-next v2 1/2] netdevsim: implement DPLL for subsystem selftests

[Michalik, Michal]

On 31 October 2023 12:00 PM CET, Jiri Pirko wrote:
> 
> Mon, Oct 30, 2023 at 05:53:25PM CET, michal.michalik@intel.com wrote:
>>DPLL subsystem integration tests require a module which mimics the
>>behavior of real driver which supports DPLL hardware. To fully test the
>>subsystem the netdevsim is amended with DPLL implementation.
>>
>>Signed-off-by: Michal Michalik <michal.michalik@intel.com>
>>---
>> drivers/net/Kconfig               |   1 +
>> drivers/net/netdevsim/Makefile    |   2 +-
>> drivers/net/netdevsim/dpll.c      | 438 ++++++++++++++++++++++++++++++++++++++
>> drivers/net/netdevsim/dpll.h      |  81 +++++++
>> drivers/net/netdevsim/netdev.c    |  20 ++
>> drivers/net/netdevsim/netdevsim.h |   4 +
>> 6 files changed, 545 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/net/netdevsim/dpll.c
>> create mode 100644 drivers/net/netdevsim/dpll.h
>>
>>diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>>index af0da4b..633ec89 100644
>>--- a/drivers/net/Kconfig
>>+++ b/drivers/net/Kconfig
>>@@ -626,6 +626,7 @@ config NETDEVSIM
>> 	depends on PSAMPLE || PSAMPLE=n
>> 	depends on PTP_1588_CLOCK_MOCK || PTP_1588_CLOCK_MOCK=n
>> 	select NET_DEVLINK
>>+	select DPLL
>> 	help
>> 	  This driver is a developer testing tool and software model that can
>> 	  be used to test various control path networking APIs, especially
>>diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
>>index f8de93b..f338ffb 100644
>>--- a/drivers/net/netdevsim/Makefile
>>+++ b/drivers/net/netdevsim/Makefile
>>@@ -3,7 +3,7 @@
>> obj-$(CONFIG_NETDEVSIM) += netdevsim.o
>> 
>> netdevsim-objs := \
>>-	netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o
>>+	netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o dpll.o
>> 
>> ifeq ($(CONFIG_BPF_SYSCALL),y)
>> netdevsim-objs += \
>>diff --git a/drivers/net/netdevsim/dpll.c b/drivers/net/netdevsim/dpll.c
>>new file mode 100644
>>index 0000000..050f68e
>>--- /dev/null
>>+++ b/drivers/net/netdevsim/dpll.c
>>@@ -0,0 +1,438 @@
>>+// SPDX-License-Identifier: GPL-2.0
>>+/*
>>+ * Copyright (c) 2023, Intel Corporation.
>>+ * Author: Michal Michalik <michal.michalik@intel.com>
>>+ */
>>+#include "dpll.h"
>>+
>>+static struct dpll_pin_properties *
>>+create_pin_properties(const char *label, enum dpll_pin_type type,
> 
> Please make sure to follow the namespace prefix notation in your patch
> everywhere, functions, structs, defines.
> "nsim_"
> 

Thanks - I overlooked that, will change.

>>+		      unsigned long caps, u32 freq_supp_num, u64 fmin, u64 fmax)
>>+{
>>+	struct dpll_pin_frequency *freq_supp;
>>+	struct dpll_pin_properties *pp;
>>+
>>+	pp = kzalloc(sizeof(*pp), GFP_KERNEL);
>>+	if (!pp)
>>+		return ERR_PTR(-ENOMEM);
>>+
>>+	freq_supp = kzalloc(sizeof(*freq_supp), GFP_KERNEL);
>>+	if (!freq_supp)
>>+		goto err;
>>+	*freq_supp =
>>+		(struct dpll_pin_frequency)DPLL_PIN_FREQUENCY_RANGE(fmin, fmax);
> 
> Drop the cast.
> 

Without the cast it does not compile.

>>+
>>+	pp->board_label = kasprintf(GFP_KERNEL, "%s_brd", label);
>>+	pp->panel_label = kasprintf(GFP_KERNEL, "%s_pnl", label);
>>+	pp->package_label = kasprintf(GFP_KERNEL, "%s_pcg", label);
>>+	pp->freq_supported_num = freq_supp_num;
>>+	pp->freq_supported = freq_supp;
>>+	pp->capabilities = caps;
>>+	pp->type = type;
>>+
>>+	return pp;
>>+err:
>>+	kfree(pp);
>>+	return ERR_PTR(-ENOMEM);
>>+}
>>+
>>+static struct dpll_pd *create_dpll_pd(int temperature, enum dpll_mode mode)
>>+{
>>+	struct dpll_pd *pd;
>>+
>>+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>>+	if (!pd)
>>+		return ERR_PTR(-ENOMEM);
>>+
>>+	pd->temperature = temperature;
>>+	pd->mode = mode;
>>+
>>+	return pd;
>>+}
>>+
>>+static struct pin_pd *create_pin_pd(u64 frequency, u32 prio,
>>+				    enum dpll_pin_direction direction)
>>+{
>>+	struct pin_pd *pd;
>>+
>>+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>>+	if (!pd)
>>+		return ERR_PTR(-ENOMEM);
>>+
>>+	pd->state_dpll = DPLL_PIN_STATE_DISCONNECTED;
>>+	pd->state_pin = DPLL_PIN_STATE_DISCONNECTED;
>>+	pd->frequency = frequency;
>>+	pd->direction = direction;
>>+	pd->prio = prio;
>>+
>>+	return pd;
>>+}
>>+
>>+static int
>>+dds_ops_mode_get(const struct dpll_device *dpll, void *dpll_priv,
>>+		 enum dpll_mode *mode, struct netlink_ext_ack *extack)
>>+{
>>+	*mode = ((struct dpll_pd *)(dpll_priv))->mode;
> 
> Please have variable assigned by dpll_priv instead of this.
> Also, fix in the rest of the code.
> 

Please excuse me, I don't think I understand what you mean. Do you mean I should
create a local variable struct dpll_pd and use it for assignment like that?
	```
	struct dpll_pd *pd = dpll_priv;
	*mode = pd->mode;
	```

>>+	return 0;
>>+};
>>+
>>+static bool
>>+dds_ops_mode_supported(const struct dpll_device *dpll, void *dpll_priv,
>>+		       const enum dpll_mode mode,
>>+		       struct netlink_ext_ack *extack)
>>+{
>>+	return true;
>>+};
>>+
>>+static int
>>+dds_ops_lock_status_get(const struct dpll_device *dpll, void *dpll_priv,
>>+			enum dpll_lock_status *status,
>>+			struct netlink_ext_ack *extack)
>>+{
>>+	if (((struct dpll_pd *)dpll_priv)->mode == DPLL_MODE_MANUAL)
>>+		*status = DPLL_LOCK_STATUS_LOCKED;
>>+	else
>>+		*status = DPLL_LOCK_STATUS_UNLOCKED;
> 
> I don't understand the logic of this function. According to mode you
> return if status is locked or not? For this, you should expose a debugfs
> knob so the user can emulate changes of the HW.
> 

Assumption was, we are testing the API not trying to simulate the actual DPLL HW
behavior. I was going for, the "simpler the better" principle. But since
somebody else might want to use it differently and test more complex scenarios,
I will add debugfs entries for this interaction.

>>+	return 0;
>>+};
>>+
>>+static int
>>+dds_ops_temp_get(const struct dpll_device *dpll, void *dpll_priv, s32 *temp,
>>+		 struct netlink_ext_ack *extack)
>>+{
>>+	*temp = ((struct dpll_pd *)dpll_priv)->temperature;
>>+	return 0;
>>+};
>>+
>>+static int
>>+pin_frequency_set(const struct dpll_pin *pin, void *pin_priv,
>>+		  const struct dpll_device *dpll, void *dpll_priv,
>>+		  const u64 frequency, struct netlink_ext_ack *extack)
>>+{
>>+	((struct pin_pd *)pin_priv)->frequency = frequency;
>>+	return 0;
>>+};
>>+
>>+static int
>>+pin_frequency_get(const struct dpll_pin *pin, void *pin_priv,
>>+		  const struct dpll_device *dpll, void *dpll_priv,
>>+		  u64 *frequency, struct netlink_ext_ack *extack)
>>+{
>>+	*frequency = ((struct pin_pd *)pin_priv)->frequency;
>>+	return 0;
>>+};
>>+
>>+static int
>>+pin_direction_set(const struct dpll_pin *pin, void *pin_priv,
>>+		  const struct dpll_device *dpll, void *dpll_priv,
>>+		  const enum dpll_pin_direction direction,
>>+		  struct netlink_ext_ack *extack)
>>+{
>>+	((struct pin_pd *)pin_priv)->direction = direction;
>>+	return 0;
>>+};
>>+
>>+static int
>>+pin_direction_get(const struct dpll_pin *pin, void *pin_priv,
>>+		  const struct dpll_device *dpll, void *dpll_priv,
>>+		  enum dpll_pin_direction *direction,
>>+		  struct netlink_ext_ack *extack)
>>+{
>>+	*direction = ((struct pin_pd *)pin_priv)->direction;
>>+	return 0;
>>+};
>>+
>>+static int
>>+pin_state_on_pin_get(const struct dpll_pin *pin, void *pin_priv,
>>+		     const struct dpll_pin *parent_pin, void *parent_priv,
>>+		     enum dpll_pin_state *state,
>>+		     struct netlink_ext_ack *extack)
>>+{
>>+	*state = ((struct pin_pd *)pin_priv)->state_pin;
>>+	return 0;
>>+};
>>+
>>+static int
>>+pin_state_on_dpll_get(const struct dpll_pin *pin, void *pin_priv,
>>+		      const struct dpll_device *dpll, void *dpll_priv,
>>+		      enum dpll_pin_state *state,
>>+		      struct netlink_ext_ack *extack)
>>+{
>>+	*state = ((struct pin_pd *)pin_priv)->state_dpll;
>>+	return 0;
>>+};
>>+
>>+static int
>>+pin_state_on_pin_set(const struct dpll_pin *pin, void *pin_priv,
>>+		     const struct dpll_pin *parent_pin, void *parent_priv,
>>+		     const enum dpll_pin_state state,
>>+		     struct netlink_ext_ack *extack)
>>+{
>>+	((struct pin_pd *)pin_priv)->state_pin = state;
>>+	return 0;
>>+};
>>+
>>+static int
>>+pin_state_on_dpll_set(const struct dpll_pin *pin, void *pin_priv,
>>+		      const struct dpll_device *dpll, void *dpll_priv,
>>+		      const enum dpll_pin_state state,
>>+		      struct netlink_ext_ack *extack)
>>+{
>>+	((struct pin_pd *)pin_priv)->state_dpll = state;
>>+	return 0;
>>+};
>>+
>>+static int
>>+pin_prio_get(const struct dpll_pin *pin, void *pin_priv,
>>+	     const struct dpll_device *dpll, void *dpll_priv,
>>+	     u32 *prio, struct netlink_ext_ack *extack)
>>+{
>>+	*prio = ((struct pin_pd *)pin_priv)->prio;
>>+	return 0;
>>+};
>>+
>>+static int
>>+pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
>>+	     const struct dpll_device *dpll, void *dpll_priv,
>>+	     const u32 prio, struct netlink_ext_ack *extack)
>>+{
>>+	((struct pin_pd *)pin_priv)->prio = prio;
>>+	return 0;
>>+};
>>+
>>+static void
>>+free_pin_properties(struct dpll_pin_properties *pp)
>>+{
>>+	if (pp) {
> 
> How exactly pp could be null?
> 

In normal circumstances it seems it cannot, I will remove the check.

>>+		kfree(pp->board_label);
>>+		kfree(pp->panel_label);
>>+		kfree(pp->package_label);
>>+		kfree(pp->freq_supported);
>>+		kfree(pp);
>>+	}
>>+}
>>+
>>+static struct dpll_device_ops dds_ops = {
>>+	.mode_get = dds_ops_mode_get,
>>+	.mode_supported = dds_ops_mode_supported,
>>+	.lock_status_get = dds_ops_lock_status_get,
>>+	.temp_get = dds_ops_temp_get,
>>+};
>>+
>>+static struct dpll_pin_ops pin_ops = {
>>+	.frequency_set = pin_frequency_set,
>>+	.frequency_get = pin_frequency_get,
>>+	.direction_set = pin_direction_set,
>>+	.direction_get = pin_direction_get,
>>+	.state_on_pin_get = pin_state_on_pin_get,
>>+	.state_on_dpll_get = pin_state_on_dpll_get,
>>+	.state_on_pin_set = pin_state_on_pin_set,
>>+	.state_on_dpll_set = pin_state_on_dpll_set,
>>+	.prio_get = pin_prio_get,
>>+	.prio_set = pin_prio_set,
>>+};
>>+
>>+int nsim_dpll_init_owner(struct nsim_dpll_info *dpll, int devid)
>>+{
>>+	/* Create EEC DPLL */
>>+	dpll->dpll_e = dpll_device_get(DPLLS_CLOCK_ID + devid, EEC_DPLL_DEV,
> 
> "#define DPLLS_CLOCK_ID 234"
> 
> You guys always come up with some funky way of making up ids and names.
> Why this can't be randomly generated u64?
> 

As mentioned, "the simpler the better". Having it randomly generated implies
need of exposing this randomly generated number to tests. Test need to be able
unambiguously get this clock. But since I will be adding debugfs entries to
change the lock state, I can also add one for returning clock id. I need that
because I'm using more devices per one netdevsim module.

>>+				       THIS_MODULE);
>>+	if (IS_ERR(dpll->dpll_e))
>>+		goto dpll_e;
>>+	dpll->dpll_e_pd = create_dpll_pd(EEC_DPLL_TEMPERATURE,
>>+					 DPLL_MODE_AUTOMATIC);
>>+	if (IS_ERR(dpll->dpll_e))
>>+		goto dpll_e_pd;
>>+	if (dpll_device_register(dpll->dpll_e, DPLL_TYPE_EEC, &dds_ops,
>>+				 (void *)dpll->dpll_e_pd))
> 
> Please avoid pointless casts. (void *) cast for arg of type (void *) are
> especially pointless. Please make sure to fix this in the rest of the
> code as well.
> 

Thanks, will do.

>>+		goto e_reg;
>>+
>>+	/* Create PPS DPLL */
>>+	dpll->dpll_p = dpll_device_get(DPLLS_CLOCK_ID + devid, PPS_DPLL_DEV,
>>+				       THIS_MODULE);
>>+	if (IS_ERR(dpll->dpll_p))
>>+		goto dpll_p;
>>+	dpll->dpll_p_pd = create_dpll_pd(PPS_DPLL_TEMPERATURE,
>>+					 DPLL_MODE_MANUAL);
>>+	if (IS_ERR(dpll->dpll_p_pd))
>>+		goto dpll_p_pd;
>>+	if (dpll_device_register(dpll->dpll_p, DPLL_TYPE_PPS, &dds_ops,
> 
> You always use "int err" to store the return value of function calls
> returning 0/-EXXX like this one.
> 

Lesson learned, will fix.

>>+				 (void *)dpll->dpll_p_pd))
>>+		goto p_reg;
>>+
>>+	/* Create first pin (GNSS) */
>>+	dpll->pp_gnss = create_pin_properties("GNSS", DPLL_PIN_TYPE_GNSS,
>>+					      PIN_GNSS_CAPABILITIES,
>>+					      1, DPLL_PIN_FREQUENCY_1_HZ,
>>+					      DPLL_PIN_FREQUENCY_1_HZ);
>>+	if (IS_ERR(dpll->pp_gnss))
>>+		goto pp_gnss;
>>+	dpll->p_gnss = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_GNSS,
>>+				    THIS_MODULE,
>>+				    dpll->pp_gnss);
>>+	if (IS_ERR(dpll->p_gnss))
>>+		goto p_gnss;
>>+	dpll->p_gnss_pd = create_pin_pd(DPLL_PIN_FREQUENCY_1_HZ,
>>+					PIN_GNSS_PRIORITY,
>>+					DPLL_PIN_DIRECTION_INPUT);
>>+	if (IS_ERR(dpll->p_gnss_pd))
>>+		goto p_gnss_pd;
>>+	if (dpll_pin_register(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>>+			      (void *)dpll->p_gnss_pd))
>>+		goto e_gnss_reg;
>>+
>>+	/* Create second pin (PPS) */
>>+	dpll->pp_pps = create_pin_properties("PPS", DPLL_PIN_TYPE_EXT,
>>+					     PIN_PPS_CAPABILITIES,
>>+					     1, DPLL_PIN_FREQUENCY_1_HZ,
>>+					     DPLL_PIN_FREQUENCY_1_HZ);
>>+	if (IS_ERR(dpll->pp_pps))
>>+		goto pp_pps;
>>+	dpll->p_pps = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_PPS, THIS_MODULE,
>>+				   dpll->pp_pps);
>>+	if (IS_ERR(dpll->p_pps))
>>+		goto p_pps;
>>+	dpll->p_pps_pd = create_pin_pd(DPLL_PIN_FREQUENCY_1_HZ,
>>+				       PIN_PPS_PRIORITY,
>>+				       DPLL_PIN_DIRECTION_INPUT);
>>+	if (IS_ERR(dpll->p_pps_pd))
>>+		goto p_pps_pd;
>>+	if (dpll_pin_register(dpll->dpll_e, dpll->p_pps, &pin_ops,
>>+			      (void *)dpll->p_pps_pd))
>>+		goto e_pps_reg;
>>+	if (dpll_pin_register(dpll->dpll_p, dpll->p_pps, &pin_ops,
>>+			      (void *)dpll->p_pps_pd))
>>+		goto p_pps_reg;
>>+
>>+	return 0;
>>+
>>+p_pps_reg:
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &pin_ops,
>>+			    (void *)dpll->p_pps_pd);
>>+e_pps_reg:
>>+	kfree(dpll->p_pps_pd);
>>+p_pps_pd:
>>+	dpll_pin_put(dpll->p_pps);
>>+p_pps:
>>+	free_pin_properties(dpll->pp_pps);
>>+pp_pps:
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>>+			    (void *)dpll->p_gnss_pd);
>>+e_gnss_reg:
>>+	kfree(dpll->p_gnss_pd);
>>+p_gnss_pd:
>>+	dpll_pin_put(dpll->p_gnss);
>>+p_gnss:
>>+	free_pin_properties(dpll->pp_gnss);
>>+pp_gnss:
>>+	dpll_device_unregister(dpll->dpll_p, &dds_ops, (void *)dpll->dpll_p_pd);
>>+p_reg:
>>+	kfree(dpll->dpll_p_pd);
>>+dpll_p_pd:
>>+	dpll_device_put(dpll->dpll_p);
>>+dpll_p:
>>+	dpll_device_unregister(dpll->dpll_e, &dds_ops, (void *)dpll->dpll_e_pd);
>>+e_reg:
>>+	kfree(dpll->dpll_e_pd);
>>+dpll_e_pd:
>>+	dpll_device_put(dpll->dpll_e);
>>+dpll_e:
>>+	return -1;
>>+}
>>+
>>+void nsim_dpll_free_owner(struct nsim_dpll_info *dpll)
>>+{
>>+	/* Free GNSS pin */
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>>+			    (void *)dpll->p_gnss_pd);
>>+	dpll_pin_put(dpll->p_gnss);
>>+	free_pin_properties(dpll->pp_gnss);
>>+	kfree(dpll->p_gnss_pd);
>>+
>>+	/* Free PPS pin */
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &pin_ops,
>>+			    (void *)dpll->p_pps_pd);
>>+	dpll_pin_unregister(dpll->dpll_p, dpll->p_pps, &pin_ops,
>>+			    (void *)dpll->p_pps_pd);
>>+	dpll_pin_put(dpll->p_pps);
>>+	free_pin_properties(dpll->pp_pps);
>>+	kfree(dpll->p_pps_pd);
>>+
>>+	/* Free DPLL EEC */
>>+	dpll_device_unregister(dpll->dpll_e, &dds_ops, (void *)dpll->dpll_e_pd);
>>+	dpll_device_put(dpll->dpll_e);
>>+	kfree(dpll->dpll_e_pd);
>>+
>>+	/* Free DPLL PPS */
>>+	dpll_device_unregister(dpll->dpll_p, &dds_ops, (void *)dpll->dpll_p_pd);
>>+	dpll_device_put(dpll->dpll_p);
>>+	kfree(dpll->dpll_p_pd);
>>+}
>>+
>>+int nsim_rclk_init(struct nsim_dpll_info *dpll, int devid, unsigned int index)
>>+{
>>+	char *name = kasprintf(GFP_KERNEL, "RCLK_%i", index);
>>+
>>+	/* Get EEC DPLL */
>>+	dpll->dpll_e = dpll_device_get(DPLLS_CLOCK_ID + devid, EEC_DPLL_DEV,
>>+				       THIS_MODULE);
>>+	if (IS_ERR(dpll->dpll_e))
>>+		goto dpll;
>>+
>>+	/* Get PPS DPLL */
>>+	dpll->dpll_p = dpll_device_get(DPLLS_CLOCK_ID + devid, PPS_DPLL_DEV,
>>+				       THIS_MODULE);
>>+	if (IS_ERR(dpll->dpll_p))
>>+		goto dpll;
>>+
>>+	/* Create Recovered clock pin (RCLK) */
>>+	dpll->pp_rclk = create_pin_properties(name,
>>+					      DPLL_PIN_TYPE_SYNCE_ETH_PORT,
>>+					      PIN_RCLK_CAPABILITIES, 1, 1e6,
>>+					      125e6);
>>+	if (IS_ERR(dpll->pp_rclk))
>>+		goto dpll;
>>+	dpll->p_rclk = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_RCLK + index,
>>+				    THIS_MODULE, dpll->pp_rclk);
>>+	if (IS_ERR(dpll->p_rclk))
>>+		goto p_rclk;
>>+	dpll->p_rclk_pd = create_pin_pd(DPLL_PIN_FREQUENCY_10_MHZ,
>>+					PIN_RCLK_PRIORITY,
>>+					DPLL_PIN_DIRECTION_INPUT);
>>+	if (IS_ERR(dpll->p_rclk_pd))
>>+		goto p_rclk_pd;
>>+	if (dpll_pin_register(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>>+			      (void *)dpll->p_rclk_pd))
>>+		goto dpll_e_reg;
>>+	if (dpll_pin_register(dpll->dpll_p, dpll->p_rclk, &pin_ops,
>>+			      (void *)dpll->p_rclk_pd))
>>+		goto dpll_p_reg;
>>+
>>+	return 0;
>>+
>>+dpll_p_reg:
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>>+			    (void *)dpll->p_rclk_pd);
>>+dpll_e_reg:
>>+	kfree(dpll->p_rclk_pd);
>>+p_rclk_pd:
>>+	dpll_pin_put(dpll->p_rclk);
>>+p_rclk:
>>+	free_pin_properties(dpll->pp_rclk);
>>+dpll:
>>+	return -1;
>>+}
>>+
>>+void nsim_rclk_free(struct nsim_dpll_info *dpll)
>>+{
>>+	/* Free RCLK pin */
>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>>+			    (void *)dpll->p_rclk_pd);
>>+	dpll_pin_unregister(dpll->dpll_p, dpll->p_rclk, &pin_ops,
>>+			    (void *)dpll->p_rclk_pd);
>>+	dpll_pin_put(dpll->p_rclk);
>>+	free_pin_properties(dpll->pp_rclk);
>>+	kfree(dpll->p_rclk_pd);
>>+	dpll_device_put(dpll->dpll_e);
>>+	dpll_device_put(dpll->dpll_p);
>>+}
>>diff --git a/drivers/net/netdevsim/dpll.h b/drivers/net/netdevsim/dpll.h
>>new file mode 100644
>>index 0000000..17db7f7
>>--- /dev/null
>>+++ b/drivers/net/netdevsim/dpll.h
>>@@ -0,0 +1,81 @@
>>+/* SPDX-License-Identifier: GPL-2.0 */
>>+/*
>>+ * Copyright (c) 2023, Intel Corporation.
>>+ * Author: Michal Michalik <michal.michalik@intel.com>
>>+ */
>>+
>>+#ifndef NSIM_DPLL_H
>>+#define NSIM_DPLL_H
> 
> Why you need a separate header for this? Just put necessary parts in
> netdevsim.h and leave the rest in the .c file.
> 

Good idea, thanks.

>>+
>>+#include <linux/types.h>
>>+#include <linux/netlink.h>
>>+
>>+#include <linux/dpll.h>
>>+#include <uapi/linux/dpll.h>
> 
> Why exactly do you need to include uapi header directly?
> 

You are right - will refactor that.

>>+
>>+#define EEC_DPLL_DEV 0
>>+#define EEC_DPLL_TEMPERATURE 20
>>+#define PPS_DPLL_DEV 1
>>+#define PPS_DPLL_TEMPERATURE 30
>>+#define DPLLS_CLOCK_ID 234
>>+
>>+#define PIN_GNSS 0
>>+#define PIN_GNSS_CAPABILITIES 2 /* DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE */
>>+#define PIN_GNSS_PRIORITY 5
>>+
>>+#define PIN_PPS 1
>>+#define PIN_PPS_CAPABILITIES 7 /* DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE
>>+				* || DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE
>>+				* || DPLL_PIN_CAPS_STATE_CAN_CHANGE
> 
> You are kidding, correct? :)
> 

Not really, it's written directly because the tests are able to read the value
from here (they don't understand DPLL subsystem defines). Since we are changing
most of the code I will try to make the tests access this data differently (e.g.
via debugfs).

>>+				*/
>>+#define PIN_PPS_PRIORITY 6
>>+
>>+#define PIN_RCLK 2
>>+#define PIN_RCLK_CAPABILITIES 6 /* DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE
>>+				 * || DPLL_PIN_CAPS_STATE_CAN_CHANGE
>>+				 */
>>+#define PIN_RCLK_PRIORITY 7
>>+
>>+#define EEC_PINS_NUMBER 3
>>+#define PPS_PINS_NUMBER 2
>>+
>>+struct dpll_pd {
> 
> Have not clue what do you mean by "pd".
> 

I meant "private data", will change this to something more meaningful.

>>+	enum dpll_mode mode;
>>+	int temperature;
>>+};
>>+
>>+struct pin_pd {
>>+	u64 frequency;
>>+	enum dpll_pin_direction direction;
>>+	enum dpll_pin_state state_pin;
>>+	enum dpll_pin_state state_dpll;
>>+	u32 prio;
>>+};
>>+
>>+struct nsim_dpll_info {
> 
> Drop "info".
> 

OK.

>>+	bool owner;
>>+
>>+	struct dpll_device *dpll_e;
>>+	struct dpll_pd *dpll_e_pd;
>>+	struct dpll_device *dpll_p;
>>+	struct dpll_pd *dpll_p_pd;
>>+
>>+	struct dpll_pin_properties *pp_gnss;
> 
> Why pointer? Just embed the properties struct, no?
> 

I can change both private data and properties to be embeded.

>>+	struct dpll_pin *p_gnss;
>>+	struct pin_pd *p_gnss_pd;
>>+
>>+	struct dpll_pin_properties *pp_pps;
>>+	struct dpll_pin *p_pps;
>>+	struct pin_pd *p_pps_pd;
>>+
>>+	struct dpll_pin_properties *pp_rclk;
>>+	struct dpll_pin *p_rclk;
>>+	struct pin_pd *p_rclk_pd;
>>+};
>>+
>>+int nsim_dpll_init_owner(struct nsim_dpll_info *dpll, int devid);
>>+void nsim_dpll_free_owner(struct nsim_dpll_info *dpll);
>>+int nsim_rclk_init(struct nsim_dpll_info *dpll, int devid, unsigned int index);
>>+void nsim_rclk_free(struct nsim_dpll_info *dpll);
>>+
>>+#endif /* NSIM_DPLL_H */
>>diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>>index aecaf5f..78a936f 100644
>>--- a/drivers/net/netdevsim/netdev.c
>>+++ b/drivers/net/netdevsim/netdev.c
>>@@ -25,6 +25,7 @@
>> #include <net/udp_tunnel.h>
>> 
>> #include "netdevsim.h"
>>+#include "dpll.h"
>> 
>> static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>> {
>>@@ -344,6 +345,20 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
>> 	if (err)
>> 		goto err_ipsec_teardown;
>> 	rtnl_unlock();
>>+
>>+	if (ns->nsim_dev_port->port_index == 0) {
> 
> Does not make any sense to treat port 0 any different.
> 
> Please, move the init of dpll device to drivers/net/netdevsim/dev.c
> probably somewhere in nsim_drv_probe().
> 

Great idea - I will do it.

>>+		err = nsim_dpll_init_owner(&ns->dpll,
>>+					   ns->nsim_dev->nsim_bus_dev->dev.id);
>>+		if (err)
>>+			goto err_ipsec_teardown;
>>+	}
>>+
>>+	err = nsim_rclk_init(&ns->dpll, ns->nsim_dev->nsim_bus_dev->dev.id,
>>+			     ns->nsim_dev_port->port_index);
> 
> This is not related to netdev directly. Please move the pin init into
> drivers/net/netdevsim/dev.c, probably somewhere inside
> __nsim_dev_port_add()
> 
> Also, you don't call netdev_dpll_pin_set() and netdev_dpll_pin_clear().
> That is actually what you should call here in netdev initialization.
> 

Good catch - I will move to dev.c and use netdev_dpll_pin_set/clear()

>>+
>>+	if (err)
>>+		goto err_ipsec_teardown;
>>+
>> 	return 0;
>> 
>> err_ipsec_teardown:
>>@@ -419,6 +434,11 @@ void nsim_destroy(struct netdevsim *ns)
>> 	if (nsim_dev_port_is_pf(ns->nsim_dev_port))
>> 		nsim_udp_tunnels_info_destroy(dev);
>> 	mock_phc_destroy(ns->phc);
>>+
>>+	nsim_rclk_free(&ns->dpll);
>>+	if (ns->nsim_dev_port->port_index == 0)
>>+		nsim_dpll_free_owner(&ns->dpll);
>>+
>> 	free_netdev(dev);
>> }
>> 
>>diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>>index 028c825..3d0138a 100644
>>--- a/drivers/net/netdevsim/netdevsim.h
>>+++ b/drivers/net/netdevsim/netdevsim.h
>>@@ -26,6 +26,8 @@
>> #include <net/xdp.h>
>> #include <net/macsec.h>
>> 
>>+#include "dpll.h"
>>+
>> #define DRV_NAME	"netdevsim"
>> 
>> #define NSIM_XDP_MAX_MTU	4000
>>@@ -125,6 +127,8 @@ struct netdevsim {
>> 	} udp_ports;
>> 
>> 	struct nsim_ethtool ethtool;
>>+
>>+	struct nsim_dpll_info dpll;
>> };
>> 
>> struct netdevsim *
>>-- 
>>2.9.5
>>

Re: [PATCH RFC net-next v2 1/2] netdevsim: implement DPLL for subsystem selftests

[Jiri Pirko]

Fri, Nov 03, 2023 at 06:45:25PM CET, michal.michalik@intel.com wrote:
>On 31 October 2023 12:00 PM CET, Jiri Pirko wrote:
>> 
>> Mon, Oct 30, 2023 at 05:53:25PM CET, michal.michalik@intel.com wrote:
>>>DPLL subsystem integration tests require a module which mimics the
>>>behavior of real driver which supports DPLL hardware. To fully test the
>>>subsystem the netdevsim is amended with DPLL implementation.
>>>
>>>Signed-off-by: Michal Michalik <michal.michalik@intel.com>
>>>---
>>> drivers/net/Kconfig               |   1 +
>>> drivers/net/netdevsim/Makefile    |   2 +-
>>> drivers/net/netdevsim/dpll.c      | 438 ++++++++++++++++++++++++++++++++++++++
>>> drivers/net/netdevsim/dpll.h      |  81 +++++++
>>> drivers/net/netdevsim/netdev.c    |  20 ++
>>> drivers/net/netdevsim/netdevsim.h |   4 +
>>> 6 files changed, 545 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/net/netdevsim/dpll.c
>>> create mode 100644 drivers/net/netdevsim/dpll.h
>>>
>>>diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>>>index af0da4b..633ec89 100644
>>>--- a/drivers/net/Kconfig
>>>+++ b/drivers/net/Kconfig
>>>@@ -626,6 +626,7 @@ config NETDEVSIM
>>> 	depends on PSAMPLE || PSAMPLE=n
>>> 	depends on PTP_1588_CLOCK_MOCK || PTP_1588_CLOCK_MOCK=n
>>> 	select NET_DEVLINK
>>>+	select DPLL
>>> 	help
>>> 	  This driver is a developer testing tool and software model that can
>>> 	  be used to test various control path networking APIs, especially
>>>diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
>>>index f8de93b..f338ffb 100644
>>>--- a/drivers/net/netdevsim/Makefile
>>>+++ b/drivers/net/netdevsim/Makefile
>>>@@ -3,7 +3,7 @@
>>> obj-$(CONFIG_NETDEVSIM) += netdevsim.o
>>> 
>>> netdevsim-objs := \
>>>-	netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o
>>>+	netdev.o dev.o ethtool.o fib.o bus.o health.o hwstats.o udp_tunnels.o dpll.o
>>> 
>>> ifeq ($(CONFIG_BPF_SYSCALL),y)
>>> netdevsim-objs += \
>>>diff --git a/drivers/net/netdevsim/dpll.c b/drivers/net/netdevsim/dpll.c
>>>new file mode 100644
>>>index 0000000..050f68e
>>>--- /dev/null
>>>+++ b/drivers/net/netdevsim/dpll.c
>>>@@ -0,0 +1,438 @@
>>>+// SPDX-License-Identifier: GPL-2.0
>>>+/*
>>>+ * Copyright (c) 2023, Intel Corporation.
>>>+ * Author: Michal Michalik <michal.michalik@intel.com>
>>>+ */
>>>+#include "dpll.h"
>>>+
>>>+static struct dpll_pin_properties *
>>>+create_pin_properties(const char *label, enum dpll_pin_type type,
>> 
>> Please make sure to follow the namespace prefix notation in your patch
>> everywhere, functions, structs, defines.
>> "nsim_"
>> 
>
>Thanks - I overlooked that, will change.
>
>>>+		      unsigned long caps, u32 freq_supp_num, u64 fmin, u64 fmax)
>>>+{
>>>+	struct dpll_pin_frequency *freq_supp;
>>>+	struct dpll_pin_properties *pp;
>>>+
>>>+	pp = kzalloc(sizeof(*pp), GFP_KERNEL);
>>>+	if (!pp)
>>>+		return ERR_PTR(-ENOMEM);
>>>+
>>>+	freq_supp = kzalloc(sizeof(*freq_supp), GFP_KERNEL);
>>>+	if (!freq_supp)
>>>+		goto err;
>>>+	*freq_supp =
>>>+		(struct dpll_pin_frequency)DPLL_PIN_FREQUENCY_RANGE(fmin, fmax);
>> 
>> Drop the cast.

Then manage this differently without cast. Setter pelper perhaps? Figure
that out.


>> 
>
>Without the cast it does not compile.
>
>>>+
>>>+	pp->board_label = kasprintf(GFP_KERNEL, "%s_brd", label);
>>>+	pp->panel_label = kasprintf(GFP_KERNEL, "%s_pnl", label);
>>>+	pp->package_label = kasprintf(GFP_KERNEL, "%s_pcg", label);
>>>+	pp->freq_supported_num = freq_supp_num;
>>>+	pp->freq_supported = freq_supp;
>>>+	pp->capabilities = caps;
>>>+	pp->type = type;
>>>+
>>>+	return pp;
>>>+err:
>>>+	kfree(pp);
>>>+	return ERR_PTR(-ENOMEM);
>>>+}
>>>+
>>>+static struct dpll_pd *create_dpll_pd(int temperature, enum dpll_mode mode)
>>>+{
>>>+	struct dpll_pd *pd;
>>>+
>>>+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>>>+	if (!pd)
>>>+		return ERR_PTR(-ENOMEM);
>>>+
>>>+	pd->temperature = temperature;
>>>+	pd->mode = mode;
>>>+
>>>+	return pd;
>>>+}
>>>+
>>>+static struct pin_pd *create_pin_pd(u64 frequency, u32 prio,
>>>+				    enum dpll_pin_direction direction)
>>>+{
>>>+	struct pin_pd *pd;
>>>+
>>>+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>>>+	if (!pd)
>>>+		return ERR_PTR(-ENOMEM);
>>>+
>>>+	pd->state_dpll = DPLL_PIN_STATE_DISCONNECTED;
>>>+	pd->state_pin = DPLL_PIN_STATE_DISCONNECTED;
>>>+	pd->frequency = frequency;
>>>+	pd->direction = direction;
>>>+	pd->prio = prio;
>>>+
>>>+	return pd;
>>>+}
>>>+
>>>+static int
>>>+dds_ops_mode_get(const struct dpll_device *dpll, void *dpll_priv,
>>>+		 enum dpll_mode *mode, struct netlink_ext_ack *extack)
>>>+{
>>>+	*mode = ((struct dpll_pd *)(dpll_priv))->mode;
>> 
>> Please have variable assigned by dpll_priv instead of this.
>> Also, fix in the rest of the code.
>> 
>
>Please excuse me, I don't think I understand what you mean. Do you mean I should
>create a local variable struct dpll_pd and use it for assignment like that?
>	```
>	struct dpll_pd *pd = dpll_priv;
>	*mode = pd->mode;
>	```

Yes.


>
>>>+	return 0;
>>>+};
>>>+
>>>+static bool
>>>+dds_ops_mode_supported(const struct dpll_device *dpll, void *dpll_priv,
>>>+		       const enum dpll_mode mode,
>>>+		       struct netlink_ext_ack *extack)
>>>+{
>>>+	return true;
>>>+};
>>>+
>>>+static int
>>>+dds_ops_lock_status_get(const struct dpll_device *dpll, void *dpll_priv,
>>>+			enum dpll_lock_status *status,
>>>+			struct netlink_ext_ack *extack)
>>>+{
>>>+	if (((struct dpll_pd *)dpll_priv)->mode == DPLL_MODE_MANUAL)
>>>+		*status = DPLL_LOCK_STATUS_LOCKED;
>>>+	else
>>>+		*status = DPLL_LOCK_STATUS_UNLOCKED;
>> 
>> I don't understand the logic of this function. According to mode you
>> return if status is locked or not? For this, you should expose a debugfs
>> knob so the user can emulate changes of the HW.
>> 
>
>Assumption was, we are testing the API not trying to simulate the actual DPLL HW
>behavior. I was going for, the "simpler the better" principle. But since
>somebody else might want to use it differently and test more complex scenarios,
>I will add debugfs entries for this interaction.

Great.


>
>>>+	return 0;
>>>+};
>>>+
>>>+static int
>>>+dds_ops_temp_get(const struct dpll_device *dpll, void *dpll_priv, s32 *temp,
>>>+		 struct netlink_ext_ack *extack)
>>>+{
>>>+	*temp = ((struct dpll_pd *)dpll_priv)->temperature;
>>>+	return 0;
>>>+};
>>>+
>>>+static int
>>>+pin_frequency_set(const struct dpll_pin *pin, void *pin_priv,
>>>+		  const struct dpll_device *dpll, void *dpll_priv,
>>>+		  const u64 frequency, struct netlink_ext_ack *extack)
>>>+{
>>>+	((struct pin_pd *)pin_priv)->frequency = frequency;
>>>+	return 0;
>>>+};
>>>+
>>>+static int
>>>+pin_frequency_get(const struct dpll_pin *pin, void *pin_priv,
>>>+		  const struct dpll_device *dpll, void *dpll_priv,
>>>+		  u64 *frequency, struct netlink_ext_ack *extack)
>>>+{
>>>+	*frequency = ((struct pin_pd *)pin_priv)->frequency;
>>>+	return 0;
>>>+};
>>>+
>>>+static int
>>>+pin_direction_set(const struct dpll_pin *pin, void *pin_priv,
>>>+		  const struct dpll_device *dpll, void *dpll_priv,
>>>+		  const enum dpll_pin_direction direction,
>>>+		  struct netlink_ext_ack *extack)
>>>+{
>>>+	((struct pin_pd *)pin_priv)->direction = direction;
>>>+	return 0;
>>>+};
>>>+
>>>+static int
>>>+pin_direction_get(const struct dpll_pin *pin, void *pin_priv,
>>>+		  const struct dpll_device *dpll, void *dpll_priv,
>>>+		  enum dpll_pin_direction *direction,
>>>+		  struct netlink_ext_ack *extack)
>>>+{
>>>+	*direction = ((struct pin_pd *)pin_priv)->direction;
>>>+	return 0;
>>>+};
>>>+
>>>+static int
>>>+pin_state_on_pin_get(const struct dpll_pin *pin, void *pin_priv,
>>>+		     const struct dpll_pin *parent_pin, void *parent_priv,
>>>+		     enum dpll_pin_state *state,
>>>+		     struct netlink_ext_ack *extack)
>>>+{
>>>+	*state = ((struct pin_pd *)pin_priv)->state_pin;
>>>+	return 0;
>>>+};
>>>+
>>>+static int
>>>+pin_state_on_dpll_get(const struct dpll_pin *pin, void *pin_priv,
>>>+		      const struct dpll_device *dpll, void *dpll_priv,
>>>+		      enum dpll_pin_state *state,
>>>+		      struct netlink_ext_ack *extack)
>>>+{
>>>+	*state = ((struct pin_pd *)pin_priv)->state_dpll;
>>>+	return 0;
>>>+};
>>>+
>>>+static int
>>>+pin_state_on_pin_set(const struct dpll_pin *pin, void *pin_priv,
>>>+		     const struct dpll_pin *parent_pin, void *parent_priv,
>>>+		     const enum dpll_pin_state state,
>>>+		     struct netlink_ext_ack *extack)
>>>+{
>>>+	((struct pin_pd *)pin_priv)->state_pin = state;
>>>+	return 0;
>>>+};
>>>+
>>>+static int
>>>+pin_state_on_dpll_set(const struct dpll_pin *pin, void *pin_priv,
>>>+		      const struct dpll_device *dpll, void *dpll_priv,
>>>+		      const enum dpll_pin_state state,
>>>+		      struct netlink_ext_ack *extack)
>>>+{
>>>+	((struct pin_pd *)pin_priv)->state_dpll = state;
>>>+	return 0;
>>>+};
>>>+
>>>+static int
>>>+pin_prio_get(const struct dpll_pin *pin, void *pin_priv,
>>>+	     const struct dpll_device *dpll, void *dpll_priv,
>>>+	     u32 *prio, struct netlink_ext_ack *extack)
>>>+{
>>>+	*prio = ((struct pin_pd *)pin_priv)->prio;
>>>+	return 0;
>>>+};
>>>+
>>>+static int
>>>+pin_prio_set(const struct dpll_pin *pin, void *pin_priv,
>>>+	     const struct dpll_device *dpll, void *dpll_priv,
>>>+	     const u32 prio, struct netlink_ext_ack *extack)
>>>+{
>>>+	((struct pin_pd *)pin_priv)->prio = prio;
>>>+	return 0;
>>>+};
>>>+
>>>+static void
>>>+free_pin_properties(struct dpll_pin_properties *pp)
>>>+{
>>>+	if (pp) {
>> 
>> How exactly pp could be null?
>> 
>
>In normal circumstances it seems it cannot, I will remove the check.

Good.


>
>>>+		kfree(pp->board_label);
>>>+		kfree(pp->panel_label);
>>>+		kfree(pp->package_label);
>>>+		kfree(pp->freq_supported);
>>>+		kfree(pp);
>>>+	}
>>>+}
>>>+
>>>+static struct dpll_device_ops dds_ops = {
>>>+	.mode_get = dds_ops_mode_get,
>>>+	.mode_supported = dds_ops_mode_supported,
>>>+	.lock_status_get = dds_ops_lock_status_get,
>>>+	.temp_get = dds_ops_temp_get,
>>>+};
>>>+
>>>+static struct dpll_pin_ops pin_ops = {
>>>+	.frequency_set = pin_frequency_set,
>>>+	.frequency_get = pin_frequency_get,
>>>+	.direction_set = pin_direction_set,
>>>+	.direction_get = pin_direction_get,
>>>+	.state_on_pin_get = pin_state_on_pin_get,
>>>+	.state_on_dpll_get = pin_state_on_dpll_get,
>>>+	.state_on_pin_set = pin_state_on_pin_set,
>>>+	.state_on_dpll_set = pin_state_on_dpll_set,
>>>+	.prio_get = pin_prio_get,
>>>+	.prio_set = pin_prio_set,
>>>+};
>>>+
>>>+int nsim_dpll_init_owner(struct nsim_dpll_info *dpll, int devid)
>>>+{
>>>+	/* Create EEC DPLL */
>>>+	dpll->dpll_e = dpll_device_get(DPLLS_CLOCK_ID + devid, EEC_DPLL_DEV,
>> 
>> "#define DPLLS_CLOCK_ID 234"
>> 
>> You guys always come up with some funky way of making up ids and names.
>> Why this can't be randomly generated u64?
>> 
>
>As mentioned, "the simpler the better". Having it randomly generated implies
>need of exposing this randomly generated number to tests. Test need to be able
>unambiguously get this clock. But since I will be adding debugfs entries to
>change the lock state, I can also add one for returning clock id. I need that
>because I'm using more devices per one netdevsim module.

The test can easily get anything according to the pin associated with
netdevice. This is the entry point. Not only for tests, but also for any
dpll/synce app/daemon.



>
>>>+				       THIS_MODULE);
>>>+	if (IS_ERR(dpll->dpll_e))
>>>+		goto dpll_e;
>>>+	dpll->dpll_e_pd = create_dpll_pd(EEC_DPLL_TEMPERATURE,
>>>+					 DPLL_MODE_AUTOMATIC);
>>>+	if (IS_ERR(dpll->dpll_e))
>>>+		goto dpll_e_pd;
>>>+	if (dpll_device_register(dpll->dpll_e, DPLL_TYPE_EEC, &dds_ops,
>>>+				 (void *)dpll->dpll_e_pd))
>> 
>> Please avoid pointless casts. (void *) cast for arg of type (void *) are
>> especially pointless. Please make sure to fix this in the rest of the
>> code as well.
>> 
>
>Thanks, will do.
>
>>>+		goto e_reg;
>>>+
>>>+	/* Create PPS DPLL */
>>>+	dpll->dpll_p = dpll_device_get(DPLLS_CLOCK_ID + devid, PPS_DPLL_DEV,
>>>+				       THIS_MODULE);
>>>+	if (IS_ERR(dpll->dpll_p))
>>>+		goto dpll_p;
>>>+	dpll->dpll_p_pd = create_dpll_pd(PPS_DPLL_TEMPERATURE,
>>>+					 DPLL_MODE_MANUAL);
>>>+	if (IS_ERR(dpll->dpll_p_pd))
>>>+		goto dpll_p_pd;
>>>+	if (dpll_device_register(dpll->dpll_p, DPLL_TYPE_PPS, &dds_ops,
>> 
>> You always use "int err" to store the return value of function calls
>> returning 0/-EXXX like this one.
>> 
>
>Lesson learned, will fix.
>
>>>+				 (void *)dpll->dpll_p_pd))
>>>+		goto p_reg;
>>>+
>>>+	/* Create first pin (GNSS) */
>>>+	dpll->pp_gnss = create_pin_properties("GNSS", DPLL_PIN_TYPE_GNSS,
>>>+					      PIN_GNSS_CAPABILITIES,
>>>+					      1, DPLL_PIN_FREQUENCY_1_HZ,
>>>+					      DPLL_PIN_FREQUENCY_1_HZ);
>>>+	if (IS_ERR(dpll->pp_gnss))
>>>+		goto pp_gnss;
>>>+	dpll->p_gnss = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_GNSS,
>>>+				    THIS_MODULE,
>>>+				    dpll->pp_gnss);
>>>+	if (IS_ERR(dpll->p_gnss))
>>>+		goto p_gnss;
>>>+	dpll->p_gnss_pd = create_pin_pd(DPLL_PIN_FREQUENCY_1_HZ,
>>>+					PIN_GNSS_PRIORITY,
>>>+					DPLL_PIN_DIRECTION_INPUT);
>>>+	if (IS_ERR(dpll->p_gnss_pd))
>>>+		goto p_gnss_pd;
>>>+	if (dpll_pin_register(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>>>+			      (void *)dpll->p_gnss_pd))
>>>+		goto e_gnss_reg;
>>>+
>>>+	/* Create second pin (PPS) */
>>>+	dpll->pp_pps = create_pin_properties("PPS", DPLL_PIN_TYPE_EXT,
>>>+					     PIN_PPS_CAPABILITIES,
>>>+					     1, DPLL_PIN_FREQUENCY_1_HZ,
>>>+					     DPLL_PIN_FREQUENCY_1_HZ);
>>>+	if (IS_ERR(dpll->pp_pps))
>>>+		goto pp_pps;
>>>+	dpll->p_pps = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_PPS, THIS_MODULE,
>>>+				   dpll->pp_pps);
>>>+	if (IS_ERR(dpll->p_pps))
>>>+		goto p_pps;
>>>+	dpll->p_pps_pd = create_pin_pd(DPLL_PIN_FREQUENCY_1_HZ,
>>>+				       PIN_PPS_PRIORITY,
>>>+				       DPLL_PIN_DIRECTION_INPUT);
>>>+	if (IS_ERR(dpll->p_pps_pd))
>>>+		goto p_pps_pd;
>>>+	if (dpll_pin_register(dpll->dpll_e, dpll->p_pps, &pin_ops,
>>>+			      (void *)dpll->p_pps_pd))
>>>+		goto e_pps_reg;
>>>+	if (dpll_pin_register(dpll->dpll_p, dpll->p_pps, &pin_ops,
>>>+			      (void *)dpll->p_pps_pd))
>>>+		goto p_pps_reg;
>>>+
>>>+	return 0;
>>>+
>>>+p_pps_reg:
>>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &pin_ops,
>>>+			    (void *)dpll->p_pps_pd);
>>>+e_pps_reg:
>>>+	kfree(dpll->p_pps_pd);
>>>+p_pps_pd:
>>>+	dpll_pin_put(dpll->p_pps);
>>>+p_pps:
>>>+	free_pin_properties(dpll->pp_pps);
>>>+pp_pps:
>>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>>>+			    (void *)dpll->p_gnss_pd);
>>>+e_gnss_reg:
>>>+	kfree(dpll->p_gnss_pd);
>>>+p_gnss_pd:
>>>+	dpll_pin_put(dpll->p_gnss);
>>>+p_gnss:
>>>+	free_pin_properties(dpll->pp_gnss);
>>>+pp_gnss:
>>>+	dpll_device_unregister(dpll->dpll_p, &dds_ops, (void *)dpll->dpll_p_pd);
>>>+p_reg:
>>>+	kfree(dpll->dpll_p_pd);
>>>+dpll_p_pd:
>>>+	dpll_device_put(dpll->dpll_p);
>>>+dpll_p:
>>>+	dpll_device_unregister(dpll->dpll_e, &dds_ops, (void *)dpll->dpll_e_pd);
>>>+e_reg:
>>>+	kfree(dpll->dpll_e_pd);
>>>+dpll_e_pd:
>>>+	dpll_device_put(dpll->dpll_e);
>>>+dpll_e:
>>>+	return -1;
>>>+}
>>>+
>>>+void nsim_dpll_free_owner(struct nsim_dpll_info *dpll)
>>>+{
>>>+	/* Free GNSS pin */
>>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &pin_ops,
>>>+			    (void *)dpll->p_gnss_pd);
>>>+	dpll_pin_put(dpll->p_gnss);
>>>+	free_pin_properties(dpll->pp_gnss);
>>>+	kfree(dpll->p_gnss_pd);
>>>+
>>>+	/* Free PPS pin */
>>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &pin_ops,
>>>+			    (void *)dpll->p_pps_pd);
>>>+	dpll_pin_unregister(dpll->dpll_p, dpll->p_pps, &pin_ops,
>>>+			    (void *)dpll->p_pps_pd);
>>>+	dpll_pin_put(dpll->p_pps);
>>>+	free_pin_properties(dpll->pp_pps);
>>>+	kfree(dpll->p_pps_pd);
>>>+
>>>+	/* Free DPLL EEC */
>>>+	dpll_device_unregister(dpll->dpll_e, &dds_ops, (void *)dpll->dpll_e_pd);
>>>+	dpll_device_put(dpll->dpll_e);
>>>+	kfree(dpll->dpll_e_pd);
>>>+
>>>+	/* Free DPLL PPS */
>>>+	dpll_device_unregister(dpll->dpll_p, &dds_ops, (void *)dpll->dpll_p_pd);
>>>+	dpll_device_put(dpll->dpll_p);
>>>+	kfree(dpll->dpll_p_pd);
>>>+}
>>>+
>>>+int nsim_rclk_init(struct nsim_dpll_info *dpll, int devid, unsigned int index)
>>>+{
>>>+	char *name = kasprintf(GFP_KERNEL, "RCLK_%i", index);
>>>+
>>>+	/* Get EEC DPLL */
>>>+	dpll->dpll_e = dpll_device_get(DPLLS_CLOCK_ID + devid, EEC_DPLL_DEV,
>>>+				       THIS_MODULE);
>>>+	if (IS_ERR(dpll->dpll_e))
>>>+		goto dpll;
>>>+
>>>+	/* Get PPS DPLL */
>>>+	dpll->dpll_p = dpll_device_get(DPLLS_CLOCK_ID + devid, PPS_DPLL_DEV,
>>>+				       THIS_MODULE);
>>>+	if (IS_ERR(dpll->dpll_p))
>>>+		goto dpll;
>>>+
>>>+	/* Create Recovered clock pin (RCLK) */
>>>+	dpll->pp_rclk = create_pin_properties(name,
>>>+					      DPLL_PIN_TYPE_SYNCE_ETH_PORT,
>>>+					      PIN_RCLK_CAPABILITIES, 1, 1e6,
>>>+					      125e6);
>>>+	if (IS_ERR(dpll->pp_rclk))
>>>+		goto dpll;
>>>+	dpll->p_rclk = dpll_pin_get(DPLLS_CLOCK_ID + devid, PIN_RCLK + index,
>>>+				    THIS_MODULE, dpll->pp_rclk);
>>>+	if (IS_ERR(dpll->p_rclk))
>>>+		goto p_rclk;
>>>+	dpll->p_rclk_pd = create_pin_pd(DPLL_PIN_FREQUENCY_10_MHZ,
>>>+					PIN_RCLK_PRIORITY,
>>>+					DPLL_PIN_DIRECTION_INPUT);
>>>+	if (IS_ERR(dpll->p_rclk_pd))
>>>+		goto p_rclk_pd;
>>>+	if (dpll_pin_register(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>>>+			      (void *)dpll->p_rclk_pd))
>>>+		goto dpll_e_reg;
>>>+	if (dpll_pin_register(dpll->dpll_p, dpll->p_rclk, &pin_ops,
>>>+			      (void *)dpll->p_rclk_pd))
>>>+		goto dpll_p_reg;
>>>+
>>>+	return 0;
>>>+
>>>+dpll_p_reg:
>>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>>>+			    (void *)dpll->p_rclk_pd);
>>>+dpll_e_reg:
>>>+	kfree(dpll->p_rclk_pd);
>>>+p_rclk_pd:
>>>+	dpll_pin_put(dpll->p_rclk);
>>>+p_rclk:
>>>+	free_pin_properties(dpll->pp_rclk);
>>>+dpll:
>>>+	return -1;
>>>+}
>>>+
>>>+void nsim_rclk_free(struct nsim_dpll_info *dpll)
>>>+{
>>>+	/* Free RCLK pin */
>>>+	dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk, &pin_ops,
>>>+			    (void *)dpll->p_rclk_pd);
>>>+	dpll_pin_unregister(dpll->dpll_p, dpll->p_rclk, &pin_ops,
>>>+			    (void *)dpll->p_rclk_pd);
>>>+	dpll_pin_put(dpll->p_rclk);
>>>+	free_pin_properties(dpll->pp_rclk);
>>>+	kfree(dpll->p_rclk_pd);
>>>+	dpll_device_put(dpll->dpll_e);
>>>+	dpll_device_put(dpll->dpll_p);
>>>+}
>>>diff --git a/drivers/net/netdevsim/dpll.h b/drivers/net/netdevsim/dpll.h
>>>new file mode 100644
>>>index 0000000..17db7f7
>>>--- /dev/null
>>>+++ b/drivers/net/netdevsim/dpll.h
>>>@@ -0,0 +1,81 @@
>>>+/* SPDX-License-Identifier: GPL-2.0 */
>>>+/*
>>>+ * Copyright (c) 2023, Intel Corporation.
>>>+ * Author: Michal Michalik <michal.michalik@intel.com>
>>>+ */
>>>+
>>>+#ifndef NSIM_DPLL_H
>>>+#define NSIM_DPLL_H
>> 
>> Why you need a separate header for this? Just put necessary parts in
>> netdevsim.h and leave the rest in the .c file.
>> 
>
>Good idea, thanks.
>
>>>+
>>>+#include <linux/types.h>
>>>+#include <linux/netlink.h>
>>>+
>>>+#include <linux/dpll.h>
>>>+#include <uapi/linux/dpll.h>
>> 
>> Why exactly do you need to include uapi header directly?
>> 
>
>You are right - will refactor that.
>
>>>+
>>>+#define EEC_DPLL_DEV 0
>>>+#define EEC_DPLL_TEMPERATURE 20
>>>+#define PPS_DPLL_DEV 1
>>>+#define PPS_DPLL_TEMPERATURE 30
>>>+#define DPLLS_CLOCK_ID 234
>>>+
>>>+#define PIN_GNSS 0
>>>+#define PIN_GNSS_CAPABILITIES 2 /* DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE */
>>>+#define PIN_GNSS_PRIORITY 5
>>>+
>>>+#define PIN_PPS 1
>>>+#define PIN_PPS_CAPABILITIES 7 /* DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE
>>>+				* || DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE
>>>+				* || DPLL_PIN_CAPS_STATE_CAN_CHANGE
>> 
>> You are kidding, correct? :)
>> 
>
>Not really, it's written directly because the tests are able to read the value
>from here (they don't understand DPLL subsystem defines). Since we are changing
>most of the code I will try to make the tests access this data differently (e.g.
>via debugfs).

The test is reading define value from header file? Why on earth? :)



>
>>>+				*/
>>>+#define PIN_PPS_PRIORITY 6
>>>+
>>>+#define PIN_RCLK 2
>>>+#define PIN_RCLK_CAPABILITIES 6 /* DPLL_PIN_CAPS_PRIORITY_CAN_CHANGE
>>>+				 * || DPLL_PIN_CAPS_STATE_CAN_CHANGE
>>>+				 */
>>>+#define PIN_RCLK_PRIORITY 7
>>>+
>>>+#define EEC_PINS_NUMBER 3
>>>+#define PPS_PINS_NUMBER 2
>>>+
>>>+struct dpll_pd {
>> 
>> Have not clue what do you mean by "pd".
>> 
>
>I meant "private data", will change this to something more meaningful.

Cool. Don't forget the nsim prefix.


>
>>>+	enum dpll_mode mode;
>>>+	int temperature;
>>>+};
>>>+
>>>+struct pin_pd {
>>>+	u64 frequency;
>>>+	enum dpll_pin_direction direction;
>>>+	enum dpll_pin_state state_pin;
>>>+	enum dpll_pin_state state_dpll;
>>>+	u32 prio;
>>>+};
>>>+
>>>+struct nsim_dpll_info {
>> 
>> Drop "info".
>> 
>
>OK.
>
>>>+	bool owner;
>>>+
>>>+	struct dpll_device *dpll_e;
>>>+	struct dpll_pd *dpll_e_pd;
>>>+	struct dpll_device *dpll_p;
>>>+	struct dpll_pd *dpll_p_pd;
>>>+
>>>+	struct dpll_pin_properties *pp_gnss;
>> 
>> Why pointer? Just embed the properties struct, no?
>> 
>
>I can change both private data and properties to be embeded.

Great.


>
>>>+	struct dpll_pin *p_gnss;
>>>+	struct pin_pd *p_gnss_pd;
>>>+
>>>+	struct dpll_pin_properties *pp_pps;
>>>+	struct dpll_pin *p_pps;
>>>+	struct pin_pd *p_pps_pd;
>>>+
>>>+	struct dpll_pin_properties *pp_rclk;
>>>+	struct dpll_pin *p_rclk;
>>>+	struct pin_pd *p_rclk_pd;
>>>+};
>>>+
>>>+int nsim_dpll_init_owner(struct nsim_dpll_info *dpll, int devid);
>>>+void nsim_dpll_free_owner(struct nsim_dpll_info *dpll);
>>>+int nsim_rclk_init(struct nsim_dpll_info *dpll, int devid, unsigned int index);
>>>+void nsim_rclk_free(struct nsim_dpll_info *dpll);
>>>+
>>>+#endif /* NSIM_DPLL_H */
>>>diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>>>index aecaf5f..78a936f 100644
>>>--- a/drivers/net/netdevsim/netdev.c
>>>+++ b/drivers/net/netdevsim/netdev.c
>>>@@ -25,6 +25,7 @@
>>> #include <net/udp_tunnel.h>
>>> 
>>> #include "netdevsim.h"
>>>+#include "dpll.h"
>>> 
>>> static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>> {
>>>@@ -344,6 +345,20 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
>>> 	if (err)
>>> 		goto err_ipsec_teardown;
>>> 	rtnl_unlock();
>>>+
>>>+	if (ns->nsim_dev_port->port_index == 0) {
>> 
>> Does not make any sense to treat port 0 any different.
>> 
>> Please, move the init of dpll device to drivers/net/netdevsim/dev.c
>> probably somewhere in nsim_drv_probe().
>> 
>
>Great idea - I will do it.
>
>>>+		err = nsim_dpll_init_owner(&ns->dpll,
>>>+					   ns->nsim_dev->nsim_bus_dev->dev.id);
>>>+		if (err)
>>>+			goto err_ipsec_teardown;
>>>+	}
>>>+
>>>+	err = nsim_rclk_init(&ns->dpll, ns->nsim_dev->nsim_bus_dev->dev.id,
>>>+			     ns->nsim_dev_port->port_index);
>> 
>> This is not related to netdev directly. Please move the pin init into
>> drivers/net/netdevsim/dev.c, probably somewhere inside
>> __nsim_dev_port_add()
>> 
>> Also, you don't call netdev_dpll_pin_set() and netdev_dpll_pin_clear().
>> That is actually what you should call here in netdev initialization.
>> 
>
>Good catch - I will move to dev.c and use netdev_dpll_pin_set/clear()

Cool.


>
>>>+
>>>+	if (err)
>>>+		goto err_ipsec_teardown;
>>>+
>>> 	return 0;
>>> 
>>> err_ipsec_teardown:
>>>@@ -419,6 +434,11 @@ void nsim_destroy(struct netdevsim *ns)
>>> 	if (nsim_dev_port_is_pf(ns->nsim_dev_port))
>>> 		nsim_udp_tunnels_info_destroy(dev);
>>> 	mock_phc_destroy(ns->phc);
>>>+
>>>+	nsim_rclk_free(&ns->dpll);
>>>+	if (ns->nsim_dev_port->port_index == 0)
>>>+		nsim_dpll_free_owner(&ns->dpll);
>>>+
>>> 	free_netdev(dev);
>>> }
>>> 
>>>diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>>>index 028c825..3d0138a 100644
>>>--- a/drivers/net/netdevsim/netdevsim.h
>>>+++ b/drivers/net/netdevsim/netdevsim.h
>>>@@ -26,6 +26,8 @@
>>> #include <net/xdp.h>
>>> #include <net/macsec.h>
>>> 
>>>+#include "dpll.h"
>>>+
>>> #define DRV_NAME	"netdevsim"
>>> 
>>> #define NSIM_XDP_MAX_MTU	4000
>>>@@ -125,6 +127,8 @@ struct netdevsim {
>>> 	} udp_ports;
>>> 
>>> 	struct nsim_ethtool ethtool;
>>>+
>>>+	struct nsim_dpll_info dpll;
>>> };
>>> 
>>> struct netdevsim *
>>>-- 
>>>2.9.5
>>>
>

[PATCH RFC net-next v2 2/2] selftests/dpll: add DPLL system integration selftests

[Michal Michalik]

The tests are written in Python3 (3.7+) and pytest testing framework.
Framework is basing on the ynl library available in the kernel tree
at: tools/net/ynl

High level flow of DPLL subsystem integration selftests:
(after running run_dpll_tests.sh or 'make -C tools/testing/selftests')
1) check if Python in correct version is installed,
2) create temporary Python virtual environment,
3) install all the required libraries,
4) run the tests,
5) do cleanup.

The DPLL system integration tests are meant to be part of selftests, so
they can be build and run using command:
  make -C tools/testing/selftests
  make -C tools/testing/selftests run_tests

Alternatively, they can be run using single command [1]:
  make kselftest

If we want to run only DPLL tests, we should set the TARGETS variable:
  make -C tools/testing/selftests TARGETS=dpll
  make -C tools/testing/selftests TARGETS=dpll run_tests

They can also be run standalone using starter script:
  ./run_dpll_tests.sh

There is a possibliy to set optional PYTEST_PARAMS environment variable
to set the pytest options, like tests filtering ("-k <filter>") or
verbose output ("-v").

[1] https://www.kernel.org/doc/html/v5.0/dev-tools/kselftest.html

Signed-off-by: Michal Michalik <michal.michalik@intel.com>
---
 tools/testing/selftests/Makefile                 |   1 +
 tools/testing/selftests/dpll/Makefile            |   8 +
 tools/testing/selftests/dpll/__init__.py         |   0
 tools/testing/selftests/dpll/config              |   2 +
 tools/testing/selftests/dpll/consts.py           |  34 ++
 tools/testing/selftests/dpll/dpll_utils.py       | 109 ++++++
 tools/testing/selftests/dpll/requirements.txt    |   3 +
 tools/testing/selftests/dpll/run_dpll_tests.sh   |  75 ++++
 tools/testing/selftests/dpll/test_dpll.py        | 414 +++++++++++++++++++++++
 tools/testing/selftests/dpll/ynlfamilyhandler.py |  49 +++
 10 files changed, 695 insertions(+)
 create mode 100644 tools/testing/selftests/dpll/Makefile
 create mode 100644 tools/testing/selftests/dpll/__init__.py
 create mode 100644 tools/testing/selftests/dpll/config
 create mode 100644 tools/testing/selftests/dpll/consts.py
 create mode 100644 tools/testing/selftests/dpll/dpll_utils.py
 create mode 100644 tools/testing/selftests/dpll/requirements.txt
 create mode 100755 tools/testing/selftests/dpll/run_dpll_tests.sh
 create mode 100644 tools/testing/selftests/dpll/test_dpll.py
 create mode 100644 tools/testing/selftests/dpll/ynlfamilyhandler.py

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 1a21d6b..648a688 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -14,6 +14,7 @@ TARGETS += cpufreq
 TARGETS += cpu-hotplug
 TARGETS += damon
 TARGETS += dmabuf-heaps
+TARGETS += dpll
 TARGETS += drivers/dma-buf
 TARGETS += drivers/s390x/uvdevice
 TARGETS += drivers/net/bonding
diff --git a/tools/testing/selftests/dpll/Makefile b/tools/testing/selftests/dpll/Makefile
new file mode 100644
index 0000000..65de011
--- /dev/null
+++ b/tools/testing/selftests/dpll/Makefile
@@ -0,0 +1,8 @@
+ifndef KSRC
+	KSRC:=${shell git rev-parse --show-toplevel}
+endif
+
+run_tests:
+	./run_dpll_tests.sh
+
+.PHONY: run_tests
\ No newline at end of file
diff --git a/tools/testing/selftests/dpll/__init__.py b/tools/testing/selftests/dpll/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/tools/testing/selftests/dpll/config b/tools/testing/selftests/dpll/config
new file mode 100644
index 0000000..e38b164
--- /dev/null
+++ b/tools/testing/selftests/dpll/config
@@ -0,0 +1,2 @@
+CONFIG_DPLL=y
+CONFIG_NETDEVSIM=m
\ No newline at end of file
diff --git a/tools/testing/selftests/dpll/consts.py b/tools/testing/selftests/dpll/consts.py
new file mode 100644
index 0000000..838ce58
--- /dev/null
+++ b/tools/testing/selftests/dpll/consts.py
@@ -0,0 +1,34 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Constants useful in DPLL system integration testing.
+#
+# Copyright (c) 2023, Intel Corporation.
+# Author: Michal Michalik <michal.michalik@intel.com>
+
+import os
+from enum import Enum
+
+
+KSRC = os.environ.get('KSRC', '')
+YNLPATH = 'tools/net/ynl/'
+YNLSPEC = 'Documentation/netlink/specs/dpll.yaml'
+
+
+class DPLL_TYPE(Enum):
+    PPS = 1
+    EEC = 2
+
+
+class DPLL_LOCK_STATUS(Enum):
+    UNLOCKED = 1
+    LOCKED = 2
+    LOCKED_HO_ACK = 3
+    HOLDOVER = 4
+
+
+class DPLL_PIN_TYPE(Enum):
+    MUX = 1,
+    EXT = 2
+    SYNCE_ETH_PORT = 3
+    INT_OSCILLATOR = 4
+    GNSS = 5
diff --git a/tools/testing/selftests/dpll/dpll_utils.py b/tools/testing/selftests/dpll/dpll_utils.py
new file mode 100644
index 0000000..f6e827a
--- /dev/null
+++ b/tools/testing/selftests/dpll/dpll_utils.py
@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Utilities useful in DPLL system integration testing.
+#
+# Copyright (c) 2023, Intel Corporation.
+# Author: Michal Michalik <michal.michalik@intel.com>
+
+import re
+
+from .ynlfamilyhandler import YnlFamilyHandler
+
+
+def parse_header(filepath):
+    '''
+    Simple parser for C headers to dynamically read the defines/consts
+    '''
+    FILTER = ('(u32)', '(u64)', '(', ')')
+
+    with open(filepath) as f:
+        header = f.read()
+
+    items = {}
+    matches = re.findall(r'''\#define
+                             \s+?
+                             ([\w\d]*)  # key
+                             \s+?
+                             ([\w\d)()"]+)  # value
+                          ''', header, re.MULTILINE | re.VERBOSE)
+    for key, value in matches:
+        for f in FILTER:
+            value = value.replace(f, '')
+        if value.isnumeric():
+            items[key] = int(value)
+        elif not value:
+            items[key] = True  # let's just treat it as a flag
+        else:
+            items[key] = value
+
+    return items
+
+
+def get_dpll_id(clock_id, test_module, type):
+    '''
+    YNL helper for getting the DPLL clock ID
+    '''
+    yfh = YnlFamilyHandler()
+    yfh.do = 'device-id-get'
+    yfh.attrs = {
+        'module-name': test_module,
+        'clock-id': clock_id,
+        'type': type
+        }
+    return yfh.execute()['id']
+
+
+def get_dpll(clock_id, test_module, type):
+    '''
+    YNL helper for getting the DPLL clock object
+    '''
+    _id = get_dpll_id(clock_id, test_module, type)
+    yfh = YnlFamilyHandler()
+    yfh.do = 'device-get'
+    yfh.attrs = {'id': _id}
+    return yfh.execute()
+
+
+def get_all_pins():
+    '''
+    YNL helper for getting the all DPLL pins
+    '''
+    yfh = YnlFamilyHandler()
+    yfh.dump = 'pin-get'
+    return yfh.execute()
+
+
+def get_pin_id(test_module, clock_id, board_l, panel_l, package_l, type):
+    '''
+    YNL helper for getting DPLL pin ID
+    '''
+    yfh = YnlFamilyHandler()
+    yfh.do = 'pin-id-get'
+    yfh.attrs = {'module-name': test_module,
+                 'clock-id': clock_id,
+                 'board-label': board_l,
+                 'panel-label': panel_l,
+                 'package-label': package_l,
+                 'type': type}
+    return yfh.execute()['id']
+
+
+def get_pin(_id):
+    '''
+    YNL helper for getting the DPLL pin object
+    '''
+    yfh = YnlFamilyHandler()
+    yfh.do = 'pin-get'
+    yfh.attrs = {'id': _id}
+    return yfh.execute()
+
+
+def set_pin(_id, params):
+    '''
+    YNL helper for setting the DPLL pin parameters
+    '''
+    yfh = YnlFamilyHandler()
+    yfh.do = 'pin-set'
+    yfh.attrs = params
+    yfh.attrs['id'] = _id
+    return yfh.execute()
diff --git a/tools/testing/selftests/dpll/requirements.txt b/tools/testing/selftests/dpll/requirements.txt
new file mode 100644
index 0000000..73180b8
--- /dev/null
+++ b/tools/testing/selftests/dpll/requirements.txt
@@ -0,0 +1,3 @@
+jsonschema==4.*
+PyYAML==6.*
+pytest==7.*
diff --git a/tools/testing/selftests/dpll/run_dpll_tests.sh b/tools/testing/selftests/dpll/run_dpll_tests.sh
new file mode 100755
index 0000000..3bed221
--- /dev/null
+++ b/tools/testing/selftests/dpll/run_dpll_tests.sh
@@ -0,0 +1,75 @@
+#!/usr/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Wraper script for running the DPLL system integration tests.
+#
+# The script check if all the requirements are fulfilled before running pytest.
+#
+# Copyright (c) 2023, Intel Corporation.
+# Author: Michal Michalik <michal.michalik@intel.com>
+
+ENOPKG=65  # Package not installed
+TEMP_VENV=$(mktemp -u)
+KSRC=${KSRC:-$(git rev-parse --show-toplevel)}
+PYTHON=${PYTHON:-python3}
+
+cleanup() {
+    [ -n "$VIRTUAL_ENV" ] && deactivate
+
+    if [[ -d "$TEMP_VENV" ]]; then
+        echo "Removing temporary virtual environment ($TEMP_VENV)"
+        rm -r "$TEMP_VENV"
+    else
+        echo "Temporary virtual environment does not exist"
+    fi
+}
+
+skip () {
+    cleanup
+    echo "SKIP: $1"
+    exit $2
+}
+
+# 1) To run tests, we need Python 3 installed
+which $PYTHON 2>&1 1> /dev/null
+if [[ $? -ne 0 ]]; then
+    skip "Python 3 is not installed" $ENOPKG
+fi
+
+# 2) ...at least Python 3.7 (2018)
+$PYTHON -c "import sys;vi=sys.version_info;
+sys.exit(0) if vi[0] == 3 and vi[1] >= 7 else sys.exit(1)"
+if [[ $? -ne 0 ]]; then
+    skip "At least Python 3.7 is required (set PYTHON for custom path)" $ENOPKG
+fi
+
+# 3) Let's make sure we have predictable environment (virtual environment)
+#   a) Create venv
+$PYTHON -m venv $TEMP_VENV
+if [[ $? -ne 0 ]]; then
+    skip "Could not create virtual environment" $ENOPKG
+fi
+
+#   b) Activate venv
+source $TEMP_VENV/bin/activate
+if [[ $? -ne 0 ]]; then
+    skip "Could not activate the virtual environment" $ENOPKG
+fi
+
+#   c) Install the exact packages versions we need
+pip install -r requirements.txt
+if [[ $? -ne 0 ]]; then
+    skip "Could not install the required packages" $ENOPKG
+fi
+
+# 4) Finally, run the tests!
+KSRC=$KSRC pytest $PYTEST_PARAMS
+result=$?
+if [[ $result -ne 0 ]]; then
+    echo "ERROR: Some of the DPLL tests failed"
+fi
+
+# 5) Clean up after execution
+cleanup
+
+exit $result
diff --git a/tools/testing/selftests/dpll/test_dpll.py b/tools/testing/selftests/dpll/test_dpll.py
new file mode 100644
index 0000000..dba0f27
--- /dev/null
+++ b/tools/testing/selftests/dpll/test_dpll.py
@@ -0,0 +1,414 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# System integration tests for DPLL interface.
+#
+# Can be used directly, but strongly suggest using wrapper: run_dpll_tests.sh
+# The wrapper takes care about fulfilling all the requirements needed to
+# run all the tests.
+#
+# Copyright (c) 2023, Intel Corporation.
+# Author: Michal Michalik <michal.michalik@intel.com>
+
+import pytest
+import subprocess
+from pathlib import Path
+
+from .consts import KSRC, DPLL_TYPE, DPLL_LOCK_STATUS, DPLL_PIN_TYPE
+from .dpll_utils import parse_header, get_dpll, get_dpll_id, get_pin_id, \
+    get_all_pins, get_pin, set_pin
+from .ynlfamilyhandler import YnlFamilyHandler
+from lib.ynl import NlError
+
+
+DPLL_CONSTS = 'drivers/net/netdevsim/dpll.h'
+TEST_MODULE = 'netdevsim'
+NETDEVSIM_PATH = '/sys/bus/netdevsim/'
+NETDEVSIM_NEW_DEVICE = Path(NETDEVSIM_PATH) / 'new_device'
+NETDEVSIM_DEL_DEVICE = Path(NETDEVSIM_PATH) / 'del_device'
+NETDEVSIM_DEVICES = Path(NETDEVSIM_PATH) / 'devices'
+
+
+class TestDPLL:
+    DEV_ID = 0
+
+    @classmethod
+    def setup_class(cls):
+        cls.module_consts = parse_header(Path(KSRC) / DPLL_CONSTS)
+        with open(NETDEVSIM_NEW_DEVICE, 'w') as f:
+            f.write(f'{cls.DEV_ID} 1 4')
+
+    @classmethod
+    def teardown_class(cls):
+        with open(NETDEVSIM_DEL_DEVICE, 'w') as f:
+            f.write(f'{cls.DEV_ID}')
+
+    def test_if_module_is_loaded(self):
+        '''
+        Checks if the module is successfully loaded at all. It should be already
+        covered in the class setup (raise exception) - but just to make sure.
+        '''
+        s = subprocess.run(['lsmod'], check=True, capture_output=True)
+        assert TEST_MODULE in str(s.stdout)
+
+    def test_get_two_dplls(self):
+        '''
+        Checks if the netlink is returning the expected DPLLs. Need to make sure
+        that even if "other" DPLLs exist in the system we check only ours.
+        '''
+        yfh = YnlFamilyHandler()
+        yfh.dump = 'device-get'
+        reply = yfh.execute()
+
+        dplls = filter(lambda i: TEST_MODULE == i['module-name']
+                       and i['clock-id'] == self.module_consts['DPLLS_CLOCK_ID'],
+                       reply)
+        assert len(list(dplls)) == 2
+
+    def test_get_two_distinct_dplls(self):
+        '''
+        Checks if the netlink is returning the expected, distinct DPLLs created
+        by the tested module. Expect EEC and PPS.
+        '''
+        yfh = YnlFamilyHandler()
+        yfh.dump = 'device-get'
+        reply = yfh.execute()
+
+        dplls = filter(lambda i: TEST_MODULE in i['module-name'], reply)
+        types = set(i['type'] for i in dplls)
+
+        assert types == {'eec', 'pps'}
+
+    @pytest.mark.parametrize("dtype", [DPLL_TYPE.EEC, DPLL_TYPE.PPS])
+    def test_finding_dpll_id(self, dtype):
+        '''
+        Checks if it is possible to find the DPLL id using 'device-id-get' do cmd.
+        '''
+        _id = get_dpll_id(self.module_consts['DPLLS_CLOCK_ID'], TEST_MODULE,
+                          dtype.value)
+        assert isinstance(_id, int)
+
+    @pytest.mark.parametrize("clk,dtype,exc", [(123, DPLL_TYPE.EEC.value, KeyError),
+                                               (234, 4, NlError),
+                                               (123, 4, NlError)])
+    def test_finding_fails_correctly(self, clk, dtype, exc):
+        '''
+        Make sure the DPLL interface does not return any garbage on incorrect
+        input like wrong DPLL type or clock id.
+        '''
+        with pytest.raises(exc):
+            get_dpll_id(clk, TEST_MODULE, dtype)
+
+    @pytest.mark.parametrize("dtype", [DPLL_TYPE.EEC, DPLL_TYPE.PPS])
+    def test_get_only_one_dpll(self, dtype):
+        '''
+        Checks if the netlink is returning the expected DPLLs created
+        by the tested module, filtered on input. Expect EEC and PPS here.
+        '''
+        _id = get_dpll_id(self.module_consts['DPLLS_CLOCK_ID'], TEST_MODULE,
+                          dtype.value)
+
+        yfh = YnlFamilyHandler()
+        yfh.do = 'device-get'
+        yfh.attrs = {'id': _id}
+        reply = yfh.execute()
+
+        assert reply['type'] == dtype.name.lower()
+
+    @pytest.mark.parametrize("dtype", [DPLL_TYPE.EEC, DPLL_TYPE.PPS])
+    def test_get_temperature(self, dtype):
+        '''
+        Checks if it is possible to get correct DPLL temperature for both DPLLs.
+        '''
+        if dtype == DPLL_TYPE.EEC:
+            desired_temp = self.module_consts['EEC_DPLL_TEMPERATURE']
+        else:
+            desired_temp = self.module_consts['PPS_DPLL_TEMPERATURE']
+
+        dpll = get_dpll(self.module_consts['DPLLS_CLOCK_ID'], TEST_MODULE,
+                        dtype.value)
+
+        assert dpll['temp'] == desired_temp
+
+    @pytest.mark.parametrize("dtype, lock", [(DPLL_TYPE.EEC, DPLL_LOCK_STATUS.UNLOCKED),
+                                             (DPLL_TYPE.PPS, DPLL_LOCK_STATUS.LOCKED)])
+    def test_get_lock(self, dtype, lock):
+        '''
+        Checks if it is possible to get correct DPLL lock status for both DPLLs.
+        '''
+        dpll = get_dpll(self.module_consts['DPLLS_CLOCK_ID'], TEST_MODULE,
+                        dtype.value)
+        assert dpll['lock-status'] == lock.name.lower()
+
+    @pytest.mark.parametrize("dtype", [DPLL_TYPE.EEC, DPLL_TYPE.PPS])
+    def test_dump_pins_in_each_dpll(self, dtype):
+        '''
+        Checks if it is possible to dump all pins for each DPLL separetely,
+        filtered on output.
+        '''
+        if dtype == DPLL_TYPE.EEC:
+            desired_pins = self.module_consts['EEC_PINS_NUMBER']
+        else:
+            desired_pins = self.module_consts['PPS_PINS_NUMBER']
+
+        dpll = get_dpll(self.module_consts['DPLLS_CLOCK_ID'], TEST_MODULE,
+                        dtype.value)
+
+        yfh = YnlFamilyHandler()
+        yfh.dump = 'pin-get'
+        reply = yfh.execute()
+
+        pins = filter(lambda p: any(i['parent-id'] == dpll['id']
+                      for i in p.get('parent-device', [])), reply)
+
+        assert len(list(pins)) == desired_pins
+
+    def test_dump_all_pins_in_both_dplls(self):
+        '''
+        Checks if it is possible to dump all pins for both DPLLs, filtered by
+        clock id on output.
+        '''
+        desired_pins = self.module_consts['EEC_PINS_NUMBER']  # all are in EEC
+        clock_id = self.module_consts['DPLLS_CLOCK_ID']
+
+        reply = get_all_pins()
+
+        pins = filter(lambda p: p['clock-id'] == clock_id, reply)
+
+        assert len(list(pins)) == desired_pins
+
+    @pytest.mark.parametrize("pin, pin_name", [(DPLL_PIN_TYPE.EXT, 'PPS'),
+                                               (DPLL_PIN_TYPE.GNSS, 'GNSS'),
+                                               (DPLL_PIN_TYPE.SYNCE_ETH_PORT, 'RCLK')])
+    def test_get_a_single_pin_from_dump(self, pin, pin_name):
+        '''
+        Checks if it is possible to get all distinct pins for both DPLLs, filtered
+        by clock id and type on output. Additionally, verify if the priority is
+        assigned correctly and not mixed up.
+        '''
+        clock_id = self.module_consts['DPLLS_CLOCK_ID']
+        priority = self.module_consts[f'PIN_{pin_name}_PRIORITY']
+        caps = self.module_consts[f'PIN_{pin_name}_CAPABILITIES']
+
+        reply = get_all_pins()
+
+        pin_name = pin.name.lower().replace('_', '-')
+        pins = filter(lambda p:
+                      p['clock-id'] == clock_id and p['type'] == pin_name, reply)
+        pins = list(pins)
+
+        assert len(pins) == 1
+        assert pins[0]['capabilities'] == caps
+        for p in pins[0]['parent-device']:
+            assert p['prio'] == priority
+
+    @pytest.mark.parametrize("pin, pin_name",
+                             [(DPLL_PIN_TYPE.EXT, 'PPS'),
+                              (DPLL_PIN_TYPE.GNSS, 'GNSS'),
+                              (DPLL_PIN_TYPE.SYNCE_ETH_PORT, 'RCLK_0')])
+    def test_get_a_single_pin_id(self, pin, pin_name):
+        '''
+        Checks if it is possible to get single pins using 'get-pin-id' do
+        command.
+        '''
+        clock_id = self.module_consts['DPLLS_CLOCK_ID']
+        board_l = f'{pin_name}_brd'
+        panel_l = f'{pin_name}_pnl'
+        package = f'{pin_name}_pcg'
+
+        _id = get_pin_id(TEST_MODULE, clock_id, board_l, panel_l, package, pin.value)
+        assert isinstance(_id, int)
+
+    @pytest.mark.parametrize("pin, pin_name, param, value",
+                             [(DPLL_PIN_TYPE.EXT, 'PPS', 'prio', 1),
+                              (DPLL_PIN_TYPE.GNSS, 'GNSS', 'prio', 2),
+                              (DPLL_PIN_TYPE.SYNCE_ETH_PORT, 'RCLK_0', 'prio', 3)])
+    def test_set_a_single_pin_prio(self, pin, pin_name, param, value):
+        '''
+        Checks if it is possible to set pins priority using 'set-pin' do
+        command.
+        '''
+        clock_id = self.module_consts['DPLLS_CLOCK_ID']
+        board_l = f'{pin_name}_brd'
+        panel_l = f'{pin_name}_pnl'
+        package = f'{pin_name}_pcg'
+
+        _id = get_pin_id(TEST_MODULE, clock_id, board_l, panel_l, package, pin.value)
+
+        pins_before = get_all_pins()
+        pin_before = get_pin(_id)
+
+        # both DPLL's are handled the same in the test module
+        first_dpll_id = pin_before['parent-device'][0]['parent-id']
+        set_pin(_id, {"parent-device": {"parent-id": first_dpll_id, param: value}})
+
+        pins_after = get_all_pins()
+
+        # assume same order, if that might change - test need to be updated
+        for i in range(len(pins_before)):
+            if pins_after[i]['id'] != _id:
+                assert pins_after[i] == pins_before[i]
+            else:
+                assert pins_after[i]["parent-device"][0][param] == value
+
+        # set the original value back to leave the same state after test
+        original_value = pin_before["parent-device"][0][param]
+        set_pin(_id, {"parent-device": {"parent-id": first_dpll_id, param: original_value}})
+
+    @pytest.mark.parametrize("pin, pin_name, param, value",
+                             [(DPLL_PIN_TYPE.SYNCE_ETH_PORT, 'RCLK_0', 'frequency', int(1e6)),
+                              (DPLL_PIN_TYPE.SYNCE_ETH_PORT, 'RCLK_0', 'frequency', int(12e6))])
+    def test_set_a_single_pin_freq(self, pin, pin_name, param, value):
+        '''
+        Checks if it is possible to set pins frequency using 'set-pin' do
+        command.
+        '''
+        clock_id = self.module_consts['DPLLS_CLOCK_ID']
+        board_l = f'{pin_name}_brd'
+        panel_l = f'{pin_name}_pnl'
+        package = f'{pin_name}_pcg'
+
+        _id = get_pin_id(TEST_MODULE, clock_id, board_l, panel_l, package, pin.value)
+
+        pins_before = get_all_pins()
+        pin_before = get_pin(_id)
+
+        set_pin(_id, {param: value})
+
+        pins_after = get_all_pins()
+
+        # assume same order, if that might change - test need to be updated
+        for i in range(len(pins_before)):
+            if pins_after[i]['id'] != _id:
+                assert pins_after[i] == pins_before[i]
+            else:
+                assert pins_after[i][param] == value
+
+        # set the original value back to leave the same state after test
+        set_pin(_id, {param: pin_before[param]})
+
+    @pytest.mark.parametrize("pin, pin_name, param, value",
+                             [(DPLL_PIN_TYPE.SYNCE_ETH_PORT, 'RCLK_0', 'frequency', int(1e5)),
+                              (DPLL_PIN_TYPE.SYNCE_ETH_PORT, 'RCLK_0', 'frequency', int(130e6))])
+    def test_set_a_single_pin_fail(self, pin, pin_name, param, value):
+        '''
+        Checks if we fail correctly trying to set incorrect pin frequency.
+        '''
+        clock_id = self.module_consts['DPLLS_CLOCK_ID']
+        board_l = f'{pin_name}_brd'
+        panel_l = f'{pin_name}_pnl'
+        package = f'{pin_name}_pcg'
+
+        _id = get_pin_id(TEST_MODULE, clock_id, board_l, panel_l, package, pin.value)
+
+        with pytest.raises(NlError):
+            set_pin(_id, {param: value})
+
+
+class TestTwoDPLLsOtherFirst(TestDPLL):
+    '''
+    Add a second module to the environment, which registers other DPLLs to make
+    sure the references are not mixed together. Other driver first.
+    '''
+    DEV_ID = 0
+    OTHER_DEV_ID = 2
+    OTHER_DEV_PORTS = 2
+
+    @classmethod
+    def setup_class(cls):
+        cls.module_consts = parse_header(Path(KSRC) / DPLL_CONSTS)
+        with open(NETDEVSIM_NEW_DEVICE, 'w') as f:
+            f.write(f'{cls.OTHER_DEV_ID} {cls.OTHER_DEV_PORTS} 4')
+        with open(NETDEVSIM_NEW_DEVICE, 'w') as f:
+            f.write(f'{cls.DEV_ID} 1 4')
+
+    @classmethod
+    def teardown_class(cls):
+        with open(NETDEVSIM_DEL_DEVICE, 'w') as f:
+            f.write(f'{cls.OTHER_DEV_ID}')
+        with open(NETDEVSIM_DEL_DEVICE, 'w') as f:
+            f.write(f'{cls.DEV_ID}')
+
+
+class TestTwoDPLLsTestedFirst(TestDPLL):
+    '''
+    Add a second module to the environment, which registers other DPLLs to make
+    sure the references are not mixed together. Tested driver first.
+    '''
+    DEV_ID = 0
+    OTHER_DEV_ID = 2
+    OTHER_DEV_PORTS = 2
+
+    @classmethod
+    def setup_class(cls):
+        cls.module_consts = parse_header(Path(KSRC) / DPLL_CONSTS)
+        with open(NETDEVSIM_NEW_DEVICE, 'w') as f:
+            f.write(f'{cls.DEV_ID} 1 4')
+        with open(NETDEVSIM_NEW_DEVICE, 'w') as f:
+            f.write(f'{cls.OTHER_DEV_ID} {cls.OTHER_DEV_PORTS} 4')
+
+    @classmethod
+    def teardown_class(cls):
+        with open(NETDEVSIM_DEL_DEVICE, 'w') as f:
+            f.write(f'{cls.DEV_ID}')
+        with open(NETDEVSIM_DEL_DEVICE, 'w') as f:
+            f.write(f'{cls.OTHER_DEV_ID}')
+
+
+class TestDPLLsNTF:
+    MULTICAST_GROUP = 'monitor'
+    DEV_ID = 0
+
+    @classmethod
+    def setup_class(cls):
+        '''
+        This test suite prepares the environment by arming the event tracking,
+        loading the driver, changing pin, unloading the driver and gathering
+        logs for further processing.
+        '''
+        cls.module_consts = parse_header(Path(KSRC) / DPLL_CONSTS)
+
+        yfh = YnlFamilyHandler(ntf=cls.MULTICAST_GROUP)
+
+        with open(NETDEVSIM_NEW_DEVICE, 'w') as f:
+            f.write(f'{cls.DEV_ID} 1 4')
+
+        pin = DPLL_PIN_TYPE.GNSS
+        clock_id = cls.module_consts['DPLLS_CLOCK_ID']
+        board_l = f'{pin.name}_brd'
+        panel_l = f'{pin.name}_pnl'
+        package = f'{pin.name}_pcg'
+
+        _id = get_pin_id(TEST_MODULE, clock_id, board_l, panel_l, package, pin.value)
+
+        g_pin = get_pin(_id)
+
+        # both DPLL's are handled the same in the test module
+        first_dpll_id = g_pin['parent-device'][0]['parent-id']
+        set_pin(_id, {"parent-device": {"parent-id": first_dpll_id, 'prio': 2}})
+
+        with open(NETDEVSIM_DEL_DEVICE, 'w') as f:
+            f.write(f'{cls.DEV_ID}')
+
+        yfh.ynl.check_ntf()
+        cls.events = yfh.ynl.async_msg_queue
+
+    @classmethod
+    def teardown_class(cls):
+        # Cleanup, in case something in the setup_class failed
+        ls_devices = subprocess.run(['ls', NETDEVSIM_DEVICES], capture_output=True)
+        if f'netdevsim{cls.DEV_ID}' in str(ls_devices.stdout):
+            with open(NETDEVSIM_DEL_DEVICE, 'w') as f:
+                f.write(f'{cls.DEV_ID}')
+
+    @pytest.mark.parametrize(('event', 'count'), [('device-create-ntf', 2),
+                                                  ('device-delete-ntf', 2),
+                                                  ('pin-change-ntf', 1),
+                                                  ('pin-create-ntf', 5),
+                                                  ('pin-delete-ntf', 5)])
+    def test_number_of_events(self, event, count):
+        '''
+        Checks if we are getting exact number of events that we expect to be
+        gathered while monitoring the DPLL subsystem.
+        '''
+        f_events = filter(lambda i: i['name'] == event, self.events)
+        assert len(list(f_events)) == count
diff --git a/tools/testing/selftests/dpll/ynlfamilyhandler.py b/tools/testing/selftests/dpll/ynlfamilyhandler.py
new file mode 100644
index 0000000..2aac23f
--- /dev/null
+++ b/tools/testing/selftests/dpll/ynlfamilyhandler.py
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Wraper for the YNL library used to interact with the netlink interface.
+#
+# Copyright (c) 2023, Intel Corporation.
+# Author: Michal Michalik <michal.michalik@intel.com>
+
+import sys
+from pathlib import Path
+from dataclasses import dataclass
+
+from .consts import KSRC, YNLSPEC, YNLPATH
+
+
+try:
+    ynl_full_path = Path(KSRC) / YNLPATH
+    sys.path.append(ynl_full_path.as_posix())
+    from lib import YnlFamily
+except ModuleNotFoundError:
+    print("Failed importing `ynl` library from kernel sources, please set KSRC")
+    sys.exit(1)
+
+
+@dataclass
+class YnlFamilyHandler:
+    spec: str = Path(KSRC) / YNLSPEC
+    schema: str = ''
+    dump: str = ''
+    ntf: str = ''
+    do: str = ''
+    attrs = {}
+
+    def __post_init__(self):
+        self.ynl = YnlFamily(self.spec, self.schema)
+
+        if self.ntf:
+            self.ynl.ntf_subscribe(self.ntf)
+
+    def execute(self):
+        if self.do and self.dump:
+            raise ValueError('Both "do" or "dump" set simultaneously - clear either of them')
+        elif self.do:
+            reply = self.ynl.do(self.do, self.attrs, [])
+        elif self.dump:
+            reply = self.ynl.dump(self.dump, self.attrs)
+        else:
+            raise ValueError('Wrong command - Set either "do" or "dump" before executing')
+
+        return reply
-- 
2.9.5

Re: [PATCH RFC net-next v2 0/2] selftests/dpll: DPLL subsystem integration tests

[Jiri Pirko]

Mon, Oct 30, 2023 at 05:53:24PM CET, michal.michalik@intel.com wrote:
>The recently merged common DPLL interface discussed on a newsletter[1]

"newsletter"? Sounds a bit odd to me :)


>is introducing new, complex subsystem which requires proper integration
>testing - this patch adds core for such framework, as well as the

"Patchset" perhaps? Also, what do you mean by "core"? The sentence
sounds a bit weird to me.


>initial test cases. Framework does not require neither any special
>hardware nor any special system architecture.
>
>To properly test the DPLL subsystem this patch adds fake DPLL devices and it's

For patch desctiption, please stay within 72cols.
Also, "it's" is most probably wrong in this sentence.


>pins implementation to netdevsim. Creating netdevsim devices and adding ports
>to it register new DPLL devices and pins. First port of each netdevsim device

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This sentence does not make
sense to me. Pehaps rephrase a bit?


>acts as a entitiy which registers two DPLL devices: EEC and PPS DPLLs. First

typo: "entitiy"

>port also register the common pins: PPS and GNSS. Additionally each port
>register also RCLK (recovered clock) pin for itself. That allow us to check
>mutliple scenarios which might be problematic in real implementations (like
>different ordering etc.)
>
>Patch adds few helper scripts, which are:
>1) tools/testing/selftests/dpll/run_dpll_tests.sh

Please make this part of
tools/testing/selftests/drivers/net/netdevsim/
No special threat of dpll needed.


>    Script is checking for all dependencies, creates temporary
>    environment, installs required libraries and run all tests - can be
>    used standalone
>2) tools/testing/selftests/dpll/ynlfamilyhandler.py˙
>    Library for easier ynl use in the pytest framework - can be used
>    standalone
>
>[1] https://lore.kernel.org/netdev/169494842736.21621.10730860855645661664.git-patchwork-notify@kernel.org/
>
>Changelog:
>v1 -> v2:
>- moved from separate module to implementation in netdevsim
>
>Michal Michalik (2):
>  netdevsim: implement DPLL for subsystem selftests
>  selftests/dpll: add DPLL system integration selftests
>
> drivers/net/Kconfig                              |   1 +
> drivers/net/netdevsim/Makefile                   |   2 +-
> drivers/net/netdevsim/dpll.c                     | 438 +++++++++++++++++++++++
> drivers/net/netdevsim/dpll.h                     |  81 +++++
> drivers/net/netdevsim/netdev.c                   |  20 ++
> drivers/net/netdevsim/netdevsim.h                |   4 +
> tools/testing/selftests/Makefile                 |   1 +
> tools/testing/selftests/dpll/Makefile            |   8 +
> tools/testing/selftests/dpll/__init__.py         |   0
> tools/testing/selftests/dpll/config              |   2 +
> tools/testing/selftests/dpll/consts.py           |  34 ++
> tools/testing/selftests/dpll/dpll_utils.py       | 109 ++++++
> tools/testing/selftests/dpll/requirements.txt    |   3 +
> tools/testing/selftests/dpll/run_dpll_tests.sh   |  75 ++++
> tools/testing/selftests/dpll/test_dpll.py        | 414 +++++++++++++++++++++
> tools/testing/selftests/dpll/ynlfamilyhandler.py |  49 +++
> 16 files changed, 1240 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/netdevsim/dpll.c
> create mode 100644 drivers/net/netdevsim/dpll.h
> create mode 100644 tools/testing/selftests/dpll/Makefile
> create mode 100644 tools/testing/selftests/dpll/__init__.py
> create mode 100644 tools/testing/selftests/dpll/config
> create mode 100644 tools/testing/selftests/dpll/consts.py
> create mode 100644 tools/testing/selftests/dpll/dpll_utils.py
> create mode 100644 tools/testing/selftests/dpll/requirements.txt
> create mode 100755 tools/testing/selftests/dpll/run_dpll_tests.sh
> create mode 100644 tools/testing/selftests/dpll/test_dpll.py
> create mode 100644 tools/testing/selftests/dpll/ynlfamilyhandler.py
>
>-- 
>2.9.5
>
>base-commit: 55c900477f5b3897d9038446f72a281cae0efd86

RE: [PATCH RFC net-next v2 0/2] selftests/dpll: DPLL subsystem integration tests

[Michalik, Michal]

On 31 October 2023 10:47 AM CET, Jiri Pirko wrote:

Jiri, much thanks for taking time and doing great review - I have learned
a lot, especially in patch 1/2 having the netdevsim DPLL implementation.

I will do my best to post the RFC v3 early next week.

> 
> Mon, Oct 30, 2023 at 05:53:24PM CET, michal.michalik@intel.com wrote:
>>The recently merged common DPLL interface discussed on a newsletter[1]
> 
> "newsletter"? Sounds a bit odd to me :)
> 

Yeah, you are right - will fix.

>>is introducing new, complex subsystem which requires proper integration
>>testing - this patch adds core for such framework, as well as the
> 
> "Patchset" perhaps? Also, what do you mean by "core"? The sentence
> sounds a bit weird to me.
> 

Will work to improve this.

>>initial test cases. Framework does not require neither any special
>>hardware nor any special system architecture.
>>
>>To properly test the DPLL subsystem this patch adds fake DPLL devices and it's
> 
> For patch desctiption, please stay within 72cols.
> Also, "it's" is most probably wrong in this sentence.
> 

Improve this as well.

>>pins implementation to netdevsim. Creating netdevsim devices and adding ports
>>to it register new DPLL devices and pins. First port of each netdevsim device
> 
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This sentence does not make
> sense to me. Pehaps rephrase a bit?
> 
> 
>>acts as a entitiy which registers two DPLL devices: EEC and PPS DPLLs. First
> 
> typo: "entitiy"

Ok.

>>port also register the common pins: PPS and GNSS. Additionally each port
>>register also RCLK (recovered clock) pin for itself. That allow us to check
>>mutliple scenarios which might be problematic in real implementations (like
>>different ordering etc.)
>>
>>Patch adds few helper scripts, which are:
>>1) tools/testing/selftests/dpll/run_dpll_tests.sh
> 
> Please make this part of
> tools/testing/selftests/drivers/net/netdevsim/
> No special threat of dpll needed.
> 

That make a lot of sense to move it there, thanks for noticing that.

>>    Script is checking for all dependencies, creates temporary
>>    environment, installs required libraries and run all tests - can be
>>    used standalone
>>2) tools/testing/selftests/dpll/ynlfamilyhandler.py˙
>>    Library for easier ynl use in the pytest framework - can be used
>>    standalone
>>
>>[1] https://lore.kernel.org/netdev/169494842736.21621.10730860855645661664.git-patchwork-notify@kernel.org/
>>
>>Changelog:
>>v1 -> v2:
>>- moved from separate module to implementation in netdevsim
>>
>>Michal Michalik (2):
>>  netdevsim: implement DPLL for subsystem selftests
>>  selftests/dpll: add DPLL system integration selftests
>>
>> drivers/net/Kconfig                              |   1 +
>> drivers/net/netdevsim/Makefile                   |   2 +-
>> drivers/net/netdevsim/dpll.c                     | 438 +++++++++++++++++++++++
>> drivers/net/netdevsim/dpll.h                     |  81 +++++
>> drivers/net/netdevsim/netdev.c                   |  20 ++
>> drivers/net/netdevsim/netdevsim.h                |   4 +
>> tools/testing/selftests/Makefile                 |   1 +
>> tools/testing/selftests/dpll/Makefile            |   8 +
>> tools/testing/selftests/dpll/__init__.py         |   0
>> tools/testing/selftests/dpll/config              |   2 +
>> tools/testing/selftests/dpll/consts.py           |  34 ++
>> tools/testing/selftests/dpll/dpll_utils.py       | 109 ++++++
>> tools/testing/selftests/dpll/requirements.txt    |   3 +
>> tools/testing/selftests/dpll/run_dpll_tests.sh   |  75 ++++
>> tools/testing/selftests/dpll/test_dpll.py        | 414 +++++++++++++++++++++
>> tools/testing/selftests/dpll/ynlfamilyhandler.py |  49 +++
>> 16 files changed, 1240 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/net/netdevsim/dpll.c
>> create mode 100644 drivers/net/netdevsim/dpll.h
>> create mode 100644 tools/testing/selftests/dpll/Makefile
>> create mode 100644 tools/testing/selftests/dpll/__init__.py
>> create mode 100644 tools/testing/selftests/dpll/config
>> create mode 100644 tools/testing/selftests/dpll/consts.py
>> create mode 100644 tools/testing/selftests/dpll/dpll_utils.py
>> create mode 100644 tools/testing/selftests/dpll/requirements.txt
>> create mode 100755 tools/testing/selftests/dpll/run_dpll_tests.sh
>> create mode 100644 tools/testing/selftests/dpll/test_dpll.py
>> create mode 100644 tools/testing/selftests/dpll/ynlfamilyhandler.py
>>
>>-- 
>>2.9.5
>>
>>base-commit: 55c900477f5b3897d9038446f72a281cae0efd86