Change in ...coreboot[master]: google/kukui: Raising the CPU frequency

[Tristan Hsieh (Code Review)]

Tristan Hsieh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32466

Change subject: google/kukui: Raising the CPU frequency
......................................................................

google/kukui: Raising the CPU frequency

Run CPU at the highest freqency (1989MHz) to speed up the boot time.

BUG=b:80501386
BRANCH=none
Test=Boots correctly on Kukui

Change-Id: I703ffcb99367f87e6792a72485f5634e0505e5ac
Signed-off-by: Tristan Shieh 
---
M src/mainboard/google/kukui/romstage.c
1 file changed, 2 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/32466/1

diff --git a/src/mainboard/google/kukui/romstage.c b/src/mainboard/google/kukui/romstage.c
index 81ae9c5..baaca43 100644
--- a/src/mainboard/google/kukui/romstage.c
+++ b/src/mainboard/google/kukui/romstage.c
@@ -17,6 +17,7 @@
 #include <soc/emi.h>
 #include <soc/mmu_operations.h>
 #include <soc/mt6358.h>
+#include <soc/pll.h>
 #include <soc/rtc.h>

#include "early_init.h"
@@ -28,6 +29,7 @@
    	mainboard_early_init();

mt6358_init();
+	mt_pll_raise_ca53_freq(1989 * MHz);
    rtc_boot();
    mt_mem_init(get_sdram_config());
    mtk_mmu_after_dram();

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raising the CPU frequency
......................................................................

Patch Set 2: Code-Review+1

[Tristan Hsieh (Code Review)]

Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raising the CPU frequency
......................................................................

Patch Set 2:

According to pmic spec (MT6358_PMIC_Data_Sheet_V1.3.pdf), the voltage will be stable in 200us.
It takes about 700us from increasing voltage to end of mt6358_init().
So, it's safe to raising CPU frequency after mt6538_init()

Here is my test patch:
diff --git a/src/soc/mediatek/mt8183/mt6358.c b/src/soc/mediatek/mt8183/mt6358.c
index 8162e3a..81e7fde 100644
--- a/src/soc/mediatek/mt8183/mt6358.c
+++ b/src/soc/mediatek/mt8183/mt6358.c
@@ -772,7 +772,7 @@ static void mt6358_lp_setting(void)
                        lp_setting[i].addr, lp_setting[i].val,
                        lp_setting[i].mask, lp_setting[i].shift);
 }
-
+#include <timestamp.h>
 void mt6358_init(void)
 {
        if (pwrap_init())
@@ -781,7 +781,9 @@ void mt6358_init(void)
        pmic_set_power_hold(true);
        pmic_wdt_set();
        mt6358_init_setting();
+timestamp_add_now(TS_START_KERNEL);
        wk_sleep_voltage_by_ddr();
        wk_power_down_seq();
        mt6358_lp_setting();
+timestamp_add_now(TS_START_KERNEL);
 }

Log...

1:start of romstage                                 334,046 (1)
1101:jumping to kernel                                 336,516 (2,470)
1101:jumping to kernel                                 337,273 (757)

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raising the CPU frequency
......................................................................

Patch Set 2:
> According to pmic spec (MT6358_PMIC_Data_Sheet_V1.3.pdf), the voltage will be stable in 200us.
> It takes about 700us from increasing voltage to end of mt6358_init().
> So, it's safe to raising CPU frequency after mt6538_init()

Thanks for the great info. Can state that in comment before we call the function to raise CPU freq?
So that if someone is trying to optimize they won't miss it.

[Tristan Hsieh (Code Review)]

Hello hsin-hsiung wang, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Weiyi Lu,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32466

