Attention is currently required from: Hung-Te Lin, Jarried Lin, Yidi Lin, Zhanzhan Ge.
Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/83928?usp=email )
Change subject: soc/mediatek/mt8196: Fix timer reset in BL31 ......................................................................
Patch Set 20:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83928/comment/5da44a55_49679356?usp... : PS8, Line 9: 1. Set systimer compensation to version 2.0. : 2. The system does not need to serve pending IRQ from systimer : after rebooting. Therefore we clear systimer IRQ pending bit : at early booting.
1 and 2 are related, 1 fixes to you, 2 fixes potential issues, but 2 depends on 1, so there is no ne […]
Okay, then they can be in the same patch. However, please explain the dependency in the commit message. It doesn't seem clear to me.
File src/soc/mediatek/mt8196/timer_prepare.c:
https://review.coreboot.org/c/coreboot/+/83928/comment/3696e970_2c949ca0?usp... : PS8, Line 18: &mtk_systimer->cnttval[id].con
Hi, mtk_systimer is already a local variable, thank you
I mean another variable, so that we don't need to perform duplicate `->cnttval[id].con` lookup.
https://review.coreboot.org/c/coreboot/+/83928/comment/2062b667_58107d71?usp... : PS8, Line 28: clrbits32
The difference between clrbits32 and clrbits32p is whether the first parameter is a pointer
Exactly, so there's no need to cast `SYSTIMER_BASE` to `void *`. Otherwise, why do you think coreboot has `clrbits32p` to begin with?