[RFC PATCH v1 1/3] dpll: Add DPLL framework base functions
Kubalewski, Arkadiusz
arkadiusz.kubalewski at intel.com
Tue Jun 28 18:46:55 PDT 2022
-----Original Message-----
From: Vadim Fedorenko <vfedorenko at novek.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
>
More information about the linux-arm-kernel
mailing list