[RFC PATCH v7 1/8] dpll: spec: Add Netlink spec in YAML
Jiri Pirko
jiri at resnulli.us
Tue May 16 07:33:32 PDT 2023
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?
>
>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 :)
>
>>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