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:
(2 comments)
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:
Hi Furquan, […]
I meant adding struct mmc_dll_params to soc_intel_common_config here https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc... and in mmc.c you can call chip_get_common_soc_structure() to calculate pointer to mmc_dll_params and use that structure directly to configure mmc dll params regs.
In mainboard you would just need something like this:
# EMMC Tx CMD Delay # Refer to EDS-Vol2-16.32. # [14:8] steps of delay for DDR mode, each 125ps. # [6:0] steps of delay for SDR mode, each 125ps. register "common_soc_config.mmc_dll_params.emmc_tx_cmd_cntl" = "0x505"
# EMMC TX DATA Delay 1 # Refer to EDS-Vol2-16.33. # [14:8] steps of delay for HS400, each 125ps. # [6:0] steps of delay for SDR104/HS200, each 125ps. register "common_soc_config.mmc_dll_params.emmc_tx_data_cntl1" = "0x0F10"
# EMMC TX DATA Delay 2 # Refer to EDS-Vol2-16.34. # [30:24] steps of delay for SDR50, each 125ps. # [22:16] steps of delay for DDR50, each 125ps. # [14:8] steps of delay for SDR25/HS50, each 125ps. # [6:0] steps of delay for SDR12, each 125ps. register "common_soc_config.mmc_dll_params.emmc_tx_data_cntl2" = "0x1C2F2D2D"
# EMMC RX CMD/DATA Delay 1 # Refer to EDS-Vol2-16.35. # [30:24] steps of delay for SDR50, each 125ps. # [22:16] steps of delay for DDR50, each 125ps. # [14:8] steps of delay for SDR25/HS50, each 125ps. # [6:0] steps of delay for SDR12, each 125ps. register "common_soc_config.mmc_dll_params.emmc_rx_cmd_data_cntl1" = "0x1C121936"
# EMMC RX CMD/DATA Delay 2 # Refer to EDS-Vol2-16.37. # [17:16] stands for Rx Clock before Output Buffer # [14:8] steps of delay for Auto Tuning Mode, each 125ps. # [6:0] steps of delay for HS200, each 125ps. register "common_soc_config.mmc_dll_params.emmc_rx_cmd_data_cntl2" = "0x1182D"
# EMMC Rx Strobe Delay # Refer to EDS-Vol2-16.36. # [14:8] Rx Strobe Delay DLL 1(HS400 Mode), each 125ps. # [6:0] Rx Strobe Delay DLL 2(HS400 Mode), each 125ps. register "common_soc_config.mmc_dll_params.emmc_rx_strobe_cntl" = "0x1414"
Then there is no need to add SoC/mainboard callbacks.
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
The idea is to prevent some other projects are overridden unexpectedly.
Can zero be treated as a special value? Or do you expect 0 to be a valid value that can be used to set dll params? If 0 can be treated as a special value, you can update set_mmc_dll to:
set_mmc_dll(...) { ... if (dll_params.emmc_tx_data_cntl1) write32(bar + EMMC_TX_DATA_CNTL1_OFFSET, dll_params.emmc_tx_data_cntl1); if (dll_params.emmc_tx_data_cntl2) write32(bar + EMMC_TX_DATA_CNTL2_OFFSET, dll_params.emmc_tx_data_cntl2); ... }
Or have a helper function:
mmc_write_dll_reg(uintptr_t bar, uint32_t reg, uint32_t val) { if (!val) return; write32(bar + reg, val); }
set_mmc_dll(...) { ... mmc_write_dll_reg(bar, EMMC_TX_DATA_CNTL1_OFFSET, dll_params.emmc_tx_data_cntl1); mmc_write_dll_reg(bar, EMMC_TX_DATA_CNTL2_OFFSET, dll_params.emmc_tx_data_cntl2); ... }