| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <DM6PR11MB4657BD5424F8845A0BB9AF119B7F9@DM6PR11MB4657.namprd11.prod.outlook.com> Date: Thu, 18 May 2023 13:24:45 +0000 From: "Kubalewski, Arkadiusz" <arkadiusz.kubalewski@...el.com> To: Jiri Pirko <jiri@...nulli.us> CC: Jakub Kicinski <kuba@...nel.org>, Vadim Fedorenko <vadfed@...a.com>, Jonathan Lemon <jonathan.lemon@...il.com>, Paolo Abeni <pabeni@...hat.com>, "Olech, Milena" <milena.olech@...el.com>, "Michalik, Michal" <michal.michalik@...el.com>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, poros <poros@...hat.com>, mschmidt <mschmidt@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>, Vadim Fedorenko <vadim.fedorenko@...ux.dev> Subject: RE: [RFC PATCH v7 1/8] dpll: spec: Add Netlink spec in YAML >From: Jiri Pirko <jiri@...nulli.us> >Sent: Tuesday, May 16, 2023 4:34 PM > >Tue, May 16, 2023 at 02:05:38PM CEST, arkadiusz.kubalewski@...el.com wrote: >>>From: Jiri Pirko <jiri@...nulli.us> >>>Sent: Monday, May 15, 2023 11:31 AM >>> >>>Thu, May 11, 2023 at 10:51:43PM CEST, arkadiusz.kubalewski@...el.com wrote: >>>>>From: Jiri Pirko <jiri@...nulli.us> >>>>>Sent: Thursday, May 11, 2023 10:00 AM >>>>> >>>>>Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@...el.com >>>>>wrote: >>>>>>>From: Jakub Kicinski <kuba@...nel.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 >>>> >>>>[...]
Powered by blists - more mailing lists