[RFC PATCH v7 1/8] dpll: spec: Add Netlink spec in YAML
Kubalewski, Arkadiusz
arkadiusz.kubalewski at intel.com
Thu May 18 06:24:45 PDT 2023
>From: Jiri Pirko <jiri at resnulli.us>
>Sent: Tuesday, May 16, 2023 4:34 PM
>
>Tue, May 16, 2023 at 02:05:38PM CEST, arkadiusz.kubalewski at intel.com wrote:
>>>From: Jiri Pirko <jiri at resnulli.us>
>>>Sent: Monday, May 15, 2023 11:31 AM
>>>
>>>Thu, May 11, 2023 at 10:51:43PM CEST, arkadiusz.kubalewski at intel.com wrote:
>>>>>From: Jiri Pirko <jiri at resnulli.us>
>>>>>Sent: Thursday, May 11, 2023 10:00 AM
>>>>>
>>>>>Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski at intel.com
>>>>>wrote:
>>>>>>>From: Jakub Kicinski <kuba at kernel.org>
>>>>>>>Sent: Thursday, May 4, 2023 11:25 PM
>>>>>>>
>>>>>>>On Thu, 4 May 2023 14:02:30 +0200 Jiri Pirko wrote:
>>>>>>>> >+definitions:
>>>>>>>> >+ -
>>>>>>>> >+ type: enum
>>>>>>>> >+ name: mode
>>>>>>>> >+ doc: |
>>>>>>>> >+ working-modes a dpll can support, differentiate if and how
>>>>>>>> >dpll selects
>>>>>>>> >+ one of its sources to syntonize with it, valid values for
>>>>>>>> >DPLL_A_MODE
>>>>>>>> >+ attribute
>>>>>>>> >+ entries:
>>>>>>>> >+ -
>>>>>>>> >+ name: unspec
>>>>>>>>
>>>>>>>> In general, why exactly do we need unspec values in enums and CMDs?
>>>>>>>> What is the usecase. If there isn't please remove.
>>>>>>>
>>>>>>>+1
>>>>>>>
>>>>>>
>>>>>>Sure, fixed.
>>>>>>
>>>>>>>> >+ doc: unspecified value
>>>>>>>> >+ -
>>>>>>>> >+ name: manual
>>>>>>>
>>>>>>>I think the documentation calls this "forced", still.
>>>>>>>
>>>>>>
>>>>>>Yes, good catch, fixed docs.
>>>>>>
>>>>>>>> >+ doc: source can be only selected by sending a request to
>>>>>>>> >dpll
>>>>>>>> >+ -
>>>>>>>> >+ name: automatic
>>>>>>>> >+ doc: highest prio, valid source, auto selected by dpll
>>>>>>>> >+ -
>>>>>>>> >+ name: holdover
>>>>>>>> >+ doc: dpll forced into holdover mode
>>>>>>>> >+ -
>>>>>>>> >+ name: freerun
>>>>>>>> >+ doc: dpll driven on system clk, no holdover available
>>>>>>>>
>>>>>>>> Remove "no holdover available". This is not a state, this is a mode
>>>>>>>> configuration. If holdover is or isn't available, is a runtime info.
>>>>>>>
>>>>>>>Agreed, seems a little confusing now. Should we expose the system clk
>>>>>>>as a pin to be able to force lock to it? Or there's some extra magic
>>>>>>>at play here?
>>>>>>
>>>>>>In freerun you cannot lock to anything it, it just uses system clock from
>>>>>>one of designated chip wires (which is not a part of source pins pool) to
>>>>>>feed the dpll. Dpll would only stabilize that signal and pass it further.
>>>>>>Locking itself is some kind of magic, as it usually takes at least ~15
>>>>>>seconds before it locks to a signal once it is selected.
>>>>>>
>>>>>>>
>>>>>>>> >+ -
>>>>>>>> >+ name: nco
>>>>>>>> >+ doc: dpll driven by Numerically Controlled Oscillator
>>>>>>>
>>>>>>>Noob question, what is NCO in terms of implementation?
>>>>>>>We source the signal from an arbitrary pin and FW / driver does
>>>>>>>the control? Or we always use system refclk and then tune?
>>>>>>>
>>>>>>
>>>>>>Documentation of chip we are using, stated NCO as similar to FREERUN, and
>>>>>>it
>>>>>
>>>>>So how exactly this is different to freerun? Does user care or he would
>>>>>be fine with "freerun" in this case? My point is, isn't "NCO" some
>>>>>device specific thing that should be abstracted out here?
>>>>>
>>>>
>>>>Sure, it is device specific, some synchronizing circuits would have this
>>>>capability, while others would not.
>>>>Should be abstracted out? It is a good question.. shall user know that he
>>>>is in
>>>>freerun with possibility to control the frequency or not?
>>>>Let's say we remove NCO, and have dpll with enabled FREERUN mode and pins
>>>>supporting multiple output frequencies.
>>>>How the one would know if those frequencies are supported only in
>>>>MANUAL/AUTOMATIC modes or also in the FREERUN mode?
>>>>In other words: As the user can I change a frequency of a dpll if active
>>>>mode is FREERUN?
>>>
>>>Okay, I think I'm deep in the DPLL infra you are pushing, but my
>>>understanding that you can control frequency in NCO mode is not present
>>>:/ That only means it may be confusing and not described properly.
>>>How do you control this frequency exactly? I see no such knob.
>>>
>>
>>The set frequency is there already, although we miss phase offset I guess.
>
>Yeah, but on a pin, right?
>
>
Yes frequency of an output pin is configurable, phase offset for a dpll or
output is not there, we might think of adding it..
>
>>
>>But I have changed my mind on having this in the kernel..
>>Initially I have added this mode as our HW supports it, while thinking that
>>dpll subsystem shall have this, and we will implement it one day..
>>But as we have not implemented it yet, let's leave work and discussion on
>>this mode for the future, when someone will actually try to implement it.
>
>Yeah, let's drop it then. One less confusing thing to wrap a head around :)
>
Dropped.
Thank you!
Arkadiusz
>
>>
>>>Can't the oscilator be modeled as a pin and then you are not in freerun
>>>but locked this "internal pin"? We know how to control frequency there.
>>>
>>
>>Hmm, yeah probably could work this way.
>>
>>
>>Thank you!
>>Arkadiusz
>>
>>>
>>>>
>>>>I would say it is better to have such mode, we could argue on naming
>>>>>though.
>>>>
>>>>>
>>>>>>runs on a SYSTEM CLOCK provided to the chip (plus some stabilization and
>>>>>>dividers before it reaches the output).
>>>>>>It doesn't count as an source pin, it uses signal form dedicated wire for
>>>>>>SYSTEM CLOCK.
>>>>>>In this case control over output frequency is done by synchronizer chip
>>>>>>firmware, but still it will not lock to any source pin signal.
>>>>>>
>>>>>>>> >+ render-max: true
>>>>>>>> >+ -
>>>>>>>> >+ type: enum
>>>>>>>> >+ name: lock-status
>>>>>>>> >+ doc: |
>>>>>>>> >+ provides information of dpll device lock status, valid
>>>>>>>> >>values for
>>>>>>>> >+ DPLL_A_LOCK_STATUS attribute
>>>>>>>> >+ entries:
>>>>>>>> >+ -
>>>>>>>> >+ name: unspec
>>>>>>>> >+ doc: unspecified value
>>>>>>>> >+ -
>>>>>>>> >+ name: unlocked
>>>>>>>> >+ doc: |
>>>>>>>> >+ dpll was not yet locked to any valid source (or is in one
>>>>>>>> >of
>>>>>>>> >+ modes: DPLL_MODE_FREERUN, DPLL_MODE_NCO)
>>>>>>>> >+ -
>>>>>>>> >+ name: calibrating
>>>>>>>> >+ doc: dpll is trying to lock to a valid signal
>>>>>>>> >+ -
>>>>>>>> >+ name: locked
>>>>>>>> >+ doc: dpll is locked
>>>>>>>> >+ -
>>>>>>>> >+ name: holdover
>>>>>>>> >+ doc: |
>>>>>>>> >+ dpll is in holdover state - lost a valid lock or was
>>>>>>>> >forced by
>>>>>>>> >+ selecting DPLL_MODE_HOLDOVER mode
>>>>>>>>
>>>>>>>> Is it needed to mention the holdover mode. It's slightly confusing,
>>>>>>>> because user might understand that the lock-status is always "holdover"
>>>>>>>> in case of "holdover" mode. But it could be "unlocked", can't it?
>>>>>>>> Perhaps I don't understand the flows there correctly :/
>>>>>>>
>>>>>>>Hm, if we want to make sure that holdover mode must result in holdover
>>>>>>>state then we need some extra atomicity requirements on the SET
>>>>>>>operation. To me it seems logical enough that after setting holdover
>>>>>>>mode we'll end up either in holdover or unlocked status, depending on
>>>>>>>lock status when request reached the HW.
>>>>>>>
>>>>>>
>>>>>>Improved the docs:
>>>>>> name: holdover
>>>>>> doc: |
>>>>>> dpll is in holdover state - lost a valid lock or was forced
>>>>>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only
>>>>>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED,
>>>>>> if it was not, the dpll's lock-status will remain
>>>>>
>>>>>"if it was not" does not really cope with the sentence above that. Could
>>>>>you iron-out the phrasing a bit please?
>>>>
>>>>
>>>>Hmmm,
>>>> name: holdover
>>>> doc: |
>>>> dpll is in holdover state - lost a valid lock or was forced
>>>> by selecting DPLL_MODE_HOLDOVER mode (latter possible only
>>>> when dpll lock-state was already DPLL_LOCK_STATUS_LOCKED,
>>>> if dpll lock-state was not DPLL_LOCK_STATUS_LOCKED, the
>>>> dpll's lock-state shall remain DPLL_LOCK_STATUS_UNLOCKED
>>>> even if DPLL_MODE_HOLDOVER was requested)
>>>>
>>>>Hope this is better?
>>>
>>>Okay.
>>>
>>>>
>>>>
>>>>Thank you!
>>>>Arkadiusz
>>>>
>>>>[...]
More information about the linux-arm-kernel
mailing list