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 16:
(17 comments)
https://review.coreboot.org/c/coreboot/+/35040/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35040/2//COMMIT_MSG@7 PS2, Line 7: intel/common/mmc
soc/intel/common/block:?
Done
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.
I plan to leave the infrastructure here in case other settings need to be overridden.
Done
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"
Done
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:
I meant adding struct mmc_dll_params to soc_intel_common_config here https://review.coreboot. […]
Done
https://review.coreboot.org/c/coreboot/+/35040/5/src/soc/intel/cannonlake/ch... PS5, Line 425: Each = 125pSec
we will put the range for all parameters.
Done
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);
ok we will go option 2
Done
https://review.coreboot.org/c/coreboot/+/35040/11/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/mmc.h:
https://review.coreboot.org/c/coreboot/+/35040/11/src/soc/intel/common/block... PS11, Line 64: SOC specific API
This is not SoC specific API.
Done
https://review.coreboot.org/c/coreboot/+/35040/11/src/soc/intel/common/block... PS11, Line 67: void *bar
Can you please add comment indicating what this BAR really means.
Done
https://review.coreboot.org/c/coreboot/+/35040/2/src/soc/intel/common/block/... File src/soc/intel/common/block/scs/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35040/2/src/soc/intel/common/block/... PS2, Line 2: CONFIG_SOC_INTEL_COMMON_BLOCK_SCS
also shouldn't we use a dedicated Kconfig to avoid the DLL getting overridden by mistake for some so […]
Done
https://review.coreboot.org/c/coreboot/+/35040/2/src/soc/intel/common/block/... File src/soc/intel/common/block/scs/mmc.c:
https://review.coreboot.org/c/coreboot/+/35040/2/src/soc/intel/common/block/... PS2, Line 22: mainboard_get_mmc_dll
yup. […]
Done
https://review.coreboot.org/c/coreboot/+/35040/2/src/soc/intel/common/block/... PS2, Line 27: ioaddr
will fix this
Done
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
no, these dll settings are good enough. […]
Done
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 31: if (dll_params->emmc_tx_data_cntl1) { : write32(bar + EMMC_TX_DATA_CNTL1_OFFSET, : dll_params->emmc_tx_data_cntl1); : override = 1; : }
You can make this slightly easier to read if you do something like: […]
Done
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; : } :
will do it […]
Done
https://review.coreboot.org/c/coreboot/+/35040/11/src/soc/intel/common/block... File src/soc/intel/common/block/scs/mmc.c:
https://review.coreboot.org/c/coreboot/+/35040/11/src/soc/intel/common/block... PS11, Line 73: cfg0
Just curious: Why is this named cfg0?
Done
https://review.coreboot.org/c/coreboot/+/35040/11/src/soc/intel/common/block... PS11, Line 75: if (CONFIG(SOC_INTEL_COMMON_MMC_OVERRIDE))
Why not put this up in line 71: […]
Done
https://review.coreboot.org/c/coreboot/+/35040/13/src/soc/intel/common/block... File src/soc/intel/common/block/scs/mmc.c:
https://review.coreboot.org/c/coreboot/+/35040/13/src/soc/intel/common/block... PS13, Line 75: (void *)
Because the set_mmc_dll also use by mmc_early_wake. […]
Done