Date:   Mon, 10 Oct 2022 16:13:48 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Vadim Fedorenko <vfedorenko@...ek.ru>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
        netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-clk@...r.kernel.org, Vadim Fedorenko <vadfed@...com>
Subject: Re: [RFC PATCH v3 3/6] dpll: add support for source selection modes

Mon, Oct 10, 2022 at 03:18:01AM CEST, vfedorenko@...ek.ru wrote:
>From: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>
>Allow to configure dpll device for different source selection modes.
>Allow to configure priority of a sources for autmoatic selection mode.
>
>Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>---
> drivers/dpll/dpll_netlink.c | 170 ++++++++++++++++++++++++++++++++++--
> drivers/dpll/dpll_netlink.h |   2 +
> include/linux/dpll.h        |   7 ++
> include/uapi/linux/dpll.h   |  22 ++++-
> 4 files changed, 192 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/dpll/dpll_netlink.c b/drivers/dpll/dpll_netlink.c
>index 6dc92b5b712e..a5779871537a 100644
>--- a/drivers/dpll/dpll_netlink.c
>+++ b/drivers/dpll/dpll_netlink.c
>@@ -23,6 +23,7 @@ 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_DEVICE_SRC_SELECT_MODE] = { .type = NLA_U32 },
> 	[DPLLA_FLAGS]		= { .type = NLA_U32 },
> };
> 
>@@ -38,13 +39,26 @@ static const struct nla_policy dpll_genl_set_output_policy[] = {
> 	[DPLLA_OUTPUT_TYPE]	= { .type = NLA_U32 },
> };
> 
>+static const struct nla_policy dpll_genl_set_src_select_mode_policy[] = {
>+	[DPLLA_DEVICE_ID]		  = { .type = NLA_U32 },
>+	[DPLLA_DEVICE_SRC_SELECT_MODE] = { .type = NLA_U32 },
>+};
>+
>+static const struct nla_policy dpll_genl_set_source_prio_policy[] = {
>+	[DPLLA_DEVICE_ID]	= { .type = NLA_U32 },
>+	[DPLLA_SOURCE_ID]	= { .type = NLA_U32 },
>+	[DPLLA_SOURCE_PRIO]	= { .type = NLA_U32 },
>+};
>+
> struct param {
> 	struct netlink_callback *cb;
> 	struct dpll_device *dpll;
> 	struct sk_buff *msg;
> 	int dpll_id;
>+	int dpll_src_select_mode;
> 	int dpll_source_id;
> 	int dpll_source_type;
>+	int dpll_source_prio;
> 	int dpll_output_id;
> 	int dpll_output_type;
> 	int dpll_status;
>@@ -84,8 +98,8 @@ static int __dpll_cmd_device_dump_one(struct dpll_device *dpll,
> static int __dpll_cmd_dump_sources(struct dpll_device *dpll,
> 					   struct sk_buff *msg)
> {
>+	int i, ret = 0, type, prio;
> 	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);
>@@ -110,6 +124,14 @@ static int __dpll_cmd_dump_sources(struct dpll_device *dpll,
> 			}
> 			ret = 0;
> 		}
>+		if (dpll->ops->get_source_prio) {
>+			prio = dpll->ops->get_source_prio(dpll, i);
>+			if (nla_put_u32(msg, DPLLA_SOURCE_PRIO, prio)) {
>+				nla_nest_cancel(msg, src_attr);
>+				ret = -EMSGSIZE;
>+				break;
>+			}
>+		}
> 		nla_nest_end(msg, src_attr);
> 	}
> 
>@@ -154,26 +176,51 @@ static int __dpll_cmd_dump_outputs(struct dpll_device *dpll,
> static int __dpll_cmd_dump_status(struct dpll_device *dpll,
> 					   struct sk_buff *msg)
> {
>-	int ret;
>+	struct dpll_device_ops *ops = dpll->ops;
>+	int ret, type, attr;
> 
>-	if (dpll->ops->get_status) {
>-		ret = dpll->ops->get_status(dpll);
>+	if (ops->get_status) {
>+		ret = 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 (ops->get_temp) {
>+		ret = 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 (ops->get_lock_status) {
>+		ret = ops->get_lock_status(dpll);
> 		if (nla_put_u32(msg, DPLLA_LOCK_STATUS, ret))
> 			return -EMSGSIZE;
> 	}
> 
>+	if (ops->get_source_select_mode) {
>+		ret = ops->get_source_select_mode(dpll);
>+		if (nla_put_u32(msg, DPLLA_DEVICE_SRC_SELECT_MODE, ret))
>+			return -EMSGSIZE;
>+	} else {
>+		if (nla_put_u32(msg, DPLLA_DEVICE_SRC_SELECT_MODE,
>+				DPLL_SRC_SELECT_FORCED))
>+			return -EMSGSIZE;
>+	}
>+
>+	if (ops->get_source_select_mode_supported) {
>+		attr = DPLLA_DEVICE_SRC_SELECT_MODE_SUPPORTED;
>+		for (type = 0; type <= DPLL_SRC_SELECT_MAX; type++) {
>+			ret = ops->get_source_select_mode_supported(dpll,
>+								    type);
>+			if (ret && nla_put_u32(msg, attr, type))
>+				return -EMSGSIZE;
>+		}
>+	} else {
>+		if (nla_put_u32(msg, DPLLA_DEVICE_SRC_SELECT_MODE_SUPPORTED,
>+				DPLL_SRC_SELECT_FORCED))
>+			return -EMSGSIZE;
>+	}
>+
> 	return 0;
> }
> 
>@@ -275,6 +322,56 @@ static int dpll_genl_cmd_set_output(struct sk_buff *skb, struct genl_info *info)
> 	return ret;
> }
> 
>+static int dpll_genl_cmd_set_source_prio(struct sk_buff *skb, struct genl_info *info)
>+{
>+	struct dpll_device *dpll = info->user_ptr[0];
>+	struct nlattr **attrs = info->attrs;
>+	int ret = 0, src_id, prio;
>+
>+	if (!attrs[DPLLA_SOURCE_ID] ||
>+	    !attrs[DPLLA_SOURCE_PRIO])
>+		return -EINVAL;
>+
>+	if (!dpll->ops->set_source_prio)
>+		return -EOPNOTSUPP;
>+
>+	src_id = nla_get_u32(attrs[DPLLA_SOURCE_ID]);
>+	prio = nla_get_u32(attrs[DPLLA_SOURCE_PRIO]);
>+
>+	mutex_lock(&dpll->lock);
>+	ret = dpll->ops->set_source_prio(dpll, src_id, prio);
>+	mutex_unlock(&dpll->lock);
>+
>+	if (!ret)
>+		dpll_notify_source_prio_change(dpll->id, src_id, prio);
>+
>+	return ret;
>+}
>+
>+static int dpll_genl_cmd_set_select_mode(struct sk_buff *skb, struct genl_info *info)
>+{
>+	struct dpll_device *dpll = info->user_ptr[0];
>+	struct nlattr **attrs = info->attrs;
>+	int ret = 0, mode;
>+
>+	if (!attrs[DPLLA_DEVICE_SRC_SELECT_MODE])
>+		return -EINVAL;
>+
>+	if (!dpll->ops->set_source_select_mode)
>+		return -EOPNOTSUPP;
>+
>+	mode = nla_get_u32(attrs[DPLLA_DEVICE_SRC_SELECT_MODE]);
>+
>+	mutex_lock(&dpll->lock);
>+	ret = dpll->ops->set_source_select_mode(dpll, mode);
>+	mutex_unlock(&dpll->lock);
>+
>+	if (!ret)
>+		dpll_notify_source_select_mode_change(dpll->id, mode);
>+
>+	return ret;
>+}
>+
> static int dpll_device_loop_cb(struct dpll_device *dpll, void *data)
> {
> 	struct dpll_dump_ctx *ctx;
>@@ -397,6 +494,20 @@ static const struct genl_ops dpll_genl_ops[] = {
> 		.policy	= dpll_genl_set_output_policy,
> 		.maxattr = ARRAY_SIZE(dpll_genl_set_output_policy) - 1,
> 	},
>+	{
>+		.cmd	= DPLL_CMD_SET_SRC_SELECT_MODE,
>+		.flags	= GENL_UNS_ADMIN_PERM,
>+		.doit	= dpll_genl_cmd_set_select_mode,
>+		.policy	= dpll_genl_set_src_select_mode_policy,
>+		.maxattr = ARRAY_SIZE(dpll_genl_set_src_select_mode_policy) - 1,
>+	},
>+	{
>+		.cmd	= DPLL_CMD_SET_SOURCE_PRIO,

I don't like the 1 netlink cmd per attr. The commands should be rather
get/set object.


>+		.flags	= GENL_UNS_ADMIN_PERM,
>+		.doit	= dpll_genl_cmd_set_source_prio,
>+		.policy	= dpll_genl_set_source_prio_policy,
>+		.maxattr = ARRAY_SIZE(dpll_genl_set_source_prio_policy) - 1,
>+	},
> };
> 
> static struct genl_family dpll_gnl_family __ro_after_init = {
>@@ -456,6 +567,26 @@ static int dpll_event_output_change(struct param *p)
> 	return 0;
> }
> 
>+static int dpll_event_source_prio(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_PRIO, p->dpll_source_prio))
>+		return -EMSGSIZE;
>+
>+	return 0;
>+}
>+
>+static int dpll_event_select_mode(struct param *p)
>+{
>+	if (nla_put_u32(p->msg, DPLLA_DEVICE_ID, p->dpll_id) ||
>+	    nla_put_u32(p->msg, DPLLA_DEVICE_SRC_SELECT_MODE,
>+		    p->dpll_src_select_mode))
>+		return -EMSGSIZE;
>+
>+	return 0;
>+}
>+
> static const cb_t event_cb[] = {
> 	[DPLL_EVENT_DEVICE_CREATE]	= dpll_event_device_create,
> 	[DPLL_EVENT_DEVICE_DELETE]	= dpll_event_device_delete,
>@@ -463,7 +594,10 @@ static const cb_t event_cb[] = {
> 	[DPLL_EVENT_STATUS_UNLOCKED]	= dpll_event_status,
> 	[DPLL_EVENT_SOURCE_CHANGE]	= dpll_event_source_change,
> 	[DPLL_EVENT_OUTPUT_CHANGE]	= dpll_event_output_change,
>+	[DPLL_EVENT_SOURCE_PRIO]        = dpll_event_source_prio,
>+	[DPLL_EVENT_SELECT_MODE]        = dpll_event_select_mode,
> };
>+
> /*
>  * Generic netlink DPLL event encoding
>  */
>@@ -552,6 +686,26 @@ int dpll_notify_output_change(int dpll_id, int output_id, int output_type)
> }
> EXPORT_SYMBOL_GPL(dpll_notify_output_change);
> 
>+int dpll_notify_source_select_mode_change(int dpll_id, int new_mode)
>+{
>+	struct param p =  { .dpll_id = dpll_id,
>+			    .dpll_src_select_mode = new_mode,
>+			    .dpll_event_group = 0 };
>+
>+	return dpll_send_event(DPLL_EVENT_SELECT_MODE, &p);
>+}
>+EXPORT_SYMBOL_GPL(dpll_notify_source_select_mode_change);
>+
>+int dpll_notify_source_prio_change(int dpll_id, int source_id, int prio)
>+{
>+	struct param p =  { .dpll_id = dpll_id, .dpll_source_id = source_id,
>+			    .dpll_source_prio = prio,
>+			    .dpll_event_group = 1 };
>+
>+	return dpll_send_event(DPLL_EVENT_SOURCE_PRIO, &p);
>+}
>+EXPORT_SYMBOL_GPL(dpll_notify_source_prio_change);
>+
> 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 5c1d1072e818..a4962fa0c8c2 100644
>--- a/drivers/dpll/dpll_netlink.h
>+++ b/drivers/dpll/dpll_netlink.h
>@@ -5,6 +5,8 @@
> 
> int dpll_notify_device_create(int dpll_id, const char *name);
> int dpll_notify_device_delete(int dpll_id);
>+int dpll_notify_source_prio(int dpll_id, int source_id, int prio);
>+int dpll_notify_select_mode(int dpll_id, int mode);
> 
> int __init dpll_netlink_init(void);
> void dpll_netlink_finish(void);
>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>index 32558965cd41..3fe957a06b90 100644
>--- a/include/linux/dpll.h
>+++ b/include/linux/dpll.h
>@@ -12,12 +12,17 @@ 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_select_mode)(struct dpll_device *dpll);
>+	int (*get_source_select_mode_supported)(struct dpll_device *dpll, int type);
> 	int (*get_source_type)(struct dpll_device *dpll, int id);
> 	int (*get_source_supported)(struct dpll_device *dpll, int id, int type);
>+	int (*get_source_prio)(struct dpll_device *dpll, int id);

