Message-ID: <DM6PR11MB46572B45C787C6D1351D606C9BBB9@DM6PR11MB4657.namprd11.prod.outlook.com>
Date:   Wed, 29 Jun 2022 01:46:55 +0000
From:   "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com>
To:     Vadim Fedorenko <vfedorenko@...ek.ru>,
        Jakub Kicinski <kuba@...nel.org>
CC:     Vadim Fedorenko <vadfed@...com>, Aya Levin <ayal@...dia.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>
Subject: RE: [RFC PATCH v1 1/3] dpll: Add DPLL framework base functions

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

Ok, that seems reasonable.

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

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

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

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

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

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

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

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

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

Yes, planning it for tomorrow.

Thanks,
Arkadiusz

>
>Thanks,
>Vadim
>

Powered by blists - more mailing lists