Tristan Hsieh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32245
Change subject: mediatek: Using a 64 bits timer ......................................................................
mediatek: Using a 64 bits timer
GPT4 is a 32 bits timer and the counter of GPT4 will be overflow about 330 seconds (0xffffffff / 13MHz). With the GPT4 version, timer and delay functions will not work when the counter is overflow. Now we use a 64 bits timer, GPT6, to fix the overflow issue.
BUG=b:80501386 BRANCH=none Test=emerge-elm coreboot; emerge-kukui coreboot
Change-Id: I9f080e47253a1b1bab4636a45cb86c8666a25302 Signed-off-by: Tristan Shieh tristan.shieh@mediatek.com --- M src/soc/mediatek/common/include/soc/timer.h M src/soc/mediatek/common/timer.c M src/soc/mediatek/mt8173/timer.c 3 files changed, 31 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/32245/1
diff --git a/src/soc/mediatek/common/include/soc/timer.h b/src/soc/mediatek/common/include/soc/timer.h index babcbba..b58d4d3 100644 --- a/src/soc/mediatek/common/include/soc/timer.h +++ b/src/soc/mediatek/common/include/soc/timer.h @@ -19,18 +19,21 @@ #include <soc/addressmap.h> #include <types.h>
-#define GPT4_MHZ 13 +#define GPT_MHZ 13
struct mtk_gpt_regs { - u32 reserved[16]; - u32 gpt4_con; - u32 gpt4_clk; - u32 gpt4_cnt; + u32 reserved1[24]; + u32 gpt6_con; + u32 gpt6_clk; + u32 gpt6_cnt_l; + u32 reserved2[3]; + u32 gpt6_cnt_h; };
-check_member(mtk_gpt_regs, gpt4_con, 0x0040); -check_member(mtk_gpt_regs, gpt4_clk, 0x0044); -check_member(mtk_gpt_regs, gpt4_cnt, 0x0048); +check_member(mtk_gpt_regs, gpt6_con, 0x0060); +check_member(mtk_gpt_regs, gpt6_clk, 0x0064); +check_member(mtk_gpt_regs, gpt6_cnt_l, 0x0068); +check_member(mtk_gpt_regs, gpt6_cnt_h, 0x0078);
enum { GPT_CON_EN = 0x01, diff --git a/src/soc/mediatek/common/timer.c b/src/soc/mediatek/common/timer.c index 0762c2b..bc0577c 100644 --- a/src/soc/mediatek/common/timer.c +++ b/src/soc/mediatek/common/timer.c @@ -25,21 +25,33 @@
__weak void timer_prepare(void) { /* do nothing */ }
+static uint64_t timer_raw_value(void) +{ + uint32_t low, high; + + do { + high = read32(&mtk_gpt->gpt6_cnt_h); + low = read32(&mtk_gpt->gpt6_cnt_l); + } while (high != read32(&mtk_gpt->gpt6_cnt_h)); + + return (uint64_t)low | ((uint64_t)high) << 32; +} + void timer_monotonic_get(struct mono_time *mt) { - mono_time_set_usecs(mt, read32(&mtk_gpt->gpt4_cnt) / GPT4_MHZ); + mono_time_set_usecs(mt, timer_raw_value() / GPT_MHZ); }
void init_timer(void) { timer_prepare();
- /* Disable GPT4 and clear the counter */ - clrbits_le32(&mtk_gpt->gpt4_con, GPT_CON_EN); - setbits_le32(&mtk_gpt->gpt4_con, GPT_CON_CLR); + /* Disable timer and clear the counter */ + clrbits_le32(&mtk_gpt->gpt6_con, GPT_CON_EN); + setbits_le32(&mtk_gpt->gpt6_con, GPT_CON_CLR);
/* Set clock source to system clock and set clock divider to 1 */ - write32(&mtk_gpt->gpt4_clk, GPT_SYS_CLK | GPT_CLK_DIV1); - /* Set operation mode to FREERUN mode and enable GTP4 */ - write32(&mtk_gpt->gpt4_con, GPT_CON_EN | GPT_MODE_FREERUN); + write32(&mtk_gpt->gpt6_clk, GPT_SYS_CLK | GPT_CLK_DIV1); + /* Set operation mode to FREERUN mode and enable timer */ + write32(&mtk_gpt->gpt6_con, GPT_CON_EN | GPT_MODE_FREERUN); } diff --git a/src/soc/mediatek/mt8173/timer.c b/src/soc/mediatek/mt8173/timer.c index 2c9995a..eb2a142 100644 --- a/src/soc/mediatek/mt8173/timer.c +++ b/src/soc/mediatek/mt8173/timer.c @@ -31,5 +31,5 @@ */ write32(&mt8173_mcucfg->xgpt_idx, 0); /* Set clock mode to 13Mhz and enable XGPT */ - write32(&mt8173_mcucfg->xgpt_ctl, (0x1 | ((26 / GPT4_MHZ) << 8))); + write32(&mt8173_mcucfg->xgpt_ctl, (0x1 | ((26 / GPT_MHZ) << 8))); }
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Using a 64 bits timer ......................................................................
Patch Set 1:
So the GPT6 regs have same address on both 8183 and 8173?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Using a 64 bits timer ......................................................................
Patch Set 1:
one question, so why did this work for 8173? shouldn't it also overflow?
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Using a 64 bits timer ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/#/c/32245/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32245/1//COMMIT_MSG@7 PS1, Line 7: Using Use
https://review.coreboot.org/#/c/32245/1//COMMIT_MSG@9 PS1, Line 9: 32 bits 32-bit
https://review.coreboot.org/#/c/32245/1//COMMIT_MSG@12 PS1, Line 12: 64 bits 64-bit
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Using a 64 bits timer ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32245/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32245/1//COMMIT_MSG@7 PS1, Line 7: a 64 bits 64-bit
Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Using a 64 bits timer ......................................................................
Patch Set 1:
Patch Set 1:
one question, so why did this work for 8173? shouldn't it also overflow?
GPT6 regs have the same address on both 8183 and 8173. I've tested this patch on both 8183 and 8173.
This issue happens on both 8183 and 8173. I guess usually we finish all jobs before overflow (330 seconds) and didn't get any trouble. Recently, we are doing some memory test overnight and get trapped in this. (If we can finish all the jobs in 330 seconds, probably this patch is not necessary.)
It only make delay and stopwatch fail if we start a delay() or stopwatch_init_usecs_expire() before (and close to) the overflow and end up with a timeout value bigger than the maximum. Since the timer warps around to a small value, it will never return from delay() or get true from stopwatch_expired().
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Using a 64 bits timer ......................................................................
Patch Set 1: Code-Review+1
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32245
to look at the new patch set (#2).
Change subject: mediatek: Use a 64-bit timer ......................................................................
mediatek: Use a 64-bit timer
GPT4 is a 32-bit timer and the counter of GPT4 will be overflow about 330 seconds (0xffffffff / 13MHz). With the GPT4 version, timer and delay functions will not work when the counter is overflow. Now we use a 64-bit timer, GPT6, to fix the overflow issue.
BUG=b:80501386 BRANCH=none Test=emerge-elm coreboot; emerge-kukui coreboot
Change-Id: I9f080e47253a1b1bab4636a45cb86c8666a25302 Signed-off-by: Tristan Shieh tristan.shieh@mediatek.com --- M src/soc/mediatek/common/include/soc/timer.h M src/soc/mediatek/common/timer.c M src/soc/mediatek/mt8173/timer.c 3 files changed, 31 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/32245/2
Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use a 64-bit timer ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/#/c/32245/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32245/1//COMMIT_MSG@7 PS1, Line 7: a 64 bits
64-bit
Done
https://review.coreboot.org/#/c/32245/1//COMMIT_MSG@7 PS1, Line 7: Using
Use
Done
https://review.coreboot.org/#/c/32245/1//COMMIT_MSG@9 PS1, Line 9: 32 bits
32-bit
Done
https://review.coreboot.org/#/c/32245/1//COMMIT_MSG@12 PS1, Line 12: 64 bits
64-bit
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use a 64-bit timer ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/32245/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32245/2//COMMIT_MSG@9 PS2, Line 9: will be overflow … will overflow at …
https://review.coreboot.org/#/c/32245/2//COMMIT_MSG@11 PS2, Line 11: is overflow overflows
https://review.coreboot.org/#/c/32245/2/src/soc/mediatek/common/timer.c File src/soc/mediatek/common/timer.c:
https://review.coreboot.org/#/c/32245/2/src/soc/mediatek/common/timer.c@37 PS2, Line 37: ((uint64_t)high) Are these () correct? What is supposed to be shifted?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use a 64-bit timer ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32245/2/src/soc/mediatek/common/timer.c File src/soc/mediatek/common/timer.c:
https://review.coreboot.org/#/c/32245/2/src/soc/mediatek/common/timer.c@37 PS2, Line 37: ((uint64_t)high)
Are these () correct? What is supposed to be shifted?
I think it should be
low | (uint64_t)high << 32 ?
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32245
to look at the new patch set (#3).
Change subject: mediatek: Use a 64-bit timer ......................................................................
mediatek: Use a 64-bit timer
GPT4 is a 32-bit timer and the counter of GPT4 will be overflow about 330 seconds (0xffffffff / 13MHz). With the GPT4 version, timer and delay functions will not work when the counter is overflow. Now we use a 64-bit timer, GPT6, to fix the overflow issue.
BUG=b:80501386 BRANCH=none Test=emerge-elm coreboot; emerge-kukui coreboot
Change-Id: I9f080e47253a1b1bab4636a45cb86c8666a25302 Signed-off-by: Tristan Shieh tristan.shieh@mediatek.com --- M src/soc/mediatek/common/include/soc/timer.h M src/soc/mediatek/common/timer.c M src/soc/mediatek/mt8173/timer.c 3 files changed, 31 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/32245/3
Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use a 64-bit timer ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32245/2/src/soc/mediatek/common/timer.c File src/soc/mediatek/common/timer.c:
https://review.coreboot.org/#/c/32245/2/src/soc/mediatek/common/timer.c@37 PS2, Line 37: ((uint64_t)high)
I think it should be […]
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use a 64-bit timer ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@7 PS3, Line 7: a the
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@9 PS3, Line 9: be will overflow (no need to add 'be')
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@9 PS3, Line 9: about in about
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@10 PS3, Line 10: With the GPT4 version, timer Timer
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@11 PS3, Line 11: when the counter is overflow properly if the counter overflows
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@11 PS3, Line 11: Now we use : a 64-bit timer, GPT6, to fix the overflow issue. To fix that we should use the 64-bit timer (GPT6).
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32245
to look at the new patch set (#4).
Change subject: mediatek: Use the 64-bit timer ......................................................................
mediatek: Use the 64-bit timer
GPT4 is a 32-bit timer and the counter of GPT4 will overflow in about 330 seconds (0xffffffff / 13MHz). Timer and delay functions will not work properly if the counter overflows. To fix that we should use the 64-bit timer (GPT6).
BUG=b:80501386 BRANCH=none Test=emerge-elm coreboot; emerge-kukui coreboot
Change-Id: I9f080e47253a1b1bab4636a45cb86c8666a25302 Signed-off-by: Tristan Shieh tristan.shieh@mediatek.com --- M src/soc/mediatek/common/include/soc/timer.h M src/soc/mediatek/common/timer.c M src/soc/mediatek/mt8173/timer.c 3 files changed, 31 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/32245/4
Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@7 PS3, Line 7: a
the
Done
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@9 PS3, Line 9: be
will overflow (no need to add 'be')
Done
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@9 PS3, Line 9: about
in about
Done
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@10 PS3, Line 10: With the GPT4 version, timer
Timer
Done
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@11 PS3, Line 11: when the counter is overflow
properly if the counter overflows
Done
https://review.coreboot.org/#/c/32245/3//COMMIT_MSG@11 PS3, Line 11: Now we use : a 64-bit timer, GPT6, to fix the overflow issue.
To fix that we should use the 64-bit timer (GPT6).
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 4: Code-Review+2
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 4: Code-Review-1
This CL makes Kukui stuck in verstage. (Last line printed in serial console: "Probing TPM: ")
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32245/4/src/soc/mediatek/common/timer.c File src/soc/mediatek/common/timer.c:
https://review.coreboot.org/#/c/32245/4/src/soc/mediatek/common/timer.c@33 PS4, Line 33: high = read32(&mtk_gpt->gpt6_cnt_h); : low = read32(&mtk_gpt->gpt6_cnt_l); It seems like the variable 'high' and 'low' are always zero on Kukui.
Besides, the datasheet of MT8183 mentioned that, reading GPT6_COUNTL will make GPT6_COUNTH fixed until next read operation of GPT6_COUNTL. Does this mean we can read the timer without a loop?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 4: -Code-Review
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 4:
good findings!
@Tristan, can you confirm if reading twice is needed, and the behavior difference between 8173 and 8183?
Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32245/4/src/soc/mediatek/common/timer.c File src/soc/mediatek/common/timer.c:
https://review.coreboot.org/#/c/32245/4/src/soc/mediatek/common/timer.c@33 PS4, Line 33: high = read32(&mtk_gpt->gpt6_cnt_h); : low = read32(&mtk_gpt->gpt6_cnt_l);
It seems like the variable 'high' and 'low' are always zero on Kukui. […]
1. Print high and low to check. They are not zero. DBG: low(ffff0006), high(0) = ffff0006 DBG: low(ffffaf16), high(0) = ffffaf16 DBG: low(5e2b), high(1) = 100005e2b
2. Yes, you are right. Reading low register will fix high register until read high register. We can also have a simple version like:
static uint64_t timer_raw_value(void) { uint32_t low = read32(&mtk_gpt->gpt6_cnt_l); uint32_t high = read32(&mtk_gpt->gpt6_cnt_h);
return low | (uint64_t)high << 32; }
But, I still prefer to do the read-back check in do-while loop for two reason. a. We don't have to care if there is a special hardware lock or not. It works in both cases. b. We don't have to care if we need to protect the critical section. (Considering context-switch or interrupt happened between reading gpt_cnt_l and gpt_cnt_h.)
If we don't have to consider interrupt in coreboot and you prefer simple version, I'm OK to modify it.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 4:
If both 8173 and 8183 supports this lock mechanism, then I think we should just do the simple form, with a comment pointing to the datasheet for why this works.
Also, there won't be interrupts in Coreboot so I think we're fine.
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32245
to look at the new patch set (#5).
Change subject: mediatek: Use the 64-bit timer ......................................................................
mediatek: Use the 64-bit timer
GPT4 is a 32-bit timer and the counter of GPT4 will overflow in about 330 seconds (0xffffffff / 13MHz). Timer and delay functions will not work properly if the counter overflows. To fix that we should use the 64-bit timer (GPT6).
BUG=b:80501386 BRANCH=none Test=emerge-elm coreboot; emerge-kukui coreboot
Change-Id: I9f080e47253a1b1bab4636a45cb86c8666a25302 Signed-off-by: Tristan Shieh tristan.shieh@mediatek.com --- M src/soc/mediatek/common/include/soc/timer.h M src/soc/mediatek/common/timer.c M src/soc/mediatek/mt8173/timer.c 3 files changed, 32 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/32245/5
Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32245/4/src/soc/mediatek/common/timer.c File src/soc/mediatek/common/timer.c:
https://review.coreboot.org/#/c/32245/4/src/soc/mediatek/common/timer.c@33 PS4, Line 33: high = read32(&mtk_gpt->gpt6_cnt_h); : low = read32(&mtk_gpt->gpt6_cnt_l);
- […]
Done
Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32245/4/src/soc/mediatek/common/timer.c File src/soc/mediatek/common/timer.c:
https://review.coreboot.org/#/c/32245/4/src/soc/mediatek/common/timer.c@33 PS4, Line 33: high = read32(&mtk_gpt->gpt6_cnt_h); : low = read32(&mtk_gpt->gpt6_cnt_l);
Done
@You-Cheng, Just a guess. Did you check 'high' and 'low' before init_timer()??
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 5: Code-Review+1
if this boots properly on both 8173/8183 then I think we're good.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 5:
Note that this timer is also used in libpayload, so if you change it here, please make sure you also change it there (TIMER_GENERIC_REG and TIMER_GENERIC_HIGH_REG in payloads/libpayload/drivers/timer/Kconfig). Otherwise we may try to use the (then uninitialized) GPT4.
(FWIW we don't generally care about 32-bit roll-over in coreboot, because it doesn't have a use case where it should take anywhere near that long. But I'm fine with fixing this if you want to. In libpayload it's a bit more important because we may sit in recovery mode for a long time.)
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32245
to look at the new patch set (#6).
Change subject: mediatek: Use the 64-bit timer ......................................................................
mediatek: Use the 64-bit timer
GPT4 is a 32-bit timer and the counter of GPT4 will overflow in about 330 seconds (0xffffffff / 13MHz). Timer and delay functions will not work properly if the counter overflows. To fix that we should use the 64-bit timer (GPT6).
BUG=b:80501386 BRANCH=none Test=emerge-elm coreboot; emerge-kukui coreboot
Change-Id: I9f080e47253a1b1bab4636a45cb86c8666a25302 Signed-off-by: Tristan Shieh tristan.shieh@mediatek.com --- M payloads/libpayload/drivers/timer/Kconfig M src/soc/mediatek/common/include/soc/timer.h M src/soc/mediatek/common/timer.c M src/soc/mediatek/mt8173/timer.c 4 files changed, 35 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/32245/6
Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 6:
Patch Set 5:
Note that this timer is also used in libpayload, so if you change it here, please make sure you also change it there (TIMER_GENERIC_REG and TIMER_GENERIC_HIGH_REG in payloads/libpayload/drivers/timer/Kconfig). Otherwise we may try to use the (then uninitialized) GPT4.
(FWIW we don't generally care about 32-bit roll-over in coreboot, because it doesn't have a use case where it should take anywhere near that long. But I'm fine with fixing this if you want to. In libpayload it's a bit more important because we may sit in recovery mode for a long time.)
Thanks, I've fixed it. Using the uninitialized GPT4, delay(1) will actually wait 2 seconds (but it still boots into kernel).
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 6: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 6: Code-Review+2
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
Patch Set 6: Code-Review+1
Verified that it doesn't block boot process now.
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32245 )
Change subject: mediatek: Use the 64-bit timer ......................................................................
mediatek: Use the 64-bit timer
GPT4 is a 32-bit timer and the counter of GPT4 will overflow in about 330 seconds (0xffffffff / 13MHz). Timer and delay functions will not work properly if the counter overflows. To fix that we should use the 64-bit timer (GPT6).
BUG=b:80501386 BRANCH=none Test=emerge-elm coreboot; emerge-kukui coreboot
Change-Id: I9f080e47253a1b1bab4636a45cb86c8666a25302 Signed-off-by: Tristan Shieh tristan.shieh@mediatek.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32245 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Hung-Te Lin hungte@chromium.org Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: You-Cheng Syu youcheng@google.com --- M payloads/libpayload/drivers/timer/Kconfig M src/soc/mediatek/common/include/soc/timer.h M src/soc/mediatek/common/timer.c M src/soc/mediatek/mt8173/timer.c 4 files changed, 35 insertions(+), 18 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Hung-Te Lin: Looks good to me, but someone else must approve You-Cheng Syu: Looks good to me, but someone else must approve
diff --git a/payloads/libpayload/drivers/timer/Kconfig b/payloads/libpayload/drivers/timer/Kconfig index f1542cf..5a61dfa 100644 --- a/payloads/libpayload/drivers/timer/Kconfig +++ b/payloads/libpayload/drivers/timer/Kconfig @@ -54,7 +54,7 @@ bool "Timer for IMG Pistachio"
config TIMER_MTK - bool "Timer for MediaTek MT8173" + bool "Timer for MediaTek"
endchoice
@@ -77,7 +77,7 @@ default 0x004A2000 if TIMER_IPQ40XX default 0x0200A028 if TIMER_IPQ806X default 0x101C0100 if TIMER_MCT - default 0x10008048 if TIMER_MTK + default 0x10008068 if TIMER_MTK default 0xff810028 if TIMER_RK3288 default 0xff850008 if TIMER_RK3399 default 0x60005010 if TIMER_TEGRA_1US @@ -89,6 +89,7 @@ hex "Generic Timer High Register Address" default 0x004A2004 if TIMER_IPQ40XX default 0x101C0104 if TIMER_MCT + default 0x10008078 if TIMER_MTK default 0xff81002C if TIMER_RK3288 default 0xff85000C if TIMER_RK3399 default 0x0 diff --git a/src/soc/mediatek/common/include/soc/timer.h b/src/soc/mediatek/common/include/soc/timer.h index babcbba..b58d4d3 100644 --- a/src/soc/mediatek/common/include/soc/timer.h +++ b/src/soc/mediatek/common/include/soc/timer.h @@ -19,18 +19,21 @@ #include <soc/addressmap.h> #include <types.h>
-#define GPT4_MHZ 13 +#define GPT_MHZ 13
struct mtk_gpt_regs { - u32 reserved[16]; - u32 gpt4_con; - u32 gpt4_clk; - u32 gpt4_cnt; + u32 reserved1[24]; + u32 gpt6_con; + u32 gpt6_clk; + u32 gpt6_cnt_l; + u32 reserved2[3]; + u32 gpt6_cnt_h; };
-check_member(mtk_gpt_regs, gpt4_con, 0x0040); -check_member(mtk_gpt_regs, gpt4_clk, 0x0044); -check_member(mtk_gpt_regs, gpt4_cnt, 0x0048); +check_member(mtk_gpt_regs, gpt6_con, 0x0060); +check_member(mtk_gpt_regs, gpt6_clk, 0x0064); +check_member(mtk_gpt_regs, gpt6_cnt_l, 0x0068); +check_member(mtk_gpt_regs, gpt6_cnt_h, 0x0078);
enum { GPT_CON_EN = 0x01, diff --git a/src/soc/mediatek/common/timer.c b/src/soc/mediatek/common/timer.c index 0762c2b..c8af2be 100644 --- a/src/soc/mediatek/common/timer.c +++ b/src/soc/mediatek/common/timer.c @@ -25,21 +25,34 @@
__weak void timer_prepare(void) { /* do nothing */ }
+static uint64_t timer_raw_value(void) +{ + /* + * According to "General-Purpose Timer (GPT).pdf", The read operation of + * gpt6_cnt_l will make gpt6_cnt_h fixed until the next read operation + * of gpt6_cnt_l. Therefore, we must read gpt6_cnt_l before gpt6_cnt_h. + */ + uint32_t low = read32(&mtk_gpt->gpt6_cnt_l); + uint32_t high = read32(&mtk_gpt->gpt6_cnt_h); + + return low | (uint64_t)high << 32; +} + void timer_monotonic_get(struct mono_time *mt) { - mono_time_set_usecs(mt, read32(&mtk_gpt->gpt4_cnt) / GPT4_MHZ); + mono_time_set_usecs(mt, timer_raw_value() / GPT_MHZ); }
void init_timer(void) { timer_prepare();
- /* Disable GPT4 and clear the counter */ - clrbits_le32(&mtk_gpt->gpt4_con, GPT_CON_EN); - setbits_le32(&mtk_gpt->gpt4_con, GPT_CON_CLR); + /* Disable timer and clear the counter */ + clrbits_le32(&mtk_gpt->gpt6_con, GPT_CON_EN); + setbits_le32(&mtk_gpt->gpt6_con, GPT_CON_CLR);
/* Set clock source to system clock and set clock divider to 1 */ - write32(&mtk_gpt->gpt4_clk, GPT_SYS_CLK | GPT_CLK_DIV1); - /* Set operation mode to FREERUN mode and enable GTP4 */ - write32(&mtk_gpt->gpt4_con, GPT_CON_EN | GPT_MODE_FREERUN); + write32(&mtk_gpt->gpt6_clk, GPT_SYS_CLK | GPT_CLK_DIV1); + /* Set operation mode to FREERUN mode and enable timer */ + write32(&mtk_gpt->gpt6_con, GPT_CON_EN | GPT_MODE_FREERUN); } diff --git a/src/soc/mediatek/mt8173/timer.c b/src/soc/mediatek/mt8173/timer.c index 2c9995a..eb2a142 100644 --- a/src/soc/mediatek/mt8173/timer.c +++ b/src/soc/mediatek/mt8173/timer.c @@ -31,5 +31,5 @@ */ write32(&mt8173_mcucfg->xgpt_idx, 0); /* Set clock mode to 13Mhz and enable XGPT */ - write32(&mt8173_mcucfg->xgpt_ctl, (0x1 | ((26 / GPT4_MHZ) << 8))); + write32(&mt8173_mcucfg->xgpt_ctl, (0x1 | ((26 / GPT_MHZ) << 8))); }
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Paul Menzel (Code Review)
-
Tristan Hsieh (Code Review)
-
You-Cheng Syu (Code Review)