[RFC PATCH v1 1/3] dpll: Add DPLL framework base functions
Vadim Fedorenko
vfedorenko at novek.ru
Wed Jun 29 16:30:08 PDT 2022
On 29.06.2022 02:46, Kubalewski, Arkadiusz wrote:
> -----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_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.
>
Yeah, got it. Will re-implement locking for next iteration.
>>
>> ... skipped ...
>>
>>>>>> +
>>>>>> +/* 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.
Ok. This is the way I was thinking about too, will make an implementation.
>
> For adjusting phase offset it would be great to have set/get of s64 phase
> offset.
This way it's getting closer and closer to ptp, but still having phase offset is
fair point and I will go this way. Jakub, do you have any objections?
>
> 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.
>
Didn't get the reason to have special eSync with the same options again. We
might want a kind of flag/indicator that the port has eSync?
>>
>>>>
>>>>> - 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
>>
Thanks,
Vadim
More information about the linux-arm-kernel
mailing list