to look at the new patch set (#3).

Change subject: google/kukui: Raising the CPU frequency
......................................................................

google/kukui: Raising the CPU frequency

Run CPU at the highest freqency (1989MHz) to speed up the boot time.

BUG=b:80501386
BRANCH=none
Test=Boots correctly on Kukui

Change-Id: I703ffcb99367f87e6792a72485f5634e0505e5ac
Signed-off-by: Tristan Shieh 
---
M src/mainboard/google/kukui/romstage.c
M src/soc/mediatek/mt8183/mt6358.c
2 files changed, 6 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/32466/3

[Tristan Hsieh (Code Review)]

Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raising the CPU frequency
......................................................................

Patch Set 3:
> Thanks for the great info. Can state that in comment before we call the function to raise CPU freq?
> So that if someone is trying to optimize they won't miss it.

I add comment in mt6358_init().
The idea is that we don't have to worry about anything if mt6358_init() return after voltage became stable.

I know it's not good to modify files of soc and mainboard in the same patch.
But, can we do that for this case?
Since we only add comments in mt6358.c and they are both for the same issue.

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raising the CPU frequency
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/32466/3/src/mainboard/google/kukui/romstage.... 
File src/mainboard/google/kukui/romstage.c:

https://review.coreboot.org/#/c/32466/3/src/mainboard/google/kukui/romstage.... 
PS3, Line 32: mt_pll_raise_ca53_freq
I think it makes more send to me to add the comment here.

mt6358_init();
 /*
  * Raising frequency need higher voltage, which is set inside mt6358_init
  * and need 200us to be stable before setting new frequency.
  * If we want to move mt_pll_raise_ca53_freq to other places, be sure
  * to measure again the duration between them.
  */
  mt_pll_rais_ca53_freq(1989 * MHz);

https://review.coreboot.org/#/c/32466/3/src/soc/mediatek/mt8183/mt6358.c 
File src/soc/mediatek/mt8183/mt6358.c:

https://review.coreboot.org/#/c/32466/3/src/soc/mediatek/mt8183/mt6358.c@783 
PS3, Line 783: /*
             : 	 * Voltage will take 200us to be stable if we change it.
             : 	 * Do not return from this function befere voltage is stable.
             : 	 */
I think it's weird to have the comment here since it's not trivial to figure out which function will be changing voltage, and how/when we can ensure 200us is met.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raising the CPU frequency
......................................................................

Patch Set 3:

Please add explicit measurements if you want to guarantee a certain minimum settle time. You could do something like this:

struct stopwatch voltage_settled;

void function_that_increases_voltage()
 {
   ...voltage increase...
   stopwatch_init_usecs_expire(&voltage_settled, 200);
 }

void mt6358_init()
 {
   ...other stuff...
   while (!stopwatch_expired(&voltage_settled))
     /* wait for voltages to settle */;
 }

[Tristan Hsieh (Code Review)]

Hello hsin-hsiung wang, Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Weiyi Lu,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32466

to look at the new patch set (#4).

Change subject: google/kukui: Raising the CPU frequency
......................................................................

google/kukui: Raising the CPU frequency

Run CPU at the highest freqency (1989MHz) to speed up the boot time.

BUG=b:80501386
BRANCH=none
Test=Boots correctly on Kukui

Change-Id: I703ffcb99367f87e6792a72485f5634e0505e5ac
Signed-off-by: Tristan Shieh 
---
M src/mainboard/google/kukui/romstage.c
1 file changed, 2 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/32466/4

[Tristan Hsieh (Code Review)]

Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raising the CPU frequency
......................................................................

Patch Set 4:
> Patch Set 3:
>
> Please add explicit measurements if you want to guarantee a certain minimum settle time. You could do something like this:
>
> struct stopwatch voltage_settled;
>
> void function_that_increases_voltage()
>  {
>    ...voltage increase...
>    stopwatch_init_usecs_expire(&voltage_settled, 200);
>  }
>
> void mt6358_init()
>  {
>    ...other stuff...
>    while (!stopwatch_expired(&voltage_settled))
>      /* wait for voltages to settle */;
>  }

Done. Thank you.
Now mt6358_init() will return with stable voltages. I think we don't need to add comment in romstage.c.

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raising the CPU frequency
......................................................................

Patch Set 4: Code-Review+2

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raising the CPU frequency
......................................................................

Patch Set 4: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raising the CPU frequency
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/32466/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32466/4//COMMIT_MSG@7 
PS4, Line 7: Raising
Raise

Change in ...coreboot[master]: google/kukui: Raise the CPU frequency

[Tristan Hsieh (Code Review)]

Hello hsin-hsiung wang, Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Weiyi Lu,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32466

to look at the new patch set (#5).

Change subject: google/kukui: Raise the CPU frequency
......................................................................

google/kukui: Raise the CPU frequency

Run CPU at the highest freqency (1989MHz) to speed up the boot time.

BUG=b:80501386
BRANCH=none
Test=Boots correctly on Kukui

Change-Id: I703ffcb99367f87e6792a72485f5634e0505e5ac
Signed-off-by: Tristan Shieh 
---
M src/mainboard/google/kukui/romstage.c
1 file changed, 2 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/32466/5

Change in ...coreboot[master]: google/kukui: Raise the CPU frequency

[Tristan Hsieh (Code Review)]

Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raise the CPU frequency
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/#/c/32466/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32466/4//COMMIT_MSG@7 
PS4, Line 7: Raising
> Raise

Done

Change in ...coreboot[master]: google/kukui: Raise the CPU frequency

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raise the CPU frequency
......................................................................

Patch Set 5: Code-Review+2

Change in ...coreboot[master]: google/kukui: Raise the CPU frequency

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raise the CPU frequency
......................................................................

Patch Set 5:

Nice. Out of curiosity, have you measured the speedup?

Change in ...coreboot[master]: google/kukui: Raise the CPU frequency

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raise the CPU frequency
......................................................................

Patch Set 5:
> Nice. Out of curiosity, have you measured the speedup?

100~150ms

Change in ...coreboot[master]: google/kukui: Raise the CPU frequency

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raise the CPU frequency
......................................................................

google/kukui: Raise the CPU frequency

Run CPU at the highest freqency (1989MHz) to speed up the boot time.

BUG=b:80501386
BRANCH=none
Test=Boots correctly on Kukui

Change-Id: I703ffcb99367f87e6792a72485f5634e0505e5ac
Signed-off-by: Tristan Shieh 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32466
Reviewed-by: Angel Pons 
Reviewed-by: Hung-Te Lin 
Reviewed-by: Julius Werner 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/kukui/romstage.c
1 file changed, 2 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved
  Angel Pons: Looks good to me, approved
  Hung-Te Lin: Looks good to me, approved

diff --git a/src/mainboard/google/kukui/romstage.c b/src/mainboard/google/kukui/romstage.c
index 81ae9c5..baaca43 100644
--- a/src/mainboard/google/kukui/romstage.c
+++ b/src/mainboard/google/kukui/romstage.c
@@ -17,6 +17,7 @@
 #include <soc/emi.h>
 #include <soc/mmu_operations.h>
 #include <soc/mt6358.h>
+#include <soc/pll.h>
 #include <soc/rtc.h>

#include "early_init.h"
@@ -28,6 +29,7 @@
    	mainboard_early_init();

mt6358_init();
+	mt_pll_raise_ca53_freq(1989 * MHz);
    rtc_boot();
    mt_mem_init(get_sdram_config());
    mtk_mmu_after_dram();

Change in ...coreboot[master]: google/kukui: Raise the CPU frequency

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32466 )

Change subject: google/kukui: Raise the CPU frequency
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/#/c/32466/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32466/6//COMMIT_MSG@9 
PS6, Line 9: freqency
frequency

https://review.coreboot.org/#/c/32466/6//COMMIT_MSG@10 
PS6, Line 10: 
Please always give hard numbers.