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@xxxxxxxxxx>
> > + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
> > + * Authors: Yangtao Li <frank.li@xxxxxxxx>
> > + */
> > +
> > +#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
