Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34988 )
Change subject: mediatek/mt8183: Implement the dramc init setting ......................................................................
Patch Set 17: -Code-Review
(9 comments)
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... PS17, Line 47: 0x2, does this come from DDR3200?
maybe leave this variable as uninitialized to make it clear?
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... PS17, Line 49: u32 sdm_pcw = 0, delta = 0; no need to initialize them - simply let switch() do that?
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... PS17, Line 76: } since freq_group is u8, I think we do need a
default: die("Invalid DDR frequency group\n"); return; }
Or, add an assert.
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... PS17, Line 78: ca_dll_mode[CHANNEL_A] = DLL_MASTER; : ca_dll_mode[CHANNEL_B] = DLL_SLAVE; : if (freq_group == LP4X_DDR1600) : ca_dll_mode[CHANNEL_A] = ca_dll_mode[CHANNEL_B] = DLL_SLAVE; : ca_dll_mode[CHANNEL_A] = (freq_group == LP4X_DDR1660) ? DLL_SLAVE : DLL_MASTER; ca_dll_mode[CHANNEL_B] = DLL_SLAVE;
or
if (freq_group == LP4X_DDR1600) ca_dll_mode[CHANNEL_A] = DLL_SLAVE; else ca_dll_mode[CHANNEL_A] = DLL_MASTER; ca_dll_mode[CHANNEL_B] = DLL_SLAVE;
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... PS17, Line 94: if (ca_dll_mode[chn] == DLL_MASTER) { : clrsetbits_le32(&ch[chn].phy.shu[0].ca_dll[0], : (0x1 << 31) | (0x1 << 30) | (0xf << 20) | (0xf << 16) | : (0xf << 12) | (0x1 << 10) | (0x1 << 9) | (0x1 << 4), : (0x0 << 31) | (0x0 << 30) | (0x6 << 20) | (0x9 << 16) | : (0x8 << 12) | (0x1 << 10) | (0x1 << 9) | (0x1 << 4)); : clrsetbits_le32(&ch[chn].phy.shu[0].ca_dll[1], : (0x1 << 2) | (0x1 << 0), (0x1 << 2) | (0x0 << 0)); : clrsetbits_le32(&ch[chn].phy.shu[0].ca_cmd[6], 0x1 << 7, 0x1 << 7); : } else { : clrsetbits_le32(&ch[chn].phy.shu[0].ca_dll[0], : (0x1 << 31) | (0x1 << 30) | (0xf << 20) | (0xf << 16) | : (0xf << 12) | (0x1 << 10) | (0x1 << 9) | (0x1 << 4), : (0x1 << 31) | (0x1 << 30) | (0x7 << 20) | (0x7 << 16) | : (0x8 << 12) | (0x1 << 10) | (0x1 << 9) | (0x0 << 4)); : clrsetbits_le32(&ch[chn].phy.shu[0].ca_dll[1], : (0x1 << 2) | (0x1 << 0), (0x0 << 2) | (0x1 << 0)); : clrsetbits_le32(&ch[chn].phy.shu[0].ca_cmd[6], 0x1 << 7, 0x0 << 7); : } do you have the name of the fields?
I think we should make this more clear by
u8 some_var_1, some_var_2;
if (ca_dll_mode[chn] == DLL_MASTER) { some_var_1 = 1; } else { some_var_1 = 2; }
clrsetbits_l332(&ch[chn].phy.shu[0].ca_dll[0], (0x1 << 31) .... some_var_1 << 31 | ... );
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... PS17, Line 208: if (freq_group > LP4X_DDR1600) { : clrsetbits_le32(&ch[chn].phy.shu[0].b[0].dq[6], : (0x1 << 26) | (0x1 << 27), (0x1 << 26) | (0x0 << 27)); : clrsetbits_le32(&ch[chn].phy.shu[0].b[1].dq[6], : (0x1 << 26) | (0x1 << 27), (0x1 << 26) | (0x0 << 27)); : clrsetbits_le32(&ch[chn].phy.shu[0].ca_cmd[6], : (0x1 << 26) | (0x1 << 27), (0x1 << 26) | (0x0 << 27)); : } else { : clrsetbits_le32(&ch[chn].phy.shu[0].b[0].dq[6], : (0x1 << 26) | (0x1 << 27), (0x0 << 26) | (0x1 << 27)); : clrsetbits_le32(&ch[chn].phy.shu[0].b[1].dq[6], : (0x1 << 26) | (0x1 << 27), (0x0 << 26) | (0x1 << 27)); : clrsetbits_le32(&ch[chn].phy.shu[0].ca_cmd[6], : (0x1 << 26) | (0x1 << 27), (0x0 << 26) | (0x1 << 27)); : } I'd recommend same style, having the variables inside if-block, and then set by vars.
if (freq_group > ...) { some_var1 = ... } else { }
clrsetbits_l32(... some_var1 << ....
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... PS17, Line 313: u16 rx_vref = 0xb; : : if (operate_fsp == FSP_1) : rx_vref = 0xb; : else : rx_vref = 0x16; u16 rx_vref = 0x16;
if (operate_fsp == FSP_1) rx_vref = 0xb;
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... PS17, Line 637: } Please add a default: here to die or assert.
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... PS17, Line 688: u8 MR01Value[FSP_MAX] = {0x26, 0x56}; : u8 MR13Value = (1 << 4) | (1 << 3); I still think we should not share variable like this, especially there's local MR02 etc.
Please make this packed in a struct, for example
struct dram_shared_data { u8 MR01Value[]... ... };
Ideally we should just pass this to whatever functions that wants to access it. If that's too hard, at least we can make it only one variable is shared.