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 5:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35040/5/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/35040/5/src/include/device/pci_ids.... PS5, Line 3224: PCI_DEVICE_ID_INTEL_CMP_MMC This should go below under "Intel EMMC device Ids"
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 the SoC callback soc_get_mmc_dll() at all.
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?
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.
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.h? This change seems to be smashing together different things.
1. It would be good to either have two separate files mmc.h and early_mmc.h or 2. Rename this file to mmc.h since this is now being used for more than early emmc initialization.
I would prefer #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?
https://review.coreboot.org/c/coreboot/+/35040/5/src/soc/intel/common/block/... PS5, Line 61: SOC_INTEL_COMMON_MMC_OVERRIDE Just a thought. Do we really need this extra config? Can this function unconditionally call set_mmc_dll() which will set the parameter that is non-zero. Zero can be treated as a special value. Thoughts?