Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44713 )
Change subject: soc/mediatek/mt8192: Add dramc ac timing setting ......................................................................
Patch Set 48:
(9 comments)
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4037: dram_freq_grp
Add const modifier, and i'd prefer to keep it for shorter name, ok?
Done
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4072: rodt_tracking_mck
remove it now.
Done
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 4080: ac_tim.datlat
update codes: […]
Done
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_main.c:
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 15: t
jesd209-4 spec term tRFCAB, so change to TRFCAB_?
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 21: enum
Actually, we may use actiming 1800 mapping to 1600 freq, so define the enum.
Ack
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/dr... PS41, Line 42: ptRFCab_Opt
Follow jesd spec, we'd prefer to use this style for clear name. […]
How about we add a comment?
struct optimize_ac_time *ptrf_cab_opt; /* tRFCab */
https://review.coreboot.org/c/coreboot/+/44713/48/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_main.c:
https://review.coreboot.org/c/coreboot/+/44713/48/src/soc/mediatek/mt8192/dr... PS48, Line 173: SET32_BITFIELDS(&ch[chn].ao.shu_ac_time_05t, SHU_AC_TIME_05T_TRFC_05T, : trfc_05t); Format it like this:
SET32_BITFIELDS(&ch[chn].ao.shu_ac_time_05t, SHU_AC_TIME_05T_TRFC_05T, trfc_05t);
https://review.coreboot.org/c/coreboot/+/44713/48/src/soc/mediatek/mt8192/dr... PS48, Line 177: SET32_BITFIELDS(&ch[chn].ao.shu_ac_time_05t, SHU_AC_TIME_05T_TRFCPB_05T, : trfrc_pb05t); Same
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/dramc_ac_timing.h:
https://review.coreboot.org/c/coreboot/+/44713/41/src/soc/mediatek/mt8192/in... PS41, Line 76: T
"T" is an standard clock interval, we'd prefer to keep the upper case for more clear. […]
Ack