Date:   Wed, 15 Mar 2023 15:43:03 +0100
From:   Jiri Pirko <jiri@...nulli.us>
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>,
        Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
        poros <poros@...hat.com>, mschmidt <mschmidt@...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: [PATCH RFC v6 2/6] dpll: Add DPLL framework base functions

Tue, Mar 14, 2023 at 07:35:55PM CET, arkadiusz.kubalewski@...el.com wrote:
>>From: Jiri Pirko <jiri@...nulli.us>
>>Sent: Tuesday, March 14, 2023 4:45 PM
>>
>>[...]
>>
>>
>>>diff --git a/MAINTAINERS b/MAINTAINERS
>>>index edd3d562beee..0222b19af545 100644
>>>--- a/MAINTAINERS
>>>+++ b/MAINTAINERS
>>>@@ -6289,6 +6289,15 @@ F:
>>	Documentation/networking/device_drivers/ethernet/freescale/dpaa2/swit
>>ch-drive
>>> F:	drivers/net/ethernet/freescale/dpaa2/dpaa2-switch*
>>> F:	drivers/net/ethernet/freescale/dpaa2/dpsw*
>>>
>>>+DPLL CLOCK SUBSYSTEM
>>
>>Why "clock"? You don't mention "clock" anywhere else.
>>
>>[...]
>>
>>
>>>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>>>new file mode 100644
>>>index 000000000000..3fc151e16751
>>>--- /dev/null
>>>+++ b/drivers/dpll/dpll_core.c
>>>@@ -0,0 +1,835 @@
>>>+// SPDX-License-Identifier: GPL-2.0
>>>+/*
>>>+ *  dpll_core.c - Generic DPLL Management class support.
>>
>>Why "class" ?
>>
>>[...]
>>
>>
>>>+static int
>>>+dpll_msg_add_pin_freq(struct sk_buff *msg, const struct dpll_pin *pin,
>>>+		      struct netlink_ext_ack *extack, bool dump_any_freq)
>>>+{
>>>+	enum dpll_pin_freq_supp fs;
>>>+	struct dpll_pin_ref *ref;
>>>+	unsigned long i;
>>>+	u32 freq;
>>>+
>>>+	xa_for_each((struct xarray *)&pin->dpll_refs, i, ref) {
>>>+		if (ref && ref->ops && ref->dpll)
>>>+			break;
>>>+	}
>>>+	if (!ref || !ref->ops || !ref->dpll)
>>>+		return -ENODEV;
>>>+	if (!ref->ops->frequency_get)
>>>+		return -EOPNOTSUPP;
>>>+	if (ref->ops->frequency_get(pin, ref->dpll, &freq, extack))
>>>+		return -EFAULT;
>>>+	if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY, freq))
>>>+		return -EMSGSIZE;
>>>+	if (!dump_any_freq)
>>>+		return 0;
>>>+	for (fs = DPLL_PIN_FREQ_SUPP_UNSPEC + 1;
>>>+	     fs <= DPLL_PIN_FREQ_SUPP_MAX; fs++) {
>>>+		if (test_bit(fs, &pin->prop.freq_supported)) {
>>>+			if (nla_put_u32(msg, DPLL_A_PIN_FREQUENCY_SUPPORTED,
>>>+			    dpll_pin_freq_value[fs]))
>>
>>This is odd. As I suggested in the yaml patch, better to treat all
>>supported frequencies the same, no matter if it is range or not. The you
>>don't need this weird bitfield.
>>
>>You can have a macro to help driver to assemble array of supported
>>frequencies and ranges.
>>
>
>I understand suggestion on yaml, but here I am confused.
>How do they relate to the supported frequency passed between driver and dpll
>subsystem?
>This bitfield is not visible to the userspace, and sure probably adding macro
>can be useful.

My point is to avoid the bitfield and to treat supported frequencies and
ranges in the same way. It can look similar to this:

in dpll.h:

struct struct dpll_pin_frequency {
	u64 min;
	u64 max;
};

#define DPLL_PIN_FREQUENCY_RANGE(_min, _mac)	\
	{					\
		.min = _min,			\
		.max = _max,			\
	}

#define DPLL_PIN_FREQUENCY(_val) DPLL_PIN_FREQUENCY_RANGE(_val, _val)
#define DPLL_PIN_FREQUENCY_1PPS DPLL_PIN_FREQUENCY(1)
#define DPLL_PIN_FREQUENCY_10MHZ DPLL_PIN_FREQUENCY(1000000)


Then in driver you have:

static const struct dpll_pin_frequency pcp_dpll_pin_freqs[] = {
	DPLL_PIN_FREQUENCY_1PPS,
	DPLL_PIN_FREQUENCY_10MHZ,
	DPLL_PIN_FREQUENCY(4000000),
	DPLL_PIN_FREQUENCY_RANGE(500, 1000),
	DPLL_PIN_FREQUENCY_RANGE(9000, 10000),
};

static const struct dpll_pin_properties pcp_dpll_pin_props = {
	.label = "SMA",
	.frequencies_supported = pcp_dpll_pin_freqs,
	.frequencies_supported_count = ARRAY_SIZE(pcp_dpll_pin_freqs),
	.type = DPLL_PIN_TYPE_EXT,
	.capabilities = DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE,
};


Then the dpll core could very easily iterate over .frequencies_supported
array and dump the supported values and ranges to user in uniform way.


>
>>
>>>+				return -EMSGSIZE;
>>>+		}
>>>+	}
>>>+	if (pin->prop.any_freq_min && pin->prop.any_freq_max) {
>>>+		if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MIN,
>>>+				pin->prop.any_freq_min))
>>>+			return -EMSGSIZE;
>>>+		if (nla_put_u32(msg, DPLL_A_PIN_ANY_FREQUENCY_MAX,
>>>+				pin->prop.any_freq_max))
>>>+			return -EMSGSIZE;
>>>+	}
>>>+
>>>+	return 0;
>>>+}
>>>+
>>
>>[...]
>>
>>
>>>+static int
>>>+dpll_cmd_pin_on_dpll_get(struct sk_buff *msg, struct dpll_pin *pin,
>>>+			 struct dpll_device *dpll,
>>>+			 struct netlink_ext_ack *extack)
>>>+{
>>>+	struct dpll_pin_ref *ref;
>>>+	int ret;
>>>+
>>>+	if (nla_put_u32(msg, DPLL_A_PIN_IDX, pin->dev_driver_id))
>>>+		return -EMSGSIZE;
>>>+	if (nla_put_string(msg, DPLL_A_PIN_DESCRIPTION, pin->prop.description))
>>>+		return -EMSGSIZE;
>>>+	if (nla_put_u8(msg, DPLL_A_PIN_TYPE, pin->prop.type))
>>>+		return -EMSGSIZE;
>>>+	if (nla_put_u32(msg, DPLL_A_PIN_DPLL_CAPS, pin->prop.capabilities))
>>>+		return -EMSGSIZE;
>>>+	ret = dpll_msg_add_pin_direction(msg, pin, extack);
>>>+	if (ret)
>>>+		return ret;
>>>+	ret = dpll_msg_add_pin_freq(msg, pin, extack, true);
>>>+	if (ret && ret != -EOPNOTSUPP)
>>>+		return ret;
>>>+	ref = dpll_xa_ref_dpll_find(&pin->dpll_refs, dpll);
>>>+	if (!ref)
>>
>>How exactly this can happen? Looks to me like only in case of a bug.
>>WARN_ON() perhaps (put directly into dpll_xa_ref_dpll_find()?
>
>Yes, makes sense.
>
>>
>>
>>>+		return -EFAULT;
>>>+	ret = dpll_msg_add_pin_prio(msg, pin, ref, extack);
>>>+	if (ret && ret != -EOPNOTSUPP)
>>>+		return ret;
>>>+	ret = dpll_msg_add_pin_on_dpll_state(msg, pin, ref, extack);
>>>+	if (ret && ret != -EOPNOTSUPP)
>>>+		return ret;
>>>+	ret = dpll_msg_add_pin_parents(msg, pin, extack);
>>>+	if (ret)
>>>+		return ret;
>>>+	if (pin->rclk_dev_name)
>>
>>Use && and single if
>>
>
>Make sense to me.
>
>>
>>>+		if (nla_put_string(msg, DPLL_A_PIN_RCLK_DEVICE,
>>>+				   pin->rclk_dev_name))
>>>+			return -EMSGSIZE;
>>>+
>>>+	return 0;
>>>+}
>>>+
>>
>>[...]
>>
>>
>>>+static int
>>>+dpll_pin_freq_set(struct dpll_pin *pin, struct nlattr *a,
>>>+		  struct netlink_ext_ack *extack)
>>>+{
>>>+	u32 freq = nla_get_u32(a);
>>>+	struct dpll_pin_ref *ref;
>>>+	unsigned long i;
>>>+	int ret;
>>>+
>>>+	if (!dpll_pin_is_freq_supported(pin, freq))
>>>+		return -EINVAL;
>>>+
>>>+	xa_for_each(&pin->dpll_refs, i, ref) {
>>>+		ret = ref->ops->frequency_set(pin, ref->dpll, freq, extack);
>>>+		if (ret)
>>>+			return -EFAULT;
>>
>>return what the op returns: ret
>
>Why would we return here a driver return code, userspace can have this info
>from extack. IMHO return values of dpll subsystem shall be not dependent on
>what is returned from the driver.

Why not to return it? The driver had some problem, errno suggests what
that was. It is completely desired to pass that along and actually,
it's been done like this in the rest of the netlink ops I can think of.
Why would you want to hide it? Extack carries string message,
not related to this directly.


>
>>
>>
>>>+		dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_FREQUENCY);
>>>+	}
>>>+
>>>+	return 0;
>>>+}
>>>+
>>
>>[...]
>>
>>
>>>+static int
>>>+dpll_pin_direction_set(struct dpll_pin *pin, struct nlattr *a,
>>>+		       struct netlink_ext_ack *extack)
>>>+{
>>>+	enum dpll_pin_direction direction = nla_get_u8(a);
>>>+	struct dpll_pin_ref *ref;
>>>+	unsigned long i;
>>>+
>>>+	if (!(DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE & pin->prop.capabilities))
>>>+		return -EOPNOTSUPP;
>>>+
>>>+	xa_for_each(&pin->dpll_refs, i, ref) {
>>>+		if (ref->ops->direction_set(pin, ref->dpll, direction, extack))
>>
>>ret = ..
>>if (ret)
>>	return ret;
>>
>>Please use this pattern in other ops call code as well.
>>
>
>This is the same as above (return code by driver) explanation.

Same reply as above.


>
>>
>>>+			return -EFAULT;
>>>+		dpll_pin_notify(ref->dpll, pin, DPLL_A_PIN_DIRECTION);
>>>+	}
>>>+
>>>+	return 0;
>>
>>[...]
>
>Thanks,
>Arkadiusz

Powered by blists - more mailing lists