From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 49EFA629 for ; Thu, 11 May 2023 15:20:55 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6B049C4339B; Thu, 11 May 2023 15:20:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1683818454; bh=LlEqzQak7c2TgFwwm3ndbqMEHDmT7FAykIqojK/MUaI=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=DeYnWczTbjryXUB8lBvhvHWi0u2Rm/cpjy+OQhjGO4NDivP+aEqko9+fC9a49IRTD iHzd80Bck8SR4V5N7RBxfAvGushssgM3AY2IVsYFCrurag+7IIvfkoN7zJtTuUByIM r/M5lCaAGascT2J7/uNiJohBvZUppQhsB2HGRKFZfGzl+nZg6PTEN+YdKJzuaovSGS Qk16NnM3cDQBCsGq28ghEaArT0tDFYrfSLfSHKxdYe86YJ6wMM2+9xAFSaujIOFKoK 3R9s2LiF1Cs4cClofYGpRebTh/3KvbG9duNxgWvXOg14n9M0K0FwaSlin1GBYLhf1R qGHwtPD+kMuyw== Date: Thu, 11 May 2023 08:20:53 -0700 From: Jakub Kicinski To: "Kubalewski, Arkadiusz" Cc: Vadim Fedorenko , Jiri Pirko , Jonathan Lemon , Paolo Abeni , "Olech, Milena" , "Michalik, Michal" , "linux-arm-kernel@lists.infradead.org" , poros , mschmidt , "netdev@vger.kernel.org" , "linux-clk@vger.kernel.org" , Vadim Fedorenko Subject: Re: [RFC PATCH v7 1/8] dpll: spec: Add Netlink spec in YAML Message-ID: <20230511082053.7d2e57e3@kernel.org> In-Reply-To: References: <20230428002009.2948020-1-vadfed@meta.com> <20230428002009.2948020-2-vadfed@meta.com> <20230504142451.4828bbb5@kernel.org> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Thu, 11 May 2023 07:40:26 +0000 Kubalewski, Arkadiusz wrote: > >> 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. Okay, I guess that makes sense. I was wondering if there may be a DPLLs which allow other input clocks to bypass the PLL logic, and output purely a stabilized signal. In which case we should model this as a generic PLL bypass, FREERUN being just one special case where we're bypassing with the system clock. But that may well be a case of "software guy thinking", so if nobody thinks this can happen in practice we can keep FREERUN. > >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 > 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. Reading wikipedia it sounds like NCO is just a way of generating a waveform from synchronous logic. Does the DPLL not allow changing clock frequency when locked? I.e. feeding it one frequency and outputting another? Because I think that'd be done by an NCO, no? > >> 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 > DPLL_LOCK_STATUS_UNLOCKED even if user requests > DPLL_MODE_HOLDOVER) > Is that better? Yes, modulo breaking it up into sentences, as Jiri says. > What extra atomicity you have on your mind? > Do you suggest to validate and allow (in dpll_netlink.c) only for 'unlocked' > or 'holdover' states of dpll, once DPLL_MODE_HOLDOVER was successfully > requested by the user? No, I was saying that making sure that we end up in holdover (rather than unlocked) when user requested holdover is hard, and we shouldn't even try to implement that.