Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44718 )
Change subject: soc/mediatek/mt8192: Do rx dqs gating training ......................................................................
Patch Set 44:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1382: if (freq_group != DDRFREQ_400) : tx_dly_dqs_gated >>= 3; : else : tx_dly_dqs_gated >>= 2; Maybe calculate the number of positions to shift out of the loop?
const u8 shift = freq_group == DDRFREQ_400 ? 2 : 3;
for (u8 dqs = 0; dqs < DQS_NUMBER; dqs++) {
...
tx_dly_dqs_gated >>= shift;
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1393: rxdqs_gating_get_tx_dly_max
This is almost identical to rxdqs_gating_get_tx_dly_min(). Please merge these 2 functions: […]
If one always wants to know the min and max values, then one function returning both is better. Another way to do this is to return a struct with two values:
struct ddr_minmax { u8 min; u8 max; };
static struct ddr_minmax rxdqs_gating_get_tx_dly_limits( const dram_freq_grp freq_group, const struct rxdqs_gating_best_win *rxdqs_best_win) { struct ddr_minmax minmax = { .min = UINT8_MAX, .max = 0, };
...
return minmax; }
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1438: size_t
int
size_t should be the right type to use for array indices
https://review.coreboot.org/c/coreboot/+/44718/44/src/soc/mediatek/mt8192/dr... PS44, Line 1443: if (get_div_mode(cali) == DIV4_MODE) : freq_div = 2; : else : freq_div = 4; Maybe make this const?
const u8 freq_div = get_div_mode(cali) == DIV4_MODE ? 2 : 4;
(No idea, but shouldn't the values be the other way around?)