Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39865 )
Change subject: soc/intel/tigerlake: Reorganize memory initialization support ......................................................................
Patch Set 3:
(1 comment)
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
Patch Set 3:
How about change all switch case to a marco? Like original code for DQS. All variable can just replace channel number.
I can do that and initially I had really liked it when the original code was done. But somehow looking back at it, it feels cryptic. If that is what feels better, I can definitely go back to it.
I'm the one that complained about the macros. I guess I'm just trying to push for not adding things like:
struct Bar { int Blah0; int Blah1; int Blah2; .... };
and just
struct Bar { int Blahs[TOTAL_BLAHS]; };
instead, because it makes everything easier to read and follow. The upstream Intel code is probably not going to change though, so maybe the macro does make it easier to read.
Umm.. the code wouldn't really look very different now that things are kind of separated for dq and dqs. It would still require a switch case like:
static void init_dq_upds(FSP_M_CONFIG *mem_cfg, int channel, const uint8_t *dq_arr) { » uint8_t *dq_upd;
» switch (channel) { » case 0: » » dq_upd = MEM_CFG_DQ_UPD(0); » » break; » case 1: » » dq_upd = MEM_CFG_DQ_UPD(1); » » break; ...
or if you really like the macros, then unfold the loop in meminit_lpddr4x to something like: » init_spd_upds(mem_cfg, 0, spd_data, 0); » INIT_DQ_UPD(mem_cfg, 0, board_cfg); » INIT_DQS_UPD(mem_cfg, 0, board_cfg);
» init_spd_upds(mem_cfg, 1, spd_data, 0); » INIT_DQ_UPD(mem_cfg, 1, board_cfg); » INIT_DQS_UPD(mem_cfg, 1, board_cfg);
» ...
» if (half_populated) { » » init_spd_upds(mem_cfg, 4, 0, 0); » » INIT_DQ_UPD_EMPTY(mem_cfg, 4); » » INIT_DQS_UPD_EMPTY(mem_cfg, 4); » ... » } else { » » init_spd_upds(mem_cfg, 4, spd_data, 0); » » INIT_DQ_UPD(mem_cfg, 4, board_cfg); » » INIT_DQS_UPD(mem_cfg, 4, board_cfg); » ... » }
Unless there is a strong preference for macros, I think we can stick to the switch case.
Yes this looks fine to me. I'm complaining about the need for DqMapCpu2DramCh0, DqMapCpu2DramCh1, DqMapCpu2DramCh2, DqMapCpu2DramCh3, etc. instead of DqMapCpu2DramCh[MAX_CHANNELS]
Aah.. Yes, That would have been better.
https://review.coreboot.org/c/coreboot/+/39865/3/src/soc/intel/tigerlake/mem... File src/soc/intel/tigerlake/meminit_tgl.c:
https://review.coreboot.org/c/coreboot/+/39865/3/src/soc/intel/tigerlake/mem... PS3, Line 30: dimm0
!dimm0
Woops. Good catch.