Message-ID: <DM6PR11MB46576B19A5DBA46C20AC26679BDA9@DM6PR11MB4657.namprd11.prod.outlook.com>
Date:   Mon, 6 Feb 2023 02:00:09 +0000
From:   "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To:     Jiri Pirko <jiri@...nulli.us>
CC:     Vadim Fedorenko <vadfed@...a.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "Olech, Milena" <milena.olech@...el.com>,
        "Michalik, Michal" <michal.michalik@...el.com>
Subject: RE: [RFC PATCH v5 1/4] dpll: Add DPLL framework base functions

>From: Jiri Pirko <jiri@...nulli.us>
>Sent: Tuesday, January 31, 2023 3:01 PM
>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@...el.com>
>Cc: Vadim Fedorenko <vadfed@...a.com>; Jakub Kicinski <kuba@...nel.org>;
>Jonathan Lemon <jonathan.lemon@...il.com>; Paolo Abeni <pabeni@...hat.com>;
>netdev@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
>clk@...r.kernel.org; Olech, Milena <milena.olech@...el.com>; Michalik,
>Michal <michal.michalik@...el.com>
>Subject: Re: [RFC PATCH v5 1/4] dpll: Add DPLL framework base functions
>
>Fri, Jan 27, 2023 at 07:12:41PM CET, arkadiusz.kubalewski@...el.com wrote:
>>>From: Jiri Pirko <jiri@...nulli.us>
>>>Sent: Thursday, January 19, 2023 6:16 PM
>>>
>>>Tue, Jan 17, 2023 at 07:00:48PM CET, vadfed@...a.com wrote:
>
>[...]
>
>
>>>>+struct dpll_device *dpll_device_get_by_clock_id(u64 clock_id,
>>>
>>>Hmm, don't you want to put an owner module as an arg here as well? I
>>>don't see how could 2 modules sanely work with the same dpll instance.
>>>
>>
>>Sorry, I don't get it.
>>How the driver that needs to find a dpll would know the owner module?
>
>Something like:
>dpll = dpll_device_get(ops, THIS_MODULE, ...)
>if (IS_ERR(dpll))
>	..
>

Oh yeah, sure, probably can do this.

>
>>The idea of this is to let another driver instance to find a dpll device
>>already registered in OS.
>>The driver that is searching dpll device is not the same as the one that
>>has
>>created the device, otherwise it wouldn't make any sense?
>
>You have to distinguish driver/driver_instance. It it is not the same
>driver(module), something is seriously wrong here.
>

Sure, will try to use THIS_MODULE as suggested in next version.

>
>>
>>>
>>>>+						enum dpll_type type, u8 idx)
>>>>+{
>>>>+	struct dpll_device *dpll, *ret = NULL;
>>>>+	unsigned long index;
>>>>+
>>>>+	mutex_lock(&dpll_device_xa_lock);
>>>>+	xa_for_each_marked(&dpll_device_xa, index, dpll, DPLL_REGISTERED) {
>>>>+		if (dpll->clock_id == clock_id) {
>>>>+			if (dpll->type == type) {
>>>>+				if (dpll->dev_driver_idx == idx) {
>>>>+					ret = dpll;
>>>>+					break;
>>>>+				}
>>>>+			}
>>>>+		}
>>>>+	}
>>>>+	mutex_unlock(&dpll_device_xa_lock);
>>>>+
>>>>+	return ret;
>>>>+}
>>>>+EXPORT_SYMBOL_GPL(dpll_device_get_by_clock_id);
>>>>+
>>>>+static void dpll_device_release(struct device *dev)
>>>>+{
>>>>+	struct dpll_device *dpll;
>>>>+
>>>>+	mutex_lock(&dpll_device_xa_lock);
>>>>+	dpll = to_dpll_device(dev);
>>>>+	dpll_device_unregister(dpll);
>>>>+	mutex_unlock(&dpll_device_xa_lock);
>>>>+	dpll_device_free(dpll);
>>>>+}
>>>>+
>>>>+static struct class dpll_class = {
>>>>+	.name = "dpll",
>>>>+	.dev_release = dpll_device_release,
>>>
>>>Why do you want to do this? Why the driver cannot do
>>>dpll_device_unregister/free() manually. I think it makes things easier
>>>to read then to rely on dev garbage collector.
>>>
>>
>>This was in the first version submitted by Vadim.
>>I think we can remove it, unless someone has different view?
>
>Cool.
>
>
>>
>>>
>>>>+};
>>>>+
>>>>+struct dpll_device
>>>>+*dpll_device_alloc(struct dpll_device_ops *ops, enum dpll_type type,
>>>>+		   const u64 clock_id, enum dpll_clock_class clock_class,
>>>>+		   u8 dev_driver_idx, void *priv, struct device *parent)
>>>>+{
>>>>+	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->parent = parent;
>>>>+	dpll->type = type;
>>>>+	dpll->dev_driver_idx = dev_driver_idx;
>>>>+	dpll->clock_id = clock_id;
>>>>+	dpll->clock_class = clock_class;
>>>>+
>>>>+	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_%s_%d_%d", dev_name(parent), type,
>>>>+		     dev_driver_idx);
>>>>+	dpll->priv = priv;
>>>>+	xa_init_flags(&dpll->pins, XA_FLAGS_ALLOC);
>>>>+	xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);
>>>
>>>What is exactly the point of using this mark?
>>>
>>
>>I think this can be also removed now, as there is no separated
>>alloc/register
>>for newly created dpll device.
>
>Cool.
>
>
>>
>>>
>>>>+	mutex_unlock(&dpll_device_xa_lock);
>>>>+	dpll_notify_device_create(dpll);
>>>>+
>>>>+	return dpll;
>>>>+
>>>>+error:
>>>>+	mutex_unlock(&dpll_device_xa_lock);
>>>>+	kfree(dpll);
>>>>+	return ERR_PTR(ret);
>>>>+}
>>>>+EXPORT_SYMBOL_GPL(dpll_device_alloc);
>
>[...]
>
>
>>>>+			return -EEXIST;
>>>>+	}
>>>>+
>>>>+	ret = xa_alloc(pins, &pin->idx, pin, xa_limit_16b, GFP_KERNEL);
>>>>+	if (!ret)
>>>>+		xa_set_mark(pins, pin->idx, PIN_REGISTERED);
>>>
>>>What is exactly the point of having this mark?
>>>
>>
>>Think this could be now removed, we got rid of separated alloc/register
>>for
>>dpll device.
>
>Cool.
>
>
>>
>>>
>>>>+
>>>>+	return ret;
>>>>+}
>>>>+
>>>>+static int dpll_pin_ref_add(struct dpll_pin *pin, struct dpll_device
>>>>*dpll,
>>>>+			    struct dpll_pin_ops *ops, void *priv)
>>>>+{
>>>>+	struct dpll_pin_ref *ref, *pos;
>>>>+	unsigned long index;
>>>>+	u32 idx;
>>>>+
>>>>+	ref = kzalloc(sizeof(struct dpll_pin_ref), GFP_KERNEL);
>>>>+	if (!ref)
>>>>+		return -ENOMEM;
>>>>+	ref->dpll = dpll;
>>>>+	ref->ops = ops;
>>>>+	ref->priv = priv;
>>>>+	if (!xa_empty(&pin->ref_dplls)) {
>>>
>>>Pointless check. Just do iterate.
>>>
>>
>>Sure, will do.
>>
>>>
>>>>+		xa_for_each(&pin->ref_dplls, index, pos) {
>>>>+			if (pos->dpll == ref->dpll)
>>>>+				return -EEXIST;
>>>>+		}
>>>>+	}
>>>>+
>>>>+	return xa_alloc(&pin->ref_dplls, &idx, ref, xa_limit_16b,
>>>>GFP_KERNEL);
>>>>+}
>>>>+
>>>>+static void dpll_pin_ref_del(struct dpll_pin *pin, struct dpll_device
>>>>*dpll)
>>>>+{
>>>>+	struct dpll_pin_ref *pos;
>>>>+	unsigned long index;
>>>>+
>>>>+	xa_for_each(&pin->ref_dplls, index, pos) {
>>>>+		if (pos->dpll == dpll) {
>>>>+			WARN_ON_ONCE(pos != xa_erase(&pin->ref_dplls, index));
>>>>+			break;
>>>>+		}
>>>>+	}
>>>>+}
>>>>+
>>>>+static int pin_deregister_from_xa(struct xarray *xa_pins, struct
>>dpll_pin
>>>>*pin)
>>>
>>>1) dpll_ prefix
>>
>>Sure, will do.
>>
>>>2) "deregister" is odd name
>>
>>Rodger that, will fix.
>>
>>>3) why don't you have it next to dpll_alloc_pin_on_xa() as it is a
>>>   symmetric function?
>>
>>Will do.
>>
>>>4) Why exactly just xa_erase() would not do?
>>
>>Might do, but need to rethink this :)
>
>Great :)
>
>
>>
>>>
>>>>+{
>>>>+	struct dpll_pin *pos;
>>>>+	unsigned long index;
>>>>+
>>>>+	xa_for_each(xa_pins, index, pos) {
>>>>+		if (pos == pin) {
>>>>+			WARN_ON_ONCE(pos != xa_erase(xa_pins, index));
>>>
>>>You have an odd pattern of functions getting pin as an arg then
>>>doing lookup for the same pin. I have to be missing to see some
>>>black magic here :O
>>>
>>
>>The black magic was done to target correct pin in case pin index differs
>>between dplls it was registered with. It would depend on the way shared
>>pins
>>are going to be allocated.
>>If mixed pins approach is allowed (shared + non-shared pins) on any dpll,
>>we
>>would end up in situation where pin index for the same physical pin on
>>multiple
>>devices may be different, depending on registering pins order.
>>
>>As desribed in below comments, I can see here one simple solution: allow
>>kernel
>>module (which registers a pin with dpll) to control/assign pin index.
>>The kernel module would only need take care of them being unique, when
>>registers with first dpll - which seems not a problem. This way we would
>>also
>>be albe to get rid of searching pin function (as indexes would be known
>>for all
>>driver instances), different driver instances would use that index to
>>share a
>>pin.
>>Also all the blackmagic like you described wouldn't be needed, thus
>>simplifing
>>a dpll subsystem.
>
>Good.
>
>
>>
>>>
>>>>+			return 0;
>>>>+		}
>>>>+	}
>>>>+
>>>>+	return -ENXIO;
>>>>+}
>>>>+
>>>>+int dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin,
>>>>+		      struct dpll_pin_ops *ops, void *priv)
>>>>+{
>>>>+	int ret;
>>>>+
>>>>+	if (!pin || !ops)
>>>>+		return -EINVAL;
>>>>+
>>>>+	mutex_lock(&dpll->lock);
>>>>+	ret = dpll_alloc_pin_on_xa(&dpll->pins, pin);
>>>>+	if (!ret) {
>>>>+		ret = dpll_pin_ref_add(pin, dpll, ops, priv);
>>>>+		if (ret)
>>>>+			pin_deregister_from_xa(&dpll->pins, pin);
>>>>+	}
>>>>+	mutex_unlock(&dpll->lock);
>>>>+	if (!ret)
>>>>+		dpll_pin_notify(dpll, pin, DPLL_CHANGE_PIN_ADD);
>>>>+
>>>>+	return ret;
>>>>+}
>>>>+EXPORT_SYMBOL_GPL(dpll_pin_register);
>>>>+
>>>>+struct dpll_pin *dpll_pin_get_by_idx_from_xa(struct xarray *xa_pins,
>>>>int
>>>>idx)
>>>>+{
>>>>+	struct dpll_pin *pos;
>>>>+	unsigned long index;
>>>>+
>>>>+	xa_for_each_marked(xa_pins, index, pos, PIN_REGISTERED) {
>>>>+		if (pos->idx == idx)
>>>>+			return pos;
>>>>+	}
>>>>+
>>>>+	return NULL;
>>>>+}
>>>>+
>>>>+/**
>>>>+ * dpll_pin_get_by_idx - find a pin by its index
>>>>+ * @dpll: dpll device pointer
>>>>+ * @idx: index of pin
>>>>+ *
>>>>+ * Allows multiple driver instances using one physical DPLL to find
>>>>+ * and share pin already registered with existing dpll device.
>>>>+ *
>>>>+ * Return: pointer if pin was found, NULL otherwise.
>>>>+ */
>>>>+struct dpll_pin *dpll_pin_get_by_idx(struct dpll_device *dpll, int idx)
>>>>+{
>>>>+	return dpll_pin_get_by_idx_from_xa(&dpll->pins, idx);
>>>>+}
>>>>+
>>>>+	struct dpll_pin
>>>>+*dpll_pin_get_by_description(struct dpll_device *dpll, const char
>>>>*description)
>>>>+{
>>>>+	struct dpll_pin *pos, *pin = NULL;
>>>>+	unsigned long index;
>>>>+
>>>>+	xa_for_each(&dpll->pins, index, pos) {
>>>>+		if (!strncmp(pos->description, description,
>>>>+			     DPLL_PIN_DESC_LEN)) {
>>>>+			pin = pos;
>>>>+			break;
>>>>+		}
>>>>+	}
>>>>+
>>>>+	return pin;
>>>>+}
>>>>+
>>>>+int
>>>>+dpll_shared_pin_register(struct dpll_device *dpll_pin_owner,
>>>>+			 struct dpll_device *dpll,
>>>>+			 const char *shared_pin_description,
>>>
>>>I don't follow why you need to pass the string. You have struct dpll_pin
>>>* in the driver. Pass that instead, avoid string to refer to kernel
>>>object. But this is something I wrote multiple times.
>>>
>>
>>I wrote this so many times :) Separated driver instances doesn't have the
>>pin
>>object pointer by default (unless they share it through some unwanted
>>static/
>>global contatiners). They need to somehow target a pin, right now only
>>unique
>>attributes on dpll/pin pair are a description and index.
>>Desription is a constant, index depends on the order of initialization and
>>is
>>internal for a dpll device.
>>Previously there were a function to obtain a pin index by its description,
>>then
>>register with obtained index, now this is merged into one function.
>>
>>Altough I agree this is still not best aproach.
>>I will fix by: fallback to targeting a pin to be shared by its index, with
>>one
>>slight design change, the pin index would have to be given by the driver
>>instance which registers it with the first dpll.
>>All the other separated driver instances which are using that pin will
>>have to
>>know the index assigned to the pin that is going to be shared, which seems
>>like a best approach to fix this issue.
>
>>
>>>
>>>>+			 struct dpll_pin_ops *ops, void *priv)
>>>>+{
>>>>+	struct dpll_pin *pin;
>>>>+	int ret;
>>>>+
>>>>+	mutex_lock(&dpll_pin_owner->lock);
>>>>+	pin = dpll_pin_get_by_description(dpll_pin_owner,
>>>>+					  shared_pin_description);
>>>>+	if (!pin) {
>>>>+		ret = -EINVAL;
>>>>+		goto unlock;
>>>>+	}
>>>>+	ret = dpll_pin_register(dpll, pin, ops, priv);
>>>>+unlock:
>>>>+	mutex_unlock(&dpll_pin_owner->lock);
>>>>+
>>>>+	return ret;
>>>
>>>I don't understand why there should be a separate function to register
>>>the shared pin. As I see it, there is a pin object that could be
>>>registered with 2 or more dpll devices. What about having:
>>>
>>>pin = dpll_pin_alloc(desc, type, ops, priv)
>>>dpll_pin_register(dpll_1, pin);
>>>dpll_pin_register(dpll_2, pin);
>>>dpll_pin_register(dpll_3, pin);
>>>
>>
>>IMHO your example works already, but it would possible only if the same
>>driver
>>instance initializes all dplls.
>
>It should be only one instance of dpll to be shared between driver
>instances as I wrote in the reply to the "ice" part. There might he some
>pins created alongside with this.
>

pin = dpll_pin_alloc(desc, type, ops, priv)
dpll_pin_register(dpll_1, pin);
dpll_pin_register(dpll_2, pin);
dpll_pin_register(dpll_3, pin);
^ there is registration of a single pin by a 3 dpll instances, and a kernel
module instance which registers them has a reference to the pin and all dplls,
thus it can just register them all without any problems, don't need to call
dpll_shared_pin_register(..).

Now imagine 2 kernel module instances.
One (#1) creates one dpll, registers pins with it.
Second (#2) creates second dpll, and want to use/register pins of dpll
registered by the first instance (#1).

>My point is, the first driver instance which creates dpll registers also
>the pins. The other driver instance does not do anything, just gets
>reference to the dpll.
>
>On cleanup path, the last driver instance tearing down would unregister
>dpll pins (Could be done automatically by dpll_device_put()).
>
>There might be some other pins (Synce) created per driver instance
>(per-PF). You have to distinguish these 2 groups.
>
>
>>dpll_shared_pin_register is designed for driver instances without the pin
>
>I think we need to make sure the terms are correct "sharing" is between
>multiple dpll instances. However, if 2 driver instances are sharing the
>same dpll instance, this instance has pins. There is no sharing unless
>there is another dpll instance in picture. Correct?
>

Yes!
If two kernel module intances sharing a dpll instance, the pins belong
to the dpll instance, and yes each kernel module instance can register pins
with that dpll instance just with: dpll_pin_register(dpll_1, pin);

dpll_shared_pin_register(..) shall be used when separated kernel module
instances are initializing separated dpll instances, and those instances are
physically sharing their pins.

>
>>object.
>>
>>>Then one pin will we in 3 xa_arrays for 3 dplls.
>>>
>>
>>As we can see dpll_shared_pin_register is a fancy wrapper for
>>dpll_pin_register. So yeah, that's the point :) Just separated driver
>>instances
>>sharing a pin are a issue, will fix with the approach described above (pin
>>index given by the registering driver instance).
>
>Yeah, driver instances and dpll instances are not the same thing. I dpll
>instance per physical dpll. Driver instances should share them.
>

Sure thing, we aim that.

>
>>
>>>
>>>>+}
>>>>+EXPORT_SYMBOL_GPL(dpll_shared_pin_register);
>
>[...]
>
>
>>>>+/**
>>>>+ * dpll_pin_parent - provide pin's parent pin if available
>>>>+ * @pin: registered pin pointer
>>>>+ *
>>>>+ * Return: pointer to aparent if found, NULL otherwise.
>>>>+ */
>>>>+struct dpll_pin *dpll_pin_parent(struct dpll_pin *pin)
>>>
>>>What exactly is the reason of having one line helpers to access struct
>>>fields for a struct which is known to the caller? Unneccesary
>>>boilerplate code. Please remove these. For pin and for dpll_device as
>>>well.
>>>
>>
>>Actually dpll_pin is defined in dpll_core.c, so it is not known to the
>>caller
>>yet. About dpll_device, yes it is known. And we need common approach here,
>>thus
>>we need a fix. I know this is kernel code, full of hacks and performance
>>related
>>bad-design stuff, so will fix as suggested.
>
>You are in the same code, just multiple files. Share the structs in .h
>files internally. Externally (to the drivers), the struct geometry
>should be hidden so the driver does not do some unwanted magic.
>

Yes, will do.

>
>>
>>>
>>>
>>>>+{
>>>>+	return pin->parent_pin;
>>>>+}
>>>>+
>
>[...]
>
>
>>>>+static int dpll_msg_add_pin_modes(struct sk_buff *msg,
>>>>+				   const struct dpll_device *dpll,
>>>>+				   const struct dpll_pin *pin)
>>>>+{
>>>>+	enum dpll_pin_mode i;
>>>>+	bool active;
>>>>+
>>>>+	for (i = DPLL_PIN_MODE_UNSPEC + 1; i <= DPLL_PIN_MODE_MAX; i++) {
>>>>+		if (dpll_pin_mode_active(dpll, pin, i, &active))
>>>>+			return 0;
>>>>+		if (active)
>>>>+			if (nla_put_s32(msg, DPLLA_PIN_MODE, i))
>>>
>>>Why this is signed?
>>>
>>
>>Because enums are signed.
>
>You use negative values in enums? Don't do that here. Have all netlink
>atrributes unsigned please.
>

No, we don't use negative values, but enum is a signed type by itself.
Doesn't seem right thing to do, put signed-type value into unsigned type TLV.
This smells very bad.
 
>
>>
>>>
>>>>+				return -EMSGSIZE;
>>>>+	}
>>>>+
>>>>+	return 0;
>>>>+}
>>>>+
>
>[...]
>
>
>>>>+static struct genl_family dpll_family __ro_after_init = {
>>>>+	.hdrsize	= 0,
>>>
>>>No need for static.
>>>
>>
>>Sorry, don't get it, why it shall be non-static?
>
>Static is already zeroed, you don't need to zero it again.
>

Sure, got it.

>
>>
>>>
>>>>+	.name		= DPLL_FAMILY_NAME,
>>>>+	.version	= DPLL_VERSION,
>>>>+	.ops		= dpll_ops,
>>>>+	.n_ops		= ARRAY_SIZE(dpll_ops),
>>>>+	.mcgrps		= dpll_mcgrps,
>>>>+	.n_mcgrps	= ARRAY_SIZE(dpll_mcgrps),
>>>>+	.pre_doit	= dpll_pre_doit,
>>>>+	.parallel_ops   = true,
>>>>+};
>
>[...]
>
>
>>>>+
>>>>+#define DPLL_FILTER_PINS	1
>>>>+#define DPLL_FILTER_STATUS	2
>>>
>>>Why again do we need any filtering here?
>>>
>>
>>A way to reduce output generated by dump/get requests. Assume the
>>userspace
>>want to have specific information instead of everything in one packet.
>>They might be not needed after we introduce separated "get pin" command.
>
>That's right, not needed.
>
>
>>
>>>
>>>>+
>>>>+/* dplla - Attributes of dpll generic netlink family
>>>>+ *
>>>>+ * @DPLLA_UNSPEC - invalid attribute
>>>>+ * @DPLLA_ID - ID of a dpll device (unsigned int)
>>>>+ * @DPLLA_NAME - human-readable name (char array of DPLL_NAME_LENGTH
>>>size)
>>>>+ * @DPLLA_MODE - working mode of dpll (enum dpll_mode)
>>>>+ * @DPLLA_MODE_SUPPORTED - list of supported working modes (enum
>>>dpll_mode)
>>>>+ * @DPLLA_SOURCE_PIN_ID - ID of source pin selected to drive dpll
>>>
>>>IDX
>>>
>>
>>Sure, will fix.
>>
>>>
>>>>+ *	(unsigned int)
>>>>+ * @DPLLA_LOCK_STATUS - dpll's lock status (enum dpll_lock_status)
>>>>+ * @DPLLA_TEMP - dpll's temperature (signed int - Celsius degrees)
>>>
>>>Hmm, wouldn't it be better to have it as 1/10 of Celsius degree for
>>>example?
>>>
>>
>>As we are not using it, I don't have any strong opinon on this, but seems
>>resonable to me, will add a divider into uAPI like:
>>
>>#define DPLL_TEMP_DIVIDER	10
>
>Okay.
>
>
>>
>>>
>>>>+ * @DPLLA_CLOCK_ID - Unique Clock Identifier of dpll (u64)
>>>>+ * @DPLLA_CLOCK_CLASS - clock quality class of dpll (enum
>>>dpll_clock_class)
>>>>+ * @DPLLA_FILTER - filter bitmask for filtering get and dump requests
>>>>(int,
>>>>+ *	sum of DPLL_DUMP_FILTER_* defines)
>>>>+ * @DPLLA_PIN - nested attribute, each contains single pin attributes
>>>>+ * @DPLLA_PIN_IDX - index of a pin on dpll (unsigned int)
>>>>+ * @DPLLA_PIN_DESCRIPTION - human-readable pin description provided by
>>>>driver
>>>>+ *	(char array of PIN_DESC_LEN size)
>>>>+ * @DPLLA_PIN_TYPE - current type of a pin (enum dpll_pin_type)
>>>>+ * @DPLLA_PIN_SIGNAL_TYPE - current type of a signal
>>>>+ *	(enum dpll_pin_signal_type)
>>>>+ * @DPLLA_PIN_SIGNAL_TYPE_SUPPORTED - pin signal types supported
>>>>+ *	(enum dpll_pin_signal_type)
>>>>+ * @DPLLA_PIN_CUSTOM_FREQ - freq value for
>>>>DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ
>>>>+ *	(unsigned int)
>>>>+ * @DPLLA_PIN_MODE - state of pin's capabilities (enum dpll_pin_mode)
>>>>+ * @DPLLA_PIN_MODE_SUPPORTED - available pin's capabilities
>>>>+ *	(enum dpll_pin_mode)
>>>>+ * @DPLLA_PIN_PRIO - priority of a pin on dpll (unsigned int)
>>>>+ * @DPLLA_PIN_PARENT_IDX - if of a parent pin (unsigned int)
>>>>+ * @DPLLA_PIN_NETIFINDEX - related network interface index for the pin
>>>>+ * @DPLLA_CHANGE_TYPE - type of device change event
>>>>+ *	(enum dpll_change_type)
>>>>+ **/
>>>>+enum dplla {
>>>>+	DPLLA_UNSPEC,
>>>>+	DPLLA_ID,
>>>>+	DPLLA_NAME,
>>>>+	DPLLA_MODE,
>>>>+	DPLLA_MODE_SUPPORTED,
>>>>+	DPLLA_SOURCE_PIN_IDX,
>>>>+	DPLLA_LOCK_STATUS,
>>>>+	DPLLA_TEMP,
>>>>+	DPLLA_CLOCK_ID,
>>>>+	DPLLA_CLOCK_CLASS,
>>>>+	DPLLA_FILTER,
>>>>+	DPLLA_PIN,
>>>>+	DPLLA_PIN_IDX,
>>>>+	DPLLA_PIN_DESCRIPTION,
>>>>+	DPLLA_PIN_TYPE,
>>>>+	DPLLA_PIN_SIGNAL_TYPE,
>>>>+	DPLLA_PIN_SIGNAL_TYPE_SUPPORTED,
>>>>+	DPLLA_PIN_CUSTOM_FREQ,
>>>>+	DPLLA_PIN_MODE,
>>>>+	DPLLA_PIN_MODE_SUPPORTED,
>>>>+	DPLLA_PIN_PRIO,
>>>>+	DPLLA_PIN_PARENT_IDX,
>>>>+	DPLLA_PIN_NETIFINDEX,
>>>
>>>I believe we cannot have this right now. The problem is, ifindexes may
>>>overlay between namespaces. So ifindex without namespace means nothing.
>>>I don't see how this can work from the dpll side.
>>>
>>
>>I am a bit confused, as it seemed we already had an agreement on v4
>discussion
>>on this. And now again you highligheted it with the same reasoning as
>>previously. Has anything changed on that matter?
>
>Not sure what we discussed, but ifindex alone is not enough as ifindexes
>from multiple namespaces overlap.
>
>
>>
>>>Lets assign dpll_pin pointer to netdev and expose it over RT netlink in
>>>a similar way devlink_port is exposed. That should be enough for the
>>>user to find a dpll instance for given netdev.
>>>
>>>It does not have to be part of this set strictly, but I would like to
>>>have it here, so the full picture could be seen.
>>>
>>
>>A "full picture" is getting broken if we would go with another place to
>>keep
>>the reference between pin and device that produces its signal.
>>
>>Why don't we add an 'struct device *' argument for dpll_pin_alloc, create
>>new attribute with dev_name macro similary to the current name of dpll
>>device
>>name, something like: DPLLA_PIN_RCLK_DEVICE (string).
>>This way any device (not only netdev) would be able to add a pin and point
>>to
>>a device which produces its signal.
>
>Okay, that sounds good.
>
>
>>
>>>
>>>
>>>>+	DPLLA_CHANGE_TYPE,
>>>>+	__DPLLA_MAX,
>>>>+};
>>>>+
>>>>+#define DPLLA_MAX (__DPLLA_MAX - 1)
>>>>+
>>>>+/* dpll_lock_status - DPLL status provides information of device status
>>>>+ *
>>>>+ * @DPLL_LOCK_STATUS_UNSPEC - unspecified value
>>>>+ * @DPLL_LOCK_STATUS_UNLOCKED - dpll was not yet locked to any valid
>>(or
>>>is in
>>>>+ *	DPLL_MODE_FREERUN/DPLL_MODE_NCO modes)
>>>>+ * @DPLL_LOCK_STATUS_CALIBRATING - dpll is trying to lock to a valid
>>>signal
>>>>+ * @DPLL_LOCK_STATUS_LOCKED - dpll is locked
>>>>+ * @DPLL_LOCK_STATUS_HOLDOVER - dpll is in holdover state - lost a
>>valid
>>>lock
>>>>+ *	or was forced by DPLL_MODE_HOLDOVER mode)
>>>>+ **/
>>>>+enum dpll_lock_status {
>>>>+	DPLL_LOCK_STATUS_UNSPEC,
>>>>+	DPLL_LOCK_STATUS_UNLOCKED,
>>>>+	DPLL_LOCK_STATUS_CALIBRATING,
>>>>+	DPLL_LOCK_STATUS_LOCKED,
>>>>+	DPLL_LOCK_STATUS_HOLDOVER,
>>>>+
>>>>+	__DPLL_LOCK_STATUS_MAX,
>>>>+};
>>>>+
>>>>+#define DPLL_LOCK_STATUS_MAX (__DPLL_LOCK_STATUS_MAX - 1)
>>>>+
>>>>+/* dpll_pin_type - signal types
>>>>+ *
>>>>+ * @DPLL_PIN_TYPE_UNSPEC - unspecified value
>>>>+ * @DPLL_PIN_TYPE_MUX - mux type pin, aggregates selectable pins
>>>>+ * @DPLL_PIN_TYPE_EXT - external source
>>>>+ * @DPLL_PIN_TYPE_SYNCE_ETH_PORT - ethernet port PHY's recovered clock
>>>>+ * @DPLL_PIN_TYPE_INT_OSCILLATOR - device internal oscillator
>>>>+ * @DPLL_PIN_TYPE_GNSS - GNSS recovered clock
>>>>+ **/
>>>>+enum dpll_pin_type {
>>>>+	DPLL_PIN_TYPE_UNSPEC,
>>>>+	DPLL_PIN_TYPE_MUX,
>>>>+	DPLL_PIN_TYPE_EXT,
>>>>+	DPLL_PIN_TYPE_SYNCE_ETH_PORT,
>>>>+	DPLL_PIN_TYPE_INT_OSCILLATOR,
>>>>+	DPLL_PIN_TYPE_GNSS,
>>>>+
>>>>+	__DPLL_PIN_TYPE_MAX,
>>>>+};
>>>>+
>>>>+#define DPLL_PIN_TYPE_MAX (__DPLL_PIN_TYPE_MAX - 1)
>>>>+
>>>>+/* dpll_pin_signal_type - signal types
>>>>+ *
>>>>+ * @DPLL_PIN_SIGNAL_TYPE_UNSPEC - unspecified value
>>>>+ * @DPLL_PIN_SIGNAL_TYPE_1_PPS - a 1Hz signal
>>>>+ * @DPLL_PIN_SIGNAL_TYPE_10_MHZ - a 10 MHz signal
>>>
>>>Why we need to have 1HZ and 10MHZ hardcoded as enums? Why can't we work
>>>with HZ value directly? For example, supported freq:
>>>1, 10000000
>>>or:
>>>1, 1000
>>>
>>>freq set 10000000
>>>freq set 1
>>>
>>>Simple and easy.
>>>
>>
>>AFAIR, we wanted to have most commonly used frequencies as enums +
>>custom_freq
>>for some exotic ones (please note that there is also possible 2PPS, which
>>is
>>0.5 Hz).
>
>In this exotic case, user might add divider netlink attribute to divide
>the frequency pass in the attr. No problem.
>
>
>>This was design decision we already agreed on.
>>The userspace shall get definite list of comonly used frequencies that can
>>be
>>used with given HW, it clearly enums are good for this.
>
>I don't see why. Each instance supports a set of frequencies. It would
>pass the values to the userspace.
>
>I fail to see the need to have some fixed values listed in enums. Mixing
>approaches for a single attribute is wrong. In ethtool we also don't
>have enum values for 10,100,1000mbits etc. It's just a number.
>

In ethtool there are defines for linkspeeds.
There must be list of defines/enums to check the driver if it is supported.
Especially for ANY_FREQ we don't want to call driver 25 milions times or more.

Also, we have to move supported frequencies to the dpll_pin_alloc as it is
constant argument, supported frequencies shall not change @ runtime?
In such case there seems to be only one way to pass in a nice way, as a
bitmask?

Back to the userspace part, do you suggest to have DPLLA_PIN_FREQ attribute
and translate kernelspace enum values to userspace defines like 
DPLL_FREQ_1_HZ, etc? also with special define for supported ones ANY_FREQ?

>
>>
>>>
>>>>+ * @DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ - custom frequency signal, value
>>>>defined
>>>>+ *	with pin's DPLLA_PIN_SIGNAL_TYPE_CUSTOM_FREQ attribute
>>>>+ **/
>>>>+enum dpll_pin_signal_type {
>>>>+	DPLL_PIN_SIGNAL_TYPE_UNSPEC,
>>>>+	DPLL_PIN_SIGNAL_TYPE_1_PPS,
>>>>+	DPLL_PIN_SIGNAL_TYPE_10_MHZ,
>>>>+	DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ,
>>>>+
>>>>+	__DPLL_PIN_SIGNAL_TYPE_MAX,
>>>>+};
>>>>+
>>>>+#define DPLL_PIN_SIGNAL_TYPE_MAX (__DPLL_PIN_SIGNAL_TYPE_MAX - 1)
>>>>+
>>>>+/* dpll_pin_mode - available pin states
>>>>+ *
>>>>+ * @DPLL_PIN_MODE_UNSPEC - unspecified value
>>>>+ * @DPLL_PIN_MODE_CONNECTED - pin connected
>>>>+ * @DPLL_PIN_MODE_DISCONNECTED - pin disconnected
>>>>+ * @DPLL_PIN_MODE_SOURCE - pin used as an input pin
>>>>+ * @DPLL_PIN_MODE_OUTPUT - pin used as an output pin
>>>>+ **/
>>>>+enum dpll_pin_mode {
>>>>+	DPLL_PIN_MODE_UNSPEC,
>>>>+	DPLL_PIN_MODE_CONNECTED,
>>>>+	DPLL_PIN_MODE_DISCONNECTED,
>>>>+	DPLL_PIN_MODE_SOURCE,
>>>>+	DPLL_PIN_MODE_OUTPUT,
>>>
>>>I don't follow. I see 2 enums:
>>>CONNECTED/DISCONNECTED
>>>SOURCE/OUTPUT
>>>why this is mangled together? How is it supposed to be working. Like a
>>>bitarray?
>>>
>>
>>The userspace shouldn't worry about bits, it recieves a list of
>attributes.
>>For current/active mode: DPLLA_PIN_MODE, and for supported modes:
>>DPLLA_PIN_MODE_SUPPORTED. I.e.
>>
>>	DPLLA_PIN_IDX			0
>>	DPLLA_PIN_MODE			1,3
>>	DPLLA_PIN_MODE_SUPPORTED	1,2,3,4
>
>I believe that mixing apples and oranges in a single attr is not correct.
>Could you please split to separate attrs as drafted below?
>
>>
>>The reason for existance of both DPLL_PIN_MODE_CONNECTED and
>>DPLL_PIN_MODE_DISCONNECTED, is that the user must request it somehow,
>>and bitmask is not a way to go for userspace.
>
>What? See nla_bitmap.
>

AFAIK, nla_bitmap is not yet merged.

>Anyway, why can't you have:
>DPLLA_PIN_CONNECTED     u8 1/0 (bool)
>DPLLA_PIN_DIRECTION     enum { SOURCE/OUTPUT }

Don't get it, why this shall be u8 with bool value, doesn't make much sense for
userspace.
All the other attributes have enum type, we can go with separated attribute:
DPLLA_PIN_STATE		enum { CONNECTED/DISCONNECTED }
Just be consistent and clear, and yes u8 is enough it to keep it, as well as
all of attribute enum values, so we can use u8 instead of u32 for all of them.

Actually for "connected/disconnected"-part there are 2 valid use-cases on my
mind:
- pin can be connected with a number of "parents" (dplls or muxed-pins)
- pin is disconnected entirely
Second case can be achieved with control over first one, thus not need for any
special approach here. Proper control would be to let userspace connect or
disconnect a pin per each node it can be connected with, right?

Then example dump of "get-pins" could look like this:
DPLL_PIN	(nested)
	DPLLA_PIN_IDX		0
	DPLLA_PIN_TYPE		DPLL_PIN_TYPE_EXT
	DPLLA_PIN_DIRECTION	SOURCE
	...
	DPLLA_DPLL			(nested)
		DPLLA_ID		0
		DPLLA_NAME		pci_0000:00:00.0
		DPLLA_PIN_STATE		CONNECTED
	DPLLA_DPLL			(nested)
		DPLLA_ID		1
		DPLLA_NAME		pci_0000:00:00.0
		DPLLA_PIN_STATE		DISCONNECTED

DPLL_PIN	(nested)
	DPLLA_PIN_IDX		1
	DPLLA_PIN_TYPE		DPLL_PIN_TYPE_MUX
	DPLLA_PIN_DIRECTION	SOURCE
	...
	DPLLA_DPLL			(nested)
		DPLLA_ID		0
		DPLLA_NAME		pci_0000:00:00.0
		DPLLA_PIN_STATE		DISCONNECTED
	DPLLA_DPLL			(nested)
		DPLLA_ID		1
		DPLLA_NAME		pci_0000:00:00.0
		DPLLA_PIN_STATE		CONNECTED

DPLL_PIN	(nested)	
	DPLLA_PIN_IDX		2
	DPLLA_PIN_TYPE		DPLL_PIN_TYPE_MUX
	DPLLA_PIN_DIRECTION	SOURCE
	...
	DPLLA_DPLL			(nested)
		DPLLA_ID		0
		DPLLA_NAME		pci_0000:00:00.0
		DPLLA_PIN_STATE		DISCONNECTED
	DPLLA_DPLL			(nested)
		DPLLA_ID		1
		DPLLA_NAME		pci_0000:00:00.0
		DPLLA_PIN_STATE		DISCONNECTED

(similar for muxed pins)
DPLL_PIN	(nested)
	DPLLA_PIN_IDX		3
	DPLLA_PIN_TYPE		DPLL_PIN_TYPE_SYNCE_ETH_PORT
	DPLLA_PIN_DIRECTION	SOURCE
	DPLLA_PIN_PARENT		(nested)
		DPLLA_PIN_IDX		1
		DPLLA_PIN_STATE		DISCONNECTED
	DPLLA_PIN_PARENT		(nested)
		DPLLA_PIN_IDX		2
		DPLLA_PIN_STATE		CONNECTED
			
DPLL_PIN	(nested)
	DPLLA_PIN_IDX		4
	DPLLA_PIN_TYPE		DPLL_PIN_TYPE_SYNCE_ETH_PORT
	DPLLA_PIN_DIRECTION	SOURCE
	DPLLA_PIN_PARENT		(nested)
		DPLLA_PIN_IDX		1
		DPLLA_PIN_STATE		CONNECTED
	DPLLA_PIN_PARENT		(nested)
		DPLLA_PIN_IDX		2
		DPLLA_PIN_STATE		DISCONNECTED

For DPLL_MODE_MANUAL a DPLLA_PIN_STATE would serve also as signal selector
mechanism.
In above example DPLL_ID=0 has only "connected" DPLL_PIN_IDX=0, now when
different pin "connect" is requested:

dpll-set request:
DPLLA_DPLL	(nested)
	DPLLA_ID=0
	DPLLA_NAME=pci_0000:00:00.0
DPLLA_PIN
	DPLLA_PIN_IDX=2
	DPLLA_PIN_CONNECTED=1

Former shall "disconnect"..
And now, dump pin-get:
DPLL_PIN	(nested)
	DPLLA_PIN_IDX		0
	...
	DPLLA_DPLL			(nested)
		DPLLA_ID		0
		DPLLA_NAME		pci_0000:00:00.0
		DPLLA_PIN_STATE		DISCONNECTED
...
DPLL_PIN	(nested)
	DPLLA_PIN_IDX		2
	...
	DPLLA_DPLL			(nested)
		DPLLA_ID		0
		DPLLA_NAME		pci_0000:00:00.0
		DPLLA_PIN_STATE		CONNECTED
		
At least that shall happen on hardware level, right?

As I can't find a use-case to have a pin "connected" but not "selected" in case
of DPLL_MODE_MANUAL.

A bit different is with DPLL_MODE_AUTOMATIC, the pins that connects with dpll
directly could be all connected, and their selection is auto-controlled with a
DPLLA_PIN_PRIO.
But still the user may also request to disconnect a pin - not use it at all
(instead of configuring lowest priority - which allows to use it, if all other
pins propagate invalid signal).

Thus, for DPLL_MODE_AUTOMATIC all ablove is the same with a one difference,
each pin/dpll pair would have a prio, like suggested in the other email.
DPLLA_PIN	(nested)
	...
	DPLLA_DPLL	(nested)
		...
		DPLLA_PIN_CONNECTED	<connected value>
		DPLLA_PIN_STATE		<prio value>

Which basically means that both DPLL_A_PIN_PRIO and DPLLA_PIN_STATE
shall be a property of a PIN-DPLL pair, and configured as such.


>DPLLA_PIN_CAPS          nla_bitfield(CAN_CHANGE_CONNECTED,
>CAN_CHANGE_DIRECTION)
>
>We can use the capabilitis bitfield eventually for other purposes as
>well, it is going to be handy I'm sure.
>

Well, in general I like the idea, altough the details...
We have 3 configuration levels:
- DPLL
- DPLL/PIN
- PIN

Considering that, there is space for 3 of such CAPABILITIES attributes, but:
- DPLL can only configure MODE for now, so we can only convert
DPLL_A_MODE_SUPPORTED to a bitfield, and add DPLL_CAPS later if required
- DPLL/PIN pair has configurable DPLLA_PIN_PRIO and DPLLA_PIN_STATE, so we
could introduce DPLLA_PIN_DPLL_CAPS for them
- PIN has now configurable frequency (but this is done by providing list of
supported ones - no need for extra attribute). We already know that pin shall
also have optional features, like phase offset, embedded sync.
For embedded sync if supported it shall also be a set of supported frequencies.
Possibly for phase offset we could use similar CAPS field, but don't think will
manage this into next version.

>
>
>>
>>
>>>
>>>>+
>>>>+	__DPLL_PIN_MODE_MAX,
>>>>+};
>>>>+
>
>[...]
>
>
>>>>+/**
>>>>+ * dpll_mode - Working-modes a dpll can support. Modes differentiate
>>>>>how
>>>>+ * dpll selects one of its sources to syntonize with a source.
>>>>+ *
>>>>+ * @DPLL_MODE_UNSPEC - invalid
>>>>+ * @DPLL_MODE_MANUAL - source can be only selected by sending a request
>>>>to dpll
>>>>+ * @DPLL_MODE_AUTOMATIC - highest prio, valid source, auto selected by
>>>>dpll
>>>>+ * @DPLL_MODE_HOLDOVER - dpll forced into holdover mode
>>>>+ * @DPLL_MODE_FREERUN - dpll driven on system clk, no holdover
>>>>available
>>>>+ * @DPLL_MODE_NCO - dpll driven by Numerically Controlled Oscillator
>>>
>>>Why does the user care which oscilator is run internally. It's freerun,
>>>isn't it? If you want to expose oscilator type, you should do it
>>>elsewhere.
>>>
>>
>>In NCO user might change frequency of an output, in freerun cannot.
>
>How this could be done?
>

I guess by some internal synchronizer frequency dividers. Same as other output
(different then input) frequencies are achievable there.

Thanks,
Arkadiusz

>
>[...]

Powered by blists - more mailing lists