From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 761F8C7EE24 for ; Mon, 15 May 2023 09:31:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=9ygBiPCeuITlOsw5IHJGmSNqGt5yKr1dzpjA/5tKvfU=; b=30p23polHXW9GQ 7fJWHeJmQmeG4MVT2UB0EJ2eyibq759PKb268i7J3TXQAblm5fZQsJso0JyEPJ+MwZfmxUbB8qPWi rVZJD1Z0K9uXAT/64JDAtbTPT+0S8qtJ0XZizsUtxf+9P6MVbj/M7EJyV7jNsIomu9QeX3yQ+2fdL qhgbdlzhEpJly4GbKGK+G6LpQriZw21kNgh1AJSEpyiM5GqOhs7CyH6Fi5aEqsTg9isqBcCsnhmQx ZUf7AkRsJuuzvCEmMrhiSchZjTnCodLPWW2kpA5znvi3kQacOd8L4MdIfvtt/NqGL3bXBxLkum2EQ J5HXrjOzyRiDBhBBmLaA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pyUXV-001YR6-1w; Mon, 15 May 2023 09:30:45 +0000 Received: from mail-wr1-x436.google.com ([2a00:1450:4864:20::436]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pyUXR-001YQ6-0f for linux-arm-kernel@lists.infradead.org; Mon, 15 May 2023 09:30:44 +0000 Received: by mail-wr1-x436.google.com with SMTP id ffacd0b85a97d-3078fa679a7so8996293f8f.3 for ; Mon, 15 May 2023 02:30:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20221208.gappssmtp.com; s=20221208; t=1684143036; x=1686735036; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=W6BSTCYK/WuLOkz9AG6TW1a3eQQDrhG76l1jFykbWQU=; b=u66WDxVUfm+0HPKxUq0rT+tvZOm6xx/h1Cb4vw2QwXBlTM5jGsxEK+iBZuVyV4zlMb a2KuKq3KcrZiDQc7crBEXsEiJO3H2/m0uxxK8TzTDJLa3I1Rre6LpT6YhXKELrwEsWwy 5xzbq3E4GnxwRos1bYU1/uNQkOfABtcgHvYZmUJz+Si0F1jskldMzs/QvM8vvFbIrYJr aGjMLR69fbVjLSbqGVeWzRSXb4gnrNq5DaTujTw8HXYu0fHz1PQ8yIdULns8fGUNxaY2 4ncf1t70/AoR7F5o+9FTNs/gcGeJuqD6Z/RCVXtWxdf7EQEHl66tUyqJDgX/Xp86Cl4i vPQg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684143036; x=1686735036; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=W6BSTCYK/WuLOkz9AG6TW1a3eQQDrhG76l1jFykbWQU=; b=IWVDG8BOurdF95waTcUcqfTYOj0E1n7wumNXqh2KWwAGkKqfvAijKoqrV1ngbf/jDp /IJctofcRb8MYU4xfHUse40lp5iAMqOzCAx9HoC/BEsBNNt2YCzwLbEOewjVVUyBxvxg rGsQpuH86T7U80iXkYC93NZpb2EC8icbWT/G4UVHAYVkqoPu+oEe3u77LNnXMTYa4MuN 8gmbWP0ICSspPw4Y4LqXx27iMxN8Y4RAVjTk8h0KgGZSWtJpd9skAhDSOk/ShFJEUCJc vfwz4IUOu10/t/1pnkaboJ2R5HlGbmNESgFxm1ORcSDYxzc6dtkj38pyVVvpZnlc3WIH Zf2w== X-Gm-Message-State: AC+VfDzt7qCMcKJFAJ/ind+AaJP1oGfA197eE24K1zPC1MV7YTlc/9s2 p2fHqLdaxZWe6n1Ff6PYKWYIjA== X-Google-Smtp-Source: ACHHUZ7HsO3IDkKNYHrf1UocuJiNC3jR9r3hPnrkeVc35ERImxr3DuADDzUzGPZ+70i4Ke4a3nRwCg== X-Received: by 2002:a5d:58f4:0:b0:306:8f5b:1b49 with SMTP id f20-20020a5d58f4000000b003068f5b1b49mr24839324wrd.47.1684143036436; Mon, 15 May 2023 02:30:36 -0700 (PDT) Received: from localhost (host-213-179-129-39.customer.m-online.net. [213.179.129.39]) by smtp.gmail.com with ESMTPSA id q12-20020a05600000cc00b0030795b2be15sm24162409wrx.103.2023.05.15.02.30.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 15 May 2023 02:30:35 -0700 (PDT) Date: Mon, 15 May 2023 11:30:34 +0200 From: Jiri Pirko To: "Kubalewski, Arkadiusz" Cc: Jakub Kicinski , Vadim Fedorenko , 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: References: <20230428002009.2948020-1-vadfed@meta.com> <20230428002009.2948020-2-vadfed@meta.com> <20230504142451.4828bbb5@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230515_023041_413164_B8B7AECD X-CRM114-Status: GOOD ( 26.41 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Thu, May 11, 2023 at 10:51:43PM CEST, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko >>Sent: Thursday, May 11, 2023 10:00 AM >> >>Thu, May 11, 2023 at 09:40:26AM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jakub Kicinski >>>>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. 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. > >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 > >[...] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel