Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35040/5/src/soc/intel/common/block/... File src/soc/intel/common/block/scs/mmc.c:
https://review.coreboot.org/c/coreboot/+/35040/5/src/soc/intel/common/block/... PS5, Line 61: SOC_INTEL_COMMON_MMC_OVERRIDE
I also think we this place can also be used in case other issue requires an override in the future
I am just concerned that we will be adding confusion about when to use what i.e. FSP UPDs v/s override in coreboot. Also, if coreboot can do the same work, what advantage do you see of using UPD in the long term?
https://review.coreboot.org/c/coreboot/+/35040/8/src/soc/intel/common/block/... File src/soc/intel/common/block/scs/mmc.c:
https://review.coreboot.org/c/coreboot/+/35040/8/src/soc/intel/common/block/... PS8, Line 67: if (override) { : printk(BIOS_INFO, "Skip Emmc dll value programming\n"); : return -1; : } :
Hi Furquan, […]
Oh okay. what about my suggestion about having another helper function: write_mmc_dll_param
With that you can: override |= write_mmc_dll_param(bar, EMMC_TX_DATA_CNTL2_OFFSET, dll_params->emmc_tx_data_cntl2);
And at the end, you can check override like you are doing right now.