[RFC PATCH v1 0/3] Create common DPLL/clock configuration API

[RFC PATCH v1 0/3] Create common DPLL/clock configuration API

[Vadim Fedorenko]

From: Vadim Fedorenko <vadfed@fb.com>

Implement common API for clock/DPLL configuration and status reporting.
The API utilises netlink interface as transport for commands and event
notifications. This API aim to extend current pin configuration and
make it flexible and easy to cover special configurations.

v0 -> v1:
 * fix code style and errors
 * add linux-arm mailing list

Vadim Fedorenko (3):
  dpll: Add DPLL framework base functions
  dpll: add netlink events
  ptp_ocp: implement DPLL ops

 MAINTAINERS                 |   8 +
 drivers/Kconfig             |   2 +
 drivers/Makefile            |   1 +
 drivers/dpll/Kconfig        |   7 +
 drivers/dpll/Makefile       |   7 +
 drivers/dpll/dpll_core.c    | 154 ++++++++++
 drivers/dpll/dpll_core.h    |  40 +++
 drivers/dpll/dpll_netlink.c | 578 ++++++++++++++++++++++++++++++++++++
 drivers/dpll/dpll_netlink.h |  14 +
 drivers/ptp/Kconfig         |   1 +
 drivers/ptp/ptp_ocp.c       |  86 ++++++
 include/linux/dpll.h        |  25 ++
 include/uapi/linux/dpll.h   |  77 +++++
 13 files changed, 1000 insertions(+)
 create mode 100644 drivers/dpll/Kconfig
 create mode 100644 drivers/dpll/Makefile
 create mode 100644 drivers/dpll/dpll_core.c
 create mode 100644 drivers/dpll/dpll_core.h
 create mode 100644 drivers/dpll/dpll_netlink.c
 create mode 100644 drivers/dpll/dpll_netlink.h
 create mode 100644 include/linux/dpll.h
 create mode 100644 include/uapi/linux/dpll.h

-- 
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

[Vadim Fedorenko]

From: Vadim Fedorenko <vadfed@fb.com>

DPLL framework is used to represent and configure DPLL devices
in systems. Each device that has DPLL and can configure sources
and outputs can use this framework.

Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
---
 MAINTAINERS                 |   8 +
 drivers/Kconfig             |   2 +
 drivers/Makefile            |   1 +
 drivers/dpll/Kconfig        |   7 +
 drivers/dpll/Makefile       |   7 +
 drivers/dpll/dpll_core.c    | 152 +++++++++++++
 drivers/dpll/dpll_core.h    |  40 ++++
 drivers/dpll/dpll_netlink.c | 437 ++++++++++++++++++++++++++++++++++++
 drivers/dpll/dpll_netlink.h |   7 +
 include/linux/dpll.h        |  25 +++
 include/uapi/linux/dpll.h   |  77 +++++++
 11 files changed, 763 insertions(+)
 create mode 100644 drivers/dpll/Kconfig
 create mode 100644 drivers/dpll/Makefile
 create mode 100644 drivers/dpll/dpll_core.c
 create mode 100644 drivers/dpll/dpll_core.h
 create mode 100644 drivers/dpll/dpll_netlink.c
 create mode 100644 drivers/dpll/dpll_netlink.h
 create mode 100644 include/linux/dpll.h
 create mode 100644 include/uapi/linux/dpll.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 05fcbea3e432..5532130baf36 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6122,6 +6122,14 @@ F:	Documentation/networking/device_drivers/ethernet/freescale/dpaa2/switch-drive
 F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-switch*
 F:	drivers/net/ethernet/freescale/dpaa2/dpsw*
 
+DPLL CLOCK SUBSYSTEM
+M:	Vadim Fedorenko <vadfed@fb.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	drivers/dpll/*
+F:	include/net/dpll.h
+F:	include/uapi/linux/dpll.h
+
 DPT_I2O SCSI RAID DRIVER
 M:	Adaptec OEM Raid Solutions <aacraid@microsemi.com>
 L:	linux-scsi@vger.kernel.org
diff --git a/drivers/Kconfig b/drivers/Kconfig
index b6a172d32a7d..dcdc23116eb8 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -241,4 +241,6 @@ source "drivers/peci/Kconfig"
 
 source "drivers/hte/Kconfig"
 
+source "drivers/dpll/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 9a30842b22c5..acc370a2cda6 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -189,3 +189,4 @@ obj-$(CONFIG_COUNTER)		+= counter/
 obj-$(CONFIG_MOST)		+= most/
 obj-$(CONFIG_PECI)		+= peci/
 obj-$(CONFIG_HTE)		+= hte/
+obj-$(CONFIG_DPLL)		+= dpll/
diff --git a/drivers/dpll/Kconfig b/drivers/dpll/Kconfig
new file mode 100644
index 000000000000..a4cae73f20d3
--- /dev/null
+++ b/drivers/dpll/Kconfig
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Generic DPLL drivers configuration
+#
+
+config DPLL
+  bool
diff --git a/drivers/dpll/Makefile b/drivers/dpll/Makefile
new file mode 100644
index 000000000000..0748c80097e4
--- /dev/null
+++ b/drivers/dpll/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for DPLL drivers.
+#
+
+obj-$(CONFIG_DPLL)          += dpll_sys.o
+dpll_sys-y                  += dpll_core.o dpll_netlink.o
diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
new file mode 100644
index 000000000000..e34767e723cf
--- /dev/null
+++ b/drivers/dpll/dpll_core.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *  dpll_core.c - Generic DPLL Management class support.
+ *
+ *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+#include "dpll_core.h"
+
+static DEFINE_MUTEX(dpll_device_xa_lock);
+static DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC);
+#define DPLL_REGISTERED XA_MARK_1
+
+#define ASSERT_DPLL_REGISTERED(d)                                           \
+	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
+#define ASSERT_DPLL_NOT_REGISTERED(d)                                      \
+	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
+
+
+int for_each_dpll_device(int id, int (*cb)(struct dpll_device *, void *), void *data)
+{
+	struct dpll_device *dpll;
+	unsigned long index;
+	int ret = 0;
+
+	mutex_lock(&dpll_device_xa_lock);
+	xa_for_each_start(&dpll_device_xa, index, dpll, id) {
+		if (!xa_get_mark(&dpll_device_xa, index, DPLL_REGISTERED))
+			continue;
+		ret = cb(dpll, data);
+		if (ret)
+			break;
+	}
+	mutex_unlock(&dpll_device_xa_lock);
+
+	return ret;
+}
+
+struct dpll_device *dpll_device_get_by_id(int id)
+{
+	struct dpll_device *dpll = NULL;
+
+	if (xa_get_mark(&dpll_device_xa, id, DPLL_REGISTERED))
+		dpll = xa_load(&dpll_device_xa, id);
+	return dpll;
+}
+
+void *dpll_priv(struct dpll_device *dpll)
+{
+	return dpll->priv;
+}
+EXPORT_SYMBOL_GPL(dpll_priv);
+
+static void dpll_device_release(struct device *dev)
+{
+	struct dpll_device *dpll;
+
+	dpll = to_dpll_device(dev);
+
+	dpll_device_unregister(dpll);
+
+	mutex_destroy(&dpll->lock);
+	kfree(dpll);
+}
+
+static struct class dpll_class = {
+	.name = "dpll",
+	.dev_release = dpll_device_release,
+};
+
+struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
+					 int outputs_count, void *priv)
+{
+	struct dpll_device *dpll;
+	int ret;
+
+	dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
+	if (!dpll)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_init(&dpll->lock);
+	dpll->ops = ops;
+	dpll->dev.class = &dpll_class;
+	dpll->sources_count = sources_count;
+	dpll->outputs_count = outputs_count;
+
+	mutex_lock(&dpll_device_xa_lock);
+	ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, GFP_KERNEL);
+	if (ret)
+		goto error;
+	dev_set_name(&dpll->dev, "dpll%d", dpll->id);
+	mutex_unlock(&dpll_device_xa_lock);
+	dpll->priv = priv;
+
+	return dpll;
+
+error:
+	mutex_unlock(&dpll_device_xa_lock);
+	kfree(dpll);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(dpll_device_alloc);
+
+void dpll_device_register(struct dpll_device *dpll)
+{
+	ASSERT_DPLL_NOT_REGISTERED(dpll);
+
+	mutex_lock(&dpll_device_xa_lock);
+	xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);
+	dpll_notify_device_create(dpll->id, dev_name(&dpll->dev));
+	mutex_unlock(&dpll_device_xa_lock);
+}
+EXPORT_SYMBOL_GPL(dpll_device_register);
+
+void dpll_device_unregister(struct dpll_device *dpll)
+{
+	ASSERT_DPLL_REGISTERED(dpll);
+
+	mutex_lock(&dpll_device_xa_lock);
+	xa_erase(&dpll_device_xa, dpll->id);
+	mutex_unlock(&dpll_device_xa_lock);
+}
+EXPORT_SYMBOL_GPL(dpll_device_unregister);
+
+static int __init dpll_init(void)
+{
+	int ret;
+
+	ret = dpll_netlink_init();
+	if (ret)
+		goto error;
+
+	ret = class_register(&dpll_class);
+	if (ret)
+		goto unregister_netlink;
+
+	return 0;
+
+unregister_netlink:
+	dpll_netlink_finish();
+error:
+	mutex_destroy(&dpll_device_xa_lock);
+	return ret;
+}
+subsys_initcall(dpll_init);
diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
new file mode 100644
index 000000000000..5ad3224d5caf
--- /dev/null
+++ b/drivers/dpll/dpll_core.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
+ */
+
+#ifndef __DPLL_CORE_H__
+#define __DPLL_CORE_H__
+
+#include <linux/dpll.h>
+
+#include "dpll_netlink.h"
+
+/**
+ * struct dpll_device - structure for a DPLL device
+ * @id:		unique id number for each edvice
+ * @dev:	&struct device for this dpll device
+ * @sources_count:	amount of input sources this dpll_device supports
+ * @outputs_count:	amount of outputs this dpll_device supports
+ * @ops:	operations this &dpll_device supports
+ * @lock:	mutex to serialize operations
+ * @priv:	pointer to private information of owner
+ */
+struct dpll_device {
+	int id;
+	struct device dev;
+	int sources_count;
+	int outputs_count;
+	struct dpll_device_ops *ops;
+	struct mutex lock;
+	void *priv;
+};
+
+#define to_dpll_device(_dev) \
+	container_of(_dev, struct dpll_device, dev)
+
+int for_each_dpll_device(int id, int (*cb)(struct dpll_device *, void *),
+			  void *data);
+struct dpll_device *dpll_device_get_by_id(int id);
+void dpll_device_unregister(struct dpll_device *dpll);
+#endif
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
new file mode 100644
index 000000000000..0bbdaa6dde8e
--- /dev/null
+++ b/drivers/dpll/dpll_netlink.c
@@ -0,0 +1,437 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic netlink for DPLL management framework
+ *
+ * Copyright (c) 2021 Meta Platforms, Inc. and affiliates
+ *
+ */
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <net/genetlink.h>
+#include "dpll_core.h"
+
+#include <uapi/linux/dpll.h>
+
+static const struct genl_multicast_group dpll_genl_mcgrps[] = {
+	{ .name = DPLL_CONFIG_DEVICE_GROUP_NAME, },
+	{ .name = DPLL_CONFIG_SOURCE_GROUP_NAME, },
+	{ .name = DPLL_CONFIG_OUTPUT_GROUP_NAME, },
+	{ .name = DPLL_MONITOR_GROUP_NAME,  },
+};
+
+static const struct nla_policy dpll_genl_get_policy[] = {
+	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
+	[DPLLA_DEVICE_NAME]	= { .type = NLA_STRING,
+				    .len = DPLL_NAME_LENGTH },
+	[DPLLA_FLAGS]		= { .type = NLA_U32 },
+};
+
+static const struct nla_policy dpll_genl_set_source_policy[] = {
+	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
+	[DPLLA_SOURCE_ID]	= { .type = NLA_U32 },
+	[DPLLA_SOURCE_TYPE]	= { .type = NLA_U32 },
+};
+
+static const struct nla_policy dpll_genl_set_output_policy[] = {
+	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
+	[DPLLA_OUTPUT_ID]	= { .type = NLA_U32 },
+	[DPLLA_OUTPUT_TYPE]	= { .type = NLA_U32 },
+};
+
+struct param {
+	struct netlink_callback *cb;
+	struct dpll_device *dpll;
+	struct nlattr **attrs;
+	struct sk_buff *msg;
+	int dpll_id;
+	int dpll_source_id;
+	int dpll_source_type;
+	int dpll_output_id;
+	int dpll_output_type;
+};
+
+struct dpll_dump_ctx {
+	struct dpll_device *dev;
+	int flags;
+	int pos_idx;
+	int pos_src_idx;
+	int pos_out_idx;
+};
+
+typedef int (*cb_t)(struct param *);
+
+static struct genl_family dpll_gnl_family;
+
+static struct dpll_dump_ctx *dpll_dump_context(struct netlink_callback *cb)
+{
+	return (struct dpll_dump_ctx *)cb->ctx;
+}
+
+static int __dpll_cmd_device_dump_one(struct dpll_device *dpll,
+					   struct sk_buff *msg)
+{
+	if (nla_put_u32(msg, DPLLA_DEVICE_ID, dpll->id))
+		return -EMSGSIZE;
+
+	if (nla_put_string(msg, DPLLA_DEVICE_NAME, dev_name(&dpll->dev)))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int __dpll_cmd_dump_sources(struct dpll_device *dpll,
+					   struct sk_buff *msg)
+{
+	struct nlattr *src_attr;
+	int i, ret = 0, type;
+
+	for (i = 0; i < dpll->sources_count; i++) {
+		src_attr = nla_nest_start(msg, DPLLA_SOURCE);
+		if (!src_attr) {
+			ret = -EMSGSIZE;
+			break;
+		}
+		type = dpll->ops->get_source_type(dpll, i);
+		if (nla_put_u32(msg, DPLLA_SOURCE_ID, i) ||
+		    nla_put_u32(msg, DPLLA_SOURCE_TYPE, type)) {
+			nla_nest_cancel(msg, src_attr);
+			ret = -EMSGSIZE;
+			break;
+		}
+		nla_nest_end(msg, src_attr);
+	}
+
+	return ret;
+}
+
+static int __dpll_cmd_dump_outputs(struct dpll_device *dpll,
+					   struct sk_buff *msg)
+{
+	struct nlattr *out_attr;
+	int i, ret = 0, type;
+
+	for (i = 0; i < dpll->outputs_count; i++) {
+		out_attr = nla_nest_start(msg, DPLLA_OUTPUT);
+		if (!out_attr) {
+			ret = -EMSGSIZE;
+			break;
+		}
+		type = dpll->ops->get_source_type(dpll, i);
+		if (nla_put_u32(msg, DPLLA_OUTPUT_ID, i) ||
+		    nla_put_u32(msg, DPLLA_OUTPUT_TYPE, type)) {
+			nla_nest_cancel(msg, out_attr);
+			ret = -EMSGSIZE;
+			break;
+		}
+		nla_nest_end(msg, out_attr);
+	}
+
+	return ret;
+}
+
+static int __dpll_cmd_dump_status(struct dpll_device *dpll,
+					   struct sk_buff *msg)
+{
+	int ret;
+
+	if (!dpll->ops->get_status && !dpll->ops->get_temp && !dpll->ops->get_lock_status)
+		return 0;
+
+	if (dpll->ops->get_status) {
+		ret = dpll->ops->get_status(dpll);
+		if (nla_put_u32(msg, DPLLA_STATUS, ret))
+			return -EMSGSIZE;
+	}
+
+	if (dpll->ops->get_temp) {
+		ret = dpll->ops->get_status(dpll);
+		if (nla_put_u32(msg, DPLLA_TEMP, ret))
+			return -EMSGSIZE;
+	}
+
+	if (dpll->ops->get_lock_status) {
+		ret = dpll->ops->get_lock_status(dpll);
+		if (nla_put_u32(msg, DPLLA_LOCK_STATUS, ret))
+			return -EMSGSIZE;
+	}
+
+	return 0;
+}
+
+static int dpll_device_dump_one(struct dpll_device *dev, struct sk_buff *msg, int flags)
+{
+	struct nlattr *hdr;
+	int ret;
+
+	hdr = nla_nest_start(msg, DPLLA_DEVICE);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	mutex_lock(&dev->lock);
+	ret = __dpll_cmd_device_dump_one(dev, msg);
+	if (ret)
+		goto out_cancel_nest;
+
+	if (flags & DPLL_FLAG_SOURCES && dev->ops->get_source_type) {
+		ret = __dpll_cmd_dump_sources(dev, msg);
+		if (ret)
+			goto out_cancel_nest;
+	}
+
+	if (flags & DPLL_FLAG_OUTPUTS && dev->ops->get_output_type) {
+		ret = __dpll_cmd_dump_outputs(dev, msg);
+		if (ret)
+			goto out_cancel_nest;
+	}
+
+	if (flags & DPLL_FLAG_STATUS) {
+		ret = __dpll_cmd_dump_status(dev, msg);
+		if (ret)
+			goto out_cancel_nest;
+	}
+
+	mutex_unlock(&dev->lock);
+	nla_nest_end(msg, hdr);
+
+	return 0;
+
+out_cancel_nest:
+	mutex_unlock(&dev->lock);
+	nla_nest_cancel(msg, hdr);
+
+	return ret;
+}
+
+static int dpll_genl_cmd_set_source(struct param *p)
+{
+	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
+	struct dpll_device *dpll = p->dpll;
+	int ret = 0, src_id, type;
+
+	if (!info->attrs[DPLLA_SOURCE_ID] ||
+	    !info->attrs[DPLLA_SOURCE_TYPE])
+		return -EINVAL;
+
+	if (!dpll->ops->set_source_type)
+		return -EOPNOTSUPP;
+
+	src_id = nla_get_u32(info->attrs[DPLLA_SOURCE_ID]);
+	type = nla_get_u32(info->attrs[DPLLA_SOURCE_TYPE]);
+
+	mutex_lock(&dpll->lock);
+	ret = dpll->ops->set_source_type(dpll, src_id, type);
+	mutex_unlock(&dpll->lock);
+
+	return ret;
+}
+
+static int dpll_genl_cmd_set_output(struct param *p)
+{
+	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
+	struct dpll_device *dpll = p->dpll;
+	int ret = 0, out_id, type;
+
+	if (!info->attrs[DPLLA_OUTPUT_ID] ||
+	    !info->attrs[DPLLA_OUTPUT_TYPE])
+		return -EINVAL;
+
+	if (!dpll->ops->set_output_type)
+		return -EOPNOTSUPP;
+
+	out_id = nla_get_u32(info->attrs[DPLLA_OUTPUT_ID]);
+	type = nla_get_u32(info->attrs[DPLLA_OUTPUT_TYPE]);
+
+	mutex_lock(&dpll->lock);
+	ret = dpll->ops->set_source_type(dpll, out_id, type);
+	mutex_unlock(&dpll->lock);
+
+	return ret;
+}
+
+static int dpll_device_loop_cb(struct dpll_device *dpll, void *data)
+{
+	struct dpll_dump_ctx *ctx;
+	struct param *p = (struct param *)data;
+
+	ctx = dpll_dump_context(p->cb);
+
+	ctx->pos_idx = dpll->id;
+
+	return dpll_device_dump_one(dpll, p->msg, ctx->flags);
+}
+
+static int dpll_cmd_device_dump(struct param *p)
+{
+	struct dpll_dump_ctx *ctx = dpll_dump_context(p->cb);
+
+	return for_each_dpll_device(ctx->pos_idx, dpll_device_loop_cb, p);
+}
+
+static int dpll_genl_cmd_device_get_id(struct param *p)
+{
+	struct dpll_device *dpll = p->dpll;
+	int flags = 0;
+
+	if (p->attrs[DPLLA_FLAGS])
+		flags = nla_get_u32(p->attrs[DPLLA_FLAGS]);
+
+	return dpll_device_dump_one(dpll, p->msg, flags);
+}
+
+static cb_t cmd_doit_cb[] = {
+	[DPLL_CMD_DEVICE_GET]		= dpll_genl_cmd_device_get_id,
+	[DPLL_CMD_SET_SOURCE_TYPE]	= dpll_genl_cmd_set_source,
+	[DPLL_CMD_SET_OUTPUT_TYPE]	= dpll_genl_cmd_set_output,
+};
+
+static cb_t cmd_dump_cb[] = {
+	[DPLL_CMD_DEVICE_GET]		= dpll_cmd_device_dump,
+};
+
+static int dpll_genl_cmd_start(struct netlink_callback *cb)
+{
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	struct dpll_dump_ctx *ctx = dpll_dump_context(cb);
+
+	ctx->dev = NULL;
+	if (info->attrs[DPLLA_FLAGS])
+		ctx->flags = nla_get_u32(info->attrs[DPLLA_FLAGS]);
+	else
+		ctx->flags = 0;
+	ctx->pos_idx = 0;
+	ctx->pos_src_idx = 0;
+	ctx->pos_out_idx = 0;
+	return 0;
+}
+
+static int dpll_genl_cmd_dumpit(struct sk_buff *skb,
+				   struct netlink_callback *cb)
+{
+	struct param p = { .cb = cb, .msg = skb };
+	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
+	int cmd = info->op.cmd;
+	int ret;
+	void *hdr;
+
+	hdr = genlmsg_put(skb, 0, 0, &dpll_gnl_family, 0, cmd);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	ret = cmd_dump_cb[cmd](&p);
+	if (ret)
+		goto out_cancel_msg;
+
+	genlmsg_end(skb, hdr);
+
+	return 0;
+
+out_cancel_msg:
+	genlmsg_cancel(skb, hdr);
+
+	return ret;
+}
+
+static int dpll_genl_cmd_doit(struct sk_buff *skb,
+				 struct genl_info *info)
+{
+	struct param p = { .attrs = info->attrs, .dpll = info->user_ptr[0] };
+	int cmd = info->genlhdr->cmd;
+	struct sk_buff *msg;
+	void *hdr;
+	int ret;
+
+	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+	p.msg = msg;
+
+	hdr = genlmsg_put_reply(msg, info, &dpll_gnl_family, 0, cmd);
+	if (!hdr) {
+		ret = -EMSGSIZE;
+		goto out_free_msg;
+	}
+
+	ret = cmd_doit_cb[cmd](&p);
+	if (ret)
+		goto out_cancel_msg;
+
+	genlmsg_end(msg, hdr);
+
+	return genlmsg_reply(msg, info);
+
+out_cancel_msg:
+	genlmsg_cancel(msg, hdr);
+out_free_msg:
+	nlmsg_free(msg);
+
+	return ret;
+}
+
+static int dpll_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
+						 struct genl_info *info)
+{
+	struct dpll_device *dpll;
+	int id;
+
+	if (!info->attrs[DPLLA_DEVICE_ID])
+		return -EINVAL;
+	id = nla_get_u32(info->attrs[DPLLA_DEVICE_ID]);
+
+	dpll = dpll_device_get_by_id(id);
+	if (!dpll)
+		return -ENODEV;
+	info->user_ptr[0] = dpll;
+
+	return 0;
+}
+
+static const struct genl_ops dpll_genl_ops[] = {
+	{
+		.cmd	= DPLL_CMD_DEVICE_GET,
+		.start	= dpll_genl_cmd_start,
+		.dumpit	= dpll_genl_cmd_dumpit,
+		.doit	= dpll_genl_cmd_doit,
+		.policy	= dpll_genl_get_policy,
+		.maxattr = ARRAY_SIZE(dpll_genl_get_policy) - 1,
+	},
+	{
+		.cmd	= DPLL_CMD_SET_SOURCE_TYPE,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= dpll_genl_cmd_doit,
+		.policy	= dpll_genl_set_source_policy,
+		.maxattr = ARRAY_SIZE(dpll_genl_set_source_policy) - 1,
+	},
+	{
+		.cmd	= DPLL_CMD_SET_OUTPUT_TYPE,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= dpll_genl_cmd_doit,
+		.policy	= dpll_genl_set_output_policy,
+		.maxattr = ARRAY_SIZE(dpll_genl_set_output_policy) - 1,
+	},
+};
+
+static struct genl_family dpll_gnl_family __ro_after_init = {
+	.hdrsize	= 0,
+	.name		= DPLL_FAMILY_NAME,
+	.version	= DPLL_VERSION,
+	.ops		= dpll_genl_ops,
+	.n_ops		= ARRAY_SIZE(dpll_genl_ops),
+	.mcgrps		= dpll_genl_mcgrps,
+	.n_mcgrps	= ARRAY_SIZE(dpll_genl_mcgrps),
+	.pre_doit	= dpll_pre_doit,
+};
+
+int __init dpll_netlink_init(void)
+{
+	return genl_register_family(&dpll_gnl_family);
+}
+
+void dpll_netlink_finish(void)
+{
+	genl_unregister_family(&dpll_gnl_family);
+}
+
+void __exit dpll_netlink_fini(void)
+{
+	dpll_netlink_finish();
+}
diff --git a/drivers/dpll/dpll_netlink.h b/drivers/dpll/dpll_netlink.h
new file mode 100644
index 000000000000..e2d100f59dd6
--- /dev/null
+++ b/drivers/dpll/dpll_netlink.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
+ */
+
+int __init dpll_netlink_init(void);
+void dpll_netlink_finish(void);
diff --git a/include/linux/dpll.h b/include/linux/dpll.h
new file mode 100644
index 000000000000..9051337bcf9e
--- /dev/null
+++ b/include/linux/dpll.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
+ */
+
+#ifndef __DPLL_H__
+#define __DPLL_H__
+
+struct dpll_device;
+
+struct dpll_device_ops {
+	int (*get_status)(struct dpll_device *dpll);
+	int (*get_temp)(struct dpll_device *dpll);
+	int (*get_lock_status)(struct dpll_device *dpll);
+	int (*get_source_type)(struct dpll_device *dpll, int id);
+	int (*get_output_type)(struct dpll_device *dpll, int id);
+	int (*set_source_type)(struct dpll_device *dpll, int id, int val);
+	int (*set_output_type)(struct dpll_device *dpll, int id, int val);
+};
+
+struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
+					 int outputs_count, void *priv);
+void dpll_device_register(struct dpll_device *dpll);
+void *dpll_priv(struct dpll_device *dpll);
+#endif
diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
new file mode 100644
index 000000000000..8c00f52736ee
--- /dev/null
+++ b/include/uapi/linux/dpll.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_DPLL_H
+#define _UAPI_LINUX_DPLL_H
+
+#define DPLL_NAME_LENGTH	20
+
+/* Adding event notification support elements */
+#define DPLL_FAMILY_NAME		"dpll"
+#define DPLL_VERSION			0x01
+#define DPLL_CONFIG_DEVICE_GROUP_NAME  "config"
+#define DPLL_CONFIG_SOURCE_GROUP_NAME  "source"
+#define DPLL_CONFIG_OUTPUT_GROUP_NAME  "output"
+#define DPLL_MONITOR_GROUP_NAME        "monitor"
+
+#define DPLL_FLAG_SOURCES	1
+#define DPLL_FLAG_OUTPUTS	2
+#define DPLL_FLAG_STATUS	4
+
+/* Attributes of dpll_genl_family */
+enum dpll_genl_get_attr {
+	DPLLA_UNSPEC,
+	DPLLA_DEVICE,
+	DPLLA_DEVICE_ID,
+	DPLLA_DEVICE_NAME,
+	DPLLA_SOURCE,
+	DPLLA_SOURCE_ID,
+	DPLLA_SOURCE_TYPE,
+	DPLLA_OUTPUT,
+	DPLLA_OUTPUT_ID,
+	DPLLA_OUTPUT_TYPE,
+	DPLLA_STATUS,
+	DPLLA_TEMP,
+	DPLLA_LOCK_STATUS,
+	DPLLA_FLAGS,
+
+	__DPLLA_MAX,
+};
+#define DPLLA_GET_MAX (__DPLLA_MAX - 1)
+
+/* DPLL signal types used as source or as output */
+enum dpll_genl_signal_type {
+	DPLL_TYPE_EXT_1PPS,
+	DPLL_TYPE_EXT_10MHZ,
+	DPLL_TYPE_SYNCE_ETH_PORT,
+	DPLL_TYPE_INT_OSCILLATOR,
+	DPLL_TYPE_GNSS,
+
+	__DPLL_TYPE_MAX,
+};
+#define DPLL_TYPE_MAX (__DPLL_TYPE_MAX - 1)
+
+/* Events of dpll_genl_family */
+enum dpll_genl_event {
+	DPLL_EVENT_UNSPEC,
+	DPLL_EVENT_DEVICE_CREATE,		/* DPLL device creation */
+	DPLL_EVENT_DEVICE_DELETE,		/* DPLL device deletion */
+	DPLL_EVENT_STATUS_LOCKED,		/* DPLL device locked to source */
+	DPLL_EVENT_STATUS_UNLOCKED,	/* DPLL device freerun */
+	DPLL_EVENT_SOURCE_CHANGE,		/* DPLL device source changed */
+	DPLL_EVENT_OUTPUT_CHANGE,		/* DPLL device output changed */
+
+	__DPLL_EVENT_MAX,
+};
+#define DPLL_EVENT_MAX (__DPLL_EVENT_MAX - 1)
+
+/* Commands supported by the dpll_genl_family */
+enum dpll_genl_cmd {
+	DPLL_CMD_UNSPEC,
+	DPLL_CMD_DEVICE_GET,	/* List of DPLL devices id */
+	DPLL_CMD_SET_SOURCE_TYPE,	/* Set the DPLL device source type */
+	DPLL_CMD_SET_OUTPUT_TYPE,	/* Get the DPLL device output type */
+
+	__DPLL_CMD_MAX,
+};
+#define DPLL_CMD_MAX (__DPLL_CMD_MAX - 1)
+
+#endif /* _UAPI_LINUX_DPLL_H */
-- 
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

