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 4841FC77B7A for ; Tue, 16 May 2023 15:45:39 +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=JwELiIWD6T0tfOp4XSbNUxLJvz86InXNP51WZsn9eTg=; b=hg47aOcAtTMtWP QFZiL8QhqEXdfIL3OOxVn2u4R5wA7DmZB9MYidlz3QKo+SGCsYZvUB4v/SUamD8OJ/GB2yjZUnF84 ks2iYIBfP5cWZJ5FXZ/yp2Bvv+SsdWKznmjbo/MCryLWIJ7UfOaqrX3kBR/kc+u1FqFZl5qgkYeHz Ob8pzrtH+hGfE358Qs2MKqRolLgQfmIdeSpeTZ2bGZVzu0DtCWCFDWdPEK+lScJBpcm15NuU3LQoO 262A1xav7SisGkGG/kiBDYnwHisMVjYVD3bYsBSa0D26XANWTmK6Pxbmuml0VFNaCN0pZBbpIphCu 4ZXhr1VP0dn1JtaBIwFg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pywre-006J9X-1Z; Tue, 16 May 2023 15:45:26 +0000 Received: from mail-ed1-x530.google.com ([2a00:1450:4864:20::530]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pyvkA-0066VM-0u for linux-arm-kernel@lists.infradead.org; Tue, 16 May 2023 14:33:42 +0000 Received: by mail-ed1-x530.google.com with SMTP id 4fb4d7f45d1cf-510b56724caso337864a12.1 for ; Tue, 16 May 2023 07:33:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20221208.gappssmtp.com; s=20221208; t=1684247615; x=1686839615; 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=+LK2OpAFas8P3MmZmqwSI9CreYUGyjnN2bl4gbEEJPE=; b=LOB613eVdoxppxmElUsDS9NLW72MvlmqIJ1rWlXnCjikMf7ujDA9v4ZeULFwZeLJj0 kQICBSgRGXDrzZkDr/LfoeDSp1S6n4rfK9slpy6NWsXQoWPnIFiNDVP5lZQ9XwOGbIdZ FSsMlVh43XqnYSu4XJilR2ye8BSPBlnNLzJl84UG9cSMHx2qrR5yaD+hQjZ/sStdKzNE ExOvtv4IfUWD0gGNJGdq690lH0DzEXGzJohxDZv/1aGJk/kK9YZCWYEUBrjEb7JNV89P 5ketniaPbYUKPLswVp96d51TBlL0oKIbAdxX+82vWyszg3RgjY4Yrhfd2RNb0fbcrS4D 8wvg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684247615; x=1686839615; 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=+LK2OpAFas8P3MmZmqwSI9CreYUGyjnN2bl4gbEEJPE=; b=ChXangotLBmRz8WTWwLThQ3+Q61XS5vLOOJTWwpTMIcLjTsKqsXyHY39Fh+8YhgDXt OJh6erZmXytDJOEDNN+/IltNDV9QsqJGJB3Ry3bEXxkMci8rL8tbRwWHCEVjezsQ0Ayw b0iRVW1M0x6OmM8QU1dtXJYkLViqUPzx6UMRaTXAop103tZ/CxhRqEao9hhU+eL0LQcO vAERRRQ3hvcGDAGGwoV7YW1CeoykQhovkQ7m7wBIeePV92ym5RIm1XAKsQyNPapVZQi+ mXHG6noFAa0pYNONuPd+9YNyOe3rDi8RuQyFdFXtuk4iVtzgqSBr3k6oJvFbCdyC6EAH eqNw== X-Gm-Message-State: AC+VfDxEVeb2Yy4l7lw4XhvrF5KAkFzA8eaz8bYW8SPfFhGJjWl/nTil Ao0bUoxrYW6G+g55h7giWVVxCw== X-Google-Smtp-Source: ACHHUZ5RjbCf8rDXcfvsTR7OhaJEnOnHYqYV1g3YaVX2C1PrNk9uJcx8rEsXXhpD/No4Aq0xGW5UqA== X-Received: by 2002:a17:907:3209:b0:94e:4b26:233c with SMTP id xg9-20020a170907320900b0094e4b26233cmr32498469ejb.16.1684247615440; Tue, 16 May 2023 07:33:35 -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 gz4-20020a170907a04400b009571293d6acsm10988779ejc.59.2023.05.16.07.33.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 May 2023 07:33:34 -0700 (PDT) Date: Tue, 16 May 2023 16:33:32 +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-20230516_073338_544612_EF5CF35C X-CRM114-Status: GOOD ( 27.09 ) 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 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? > >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 >>> >>>[...] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel