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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 15B1CC433FE for ; Thu, 28 Apr 2022 15:59:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1349746AbiD1QCp (ORCPT ); Thu, 28 Apr 2022 12:02:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58612 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1349737AbiD1QCn (ORCPT ); Thu, 28 Apr 2022 12:02:43 -0400 Received: from mail-oa1-x2b.google.com (mail-oa1-x2b.google.com [IPv6:2001:4860:4864:20::2b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B5A58ADD75 for ; Thu, 28 Apr 2022 08:59:27 -0700 (PDT) Received: by mail-oa1-x2b.google.com with SMTP id 586e51a60fabf-e5e433d66dso5563359fac.5 for ; Thu, 28 Apr 2022 08:59:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=RA5VF4RpL+61fXTh4Gfb3qkFwZI/LPrSMrG3g+d0iY4=; b=tNO44O0A62NXKxNx7Jlp/mB7cZJdyYrl8dtt4xWYmRSg6gWx9uCVh+z57vfuH1b2Uh 9Z8SueFxPCJ2g5ClOArf3S5f66/6YZOCXrprhfAioy3U5dBCSSndbekFjfii2DGUBqaR /KPNkR4QGy3idrCl7vK8wxhlVoF+r/6BxG7qhxoLIpg96r7gJTLSMCdupHEZz+pSmn8B XIr9fossAvwQR67bxd4yLMhm4qD49McjTgbOSm3xc26BsnaMnkAD/y91WnYwy+PZQiOS 1mKTkPYRCCvj9binmHztJiVs5/fPG7fiU7BoeI7JulJGPKgYSvymyxg3p4/C9/fsg/KI 8X2A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=RA5VF4RpL+61fXTh4Gfb3qkFwZI/LPrSMrG3g+d0iY4=; b=XLXgf+tdkiu0dlYMwf/mAht3iNDEix5l8wlkoBLcrqQH8ZScEz0iv1cE/dcSPkVaqW Y5BUzknoEYu3F5mU3ZuYhbfCWypn1ePB0v8c2j1F6hofO/NYwzkM/5Eg1PHE7ceeRv5x qpWZrBOxOJIxP4Kvn/OrEa34LJaz0xV2LhCDjh0raSzBZMwSKx87T0Uq0XibeWm246mS FpDM6XRg4fcwH4ikq5xXKSe54+6R39UzcEQ9BqvhL9XbsDvkJ3rNFCJ4L5PCXLmyc4G+ WOH1/1t0S+3PXEBCIOobqjTPorU3Upe9NDKRoL+NbvojzR3hsYCiuixN8FYMS/r19WV5 g4vw== X-Gm-Message-State: AOAM532h1O3mI6eJNZm2yAaNnwrwNtcTRWla5H1/RSWod6gDFX+jIg1e hNb+q4NCnZbafzr13/YvhhuEgQ== X-Google-Smtp-Source: ABdhPJxhWKDV59y7XG07sok41TBFgDXIymy0jBtyvCQaoaeW7aHYiAkZVx9RK1PIFvCFrrvrFvDUQw== X-Received: by 2002:a05:6871:78b:b0:d4:2636:b26 with SMTP id o11-20020a056871078b00b000d426360b26mr14114234oap.14.1651161567048; Thu, 28 Apr 2022 08:59:27 -0700 (PDT) Received: from ripper ([2600:1700:a0:3dc8:205:1bff:fec0:b9b3]) by smtp.gmail.com with ESMTPSA id c30-20020a4a9c5e000000b0035e9d4fc486sm150270ook.39.2022.04.28.08.59.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Apr 2022 08:59:25 -0700 (PDT) Date: Thu, 28 Apr 2022 09:01:20 -0700 From: Bjorn Andersson To: Dmitry Baryshkov Cc: Stephen Boyd , Krzysztof Kozlowski , Michael Turquette , Rob Herring , linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, quic_tdas@quicinc.com Subject: Re: [PATCH v2 1/2] dt-bindings: clock: Add Qualcomm SC8280XP GCC bindings Message-ID: References: <20220422230013.1332993-1-bjorn.andersson@linaro.org> <20220423014824.912ACC385A0@smtp.kernel.org> <20220423031350.01299C385A0@smtp.kernel.org> <20220425223426.BE973C385A4@smtp.kernel.org> <3fb043e6-2748-24f8-0115-b5372c747a12@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3fb043e6-2748-24f8-0115-b5372c747a12@linaro.org> Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Thu 28 Apr 08:44 PDT 2022, Dmitry Baryshkov wrote: > On 26/04/2022 01:34, Stephen Boyd wrote: > > Quoting Bjorn Andersson (2022-04-22 20:43:18) > > > On Fri 22 Apr 20:13 PDT 2022, Stephen Boyd wrote: > > > > > > > > I'd really rather not have clock-names at all because we spend a bunch > > > > of time comparing strings with them when we could just as easily use > > > > a number. > > > > > > I know that you would like to get rid of the clock-names for the clock > > > controllers. I've looked at it since and while it will be faster to > > > execute I still feel that it's going to be harder to write and maintain. > > > > > > E.g. look at gcc_pcie_4_pipe_clk_src, its parents today are > > > pcie_4_pipe_clk and bi_tcxo. Something I can reason about being correct > > > or not. > > > > > > If we ditch the clock-names I will have: > > > > > > static const struct clk_parent_data gcc_parent_data_14[] = { > > > { .index = 30 }, > > > { .index = 0 }, > > > > Those numbers could have some #define. > > > > { .index = PCIE_4_PIPE_CLK_DT } > > { .index = BI_TCXO_DT } > > > > > }; > > > > > > Generally we would perhaps use some compile time constant, but that > > > won't work here because we're talking about the index in the clocks > > > array in the yaml. > > > > > > > > > But perhaps I'm missing something that would make this manageable? > > > > I dunno. Maybe a macro in the dt-binding header could be used to specify > > the 'clocks' property of the DT node that is providing the other side? > > The idea is to make a bunch of macros that insert the arguments of the > > macro in the right place for the clocks property and then define the > > order of arguments otherwise. It would be similar to how > > CREATE_TRACE_POINTS is used in include/trace/define_trace.h > > > > In the dt-bindings/qcom,gcc-soc.h file: > > > > #ifdef IN_DTSI > > > > #undef GCC_DT_NODE_CLOCKS > > #define GCC_DT_NODE_CLOCKS > > clocks = , > > ; > > > > #endif /* IN_DTSI */ > > > > #define BI_TCXO_DT 0 > > #define SLEEP_CLK_DT 1 BI_TCXO_DT is not the value, its the index of the entry in the clocks array. And the actual values of the clock controller's clocks property is not a property of the clock controller, but the system definition. I.e. that should be clear and explicitly expressed in the dts. > > Isn't this being an overkill, to define exact properties in the bindings > header? Also this would mean that we'd have to add dt-binding headers for > all _consumers_ of clocks. And to make things more complex, e.g. for PCIe > devices different instances of the device would use different amount of > clocks. This would mean that we'd have to define SM8250_PCI0_CLOCKS, > SM8250_PCIE1_CLOCKS and SM8250_PCIE2_CLOCKS. > > > If we were to switch to this fragile path of using indices (yes I consider > it to be very fragile), I'd consider something like the following to work in > the platform dtsi file: > > clocks = > BEGIN_CLOCK > CLOCK(BI_TCXO_DT, &bi_tcxo) > CLOCK(SLEEP_CLK_DT, &sleep_clk) > END_CLOCK; > > While the following should give an error: > clocks = > BEGIN_CLOCK > CLOCK(SLEEP_CLK_DT, &sleep_clk) > CLOCK(BI_TCXO_DT, &bi_tcxo) > END_CLOCK; > > I think we can make this error out by using some additional tool (or > additional preprocessor pass over the sources) > Let's not invent some magical syntax for describing the clocks in the DT. These macros can't expand to sparse arrays anyways, so iiuc this would give a sense that the ordering might not be significant, when it really is. > > And then in the SoC.dtsi file have > > > > #define IN_DTSI > > #include > > > > #define BI_TCXO_DT &xo_board > > #define SLEEP_CLK_DT &sleep_clk > > > > ... > > > > clock-controller@a000000 { > > compatible = "qcom,gcc-soc"; > > reg = <0xa000000 0x10000>; > > GCC_DT_NODE_CLOCKS > > }; > > > > > > and then in drivers/clk/qcom/gcc-soc.c file: > > > > #include > > > > static const struct clk_parent_data gcc_parent_data_14[] = { > > { .index = PCIE_4_PIPE_CLK_DT }, > > { .index = BI_TCXO_DT }, > > }; > > > > The benefit I see to this is that the index for each clock is in the > > header file (BI_TCXO_DT is 0) and it's next to the clocks property. > > Someone could still mess up the index based on where the macro is used > > in the clocks property though. > > And actually might I suggest an alternative approach to manually using > indices everywhere? What about spending the time once during the boot to > convert .fw_name and clock_names to parent indices during clock registration > and then using them for all the further operations? > I'm pretty sure that's what clk_core_fill_parent_index() already does. Regards, Bjorn