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 99732C77B7D for ; Thu, 18 May 2023 14:03:21 +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=JtHAC4r8ZTymh6AbRZFoqkE5uX4RgLYSoFfPlyJdbNA=; b=OhiZMFs7EDE35/ Mqh8AHWmWbYB9RJYet+mOo94JK3ZG5SohSaFMIDC1/JAZcX5zsnVgccjvlSR1D06jIqHfolUnyHUL pMSdzwKspP36sxcDCIxE7o5O7mgK5BMeo1lUx1G7yLQUongsGtFCyQwjTyoTL1mInAjjrIvC0P/53 RfCsSu56casutc15IrpRXbXrr/85pg3x+P9/PnbEpxsM5UwZ6GfiPub2istlNQh+ytR/vBfBqtYBl j4el1MNiIgiLzqbYqrsQ7AmvlcSM0u6pFlUwTSvcrpB8B6Y036tyCQA6XlqYf4YfgsNaskoRFdVCw il9D0PsHwD0hbiPvtssQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pzeDX-00D8z3-0h; Thu, 18 May 2023 14:02:55 +0000 Received: from mail-ej1-x635.google.com ([2a00:1450:4864:20::635]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pzeDT-00D8xX-0E for linux-arm-kernel@lists.infradead.org; Thu, 18 May 2023 14:02:54 +0000 Received: by mail-ej1-x635.google.com with SMTP id a640c23a62f3a-9659c5b14d8so322968566b.3 for ; Thu, 18 May 2023 07:02:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20221208.gappssmtp.com; s=20221208; t=1684418566; x=1687010566; 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=uD7kSTiPWuwX+1ADZs48IGh6DhAs9FcfnrRRwpbYrzQ=; b=HOxKqmFmJbdu8mHqDiXbfgreSwul+o8HWlGyl0UbruUSHJFT7QGycSf5RZrbbo0rAQ I7VzBcgYXmfWo32648NZUsmX4lK/fLtu2Qka1PVDUq99+ahOJqb8TP0miRErvTngT5YS geWgKnLsjrUlHrzQSBvHBX7Wsr1ws0BnAEdgo3xiCJDcIUpo1Cw1HAjcK6LHpY9tHo8n pAu/ggeaJOtcFY8uN7AdPTA9f5gaCdD+p4swbM2ZP3QtiKDnmDzu9BC/TR2yKERA7Klu JyiO5cvtPcQ3obAB42PCdk+36NWZrXaKc8oTY+d/WaJ5CPk+B7yFqGmAwLnKGF7n6xkg Il2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684418566; x=1687010566; 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=uD7kSTiPWuwX+1ADZs48IGh6DhAs9FcfnrRRwpbYrzQ=; b=V+L2/OhV3d5oo/7lgXpFKXU+gBCv7Fgy9fLoz5IXtPGgH8oNIYsT2EOGlCJfNRGa8k maMdk8sv+Tl+62TLqW/4XoL4g5ite3nhPAqzXKS6qfCFIR9pi5ihq9zESTqfpysUkzd8 DVmx5R4WgALPVUkZUXA3CcWsq7MswbKubqt+fb6o8f1Dr3fte5Xb6iU8Y2ZUDMmNct2Z AF/PRe/JTyPGGSq5YvZso3NZ4p3JikxSgkt0itz3zYINMXZBaBoWBpbIahFz2/b4Ua+R e7EgdrLzQtAZHLUQzHkvWXrwlWHZNNM8pOA0axwHFZPrmkiZz5aMi1jfRjoKYx4C+smg GWww== X-Gm-Message-State: AC+VfDwgdLs6hET6i0trCUsenpOe0du7iGCLUfH3xHwUbqQtj18J3/df VKzMH+XGrojZxfh6/nZct0cmHw== X-Google-Smtp-Source: ACHHUZ4jmrayQs8toXPqTKczKGSnFItmbMOMAbRxyHHvC3+cc2E/0hV2iyG+9PoiR38GIIpOfsytPg== X-Received: by 2002:a17:907:36c6:b0:94f:562b:2979 with SMTP id bj6-20020a17090736c600b0094f562b2979mr40151142ejc.31.1684418566136; Thu, 18 May 2023 07:02:46 -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 ck7-20020a170906c44700b0094e6a9c1d24sm1025637ejb.12.2023.05.18.07.02.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 May 2023 07:02:45 -0700 (PDT) Date: Thu, 18 May 2023 16:02:43 +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-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-20230518_070251_287775_109D45F4 X-CRM114-Status: GOOD ( 27.32 ) 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 18, 2023 at 03:24:45PM CEST, arkadiusz.kubalewski@intel.com wrote: > >>From: Jiri Pirko >>Sent: Tuesday, May 16, 2023 4:34 PM >> >>Tue, May 16, 2023 at 02:05:38PM CEST, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko >>>>Sent: Monday, May 15, 2023 11:31 AM >>>> >>>>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. >>>> >>> >>>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 wait, we are talking about controllig freq of NCO, that has to be a different knob. Anyway, it is dropped, so the discussion is poitless now. > >> >>> >>>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 >>>>> >>>>>[...] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel