Re: [PATCH v4 3/3] clk: add TI CDCE6214 clock driver

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, May 01, 2025 at 11:48:15AM -0700, Stephen Boyd wrote:
> Quoting Sascha Hauer (2025-04-30 02:01:36)
> > diff --git a/drivers/clk/clk-cdce6214.c b/drivers/clk/clk-cdce6214.c
> > new file mode 100644
> > index 0000000000000..62e832dd85ba5
> > --- /dev/null
> > +++ b/drivers/clk/clk-cdce6214.c
> > @@ -0,0 +1,1310 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Driver for the TI CDCE6214 clock generator
> 
> Maybe you can link to the datasheet site

Ok, can do.

> 
> > + *
> > + * Copyright (c) 2023 Alvin Šipraga <alsi@bang-olufsen.dk>
> > + * Copyright (c) 2025 Sascha Hauer <s.hauer@pengutronix.de>
> > + */
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/of.h>
> > +#include <linux/clk.h>
> 
> Drop this include assuming it isn't used.

Dropped.

> > +#define CDCE6214_NUM_CLOCKS    ARRAY_SIZE(clk_names)
> > +
> > +struct cdce6214;
> > +
> > +struct cdce6214_clock {
> > +       struct clk_hw hw;
> > +       struct cdce6214 *priv;
> > +       int index;
> 
> Does it need to be signed?

No, changed to unsigned.

> > +static u8 cdce6214_clk_out0_get_parent(struct clk_hw *hw)
> > +{
> > +       struct cdce6214_clock *clock = hw_to_cdce6214_clk(hw);
> > +       struct cdce6214 *priv = clock->priv;
> > +       unsigned int val, idx;
> > +
> > +       regmap_read(priv->regmap, 2, &val);
> > +
> > +       idx = FIELD_GET(R2_REFSEL_SW, val);
> > +
> > +       switch (idx) {
> > +       case 0:
> > +       case 1:
> 
> Why isn't case 3 here?
> 
> > +               idx = 0;
> > +               break;
> > +       case 2:
> > +               idx = 1;
> > +               break;
> > +       case 3:
> > +               idx = 0;
> > +               break;
> > +       };
> 
> Or even better, idx = 0 by default and if the FIELD_GET() returns 2 idx
> is 1.
> 
> 	if (FIELD_GET(R2_REFSEL_SW, val) == 2)
> 		return 1;
> 
> 	return 0;

Ok, changed.

> > +static int cdce6214_get_out_div(unsigned long rate, unsigned long parent_rate)
> > +{
> > +       unsigned int div = DIV_ROUND_CLOSEST(parent_rate, rate);
> > +
> > +       if (div < 1)
> > +               div = 1;
> > +
> > +       if (div > R72_CH4_DIV)
> > +               div = R72_CH4_DIV;
> > +
> > +       return div;
> 
> Is this divider_get_val(rate, parent_rate, NULL, 13,
> CLK_DIVIDER_ROUND_CLOSEST)?

Will check. One difference is that divider_get_val() actually ignores
the CLK_DIVIDER_ROUND_CLOSEST flag, but maybe DIV_ROUND_UP_ULL() is even
more correct here.

> > +static int pll_calc_values(unsigned long parent_rate, unsigned long out,
> > +                          unsigned long *ndiv, unsigned long *num, unsigned long *den)
> > +{
> > +       u64 a;
> > +
> > +       if (out < CDCE6214_VCO_MIN || out > CDCE6214_VCO_MAX)
> > +               return -EINVAL;
> > +
> > +       *den = 10000000;
> > +       *ndiv = out / parent_rate;
> > +       a = (out % parent_rate);
> 
> Drop useless parenthesis please.

Ok.

> 
> > +       a *= *den;
> > +       do_div(a, parent_rate);
> > +       *num = a;
> > +
> > +       return 0;
> > +}
> > +
> > +static unsigned long cdce6214_clk_pll_recalc_rate(struct clk_hw *hw,
> > +                                                 unsigned long parent_rate)
> > +{
> > +       struct cdce6214_clock *clock = hw_to_cdce6214_clk(hw);
> > +       struct cdce6214 *priv = clock->priv;
> > +       unsigned long ndiv, num, den;
> > +       unsigned int val;
> > +
> > +       regmap_read(priv->regmap, 30, &val);
> 
> Maybe it would be better to have '#define R30 30' just so we can easily
> jump to the fields like R30_PLL_NDIV. I see that the datasheet doesn't
> give a name to these registers besides prefixing the decimal offset with
> the letter 'R'.

Ok.

> 
> > +       ndiv = FIELD_GET(R30_PLL_NDIV, val);
> > +
> > +       regmap_read(priv->regmap, 31, &val);
> > +       num = FIELD_GET(R31_PLL_NUM_15_0, val);
> > +
> > +       regmap_read(priv->regmap, 32, &val);
> > +       num |= FIELD_GET(R32_PLL_NUM_23_16, val) << 16;
> > +
> > +       regmap_read(priv->regmap, 33, &val);
> > +       den = FIELD_GET(R33_PLL_DEN_15_0, val);
> > +
> > +       regmap_read(priv->regmap, 34, &val);
> > +       den |= FIELD_GET(R34_PLL_DEN_23_16, val) << 16;
> > +
> > +       if (!den)
> > +               den = CDCE6214_DENOM_DEFAULT;
> > +
> > +       return parent_rate * ndiv + DIV_ROUND_CLOSEST(parent_rate * num, den);
> > +}
> > +
> > +static long cdce6214_clk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
> > +                                       unsigned long *best_parent_rate)
> > +{
> > +       if (rate < CDCE6214_VCO_MIN)
> > +               rate = CDCE6214_VCO_MIN;
> > +       if (rate > CDCE6214_VCO_MAX)
> > +               rate = CDCE6214_VCO_MAX;
> > +       if (rate < *best_parent_rate * 24)
> 
> What is 24?

It's the minimum allowed divider value for the PLL. I'll add a define
for it.

> > +
> > +       if (enable > 0) {
> > +               regmap_clear_bits(priv->regmap, 5, mask);
> > +       } else if (!enable) {
> > +               regmap_set_bits(priv->regmap, 5, mask);
> > +       } else {
> > +               regmap_read(priv->regmap, 5, &val);
> > +
> > +               return !(val & mask);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int cdce6214_clk_psx_prepare(struct clk_hw *hw)
> > +{
> > +       return cdce6214_clk_psx_ldo(hw, 1);
> 
> Instead of this multiplexing with 1/0/-1 can we have logic that returns
> the mask?
> 
> 	unsigned int cdce6214_clk_psx_mask(struct clk_hw *hw)
> 
> This prepare function would be easier to read because we can see that it
> clears bits
> 
> 	static int cdce6214_clk_psx_prepare(struct clk_hw *hw)
> 	{
> 		struct cdce6214_clock *clock = hw_to_cdce6214_clk(hw);
> 		struct regmap *regmap = clock->priv->regmap;
> 		unsigned int mask = cdce6214_clk_psx_mask(hw);
> 
> 		return regmap_clear_bits(regmap, 5, mask);
> 	}

Ok, can do.

> > +static unsigned long cdce6214_clk_psx_recalc_rate(struct clk_hw *hw,
> > +                                                 unsigned long parent_rate)
> > +{
> > +       struct cdce6214_clock *clock = hw_to_cdce6214_clk(hw);
> > +       struct cdce6214 *priv = clock->priv;
> > +       unsigned int psx[] = { 4, 5, 6, 6 };
> 
> const?

Yes.

> > +static int cdce6214_get_psx_div(unsigned long rate, unsigned long parent_rate)
> > +{
> > +       unsigned int div = DIV_ROUND_CLOSEST(parent_rate, rate);
> > +
> > +       if (div < 4)
> > +               div = 4;
> > +
> > +       if (div > 6)
> > +               div = 6;
> 
> Use 'return clamp(div, 4, 6)'

Ok.

> > +static int cdce6214_clk_register(struct cdce6214 *priv)
> > +{
> > +       struct clk_init_data init[CDCE6214_NUM_CLOCKS] = { 0 };
> > +       struct clk_parent_data pdata_out0[2] = {};
> > +       struct clk_parent_data pdata_out[4] = {};
> > +       struct clk_parent_data pdata_pll = {};
> > +       struct clk_parent_data pdata_psx = {};
> > +       int i, ret;
> > +
> > +       pdata_out0[0].fw_name = "priref";
> > +       pdata_out0[1].fw_name = "secref";
> > +
> > +       init[CDCE6214_CLK_OUT0].ops = &cdce6214_clk_out0_ops;
> > +       init[CDCE6214_CLK_OUT0].num_parents = 2;
> > +       init[CDCE6214_CLK_OUT0].parent_data = pdata_out0;
> > +       init[CDCE6214_CLK_OUT0].flags = CLK_SET_RATE_NO_REPARENT;
> > +
> > +       pdata_out[0].hw = &priv->clk[CDCE6214_CLK_PSA].hw;
> > +       pdata_out[1].hw = &priv->clk[CDCE6214_CLK_PSB].hw;
> > +       pdata_out[3].hw = &priv->clk[CDCE6214_CLK_OUT0].hw;
> > +
> > +       for (i = CDCE6214_CLK_OUT1; i <= CDCE6214_CLK_OUT4; i++) {
> > +               init[i].ops = &cdce6214_clk_out_ops;
> > +               init[i].num_parents = 4;
> 
> Please use ARRAY_SIZE(pdata_out) so we don't worry that the static
> assignment above gets changed without this changing too.

Ok.

> 
> > +               init[i].parent_data = pdata_out;
> > +               init[i].flags = CLK_SET_RATE_NO_REPARENT;
> > +       }
> > +
> > +       init[CDCE6214_CLK_PLL].ops = &cdce6214_clk_pll_ops;
> > +       init[CDCE6214_CLK_PLL].num_parents = 1;
> > +       pdata_pll.hw = &priv->clk[CDCE6214_CLK_OUT0].hw;
> > +       init[CDCE6214_CLK_PLL].parent_data = &pdata_pll;
> > +
> > +       pdata_psx.hw = &priv->clk[CDCE6214_CLK_PLL].hw;
> > +       for (i = CDCE6214_CLK_PSA; i <= CDCE6214_CLK_PSB; i++) {
> > +               init[i].ops = &cdce6214_clk_psx_ops;
> > +               init[i].num_parents = 1;
> 
> Same sort of comment.

I don't understand this comment here. num_parents is 1, there is no
array. Did you mean to comment on the place further up where num_parents
is set to 2 instead?

> 
> > +               init[i].parent_data = &pdata_psx;
> > +       }
> > +
> > +       for (i = 0; i < CDCE6214_NUM_CLOCKS; i++) {
> > +               struct cdce6214_clock *clk = &priv->clk[i];
> > +               char name[128];
> > +
> > +               if (!init[i].ops)
> > +                       continue;
> > +
> > +               snprintf(name, sizeof(name), "%s_%s", dev_name(priv->dev), clk_names[i]);
> > +               init[i].name = name;
> > +               clk->hw.init = &init[i];
> > +               clk->priv = priv;
> > +               clk->index = i;
> > +               ret = devm_clk_hw_register(priv->dev, &clk->hw);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static void cdce6214_setup_xtal(struct cdce6214 *priv, struct device_node *np)
> > +{
> > +       unsigned short ip_xo_cload[] = {
> 
> const?

Yes.

> 
> > +               /* index is the register value */
> > +               3000, 3200, 3400, 3600, 3800, 4000, 4200, 4400,
> > +               4600, 4800, 5000, 5200, 5400, 5600, 5800, 6000,
> > +               6200, 6400, 6500, 6700, 6900, 7100, 7300, 7500,
> > +               7700, 7900, 8100, 8300, 8500, 8700, 8900, 9000
> > +       };
> > +
> > +       unsigned short ip_bias_sel_xo[] = {
> 
> const?

Yes.

> > +static int cdce6214_get_clkout_fmt(struct cdce6214 *priv, struct device_node *np)
> > +{
> > +       const char *fmt;
> > +       int ret;
> > +
> > +       ret = of_property_read_string(np, "ti,clkout-fmt", &fmt);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return match_string(clkkout_fmt_names, ARRAY_SIZE(clkkout_fmt_names), fmt);
> 
> We have a helper for this sort of thing.
> device_property_match_property_string()? Likely you can get rid of these
> helpers and inline the call to that function instead.

device_property_match_property_string() uses dev->of_node, but we have a
subnode of that node here.

> > +static int cdce6214_parse_subnode(struct cdce6214 *priv, struct device_node *np)
> > +{
> > +       struct regmap *reg = priv->regmap;
> > +       unsigned int idx, val;
> > +       int fmt;
> > +       int ret;
> > +
> > +       ret = of_property_read_u32(np, "reg", &idx);
> > +       if (ret) {
> > +               dev_err(priv->dev, "missing reg property in child: %s\n",
> > +                       np->full_name);
> > +               return ret;
> > +       }
> 
> I don't like this binding design. It is too much one node per clk style,
> which we don't want.

Note the subnodes are merely used for configuring the in/outputs, not
for referring to as a clock. Maybe I should have named them input/output
instead of clk.

> Assuming these clkout formats are configuring
> things, can we have that be an array of strings indexed based on the
> DT specifier for the provider, similar to assigned-clocks? Then we don't
> need a node for each configuration.

We have two inputs and four outputs with a different set of properties.

With your suggestion we would get something like this:

	ti,clkin-fmt = "lvcmos", "xtal"; /* two inputs */

	ti,xo-cload-femtofarads = <4500>; /* XO, only on PRIREF, no array */
	ti,xo-bias-current-microamp = <1758>; /* XO, only on PRIREF, no array */

	ti,clkout-fmt = "cmos", "lvds", "cmos", "cmos"; /* four outputs */
	ti,cmosn-mode = "low", "disabled", "disabled", "disabled";
	ti,cmosp-mode = "high", "disabled", "disabled", "disabled";

Would that be Ok?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |