* [RFC PATCH v2 1/3] dpll: Add DPLL framework base functions
2022-06-26 19:24 [RFC PATCH v2 0/3] Create common DPLL/clock configuration API Vadim Fedorenko
@ 2022-06-26 19:24 ` Vadim Fedorenko
2022-06-29 8:34 ` Stephen Boyd
2022-07-11 9:01 ` Kubalewski, Arkadiusz
2022-06-26 19:24 ` [RFC PATCH v2 2/3] dpll: add netlink events Vadim Fedorenko
` (3 subsequent siblings)
4 siblings, 2 replies; 36+ messages in thread
From: Vadim Fedorenko @ 2022-06-26 19:24 UTC (permalink / raw)
To: Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
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 | 159 +++++++++++++
drivers/dpll/dpll_core.h | 40 ++++
drivers/dpll/dpll_netlink.c | 454 ++++++++++++++++++++++++++++++++++++
drivers/dpll/dpll_netlink.h | 7 +
include/linux/dpll.h | 29 +++
include/uapi/linux/dpll.h | 79 +++++++
11 files changed, 793 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..dc0330e3687d
--- /dev/null
+++ b/drivers/dpll/dpll_core.c
@@ -0,0 +1,159 @@
+// 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);
+ dpll_device_free(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_free(struct dpll_device *dpll)
+{
+ if (!dpll)
+ return;
+
+ mutex_destroy(&dpll->lock);
+ kfree(dpll);
+}
+
+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..e15106f30377
--- /dev/null
+++ b/drivers/dpll/dpll_netlink.c
@@ -0,0 +1,454 @@
+// 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;
+ }
+ if (dpll->ops->get_source_supported) {
+ for (type = 0; type <= DPLL_TYPE_MAX; type++) {
+ ret = dpll->ops->get_source_supported(dpll, i, type);
+ if (ret && nla_put_u32(msg, DPLLA_SOURCE_SUPPORTED, type)) {
+ ret = -EMSGSIZE;
+ break;
+ }
+ }
+ ret = 0;
+ }
+ 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_output_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;
+ }
+ if (dpll->ops->get_output_supported) {
+ for (type = 0; type <= DPLL_TYPE_MAX; type++) {
+ ret = dpll->ops->get_output_supported(dpll, i, type);
+ if (ret && nla_put_u32(msg, DPLLA_OUTPUT_SUPPORTED, type)) {
+ ret = -EMSGSIZE;
+ break;
+ }
+ }
+ ret = 0;
+ }
+ 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) {
+ 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_temp(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 *dpll, struct sk_buff *msg, int flags)
+{
+ struct nlattr *hdr;
+ int ret;
+
+ hdr = nla_nest_start(msg, DPLLA_DEVICE);
+ if (!hdr)
+ return -EMSGSIZE;
+
+ mutex_lock(&dpll->lock);
+ ret = __dpll_cmd_device_dump_one(dpll, msg);
+ if (ret)
+ goto out_cancel_nest;
+
+ if (flags & DPLL_FLAG_SOURCES && dpll->ops->get_source_type) {
+ ret = __dpll_cmd_dump_sources(dpll, msg);
+ if (ret)
+ goto out_cancel_nest;
+ }
+
+ if (flags & DPLL_FLAG_OUTPUTS && dpll->ops->get_output_type) {
+ ret = __dpll_cmd_dump_outputs(dpll, msg);
+ if (ret)
+ goto out_cancel_nest;
+ }
+
+ if (flags & DPLL_FLAG_STATUS) {
+ ret = __dpll_cmd_dump_status(dpll, msg);
+ if (ret)
+ goto out_cancel_nest;
+ }
+
+ mutex_unlock(&dpll->lock);
+ nla_nest_end(msg, hdr);
+
+ return 0;
+
+out_cancel_nest:
+ mutex_unlock(&dpll->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..4ebda933d5f6
--- /dev/null
+++ b/include/linux/dpll.h
@@ -0,0 +1,29 @@
+/* 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_source_supported)(struct dpll_device *dpll, int id, int type);
+ int (*get_output_type)(struct dpll_device *dpll, int id);
+ int (*get_output_supported)(struct dpll_device *dpll, int id, int type);
+ 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_device_unregister(struct dpll_device *dpll);
+void dpll_device_free(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..7ce45c6b4fd4
--- /dev/null
+++ b/include/uapi/linux/dpll.h
@@ -0,0 +1,79 @@
+/* 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_attr {
+ DPLLA_UNSPEC,
+ DPLLA_DEVICE,
+ DPLLA_DEVICE_ID,
+ DPLLA_DEVICE_NAME,
+ DPLLA_SOURCE,
+ DPLLA_SOURCE_ID,
+ DPLLA_SOURCE_TYPE,
+ DPLLA_SOURCE_SUPPORTED,
+ DPLLA_OUTPUT,
+ DPLLA_OUTPUT_ID,
+ DPLLA_OUTPUT_TYPE,
+ DPLLA_OUTPUT_SUPPORTED,
+ DPLLA_STATUS,
+ DPLLA_TEMP,
+ DPLLA_LOCK_STATUS,
+ DPLLA_FLAGS,
+
+ __DPLLA_MAX,
+};
+#define DPLLA_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, /* Set 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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 1/3] dpll: Add DPLL framework base functions
2022-06-26 19:24 ` [RFC PATCH v2 1/3] dpll: Add DPLL framework base functions Vadim Fedorenko
@ 2022-06-29 8:34 ` Stephen Boyd
2022-06-29 23:37 ` Vadim Fedorenko
2022-07-11 9:01 ` Kubalewski, Arkadiusz
1 sibling, 1 reply; 36+ messages in thread
From: Stephen Boyd @ 2022-06-29 8:34 UTC (permalink / raw)
To: Arkadiusz Kubalewski, Jakub Kicinski, Jonathan Lemon, Vadim Fedorenko
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
Quoting Vadim Fedorenko (2022-06-26 12:24:42)
> 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.
Please add more details to the commit text, and possibly introduce some
Documentation/ about this driver subsystem. I'm curious what is
different from drivers/clk/, is it super large frequencies that don't
fit into 32-bits when represented in Hz? Or PLL focused? Or is sub-Hz
required?
Details please!
Does DPLL stand for digital phase locked loop? Again, I have no idea! I
think you get my point.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 1/3] dpll: Add DPLL framework base functions
2022-06-29 8:34 ` Stephen Boyd
@ 2022-06-29 23:37 ` Vadim Fedorenko
0 siblings, 0 replies; 36+ messages in thread
From: Vadim Fedorenko @ 2022-06-29 23:37 UTC (permalink / raw)
To: Stephen Boyd, Arkadiusz Kubalewski, Jakub Kicinski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
Hi Stephen!
On 29.06.2022 09:34, Stephen Boyd wrote:
> Quoting Vadim Fedorenko (2022-06-26 12:24:42)
>> 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.
>
> Please add more details to the commit text, and possibly introduce some
> Documentation/ about this driver subsystem. I'm curious what is
> different from drivers/clk/, is it super large frequencies that don't
> fit into 32-bits when represented in Hz? Or PLL focused? Or is sub-Hz
> required?
Sure, I'm working on adding Documentation/ patch in the next iteration. For now
I would it's mostly focused on PLL configuration rather then clocking thing. And
the main reason is to provide flexible netlink API.
>
> Details please!
>
> Does DPLL stand for digital phase locked loop? Again, I have no idea! I
> think you get my point.
Yes, you are right, DPLL stands for digital phase locked loop.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* RE: [RFC PATCH v2 1/3] dpll: Add DPLL framework base functions
2022-06-26 19:24 ` [RFC PATCH v2 1/3] dpll: Add DPLL framework base functions Vadim Fedorenko
2022-06-29 8:34 ` Stephen Boyd
@ 2022-07-11 9:01 ` Kubalewski, Arkadiusz
2022-07-14 23:23 ` Vadim Fedorenko
1 sibling, 1 reply; 36+ messages in thread
From: Kubalewski, Arkadiusz @ 2022-07-11 9:01 UTC (permalink / raw)
To: Vadim Fedorenko, Jakub Kicinski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
-----Original Message-----
From: Vadim Fedorenko <vfedorenko@novek.ru>
Sent: Sunday, June 26, 2022 9:25 PM
>
>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>
Hi Vadim,
I've been trying to implement usage of this code in our driver.
Any chance for some testing/example app?
>---
> MAINTAINERS | 8 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/dpll/Kconfig | 7 +
> drivers/dpll/Makefile | 7 +
> drivers/dpll/dpll_core.c | 159 +++++++++++++
> drivers/dpll/dpll_core.h | 40 ++++
> drivers/dpll/dpll_netlink.c | 454 ++++++++++++++++++++++++++++++++++++
> drivers/dpll/dpll_netlink.h | 7 +
> include/linux/dpll.h | 29 +++
> include/uapi/linux/dpll.h | 79 +++++++
> 11 files changed, 793 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..dc0330e3687d
>--- /dev/null
>+++ b/drivers/dpll/dpll_core.c
>@@ -0,0 +1,159 @@
>+// 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)
Line is over 80 chars
>+{
>+ 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);
>+ dpll_device_free(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)
Aren't there some alignment issues around function definitions?
>+{
>+ 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);
Not sure if I mentioned it before, the user must be able to identify the
purpose and origin of dpll. Right now, if 2 dplls register in the system, it is
not possible to determine where they belong or what do they do. I would say,
easiest to let caller of dpll_device_alloc assign some name or description.
>+ 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_free(struct dpll_device *dpll)
>+{
>+ if (!dpll)
>+ return;
>+
>+ mutex_destroy(&dpll->lock);
>+ kfree(dpll);
>+}
dpll_device_free() is defined in header, shouldn't it be exported?
>+
>+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));
dpll_notify_device_create is not yet defined, this is part of patch 2/3?
Also in patch 2/3 similar call was added in dpll_device_alloc().
>+ 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..e15106f30377
>--- /dev/null
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -0,0 +1,454 @@
>+// 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;
>+ }
>+ if (dpll->ops->get_source_supported) {
>+ for (type = 0; type <= DPLL_TYPE_MAX; type++) {
>+ ret = dpll->ops->get_source_supported(dpll, i, type);
>+ if (ret && nla_put_u32(msg, DPLLA_SOURCE_SUPPORTED, type)) {
>+ ret = -EMSGSIZE;
>+ break;
>+ }
>+ }
>+ ret = 0;
>+ }
>+ 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_output_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;
>+ }
>+ if (dpll->ops->get_output_supported) {
>+ for (type = 0; type <= DPLL_TYPE_MAX; type++) {
>+ ret = dpll->ops->get_output_supported(dpll, i, type);
>+ if (ret && nla_put_u32(msg, DPLLA_OUTPUT_SUPPORTED, type)) {
>+ ret = -EMSGSIZE;
>+ break;
>+ }
>+ }
>+ ret = 0;
>+ }
>+ 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) {
>+ 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_temp(dpll);
>+ if (nla_put_u32(msg, DPLLA_TEMP, ret))
>+ return -EMSGSIZE;
>+ }
>+
>+ if (dpll->ops->get_lock_status) {
>+ ret = dpll->ops->get_lock_status(dpll);
We shall have defined lock states, same for get status, as int is returned
by both, the userspace must have common understanding of values returned by
those functions.
>+ if (nla_put_u32(msg, DPLLA_LOCK_STATUS, ret))
>+ return -EMSGSIZE;
>+ }
>+
>+ return 0;
>+}
>+
>+static int dpll_device_dump_one(struct dpll_device *dpll, struct sk_buff *msg, int flags)
>+{
>+ struct nlattr *hdr;
>+ int ret;
>+
>+ hdr = nla_nest_start(msg, DPLLA_DEVICE);
>+ if (!hdr)
>+ return -EMSGSIZE;
>+
>+ mutex_lock(&dpll->lock);
>+ ret = __dpll_cmd_device_dump_one(dpll, msg);
>+ if (ret)
>+ goto out_cancel_nest;
>+
>+ if (flags & DPLL_FLAG_SOURCES && dpll->ops->get_source_type) {
>+ ret = __dpll_cmd_dump_sources(dpll, msg);
>+ if (ret)
>+ goto out_cancel_nest;
>+ }
>+
>+ if (flags & DPLL_FLAG_OUTPUTS && dpll->ops->get_output_type) {
>+ ret = __dpll_cmd_dump_outputs(dpll, msg);
>+ if (ret)
>+ goto out_cancel_nest;
>+ }
>+
>+ if (flags & DPLL_FLAG_STATUS) {
>+ ret = __dpll_cmd_dump_status(dpll, msg);
>+ if (ret)
>+ goto out_cancel_nest;
>+ }
>+
>+ mutex_unlock(&dpll->lock);
>+ nla_nest_end(msg, hdr);
>+
>+ return 0;
>+
>+out_cancel_nest:
>+ mutex_unlock(&dpll->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])
Got a crash here (when tried to send a message without 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,
>+ },
I wouldn't leave non-privileged user with possibility to call any HW requests.
>+ {
>+ .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..4ebda933d5f6
>--- /dev/null
>+++ b/include/linux/dpll.h
>@@ -0,0 +1,29 @@
>+/* 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_source_supported)(struct dpll_device *dpll, int id, int type);
>+ int (*get_output_type)(struct dpll_device *dpll, int id);
>+ int (*get_output_supported)(struct dpll_device *dpll, int id, int type);
>+ 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_device_unregister(struct dpll_device *dpll);
>+void dpll_device_free(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..7ce45c6b4fd4
>--- /dev/null
>+++ b/include/uapi/linux/dpll.h
>@@ -0,0 +1,79 @@
>+/* 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_attr {
>+ DPLLA_UNSPEC,
>+ DPLLA_DEVICE,
>+ DPLLA_DEVICE_ID,
>+ DPLLA_DEVICE_NAME,
>+ DPLLA_SOURCE,
>+ DPLLA_SOURCE_ID,
>+ DPLLA_SOURCE_TYPE,
>+ DPLLA_SOURCE_SUPPORTED,
>+ DPLLA_OUTPUT,
>+ DPLLA_OUTPUT_ID,
>+ DPLLA_OUTPUT_TYPE,
>+ DPLLA_OUTPUT_SUPPORTED,
>+ DPLLA_STATUS,
>+ DPLLA_TEMP,
>+ DPLLA_LOCK_STATUS,
>+ DPLLA_FLAGS,
>+
>+ __DPLLA_MAX,
>+};
>+#define DPLLA_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, /* Set the DPLL device output type */
This week, I am going to prepare the patch for DPLL mode and input priority list
we have discussed on the previous patch series.
Thank you!
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
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 1/3] dpll: Add DPLL framework base functions
2022-07-11 9:01 ` Kubalewski, Arkadiusz
@ 2022-07-14 23:23 ` Vadim Fedorenko
2022-07-15 17:31 ` Kubalewski, Arkadiusz
0 siblings, 1 reply; 36+ messages in thread
From: Vadim Fedorenko @ 2022-07-14 23:23 UTC (permalink / raw)
To: Kubalewski, Arkadiusz, Jakub Kicinski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
On 11.07.2022 10:01, Kubalewski, Arkadiusz wrote:
> -----Original Message-----
> From: Vadim Fedorenko <vfedorenko@novek.ru>
> Sent: Sunday, June 26, 2022 9:25 PM
>>
>> 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>
>
> Hi Vadim,
> I've been trying to implement usage of this code in our driver.
> Any chance for some testing/example app?
>
Hi Arkadiusz,
Sorry, but I don't have any working app yet, this subsystem is
treated as experimental and as we now have different interface
for configuring features we need, app implementation is postponed
a bit. After some conversation with Jakub I'm thinking about library
to provide easy interface to this subsystem, but stil no code yet.
>> +struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
>> + int outputs_count, void *priv)
>
> Aren't there some alignment issues around function definitions?
>
Yeah, I know about some style issues, trying to fix them for the next round.
>> +{
>> + 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);
>
> Not sure if I mentioned it before, the user must be able to identify the
> purpose and origin of dpll. Right now, if 2 dplls register in the system, it is
> not possible to determine where they belong or what do they do. I would say,
> easiest to let caller of dpll_device_alloc assign some name or description.
>
Maybe driver can export information about dpll device name after registering it?
But at the same time looks like it's easy enough to implement custom naming. The
only problem is to control that names are unique.
>> + 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_free(struct dpll_device *dpll)
>> +{
>> + if (!dpll)
>> + return;
>> +
>> + mutex_destroy(&dpll->lock);
>> + kfree(dpll);
>> +}
>
> dpll_device_free() is defined in header, shouldn't it be exported?
>
yeah, definitely. Already changed in new draft.
>> +
>> +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));
>
> dpll_notify_device_create is not yet defined, this is part of patch 2/3?
> Also in patch 2/3 similar call was added in dpll_device_alloc().
>
Ah. Yes, there was some mess with patches, looks like I missed this thing, thank
you for pointing it.
>> +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,
>> + },
>
> I wouldn't leave non-privileged user with possibility to call any HW requests.
>
Yep, definitely. Didn't thought about security restrictions yet.
>> +/* 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, /* Set the DPLL device output type */
>
> This week, I am going to prepare the patch for DPLL mode and input priority list
> we have discussed on the previous patch series.
>
Great! Do you have this work somewhere in public git? If not I will try to
publish this branch somewhere to make collaboration easier.
> Thank you!
> 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
^ permalink raw reply [flat|nested] 36+ messages in thread* RE: [RFC PATCH v2 1/3] dpll: Add DPLL framework base functions
2022-07-14 23:23 ` Vadim Fedorenko
@ 2022-07-15 17:31 ` Kubalewski, Arkadiusz
0 siblings, 0 replies; 36+ messages in thread
From: Kubalewski, Arkadiusz @ 2022-07-15 17:31 UTC (permalink / raw)
To: Vadim Fedorenko, Jakub Kicinski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
-----Original Message-----
From: Vadim Fedorenko <vfedorenko@novek.ru>
Sent: Friday, July 15, 2022 1:24 AM
>
>On 11.07.2022 10:01, Kubalewski, Arkadiusz wrote:
>> -----Original Message-----
>> From: Vadim Fedorenko <vfedorenko@novek.ru>
>> Sent: Sunday, June 26, 2022 9:25 PM
>>>
>>> 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>
>>
>> Hi Vadim,
>> I've been trying to implement usage of this code in our driver.
>> Any chance for some testing/example app?
>>
>
>Hi Arkadiusz,
>Sorry, but I don't have any working app yet, this subsystem is
>treated as experimental and as we now have different interface
>for configuring features we need, app implementation is postponed
>a bit. After some conversation with Jakub I'm thinking about library
>to provide easy interface to this subsystem, but stil no code yet.
>
>>> +struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, int sources_count,
>>> + int outputs_count, void *priv)
>>
>> Aren't there some alignment issues around function definitions?
>>
>
>Yeah, I know about some style issues, trying to fix them for the next round.
>
>>> +{
>>> + 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);
>>
>> Not sure if I mentioned it before, the user must be able to identify the
>> purpose and origin of dpll. Right now, if 2 dplls register in the system, it is
>> not possible to determine where they belong or what do they do. I would say,
>> easiest to let caller of dpll_device_alloc assign some name or description.
>>
>
>Maybe driver can export information about dpll device name after registering it?
>But at the same time looks like it's easy enough to implement custom naming. The
>only problem is to control that names are unique.
Right now the name is unique, but its not persistent, so this also might be
hard for the user. Reloading drivers which implemented the dpll interface will
result in a userspace app config mess, as order of loading will impact their id
in dpll<id> format.
Maybe some additional string id in format like <pci_id>.<idx>?
As I understand whatever is registering a dpll, it should be some kind of
device, thus we could pass pointer to dev on alloc and ask it for device name
(dev_name(dev))?
The <idx> could be given also on alloc, the driver developer which would be
using dpll subsystem will take care of getting uniqe values for it, as it won't
be fully usable without it being unique.
>
>>> + 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_free(struct dpll_device *dpll)
>>> +{
>>> + if (!dpll)
>>> + return;
>>> +
>>> + mutex_destroy(&dpll->lock);
>>> + kfree(dpll);
>>> +}
>>
>> dpll_device_free() is defined in header, shouldn't it be exported?
>>
>
>yeah, definitely. Already changed in new draft.
>
>>> +
>>> +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));
>>
>> dpll_notify_device_create is not yet defined, this is part of patch 2/3?
>> Also in patch 2/3 similar call was added in dpll_device_alloc().
>>
>
>Ah. Yes, there was some mess with patches, looks like I missed this thing, thank
>you for pointing it.
>
>
>>> +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,
>>> + },
>>
>> I wouldn't leave non-privileged user with possibility to call any HW requests.
>>
>
>Yep, definitely. Didn't thought about security restrictions yet.
>
>
>>> +/* 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, /* Set the DPLL device output type */
>>
>> This week, I am going to prepare the patch for DPLL mode and input priority list
>> we have discussed on the previous patch series.
>>
>
>Great! Do you have this work somewhere in public git? If not I will try to
>publish this branch somewhere to make collaboration easier.
Not yet, but sure, please start the branch for collaboration.
Thanks,
Arkadiusz
>
>> Thank you!
>> 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC PATCH v2 2/3] dpll: add netlink events
2022-06-26 19:24 [RFC PATCH v2 0/3] Create common DPLL/clock configuration API Vadim Fedorenko
2022-06-26 19:24 ` [RFC PATCH v2 1/3] dpll: Add DPLL framework base functions Vadim Fedorenko
@ 2022-06-26 19:24 ` Vadim Fedorenko
2022-07-11 9:02 ` Kubalewski, Arkadiusz
` (2 more replies)
2022-06-26 19:24 ` [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops Vadim Fedorenko
` (2 subsequent siblings)
4 siblings, 3 replies; 36+ messages in thread
From: Vadim Fedorenko @ 2022-06-26 19:24 UTC (permalink / raw)
To: Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
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 dc0330e3687d..387644aa910e 100644
--- a/drivers/dpll/dpll_core.c
+++ b/drivers/dpll/dpll_core.c
@@ -97,6 +97,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 e15106f30377..4b1684fcf41e 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 {
@@ -239,6 +241,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;
}
@@ -262,6 +266,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;
}
@@ -438,6 +444,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
^ permalink raw reply related [flat|nested] 36+ messages in thread* RE: [RFC PATCH v2 2/3] dpll: add netlink events
2022-06-26 19:24 ` [RFC PATCH v2 2/3] dpll: add netlink events Vadim Fedorenko
@ 2022-07-11 9:02 ` Kubalewski, Arkadiusz
2022-07-14 23:29 ` Vadim Fedorenko
2022-08-03 15:21 ` Stephen Hemminger
2022-09-29 12:13 ` Jiri Pirko
2 siblings, 1 reply; 36+ messages in thread
From: Kubalewski, Arkadiusz @ 2022-07-11 9:02 UTC (permalink / raw)
To: Vadim Fedorenko, Jakub Kicinski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
-----Original Message-----
From: Vadim Fedorenko <vfedorenko@novek.ru>
Sent: Sunday, June 26, 2022 9:25 PM
>
>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 dc0330e3687d..387644aa910e 100644
>--- a/drivers/dpll/dpll_core.c
>+++ b/drivers/dpll/dpll_core.c
>@@ -97,6 +97,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 e15106f30377..4b1684fcf41e 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 {
>@@ -239,6 +241,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;
> }
>
>@@ -262,6 +266,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;
> }
>
>@@ -438,6 +444,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);
All multicasts are send only for group "1" (DPLL_CONFIG_SOURCE_GROUP_NAME),
but 4 groups were defined.
>+
>+ 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);
Only dpll_notify_device_create is actually used, rest is not.
I am getting confused a bit, who should call those "notify" functions?
It is straightforward for create/delete, dpll subsystem shall do it, but what
about the rest?
I would say notifications about status or source/output change shall originate
in the driver implementing dpll interface, thus they shall be exported and
defined in the header included by the driver.
>+
> 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
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 2/3] dpll: add netlink events
2022-07-11 9:02 ` Kubalewski, Arkadiusz
@ 2022-07-14 23:29 ` Vadim Fedorenko
2022-07-15 17:31 ` Kubalewski, Arkadiusz
0 siblings, 1 reply; 36+ messages in thread
From: Vadim Fedorenko @ 2022-07-14 23:29 UTC (permalink / raw)
To: Kubalewski, Arkadiusz, Jakub Kicinski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
On 11.07.2022 10:02, Kubalewski, Arkadiusz wrote:
> -----Original Message-----
> From: Vadim Fedorenko <vfedorenko@novek.ru>
> Sent: Sunday, June 26, 2022 9:25 PM
>>
>> 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 dc0330e3687d..387644aa910e 100644
>> --- a/drivers/dpll/dpll_core.c
>> +++ b/drivers/dpll/dpll_core.c
>> @@ -97,6 +97,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 e15106f30377..4b1684fcf41e 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 {
>> @@ -239,6 +241,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;
>> }
>>
>> @@ -262,6 +266,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;
>> }
>>
>> @@ -438,6 +444,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);
>
> All multicasts are send only for group "1" (DPLL_CONFIG_SOURCE_GROUP_NAME),
> but 4 groups were defined.
>
Yes, you are right! Will update it in the next round.
>> +
>> + 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);
>
> Only dpll_notify_device_create is actually used, rest is not.
> I am getting confused a bit, who should call those "notify" functions?
> It is straightforward for create/delete, dpll subsystem shall do it, but what
> about the rest?
> I would say notifications about status or source/output change shall originate
> in the driver implementing dpll interface, thus they shall be exported and
> defined in the header included by the driver.
>
I was thinking about driver too, because device can have different interfaces to
configure source/output, and different notifications to update status. I will
update ptp_ocp driver to implement this logic. And it will also cover question
of exporting these functions and their definitions.
>> +
>> 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
^ permalink raw reply [flat|nested] 36+ messages in thread* RE: [RFC PATCH v2 2/3] dpll: add netlink events
2022-07-14 23:29 ` Vadim Fedorenko
@ 2022-07-15 17:31 ` Kubalewski, Arkadiusz
2022-08-02 14:02 ` Kubalewski, Arkadiusz
0 siblings, 1 reply; 36+ messages in thread
From: Kubalewski, Arkadiusz @ 2022-07-15 17:31 UTC (permalink / raw)
To: Vadim Fedorenko, Jakub Kicinski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
-----Original Message-----
From: Vadim Fedorenko <vfedorenko@novek.ru>
Sent: Friday, July 15, 2022 1:29 AM
>
>On 11.07.2022 10:02, Kubalewski, Arkadiusz wrote:
>> -----Original Message-----
>> From: Vadim Fedorenko <vfedorenko@novek.ru>
>> Sent: Sunday, June 26, 2022 9:25 PM
>>>
>>> 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 dc0330e3687d..387644aa910e 100644
>>> --- a/drivers/dpll/dpll_core.c
>>> +++ b/drivers/dpll/dpll_core.c
>>> @@ -97,6 +97,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 e15106f30377..4b1684fcf41e 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 {
>>> @@ -239,6 +241,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;
>>> }
>>>
>>> @@ -262,6 +266,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;
>>> }
>>>
>>> @@ -438,6 +444,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);
>>
>> All multicasts are send only for group "1" (DPLL_CONFIG_SOURCE_GROUP_NAME),
>> but 4 groups were defined.
>>
>
>Yes, you are right! Will update it in the next round.
>
>>> +
>>> + 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);
>>
>> Only dpll_notify_device_create is actually used, rest is not.
>> I am getting confused a bit, who should call those "notify" functions?
>> It is straightforward for create/delete, dpll subsystem shall do it, but what
>> about the rest?
>> I would say notifications about status or source/output change shall originate
>> in the driver implementing dpll interface, thus they shall be exported and
>> defined in the header included by the driver.
>>
>
>I was thinking about driver too, because device can have different interfaces to
>configure source/output, and different notifications to update status. I will
>update ptp_ocp driver to implement this logic. And it will also cover question
>of exporting these functions and their definitions.
>
Great!
Thank,
Arkadiusz
>>> +
>>> 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
^ permalink raw reply [flat|nested] 36+ messages in thread* RE: [RFC PATCH v2 2/3] dpll: add netlink events
2022-07-15 17:31 ` Kubalewski, Arkadiusz
@ 2022-08-02 14:02 ` Kubalewski, Arkadiusz
2022-08-02 15:52 ` Jakub Kicinski
2022-08-03 0:05 ` Vadim Fedorenko
0 siblings, 2 replies; 36+ messages in thread
From: Kubalewski, Arkadiusz @ 2022-08-02 14:02 UTC (permalink / raw)
To: Vadim Fedorenko, Jakub Kicinski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
>-----Original Message-----
>From: Vadim Fedorenko <vfedorenko@novek.ru>
>Sent: Friday, July 15, 2022 1:29 AM
>>
>>On 11.07.2022 10:02, Kubalewski, Arkadiusz wrote:
>>> -----Original Message-----
>>> From: Vadim Fedorenko <vfedorenko@novek.ru>
>>> Sent: Sunday, June 26, 2022 9:25 PM
>>>>
>>>> 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 dc0330e3687d..387644aa910e 100644
>>>> --- a/drivers/dpll/dpll_core.c
>>>> +++ b/drivers/dpll/dpll_core.c
>>>> @@ -97,6 +97,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 e15106f30377..4b1684fcf41e 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 {
>>>> @@ -239,6 +241,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;
>>>> }
>>>>
>>>> @@ -262,6 +266,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;
>>>> }
>>>>
>>>> @@ -438,6 +444,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);
>>>
>>> All multicasts are send only for group "1" (DPLL_CONFIG_SOURCE_GROUP_NAME),
>>> but 4 groups were defined.
>>>
>>
>>Yes, you are right! Will update it in the next round.
>>
>>>> +
>>>> + 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);
>>>
>>> Only dpll_notify_device_create is actually used, rest is not.
>>> I am getting confused a bit, who should call those "notify" functions?
>>> It is straightforward for create/delete, dpll subsystem shall do it, but what
>>> about the rest?
>>> I would say notifications about status or source/output change shall originate
>>> in the driver implementing dpll interface, thus they shall be exported and
>>> defined in the header included by the driver.
>>>
>>
>>I was thinking about driver too, because device can have different interfaces to
>>configure source/output, and different notifications to update status. I will
>>update ptp_ocp driver to implement this logic. And it will also cover question
>>of exporting these functions and their definitions.
>>
>
>Great!
>
>Thank,
>Arkadiusz
>>>> +
>>>> int __init dpll_netlink_init(void);
>>>> void dpll_netlink_finish(void);
>>>> --
>>>> 2.27.0
>>>>
>>
>
Good day Vadim,
I just wanted to make sure I didn't miss anything through the spam filters or
something? We are still waiting for that github repo, or you were on
vacation/busy, right?
Thanks,
Arkadiusz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 2/3] dpll: add netlink events
2022-08-02 14:02 ` Kubalewski, Arkadiusz
@ 2022-08-02 15:52 ` Jakub Kicinski
2022-08-03 0:05 ` Vadim Fedorenko
1 sibling, 0 replies; 36+ messages in thread
From: Jakub Kicinski @ 2022-08-02 15:52 UTC (permalink / raw)
To: Kubalewski, Arkadiusz
Cc: Vadim Fedorenko, Jonathan Lemon, Vadim Fedorenko, Aya Levin,
netdev, linux-arm-kernel, linux-clk
On Tue, 2 Aug 2022 14:02:31 +0000 Kubalewski, Arkadiusz wrote:
> I just wanted to make sure I didn't miss anything through the spam filters or
> something? We are still waiting for that github repo, or you were on
> vacation/busy, right?
Great timing, I was just asking Vadim for the code as well.
I should be able to share the auto-generated user space library
for the netlink interface soon after Vadim shares the code.
Still very much WIP but, well, likely better than parsing by hand.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 2/3] dpll: add netlink events
2022-08-02 14:02 ` Kubalewski, Arkadiusz
2022-08-02 15:52 ` Jakub Kicinski
@ 2022-08-03 0:05 ` Vadim Fedorenko
1 sibling, 0 replies; 36+ messages in thread
From: Vadim Fedorenko @ 2022-08-03 0:05 UTC (permalink / raw)
To: Kubalewski, Arkadiusz, Jakub Kicinski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
On 02.08.2022 15:02, Kubalewski, Arkadiusz wrote:
>> -----Original Message-----
>> From: Vadim Fedorenko <vfedorenko@novek.ru>
>> Sent: Friday, July 15, 2022 1:29 AM
>>>
>>> On 11.07.2022 10:02, Kubalewski, Arkadiusz wrote:
>>>> -----Original Message-----
>>>> From: Vadim Fedorenko <vfedorenko@novek.ru>
>>>> Sent: Sunday, June 26, 2022 9:25 PM
>>>>>
>>>>> 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 dc0330e3687d..387644aa910e 100644
>>>>> --- a/drivers/dpll/dpll_core.c
>>>>> +++ b/drivers/dpll/dpll_core.c
>>>>> @@ -97,6 +97,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 e15106f30377..4b1684fcf41e 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 {
>>>>> @@ -239,6 +241,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;
>>>>> }
>>>>>
>>>>> @@ -262,6 +266,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;
>>>>> }
>>>>>
>>>>> @@ -438,6 +444,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);
>>>>
>>>> All multicasts are send only for group "1" (DPLL_CONFIG_SOURCE_GROUP_NAME),
>>>> but 4 groups were defined.
>>>>
>>>
>>> Yes, you are right! Will update it in the next round.
>>>
>>>>> +
>>>>> + 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);
>>>>
>>>> Only dpll_notify_device_create is actually used, rest is not.
>>>> I am getting confused a bit, who should call those "notify" functions?
>>>> It is straightforward for create/delete, dpll subsystem shall do it, but what
>>>> about the rest?
>>>> I would say notifications about status or source/output change shall originate
>>>> in the driver implementing dpll interface, thus they shall be exported and
>>>> defined in the header included by the driver.
>>>>
>>>
>>> I was thinking about driver too, because device can have different interfaces to
>>> configure source/output, and different notifications to update status. I will
>>> update ptp_ocp driver to implement this logic. And it will also cover question
>>> of exporting these functions and their definitions.
>>>
>>
>> Great!
>>
>> Thank,
>> Arkadiusz
>>>>> +
>>>>> int __init dpll_netlink_init(void);
>>>>> void dpll_netlink_finish(void);
>>>>> --
>>>>> 2.27.0
>>>>>
>>>
>>
>
> Good day Vadim,
>
> I just wanted to make sure I didn't miss anything through the spam filters or
> something? We are still waiting for that github repo, or you were on
> vacation/busy, right?
>
Hi Arkadiusz, Jakub
Actually I was on vacation which lasted unexpectedly long thanks for european
airlines. So was busy catching up all the things happened while I was away.
Finally I created github repo with all the comments from previous conversation
addressed and rebased on top of current net-next. No special progress apart from
that, still need some time to prepare RFC v3 with documentation and proper
driver usage, but current state should be usable for priorities implementation
and simple tests:
https://github.com/vvfedorenko/linux-dpll.git
Ping me on github to have a write access to this repo, and sorry for being so late.
All best,
Vadim
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 2/3] dpll: add netlink events
2022-06-26 19:24 ` [RFC PATCH v2 2/3] dpll: add netlink events Vadim Fedorenko
2022-07-11 9:02 ` Kubalewski, Arkadiusz
@ 2022-08-03 15:21 ` Stephen Hemminger
2022-09-29 12:13 ` Jiri Pirko
2 siblings, 0 replies; 36+ messages in thread
From: Stephen Hemminger @ 2022-08-03 15:21 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
On Sun, 26 Jun 2022 22:24:43 +0300
Vadim Fedorenko <vfedorenko@novek.ru> wrote:
> +
> +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,
> +};
Function tables in kernel should always be const for added security
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 2/3] dpll: add netlink events
2022-06-26 19:24 ` [RFC PATCH v2 2/3] dpll: add netlink events Vadim Fedorenko
2022-07-11 9:02 ` Kubalewski, Arkadiusz
2022-08-03 15:21 ` Stephen Hemminger
@ 2022-09-29 12:13 ` Jiri Pirko
2022-09-30 0:48 ` Vadim Fedorenko
2 siblings, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2022-09-29 12:13 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
Sun, Jun 26, 2022 at 09:24:43PM CEST, vfedorenko@novek.ru wrote:
>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.
I don't get why this is a separate patch. I believe it should be
squashed to the previous one, making it easier to review as a whole
thing.
>
>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 dc0330e3687d..387644aa910e 100644
>--- a/drivers/dpll/dpll_core.c
>+++ b/drivers/dpll/dpll_core.c
>@@ -97,6 +97,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 e15106f30377..4b1684fcf41e 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 {
>@@ -239,6 +241,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;
> }
>
>@@ -262,6 +266,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;
> }
>
>@@ -438,6 +444,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 param". Can't you please maintain some namespace for
function/struct names?
>+{
>+ 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);
Just do that automatically in register() and avoid the need to rely on
drivers to be so good to do it themselves. They won't.
>+}
>+
>+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)
For all notification functions called from the driver, please use struct
dpll as an arg.
>+{
>+ 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);
Why these are not returning void? Does driver care about the return
value? Why?
>+
> 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
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 2/3] dpll: add netlink events
2022-09-29 12:13 ` Jiri Pirko
@ 2022-09-30 0:48 ` Vadim Fedorenko
0 siblings, 0 replies; 36+ messages in thread
From: Vadim Fedorenko @ 2022-09-30 0:48 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
On 29.09.2022 13:13, Jiri Pirko wrote:
> Sun, Jun 26, 2022 at 09:24:43PM CEST, vfedorenko@novek.ru wrote:
>> 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.
>
> I don't get why this is a separate patch. I believe it should be
> squashed to the previous one, making it easier to review as a whole
> thing.
>
I was trying to separate some functions to make review process a bit simplier.
>
>>
>> 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 dc0330e3687d..387644aa910e 100644
>> --- a/drivers/dpll/dpll_core.c
>> +++ b/drivers/dpll/dpll_core.c
>> @@ -97,6 +97,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 e15106f30377..4b1684fcf41e 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 {
>> @@ -239,6 +241,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;
>> }
>>
>> @@ -262,6 +266,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;
>> }
>>
>> @@ -438,6 +444,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 param". Can't you please maintain some namespace for
> function/struct names?
>
Ok, sure!
>
>> +{
>> + 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);
>
> Just do that automatically in register() and avoid the need to rely on
> drivers to be so good to do it themselves. They won't.
>
Yeah, the next version will have these changes.
>
>> +}
>> +
>> +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)
>
> For all notification functions called from the driver, please use struct
> dpll as an arg.
Will change it, thanks!
>
>> +{
>> + 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);
>
> Why these are not returning void? Does driver care about the return
> value? Why?
>
Yep, you are right, I will change to void because there is no reason to have a
return value, thanks!
>
>> +
>> 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops
2022-06-26 19:24 [RFC PATCH v2 0/3] Create common DPLL/clock configuration API Vadim Fedorenko
2022-06-26 19:24 ` [RFC PATCH v2 1/3] dpll: Add DPLL framework base functions Vadim Fedorenko
2022-06-26 19:24 ` [RFC PATCH v2 2/3] dpll: add netlink events Vadim Fedorenko
@ 2022-06-26 19:24 ` Vadim Fedorenko
2022-06-27 19:34 ` Jonathan Lemon
2022-09-29 11:33 ` Jiri Pirko
2022-09-01 12:02 ` [RFC PATCH v2 0/3] Create common DPLL/clock configuration API Gal Pressman
2022-09-29 11:40 ` Jiri Pirko
4 siblings, 2 replies; 36+ messages in thread
From: Vadim Fedorenko @ 2022-06-26 19:24 UTC (permalink / raw)
To: Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
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 | 169 ++++++++++++++++++++++++++++++--------
include/uapi/linux/dpll.h | 2 +
3 files changed, 136 insertions(+), 36 deletions(-)
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..f830625a63a3 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)
@@ -660,18 +662,19 @@ static DEFINE_IDR(ptp_ocp_idr);
struct ocp_selector {
const char *name;
int value;
+ int dpll_type;
};
static const struct ocp_selector ptp_ocp_clock[] = {
- { .name = "NONE", .value = 0 },
- { .name = "TOD", .value = 1 },
- { .name = "IRIG", .value = 2 },
- { .name = "PPS", .value = 3 },
- { .name = "PTP", .value = 4 },
- { .name = "RTC", .value = 5 },
- { .name = "DCF", .value = 6 },
- { .name = "REGS", .value = 0xfe },
- { .name = "EXT", .value = 0xff },
+ { .name = "NONE", .value = 0, .dpll_type = 0 },
+ { .name = "TOD", .value = 1, .dpll_type = 0 },
+ { .name = "IRIG", .value = 2, .dpll_type = 0 },
+ { .name = "PPS", .value = 3, .dpll_type = 0 },
+ { .name = "PTP", .value = 4, .dpll_type = 0 },
+ { .name = "RTC", .value = 5, .dpll_type = 0 },
+ { .name = "DCF", .value = 6, .dpll_type = 0 },
+ { .name = "REGS", .value = 0xfe, .dpll_type = 0 },
+ { .name = "EXT", .value = 0xff, .dpll_type = 0 },
{ }
};
@@ -680,37 +683,37 @@ static const struct ocp_selector ptp_ocp_clock[] = {
#define SMA_SELECT_MASK GENMASK(14, 0)
static const struct ocp_selector ptp_ocp_sma_in[] = {
- { .name = "10Mhz", .value = 0x0000 },
- { .name = "PPS1", .value = 0x0001 },
- { .name = "PPS2", .value = 0x0002 },
- { .name = "TS1", .value = 0x0004 },
- { .name = "TS2", .value = 0x0008 },
- { .name = "IRIG", .value = 0x0010 },
- { .name = "DCF", .value = 0x0020 },
- { .name = "TS3", .value = 0x0040 },
- { .name = "TS4", .value = 0x0080 },
- { .name = "FREQ1", .value = 0x0100 },
- { .name = "FREQ2", .value = 0x0200 },
- { .name = "FREQ3", .value = 0x0400 },
- { .name = "FREQ4", .value = 0x0800 },
- { .name = "None", .value = SMA_DISABLE },
+ { .name = "10Mhz", .value = 0x0000, .dpll_type = DPLL_TYPE_EXT_10MHZ },
+ { .name = "PPS1", .value = 0x0001, .dpll_type = DPLL_TYPE_EXT_1PPS },
+ { .name = "PPS2", .value = 0x0002, .dpll_type = DPLL_TYPE_EXT_1PPS },
+ { .name = "TS1", .value = 0x0004, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "TS2", .value = 0x0008, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "TS3", .value = 0x0040, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "TS4", .value = 0x0080, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "FREQ1", .value = 0x0100, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "FREQ2", .value = 0x0200, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "FREQ3", .value = 0x0400, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "FREQ4", .value = 0x0800, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "None", .value = SMA_DISABLE, .dpll_type = DPLL_TYPE_NONE },
{ }
};
static const struct ocp_selector ptp_ocp_sma_out[] = {
- { .name = "10Mhz", .value = 0x0000 },
- { .name = "PHC", .value = 0x0001 },
- { .name = "MAC", .value = 0x0002 },
- { .name = "GNSS1", .value = 0x0004 },
- { .name = "GNSS2", .value = 0x0008 },
- { .name = "IRIG", .value = 0x0010 },
- { .name = "DCF", .value = 0x0020 },
- { .name = "GEN1", .value = 0x0040 },
- { .name = "GEN2", .value = 0x0080 },
- { .name = "GEN3", .value = 0x0100 },
- { .name = "GEN4", .value = 0x0200 },
- { .name = "GND", .value = 0x2000 },
- { .name = "VCC", .value = 0x4000 },
+ { .name = "10Mhz", .value = 0x0000, .dpll_type = DPLL_TYPE_EXT_10MHZ },
+ { .name = "PHC", .value = 0x0001, .dpll_type = DPLL_TYPE_INT_OSCILLATOR },
+ { .name = "MAC", .value = 0x0002, .dpll_type = DPLL_TYPE_INT_OSCILLATOR },
+ { .name = "GNSS1", .value = 0x0004, .dpll_type = DPLL_TYPE_GNSS },
+ { .name = "GNSS2", .value = 0x0008, .dpll_type = DPLL_TYPE_GNSS },
+ { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "GEN1", .value = 0x0040, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "GEN2", .value = 0x0080, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "GEN3", .value = 0x0100, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "GEN4", .value = 0x0200, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "GND", .value = 0x2000, .dpll_type = DPLL_TYPE_CUSTOM },
+ { .name = "VCC", .value = 0x4000, .dpll_type = DPLL_TYPE_CUSTOM },
{ }
};
@@ -3713,6 +3716,90 @@ 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_sma_get_dpll_type(struct ptp_ocp *bp, int sma_nr)
+{
+ struct ocp_selector *tbl;
+ u32 val;
+
+ if (bp->sma[sma_nr].mode == SMA_MODE_IN)
+ tbl = bp->sma_op->tbl[0];
+ else
+ tbl = bp->sma_op->tbl[1];
+
+ val = ptp_ocp_sma_get(bp, sma_nr);
+ return tbl[val].dpll_type;
+}
+
+static int ptp_ocp_dpll_type_supported(struct dpll_device *dpll, int sma, int type, int dir)
+{
+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+ struct ocp_selector *tbl = bp->sma_op->tbl[dir];
+ int i;
+
+ for (i = 0; i < sizeof(*tbl); i++) {
+ if (tbl[i].dpll_type == type)
+ return 1;
+ }
+ return 0;
+}
+
+static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
+{
+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+
+ if (bp->sma[sma].mode != SMA_MODE_IN)
+ return -1;
+
+ return ptp_ocp_sma_get_dpll_type(bp, sma);
+}
+
+static int ptp_ocp_dpll_get_source_supported(struct dpll_device *dpll, int sma, int type)
+{
+ return ptp_ocp_dpll_type_supported(dpll, sma, type, 0);
+}
+
+static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
+{
+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+
+ if (bp->sma[sma].mode != SMA_MODE_OUT)
+ return -1;
+
+ return ptp_ocp_sma_get_dpll_type(bp, sma);
+}
+
+static int ptp_ocp_dpll_get_output_supported(struct dpll_device *dpll, int sma, int type)
+{
+ return ptp_ocp_dpll_type_supported(dpll, sma, type, 1);
+}
+
+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_source_supported = ptp_ocp_dpll_get_source_supported,
+ .get_output_type = ptp_ocp_dpll_get_output_type,
+ .get_output_supported = ptp_ocp_dpll_get_output_supported,
+};
+
static int
ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
@@ -3768,6 +3855,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:
@@ -3785,6 +3880,8 @@ ptp_ocp_remove(struct pci_dev *pdev)
struct ptp_ocp *bp = pci_get_drvdata(pdev);
struct devlink *devlink = priv_to_devlink(bp);
+ dpll_device_unregister(bp->dpll);
+ dpll_device_free(bp->dpll);
devlink_unregister(devlink);
ptp_ocp_detach(bp);
pci_disable_device(pdev);
diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
index 7ce45c6b4fd4..5e8c46712d18 100644
--- a/include/uapi/linux/dpll.h
+++ b/include/uapi/linux/dpll.h
@@ -41,11 +41,13 @@ enum dpll_genl_attr {
/* DPLL signal types used as source or as output */
enum dpll_genl_signal_type {
+ DPLL_TYPE_NONE,
DPLL_TYPE_EXT_1PPS,
DPLL_TYPE_EXT_10MHZ,
DPLL_TYPE_SYNCE_ETH_PORT,
DPLL_TYPE_INT_OSCILLATOR,
DPLL_TYPE_GNSS,
+ DPLL_TYPE_CUSTOM,
__DPLL_TYPE_MAX,
};
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops
2022-06-26 19:24 ` [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops Vadim Fedorenko
@ 2022-06-27 19:34 ` Jonathan Lemon
2022-06-27 22:13 ` Vadim Fedorenko
2022-09-29 11:33 ` Jiri Pirko
1 sibling, 1 reply; 36+ messages in thread
From: Jonathan Lemon @ 2022-06-27 19:34 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Jakub Kicinski, Arkadiusz Kubalewski, Vadim Fedorenko, Aya Levin,
netdev, linux-arm-kernel, linux-clk
On Sun, Jun 26, 2022 at 10:24:44PM +0300, Vadim Fedorenko wrote:
> 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 | 169 ++++++++++++++++++++++++++++++--------
> include/uapi/linux/dpll.h | 2 +
> 3 files changed, 136 insertions(+), 36 deletions(-)
>
> 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..f830625a63a3 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)
> @@ -660,18 +662,19 @@ static DEFINE_IDR(ptp_ocp_idr);
> struct ocp_selector {
> const char *name;
> int value;
> + int dpll_type;
> };
>
> static const struct ocp_selector ptp_ocp_clock[] = {
> - { .name = "NONE", .value = 0 },
> - { .name = "TOD", .value = 1 },
> - { .name = "IRIG", .value = 2 },
> - { .name = "PPS", .value = 3 },
> - { .name = "PTP", .value = 4 },
> - { .name = "RTC", .value = 5 },
> - { .name = "DCF", .value = 6 },
> - { .name = "REGS", .value = 0xfe },
> - { .name = "EXT", .value = 0xff },
> + { .name = "NONE", .value = 0, .dpll_type = 0 },
> + { .name = "TOD", .value = 1, .dpll_type = 0 },
> + { .name = "IRIG", .value = 2, .dpll_type = 0 },
> + { .name = "PPS", .value = 3, .dpll_type = 0 },
> + { .name = "PTP", .value = 4, .dpll_type = 0 },
> + { .name = "RTC", .value = 5, .dpll_type = 0 },
> + { .name = "DCF", .value = 6, .dpll_type = 0 },
> + { .name = "REGS", .value = 0xfe, .dpll_type = 0 },
> + { .name = "EXT", .value = 0xff, .dpll_type = 0 },
No need for zeros, or are they just temp stubs?
> @@ -680,37 +683,37 @@ static const struct ocp_selector ptp_ocp_clock[] = {
> #define SMA_SELECT_MASK GENMASK(14, 0)
>
> static const struct ocp_selector ptp_ocp_sma_in[] = {
> - { .name = "10Mhz", .value = 0x0000 },
> - { .name = "PPS1", .value = 0x0001 },
> - { .name = "PPS2", .value = 0x0002 },
> - { .name = "TS1", .value = 0x0004 },
> - { .name = "TS2", .value = 0x0008 },
> - { .name = "IRIG", .value = 0x0010 },
> - { .name = "DCF", .value = 0x0020 },
> - { .name = "TS3", .value = 0x0040 },
> - { .name = "TS4", .value = 0x0080 },
> - { .name = "FREQ1", .value = 0x0100 },
> - { .name = "FREQ2", .value = 0x0200 },
> - { .name = "FREQ3", .value = 0x0400 },
> - { .name = "FREQ4", .value = 0x0800 },
> - { .name = "None", .value = SMA_DISABLE },
> + { .name = "10Mhz", .value = 0x0000, .dpll_type = DPLL_TYPE_EXT_10MHZ },
> + { .name = "PPS1", .value = 0x0001, .dpll_type = DPLL_TYPE_EXT_1PPS },
> + { .name = "PPS2", .value = 0x0002, .dpll_type = DPLL_TYPE_EXT_1PPS },
> + { .name = "TS1", .value = 0x0004, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "TS2", .value = 0x0008, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "TS3", .value = 0x0040, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "TS4", .value = 0x0080, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "FREQ1", .value = 0x0100, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "FREQ2", .value = 0x0200, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "FREQ3", .value = 0x0400, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "FREQ4", .value = 0x0800, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "None", .value = SMA_DISABLE, .dpll_type = DPLL_TYPE_NONE },
80-column limit (here and throughout the file)
> static const struct ocp_selector ptp_ocp_sma_out[] = {
> - { .name = "10Mhz", .value = 0x0000 },
> - { .name = "PHC", .value = 0x0001 },
> - { .name = "MAC", .value = 0x0002 },
> - { .name = "GNSS1", .value = 0x0004 },
> - { .name = "GNSS2", .value = 0x0008 },
> - { .name = "IRIG", .value = 0x0010 },
> - { .name = "DCF", .value = 0x0020 },
> - { .name = "GEN1", .value = 0x0040 },
> - { .name = "GEN2", .value = 0x0080 },
> - { .name = "GEN3", .value = 0x0100 },
> - { .name = "GEN4", .value = 0x0200 },
> - { .name = "GND", .value = 0x2000 },
> - { .name = "VCC", .value = 0x4000 },
> + { .name = "10Mhz", .value = 0x0000, .dpll_type = DPLL_TYPE_EXT_10MHZ },
> + { .name = "PHC", .value = 0x0001, .dpll_type = DPLL_TYPE_INT_OSCILLATOR },
> + { .name = "MAC", .value = 0x0002, .dpll_type = DPLL_TYPE_INT_OSCILLATOR },
How does this work? The DPLL types seem somewhat restrictive here.
> + { .name = "GNSS1", .value = 0x0004, .dpll_type = DPLL_TYPE_GNSS },
> + { .name = "GNSS2", .value = 0x0008, .dpll_type = DPLL_TYPE_GNSS },
> + { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "GEN1", .value = 0x0040, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "GEN2", .value = 0x0080, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "GEN3", .value = 0x0100, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "GEN4", .value = 0x0200, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "GND", .value = 0x2000, .dpll_type = DPLL_TYPE_CUSTOM },
> + { .name = "VCC", .value = 0x4000, .dpll_type = DPLL_TYPE_CUSTOM },
80-columns
>
> @@ -3713,6 +3716,90 @@ 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_sma_get_dpll_type(struct ptp_ocp *bp, int sma_nr)
> +{
> + struct ocp_selector *tbl;
> + u32 val;
> +
> + if (bp->sma[sma_nr].mode == SMA_MODE_IN)
> + tbl = bp->sma_op->tbl[0];
> + else
> + tbl = bp->sma_op->tbl[1];
> +
> + val = ptp_ocp_sma_get(bp, sma_nr);
> + return tbl[val].dpll_type;
> +}
> +
> +static int ptp_ocp_dpll_type_supported(struct dpll_device *dpll, int sma, int type, int dir)
> +{
> + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> + struct ocp_selector *tbl = bp->sma_op->tbl[dir];
> + int i;
> +
> + for (i = 0; i < sizeof(*tbl); i++) {
> + if (tbl[i].dpll_type == type)
> + return 1;
> + }
> + return 0;
> +}
> +
> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
> +{
> + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +
> + if (bp->sma[sma].mode != SMA_MODE_IN)
> + return -1;
> +
> + return ptp_ocp_sma_get_dpll_type(bp, sma);
> +}
> +
> +static int ptp_ocp_dpll_get_source_supported(struct dpll_device *dpll, int sma, int type)
> +{
> + return ptp_ocp_dpll_type_supported(dpll, sma, type, 0);
> +}
> +
> +static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
> +{
> + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +
> + if (bp->sma[sma].mode != SMA_MODE_OUT)
> + return -1;
> +
> + return ptp_ocp_sma_get_dpll_type(bp, sma);
> +}
> +
> +static int ptp_ocp_dpll_get_output_supported(struct dpll_device *dpll, int sma, int type)
> +{
> + return ptp_ocp_dpll_type_supported(dpll, sma, type, 1);
> +}
> +
> +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_source_supported = ptp_ocp_dpll_get_source_supported,
> + .get_output_type = ptp_ocp_dpll_get_output_type,
> + .get_output_supported = ptp_ocp_dpll_get_output_supported,
> +};
> +
> static int
> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> @@ -3768,6 +3855,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;
80 cols, and this should be done before ptp_ocp_complete()
Also, should 'goto out', not return 0 and leak resources.
>
> out:
> @@ -3785,6 +3880,8 @@ ptp_ocp_remove(struct pci_dev *pdev)
> struct ptp_ocp *bp = pci_get_drvdata(pdev);
> struct devlink *devlink = priv_to_devlink(bp);
>
> + dpll_device_unregister(bp->dpll);
> + dpll_device_free(bp->dpll);
> devlink_unregister(devlink);
> ptp_ocp_detach(bp);
> pci_disable_device(pdev);
This should be in ptp_ocp_detach() to handle probe failures.
> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
> index 7ce45c6b4fd4..5e8c46712d18 100644
> --- a/include/uapi/linux/dpll.h
> +++ b/include/uapi/linux/dpll.h
> @@ -41,11 +41,13 @@ enum dpll_genl_attr {
>
> /* DPLL signal types used as source or as output */
> enum dpll_genl_signal_type {
> + DPLL_TYPE_NONE,
> DPLL_TYPE_EXT_1PPS,
> DPLL_TYPE_EXT_10MHZ,
> DPLL_TYPE_SYNCE_ETH_PORT,
> DPLL_TYPE_INT_OSCILLATOR,
> DPLL_TYPE_GNSS,
> + DPLL_TYPE_CUSTOM,
>
> __DPLL_TYPE_MAX,
> };
> --
> 2.27.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops
2022-06-27 19:34 ` Jonathan Lemon
@ 2022-06-27 22:13 ` Vadim Fedorenko
2022-06-28 19:11 ` Jonathan Lemon
0 siblings, 1 reply; 36+ messages in thread
From: Vadim Fedorenko @ 2022-06-27 22:13 UTC (permalink / raw)
To: Jonathan Lemon
Cc: Jakub Kicinski, Arkadiusz Kubalewski, Vadim Fedorenko, Aya Levin,
netdev, linux-arm-kernel, linux-clk
On 27.06.2022 20:34, Jonathan Lemon wrote:
> On Sun, Jun 26, 2022 at 10:24:44PM +0300, Vadim Fedorenko wrote:
>> 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 | 169 ++++++++++++++++++++++++++++++--------
>> include/uapi/linux/dpll.h | 2 +
>> 3 files changed, 136 insertions(+), 36 deletions(-)
>>
>> 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..f830625a63a3 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)
>> @@ -660,18 +662,19 @@ static DEFINE_IDR(ptp_ocp_idr);
>> struct ocp_selector {
>> const char *name;
>> int value;
>> + int dpll_type;
>> };
>>
>> static const struct ocp_selector ptp_ocp_clock[] = {
>> - { .name = "NONE", .value = 0 },
>> - { .name = "TOD", .value = 1 },
>> - { .name = "IRIG", .value = 2 },
>> - { .name = "PPS", .value = 3 },
>> - { .name = "PTP", .value = 4 },
>> - { .name = "RTC", .value = 5 },
>> - { .name = "DCF", .value = 6 },
>> - { .name = "REGS", .value = 0xfe },
>> - { .name = "EXT", .value = 0xff },
>> + { .name = "NONE", .value = 0, .dpll_type = 0 },
>> + { .name = "TOD", .value = 1, .dpll_type = 0 },
>> + { .name = "IRIG", .value = 2, .dpll_type = 0 },
>> + { .name = "PPS", .value = 3, .dpll_type = 0 },
>> + { .name = "PTP", .value = 4, .dpll_type = 0 },
>> + { .name = "RTC", .value = 5, .dpll_type = 0 },
>> + { .name = "DCF", .value = 6, .dpll_type = 0 },
>> + { .name = "REGS", .value = 0xfe, .dpll_type = 0 },
>> + { .name = "EXT", .value = 0xff, .dpll_type = 0 },
>
> No need for zeros, or are they just temp stubs?
These are just temp stubs for now.
>
>> @@ -680,37 +683,37 @@ static const struct ocp_selector ptp_ocp_clock[] = {
>> #define SMA_SELECT_MASK GENMASK(14, 0)
>>
>> static const struct ocp_selector ptp_ocp_sma_in[] = {
>> - { .name = "10Mhz", .value = 0x0000 },
>> - { .name = "PPS1", .value = 0x0001 },
>> - { .name = "PPS2", .value = 0x0002 },
>> - { .name = "TS1", .value = 0x0004 },
>> - { .name = "TS2", .value = 0x0008 },
>> - { .name = "IRIG", .value = 0x0010 },
>> - { .name = "DCF", .value = 0x0020 },
>> - { .name = "TS3", .value = 0x0040 },
>> - { .name = "TS4", .value = 0x0080 },
>> - { .name = "FREQ1", .value = 0x0100 },
>> - { .name = "FREQ2", .value = 0x0200 },
>> - { .name = "FREQ3", .value = 0x0400 },
>> - { .name = "FREQ4", .value = 0x0800 },
>> - { .name = "None", .value = SMA_DISABLE },
>> + { .name = "10Mhz", .value = 0x0000, .dpll_type = DPLL_TYPE_EXT_10MHZ },
>> + { .name = "PPS1", .value = 0x0001, .dpll_type = DPLL_TYPE_EXT_1PPS },
>> + { .name = "PPS2", .value = 0x0002, .dpll_type = DPLL_TYPE_EXT_1PPS },
>> + { .name = "TS1", .value = 0x0004, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "TS2", .value = 0x0008, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "TS3", .value = 0x0040, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "TS4", .value = 0x0080, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "FREQ1", .value = 0x0100, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "FREQ2", .value = 0x0200, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "FREQ3", .value = 0x0400, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "FREQ4", .value = 0x0800, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "None", .value = SMA_DISABLE, .dpll_type = DPLL_TYPE_NONE },
>
> 80-column limit (here and throughout the file)
I thought this rule was relaxed up to 100-columns?
>
>
>> static const struct ocp_selector ptp_ocp_sma_out[] = {
>> - { .name = "10Mhz", .value = 0x0000 },
>> - { .name = "PHC", .value = 0x0001 },
>> - { .name = "MAC", .value = 0x0002 },
>> - { .name = "GNSS1", .value = 0x0004 },
>> - { .name = "GNSS2", .value = 0x0008 },
>> - { .name = "IRIG", .value = 0x0010 },
>> - { .name = "DCF", .value = 0x0020 },
>> - { .name = "GEN1", .value = 0x0040 },
>> - { .name = "GEN2", .value = 0x0080 },
>> - { .name = "GEN3", .value = 0x0100 },
>> - { .name = "GEN4", .value = 0x0200 },
>> - { .name = "GND", .value = 0x2000 },
>> - { .name = "VCC", .value = 0x4000 },
>> + { .name = "10Mhz", .value = 0x0000, .dpll_type = DPLL_TYPE_EXT_10MHZ },
>> + { .name = "PHC", .value = 0x0001, .dpll_type = DPLL_TYPE_INT_OSCILLATOR },
>> + { .name = "MAC", .value = 0x0002, .dpll_type = DPLL_TYPE_INT_OSCILLATOR },
>
> How does this work? The DPLL types seem somewhat restrictive here.
Well, this is more like stubs, but a bit informative. I think I have to
implement additional attribute to address specifics like second GNSS receiver
and different oscillators (PHC/MAC) as well as other configurable values.
>
>
>> + { .name = "GNSS1", .value = 0x0004, .dpll_type = DPLL_TYPE_GNSS },
>> + { .name = "GNSS2", .value = 0x0008, .dpll_type = DPLL_TYPE_GNSS },
>> + { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "GEN1", .value = 0x0040, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "GEN2", .value = 0x0080, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "GEN3", .value = 0x0100, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "GEN4", .value = 0x0200, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "GND", .value = 0x2000, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "VCC", .value = 0x4000, .dpll_type = DPLL_TYPE_CUSTOM },
>
> 80-columns
>
>
>>
>> @@ -3713,6 +3716,90 @@ 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_sma_get_dpll_type(struct ptp_ocp *bp, int sma_nr)
>> +{
>> + struct ocp_selector *tbl;
>> + u32 val;
>> +
>> + if (bp->sma[sma_nr].mode == SMA_MODE_IN)
>> + tbl = bp->sma_op->tbl[0];
>> + else
>> + tbl = bp->sma_op->tbl[1];
>> +
>> + val = ptp_ocp_sma_get(bp, sma_nr);
>> + return tbl[val].dpll_type;
>> +}
>> +
>> +static int ptp_ocp_dpll_type_supported(struct dpll_device *dpll, int sma, int type, int dir)
>> +{
>> + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> + struct ocp_selector *tbl = bp->sma_op->tbl[dir];
>> + int i;
>> +
>> + for (i = 0; i < sizeof(*tbl); i++) {
>> + if (tbl[i].dpll_type == type)
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +
>> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
>> +{
>> + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +
>> + if (bp->sma[sma].mode != SMA_MODE_IN)
>> + return -1;
>> +
>> + return ptp_ocp_sma_get_dpll_type(bp, sma);
>> +}
>> +
>> +static int ptp_ocp_dpll_get_source_supported(struct dpll_device *dpll, int sma, int type)
>> +{
>> + return ptp_ocp_dpll_type_supported(dpll, sma, type, 0);
>> +}
>> +
>> +static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
>> +{
>> + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +
>> + if (bp->sma[sma].mode != SMA_MODE_OUT)
>> + return -1;
>> +
>> + return ptp_ocp_sma_get_dpll_type(bp, sma);
>> +}
>> +
>> +static int ptp_ocp_dpll_get_output_supported(struct dpll_device *dpll, int sma, int type)
>> +{
>> + return ptp_ocp_dpll_type_supported(dpll, sma, type, 1);
>> +}
>> +
>> +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_source_supported = ptp_ocp_dpll_get_source_supported,
>> + .get_output_type = ptp_ocp_dpll_get_output_type,
>> + .get_output_supported = ptp_ocp_dpll_get_output_supported,
>> +};
>> +
>> static int
>> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> {
>> @@ -3768,6 +3855,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;
>
> 80 cols, and this should be done before ptp_ocp_complete()
> Also, should 'goto out', not return 0 and leak resources.
I don't think we have to go with error path. Driver itself can work without DPLL
device registered, there is no hard dependency. The DPLL device will not be
registered and HW could not be configured/monitored via netlink, but could still
be usable.
>
>
>>
>> out:
>> @@ -3785,6 +3880,8 @@ ptp_ocp_remove(struct pci_dev *pdev)
>> struct ptp_ocp *bp = pci_get_drvdata(pdev);
>> struct devlink *devlink = priv_to_devlink(bp);
>>
>> + dpll_device_unregister(bp->dpll);
>> + dpll_device_free(bp->dpll);
>> devlink_unregister(devlink);
>> ptp_ocp_detach(bp);
>> pci_disable_device(pdev);
>
> This should be in ptp_ocp_detach() to handle probe failures.
Will move it in the next version, thanks!
>
>
>> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>> index 7ce45c6b4fd4..5e8c46712d18 100644
>> --- a/include/uapi/linux/dpll.h
>> +++ b/include/uapi/linux/dpll.h
>> @@ -41,11 +41,13 @@ enum dpll_genl_attr {
>>
>> /* DPLL signal types used as source or as output */
>> enum dpll_genl_signal_type {
>> + DPLL_TYPE_NONE,
>> DPLL_TYPE_EXT_1PPS,
>> DPLL_TYPE_EXT_10MHZ,
>> DPLL_TYPE_SYNCE_ETH_PORT,
>> DPLL_TYPE_INT_OSCILLATOR,
>> DPLL_TYPE_GNSS,
>> + DPLL_TYPE_CUSTOM,
>>
>> __DPLL_TYPE_MAX,
>> };
>> --
>> 2.27.0
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops
2022-06-27 22:13 ` Vadim Fedorenko
@ 2022-06-28 19:11 ` Jonathan Lemon
2022-06-29 3:24 ` Jakub Kicinski
0 siblings, 1 reply; 36+ messages in thread
From: Jonathan Lemon @ 2022-06-28 19:11 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Jakub Kicinski, Arkadiusz Kubalewski, Vadim Fedorenko, Aya Levin,
netdev, linux-arm-kernel, linux-clk
On Mon, Jun 27, 2022 at 11:13:22PM +0100, Vadim Fedorenko wrote:
> On 27.06.2022 20:34, Jonathan Lemon wrote:
> > On Sun, Jun 26, 2022 at 10:24:44PM +0300, Vadim Fedorenko wrote:
> > > 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 | 169 ++++++++++++++++++++++++++++++--------
> > > include/uapi/linux/dpll.h | 2 +
> > > 3 files changed, 136 insertions(+), 36 deletions(-)
> > >
> > > 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..f830625a63a3 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)
> > > @@ -660,18 +662,19 @@ static DEFINE_IDR(ptp_ocp_idr);
> > > struct ocp_selector {
> > > const char *name;
> > > int value;
> > > + int dpll_type;
> > > };
> > > static const struct ocp_selector ptp_ocp_clock[] = {
> > > - { .name = "NONE", .value = 0 },
> > > - { .name = "TOD", .value = 1 },
> > > - { .name = "IRIG", .value = 2 },
> > > - { .name = "PPS", .value = 3 },
> > > - { .name = "PTP", .value = 4 },
> > > - { .name = "RTC", .value = 5 },
> > > - { .name = "DCF", .value = 6 },
> > > - { .name = "REGS", .value = 0xfe },
> > > - { .name = "EXT", .value = 0xff },
> > > + { .name = "NONE", .value = 0, .dpll_type = 0 },
> > > + { .name = "TOD", .value = 1, .dpll_type = 0 },
> > > + { .name = "IRIG", .value = 2, .dpll_type = 0 },
> > > + { .name = "PPS", .value = 3, .dpll_type = 0 },
> > > + { .name = "PTP", .value = 4, .dpll_type = 0 },
> > > + { .name = "RTC", .value = 5, .dpll_type = 0 },
> > > + { .name = "DCF", .value = 6, .dpll_type = 0 },
> > > + { .name = "REGS", .value = 0xfe, .dpll_type = 0 },
> > > + { .name = "EXT", .value = 0xff, .dpll_type = 0 },
> >
> > No need for zeros, or are they just temp stubs?
>
> These are just temp stubs for now.
>
> >
> > > @@ -680,37 +683,37 @@ static const struct ocp_selector ptp_ocp_clock[] = {
> > > #define SMA_SELECT_MASK GENMASK(14, 0)
> > > static const struct ocp_selector ptp_ocp_sma_in[] = {
> > > - { .name = "10Mhz", .value = 0x0000 },
> > > - { .name = "PPS1", .value = 0x0001 },
> > > - { .name = "PPS2", .value = 0x0002 },
> > > - { .name = "TS1", .value = 0x0004 },
> > > - { .name = "TS2", .value = 0x0008 },
> > > - { .name = "IRIG", .value = 0x0010 },
> > > - { .name = "DCF", .value = 0x0020 },
> > > - { .name = "TS3", .value = 0x0040 },
> > > - { .name = "TS4", .value = 0x0080 },
> > > - { .name = "FREQ1", .value = 0x0100 },
> > > - { .name = "FREQ2", .value = 0x0200 },
> > > - { .name = "FREQ3", .value = 0x0400 },
> > > - { .name = "FREQ4", .value = 0x0800 },
> > > - { .name = "None", .value = SMA_DISABLE },
> > > + { .name = "10Mhz", .value = 0x0000, .dpll_type = DPLL_TYPE_EXT_10MHZ },
> > > + { .name = "PPS1", .value = 0x0001, .dpll_type = DPLL_TYPE_EXT_1PPS },
> > > + { .name = "PPS2", .value = 0x0002, .dpll_type = DPLL_TYPE_EXT_1PPS },
> > > + { .name = "TS1", .value = 0x0004, .dpll_type = DPLL_TYPE_CUSTOM },
> > > + { .name = "TS2", .value = 0x0008, .dpll_type = DPLL_TYPE_CUSTOM },
> > > + { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_TYPE_CUSTOM },
> > > + { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_TYPE_CUSTOM },
> > > + { .name = "TS3", .value = 0x0040, .dpll_type = DPLL_TYPE_CUSTOM },
> > > + { .name = "TS4", .value = 0x0080, .dpll_type = DPLL_TYPE_CUSTOM },
> > > + { .name = "FREQ1", .value = 0x0100, .dpll_type = DPLL_TYPE_CUSTOM },
> > > + { .name = "FREQ2", .value = 0x0200, .dpll_type = DPLL_TYPE_CUSTOM },
> > > + { .name = "FREQ3", .value = 0x0400, .dpll_type = DPLL_TYPE_CUSTOM },
> > > + { .name = "FREQ4", .value = 0x0800, .dpll_type = DPLL_TYPE_CUSTOM },
> > > + { .name = "None", .value = SMA_DISABLE, .dpll_type = DPLL_TYPE_NONE },
> >
> > 80-column limit (here and throughout the file)
>
> I thought this rule was relaxed up to 100-columns?
Only in exceptional cases, IIRC. checkpatch complains too.
> > > static int
> > > ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > {
> > > @@ -3768,6 +3855,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;
> >
> > 80 cols, and this should be done before ptp_ocp_complete()
> > Also, should 'goto out', not return 0 and leak resources.
>
> I don't think we have to go with error path. Driver itself can work without
> DPLL device registered, there is no hard dependency. The DPLL device will
> not be registered and HW could not be configured/monitored via netlink, but
> could still be usable.
Not sure I agree with that - the DPLL device is selected in Kconfig, so
users would expect to have it present. I think it makes more sense to
fail if it cannot be allocated.
--
Jonathan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops
2022-06-28 19:11 ` Jonathan Lemon
@ 2022-06-29 3:24 ` Jakub Kicinski
2022-06-29 23:31 ` Vadim Fedorenko
0 siblings, 1 reply; 36+ messages in thread
From: Jakub Kicinski @ 2022-06-29 3:24 UTC (permalink / raw)
To: Jonathan Lemon
Cc: Vadim Fedorenko, Arkadiusz Kubalewski, Vadim Fedorenko,
Aya Levin, netdev, linux-arm-kernel, linux-clk
On Tue, 28 Jun 2022 12:11:24 -0700 Jonathan Lemon wrote:
> > > 80-column limit (here and throughout the file)
> >
> > I thought this rule was relaxed up to 100-columns?
>
> Only in exceptional cases, IIRC. checkpatch complains too.
Yup, for networking I still prefer 80 chars.
My field of vision is narrow.
> > > 80 cols, and this should be done before ptp_ocp_complete()
> > > Also, should 'goto out', not return 0 and leak resources.
> >
> > I don't think we have to go with error path. Driver itself can work without
> > DPLL device registered, there is no hard dependency. The DPLL device will
> > not be registered and HW could not be configured/monitored via netlink, but
> > could still be usable.
>
> Not sure I agree with that - the DPLL device is selected in Kconfig, so
> users would expect to have it present. I think it makes more sense to
> fail if it cannot be allocated.
+1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops
2022-06-29 3:24 ` Jakub Kicinski
@ 2022-06-29 23:31 ` Vadim Fedorenko
0 siblings, 0 replies; 36+ messages in thread
From: Vadim Fedorenko @ 2022-06-29 23:31 UTC (permalink / raw)
To: Jakub Kicinski, Jonathan Lemon
Cc: Arkadiusz Kubalewski, Vadim Fedorenko, Aya Levin, netdev,
linux-arm-kernel, linux-clk
On 29.06.2022 04:24, Jakub Kicinski wrote:
> On Tue, 28 Jun 2022 12:11:24 -0700 Jonathan Lemon wrote:
>>>> 80-column limit (here and throughout the file)
>>>
>>> I thought this rule was relaxed up to 100-columns?
>>
>> Only in exceptional cases, IIRC. checkpatch complains too.
>
> Yup, for networking I still prefer 80 chars.
> My field of vision is narrow.
>
Ok, no problem, will follow strict rules next time.
>>>> 80 cols, and this should be done before ptp_ocp_complete()
>>>> Also, should 'goto out', not return 0 and leak resources.
>>>
>>> I don't think we have to go with error path. Driver itself can work without
>>> DPLL device registered, there is no hard dependency. The DPLL device will
>>> not be registered and HW could not be configured/monitored via netlink, but
>>> could still be usable.
>>
>> Not sure I agree with that - the DPLL device is selected in Kconfig, so
>> users would expect to have it present. I think it makes more sense to
>> fail if it cannot be allocated.
>
> +1
Ok, it's not a big deal to make it fail in case of DPLL error, will do it in
next iteration.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops
2022-06-26 19:24 ` [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops Vadim Fedorenko
2022-06-27 19:34 ` Jonathan Lemon
@ 2022-09-29 11:33 ` Jiri Pirko
2022-09-30 0:56 ` Vadim Fedorenko
1 sibling, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2022-09-29 11:33 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
Sun, Jun 26, 2022 at 09:24:44PM CEST, vfedorenko@novek.ru wrote:
>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 | 169 ++++++++++++++++++++++++++++++--------
> include/uapi/linux/dpll.h | 2 +
> 3 files changed, 136 insertions(+), 36 deletions(-)
>
>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..f830625a63a3 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)
>@@ -660,18 +662,19 @@ static DEFINE_IDR(ptp_ocp_idr);
> struct ocp_selector {
> const char *name;
> int value;
>+ int dpll_type;
> };
>
> static const struct ocp_selector ptp_ocp_clock[] = {
>- { .name = "NONE", .value = 0 },
>- { .name = "TOD", .value = 1 },
>- { .name = "IRIG", .value = 2 },
>- { .name = "PPS", .value = 3 },
>- { .name = "PTP", .value = 4 },
>- { .name = "RTC", .value = 5 },
>- { .name = "DCF", .value = 6 },
>- { .name = "REGS", .value = 0xfe },
>- { .name = "EXT", .value = 0xff },
>+ { .name = "NONE", .value = 0, .dpll_type = 0 },
>+ { .name = "TOD", .value = 1, .dpll_type = 0 },
>+ { .name = "IRIG", .value = 2, .dpll_type = 0 },
>+ { .name = "PPS", .value = 3, .dpll_type = 0 },
>+ { .name = "PTP", .value = 4, .dpll_type = 0 },
>+ { .name = "RTC", .value = 5, .dpll_type = 0 },
>+ { .name = "DCF", .value = 6, .dpll_type = 0 },
>+ { .name = "REGS", .value = 0xfe, .dpll_type = 0 },
>+ { .name = "EXT", .value = 0xff, .dpll_type = 0 },
> { }
> };
>
>@@ -680,37 +683,37 @@ static const struct ocp_selector ptp_ocp_clock[] = {
> #define SMA_SELECT_MASK GENMASK(14, 0)
>
> static const struct ocp_selector ptp_ocp_sma_in[] = {
>- { .name = "10Mhz", .value = 0x0000 },
>- { .name = "PPS1", .value = 0x0001 },
>- { .name = "PPS2", .value = 0x0002 },
>- { .name = "TS1", .value = 0x0004 },
>- { .name = "TS2", .value = 0x0008 },
>- { .name = "IRIG", .value = 0x0010 },
>- { .name = "DCF", .value = 0x0020 },
>- { .name = "TS3", .value = 0x0040 },
>- { .name = "TS4", .value = 0x0080 },
>- { .name = "FREQ1", .value = 0x0100 },
>- { .name = "FREQ2", .value = 0x0200 },
>- { .name = "FREQ3", .value = 0x0400 },
>- { .name = "FREQ4", .value = 0x0800 },
>- { .name = "None", .value = SMA_DISABLE },
>+ { .name = "10Mhz", .value = 0x0000, .dpll_type = DPLL_TYPE_EXT_10MHZ },
>+ { .name = "PPS1", .value = 0x0001, .dpll_type = DPLL_TYPE_EXT_1PPS },
>+ { .name = "PPS2", .value = 0x0002, .dpll_type = DPLL_TYPE_EXT_1PPS },
>+ { .name = "TS1", .value = 0x0004, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "TS2", .value = 0x0008, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "TS3", .value = 0x0040, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "TS4", .value = 0x0080, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "FREQ1", .value = 0x0100, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "FREQ2", .value = 0x0200, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "FREQ3", .value = 0x0400, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "FREQ4", .value = 0x0800, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "None", .value = SMA_DISABLE, .dpll_type = DPLL_TYPE_NONE },
> { }
> };
>
> static const struct ocp_selector ptp_ocp_sma_out[] = {
>- { .name = "10Mhz", .value = 0x0000 },
>- { .name = "PHC", .value = 0x0001 },
>- { .name = "MAC", .value = 0x0002 },
>- { .name = "GNSS1", .value = 0x0004 },
>- { .name = "GNSS2", .value = 0x0008 },
>- { .name = "IRIG", .value = 0x0010 },
>- { .name = "DCF", .value = 0x0020 },
>- { .name = "GEN1", .value = 0x0040 },
>- { .name = "GEN2", .value = 0x0080 },
>- { .name = "GEN3", .value = 0x0100 },
>- { .name = "GEN4", .value = 0x0200 },
>- { .name = "GND", .value = 0x2000 },
>- { .name = "VCC", .value = 0x4000 },
>+ { .name = "10Mhz", .value = 0x0000, .dpll_type = DPLL_TYPE_EXT_10MHZ },
>+ { .name = "PHC", .value = 0x0001, .dpll_type = DPLL_TYPE_INT_OSCILLATOR },
>+ { .name = "MAC", .value = 0x0002, .dpll_type = DPLL_TYPE_INT_OSCILLATOR },
>+ { .name = "GNSS1", .value = 0x0004, .dpll_type = DPLL_TYPE_GNSS },
>+ { .name = "GNSS2", .value = 0x0008, .dpll_type = DPLL_TYPE_GNSS },
>+ { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "GEN1", .value = 0x0040, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "GEN2", .value = 0x0080, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "GEN3", .value = 0x0100, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "GEN4", .value = 0x0200, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "GND", .value = 0x2000, .dpll_type = DPLL_TYPE_CUSTOM },
>+ { .name = "VCC", .value = 0x4000, .dpll_type = DPLL_TYPE_CUSTOM },
> { }
> };
>
>@@ -3713,6 +3716,90 @@ 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);
Unnecessary cast.
>+ int sync;
>+
>+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
Just return without having "sync" variable.
>+ 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;
This is very odd. You return something you read from device directly
over what likes to be an abstract API. Is it supposed to be a bool? If
yes, make it so.
>+}
>+
>+static int ptp_ocp_sma_get_dpll_type(struct ptp_ocp *bp, int sma_nr)
Type should be most probably enum instead of int.
>+{
>+ struct ocp_selector *tbl;
>+ u32 val;
>+
>+ if (bp->sma[sma_nr].mode == SMA_MODE_IN)
>+ tbl = bp->sma_op->tbl[0];
>+ else
>+ tbl = bp->sma_op->tbl[1];
>+
>+ val = ptp_ocp_sma_get(bp, sma_nr);
>+ return tbl[val].dpll_type;
>+}
>+
>+static int ptp_ocp_dpll_type_supported(struct dpll_device *dpll, int sma, int type, int dir)
>+{
>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+ struct ocp_selector *tbl = bp->sma_op->tbl[dir];
>+ int i;
>+
>+ for (i = 0; i < sizeof(*tbl); i++) {
>+ if (tbl[i].dpll_type == type)
>+ return 1;
>+ }
>+ return 0;
1/0? Bool please.
>+}
>+
>+static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
>+{
>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+
>+ if (bp->sma[sma].mode != SMA_MODE_IN)
>+ return -1;
>+
>+ return ptp_ocp_sma_get_dpll_type(bp, sma);
enum.
>+}
>+
>+static int ptp_ocp_dpll_get_source_supported(struct dpll_device *dpll, int sma, int type)
>+{
>+ return ptp_ocp_dpll_type_supported(dpll, sma, type, 0);
>+}
>+
>+static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
>+{
>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+
>+ if (bp->sma[sma].mode != SMA_MODE_OUT)
>+ return -1;
>+
>+ return ptp_ocp_sma_get_dpll_type(bp, sma);
>+}
>+
>+static int ptp_ocp_dpll_get_output_supported(struct dpll_device *dpll, int sma, int type)
>+{
>+ return ptp_ocp_dpll_type_supported(dpll, sma, type, 1);
1? Have the "dir" to be enum please.
>+}
>+
>+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_source_supported = ptp_ocp_dpll_get_source_supported,
>+ .get_output_type = ptp_ocp_dpll_get_output_type,
>+ .get_output_supported = ptp_ocp_dpll_get_output_supported,
>+};
>+
> static int
> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
>@@ -3768,6 +3855,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);
I wonder, why you don't have alloc-register and unregister-free
squashed?
>+
> return 0;
>
> out:
>@@ -3785,6 +3880,8 @@ ptp_ocp_remove(struct pci_dev *pdev)
> struct ptp_ocp *bp = pci_get_drvdata(pdev);
> struct devlink *devlink = priv_to_devlink(bp);
>
>+ dpll_device_unregister(bp->dpll);
>+ dpll_device_free(bp->dpll);
> devlink_unregister(devlink);
> ptp_ocp_detach(bp);
> pci_disable_device(pdev);
>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>index 7ce45c6b4fd4..5e8c46712d18 100644
>--- a/include/uapi/linux/dpll.h
>+++ b/include/uapi/linux/dpll.h
>@@ -41,11 +41,13 @@ enum dpll_genl_attr {
>
> /* DPLL signal types used as source or as output */
> enum dpll_genl_signal_type {
>+ DPLL_TYPE_NONE,
> DPLL_TYPE_EXT_1PPS,
> DPLL_TYPE_EXT_10MHZ,
> DPLL_TYPE_SYNCE_ETH_PORT,
> DPLL_TYPE_INT_OSCILLATOR,
> DPLL_TYPE_GNSS,
>+ DPLL_TYPE_CUSTOM,
This hunk does not belong to this patch.
>
> __DPLL_TYPE_MAX,
> };
>--
>2.27.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops
2022-09-29 11:33 ` Jiri Pirko
@ 2022-09-30 0:56 ` Vadim Fedorenko
0 siblings, 0 replies; 36+ messages in thread
From: Vadim Fedorenko @ 2022-09-30 0:56 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
On 29.09.2022 12:33, Jiri Pirko wrote:
> Sun, Jun 26, 2022 at 09:24:44PM CEST, vfedorenko@novek.ru wrote:
>> 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 | 169 ++++++++++++++++++++++++++++++--------
>> include/uapi/linux/dpll.h | 2 +
>> 3 files changed, 136 insertions(+), 36 deletions(-)
>>
>> 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..f830625a63a3 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)
>> @@ -660,18 +662,19 @@ static DEFINE_IDR(ptp_ocp_idr);
>> struct ocp_selector {
>> const char *name;
>> int value;
>> + int dpll_type;
>> };
>>
>> static const struct ocp_selector ptp_ocp_clock[] = {
>> - { .name = "NONE", .value = 0 },
>> - { .name = "TOD", .value = 1 },
>> - { .name = "IRIG", .value = 2 },
>> - { .name = "PPS", .value = 3 },
>> - { .name = "PTP", .value = 4 },
>> - { .name = "RTC", .value = 5 },
>> - { .name = "DCF", .value = 6 },
>> - { .name = "REGS", .value = 0xfe },
>> - { .name = "EXT", .value = 0xff },
>> + { .name = "NONE", .value = 0, .dpll_type = 0 },
>> + { .name = "TOD", .value = 1, .dpll_type = 0 },
>> + { .name = "IRIG", .value = 2, .dpll_type = 0 },
>> + { .name = "PPS", .value = 3, .dpll_type = 0 },
>> + { .name = "PTP", .value = 4, .dpll_type = 0 },
>> + { .name = "RTC", .value = 5, .dpll_type = 0 },
>> + { .name = "DCF", .value = 6, .dpll_type = 0 },
>> + { .name = "REGS", .value = 0xfe, .dpll_type = 0 },
>> + { .name = "EXT", .value = 0xff, .dpll_type = 0 },
>> { }
>> };
>>
>> @@ -680,37 +683,37 @@ static const struct ocp_selector ptp_ocp_clock[] = {
>> #define SMA_SELECT_MASK GENMASK(14, 0)
>>
>> static const struct ocp_selector ptp_ocp_sma_in[] = {
>> - { .name = "10Mhz", .value = 0x0000 },
>> - { .name = "PPS1", .value = 0x0001 },
>> - { .name = "PPS2", .value = 0x0002 },
>> - { .name = "TS1", .value = 0x0004 },
>> - { .name = "TS2", .value = 0x0008 },
>> - { .name = "IRIG", .value = 0x0010 },
>> - { .name = "DCF", .value = 0x0020 },
>> - { .name = "TS3", .value = 0x0040 },
>> - { .name = "TS4", .value = 0x0080 },
>> - { .name = "FREQ1", .value = 0x0100 },
>> - { .name = "FREQ2", .value = 0x0200 },
>> - { .name = "FREQ3", .value = 0x0400 },
>> - { .name = "FREQ4", .value = 0x0800 },
>> - { .name = "None", .value = SMA_DISABLE },
>> + { .name = "10Mhz", .value = 0x0000, .dpll_type = DPLL_TYPE_EXT_10MHZ },
>> + { .name = "PPS1", .value = 0x0001, .dpll_type = DPLL_TYPE_EXT_1PPS },
>> + { .name = "PPS2", .value = 0x0002, .dpll_type = DPLL_TYPE_EXT_1PPS },
>> + { .name = "TS1", .value = 0x0004, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "TS2", .value = 0x0008, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "TS3", .value = 0x0040, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "TS4", .value = 0x0080, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "FREQ1", .value = 0x0100, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "FREQ2", .value = 0x0200, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "FREQ3", .value = 0x0400, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "FREQ4", .value = 0x0800, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "None", .value = SMA_DISABLE, .dpll_type = DPLL_TYPE_NONE },
>> { }
>> };
>>
>> static const struct ocp_selector ptp_ocp_sma_out[] = {
>> - { .name = "10Mhz", .value = 0x0000 },
>> - { .name = "PHC", .value = 0x0001 },
>> - { .name = "MAC", .value = 0x0002 },
>> - { .name = "GNSS1", .value = 0x0004 },
>> - { .name = "GNSS2", .value = 0x0008 },
>> - { .name = "IRIG", .value = 0x0010 },
>> - { .name = "DCF", .value = 0x0020 },
>> - { .name = "GEN1", .value = 0x0040 },
>> - { .name = "GEN2", .value = 0x0080 },
>> - { .name = "GEN3", .value = 0x0100 },
>> - { .name = "GEN4", .value = 0x0200 },
>> - { .name = "GND", .value = 0x2000 },
>> - { .name = "VCC", .value = 0x4000 },
>> + { .name = "10Mhz", .value = 0x0000, .dpll_type = DPLL_TYPE_EXT_10MHZ },
>> + { .name = "PHC", .value = 0x0001, .dpll_type = DPLL_TYPE_INT_OSCILLATOR },
>> + { .name = "MAC", .value = 0x0002, .dpll_type = DPLL_TYPE_INT_OSCILLATOR },
>> + { .name = "GNSS1", .value = 0x0004, .dpll_type = DPLL_TYPE_GNSS },
>> + { .name = "GNSS2", .value = 0x0008, .dpll_type = DPLL_TYPE_GNSS },
>> + { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "GEN1", .value = 0x0040, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "GEN2", .value = 0x0080, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "GEN3", .value = 0x0100, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "GEN4", .value = 0x0200, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "GND", .value = 0x2000, .dpll_type = DPLL_TYPE_CUSTOM },
>> + { .name = "VCC", .value = 0x4000, .dpll_type = DPLL_TYPE_CUSTOM },
>> { }
>> };
>>
>> @@ -3713,6 +3716,90 @@ 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);
>
> Unnecessary cast.
>
>
>> + int sync;
>> +
>> + sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
>
> Just return without having "sync" variable.
>
>> + 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;
>
> This is very odd. You return something you read from device directly
> over what likes to be an abstract API. Is it supposed to be a bool? If
> yes, make it so.
>
Thanks for pointing to the issue. I will make it boolean type, as well as other
points.
>
>> +}
>> +
>> +static int ptp_ocp_sma_get_dpll_type(struct ptp_ocp *bp, int sma_nr)
>
> Type should be most probably enum instead of int.
>
There is a process going on to reimplement the interface around inputs and
outputs, and we are going to use enums for all such cases. But thanks for
pointing it too, it feels like we are on the same wave.
>
>> +{
>> + struct ocp_selector *tbl;
>> + u32 val;
>> +
>> + if (bp->sma[sma_nr].mode == SMA_MODE_IN)
>> + tbl = bp->sma_op->tbl[0];
>> + else
>> + tbl = bp->sma_op->tbl[1];
>> +
>> + val = ptp_ocp_sma_get(bp, sma_nr);
>> + return tbl[val].dpll_type;
>> +}
>> +
>> +static int ptp_ocp_dpll_type_supported(struct dpll_device *dpll, int sma, int type, int dir)
>> +{
>> + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> + struct ocp_selector *tbl = bp->sma_op->tbl[dir];
>> + int i;
>> +
>> + for (i = 0; i < sizeof(*tbl); i++) {
>> + if (tbl[i].dpll_type == type)
>> + return 1;
>> + }
>> + return 0;
>
> 1/0? Bool please.
>
>
>> +}
>> +
>> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
>> +{
>> + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +
>> + if (bp->sma[sma].mode != SMA_MODE_IN)
>> + return -1;
>> +
>> + return ptp_ocp_sma_get_dpll_type(bp, sma);
>
> enum.
>
>
>> +}
>> +
>> +static int ptp_ocp_dpll_get_source_supported(struct dpll_device *dpll, int sma, int type)
>> +{
>> + return ptp_ocp_dpll_type_supported(dpll, sma, type, 0);
>> +}
>> +
>> +static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
>> +{
>> + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +
>> + if (bp->sma[sma].mode != SMA_MODE_OUT)
>> + return -1;
>> +
>> + return ptp_ocp_sma_get_dpll_type(bp, sma);
>> +}
>> +
>> +static int ptp_ocp_dpll_get_output_supported(struct dpll_device *dpll, int sma, int type)
>> +{
>> + return ptp_ocp_dpll_type_supported(dpll, sma, type, 1);
>
> 1? Have the "dir" to be enum please.
>
>
>> +}
>> +
>> +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_source_supported = ptp_ocp_dpll_get_source_supported,
>> + .get_output_type = ptp_ocp_dpll_get_output_type,
>> + .get_output_supported = ptp_ocp_dpll_get_output_supported,
>> +};
>> +
>> static int
>> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> {
>> @@ -3768,6 +3855,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);
>
> I wonder, why you don't have alloc-register and unregister-free
> squashed?
>
There were some concerns about how to deal with locking in case of squashing
these functions. But it looks like we will end up with special locks and then I
will try to squash them.
>
>> +
>> return 0;
>>
>> out:
>> @@ -3785,6 +3880,8 @@ ptp_ocp_remove(struct pci_dev *pdev)
>> struct ptp_ocp *bp = pci_get_drvdata(pdev);
>> struct devlink *devlink = priv_to_devlink(bp);
>>
>> + dpll_device_unregister(bp->dpll);
>> + dpll_device_free(bp->dpll);
>> devlink_unregister(devlink);
>> ptp_ocp_detach(bp);
>> pci_disable_device(pdev);
>> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>> index 7ce45c6b4fd4..5e8c46712d18 100644
>> --- a/include/uapi/linux/dpll.h
>> +++ b/include/uapi/linux/dpll.h
>> @@ -41,11 +41,13 @@ enum dpll_genl_attr {
>>
>> /* DPLL signal types used as source or as output */
>> enum dpll_genl_signal_type {
>> + DPLL_TYPE_NONE,
>> DPLL_TYPE_EXT_1PPS,
>> DPLL_TYPE_EXT_10MHZ,
>> DPLL_TYPE_SYNCE_ETH_PORT,
>> DPLL_TYPE_INT_OSCILLATOR,
>> DPLL_TYPE_GNSS,
>> + DPLL_TYPE_CUSTOM,
>
> This hunk does not belong to this patch.
Oh yeah, thanks for catching!
>
>
>>
>> __DPLL_TYPE_MAX,
>> };
>> --
>> 2.27.0
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 0/3] Create common DPLL/clock configuration API
2022-06-26 19:24 [RFC PATCH v2 0/3] Create common DPLL/clock configuration API Vadim Fedorenko
` (2 preceding siblings ...)
2022-06-26 19:24 ` [RFC PATCH v2 3/3] ptp_ocp: implement DPLL ops Vadim Fedorenko
@ 2022-09-01 12:02 ` Gal Pressman
2022-09-29 11:40 ` Jiri Pirko
4 siblings, 0 replies; 36+ messages in thread
From: Gal Pressman @ 2022-09-01 12:02 UTC (permalink / raw)
To: Vadim Fedorenko, Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon
Cc: Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk,
Ariel Almog, Bar Shapira
On 26/06/2022 22:24, 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.
Hello Vadim,
I'm trying to understand how we can register our mlx5 hardware with this
DPLL subsystem, and honestly I don't understand how this is going to be
used eventually.
Is there anywhere else I can read more about this work? Maybe some
documentation explaining the use-cases? Is there userspace code I can
see to get a sense of the full picture?
Thanks
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 0/3] Create common DPLL/clock configuration API
2022-06-26 19:24 [RFC PATCH v2 0/3] Create common DPLL/clock configuration API Vadim Fedorenko
` (3 preceding siblings ...)
2022-09-01 12:02 ` [RFC PATCH v2 0/3] Create common DPLL/clock configuration API Gal Pressman
@ 2022-09-29 11:40 ` Jiri Pirko
2022-09-30 0:44 ` Vadim Fedorenko
4 siblings, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2022-09-29 11:40 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
Sun, Jun 26, 2022 at 09:24:41PM CEST, vfedorenko@novek.ru 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.
Do you have the userspace part somewhere?
It is very nice to add example outputs of user cmdline of such tool to
the patch description/cover letter.
Also, did you consider usage of sysfs? Why it isn't a better fit than
netlink?
Regarding the naming, is "dpll" the correct one. Forgive me for being a
syncE greenie, but isn't dpll just one algo to achieve syntonous
clocks? Perhaps "dco" as for "Digitally Controlled Oscillator" would be
somewhat better fit?
>
>v1 -> v2:
> * implement returning supported input/output types
> * ptp_ocp: follow suggestions from Jonathan
> * add linux-clk mailing list
>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 | 161 ++++++++++
> drivers/dpll/dpll_core.h | 40 +++
> drivers/dpll/dpll_netlink.c | 595 ++++++++++++++++++++++++++++++++++++
> drivers/dpll/dpll_netlink.h | 14 +
> drivers/ptp/Kconfig | 1 +
> drivers/ptp/ptp_ocp.c | 169 +++++++---
> include/linux/dpll.h | 29 ++
> include/uapi/linux/dpll.h | 81 +++++
> 13 files changed, 1079 insertions(+), 36 deletions(-)
> 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
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC PATCH v2 0/3] Create common DPLL/clock configuration API
2022-09-29 11:40 ` Jiri Pirko
@ 2022-09-30 0:44 ` Vadim Fedorenko
2022-09-30 8:33 ` Jiri Pirko
0 siblings, 1 reply; 36+ messages in thread
From: Vadim Fedorenko @ 2022-09-30 0:44 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
On 29.09.2022 12:40, Jiri Pirko wrote:
> Sun, Jun 26, 2022 at 09:24:41PM CEST, vfedorenko@novek.ru 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.
>
> Do you have the userspace part somewhere?
> It is very nice to add example outputs of user cmdline of such tool to
> the patch description/cover letter.
Sorry, but we don't have any user-space part for now. It's still WIP and there
are too many changes in the protocol to implement anything useful on top of it.
Once we will get to a kind of "stable" proto, I will implement a library to use it.
>
> Also, did you consider usage of sysfs? Why it isn't a better fit than
> netlink?
We already have sysfs implemented in the ptp_ocp driver. But it looks like more
hardware is going to be available soon with almost the same functions, so it
would be great to have common protocol to configure such devices.
>
> Regarding the naming, is "dpll" the correct one. Forgive me for being a
> syncE greenie, but isn't dpll just one algo to achieve syntonous
> clocks? Perhaps "dco" as for "Digitally Controlled Oscillator" would be
> somewhat better fit?
>
We will discuss the naming too, thanks!
>
>>
>> v1 -> v2:
>> * implement returning supported input/output types
>> * ptp_ocp: follow suggestions from Jonathan
>> * add linux-clk mailing list
>> 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 | 161 ++++++++++
>> drivers/dpll/dpll_core.h | 40 +++
>> drivers/dpll/dpll_netlink.c | 595 ++++++++++++++++++++++++++++++++++++
>> drivers/dpll/dpll_netlink.h | 14 +
>> drivers/ptp/Kconfig | 1 +
>> drivers/ptp/ptp_ocp.c | 169 +++++++---
>> include/linux/dpll.h | 29 ++
>> include/uapi/linux/dpll.h | 81 +++++
>> 13 files changed, 1079 insertions(+), 36 deletions(-)
>> 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 0/3] Create common DPLL/clock configuration API
2022-09-30 0:44 ` Vadim Fedorenko
@ 2022-09-30 8:33 ` Jiri Pirko
2022-09-30 14:33 ` Jakub Kicinski
0 siblings, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2022-09-30 8:33 UTC (permalink / raw)
To: Vadim Fedorenko
Cc: Jakub Kicinski, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
Fri, Sep 30, 2022 at 02:44:25AM CEST, vfedorenko@novek.ru wrote:
>On 29.09.2022 12:40, Jiri Pirko wrote:
>> Sun, Jun 26, 2022 at 09:24:41PM CEST, vfedorenko@novek.ru 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.
>>
>> Do you have the userspace part somewhere?
>> It is very nice to add example outputs of user cmdline of such tool to
>> the patch description/cover letter.
>
>Sorry, but we don't have any user-space part for now. It's still WIP and
>there are too many changes in the protocol to implement anything useful on
What protocol?
>top of it. Once we will get to a kind of "stable" proto, I will implement a
>library to use it.
>
>>
>> Also, did you consider usage of sysfs? Why it isn't a better fit than
>> netlink?
>
>We already have sysfs implemented in the ptp_ocp driver. But it looks like
>more hardware is going to be available soon with almost the same functions,
>so it would be great to have common protocol to configure such devices.
Sure, but more hw does not mean you can't use sysfs. Take netdev as an
example. The sysfs exposed for it is implemented net/core/net-sysfs.c
and is exposed for all netdev instances, no matter what the
driver/hardware is.
>>
>> Regarding the naming, is "dpll" the correct one. Forgive me for being a
>> syncE greenie, but isn't dpll just one algo to achieve syntonous
>> clocks? Perhaps "dco" as for "Digitally Controlled Oscillator" would be
>> somewhat better fit?
>>
>
>We will discuss the naming too, thanks!
>
>>
>> >
>> > v1 -> v2:
>> > * implement returning supported input/output types
>> > * ptp_ocp: follow suggestions from Jonathan
>> > * add linux-clk mailing list
>> > 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 | 161 ++++++++++
>> > drivers/dpll/dpll_core.h | 40 +++
>> > drivers/dpll/dpll_netlink.c | 595 ++++++++++++++++++++++++++++++++++++
>> > drivers/dpll/dpll_netlink.h | 14 +
>> > drivers/ptp/Kconfig | 1 +
>> > drivers/ptp/ptp_ocp.c | 169 +++++++---
>> > include/linux/dpll.h | 29 ++
>> > include/uapi/linux/dpll.h | 81 +++++
>> > 13 files changed, 1079 insertions(+), 36 deletions(-)
>> > 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 0/3] Create common DPLL/clock configuration API
2022-09-30 8:33 ` Jiri Pirko
@ 2022-09-30 14:33 ` Jakub Kicinski
2022-10-01 5:47 ` Jiri Pirko
0 siblings, 1 reply; 36+ messages in thread
From: Jakub Kicinski @ 2022-09-30 14:33 UTC (permalink / raw)
To: Jiri Pirko
Cc: Vadim Fedorenko, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
On Fri, 30 Sep 2022 10:33:57 +0200 Jiri Pirko wrote:
> >> Also, did you consider usage of sysfs? Why it isn't a better fit than
> >> netlink?
> >
> >We already have sysfs implemented in the ptp_ocp driver. But it looks like
> >more hardware is going to be available soon with almost the same functions,
> >so it would be great to have common protocol to configure such devices.
>
> Sure, but more hw does not mean you can't use sysfs. Take netdev as an
> example. The sysfs exposed for it is implemented net/core/net-sysfs.c
> and is exposed for all netdev instances, no matter what the
> driver/hardware is.
Wait, *you* are suggesting someone uses sysfs instead of netlink?
Could you say more because I feel like that's kicking the absolute.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 0/3] Create common DPLL/clock configuration API
2022-09-30 14:33 ` Jakub Kicinski
@ 2022-10-01 5:47 ` Jiri Pirko
2022-10-01 14:18 ` Jakub Kicinski
0 siblings, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2022-10-01 5:47 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vadim Fedorenko, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk
Fri, Sep 30, 2022 at 04:33:12PM CEST, kuba@kernel.org wrote:
>On Fri, 30 Sep 2022 10:33:57 +0200 Jiri Pirko wrote:
>> >> Also, did you consider usage of sysfs? Why it isn't a better fit than
>> >> netlink?
>> >
>> >We already have sysfs implemented in the ptp_ocp driver. But it looks like
>> >more hardware is going to be available soon with almost the same functions,
>> >so it would be great to have common protocol to configure such devices.
>>
>> Sure, but more hw does not mean you can't use sysfs. Take netdev as an
>> example. The sysfs exposed for it is implemented net/core/net-sysfs.c
>> and is exposed for all netdev instances, no matter what the
>> driver/hardware is.
>
>Wait, *you* are suggesting someone uses sysfs instead of netlink?
>
>Could you say more because I feel like that's kicking the absolute.
I don't understand why that would be a problem. What I'm trying to say
is, perhaps sysfs is a better API for this purpose. The API looks very
neat and there is no probabilito of huge grow. Also, with sysfs, you
don't need userspace app to do basic work with the api. In this case, I
don't see why the app is needed. These are 2 biggest arguments for sysfs
in this case as I see it.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 0/3] Create common DPLL/clock configuration API
2022-10-01 5:47 ` Jiri Pirko
@ 2022-10-01 14:18 ` Jakub Kicinski
2022-10-02 14:35 ` Jiri Pirko
0 siblings, 1 reply; 36+ messages in thread
From: Jakub Kicinski @ 2022-10-01 14:18 UTC (permalink / raw)
To: Jiri Pirko
Cc: Vadim Fedorenko, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk,
Gal Pressman
On Sat, 1 Oct 2022 07:47:24 +0200 Jiri Pirko wrote:
> >> Sure, but more hw does not mean you can't use sysfs. Take netdev as an
> >> example. The sysfs exposed for it is implemented net/core/net-sysfs.c
> >> and is exposed for all netdev instances, no matter what the
> >> driver/hardware is.
> >
> >Wait, *you* are suggesting someone uses sysfs instead of netlink?
> >
> >Could you say more because I feel like that's kicking the absolute.
>
> I don't understand why that would be a problem.
Why did you do devlink over netlink then?
The bus device is already there in sysfs.
> What I'm trying to say
> is, perhaps sysfs is a better API for this purpose. The API looks very
> neat and there is no probabilito of huge grow.
"this API is nice and small" said everyone about every new API ever,
APIs grow.
> Also, with sysfs, you
> don't need userspace app to do basic work with the api. In this case, I
> don't see why the app is needed.
Yes, with the YAML specs you don't need a per-family APP.
A generic app can support any family, just JSON in JSON out.
DPLL-nl will come with a YAML spec.
> These are 2 biggest arguments for sysfs in this case as I see it.
2 biggest arguments? Is "this API is small" one of the _biggest_
arguments you see? I don't think it's an argument at all. The OCP PTP
driver started small and now its not small. And the files don't even
follow sysfs rules. Trust me, we have some experience here :/
As I said to you in private I feel like there may be some political
games being played here, so I'd like to urge you to focus on real
issues.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 0/3] Create common DPLL/clock configuration API
2022-10-01 14:18 ` Jakub Kicinski
@ 2022-10-02 14:35 ` Jiri Pirko
2022-10-03 14:28 ` Jakub Kicinski
0 siblings, 1 reply; 36+ messages in thread
From: Jiri Pirko @ 2022-10-02 14:35 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vadim Fedorenko, Arkadiusz Kubalewski, Jonathan Lemon,
Vadim Fedorenko, Aya Levin, netdev, linux-arm-kernel, linux-clk,
Gal Pressman
Sat, Oct 01, 2022 at 04:18:27PM CEST, kuba@kernel.org wrote:
>On Sat, 1 Oct 2022 07:47:24 +0200 Jiri Pirko wrote:
>> >> Sure, but more hw does not mean you can't use sysfs. Take netdev as an
>> >> example. The sysfs exposed for it is implemented net/core/net-sysfs.c
>> >> and is exposed for all netdev instances, no matter what the
>> >> driver/hardware is.
>> >
>> >Wait, *you* are suggesting someone uses sysfs instead of netlink?
>> >
>> >Could you say more because I feel like that's kicking the absolute.
>>
>> I don't understand why that would be a problem.
>
>Why did you do devlink over netlink then?
There were good reasons why to use netlink, many of those. I find it
redundant to list them here.
>The bus device is already there in sysfs.
>
>> What I'm trying to say
>> is, perhaps sysfs is a better API for this purpose. The API looks very
>> neat and there is no probabilito of huge grow.
>
>"this API is nice and small" said everyone about every new API ever,
>APIs grow.
Sure, what what are the odds.
>
>> Also, with sysfs, you
>> don't need userspace app to do basic work with the api. In this case, I
>> don't see why the app is needed.
>
>Yes, with the YAML specs you don't need a per-family APP.
>A generic app can support any family, just JSON in JSON out.
>DPLL-nl will come with a YAML spec.
Yeah, but still. For sysfs, you don't need any app.
Just saying.
>
>> These are 2 biggest arguments for sysfs in this case as I see it.
>
>2 biggest arguments? Is "this API is small" one of the _biggest_
>arguments you see? I don't think it's an argument at all. The OCP PTP
>driver started small and now its not small. And the files don't even
>follow sysfs rules. Trust me, we have some experience here :/
No problem. I don't mind one bit, don't get me wrong :)
I just pointed out alternative.
>
>As I said to you in private I feel like there may be some political
>games being played here, so I'd like to urge you to focus on real
>issues.
I don't know anything about any politics. I don't care about it at all
to be honest. You know me :)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 0/3] Create common DPLL/clock configuration API
2022-10-02 14:35 ` Jiri Pirko
@ 2022-10-03 14:28 ` Jakub Kicinski
2022-10-03 17:20 ` Vadim Fedorenko
2022-10-04 6:33 ` Jiri Pirko
0 siblings, 2 replies; 36+ messages in thread
From: Jakub Kicinski @ 2022-10-03 14:28 UTC (permalink / raw)
To: Jiri Pirko, Vadim Fedorenko
Cc: Vadim Fedorenko, Arkadiusz Kubalewski, Jonathan Lemon, Aya Levin,
netdev, linux-arm-kernel, linux-clk, Gal Pressman
On Sun, 2 Oct 2022 16:35:07 +0200 Jiri Pirko wrote:
>>> What I'm trying to say
>>> is, perhaps sysfs is a better API for this purpose. The API looks very
>>> neat and there is no probabilito of huge grow.
>>
>> "this API is nice and small" said everyone about every new API ever,
>> APIs grow.
>
> Sure, what what are the odds.
The pins were made into full objects now, and we also model muxes.
Vadim, could you share the link to the GH repo?
What's your feeling on posting the latest patches upstream as RFC,
whatever state things are in right now?
My preference would be to move the development to the list at this
stage, FWIW.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 0/3] Create common DPLL/clock configuration API
2022-10-03 14:28 ` Jakub Kicinski
@ 2022-10-03 17:20 ` Vadim Fedorenko
2022-10-04 6:33 ` Jiri Pirko
1 sibling, 0 replies; 36+ messages in thread
From: Vadim Fedorenko @ 2022-10-03 17:20 UTC (permalink / raw)
To: Jakub Kicinski, Jiri Pirko, Vadim Fedorenko
Cc: Arkadiusz Kubalewski, Jonathan Lemon, Aya Levin, netdev,
linux-arm-kernel, linux-clk, Gal Pressman
On 03.10.2022 15:28, Jakub Kicinski wrote:
> On Sun, 2 Oct 2022 16:35:07 +0200 Jiri Pirko wrote:
>>>> What I'm trying to say
>>>> is, perhaps sysfs is a better API for this purpose. The API looks very
>>>> neat and there is no probabilito of huge grow.
>>>
>>> "this API is nice and small" said everyone about every new API ever,
>>> APIs grow.
>>
>> Sure, what what are the odds.
>
> The pins were made into full objects now, and we also model muxes.
>
> Vadim, could you share the link to the GH repo?
I've already shared the link to Jiri.
>
> What's your feeling on posting the latest patches upstream as RFC,
> whatever state things are in right now?
>
I'll post RFC v3 once we agree on locking mechanics for pins. Right now there is
no good solution AFAIU, discussions are in progress.
> My preference would be to move the development to the list at this
> stage, FWIW.
I got this point and I will follow it once we will have something ready.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC PATCH v2 0/3] Create common DPLL/clock configuration API
2022-10-03 14:28 ` Jakub Kicinski
2022-10-03 17:20 ` Vadim Fedorenko
@ 2022-10-04 6:33 ` Jiri Pirko
1 sibling, 0 replies; 36+ messages in thread
From: Jiri Pirko @ 2022-10-04 6:33 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vadim Fedorenko, Vadim Fedorenko, Arkadiusz Kubalewski,
Jonathan Lemon, Aya Levin, netdev, linux-arm-kernel, linux-clk,
Gal Pressman
Mon, Oct 03, 2022 at 04:28:31PM CEST, kuba@kernel.org wrote:
>On Sun, 2 Oct 2022 16:35:07 +0200 Jiri Pirko wrote:
>>>> What I'm trying to say
>>>> is, perhaps sysfs is a better API for this purpose. The API looks very
>>>> neat and there is no probabilito of huge grow.
>>>
>>> "this API is nice and small" said everyone about every new API ever,
>>> APIs grow.
>>
>> Sure, what what are the odds.
>
>The pins were made into full objects now, and we also model muxes.
>
>Vadim, could you share the link to the GH repo?
>
>What's your feeling on posting the latest patches upstream as RFC,
>whatever state things are in right now?
>
>My preference would be to move the development to the list at this
>stage, FWIW.
I agree, that would be great.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 36+ messages in thread