Re: [PATCH v2 2/2] clk: qcom: Add clock driver for SM8450

From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>, Vinod Koul <vkoul@kernel.org>,
	linux-arm-msm@vger.kernel.org, Andy Gross <agross@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Vamsi Krishna Lanka <quic_vamslank@quicinc.com>,
	linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] clk: qcom: Add clock driver for SM8450
Date: Sat, 11 Dec 2021 12:13:17 -0600	[thread overview]
Message-ID: <YbTqPfs7026l6LFE@builder.lan> (raw)
In-Reply-To: <20211209082537.1AF6CC341C8@smtp.kernel.org>

On Thu 09 Dec 02:25 CST 2021, Stephen Boyd wrote:

> Quoting Vinod Koul (2021-12-07 03:40:03)
> > diff --git a/drivers/clk/qcom/gcc-sm8450.c b/drivers/clk/qcom/gcc-sm8450.c
> > new file mode 100644
> > index 000000000000..82ac419718d7
> > --- /dev/null
> > +++ b/drivers/clk/qcom/gcc-sm8450.c
> > @@ -0,0 +1,3303 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2021, Linaro Limited
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <dt-bindings/clock/qcom,gcc-sm8450.h>
> > +
> > +#include "clk-alpha-pll.h"
> > +#include "clk-branch.h"
> > +#include "clk-rcg.h"
> > +#include "clk-regmap.h"
> > +#include "clk-regmap-divider.h"
> > +#include "clk-regmap-mux.h"
> > +#include "gdsc.h"
> > +#include "reset.h"
> > +
> > +enum {
> > +       P_BI_TCXO,
> > +       P_GCC_GPLL0_OUT_EVEN,
> > +       P_GCC_GPLL0_OUT_MAIN,
> > +       P_GCC_GPLL4_OUT_MAIN,
> > +       P_GCC_GPLL9_OUT_MAIN,
> > +       P_PCIE_0_PIPE_CLK,
> > +       P_PCIE_1_PHY_AUX_CLK,
> > +       P_PCIE_1_PIPE_CLK,
> > +       P_SLEEP_CLK,
> > +       P_UFS_PHY_RX_SYMBOL_0_CLK,
> > +       P_UFS_PHY_RX_SYMBOL_1_CLK,
> > +       P_UFS_PHY_TX_SYMBOL_0_CLK,
> > +       P_USB3_PHY_WRAPPER_GCC_USB30_PIPE_CLK,
> > +};
> > +
> > +static struct clk_alpha_pll gcc_gpll0 = {
> > +       .offset = 0x0,
> > +       .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID_EVO],
> > +       .clkr = {
> > +               .enable_reg = 0x62018,
> > +               .enable_mask = BIT(0),
> > +               .hw.init = &(struct clk_init_data){
> > +                       .name = "gcc_gpll0",
> > +                       .parent_data = &(const struct clk_parent_data){
> > +                               .fw_name = "bi_tcxo",
> 
> Maybe you want to drop these strings and use the dt index directly? That
> may actually be faster because we don't do as many string comparisons
> and the code may be smaller if we don't have to store bi_tcxo. I suppose
> to make it more readable we could have #defines for each DT index like
> 
>  #define DT_BI_TCXO	0
>  #define DT_SLEEP_CLK	1
> 
> Blaze a new trail!
> 

I like the idea, and iiuc it's just a matter of replacing .fw_name with
.index?

I am however worried that people will get the order wrong as they are
hacking on their dts/drivers, because (at least in my view) the order of
clocks & clock-names has been seen as "a dt binding requirement" up
until such change. But if we replace the names with indices such enum
would have to be kept in sync with the DT binding and there's no way to
validate it.

If we do this we should force the driver and dts-writers to rely on the
binding document by omitting clock-names from the binding (and hence
dts). Otherwise people will (I will) assume that the clock-names are
still what matters...

Regards,
Bjorn

> > +                       },
> > +                       .num_parents = 1,
> > +                       .ops = &clk_alpha_pll_fixed_lucid_evo_ops,
> > +               },
> > +       },
> > +};