[Kubalewski, Arkadiusz]

Hi Vadim,

Great work!

Although, I've been thinking that you already forget about it, so I have
started development of something similar.


-----Original Message-----
From: Vadim Fedorenko <vfedorenko@novek.ru> 
Sent: Thursday, June 23, 2022 2:57 AM
> 
> From: Vadim Fedorenko <vadfed@fb.com>
> 
> DPLL framework is used to represent and configure DPLL devices
> in systems. Each device that has DPLL and can configure sources
> and outputs can use this framework.
> 
> Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
> ---
>  MAINTAINERS                 |   8 +
>  drivers/Kconfig             |   2 +
>  drivers/Makefile            |   1 +
>  drivers/dpll/Kconfig        |   7 +
>  drivers/dpll/Makefile       |   7 +
>  drivers/dpll/dpll_core.c    | 152 +++++++++++++
>  drivers/dpll/dpll_core.h    |  40 ++++
>  drivers/dpll/dpll_netlink.c | 437 ++++++++++++++++++++++++++++++++++++
>  drivers/dpll/dpll_netlink.h |   7 +
>  include/linux/dpll.h        |  25 +++
>  include/uapi/linux/dpll.h   |  77 +++++++
>  11 files changed, 763 insertions(+)
>  create mode 100644 drivers/dpll/Kconfig
>  create mode 100644 drivers/dpll/Makefile
>  create mode 100644 drivers/dpll/dpll_core.c
>  create mode 100644 drivers/dpll/dpll_core.h
>  create mode 100644 drivers/dpll/dpll_netlink.c
>  create mode 100644 drivers/dpll/dpll_netlink.h
>  create mode 100644 include/linux/dpll.h
>  create mode 100644 include/uapi/linux/dpll.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 05fcbea3e432..5532130baf36 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6122,6 +6122,14 @@ F:	Documentation/networking/device_drivers/ethernet/freescale/dpaa2/switch-drive
>  F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-switch*
>  F:	drivers/net/ethernet/freescale/dpaa2/dpsw*
>  
> +DPLL CLOCK SUBSYSTEM
> +M:	Vadim Fedorenko <vadfed@fb.com>
> +L:	netdev@vger.kernel.org
> +S:	Maintained
> +F:	drivers/dpll/*
> +F:	include/net/dpll.h
> +F:	include/uapi/linux/dpll.h
> +
>  DPT_I2O SCSI RAID DRIVER
>  M:	Adaptec OEM Raid Solutions <aacraid@microsemi.com>
>  L:	linux-scsi@vger.kernel.org
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index b6a172d32a7d..dcdc23116eb8 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -241,4 +241,6 @@ source "drivers/peci/Kconfig"
>  
>  source "drivers/hte/Kconfig"
>  
> +source "drivers/dpll/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 9a30842b22c5..acc370a2cda6 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -189,3 +189,4 @@ obj-$(CONFIG_COUNTER)		+= counter/
>  obj-$(CONFIG_MOST)		+= most/
>  obj-$(CONFIG_PECI)		+= peci/
>  obj-$(CONFIG_HTE)		+= hte/
> +obj-$(CONFIG_DPLL)		+= dpll/
> diff --git a/drivers/dpll/Kconfig b/drivers/dpll/Kconfig
> new file mode 100644
> index 000000000000..a4cae73f20d3
> --- /dev/null
> +++ b/drivers/dpll/Kconfig
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Generic DPLL drivers configuration
> +#
> +
> +config DPLL
> +  bool

for RFC help and default were ommited?

> diff --git a/drivers/dpll/Makefile b/drivers/dpll/Makefile
> new file mode 100644
> index 000000000000..0748c80097e4
> --- /dev/null
> +++ b/drivers/dpll/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for DPLL drivers.
> +#
> +
> +obj-$(CONFIG_DPLL)          += dpll_sys.o
> +dpll_sys-y                  += dpll_core.o dpll_netlink.o
> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
> new file mode 100644
> index 000000000000..e34767e723cf
> --- /dev/null
> +++ b/drivers/dpll/dpll_core.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  dpll_core.c - Generic DPLL Management class support.
> + *
> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +
> +#include "dpll_core.h"
> +
> +static DEFINE_MUTEX(dpll_device_xa_lock);
> +static DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC);
> +#define DPLL_REGISTERED XA_MARK_1
> +
> +#define ASSERT_DPLL_REGISTERED(d)                                           \
> +	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
> +#define ASSERT_DPLL_NOT_REGISTERED(d)                                      \
> +	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
> +
> +
> +int for_each_dpll_device(int id, int (*cb)(struct dpll_device *, void *), void *data)
> +{
> +	struct dpll_device *dpll;
> +	unsigned long index;
> +	int ret = 0;
> +
> +	mutex_lock(&dpll_device_xa_lock);
> +	xa_for_each_start(&dpll_device_xa, index, dpll, id) {
> +		if (!xa_get_mark(&dpll_device_xa, index, DPLL_REGISTERED))
> +			continue;
> +		ret = cb(dpll, data);
> +		if (ret)
> +			break;
> +	}
> +	mutex_unlock(&dpll_device_xa_lock);
> +
> +	return ret;
> +}
> +
> +struct dpll_device *dpll_device_get_by_id(int id)
> +{
> +	struct dpll_device *dpll = NULL;
> +
> +	if (xa_get_mark(&dpll_device_xa, id, DPLL_REGISTERED))
> +		dpll = xa_load(&dpll_device_xa, id);
> +	return dpll;
> +}
> +
> +void *dpll_priv(struct dpll_device *dpll)
> +{
> +	return dpll->priv;
> +}
> +EXPORT_SYMBOL_GPL(dpll_priv);
> +
> +static void dpll_device_release(struct device *dev)
> +{
> +	struct dpll_device *dpll;
> +
> +	dpll = to_dpll_device(dev);
> +
> +	dpll_device_unregister(dpll);
> +
> +	mutex_destroy(&dpll->lock);
> +	kfree(dpll);
> +}
> +
> +static struct class dpll_class = {
> +	.name = "dpll",
> +	.dev_release = dpll_device_release,
> +};
> +
> +struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
> +					 int outputs_count, void *priv)
> +{
> +	struct dpll_device *dpll;
> +	int ret;
> +
> +	dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
> +	if (!dpll)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mutex_init(&dpll->lock);
> +	dpll->ops = ops;
> +	dpll->dev.class = &dpll_class;
> +	dpll->sources_count = sources_count;
> +	dpll->outputs_count = outputs_count;
> +
> +	mutex_lock(&dpll_device_xa_lock);
> +	ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, GFP_KERNEL);
> +	if (ret)
> +		goto error;
> +	dev_set_name(&dpll->dev, "dpll%d", dpll->id);
> +	mutex_unlock(&dpll_device_xa_lock);
> +	dpll->priv = priv;
> +
> +	return dpll;
> +
> +error:
> +	mutex_unlock(&dpll_device_xa_lock);
> +	kfree(dpll);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(dpll_device_alloc);
> +
> +void dpll_device_register(struct dpll_device *dpll)
> +{
> +	ASSERT_DPLL_NOT_REGISTERED(dpll);
> +
> +	mutex_lock(&dpll_device_xa_lock);
> +	xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);
> +	dpll_notify_device_create(dpll->id, dev_name(&dpll->dev));
> +	mutex_unlock(&dpll_device_xa_lock);
> +}
> +EXPORT_SYMBOL_GPL(dpll_device_register);
> +
> +void dpll_device_unregister(struct dpll_device *dpll)
> +{
> +	ASSERT_DPLL_REGISTERED(dpll);
> +
> +	mutex_lock(&dpll_device_xa_lock);
> +	xa_erase(&dpll_device_xa, dpll->id);
> +	mutex_unlock(&dpll_device_xa_lock);
> +}
> +EXPORT_SYMBOL_GPL(dpll_device_unregister);
> +
> +static int __init dpll_init(void)
> +{
> +	int ret;
> +
> +	ret = dpll_netlink_init();
> +	if (ret)
> +		goto error;
> +
> +	ret = class_register(&dpll_class);
> +	if (ret)
> +		goto unregister_netlink;
> +
> +	return 0;
> +
> +unregister_netlink:
> +	dpll_netlink_finish();
> +error:
> +	mutex_destroy(&dpll_device_xa_lock);
> +	return ret;
> +}
> +subsys_initcall(dpll_init);
> diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
> new file mode 100644
> index 000000000000..5ad3224d5caf
> --- /dev/null
> +++ b/drivers/dpll/dpll_core.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
> + */
> +
> +#ifndef __DPLL_CORE_H__
> +#define __DPLL_CORE_H__
> +
> +#include <linux/dpll.h>
> +
> +#include "dpll_netlink.h"
> +
> +/**
> + * struct dpll_device - structure for a DPLL device
> + * @id:		unique id number for each edvice
> + * @dev:	&struct device for this dpll device
> + * @sources_count:	amount of input sources this dpll_device supports
> + * @outputs_count:	amount of outputs this dpll_device supports
> + * @ops:	operations this &dpll_device supports
> + * @lock:	mutex to serialize operations
> + * @priv:	pointer to private information of owner
> + */
> +struct dpll_device {
> +	int id;
> +	struct device dev;
> +	int sources_count;
> +	int outputs_count;
> +	struct dpll_device_ops *ops;
> +	struct mutex lock;
> +	void *priv;
> +};
> +
> +#define to_dpll_device(_dev) \
> +	container_of(_dev, struct dpll_device, dev)
> +
> +int for_each_dpll_device(int id, int (*cb)(struct dpll_device *, void *),
> +			  void *data);
> +struct dpll_device *dpll_device_get_by_id(int id);
> +void dpll_device_unregister(struct dpll_device *dpll);
> +#endif
> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
> new file mode 100644
> index 000000000000..0bbdaa6dde8e
> --- /dev/null
> +++ b/drivers/dpll/dpll_netlink.c
> @@ -0,0 +1,437 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Generic netlink for DPLL management framework
> + *
> + * Copyright (c) 2021 Meta Platforms, Inc. and affiliates
> + *
> + */
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <net/genetlink.h>
> +#include "dpll_core.h"
> +
> +#include <uapi/linux/dpll.h>
> +
> +static const struct genl_multicast_group dpll_genl_mcgrps[] = {
> +	{ .name = DPLL_CONFIG_DEVICE_GROUP_NAME, },
> +	{ .name = DPLL_CONFIG_SOURCE_GROUP_NAME, },
> +	{ .name = DPLL_CONFIG_OUTPUT_GROUP_NAME, },
> +	{ .name = DPLL_MONITOR_GROUP_NAME,  },
> +};
> +
> +static const struct nla_policy dpll_genl_get_policy[] = {
> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
> +	[DPLLA_DEVICE_NAME]	= { .type = NLA_STRING,
> +				    .len = DPLL_NAME_LENGTH },
> +	[DPLLA_FLAGS]		= { .type = NLA_U32 },
> +};
> +
> +static const struct nla_policy dpll_genl_set_source_policy[] = {
> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
> +	[DPLLA_SOURCE_ID]	= { .type = NLA_U32 },
> +	[DPLLA_SOURCE_TYPE]	= { .type = NLA_U32 },
> +};
> +
> +static const struct nla_policy dpll_genl_set_output_policy[] = {
> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
> +	[DPLLA_OUTPUT_ID]	= { .type = NLA_U32 },
> +	[DPLLA_OUTPUT_TYPE]	= { .type = NLA_U32 },
> +};
> +
> +struct param {
> +	struct netlink_callback *cb;
> +	struct dpll_device *dpll;
> +	struct nlattr **attrs;
> +	struct sk_buff *msg;
> +	int dpll_id;
> +	int dpll_source_id;
> +	int dpll_source_type;
> +	int dpll_output_id;
> +	int dpll_output_type;
> +};
> +
> +struct dpll_dump_ctx {
> +	struct dpll_device *dev;
> +	int flags;
> +	int pos_idx;
> +	int pos_src_idx;
> +	int pos_out_idx;
> +};
> +
> +typedef int (*cb_t)(struct param *);
> +
> +static struct genl_family dpll_gnl_family;
> +
> +static struct dpll_dump_ctx *dpll_dump_context(struct netlink_callback *cb)
> +{
> +	return (struct dpll_dump_ctx *)cb->ctx;
> +}
> +
> +static int __dpll_cmd_device_dump_one(struct dpll_device *dpll,
> +					   struct sk_buff *msg)
> +{
> +	if (nla_put_u32(msg, DPLLA_DEVICE_ID, dpll->id))
> +		return -EMSGSIZE;
> +
> +	if (nla_put_string(msg, DPLLA_DEVICE_NAME, dev_name(&dpll->dev)))
> +		return -EMSGSIZE;
> +
> +	return 0;
> +}
> +
> +static int __dpll_cmd_dump_sources(struct dpll_device *dpll,
> +					   struct sk_buff *msg)
> +{
> +	struct nlattr *src_attr;
> +	int i, ret = 0, type;
> +
> +	for (i = 0; i < dpll->sources_count; i++) {
> +		src_attr = nla_nest_start(msg, DPLLA_SOURCE);
> +		if (!src_attr) {
> +			ret = -EMSGSIZE;
> +			break;
> +		}
> +		type = dpll->ops->get_source_type(dpll, i);
> +		if (nla_put_u32(msg, DPLLA_SOURCE_ID, i) ||
> +		    nla_put_u32(msg, DPLLA_SOURCE_TYPE, type)) {
> +			nla_nest_cancel(msg, src_attr);
> +			ret = -EMSGSIZE;
> +			break;
> +		}
> +		nla_nest_end(msg, src_attr);
> +	}
> +
> +	return ret;
> +}
> +
> +static int __dpll_cmd_dump_outputs(struct dpll_device *dpll,
> +					   struct sk_buff *msg)
> +{
> +	struct nlattr *out_attr;
> +	int i, ret = 0, type;
> +
> +	for (i = 0; i < dpll->outputs_count; i++) {
> +		out_attr = nla_nest_start(msg, DPLLA_OUTPUT);
> +		if (!out_attr) {
> +			ret = -EMSGSIZE;
> +			break;
> +		}
> +		type = dpll->ops->get_source_type(dpll, i);
> +		if (nla_put_u32(msg, DPLLA_OUTPUT_ID, i) ||
> +		    nla_put_u32(msg, DPLLA_OUTPUT_TYPE, type)) {
> +			nla_nest_cancel(msg, out_attr);
> +			ret = -EMSGSIZE;
> +			break;
> +		}
> +		nla_nest_end(msg, out_attr);
> +	}
> +
> +	return ret;
> +}
> +
> +static int __dpll_cmd_dump_status(struct dpll_device *dpll,
> +					   struct sk_buff *msg)
> +{
> +	int ret;
> +
> +	if (!dpll->ops->get_status && !dpll->ops->get_temp && !dpll->ops->get_lock_status)
> +		return 0;

what if dpll doesn't support one of those commands?

> +
> +	if (dpll->ops->get_status) {
> +		ret = dpll->ops->get_status(dpll);
> +		if (nla_put_u32(msg, DPLLA_STATUS, ret))
> +			return -EMSGSIZE;
> +	}
> +
> +	if (dpll->ops->get_temp) {
> +		ret = dpll->ops->get_status(dpll);
> +		if (nla_put_u32(msg, DPLLA_TEMP, ret))
> +			return -EMSGSIZE;
> +	}

shouldn't be get_temp(dpll)?

> +
> +	if (dpll->ops->get_lock_status) {
> +		ret = dpll->ops->get_lock_status(dpll);
> +		if (nla_put_u32(msg, DPLLA_LOCK_STATUS, ret))
> +			return -EMSGSIZE;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dpll_device_dump_one(struct dpll_device *dev, struct sk_buff *msg, int flags)
> +{
> +	struct nlattr *hdr;
> +	int ret;
> +
> +	hdr = nla_nest_start(msg, DPLLA_DEVICE);
> +	if (!hdr)
> +		return -EMSGSIZE;
> +
> +	mutex_lock(&dev->lock);
> +	ret = __dpll_cmd_device_dump_one(dev, msg);
> +	if (ret)
> +		goto out_cancel_nest;
> +
> +	if (flags & DPLL_FLAG_SOURCES && dev->ops->get_source_type) {
> +		ret = __dpll_cmd_dump_sources(dev, msg);
> +		if (ret)
> +			goto out_cancel_nest;
> +	}
> +
> +	if (flags & DPLL_FLAG_OUTPUTS && dev->ops->get_output_type) {
> +		ret = __dpll_cmd_dump_outputs(dev, msg);
> +		if (ret)
> +			goto out_cancel_nest;
> +	}
> +
> +	if (flags & DPLL_FLAG_STATUS) {
> +		ret = __dpll_cmd_dump_status(dev, msg);
> +		if (ret)
> +			goto out_cancel_nest;
> +	}
> +
> +	mutex_unlock(&dev->lock);
> +	nla_nest_end(msg, hdr);
> +
> +	return 0;
> +
> +out_cancel_nest:
> +	mutex_unlock(&dev->lock);
> +	nla_nest_cancel(msg, hdr);
> +
> +	return ret;
> +}
> +
> +static int dpll_genl_cmd_set_source(struct param *p)
> +{
> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
> +	struct dpll_device *dpll = p->dpll;
> +	int ret = 0, src_id, type;
> +
> +	if (!info->attrs[DPLLA_SOURCE_ID] ||
> +	    !info->attrs[DPLLA_SOURCE_TYPE])
> +		return -EINVAL;
> +
> +	if (!dpll->ops->set_source_type)
> +		return -EOPNOTSUPP;
> +
> +	src_id = nla_get_u32(info->attrs[DPLLA_SOURCE_ID]);
> +	type = nla_get_u32(info->attrs[DPLLA_SOURCE_TYPE]);
> +
> +	mutex_lock(&dpll->lock);
> +	ret = dpll->ops->set_source_type(dpll, src_id, type);
> +	mutex_unlock(&dpll->lock);
> +
> +	return ret;
> +}
> +
> +static int dpll_genl_cmd_set_output(struct param *p)
> +{
> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
> +	struct dpll_device *dpll = p->dpll;
> +	int ret = 0, out_id, type;
> +
> +	if (!info->attrs[DPLLA_OUTPUT_ID] ||
> +	    !info->attrs[DPLLA_OUTPUT_TYPE])
> +		return -EINVAL;
> +
> +	if (!dpll->ops->set_output_type)
> +		return -EOPNOTSUPP;
> +
> +	out_id = nla_get_u32(info->attrs[DPLLA_OUTPUT_ID]);
> +	type = nla_get_u32(info->attrs[DPLLA_OUTPUT_TYPE]);
> +
> +	mutex_lock(&dpll->lock);
> +	ret = dpll->ops->set_source_type(dpll, out_id, type);
> +	mutex_unlock(&dpll->lock);
> +
> +	return ret;
> +}
> +
> +static int dpll_device_loop_cb(struct dpll_device *dpll, void *data)
> +{
> +	struct dpll_dump_ctx *ctx;
> +	struct param *p = (struct param *)data;
> +
> +	ctx = dpll_dump_context(p->cb);
> +
> +	ctx->pos_idx = dpll->id;
> +
> +	return dpll_device_dump_one(dpll, p->msg, ctx->flags);
> +}
> +
> +static int dpll_cmd_device_dump(struct param *p)
> +{
> +	struct dpll_dump_ctx *ctx = dpll_dump_context(p->cb);
> +
> +	return for_each_dpll_device(ctx->pos_idx, dpll_device_loop_cb, p);
> +}
> +
> +static int dpll_genl_cmd_device_get_id(struct param *p)
> +{
> +	struct dpll_device *dpll = p->dpll;
> +	int flags = 0;
> +
> +	if (p->attrs[DPLLA_FLAGS])
> +		flags = nla_get_u32(p->attrs[DPLLA_FLAGS]);
> +
> +	return dpll_device_dump_one(dpll, p->msg, flags);
> +}
> +
> +static cb_t cmd_doit_cb[] = {
> +	[DPLL_CMD_DEVICE_GET]		= dpll_genl_cmd_device_get_id,
> +	[DPLL_CMD_SET_SOURCE_TYPE]	= dpll_genl_cmd_set_source,
> +	[DPLL_CMD_SET_OUTPUT_TYPE]	= dpll_genl_cmd_set_output,
> +};
> +
> +static cb_t cmd_dump_cb[] = {
> +	[DPLL_CMD_DEVICE_GET]		= dpll_cmd_device_dump,
> +};
> +
> +static int dpll_genl_cmd_start(struct netlink_callback *cb)
> +{
> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> +	struct dpll_dump_ctx *ctx = dpll_dump_context(cb);
> +
> +	ctx->dev = NULL;
> +	if (info->attrs[DPLLA_FLAGS])
> +		ctx->flags = nla_get_u32(info->attrs[DPLLA_FLAGS]);
> +	else
> +		ctx->flags = 0;
> +	ctx->pos_idx = 0;
> +	ctx->pos_src_idx = 0;
> +	ctx->pos_out_idx = 0;
> +	return 0;
> +}
> +
> +static int dpll_genl_cmd_dumpit(struct sk_buff *skb,
> +				   struct netlink_callback *cb)
> +{
> +	struct param p = { .cb = cb, .msg = skb };
> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
> +	int cmd = info->op.cmd;
> +	int ret;
> +	void *hdr;
> +
> +	hdr = genlmsg_put(skb, 0, 0, &dpll_gnl_family, 0, cmd);
> +	if (!hdr)
> +		return -EMSGSIZE;
> +
> +	ret = cmd_dump_cb[cmd](&p);
> +	if (ret)
> +		goto out_cancel_msg;
> +
> +	genlmsg_end(skb, hdr);
> +
> +	return 0;
> +
> +out_cancel_msg:
> +	genlmsg_cancel(skb, hdr);
> +
> +	return ret;
> +}
> +
> +static int dpll_genl_cmd_doit(struct sk_buff *skb,
> +				 struct genl_info *info)
> +{
> +	struct param p = { .attrs = info->attrs, .dpll = info->user_ptr[0] };
> +	int cmd = info->genlhdr->cmd;
> +	struct sk_buff *msg;
> +	void *hdr;
> +	int ret;
> +
> +	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +	p.msg = msg;
> +
> +	hdr = genlmsg_put_reply(msg, info, &dpll_gnl_family, 0, cmd);
> +	if (!hdr) {
> +		ret = -EMSGSIZE;
> +		goto out_free_msg;
> +	}
> +
> +	ret = cmd_doit_cb[cmd](&p);
> +	if (ret)
> +		goto out_cancel_msg;
> +
> +	genlmsg_end(msg, hdr);
> +
> +	return genlmsg_reply(msg, info);
> +
> +out_cancel_msg:
> +	genlmsg_cancel(msg, hdr);
> +out_free_msg:
> +	nlmsg_free(msg);
> +
> +	return ret;
> +}
> +
> +static int dpll_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
> +						 struct genl_info *info)
> +{
> +	struct dpll_device *dpll;
> +	int id;
> +
> +	if (!info->attrs[DPLLA_DEVICE_ID])
> +		return -EINVAL;
> +	id = nla_get_u32(info->attrs[DPLLA_DEVICE_ID]);
> +
> +	dpll = dpll_device_get_by_id(id);
> +	if (!dpll)
> +		return -ENODEV;
> +	info->user_ptr[0] = dpll;
> +
> +	return 0;
> +}
> +
> +static const struct genl_ops dpll_genl_ops[] = {
> +	{
> +		.cmd	= DPLL_CMD_DEVICE_GET,
> +		.start	= dpll_genl_cmd_start,
> +		.dumpit	= dpll_genl_cmd_dumpit,
> +		.doit	= dpll_genl_cmd_doit,
> +		.policy	= dpll_genl_get_policy,
> +		.maxattr = ARRAY_SIZE(dpll_genl_get_policy) - 1,
> +	},
> +	{
> +		.cmd	= DPLL_CMD_SET_SOURCE_TYPE,
> +		.flags	= GENL_UNS_ADMIN_PERM,
> +		.doit	= dpll_genl_cmd_doit,
> +		.policy	= dpll_genl_set_source_policy,
> +		.maxattr = ARRAY_SIZE(dpll_genl_set_source_policy) - 1,
> +	},
> +	{
> +		.cmd	= DPLL_CMD_SET_OUTPUT_TYPE,
> +		.flags	= GENL_UNS_ADMIN_PERM,
> +		.doit	= dpll_genl_cmd_doit,
> +		.policy	= dpll_genl_set_output_policy,
> +		.maxattr = ARRAY_SIZE(dpll_genl_set_output_policy) - 1,
> +	},
> +};
> +
> +static struct genl_family dpll_gnl_family __ro_after_init = {
> +	.hdrsize	= 0,
> +	.name		= DPLL_FAMILY_NAME,
> +	.version	= DPLL_VERSION,
> +	.ops		= dpll_genl_ops,
> +	.n_ops		= ARRAY_SIZE(dpll_genl_ops),
> +	.mcgrps		= dpll_genl_mcgrps,
> +	.n_mcgrps	= ARRAY_SIZE(dpll_genl_mcgrps),
> +	.pre_doit	= dpll_pre_doit,
> +};
> +
> +int __init dpll_netlink_init(void)
> +{
> +	return genl_register_family(&dpll_gnl_family);
> +}
> +
> +void dpll_netlink_finish(void)
> +{
> +	genl_unregister_family(&dpll_gnl_family);
> +}
> +
> +void __exit dpll_netlink_fini(void)
> +{
> +	dpll_netlink_finish();
> +}
> diff --git a/drivers/dpll/dpll_netlink.h b/drivers/dpll/dpll_netlink.h
> new file mode 100644
> index 000000000000..e2d100f59dd6
> --- /dev/null
> +++ b/drivers/dpll/dpll_netlink.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
> + */
> +
> +int __init dpll_netlink_init(void);
> +void dpll_netlink_finish(void);
> diff --git a/include/linux/dpll.h b/include/linux/dpll.h
> new file mode 100644
> index 000000000000..9051337bcf9e
> --- /dev/null
> +++ b/include/linux/dpll.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
> + */
> +
> +#ifndef __DPLL_H__
> +#define __DPLL_H__
> +
> +struct dpll_device;
> +
> +struct dpll_device_ops {
> +	int (*get_status)(struct dpll_device *dpll);
> +	int (*get_temp)(struct dpll_device *dpll);
> +	int (*get_lock_status)(struct dpll_device *dpll);
> +	int (*get_source_type)(struct dpll_device *dpll, int id);
> +	int (*get_output_type)(struct dpll_device *dpll, int id);
> +	int (*set_source_type)(struct dpll_device *dpll, int id, int val);
> +	int (*set_output_type)(struct dpll_device *dpll, int id, int val);
> +};
> +
> +struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
> +					 int outputs_count, void *priv);
> +void dpll_device_register(struct dpll_device *dpll);
> +void *dpll_priv(struct dpll_device *dpll);
> +#endif
> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
> new file mode 100644
> index 000000000000..8c00f52736ee
> --- /dev/null
> +++ b/include/uapi/linux/dpll.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_DPLL_H
> +#define _UAPI_LINUX_DPLL_H
> +
> +#define DPLL_NAME_LENGTH	20
> +
> +/* Adding event notification support elements */
> +#define DPLL_FAMILY_NAME		"dpll"
> +#define DPLL_VERSION			0x01
> +#define DPLL_CONFIG_DEVICE_GROUP_NAME  "config"
> +#define DPLL_CONFIG_SOURCE_GROUP_NAME  "source"
> +#define DPLL_CONFIG_OUTPUT_GROUP_NAME  "output"
> +#define DPLL_MONITOR_GROUP_NAME        "monitor"
> +
> +#define DPLL_FLAG_SOURCES	1
> +#define DPLL_FLAG_OUTPUTS	2
> +#define DPLL_FLAG_STATUS	4
> +
> +/* Attributes of dpll_genl_family */
> +enum dpll_genl_get_attr {
> +	DPLLA_UNSPEC,
> +	DPLLA_DEVICE,
> +	DPLLA_DEVICE_ID,
> +	DPLLA_DEVICE_NAME,
> +	DPLLA_SOURCE,
> +	DPLLA_SOURCE_ID,
> +	DPLLA_SOURCE_TYPE,
> +	DPLLA_OUTPUT,
> +	DPLLA_OUTPUT_ID,
> +	DPLLA_OUTPUT_TYPE,
> +	DPLLA_STATUS,
> +	DPLLA_TEMP,
> +	DPLLA_LOCK_STATUS,
> +	DPLLA_FLAGS,
> +
> +	__DPLLA_MAX,
> +};
> +#define DPLLA_GET_MAX (__DPLLA_MAX - 1)

