Received: by 2002:a05:6358:3188:b0:123:57c1:9b43 with SMTP id q8csp4738965rwd; Tue, 30 May 2023 09:11:13 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5dQwlTyFXC34yOaCgHwlgjsNO31sFH3y0odDJHLzhw2UTPpohGa7yKJFyNtJnsoZzxREvp X-Received: by 2002:a05:6a20:3945:b0:10e:a93a:3b with SMTP id r5-20020a056a20394500b0010ea93a003bmr3582506pzg.22.1685463072237; Tue, 30 May 2023 09:11:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685463072; cv=none; d=google.com; s=arc-20160816; b=S+ervr/iU3WqNCc72H5Ci1xIbim0YLUPshmIQF5wbXa2xy4MS4JbsRXnq64PlHSX/d ibO/eoBlmzDxxvXBqFN2nnA5SayeO933w8khSgXb9CbJRcBBoX5tqTkaUzbc+Qv2X8X8 x7+bA9kqlCM653/50+AvM6fv5b13Zl/MNu90UFF4DiJEpkgDRF9lh94ydMXLdI3Z/3Jr +5F3S3eTldj0DG61j0baeqWJJVqHBDsiEmNpc3nz49KzxZ2XoNNfnw6PE5yespHp4D6Z pi8mlWjOK1nluSrsYbaqNNqEQlJgDjK9EcrX33bPlPEhIdIX/nmqQUpNEBpeYaE62ctb hL3g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:user-agent:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=rHLpWB2idDx6NUu9uDREPJ6CN0gKgc15y+Jr03H4B7w=; b=R7wO+JzX5wNsAwDRwKS43wvQRqf4QV1FMpq7OAI1rnK2H1g+7flF3SHNasqcfdXQj7 qQglWqx8nBDcLWxoa4RV0rYu5WjNC13hfI6YldrVatOYxN54EMGk1+4iTpV14GYJjyxY BFyFdGCKX8es9EFnHRhOiTyAeY8fLxPnUkfmgP8F9Y8viUeJhUBbCOIgYbCn+T3/QPWn p+hshMDrsW4UhGsOEUOIkw7z619lrbF6sozZopv5mLhJ9nGkkFYSZXPRUXRHDVgQTmXJ OiKYqKQ6ECU5xtGLtdimst04BAMZj3AaKndc7A/HJNs+BOuDdlGL/8fOJBKP5lM18Xer i3zw== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@sberdevices.ru header.s=mail header.b=AjAmrRH9; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=sberdevices.ru Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id y11-20020a637d0b000000b00535a71508f3si10247405pgc.77.2023.05.30.09.10.56; Tue, 30 May 2023 09:11:12 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@sberdevices.ru header.s=mail header.b=AjAmrRH9; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=sberdevices.ru Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231910AbjE3QEd (ORCPT + 99 others); Tue, 30 May 2023 12:04:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51154 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232476AbjE3QEa (ORCPT ); Tue, 30 May 2023 12:04:30 -0400 Received: from mx.sberdevices.ru (mx.sberdevices.ru [45.89.227.171]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8BBDC138; Tue, 30 May 2023 09:03:59 -0700 (PDT) Received: from s-lin-edge02.sberdevices.ru (localhost [127.0.0.1]) by mx.sberdevices.ru (Postfix) with ESMTP id 2F4F95FD3A; Tue, 30 May 2023 19:03:36 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sberdevices.ru; s=mail; t=1685462616; bh=rHLpWB2idDx6NUu9uDREPJ6CN0gKgc15y+Jr03H4B7w=; h=Date:From:To:Subject:Message-ID:MIME-Version:Content-Type; b=AjAmrRH9suMXhwS9xcW/H6stcTQmHozkPsHnFyr+6PSubFUTXZqpydgCgJ5UixBWB Z46yKsfJmCR0WUdFVkhLVk4fVXPDuM7Xx4XqyCldWzeECPUJjwDf4wJRXSF33Wl9gX aKrVxGMPrdJ6lhbvEPLaiZxn1gLz3/bN26CAc/GmmAriYxjsdDuJkuF+6mI4pI6cZq x7J+h3Z6id3sSCc/+CpolgrCLc5MJI6wRS4WFwY8lEYZxrp7YhxbbZtMRaGgNAjtx4 zlTrS+ylpqDJWYCy+0g1ed3ztuTJTCQsp3HW9S9ujWoOFzvw/3l8Xna3Zfz0l+UXkN nemWqWE2fV9KQ== Received: from S-MS-EXCH01.sberdevices.ru (S-MS-EXCH01.sberdevices.ru [172.16.1.4]) by mx.sberdevices.ru (Postfix) with ESMTP; Tue, 30 May 2023 19:03:35 +0300 (MSK) Date: Tue, 30 May 2023 19:03:34 +0300 From: Dmitry Rokosov To: Conor Dooley , Martin Blumenstingl , CC: , , , , , , , , , , , , , Subject: Re: [PATCH v15 5/6] dt-bindings: clock: meson: add A1 Peripherals clock controller bindings Message-ID: <20230530160334.z6sclbmqccs6ju4y@CAB-WSD-L081021> References: <20230517133309.9874-1-ddrokosov@sberdevices.ru><20230517133309.9874-6-ddrokosov@sberdevices.ru><20230522130033.a47vlybocme66rev@CAB-WSD-L081021><20230530-illusive-pushpin-1e35d0a50e0d@wendy> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230530-illusive-pushpin-1e35d0a50e0d@wendy> User-Agent: NeoMutt/20220415 X-Originating-IP: [172.16.1.6] X-ClientProxiedBy: S-MS-EXCH01.sberdevices.ru (172.16.1.4) To S-MS-EXCH01.sberdevices.ru (172.16.1.4) X-KSMG-Rule-ID: 4 X-KSMG-Message-Action: clean X-KSMG-AntiSpam-Status: not scanned, disabled by settings X-KSMG-AntiSpam-Interceptor-Info: not scanned X-KSMG-AntiPhishing: not scanned, disabled by settings X-KSMG-AntiVirus: Kaspersky Secure Mail Gateway, version 1.1.2.30, bases: 2023/05/30 11:20:00 #21377521 X-KSMG-AntiVirus-Status: Clean, skipped X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_NONE, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Conor, Martin and Jerome, Thank you so much for the detailed information and thoughts you shared about the important process of changing DT bindings. Please find my comments below. On Tue, May 30, 2023 at 10:34:07AM +0100, Conor Dooley wrote: > Yo, > > On Mon, May 29, 2023 at 10:38:33PM +0200, Martin Blumenstingl wrote: > > On Mon, May 22, 2023 at 3:00 PM Dmitry Rokosov wrote: > > [...] > > > > This IP block has at least one additional input called "sys_pll_div16". > > > > My understanding is that the "sys_pll_div16" clock is generated by the > > > > CPU clock controller. Support for the CPU clock controller > > > > (dt-bindings and a driver) will be added at a later time by Dmitry. > > > > How can we manage incrementally implementing the clock controllers? > > > > From a hardware perspective the "sys_pll_div16" input is mandatory. > > > > How to manage this in the .dts patches then (for example: does this > > > > mean that Dmitry can only add the clock controller to the .dts when > > > > all clock controller bindings have been implemented - or is there > > > > another way)? > > > > > > You're absolutely right: currently, not all inputs are supported because > > > the CPU clock controller isn't ready yet – I'm working on it at the > > > moment. > > > > > > I understand your concerns about bindings and schema description, but > > > there is an issue to be considered. I'm developing the entire clock > > > controller A1 subsystem incrementally in three stages: peripherals and > > > PLL, CPU, and Audio. This is because the CPU can operate at a static > > > frequency and voltage, and the board boots normally without the CPU > > > clock controller, thermal sensor, and OPP table. Audio is also > > > important, but it's optional. On the other hand, without setting up the > > > peripherals and PLL controllers, the board won't function because > > > they're fundamental. > > I understand your approach and I like it (without that incremental > > approach you would probably be looking at a series with 15-20 > > patches). > > > > Maybe the dt-binding maintainers have a suggestion for us here? > > Let me try to summarize the issue in a few bullet points: > > - There's (at least) four clock controllers on the Amlogic A1 SoC > > - Some of these clock controllers take the outputs of another clock > > controller as inputs > > - In this series patch the peripheral clock controller has an input > > called "sys_pll_div16" > > - The clock controller which provides the "sys_pll_div16" clock is not > > implemented yet (my understanding is that implementing it and adding > > it to this series is not easy: it would add even more patches that > > need to be reviewed and in general it's a tricky clock controller to > > implement as it manages the CPU clocks) > > If I am understanding correctly, this series implements the child > controller and a parent, which is unimplemented, provides the child with > sys_pll_div16. > The thing I am missing is whether the child controller has some outputs > that depend on this sys_pll_div16 input & whether those are documented > in this series. Regardless, you should be able to add more output clocks > without compatibility issues. > > > > Right now, we're in the first stage of the plan. Unfortunately, I can't > > > disclose the exact names and number of clock bindings for the CPU and > > > Audio, as they're still in development and only exist in my head or > > > draft versions. > > > > > > If possible, I'd prefer to provide the new bindings and connections once > > > all the appropriate drivers are finalized. > > Question to Conor and Krzysztof (assuming you read my summary above): > > Is it fine that Dmitry adds additional inputs to the peripheral clock > > controller binding in later patches? > > Perhaps Krzysztof will disagree with me, but my take on it would be that > the binding should describe the individual clock controller in its > totality, but the driver can choose to only implement a subset of it. > > If you define the binding as only needing N inputs, but then later > expand it to having N+M inputs, the driver will have to support N & N+M > input clocks to preserve compatibility. > If you define it as needing N+M inputs from the beginning, but only use > N, there is no issue with backwards compatibility when you later use > them all. > In short, I agree with Jerome's position, which he shared in the parallel email: https://lore.kernel.org/all/1jmt1m42m1.fsf@starbuckisacylon.baylibre.com/ Below I will try to explain my position. Please correct me if I am mistaken, but it appears we are currently discussing a scenario where a new kernel X+1 image version is flashed onto a device with a device tree from kernel X-1 image version, and whether a new driver with clock object connections can function on the previous Device Tree version. If I'm grasping the issue correctly, it seems that we have four potential scenarios at play here: 1) we may have altered the relationships between internal clock objects; 2) we may have introduced new output clock connections; 3) we may have introduced new input clock connections; 4) we may have removed existing connections from the driver. It seems that options 1 and 4 are straightforward and easy to understand. The 4 case is fully prohibited. We cannot remove any object or connection from the clock driver if another consumer uses it in the previous Device Tree versions. It is also important to note that any internal changes within the driver do not affect the device tree connections, as this is already clear. The second and third cases are more complex. For instance, let's say we are preparing a new patchset for an existing clock driver, such as Peripherals, where we are adding an Audio clock that is handled by an MMIO register inside Peripherals clock driver. X-1 dts does not recognize this new clock and doesn't have any connection to any node or to any other clock driver, thereby eliminating any backwards compatibility issues. As for new input clock connections, such as the cpu_clock (sys_pll_div16), these are handled by clock muxing abstraction, allowing CCF to find the clock object by fw.name and returning -ENOENT if the connection is missing without breaking any CCF flow. It happens in the kernel function clk_core_fill_parent_index() https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L424 Despite not having the connection for the new input in the old Device Tree version, this will not break kernel boot flow and workflow, and the new clock object just would not be utilized. Based on the presented arguments, I fully agree with Jerome's position. We can add new connections and objects in new driver versions, but their removal is prohibited. If it's alright with you, I would prefer to keep the Peripherals and PLL clock driver and their bindings as they are, and continue with the CPU and Audio clock controllers in a separate patch series. Would that be feasible for you? > > If not: how can we proceed in case we need to add them now (the > > dt-binding example is the easy part for me as we can just make up a > > phandle like &sys_pll_div16_clk and use that - but this can't work > > when Dmitry tries to add the clock controller to meson-a1.dtsi) > > I would be inclined to do the same thing in the dts as the example, > and make up a fixed-frequency clock and use it to plug the hole. > When you have bindings etc written for the clock controller providing > that clock, the fixed-frequency clock could be swapped out for the real > one. > > > PS: Dmitry is trying to get this series into Linux 6.5. As far as I > > remember the common clock maintainers don't take pull requests with > > new features after -rc6 (which is in less than two weeks). > > So time is getting a bit short and for me this is the very last > > outstanding question. If you say that it's fine to add clocks later on > > this will immediately get my Reviewed-by. > > I *think* that what I've just said should not get in the way of such a > timeline, as it would only involve a "small" change to the dt-binding, > but not require additional bindings or driver. > > Cheers, > Conor. > -- Thank you, Dmitry УВЕДОМЛЕНИЕ О КОНФИДЕНЦИАЛЬНОСТИ: Это электронное сообщение и любые документы, приложенные к нему, содержат конфиденциальную информацию. Настоящим уведомляем Вас о том, что если это сообщение не предназначено Вам, использование, копирование, распространение информации, содержащейся в настоящем сообщении, а также осуществление любых действий на основе этой информации, строго запрещено. Если Вы получили это сообщение по ошибке, пожалуйста, сообщите об этом отправителю по электронной почте и удалите это сообщение. CONFIDENTIALITY NOTICE: This email and any files attached to it are confidential. If you are not the intended recipient you are notified that using, copying, distributing or taking any action in reliance on the contents of this information is strictly prohibited. If you have received this email in error please notify the sender and delete this email.