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 6508AC3DA63 for ; Thu, 18 Jul 2024 14:41:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:References:Cc:To:Subject:From: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=d5zeQJ5Ow3UH0+RdZ7WJRjKIJGbUGTjL+v0QPQ6ApNI=; b=J7IpVCw7fj74s4bNKSVUa1Ugsq FAPmxTQC1pY1rjVGqvcKzg+hPglM7obpS5eMqlaOWL+EVw3OlAv/EVEtBSqMPyv68yFeb9Q4RHV98 w4JuCeFqa+py6hQ6OIiXZ4eAJFoIMNvOvVdT3bwXfBUaSPylIRCvGln6NhX+iwGUFoQ3sLM9mptpJ vWd/dU5kUmN2aWnZwNdFpi1c2lhbfViCXNp6bDvtIf+CbEPDJoPFcr4I8lS9AtWOhQXM7lqrn2Sxn Ig+3WTviAJVvnaPdtUN36VPlcAMfwJm+zZg46D3NZNHLRTLKErNf2mWOr3PD8I4Y7NKLnydMdaJHu 1xJL43+w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sUSK7-0000000HIZx-10Ou; Thu, 18 Jul 2024 14:41:35 +0000 Received: from mail-wr1-x42a.google.com ([2a00:1450:4864:20::42a]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sUSJg-0000000HIQ4-2enM for linux-arm-kernel@lists.infradead.org; Thu, 18 Jul 2024 14:41:10 +0000 Received: by mail-wr1-x42a.google.com with SMTP id ffacd0b85a97d-3686b554cfcso91891f8f.1 for ; Thu, 18 Jul 2024 07:41:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tuxon.dev; s=google; t=1721313666; x=1721918466; darn=lists.infradead.org; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id:from:to :cc:subject:date:message-id:reply-to; bh=d5zeQJ5Ow3UH0+RdZ7WJRjKIJGbUGTjL+v0QPQ6ApNI=; b=oi8beGIEvnyJVE2r/vRvZ0/H1PLZ8+S8vAAB8MrB72N2nVr4LOiTqwpAmvajbatuM1 N33vwGCwjOxmHlhx5fOMJS85Ou3n4wPfrySPFYuidVPBzymo5xS2n4CzL0WutofP96GC gWELCS/fKDlId0nGcBEQ7wT7nQWCU6h+ZG0pIMqoKHNlWpfO0FBeWuI6vwVQ5Ds82uHr 47Fy/RhKbpNV6ApeK0r6loXB9dpTnUFFI0lfsMkxIoDPzFoGTa3nQxQlHyCZCya7ZieC vSUwKmfm7xOs1r/EPmqw0YDCPMnOGB//5tHU1FcKST0cGzrMsaudricbpYjkNpmolII7 Nnhg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721313666; x=1721918466; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=d5zeQJ5Ow3UH0+RdZ7WJRjKIJGbUGTjL+v0QPQ6ApNI=; b=bv4VRlzTEQo+JULLvDEy549nltLcn7PwVfqVzTGGbmv7Z75Q995P0REHa79scY2t2y Fd7CMDdXHiLQJf/r9bJSwk+Pk7ne+GMJ6CFBv7gj2QJmwNBtYrn3nbV8C78NZ/wy27Nn nvEwQbtntH6tn3x2E5ST1F10lLkEA0DF3fbIqw9DND4tWADvx3Qtu7HZ+A6VAy3GWszb HS1qXHNx+tjBIW0rOQSq1yD6/vSJrTWJhwdpbKHcdDvcJnAGVUi3ty/oyir6nzTEsrRU 7hX10P76VLMV7LdbDv1vUcuu4SPy0XEVMoywxMB7kk9U8/hlURiKvI5ZUQhXgtXVPjl1 TGDA== X-Forwarded-Encrypted: i=1; AJvYcCU02E5Cu++dqo0igxI0QbfqnRq/ScXJYheLOsMelueFC+16a5NFRUjavWASTNbl/qxsPJrTGrMfxBD/1O0VlsJc0qL9fLtmERccrbYUsUksRZ2kbvE= X-Gm-Message-State: AOJu0YyTPWVH/B7A98HAQmrhK85vjN9gEWI7cNXj+lrR2B/s50mgA69P Q8qrB33QNzLyDFAjMNr/iRXOW408IUqGEAEN8pl9XwTFondVWMk78bY6k9NmOS0= X-Google-Smtp-Source: AGHT+IHLLY7FV8d3TW5DeDftcP9CR4qKgZexsCdXD1McSEmqtB8zHI2JozSns5pNcAAnLe1mJiEB7g== X-Received: by 2002:a5d:59a2:0:b0:367:9073:3496 with SMTP id ffacd0b85a97d-3683160e527mr4242349f8f.29.1721313666237; Thu, 18 Jul 2024 07:41:06 -0700 (PDT) Received: from [192.168.50.4] ([82.78.167.144]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3685043637fsm1903132f8f.66.2024.07.18.07.41.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 18 Jul 2024 07:41:05 -0700 (PDT) Message-ID: Date: Thu, 18 Jul 2024 17:41:03 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: claudiu beznea Subject: Re: [PATCH v2 03/11] clk: renesas: clk-vbattb: Add VBATTB clock driver To: Stephen Boyd , alexandre.belloni@bootlin.com, conor+dt@kernel.org, geert+renesas@glider.be, krzk+dt@kernel.org, lee@kernel.org, magnus.damm@gmail.com, mturquette@baylibre.com, p.zabel@pengutronix.de, robh@kernel.org Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-rtc@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, Claudiu Beznea References: <20240716103025.1198495-1-claudiu.beznea.uj@bp.renesas.com> <20240716103025.1198495-4-claudiu.beznea.uj@bp.renesas.com> <2abcd440664067d95b1ac0e765ad55a3.sboyd@kernel.org> <4cacf090dc56c3ffd15bccd960065769.sboyd@kernel.org> Content-Language: en-US In-Reply-To: <4cacf090dc56c3ffd15bccd960065769.sboyd@kernel.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240718_074108_885345_A793152C X-CRM114-Status: GOOD ( 47.15 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 18.07.2024 03:39, Stephen Boyd wrote: > Quoting claudiu beznea (2024-07-17 01:31:20) >> Hi, Stephen, >> >> On 17.07.2024 01:28, Stephen Boyd wrote: >>> Quoting Claudiu (2024-07-16 03:30:17) >>>> diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c >>>> new file mode 100644 >>>> index 000000000000..8effe141fc0b >>>> --- /dev/null >>>> +++ b/drivers/clk/renesas/clk-vbattb.c >>>> @@ -0,0 +1,212 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * VBATTB clock driver >>>> + * >>>> + * Copyright (C) 2024 Renesas Electronics Corp. >>>> + */ >>>> + >>>> +#include >>>> +#include >>> >>> Prefer clk providers to not be clk consumers. >> >> I added it here to be able to use devm_clk_get_optional() as it was >> proposed to me in v1 to avoid adding a new binding for bypass and detect if >> it's needed by checking the input clock name. >> > > Understood. > >> >>> >>> I also wonder if this is really a mux, >> >> It's a way to determine what type of clock (crystal oscillator or device >> clock) is connected to RTXIN/RTXOUT pins of the module >> (the module block diagram at [1]) based on the clock name. Depending on the >> type of the clock connected to RTXIN/RTXOUT we need to select the XC or >> XBYP as input for the mux at [1]. >> >> [1] https://gcdnb.pbrd.co/images/QYsCvhfQlX6n.png > > That diagram shows a mux block, so at least something in there is a mux. > From what I can tell the binding uses clock-names to describe the mux. > What I'd like to avoid is using clk_get() to determine how to configure > the mux. That's because clk_get() is a clk consumer API, and because we > want clk providers to be able to register clks without making sure that > the entire parent chain has been registered first. Eventually, we'd like > clk_get() to probe defer if the clk is an orphan. Having clk providers > use clk_get() breaks that pretty quickly. > >> >> >>> and either assigned-clock-parents should be used, >>> or the clk_ops should have an init routine that looks at >>> which parent is present by determining the index and then use that to >>> set the mux. The framework can take care of failing to set the other >>> parent when it isn't present. >> >> >> On the board, at any moment, it will be only one clock as input to the >> VBATTB clock (either crystal oscillator or a clock device). If I'm getting >> you correctly, this will involve describing both clocks in some scenarios. >> >> E.g. if want to use crystal osc, I can use this DT description: >> >> vbattclk: clock-controller@1c { >> compatible = "renesas,r9a08g045-vbattb-clk"; >> reg = <0 0x1c 0 0x10>; >> clocks = <&vbattb_xtal>; >> clock-names = "xin"; >> #clock-cells = <0>; >> status = "disabled"; >> }; >> >> vbattb_xtal: vbattb-xtal { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> clock-frequency = <32768>; >> }; >> >> If external clock device is to be used, I should describe a fake clock too: >> >> vbattclk: clock-controller@1c { >> compatible = "renesas,r9a08g045-vbattb-clk"; >> reg = <0 0x1c 0 0x10>; >> clocks = <&vbattb_xtal>, <&ext_clk>; > > Is vbattb_xtal the fake clk? If so, I'd expect this to be > > clocks = <0>, <&ext_clk>; > > so that we don't have a useless clk node. > >> clock-names = "xin", "clkin"; >> #clock-cells = <0>; >> status = "disabled"; >> }; >> >> vbattb_xtal: vbattb-xtal { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> clock-frequency = <0>; >> }; >> >> ext_clk: ext-clk { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> clock-frequency = <32768>; >> }; >> >> Is this what you are suggesting? >> > > Sort of. Ignoring the problem with the subnode for the clk driver, I > don't really like having clock-names that don't match the hardware pin > names. From the diagram you provided, it looks like clock-names should > be "bclk" and "rtxin" for the bus clock and the rtxin signal. Then the > clock-cells should be "1" instead of "0", and the mux should be one of > those provided clks and "xc" and "xbyp" should be the other two. If that > was done, then assigned-clocks could be used to assign the parent of the > mux. > > #define VBATTBCLK 0 > #define VBATTB_XBYP 1 > #define VBATTB_XC 2 > > vbattb: vbattb@1005c000 { > compatible = "renesas,r9a08g045-vbattb"; > reg = <0x1005c000 0x1000>; > ranges = <0 0 0x1005c000 0 0x1000>; > interrupts = ; > interrupt-names = "tampdi"; > clocks = <&cpg CPG_MOD R9A08G045_VBAT_BCLK>, <&ext_clk>; > clock-names = "bclk", "rtxin"; > power-domains = <&cpg>; > resets = <&cpg R9A08G045_VBAT_BRESETN>; > #clock-cells = <1>; > assigned-clocks = <&vbattb VBATTBCLK>; > assigned-clock-parents = <&vbattb VBATTB_XBYP>; > renesas,vbattb-load-nanofarads = <12500>; > }; I think I got it now. Thank you for the detailed explanation. > > One last thing that I don't really understand is why this needs to be a > clk provider. In the diagram, the RTC is also part of vbattb, so it > looks odd to have this node be a clk provider with #clock-cells at all. I did it like this because the RTC is a different IP mapped at it's own address and considering the other VBATTB functionalities (tamper, SRAM) might be implemented at some point. I also failed to notice that RTC might not work w/o bclk being enabled (thanks for pointing it). I saw that diagram more like describing the always-on power domain (PD_VBATTB) where the VBATTB logic and RTC resides. That power domain is backed by battery. From HW manual [1]: "PD_VBATT domain is the area where the RTC/backup register is located, works on battery power when the power of PD_VCC and PD_ISOVCC domain are turned off." [1] https://www.renesas.com/us/en/products/microcontrollers-microprocessors/rz-mpus/rzg3s-general-purpose-microprocessors-single-core-arm-cortex-a55-11-ghz-cpu-and-dual-core-cortex-m33-250 > Is it the case that if the rtxin pin is connected, you mux that over, > and if the pin is disconnected you mux over the internal oscillator? >From the description here at [2] I'm getting that the "32-KHz clock oscillator" block is used when crystal oscillator is connected to RTXIN, RTXOUT pins and it is skipped if external clock device is connected. [2] https://i2.paste.pics/RFKJ0.png?rand=Xq8w1RLDvZ > I'm > really wondering why a clk provider is implemented at all. Why not just > hit the registers directly from the RTC driver depending on a > devm_clk_get_optional() call? I did it like this because the RTC is a different IP mapped at it's own address with it's own interrupts, clock, power domain and considering that the other VBATTB functionalities (tamper, SRAM) might be used at some point in future. At the same time I failed to noticed the VBATTB clock might be needed for RTC. Do you consider better to just take a regmap to VBATTB from RTC driver and set the VBATTB from RTC driver itself? Thank you, Claudiu Beznea