I think "_get_/_GET_" in above names is outdated?

> +
> +/* DPLL signal types used as source or as output */
> +enum dpll_genl_signal_type {
> +	DPLL_TYPE_EXT_1PPS,
> +	DPLL_TYPE_EXT_10MHZ,
> +	DPLL_TYPE_SYNCE_ETH_PORT,
> +	DPLL_TYPE_INT_OSCILLATOR,
> +	DPLL_TYPE_GNSS,
> +
> +	__DPLL_TYPE_MAX,
> +};
> +#define DPLL_TYPE_MAX (__DPLL_TYPE_MAX - 1)
> +
> +/* Events of dpll_genl_family */
> +enum dpll_genl_event {
> +	DPLL_EVENT_UNSPEC,
> +	DPLL_EVENT_DEVICE_CREATE,		/* DPLL device creation */
> +	DPLL_EVENT_DEVICE_DELETE,		/* DPLL device deletion */
> +	DPLL_EVENT_STATUS_LOCKED,		/* DPLL device locked to source */
> +	DPLL_EVENT_STATUS_UNLOCKED,	/* DPLL device freerun */
> +	DPLL_EVENT_SOURCE_CHANGE,		/* DPLL device source changed */
> +	DPLL_EVENT_OUTPUT_CHANGE,		/* DPLL device output changed */
> +
> +	__DPLL_EVENT_MAX,
> +};
> +#define DPLL_EVENT_MAX (__DPLL_EVENT_MAX - 1)
> +
> +/* Commands supported by the dpll_genl_family */
> +enum dpll_genl_cmd {
> +	DPLL_CMD_UNSPEC,
> +	DPLL_CMD_DEVICE_GET,	/* List of DPLL devices id */
> +	DPLL_CMD_SET_SOURCE_TYPE,	/* Set the DPLL device source type */
> +	DPLL_CMD_SET_OUTPUT_TYPE,	/* Get the DPLL device output type */

"Get" in comment description looks like a typo.
I am getting bit confused with the name and comments.
For me, first look says: it is selection of a type of a source.
But in the code I can see it selects a source id and a type.
Type of source originates in HW design, why would the one want to "set" it?
I can imagine a HW design where a single source or output would allow to choose
where the signal originates/goes, some kind of extra selector layer for a
source/output, but was that the intention?
If so, shouldn't the user get some bitmap/list of modes available for each
source/output?

The user shall get some extra information about the source/output. Right now
there can be multiple sources/outputs of the same type, but not really possible
to find out their purpose. I.e. a dpll equipped with four source of 
DPLL_TYPE_EXT_1PPS type.

This implementation looks like designed for a "forced reference lock" mode
where the user must explicitly select one source. But a multi source/output
DPLL could be running in different modes. I believe most important is automatic
mode, where it tries to lock to a user-configured source priority list.
However, there is also freerun mode, where dpll isn't even trying to lock to
anything, or NCO - Numerically Controlled Oscillator mode. 
It would be great to have ability to select DPLL modes, but also to be able to
configure priorities, read failure status, configure extra "features" (i.e.
Embedded Sync, EEC modes, Fast Lock).

The sources and outputs can also have some extra features or capabilities, like:
- enable Embedded Sync
- add phase delay
- configure frequency (user might need to use source/output with different
  frequency then 1 PPS or 10MHz)

Generally, for simple DPLL designs this interface could do the job (although,
I still think user needs more information about the sources/outputs), but for
more complex ones, there should be something different, which takes care of my
comments regarding extra configuration needed.

Thanks,
Arkadiusz

> +
> +	__DPLL_CMD_MAX,
> +};
> +#define DPLL_CMD_MAX (__DPLL_CMD_MAX - 1)
> +
> +#endif /* _UAPI_LINUX_DPLL_H */
> -- 
> 2.27.0
> 
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

[Vadim Fedorenko]

Hi Arkadiusz!

On 23.06.2022 16:33, Kubalewski, Arkadiusz wrote:
> Hi Vadim,
> 
> Great work!
> 
> Although, I've been thinking that you already forget about it, so I have
> started development of something similar.
>

Sorry for making you wait for so long. I'm happy to merge your work into these 
series and to continue collaboration to further improve subsystem.

>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# Generic DPLL drivers configuration
>> +#
>> +
>> +config DPLL
>> +  bool
> 
> for RFC help and default were ommited?
> 

In private conversation with Jakub we decided to hide this subsystem from user
facing menu and enable via in-kernel customer's dependency. If you think it's 
better to let users enable or disable it, we can bring this discussion back to 
wider audience.

