Messages in this thread | | | From | Ryan Chen <> | Subject | RE: [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula | Date | Fri, 29 Oct 2021 05:35:03 +0000 |
| |
> -----Original Message----- > From: Andrew Jeffery <andrew@aj.id.au> > Sent: Friday, October 29, 2021 11:46 AM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Joel Stanley <joel@jms.id.au> > Cc: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd > <sboyd@kernel.org>; linux-clk@vger.kernel.org; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org> > Subject: Re: [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula > > > > On Fri, 8 Oct 2021, at 18:02, Ryan Chen wrote: > >> -----Original Message----- > >> From: Joel Stanley <joel@jms.id.au> > >> Sent: Friday, October 8, 2021 12:06 PM > >> To: Ryan Chen <ryan_chen@aspeedtech.com> > >> Cc: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd > >> <sboyd@kernel.org>; Andrew Jeffery <andrew@aj.id.au>; > >> linux-clk@vger.kernel.org; Linux Kernel Mailing List > >> <linux-kernel@vger.kernel.org> > >> Subject: Re: [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula > >> > >> On Sat, 25 Sept 2021 at 02:24, Ryan Chen <ryan_chen@aspeedtech.com> > >> wrote: > >> > > >> > >> A few notes on process: > >> > >> > v2 -> v3: change else than if to directly else if > >> > v1 -> v2: add Fixes commit hash > >> > >> As this is normally information for reviewers to know what you've > >> changed since the last version, we normally put this below the --- in > >> the patch, which means it is not included in the commit message. > >> > >> Also we put a space between the PATCH and v3 in the subject. If you > >> use the tools, it will generate this for you: > >> > >> git format-patch -v3 -1 --to=... > >> > >> > > >> > AST2600 HPLL calculate formula [SCU200] HPLL Numerator(M): have > >> > fixed value depend on SCU strap. > >> > M = SCU500[10] ? 0x5F : SCU500[8] ? 0xBF : SCU200[12:0] > >> > >> I recommend adding to the commit message the text from my first review: > >> > >> From the datasheet: > >> > >> CPU frequency selection > >> 000 1.2GHz > >> 001 1.6GHz > >> 010 1.2GHz > >> 011 1.6GHz > >> 100 800MHz > >> 101 800MHz > >> 110 800MHz > >> 111 800MHz > >> > >> So when the system is running at 800MHz or 1.6GHz, the value for the > >> numerator (m) in SCU204 is incorrect, and must be overridden. > > > > Yes, SCU204 will be overridden by chip design. > > Let me clarify m is in SCU200[12:0] not SCU204. SCU204 is NB not > > related with freq. > > > >> > >> > > >> > if SCU500[10] = 1, M=0x5F. > >> > else if SCU500[10]=0 & SCU500[8]=1, M=0xBF. > >> > others (SCU510[10]=0 and SCU510[8]=0) depend on SCU200[12:0] > >> > (default 0x8F) register setting. > >> > > >> > HPLL Denumerator (N) = SCU200[18:13] (default 0x2) > >> > HPLL Divider (P) = SCU200[22:19] (default 0x0) > >> > > >> > Fixes: d3d04f6c330a ("clk: Add support for AST2600 SoC") > >> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > >> > --- > >> > drivers/clk/clk-ast2600.c | 28 +++++++++++++++++++++++++++- > >> > 1 file changed, 27 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c > >> > index 085d0a18b2b6..d30188355aaf 100644 > >> > --- a/drivers/clk/clk-ast2600.c > >> > +++ b/drivers/clk/clk-ast2600.c > >> > @@ -169,6 +169,32 @@ static const struct clk_div_table > >> > ast2600_div_table[] = { }; > >> > > >> > /* For hpll/dpll/epll/mpll */ > >> > +static struct clk_hw *ast2600_calc_hpll(const char *name, u32 val) { > >> > + unsigned int mult, div; > >> > + u32 hwstrap = readl(scu_g6_base + ASPEED_G6_STRAP1); > >> > + > >> > + if (val & BIT(24)) { > >> > + /* Pass through mode */ > >> > + mult = div = 1; > >> > + } else { > >> > + /* F = 25Mhz * [(M + 2) / (n + 1)] / (p + 1) */ > >> > + u32 m = val & 0x1fff; > >> > + u32 n = (val >> 13) & 0x3f; > >> > + u32 p = (val >> 19) & 0xf; > >> > + > >> > >> Add a comment: > >> > >> /* If the CPU is running at 800Mhz. */ > >> > >> > + if (hwstrap & BIT(10)) > >> > + m = 0x5F; > >> > >> /* If the CPU is running at 1600Mhz. */ > >> > >> > + else if (hwstrap & BIT(8)) > >> > + m = 0xBF; > >> > >> > >> Or you could copy what I suggested in the first patch, and write it > >> like this, which I think is clear: > >> > >> ff (hwstrap & BIT(10)) { > >> /* CPU running at 800MHz */ > >> m = 95; > >> } else if (hwstrap & BIT(10)) { > >> /* CPU running at 1.6GHz */ > >> m = 191; > >> } else { > >> /* CPU running at 1.2Ghz */ > >> m = val & 0x1fff; > >> } > > > > How about following > > > > ff (hwstrap & BIT(10)) { > > /* CPU running at 800MHz */ > > m = 0x5F; > > } else if (hwstrap & BIT(10)) { > > This is the same condition as the `if` above. That doesn't seem right.
It would modify to following } else if (hwstrap & BIT(8)) { > > > /* CPU running at 1.6GHz */ > > m = 0xBF; > > } else { > > /* CPU running at 1.2Ghz */ > > m = val & 0x1fff; > > } > > > >>
| |