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 8CB73C3DA4B for ; Wed, 17 Jul 2024 08:32:00 +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: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=odTbO9u7AyhS8xreJluNKyj0ekqxabepcpFkDx/pdXM=; b=rCFaQ1KRs491+/Td2T9R8nVmga lM3eWMpCmoyTNs32BJ0sFebqgX7sb2me6SFfhV0odvIyyAZp3KXPbDtuE9QUuKNDdLhDjy2G7l92s Q4Ufw3HtN1c7HfIhGwe7IWHVX8ooyxdPwMgBsdvn6DRm/I0Cu+qBG74C5sKEoDCc1mFGgKMD5z/A2 r+eg2a0ReTQU69MK6+JHpfdCnwnjIlLJiDJNdKT2GYK3lA8/ORoWHsnwvmS3DJ1jNsYOniKd8ZuO4 DbYxgkRIsehglLh9JyRB1Jyu2flIqByKNdwUbMvUe98kQlMTB3NFgUoEVK2FwSz2SuvpXC6sSRad3 cyD2kApQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sU04f-0000000D9DQ-3etE; Wed, 17 Jul 2024 08:31:45 +0000 Received: from mail-wm1-x32d.google.com ([2a00:1450:4864:20::32d]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sU04K-0000000D95Z-1WDm for linux-arm-kernel@lists.infradead.org; Wed, 17 Jul 2024 08:31:26 +0000 Received: by mail-wm1-x32d.google.com with SMTP id 5b1f17b1804b1-42795086628so44106705e9.3 for ; Wed, 17 Jul 2024 01:31:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tuxon.dev; s=google; t=1721205083; x=1721809883; darn=lists.infradead.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=odTbO9u7AyhS8xreJluNKyj0ekqxabepcpFkDx/pdXM=; b=Rxg5D9uLfuiN9O5OKQ8K4isZCU9jSOdQc3r7nn76+3S1r1KMG0iWJsJy7q6hAJZ4bO voDcECoeuP6hii8bsCyN12ckAyAD9QOij6Fkmh/1u1ShLNMvAQlZ/cu9+M1M3VkCdB2S sI8LWkCV/trAQEev3JQdiAk9gLjyYSaGfQWRYDm95pbCzjtAYFAB0j2yRh0HY7AwF9eE jDMLJYk/y8jfAtZhk1in8ABr5W8RBNv7603LAmnr7CToKQgo/7dYrUzA0Vz2DR6PtXrd UfW2FJpYwWco1IGamjcFF+dHb7B+9J6ge1UvpBx929SRn71PuqninuPwlSYTyQ7+SqqQ U3Tw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721205083; x=1721809883; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=odTbO9u7AyhS8xreJluNKyj0ekqxabepcpFkDx/pdXM=; b=ElnoP1Px8zCei9E1rd/8Y5ZwmNfu5KbpX8ygAHl/fDP2u5yfSoj8mobEGP3AT1jo9o Wn0IgVoGhJSlhtBGVzIjnIORBayockz+0VSctaQYRmJ+Zxbhmnbg70ialwR32cOhUrNx xtZxLwaG4cOQDQJWnE0rPN0eutW1vYngUwG22tx4mDbaM7pXEslW+ojWFi0JF/s/Si4d qRVp/A1onktVhH/Kfri9/JRZD++qo4jaSV2Th9jmiwsI62Y/+FJ+9UXg28tDXJzZY34V W9JPLor+dOuBwnTEDLeIJkFvRNEdFd9r1v+ySYOqGn+0FIdqN/MlQLeNaYMfM43FZdMn 4xCw== X-Forwarded-Encrypted: i=1; AJvYcCUEVh9co83AoxP7BXLH7MbtGVg26OfP2SVmNQrlLaWbiOKqsaLGvJBQLWLkTfg7ToulccdL86Co6bjT1fd5GC0thVlaHFHwDPaXTzmYvRo+FcLYpg8= X-Gm-Message-State: AOJu0Yy7oTovaZT7R01ytcJ/ImDyX10Falp0L+Elw/BGNXfmeIuRNeVr Ba3ViBDY8H/t05psg74TosBd/Qo9XxHUhrff0QV/SCc7oOX7e02M2ZV5AMx5U64= X-Google-Smtp-Source: AGHT+IFxAFyo8cL/V89UtkMiApuS4cDBAYaqIrX7GtE+oWaOkWacfHj+o5WDxnzLrAffsH1ZbEmYMw== X-Received: by 2002:a05:600c:1f0e:b0:426:60e4:c691 with SMTP id 5b1f17b1804b1-427c2cb36b1mr6643635e9.11.1721205082759; Wed, 17 Jul 2024 01:31:22 -0700 (PDT) Received: from [192.168.50.4] ([82.78.167.171]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4279f27846bsm196825965e9.25.2024.07.17.01.31.21 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 17 Jul 2024 01:31:22 -0700 (PDT) Message-ID: Date: Wed, 17 Jul 2024 11:31:20 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 03/11] clk: renesas: clk-vbattb: Add VBATTB clock driver Content-Language: en-US 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> From: claudiu beznea In-Reply-To: <2abcd440664067d95b1ac0e765ad55a3.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-20240717_013124_443340_7D5FCECA X-CRM114-Status: GOOD ( 29.04 ) 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 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. > >> +#include >> +#include >> +#include >> +#include >> +#include > > Is of_platform.h used? > > Include mod_devicetable.h for of_device_id. Ok. > >> +#include >> + >> +#define VBATTB_BKSCCR 0x0 >> +#define VBATTB_BKSCCR_SOSEL BIT(6) >> +#define VBATTB_SOSCCR2 0x8 >> +#define VBATTB_SOSCCR2_SOSTP2 BIT(0) > [..] >> + >> +static int vbattb_clk_probe(struct platform_device *pdev) >> +{ >> + struct device_node *np = pdev->dev.of_node; >> + struct clk_parent_data parent_data = {}; >> + struct device *dev = &pdev->dev; >> + struct clk_init_data init = {}; >> + struct vbattb_clk *vbclk; >> + u32 load_capacitance; >> + struct clk_hw *hw; >> + int ret, bypass; >> + >> + vbclk = devm_kzalloc(dev, sizeof(*vbclk), GFP_KERNEL); >> + if (!vbclk) >> + return -ENOMEM; >> + >> + vbclk->base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(vbclk->base)) >> + return PTR_ERR(vbclk->base); >> + >> + bypass = vbattb_clk_need_bypass(dev); > > This is a tri-state bool :( > >> + if (bypass < 0) { >> + return bypass; >> + } else if (bypass) { >> + parent_data.fw_name = "clkin"; >> + bypass = VBATTB_BKSCCR_SOSEL; > > And now it is a mask value. > >> + } else { >> + parent_data.fw_name = "xin"; >> + } >> + >> + ret = of_property_read_u32(np, "renesas,vbattb-load-nanofarads", &load_capacitance); >> + if (ret) >> + return ret; >> + >> + ret = vbattb_clk_validate_load_capacitance(vbclk, load_capacitance); >> + if (ret) >> + return ret; >> + >> + vbattb_clk_update_bits(vbclk->base, VBATTB_BKSCCR, VBATTB_BKSCCR_SOSEL, bypass); > > Please don't overload 'bypass'. Use two variables or a conditional. OK. > > 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 > 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>; 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? > >> + >> + spin_lock_init(&vbclk->lock); >> + >> + init.name = "vbattclk"; >> + init.ops = &vbattb_clk_ops; >> + init.parent_data = &parent_data; >> + init.num_parents = 1; >> + init.flags = 0; >> + >> + vbclk->hw.init = &init; >> + hw = &vbclk->hw; >> + >> + ret = devm_clk_hw_register(dev, hw); >> + if (ret) >> + return ret; >> + >> + return of_clk_add_hw_provider(np, of_clk_hw_simple_get, hw); >> +} >> + >> +static const struct of_device_id vbattb_clk_match[] = { >> + { .compatible = "renesas,r9a08g045-vbattb-clk" }, >> + { /* sentinel */ } >> +}; > > Any MODULE_DEVICE_TABLE? I failed to added it. Thank you for your review, Claudiu Beznea