I don't thing this is a good model to have 1 ops per object attribute.
Did you consider having driver to "register" a source/output with type and
other attributes?



> 	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);
>+	int (*set_source_select_mode)(struct dpll_device *dpll, int mode);
>+	int (*set_source_prio)(struct dpll_device *dpll, int id, int prio);
> };
> 
> struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, const char *name,
>@@ -31,4 +36,6 @@ 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 dpll_notify_source_select_mode_change(int dpll_id, int source_select_mode);
>+int dpll_notify_source_prio_change(int dpll_id, int source_id, int prio);
> #endif
>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>index fcbea5a5e4d6..f6b674e5cf01 100644
>--- a/include/uapi/linux/dpll.h
>+++ b/include/uapi/linux/dpll.h
>@@ -21,10 +21,13 @@ enum dpll_genl_attr {
> 	DPLLA_UNSPEC,
> 	DPLLA_DEVICE_ID,
> 	DPLLA_DEVICE_NAME,
>+	DPLLA_DEVICE_SRC_SELECT_MODE,
>+	DPLLA_DEVICE_SRC_SELECT_MODE_SUPPORTED,
> 	DPLLA_SOURCE,
> 	DPLLA_SOURCE_ID,
> 	DPLLA_SOURCE_TYPE,
> 	DPLLA_SOURCE_SUPPORTED,
>+	DPLLA_SOURCE_PRIO,
> 	DPLLA_OUTPUT,
> 	DPLLA_OUTPUT_ID,
> 	DPLLA_OUTPUT_TYPE,
>@@ -82,6 +85,8 @@ enum dpll_genl_event {
> 	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_SOURCE_PRIO,
>+	DPLL_EVENT_SELECT_MODE,
> 
> 	__DPLL_EVENT_MAX,
> };
>@@ -90,12 +95,27 @@ enum dpll_genl_event {
> /* 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_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_SET_SRC_SELECT_MODE,/* Set mode for selection of a source */
>+	DPLL_CMD_SET_SOURCE_PRIO,	/* Set priority of a source */
> 
> 	__DPLL_CMD_MAX,
> };
> #define DPLL_CMD_MAX (__DPLL_CMD_MAX - 1)
> 
>+/* Source select modes of dpll */
>+enum dpll_genl_source_select_mode {
>+	DPLL_SRC_SELECT_UNSPEC,

consistency please: "source"/"SOURCE".


>+	DPLL_SRC_SELECT_FORCED,   /* Source forced by DPLL_CMD_SET_SOURCE_TYPE */
>+	DPLL_SRC_SELECT_AUTOMATIC,/* highest prio, valid source, auto selected by dpll */
>+	DPLL_SRC_SELECT_HOLDOVER, /* forced holdover */

I think this is mixing up things a bit. I was under impression this is
to set the mode or souce select. So either user select specific source
by index (not type as you suggest above), or you leave the selection up
to the device (according to prio).

Now holdover is not source select.
Isn't think more like admin state/oper state?
oper state would be the lock status
admin state would be "force-holdover"




>+	DPLL_SRC_SELECT_FREERUN,  /* dpll driven on system clk, no holdover available */
>+	DPLL_SRC_SELECT_NCO,	     /* Set the DPLL device output type */

I don't understand what this "NCO" is, the comment didn't help.


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

Powered by blists - more mailing lists