>> diff --git a/drivers/dpll/Makefile b/drivers/dpll/Makefile
>> new file mode 100644
>> index 000000000000..0748c80097e4
>> --- /dev/null
>> +++ b/drivers/dpll/Makefile
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# Makefile for DPLL drivers.
>> +#
>> +
>> +obj-$(CONFIG_DPLL)          += dpll_sys.o
>> +dpll_sys-y                  += dpll_core.o dpll_netlink.o
>> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>> new file mode 100644
>> index 000000000000..e34767e723cf
>> --- /dev/null
>> +++ b/drivers/dpll/dpll_core.c
>> @@ -0,0 +1,152 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + *  dpll_core.c - Generic DPLL Management class support.
>> + *
>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +#include <linux/string.h>
>> +
>> +#include "dpll_core.h"
>> +
>> +static DEFINE_MUTEX(dpll_device_xa_lock);
>> +static DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC);
>> +#define DPLL_REGISTERED XA_MARK_1
>> +
>> +#define ASSERT_DPLL_REGISTERED(d)                                           \
>> +	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>> +#define ASSERT_DPLL_NOT_REGISTERED(d)                                      \
>> +	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>> +
>> +
>> +int for_each_dpll_device(int id, int (*cb)(struct dpll_device *, void *), void *data)
>> +{
>> +	struct dpll_device *dpll;
>> +	unsigned long index;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&dpll_device_xa_lock);
>> +	xa_for_each_start(&dpll_device_xa, index, dpll, id) {
>> +		if (!xa_get_mark(&dpll_device_xa, index, DPLL_REGISTERED))
>> +			continue;
>> +		ret = cb(dpll, data);
>> +		if (ret)
>> +			break;
>> +	}
>> +	mutex_unlock(&dpll_device_xa_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +struct dpll_device *dpll_device_get_by_id(int id)
>> +{
>> +	struct dpll_device *dpll = NULL;
>> +
>> +	if (xa_get_mark(&dpll_device_xa, id, DPLL_REGISTERED))
>> +		dpll = xa_load(&dpll_device_xa, id);
>> +	return dpll;
>> +}
>> +
>> +void *dpll_priv(struct dpll_device *dpll)
>> +{
>> +	return dpll->priv;
>> +}
>> +EXPORT_SYMBOL_GPL(dpll_priv);
>> +
>> +static void dpll_device_release(struct device *dev)
>> +{
>> +	struct dpll_device *dpll;
>> +
>> +	dpll = to_dpll_device(dev);
>> +
>> +	dpll_device_unregister(dpll);
>> +
>> +	mutex_destroy(&dpll->lock);
>> +	kfree(dpll);
>> +}
>> +
>> +static struct class dpll_class = {
>> +	.name = "dpll",
>> +	.dev_release = dpll_device_release,
>> +};
>> +
>> +struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
>> +					 int outputs_count, void *priv)
>> +{
>> +	struct dpll_device *dpll;
>> +	int ret;
>> +
>> +	dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
>> +	if (!dpll)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	mutex_init(&dpll->lock);
>> +	dpll->ops = ops;
>> +	dpll->dev.class = &dpll_class;
>> +	dpll->sources_count = sources_count;
>> +	dpll->outputs_count = outputs_count;
>> +
>> +	mutex_lock(&dpll_device_xa_lock);
>> +	ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, GFP_KERNEL);
>> +	if (ret)
>> +		goto error;
>> +	dev_set_name(&dpll->dev, "dpll%d", dpll->id);
>> +	mutex_unlock(&dpll_device_xa_lock);
>> +	dpll->priv = priv;
>> +
>> +	return dpll;
>> +
>> +error:
>> +	mutex_unlock(&dpll_device_xa_lock);
>> +	kfree(dpll);
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(dpll_device_alloc);
>> +
>> +void dpll_device_register(struct dpll_device *dpll)
>> +{
>> +	ASSERT_DPLL_NOT_REGISTERED(dpll);
>> +
>> +	mutex_lock(&dpll_device_xa_lock);
>> +	xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);
>> +	dpll_notify_device_create(dpll->id, dev_name(&dpll->dev));
>> +	mutex_unlock(&dpll_device_xa_lock);
>> +}
>> +EXPORT_SYMBOL_GPL(dpll_device_register);
>> +
>> +void dpll_device_unregister(struct dpll_device *dpll)
>> +{
>> +	ASSERT_DPLL_REGISTERED(dpll);
>> +
>> +	mutex_lock(&dpll_device_xa_lock);
>> +	xa_erase(&dpll_device_xa, dpll->id);
>> +	mutex_unlock(&dpll_device_xa_lock);
>> +}
>> +EXPORT_SYMBOL_GPL(dpll_device_unregister);
>> +
>> +static int __init dpll_init(void)
>> +{
>> +	int ret;
>> +
>> +	ret = dpll_netlink_init();
>> +	if (ret)
>> +		goto error;
>> +
>> +	ret = class_register(&dpll_class);
>> +	if (ret)
>> +		goto unregister_netlink;
>> +
>> +	return 0;
>> +
>> +unregister_netlink:
>> +	dpll_netlink_finish();
>> +error:
>> +	mutex_destroy(&dpll_device_xa_lock);
>> +	return ret;
>> +}
>> +subsys_initcall(dpll_init);
>> diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>> new file mode 100644
>> index 000000000000..5ad3224d5caf
>> --- /dev/null
>> +++ b/drivers/dpll/dpll_core.h
>> @@ -0,0 +1,40 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>> + */
>> +
>> +#ifndef __DPLL_CORE_H__
>> +#define __DPLL_CORE_H__
>> +
>> +#include <linux/dpll.h>
>> +
>> +#include "dpll_netlink.h"
>> +
>> +/**
>> + * struct dpll_device - structure for a DPLL device
>> + * @id:		unique id number for each edvice
>> + * @dev:	&struct device for this dpll device
>> + * @sources_count:	amount of input sources this dpll_device supports
>> + * @outputs_count:	amount of outputs this dpll_device supports
>> + * @ops:	operations this &dpll_device supports
>> + * @lock:	mutex to serialize operations
>> + * @priv:	pointer to private information of owner
>> + */
>> +struct dpll_device {
>> +	int id;
>> +	struct device dev;
>> +	int sources_count;
>> +	int outputs_count;
>> +	struct dpll_device_ops *ops;
>> +	struct mutex lock;
>> +	void *priv;
>> +};
>> +
>> +#define to_dpll_device(_dev) \
>> +	container_of(_dev, struct dpll_device, dev)
>> +
>> +int for_each_dpll_device(int id, int (*cb)(struct dpll_device *, void *),
>> +			  void *data);
>> +struct dpll_device *dpll_device_get_by_id(int id);
>> +void dpll_device_unregister(struct dpll_device *dpll);
>> +#endif
>> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>> new file mode 100644
>> index 000000000000..0bbdaa6dde8e
>> --- /dev/null
>> +++ b/drivers/dpll/dpll_netlink.c
>> @@ -0,0 +1,437 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Generic netlink for DPLL management framework
>> + *
>> + * Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>> + *
>> + */
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <net/genetlink.h>
>> +#include "dpll_core.h"
>> +
>> +#include <uapi/linux/dpll.h>
>> +
>> +static const struct genl_multicast_group dpll_genl_mcgrps[] = {
>> +	{ .name = DPLL_CONFIG_DEVICE_GROUP_NAME, },
>> +	{ .name = DPLL_CONFIG_SOURCE_GROUP_NAME, },
>> +	{ .name = DPLL_CONFIG_OUTPUT_GROUP_NAME, },
>> +	{ .name = DPLL_MONITOR_GROUP_NAME,  },
>> +};
>> +
>> +static const struct nla_policy dpll_genl_get_policy[] = {
>> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
>> +	[DPLLA_DEVICE_NAME]	= { .type = NLA_STRING,
>> +				    .len = DPLL_NAME_LENGTH },
>> +	[DPLLA_FLAGS]		= { .type = NLA_U32 },
>> +};
>> +
>> +static const struct nla_policy dpll_genl_set_source_policy[] = {
>> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
>> +	[DPLLA_SOURCE_ID]	= { .type = NLA_U32 },
>> +	[DPLLA_SOURCE_TYPE]	= { .type = NLA_U32 },
>> +};
>> +
>> +static const struct nla_policy dpll_genl_set_output_policy[] = {
>> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
>> +	[DPLLA_OUTPUT_ID]	= { .type = NLA_U32 },
>> +	[DPLLA_OUTPUT_TYPE]	= { .type = NLA_U32 },
>> +};
>> +
>> +struct param {
>> +	struct netlink_callback *cb;
>> +	struct dpll_device *dpll;
>> +	struct nlattr **attrs;
>> +	struct sk_buff *msg;
>> +	int dpll_id;
>> +	int dpll_source_id;
>> +	int dpll_source_type;
>> +	int dpll_output_id;
>> +	int dpll_output_type;
>> +};
>> +
>> +struct dpll_dump_ctx {
>> +	struct dpll_device *dev;
>> +	int flags;
>> +	int pos_idx;
>> +	int pos_src_idx;
>> +	int pos_out_idx;
>> +};
>> +
>> +typedef int (*cb_t)(struct param *);
>> +
>> +static struct genl_family dpll_gnl_family;
>> +
>> +static struct dpll_dump_ctx *dpll_dump_context(struct netlink_callback *cb)
>> +{
>> +	return (struct dpll_dump_ctx *)cb->ctx;
>> +}
>> +
>> +static int __dpll_cmd_device_dump_one(struct dpll_device *dpll,
>> +					   struct sk_buff *msg)
>> +{
>> +	if (nla_put_u32(msg, DPLLA_DEVICE_ID, dpll->id))
>> +		return -EMSGSIZE;
>> +
>> +	if (nla_put_string(msg, DPLLA_DEVICE_NAME, dev_name(&dpll->dev)))
>> +		return -EMSGSIZE;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __dpll_cmd_dump_sources(struct dpll_device *dpll,
>> +					   struct sk_buff *msg)
>> +{
>> +	struct nlattr *src_attr;
>> +	int i, ret = 0, type;
>> +
>> +	for (i = 0; i < dpll->sources_count; i++) {
>> +		src_attr = nla_nest_start(msg, DPLLA_SOURCE);
>> +		if (!src_attr) {
>> +			ret = -EMSGSIZE;
>> +			break;
>> +		}
>> +		type = dpll->ops->get_source_type(dpll, i);
>> +		if (nla_put_u32(msg, DPLLA_SOURCE_ID, i) ||
>> +		    nla_put_u32(msg, DPLLA_SOURCE_TYPE, type)) {
>> +			nla_nest_cancel(msg, src_attr);
>> +			ret = -EMSGSIZE;
>> +			break;
>> +		}
>> +		nla_nest_end(msg, src_attr);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int __dpll_cmd_dump_outputs(struct dpll_device *dpll,
>> +					   struct sk_buff *msg)
>> +{
>> +	struct nlattr *out_attr;
>> +	int i, ret = 0, type;
>> +
>> +	for (i = 0; i < dpll->outputs_count; i++) {
>> +		out_attr = nla_nest_start(msg, DPLLA_OUTPUT);
>> +		if (!out_attr) {
>> +			ret = -EMSGSIZE;
>> +			break;
>> +		}
>> +		type = dpll->ops->get_source_type(dpll, i);
>> +		if (nla_put_u32(msg, DPLLA_OUTPUT_ID, i) ||
>> +		    nla_put_u32(msg, DPLLA_OUTPUT_TYPE, type)) {
>> +			nla_nest_cancel(msg, out_attr);
>> +			ret = -EMSGSIZE;
>> +			break;
>> +		}
>> +		nla_nest_end(msg, out_attr);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int __dpll_cmd_dump_status(struct dpll_device *dpll,
>> +					   struct sk_buff *msg)
>> +{
>> +	int ret;
>> +
>> +	if (!dpll->ops->get_status && !dpll->ops->get_temp && !dpll->ops->get_lock_status)
>> +		return 0;
> 
> what if dpll doesn't support one of those commands?
> 

then only supported attributes will be messaged back to user

>> +
>> +	if (dpll->ops->get_status) {
>> +		ret = dpll->ops->get_status(dpll);
>> +		if (nla_put_u32(msg, DPLLA_STATUS, ret))
>> +			return -EMSGSIZE;
>> +	}
>> +
>> +	if (dpll->ops->get_temp) {
>> +		ret = dpll->ops->get_status(dpll);
>> +		if (nla_put_u32(msg, DPLLA_TEMP, ret))
>> +			return -EMSGSIZE;
>> +	}
> 
> shouldn't be get_temp(dpll)?

good catch, copy-paste error

>> +
>> +	if (dpll->ops->get_lock_status) {
>> +		ret = dpll->ops->get_lock_status(dpll);
>> +		if (nla_put_u32(msg, DPLLA_LOCK_STATUS, ret))
>> +			return -EMSGSIZE;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int dpll_device_dump_one(struct dpll_device *dev, struct sk_buff *msg, int flags)
>> +{
>> +	struct nlattr *hdr;
>> +	int ret;
>> +
>> +	hdr = nla_nest_start(msg, DPLLA_DEVICE);
>> +	if (!hdr)
>> +		return -EMSGSIZE;
>> +
>> +	mutex_lock(&dev->lock);
>> +	ret = __dpll_cmd_device_dump_one(dev, msg);
>> +	if (ret)
>> +		goto out_cancel_nest;
>> +
>> +	if (flags & DPLL_FLAG_SOURCES && dev->ops->get_source_type) {
>> +		ret = __dpll_cmd_dump_sources(dev, msg);
>> +		if (ret)
>> +			goto out_cancel_nest;
>> +	}
>> +
>> +	if (flags & DPLL_FLAG_OUTPUTS && dev->ops->get_output_type) {
>> +		ret = __dpll_cmd_dump_outputs(dev, msg);
>> +		if (ret)
>> +			goto out_cancel_nest;
>> +	}
>> +
>> +	if (flags & DPLL_FLAG_STATUS) {
>> +		ret = __dpll_cmd_dump_status(dev, msg);
>> +		if (ret)
>> +			goto out_cancel_nest;
>> +	}
>> +
>> +	mutex_unlock(&dev->lock);
>> +	nla_nest_end(msg, hdr);
>> +
>> +	return 0;
>> +
>> +out_cancel_nest:
>> +	mutex_unlock(&dev->lock);
>> +	nla_nest_cancel(msg, hdr);
>> +
>> +	return ret;
>> +}
>> +
>> +static int dpll_genl_cmd_set_source(struct param *p)
>> +{
>> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
>> +	struct dpll_device *dpll = p->dpll;
>> +	int ret = 0, src_id, type;
>> +
>> +	if (!info->attrs[DPLLA_SOURCE_ID] ||
>> +	    !info->attrs[DPLLA_SOURCE_TYPE])
>> +		return -EINVAL;
>> +
>> +	if (!dpll->ops->set_source_type)
>> +		return -EOPNOTSUPP;
>> +
>> +	src_id = nla_get_u32(info->attrs[DPLLA_SOURCE_ID]);
>> +	type = nla_get_u32(info->attrs[DPLLA_SOURCE_TYPE]);
>> +
>> +	mutex_lock(&dpll->lock);
>> +	ret = dpll->ops->set_source_type(dpll, src_id, type);
>> +	mutex_unlock(&dpll->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int dpll_genl_cmd_set_output(struct param *p)
>> +{
>> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
>> +	struct dpll_device *dpll = p->dpll;
>> +	int ret = 0, out_id, type;
>> +
>> +	if (!info->attrs[DPLLA_OUTPUT_ID] ||
>> +	    !info->attrs[DPLLA_OUTPUT_TYPE])
>> +		return -EINVAL;
>> +
>> +	if (!dpll->ops->set_output_type)
>> +		return -EOPNOTSUPP;
>> +
>> +	out_id = nla_get_u32(info->attrs[DPLLA_OUTPUT_ID]);
>> +	type = nla_get_u32(info->attrs[DPLLA_OUTPUT_TYPE]);
>> +
>> +	mutex_lock(&dpll->lock);
>> +	ret = dpll->ops->set_source_type(dpll, out_id, type);
>> +	mutex_unlock(&dpll->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int dpll_device_loop_cb(struct dpll_device *dpll, void *data)
>> +{
>> +	struct dpll_dump_ctx *ctx;
>> +	struct param *p = (struct param *)data;
>> +
>> +	ctx = dpll_dump_context(p->cb);
>> +
>> +	ctx->pos_idx = dpll->id;
>> +
>> +	return dpll_device_dump_one(dpll, p->msg, ctx->flags);
>> +}
>> +
>> +static int dpll_cmd_device_dump(struct param *p)
>> +{
>> +	struct dpll_dump_ctx *ctx = dpll_dump_context(p->cb);
>> +
>> +	return for_each_dpll_device(ctx->pos_idx, dpll_device_loop_cb, p);
>> +}
>> +
>> +static int dpll_genl_cmd_device_get_id(struct param *p)
>> +{
>> +	struct dpll_device *dpll = p->dpll;
>> +	int flags = 0;
>> +
>> +	if (p->attrs[DPLLA_FLAGS])
>> +		flags = nla_get_u32(p->attrs[DPLLA_FLAGS]);
>> +
>> +	return dpll_device_dump_one(dpll, p->msg, flags);
>> +}
>> +
>> +static cb_t cmd_doit_cb[] = {
>> +	[DPLL_CMD_DEVICE_GET]		= dpll_genl_cmd_device_get_id,
>> +	[DPLL_CMD_SET_SOURCE_TYPE]	= dpll_genl_cmd_set_source,
>> +	[DPLL_CMD_SET_OUTPUT_TYPE]	= dpll_genl_cmd_set_output,
>> +};
>> +
>> +static cb_t cmd_dump_cb[] = {
>> +	[DPLL_CMD_DEVICE_GET]		= dpll_cmd_device_dump,
>> +};
>> +
>> +static int dpll_genl_cmd_start(struct netlink_callback *cb)
>> +{
>> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>> +	struct dpll_dump_ctx *ctx = dpll_dump_context(cb);
>> +
>> +	ctx->dev = NULL;
>> +	if (info->attrs[DPLLA_FLAGS])
>> +		ctx->flags = nla_get_u32(info->attrs[DPLLA_FLAGS]);
>> +	else
>> +		ctx->flags = 0;
>> +	ctx->pos_idx = 0;
>> +	ctx->pos_src_idx = 0;
>> +	ctx->pos_out_idx = 0;
>> +	return 0;
>> +}
>> +
>> +static int dpll_genl_cmd_dumpit(struct sk_buff *skb,
>> +				   struct netlink_callback *cb)
>> +{
>> +	struct param p = { .cb = cb, .msg = skb };
>> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>> +	int cmd = info->op.cmd;
>> +	int ret;
>> +	void *hdr;
>> +
>> +	hdr = genlmsg_put(skb, 0, 0, &dpll_gnl_family, 0, cmd);
>> +	if (!hdr)
>> +		return -EMSGSIZE;
>> +
>> +	ret = cmd_dump_cb[cmd](&p);
>> +	if (ret)
>> +		goto out_cancel_msg;
>> +
>> +	genlmsg_end(skb, hdr);
>> +
>> +	return 0;
>> +
>> +out_cancel_msg:
>> +	genlmsg_cancel(skb, hdr);
>> +
>> +	return ret;
>> +}
>> +
>> +static int dpll_genl_cmd_doit(struct sk_buff *skb,
>> +				 struct genl_info *info)
>> +{
>> +	struct param p = { .attrs = info->attrs, .dpll = info->user_ptr[0] };
>> +	int cmd = info->genlhdr->cmd;
>> +	struct sk_buff *msg;
>> +	void *hdr;
>> +	int ret;
>> +
>> +	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>> +	if (!msg)
>> +		return -ENOMEM;
>> +	p.msg = msg;
>> +
>> +	hdr = genlmsg_put_reply(msg, info, &dpll_gnl_family, 0, cmd);
>> +	if (!hdr) {
>> +		ret = -EMSGSIZE;
>> +		goto out_free_msg;
>> +	}
>> +
>> +	ret = cmd_doit_cb[cmd](&p);
>> +	if (ret)
>> +		goto out_cancel_msg;
>> +
>> +	genlmsg_end(msg, hdr);
>> +
>> +	return genlmsg_reply(msg, info);
>> +
>> +out_cancel_msg:
>> +	genlmsg_cancel(msg, hdr);
>> +out_free_msg:
>> +	nlmsg_free(msg);
>> +
>> +	return ret;
>> +}
>> +
>> +static int dpll_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
>> +						 struct genl_info *info)
>> +{
>> +	struct dpll_device *dpll;
>> +	int id;
>> +
>> +	if (!info->attrs[DPLLA_DEVICE_ID])
>> +		return -EINVAL;
>> +	id = nla_get_u32(info->attrs[DPLLA_DEVICE_ID]);
>> +
>> +	dpll = dpll_device_get_by_id(id);
>> +	if (!dpll)
>> +		return -ENODEV;
>> +	info->user_ptr[0] = dpll;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct genl_ops dpll_genl_ops[] = {
>> +	{
>> +		.cmd	= DPLL_CMD_DEVICE_GET,
>> +		.start	= dpll_genl_cmd_start,
>> +		.dumpit	= dpll_genl_cmd_dumpit,
>> +		.doit	= dpll_genl_cmd_doit,
>> +		.policy	= dpll_genl_get_policy,
>> +		.maxattr = ARRAY_SIZE(dpll_genl_get_policy) - 1,
>> +	},
>> +	{
>> +		.cmd	= DPLL_CMD_SET_SOURCE_TYPE,
>> +		.flags	= GENL_UNS_ADMIN_PERM,
>> +		.doit	= dpll_genl_cmd_doit,
>> +		.policy	= dpll_genl_set_source_policy,
>> +		.maxattr = ARRAY_SIZE(dpll_genl_set_source_policy) - 1,
>> +	},
>> +	{
>> +		.cmd	= DPLL_CMD_SET_OUTPUT_TYPE,
>> +		.flags	= GENL_UNS_ADMIN_PERM,
>> +		.doit	= dpll_genl_cmd_doit,
>> +		.policy	= dpll_genl_set_output_policy,
>> +		.maxattr = ARRAY_SIZE(dpll_genl_set_output_policy) - 1,
>> +	},
>> +};
>> +
>> +static struct genl_family dpll_gnl_family __ro_after_init = {
>> +	.hdrsize	= 0,
>> +	.name		= DPLL_FAMILY_NAME,
>> +	.version	= DPLL_VERSION,
>> +	.ops		= dpll_genl_ops,
>> +	.n_ops		= ARRAY_SIZE(dpll_genl_ops),
>> +	.mcgrps		= dpll_genl_mcgrps,
>> +	.n_mcgrps	= ARRAY_SIZE(dpll_genl_mcgrps),
>> +	.pre_doit	= dpll_pre_doit,
>> +};
>> +
>> +int __init dpll_netlink_init(void)
>> +{
>> +	return genl_register_family(&dpll_gnl_family);
>> +}
>> +
>> +void dpll_netlink_finish(void)
>> +{
>> +	genl_unregister_family(&dpll_gnl_family);
>> +}
>> +
>> +void __exit dpll_netlink_fini(void)
>> +{
>> +	dpll_netlink_finish();
>> +}
>> diff --git a/drivers/dpll/dpll_netlink.h b/drivers/dpll/dpll_netlink.h
>> new file mode 100644
>> index 000000000000..e2d100f59dd6
>> --- /dev/null
>> +++ b/drivers/dpll/dpll_netlink.h
>> @@ -0,0 +1,7 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>> + */
>> +
>> +int __init dpll_netlink_init(void);
>> +void dpll_netlink_finish(void);
>> diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>> new file mode 100644
>> index 000000000000..9051337bcf9e
>> --- /dev/null
>> +++ b/include/linux/dpll.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>> + */
>> +
>> +#ifndef __DPLL_H__
>> +#define __DPLL_H__
>> +
>> +struct dpll_device;
>> +
>> +struct dpll_device_ops {
>> +	int (*get_status)(struct dpll_device *dpll);
>> +	int (*get_temp)(struct dpll_device *dpll);
>> +	int (*get_lock_status)(struct dpll_device *dpll);
>> +	int (*get_source_type)(struct dpll_device *dpll, int id);
>> +	int (*get_output_type)(struct dpll_device *dpll, int id);
>> +	int (*set_source_type)(struct dpll_device *dpll, int id, int val);
>> +	int (*set_output_type)(struct dpll_device *dpll, int id, int val);
>> +};
>> +
>> +struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
>> +					 int outputs_count, void *priv);
>> +void dpll_device_register(struct dpll_device *dpll);
>> +void *dpll_priv(struct dpll_device *dpll);
>> +#endif
>> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>> new file mode 100644
>> index 000000000000..8c00f52736ee
>> --- /dev/null
>> +++ b/include/uapi/linux/dpll.h
>> @@ -0,0 +1,77 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +#ifndef _UAPI_LINUX_DPLL_H
>> +#define _UAPI_LINUX_DPLL_H
>> +
>> +#define DPLL_NAME_LENGTH	20
>> +
>> +/* Adding event notification support elements */
>> +#define DPLL_FAMILY_NAME		"dpll"
>> +#define DPLL_VERSION			0x01
>> +#define DPLL_CONFIG_DEVICE_GROUP_NAME  "config"
>> +#define DPLL_CONFIG_SOURCE_GROUP_NAME  "source"
>> +#define DPLL_CONFIG_OUTPUT_GROUP_NAME  "output"
>> +#define DPLL_MONITOR_GROUP_NAME        "monitor"
>> +
>> +#define DPLL_FLAG_SOURCES	1
>> +#define DPLL_FLAG_OUTPUTS	2
>> +#define DPLL_FLAG_STATUS	4
>> +
>> +/* Attributes of dpll_genl_family */
>> +enum dpll_genl_get_attr {
>> +	DPLLA_UNSPEC,
>> +	DPLLA_DEVICE,
>> +	DPLLA_DEVICE_ID,
>> +	DPLLA_DEVICE_NAME,
>> +	DPLLA_SOURCE,
>> +	DPLLA_SOURCE_ID,
>> +	DPLLA_SOURCE_TYPE,
>> +	DPLLA_OUTPUT,
>> +	DPLLA_OUTPUT_ID,
>> +	DPLLA_OUTPUT_TYPE,
>> +	DPLLA_STATUS,
>> +	DPLLA_TEMP,
>> +	DPLLA_LOCK_STATUS,
>> +	DPLLA_FLAGS,
>> +
>> +	__DPLLA_MAX,
>> +};
>> +#define DPLLA_GET_MAX (__DPLLA_MAX - 1)
> 
> I think "_get_/_GET_" in above names is outdated?
> 

Yes, you are right. The earliest revision had these "GET/SET" in attributes but 
later we decided to unite them into common attributes. I will remove these 
artifacts in the next revision.

>> +
>> +/* DPLL signal types used as source or as output */
>> +enum dpll_genl_signal_type {
>> +	DPLL_TYPE_EXT_1PPS,
>> +	DPLL_TYPE_EXT_10MHZ,
>> +	DPLL_TYPE_SYNCE_ETH_PORT,
>> +	DPLL_TYPE_INT_OSCILLATOR,
>> +	DPLL_TYPE_GNSS,
>> +
>> +	__DPLL_TYPE_MAX,
>> +};
>> +#define DPLL_TYPE_MAX (__DPLL_TYPE_MAX - 1)
>> +
>> +/* Events of dpll_genl_family */
>> +enum dpll_genl_event {
>> +	DPLL_EVENT_UNSPEC,
>> +	DPLL_EVENT_DEVICE_CREATE,		/* DPLL device creation */
>> +	DPLL_EVENT_DEVICE_DELETE,		/* DPLL device deletion */
>> +	DPLL_EVENT_STATUS_LOCKED,		/* DPLL device locked to source */
>> +	DPLL_EVENT_STATUS_UNLOCKED,	/* DPLL device freerun */
>> +	DPLL_EVENT_SOURCE_CHANGE,		/* DPLL device source changed */
>> +	DPLL_EVENT_OUTPUT_CHANGE,		/* DPLL device output changed */
>> +
>> +	__DPLL_EVENT_MAX,
>> +};
>> +#define DPLL_EVENT_MAX (__DPLL_EVENT_MAX - 1)
>> +
>> +/* Commands supported by the dpll_genl_family */
>> +enum dpll_genl_cmd {
>> +	DPLL_CMD_UNSPEC,
>> +	DPLL_CMD_DEVICE_GET,	/* List of DPLL devices id */
>> +	DPLL_CMD_SET_SOURCE_TYPE,	/* Set the DPLL device source type */
>> +	DPLL_CMD_SET_OUTPUT_TYPE,	/* Get the DPLL device output type */
> 
> "Get" in comment description looks like a typo.
> I am getting bit confused with the name and comments.
> For me, first look says: it is selection of a type of a source.
> But in the code I can see it selects a source id and a type.
> Type of source originates in HW design, why would the one want to "set" it?
> I can imagine a HW design where a single source or output would allow to choose
> where the signal originates/goes, some kind of extra selector layer for a
> source/output, but was that the intention?

In general - yes, we have experimented with our time card providing different 
types of source synchronisation signal on different input pins, i.e. 1PPS, 
10MHz, IRIG-B, etc. Any of these signals could be connected to any of 4 external 
pins, that's why I source id is treated as input pin identifier and source type 
is the signal type it receives.

> If so, shouldn't the user get some bitmap/list of modes available for each
> source/output?

Good idea. We have list of available modes exposed via sysfs file, and I agree 
that it's worth to expose them via netlink interface. I will try to address this 
in the next version.

> 
> The user shall get some extra information about the source/output. Right now
> there can be multiple sources/outputs of the same type, but not really possible
> to find out their purpose. I.e. a dpll equipped with four source of
> DPLL_TYPE_EXT_1PPS type.
>  > This implementation looks like designed for a "forced reference lock" mode
> where the user must explicitly select one source. But a multi source/output
> DPLL could be running in different modes. I believe most important is automatic
> mode, where it tries to lock to a user-configured source priority list.
> However, there is also freerun mode, where dpll isn't even trying to lock to
> anything, or NCO - Numerically Controlled Oscillator mode.

Yes, you are right, my focus was on "forced reference lock" mode as currently 
this is the only mode supported by our hardware. But I'm happy to extend it to 
other usecases.

> It would be great to have ability to select DPLL modes, but also to be able to
> configure priorities, read failure status, configure extra "features" (i.e.
> Embedded Sync, EEC modes, Fast Lock)
I absolutely agree on this way of improvement, and I already have some ongoing 
work about failures/events/status change messages. I can see no stoppers for 
creating priorities (if supported by HW) and other extra "features", but we have 
to agree on the interface with flexibility in mind.

> The sources and outputs can also have some extra features or capabilities, like:
> - enable Embedded Sync

Does it mean we want to enable or disable Embedded Sync within one protocol? Is 
it like Time-Sensitive Networking (TSN) for Ethernet?

> - add phase delay
> - configure frequency (user might need to use source/output with different
>    frequency then 1 PPS or 10MHz)

Providing these modes I was thinking about the most common and widely used 
signals in measurement equipment. So my point here is that both 1PPS and 10MHz 
should stay as is, but another type of signal should be added, let's say 
CUSTOM_FREQ, which will also consider special attribute in netlink terms. is it ok?

> Generally, for simple DPLL designs this interface could do the job (although,
> I still think user needs more information about the sources/outputs), but for
> more complex ones, there should be something different, which takes care of my
> comments regarding extra configuration needed.
> 

As I already mentioned earlier, I'm open for improvements and happy to 
collaborate to cover other use cases of this subsystem from the very beginning 
of development. We can even create an open github repo to share implementation 
details together with comments if it works better for you.

> Thanks,
> Arkadiusz
> 
>> +
>> +	__DPLL_CMD_MAX,
>> +};
>> +#define DPLL_CMD_MAX (__DPLL_CMD_MAX - 1)
>> +
>> +#endif /* _UAPI_LINUX_DPLL_H */
>> -- 
>> 2.27.0
>>
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

[Kubalewski, Arkadiusz]

-----Original Message-----
From: Vadim Fedorenko <vfedorenko@novek.ru> 
Sent: Friday, June 24, 2022 12:48 AM

>Hi Arkadiusz!
>
>On 23.06.2022 16:33, Kubalewski, Arkadiusz wrote:
>> Hi Vadim,
>> 
>> Great work!
>> 
>> Although, I've been thinking that you already forget about it, so I have
>> started development of something similar.
>>
>
>Sorry for making you wait for so long. I'm happy to merge your work into these 
>series and to continue collaboration to further improve subsystem.

Not a problem, sounds great!

>
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +#
>>> +# Generic DPLL drivers configuration
>>> +#
>>> +
>>> +config DPLL
>>> +  bool
>> 
>> for RFC help and default were ommited?
>> 
>
>In private conversation with Jakub we decided to hide this subsystem from user
>facing menu and enable via in-kernel customer's dependency. If you think it's 
>better to let users enable or disable it, we can bring this discussion back to 
>wider audience.

Well, I won't insist on it. Seems fair.

>
>>> diff --git a/drivers/dpll/Makefile b/drivers/dpll/Makefile
>>> new file mode 100644
>>> index 000000000000..0748c80097e4
>>> --- /dev/null
>>> +++ b/drivers/dpll/Makefile
>>> @@ -0,0 +1,7 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +#
>>> +# Makefile for DPLL drivers.
>>> +#
>>> +
>>> +obj-$(CONFIG_DPLL)          += dpll_sys.o
>>> +dpll_sys-y                  += dpll_core.o dpll_netlink.o
>>> diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>> new file mode 100644
>>> index 000000000000..e34767e723cf
>>> --- /dev/null
>>> +++ b/drivers/dpll/dpll_core.c
>>> @@ -0,0 +1,152 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + *  dpll_core.c - Generic DPLL Management class support.
>>> + *
>>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>>> + */
>>> +
>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/err.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/string.h>
>>> +
>>> +#include "dpll_core.h"
>>> +
>>> +static DEFINE_MUTEX(dpll_device_xa_lock);
>>> +static DEFINE_XARRAY_FLAGS(dpll_device_xa, XA_FLAGS_ALLOC);
>>> +#define DPLL_REGISTERED XA_MARK_1
>>> +
>>> +#define ASSERT_DPLL_REGISTERED(d)                                           \
>>> +	WARN_ON_ONCE(!xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>>> +#define ASSERT_DPLL_NOT_REGISTERED(d)                                      \
>>> +	WARN_ON_ONCE(xa_get_mark(&dpll_device_xa, (d)->id, DPLL_REGISTERED))
>>> +
>>> +
>>> +int for_each_dpll_device(int id, int (*cb)(struct dpll_device *, void *), void *data)
>>> +{
>>> +	struct dpll_device *dpll;
>>> +	unsigned long index;
>>> +	int ret = 0;
>>> +
>>> +	mutex_lock(&dpll_device_xa_lock);
>>> +	xa_for_each_start(&dpll_device_xa, index, dpll, id) {
>>> +		if (!xa_get_mark(&dpll_device_xa, index, DPLL_REGISTERED))
>>> +			continue;
>>> +		ret = cb(dpll, data);
>>> +		if (ret)
>>> +			break;
>>> +	}
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +struct dpll_device *dpll_device_get_by_id(int id)
>>> +{
>>> +	struct dpll_device *dpll = NULL;
>>> +
>>> +	if (xa_get_mark(&dpll_device_xa, id, DPLL_REGISTERED))
>>> +		dpll = xa_load(&dpll_device_xa, id);
>>> +	return dpll;
>>> +}
>>> +
>>> +void *dpll_priv(struct dpll_device *dpll)
>>> +{
>>> +	return dpll->priv;
>>> +}
>>> +EXPORT_SYMBOL_GPL(dpll_priv);
>>> +
>>> +static void dpll_device_release(struct device *dev)
>>> +{
>>> +	struct dpll_device *dpll;
>>> +
>>> +	dpll = to_dpll_device(dev);
>>> +
>>> +	dpll_device_unregister(dpll);
>>> +
>>> +	mutex_destroy(&dpll->lock);
>>> +	kfree(dpll);
>>> +}
>>> +
>>> +static struct class dpll_class = {
>>> +	.name = "dpll",
>>> +	.dev_release = dpll_device_release,
>>> +};
>>> +
>>> +struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
>>> +					 int outputs_count, void *priv)
>>> +{
>>> +	struct dpll_device *dpll;
>>> +	int ret;
>>> +
>>> +	dpll = kzalloc(sizeof(*dpll), GFP_KERNEL);
>>> +	if (!dpll)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	mutex_init(&dpll->lock);
>>> +	dpll->ops = ops;
>>> +	dpll->dev.class = &dpll_class;
>>> +	dpll->sources_count = sources_count;
>>> +	dpll->outputs_count = outputs_count;
>>> +
>>> +	mutex_lock(&dpll_device_xa_lock);
>>> +	ret = xa_alloc(&dpll_device_xa, &dpll->id, dpll, xa_limit_16b, GFP_KERNEL);
>>> +	if (ret)
>>> +		goto error;
>>> +	dev_set_name(&dpll->dev, "dpll%d", dpll->id);
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +	dpll->priv = priv;
>>> +
>>> +	return dpll;
>>> +
>>> +error:
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +	kfree(dpll);
>>> +	return ERR_PTR(ret);
>>> +}
>>> +EXPORT_SYMBOL_GPL(dpll_device_alloc);
>>> +
>>> +void dpll_device_register(struct dpll_device *dpll)
>>> +{
>>> +	ASSERT_DPLL_NOT_REGISTERED(dpll);
>>> +
>>> +	mutex_lock(&dpll_device_xa_lock);
>>> +	xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);
>>> +	dpll_notify_device_create(dpll->id, dev_name(&dpll->dev));
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(dpll_device_register);
>>> +
>>> +void dpll_device_unregister(struct dpll_device *dpll)
>>> +{
>>> +	ASSERT_DPLL_REGISTERED(dpll);
>>> +
>>> +	mutex_lock(&dpll_device_xa_lock);
>>> +	xa_erase(&dpll_device_xa, dpll->id);
>>> +	mutex_unlock(&dpll_device_xa_lock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(dpll_device_unregister);
>>> +
>>> +static int __init dpll_init(void)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = dpll_netlink_init();
>>> +	if (ret)
>>> +		goto error;
>>> +
>>> +	ret = class_register(&dpll_class);
>>> +	if (ret)
>>> +		goto unregister_netlink;
>>> +
>>> +	return 0;
>>> +
>>> +unregister_netlink:
>>> +	dpll_netlink_finish();
>>> +error:
>>> +	mutex_destroy(&dpll_device_xa_lock);
>>> +	return ret;
>>> +}
>>> +subsys_initcall(dpll_init);
>>> diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>>> new file mode 100644
>>> index 000000000000..5ad3224d5caf
>>> --- /dev/null
>>> +++ b/drivers/dpll/dpll_core.h
>>> @@ -0,0 +1,40 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>>> + */
>>> +
>>> +#ifndef __DPLL_CORE_H__
>>> +#define __DPLL_CORE_H__
>>> +
>>> +#include <linux/dpll.h>
>>> +
>>> +#include "dpll_netlink.h"
>>> +
>>> +/**
>>> + * struct dpll_device - structure for a DPLL device
>>> + * @id:		unique id number for each edvice
>>> + * @dev:	&struct device for this dpll device
>>> + * @sources_count:	amount of input sources this dpll_device supports
>>> + * @outputs_count:	amount of outputs this dpll_device supports
>>> + * @ops:	operations this &dpll_device supports
>>> + * @lock:	mutex to serialize operations
>>> + * @priv:	pointer to private information of owner
>>> + */
>>> +struct dpll_device {
>>> +	int id;
>>> +	struct device dev;
>>> +	int sources_count;
>>> +	int outputs_count;
>>> +	struct dpll_device_ops *ops;
>>> +	struct mutex lock;
>>> +	void *priv;
>>> +};
>>> +
>>> +#define to_dpll_device(_dev) \
>>> +	container_of(_dev, struct dpll_device, dev)
>>> +
>>> +int for_each_dpll_device(int id, int (*cb)(struct dpll_device *, void *),
>>> +			  void *data);
>>> +struct dpll_device *dpll_device_get_by_id(int id);
>>> +void dpll_device_unregister(struct dpll_device *dpll);
>>> +#endif
>>> diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>>> new file mode 100644
>>> index 000000000000..0bbdaa6dde8e
>>> --- /dev/null
>>> +++ b/drivers/dpll/dpll_netlink.c
>>> @@ -0,0 +1,437 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Generic netlink for DPLL management framework
>>> + *
>>> + * Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>>> + *
>>> + */
>>> +#include <linux/module.h>
>>> +#include <linux/kernel.h>
>>> +#include <net/genetlink.h>
>>> +#include "dpll_core.h"
>>> +
>>> +#include <uapi/linux/dpll.h>
>>> +
>>> +static const struct genl_multicast_group dpll_genl_mcgrps[] = {
>>> +	{ .name = DPLL_CONFIG_DEVICE_GROUP_NAME, },
>>> +	{ .name = DPLL_CONFIG_SOURCE_GROUP_NAME, },
>>> +	{ .name = DPLL_CONFIG_OUTPUT_GROUP_NAME, },
>>> +	{ .name = DPLL_MONITOR_GROUP_NAME,  },
>>> +};
>>> +
>>> +static const struct nla_policy dpll_genl_get_policy[] = {
>>> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
>>> +	[DPLLA_DEVICE_NAME]	= { .type = NLA_STRING,
>>> +				    .len = DPLL_NAME_LENGTH },
>>> +	[DPLLA_FLAGS]		= { .type = NLA_U32 },
>>> +};
>>> +
>>> +static const struct nla_policy dpll_genl_set_source_policy[] = {
>>> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
>>> +	[DPLLA_SOURCE_ID]	= { .type = NLA_U32 },
>>> +	[DPLLA_SOURCE_TYPE]	= { .type = NLA_U32 },
>>> +};
>>> +
>>> +static const struct nla_policy dpll_genl_set_output_policy[] = {
>>> +	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
>>> +	[DPLLA_OUTPUT_ID]	= { .type = NLA_U32 },
>>> +	[DPLLA_OUTPUT_TYPE]	= { .type = NLA_U32 },
>>> +};
>>> +
>>> +struct param {
>>> +	struct netlink_callback *cb;
>>> +	struct dpll_device *dpll;
>>> +	struct nlattr **attrs;
>>> +	struct sk_buff *msg;
>>> +	int dpll_id;
>>> +	int dpll_source_id;
>>> +	int dpll_source_type;
>>> +	int dpll_output_id;
>>> +	int dpll_output_type;
>>> +};
>>> +
>>> +struct dpll_dump_ctx {
>>> +	struct dpll_device *dev;
>>> +	int flags;
>>> +	int pos_idx;
>>> +	int pos_src_idx;
>>> +	int pos_out_idx;
>>> +};
>>> +
>>> +typedef int (*cb_t)(struct param *);
>>> +
>>> +static struct genl_family dpll_gnl_family;
>>> +
>>> +static struct dpll_dump_ctx *dpll_dump_context(struct netlink_callback *cb)
>>> +{
>>> +	return (struct dpll_dump_ctx *)cb->ctx;
>>> +}
>>> +
>>> +static int __dpll_cmd_device_dump_one(struct dpll_device *dpll,
>>> +					   struct sk_buff *msg)
>>> +{
>>> +	if (nla_put_u32(msg, DPLLA_DEVICE_ID, dpll->id))
>>> +		return -EMSGSIZE;
>>> +
>>> +	if (nla_put_string(msg, DPLLA_DEVICE_NAME, dev_name(&dpll->dev)))
>>> +		return -EMSGSIZE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int __dpll_cmd_dump_sources(struct dpll_device *dpll,
>>> +					   struct sk_buff *msg)
>>> +{
>>> +	struct nlattr *src_attr;
>>> +	int i, ret = 0, type;
>>> +
>>> +	for (i = 0; i < dpll->sources_count; i++) {
>>> +		src_attr = nla_nest_start(msg, DPLLA_SOURCE);
>>> +		if (!src_attr) {
>>> +			ret = -EMSGSIZE;
>>> +			break;
>>> +		}
>>> +		type = dpll->ops->get_source_type(dpll, i);
>>> +		if (nla_put_u32(msg, DPLLA_SOURCE_ID, i) ||
>>> +		    nla_put_u32(msg, DPLLA_SOURCE_TYPE, type)) {
>>> +			nla_nest_cancel(msg, src_attr);
>>> +			ret = -EMSGSIZE;
>>> +			break;
>>> +		}
>>> +		nla_nest_end(msg, src_attr);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int __dpll_cmd_dump_outputs(struct dpll_device *dpll,
>>> +					   struct sk_buff *msg)
>>> +{
>>> +	struct nlattr *out_attr;
>>> +	int i, ret = 0, type;
>>> +
>>> +	for (i = 0; i < dpll->outputs_count; i++) {
>>> +		out_attr = nla_nest_start(msg, DPLLA_OUTPUT);
>>> +		if (!out_attr) {
>>> +			ret = -EMSGSIZE;
>>> +			break;
>>> +		}
>>> +		type = dpll->ops->get_source_type(dpll, i);
>>> +		if (nla_put_u32(msg, DPLLA_OUTPUT_ID, i) ||
>>> +		    nla_put_u32(msg, DPLLA_OUTPUT_TYPE, type)) {
>>> +			nla_nest_cancel(msg, out_attr);
>>> +			ret = -EMSGSIZE;
>>> +			break;
>>> +		}
>>> +		nla_nest_end(msg, out_attr);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int __dpll_cmd_dump_status(struct dpll_device *dpll,
>>> +					   struct sk_buff *msg)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (!dpll->ops->get_status && !dpll->ops->get_temp && !dpll->ops->get_lock_status)
>>> +		return 0;
>> 
>> what if dpll doesn't support one of those commands?
>> 
>
>then only supported attributes will be messaged back to user

Hmm, isn't that redundat if we need to check those again below?

>
>>> +
>>> +	if (dpll->ops->get_status) {
>>> +		ret = dpll->ops->get_status(dpll);
>>> +		if (nla_put_u32(msg, DPLLA_STATUS, ret))
>>> +			return -EMSGSIZE;
>>> +	}
>>> +
>>> +	if (dpll->ops->get_temp) {
>>> +		ret = dpll->ops->get_status(dpll);
>>> +		if (nla_put_u32(msg, DPLLA_TEMP, ret))
>>> +			return -EMSGSIZE;
>>> +	}
>> 
>> shouldn't be get_temp(dpll)?
>
>good catch, copy-paste error
>
>>> +
>>> +	if (dpll->ops->get_lock_status) {
>>> +		ret = dpll->ops->get_lock_status(dpll);
>>> +		if (nla_put_u32(msg, DPLLA_LOCK_STATUS, ret))
>>> +			return -EMSGSIZE;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int dpll_device_dump_one(struct dpll_device *dev, struct sk_buff *msg, int flags)
>>> +{
>>> +	struct nlattr *hdr;
>>> +	int ret;
>>> +
>>> +	hdr = nla_nest_start(msg, DPLLA_DEVICE);
>>> +	if (!hdr)
>>> +		return -EMSGSIZE;
>>> +
>>> +	mutex_lock(&dev->lock);
>>> +	ret = __dpll_cmd_device_dump_one(dev, msg);
>>> +	if (ret)
>>> +		goto out_cancel_nest;
>>> +
>>> +	if (flags & DPLL_FLAG_SOURCES && dev->ops->get_source_type) {
>>> +		ret = __dpll_cmd_dump_sources(dev, msg);
>>> +		if (ret)
>>> +			goto out_cancel_nest;
>>> +	}
>>> +
>>> +	if (flags & DPLL_FLAG_OUTPUTS && dev->ops->get_output_type) {
>>> +		ret = __dpll_cmd_dump_outputs(dev, msg);
>>> +		if (ret)
>>> +			goto out_cancel_nest;
>>> +	}
>>> +
>>> +	if (flags & DPLL_FLAG_STATUS) {
>>> +		ret = __dpll_cmd_dump_status(dev, msg);
>>> +		if (ret)
>>> +			goto out_cancel_nest;
>>> +	}
>>> +
>>> +	mutex_unlock(&dev->lock);
>>> +	nla_nest_end(msg, hdr);
>>> +
>>> +	return 0;
>>> +
>>> +out_cancel_nest:
>>> +	mutex_unlock(&dev->lock);
>>> +	nla_nest_cancel(msg, hdr);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int dpll_genl_cmd_set_source(struct param *p)
>>> +{
>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
>>> +	struct dpll_device *dpll = p->dpll;
>>> +	int ret = 0, src_id, type;
>>> +
>>> +	if (!info->attrs[DPLLA_SOURCE_ID] ||
>>> +	    !info->attrs[DPLLA_SOURCE_TYPE])
>>> +		return -EINVAL;
>>> +
>>> +	if (!dpll->ops->set_source_type)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	src_id = nla_get_u32(info->attrs[DPLLA_SOURCE_ID]);
>>> +	type = nla_get_u32(info->attrs[DPLLA_SOURCE_TYPE]);
>>> +
>>> +	mutex_lock(&dpll->lock);
>>> +	ret = dpll->ops->set_source_type(dpll, src_id, type);
>>> +	mutex_unlock(&dpll->lock);

I wonder if shouldn't the dpll ptr be validated here, and in similar cases.
I mean, between calling dpll_pre_doit and actually doing something on a 'dpll',
it is possible that device gets removed?

Or maybe pre_doit/post_doit shall lock and unlock some other mutex?
Altough, I am not an expert in the netlink stuff, thus just raising a concern.

>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int dpll_genl_cmd_set_output(struct param *p)
>>> +{
>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
>>> +	struct dpll_device *dpll = p->dpll;
>>> +	int ret = 0, out_id, type;
>>> +
>>> +	if (!info->attrs[DPLLA_OUTPUT_ID] ||
>>> +	    !info->attrs[DPLLA_OUTPUT_TYPE])
>>> +		return -EINVAL;
>>> +
>>> +	if (!dpll->ops->set_output_type)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	out_id = nla_get_u32(info->attrs[DPLLA_OUTPUT_ID]);
>>> +	type = nla_get_u32(info->attrs[DPLLA_OUTPUT_TYPE]);
>>> +
>>> +	mutex_lock(&dpll->lock);
>>> +	ret = dpll->ops->set_source_type(dpll, out_id, type);
>>> +	mutex_unlock(&dpll->lock);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int dpll_device_loop_cb(struct dpll_device *dpll, void *data)
>>> +{
>>> +	struct dpll_dump_ctx *ctx;
>>> +	struct param *p = (struct param *)data;
>>> +
>>> +	ctx = dpll_dump_context(p->cb);
>>> +
>>> +	ctx->pos_idx = dpll->id;
>>> +
>>> +	return dpll_device_dump_one(dpll, p->msg, ctx->flags);
>>> +}
>>> +
>>> +static int dpll_cmd_device_dump(struct param *p)
>>> +{
>>> +	struct dpll_dump_ctx *ctx = dpll_dump_context(p->cb);
>>> +
>>> +	return for_each_dpll_device(ctx->pos_idx, dpll_device_loop_cb, p);
>>> +}
>>> +
>>> +static int dpll_genl_cmd_device_get_id(struct param *p)
>>> +{
>>> +	struct dpll_device *dpll = p->dpll;
>>> +	int flags = 0;
>>> +
>>> +	if (p->attrs[DPLLA_FLAGS])
>>> +		flags = nla_get_u32(p->attrs[DPLLA_FLAGS]);
>>> +
>>> +	return dpll_device_dump_one(dpll, p->msg, flags);
>>> +}
>>> +
>>> +static cb_t cmd_doit_cb[] = {
>>> +	[DPLL_CMD_DEVICE_GET]		= dpll_genl_cmd_device_get_id,
>>> +	[DPLL_CMD_SET_SOURCE_TYPE]	= dpll_genl_cmd_set_source,
>>> +	[DPLL_CMD_SET_OUTPUT_TYPE]	= dpll_genl_cmd_set_output,
>>> +};
>>> +
>>> +static cb_t cmd_dump_cb[] = {
>>> +	[DPLL_CMD_DEVICE_GET]		= dpll_cmd_device_dump,
>>> +};
>>> +
>>> +static int dpll_genl_cmd_start(struct netlink_callback *cb)
>>> +{
>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>>> +	struct dpll_dump_ctx *ctx = dpll_dump_context(cb);
>>> +
>>> +	ctx->dev = NULL;
>>> +	if (info->attrs[DPLLA_FLAGS])
>>> +		ctx->flags = nla_get_u32(info->attrs[DPLLA_FLAGS]);
>>> +	else
>>> +		ctx->flags = 0;
>>> +	ctx->pos_idx = 0;
>>> +	ctx->pos_src_idx = 0;
>>> +	ctx->pos_out_idx = 0;
>>> +	return 0;
>>> +}
>>> +
>>> +static int dpll_genl_cmd_dumpit(struct sk_buff *skb,
>>> +				   struct netlink_callback *cb)
>>> +{
>>> +	struct param p = { .cb = cb, .msg = skb };
>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(cb);
>>> +	int cmd = info->op.cmd;
>>> +	int ret;
>>> +	void *hdr;
>>> +
>>> +	hdr = genlmsg_put(skb, 0, 0, &dpll_gnl_family, 0, cmd);
>>> +	if (!hdr)
>>> +		return -EMSGSIZE;
>>> +
>>> +	ret = cmd_dump_cb[cmd](&p);
>>> +	if (ret)
>>> +		goto out_cancel_msg;
>>> +
>>> +	genlmsg_end(skb, hdr);
>>> +
>>> +	return 0;
>>> +
>>> +out_cancel_msg:
>>> +	genlmsg_cancel(skb, hdr);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int dpll_genl_cmd_doit(struct sk_buff *skb,
>>> +				 struct genl_info *info)
>>> +{
>>> +	struct param p = { .attrs = info->attrs, .dpll = info->user_ptr[0] };
>>> +	int cmd = info->genlhdr->cmd;
>>> +	struct sk_buff *msg;
>>> +	void *hdr;
>>> +	int ret;
>>> +
>>> +	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>>> +	if (!msg)
>>> +		return -ENOMEM;
>>> +	p.msg = msg;
>>> +
>>> +	hdr = genlmsg_put_reply(msg, info, &dpll_gnl_family, 0, cmd);
>>> +	if (!hdr) {
>>> +		ret = -EMSGSIZE;
>>> +		goto out_free_msg;
>>> +	}
>>> +
>>> +	ret = cmd_doit_cb[cmd](&p);
>>> +	if (ret)
>>> +		goto out_cancel_msg;
>>> +
>>> +	genlmsg_end(msg, hdr);
>>> +
>>> +	return genlmsg_reply(msg, info);
>>> +
>>> +out_cancel_msg:
>>> +	genlmsg_cancel(msg, hdr);
>>> +out_free_msg:
>>> +	nlmsg_free(msg);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int dpll_pre_doit(const struct genl_ops *ops, struct sk_buff *skb,
>>> +						 struct genl_info *info)
>>> +{
>>> +	struct dpll_device *dpll;
>>> +	int id;
>>> +
>>> +	if (!info->attrs[DPLLA_DEVICE_ID])
>>> +		return -EINVAL;
>>> +	id = nla_get_u32(info->attrs[DPLLA_DEVICE_ID]);
>>> +
>>> +	dpll = dpll_device_get_by_id(id);
>>> +	if (!dpll)
>>> +		return -ENODEV;
>>> +	info->user_ptr[0] = dpll;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct genl_ops dpll_genl_ops[] = {
>>> +	{
>>> +		.cmd	= DPLL_CMD_DEVICE_GET,
>>> +		.start	= dpll_genl_cmd_start,
>>> +		.dumpit	= dpll_genl_cmd_dumpit,
>>> +		.doit	= dpll_genl_cmd_doit,
>>> +		.policy	= dpll_genl_get_policy,
>>> +		.maxattr = ARRAY_SIZE(dpll_genl_get_policy) - 1,
>>> +	},
>>> +	{
>>> +		.cmd	= DPLL_CMD_SET_SOURCE_TYPE,
>>> +		.flags	= GENL_UNS_ADMIN_PERM,
>>> +		.doit	= dpll_genl_cmd_doit,
>>> +		.policy	= dpll_genl_set_source_policy,
>>> +		.maxattr = ARRAY_SIZE(dpll_genl_set_source_policy) - 1,
>>> +	},
>>> +	{
>>> +		.cmd	= DPLL_CMD_SET_OUTPUT_TYPE,
>>> +		.flags	= GENL_UNS_ADMIN_PERM,
>>> +		.doit	= dpll_genl_cmd_doit,
>>> +		.policy	= dpll_genl_set_output_policy,
>>> +		.maxattr = ARRAY_SIZE(dpll_genl_set_output_policy) - 1,
>>> +	},
>>> +};
>>> +
>>> +static struct genl_family dpll_gnl_family __ro_after_init = {
>>> +	.hdrsize	= 0,
>>> +	.name		= DPLL_FAMILY_NAME,
>>> +	.version	= DPLL_VERSION,
>>> +	.ops		= dpll_genl_ops,
>>> +	.n_ops		= ARRAY_SIZE(dpll_genl_ops),
>>> +	.mcgrps		= dpll_genl_mcgrps,
>>> +	.n_mcgrps	= ARRAY_SIZE(dpll_genl_mcgrps),
>>> +	.pre_doit	= dpll_pre_doit,
>>> +};
>>> +
>>> +int __init dpll_netlink_init(void)
>>> +{
>>> +	return genl_register_family(&dpll_gnl_family);
>>> +}
>>> +
>>> +void dpll_netlink_finish(void)
>>> +{
>>> +	genl_unregister_family(&dpll_gnl_family);
>>> +}
>>> +
>>> +void __exit dpll_netlink_fini(void)
>>> +{
>>> +	dpll_netlink_finish();
>>> +}
>>> diff --git a/drivers/dpll/dpll_netlink.h b/drivers/dpll/dpll_netlink.h
>>> new file mode 100644
>>> index 000000000000..e2d100f59dd6
>>> --- /dev/null
>>> +++ b/drivers/dpll/dpll_netlink.h
>>> @@ -0,0 +1,7 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>>> + */
>>> +
>>> +int __init dpll_netlink_init(void);
>>> +void dpll_netlink_finish(void);
>>> diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>>> new file mode 100644
>>> index 000000000000..9051337bcf9e
>>> --- /dev/null
>>> +++ b/include/linux/dpll.h
>>> @@ -0,0 +1,25 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
>>> + */
>>> +
>>> +#ifndef __DPLL_H__
>>> +#define __DPLL_H__
>>> +
>>> +struct dpll_device;
>>> +
>>> +struct dpll_device_ops {
>>> +	int (*get_status)(struct dpll_device *dpll);
>>> +	int (*get_temp)(struct dpll_device *dpll);
>>> +	int (*get_lock_status)(struct dpll_device *dpll);
>>> +	int (*get_source_type)(struct dpll_device *dpll, int id);
>>> +	int (*get_output_type)(struct dpll_device *dpll, int id);
>>> +	int (*set_source_type)(struct dpll_device *dpll, int id, int val);
>>> +	int (*set_output_type)(struct dpll_device *dpll, int id, int val);
>>> +};
>>> +
>>> +struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
>>> +					 int outputs_count, void *priv);
>>> +void dpll_device_register(struct dpll_device *dpll);
>>> +void *dpll_priv(struct dpll_device *dpll);
>>> +#endif
>>> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>>> new file mode 100644
>>> index 000000000000..8c00f52736ee
>>> --- /dev/null
>>> +++ b/include/uapi/linux/dpll.h
>>> @@ -0,0 +1,77 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>> +#ifndef _UAPI_LINUX_DPLL_H
>>> +#define _UAPI_LINUX_DPLL_H
>>> +
>>> +#define DPLL_NAME_LENGTH	20
>>> +
>>> +/* Adding event notification support elements */
>>> +#define DPLL_FAMILY_NAME		"dpll"
>>> +#define DPLL_VERSION			0x01
>>> +#define DPLL_CONFIG_DEVICE_GROUP_NAME  "config"
>>> +#define DPLL_CONFIG_SOURCE_GROUP_NAME  "source"
>>> +#define DPLL_CONFIG_OUTPUT_GROUP_NAME  "output"
>>> +#define DPLL_MONITOR_GROUP_NAME        "monitor"
>>> +
>>> +#define DPLL_FLAG_SOURCES	1
>>> +#define DPLL_FLAG_OUTPUTS	2
>>> +#define DPLL_FLAG_STATUS	4
>>> +
>>> +/* Attributes of dpll_genl_family */
>>> +enum dpll_genl_get_attr {
>>> +	DPLLA_UNSPEC,
>>> +	DPLLA_DEVICE,
>>> +	DPLLA_DEVICE_ID,
>>> +	DPLLA_DEVICE_NAME,
>>> +	DPLLA_SOURCE,
>>> +	DPLLA_SOURCE_ID,
>>> +	DPLLA_SOURCE_TYPE,
>>> +	DPLLA_OUTPUT,
>>> +	DPLLA_OUTPUT_ID,
>>> +	DPLLA_OUTPUT_TYPE,
>>> +	DPLLA_STATUS,
>>> +	DPLLA_TEMP,
>>> +	DPLLA_LOCK_STATUS,
>>> +	DPLLA_FLAGS,
>>> +
>>> +	__DPLLA_MAX,
>>> +};
>>> +#define DPLLA_GET_MAX (__DPLLA_MAX - 1)
>> 
>> I think "_get_/_GET_" in above names is outdated?
>> 
>
>Yes, you are right. The earliest revision had these "GET/SET" in attributes but 
>later we decided to unite them into common attributes. I will remove these 
>artifacts in the next revision.
>
>>> +
>>> +/* DPLL signal types used as source or as output */
>>> +enum dpll_genl_signal_type {
>>> +	DPLL_TYPE_EXT_1PPS,
>>> +	DPLL_TYPE_EXT_10MHZ,
>>> +	DPLL_TYPE_SYNCE_ETH_PORT,
>>> +	DPLL_TYPE_INT_OSCILLATOR,
>>> +	DPLL_TYPE_GNSS,
>>> +
>>> +	__DPLL_TYPE_MAX,
>>> +};
>>> +#define DPLL_TYPE_MAX (__DPLL_TYPE_MAX - 1)
>>> +
>>> +/* Events of dpll_genl_family */
>>> +enum dpll_genl_event {
>>> +	DPLL_EVENT_UNSPEC,
>>> +	DPLL_EVENT_DEVICE_CREATE,		/* DPLL device creation */
>>> +	DPLL_EVENT_DEVICE_DELETE,		/* DPLL device deletion */
>>> +	DPLL_EVENT_STATUS_LOCKED,		/* DPLL device locked to source */
>>> +	DPLL_EVENT_STATUS_UNLOCKED,	/* DPLL device freerun */
>>> +	DPLL_EVENT_SOURCE_CHANGE,		/* DPLL device source changed */
>>> +	DPLL_EVENT_OUTPUT_CHANGE,		/* DPLL device output changed */
>>> +
>>> +	__DPLL_EVENT_MAX,
>>> +};
>>> +#define DPLL_EVENT_MAX (__DPLL_EVENT_MAX - 1)
>>> +
>>> +/* Commands supported by the dpll_genl_family */
>>> +enum dpll_genl_cmd {
>>> +	DPLL_CMD_UNSPEC,
>>> +	DPLL_CMD_DEVICE_GET,	/* List of DPLL devices id */
>>> +	DPLL_CMD_SET_SOURCE_TYPE,	/* Set the DPLL device source type */
>>> +	DPLL_CMD_SET_OUTPUT_TYPE,	/* Get the DPLL device output type */
>> 
>> "Get" in comment description looks like a typo.
>> I am getting bit confused with the name and comments.
>> For me, first look says: it is selection of a type of a source.
>> But in the code I can see it selects a source id and a type.
>> Type of source originates in HW design, why would the one want to "set" it?
>> I can imagine a HW design where a single source or output would allow to choose
>> where the signal originates/goes, some kind of extra selector layer for a
>> source/output, but was that the intention?
>
>In general - yes, we have experimented with our time card providing different 
>types of source synchronisation signal on different input pins, i.e. 1PPS, 
>10MHz, IRIG-B, etc. Any of these signals could be connected to any of 4 external 
>pins, that's why I source id is treated as input pin identifier and source type 
>is the signal type it receives.
>
>> If so, shouldn't the user get some bitmap/list of modes available for each
>> source/output?
>
>Good idea. We have list of available modes exposed via sysfs file, and I agree 
>that it's worth to expose them via netlink interface. I will try to address this 
>in the next version.
>
>> 
>> The user shall get some extra information about the source/output. Right now
>> there can be multiple sources/outputs of the same type, but not really possible
>> to find out their purpose. I.e. a dpll equipped with four source of
>> DPLL_TYPE_EXT_1PPS type.
>>  > This implementation looks like designed for a "forced reference lock" mode
>> where the user must explicitly select one source. But a multi source/output
>> DPLL could be running in different modes. I believe most important is automatic
>> mode, where it tries to lock to a user-configured source priority list.
>> However, there is also freerun mode, where dpll isn't even trying to lock to
>> anything, or NCO - Numerically Controlled Oscillator mode.
>
>Yes, you are right, my focus was on "forced reference lock" mode as currently 
>this is the only mode supported by our hardware. But I'm happy to extend it to 
>other usecases.
>
>> It would be great to have ability to select DPLL modes, but also to be able to
>> configure priorities, read failure status, configure extra "features" (i.e.
>> Embedded Sync, EEC modes, Fast Lock)
>I absolutely agree on this way of improvement, and I already have some ongoing 
>work about failures/events/status change messages. I can see no stoppers for 
>creating priorities (if supported by HW) and other extra "features", but we have 
>to agree on the interface with flexibility in mind.

Great and makes perfect sense!

>
>> The sources and outputs can also have some extra features or capabilities, like:
>> - enable Embedded Sync
>
>Does it mean we want to enable or disable Embedded Sync within one protocol? Is 
>it like Time-Sensitive Networking (TSN) for Ethernet?

Well, from what I know, Embedded PPS (ePPS), Embedded Pulse Per 2 Seconds
(ePP2S) and Embedded Sync (eSync) can be either 25/75 or 75/25, which describes
a ratio of how the 'embedded pulse' is divided into HIGH and LOW states on a
pulse of higher frequency signal in which EPPS/EPP2S/ESync is embedded.

EPPS and EPP2S are rather straightforward, once an EPPS enabled input is
selected as a source, then output configured as PPS(PP2S) shall tick in the
same periods as signal "embedded" in input.
Embedded Sync (eSync) is similar but it allows for configuration of frequency
of a 'sync' signal, i.e. source is 10MHz with eSync configured as 100 HZ, where
the output configured for 100HZ could use it.

I cannot say how exactly Embedded Sync/PPS will be used, as from my perspective
this is user specific, and I am not very familiar with any standard describing
its usage.
I am working on SyncE, where either Embedded Sync or PPS is not a part of SyncE
standard, but I strongly believe that users would need to run a DPLL with
Embedded Sync/PPS enabled for some other things. And probably would love to
have SW control over the dpll.

Lets assume following simplified example:
input1 +-------------+ output1 
-------|             |---------
       |  DPLL 1     |         
input2 |             | output2 
-------|             |---------
       +-------------+         
where:
- input1 is external 10MHz with 25/75 Embedded PPS enabled,
- input2 is a fallback PPS from GNSS
user expects:
- output1 as a 10MHz with embedded sync
- output2 as a PPS
As long as input1 is selected source, output1 is synchonized with it and
output2 ticks are synchronized with ePPS.
Now the user selects input2 as a source, where outputs are unchanged,
both output2 and output1-ePPS are synchronized with input2, and output1
10MHz must be generated by DPLL.

I am trying to show example of what DPLL user might want to configure, this
would be a separated configuration of ePPS/ePP2S/eSync per source as well as
per output.
Also a DPLL itself can have explicitly disabled embedded signal processing on
its sources.

>
>> - add phase delay
>> - configure frequency (user might need to use source/output with different
>>    frequency then 1 PPS or 10MHz)
>
>Providing these modes I was thinking about the most common and widely used 
>signals in measurement equipment. So my point here is that both 1PPS and 10MHz 
>should stay as is, but another type of signal should be added, let's say 
>CUSTOM_FREQ, which will also consider special attribute in netlink terms. is it ok?

Sure, makes sense.

>
>> Generally, for simple DPLL designs this interface could do the job (although,
>> I still think user needs more information about the sources/outputs), but for
>> more complex ones, there should be something different, which takes care of my
>> comments regarding extra configuration needed.
>> 
>
>As I already mentioned earlier, I'm open for improvements and happy to 
>collaborate to cover other use cases of this subsystem from the very beginning 
>of development. We can even create an open github repo to share implementation 
>details together with comments if it works better for you.
>

Sure, great! I am happy to help.
I could start working on a part for extra DPLL modes and source-priorities as
automatic mode doesn't make sense without them.

Thank you,
Arkadiusz  

>> Thanks,
>> Arkadiusz
>> 
>>> +
>>> +	__DPLL_CMD_MAX,
>>> +};
>>> +#define DPLL_CMD_MAX (__DPLL_CMD_MAX - 1)
>>> +
>>> +#endif /* _UAPI_LINUX_DPLL_H */
>>> -- 
>>> 2.27.0
>>>
>>>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

[Vadim Fedorenko]

On 24.06.2022 18:36, Kubalewski, Arkadiusz wrote:

... skipped ...

>>>> +static int __dpll_cmd_dump_status(struct dpll_device *dpll,
>>>> +					   struct sk_buff *msg)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	if (!dpll->ops->get_status && !dpll->ops->get_temp && !dpll->ops->get_lock_status)
>>>> +		return 0;
>>>
>>> what if dpll doesn't support one of those commands?
>>>
>>
>> then only supported attributes will be messaged back to user
> 
> Hmm, isn't that redundat if we need to check those again below?
> 

I removed this check for now. Maybe I will return -NOOPSUPP here.

>>
>>>> +
>>>> +	if (dpll->ops->get_status) {
>>>> +		ret = dpll->ops->get_status(dpll);
>>>> +		if (nla_put_u32(msg, DPLLA_STATUS, ret))
>>>> +			return -EMSGSIZE;
>>>> +	}
>>>> +
>>>> +	if (dpll->ops->get_temp) {
>>>> +		ret = dpll->ops->get_status(dpll);
>>>> +		if (nla_put_u32(msg, DPLLA_TEMP, ret))
>>>> +			return -EMSGSIZE;
>>>> +	}
>>>
>>> shouldn't be get_temp(dpll)?
>>
>> good catch, copy-paste error
>>
>>>> +
>>>> +	if (dpll->ops->get_lock_status) {
>>>> +		ret = dpll->ops->get_lock_status(dpll);
>>>> +		if (nla_put_u32(msg, DPLLA_LOCK_STATUS, ret))
>>>> +			return -EMSGSIZE;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int dpll_device_dump_one(struct dpll_device *dev, struct sk_buff *msg, int flags)
>>>> +{
>>>> +	struct nlattr *hdr;
>>>> +	int ret;
>>>> +
>>>> +	hdr = nla_nest_start(msg, DPLLA_DEVICE);
>>>> +	if (!hdr)
>>>> +		return -EMSGSIZE;
>>>> +
>>>> +	mutex_lock(&dev->lock);
>>>> +	ret = __dpll_cmd_device_dump_one(dev, msg);
>>>> +	if (ret)
>>>> +		goto out_cancel_nest;
>>>> +
>>>> +	if (flags & DPLL_FLAG_SOURCES && dev->ops->get_source_type) {
>>>> +		ret = __dpll_cmd_dump_sources(dev, msg);
>>>> +		if (ret)
>>>> +			goto out_cancel_nest;
>>>> +	}
>>>> +
>>>> +	if (flags & DPLL_FLAG_OUTPUTS && dev->ops->get_output_type) {
>>>> +		ret = __dpll_cmd_dump_outputs(dev, msg);
>>>> +		if (ret)
>>>> +			goto out_cancel_nest;
>>>> +	}
>>>> +
>>>> +	if (flags & DPLL_FLAG_STATUS) {
>>>> +		ret = __dpll_cmd_dump_status(dev, msg);
>>>> +		if (ret)
>>>> +			goto out_cancel_nest;
>>>> +	}
>>>> +
>>>> +	mutex_unlock(&dev->lock);
>>>> +	nla_nest_end(msg, hdr);
>>>> +
>>>> +	return 0;
>>>> +
>>>> +out_cancel_nest:
>>>> +	mutex_unlock(&dev->lock);
>>>> +	nla_nest_cancel(msg, hdr);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int dpll_genl_cmd_set_source(struct param *p)
>>>> +{
>>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
>>>> +	struct dpll_device *dpll = p->dpll;
>>>> +	int ret = 0, src_id, type;
>>>> +
>>>> +	if (!info->attrs[DPLLA_SOURCE_ID] ||
>>>> +	    !info->attrs[DPLLA_SOURCE_TYPE])
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (!dpll->ops->set_source_type)
>>>> +		return -EOPNOTSUPP;
>>>> +
>>>> +	src_id = nla_get_u32(info->attrs[DPLLA_SOURCE_ID]);
>>>> +	type = nla_get_u32(info->attrs[DPLLA_SOURCE_TYPE]);
>>>> +
>>>> +	mutex_lock(&dpll->lock);
>>>> +	ret = dpll->ops->set_source_type(dpll, src_id, type);
>>>> +	mutex_unlock(&dpll->lock);
> 
> I wonder if shouldn't the dpll ptr be validated here, and in similar cases.
> I mean, between calling dpll_pre_doit and actually doing something on a 'dpll',
> it is possible that device gets removed?
> 
> Or maybe pre_doit/post_doit shall lock and unlock some other mutex?
> Altough, I am not an expert in the netlink stuff, thus just raising a concern.
>

I was trying to reduce the scope of mutex as much as possible, but it looks like 
it's better to extend it to cover whole iteration with dpll ptr. Not sure if 
this is a correct way though.

... skipped ...

>>>> +
>>>> +/* Attributes of dpll_genl_family */
>>>> +enum dpll_genl_get_attr {
>>>> +	DPLLA_UNSPEC,
>>>> +	DPLLA_DEVICE,
>>>> +	DPLLA_DEVICE_ID,
>>>> +	DPLLA_DEVICE_NAME,
>>>> +	DPLLA_SOURCE,
>>>> +	DPLLA_SOURCE_ID,
>>>> +	DPLLA_SOURCE_TYPE,
>>>> +	DPLLA_OUTPUT,
>>>> +	DPLLA_OUTPUT_ID,
>>>> +	DPLLA_OUTPUT_TYPE,
>>>> +	DPLLA_STATUS,
>>>> +	DPLLA_TEMP,
>>>> +	DPLLA_LOCK_STATUS,
>>>> +	DPLLA_FLAGS,
>>>> +
>>>> +	__DPLLA_MAX,
>>>> +};
>>>> +#define DPLLA_GET_MAX (__DPLLA_MAX - 1)
>>>
>>> I think "_get_/_GET_" in above names is outdated?
>>>
>>
>> Yes, you are right. The earliest revision had these "GET/SET" in attributes but
>> later we decided to unite them into common attributes. I will remove these
>> artifacts in the next revision.
>>

removed these artifacts

>>>> +
>>>> +/* DPLL signal types used as source or as output */
>>>> +enum dpll_genl_signal_type {
>>>> +	DPLL_TYPE_EXT_1PPS,
>>>> +	DPLL_TYPE_EXT_10MHZ,
>>>> +	DPLL_TYPE_SYNCE_ETH_PORT,
>>>> +	DPLL_TYPE_INT_OSCILLATOR,
>>>> +	DPLL_TYPE_GNSS,
>>>> +
>>>> +	__DPLL_TYPE_MAX,
>>>> +};
>>>> +#define DPLL_TYPE_MAX (__DPLL_TYPE_MAX - 1)
>>>> +
>>>> +/* Events of dpll_genl_family */
>>>> +enum dpll_genl_event {
>>>> +	DPLL_EVENT_UNSPEC,
>>>> +	DPLL_EVENT_DEVICE_CREATE,		/* DPLL device creation */
>>>> +	DPLL_EVENT_DEVICE_DELETE,		/* DPLL device deletion */
>>>> +	DPLL_EVENT_STATUS_LOCKED,		/* DPLL device locked to source */
>>>> +	DPLL_EVENT_STATUS_UNLOCKED,	/* DPLL device freerun */
>>>> +	DPLL_EVENT_SOURCE_CHANGE,		/* DPLL device source changed */
>>>> +	DPLL_EVENT_OUTPUT_CHANGE,		/* DPLL device output changed */
>>>> +
>>>> +	__DPLL_EVENT_MAX,
>>>> +};
>>>> +#define DPLL_EVENT_MAX (__DPLL_EVENT_MAX - 1)
>>>> +
>>>> +/* Commands supported by the dpll_genl_family */
>>>> +enum dpll_genl_cmd {
>>>> +	DPLL_CMD_UNSPEC,
>>>> +	DPLL_CMD_DEVICE_GET,	/* List of DPLL devices id */
>>>> +	DPLL_CMD_SET_SOURCE_TYPE,	/* Set the DPLL device source type */
>>>> +	DPLL_CMD_SET_OUTPUT_TYPE,	/* Get the DPLL device output type */
>>>
>>> "Get" in comment description looks like a typo.
>>> I am getting bit confused with the name and comments.
>>> For me, first look says: it is selection of a type of a source.
>>> But in the code I can see it selects a source id and a type.
>>> Type of source originates in HW design, why would the one want to "set" it?
>>> I can imagine a HW design where a single source or output would allow to choose
>>> where the signal originates/goes, some kind of extra selector layer for a
>>> source/output, but was that the intention?
>>
>> In general - yes, we have experimented with our time card providing different
>> types of source synchronisation signal on different input pins, i.e. 1PPS,
>> 10MHz, IRIG-B, etc. Any of these signals could be connected to any of 4 external
>> pins, that's why I source id is treated as input pin identifier and source type
>> is the signal type it receives.
>>
>>> If so, shouldn't the user get some bitmap/list of modes available for each
>>> source/output?
>>
>> Good idea. We have list of available modes exposed via sysfs file, and I agree
>> that it's worth to expose them via netlink interface. I will try to address this
>> in the next version.
>>
>>>
>>> The user shall get some extra information about the source/output. Right now
>>> there can be multiple sources/outputs of the same type, but not really possible
>>> to find out their purpose. I.e. a dpll equipped with four source of
>>> DPLL_TYPE_EXT_1PPS type.
>>>   > This implementation looks like designed for a "forced reference lock" mode
>>> where the user must explicitly select one source. But a multi source/output
>>> DPLL could be running in different modes. I believe most important is automatic
>>> mode, where it tries to lock to a user-configured source priority list.
>>> However, there is also freerun mode, where dpll isn't even trying to lock to
>>> anything, or NCO - Numerically Controlled Oscillator mode.
>>
>> Yes, you are right, my focus was on "forced reference lock" mode as currently
>> this is the only mode supported by our hardware. But I'm happy to extend it to
>> other usecases.
>>
>>> It would be great to have ability to select DPLL modes, but also to be able to
>>> configure priorities, read failure status, configure extra "features" (i.e.
>>> Embedded Sync, EEC modes, Fast Lock)
>> I absolutely agree on this way of improvement, and I already have some ongoing
>> work about failures/events/status change messages. I can see no stoppers for
>> creating priorities (if supported by HW) and other extra "features", but we have
>> to agree on the interface with flexibility in mind.
> 
> Great and makes perfect sense!
> 
>>
>>> The sources and outputs can also have some extra features or capabilities, like:
>>> - enable Embedded Sync
>>
>> Does it mean we want to enable or disable Embedded Sync within one protocol? Is
>> it like Time-Sensitive Networking (TSN) for Ethernet?
> 
> Well, from what I know, Embedded PPS (ePPS), Embedded Pulse Per 2 Seconds
> (ePP2S) and Embedded Sync (eSync) can be either 25/75 or 75/25, which describes
> a ratio of how the 'embedded pulse' is divided into HIGH and LOW states on a
> pulse of higher frequency signal in which EPPS/EPP2S/ESync is embedded.
> 
> EPPS and EPP2S are rather straightforward, once an EPPS enabled input is
> selected as a source, then output configured as PPS(PP2S) shall tick in the
> same periods as signal "embedded" in input.
> Embedded Sync (eSync) is similar but it allows for configuration of frequency
> of a 'sync' signal, i.e. source is 10MHz with eSync configured as 100 HZ, where
> the output configured for 100HZ could use it.
> 
> I cannot say how exactly Embedded Sync/PPS will be used, as from my perspective
> this is user specific, and I am not very familiar with any standard describing
> its usage.
> I am working on SyncE, where either Embedded Sync or PPS is not a part of SyncE
> standard, but I strongly believe that users would need to run a DPLL with
> Embedded Sync/PPS enabled for some other things. And probably would love to
> have SW control over the dpll.
> 
> Lets assume following simplified example:
> input1 +-------------+ output1
> -------|             |---------
>         |  DPLL 1     |
> input2 |             | output2
> -------|             |---------
>         +-------------+
> where:
> - input1 is external 10MHz with 25/75 Embedded PPS enabled,
> - input2 is a fallback PPS from GNSS
> user expects:
> - output1 as a 10MHz with embedded sync
> - output2 as a PPS
> As long as input1 is selected source, output1 is synchonized with it and
> output2 ticks are synchronized with ePPS.
> Now the user selects input2 as a source, where outputs are unchanged,
> both output2 and output1-ePPS are synchronized with input2, and output1
> 10MHz must be generated by DPLL.
> 
> I am trying to show example of what DPLL user might want to configure, this
> would be a separated configuration of ePPS/ePP2S/eSync per source as well as
> per output.
> Also a DPLL itself can have explicitly disabled embedded signal processing on
> its sources.
>

Thank you for the explanation. Looks like we will need several attributes to 
configure inputs/outputs. And this way can also answer some questions from 
Jonathan regarding support of different specific features of hardware. I'm in 
process of finding an interface to implement it, if you have any suggestions, 
please share it.

>>
>>> - add phase delay
>>> - configure frequency (user might need to use source/output with different
>>>     frequency then 1 PPS or 10MHz)
>>
>> Providing these modes I was thinking about the most common and widely used
>> signals in measurement equipment. So my point here is that both 1PPS and 10MHz
>> should stay as is, but another type of signal should be added, let's say
>> CUSTOM_FREQ, which will also consider special attribute in netlink terms. is it ok?
> 
> Sure, makes sense.
> 
>>
>>> Generally, for simple DPLL designs this interface could do the job (although,
>>> I still think user needs more information about the sources/outputs), but for
>>> more complex ones, there should be something different, which takes care of my
>>> comments regarding extra configuration needed.
>>>
>>
>> As I already mentioned earlier, I'm open for improvements and happy to
>> collaborate to cover other use cases of this subsystem from the very beginning
>> of development. We can even create an open github repo to share implementation
>> details together with comments if it works better for you.
>>
> 
> Sure, great! I am happy to help.
> I could start working on a part for extra DPLL modes and source-priorities as
> automatic mode doesn't make sense without them.
> 
> Thank you,
> Arkadiusz
> 

Please, take a look at RFC v2, I tried to address some of the comments.

Thanks,
Vadim

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

[Kubalewski, Arkadiusz]

-----Original Message-----
From: Vadim Fedorenko <vfedorenko@novek.ru> 
Sent: Sunday, June 26, 2022 9:39 PM
>
>On 24.06.2022 18:36, Kubalewski, Arkadiusz wrote:
>
>... skipped ...
>
>>>>> +static int __dpll_cmd_dump_status(struct dpll_device *dpll,
>>>>> +					   struct sk_buff *msg)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	if (!dpll->ops->get_status && !dpll->ops->get_temp && !dpll->ops->get_lock_status)
>>>>> +		return 0;
>>>>
>>>> what if dpll doesn't support one of those commands?
>>>>
>>>
>>> then only supported attributes will be messaged back to user
>> 
>> Hmm, isn't that redundat if we need to check those again below?
>> 
>
>I removed this check for now. Maybe I will return -NOOPSUPP here.

Ok, that seems reasonable.

>
>>>
>>>>> +
>>>>> +	if (dpll->ops->get_status) {
>>>>> +		ret = dpll->ops->get_status(dpll);
>>>>> +		if (nla_put_u32(msg, DPLLA_STATUS, ret))
>>>>> +			return -EMSGSIZE;
>>>>> +	}
>>>>> +
>>>>> +	if (dpll->ops->get_temp) {
>>>>> +		ret = dpll->ops->get_status(dpll);
>>>>> +		if (nla_put_u32(msg, DPLLA_TEMP, ret))
>>>>> +			return -EMSGSIZE;
>>>>> +	}
>>>>
>>>> shouldn't be get_temp(dpll)?
>>>
>>> good catch, copy-paste error
>>>
>>>>> +
>>>>> +	if (dpll->ops->get_lock_status) {
>>>>> +		ret = dpll->ops->get_lock_status(dpll);
>>>>> +		if (nla_put_u32(msg, DPLLA_LOCK_STATUS, ret))
>>>>> +			return -EMSGSIZE;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int dpll_device_dump_one(struct dpll_device *dev, struct sk_buff *msg, int flags)
>>>>> +{
>>>>> +	struct nlattr *hdr;
>>>>> +	int ret;
>>>>> +
>>>>> +	hdr = nla_nest_start(msg, DPLLA_DEVICE);
>>>>> +	if (!hdr)
>>>>> +		return -EMSGSIZE;
>>>>> +
>>>>> +	mutex_lock(&dev->lock);
>>>>> +	ret = __dpll_cmd_device_dump_one(dev, msg);
>>>>> +	if (ret)
>>>>> +		goto out_cancel_nest;
>>>>> +
>>>>> +	if (flags & DPLL_FLAG_SOURCES && dev->ops->get_source_type) {
>>>>> +		ret = __dpll_cmd_dump_sources(dev, msg);
>>>>> +		if (ret)
>>>>> +			goto out_cancel_nest;
>>>>> +	}
>>>>> +
>>>>> +	if (flags & DPLL_FLAG_OUTPUTS && dev->ops->get_output_type) {
>>>>> +		ret = __dpll_cmd_dump_outputs(dev, msg);
>>>>> +		if (ret)
>>>>> +			goto out_cancel_nest;
>>>>> +	}
>>>>> +
>>>>> +	if (flags & DPLL_FLAG_STATUS) {
>>>>> +		ret = __dpll_cmd_dump_status(dev, msg);
>>>>> +		if (ret)
>>>>> +			goto out_cancel_nest;
>>>>> +	}
>>>>> +
>>>>> +	mutex_unlock(&dev->lock);
>>>>> +	nla_nest_end(msg, hdr);
>>>>> +
>>>>> +	return 0;
>>>>> +
>>>>> +out_cancel_nest:
>>>>> +	mutex_unlock(&dev->lock);
>>>>> +	nla_nest_cancel(msg, hdr);
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static int dpll_genl_cmd_set_source(struct param *p)
>>>>> +{
>>>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
>>>>> +	struct dpll_device *dpll = p->dpll;
>>>>> +	int ret = 0, src_id, type;
>>>>> +
>>>>> +	if (!info->attrs[DPLLA_SOURCE_ID] ||
>>>>> +	    !info->attrs[DPLLA_SOURCE_TYPE])
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	if (!dpll->ops->set_source_type)
>>>>> +		return -EOPNOTSUPP;
>>>>> +
>>>>> +	src_id = nla_get_u32(info->attrs[DPLLA_SOURCE_ID]);
>>>>> +	type = nla_get_u32(info->attrs[DPLLA_SOURCE_TYPE]);
>>>>> +
>>>>> +	mutex_lock(&dpll->lock);
>>>>> +	ret = dpll->ops->set_source_type(dpll, src_id, type);
>>>>> +	mutex_unlock(&dpll->lock);
>> 
>> I wonder if shouldn't the dpll ptr be validated here, and in similar cases.
>> I mean, between calling dpll_pre_doit and actually doing something on a 'dpll',
>> it is possible that device gets removed?
>> 
>> Or maybe pre_doit/post_doit shall lock and unlock some other mutex?
>> Altough, I am not an expert in the netlink stuff, thus just raising a concern.
>>
>
>I was trying to reduce the scope of mutex as much as possible, but it looks like 
>it's better to extend it to cover whole iteration with dpll ptr. Not sure if 
>this is a correct way though.

According to docs, pre/post-doit are designed for that.

>
>... skipped ...
>
>>>>> +
>>>>> +/* Attributes of dpll_genl_family */
>>>>> +enum dpll_genl_get_attr {
>>>>> +	DPLLA_UNSPEC,
>>>>> +	DPLLA_DEVICE,
>>>>> +	DPLLA_DEVICE_ID,
>>>>> +	DPLLA_DEVICE_NAME,
>>>>> +	DPLLA_SOURCE,
>>>>> +	DPLLA_SOURCE_ID,
>>>>> +	DPLLA_SOURCE_TYPE,
>>>>> +	DPLLA_OUTPUT,
>>>>> +	DPLLA_OUTPUT_ID,
>>>>> +	DPLLA_OUTPUT_TYPE,
>>>>> +	DPLLA_STATUS,
>>>>> +	DPLLA_TEMP,
>>>>> +	DPLLA_LOCK_STATUS,
>>>>> +	DPLLA_FLAGS,
>>>>> +
>>>>> +	__DPLLA_MAX,
>>>>> +};
>>>>> +#define DPLLA_GET_MAX (__DPLLA_MAX - 1)
>>>>
>>>> I think "_get_/_GET_" in above names is outdated?
>>>>
>>>
>>> Yes, you are right. The earliest revision had these "GET/SET" in attributes but
>>> later we decided to unite them into common attributes. I will remove these
>>> artifacts in the next revision.
>>>
>
>removed these artifacts
>
>>>>> +
>>>>> +/* DPLL signal types used as source or as output */
>>>>> +enum dpll_genl_signal_type {
>>>>> +	DPLL_TYPE_EXT_1PPS,
>>>>> +	DPLL_TYPE_EXT_10MHZ,
>>>>> +	DPLL_TYPE_SYNCE_ETH_PORT,
>>>>> +	DPLL_TYPE_INT_OSCILLATOR,
>>>>> +	DPLL_TYPE_GNSS,
>>>>> +
>>>>> +	__DPLL_TYPE_MAX,
>>>>> +};
>>>>> +#define DPLL_TYPE_MAX (__DPLL_TYPE_MAX - 1)
>>>>> +
>>>>> +/* Events of dpll_genl_family */
>>>>> +enum dpll_genl_event {
>>>>> +	DPLL_EVENT_UNSPEC,
>>>>> +	DPLL_EVENT_DEVICE_CREATE,		/* DPLL device creation */
>>>>> +	DPLL_EVENT_DEVICE_DELETE,		/* DPLL device deletion */
>>>>> +	DPLL_EVENT_STATUS_LOCKED,		/* DPLL device locked to source */
>>>>> +	DPLL_EVENT_STATUS_UNLOCKED,	/* DPLL device freerun */
>>>>> +	DPLL_EVENT_SOURCE_CHANGE,		/* DPLL device source changed */
>>>>> +	DPLL_EVENT_OUTPUT_CHANGE,		/* DPLL device output changed */
>>>>> +
>>>>> +	__DPLL_EVENT_MAX,
>>>>> +};
>>>>> +#define DPLL_EVENT_MAX (__DPLL_EVENT_MAX - 1)
>>>>> +
>>>>> +/* Commands supported by the dpll_genl_family */
>>>>> +enum dpll_genl_cmd {
>>>>> +	DPLL_CMD_UNSPEC,
>>>>> +	DPLL_CMD_DEVICE_GET,	/* List of DPLL devices id */
>>>>> +	DPLL_CMD_SET_SOURCE_TYPE,	/* Set the DPLL device source type */
>>>>> +	DPLL_CMD_SET_OUTPUT_TYPE,	/* Get the DPLL device output type */
>>>>
>>>> "Get" in comment description looks like a typo.
>>>> I am getting bit confused with the name and comments.
>>>> For me, first look says: it is selection of a type of a source.
>>>> But in the code I can see it selects a source id and a type.
>>>> Type of source originates in HW design, why would the one want to "set" it?
>>>> I can imagine a HW design where a single source or output would allow to choose
>>>> where the signal originates/goes, some kind of extra selector layer for a
>>>> source/output, but was that the intention?
>>>
>>> In general - yes, we have experimented with our time card providing different
>>> types of source synchronisation signal on different input pins, i.e. 1PPS,
>>> 10MHz, IRIG-B, etc. Any of these signals could be connected to any of 4 external
>>> pins, that's why I source id is treated as input pin identifier and source type
>>> is the signal type it receives.
>>>
>>>> If so, shouldn't the user get some bitmap/list of modes available for each
>>>> source/output?
>>>
>>> Good idea. We have list of available modes exposed via sysfs file, and I agree
>>> that it's worth to expose them via netlink interface. I will try to address this
>>> in the next version.
>>>
>>>>
>>>> The user shall get some extra information about the source/output. Right now
>>>> there can be multiple sources/outputs of the same type, but not really possible
>>>> to find out their purpose. I.e. a dpll equipped with four source of
>>>> DPLL_TYPE_EXT_1PPS type.
>>>>   > This implementation looks like designed for a "forced reference lock" mode
>>>> where the user must explicitly select one source. But a multi source/output
>>>> DPLL could be running in different modes. I believe most important is automatic
>>>> mode, where it tries to lock to a user-configured source priority list.
>>>> However, there is also freerun mode, where dpll isn't even trying to lock to
>>>> anything, or NCO - Numerically Controlled Oscillator mode.
>>>
>>> Yes, you are right, my focus was on "forced reference lock" mode as currently
>>> this is the only mode supported by our hardware. But I'm happy to extend it to
>>> other usecases.
>>>
>>>> It would be great to have ability to select DPLL modes, but also to be able to
>>>> configure priorities, read failure status, configure extra "features" (i.e.
>>>> Embedded Sync, EEC modes, Fast Lock)
>>> I absolutely agree on this way of improvement, and I already have some ongoing
>>> work about failures/events/status change messages. I can see no stoppers for
>>> creating priorities (if supported by HW) and other extra "features", but we have
>>> to agree on the interface with flexibility in mind.
>> 
>> Great and makes perfect sense!
>> 
>>>
>>>> The sources and outputs can also have some extra features or capabilities, like:
>>>> - enable Embedded Sync
>>>
>>> Does it mean we want to enable or disable Embedded Sync within one protocol? Is
>>> it like Time-Sensitive Networking (TSN) for Ethernet?
>> 
>> Well, from what I know, Embedded PPS (ePPS), Embedded Pulse Per 2 Seconds
>> (ePP2S) and Embedded Sync (eSync) can be either 25/75 or 75/25, which describes
>> a ratio of how the 'embedded pulse' is divided into HIGH and LOW states on a
>> pulse of higher frequency signal in which EPPS/EPP2S/ESync is embedded.
>> 
>> EPPS and EPP2S are rather straightforward, once an EPPS enabled input is
>> selected as a source, then output configured as PPS(PP2S) shall tick in the
>> same periods as signal "embedded" in input.
>> Embedded Sync (eSync) is similar but it allows for configuration of frequency
>> of a 'sync' signal, i.e. source is 10MHz with eSync configured as 100 HZ, where
>> the output configured for 100HZ could use it.
>> 
>> I cannot say how exactly Embedded Sync/PPS will be used, as from my perspective
>> this is user specific, and I am not very familiar with any standard describing
>> its usage.
>> I am working on SyncE, where either Embedded Sync or PPS is not a part of SyncE
>> standard, but I strongly believe that users would need to run a DPLL with
>> Embedded Sync/PPS enabled for some other things. And probably would love to
>> have SW control over the dpll.
>> 
>> Lets assume following simplified example:
>> input1 +-------------+ output1
>> -------|             |---------
>>         |  DPLL 1     |
>> input2 |             | output2
>> -------|             |---------
>>         +-------------+
>> where:
>> - input1 is external 10MHz with 25/75 Embedded PPS enabled,
>> - input2 is a fallback PPS from GNSS
>> user expects:
>> - output1 as a 10MHz with embedded sync
>> - output2 as a PPS
>> As long as input1 is selected source, output1 is synchonized with it and
>> output2 ticks are synchronized with ePPS.
>> Now the user selects input2 as a source, where outputs are unchanged,
>> both output2 and output1-ePPS are synchronized with input2, and output1
>> 10MHz must be generated by DPLL.
>> 
>> I am trying to show example of what DPLL user might want to configure, this
>> would be a separated configuration of ePPS/ePP2S/eSync per source as well as
>> per output.
>> Also a DPLL itself can have explicitly disabled embedded signal processing on
>> its sources.
>>
>
>Thank you for the explanation. Looks like we will need several attributes to 
>configure inputs/outputs. And this way can also answer some questions from 
>Jonathan regarding support of different specific features of hardware. I'm in 
>process of finding an interface to implement it, if you have any suggestions, 
>please share it.

I would define feature-bit's and get/set the bitmask on them.

For pulse-per-2-seconds (PP2S) - 0.5 HZ is possible, so looks like more enum
types would be needed: 0.5Hz/1Hz/10MHz/custom/2kHz/8kHz (last two for
SONET/SDH).

For custom-frequency input/output it would be great to have set/get of u32
frequency.

For adjusting phase offset it would be great to have set/get of s64 phase
offset.

source/output can be additionally provided with "eSync frequency". Which shall
be handled similarly to frequency of source/output, where common modes are
defined (0.5Hz/1Hz/10MHz/custom/2kHz/8kHz) and possibility to set any freq if
custom is chosen. 

>
>>>
>>>> - add phase delay
>>>> - configure frequency (user might need to use source/output with different
>>>>     frequency then 1 PPS or 10MHz)
>>>
>>> Providing these modes I was thinking about the most common and widely used
>>> signals in measurement equipment. So my point here is that both 1PPS and 10MHz
>>> should stay as is, but another type of signal should be added, let's say
>>> CUSTOM_FREQ, which will also consider special attribute in netlink terms. is it ok?
>> 
>> Sure, makes sense.
>> 
>>>
>>>> Generally, for simple DPLL designs this interface could do the job (although,
>>>> I still think user needs more information about the sources/outputs), but for
>>>> more complex ones, there should be something different, which takes care of my
>>>> comments regarding extra configuration needed.
>>>>
>>>
>>> As I already mentioned earlier, I'm open for improvements and happy to
>>> collaborate to cover other use cases of this subsystem from the very beginning
>>> of development. We can even create an open github repo to share implementation
>>> details together with comments if it works better for you.
>>>
>> 
>> Sure, great! I am happy to help.
>> I could start working on a part for extra DPLL modes and source-priorities as
>> automatic mode doesn't make sense without them.
>> 
>> Thank you,
>> Arkadiusz
>> 
>
>Please, take a look at RFC v2, I tried to address some of the comments.

Yes, planning it for tomorrow.

Thanks,
Arkadiusz

>
>Thanks,
>Vadim
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

[Vadim Fedorenko]

On 29.06.2022 02:46, Kubalewski, Arkadiusz wrote:
> -----Original Message-----
> From: Vadim Fedorenko <vfedorenko@novek.ru>
> Sent: Sunday, June 26, 2022 9:39 PM
>>
>> On 24.06.2022 18:36, Kubalewski, Arkadiusz wrote:
>>
>> ... skipped ...
>>
>>>>>> +static int dpll_device_dump_one(struct dpll_device *dev, struct sk_buff *msg, int flags)
>>>>>> +{
>>>>>> +	struct nlattr *hdr;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	hdr = nla_nest_start(msg, DPLLA_DEVICE);
>>>>>> +	if (!hdr)
>>>>>> +		return -EMSGSIZE;
>>>>>> +
>>>>>> +	mutex_lock(&dev->lock);
>>>>>> +	ret = __dpll_cmd_device_dump_one(dev, msg);
>>>>>> +	if (ret)
>>>>>> +		goto out_cancel_nest;
>>>>>> +
>>>>>> +	if (flags & DPLL_FLAG_SOURCES && dev->ops->get_source_type) {
>>>>>> +		ret = __dpll_cmd_dump_sources(dev, msg);
>>>>>> +		if (ret)
>>>>>> +			goto out_cancel_nest;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (flags & DPLL_FLAG_OUTPUTS && dev->ops->get_output_type) {
>>>>>> +		ret = __dpll_cmd_dump_outputs(dev, msg);
>>>>>> +		if (ret)
>>>>>> +			goto out_cancel_nest;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (flags & DPLL_FLAG_STATUS) {
>>>>>> +		ret = __dpll_cmd_dump_status(dev, msg);
>>>>>> +		if (ret)
>>>>>> +			goto out_cancel_nest;
>>>>>> +	}
>>>>>> +
>>>>>> +	mutex_unlock(&dev->lock);
>>>>>> +	nla_nest_end(msg, hdr);
>>>>>> +
>>>>>> +	return 0;
>>>>>> +
>>>>>> +out_cancel_nest:
>>>>>> +	mutex_unlock(&dev->lock);
>>>>>> +	nla_nest_cancel(msg, hdr);
>>>>>> +
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int dpll_genl_cmd_set_source(struct param *p)
>>>>>> +{
>>>>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
>>>>>> +	struct dpll_device *dpll = p->dpll;
>>>>>> +	int ret = 0, src_id, type;
>>>>>> +
>>>>>> +	if (!info->attrs[DPLLA_SOURCE_ID] ||
>>>>>> +	    !info->attrs[DPLLA_SOURCE_TYPE])
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	if (!dpll->ops->set_source_type)
>>>>>> +		return -EOPNOTSUPP;
>>>>>> +
>>>>>> +	src_id = nla_get_u32(info->attrs[DPLLA_SOURCE_ID]);
>>>>>> +	type = nla_get_u32(info->attrs[DPLLA_SOURCE_TYPE]);
>>>>>> +
>>>>>> +	mutex_lock(&dpll->lock);
>>>>>> +	ret = dpll->ops->set_source_type(dpll, src_id, type);
>>>>>> +	mutex_unlock(&dpll->lock);
>>>
>>> I wonder if shouldn't the dpll ptr be validated here, and in similar cases.
>>> I mean, between calling dpll_pre_doit and actually doing something on a 'dpll',
>>> it is possible that device gets removed?
>>>
>>> Or maybe pre_doit/post_doit shall lock and unlock some other mutex?
>>> Altough, I am not an expert in the netlink stuff, thus just raising a concern.
>>>
>>
>> I was trying to reduce the scope of mutex as much as possible, but it looks like
>> it's better to extend it to cover whole iteration with dpll ptr. Not sure if
>> this is a correct way though.
> 
> According to docs, pre/post-doit are designed for that.
> 

Yeah, got it. Will re-implement locking for next iteration.

>>
>> ... skipped ...
>>
>>>>>> +
>>>>>> +/* DPLL signal types used as source or as output */
>>>>>> +enum dpll_genl_signal_type {
>>>>>> +	DPLL_TYPE_EXT_1PPS,
>>>>>> +	DPLL_TYPE_EXT_10MHZ,
>>>>>> +	DPLL_TYPE_SYNCE_ETH_PORT,
>>>>>> +	DPLL_TYPE_INT_OSCILLATOR,
>>>>>> +	DPLL_TYPE_GNSS,
>>>>>> +
>>>>>> +	__DPLL_TYPE_MAX,
>>>>>> +};
>>>>>> +#define DPLL_TYPE_MAX (__DPLL_TYPE_MAX - 1)
>>>>>> +
>>>>>> +/* Events of dpll_genl_family */
>>>>>> +enum dpll_genl_event {
>>>>>> +	DPLL_EVENT_UNSPEC,
>>>>>> +	DPLL_EVENT_DEVICE_CREATE,		/* DPLL device creation */
>>>>>> +	DPLL_EVENT_DEVICE_DELETE,		/* DPLL device deletion */
>>>>>> +	DPLL_EVENT_STATUS_LOCKED,		/* DPLL device locked to source */
>>>>>> +	DPLL_EVENT_STATUS_UNLOCKED,	/* DPLL device freerun */
>>>>>> +	DPLL_EVENT_SOURCE_CHANGE,		/* DPLL device source changed */
>>>>>> +	DPLL_EVENT_OUTPUT_CHANGE,		/* DPLL device output changed */
>>>>>> +
>>>>>> +	__DPLL_EVENT_MAX,
>>>>>> +};
>>>>>> +#define DPLL_EVENT_MAX (__DPLL_EVENT_MAX - 1)
>>>>>> +
>>>>>> +/* Commands supported by the dpll_genl_family */
>>>>>> +enum dpll_genl_cmd {
>>>>>> +	DPLL_CMD_UNSPEC,
>>>>>> +	DPLL_CMD_DEVICE_GET,	/* List of DPLL devices id */
>>>>>> +	DPLL_CMD_SET_SOURCE_TYPE,	/* Set the DPLL device source type */
>>>>>> +	DPLL_CMD_SET_OUTPUT_TYPE,	/* Get the DPLL device output type */
>>>>>
>>>>> "Get" in comment description looks like a typo.
>>>>> I am getting bit confused with the name and comments.
>>>>> For me, first look says: it is selection of a type of a source.
>>>>> But in the code I can see it selects a source id and a type.
>>>>> Type of source originates in HW design, why would the one want to "set" it?
>>>>> I can imagine a HW design where a single source or output would allow to choose
>>>>> where the signal originates/goes, some kind of extra selector layer for a
>>>>> source/output, but was that the intention?
>>>>
>>>> In general - yes, we have experimented with our time card providing different
>>>> types of source synchronisation signal on different input pins, i.e. 1PPS,
>>>> 10MHz, IRIG-B, etc. Any of these signals could be connected to any of 4 external
>>>> pins, that's why I source id is treated as input pin identifier and source type
>>>> is the signal type it receives.
>>>>
>>>>> If so, shouldn't the user get some bitmap/list of modes available for each
>>>>> source/output?
>>>>
>>>> Good idea. We have list of available modes exposed via sysfs file, and I agree
>>>> that it's worth to expose them via netlink interface. I will try to address this
>>>> in the next version.
>>>>
>>>>>
>>>>> The user shall get some extra information about the source/output. Right now
>>>>> there can be multiple sources/outputs of the same type, but not really possible
>>>>> to find out their purpose. I.e. a dpll equipped with four source of
>>>>> DPLL_TYPE_EXT_1PPS type.
>>>>>    > This implementation looks like designed for a "forced reference lock" mode
>>>>> where the user must explicitly select one source. But a multi source/output
>>>>> DPLL could be running in different modes. I believe most important is automatic
>>>>> mode, where it tries to lock to a user-configured source priority list.
>>>>> However, there is also freerun mode, where dpll isn't even trying to lock to
>>>>> anything, or NCO - Numerically Controlled Oscillator mode.
>>>>
>>>> Yes, you are right, my focus was on "forced reference lock" mode as currently
>>>> this is the only mode supported by our hardware. But I'm happy to extend it to
>>>> other usecases.
>>>>
>>>>> It would be great to have ability to select DPLL modes, but also to be able to
>>>>> configure priorities, read failure status, configure extra "features" (i.e.
>>>>> Embedded Sync, EEC modes, Fast Lock)
>>>> I absolutely agree on this way of improvement, and I already have some ongoing
>>>> work about failures/events/status change messages. I can see no stoppers for
>>>> creating priorities (if supported by HW) and other extra "features", but we have
>>>> to agree on the interface with flexibility in mind.
>>>
>>> Great and makes perfect sense!
>>>
>>>>
>>>>> The sources and outputs can also have some extra features or capabilities, like:
>>>>> - enable Embedded Sync
>>>>
>>>> Does it mean we want to enable or disable Embedded Sync within one protocol? Is
>>>> it like Time-Sensitive Networking (TSN) for Ethernet?
>>>
>>> Well, from what I know, Embedded PPS (ePPS), Embedded Pulse Per 2 Seconds
>>> (ePP2S) and Embedded Sync (eSync) can be either 25/75 or 75/25, which describes
>>> a ratio of how the 'embedded pulse' is divided into HIGH and LOW states on a
>>> pulse of higher frequency signal in which EPPS/EPP2S/ESync is embedded.
>>>
>>> EPPS and EPP2S are rather straightforward, once an EPPS enabled input is
>>> selected as a source, then output configured as PPS(PP2S) shall tick in the
>>> same periods as signal "embedded" in input.
>>> Embedded Sync (eSync) is similar but it allows for configuration of frequency
>>> of a 'sync' signal, i.e. source is 10MHz with eSync configured as 100 HZ, where
>>> the output configured for 100HZ could use it.
>>>
>>> I cannot say how exactly Embedded Sync/PPS will be used, as from my perspective
>>> this is user specific, and I am not very familiar with any standard describing
>>> its usage.
>>> I am working on SyncE, where either Embedded Sync or PPS is not a part of SyncE
>>> standard, but I strongly believe that users would need to run a DPLL with
>>> Embedded Sync/PPS enabled for some other things. And probably would love to
>>> have SW control over the dpll.
>>>
>>> Lets assume following simplified example:
>>> input1 +-------------+ output1
>>> -------|             |---------
>>>          |  DPLL 1     |
>>> input2 |             | output2
>>> -------|             |---------
>>>          +-------------+
>>> where:
>>> - input1 is external 10MHz with 25/75 Embedded PPS enabled,
>>> - input2 is a fallback PPS from GNSS
>>> user expects:
>>> - output1 as a 10MHz with embedded sync
>>> - output2 as a PPS
>>> As long as input1 is selected source, output1 is synchonized with it and
>>> output2 ticks are synchronized with ePPS.
>>> Now the user selects input2 as a source, where outputs are unchanged,
>>> both output2 and output1-ePPS are synchronized with input2, and output1
>>> 10MHz must be generated by DPLL.
>>>
>>> I am trying to show example of what DPLL user might want to configure, this
>>> would be a separated configuration of ePPS/ePP2S/eSync per source as well as
>>> per output.
>>> Also a DPLL itself can have explicitly disabled embedded signal processing on
>>> its sources.
>>>
>>
>> Thank you for the explanation. Looks like we will need several attributes to
>> configure inputs/outputs. And this way can also answer some questions from
>> Jonathan regarding support of different specific features of hardware. I'm in
>> process of finding an interface to implement it, if you have any suggestions,
>> please share it.
> 
> I would define feature-bit's and get/set the bitmask on them.
> 
> For pulse-per-2-seconds (PP2S) - 0.5 HZ is possible, so looks like more enum
> types would be needed: 0.5Hz/1Hz/10MHz/custom/2kHz/8kHz (last two for
> SONET/SDH).
> 
> For custom-frequency input/output it would be great to have set/get of u32
> frequency.

Ok. This is the way I was thinking about too, will make an implementation.

> 
> For adjusting phase offset it would be great to have set/get of s64 phase
> offset.

This way it's getting closer and closer to ptp, but still having phase offset is 
fair point and I will go this way. Jakub, do you have any objections?

> 
> source/output can be additionally provided with "eSync frequency". Which shall
> be handled similarly to frequency of source/output, where common modes are
> defined (0.5Hz/1Hz/10MHz/custom/2kHz/8kHz) and possibility to set any freq if
> custom is chosen.
> 

Didn't get the reason to have special eSync with the same options again. We 
might want a kind of flag/indicator that the port has eSync?

>>
>>>>
>>>>> - add phase delay
>>>>> - configure frequency (user might need to use source/output with different
>>>>>      frequency then 1 PPS or 10MHz)
>>>>
>>>> Providing these modes I was thinking about the most common and widely used
>>>> signals in measurement equipment. So my point here is that both 1PPS and 10MHz
>>>> should stay as is, but another type of signal should be added, let's say
>>>> CUSTOM_FREQ, which will also consider special attribute in netlink terms. is it ok?
>>>
>>> Sure, makes sense.
>>>
>>>>
>>>>> Generally, for simple DPLL designs this interface could do the job (although,
>>>>> I still think user needs more information about the sources/outputs), but for
>>>>> more complex ones, there should be something different, which takes care of my
>>>>> comments regarding extra configuration needed.
>>>>>
>>>>
>>>> As I already mentioned earlier, I'm open for improvements and happy to
>>>> collaborate to cover other use cases of this subsystem from the very beginning
>>>> of development. We can even create an open github repo to share implementation
>>>> details together with comments if it works better for you.
>>>>
>>>
>>> Sure, great! I am happy to help.
>>> I could start working on a part for extra DPLL modes and source-priorities as
>>> automatic mode doesn't make sense without them.
>>>
>>> Thank you,
>>> Arkadiusz
>>>
>>
>> Please, take a look at RFC v2, I tried to address some of the comments.
> 
> Yes, planning it for tomorrow.
> 
> Thanks,
> Arkadiusz
> 
>>
>> Thanks,
>> Vadim
>>

Thanks,
Vadim

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

[Kubalewski, Arkadiusz]

-----Original Message-----
From: Vadim Fedorenko <vfedorenko@novek.ru> 
Sent: Thursday, June 30, 2022 1:30 AM
>
>On 29.06.2022 02:46, Kubalewski, Arkadiusz wrote:
>> -----Original Message-----
>> From: Vadim Fedorenko <vfedorenko@novek.ru>
>> Sent: Sunday, June 26, 2022 9:39 PM
>>>
>>> On 24.06.2022 18:36, Kubalewski, Arkadiusz wrote:
>>>
>>> ... skipped ...
>>>
>>>>>>> +static int dpll_device_dump_one(struct dpll_device *dev, struct sk_buff *msg, int flags)
>>>>>>> +{
>>>>>>> +	struct nlattr *hdr;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	hdr = nla_nest_start(msg, DPLLA_DEVICE);
>>>>>>> +	if (!hdr)
>>>>>>> +		return -EMSGSIZE;
>>>>>>> +
>>>>>>> +	mutex_lock(&dev->lock);
>>>>>>> +	ret = __dpll_cmd_device_dump_one(dev, msg);
>>>>>>> +	if (ret)
>>>>>>> +		goto out_cancel_nest;
>>>>>>> +
>>>>>>> +	if (flags & DPLL_FLAG_SOURCES && dev->ops->get_source_type) {
>>>>>>> +		ret = __dpll_cmd_dump_sources(dev, msg);
>>>>>>> +		if (ret)
>>>>>>> +			goto out_cancel_nest;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (flags & DPLL_FLAG_OUTPUTS && dev->ops->get_output_type) {
>>>>>>> +		ret = __dpll_cmd_dump_outputs(dev, msg);
>>>>>>> +		if (ret)
>>>>>>> +			goto out_cancel_nest;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (flags & DPLL_FLAG_STATUS) {
>>>>>>> +		ret = __dpll_cmd_dump_status(dev, msg);
>>>>>>> +		if (ret)
>>>>>>> +			goto out_cancel_nest;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	mutex_unlock(&dev->lock);
>>>>>>> +	nla_nest_end(msg, hdr);
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +
>>>>>>> +out_cancel_nest:
>>>>>>> +	mutex_unlock(&dev->lock);
>>>>>>> +	nla_nest_cancel(msg, hdr);
>>>>>>> +
>>>>>>> +	return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int dpll_genl_cmd_set_source(struct param *p)
>>>>>>> +{
>>>>>>> +	const struct genl_dumpit_info *info = genl_dumpit_info(p->cb);
>>>>>>> +	struct dpll_device *dpll = p->dpll;
>>>>>>> +	int ret = 0, src_id, type;
>>>>>>> +
>>>>>>> +	if (!info->attrs[DPLLA_SOURCE_ID] ||
>>>>>>> +	    !info->attrs[DPLLA_SOURCE_TYPE])
>>>>>>> +		return -EINVAL;
>>>>>>> +
>>>>>>> +	if (!dpll->ops->set_source_type)
>>>>>>> +		return -EOPNOTSUPP;
>>>>>>> +
>>>>>>> +	src_id = nla_get_u32(info->attrs[DPLLA_SOURCE_ID]);
>>>>>>> +	type = nla_get_u32(info->attrs[DPLLA_SOURCE_TYPE]);
>>>>>>> +
>>>>>>> +	mutex_lock(&dpll->lock);
>>>>>>> +	ret = dpll->ops->set_source_type(dpll, src_id, type);
>>>>>>> +	mutex_unlock(&dpll->lock);
>>>>
>>>> I wonder if shouldn't the dpll ptr be validated here, and in similar cases.
>>>> I mean, between calling dpll_pre_doit and actually doing something on a 'dpll',
>>>> it is possible that device gets removed?
>>>>
>>>> Or maybe pre_doit/post_doit shall lock and unlock some other mutex?
>>>> Altough, I am not an expert in the netlink stuff, thus just raising a concern.
>>>>
>>>
>>> I was trying to reduce the scope of mutex as much as possible, but it looks like
>>> it's better to extend it to cover whole iteration with dpll ptr. Not sure if
>>> this is a correct way though.
>> 
>> According to docs, pre/post-doit are designed for that.
>> 
>
>Yeah, got it. Will re-implement locking for next iteration.

Great, thanks!

>
>>>
>>> ... skipped ...
>>>
>>>>>>> +
>>>>>>> +/* DPLL signal types used as source or as output */
>>>>>>> +enum dpll_genl_signal_type {
>>>>>>> +	DPLL_TYPE_EXT_1PPS,
>>>>>>> +	DPLL_TYPE_EXT_10MHZ,
>>>>>>> +	DPLL_TYPE_SYNCE_ETH_PORT,
>>>>>>> +	DPLL_TYPE_INT_OSCILLATOR,
>>>>>>> +	DPLL_TYPE_GNSS,
>>>>>>> +
>>>>>>> +	__DPLL_TYPE_MAX,
>>>>>>> +};
>>>>>>> +#define DPLL_TYPE_MAX (__DPLL_TYPE_MAX - 1)
>>>>>>> +
>>>>>>> +/* Events of dpll_genl_family */
>>>>>>> +enum dpll_genl_event {
>>>>>>> +	DPLL_EVENT_UNSPEC,
>>>>>>> +	DPLL_EVENT_DEVICE_CREATE,		/* DPLL device creation */
>>>>>>> +	DPLL_EVENT_DEVICE_DELETE,		/* DPLL device deletion */
>>>>>>> +	DPLL_EVENT_STATUS_LOCKED,		/* DPLL device locked to source */
>>>>>>> +	DPLL_EVENT_STATUS_UNLOCKED,	/* DPLL device freerun */
>>>>>>> +	DPLL_EVENT_SOURCE_CHANGE,		/* DPLL device source changed */
>>>>>>> +	DPLL_EVENT_OUTPUT_CHANGE,		/* DPLL device output changed */
>>>>>>> +
>>>>>>> +	__DPLL_EVENT_MAX,
>>>>>>> +};
>>>>>>> +#define DPLL_EVENT_MAX (__DPLL_EVENT_MAX - 1)
>>>>>>> +
>>>>>>> +/* Commands supported by the dpll_genl_family */
>>>>>>> +enum dpll_genl_cmd {
>>>>>>> +	DPLL_CMD_UNSPEC,
>>>>>>> +	DPLL_CMD_DEVICE_GET,	/* List of DPLL devices id */
>>>>>>> +	DPLL_CMD_SET_SOURCE_TYPE,	/* Set the DPLL device source type */
>>>>>>> +	DPLL_CMD_SET_OUTPUT_TYPE,	/* Get the DPLL device output type */
>>>>>>
>>>>>> "Get" in comment description looks like a typo.
>>>>>> I am getting bit confused with the name and comments.
>>>>>> For me, first look says: it is selection of a type of a source.
>>>>>> But in the code I can see it selects a source id and a type.
>>>>>> Type of source originates in HW design, why would the one want to "set" it?
>>>>>> I can imagine a HW design where a single source or output would allow to choose
>>>>>> where the signal originates/goes, some kind of extra selector layer for a
>>>>>> source/output, but was that the intention?
>>>>>
>>>>> In general - yes, we have experimented with our time card providing different
>>>>> types of source synchronisation signal on different input pins, i.e. 1PPS,
>>>>> 10MHz, IRIG-B, etc. Any of these signals could be connected to any of 4 external
>>>>> pins, that's why I source id is treated as input pin identifier and source type
>>>>> is the signal type it receives.
>>>>>
>>>>>> If so, shouldn't the user get some bitmap/list of modes available for each
>>>>>> source/output?
>>>>>
>>>>> Good idea. We have list of available modes exposed via sysfs file, and I agree
>>>>> that it's worth to expose them via netlink interface. I will try to address this
>>>>> in the next version.
>>>>>
>>>>>>
>>>>>> The user shall get some extra information about the source/output. Right now
>>>>>> there can be multiple sources/outputs of the same type, but not really possible
>>>>>> to find out their purpose. I.e. a dpll equipped with four source of
>>>>>> DPLL_TYPE_EXT_1PPS type.
>>>>>>    > This implementation looks like designed for a "forced reference lock" mode
>>>>>> where the user must explicitly select one source. But a multi source/output
>>>>>> DPLL could be running in different modes. I believe most important is automatic
>>>>>> mode, where it tries to lock to a user-configured source priority list.
>>>>>> However, there is also freerun mode, where dpll isn't even trying to lock to
>>>>>> anything, or NCO - Numerically Controlled Oscillator mode.
>>>>>
>>>>> Yes, you are right, my focus was on "forced reference lock" mode as currently
>>>>> this is the only mode supported by our hardware. But I'm happy to extend it to
>>>>> other usecases.
>>>>>
>>>>>> It would be great to have ability to select DPLL modes, but also to be able to
>>>>>> configure priorities, read failure status, configure extra "features" (i.e.
>>>>>> Embedded Sync, EEC modes, Fast Lock)
>>>>> I absolutely agree on this way of improvement, and I already have some ongoing
>>>>> work about failures/events/status change messages. I can see no stoppers for
>>>>> creating priorities (if supported by HW) and other extra "features", but we have
>>>>> to agree on the interface with flexibility in mind.
>>>>
>>>> Great and makes perfect sense!
>>>>
>>>>>
>>>>>> The sources and outputs can also have some extra features or capabilities, like:
>>>>>> - enable Embedded Sync
>>>>>
>>>>> Does it mean we want to enable or disable Embedded Sync within one protocol? Is
>>>>> it like Time-Sensitive Networking (TSN) for Ethernet?
>>>>
>>>> Well, from what I know, Embedded PPS (ePPS), Embedded Pulse Per 2 Seconds
>>>> (ePP2S) and Embedded Sync (eSync) can be either 25/75 or 75/25, which describes
>>>> a ratio of how the 'embedded pulse' is divided into HIGH and LOW states on a
>>>> pulse of higher frequency signal in which EPPS/EPP2S/ESync is embedded.
>>>>
>>>> EPPS and EPP2S are rather straightforward, once an EPPS enabled input is
>>>> selected as a source, then output configured as PPS(PP2S) shall tick in the
>>>> same periods as signal "embedded" in input.
>>>> Embedded Sync (eSync) is similar but it allows for configuration of frequency
>>>> of a 'sync' signal, i.e. source is 10MHz with eSync configured as 100 HZ, where
>>>> the output configured for 100HZ could use it.
>>>>
>>>> I cannot say how exactly Embedded Sync/PPS will be used, as from my perspective
>>>> this is user specific, and I am not very familiar with any standard describing
>>>> its usage.
>>>> I am working on SyncE, where either Embedded Sync or PPS is not a part of SyncE
>>>> standard, but I strongly believe that users would need to run a DPLL with
>>>> Embedded Sync/PPS enabled for some other things. And probably would love to
>>>> have SW control over the dpll.
>>>>
>>>> Lets assume following simplified example:
>>>> input1 +-------------+ output1
>>>> -------|             |---------
>>>>          |  DPLL 1     |
>>>> input2 |             | output2
>>>> -------|             |---------
>>>>          +-------------+
>>>> where:
>>>> - input1 is external 10MHz with 25/75 Embedded PPS enabled,
>>>> - input2 is a fallback PPS from GNSS
>>>> user expects:
>>>> - output1 as a 10MHz with embedded sync
>>>> - output2 as a PPS
>>>> As long as input1 is selected source, output1 is synchonized with it and
>>>> output2 ticks are synchronized with ePPS.
>>>> Now the user selects input2 as a source, where outputs are unchanged,
>>>> both output2 and output1-ePPS are synchronized with input2, and output1
>>>> 10MHz must be generated by DPLL.
>>>>
>>>> I am trying to show example of what DPLL user might want to configure, this
>>>> would be a separated configuration of ePPS/ePP2S/eSync per source as well as
>>>> per output.
>>>> Also a DPLL itself can have explicitly disabled embedded signal processing on
>>>> its sources.
>>>>
>>>
>>> Thank you for the explanation. Looks like we will need several attributes to
>>> configure inputs/outputs. And this way can also answer some questions from
>>> Jonathan regarding support of different specific features of hardware. I'm in
>>> process of finding an interface to implement it, if you have any suggestions,
>>> please share it.
>> 
>> I would define feature-bit's and get/set the bitmask on them.
>> 
>> For pulse-per-2-seconds (PP2S) - 0.5 HZ is possible, so looks like more enum
>> types would be needed: 0.5Hz/1Hz/10MHz/custom/2kHz/8kHz (last two for
>> SONET/SDH).
>> 
>> For custom-frequency input/output it would be great to have set/get of u32
>> frequency.
>
>Ok. This is the way I was thinking about too, will make an implementation.
>
>> 
>> For adjusting phase offset it would be great to have set/get of s64 phase
>> offset.
>
>This way it's getting closer and closer to ptp, but still having phase offset is 
>fair point and I will go this way. Jakub, do you have any objections?

This is different than PTP, where API could "Adjusts the phase offset of the
hardware clock.", I know that sometimes DPLL is driving the HW clock, in such
case PTP interface maybe enough.

For generic DPLL we would like to have an ability to set phase on each layer:
source, output and dpll itself. Besides, e.g. SyncE will not use PTP interface
but the user might want to adjust phase of its DPLL. 

>
>> 
>> source/output can be additionally provided with "eSync frequency". Which shall
>> be handled similarly to frequency of source/output, where common modes are
>> defined (0.5Hz/1Hz/10MHz/custom/2kHz/8kHz) and possibility to set any freq if
>> custom is chosen.
>> 
>
>Didn't get the reason to have special eSync with the same options again. We 
>might want a kind of flag/indicator that the port has eSync?

About 10MHz/2kHz/8kHz, I won't insist on having those eSync modes defined.

The eSync-frequency is different (and lower) than source or output frequency.
Special/named modes are ePPS/ePP2S (1Hz/0.5Hz), but the user might also want to
set any hardware supported eSync frequency.
For example:
Source frequency is 10MHz, then it can have for exampe:
- ePPS (embedded Pulse Per Second) - which is eSync with frequency of 1Hz
- ePP2S (embedded Pulse Per 2 Seconds) - which is eSync with frequency of 0.5Hz
- eSync with any custom frequency, lets say 1 kHz or 1MHz
- eSync with 2 kHz 
- eSync with 8 kHz

The easiest way would be to have a feature flag for enabling eSync and add
configurable eSync-frequency value, but as I mentioned there is also this 0.5
Hz, which complicates things...

Right now, I am thinking to have feature flags like:
- enable ePPS
- enable ePP2S
- enable eSync.
Plus an ability to set "eSync-frequency" to any u32 value.


Thanks,
Arkadiusz

>
>>>
>>>>>
>>>>>> - add phase delay
>>>>>> - configure frequency (user might need to use source/output with different
>>>>>>      frequency then 1 PPS or 10MHz)
>>>>>
>>>>> Providing these modes I was thinking about the most common and widely used
>>>>> signals in measurement equipment. So my point here is that both 1PPS and 10MHz
>>>>> should stay as is, but another type of signal should be added, let's say
>>>>> CUSTOM_FREQ, which will also consider special attribute in netlink terms. is it ok?
>>>>
>>>> Sure, makes sense.
>>>>
>>>>>
>>>>>> Generally, for simple DPLL designs this interface could do the job (although,
>>>>>> I still think user needs more information about the sources/outputs), but for
>>>>>> more complex ones, there should be something different, which takes care of my
>>>>>> comments regarding extra configuration needed.
>>>>>>
>>>>>
>>>>> As I already mentioned earlier, I'm open for improvements and happy to
>>>>> collaborate to cover other use cases of this subsystem from the very beginning
>>>>> of development. We can even create an open github repo to share implementation
>>>>> details together with comments if it works better for you.
>>>>>
>>>>
>>>> Sure, great! I am happy to help.
>>>> I could start working on a part for extra DPLL modes and source-priorities as
>>>> automatic mode doesn't make sense without them.
>>>>
>>>> Thank you,
>>>> Arkadiusz
>>>>
>>>
>>> Please, take a look at RFC v2, I tried to address some of the comments.
>> 
>> Yes, planning it for tomorrow.
>> 
>> Thanks,
>> Arkadiusz
>> 
>>>
>>> Thanks,
>>> Vadim
>>>
>
>Thanks,
>Vadim
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

[Vadim Fedorenko]

On 30.06.2022 03:23, Jakub Kicinski wrote:
> On Thu, 30 Jun 2022 00:30:08 +0100 Vadim Fedorenko wrote:
>>> For adjusting phase offset it would be great to have set/get of s64 phase
>>> offset.
>>
>> This way it's getting closer and closer to ptp, but still having phase offset is
>> fair point and I will go this way. Jakub, do you have any objections?
> 
> How does the DPLL interface interact with PTP? Either API can set the
> phase.

Well, if the same hardware is exposed to both subsystem, it will be serialised 
by hardware driver. And it goes to hardware implementation on how to deal with 
such changes. Am I wrong?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

[Jakub Kicinski]

On Thu, 30 Jun 2022 16:50:46 +0100 Vadim Fedorenko wrote:
> On 30.06.2022 03:23, Jakub Kicinski wrote:
>> On Thu, 30 Jun 2022 00:30:08 +0100 Vadim Fedorenko wrote:  
>>> This way it's getting closer and closer to ptp, but still having phase offset is
>>> fair point and I will go this way. Jakub, do you have any objections?  
>> 
>> How does the DPLL interface interact with PTP? Either API can set the
>> phase.  
> 
> Well, if the same hardware is exposed to both subsystem, it will be serialised 
> by hardware driver. And it goes to hardware implementation on how to deal with 
> such changes. Am I wrong?

That's what ends up happening in practice. But it's a pretty poor
experience for everyone involved :(

Stating the obvious, perhaps, but the goal should be that either the
APIs are disjoint or one is a superset of the other and there can be 
a kernel translation layer so that the driver only has to implement 
one.

By a quick look at the PTP header it has phase offsets for both the
clock and the outputs? Not sure. Don't see much in the docs either.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v1 2/3] dpll: add netlink events

[Vadim Fedorenko]

From: Vadim Fedorenko <vadfed@fb.com>

Add netlink interface to enable notification of users about
events in DPLL framework. Part of this interface should be
used by drivers directly, i.e. lock status changes.

Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
---
 drivers/dpll/dpll_core.c    |   2 +
 drivers/dpll/dpll_netlink.c | 141 ++++++++++++++++++++++++++++++++++++
 drivers/dpll/dpll_netlink.h |   7 ++
 3 files changed, 150 insertions(+)

diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
index e34767e723cf..6be169df3c9e 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -99,6 +99,8 @@ struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_c
 	mutex_unlock(&dpll_device_xa_lock);
 	dpll->priv = priv;
 
+	dpll_notify_device_create(dpll->id, dev_name(&dpll->dev));
+
 	return dpll;
 
 error:
diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
index 0bbdaa6dde8e..6c1dd4ce1f89 100644
--- a/drivers/dpll/dpll_netlink.c
+++ b/drivers/dpll/dpll_netlink.c
@@ -48,6 +48,8 @@ struct param {
 	int dpll_source_type;
 	int dpll_output_id;
 	int dpll_output_type;
+	int dpll_status;
+	const char *dpll_name;
 };
 
 struct dpll_dump_ctx {
@@ -222,6 +224,8 @@ static int dpll_genl_cmd_set_source(struct param *p)
 	ret = dpll->ops->set_source_type(dpll, src_id, type);
 	mutex_unlock(&dpll->lock);
 
+	dpll_notify_source_change(dpll->id, src_id, type);
+
 	return ret;
 }
 
@@ -245,6 +249,8 @@ static int dpll_genl_cmd_set_output(struct param *p)
 	ret = dpll->ops->set_source_type(dpll, out_id, type);
 	mutex_unlock(&dpll->lock);
 
+	dpll_notify_source_change(dpll->id, out_id, type);
+
 	return ret;
 }
 
@@ -421,6 +427,141 @@ static struct genl_family dpll_gnl_family __ro_after_init = {
 	.pre_doit	= dpll_pre_doit,
 };
 
+static int dpll_event_device_create(struct param *p)
+{
+	if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) ||
+	    nla_put_string(p->msg, DPLLA_DEVICE_NAME, p->dpll_name))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int dpll_event_device_delete(struct param *p)
+{
+	if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int dpll_event_status(struct param *p)
+{
+	if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) ||
+		nla_put_u32(p->msg, DPLLA_LOCK_STATUS, p->dpll_status))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int dpll_event_source_change(struct param *p)
+{
+	if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) ||
+	    nla_put_u32(p->msg, DPLLA_SOURCE_ID, p->dpll_source_id) ||
+		nla_put_u32(p->msg, DPLLA_SOURCE_TYPE, p->dpll_source_type))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static int dpll_event_output_change(struct param *p)
+{
+	if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) ||
+	    nla_put_u32(p->msg, DPLLA_OUTPUT_ID, p->dpll_output_id) ||
+		nla_put_u32(p->msg, DPLLA_OUTPUT_TYPE, p->dpll_output_type))
+		return -EMSGSIZE;
+
+	return 0;
+}
+
+static cb_t event_cb[] = {
+	[DPLL_EVENT_DEVICE_CREATE]	= dpll_event_device_create,
+	[DPLL_EVENT_DEVICE_DELETE]	= dpll_event_device_delete,
+	[DPLL_EVENT_STATUS_LOCKED]	= dpll_event_status,
+	[DPLL_EVENT_STATUS_UNLOCKED]	= dpll_event_status,
+	[DPLL_EVENT_SOURCE_CHANGE]	= dpll_event_source_change,
+	[DPLL_EVENT_OUTPUT_CHANGE]	= dpll_event_output_change,
+};
+/*
+ * Generic netlink DPLL event encoding
+ */
+static int dpll_send_event(enum dpll_genl_event event,
+				   struct param *p)
+{
+	struct sk_buff *msg;
+	int ret = -EMSGSIZE;
+	void *hdr;
+
+	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+	p->msg = msg;
+
+	hdr = genlmsg_put(msg, 0, 0, &dpll_gnl_family, 0, event);
+	if (!hdr)
+		goto out_free_msg;
+
+	ret = event_cb[event](p);
+	if (ret)
+		goto out_cancel_msg;
+
+	genlmsg_end(msg, hdr);
+
+	genlmsg_multicast(&dpll_gnl_family, msg, 0, 1, GFP_KERNEL);
+
+	return 0;
+
+out_cancel_msg:
+	genlmsg_cancel(msg, hdr);
+out_free_msg:
+	nlmsg_free(msg);
+
+	return ret;
+}
+
+int dpll_notify_device_create(int dpll_id, const char *name)
+{
+	struct param p = { .dpll_id = dpll_id, .dpll_name = name };
+
+	return dpll_send_event(DPLL_EVENT_DEVICE_CREATE, &p);
+}
+
+int dpll_notify_device_delete(int dpll_id)
+{
+	struct param p = { .dpll_id = dpll_id };
+
+	return dpll_send_event(DPLL_EVENT_DEVICE_DELETE, &p);
+}
+
+int dpll_notify_status_locked(int dpll_id)
+{
+	struct param p = { .dpll_id = dpll_id, .dpll_status = 1 };
+
+	return dpll_send_event(DPLL_EVENT_STATUS_LOCKED, &p);
+}
+
+int dpll_notify_status_unlocked(int dpll_id)
+{
+	struct param p = { .dpll_id = dpll_id, .dpll_status = 0 };
+
+	return dpll_send_event(DPLL_EVENT_STATUS_UNLOCKED, &p);
+}
+
+int dpll_notify_source_change(int dpll_id, int source_id, int source_type)
+{
+	struct param p =  { .dpll_id = dpll_id, .dpll_source_id = source_id,
+						.dpll_source_type = source_type };
+
+	return dpll_send_event(DPLL_EVENT_SOURCE_CHANGE, &p);
+}
+
+int dpll_notify_output_change(int dpll_id, int output_id, int output_type)
+{
+	struct param p =  { .dpll_id = dpll_id, .dpll_output_id = output_id,
+						.dpll_output_type = output_type };
+
+	return dpll_send_event(DPLL_EVENT_OUTPUT_CHANGE, &p);
+}
+
 int __init dpll_netlink_init(void)
 {
 	return genl_register_family(&dpll_gnl_family);
diff --git a/drivers/dpll/dpll_netlink.h b/drivers/dpll/dpll_netlink.h
index e2d100f59dd6..0dc81320f982 100644
--- a/drivers/dpll/dpll_netlink.h
+++ b/drivers/dpll/dpll_netlink.h
@@ -3,5 +3,12 @@
  *  Copyright (c) 2021 Meta Platforms, Inc. and affiliates
  */
 
+int dpll_notify_device_create(int dpll_id, const char *name);
+int dpll_notify_device_delete(int dpll_id);
+int dpll_notify_status_locked(int dpll_id);
+int dpll_notify_status_unlocked(int dpll_id);
+int dpll_notify_source_change(int dpll_id, int source_id, int source_type);
+int dpll_notify_output_change(int dpll_id, int output_id, int output_type);
+
 int __init dpll_netlink_init(void);
 void dpll_netlink_finish(void);
-- 
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Vadim Fedorenko]

From: Vadim Fedorenko <vadfed@fb.com>

Implement DPLL operations in ptp_ocp driver.

Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
---
 drivers/ptp/Kconfig   |  1 +
 drivers/ptp/ptp_ocp.c | 86 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 458218f88c5e..f74846ebc177 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -176,6 +176,7 @@ config PTP_1588_CLOCK_OCP
 	depends on !S390
 	depends on COMMON_CLK
 	select NET_DEVLINK
+	select DPLL
 	help
 	  This driver adds support for an OpenCompute time card.
 
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index e59ea2173aac..693168fdda6c 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -21,6 +21,7 @@
 #include <linux/mtd/mtd.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/crc16.h>
+#include <uapi/linux/dpll.h>
 
 #define PCI_VENDOR_ID_FACEBOOK			0x1d9b
 #define PCI_DEVICE_ID_FACEBOOK_TIMECARD		0x0400
@@ -336,6 +337,7 @@ struct ptp_ocp {
 	struct ptp_ocp_signal	signal[4];
 	struct ptp_ocp_sma_connector sma[4];
 	const struct ocp_sma_op *sma_op;
+	struct dpll_device *dpll;
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -3713,6 +3715,82 @@ ptp_ocp_detach(struct ptp_ocp *bp)
 	device_unregister(&bp->dev);
 }
 
+static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+	int sync;
+
+	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
+	return sync;
+}
+
+static int ptp_ocp_dpll_get_lock_status(struct dpll_device *dpll)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+	int sync;
+
+	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
+	return sync;
+}
+
+static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+	int ret;
+
+	if (bp->sma[sma].mode != SMA_MODE_IN)
+		return -1;
+
+	switch (ptp_ocp_sma_get(bp, sma)) {
+	case 0:
+		ret = DPLL_TYPE_EXT_10MHZ;
+		break;
+	case 1:
+	case 2:
+		ret = DPLL_TYPE_EXT_1PPS;
+		break;
+	default:
+		ret = DPLL_TYPE_INT_OSCILLATOR;
+	}
+
+	return ret;
+}
+
+
+static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+	int ret;
+
+	if (bp->sma[sma].mode != SMA_MODE_OUT)
+		return -1;
+
+	switch (ptp_ocp_sma_get(bp, sma)) {
+	case 0:
+		ret = DPLL_TYPE_EXT_10MHZ;
+		break;
+	case 1:
+	case 2:
+		ret = DPLL_TYPE_INT_OSCILLATOR;
+		break;
+	case 4:
+	case 8:
+		ret = DPLL_TYPE_GNSS;
+		break;
+	default:
+		ret = DPLL_TYPE_INT_OSCILLATOR;
+	}
+
+	return ret;
+}
+
+static struct dpll_device_ops dpll_ops = {
+	.get_status		= ptp_ocp_dpll_get_status,
+	.get_lock_status	= ptp_ocp_dpll_get_lock_status,
+	.get_source_type	= ptp_ocp_dpll_get_source_type,
+	.get_output_type	= ptp_ocp_dpll_get_output_type,
+};
+
 static int
 ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
@@ -3768,6 +3846,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	ptp_ocp_info(bp);
 	devlink_register(devlink);
+
+	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
+	if (!bp->dpll) {
+		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
+		return 0;
+	}
+	dpll_device_register(bp->dpll);
+
 	return 0;
 
 out:
-- 
2.27.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Jonathan Lemon]

On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
> From: Vadim Fedorenko <vadfed@fb.com>
> 
> Implement DPLL operations in ptp_ocp driver.

Please CC: me as well.


> +static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
> +{
> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +	int sync;
> +
> +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
> +	return sync;
> +}

Please match existing code style.


> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
> +{
> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +	int ret;
> +
> +	if (bp->sma[sma].mode != SMA_MODE_IN)
> +		return -1;
> +
> +	switch (ptp_ocp_sma_get(bp, sma)) {
> +	case 0:
> +		ret = DPLL_TYPE_EXT_10MHZ;
> +		break;
> +	case 1:
> +	case 2:
> +		ret = DPLL_TYPE_EXT_1PPS;
> +		break;
> +	default:
> +		ret = DPLL_TYPE_INT_OSCILLATOR;
> +	}
> +
> +	return ret;
> +}

These case statements switch on private bits.  This needs to match
on the selector name instead.


> +static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
> +{
> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +	int ret;
> +
> +	if (bp->sma[sma].mode != SMA_MODE_OUT)
> +		return -1;
> +
> +	switch (ptp_ocp_sma_get(bp, sma)) {
> +	case 0:
> +		ret = DPLL_TYPE_EXT_10MHZ;
> +		break;
> +	case 1:
> +	case 2:
> +		ret = DPLL_TYPE_INT_OSCILLATOR;
> +		break;
> +	case 4:
> +	case 8:
> +		ret = DPLL_TYPE_GNSS;
> +		break;
> +	default:
> +		ret = DPLL_TYPE_INT_OSCILLATOR;

Missing break;


> +	}
> +
> +	return ret;
> +}
> +
> +static struct dpll_device_ops dpll_ops = {
> +	.get_status		= ptp_ocp_dpll_get_status,
> +	.get_lock_status	= ptp_ocp_dpll_get_lock_status,
> +	.get_source_type	= ptp_ocp_dpll_get_source_type,
> +	.get_output_type	= ptp_ocp_dpll_get_output_type,
> +};

No 'set' statements here?  Also, what happens if there is more than
one GNSS receiver, how is this differentiated?
>  static int
>  ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
> @@ -3768,6 +3846,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	ptp_ocp_info(bp);
>  	devlink_register(devlink);
> +
> +	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
> +	if (!bp->dpll) {
> +		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
> +		return 0;
> +	}
> +	dpll_device_register(bp->dpll);
> +

How is the release/unregister path called when the module is unloaded?
-- 
Jonathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Vadim Fedorenko]

On 23.06.2022 19:28, Jonathan Lemon wrote:
> On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
>> From: Vadim Fedorenko <vadfed@fb.com>
>>
>> Implement DPLL operations in ptp_ocp driver.
> 
> Please CC: me as well.
> 

Sure, sorry for not doing it last.

> 
>> +static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +	int sync;
>> +
>> +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
>> +	return sync;
>> +}
> 
> Please match existing code style.
> 
> 
>> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +	int ret;
>> +
>> +	if (bp->sma[sma].mode != SMA_MODE_IN)
>> +		return -1;
>> +
>> +	switch (ptp_ocp_sma_get(bp, sma)) {
>> +	case 0:
>> +		ret = DPLL_TYPE_EXT_10MHZ;
>> +		break;
>> +	case 1:
>> +	case 2:
>> +		ret = DPLL_TYPE_EXT_1PPS;
>> +		break;
>> +	default:
>> +		ret = DPLL_TYPE_INT_OSCILLATOR;
>> +	}
>> +
>> +	return ret;
>> +}
> 
> These case statements switch on private bits.  This needs to match
> on the selector name instead.
> 

Not sure that string comparison is a good idea. Maybe it's better to extend 
struct ocp_selector with netlink type id and fill it according to hardware?

> 
>> +static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +	int ret;
>> +
>> +	if (bp->sma[sma].mode != SMA_MODE_OUT)
>> +		return -1;
>> +
>> +	switch (ptp_ocp_sma_get(bp, sma)) {
>> +	case 0:
>> +		ret = DPLL_TYPE_EXT_10MHZ;
>> +		break;
>> +	case 1:
>> +	case 2:
>> +		ret = DPLL_TYPE_INT_OSCILLATOR;
>> +		break;
>> +	case 4:
>> +	case 8:
>> +		ret = DPLL_TYPE_GNSS;
>> +		break;
>> +	default:
>> +		ret = DPLL_TYPE_INT_OSCILLATOR;
> 
> Missing break;
> 
> 
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static struct dpll_device_ops dpll_ops = {
>> +	.get_status		= ptp_ocp_dpll_get_status,
>> +	.get_lock_status	= ptp_ocp_dpll_get_lock_status,
>> +	.get_source_type	= ptp_ocp_dpll_get_source_type,
>> +	.get_output_type	= ptp_ocp_dpll_get_output_type,
>> +};
> 
> No 'set' statements here?
No, didn't implement it yet

> Also, what happens if there is more than
> one GNSS receiver, how is this differentiated?

Good question. Haven't thought about such case, open for suggestions!

>>   static int
>>   ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   {
>> @@ -3768,6 +3846,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   
>>   	ptp_ocp_info(bp);
>>   	devlink_register(devlink);
>> +
>> +	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
>> +	if (!bp->dpll) {
>> +		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
>> +		return 0;
>> +	}
>> +	dpll_device_register(bp->dpll);
>> +
> 
> How is the release/unregister path called when the module is unloaded?

Thank you Jonathan for pointing it out. I've just realised that unregister path 
is messy in the subsystem, will polish it in the next version and add it to 
driver unload.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Jonathan Lemon]

On Fri, Jun 24, 2022 at 12:11:43AM +0100, Vadim Fedorenko wrote:
> On 23.06.2022 19:28, Jonathan Lemon wrote:
> > On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
> > > From: Vadim Fedorenko <vadfed@fb.com>
> > > +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
> > > +{
> > > +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> > > +	int ret;
> > > +
> > > +	if (bp->sma[sma].mode != SMA_MODE_IN)
> > > +		return -1;
> > > +
> > > +	switch (ptp_ocp_sma_get(bp, sma)) {
> > > +	case 0:
> > > +		ret = DPLL_TYPE_EXT_10MHZ;
> > > +		break;
> > > +	case 1:
> > > +	case 2:
> > > +		ret = DPLL_TYPE_EXT_1PPS;
> > > +		break;
> > > +	default:
> > > +		ret = DPLL_TYPE_INT_OSCILLATOR;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > 
> > These case statements switch on private bits.  This needs to match
> > on the selector name instead.
> > 
> 
> Not sure that string comparison is a good idea. Maybe it's better to extend
> struct ocp_selector with netlink type id and fill it according to hardware?

Sure, that could be an option.  But, as this is DPLL only, how does it
handle things when a pin is used for non-clock IO?  In the timecard,
for example, we have the frequency counters for input, and the frequency
generators/VCC/GND for output.

Actually our HW has a multi-level input, where the DPLL selects its
source from an internal mux - this isn't reflected here.  The external
pins feed into some complex HW logic, which performs its own priority
calculations before presenting the end result as an available selection
for the DPLL.
-- 
Jonathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Vadim Fedorenko]

On 24.06.2022 00:36, Jonathan Lemon wrote:
> On Fri, Jun 24, 2022 at 12:11:43AM +0100, Vadim Fedorenko wrote:
>> On 23.06.2022 19:28, Jonathan Lemon wrote:
>>> On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
>>>> From: Vadim Fedorenko <vadfed@fb.com>
>>>> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
>>>> +{
>>>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>>>> +	int ret;
>>>> +
>>>> +	if (bp->sma[sma].mode != SMA_MODE_IN)
>>>> +		return -1;
>>>> +
>>>> +	switch (ptp_ocp_sma_get(bp, sma)) {
>>>> +	case 0:
>>>> +		ret = DPLL_TYPE_EXT_10MHZ;
>>>> +		break;
>>>> +	case 1:
>>>> +	case 2:
>>>> +		ret = DPLL_TYPE_EXT_1PPS;
>>>> +		break;
>>>> +	default:
>>>> +		ret = DPLL_TYPE_INT_OSCILLATOR;
>>>> +	}
>>>> +
>>>> +	return ret;
>>>> +}
>>>
>>> These case statements switch on private bits.  This needs to match
>>> on the selector name instead.
>>>
>>
>> Not sure that string comparison is a good idea. Maybe it's better to extend
>> struct ocp_selector with netlink type id and fill it according to hardware?
> 
> Sure, that could be an option.  But, as this is DPLL only, how does it
> handle things when a pin is used for non-clock IO?  In the timecard,
> for example, we have the frequency counters for input, and the frequency
> generators/VCC/GND for output.
> 
> Actually our HW has a multi-level input, where the DPLL selects its
> source from an internal mux - this isn't reflected here.  The external
> pins feed into some complex HW logic, which performs its own priority
> calculations before presenting the end result as an available selection
> for the DPLL.

I don't really know how to deal with such configuration. For now I simply added
CUSTOM type but I'm not sure how to deal it 'set' functions. Do you have any 
suggestions?



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Jonathan Lemon]

On Sun, Jun 26, 2022 at 08:28:34PM +0100, Vadim Fedorenko wrote:
> On 24.06.2022 00:36, Jonathan Lemon wrote:
> > On Fri, Jun 24, 2022 at 12:11:43AM +0100, Vadim Fedorenko wrote:
> > > On 23.06.2022 19:28, Jonathan Lemon wrote:
> > > > On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
> > > > > From: Vadim Fedorenko <vadfed@fb.com>
> > > > > +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
> > > > > +{
> > > > > +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> > > > > +	int ret;
> > > > > +
> > > > > +	if (bp->sma[sma].mode != SMA_MODE_IN)
> > > > > +		return -1;
> > > > > +
> > > > > +	switch (ptp_ocp_sma_get(bp, sma)) {
> > > > > +	case 0:
> > > > > +		ret = DPLL_TYPE_EXT_10MHZ;
> > > > > +		break;
> > > > > +	case 1:
> > > > > +	case 2:
> > > > > +		ret = DPLL_TYPE_EXT_1PPS;
> > > > > +		break;
> > > > > +	default:
> > > > > +		ret = DPLL_TYPE_INT_OSCILLATOR;
> > > > > +	}
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > 
> > > > These case statements switch on private bits.  This needs to match
> > > > on the selector name instead.
> > > > 
> > > 
> > > Not sure that string comparison is a good idea. Maybe it's better to extend
> > > struct ocp_selector with netlink type id and fill it according to hardware?
> > 
> > Sure, that could be an option.  But, as this is DPLL only, how does it
> > handle things when a pin is used for non-clock IO?  In the timecard,
> > for example, we have the frequency counters for input, and the frequency
> > generators/VCC/GND for output.
> > 
> > Actually our HW has a multi-level input, where the DPLL selects its
> > source from an internal mux - this isn't reflected here.  The external
> > pins feed into some complex HW logic, which performs its own priority
> > calculations before presenting the end result as an available selection
> > for the DPLL.
> 
> I don't really know how to deal with such configuration. For now I simply added
> CUSTOM type but I'm not sure how to deal it 'set' functions. Do you have any
> suggestions?

No suggestions other than the API should be able to handle things?

Also, should there be a notifier if the source changes?
-- 
Jonathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Vadim Fedorenko]

On 27.06.2022 20:27, Jonathan Lemon wrote:
> On Sun, Jun 26, 2022 at 08:28:34PM +0100, Vadim Fedorenko wrote:
>> On 24.06.2022 00:36, Jonathan Lemon wrote:
>>> On Fri, Jun 24, 2022 at 12:11:43AM +0100, Vadim Fedorenko wrote:
>>>> On 23.06.2022 19:28, Jonathan Lemon wrote:
>>>>> On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
>>>>>> From: Vadim Fedorenko <vadfed@fb.com>
>>>>>> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
>>>>>> +{
>>>>>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	if (bp->sma[sma].mode != SMA_MODE_IN)
>>>>>> +		return -1;
>>>>>> +
>>>>>> +	switch (ptp_ocp_sma_get(bp, sma)) {
>>>>>> +	case 0:
>>>>>> +		ret = DPLL_TYPE_EXT_10MHZ;
>>>>>> +		break;
>>>>>> +	case 1:
>>>>>> +	case 2:
>>>>>> +		ret = DPLL_TYPE_EXT_1PPS;
>>>>>> +		break;
>>>>>> +	default:
>>>>>> +		ret = DPLL_TYPE_INT_OSCILLATOR;
>>>>>> +	}
>>>>>> +
>>>>>> +	return ret;
>>>>>> +}
>>>>>
>>>>> These case statements switch on private bits.  This needs to match
>>>>> on the selector name instead.
>>>>>
>>>>
>>>> Not sure that string comparison is a good idea. Maybe it's better to extend
>>>> struct ocp_selector with netlink type id and fill it according to hardware?
>>>
>>> Sure, that could be an option.  But, as this is DPLL only, how does it
>>> handle things when a pin is used for non-clock IO?  In the timecard,
>>> for example, we have the frequency counters for input, and the frequency
>>> generators/VCC/GND for output.
>>>
>>> Actually our HW has a multi-level input, where the DPLL selects its
>>> source from an internal mux - this isn't reflected here.  The external
>>> pins feed into some complex HW logic, which performs its own priority
>>> calculations before presenting the end result as an available selection
>>> for the DPLL.
>>
>> I don't really know how to deal with such configuration. For now I simply added
>> CUSTOM type but I'm not sure how to deal it 'set' functions. Do you have any
>> suggestions?
> 
> No suggestions other than the API should be able to handle things?

Ok, I will try to combine this with Arkadiusz's suggestion with priorities.
> 
> Also, should there be a notifier if the source changes?

Definitely. I'm working on *set operations and will add notifiers at the same time.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Vadim Fedorenko]

On 23.06.2022 19:28, Jonathan Lemon wrote:
> On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
>> From: Vadim Fedorenko <vadfed@fb.com>
>>
>> +static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +	int sync;
>> +
>> +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
>> +	return sync;
>> +}
> 
> Please match existing code style.
> 

Didn't get this point. The same code is used through out the driver.
Could you please explain?

> 
>> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +	int ret;
>> +
>> +	if (bp->sma[sma].mode != SMA_MODE_IN)
>> +		return -1;
>> +
>> +	switch (ptp_ocp_sma_get(bp, sma)) {
>> +	case 0:
>> +		ret = DPLL_TYPE_EXT_10MHZ;
>> +		break;
>> +	case 1:
>> +	case 2:
>> +		ret = DPLL_TYPE_EXT_1PPS;
>> +		break;
>> +	default:
>> +		ret = DPLL_TYPE_INT_OSCILLATOR;
>> +	}
>> +
>> +	return ret;
>> +}
> 
> These case statements switch on private bits.  This needs to match
> on the selector name instead.
> 

Addressed this in v2

> 
>> +static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +	int ret;
>> +
>> +	if (bp->sma[sma].mode != SMA_MODE_OUT)
>> +		return -1;
>> +
>> +	switch (ptp_ocp_sma_get(bp, sma)) {
>> +	case 0:
>> +		ret = DPLL_TYPE_EXT_10MHZ;
>> +		break;
>> +	case 1:
>> +	case 2:
>> +		ret = DPLL_TYPE_INT_OSCILLATOR;
>> +		break;
>> +	case 4:
>> +	case 8:
>> +		ret = DPLL_TYPE_GNSS;
>> +		break;
>> +	default:
>> +		ret = DPLL_TYPE_INT_OSCILLATOR;
> 
> Missing break;
> 
> 
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static struct dpll_device_ops dpll_ops = {
>> +	.get_status		= ptp_ocp_dpll_get_status,
>> +	.get_lock_status	= ptp_ocp_dpll_get_lock_status,
>> +	.get_source_type	= ptp_ocp_dpll_get_source_type,
>> +	.get_output_type	= ptp_ocp_dpll_get_output_type,
>> +};
> 
> No 'set' statements here?  Also, what happens if there is more than
> one GNSS receiver, how is this differentiated?
>>   static int
>>   ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   {
>> @@ -3768,6 +3846,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   
>>   	ptp_ocp_info(bp);
>>   	devlink_register(devlink);
>> +
>> +	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
>> +	if (!bp->dpll) {
>> +		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
>> +		return 0;
>> +	}
>> +	dpll_device_register(bp->dpll);
>> +
> 
> How is the release/unregister path called when the module is unloaded?

I re-implemented unregister/free in v2. Hope it fixes questions.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Jonathan Lemon]

On Sun, Jun 26, 2022 at 08:27:17PM +0100, Vadim Fedorenko wrote:
> On 23.06.2022 19:28, Jonathan Lemon wrote:
> > On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
> > > From: Vadim Fedorenko <vadfed@fb.com>
> > > 
> > > +static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
> > > +{
> > > +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> > > +	int sync;
> > > +
> > > +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
> > > +	return sync;
> > > +}
> > 
> > Please match existing code style.
> > 
> 
> Didn't get this point. The same code is used through out the driver.
> Could you please explain?

Match existing function definition style.
-- 
Jonathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 3/3] ptp_ocp: implement DPLL ops

[Vadim Fedorenko]

On 27.06.2022 20:23, Jonathan Lemon wrote:
> On Sun, Jun 26, 2022 at 08:27:17PM +0100, Vadim Fedorenko wrote:
>> On 23.06.2022 19:28, Jonathan Lemon wrote:
>>> On Thu, Jun 23, 2022 at 03:57:17AM +0300, Vadim Fedorenko wrote:
>>>> From: Vadim Fedorenko <vadfed@fb.com>
>>>>
>>>> +static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
>>>> +{
>>>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>>>> +	int sync;
>>>> +
>>>> +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
>>>> +	return sync;
>>>> +}
>>>
>>> Please match existing code style.
>>>
>>
>> Didn't get this point. The same code is used through out the driver.
>> Could you please explain?
> 
> Match existing function definition style.

Got it. Will address in the next version, thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 0/3] Create common DPLL/clock configuration API

[Florian Fainelli]

On 6/22/22 17:57, Vadim Fedorenko wrote:
> From: Vadim Fedorenko <vadfed@fb.com>
> 
> Implement common API for clock/DPLL configuration and status reporting.
> The API utilises netlink interface as transport for commands and event
> notifications. This API aim to extend current pin configuration and
> make it flexible and easy to cover special configurations.

Any reasons why you are not copying the Linux common clock framework 
maintainers and not seeking to get your code included under drivers/clk/ 
where it would seem like a more natural place for it?

Is netlink really a necessary configuration interface for those devices?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v1 0/3] Create common DPLL/clock configuration API

[Jakub Kicinski]

On Fri, 24 Jun 2022 10:20:44 -0700 Florian Fainelli wrote:
> On 6/22/22 17:57, Vadim Fedorenko wrote:
> > Implement common API for clock/DPLL configuration and status reporting.
> > The API utilises netlink interface as transport for commands and event
> > notifications. This API aim to extend current pin configuration and
> > make it flexible and easy to cover special configurations.  
> 
> Any reasons why you are not copying the Linux common clock framework 
> maintainers and not seeking to get your code included under drivers/clk/ 
> where it would seem like a more natural place for it?
> 
> Is netlink really a necessary configuration interface for those devices?

Sorry, likely my fault. Vadim asked me who to CC and I suggested to just
hit linux-arm-kernel assuming it's the best place for chance encounters
with embedded folks. An assumption based on no real data or experience.

Regarding the clock framework I think I brought that suggestion up in
the mega thread when Maciej M proposed the SyncE support and putting
all the PLL info directly in rtnetlink. There wasn't much support,
and perhaps that's fair, clock generation vs runtime DPLL config for
time source purposes are different use cases.

With that longish excuse out of the way, CCing linux-clk now, here's
the lore link to the thread:

https://lore.kernel.org/r/20220623005717.31040-2-vfedorenko@novek.ru/


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel