Kane Chen 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 5:
(7 comments)
Hi Furquan, Should we do things like patch set 2? The patch set 2 makes a weak function in mmc.c and real function is in kindred mainboard for override.
Now, I feel the dev tree dll settings should be dedicated for the FSP UPD in the future.
And the settings in mainboard can be different in dev tree. We could also use this to override other settings if it's needed some day.
is it ok?
thank you
https://review.coreboot.org/c/coreboot/+/35040/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35040/5//COMMIT_MSG@9 PS5, Line 9: Currently, we don't have UPDs to set emmc settings per mainboard on CML.
So, do you plan to get rid of this code once FSP UPDs are added?
I plan to leave the infrastructure here in case other settings need to be overridden.
https://review.coreboot.org/c/coreboot/+/35040/5/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/35040/5/src/soc/intel/cannonlake/ch... PS5, Line 424:
Why not use mmc_dll_params here? In fact, if you add this to soc_intel_common_config, you won't need […]
Hi Furquan, did you mean make a function call to mainboard directly if the override is needed?
https://review.coreboot.org/c/coreboot/+/35040/5/src/soc/intel/cannonlake/ch... PS5, Line 425: dealy
delay? here and rest of the comments here.
Ack
https://review.coreboot.org/c/coreboot/+/35040/5/src/soc/intel/cannonlake/ch... PS5, Line 425: Each = 125pSec
What is the valid range? 0-127 is all good?
we will put the range for all parameters.
https://review.coreboot.org/c/coreboot/+/35040/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/early_mmc.h:
https://review.coreboot.org/c/coreboot/+/35040/5/src/soc/intel/common/block/... PS5, Line 58: /* : * SOC specific API to get mmc delay register settings. : * returns 0, if able to get register settings; otherwise returns -1 : */ : int soc_get_mmc_dll(struct mmc_dll_params *params); : /* : * SOC specific API to set mmc delay register settings. : * returns 0, if able to set register settings; otherwise returns -1 : */ : int set_mmc_dll(void *bar);
I don't know why these have to live in early_mmc. […]
ok we will go option 2
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 21: set_mmc_dll
Do we lose anything by configuring these here instead of FSP?
no, these dll settings are good enough. Jamie also verified the change, it can boot smoothly. much better than before
https://review.coreboot.org/c/coreboot/+/35040/5/src/soc/intel/common/block/... PS5, Line 61: SOC_INTEL_COMMON_MMC_OVERRIDE
Just a thought. […]
The idea is to prevent some other projects are overridden unexpectedly.