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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACPK8XfrR-m6HOx5VOQ1-AVf0r9-0BKsr019D7T6y9Qd4qL9gA@mail.gmail.com>
Date:   Wed, 1 Sep 2021 06:49:26 +0000
From:   Joel Stanley <joel@....id.au>
To:     Ryan Chen <ryan_chen@...eedtech.com>
Cc:     BMC-SW <bmc-sw@...eedtech.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Andrew Jeffery <andrew@...id.au>, linux-clk@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] clk:aspeed:Fix AST2600 hpll calculate formula

Hello Ryan,

Thanks for the patch. I have some questions about it below.

On Wed, 18 Aug 2021 at 08:05, Ryan Chen <ryan_chen@...eedtech.com> wrote:
>
> 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 see 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?

> 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)

Is this the case for all revisions of the soc, from A0 through to A3?

Do you have a datasheet update that captures this information?

When you resend, please add a fixes line as follows, as this code has
been present since we introduced the driver:

Fixes: d3d04f6c330a ("clk: Add support for AST2600 SoC")

>
> Signed-off-by: Ryan Chen <ryan_chen@...eedtech.com>
> ---
>  drivers/clk/clk-ast2600.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> index 085d0a18b2b6..5d8c46bcf237 100644
> --- a/drivers/clk/clk-ast2600.c
> +++ b/drivers/clk/clk-ast2600.c
> @@ -169,6 +169,33 @@ static const struct clk_div_table ast2600_div_table[] = {
>  };
>
>  /* For hpll/dpll/epll/mpll */

This comment needs to stay with ast2600_calc_pll, and just drop hpll
from the list.

> +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;
> +
> +               if (hwstrap & BIT(10))

So this is testing if the CPU is running at 800Mhz.

> +                       m = 0x5F;
> +               else {
> +                       if (hwstrap & BIT(8))

And this is testing if the CPU is running at 1.6GHz.

I would write it like this:

u32 m;

if (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;
}

Cheers,

Joel

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