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 80ACEC41513 for ; Thu, 27 Jul 2023 10:28:48 +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-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=RKOTf9KQ4mkpqBgHNVXaaerWW59b6dBMD0QBtdbKx9E=; b=FchEWp4P+cPZcR +vLWPVPVec23pWMWx0D5Af2lbbSEsGNWaMwZygy4n1En4C/IsHXuj1outQl1mSwrnDYSAcG5P/9X7 bOBF01p6LChxhmhUqVo/gYroL0RdtVvqRwq3gOcU1rOKMR0oGigNHDBb9g4AUlZiy1D3ltsXFKlHP OvRaYYgEroiuQwdofjaVPFrFqDAkiy1AJGR8eTZbO1dboj3WZgN02qS1nt6MWxOT+7P0WkCINeL14 ipTNhjQdlEXKcPKti95hpfobsbDS5kjODqp97ZQ9SI6tlRAqer1Fxp4M5pYcTny3p+Tu89X4G8tex fH9YCWFT2yaI+gyiKzlQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qOyEO-00CzuT-1O; Thu, 27 Jul 2023 10:28:28 +0000 Received: from out-86.mta0.migadu.com ([2001:41d0:1004:224b::56]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qOyEL-00Czsf-0q for linux-arm-kernel@lists.infradead.org; Thu, 27 Jul 2023 10:28:27 +0000 Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1690453697; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=cdNMArdaFWVI5SKSPm1vQyJoyF/bmKiSh+QjqHSwJY0=; b=NriIWc+uIleh+ze4o2J+UOpT04I4WThX6gf3dXAI8lj9M09YHk1OVVkXck++hhTRBTGk8E 4eD11Z3y+oNLibSGtUV+HnOmq1OsvnARyeqfL2Wx9+gW02TZ+HZXRAkKi1tB/Rw4BihKB4 GLLXJpQFGs6Ro14ZLlvr9uCoX7+GCBs= Date: Thu, 27 Jul 2023 11:28:12 +0100 MIME-Version: 1.0 Subject: Re: [PATCH 09/11] ice: implement dpll interface to control cgu Content-Language: en-US To: "Kubalewski, Arkadiusz" , Jiri Pirko , Jakub Kicinski , Paolo Abeni Cc: Jonathan Lemon , "Olech, Milena" , "Michalik, Michal" , "linux-arm-kernel@lists.infradead.org" , poros , mschmidt , "netdev@vger.kernel.org" , "linux-clk@vger.kernel.org" , Bart Van Assche References: X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Vadim Fedorenko In-Reply-To: X-Migadu-Flow: FLOW_OUT X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230727_032825_742225_4B913A57 X-CRM114-Status: GOOD ( 31.98 ) 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-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 26/07/2023 22:11, Kubalewski, Arkadiusz wrote: >> From: Jiri Pirko >> Sent: Wednesday, July 26, 2023 8:38 AM >> > > [...] > >>>>>>> >>>>>>> Just to make it clear: >>>>>>> >>>>>>> AUTOMATIC: >>>>>>> - inputs monitored, validated, phase measurements available >>>>>>> - possible states: unlocked, locked, locked-ho-acq, holdover >>>>>>> >>>>>>> FREERUN: >>>>>>> - inputs not monitored, not validated, no phase measurements available >>>>>>> - possible states: unlocked >>>>>> >>>>>> This is your implementation of DPLL. Others may have it done >>>>>> differently. But the fact the input is monitored or not, does not make >>>>>> any difference from user perspective. >>>>>> >>>>>> When he has automatic mode and does: >>>>>> 1) disconnect all pins >>>>>> 2) reset state (however you implement it in the driver is totaly up >>>>>> to the device, you may go to your freerun dpll mode >>>>>> internally and to automatic back, up to you) >>>>>> -> state will go to unlocked >>>>>> >>>>>> The behaviour is exactly the same, without any special mode. >>>>> >>>>> In this case there is special reset button, which doesn't exist in >>>>> reality, actually your suggestion to go into FREERUN and back to AUTOMATIC >>>>> to pretend the some kind of reset has happened, where in reality dpll went >>>>> to >>>>> FREERUN and AUTOMATIC. >>>> >>>> There are 3 pin states: >>>> disconnected >>>> connected >>>> selectable >>>> >>>> When the last source disconnects, go to your internal freerun. >>>> When some source gets selectable or connected, go to your internal >>>> automatic mode. >>>> >>> >>> This would make the driver to check if all the sources are disconnected >>> each time someone disconnects a source. Which in first place is not >>> efficient, but also dpll design already allows different driver instances >>> to >>> control separated sources, which in this case would force a driver to >>> implement >>> additional communication between the instances just to allow such hidden >>> FREERUN mode. >>> Which seems another argument not to do this in the way you are proposing: >>> inefficient and unnecessarily complicated. >>> >>> We know that you could also implement FREERUN mode by disconnecting all >>> the >>> sources, even if HW doesn't support it explicitly. >>> >>> >From user perspactive, the mode didn't change. >>>> >>> >>> The user didn't change the mode, the mode shall not change. >>> You wrote to do it silently, so user didn't change the mode but it would >> have >>> changed, and we would have pretended the different working mode of DPLL >> doesn't >>> exist. >>> >>> >From user perepective, this is exacly the behaviour he requested. >>>> >>> >>> IMHO this is wrong and comes from the definition of pin state DISCONNECTED, >>> which is not sharp, for our HW means that the input will not be considered >>> as valid input, but is not disconnecting anything, as input is still >>> monitored and measured. >>> Shall we have additional mode like PIN_STATE_NOT_SELECTABLE? As it is not >>> possible to actually disconnect a pin.. >>> >>>> >>>>> For me it seems it seems like unnecessary complication of user's life. >>>>> The idea of FREERUN mode is to run dpll on its system clock, so all the >>>>> "external" dpll sources shall be disconnected when dpll is in FREERUN. >>>> >>>> Yes, that is when you set all pins to disconnect. no mode change needed. >>>> >>> >>> We don't disconnect anything, we used a pin state DISCONNECTED as this >>> seemed >>> most appropriate. >>> >>>> >>>>> Let's assume your HW doesn't have a FREERUN, can't you just create it by >>>>> disconnecting all the sources? >>>> >>>> Yep, that's what we do. >>>> >>> >>> No, you were saying that the mode doesn't exist and that your hardware >>> doesn't >>> support it. At the same time it can be achieved by manually disconnecting >>> all >>> the sources. >>> >>>> >>>>> BTW, what chip are you using on mlx5 for this? >>>>> I don't understand why the user would have to mangle state of all the pins >>>>> just >>>>> to stop dpll's work if he could just go into FREERUN and voila. Also what >>>>> if >>>>> user doesn't want change the configuration of the pins at all, and he just >>>>> want >>>>> to desynchronize it's dpll for i.e. testing reason. >>>> >>>> I tried to explain multiple times. Let the user have clean an abstracted >>>> api, with clear semantics. Simple as that. Your internal freerun mode is >>>> just something to abstract out, it is not needed to expose it. >>>> >>> >>> Our hardware can support in total 4 modes, and 2 are now supported in ice. >>> I don't get the idea for abstraction of hardware switches, modes or >>> capabilities, and having those somehow achievable through different >>> functionalities. >>> >>> I think we already discussed this long enough to make a decision.. >>> Though I am not convinced by your arguments, and you are not convinced by >>> mine. >>> >>> Perhaps someone else could step in and cut the rope, so we could go further >>> with this? >> >> Or, even better, please drop this for the initial patchset and have this >> as a follow-up. Thanks! >> >> > > On the responses from Jakub and Paolo, they supported the idea of having > such mode. > > Although Jakub have asked if there could be better name then FREERUN, also > suggested DETACHED and STANDALONE. > For me DETACHED seems pretty good, STANDALONE a bit too far.. > I am biased by the FREERUN from chip docs and don't have strong opinion > on any of those.. > > Any suggestions? It looks like we have a kind of split-brain situation, and my thoughts are following: Even though right now we don't have any hardware supporting freerun/standalone mode, I do really like the idea to have it. It will be used in monitoring implementations where we refer to internal oscillator (Rb/Cs) as a source of truth to compare with the signal on the other pins. We can name it DETACHED if it sounds better. > > Thank you! > Arkadiusz > > [...] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel