Attention is currently required from: Hung-Te Lin, Paul Menzel, Yu-Ping Wu. flora.fu@mediatek.com has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58969 )
Change subject: soc/mediatek/mt8195: Add APU device apc driver ......................................................................
Patch Set 8:
(12 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/58969/comment/c92a83f6_f83af9ef PS6, Line 12: are
is
Done
https://review.coreboot.org/c/coreboot/+/58969/comment/5eaba9dc_a5bdc76a PS6, Line 19: others
other
Done
File src/soc/mediatek/mt8195/apusys_devapc.c:
https://review.coreboot.org/c/coreboot/+/58969/comment/eae62091_7bef14bf PS6, Line 167: DEBUG
ERR
Done
https://review.coreboot.org/c/coreboot/+/58969/comment/f163bd76_25c8e760 PS6, Line 175: DEBUG
ERR
Done
https://review.coreboot.org/c/coreboot/+/58969/comment/97b24a1f_05a1cafd PS6, Line 196: ARRAY_SIZE(apusys_noc_dapc)
Does this have the same meaning as APUSYS_NOC_DAPC_AO_SLAVE_NUM? If so, do we still need APUSYS_NOC_ […]
Yes, but the APUSYS_NOC_DAPC_AO_SLAVE_NUM is used to do runtime checking when setting devapc registers. I think we can keep it.
https://review.coreboot.org/c/coreboot/+/58969/comment/3c00dc28_9d9bf4aa PS6, Line 201: i,
Can fit into previous line. Line length limit is now 96 chars in coreboot.
OK. just run the clang-format and columnlimit to 96.
https://review.coreboot.org/c/coreboot/+/58969/comment/6ee29a14_2763f281 PS6, Line 217: DEBUG
ERR
Done
https://review.coreboot.org/c/coreboot/+/58969/comment/a412d776_18310bd8 PS6, Line 224: DEBUG
ERR
Done
https://review.coreboot.org/c/coreboot/+/58969/comment/bde578d2_04292de5 PS6, Line 265: +=
Does it make sense to add all the return values?
It is not necessary. Update code in Patchset8.
File src/soc/mediatek/mt8195/include/soc/apusys_devapc.h:
https://review.coreboot.org/c/coreboot/+/58969/comment/103114ea_e600d3fc PS6, Line 6: APUSYS_APC_ERR_STATUS
Lower case.
Done
File src/soc/mediatek/mt8195/include/soc/apusys_devapc_def.h:
https://review.coreboot.org/c/coreboot/+/58969/comment/c1826042_22ddff70 PS2, Line 10: APUSYS_APC_ERR_STATUS
Lower case name.
Done
https://review.coreboot.org/c/coreboot/+/58969/comment/17401231_db7e87b8 PS2, Line 13: 0x1000
Actually, it is the legacy error codes that we usually use for APU dapc.
By the way, I just make the error code sort and increase 1.
-
flora.fu＠mediatek.com (Code Review)