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.