* [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula
@ 2021-09-25 2:23 Ryan Chen
2021-10-07 7:45 ` Ryan Chen
2021-10-08 4:06 ` Joel Stanley
0 siblings, 2 replies; 6+ messages in thread
From: Ryan Chen @ 2021-09-25 2:23 UTC (permalink / raw)
To: ryan_chen, Michael Turquette, Stephen Boyd, Joel Stanley,
Andrew Jeffery, linux-clk, linux-kernel
v2 -> v3: change else than if to directly else if
v1 -> v2: add Fixes commit hash
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]
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;
+
+ if (hwstrap & BIT(10))
+ m = 0x5F;
+ else if (hwstrap & BIT(8))
+ m = 0xBF;
+
+ mult = (m + 1) / (n + 1);
+ div = (p + 1);
+ }
+ return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
+ mult, div);
+};
+
static struct clk_hw *ast2600_calc_pll(const char *name, u32 val)
{
unsigned int mult, div;
@@ -716,7 +742,7 @@ static void __init aspeed_g6_cc(struct regmap *map)
* and we assume that it is enabled
*/
regmap_read(map, ASPEED_HPLL_PARAM, &val);
- aspeed_g6_clk_data->hws[ASPEED_CLK_HPLL] = ast2600_calc_pll("hpll", val);
+ aspeed_g6_clk_data->hws[ASPEED_CLK_HPLL] = ast2600_calc_hpll("hpll", val);
regmap_read(map, ASPEED_MPLL_PARAM, &val);
aspeed_g6_clk_data->hws[ASPEED_CLK_MPLL] = ast2600_calc_pll("mpll", val);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula
2021-09-25 2:23 [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula Ryan Chen
@ 2021-10-07 7:45 ` Ryan Chen
2021-10-08 4:06 ` Joel Stanley
1 sibling, 0 replies; 6+ messages in thread
From: Ryan Chen @ 2021-10-07 7:45 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Joel Stanley, Andrew Jeffery,
linux-clk, linux-kernel
Hello Joel,
Do you have time check this path?
Ryan Chen
Tel : 886-3-5751185 ext:8857
> -----Original Message-----
> From: Ryan Chen <ryan_chen@aspeedtech.com>
> Sent: Saturday, September 25, 2021 10:24 AM
> To: Ryan Chen <ryan_chen@aspeedtech.com>; Michael Turquette
> <mturquette@baylibre.com>; Stephen Boyd <sboyd@kernel.org>; Joel Stanley
> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>;
> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula
>
> v2 -> v3: change else than if to directly else if
> v1 -> v2: add Fixes commit hash
>
> 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]
>
> 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;
> +
> + if (hwstrap & BIT(10))
> + m = 0x5F;
> + else if (hwstrap & BIT(8))
> + m = 0xBF;
> +
> + mult = (m + 1) / (n + 1);
> + div = (p + 1);
> + }
> + return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
> + mult, div);
> +};
> +
> static struct clk_hw *ast2600_calc_pll(const char *name, u32 val) {
> unsigned int mult, div;
> @@ -716,7 +742,7 @@ static void __init aspeed_g6_cc(struct regmap *map)
> * and we assume that it is enabled
> */
> regmap_read(map, ASPEED_HPLL_PARAM, &val);
> - aspeed_g6_clk_data->hws[ASPEED_CLK_HPLL] = ast2600_calc_pll("hpll",
> val);
> + aspeed_g6_clk_data->hws[ASPEED_CLK_HPLL] = ast2600_calc_hpll("hpll",
> +val);
>
> regmap_read(map, ASPEED_MPLL_PARAM, &val);
> aspeed_g6_clk_data->hws[ASPEED_CLK_MPLL] = ast2600_calc_pll("mpll",
> val);
> --
> 2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula
2021-09-25 2:23 [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula Ryan Chen
2021-10-07 7:45 ` Ryan Chen
@ 2021-10-08 4:06 ` Joel Stanley
2021-10-08 7:32 ` Ryan Chen
1 sibling, 1 reply; 6+ messages in thread
From: Joel Stanley @ 2021-10-08 4:06 UTC (permalink / raw)
To: Ryan Chen
Cc: Michael Turquette, Stephen Boyd, Andrew Jeffery, linux-clk,
Linux Kernel Mailing List
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.
>
> 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;
}
Please send a v4 with some more comments so we don't need to go back
to the datasheet each time we read this code. Feel free to cut/paste
my suggestion and use it as-is.
Cheers,
Joel
> +
> + mult = (m + 1) / (n + 1);
> + div = (p + 1);
> + }
> + return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
> + mult, div);
> +};
> +
> static struct clk_hw *ast2600_calc_pll(const char *name, u32 val)
> {
> unsigned int mult, div;
> @@ -716,7 +742,7 @@ static void __init aspeed_g6_cc(struct regmap *map)
> * and we assume that it is enabled
> */
> regmap_read(map, ASPEED_HPLL_PARAM, &val);
> - aspeed_g6_clk_data->hws[ASPEED_CLK_HPLL] = ast2600_calc_pll("hpll", val);
> + aspeed_g6_clk_data->hws[ASPEED_CLK_HPLL] = ast2600_calc_hpll("hpll", val);
>
> regmap_read(map, ASPEED_MPLL_PARAM, &val);
> aspeed_g6_clk_data->hws[ASPEED_CLK_MPLL] = ast2600_calc_pll("mpll", val);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula
2021-10-08 4:06 ` Joel Stanley
@ 2021-10-08 7:32 ` Ryan Chen
2021-10-29 3:45 ` Andrew Jeffery
0 siblings, 1 reply; 6+ messages in thread
From: Ryan Chen @ 2021-10-08 7:32 UTC (permalink / raw)
To: Joel Stanley
Cc: Michael Turquette, Stephen Boyd, Andrew Jeffery, linux-clk,
Linux Kernel Mailing List
> -----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)) {
/* CPU running at 1.6GHz */
m = 0xBF;
} else {
/* CPU running at 1.2Ghz */
m = val & 0x1fff;
}
>
> Please send a v4 with some more comments so we don't need to go back to
> the datasheet each time we read this code. Feel free to cut/paste my
> suggestion and use it as-is.
>
Thanks a lot.
> Cheers,
>
> Joel
>
> > +
> > + mult = (m + 1) / (n + 1);
> > + div = (p + 1);
> > + }
> > + return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
> > + mult, div);
> > +};
> > +
> > static struct clk_hw *ast2600_calc_pll(const char *name, u32 val) {
> > unsigned int mult, div;
> > @@ -716,7 +742,7 @@ static void __init aspeed_g6_cc(struct regmap
> *map)
> > * and we assume that it is enabled
> > */
> > regmap_read(map, ASPEED_HPLL_PARAM, &val);
> > - aspeed_g6_clk_data->hws[ASPEED_CLK_HPLL] =
> ast2600_calc_pll("hpll", val);
> > + aspeed_g6_clk_data->hws[ASPEED_CLK_HPLL] =
> > + ast2600_calc_hpll("hpll", val);
> >
> > regmap_read(map, ASPEED_MPLL_PARAM, &val);
> > aspeed_g6_clk_data->hws[ASPEED_CLK_MPLL] =
> ast2600_calc_pll("mpll", val);
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula
2021-10-08 7:32 ` Ryan Chen
@ 2021-10-29 3:45 ` Andrew Jeffery
2021-10-29 5:35 ` Ryan Chen
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Jeffery @ 2021-10-29 3:45 UTC (permalink / raw)
To: Ryan Chen, Joel Stanley
Cc: Michael Turquette, Stephen Boyd, linux-clk, Linux Kernel Mailing List
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.
> /* CPU running at 1.6GHz */
> m = 0xBF;
> } else {
> /* CPU running at 1.2Ghz */
> m = val & 0x1fff;
> }
>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula
2021-10-29 3:45 ` Andrew Jeffery
@ 2021-10-29 5:35 ` Ryan Chen
0 siblings, 0 replies; 6+ messages in thread
From: Ryan Chen @ 2021-10-29 5:35 UTC (permalink / raw)
To: Andrew Jeffery, Joel Stanley
Cc: Michael Turquette, Stephen Boyd, linux-clk, Linux Kernel Mailing List
> -----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;
> > }
> >
> >>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-29 5:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-25 2:23 [PATCHv3] clk:aspeed:Fix AST2600 hpll calculate formula Ryan Chen
2021-10-07 7:45 ` Ryan Chen
2021-10-08 4:06 ` Joel Stanley
2021-10-08 7:32 ` Ryan Chen
2021-10-29 3:45 ` Andrew Jeffery
2021-10-29 5:35 ` Ryan Chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.