Re: [PATCH v2 2/7] clk: thead: Add support for T-Head TH1520 AP_SUBSYS clocks

[Drew Fustini <dfustini@tenstorrent.com>]

On Wed, Jul 10, 2024 at 04:17:12PM -0700, Stephen Boyd wrote:
> Quoting Drew Fustini (2024-06-23 19:12:32)
> > diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c
> > new file mode 100644
> > index 000000000000..982d4d40f783
> > --- /dev/null
> > +++ b/drivers/clk/thead/clk-th1520-ap.c
> > @@ -0,0 +1,1086 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
> > + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
> > + *  Authors: Yangtao Li <frank.li@vivo.com>
> > + */
> > +
> > +#include <dt-bindings/clock/thead,th1520-clk-ap.h>
> 
> Preferably include dt-bindings after linux includes.

Okay, I will move it.

> 
> > +#include <linux/bitfield.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#define TH1520_PLL_POSTDIV2    GENMASK(26, 24)
> > +#define TH1520_PLL_POSTDIV1    GENMASK(22, 20)
> > +#define TH1520_PLL_FBDIV       GENMASK(19, 8)
> > +#define TH1520_PLL_REFDIV      GENMASK(5, 0)
> > +#define TH1520_PLL_BYPASS      BIT(30)
> > +#define TH1520_PLL_DSMPD       BIT(24)
> > +#define TH1520_PLL_FRAC                GENMASK(23, 0)
> > +#define TH1520_PLL_FRAC_BITS    24
> [...]
> > +
> > +static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw,
> > +                                               unsigned long parent_rate)
> > +{
> > +       struct ccu_pll *pll = hw_to_ccu_pll(hw);
> > +       unsigned long div, mul, frac, rate = parent_rate;
> > +       unsigned int cfg0, cfg1;
> > +
> > +       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> > +       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> > +
> > +       mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0);
> > +       div = FIELD_GET(TH1520_PLL_REFDIV, cfg0);
> > +       if (!(cfg1 & TH1520_PLL_DSMPD)) {
> > +               mul <<= TH1520_PLL_FRAC_BITS;
> > +               frac = FIELD_GET(TH1520_PLL_FRAC, cfg1);
> > +               mul += frac;
> > +               div <<= TH1520_PLL_FRAC_BITS;
> > +       }
> > +       rate = parent_rate * mul;
> > +       do_div(rate, div);
> 
> 'rate' is only unsigned long, so do_div() isn't needed here. Perhaps if
> 'parent_rate * mul' can overflow 32-bits then 'rate' should be
> u64.

Thanks for pointing that out. I will make 'rate' u64 as I believe
'parent_rate * mul' could overflow:

The ref clock for all the PLLs on this SoC is intended to be 24 MHz
(section 4.3.2 PLL Resources [1]). Thus it is expected that parent_rate
will use 24 bits.

'mul' is set to TH1520_PLL_FBDIV which is 12 bits. In DSMPD mode, 'mul'
gets shifted left by TH1520_PLL_FRAC_BITS which is 24 bits.

> > +       return rate;
> > +}
> > +
> > +static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw,
> > +                                                   unsigned long parent_rate)
> > +{
> > +       struct ccu_pll *pll = hw_to_ccu_pll(hw);
> > +       unsigned long rate = parent_rate;
> > +       unsigned int cfg0, cfg1;
> > +
> > +       regmap_read(pll->common.map, pll->common.cfg0, &cfg0);
> > +       regmap_read(pll->common.map, pll->common.cfg1, &cfg1);
> > +
> > +       if (cfg1 & TH1520_PLL_BYPASS)
> > +               return rate;
> > +
> > +       do_div(rate, FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) *
> 
> Same, 'rate' is unsigned long. Did you get some compilation error
> without this? How big is the divisor going to be? The fields are only
> 3-bits wide, so the multiplication would fit into a u32 just fine. Given
> that 'rate' is unsigned long though I think you can just put the
> multiplication result into a local variable that's also unsigned long
> and then just write the divide with unsigned longs
> 
> 	div = FIELD_GET(...) * FIELD_GET(...);
> 
> 	return rate / div;

I didn't get any compiler errors. I had copied do_div() from another
driver that I was looking at.

You are right that TH1520_PLL_POSTDIV1 and TH1520_PLL_POSTDIV2 are both
just 3 bits each. Thus I think the maximum divisor is 64. I'll change
to the simpler "rate / div" that you suggest.

> > +                    FIELD_GET(TH1520_PLL_POSTDIV2, cfg0));
> > +
> > +       return rate;
> > +}
> > +
> > +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw,
> > +                                        unsigned long parent_rate)
> > +{
> > +       unsigned long rate = parent_rate;
> > +
> > +       rate = th1520_pll_vco_recalc_rate(hw, rate);
> > +       rate = th1520_pll_postdiv_recalc_rate(hw, rate);
> > +
> > +       return rate;
> > +}
> 
> Please fold this in

Will do.

Thanks for the review,
Drew

[1] https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf