| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zo+B6yzFwRwSkPpH@x1>
Date: Wed, 10 Jul 2024 23:55:39 -0700
From: Drew Fustini <dfustini@...storrent.com>
To: Stephen Boyd <sboyd@...nel.org>
Cc: Albert Ou <aou@...s.berkeley.edu>, Conor Dooley <conor+dt@...nel.org>,
Emil Renner Berthing <emil.renner.berthing@...onical.com>,
Fu Wei <wefu@...hat.com>, Guo Ren <guoren@...nel.org>,
Jisheng Zhang <jszhang@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Palmer Dabbelt <palmer@...belt.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
Rob Herring <robh@...nel.org>,
Thomas Bonnefille <thomas.bonnefille@...tlin.com>,
Yangtao Li <frank.li@...o.com>, linux-riscv@...ts.infradead.org,
linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/7] clk: thead: Add support for T-Head TH1520
AP_SUBSYS clocks
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@...nel.org>
> > + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd.
> > + * Authors: Yangtao Li <frank.li@...o.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
Powered by blists - more mailing lists