Hello Kane Chen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to review the following change.
Change subject: intel/common/mmc: Provide mmc.c for setting dll registers. ......................................................................
intel/common/mmc: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: mainboard_get_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/include/device/pci_ids.h M src/soc/intel/common/block/include/intelblocks/early_mmc.h M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 5 files changed, 107 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/1
diff --git a/src/include/device/pci_ids.h b/src/include/device/pci_ids.h index 3ac6560..f7a4c95 100644 --- a/src/include/device/pci_ids.h +++ b/src/include/device/pci_ids.h @@ -3220,6 +3220,7 @@ #define PCI_DEVICE_ID_INTEL_CNP_H_SD 0xa375 #define PCI_DEVICE_ID_INTEL_ICL_SD 0x34f8 #define PCI_DEVICE_ID_INTEL_CMP_SD 0x02f5 +#define PCI_DEVICE_ID_INTEL_CMP_MMC 0x02c4
/* Intel EMMC device Ids */ #define PCI_DEVICE_ID_INTEL_SKL_EMMC 0x9d2b diff --git a/src/soc/intel/common/block/include/intelblocks/early_mmc.h b/src/soc/intel/common/block/include/intelblocks/early_mmc.h index 39aaf58..2ec22ec 100644 --- a/src/soc/intel/common/block/include/intelblocks/early_mmc.h +++ b/src/soc/intel/common/block/include/intelblocks/early_mmc.h @@ -60,6 +60,23 @@ * 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 *ioaddr); +/* + * mainboard specific API to get mmc delay register settings. + * returns 0, if able to get register settings; otherwise returns -1 + */ +int mainboard_get_mmc_dll(struct mmc_dll_params *params); + +#define EMMC_TX_CMD_CNTL_OFFSET 0x820 +#define EMMC_TX_DATA_CNTL1_OFFSET 0x824 +#define EMMC_TX_DATA_CNTL2_OFFSET 0x828 +#define EMMC_RX_CMD_DATA_CNTL1_OFFSET 0x82C +#define EMMC_RX_STROBE_CNTL_OFFSET 0x830 +#define EMMC_RX_CMD_DATA_CNTL2_OFFSET 0x834
#if CONFIG(SOC_INTEL_COMMON_EARLY_MMC_WAKE) /* diff --git a/src/soc/intel/common/block/scs/Makefile.inc b/src/soc/intel/common/block/scs/Makefile.inc index 1160802..707a334 100644 --- a/src/soc/intel/common/block/scs/Makefile.inc +++ b/src/soc/intel/common/block/scs/Makefile.inc @@ -1,2 +1,3 @@ ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_SCS) += sd.c +ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_SCS) += mmc.c romstage-$(CONFIG_SOC_INTEL_COMMON_EARLY_MMC_WAKE) += early_mmc.c diff --git a/src/soc/intel/common/block/scs/early_mmc.c b/src/soc/intel/common/block/scs/early_mmc.c index 8f47ec7..297b9c4 100644 --- a/src/soc/intel/common/block/scs/early_mmc.c +++ b/src/soc/intel/common/block/scs/early_mmc.c @@ -26,13 +26,6 @@ #include <soc/pci_devs.h> #include <string.h>
-#define EMMC_TX_CMD_CNTL_OFFSET 0x820 -#define EMMC_TX_DATA_CNTL1_OFFSET 0x824 -#define EMMC_TX_DATA_CNTL2_OFFSET 0x828 -#define EMMC_RX_CMD_DATA_CNTL1_OFFSET 0x82C -#define EMMC_RX_STROBE_CNTL_OFFSET 0x830 -#define EMMC_RX_CMD_DATA_CNTL2_OFFSET 0x834 - void soc_sd_mmc_controller_quirks(struct sd_mmc_ctrlr *ctrlr) { uint32_t f_min, f_max; @@ -62,32 +55,6 @@ ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY)); }
-static int set_mmc_dll(void *ioaddr) -{ - struct mmc_dll_params dll_params; - - if (soc_get_mmc_dll(&dll_params) < 0) { - printk(BIOS_ERR, - "MMC early init: failed to get mmc DLL parameters\n"); - return -1; - } - - write32(ioaddr + EMMC_TX_DATA_CNTL1_OFFSET, - dll_params.emmc_tx_data_cntl1); - write32(ioaddr + EMMC_TX_DATA_CNTL2_OFFSET, - dll_params.emmc_tx_data_cntl2); - write32(ioaddr + EMMC_RX_CMD_DATA_CNTL1_OFFSET, - dll_params.emmc_rx_cmd_data_cntl1); - write32(ioaddr + EMMC_RX_CMD_DATA_CNTL2_OFFSET, - dll_params.emmc_rx_cmd_data_cntl2); - write32(ioaddr + EMMC_RX_STROBE_CNTL_OFFSET, - dll_params.emmc_rx_strobe_cntl); - write32(ioaddr + EMMC_TX_CMD_CNTL_OFFSET, - dll_params.emmc_tx_cmd_cntl); - - return 0; -} - static void set_early_mmc_wake_status(int32_t status) { int32_t *ms_cbmem; diff --git a/src/soc/intel/common/block/scs/mmc.c b/src/soc/intel/common/block/scs/mmc.c new file mode 100644 index 0000000..b55db4a --- /dev/null +++ b/src/soc/intel/common/block/scs/mmc.c @@ -0,0 +1,88 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <device/pci.h> +#include <device/pci_ids.h> +#include <console/console.h> +#include <intelblocks/early_mmc.h> + +/* Common weak definition, needs to be implemented in mainboard */ +int __weak mainboard_get_mmc_dll(struct mmc_dll_params *params) +{ + return -1; +} + +int set_mmc_dll(void *ioaddr) +{ + struct mmc_dll_params dll_params; + + if (mainboard_get_mmc_dll(&dll_params) < 0) { + printk(BIOS_INFO, "Skip Emmc dll value programming \n"); + return -1; + } + + printk(BIOS_INFO, "Override emmc dll value \n"); + + write32(ioaddr + EMMC_TX_DATA_CNTL1_OFFSET, + dll_params.emmc_tx_data_cntl1); + + write32(ioaddr + EMMC_TX_DATA_CNTL2_OFFSET, + dll_params.emmc_tx_data_cntl2); + + write32(ioaddr + EMMC_RX_CMD_DATA_CNTL1_OFFSET, + dll_params.emmc_rx_cmd_data_cntl1); + + write32(ioaddr + EMMC_RX_CMD_DATA_CNTL2_OFFSET, + dll_params.emmc_rx_cmd_data_cntl2); + + write32(ioaddr + EMMC_RX_STROBE_CNTL_OFFSET, + dll_params.emmc_rx_strobe_cntl); + + write32(ioaddr + EMMC_TX_CMD_CNTL_OFFSET, + dll_params.emmc_tx_cmd_cntl); + + return 0; +} + +static void mmc_soc_init(struct device *dev) +{ + uint32_t *cfg0; + const struct resource *res; + + res = find_resource(dev, PCI_BASE_ADDRESS_0); + cfg0 = (void *)(uintptr_t)(res->base); + + set_mmc_dll(cfg0); +} + + +static struct device_operations dev_ops = { + .read_resources = pci_dev_read_resources, + .set_resources = pci_dev_set_resources, + .enable_resources = pci_dev_enable_resources, + .init = mmc_soc_init, + .ops_pci = &pci_dev_ops_pci, +}; + +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_INTEL_CMP_MMC, + 0 +}; + +static const struct pci_driver pch_sd __pci_driver = { + .ops = &dev_ops, + .vendor = PCI_VENDOR_ID_INTEL, + .devices = pci_device_ids, +};
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: intel/common/mmc: Provide mmc.c for setting dll registers. ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35040/1/src/soc/intel/common/block/... File src/soc/intel/common/block/scs/mmc.c:
https://review.coreboot.org/c/coreboot/+/35040/1/src/soc/intel/common/block/... PS1, Line 29: struct mmc_dll_params dll_params; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/1/src/soc/intel/common/block/... PS1, Line 29: struct mmc_dll_params dll_params; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35040/1/src/soc/intel/common/block/... PS1, Line 32: printk(BIOS_INFO, "Skip Emmc dll value programming \n"); unnecessary whitespace before a quoted newline
https://review.coreboot.org/c/coreboot/+/35040/1/src/soc/intel/common/block/... PS1, Line 34: } code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/1/src/soc/intel/common/block/... PS1, Line 34: } please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35040/1/src/soc/intel/common/block/... PS1, Line 36: printk(BIOS_INFO, "Override emmc dll value \n"); unnecessary whitespace before a quoted newline
Hello Patrick Rudolph, Kane Chen, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#2).
Change subject: intel/common/mmc: Provide mmc.c for setting dll registers. ......................................................................
intel/common/mmc: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: mainboard_get_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/include/device/pci_ids.h M src/soc/intel/common/block/include/intelblocks/early_mmc.h M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 5 files changed, 107 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/2
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: intel/common/mmc: Provide mmc.c for setting dll registers. ......................................................................
Patch Set 2:
f
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: intel/common/mmc: Provide mmc.c for setting dll registers. ......................................................................
Patch Set 2:
(3 comments)
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 i would have expect this call to go into soc code and soc code gets required value from chip.h config which might be mainboard specific rather rewriting the same function into each mainboard. I hope chip.h configs should be able to provide mainboard level abstraction into soc code. Thoughts ?
https://review.coreboot.org/c/coreboot/+/35040/2/src/soc/intel/common/block/... PS2, Line 27: ioaddr why you are calling this as ioaddress ? rather calling as BAR ?
https://review.coreboot.org/c/coreboot/+/35040/2/src/soc/intel/common/block/... PS2, Line 27: int why can't be static ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: intel/common/mmc: Provide mmc.c for setting dll registers. ......................................................................
Patch Set 2:
(1 comment)
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:?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: intel/common/mmc: Provide mmc.c for setting dll registers. ......................................................................
Patch Set 2:
(1 comment)
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 soc?
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: intel/common/mmc: Provide mmc.c for setting dll registers. ......................................................................
Patch Set 2:
(1 comment)
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 27: int
why can't be static ?
We moved this function from early_emmc.c. so this is for early_emmc.c to call.
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: intel/common/mmc: Provide mmc.c for setting dll registers. ......................................................................
Patch Set 2:
(2 comments)
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
i would have expect this call to go into soc code and soc code gets required value from chip. […]
sounds good. then , every proj can put dll value in devicetree.cb and it can also be used once the new FSP has dll UPDs is landed
https://review.coreboot.org/c/coreboot/+/35040/2/src/soc/intel/common/block/... PS2, Line 27: ioaddr
why you are calling this as ioaddress ? rather calling as BAR ?
will fix this
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: intel/common/mmc: Provide mmc.c for setting dll registers. ......................................................................
Patch Set 2:
(2 comments)
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
sounds good. […]
yup. SG
https://review.coreboot.org/c/coreboot/+/35040/2/src/soc/intel/common/block/... PS2, Line 27: int
We moved this function from early_emmc.c. so this is for early_emmc.c to call.
Ack
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#3).
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: mainboard_get_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/include/device/pci_ids.h M src/soc/intel/cannonlake/chip.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/common/block/include/intelblocks/early_mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 8 files changed, 155 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/3
build bot (Jenkins) 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 3:
(36 comments)
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 425: /* [14:8] DDR mode Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 426: * [6:0] SDR mode Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 427: */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 428: uint32_t emmc_tx_cmd_cntl; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 428: uint32_t emmc_tx_cmd_cntl; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 430: /* [14:8] HS400 mode Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 431: * [6:0] SDR104/HS200 mode Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 432: */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 433: uint32_t emmc_tx_data_cntl1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 433: uint32_t emmc_tx_data_cntl1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 435: /* [30:24] SDR50 mode Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 436: * [22:16] DDR50 mode Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 437: * [14:8] SDR25/HS50 mode Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 438: * [6:0] SDR12/Compatibility mode Number of dealy elements. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 439: * Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 440: */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 441: uint32_t emmc_tx_data_cntl2; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 441: uint32_t emmc_tx_data_cntl2; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 443: /* [30:24] SDR50 mode Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 444: * [22:16] DDR50 mode Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 445: * [14:8] SDR25/HS50 mode Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 446: * [6:0] SDR12/Compatibility mode Number of dealy elements. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 447: * Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 448: */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 449: uint32_t emmc_rx_cmd_data_cntl1; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 449: uint32_t emmc_rx_cmd_data_cntl1; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 451: /* [14:8] HS400 mode 1 Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 452: * [6:0] HS400 mode 2 Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 453: */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 454: uint32_t emmc_rx_strobe_cntl; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 454: uint32_t emmc_rx_strobe_cntl; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 456: /* [13:8] Auto Tuning mode Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 457: * [6:0] SDR104/HS200 Number of dealy elements.Each = 125pSec. code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 458: */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 459: uint32_t emmc_rx_cmd_data_cntl2; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/3/src/soc/intel/cannonlake/ch... PS3, Line 459: uint32_t emmc_rx_cmd_data_cntl2; please, no spaces at the start of a line
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#4).
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: mainboard_get_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/include/device/pci_ids.h M src/soc/intel/cannonlake/chip.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/common/block/include/intelblocks/early_mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 8 files changed, 155 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/4
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#5).
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: mainboard_get_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/include/device/pci_ids.h M src/soc/intel/cannonlake/chip.c M src/soc/intel/cannonlake/chip.h M src/soc/intel/common/block/include/intelblocks/early_mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 8 files changed, 155 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/5
Subrata Banik 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:
LGTM
only 1 feedback to split this CL into 3 parts
1. Common code alone for eMMC 2. soc/intel/cnl code 3. PCI ID for eMMC
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:
Patch Set 5:
LGTM
only 1 feedback to split this CL into 3 parts
- Common code alone for eMMC
- soc/intel/cnl code
- PCI ID for eMMC
+1 to splitting the CL.
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?
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:
(1 comment)
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?
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.
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); ... }
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:
(1 comment)
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
Can zero be treated as a special value? Or do you expect 0 to be a valid value that can be used to s […]
Hi Furquan, Could we still have the Kconfig?
Here is the reason.
These dll settings in devtree will also be used after the newer FSP is landed. The new dll UPDs value will be fed by the settings in devtree.
Once the new fsp is landed, if we use 0 as a condition to check, then this code will do duplicate things again.
So far i think this function is mainly to override the settings when FSP doesn't provide specific UPDs
After newer FSP is landed, we can turn off Kconfig and the code won't run duplicate things
Thanks.
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#6).
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: set_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h R src/soc/intel/common/block/include/intelblocks/mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 6 files changed, 130 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/6
Kane Chen has uploaded a new patch set (#8) to the change originally created by Jamie Chen. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: set_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h R src/soc/intel/common/block/include/intelblocks/mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 6 files changed, 130 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/8
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:
(1 comment)
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
Hi Furquan, […]
Actually, I wanted to see if we can avoid configuring the UPDs completely for eMMC? If coreboot can take care of configuring the values, do we still need to set FSP UPDs at all?
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/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:
static void write_mmc_dll_param(bar, reg, val) { if (val) write32(bar + reg, val); }
and call it with: write_mmc_dll_param(bar, EMMC_TX_DATA_CNTL2_OFFSET, dll_params->emmc_tx_data_cntl2);
and so on.
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; : } : Why is this required? Return value is anyways not used.
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 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
Actually, I wanted to see if we can avoid configuring the UPDs completely for eMMC? If coreboot can […]
Hi Furquan, I would still suggest to use UPD for long term. So far this override mechanism is just for short term WA for kindred before newer FSP is landed thanks.
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; : } :
Why is this required? Return value is anyways not used.
Hi Furquan, set_mmc_dll is also being used by early_mmc.c. It will check the return value. thanks
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 8:
(1 comment)
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
Hi Furquan, […]
I also think we this place can also be used in case other issue requires an override in the future
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.
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 8:
(1 comment)
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 am just concerned that we will be adding confusion about when to use what i.e. […]
I would prefer we only have one place to program this DLL. And the FSP has the full control for this DLL in the future. Otherwise, it would also create confusion when we do the debug inside FSP.
so far i think this override is just to mitigate the schedule. Thanks.
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:
(1 comment)
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 would prefer we only have one place to program this DLL. […]
Okay. Can you please update the Kconfig help text to clearly indicate when it should be used.
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 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
Okay. Can you please update the Kconfig help text to clearly indicate when it should be used.
ok, will do it. thank you
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; : } :
Oh okay. what about my suggestion about having another helper function: write_mmc_dll_param […]
will do it thank you
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#9).
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: set_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h R src/soc/intel/common/block/include/intelblocks/mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 6 files changed, 123 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/9
build bot (Jenkins) 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 9:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35040/9/src/soc/intel/common/block/... File src/soc/intel/common/block/scs/mmc.c:
https://review.coreboot.org/c/coreboot/+/35040/9/src/soc/intel/common/block/... PS9, Line 22: static int mmc_write_dll_reg(void* bar, uint32_t reg, uint32_t val) "foo* bar" should be "foo *bar"
https://review.coreboot.org/c/coreboot/+/35040/9/src/soc/intel/common/block/... PS9, Line 42: dll_params->emmc_tx_cmd_cntl); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/9/src/soc/intel/common/block/... PS9, Line 42: dll_params->emmc_tx_cmd_cntl); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35040/9/src/soc/intel/common/block/... PS9, Line 54: dll_params->emmc_rx_strobe_cntl); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35040/9/src/soc/intel/common/block/... PS9, Line 54: dll_params->emmc_rx_strobe_cntl); please, no spaces at the start of a line
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#10).
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: set_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h R src/soc/intel/common/block/include/intelblocks/mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 6 files changed, 123 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/10
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 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35040/10/src/soc/intel/common/block... File src/soc/intel/common/block/scs/mmc.c:
https://review.coreboot.org/c/coreboot/+/35040/10/src/soc/intel/common/block... PS10, Line 59: if (override > 0) { : printk(BIOS_INFO, "Skip Emmc dll value programming\n"); : return -1; : } the logic doesn't seem right. If the dll override does happen, mmc_write_dll_reg will return 1 And then, it will return -1 eventually.
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#11).
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: set_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h R src/soc/intel/common/block/include/intelblocks/mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 6 files changed, 123 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/11
Jamie 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 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35040/10/src/soc/intel/common/block... File src/soc/intel/common/block/scs/mmc.c:
https://review.coreboot.org/c/coreboot/+/35040/10/src/soc/intel/common/block... PS10, Line 59: if (override > 0) { : printk(BIOS_INFO, "Skip Emmc dll value programming\n"); : return -1; : }
the logic doesn't seem right. […]
Resolved
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 11:
(4 comments)
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.
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.
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?
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:
if (!CONFIG(SOC_INTEL_COMMON_MMC_OVERRIDE)) return;
No need to calculate base address if its not going to be used.
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#12).
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: set_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h R src/soc/intel/common/block/include/intelblocks/mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 6 files changed, 123 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/12
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#13).
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: set_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h R src/soc/intel/common/block/include/intelblocks/mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 6 files changed, 122 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/13
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 13: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/35040/13/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/mmc.h:
https://review.coreboot.org/c/coreboot/+/35040/13/src/soc/intel/common/block... PS13, Line 21: /* : * Following should be defined in soc/iomap.h : * PRERAM_MMC_BASE_ADDRESS - Provide an address to setup emmc controller's : PCI BAR. : */ nit: Can you please move this under SOC_INTEL_COMMON_EARLY_MMC_WAKE (i.e. line 77)
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 *) nit: Why not pass uintptr_t all the way to mmc_write_dll_reg() and let it cast to (void *)(bar + reg) ?
Jamie 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 13:
(1 comment)
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 *)
nit: Why not pass uintptr_t all the way to mmc_write_dll_reg() and let it cast to (void *)(bar + reg […]
Because the set_mmc_dll also use by mmc_early_wake. If we change the type to uintptr_t, we will need more code change. Can we keep it?
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#14).
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: set_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h R src/soc/intel/common/block/include/intelblocks/mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 6 files changed, 128 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/14
build bot (Jenkins) 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 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35040/14/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/mmc.h:
https://review.coreboot.org/c/coreboot/+/35040/14/src/soc/intel/common/block... PS14, Line 75: PCI BAR. code indent should use tabs where possible
Jamie 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 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35040/13/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/mmc.h:
https://review.coreboot.org/c/coreboot/+/35040/13/src/soc/intel/common/block... PS13, Line 21: /* : * Following should be defined in soc/iomap.h : * PRERAM_MMC_BASE_ADDRESS - Provide an address to setup emmc controller's : PCI BAR. : */
nit: Can you please move this under SOC_INTEL_COMMON_EARLY_MMC_WAKE (i.e. […]
Done
Hello Patrick Rudolph, Subrata Banik, Kane Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35040
to look at the new patch set (#15).
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers. ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers.
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard. Notice: set_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overrided.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h R src/soc/intel/common/block/include/intelblocks/mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 6 files changed, 128 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/15
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 15: Code-Review+2
Paul Menzel 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 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35040/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35040/15//COMMIT_MSG@7 PS15, Line 7: soc/intel/common/block: Provide mmc.c for setting dll registers. Please remove the dot/period at the end of the commit message summary.
https://review.coreboot.org/c/coreboot/+/35040/15//COMMIT_MSG@13 PS15, Line 13: Notice: set_mmc_dll function will override the dll values in FSP. Please add a blank line above.
https://review.coreboot.org/c/coreboot/+/35040/15//COMMIT_MSG@17 PS15, Line 17: overrided overridden?
Kane Chen has uploaded a new patch set (#16) to the change originally created by Jamie Chen. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard.
Notice: set_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overridden.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com --- M src/soc/intel/common/block/include/intelblocks/chip.h R src/soc/intel/common/block/include/intelblocks/mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 6 files changed, 128 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/40/35040/16
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:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35040/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35040/15//COMMIT_MSG@7 PS15, Line 7: soc/intel/common/block: Provide mmc.c for setting dll registers.
Please remove the dot/period at the end of the commit message summary.
Done
https://review.coreboot.org/c/coreboot/+/35040/15//COMMIT_MSG@13 PS15, Line 13: Notice: set_mmc_dll function will override the dll values in FSP.
Please add a blank line above.
Done
https://review.coreboot.org/c/coreboot/+/35040/15//COMMIT_MSG@17 PS15, Line 17: overrided
overridden?
Done
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
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/35040 )
Change subject: soc/intel/common/block: Provide mmc.c for setting dll registers ......................................................................
soc/intel/common/block: Provide mmc.c for setting dll registers
Currently, we don't have UPDs to set emmc settings per mainboard on CML.
This code change is to create mmc.c to provide interface to override dll settings per mainboard.
Notice: set_mmc_dll function will override the dll values in FSP.
BUG=b:131401116 BRANCH=none TEST=Boot to OS and confirm the dll values have been overridden.
Change-Id: Ib3c72b9851f41585ec099d8ae83a721af87ed383 Signed-off-by: Kane Chen kane.chen@intel.com Signed-off-by: Jamie Chen jamie.chen@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35040 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/common/block/include/intelblocks/chip.h R src/soc/intel/common/block/include/intelblocks/mmc.h M src/soc/intel/common/block/scs/Kconfig M src/soc/intel/common/block/scs/Makefile.inc M src/soc/intel/common/block/scs/early_mmc.c A src/soc/intel/common/block/scs/mmc.c 6 files changed, 128 insertions(+), 43 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/intel/common/block/include/intelblocks/chip.h b/src/soc/intel/common/block/include/intelblocks/chip.h index 9fe165e..1e830d5 100644 --- a/src/soc/intel/common/block/include/intelblocks/chip.h +++ b/src/soc/intel/common/block/include/intelblocks/chip.h @@ -18,6 +18,7 @@
#include <intelblocks/gspi.h> #include <drivers/i2c/designware/dw_i2c.h> +#include <intelblocks/mmc.h>
enum { CHIPSET_LOCKDOWN_FSP = 0, /* FSP handles locking per UPDs */ @@ -35,6 +36,7 @@ struct dw_i2c_bus_config i2c[CONFIG_SOC_INTEL_I2C_DEV_MAX]; /* PCH Thermal Trip Temperature in deg C */ uint8_t pch_thermal_trip; + struct mmc_dll_params emmc_dll; };
/* This function to retrieve soc config structure required by common code */ diff --git a/src/soc/intel/common/block/include/intelblocks/early_mmc.h b/src/soc/intel/common/block/include/intelblocks/mmc.h similarity index 81% rename from src/soc/intel/common/block/include/intelblocks/early_mmc.h rename to src/soc/intel/common/block/include/intelblocks/mmc.h index 39aaf58..a8776ea 100644 --- a/src/soc/intel/common/block/include/intelblocks/early_mmc.h +++ b/src/soc/intel/common/block/include/intelblocks/mmc.h @@ -13,18 +13,12 @@ * GNU General Public License for more details. */
-#ifndef SOC_INTEL_COMMON_BLOCK_EARLY_MMC_H -#define SOC_INTEL_COMMON_BLOCK_EARLY_MMC_H +#ifndef SOC_INTEL_COMMON_BLOCK_MMC_H +#define SOC_INTEL_COMMON_BLOCK_MMC_H
#include <stdint.h>
/* - * Following should be defined in soc/iomap.h - * PRERAM_MMC_BASE_ADDRESS - Provide an address to setup emmc controller's - PCI BAR. - */ - -/* * Structure for the following delay registers * emmc_tx_data_cntl1: Tx Delay Control 1 (Tx_DATA_dly_1)-Offset 824h * emmc_tx_data_cntl2: Tx Delay Control 2 (Tx_DATA_dly_2)-Offset 828h @@ -60,9 +54,28 @@ * returns 0, if able to get register settings; otherwise returns -1 */ int soc_get_mmc_dll(struct mmc_dll_params *params); +/* + * Set mmc delay register settings. + * bar: eMMC controller MMIO base address. + * returns 0, if able to set register settings; otherwise returns -1 + */ +int set_mmc_dll(void *bar); + +#define EMMC_TX_CMD_CNTL_OFFSET 0x820 +#define EMMC_TX_DATA_CNTL1_OFFSET 0x824 +#define EMMC_TX_DATA_CNTL2_OFFSET 0x828 +#define EMMC_RX_CMD_DATA_CNTL1_OFFSET 0x82C +#define EMMC_RX_STROBE_CNTL_OFFSET 0x830 +#define EMMC_RX_CMD_DATA_CNTL2_OFFSET 0x834
#if CONFIG(SOC_INTEL_COMMON_EARLY_MMC_WAKE) /* + * Following should be defined in soc/iomap.h + * PRERAM_MMC_BASE_ADDRESS - Provide an address to setup emmc controller's + PCI BAR. + */ + +/* * Initializes sdhci / mmc controller and sends CMD0, CMD1 to emmc card. * In case of success: It returns 0 and adds cbmem entry CBMEM_ID_MMC_STATUS * and sets it to 1. Payload can start by sending CMD1, there is no need to @@ -77,4 +90,4 @@ return -1; } #endif /* CONFIG_SOC_INTEL_COMMON_EARLY_MMC_WAKE */ -#endif /* SOC_INTEL_COMMON_BLOCK_EARLY_MMC_H */ +#endif /* SOC_INTEL_COMMON_BLOCK_MMC_H */ diff --git a/src/soc/intel/common/block/scs/Kconfig b/src/soc/intel/common/block/scs/Kconfig index 06ad8e4..192425c 100644 --- a/src/soc/intel/common/block/scs/Kconfig +++ b/src/soc/intel/common/block/scs/Kconfig @@ -12,3 +12,10 @@ help Send CMD1 early in romstage to improve boot time. It requires emmc DLL tuning parameters to be added to devicetree.cb + +config SOC_INTEL_COMMON_MMC_OVERRIDE + bool + default n + help + Override the MMC settings after FSP-S. + It should be used only when there is no FSP UPDs for certain setting. diff --git a/src/soc/intel/common/block/scs/Makefile.inc b/src/soc/intel/common/block/scs/Makefile.inc index 1160802..707a334 100644 --- a/src/soc/intel/common/block/scs/Makefile.inc +++ b/src/soc/intel/common/block/scs/Makefile.inc @@ -1,2 +1,3 @@ ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_SCS) += sd.c +ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_SCS) += mmc.c romstage-$(CONFIG_SOC_INTEL_COMMON_EARLY_MMC_WAKE) += early_mmc.c diff --git a/src/soc/intel/common/block/scs/early_mmc.c b/src/soc/intel/common/block/scs/early_mmc.c index 8f47ec7..8036450 100644 --- a/src/soc/intel/common/block/scs/early_mmc.c +++ b/src/soc/intel/common/block/scs/early_mmc.c @@ -21,18 +21,11 @@ #include <compiler.h> #include <console/console.h> #include <device/pci.h> -#include <intelblocks/early_mmc.h> +#include <intelblocks/mmc.h> #include <soc/iomap.h> #include <soc/pci_devs.h> #include <string.h>
-#define EMMC_TX_CMD_CNTL_OFFSET 0x820 -#define EMMC_TX_DATA_CNTL1_OFFSET 0x824 -#define EMMC_TX_DATA_CNTL2_OFFSET 0x828 -#define EMMC_RX_CMD_DATA_CNTL1_OFFSET 0x82C -#define EMMC_RX_STROBE_CNTL_OFFSET 0x830 -#define EMMC_RX_CMD_DATA_CNTL2_OFFSET 0x834 - void soc_sd_mmc_controller_quirks(struct sd_mmc_ctrlr *ctrlr) { uint32_t f_min, f_max; @@ -62,32 +55,6 @@ ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY)); }
-static int set_mmc_dll(void *ioaddr) -{ - struct mmc_dll_params dll_params; - - if (soc_get_mmc_dll(&dll_params) < 0) { - printk(BIOS_ERR, - "MMC early init: failed to get mmc DLL parameters\n"); - return -1; - } - - write32(ioaddr + EMMC_TX_DATA_CNTL1_OFFSET, - dll_params.emmc_tx_data_cntl1); - write32(ioaddr + EMMC_TX_DATA_CNTL2_OFFSET, - dll_params.emmc_tx_data_cntl2); - write32(ioaddr + EMMC_RX_CMD_DATA_CNTL1_OFFSET, - dll_params.emmc_rx_cmd_data_cntl1); - write32(ioaddr + EMMC_RX_CMD_DATA_CNTL2_OFFSET, - dll_params.emmc_rx_cmd_data_cntl2); - write32(ioaddr + EMMC_RX_STROBE_CNTL_OFFSET, - dll_params.emmc_rx_strobe_cntl); - write32(ioaddr + EMMC_TX_CMD_CNTL_OFFSET, - dll_params.emmc_tx_cmd_cntl); - - return 0; -} - static void set_early_mmc_wake_status(int32_t status) { int32_t *ms_cbmem; diff --git a/src/soc/intel/common/block/scs/mmc.c b/src/soc/intel/common/block/scs/mmc.c new file mode 100644 index 0000000..5b2e2c7 --- /dev/null +++ b/src/soc/intel/common/block/scs/mmc.c @@ -0,0 +1,95 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <device/pci.h> +#include <device/pci_ids.h> +#include <console/console.h> +#include <intelblocks/chip.h> +#include <intelblocks/mmc.h> + +static int mmc_write_dll_reg(void *bar, uint32_t reg, uint32_t val) +{ + int ret = 0; + if (val) { + write32(bar + reg, val); + ret = 1; + } + return ret; +} + +int set_mmc_dll(void *bar) +{ + const struct soc_intel_common_config *common_config; + const struct mmc_dll_params *dll_params; + int override = 0; + + common_config = chip_get_common_soc_structure(); + dll_params = &common_config->emmc_dll; + + override |= mmc_write_dll_reg(bar, EMMC_TX_CMD_CNTL_OFFSET, + dll_params->emmc_tx_cmd_cntl); + + override |= mmc_write_dll_reg(bar, EMMC_TX_DATA_CNTL1_OFFSET, + dll_params->emmc_tx_data_cntl1); + + override |= mmc_write_dll_reg(bar, EMMC_TX_DATA_CNTL2_OFFSET, + dll_params->emmc_tx_data_cntl2); + + override |= mmc_write_dll_reg(bar, EMMC_RX_CMD_DATA_CNTL1_OFFSET, + dll_params->emmc_rx_cmd_data_cntl1); + + override |= mmc_write_dll_reg(bar, EMMC_RX_STROBE_CNTL_OFFSET, + dll_params->emmc_rx_strobe_cntl); + + override |= mmc_write_dll_reg(bar, EMMC_RX_CMD_DATA_CNTL2_OFFSET, + dll_params->emmc_rx_cmd_data_cntl2); + + if (override == 0) { + printk(BIOS_INFO, "Skip Emmc dll value programming\n"); + return -1; + } + + return 0; +} + +static void mmc_soc_init(struct device *dev) +{ + const struct resource *res; + + if (!CONFIG(SOC_INTEL_COMMON_MMC_OVERRIDE)) + return; + + res = find_resource(dev, PCI_BASE_ADDRESS_0); + set_mmc_dll((void *)(uintptr_t)(res->base)); +} + +static struct device_operations dev_ops = { + .read_resources = pci_dev_read_resources, + .set_resources = pci_dev_set_resources, + .enable_resources = pci_dev_enable_resources, + .init = mmc_soc_init, + .ops_pci = &pci_dev_ops_pci, +}; + +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_INTEL_CMP_EMMC, + 0 +}; + +static const struct pci_driver pch_sd __pci_driver = { + .ops = &dev_ops, + .vendor = PCI_VENDOR_ID_INTEL, + .devices = pci_device_ids, +};
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
Jamie Chen (Code Review)
-
Kane Chen (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)